Re: [PATCH v2 2/3] mmc: sdhci-s3c: correct kerneldoc of sdhci_s3c_drv_data
On 15.04.2021 10:44, Krzysztof Kozlowski wrote: Correct the name of sdhci_s3c_drv_data structure in kerneldoc: drivers/mmc/host/sdhci-s3c.c:143: warning: expecting prototype for struct sdhci_s3c_driver_data. Prototype was for struct sdhci_s3c_drv_data instead Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 3/3] spi: s3c64xx: constify driver/match data
On 14.04.2021 22:33, Krzysztof Kozlowski wrote: The match data (struct s3c64xx_spi_port_config) stored in of_device_id and platform_device_id tables is not modified by the driver and can be handled entirely in a const-way to increase the code safety. Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 2/3] spi: s3c64xx: correct kerneldoc of s3c64xx_spi_port_config
On 14.04.2021 22:33, Krzysztof Kozlowski wrote: Correct the name of s3c64xx_spi_port_config structure in kerneldoc: drivers/spi/spi-s3c64xx.c:154: warning: expecting prototype for struct s3c64xx_spi_info. Prototype was for struct s3c64xx_spi_port_config instead Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 1/3] spi: s3c64xx: simplify getting of_device_id match data
On 14.04.2021 22:33, Krzysztof Kozlowski wrote: Use of_device_get_match_data() to make the code slightly smaller and to remove the of_device_id table forward declaration. Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 1/2] i2c: s3c2410: simplify getting of_device_id match data
On 15.04.2021 11:38, Krzysztof Kozlowski wrote: Use of_device_get_match_data() to make the code slightly smaller. Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH v2 3/3] mmc: sdhci-s3c: constify uses of driver/match data
On 15.04.2021 10:44, Krzysztof Kozlowski wrote: The driver data (struct sdhci_s3c_drv_data) stored in of_device_id table is allocated as const and used only in const-way. Skip unnecessary const-away casts and convert all users to work with pointer to const. This is both more logical and safer. Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH v2 1/3] mmc: sdhci-s3c: simplify getting of_device_id match data
On 15.04.2021 10:44, Krzysztof Kozlowski wrote: Use of_device_get_match_data() to make the code slightly smaller and to remove the of_device_id table forward declaration. Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 1/3] mmc: sdhci-s3c: fix possible NULL pointer dereference when probed via platform
On 14.04.2021 17:25, Krzysztof Kozlowski wrote:
On 14/04/2021 17:12, Krzysztof Kozlowski wrote:
The driver can be matched by legacy platform way or OF-device matching.
In the first case, of_match_node() can return NULL, which immediately
would be dereferenced to get the match data.
Addresses-Coverity: Dereference null return value
Fixes: cd1b00eb24b0 ("mmc: sdhci-s3c: Add device tree support")
Signed-off-by: Krzysztof Kozlowski
-#ifdef CONFIG_OF
-static const struct of_device_id sdhci_s3c_dt_match[];
-#endif
-
static inline struct sdhci_s3c_drv_data *sdhci_s3c_get_driver_data(
struct platform_device *pdev)
{
#ifdef CONFIG_OF
- if (pdev->dev.of_node) {
- const struct of_device_id *match;
- match = of_match_node(sdhci_s3c_dt_match, pdev->dev.of_node);
Now I have second thoughts whether NULL pointer can actually happen. If
device is matched via platform/board files, maybe the pdev->dev.of_node
will be NULL thus skipping this branch?
Could there be a case where device is matched via platform_device_id()
(which has different name than compatible!) and (pdev->dev.of_node) is
still assigned? Maybe in case of out of tree DTS?
That seems unlikely, the platform device would need to be initialized
via board file and then its of_node assigned somehow. It doesn't make
much sense to me. We either use board file or dtb to instantiate devices.
Anyway, the patch makes the code simpler/smaller, so I still think it is
reasonable. Just the severity of issue is questionable...
Yes, the patch looks good. Similar cleanups are already done in most of
the s3c/s5p/exynos drivers.
--
Thanks,
Sylwester
Re: [PATCH] pinctrl: samsung: use 'int' for register masks in Exynos
On 08.04.2021 21:50, Krzysztof Kozlowski wrote: > The Special Function Registers on all Exynos SoC, including ARM64, are > 32-bit wide, so entire driver uses matching functions like readl() or > writel(). On 64-bit ARM using unsigned long for register masks: > 1. makes little sense as immediately after bitwise operation it will be >cast to 32-bit value when calling writel(), > 2. is actually error-prone because it might promote other operands to >64-bit. > > Addresses-Coverity: Unintentional integer overflow > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki > --- > > Not tested on ARM64. I have tested it on exynos5433/tm2e and didn't notice any issues as we could expect. The patch looks good to me, however I would personally use u32 rather than "unsigned int", like in other places for the register value variables. -- Regards, Sylwester
Re: [PATCH -next] clk: samsung: Remove redundant dev_err calls
On 08.04.2021 18:21, Krzysztof Kozlowski wrote: > On 08/04/2021 15:48, Chen Hui wrote: >> There is error message within devm_ioremap_resource >> already, so remove the dev_err calls to avoid redundant >> error messages. >> >> Signed-off-by: Chen Hui >> --- >> drivers/clk/samsung/clk-exynos4412-isp.c | 4 +--- >> drivers/clk/samsung/clk-s5pv210-audss.c | 4 +--- >> 2 files changed, 2 insertions(+), 6 deletions(-) > > Reviewed-by: Krzysztof Kozlowski Thank you, patch applied.
Re: [PATCH] clk: exynos7: Mark aclk_fsys1_200 as critical
On 24.10.2020 17:43, Paweł Chmiel wrote:
This clock must be always enabled to allow access to any registers in
fsys1 CMU. Until proper solution based on runtime PM is applied
(similar to what was done for Exynos5433), mark that clock as critical
so it won't be disabled.
It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
UFS module is probed before pmic used to power that device.
In this case defer probe was happening and that clock was disabled by
UFS driver, causing whole boot to hang on next CMU access.
Signed-off-by: Paweł Chmiel
---
drivers/clk/samsung/clk-exynos7.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/samsung/clk-exynos7.c
b/drivers/clk/samsung/clk-exynos7.c
index c1ff715e960c..1048d83f097b 100644
--- a/drivers/clk/samsung/clk-exynos7.c
+++ b/drivers/clk/samsung/clk-exynos7.c
@@ -538,7 +538,8 @@ static const struct samsung_gate_clock top1_gate_clks[]
__initconst = {
ENABLE_ACLK_TOP13, 28, CLK_SET_RATE_PARENT |
CLK_IS_CRITICAL, 0),
As this patch can be backported up to the commit that introduced regression
I have applied it instead of your v3, with a comment as below.
+ /*
+* This clock is required for the CMU_FSYS1 registers access, keep it
+* enabled permanently until proper runtime PM support is added.
+*/
GATE(CLK_ACLK_FSYS1_200, "aclk_fsys1_200", "dout_aclk_fsys1_200",
- ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT, 0),
+ ENABLE_ACLK_TOP13, 24, CLK_SET_RATE_PARENT |
+ CLK_IS_CRITICAL, 0),
GATE(CLK_SCLK_PHY_FSYS1_26M, "sclk_phy_fsys1_26m",
"dout_sclk_phy_fsys1_26m", ENABLE_SCLK_TOP1_FSYS11,
--
Regards,
Sylwester
Re: [PATCH v3] clk: exynos7: Keep aclk_fsys1_200 enabled
On 09.02.2021 08:48, Stephen Boyd wrote:
> Quoting (2021-01-31 09:04:28)
>> This clock must be always enabled to allow access to any registers in
>> fsys1 CMU. Until proper solution based on runtime PM is applied
>> (similar to what was done for Exynos5433), fix this by calling
>> clk_prepare_enable() directly from clock provider driver.
>>
>> It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
>> UFS module is probed before pmic used to power that device.
>> In this case defer probe was happening and that clock was disabled by
>> UFS driver, causing whole boot to hang on next CMU access.
>>
>
> Does this need a Fixes tag?
That would be
Fixes: 753195a749a6 ("clk: samsung: exynos7: Correct CMU_FSYS1 clocks names")
i.e. commit that introduced definition of the clock. But the fix cannot be
backported that far as build fails with an error:
drivers/clk/samsung/clk-exynos7.c: In function ‘exynos7_clk_top1_init’:
drivers/clk/samsung/clk-exynos7.c:554:21: error: ‘struct clk_onecell_data’ has
no member named ‘hws’
554 | hws = ctx->clk_data.hws;
It could only by backported up to:
ecb1f1f7311f ("clk: samsung: Convert common drivers to the new clk_hw API")
We need a different patch to fix it properly in stable kernels.
And dts for board this bugfix patch was prepared is not upstream yet.
Re: [PATCH v3] clk: exynos7: Keep aclk_fsys1_200 enabled
On 1/31/21 18:04, Paweł Chmiel wrote: This clock must be always enabled to allow access to any registers in fsys1 CMU. Until proper solution based on runtime PM is applied (similar to what was done for Exynos5433), fix this by calling clk_prepare_enable() directly from clock provider driver. It was observed on Samsung Galaxy S6 device (based on Exynos7420), where UFS module is probed before pmic used to power that device. In this case defer probe was happening and that clock was disabled by UFS driver, causing whole boot to hang on next CMU access. Signed-off-by: Paweł Chmiel --- Changes from v2: - Avoid __clk_lookup() call when enabling clock Changes from v1: - Instead of marking clock as critical, enable it manually in driver. Acked-by: Sylwester Nawrocki
Re: [PATCH] [v2] clk: samsung: mark PM functions as __maybe_unused
On 12/4/20 10:16, Arnd Bergmann wrote:
From: Arnd Bergmann
The use of SIMPLE_DEV_PM_OPS() means that the suspend/resume
functions are now unused when CONFIG_PM is disabled:
drivers/clk/samsung/clk-exynos-clkout.c:219:12: error: 'exynos_clkout_resume'
defined but not used [-Werror=unused-function]
219 | static int exynos_clkout_resume(struct device *dev)
|^~~~
drivers/clk/samsung/clk-exynos-clkout.c:210:12: error: 'exynos_clkout_suspend'
defined but not used [-Werror=unused-function]
210 | static int exynos_clkout_suspend(struct device *dev)
|^
Mark them as __maybe_unused to shut up the otherwise harmless warning.
Fixes: 9484f2cb8332 ("clk: samsung: exynos-clkout: convert to module driver")
Reviewed-by: Krzysztof Kozlowski
Signed-off-by: Arnd Bergmann
---
v2: add proper changelog text
Acked-by: Sylwester Nawrocki
[PATCH] [v2] clk: samsung: mark PM functions as __maybe_unused
From: Arnd Bergmann
The use of SIMPLE_DEV_PM_OPS() means that the suspend/resume
functions are now unused when CONFIG_PM is disabled:
drivers/clk/samsung/clk-exynos-clkout.c:219:12: error:
'exynos_clkout_resume' defined but not used [-Werror=unused-function]
219 | static int exynos_clkout_resume(struct device *dev)
|^~~~
drivers/clk/samsung/clk-exynos-clkout.c:210:12: error:
'exynos_clkout_suspend' defined but not used [-Werror=unused-function]
210 | static int exynos_clkout_suspend(struct device *dev)
|^
Mark them as __maybe_unused to shut up the otherwise harmless warning.
Fixes: 9484f2cb8332 ("clk: samsung: exynos-clkout: convert to module
driver")
Reviewed-by: Krzysztof Kozlowski
Signed-off-by: Arnd Bergmann
---
v2: add proper changelog text
Acked-by: Sylwester Nawrocki
Re: [PATCH v5] clk: samsung: Prevent potential endless loop in the PLL ops
On 11/22/20 16:16, Krzysztof Kozlowski wrote: On Fri, Nov 20, 2020 at 04:57:31PM +0100, Sylwester Nawrocki wrote: The PLL status polling loops in the set_rate callbacks of some PLLs have no timeout detection and may become endless loops when something goes wrong with the PLL. For some PLLs there is already the ktime API based timeout detection, but it will not work in all conditions when .set_rate gets called. In particular, before the clocksource is initialized or when the timekeeping is suspended. This patch adds a common helper with the PLL status bit polling and timeout detection. For conditions where the timekeeping API should not be used a simple readl_relaxed/cpu_relax() busy loop is added with the iterations limit derived from measurements of readl_relaxed() execution time for various PLL types and Exynos SoCs variants. Actual PLL lock time depends on the P divider value, the VCO frequency and a constant PLL type specific LOCK_FACTOR and can be calculated as lock_time = Pdiv * LOCK_FACTOR / VCO_freq For the ktime API use cases a common timeout value of 20 ms is applied for all the PLLs with an assumption that maximum possible value of Pdiv is 64, maximum possible LOCK_FACTOR value is 3000 and minimum VCO frequency is 24 MHz. Signed-off-by: Sylwester Nawrocki Reviewed-by: Krzysztof Kozlowski Thanks, patch applied. -- Regards, Sylwester
Re: [PATCH] clk: samsung: allow compile testing of Exynos, S3C64xx and S5Pv210
On 11/22/20 12:34, Krzysztof Kozlowski wrote: > On Fri, Nov 20, 2020 at 05:36:35PM +0100, Sylwester Nawrocki wrote: >> On 11/19/20 17:45, Krzysztof Kozlowski wrote: >>> So far all Exynos, S3C64xx and S5Pv210 clock units were selected by >>> respective SOC/ARCH Kconfig option. On a kernel built for selected >>> SoCs, this allowed to build only limited set of matching clock drivers. >>> However compile testing was not possible in such case as Makefile object >>> depent on SOC/ARCH option. >> >> "objects depend" or "object depends" ? > > "object depends" > >>> Add separate Kconfig options for each of them to be able to compile >>> test. >>> >>> Signed-off-by: Krzysztof Kozlowski >> >> The patch look good to me, thanks. >> Acked-by: Sylwester Nawrocki >> >> I guess it's best now to merge it through your tree as it depends on >> patches already sent to arm-soc? Next time it might be better to use >> immutable branches right away to keep the clk changes in the clk >> maintainer's tree. > > At that time I had only one clk patch so I did not put it on separate > branch. > > Anyway, this does not depend on the clkout patches and only minor patch > adjustement is needed. Cherry-pick can solve it (you would need to apply > on next/master and then cherry pick) or I can resend you one rebased on > linus/master. > > There should be no conflicts when merging later into next or linus. > > I propose you should take it via clk tree. Indeed, the conflict is minimal, I should have checked with git cherry-pick once I found a branch where the patch applied cleanly. I have corrected the typo an applied, thank you! -- Regards, Sylwester
Re: [PATCH 38/38] ASoC: samsung: smdk_wm8994: remove redundant of_match_ptr()
On 11/20/20 17:16, Krzysztof Kozlowski wrote: of_match_device() already handles properly !CONFIG_OF case, so passing the argument via of_match_ptr() is not needed. Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 09/38] ASoC: samsung: smdk_wm8994: drop of_match_ptr from of_device_id table
On 11/20/20 17:16, Krzysztof Kozlowski wrote: The driver can match only via the DT table so the table should be always used and the of_match_ptr does not have any sense (this also allows ACPI matching via PRP0001, even though it is not relevant here). This fixes compile warning (!CONFIG_OF on x86_64): sound/soc/samsung/smdk_wm8994.c:139:34: warning: ‘samsung_wm8994_of_match’ defined but not used [-Wunused-const-variable=] Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 34/38] ASoC: samsung: i2s: mark OF related data as maybe unused
On 11/20/20 17:16, Krzysztof Kozlowski wrote: The driver can be compile tested with !CONFIG_OF making certain data unused: sound/soc/samsung/i2s.c:1646:42: warning: ‘i2sv5_dai_type_i2s1’ defined but not used [-Wunused-const-variable=] sound/soc/samsung/i2s.c:1639:42: warning: ‘i2sv7_dai_type’ defined but not used [-Wunused-const-variable=] Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH] clk: samsung: allow compile testing of Exynos, S3C64xx and S5Pv210
On 11/19/20 17:45, Krzysztof Kozlowski wrote: > So far all Exynos, S3C64xx and S5Pv210 clock units were selected by > respective SOC/ARCH Kconfig option. On a kernel built for selected > SoCs, this allowed to build only limited set of matching clock drivers. > However compile testing was not possible in such case as Makefile object > depent on SOC/ARCH option. "objects depend" or "object depends" ? > Add separate Kconfig options for each of them to be able to compile > test. > > Signed-off-by: Krzysztof Kozlowski The patch look good to me, thanks. Acked-by: Sylwester Nawrocki I guess it's best now to merge it through your tree as it depends on patches already sent to arm-soc? Next time it might be better to use immutable branches right away to keep the clk changes in the clk maintainer's tree. -- Regards, Sylwester
[PATCH v5] clk: samsung: Prevent potential endless loop in the PLL ops
The PLL status polling loops in the set_rate callbacks of some PLLs
have no timeout detection and may become endless loops when something
goes wrong with the PLL.
For some PLLs there is already the ktime API based timeout detection,
but it will not work in all conditions when .set_rate gets called.
In particular, before the clocksource is initialized or when the
timekeeping is suspended.
This patch adds a common helper with the PLL status bit polling and
timeout detection. For conditions where the timekeeping API should not
be used a simple readl_relaxed/cpu_relax() busy loop is added with the
iterations limit derived from measurements of readl_relaxed() execution
time for various PLL types and Exynos SoCs variants.
Actual PLL lock time depends on the P divider value, the VCO frequency
and a constant PLL type specific LOCK_FACTOR and can be calculated as
lock_time = Pdiv * LOCK_FACTOR / VCO_freq
For the ktime API use cases a common timeout value of 20 ms is applied
for all the PLLs with an assumption that maximum possible value of Pdiv
is 64, maximum possible LOCK_FACTOR value is 3000 and minimum VCO
frequency is 24 MHz.
Signed-off-by: Sylwester Nawrocki
---
I'm not sure whether we actually need to implement precise timeouts,
likely the simple busy loop case would be enough. AFAIK the PLL
failures happen very rarely, mostly in early code development stage
for given platform.
Changes since v4:
- s/__early_timeout/pll_early_timeout
Changes since v3:
- dropped udelay() from the PLL status bit polling loop as it didn't
work on arm64 at early boot time, before timekeeping was initialized,
- use the timekeeping API in cases when it is already initialized and
not suspended,
- use samsung_pll_lock_wait() also in samsung_pll3xxx_enable() function,
now all potential endless loops are eliminated.
---
drivers/clk/samsung/clk-pll.c | 147 --
1 file changed, 71 insertions(+), 76 deletions(-)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index ac70ad7..5873a93 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -8,14 +8,17 @@
#include
#include
+#include
#include
#include
+#include
#include
#include
#include "clk.h"
#include "clk-pll.h"
-#define PLL_TIMEOUT_MS 10
+#define PLL_TIMEOUT_US 2U
+#define PLL_TIMEOUT_LOOPS 100U
struct samsung_clk_pll {
struct clk_hw hw;
@@ -63,6 +66,53 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
return rate_table[i - 1].rate;
}
+static bool pll_early_timeout = true;
+
+static int __init samsung_pll_disable_early_timeout(void)
+{
+ pll_early_timeout = false;
+ return 0;
+}
+arch_initcall(samsung_pll_disable_early_timeout);
+
+/* Wait until the PLL is locked */
+static int samsung_pll_lock_wait(struct samsung_clk_pll *pll,
+unsigned int reg_mask)
+{
+ int i, ret;
+ u32 val;
+
+ /*
+* This function might be called when the timekeeping API can't be used
+* to detect timeouts. One situation is when the clocksource is not yet
+* initialized, another when the timekeeping is suspended. udelay() also
+* cannot be used when the clocksource is not running on arm64, since
+* the current timer is used as cycle counter. So a simple busy loop
+* is used here in that special cases. The limit of iterations has been
+* derived from experimental measurements of various PLLs on multiple
+* Exynos SoC variants. Single register read time was usually in range
+* 0.4...1.5 us, never less than 0.4 us.
+*/
+ if (pll_early_timeout || timekeeping_suspended) {
+ i = PLL_TIMEOUT_LOOPS;
+ while (i-- > 0) {
+ if (readl_relaxed(pll->con_reg) & reg_mask)
+ return 0;
+
+ cpu_relax();
+ }
+ ret = -ETIMEDOUT;
+ } else {
+ ret = readl_relaxed_poll_timeout_atomic(pll->con_reg, val,
+ val & reg_mask, 0, PLL_TIMEOUT_US);
+ }
+
+ if (ret < 0)
+ pr_err("Could not lock PLL %s\n", clk_hw_get_name(>hw));
+
+ return ret;
+}
+
static int samsung_pll3xxx_enable(struct clk_hw *hw)
{
struct samsung_clk_pll *pll = to_clk_pll(hw);
@@ -72,13 +122,7 @@ static int samsung_pll3xxx_enable(struct clk_hw *hw)
tmp |= BIT(pll->enable_offs);
writel_relaxed(tmp, pll->con_reg);
- /* wait lock time */
- do {
- cpu_relax();
- tmp = readl_relaxed(pll->con_reg);
- } while (!(tmp & BIT(pll->lock_offs)));
-
- return 0;
+ return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
}
static void samsung_pll3xxx_disable(struct clk_
Re: [PATCH v9 0/5] Exynos: Simple QoS for exynos-bus using interconnect
Hi Georgi, Chanwoo,
On 13.11.2020 10:07, Chanwoo Choi wrote:
> On 11/13/20 5:48 PM, Georgi Djakov wrote:
>> On 11/12/20 16:09, Sylwester Nawrocki wrote:
[...]
>>
>> Good work Sylwester! Thank you and all the reviewers! What would be the merge
>> path for this patchset? Looks like there is no build dependency between
>> patches.
>> Should i take just patches 2,3 or also patch 1? Chanwoo?
>
> Hi Georgi,
>
> If you take the patch 2,3, I'll apply patch 1,4 to devfreq.git.
> Hi Sylwester,
> First of all, thanks for your work to finish it for a long time.
> I'm very happy about finishing this work. It is very necessary feature
> for the QoS. Once again, thank for your work.
I would also like to thank everyone for provided feedback!
As far as building is concerned the patches could be applied in any
order. I think we could also apply the drm/exynos patch in same
merge window. There could be runtime (or git bisect) regression
only in case when INTERCONNECT is enabled and only (or as first)
the dts and drm/exynos patches are applied.
Hmm, maybe it's better to hold on with the drm patch, INTERCONNECT
is disabled in arch/arm/configs/{multi_v7_defconfig, exynos_defconfig}
but it is enabled in arch/arm64/configs/defconfig.
--
Regards,
Sylwester
Re: [PATCH] clk: samsung: allow building the clkout driver as module
On 11/10/20 20:37, Krzysztof Kozlowski wrote: > The Exynos clock output driver can be built as module (it does not have > to be part of core init process) for better customization. Adding a > KConfig entry allows also compile testing for build coverage. > > Signed-off-by: Krzysztof Kozlowski This needs to go through your tree due to dependencies on your previous patches, so Acked-by: Sylwester Nawrocki > --- > drivers/clk/samsung/Kconfig | 10 ++ > drivers/clk/samsung/Makefile| 2 +- > drivers/clk/samsung/clk-exynos-clkout.c | 1 + > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig > index 57d4b3f20417..b6b2cb209543 100644 > --- a/drivers/clk/samsung/Kconfig > +++ b/drivers/clk/samsung/Kconfig > @@ -19,6 +19,16 @@ config EXYNOS_AUDSS_CLK_CON > on some Exynos SoC variants. Choose M or Y here if you want to > use audio devices such as I2S, PCM, etc. > > +config EXYNOS_CLK_OUT Perhaps change it EXYNOS_CLKOUT for a better match with the SoC documentation? > + tristate "Samsung Exynos clock output driver" > + depends on COMMON_CLK_SAMSUNG > + default y if ARCH_EXYNOS > + help > + Support for the clock output (XCLKOUT) driver present on some of > + Exynos SoC variants. Usually the XCLKOUT is used to monitor the > + status of the certains clocks from SoC, but it could also be tied to > + other devices as an input clock. > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile > index 1a4e6b787978..4adbf972e9f6 100644 > --- a/drivers/clk/samsung/Makefile > +++ b/drivers/clk/samsung/Makefile > @@ -15,7 +15,7 @@ obj-$(CONFIG_SOC_EXYNOS5420)+= clk-exynos5420.o > obj-$(CONFIG_SOC_EXYNOS5420)+= clk-exynos5-subcmu.o > obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos5433.o > obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o > -obj-$(CONFIG_ARCH_EXYNOS)+= clk-exynos-clkout.o > +obj-$(CONFIG_EXYNOS_CLK_OUT) += clk-exynos-clkout.o -- Regards, Sylwester
[PATCH v9 4/5] PM / devfreq: exynos-bus: Add registration of interconnect child device
This patch adds registration of a child platform device for the exynos
interconnect driver. It is assumed that the interconnect provider will
only be needed when #interconnect-cells property is present in the bus
DT node, hence the child device will be created only when such a property
is present.
Acked-by: Krzysztof Kozlowski
Acked-by: Chanwoo Choi
Signed-off-by: Sylwester Nawrocki
---
Changes for v9:
- whitespace (indentation) corrections.
Changes for v8...v6:
- none.
Changes for v5:
- new patch.
---
drivers/devfreq/exynos-bus.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 1e684a4..20447a4 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -24,6 +24,7 @@
struct exynos_bus {
struct device *dev;
+ struct platform_device *icc_pdev;
struct devfreq *devfreq;
struct devfreq_event_dev **edev;
@@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev)
if (ret < 0)
dev_warn(dev, "failed to disable the devfreq-event devices\n");
+ platform_device_unregister(bus->icc_pdev);
+
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
if (bus->opp_table) {
@@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev)
{
struct exynos_bus *bus = dev_get_drvdata(dev);
+ platform_device_unregister(bus->icc_pdev);
+
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
}
@@ -432,6 +437,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
+ /* Create child platform device for the interconnect provider */
+ if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
+ bus->icc_pdev = platform_device_register_data(
+ dev, "exynos-generic-icc",
+ PLATFORM_DEVID_AUTO, NULL, 0);
+
+ if (IS_ERR(bus->icc_pdev)) {
+ ret = PTR_ERR(bus->icc_pdev);
+ goto err;
+ }
+ }
+
max_state = bus->devfreq->profile->max_state;
min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
--
2.7.4
[PATCH v9 5/5] drm: exynos: mixer: Add interconnect support
This patch adds interconnect support to exynos-mixer. The mixer works
the same as before when CONFIG_INTERCONNECT is 'n'.
For proper operation of the video mixer block we need to ensure the
interconnect busses like DMC or LEFTBUS provide enough bandwidth so
as to avoid DMA buffer underruns in the mixer block. I.e we need to
prevent those busses from operating in low perfomance OPPs when
the mixer is running.
In this patch the bus bandwidth request is done through the interconnect
API, the bandwidth value is calculated from selected DRM mode, i.e.
video plane width, height, refresh rate and pixel format.
The bandwidth setting is synchronized with VSYNC when we are switching
to lower bandwidth. This is required to ensure enough bandwidth for
the device since new settings are normally being applied in the hardware
synchronously with VSYNC.
Acked-by: Krzysztof Kozlowski
Reviewed-by: Chanwoo Choi
Tested-by: Chanwoo Choi
Co-developed-by: Artur Świgoń
Signed-off-by: Artur Świgoń
Co-developed-by: Marek Szyprowski
Signed-off-by: Marek Szyprowski
Signed-off-by: Sylwester Nawrocki
---
Changes for v9:
- whitespace cleanup,
- Co-developed-by/Signed-off-by tags corrections.
Changes for v8:
- updated comment in mixer_probe()
Changes for v7:
- fixed incorrect setting of the ICC bandwidth when the mixer is
disabled, now the bandwidth is set explicitly to 0 in such case.
Changes for v6:
- the icc_set_bw() call is now only done when calculated value for
a crtc changes, this avoids unnecessary calls per each video frame
- added synchronization of the interconnect bandwidth setting with
the mixer VSYNC in order to avoid buffer underflow when we lower
the interconnect bandwidth when the hardware still operates with
previous mode settings that require higher bandwidth. This fixed
IOMMU faults observed e.g. during switching from two planes to
a single plane operation.
Changes for v5:
- renamed soc_path variable to icc_path
---
drivers/gpu/drm/exynos/exynos_mixer.c | 146 --
1 file changed, 138 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
b/drivers/gpu/drm/exynos/exynos_mixer.c
index af192e5..502ce04 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -73,6 +74,7 @@ enum mixer_flag_bits {
MXR_BIT_INTERLACE,
MXR_BIT_VP_ENABLED,
MXR_BIT_HAS_SCLK,
+ MXR_BIT_REQUEST_BW,
};
static const uint32_t mixer_formats[] = {
@@ -99,6 +101,13 @@ struct mixer_context {
struct exynos_drm_plane planes[MIXER_WIN_NR];
unsigned long flags;
+ struct icc_path *icc_path;
+ /* memory bandwidth on the interconnect bus in B/s */
+ unsigned long icc_bandwidth;
+ /* mutex protecting @icc_bandwidth */
+ struct mutexicc_lock;
+ struct work_struct work;
+
int irq;
void __iomem*mixer_regs;
void __iomem*vp_regs;
@@ -754,6 +763,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
val |= MXR_INT_CLEAR_VSYNC;
val &= ~MXR_INT_STATUS_VSYNC;
+ if (test_and_clear_bit(MXR_BIT_REQUEST_BW, >flags))
+ schedule_work(>work);
+
/* interlace scan need to check shadow register */
if (test_bit(MXR_BIT_INTERLACE, >flags)
&& !mixer_is_synced(ctx))
@@ -934,6 +946,76 @@ static void mixer_disable_vblank(struct exynos_drm_crtc
*crtc)
mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
}
+/**
+ * mixer_get_memory_bandwidth - calculate memory bandwidth for current crtc
mode
+ * @crtc: a crtc with DRM mode to calculate the bandwidth for
+ *
+ * Return: memory bandwidth in B/s
+ *
+ * This function returns memory bandwidth calculated as a sum of amount of data
+ * per second for each plane. The calculation is based on maximum possible
pixel
+ * resolution for a plane so as to avoid different bandwidth request per each
+ * video frame. The formula used for calculation for each plane is:
+ *
+ * bw = width * height * frame_rate / interlace / (hor_subs * vert_subs)
+ *
+ * where:
+ * - width, height - (DRM mode) video frame width and height in pixels,
+ * - frame_rate - DRM mode frame refresh rate,
+ * - interlace: 1 - in case of progressive and 2 in case of interlaced video,
+ * - hor_subs, vert_subs - accordingly horizontal and vertical pixel
+ *subsampling for a plane.
+ */
+static unsigned int mixer_get_memory_bandwidth(struct exynos_drm_crtc *crtc)
+{
+ struct drm_display_mode *mode = >base.state->adjusted_mode;
+ struct mixer_context *ctx = crtc->ctx;
+ unsigned long bw, bandwidth = 0;
+ int i, j, sub;
+
+ f
[PATCH v9 2/5] interconnect: Add generic interconnect driver for Exynos SoCs
This patch adds a generic interconnect driver for Exynos SoCs in order to provide interconnect functionality for each "samsung,exynos-bus" compatible device. The SoC topology is a graph (or more specifically, a tree) and its edges are described by specifying in the 'interconnects' property the interconnect consumer path for each interconnect provider DT node. Each bus is now an interconnect provider and an interconnect node as well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers itself as a node. Node IDs are not hard coded but rather assigned dynamically at runtime. This approach allows for using this driver with various Exynos SoCs. Frequencies requested via the interconnect API for a given node are propagated to devfreq using dev_pm_qos_update_request(). Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in which case all interconnect API functions are no-op. The samsung,data-clk-ratio DT property is used to specify the ratio of the interconect bandwidth to the minimum data clock frequency for each bus. Due to unspecified relative probing order, -EPROBE_DEFER may be propagated to ensure that the parent is probed before its children. Reviewed-by: Chanwoo Choi Tested-by: Chanwoo Choi Acked-by: Krzysztof Kozlowski Signed-off-by: Artur Świgoń Signed-off-by: Sylwester Nawrocki --- Changes for v9: - Makefile and Kconfig fixes/improvements. Changes for v8: - renamed drivers/interconnect/exynos to drivers/interconnect/samsung, - added missing driver sync_state callback assignment. Changes for v7: - adjusted to the DT property changes: "interconnects" instead of "samsung,interconnect-parent", "samsung,data-clk-ratio" instead of "bus-width", - adaptation to of_icc_get_from_provider() function changes in v5.10-rc1. Changes for v6: - corrected of_node dereferencing in exynos_icc_get_parent() function, - corrected initialization of icc_node->name so as to avoid direct of_node->name dereferencing, - added parsing of bus-width DT property. Changes for v5: - adjust to renamed exynos,interconnect-parent-node property, - use automatically generated platform device id as the interconect node id instead of a now unavailable devfreq->id field, - add icc_ prefix to some variables to make the code more self-commenting, - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(), - adjust to exynos,interconnect-parent-node property rename to samsung,interconnect-parent, - converted to a separate platform driver in drivers/interconnect. --- drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile | 1 + drivers/interconnect/samsung/Kconfig | 13 +++ drivers/interconnect/samsung/Makefile | 4 + drivers/interconnect/samsung/exynos.c | 199 ++ 5 files changed, 218 insertions(+) create mode 100644 drivers/interconnect/samsung/Kconfig create mode 100644 drivers/interconnect/samsung/Makefile create mode 100644 drivers/interconnect/samsung/exynos.c diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig index 5b7204e..d637a89 100644 --- a/drivers/interconnect/Kconfig +++ b/drivers/interconnect/Kconfig @@ -13,5 +13,6 @@ if INTERCONNECT source "drivers/interconnect/imx/Kconfig" source "drivers/interconnect/qcom/Kconfig" +source "drivers/interconnect/samsung/Kconfig" endif diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile index d203520..97d393f 100644 --- a/drivers/interconnect/Makefile +++ b/drivers/interconnect/Makefile @@ -6,3 +6,4 @@ icc-core-objs := core.o bulk.o obj-$(CONFIG_INTERCONNECT) += icc-core.o obj-$(CONFIG_INTERCONNECT_IMX) += imx/ obj-$(CONFIG_INTERCONNECT_QCOM)+= qcom/ +obj-$(CONFIG_INTERCONNECT_SAMSUNG) += samsung/ diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig new file mode 100644 index 000..6820e4f --- /dev/null +++ b/drivers/interconnect/samsung/Kconfig @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: GPL-2.0-only +config INTERCONNECT_SAMSUNG + bool "Samsung SoC interconnect drivers" + depends on ARCH_EXYNOS || COMPILE_TEST + help + Interconnect drivers for Samsung SoCs. + +config INTERCONNECT_EXYNOS + tristate "Exynos generic interconnect driver" + depends on INTERCONNECT_SAMSUNG + default y if ARCH_EXYNOS + help + Generic interconnect driver for Exynos SoCs. diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile new file mode 100644 index 000..e19d1df --- /dev/null +++ b/drivers/interconnect/samsung/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +exynos-interconnect-objs := exynos.o + +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos-interconnec
[PATCH v9 1/5] dt-bindings: devfreq: Add documentation for the interconnect properties
Add documentation for new optional properties in the exynos bus nodes:
interconnects, #interconnect-cells, samsung,data-clock-ratio.
These properties allow to specify the SoC interconnect structure which
then allows the interconnect consumer devices to request specific
bandwidth requirements.
Acked-by: Krzysztof Kozlowski
Acked-by: Chanwoo Choi
Tested-by: Chanwoo Choi
Acked-by: Rob Herring
Signed-off-by: Artur Świgoń
Signed-off-by: Sylwester Nawrocki
---
Changes for v9:
- added Ack tags
Changes for v8:
- updated description of the interconnects property,
- fixed typo in samsung,data-clk-ratio property description.
Changes for v7:
- bus-width property replaced with samsung,data-clock-ratio,
- the interconnect consumer bindings used instead of vendor specific
properties
Changes for v6:
- added dts example of bus hierarchy definition and the interconnect
consumer,
- added new bus-width property.
Changes for v5:
- exynos,interconnect-parent-node renamed to samsung,interconnect-parent
---
.../devicetree/bindings/devfreq/exynos-bus.txt | 71 +-
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index e71f752..bcaa2c0 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -51,6 +51,19 @@ Optional properties only for parent bus device:
- exynos,saturation-ratio: the percentage value which is used to calibrate
the performance count against total cycle count.
+Optional properties for the interconnect functionality (QoS frequency
+constraints):
+- #interconnect-cells: should be 0.
+- interconnects: as documented in ../interconnect.txt, describes a path at the
+ higher level interconnects used by this interconnect provider.
+ If this interconnect provider is directly linked to a top level interconnect
+ provider the property contains only one phandle. The provider extends
+ the interconnect graph by linking its node to a node registered by provider
+ pointed to by first phandle in the 'interconnects' property.
+
+- samsung,data-clock-ratio: ratio of the data throughput in B/s to minimum data
+ clock frequency in Hz, default value is 8 when this property is missing.
+
Detailed correlation between sub-blocks and power line according to Exynos SoC:
- In case of Exynos3250, there are two power line as following:
VDD_MIF |--- DMC
@@ -135,7 +148,7 @@ Detailed correlation between sub-blocks and power line
according to Exynos SoC:
|--- PERIC (Fixed clock rate)
|--- FSYS (Fixed clock rate)
-Example1:
+Example 1:
Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
power line (regulator). The MIF (Memory Interface) AXI bus is used to
transfer data between DRAM and CPU and uses the VDD_MIF regulator.
@@ -184,7 +197,7 @@ Example1:
|L5 |20 |20 |40 |30 | ||100 |
--
-Example2 :
+Example 2:
The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
is listed below:
@@ -419,3 +432,57 @@ Example2 :
devfreq = <_leftbus>;
status = "okay";
};
+
+Example 3:
+ An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
+ Exynos4412 SoC with video mixer as an interconnect consumer device.
+
+ soc {
+ bus_dmc: bus_dmc {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_DIV_DMC>;
+ clock-names = "bus";
+ operating-points-v2 = <_dmc_opp_table>;
+ samsung,data-clock-ratio = <4>;
+ #interconnect-cells = <0>;
+ };
+
+ bus_leftbus: bus_leftbus {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_DIV_GDL>;
+ clock-names = "bus";
+ operating-points-v2 = <_leftbus_opp_table>;
+ #interconnect-cells = <0>;
+ interconnects = <_dmc>;
+ };
+
+ bus_display: bus_display {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_ACLK160>;
+ clock-names = "bus";
+ operating-points-v2 = <_display_opp_table>;
+ #interconnect-cells = <0>;
+ interconnects = <_leftbus _dmc>;
+ };
+
+ bus_dmc_opp_table: opp_table1 {
+ compatible = "
[PATCH v9 3/5] MAINTAINERS: Add entry for Samsung interconnect drivers
Add maintainers entry for the Samsung SoC interconnect drivers, this currently includes the Exynos generic interconnect driver. Reviewed-by: Chanwoo Choi Signed-off-by: Sylwester Nawrocki --- Changes for v9: - add linux-samsung-soc ML, - fixed Artur's last name spelling, - whole entry moved to maintain alphabetical order, - added missing traling '/' to directory path. Changes since v7: - new patch. --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index e73636b..1d8246c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15400,6 +15400,14 @@ L: linux-fb...@vger.kernel.org S: Maintained F: drivers/video/fbdev/s3c-fb.c +SAMSUNG INTERCONNECT DRIVERS +M: Sylwester Nawrocki +M: Artur Świgoń +L: linux...@vger.kernel.org +L: linux-samsung-...@vger.kernel.org +S: Supported +F: drivers/interconnect/samsung/ + SAMSUNG LAPTOP DRIVER M: Corentin Chary L: platform-driver-...@vger.kernel.org -- 2.7.4
[PATCH v9 0/5] Exynos: Simple QoS for exynos-bus using interconnect
This patchset adds interconnect API support for the Exynos SoC "samsung, exynos-bus" compatible devices, which already have their corresponding exynos-bus driver in the devfreq subsystem. Complementing the devfreq driver with an interconnect functionality allows to ensure the QoS requirements of devices accessing the system memory (e.g. video processing devices) are fulfilled and allows to avoid issues like the one discussed in thread [1]. This patch series adds implementation of the interconnect provider per each "samsung,exynos-bus" compatible DT node, with one interconnect node per provider. The interconnect code which was previously added as a part of the devfreq driver has been converted to a separate platform driver. In the devfreq a corresponding virtual child platform device is registered. Integration of devfreq and interconnect frameworks is achieved through the PM QoS API. A sample interconnect consumer for exynos-mixer is added in patch 5/5, it is currently added only for exynos4412 and allows to address the mixer DMA underrun error issues [1]. Changes since v8: - excluded from the series already applied dts patches, - Co-developed-by/Signed-off-by tag corrections, Ack tags added, - the maintainers entry corrections adressing review comments, - Kconfig/Makefile improvements/corrections, - whitespace/indentation cleanup. The series has been tested on Odroid U3 board. It is based on v5.10-rc1. -- Regards, Sylwester Changes since v7: - drivers/interconnect/exynos renamed to drivers/interconnect/samsung, - added INTERCONNECT_SAMSUNG Kconfig symbol, - added missing driver sync_state callback, - improved the DT binding description, - added a patch adding maintainers entry, - updated comment in patch 7/7, typo fix (patch 1/7). Changes since v6: - the interconnect consumer DT bindings are now used to describe dependencies of the interconnects (samsung,exynos-bus nodes), - bus-width property replaced with samsung,data-clk-ratio, - adaptation to recent changes in the interconnect code (of_icc_get_from_provider(), icc_node_add()). Changes since v5: - addition of "bus-width: DT property, which specifies data width of the interconnect bus (patches 1...2/6), - addition of synchronization of the interconnect bandwidth setting with VSYNC (patch 6/6). Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed changes are listed in patches: - conversion to a separate interconnect (platform) driver, - an update of the DT binding documenting new optional properties: #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus" nodes, - new DT properties added to the SoC, rather than to the board specific files. Changes since v2 [5]: - Use icc_std_aggregate(). - Implement a different modification of apply_constraints() in drivers/interconnect/core.c (patch 03). - Use 'exynos,interconnect-parent-node' in the DT instead of 'devfreq'/'parent', depending on the bus. - Rebase on DT patches that deprecate the 'devfreq' DT property. - Improve error handling, including freeing generated IDs on failure. - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent(). Changes since v1 [6]: - Rebase on coupled regulators patches. - Use dev_pm_qos_*() API instead of overriding frequency in exynos_bus_target(). - Use IDR for node ID allocation. - Reverse order of multiplication and division in mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow. References: [1] https://patchwork.kernel.org/patch/10861757/ (original issue) [2] https://www.spinics.net/lists/linux-samsung-soc/msg70014.html [3] https://www.spinics.net/lists/arm-kernel/msg810722.html [4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swi...@samsung.com [5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC) [6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC) Sylwester Nawrocki (5): dt-bindings: devfreq: Add documentation for the interconnect properties interconnect: Add generic interconnect driver for Exynos SoCs MAINTAINERS: Add entry for Samsung interconnect drivers PM / devfreq: exynos-bus: Add registration of interconnect child device drm: exynos: mixer: Add interconnect support .../devicetree/bindings/devfreq/exynos-bus.txt | 71 +++- MAINTAINERS| 8 + drivers/devfreq/exynos-bus.c | 17 ++ drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++- drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile | 1 + drivers/interconnect/samsung/Kconfig | 13 ++ drivers/interconnect/samsung/Makefile | 4 + drivers/interconnect/samsung/exynos.c | 199 + 9 files changed, 450 insertions(+), 10 deletions(-) create mode 100644 drivers/interconnect/samsung/
[PATCH v4] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
The PLL status polling loops in the set_rate callbacks of some PLLs
have no timeout detection and may become endless loops when something
goes wrong with the PLL.
For some PLLs there is already the ktime API based timeout detection,
but it will not work in all conditions when .set_rate gets called.
In particular, before the clocksource is initialized or when the
timekeeping is suspended.
This patch adds a common helper with the PLL status bit polling and
timeout detection. For conditions where the timekeeping API should not
be used a simple readl_relaxed/cpu_relax() busy loop is added with the
iterations limit derived from measurements of readl_relaxed() execution
time for various PLL types and Exynos SoCs variants.
Actual PLL lock time depends on the P divider value, the VCO frequency
and a constant PLL type specific LOCK_FACTOR and can be calculated as
lock_time = Pdiv * LOCK_FACTOR / VCO_freq
For the ktime API use cases a common timeout value of 20 ms is applied
for all the PLLs with an assumption that maximum possible value of Pdiv
is 64, maximum possible LOCK_FACTOR value is 3000 and minimum VCO
frequency is 24 MHz.
Signed-off-by: Sylwester Nawrocki
---
I'm not sure whether we actually need to implement precise timeouts,
likely the simple busy loop case would be enough. AFAIK the PLL
failures happen very rarely, mostly in early code development stage
for given platform.
Changes since v3:
- dropped udelay() from the PLL status bit polling loop as it didn't
work on arm64 at early boot time, before timekeeping was initialized,
- use the timekeeping API in cases when it is already initialized and
not suspended,
- use samsung_pll_lock_wait() also in samsung_pll3xxx_enable() function,
now all potential endless loops are removed.
---
drivers/clk/samsung/clk-pll.c | 147 --
1 file changed, 71 insertions(+), 76 deletions(-)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index ac70ad7..cefb57e 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -8,14 +8,17 @@
#include
#include
+#include
#include
#include
+#include
#include
#include
#include "clk.h"
#include "clk-pll.h"
-#define PLL_TIMEOUT_MS 10
+#define PLL_TIMEOUT_US 2U
+#define PLL_TIMEOUT_LOOPS 100U
struct samsung_clk_pll {
struct clk_hw hw;
@@ -63,6 +66,53 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
return rate_table[i - 1].rate;
}
+static bool __early_timeout = true;
+
+static int __init samsung_pll_disable_early_timeout(void)
+{
+ __early_timeout = false;
+ return 0;
+}
+arch_initcall(samsung_pll_disable_early_timeout);
+
+/* Wait until the PLL is locked */
+static int samsung_pll_lock_wait(struct samsung_clk_pll *pll,
+unsigned int reg_mask)
+{
+ int i, ret;
+ u32 val;
+
+ /*
+* This function might be called when the timekeeping API can't be used
+* to detect timeouts. One situation is when the clocksource is not yet
+* initialized, another when the timekeeping is suspended. udelay() also
+* cannot be used when the clocksource is not running on arm64, since
+* the current timer is used as cycle counter. So a simple busy loop
+* is used here in that special cases. The limit of iterations has been
+* derived from experimental measurements of various PLLs on multiple
+* Exynos SoC variants. Single register read time was usually in range
+* 0.4...1.5 us, never less than 0.4 us.
+*/
+ if (__early_timeout || timekeeping_suspended) {
+ i = PLL_TIMEOUT_LOOPS;
+ while (i-- > 0) {
+ if (readl_relaxed(pll->con_reg) & reg_mask)
+ return 0;
+
+ cpu_relax();
+ }
+ ret = -ETIMEDOUT;
+ } else {
+ ret = readl_relaxed_poll_timeout_atomic(pll->con_reg, val,
+ val & reg_mask, 0, PLL_TIMEOUT_US);
+ }
+
+ if (ret < 0)
+ pr_err("Could not lock PLL %s\n", clk_hw_get_name(>hw));
+
+ return ret;
+}
+
static int samsung_pll3xxx_enable(struct clk_hw *hw)
{
struct samsung_clk_pll *pll = to_clk_pll(hw);
@@ -72,13 +122,7 @@ static int samsung_pll3xxx_enable(struct clk_hw *hw)
tmp |= BIT(pll->enable_offs);
writel_relaxed(tmp, pll->con_reg);
- /* wait lock time */
- do {
- cpu_relax();
- tmp = readl_relaxed(pll->con_reg);
- } while (!(tmp & BIT(pll->lock_offs)));
-
- return 0;
+ return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
}
static void samsung_pll3xxx_disable(struct clk_hw *hw)
@@ -240,13 +284,10 @@ static int samsung_pll35xx_set_rate(stru
Re: [PATCH v2] clk: exynos7: Keep aclk_fsys1_200 enabled
On 11/9/20 13:32, Sylwester Nawrocki wrote:
-8<
diff --git a/drivers/clk/samsung/clk-exynos7.c
b/drivers/clk/samsung/clk-exynos7.c
index 87ee1ba..9ecf498 100644
--- a/drivers/clk/samsung/clk-exynos7.c
+++ b/drivers/clk/samsung/clk-exynos7.c
@@ -570,7 +570,15 @@ static const struct samsung_cmu_info top1_cmu_info
__initconst = {
static void __init exynos7_clk_top1_init(struct device_node *np)
{
- samsung_cmu_register_one(np, _cmu_info);
+ struct samsung_clk_provider *ctx;
+ struct clk_hw **hws;
+
+ ctx = samsung_cmu_register_one(np, _cmu_info);
+ if (!ctx)
+ return;
+ hws = ctx->clk_data.hws;
+
+ clk_prepare_enable(hws[CLK_ACLK_FSYS1_200]);
Of course it was supposed to be:
clk_prepare_enable(hws[CLK_ACLK_FSYS1_200]->clk);
}
CLK_OF_DECLARE(exynos7_clk_top1, "samsung,exynos7-clock-top1",
-8<
Re: [PATCH v2] clk: exynos7: Keep aclk_fsys1_200 enabled
Hi Paweł,
On 11/7/20 13:14, Paweł Chmiel wrote:
> This clock must be always enabled to allow access to any registers in
> fsys1 CMU. Until proper solution based on runtime PM is applied
> (similar to what was done for Exynos5433), fix this by calling
> clk_prepare_enable() directly from clock provider driver.
>
> It was observed on Samsung Galaxy S6 device (based on Exynos7420), where
> UFS module is probed before pmic used to power that device.
> In this case defer probe was happening and that clock was disabled by
> UFS driver, causing whole boot to hang on next CMU access.
>
> Signed-off-by: Paweł Chmiel
> --- a/drivers/clk/samsung/clk-exynos7.c
> +++ b/drivers/clk/samsung/clk-exynos7.c
> @@ -571,6 +572,10 @@ static const struct samsung_cmu_info top1_cmu_info
> __initconst = {
> static void __init exynos7_clk_top1_init(struct device_node *np)
> {
> samsung_cmu_register_one(np, _cmu_info);
> + /*
> + * Keep top FSYS1 aclk enabled permanently. It's required for CMU
> register access.
> + */
> + clk_prepare_enable(__clk_lookup("aclk_fsys1_200"));
Thanks for the patch. Could you rework it to avoid the __clk_lookup() call?
I.e. could you change it to something along the lines of:
-8<
diff --git a/drivers/clk/samsung/clk-exynos7.c
b/drivers/clk/samsung/clk-exynos7.c
index 87ee1ba..9ecf498 100644
--- a/drivers/clk/samsung/clk-exynos7.c
+++ b/drivers/clk/samsung/clk-exynos7.c
@@ -570,7 +570,15 @@ static const struct samsung_cmu_info top1_cmu_info
__initconst = {
static void __init exynos7_clk_top1_init(struct device_node *np)
{
- samsung_cmu_register_one(np, _cmu_info);
+ struct samsung_clk_provider *ctx;
+ struct clk_hw **hws;
+
+ ctx = samsung_cmu_register_one(np, _cmu_info);
+ if (!ctx)
+ return;
+ hws = ctx->clk_data.hws;
+
+ clk_prepare_enable(hws[CLK_ACLK_FSYS1_200]);
}
CLK_OF_DECLARE(exynos7_clk_top1, "samsung,exynos7-clock-top1",
-8<
?
--
Regards,
Sylwester
Re: [PATCH v8 3/7] MAINTAINERS: Add entry for Samsung interconnect drivers
On 04.11.2020 13:27, Krzysztof Kozlowski wrote: > On Wed, Nov 04, 2020 at 11:36:53AM +0100, Sylwester Nawrocki wrote: >> Add maintainers entry for the Samsung interconnect drivers, this currently >> includes Exynos SoC generic interconnect driver. >> >> Signed-off-by: Sylwester Nawrocki >> diff --git a/MAINTAINERS b/MAINTAINERS >> index e73636b..4bbafef 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -9156,6 +9156,13 @@ F:include/dt-bindings/interconnect/ >> F: include/linux/interconnect-provider.h >> F: include/linux/interconnect.h >> >> +SAMSUNG INTERCONNECT DRIVERS > > Does not look like ordered alphabetically. >> +M: Sylwester Nawrocki >> +M: Artur Swigoń >> +L: linux...@vger.kernel.org > > Also: > L: linux-samsung-...@vger.kernel.org >> +S: Supported >> +F: drivers/interconnect/samsung > > Add trailing /. Fixed all issues, thanks for your review! -- Regards, Sylwester
Re: [PATCH v8 2/7] interconnect: Add generic interconnect driver for Exynos SoCs
On 04.11.2020 13:37, Krzysztof Kozlowski wrote: > On Wed, Nov 04, 2020 at 11:36:52AM +0100, Sylwester Nawrocki wrote: >> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile >> index d203520..c2f9e9d 100644 >> --- a/drivers/interconnect/Makefile >> +++ b/drivers/interconnect/Makefile >> @@ -6,3 +6,4 @@ icc-core-objs:= core.o bulk.o >> obj-$(CONFIG_INTERCONNECT) += icc-core.o >> obj-$(CONFIG_INTERCONNECT_IMX) += imx/ >> obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/ >> +obj-$(CONFIG_INTERCONNECT_SAMSUNG) += samsung/ >> \ No newline at end of file > > This needs a fix. Corrected, thanks for pointing out. >> diff --git a/drivers/interconnect/samsung/Kconfig >> b/drivers/interconnect/samsung/Kconfig >> new file mode 100644 >> index 000..508ed64 >> --- /dev/null >> +++ b/drivers/interconnect/samsung/Kconfig >> @@ -0,0 +1,13 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +config INTERCONNECT_SAMSUNG >> +bool "Samsung interconnect drivers" > > "Samsung SoC interconnect drivers" Changed. >> +depends on ARCH_EXYNOS || COMPILE_TEST > > Don't the depend on INTERCONNECT? This file gets included only if INTERCONNECT is enabled, see higher level Kconfig file. >> +help >> + Interconnect drivers for Samsung SoCs. >> + >> + > > One line break Fixed. >> +config INTERCONNECT_EXYNOS >> +tristate "Exynos generic interconnect driver" >> +depends on INTERCONNECT_SAMSUNG > > How about: > default y if ARCH_EXYNOS OK, added. >> +help >> + Generic interconnect driver for Exynos SoCs. >> diff --git a/drivers/interconnect/samsung/Makefile >> b/drivers/interconnect/samsung/Makefile >> new file mode 100644 >> index 000..e19d1df >> --- /dev/null >> +++ b/drivers/interconnect/samsung/Makefile >> @@ -0,0 +1,4 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +exynos-interconnect-objs:= exynos.o > > What is this line for? That allows to change the module name, so it's exynos-interconnect.ko rather than just exynos.c. It's done similarly for other SoCs in the subsystem. >> +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos-interconnect.o -- Regards, Sylwester
[PATCH v8 3/7] MAINTAINERS: Add entry for Samsung interconnect drivers
Add maintainers entry for the Samsung interconnect drivers, this currently includes Exynos SoC generic interconnect driver. Signed-off-by: Sylwester Nawrocki --- Changes since v7: - new patch. --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index e73636b..4bbafef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9156,6 +9156,13 @@ F: include/dt-bindings/interconnect/ F: include/linux/interconnect-provider.h F: include/linux/interconnect.h +SAMSUNG INTERCONNECT DRIVERS +M: Sylwester Nawrocki +M: Artur Swigoń +L: linux...@vger.kernel.org +S: Supported +F: drivers/interconnect/samsung + INVENSENSE ICM-426xx IMU DRIVER M: Jean-Baptiste Maneyrol L: linux-...@vger.kernel.org -- 2.7.4
[PATCH v8 7/7] drm: exynos: mixer: Add interconnect support
This patch adds interconnect support to exynos-mixer. The mixer works
the same as before when CONFIG_INTERCONNECT is 'n'.
For proper operation of the video mixer block we need to ensure the
interconnect busses like DMC or LEFTBUS provide enough bandwidth so
as to avoid DMA buffer underruns in the mixer block. I.e we need to
prevent those busses from operating in low perfomance OPPs when
the mixer is running.
In this patch the bus bandwidth request is done through the interconnect
API, the bandwidth value is calculated from selected DRM mode, i.e.
video plane width, height, refresh rate and pixel format.
The bandwidth setting is synchronized with VSYNC when we are switching
to lower bandwidth. This is required to ensure enough bandwidth for
the device since new settings are normally being applied in the hardware
synchronously with VSYNC.
Acked-by: Krzysztof Kozlowski
Co-developed-by: Artur Świgoń
Signed-off-by: Marek Szyprowski
Signed-off-by: Sylwester Nawrocki
---
Changes for v8:
- updated comment in mixer_probe()
Changes for v7:
- fixed incorrect setting of the ICC bandwidth when the mixer is
disabled, now the bandwidth is set explicitly to 0 in such case.
Changes for v6:
- the icc_set_bw() call is now only done when calculated value for
a crtc changes, this avoids unnecessary calls per each video frame
- added synchronization of the interconnect bandwidth setting with
the mixer VSYNC in order to avoid buffer underflow when we lower
the interconnect bandwidth when the hardware still operates with
previous mode settings that require higher bandwidth. This fixed
IOMMU faults observed e.g. during switching from two planes to
a single plane operation.
Changes for v5:
- renamed soc_path variable to icc_path
---
drivers/gpu/drm/exynos/exynos_mixer.c | 146 --
1 file changed, 138 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
b/drivers/gpu/drm/exynos/exynos_mixer.c
index af192e5..8c1509e 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -73,6 +74,7 @@ enum mixer_flag_bits {
MXR_BIT_INTERLACE,
MXR_BIT_VP_ENABLED,
MXR_BIT_HAS_SCLK,
+ MXR_BIT_REQUEST_BW,
};
static const uint32_t mixer_formats[] = {
@@ -99,6 +101,13 @@ struct mixer_context {
struct exynos_drm_plane planes[MIXER_WIN_NR];
unsigned long flags;
+ struct icc_path *icc_path;
+ /* memory bandwidth on the interconnect bus in B/s */
+ unsigned long icc_bandwidth;
+ /* mutex protecting @icc_bandwidth */
+ struct mutexicc_lock;
+ struct work_struct work;
+
int irq;
void __iomem*mixer_regs;
void __iomem*vp_regs;
@@ -754,6 +763,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
val |= MXR_INT_CLEAR_VSYNC;
val &= ~MXR_INT_STATUS_VSYNC;
+ if (test_and_clear_bit(MXR_BIT_REQUEST_BW, >flags))
+ schedule_work(>work);
+
/* interlace scan need to check shadow register */
if (test_bit(MXR_BIT_INTERLACE, >flags)
&& !mixer_is_synced(ctx))
@@ -934,6 +946,76 @@ static void mixer_disable_vblank(struct exynos_drm_crtc
*crtc)
mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
}
+/**
+ * mixer_get_memory_bandwidth - calculate memory bandwidth for current crtc
mode
+ * @crtc: a crtc with DRM mode to calculate the bandwidth for
+ *
+ * Return: memory bandwidth in B/s
+ *
+ * This function returns memory bandwidth calculated as a sum of amount of data
+ * per second for each plane. The calculation is based on maximum possible
pixel
+ * resolution for a plane so as to avoid different bandwidth request per each
+ * video frame. The formula used for calculation for each plane is:
+ *
+ * bw = width * height * frame_rate / interlace / (hor_subs * vert_subs)
+ *
+ * where:
+ * - width, height - (DRM mode) video frame width and height in pixels,
+ * - frame_rate - DRM mode frame refresh rate,
+ * - interlace: 1 - in case of progressive and 2 in case of interlaced video,
+ * - hor_subs, vert_subs - accordingly horizontal and vertical pixel
+ *subsampling for a plane.
+ */
+static unsigned int mixer_get_memory_bandwidth(struct exynos_drm_crtc *crtc)
+{
+ struct drm_display_mode *mode = >base.state->adjusted_mode;
+ struct mixer_context *ctx = crtc->ctx;
+ unsigned long bw, bandwidth = 0;
+ int i, j, sub;
+
+ for (i = 0; i < MIXER_WIN_NR; i++) {
+ struct drm_plane *plane = >planes[i].base;
+ const struct drm_format_info *format;
+
+ if (plane->state &a
[PATCH v8 5/7] ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
This patch adds the following properties for Exynos4412 interconnect bus nodes: - interconnects: to declare connections between nodes in order to guarantee PM QoS requirements between nodes, - #interconnect-cells: required by the interconnect framework, - samsung,data-clk-ratio: which allows to specify minimum data clock frequency corresponding to requested bandwidth for each bus. Note that #interconnect-cells is always zero and node IDs are not hardcoded anywhere. Signed-off-by: Artur Świgoń Signed-off-by: Sylwester Nawrocki --- Changes for v8: - none. Changes for v7: - adjusted to the DT property changes: "interconnects" instead of "samsung,interconnect-parent", "samsung,data-clk-ratio" instead of "bus-width". Changes for v6: - added bus-width property in bus_dmc node. Changes for v5: - adjust to renamed exynos,interconnect-parent-node property, - add properties in common exynos4412.dtsi file rather than in Odroid specific odroid4412-odroid-common.dtsi. --- arch/arm/boot/dts/exynos4412.dtsi | 6 ++ 1 file changed, 6 insertions(+) diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi index e76881d..4999e68 100644 --- a/arch/arm/boot/dts/exynos4412.dtsi +++ b/arch/arm/boot/dts/exynos4412.dtsi @@ -383,6 +383,8 @@ clocks = < CLK_DIV_DMC>; clock-names = "bus"; operating-points-v2 = <_dmc_opp_table>; + samsung,data-clock-ratio = <4>; + #interconnect-cells = <0>; status = "disabled"; }; @@ -452,6 +454,8 @@ clocks = < CLK_DIV_GDL>; clock-names = "bus"; operating-points-v2 = <_leftbus_opp_table>; + interconnects = <_dmc>; + #interconnect-cells = <0>; status = "disabled"; }; @@ -468,6 +472,8 @@ clocks = < CLK_ACLK160>; clock-names = "bus"; operating-points-v2 = <_display_opp_table>; + interconnects = <_leftbus _dmc>; + #interconnect-cells = <0>; status = "disabled"; }; -- 2.7.4
[PATCH v8 1/7] dt-bindings: devfreq: Add documentation for the interconnect properties
Add documentation for new optional properties in the exynos bus nodes:
interconnects, #interconnect-cells, samsung,data-clock-ratio.
These properties allow to specify the SoC interconnect structure which
then allows the interconnect consumer devices to request specific
bandwidth requirements.
Signed-off-by: Artur Świgoń
Signed-off-by: Sylwester Nawrocki
---
Changes for v8:
- updated description of the interconnects property,
- fixed typo in samsung,data-clk-ratio property description.
Changes for v7:
- bus-width property replaced with samsung,data-clock-ratio,
- the interconnect consumer bindings used instead of vendor specific
properties
Changes for v6:
- added dts example of bus hierarchy definition and the interconnect
consumer,
- added new bus-width property.
Changes for v5:
- exynos,interconnect-parent-node renamed to samsung,interconnect-parent
---
.../devicetree/bindings/devfreq/exynos-bus.txt | 71 +-
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index e71f752..bcaa2c0 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -51,6 +51,19 @@ Optional properties only for parent bus device:
- exynos,saturation-ratio: the percentage value which is used to calibrate
the performance count against total cycle count.
+Optional properties for the interconnect functionality (QoS frequency
+constraints):
+- #interconnect-cells: should be 0.
+- interconnects: as documented in ../interconnect.txt, describes a path at the
+ higher level interconnects used by this interconnect provider.
+ If this interconnect provider is directly linked to a top level interconnect
+ provider the property contains only one phandle. The provider extends
+ the interconnect graph by linking its node to a node registered by provider
+ pointed to by first phandle in the 'interconnects' property.
+
+- samsung,data-clock-ratio: ratio of the data throughput in B/s to minimum data
+ clock frequency in Hz, default value is 8 when this property is missing.
+
Detailed correlation between sub-blocks and power line according to Exynos SoC:
- In case of Exynos3250, there are two power line as following:
VDD_MIF |--- DMC
@@ -135,7 +148,7 @@ Detailed correlation between sub-blocks and power line
according to Exynos SoC:
|--- PERIC (Fixed clock rate)
|--- FSYS (Fixed clock rate)
-Example1:
+Example 1:
Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
power line (regulator). The MIF (Memory Interface) AXI bus is used to
transfer data between DRAM and CPU and uses the VDD_MIF regulator.
@@ -184,7 +197,7 @@ Example1:
|L5 |20 |20 |40 |30 | ||100 |
--
-Example2 :
+Example 2:
The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
is listed below:
@@ -419,3 +432,57 @@ Example2 :
devfreq = <_leftbus>;
status = "okay";
};
+
+Example 3:
+ An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
+ Exynos4412 SoC with video mixer as an interconnect consumer device.
+
+ soc {
+ bus_dmc: bus_dmc {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_DIV_DMC>;
+ clock-names = "bus";
+ operating-points-v2 = <_dmc_opp_table>;
+ samsung,data-clock-ratio = <4>;
+ #interconnect-cells = <0>;
+ };
+
+ bus_leftbus: bus_leftbus {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_DIV_GDL>;
+ clock-names = "bus";
+ operating-points-v2 = <_leftbus_opp_table>;
+ #interconnect-cells = <0>;
+ interconnects = <_dmc>;
+ };
+
+ bus_display: bus_display {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_ACLK160>;
+ clock-names = "bus";
+ operating-points-v2 = <_display_opp_table>;
+ #interconnect-cells = <0>;
+ interconnects = <_leftbus _dmc>;
+ };
+
+ bus_dmc_opp_table: opp_table1 {
+ compatible = "operating-points-v2";
+ /* ... */
+ }
+
+ bus_leftbus_opp_table: opp_table
[PATCH v8 6/7] ARM: dts: exynos: Add interconnects to Exynos4412 mixer
From: Artur Świgoń
This patch adds an 'interconnects' property to Exynos4412 DTS in order to
declare the interconnect path used by the mixer. Please note that the
'interconnect-names' property is not needed when there is only one path in
'interconnects', in which case calling of_icc_get() with a NULL name simply
returns the right path.
Reviewed-by: Chanwoo Choi
Signed-off-by: Artur Świgoń
Signed-off-by: Sylwester Nawrocki
---
Changes for v8...v5:
- none.
---
arch/arm/boot/dts/exynos4412.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/exynos4412.dtsi
b/arch/arm/boot/dts/exynos4412.dtsi
index 4999e68..d07739e 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -779,6 +779,7 @@
clock-names = "mixer", "hdmi", "sclk_hdmi", "vp";
clocks = < CLK_MIXER>, < CLK_HDMI>,
< CLK_SCLK_HDMI>, < CLK_VP>;
+ interconnects = <_display _dmc>;
};
{
--
2.7.4
[PATCH v8 0/7] Exynos: Simple QoS for exynos-bus using interconnect
This patchset adds interconnect API support for the Exynos SoC "samsung, exynos-bus" compatible devices, which already have their corresponding exynos-bus driver in the devfreq subsystem. Complementing the devfreq driver with an interconnect functionality allows to ensure the QoS requirements of devices accessing the system memory (e.g. video processing devices) are fulfilled and aallows to avoid issues like the one discussed in thread [1]. This patch series adds implementation of the interconnect provider per each "samsung,exynos-bus" compatible DT node, with one interconnect node per provider. The interconnect code which was previously added as a part of the devfreq driver has been converted to a separate platform driver. In the devfreq a corresponding virtual child platform device is registered. Integration of devfreq and interconnect frameworks is achieved through the PM QoS API. A sample interconnect consumer for exynos-mixer is added in patches 6/7, 7/7, it is currently added only for exynos4412 and allows to address the mixer DMA underrun error issues [1]. Changes since v7: - drivers/interconnect/exynos renamed to drivers/interconnect/samsung, - added INTERCONNECT_SAMSUNG Kconfig symbol, - added missing driver sync_state callback, - improved the DT binding description, - added a patch adding maintainers entry, - updated comment in patch 7/7, typo fix (patch 1/7). The series has been tested on Odroid U3 board. It is based on v5.10-rc1. -- Regards, Sylwester Changes since v6: - the interconnect consumer DT bindings are now used to describe dependencies of the interconnects (samsung,exynos-bus nodes), - bus-width property replaced with samsung,data-clk-ratio, - adaptation to recent changes in the interconnect code (of_icc_get_from_provider(), icc_node_add()). Changes since v5: - addition of "bus-width: DT property, which specifies data width of the interconnect bus (patches 1...2/6), - addition of synchronization of the interconnect bandwidth setting with VSYNC (patch 6/6). Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed changes are listed in patches: - conversion to a separate interconnect (platform) driver, - an update of the DT binding documenting new optional properties: #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus" nodes, - new DT properties added to the SoC, rather than to the board specific files. Changes since v2 [5]: - Use icc_std_aggregate(). - Implement a different modification of apply_constraints() in drivers/interconnect/core.c (patch 03). - Use 'exynos,interconnect-parent-node' in the DT instead of 'devfreq'/'parent', depending on the bus. - Rebase on DT patches that deprecate the 'devfreq' DT property. - Improve error handling, including freeing generated IDs on failure. - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent(). Changes since v1 [6]: - Rebase on coupled regulators patches. - Use dev_pm_qos_*() API instead of overriding frequency in exynos_bus_target(). - Use IDR for node ID allocation. - Reverse order of multiplication and division in mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow. References: [1] https://patchwork.kernel.org/patch/10861757/ (original issue) [2] https://www.spinics.net/lists/linux-samsung-soc/msg70014.html [3] https://www.spinics.net/lists/arm-kernel/msg810722.html [4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swi...@samsung.com [5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC) [6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC) Artur Świgoń (1): ARM: dts: exynos: Add interconnects to Exynos4412 mixer Sylwester Nawrocki (6): dt-bindings: devfreq: Add documentation for the interconnect properties interconnect: Add generic interconnect driver for Exynos SoCs MAINTAINERS: Add entry for Samsung interconnect drivers PM / devfreq: exynos-bus: Add registration of interconnect child device ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes drm: exynos: mixer: Add interconnect support .../devicetree/bindings/devfreq/exynos-bus.txt | 71 +++- MAINTAINERS| 7 + arch/arm/boot/dts/exynos4412.dtsi | 7 + drivers/devfreq/exynos-bus.c | 17 ++ drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++- drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile | 1 + drivers/interconnect/samsung/Kconfig | 13 ++ drivers/interconnect/samsung/Makefile | 4 + drivers/interconnect/samsung/exynos.c | 199 + 10 files changed, 456 insertions(+), 10 deletions(-) create mode 100644 drivers/interconnect/samsung/Kconfig create mode 100644 drivers/interconnect/samsung/Makefile crea
[PATCH v8 2/7] interconnect: Add generic interconnect driver for Exynos SoCs
This patch adds a generic interconnect driver for Exynos SoCs in order to provide interconnect functionality for each "samsung,exynos-bus" compatible device. The SoC topology is a graph (or more specifically, a tree) and its edges are described by specifying in the 'interconnects' property the interconnect consumer path for each interconnect provider DT node. Each bus is now an interconnect provider and an interconnect node as well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers itself as a node. Node IDs are not hard coded but rather assigned dynamically at runtime. This approach allows for using this driver with various Exynos SoCs. Frequencies requested via the interconnect API for a given node are propagated to devfreq using dev_pm_qos_update_request(). Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in which case all interconnect API functions are no-op. The samsung,data-clk-ratio DT property is used to specify the ratio of the interconect bandwidth to the minimum data clock frequency for each bus. Due to unspecified relative probing order, -EPROBE_DEFER may be propagated to ensure that the parent is probed before its children. Signed-off-by: Artur Świgoń Signed-off-by: Sylwester Nawrocki --- Changes for v8: - renamed drivers/interconnect/exynos to drivers/interconnect/samsung, - added missing driver sync_state callback assignment. Changes for v7: - adjusted to the DT property changes: "interconnects" instead of "samsung,interconnect-parent", "samsung,data-clk-ratio" instead of "bus-width", - adaptation to of_icc_get_from_provider() function changes in v5.10-rc1. Changes for v6: - corrected of_node dereferencing in exynos_icc_get_parent() function, - corrected initialization of icc_node->name so as to avoid direct of_node->name dereferencing, - added parsing of bus-width DT property. Changes for v5: - adjust to renamed exynos,interconnect-parent-node property, - use automatically generated platform device id as the interconect node id instead of a now unavailable devfreq->id field, - add icc_ prefix to some variables to make the code more self-commenting, - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(), - adjust to exynos,interconnect-parent-node property rename to samsung,interconnect-parent, - converted to a separate platform driver in drivers/interconnect. --- drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile | 1 + drivers/interconnect/samsung/Kconfig | 13 +++ drivers/interconnect/samsung/Makefile | 4 + drivers/interconnect/samsung/exynos.c | 199 ++ 5 files changed, 218 insertions(+) create mode 100644 drivers/interconnect/samsung/Kconfig create mode 100644 drivers/interconnect/samsung/Makefile create mode 100644 drivers/interconnect/samsung/exynos.c diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig index 5b7204e..d637a89 100644 --- a/drivers/interconnect/Kconfig +++ b/drivers/interconnect/Kconfig @@ -13,5 +13,6 @@ if INTERCONNECT source "drivers/interconnect/imx/Kconfig" source "drivers/interconnect/qcom/Kconfig" +source "drivers/interconnect/samsung/Kconfig" endif diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile index d203520..c2f9e9d 100644 --- a/drivers/interconnect/Makefile +++ b/drivers/interconnect/Makefile @@ -6,3 +6,4 @@ icc-core-objs := core.o bulk.o obj-$(CONFIG_INTERCONNECT) += icc-core.o obj-$(CONFIG_INTERCONNECT_IMX) += imx/ obj-$(CONFIG_INTERCONNECT_QCOM)+= qcom/ +obj-$(CONFIG_INTERCONNECT_SAMSUNG) += samsung/ \ No newline at end of file diff --git a/drivers/interconnect/samsung/Kconfig b/drivers/interconnect/samsung/Kconfig new file mode 100644 index 000..508ed64 --- /dev/null +++ b/drivers/interconnect/samsung/Kconfig @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: GPL-2.0-only +config INTERCONNECT_SAMSUNG + bool "Samsung interconnect drivers" + depends on ARCH_EXYNOS || COMPILE_TEST + help + Interconnect drivers for Samsung SoCs. + + +config INTERCONNECT_EXYNOS + tristate "Exynos generic interconnect driver" + depends on INTERCONNECT_SAMSUNG + help + Generic interconnect driver for Exynos SoCs. diff --git a/drivers/interconnect/samsung/Makefile b/drivers/interconnect/samsung/Makefile new file mode 100644 index 000..e19d1df --- /dev/null +++ b/drivers/interconnect/samsung/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +exynos-interconnect-objs := exynos.o + +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos-interconnect.o diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c new file mode 100644 index 000..6559d8c --- /dev
[PATCH v8 4/7] PM / devfreq: exynos-bus: Add registration of interconnect child device
This patch adds registration of a child platform device for the exynos
interconnect driver. It is assumed that the interconnect provider will
only be needed when #interconnect-cells property is present in the bus
DT node, hence the child device will be created only when such a property
is present.
Acked-by: Krzysztof Kozlowski
Acked-by: Chanwoo Choi
Signed-off-by: Sylwester Nawrocki
---
Changes for v8, v7, v6:
- none.
Changes for v5:
- new patch.
---
drivers/devfreq/exynos-bus.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 1e684a4..ee300ee 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -24,6 +24,7 @@
struct exynos_bus {
struct device *dev;
+ struct platform_device *icc_pdev;
struct devfreq *devfreq;
struct devfreq_event_dev **edev;
@@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev)
if (ret < 0)
dev_warn(dev, "failed to disable the devfreq-event devices\n");
+ platform_device_unregister(bus->icc_pdev);
+
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
if (bus->opp_table) {
@@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev)
{
struct exynos_bus *bus = dev_get_drvdata(dev);
+ platform_device_unregister(bus->icc_pdev);
+
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
}
@@ -432,6 +437,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
+ /* Create child platform device for the interconnect provider */
+ if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
+ bus->icc_pdev = platform_device_register_data(
+ dev, "exynos-generic-icc",
+ PLATFORM_DEVID_AUTO, NULL, 0);
+
+ if (IS_ERR(bus->icc_pdev)) {
+ ret = PTR_ERR(bus->icc_pdev);
+ goto err;
+ }
+ }
+
max_state = bus->devfreq->profile->max_state;
min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
--
2.7.4
Re: [PATCH v7 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
On 03.11.2020 15:12, Chanwoo Choi wrote: >>> I have a question about exynos_icc_get_parent(). >>> As I checked, this function returns the only one icc_node >>> as parent node. But, bus_display dt node in the exynos4412.dtsi >>> specifies the two interconnect node as following with bus_leftbus, bus_dmc, >>> >>> When I checked the return value of exynos_icc_get_parent() >>> during probing for bus_display device, exynos_icc_get_parent() function >>> only returns 'bus_leftbus' icc_node. Do you need to add two phandle >>> of icc node? >> Yes, as we use the interconnect consumer bindings we need to specify a path, >> i.e. a pair. When the provider node initializes it will >> link itself to that path. Currently the provider driver uses just the first >> phandle. > As I knew, the interconnect consumer bindings use the two phandles > in the interconnect core as you commented. But, in case of this, > even if add two phandles with interconnect consuming binding style, > the exynos interconnect driver only uses the first phandle. > > Instead, I think we better explain this case into a dt-binding > document for users. Fair enough, I'll try to improve the description, do you perhaps have any suggestions? The DT binding reflects how the hardware structure looks like and the fact that the driver currently uses only one of the phandles could be considered an implementation detail. -- Regards, Sylwester
Re: [PATCH v7 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device
Hi Chanwoo, On 03.11.2020 11:45, Chanwoo Choi wrote: > On 10/30/20 9:51 PM, Sylwester Nawrocki wrote: >> This patch adds registration of a child platform device for the exynos >> interconnect driver. It is assumed that the interconnect provider will >> only be needed when #interconnect-cells property is present in the bus >> DT node, hence the child device will be created only when such a property >> is present. >> >> Signed-off-by: Sylwester Nawrocki >> drivers/devfreq/exynos-bus.c | 17 + > We don't need to add 'select INTERCONNECT_EXYNOS' in Kconfig? I think by doing so we could run into some dependency issues. > Do you want to remain it as optional according to user? Yes, I would prefer to keep it selectable through defconfig. Currently it's only needed by one 32-bit ARM board. -- Regards, Sylwester
Re: [PATCH v7 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
On 03.11.2020 10:37, Chanwoo Choi wrote:
> On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' in the
>
> samsung,interconnect-parent -> interconnects?
Yes, I will rephrase the whole commit message as it's a bit outdated now.
I've changed the sentence to:
"The SoC topology is a graph (or more specifically, a tree) and its
edges are described by specifying in the 'interconnects' property
the interconnect consumer path for each interconnect provider DT node."
>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>> propagated to ensure that the parent is probed before its children.
>>
>> Each bus is now an interconnect provider and an interconnect node as
>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>> registers itself as a node. Node IDs are not hardcoded but rather
>> assigned dynamically at runtime. This approach allows for using this
>> driver with various Exynos SoCs.
>>
>> Frequencies requested via the interconnect API for a given node are
>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>> case all interconnect API functions are no-op.
>>
>> The bus-width DT property is to determine the interconnect data
>> width and traslate requested bandwidth to clock frequency for each
>> bus.
>>
>> Signed-off-by: Artur Świgoń
>> Signed-off-by: Sylwester Nawrocki
>> +++ b/drivers/interconnect/exynos/exynos.c
>> +struct exynos_icc_priv {
>> +struct device *dev;
>> +
>> +/* One interconnect node per provider */
>> +struct icc_provider provider;
>> +struct icc_node *node;
>> +
>> +struct dev_pm_qos_request qos_req;
>> +u32 bus_clk_ratio;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> +struct of_phandle_args args;
>> +struct icc_node_data *icc_node_data;
>> +struct icc_node *icc_node;
>> +int num, ret;
>> +
>> +num = of_count_phandle_with_args(np, "interconnects",
>> + "#interconnect-cells");
>> +if (num < 1)
>> +return NULL; /* parent nodes are optional */
>> +
>> +/* Get the interconnect target node */
>> +ret = of_parse_phandle_with_args(np, "interconnects",
>> +"#interconnect-cells", 0, );
>> +if (ret < 0)
>> +return ERR_PTR(ret);
>> +
>> +icc_node_data = of_icc_get_from_provider();
>> +of_node_put(args.np);
>> +
>> +if (IS_ERR(icc_node_data))
>> +return ERR_CAST(icc_node_data);
>> +
>> +icc_node = icc_node_data->node;
>> +kfree(icc_node_data);
>> +
>> +return icc_node;
>> +}
>
> I have a question about exynos_icc_get_parent().
> As I checked, this function returns the only one icc_node
> as parent node. But, bus_display dt node in the exynos4412.dtsi
> specifies the two interconnect node as following with bus_leftbus, bus_dmc,
>
> When I checked the return value of exynos_icc_get_parent()
> during probing for bus_display device, exynos_icc_get_parent() function
> only returns 'bus_leftbus' icc_node. Do you need to add two phandle
> of icc node?
Yes, as we use the interconnect consumer bindings we need to specify a path,
i.e. a pair. When the provider node initializes it will
link itself to that path. Currently the provider driver uses just the first
phandle.
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -472,7 +472,7 @@
> clocks = < CLK_ACLK160>;
> clock-names = "bus";
> operating-points-v2 = <_display_opp_table>;
> interconnects = <_leftbus _dmc>;
> #interconnect-cells = <0>;
> status = "disabled";
> };
--
Regards,
Sylwester
Re: [PATCH v7 0/6] Exynos: Simple QoS for exynos-bus using interconnect
Hi Chanwoo, Georgi On 03.11.2020 09:53, Chanwoo Choi wrote: > On 11/3/20 5:29 PM, Georgi Djakov wrote: >> On 11/3/20 09:54, Chanwoo Choi wrote: >>> When I tested this patchset on Odroid-U3, >>> After setting 0 bps by interconnect[1][2], >>> the frequency of devfreq devs sustain the high frequency >>> according to the pm qos request. >>> >>> So, I try to find the cause of this situation. >>> In result, it seems that interconnect exynos driver >>> updates the pm qos request to devfreq device >>> during the kernel booting. Do you know why the exynos >>> interconnect driver request the pm qos during probe >>> without the mixer request? >> >> That's probably because of the sync_state support, that was introduced >> recently. The icc_sync_state callback needs to be added to the driver >> (i just left a comment on that patch), and then check again if it works. >> >> The idea of the sync_state is that there could be multiple users of a >> path and we must wait for all consumers to tell their bandwidth needs. >> Otherwise the first consumer may lower the bandwidth or disable a path >> needed for another consumer (driver), which has not probed yet. So we >> maintain a floor bandwidth until everyone has probed. By default the floor >> bandwidth is INT_MAX, but can be overridden by implementing the get_bw() >> callback. Thanks for detailed explanation Georgi. > Thanks for guide. I tested it with your comment of patch2. > It is well working without problem as I mentioned previously. > > I caught the reset operation of PM QoS requested from interconnect > on kernel log. In result, after completed the kernel booting, > there is no pm qos request if hdmi cable is not connected. Thanks for the bug report Chanwoo, it's related to the sync_state feature as you guys already figured out. I had to reorder some code in the interconnect driver probe() to avoid some issues, i.e. to register PM QoS request before icc_node_add() call but I forgot to check initial state of the bus frequencies. I thought the get_bw implementation might be needed but the default behaviour seems fine, the PM QoS derived bus frequencies will be clamped in the devfreq to valid OPP values. Chanwoo, in order to set the bandwidth to 0 we could also just blank the display. Below are some of the commands I use for testing. # blank display (disable the mixer entirely) echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # unblank display echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank # modetest with 2 planes (higher bandwidth test) ./modetest -s 47:1920x1080 -P 45:1920x1080 -v -- Regards, Sylwester
Re: [PATCH v7 6/6] drm: exynos: mixer: Add interconnect support
On 31.10.2020 13:47, Krzysztof Kozlowski wrote: >> @@ -1223,19 +1330,33 @@ static int mixer_probe(struct platform_device *pdev) >> struct device *dev = >dev; >> const struct mixer_drv_data *drv; >> struct mixer_context *ctx; >> +struct icc_path *path; >> int ret; >> >> +/* >> + * Returns NULL if CONFIG_INTERCONNECT is disabled. > You could add here: > or if "interconnects" property does not exist. Right, thanks for pointing out, I will update that comment. -- Regards, Sylwester
Re: [PATCH v7 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
On 31.10.2020 13:17, Krzysztof Kozlowski wrote: > On Fri, Oct 30, 2020 at 01:51:45PM +0100, Sylwester Nawrocki wrote: >> This patch adds a generic interconnect driver for Exynos SoCs in order >> to provide interconnect functionality for each "samsung,exynos-bus" >> compatible device. >> Signed-off-by: Artur Świgoń >> Signed-off-by: Sylwester Nawrocki >> drivers/interconnect/Kconfig | 1 + >> drivers/interconnect/Makefile| 1 + >> drivers/interconnect/exynos/Kconfig | 6 ++ >> drivers/interconnect/exynos/Makefile | 4 + >> drivers/interconnect/exynos/exynos.c | 198 >> +++ > > How about naming the directory as "samsung"? I don't expect interconnect > drivers for the old Samsung S3C or S5P platforms, but it would be > consisteny with other names (memory, clk, pinctrl). Sure, I will rename the directory. > How about adding separate maintainers entry for the driver with you and > Artur (if he still works on this)? I'm not sure what's the preference in the subsystem, I'm going to add a patch introducing such a maintainers entry as it might be helpful for reviews/testing. -- Regards, Sylwester
[PATCH v7 5/6] ARM: dts: exynos: Add interconnects to Exynos4412 mixer
From: Artur Świgoń
This patch adds an 'interconnects' property to Exynos4412 DTS in order to
declare the interconnect path used by the mixer. Please note that the
'interconnect-names' property is not needed when there is only one path in
'interconnects', in which case calling of_icc_get() with a NULL name simply
returns the right path.
Signed-off-by: Artur Świgoń
Reviewed-by: Chanwoo Choi
---
Changes for v7, v6, v5:
- none.
---
arch/arm/boot/dts/exynos4412.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/exynos4412.dtsi
b/arch/arm/boot/dts/exynos4412.dtsi
index 4999e68..d07739e 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -779,6 +779,7 @@
clock-names = "mixer", "hdmi", "sclk_hdmi", "vp";
clocks = < CLK_MIXER>, < CLK_HDMI>,
< CLK_SCLK_HDMI>, < CLK_VP>;
+ interconnects = <_display _dmc>;
};
{
--
2.7.4
[PATCH v7 4/6] ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
This patch adds the following properties for Exynos4412 interconnect bus nodes: - interconnects: to declare connections between nodes in order to guarantee PM QoS requirements between nodes, - #interconnect-cells: required by the interconnect framework, - samsung,data-clk-ratio: which allows to specify minimum data clock frequency corresponding to requested bandwidth for each bus. Note that #interconnect-cells is always zero and node IDs are not hardcoded anywhere. Signed-off-by: Artur Świgoń Signed-off-by: Sylwester Nawrocki --- Changes for v7: - adjusted to the DT property changes: "interconnects" instead of "samsung,interconnect-parent", "samsung,data-clk-ratio" instead of "bus-width". Changes for v6: - added bus-width property in bus_dmc node. Changes for v5: - adjust to renamed exynos,interconnect-parent-node property, - add properties in common exynos4412.dtsi file rather than in Odroid specific odroid4412-odroid-common.dtsi. --- arch/arm/boot/dts/exynos4412.dtsi | 6 ++ 1 file changed, 6 insertions(+) diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi index e76881d..4999e68 100644 --- a/arch/arm/boot/dts/exynos4412.dtsi +++ b/arch/arm/boot/dts/exynos4412.dtsi @@ -383,6 +383,8 @@ clocks = < CLK_DIV_DMC>; clock-names = "bus"; operating-points-v2 = <_dmc_opp_table>; + samsung,data-clock-ratio = <4>; + #interconnect-cells = <0>; status = "disabled"; }; @@ -452,6 +454,8 @@ clocks = < CLK_DIV_GDL>; clock-names = "bus"; operating-points-v2 = <_leftbus_opp_table>; + interconnects = <_dmc>; + #interconnect-cells = <0>; status = "disabled"; }; @@ -468,6 +472,8 @@ clocks = < CLK_ACLK160>; clock-names = "bus"; operating-points-v2 = <_display_opp_table>; + interconnects = <_leftbus _dmc>; + #interconnect-cells = <0>; status = "disabled"; }; -- 2.7.4
[PATCH v7 6/6] drm: exynos: mixer: Add interconnect support
This patch adds interconnect support to exynos-mixer. The mixer works
the same as before when CONFIG_INTERCONNECT is 'n'.
For proper operation of the video mixer block we need to ensure the
interconnect busses like DMC or LEFTBUS provide enough bandwidth so
as to avoid DMA buffer underruns in the mixer block. I.e we need to
prevent those busses from operating in low perfomance OPPs when
the mixer is running.
In this patch the bus bandwidth request is done through the interconnect
API, the bandwidth value is calculated from selected DRM mode, i.e.
video plane width, height, refresh rate and pixel format.
The bandwidth setting is synchronized with VSYNC when we are switching
to lower bandwidth. This is required to ensure enough bandwidth for
the device since new settings are normally being applied in the hardware
synchronously with VSYNC.
Co-developed-by: Artur Świgoń
Signed-off-by: Marek Szyprowski
Signed-off-by: Sylwester Nawrocki
---
Changes for v7:
- fixed incorrect setting of the ICC bandwidth when the mixer is
disabled, now the bandwidth is set explicitly to 0 in such case.
Changes for v6:
- the icc_set_bw() call is now only done when calculated value for
a crtc changes, this avoids unnecessary calls per each video frame
- added synchronization of the interconnect bandwidth setting with
the mixer VSYNC in order to avoid buffer underflow when we lower
the interconnect bandwidth when the hardware still operates with
previous mode settings that require higher bandwidth. This fixed
IOMMU faults observed e.g. during switching from two planes to
a single plane operation.
Changes for v5:
- renamed soc_path variable to icc_path
---
drivers/gpu/drm/exynos/exynos_mixer.c | 146 --
1 file changed, 138 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
b/drivers/gpu/drm/exynos/exynos_mixer.c
index af192e5..61182cf 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -73,6 +74,7 @@ enum mixer_flag_bits {
MXR_BIT_INTERLACE,
MXR_BIT_VP_ENABLED,
MXR_BIT_HAS_SCLK,
+ MXR_BIT_REQUEST_BW,
};
static const uint32_t mixer_formats[] = {
@@ -99,6 +101,13 @@ struct mixer_context {
struct exynos_drm_plane planes[MIXER_WIN_NR];
unsigned long flags;
+ struct icc_path *icc_path;
+ /* memory bandwidth on the interconnect bus in B/s */
+ unsigned long icc_bandwidth;
+ /* mutex protecting @icc_bandwidth */
+ struct mutexicc_lock;
+ struct work_struct work;
+
int irq;
void __iomem*mixer_regs;
void __iomem*vp_regs;
@@ -754,6 +763,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
val |= MXR_INT_CLEAR_VSYNC;
val &= ~MXR_INT_STATUS_VSYNC;
+ if (test_and_clear_bit(MXR_BIT_REQUEST_BW, >flags))
+ schedule_work(>work);
+
/* interlace scan need to check shadow register */
if (test_bit(MXR_BIT_INTERLACE, >flags)
&& !mixer_is_synced(ctx))
@@ -934,6 +946,76 @@ static void mixer_disable_vblank(struct exynos_drm_crtc
*crtc)
mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
}
+/**
+ * mixer_get_memory_bandwidth - calculate memory bandwidth for current crtc
mode
+ * @crtc: a crtc with DRM mode to calculate the bandwidth for
+ *
+ * Return: memory bandwidth in B/s
+ *
+ * This function returns memory bandwidth calculated as a sum of amount of data
+ * per second for each plane. The calculation is based on maximum possible
pixel
+ * resolution for a plane so as to avoid different bandwidth request per each
+ * video frame. The formula used for calculation for each plane is:
+ *
+ * bw = width * height * frame_rate / interlace / (hor_subs * vert_subs)
+ *
+ * where:
+ * - width, height - (DRM mode) video frame width and height in pixels,
+ * - frame_rate - DRM mode frame refresh rate,
+ * - interlace: 1 - in case of progressive and 2 in case of interlaced video,
+ * - hor_subs, vert_subs - accordingly horizontal and vertical pixel
+ *subsampling for a plane.
+ */
+static unsigned int mixer_get_memory_bandwidth(struct exynos_drm_crtc *crtc)
+{
+ struct drm_display_mode *mode = >base.state->adjusted_mode;
+ struct mixer_context *ctx = crtc->ctx;
+ unsigned long bw, bandwidth = 0;
+ int i, j, sub;
+
+ for (i = 0; i < MIXER_WIN_NR; i++) {
+ struct drm_plane *plane = >planes[i].base;
+ const struct drm_format_info *format;
+
+ if (plane->state && plane->state->crtc && plane->state->fb) {
+ format = pl
[PATCH v7 3/6] PM / devfreq: exynos-bus: Add registration of interconnect child device
This patch adds registration of a child platform device for the exynos
interconnect driver. It is assumed that the interconnect provider will
only be needed when #interconnect-cells property is present in the bus
DT node, hence the child device will be created only when such a property
is present.
Signed-off-by: Sylwester Nawrocki
---
Changes for v7, v6:
- none.
Changes for v5:
- new patch.
---
drivers/devfreq/exynos-bus.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 1e684a4..ee300ee 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -24,6 +24,7 @@
struct exynos_bus {
struct device *dev;
+ struct platform_device *icc_pdev;
struct devfreq *devfreq;
struct devfreq_event_dev **edev;
@@ -156,6 +157,8 @@ static void exynos_bus_exit(struct device *dev)
if (ret < 0)
dev_warn(dev, "failed to disable the devfreq-event devices\n");
+ platform_device_unregister(bus->icc_pdev);
+
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
if (bus->opp_table) {
@@ -168,6 +171,8 @@ static void exynos_bus_passive_exit(struct device *dev)
{
struct exynos_bus *bus = dev_get_drvdata(dev);
+ platform_device_unregister(bus->icc_pdev);
+
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
}
@@ -432,6 +437,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
+ /* Create child platform device for the interconnect provider */
+ if (of_get_property(dev->of_node, "#interconnect-cells", NULL)) {
+ bus->icc_pdev = platform_device_register_data(
+ dev, "exynos-generic-icc",
+ PLATFORM_DEVID_AUTO, NULL, 0);
+
+ if (IS_ERR(bus->icc_pdev)) {
+ ret = PTR_ERR(bus->icc_pdev);
+ goto err;
+ }
+ }
+
max_state = bus->devfreq->profile->max_state;
min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
--
2.7.4
[PATCH v7 0/6] Exynos: Simple QoS for exynos-bus using interconnect
This patchset adds interconnect API support for the Exynos SoC "samsung, exynos-bus" compatible devices, which already have their corresponding exynos-bus driver in the devfreq subsystem. Complementing the devfreq driver with an interconnect functionality allows to ensure the QoS requirements of devices accessing the system memory (e.g. video processing devices) are fulfilled and aallows to avoid issues like the one discussed in thread [1]. This patch series adds implementation of the interconnect provider per each "samsung,exynos-bus" compatible DT node, with one interconnect node per provider. The interconnect code which was previously added as a part of the devfreq driver has been converted to a separate platform driver. In the devfreq a corresponding virtual child platform device is registered. Integration of devfreq and interconnect frameworks is achieved through the PM QoS API. A sample interconnect consumer for exynos-mixer is added in patches 5/6, 6/6, it is currently added only for exynos4412 and allows to address the mixer DMA underrun error issues [1]. Changes since v6: - the interconnect consumer DT bindings are now used to describe dependencies of the interconnects (samsung,exynos-bus nodes), - bus-width property replaced with samsung,data-clk-ratio, - adaptation to recent changes in the interconnect code (of_icc_get_from_provider(), icc_node_add()). The series has been tested on Odroid U3 board. It is based on v5.10-rc1. -- Regards, Sylwester Changes since v5: - addition of "bus-width: DT property, which specifies data width of the interconnect bus (patches 1...2/6), - addition of synchronization of the interconnect bandwidth setting with VSYNC (patch 6/6). Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed changes are listed in each patch: - conversion to a separate interconnect (platform) driver, - an update of the DT binding documenting new optional properties: #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus" nodes, - new DT properties added to the SoC, rather than to the board specific files. Changes since v2 [5]: - Use icc_std_aggregate(). - Implement a different modification of apply_constraints() in drivers/interconnect/core.c (patch 03). - Use 'exynos,interconnect-parent-node' in the DT instead of 'devfreq'/'parent', depending on the bus. - Rebase on DT patches that deprecate the 'devfreq' DT property. - Improve error handling, including freeing generated IDs on failure. - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent(). Changes since v1 [6]: - Rebase on coupled regulators patches. - Use dev_pm_qos_*() API instead of overriding frequency in exynos_bus_target(). - Use IDR for node ID allocation. - Reverse order of multiplication and division in mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow. References: [1] https://patchwork.kernel.org/patch/10861757/ (original issue) [2] https://www.spinics.net/lists/linux-samsung-soc/msg70014.html [3] https://www.spinics.net/lists/arm-kernel/msg810722.html [4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swi...@samsung.com [5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC) [6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC) Artur Świgoń (1): ARM: dts: exynos: Add interconnects to Exynos4412 mixer Sylwester Nawrocki (5): dt-bindings: devfreq: Add documentation for the interconnect properties interconnect: Add generic interconnect driver for Exynos SoCs PM / devfreq: exynos-bus: Add registration of interconnect child device ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes drm: exynos: mixer: Add interconnect support .../devicetree/bindings/devfreq/exynos-bus.txt | 68 ++- arch/arm/boot/dts/exynos4412.dtsi | 7 + drivers/devfreq/exynos-bus.c | 17 ++ drivers/gpu/drm/exynos/exynos_mixer.c | 146 ++- drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile | 1 + drivers/interconnect/exynos/Kconfig| 6 + drivers/interconnect/exynos/Makefile | 4 + drivers/interconnect/exynos/exynos.c | 198 + 9 files changed, 438 insertions(+), 10 deletions(-) create mode 100644 drivers/interconnect/exynos/Kconfig create mode 100644 drivers/interconnect/exynos/Makefile create mode 100644 drivers/interconnect/exynos/exynos.c -- 2.7.4
[PATCH v7 1/6] dt-bindings: devfreq: Add documentation for the interconnect properties
Add documentation for new optional properties in the exynos bus nodes:
interconnects, #interconnect-cells, samsung,data-clock-ratio.
These properties allow to specify the SoC interconnect structure which
then allows the interconnect consumer devices to request specific
bandwidth requirements.
Signed-off-by: Artur Świgoń
Signed-off-by: Sylwester Nawrocki
---
Changes for v7:
- bus-width property replaced with samsung,data-clock-ratio,
- the interconnect consumer bindings used instead of vendor specific
properties
Changes for v6:
- added dts example of bus hierarchy definition and the interconnect
consumer,
- added new bus-width property.
Changes for v5:
- exynos,interconnect-parent-node renamed to samsung,interconnect-parent
---
.../devicetree/bindings/devfreq/exynos-bus.txt | 68 +-
1 file changed, 66 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index e71f752..e34175c 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -51,6 +51,16 @@ Optional properties only for parent bus device:
- exynos,saturation-ratio: the percentage value which is used to calibrate
the performance count against total cycle count.
+Optional properties for interconnect functionality (QoS frequency constraints):
+- #interconnect-cells: should be 0.
+- interconnects: as documented in ../interconnect.txt, describes a path
+ at the higher level interconnects used by this interconnect provider.
+ If this interconnect provider is a parent of a top level interconnect
+ provider the property contains only one phandle.
+
+- samsung,data-clock-ratio: ratio of the data troughput in B/s to minimum data
+ clock frequency in Hz, default value is 8 when this property is missing.
+
Detailed correlation between sub-blocks and power line according to Exynos SoC:
- In case of Exynos3250, there are two power line as following:
VDD_MIF |--- DMC
@@ -135,7 +145,7 @@ Detailed correlation between sub-blocks and power line
according to Exynos SoC:
|--- PERIC (Fixed clock rate)
|--- FSYS (Fixed clock rate)
-Example1:
+Example 1:
Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to
power line (regulator). The MIF (Memory Interface) AXI bus is used to
transfer data between DRAM and CPU and uses the VDD_MIF regulator.
@@ -184,7 +194,7 @@ Example1:
|L5 |20 |20 |40 |30 | ||100 |
--
-Example2 :
+Example 2:
The bus of DMC (Dynamic Memory Controller) block in exynos3250.dtsi
is listed below:
@@ -419,3 +429,57 @@ Example2 :
devfreq = <_leftbus>;
status = "okay";
};
+
+Example 3:
+ An interconnect path "bus_display -- bus_leftbus -- bus_dmc" on
+ Exynos4412 SoC with video mixer as an interconnect consumer device.
+
+ soc {
+ bus_dmc: bus_dmc {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_DIV_DMC>;
+ clock-names = "bus";
+ operating-points-v2 = <_dmc_opp_table>;
+ samsung,data-clock-ratio = <4>;
+ #interconnect-cells = <0>;
+ };
+
+ bus_leftbus: bus_leftbus {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_DIV_GDL>;
+ clock-names = "bus";
+ operating-points-v2 = <_leftbus_opp_table>;
+ #interconnect-cells = <0>;
+ interconnects = <_dmc>;
+ };
+
+ bus_display: bus_display {
+ compatible = "samsung,exynos-bus";
+ clocks = < CLK_ACLK160>;
+ clock-names = "bus";
+ operating-points-v2 = <_display_opp_table>;
+ #interconnect-cells = <0>;
+ interconnects = <_leftbus _dmc>;
+ };
+
+ bus_dmc_opp_table: opp_table1 {
+ compatible = "operating-points-v2";
+ /* ... */
+ }
+
+ bus_leftbus_opp_table: opp_table3 {
+ compatible = "operating-points-v2";
+ /* ... */
+ };
+
+ bus_display_opp_table: opp_table4 {
+ compatible = "operating-points-v2";
+
[PATCH v7 2/6] interconnect: Add generic interconnect driver for Exynos SoCs
This patch adds a generic interconnect driver for Exynos SoCs in order to provide interconnect functionality for each "samsung,exynos-bus" compatible device. The SoC topology is a graph (or more specifically, a tree) and its edges are specified using the 'samsung,interconnect-parent' in the DT. Due to unspecified relative probing order, -EPROBE_DEFER may be propagated to ensure that the parent is probed before its children. Each bus is now an interconnect provider and an interconnect node as well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers itself as a node. Node IDs are not hardcoded but rather assigned dynamically at runtime. This approach allows for using this driver with various Exynos SoCs. Frequencies requested via the interconnect API for a given node are propagated to devfreq using dev_pm_qos_update_request(). Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in which case all interconnect API functions are no-op. The bus-width DT property is to determine the interconnect data width and traslate requested bandwidth to clock frequency for each bus. Signed-off-by: Artur Świgoń Signed-off-by: Sylwester Nawrocki --- Changes for v7: - adjusted to the DT property changes: "interconnects" instead of "samsung,interconnect-parent", "samsung,data-clk-ratio" instead of "bus-width", - adaptation to of_icc_get_from_provider() function changes in v5.10-rc1. Changes for v6: - corrected of_node dereferencing in exynos_icc_get_parent() function, - corrected initialization of icc_node->name so as to avoid direct of_node->name dereferencing, - added parsing of bus-width DT property. Changes for v5: - adjust to renamed exynos,interconnect-parent-node property, - use automatically generated platform device id as the interconect node id instead of a now unavailable devfreq->id field, - add icc_ prefix to some variables to make the code more self-commenting, - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(), - adjust to exynos,interconnect-parent-node property rename to samsung,interconnect-parent, - converted to a separate platform driver in drivers/interconnect. --- drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile| 1 + drivers/interconnect/exynos/Kconfig | 6 ++ drivers/interconnect/exynos/Makefile | 4 + drivers/interconnect/exynos/exynos.c | 198 +++ 5 files changed, 210 insertions(+) create mode 100644 drivers/interconnect/exynos/Kconfig create mode 100644 drivers/interconnect/exynos/Makefile create mode 100644 drivers/interconnect/exynos/exynos.c diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig index 5b7204e..eca6eda 100644 --- a/drivers/interconnect/Kconfig +++ b/drivers/interconnect/Kconfig @@ -11,6 +11,7 @@ menuconfig INTERCONNECT if INTERCONNECT +source "drivers/interconnect/exynos/Kconfig" source "drivers/interconnect/imx/Kconfig" source "drivers/interconnect/qcom/Kconfig" diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile index d203520..665538d 100644 --- a/drivers/interconnect/Makefile +++ b/drivers/interconnect/Makefile @@ -4,5 +4,6 @@ CFLAGS_core.o := -I$(src) icc-core-objs := core.o bulk.o obj-$(CONFIG_INTERCONNECT) += icc-core.o +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos/ obj-$(CONFIG_INTERCONNECT_IMX) += imx/ obj-$(CONFIG_INTERCONNECT_QCOM)+= qcom/ diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig new file mode 100644 index 000..e51e52e --- /dev/null +++ b/drivers/interconnect/exynos/Kconfig @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-only +config INTERCONNECT_EXYNOS + tristate "Exynos generic interconnect driver" + depends on ARCH_EXYNOS || COMPILE_TEST + help + Generic interconnect driver for Exynos SoCs. diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile new file mode 100644 index 000..e19d1df --- /dev/null +++ b/drivers/interconnect/exynos/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +exynos-interconnect-objs := exynos.o + +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos-interconnect.o diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c new file mode 100644 index 000..772d1fc --- /dev/null +++ b/drivers/interconnect/exynos/exynos.c @@ -0,0 +1,198 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Exynos generic interconnect provider driver + * + * Copyright (c) 2020 Samsung Electronics Co., Ltd. + * + * Authors: Artur Świgoń + * Sylwester Nawrocki + */ +#include +#include +#include +#include +#include +#include +#include + +#define EXYNOS_ICC_DEF
Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
Hi Georgi,
On 15.09.2020 23:40, Georgi Djakov wrote:
> On 9/9/20 17:47, Sylwester Nawrocki wrote:
>> On 09.09.2020 11:07, Georgi Djakov wrote:
>>> On 8/28/20 17:49, Sylwester Nawrocki wrote:
>>>> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>>>>> On 09.07.2020 23:04, Rob Herring wrote:
>>>>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>>>>> These properties allow to specify the SoC interconnect structure which
>>>>>>> then allows the interconnect consumer devices to request specific
>>>>>>> bandwidth requirements.
>>>>>>> Signed-off-by: Artur Świgoń
>>>>>>> Signed-off-by: Sylwester Nawrocki
>>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>> Actually we could do without any new property if we used existing
>>>> interconnect
>>>> consumers binding to specify linking between the provider nodes. I think
>>>> those
>>>> exynos-bus nodes could well be considered both the interconnect providers
>>>> and consumers. The example would then be something along the lines
>>>> (yes, I know the bus node naming needs to be fixed):
>>>>
>>>>soc {
>>>>bus_dmc: bus_dmc {
>>>>compatible = "samsung,exynos-bus";
>>>>/* ... */
>>>>samsung,data-clock-ratio = <4>;
>>>>#interconnect-cells = <0>;
>>>>};
>>>>
>>>>bus_leftbus: bus_leftbus {
>>>>compatible = "samsung,exynos-bus";
>>>>/* ... */
>>>>interconnects = <_leftbus _dmc>;
>>>>#interconnect-cells = <0>;
>>>>};
>>>>
>>>>bus_display: bus_display {
>>>>compatible = "samsung,exynos-bus";
>>>>/* ... */
>>>>interconnects = <_display _leftbus>;
>>>
>>> Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
>>> interconnects = <_dmc _leftbus>;
>>
>> Might be, but we would need to swap the phandles so
>> order is maintained, i.e. interconnects = <_leftbus _dmc>;
>
> Ok, i see. Thanks for clarifying! Currently the "interconnects" property is
> defined as a pair of initiator and target nodes. You can keep it also as
> interconnects = <_display _dmc>, but you will need to figure out
> during probe that there is another node in the middle and defer. I assume
> that if a provider is also an interconnect consumer, we will link it to
> whatever nodes are specified in the "interconnects" property?
My apologies for the delay.
Yes, the "interconnect" property would be used (only) to determine what
links should be created.
>> My intention here was to describe the 'bus_display -> bus_leftbus' part
>> of data path 'bus_display -> bus_leftbus -> bus_dmc', bus_display is
>> really a consumer of 'bus_leftbus -> bus_dmc' path.
>>
>> I'm not sure if it is allowed to specify only single phandle (and
>> interconnect provider specifier) in the interconnect property, that would
>> be needed for the bus_leftbus node to define bus_dmc as the interconnect
>> destination port. There seems to be such a use case in arch/arm64/boot/
>> dts/allwinner/sun50i-a64.dtsi.
>
> In the general case you have to specify pairs. The "dma-mem" is a reserved
> name for devices that perform DMA through another bus with separate address
> translation rules.
I see, thanks for clarifying.
>>> I can't understand the above example with bus_display being it's own
>>> consumer.
>>> This seems strange to me. Could you please clarify it?
>>
>>> Otherwise the interconnect consumer DT bindings are already well established
>>> and i don't see anything preventing a node to be both consumer and provider.
>>> So this should be okay in general.
>>
>> Thanks, below is an updated example according to your suggestions.
>> Does it look better now?
>>
>> ---8<--
>> s
Re: [PATCH 2/2] clk: samsung: exynos-clkout: convert to module driver
On 10/1/20 18:56, Krzysztof Kozlowski wrote:
The Exynos clkout driver depends on board input clock (typically XXTI or
XUSBXTI), however on Exynos4 boards these clocks were modeled as part of
SoC clocks (Exynos4 clocks driver). Obviously this is not proper, but
correcting it would break DT backward compatibility.
Both drivers - clkout and Exynos4 clocks - register the clock providers
with CLK_OF_DECLARE/OF_DECLARE_1 so their order is fragile (in the
Makefile clkout is behind Exynos4 clock). It will work only if the
Exynos4 clock driver comes up before clkout.
A change in DTS adding input clock reference to Exynos4 clocks input
PLL, see reverted commit eaf2d2f6895d ("ARM: dts: exynos: add input
clock to CMU in Exynos4412 Odroid"), caused probe reorder: the clkout
appeared before Exynos4 clock provider. Since clkout depends on Exynos4
clocks and does not support deferred probe, this did not work and caused
later failure of usb3503 USB hub probe which needs clkout:
[5.007442] usb3503 0-0008: unable to request refclk (-517)
The Exynos clkout driver is not a critical/core clock so there is
actually no problem in instantiating it later, as a regular module.
This removes specific probe ordering and adds support for probe
deferral.
The patch looks good to me, I have tested it on Trats2, where CLKOUT
provides master clock for the audio codec.
Tested-by: Sylwester Nawrocki
With the debug print removed feel free to apply it through your tree.
Reviewed-by: Sylwester Nawrocki
--
Regards,
Sylwester
Re: [PATCH 1/2] soc: samsung: exynos-pmu: instantiate clkout driver as MFD
Hi, On 10/1/20 18:56, Krzysztof Kozlowski wrote: > The Exynos clock output (clkout) driver uses same register address space > (Power Management Unit address space) as Exynos PMU driver and same set > of compatibles. It was modeled as clock provider instantiated with > CLK_OF_DECLARE_DRIVE(). > > This however brings ordering problems and lack of probe deferral, > therefore clkout driver should be converted to a regular module and > instantiated as a child of PMU driver to be able to use existing > compatibles and address space. It might have been cleaner to have the CLKOUT device as a PMU subnode in DT, then device instantiation would be already covered by devm_of_platform_populate(). But it gets a bit complicated to make such a change in a backward compatible way. I have tested both patches on Trats2, where CLKOUT provides master clock for the audio codec. Reviewed-by: Sylwester Nawrocki Tested-by: Sylwester Nawrocki > @@ -128,6 +134,11 @@ static int exynos_pmu_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pmu_context); > > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, exynos_pmu_devs, > +ARRAY_SIZE(exynos_pmu_devs), NULL, 0, NULL); > + if (ret) > + return ret; > + > if (devm_of_platform_populate(dev)) > dev_err(dev, "Error populating children, reboot and poweroff > might not work properly\n");
Re: [PATCH v3] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
On 15.09.2020 13:34, Sylwester Nawrocki wrote: > On 14.08.2020 02:46, Chanwoo Choi wrote: >> On 8/13/20 6:55 PM, Sylwester Nawrocki wrote: >>> In the .set_rate callback for some PLLs there is a loop polling state >>> of the PLL lock bit and it may become an endless loop when something >>> goes wrong with the PLL. For some PLLs there is already code for polling >>> with a timeout but it uses the ktime API, which doesn't work in some >>> conditions when the set_rate op is called, in particular during >>> initialization of the clk provider before the clocksource initialization >>> has completed. Hence the ktime API cannot be used to reliably detect >>> the PLL locking timeout. >>> >>> This patch adds a common helper function for busy waiting on the PLL lock >>> bit with timeout detection. >>> Signed-off-by: Sylwester Nawrocki >>> --- >>> Changes for v3: >>> - use busy-loop with udelay() instead of ktime API >>> Changes for v2: >>> - use common readl_relaxed_poll_timeout_atomic() macro >>> --- >>> drivers/clk/samsung/clk-pll.c | 94 >>> --- >>> 1 file changed, 34 insertions(+), 60 deletions(-) >> Thanks. >> >> Acked-by: Chanwoo Choi > > Patch applied, thank you for your comments. And dropped now as it causes issues on arm64. As reported by Marek it seems udelay() doesn't work before the system timer is initialized.
Re: [PATCH v2 2/2] clk: samsung: exynos5420: Avoid __clk_lookup() calls when enabling clocks
On 11.08.2020 17:12, Sylwester Nawrocki wrote: > This patch adds a clk ID to the mout_sw_aclk_g3d clk definition so related > clk pointer gets cached in the driver's private data and can be used > later instead of a __clk_lookup() call. > > With that we have all clocks used in the clk_prepare_enable() calls in the > clk provider init callback cached in clk_data.hws[] and we can reference > the clk pointers directly rather than using __clk_lookup() with global names. > > Signed-off-by: Sylwester Nawrocki > --- > Changes for v2: > - added missing part of the patch lost during rebase of the previous version Actually that conflict resolution was incorrect and I squashed below patch as a correction. -8< >From 1594bdb8fd1ab85e994d638256d214adff4e9d40 Mon Sep 17 00:00:00 2001 From: Sylwester Nawrocki Date: Thu, 17 Sep 2020 11:42:14 +0200 Subject: [PATCH] clk: samsung: exynos5420: Fix assignment of hws Fix incorrect rebase conflict resolution. Reported-by: Marek Szyprowski Signed-off-by: Sylwester Nawrocki --- drivers/clk/samsung/clk-exynos5420.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c index ba4e0a4..3ccd4ea 100644 --- a/drivers/clk/samsung/clk-exynos5420.c +++ b/drivers/clk/samsung/clk-exynos5420.c @@ -1574,6 +1574,7 @@ static void __init exynos5x_clk_init(struct device_node *np, exynos5x_soc = soc; ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS); + hws = ctx->clk_data.hws; samsung_clk_of_register_fixed_ext(ctx, exynos5x_fixed_rate_ext_clks, ARRAY_SIZE(exynos5x_fixed_rate_ext_clks), @@ -1651,7 +1652,6 @@ static void __init exynos5x_clk_init(struct device_node *np, exynos5x_subcmus); } - hws = ctx->clk_data.hws; /* * Keep top part of G3D clock path enabled permanently to ensure * that the internal busses get their clock regardless of the -- 2.7.4 -8<
Re: [PATCH 1/3] clk: samsung: Add clk ID definitions for the CPU parent clocks
On 26.08.2020 19:15, Sylwester Nawrocki wrote: > Add clock ID definitions for the CPU parent clocks for SoCs > which don't have such definitions yet. This will allow us to > reference the parent clocks directly by cached struct clk_hw > pointers in the clock provider, rather than doing clk lookup > by name. > > Signed-off-by: Sylwester Nawrocki Applied.
Re: [PATCH 2/3] clk: samsung: exynos5420/5250: Add IDs to the CPU parent clk definitions
On 26.08.2020 19:15, Sylwester Nawrocki wrote: > Use non-zero clock IDs in definitions of the CPU parent clocks > for exynos5420, exynos5250 SoCs. This will allow us to reference > the parent clocks directly in the driver by cached struct clk_hw > pointers, rather than doing clk lookup by name. > > Signed-off-by: Sylwester Nawrocki Applied.
Re: [PATCH 3/3] clk: samsung: Use cached clk_hws instead of __clk_lookup() calls
On 26.08.2020 19:15, Sylwester Nawrocki wrote: > For the CPU clock registration two parent clocks are required, these > are now being passed as struct clk_hw pointers, rather than by the > global scope names. That allows us to avoid __clk_lookup() calls > and simplifies a bit the CPU clock registration function. > While at it drop unneeded extern keyword in the function declaration. > > Signed-off-by: Sylwester Nawrocki Applied.
Re: [PATCH v2 1/2] clk: samsung: exynos5420: Add definition of clock ID for mout_sw_aclk_g3d
On 11.08.2020 17:12, Sylwester Nawrocki wrote: > This patch adds ID for the mout_sw_aclk_g3d (SW_CLKMUX_ACLK_G3D) clock, > mostly for internal use in the CMU driver. It will allow to avoid the > __clk_lookup() call when setting up the clock during the clock provider > initialization. > > Signed-off-by: Sylwester Nawrocki Applied both patches.
Re: [PATCH v3] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
On 14.08.2020 02:46, Chanwoo Choi wrote: > On 8/13/20 6:55 PM, Sylwester Nawrocki wrote: >> In the .set_rate callback for some PLLs there is a loop polling state >> of the PLL lock bit and it may become an endless loop when something >> goes wrong with the PLL. For some PLLs there is already code for polling >> with a timeout but it uses the ktime API, which doesn't work in some >> conditions when the set_rate op is called, in particular during >> initialization of the clk provider before the clocksource initialization >> has completed. Hence the ktime API cannot be used to reliably detect >> the PLL locking timeout. >> >> This patch adds a common helper function for busy waiting on the PLL lock >> bit with timeout detection. >> Signed-off-by: Sylwester Nawrocki >> --- >> Changes for v3: >> - use busy-loop with udelay() instead of ktime API >> Changes for v2: >> - use common readl_relaxed_poll_timeout_atomic() macro >> --- >> drivers/clk/samsung/clk-pll.c | 94 >> --- >> 1 file changed, 34 insertions(+), 60 deletions(-) > Thanks. > > Acked-by: Chanwoo Choi Patch applied, thank you for your comments.
Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
Hi Georgi,
On 09.09.2020 11:07, Georgi Djakov wrote:
> On 8/28/20 17:49, Sylwester Nawrocki wrote:
>> On 30.07.2020 14:28, Sylwester Nawrocki wrote:
>>> On 09.07.2020 23:04, Rob Herring wrote:
>>>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>>>> Add documentation for new optional properties in the exynos bus nodes:
>>>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>>>> These properties allow to specify the SoC interconnect structure which
>>>>> then allows the interconnect consumer devices to request specific
>>>>> bandwidth requirements.
>>>>>
>>>>> Signed-off-by: Artur Świgoń
>>>>> Signed-off-by: Sylwester Nawrocki
>>
>>>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>>>> +Optional properties for interconnect functionality (QoS frequency
>>>>> constraints):
>>>>> +- samsung,interconnect-parent: phandle to the parent interconnect node;
>>>>> for
>>>>> + passive devices should point to same node as the exynos,parent-bus
>>>>> property.
>>
>>>> Adding vendor specific properties for a common binding defeats the
>>>> point.
>>
>> Actually we could do without any new property if we used existing
>> interconnect
>> consumers binding to specify linking between the provider nodes. I think
>> those
>> exynos-bus nodes could well be considered both the interconnect providers
>> and consumers. The example would then be something along the lines
>> (yes, I know the bus node naming needs to be fixed):
>>
>> soc {
>> bus_dmc: bus_dmc {
>> compatible = "samsung,exynos-bus";
>> /* ... */
>> samsung,data-clock-ratio = <4>;
>> #interconnect-cells = <0>;
>> };
>>
>> bus_leftbus: bus_leftbus {
>> compatible = "samsung,exynos-bus";
>> /* ... */
>> interconnects = <_leftbus _dmc>;
>> #interconnect-cells = <0>;
>> };
>>
>> bus_display: bus_display {
>> compatible = "samsung,exynos-bus";
>> /* ... */
>> interconnects = <_display _leftbus>;
>
> Hmm, bus_display being a consumer of itself is a bit odd? Did you mean:
> interconnects = <_dmc _leftbus>;
Might be, but we would need to swap the phandles so
order is maintained, i.e. interconnects = <_leftbus _dmc>;
My intention here was to describe the 'bus_display -> bus_leftbus' part
of data path 'bus_display -> bus_leftbus -> bus_dmc', bus_display is
really a consumer of 'bus_leftbus -> bus_dmc' path.
I'm not sure if it is allowed to specify only single phandle (and
interconnect provider specifier) in the interconnect property, that would
be needed for the bus_leftbus node to define bus_dmc as the interconnect
destination port. There seems to be such a use case in arch/arm64/boot/
dts/allwinner/sun50i-a64.dtsi.
>> #interconnect-cells = <0>;
>> };
>>
>>
>> {
>> compatible = "samsung,exynos4212-mixer";
>> interconnects = <_display _dmc>;
>> /* ... */
>> };
>> };
>>
>> What do you think, Georgi, Rob?
>
> I can't understand the above example with bus_display being it's own consumer.
> This seems strange to me. Could you please clarify it?
> Otherwise the interconnect consumer DT bindings are already well established
> and i don't see anything preventing a node to be both consumer and provider.
> So this should be okay in general.
Thanks, below is an updated example according to your suggestions.
Does it look better now?
---8<--
soc {
bus_dmc: bus_dmc {
compatible = "samsung,exynos-bus";
/* ... */
samsung,data-clock-ratio = <4>;
#interconnect-cells = <0>;
};
bus_leftbus: bus_leftbus {
compatible = "samsung,exynos-bus";
/* ... */
interconnects = <_dmc>;
#interconnect-cells = <0>;
};
bus_display: bus_display {
compatible = "samsung,exynos-bus";
/* ... */
interconnects = <_leftbus _dmc>;
#interconnect-cells = <0>;
};
{
compatible = "samsung,exynos4212-mixer";
interconnects = <_display _dmc>;
/* ... */
};
};
---8<--
--
Regards,
Sylwester
Re: [RFT 09/25] ARM: dts: s5pv210: fix number of I2S DAI cells
On 9/8/20 08:53, Krzysztof Kozlowski wrote: On Mon, Sep 07, 2020 at 04:55:26PM -0700, Jonathan Bakker wrote: Sadly, this is causing issues for me. The machine driver is no longer probing correctly on the Galaxy S. The failing call in sound/soc/samsung/aries_wm8994.c is /* Set CPU of_node for BT DAI */ aries_dai[2].cpus->of_node = of_parse_phandle(cpu, "sound-dai", 1); where cpus->of_node is not set properly. Which is definitely weird because it doesn't look like this should affect that. Let me know if there's any specific test that you want me to do. Thanks for the tests. I wonder now if this was working before because really my change should not break it... I'll think more about it. I think of_parse_phandle_with_args() needs to be used instead of just of_parse_phandle() for that to work, as AFAICS the latter assumes the cells count == 0. We would need first to update the driver and then dts.
Re: [PATCH v2 1/3] ARM: dts: exynos: Add assigned clock parent to CMU in Exynos3250
On 06.09.2020 14:44, Krzysztof Kozlowski wrote: >>> diff --git a/arch/arm/boot/dts/exynos3250.dtsi >>> b/arch/arm/boot/dts/exynos3250.dtsi >>> index a1e93fb7f694..89b160280469 100644 >>> --- a/arch/arm/boot/dts/exynos3250.dtsi >>> +++ b/arch/arm/boot/dts/exynos3250.dtsi >>> @@ -214,6 +214,7 @@ >>> compatible = "samsung,exynos3250-cmu"; >>> reg = <0x1003 0x2>; >>> #clock-cells = <1>; >>> + clocks = < CLK_FIN_PLL>; >> This is not a correct input clock for this CMU. Please assign it to >> xusbxti, xxti or xtcxo in the respective board dts, as this is a board >> property. > Makes sense, although all this is kind of a hack as neither the bindings > nor the driver take the input clock. I think we should update the bindings so possible input clocks to the CMU are documented for all SoCs. This is actually a bug in the clock controller DT bindings that the input clocks are missing. Then the driver would handle both the old and the updated bindings but the "clocks" property would be documented as mandatory. I will try to have a look at this.
Re: [PATCH v2 3/3] ARM: dts: exynos: Add assigned clock parent to CMU in Exynos5422 Odroid XU3
On 9/3/20 20:14, Krzysztof Kozlowski wrote:
> Commit 78a68acf3d33 ("ARM: dts: exynos: Switch to dedicated Odroid XU3
> sound card binding") added assigned clocks under sound device node.
>
> However the dtschema expects "clocks" property if "assigned-clocks" are
> used. Add reference to input clock, the parent used in
> "assigned-clock-parents" to silence the dtschema warnings:
I'm afraid it doesn't improve anything, we just add another violation of
the DT binding rules as the 'sound' node doesn't represent a real HW and
shouldn't have 'clocks' property. Instead we could move the assigned-clock*
properties to the I2S node, as in below patch. I have tested that already
on xu3.
--8<---
>From f98d2f5ac86d1ae13a77ef481fcbf073a1740f26 Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki
Date: Fri, 4 Sep 2020 12:02:11 +0200
Subject: [PATCH] ARM: dts: samsung: odroid-xu3: Move assigned-clock*
properties to i2s0 node
The purpose of those assigned-clock-* properties is to configure clock for
for the I2S device so move them to respective node.
This suppresses the dtbs_check warning:
arch/arm/boot/dts/exynos5422-odroidxu3.dt.yaml: sound: 'clocks' is a dependency
of 'assigned-clocks'
Signed-off-by: Sylwester Nawrocki
---
arch/arm/boot/dts/exynos5422-odroidxu3-audio.dtsi | 60 ++-
1 file changed, 27 insertions(+), 33 deletions(-)
diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3-audio.dtsi
b/arch/arm/boot/dts/exynos5422-odroidxu3-audio.dtsi
index c3c2d85..b5ec4f4 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3-audio.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3-audio.dtsi
@@ -29,30 +29,6 @@
"HiFi Playback", "Mixer DAI TX",
"Mixer DAI RX", "HiFi Capture";
- assigned-clocks = < CLK_MOUT_EPLL>,
- < CLK_MOUT_MAU_EPLL>,
- < CLK_MOUT_USER_MAU_EPLL>,
- <_audss EXYNOS_MOUT_AUDSS>,
- <_audss EXYNOS_MOUT_I2S>,
- <_audss EXYNOS_DOUT_SRP>,
- <_audss EXYNOS_DOUT_AUD_BUS>,
- <_audss EXYNOS_DOUT_I2S>;
-
- assigned-clock-parents = < CLK_FOUT_EPLL>,
- < CLK_MOUT_EPLL>,
- < CLK_MOUT_MAU_EPLL>,
- < CLK_MAU_EPLL>,
- <_audss EXYNOS_MOUT_AUDSS>;
-
- assigned-clock-rates = <0>,
- <0>,
- <0>,
- <0>,
- <0>,
- <196608001>,
- <(196608002 / 2)>,
- <196608000>;
-
cpu {
sound-dai = < 0>, < 1>;
};
@@ -62,13 +38,6 @@
};
};
-_audss {
- assigned-clocks = <_audss EXYNOS_DOUT_SRP>,
- < CLK_FOUT_EPLL>;
- assigned-clock-rates = <(196608000 / 256)>,
- <196608000>;
-};
-
_5 {
status = "okay";
max98090: max98090@10 {
@@ -84,6 +53,31 @@
{
status = "okay";
- assigned-clocks = < CLK_I2S_RCLK_SRC>;
- assigned-clock-parents = <_audss EXYNOS_SCLK_I2S>;
+ assigned-clocks = < CLK_MOUT_EPLL>,
+ < CLK_MOUT_MAU_EPLL>,
+ < CLK_MOUT_USER_MAU_EPLL>,
+ <_audss EXYNOS_MOUT_AUDSS>,
+ <_audss EXYNOS_MOUT_I2S>,
+ < CLK_I2S_RCLK_SRC>,
+ <_audss EXYNOS_DOUT_SRP>,
+ <_audss EXYNOS_DOUT_AUD_BUS>,
+ <_audss EXYNOS_DOUT_I2S>;
+
+ assigned-clock-parents = < CLK_FOUT_EPLL>,
+ < CLK_MOUT_EPLL>,
+ < CLK_MOUT_MAU_EPLL>,
+ < CLK_MAU_EPLL>,
+ <_audss EXYNOS_MOUT_AUDSS>,
+ <_audss EXYNOS_SCLK_I2S>;
+
+ assigned-clock-rates = <0>,
+ <0>,
+ <0>,
+ <0>,
+ <0>,
+ <0>,
+ <196608001>,
+ <(196608002 / 2)>,
+ <196608000>;
+
};
--
2.7.4
--8<---
Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
On 9/3/20 10:45, Lukasz Stelmach wrote:
> It was <2020-09-02 śro 10:14>, when Sylwester Nawrocki wrote:
>> On 9/1/20 17:21, Lukasz Stelmach wrote:
>>> It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote:
>>>> On 8/21/20 18:13, Łukasz Stelmach wrote:
>>>>> Check return values in prepare_dma() and s3c64xx_spi_config() and
>>>>> propagate errors upwards.
>>>>>
>>>>> Signed-off-by: Łukasz Stelmach
>>>>> ---
>>>>> drivers/spi/spi-s3c64xx.c | 47 ---
>>>>> 1 file changed, 39 insertions(+), 8 deletions(-)
>>>>
>>>>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data
>>>>> *dma,
>>>>> desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>>>>> dma->direction,
>>>>> DMA_PREP_INTERRUPT);
>>>>> + if (!desc) {
>>>>> + dev_err(>pdev->dev, "unable to prepare %s scatterlist",
>>>>> + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> desc->callback = s3c64xx_spi_dmacb;
>>>>> desc->callback_param = dma;
>>>>> dma->cookie = dmaengine_submit(desc);
>>>>> + ret = dma_submit_error(dma->cookie);
>>>>> + if (ret) {
>>>>> + dev_err(>pdev->dev, "DMA submission failed");
>>>>> + return -EIO;
>>>>
>>>> Just return the error value from dma_submit_error() here?
>>>>
>>>
>>> --8<---cut here---start->8---
>>> static inline int dma_submit_error(dma_cookie_t cookie)
>>> {
>>> return cookie < 0 ? cookie : 0;
>>>
>>> }
>>> --8<---cut here---end--->8---
>>>
>>> Not quite meaningful IMHO, is it?
>>
>> dma_submit_error() returns 0 or an error code, I think it makes sense
>> to propagate that error code rather than replacing it with -EIO.
>
> It is not an error code that d_s_e() returns it is a value returned by
> dma_cookie_assign() called from within the tx_submit() operation of a
> DMA driver.
>
> --8<---cut here---start->8---
> static inline dma_cookie_t dma_cookie_assign(struct
> dma_async_tx_descriptor *tx)
> {
> struct dma_chan *chan = tx->chan;
> dma_cookie_t cookie;
>
> cookie = chan->cookie + 1;
> if (cookie < DMA_MIN_COOKIE)
> cookie = DMA_MIN_COOKIE;
> tx->cookie = chan->cookie = cookie;
>
> return cookie;
> }
> --8<---cut here---end--->8---
>
> Yes, a non-zero value returned by d_s_e() indicates an error but it
> definitely isn't one of error codes from errno*.h.
I guess we can end that discussion at this point and keep your patch
as is, I just followed comment at the dma_submit_error() function:
"if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code"
AFAICS dma_cookie_assign() always returns value > 0 and dma_submit_error()
only returns the cookie if its value is < 0 so in consequence d_s_e() will
be always returning 0 in your case (PL330 DMAC)?
The below commit, likely a result of static code analysis, might be
a confirmation. It could also explain why some drivers overwrite the return
value of d_s_e() and some just pass it up to the callers.
8<
commit 71ea148370f8b6c745a8a42f6fd983cf5ebade18
Author: Dan Carpenter
Date: Sat Aug 10 10:46:50 2013 +0300
dmaengine: make dma_submit_error() return an error code
The problem here is that the dma_xfer() functions in
drivers/ata/pata_arasan_cf.c and drivers/mtd/nand/fsmc_nand.c expect
dma_submit_error() to return an error code so they return 1 when they
intended to return a negative.
So far as I can tell, none of the ->tx_submit() functions ever do
return error codes so this patch should have no effect in the current
code.
I also changed it from a define to an inline.
Signed-off-by: Dan Carpenter
Signed-off-by: Dan Williams
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index cb286b1a..b3ba7e4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -38,7 +38,10 @@ typedef s32 dma_cookie_t;
#define DMA_MIN_COOKIE 1
#define DMA_MAX_COOKIE INT_MAX
-#define dma_submit_error(cookie) ((cookie) < 0 ? 1 : 0)
+static inline int dma_submit_error(dma_cookie_t cookie)
+{
+ return cookie < 0 ? cookie : 0;
+}
8<
Re: [PATCH v2 1/2] dt-bindings: mfd: syscon: Merge Samsung Exynos Sysreg bindings
On 02.09.2020 18:14, Krzysztof Kozlowski wrote: > The Samsung Exynos System Registers (Sysreg) bindings are quite simple - > just additional compatible to the syscon. They do not have any value so > merge them into generic MFD syscon bindings. > > Suggested-by: Sylwester Nawrocki > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
On 9/1/20 17:21, Lukasz Stelmach wrote:
It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote:
On 8/21/20 18:13, Łukasz Stelmach wrote:
Check return values in prepare_dma() and s3c64xx_spi_config() and
propagate errors upwards.
Signed-off-by: Łukasz Stelmach
---
drivers/spi/spi-s3c64xx.c | 47 ---
1 file changed, 39 insertions(+), 8 deletions(-)
@@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
dma->direction, DMA_PREP_INTERRUPT);
+ if (!desc) {
+ dev_err(>pdev->dev, "unable to prepare %s scatterlist",
+ dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
+ return -ENOMEM;
+ }
desc->callback = s3c64xx_spi_dmacb;
desc->callback_param = dma;
dma->cookie = dmaengine_submit(desc);
+ ret = dma_submit_error(dma->cookie);
+ if (ret) {
+ dev_err(>pdev->dev, "DMA submission failed");
+ return -EIO;
Just return the error value from dma_submit_error() here?
--8<---cut here---start->8---
static inline int dma_submit_error(dma_cookie_t cookie)
{
return cookie < 0 ? cookie : 0;
}
--8<---cut here---end--->8---
Not quite meaningful IMHO, is it?
dma_submit_error() returns 0 or an error code, I think it makes sense
to propagate that error code rather than replacing it with -EIO.
Re: [PATCH 1/2] dt-bindings: sound: midas-audio: Correct parsing sound-dai phandles
On 30.08.2020 13:26, Krzysztof Kozlowski wrote: > The "sound-dai" property has cells therefore phandle-array should be > used, even if it is just one phandle. This fixes dtbs_check warnings > like: > > arch/arm/boot/dts/exynos4412-trats2.dt.yaml: sound: cpu:sound-dai:0:1: > missing phandle tag in 0 > arch/arm/boot/dts/exynos4412-trats2.dt.yaml: sound: cpu:sound-dai:0: [158, > 0] is too long > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 2/2] dt-bindings: sound: odroid: Use unevaluatedProperties
On 30.08.2020 13:26, Krzysztof Kozlowski wrote: > Additional properties or nodes actually might appear (e.g. > assigned-clocks) so use unevaluatedProperties to fix dtbs_check warnings > like: > > arch/arm/boot/dts/exynos5422-odroidxu3.dt.yaml: sound: > 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' do > not match any of the regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 23/33] ARM: dts: exynos: Remove empty camera pinctrl configuration in Odroid X/U3
On 31.08.2020 12:42, Krzysztof Kozlowski wrote: > On Mon, 31 Aug 2020 at 12:35, Sylwester Nawrocki wrote: >> On 8/31/20 10:38, Krzysztof Kozlowski wrote: >>> On Mon, 31 Aug 2020 at 10:31, Marek Szyprowski >>> wrote: >>>> On 30.08.2020 15:51, Krzysztof Kozlowski wrote: >>>>> The camera's pinctrl configuration is simply empty and not effective. >>>>> Remove it to fix dtbs_check warning: >>>>> >>>>> arch/arm/boot/dts/exynos4412-odroidx.dt.yaml: camera: pinctrl-0: True >>>>> is not of type 'array' >>>>> >>>>> Signed-off-by: Krzysztof Kozlowski >>>> >>>> I think that this was intentional to properly enable support for >>>> mem-2-mem mode in Exynos4-IS (FIMC), but I'm not sure what are the >>>> default values if no pinctrl properties are provided. Sylwester, could >>>> you comment? >>> >>> Indeed it could be intentional... I see now errors: >>> [ 33.752203] s5p-fimc-md soc:camera: Failed to get pinctrl: -19 >>> >>> I wonder why getting an empty pinctrl is needed... maybe the driver >>> should accept missing pinctrl? >> >> It might have been better to have the pinctrl properties optional, as there >> might be boards without the image sensor attached and FIMC could still be >> used in memory-to-memory mode, as Marek pointed out. But it seems too late >> now to change that. The binding defines the pinctrl properties as required >> (Documentation/devicetree/bindings/media/samsung-fimc.txt) and we need to >> keep them in dtses. > > You can always make a required property optional and it is not a break > of ABI. The other way around would be a break. Why then not changing > the driver to accept optional pinctrl? Feel free to send the patch, I would prefer to leave that as is though. Is it really suddenly a problem to use an empty property? The pinctrl bindings allows it. -- Regards, Sylwester
Re: [PATCH 08/10] arm64: dts: exynos: Add compatibles to sysreg nodes
On 29.08.2020 16:24, Krzysztof Kozlowski wrote: > System register nodes, implementing syscon binding, should use > appropriate compatible. This fixes dtbs_check warnings: > > arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: syscon@13b8: > compatible: ['syscon'] is not valid under any of the given schemas > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 07/10] arm64: dts: exynos: Replace deprecated "gpios" i2c-gpio property in Exynos5433
On 29.08.2020 16:24, Krzysztof Kozlowski wrote: > "gpios" property is deprecated. Update the Exynos5433 DTS to fix > dtbs_checks warnings like: > > arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: i2c-gpio-0: 'sda-gpios' > is a required property > arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: i2c-gpio-0: 'scl-gpios' > is a required property > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 06/10] dt-bindings: sound: samsung-i2s: Use unevaluatedProperties
On 29.08.2020 16:24, Krzysztof Kozlowski wrote:
> Additional properties actually might appear (e.g. power-domains) so use
> unevaluatedProperties to fix dtbs_check warnings like:
>
> arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: i2s@1144:
> Additional properties are not allowed ('power-domains', '#address-cells',
> 'interrupts', '#size-cells' were unexpected)
>
> Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Sylwester Nawrocki
Re: [PATCH 04/10] dt-bindings: mfd: syscon: Document Samsung Exynos compatibles
On 29.08.2020 16:24, Krzysztof Kozlowski wrote: > Samsung Exynos SoCs use syscon for system registers so document its > compatibles. > > Signed-off-by: Krzysztof Kozlowski > --- > Documentation/devicetree/bindings/mfd/syscon.yaml | 4 > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml > b/Documentation/devicetree/bindings/mfd/syscon.yaml > index 049ec2ffc7f9..0f21943dea28 100644 > --- a/Documentation/devicetree/bindings/mfd/syscon.yaml > +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml > @@ -40,6 +40,10 @@ properties: >- allwinner,sun50i-a64-system-controller >- microchip,sparx5-cpu-syscon >- mstar,msc313-pmsleep > + - samsung,exynos3-sysreg > + - samsung,exynos4-sysreg > + - samsung,exynos5-sysreg > + - samsung,exynos5433-sysreg Reviewed-by: Sylwester Nawrocki Do you also have a patch updating Documentation/devicetree/ bindings/arm/samsung/sysreg.yaml with new compatibles? -- Regards, Sylwester
Re: [PATCH 03/10] dt-bindings: timer: exynos4210-mct: Use unevaluatedProperties
On 29.08.2020 16:24, Krzysztof Kozlowski wrote: > Additional properties actually might appear (e.g. clocks) so use > unevaluatedProperties to fix dtbs_check warnings like: > > arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: timer@101c: > 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 02/10] dt-bindings: gpu: arm,mali-midgard: Use unevaluatedProperties
On 29.08.2020 16:24, Krzysztof Kozlowski wrote: > Additional properties or nodes actually might appear (e.g. operating > points table) so use unevaluatedProperties to fix dtbs_check warnings > like: > > arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: gpu@14ac: > 'opp_table' does not match any of the regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 01/10] dt-bindings: arm: samsung: pmu: Use unevaluatedProperties
On 29.08.2020 16:24, Krzysztof Kozlowski wrote: > Additional properties actually might appear (e.g. assigned-clocks) so > use unevaluatedProperties to fix dtbs_check warnings like: > > arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: > system-controller@105c: > 'assigned-clock-parents', 'assigned-clocks' do not match any of the > regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Sylwester Nawrocki
Re: [PATCH 23/33] ARM: dts: exynos: Remove empty camera pinctrl configuration in Odroid X/U3
Hi, On 8/31/20 10:38, Krzysztof Kozlowski wrote: > On Mon, 31 Aug 2020 at 10:31, Marek Szyprowski > wrote: >> On 30.08.2020 15:51, Krzysztof Kozlowski wrote: >>> The camera's pinctrl configuration is simply empty and not effective. >>> Remove it to fix dtbs_check warning: >>> >>> arch/arm/boot/dts/exynos4412-odroidx.dt.yaml: camera: pinctrl-0: True >>> is not of type 'array' >>> >>> Signed-off-by: Krzysztof Kozlowski >> >> I think that this was intentional to properly enable support for >> mem-2-mem mode in Exynos4-IS (FIMC), but I'm not sure what are the >> default values if no pinctrl properties are provided. Sylwester, could >> you comment? > > Indeed it could be intentional... I see now errors: > [ 33.752203] s5p-fimc-md soc:camera: Failed to get pinctrl: -19 > > I wonder why getting an empty pinctrl is needed... maybe the driver > should accept missing pinctrl? It might have been better to have the pinctrl properties optional, as there might be boards without the image sensor attached and FIMC could still be used in memory-to-memory mode, as Marek pointed out. But it seems too late now to change that. The binding defines the pinctrl properties as required (Documentation/devicetree/bindings/media/samsung-fimc.txt) and we need to keep them in dtses. -- Regards, Sylwester
Re: [PATCH RFC v6 1/6] dt-bindings: exynos-bus: Add documentation for interconnect properties
On 30.07.2020 14:28, Sylwester Nawrocki wrote:
> On 09.07.2020 23:04, Rob Herring wrote:
>> On Thu, Jul 02, 2020 at 06:37:19PM +0200, Sylwester Nawrocki wrote:
>>> Add documentation for new optional properties in the exynos bus nodes:
>>> samsung,interconnect-parent, #interconnect-cells, bus-width.
>>> These properties allow to specify the SoC interconnect structure which
>>> then allows the interconnect consumer devices to request specific
>>> bandwidth requirements.
>>>
>>> Signed-off-by: Artur Świgoń
>>> Signed-off-by: Sylwester Nawrocki
>>> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>>> @@ -51,6 +51,13 @@ Optional properties only for parent bus device:
>>> - exynos,saturation-ratio: the percentage value which is used to calibrate
>>> the performance count against total cycle count.
>>>
>>> +Optional properties for interconnect functionality (QoS frequency
>>> constraints):
>>> +- samsung,interconnect-parent: phandle to the parent interconnect node; for
>>> + passive devices should point to same node as the exynos,parent-bus
>>> property.
>> Adding vendor specific properties for a common binding defeats the
>> point.
Actually we could do without any new property if we used existing interconnect
consumers binding to specify linking between the provider nodes. I think those
exynos-bus nodes could well be considered both the interconnect providers
and consumers. The example would then be something along the lines
(yes, I know the bus node naming needs to be fixed):
soc {
bus_dmc: bus_dmc {
compatible = "samsung,exynos-bus";
/* ... */
samsung,data-clock-ratio = <4>;
#interconnect-cells = <0>;
};
bus_leftbus: bus_leftbus {
compatible = "samsung,exynos-bus";
/* ... */
interconnects = <_leftbus _dmc>;
#interconnect-cells = <0>;
};
bus_display: bus_display {
compatible = "samsung,exynos-bus";
/* ... */
interconnects = <_display _leftbus>;
#interconnect-cells = <0>;
};
{
compatible = "samsung,exynos4212-mixer";
interconnects = <_display _dmc>;
/* ... */
};
};
What do you think, Georgi, Rob?
--
Regards
Sylwester
Re: [PATCH 2/2] ASoC: wm8994: Ensure the device is resumed in wm89xx_mic_detect functions
On 28.08.2020 08:48, Krzysztof Kozlowski wrote: >> diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c >> index b3ba053..fc9ea19 100644 >> --- a/sound/soc/codecs/wm8994.c >> +++ b/sound/soc/codecs/wm8994.c >> @@ -3514,6 +3514,8 @@ int wm8994_mic_detect(struct snd_soc_component >> *component, struct snd_soc_jack * >> return -EINVAL; >> } >> >> +pm_runtime_get_sync(component->dev); > The driver enables PM runtime unconditionally so you should probably > handle the error code here. I know that driver does not do it in other > cases but it should not be a reason to multiple this pattern... unless > it really does not matter as there are no runtime PM ops? The regmap is provided by the parent MFD device (drivers/mfd/wm8994-core.c) and that is where those runtime PM calls get propagated, we could possibly get en error if there is something wrong with resuming the parent device. If you don't mind I would prefer to omit the return value tests in that fix patch. Existing callers of the wm89*_mic_detect() functions are ignoring the return value anyway. Probably the checks could be added in a separate patch. -- Thanks, Sylwester
[PATCH 2/2] ASoC: wm8994: Ensure the device is resumed in wm89xx_mic_detect functions
When the wm8958_mic_detect, wm8994_mic_detect functions get called from
the machine driver, e.g. from the card's late_probe() callback, the CODEC
device may be PM runtime suspended and any regmap writes have no effect.
Add PM runtime calls to these functions to ensure the device registers
are updated as expected.
This suppresses an error during boot
"wm8994-codec: ASoC: error at snd_soc_component_update_bits on wm8994-codec"
caused by the regmap access error due to the cache_only flag being set.
Signed-off-by: Sylwester Nawrocki
---
sound/soc/codecs/wm8994.c | 8
1 file changed, 8 insertions(+)
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index b3ba053..fc9ea19 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -3514,6 +3514,8 @@ int wm8994_mic_detect(struct snd_soc_component
*component, struct snd_soc_jack *
return -EINVAL;
}
+ pm_runtime_get_sync(component->dev);
+
switch (micbias) {
case 1:
micdet = >micdet[0];
@@ -3561,6 +3563,8 @@ int wm8994_mic_detect(struct snd_soc_component
*component, struct snd_soc_jack *
snd_soc_dapm_sync(dapm);
+ pm_runtime_put(component->dev);
+
return 0;
}
EXPORT_SYMBOL_GPL(wm8994_mic_detect);
@@ -3932,6 +3936,8 @@ int wm8958_mic_detect(struct snd_soc_component
*component, struct snd_soc_jack *
return -EINVAL;
}
+ pm_runtime_get_sync(component->dev);
+
if (jack) {
snd_soc_dapm_force_enable_pin(dapm, "CLK_SYS");
snd_soc_dapm_sync(dapm);
@@ -4000,6 +4006,8 @@ int wm8958_mic_detect(struct snd_soc_component
*component, struct snd_soc_jack *
snd_soc_dapm_sync(dapm);
}
+ pm_runtime_put(component->dev);
+
return 0;
}
EXPORT_SYMBOL_GPL(wm8958_mic_detect);
--
2.7.4
[PATCH 1/2] ASoC: wm8994: Skip setting of the WM8994_MICBIAS register for WM1811
The WM8994_MICBIAS register is not available in the WM1811 CODEC so skip
initialization of that register for that device.
This suppresses an error during boot:
"wm8994-codec: ASoC: error at snd_soc_component_update_bits on wm8994-codec"
Signed-off-by: Sylwester Nawrocki
---
sound/soc/codecs/wm8994.c | 2 ++
sound/soc/codecs/wm_hubs.c | 3 +++
sound/soc/codecs/wm_hubs.h | 1 +
3 files changed, 6 insertions(+)
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c
index 038be66..b3ba053 100644
--- a/sound/soc/codecs/wm8994.c
+++ b/sound/soc/codecs/wm8994.c
@@ -4193,11 +4193,13 @@ static int wm8994_component_probe(struct
snd_soc_component *component)
wm8994->hubs.dcs_readback_mode = 2;
break;
}
+ wm8994->hubs.micd_scthr = true;
break;
case WM8958:
wm8994->hubs.dcs_readback_mode = 1;
wm8994->hubs.hp_startup_mode = 1;
+ wm8994->hubs.micd_scthr = true;
switch (control->revision) {
case 0:
diff --git a/sound/soc/codecs/wm_hubs.c b/sound/soc/codecs/wm_hubs.c
index 891effe..0c88184 100644
--- a/sound/soc/codecs/wm_hubs.c
+++ b/sound/soc/codecs/wm_hubs.c
@@ -1223,6 +1223,9 @@ int wm_hubs_handle_analogue_pdata(struct
snd_soc_component *component,
snd_soc_component_update_bits(component,
WM8993_ADDITIONAL_CONTROL,
WM8993_LINEOUT2_FB, WM8993_LINEOUT2_FB);
+ if (!hubs->micd_scthr)
+ return 0;
+
snd_soc_component_update_bits(component, WM8993_MICBIAS,
WM8993_JD_SCTHR_MASK | WM8993_JD_THR_MASK |
WM8993_MICB1_LVL | WM8993_MICB2_LVL,
diff --git a/sound/soc/codecs/wm_hubs.h b/sound/soc/codecs/wm_hubs.h
index 4b8e5f0..988b29e 100644
--- a/sound/soc/codecs/wm_hubs.h
+++ b/sound/soc/codecs/wm_hubs.h
@@ -27,6 +27,7 @@ struct wm_hubs_data {
int hp_startup_mode;
int series_startup;
int no_series_update;
+ bool micd_scthr;
bool no_cache_dac_hp_direct;
struct list_head dcs_cache;
--
2.7.4
[PATCH 3/3] clk: samsung: Use cached clk_hws instead of __clk_lookup() calls
For the CPU clock registration two parent clocks are required, these
are now being passed as struct clk_hw pointers, rather than by the
global scope names. That allows us to avoid __clk_lookup() calls
and simplifies a bit the CPU clock registration function.
While at it drop unneeded extern keyword in the function declaration.
Signed-off-by: Sylwester Nawrocki
---
drivers/clk/samsung/clk-cpu.c| 37 +++-
drivers/clk/samsung/clk-cpu.h| 6 +++---
drivers/clk/samsung/clk-exynos3250.c | 6 --
drivers/clk/samsung/clk-exynos4.c| 7 +--
drivers/clk/samsung/clk-exynos5250.c | 4 +++-
drivers/clk/samsung/clk-exynos5420.c | 6 +++---
drivers/clk/samsung/clk-exynos5433.c | 10 --
7 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
index efc4fa6..00ef4d1 100644
--- a/drivers/clk/samsung/clk-cpu.c
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -401,26 +401,34 @@ static int exynos5433_cpuclk_notifier_cb(struct
notifier_block *nb,
/* helper function to register a CPU clock */
int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
- unsigned int lookup_id, const char *name, const char *parent,
- const char *alt_parent, unsigned long offset,
- const struct exynos_cpuclk_cfg_data *cfg,
+ unsigned int lookup_id, const char *name,
+ const struct clk_hw *parent, const struct clk_hw *alt_parent,
+ unsigned long offset, const struct exynos_cpuclk_cfg_data *cfg,
unsigned long num_cfgs, unsigned long flags)
{
struct exynos_cpuclk *cpuclk;
struct clk_init_data init;
- struct clk *parent_clk;
+ const char *parent_name;
int ret = 0;
+ if (IS_ERR(parent) || IS_ERR(alt_parent)) {
+ pr_err("%s: invalid parent clock(s)\n", __func__);
+ return -EINVAL;
+ }
+
cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
if (!cpuclk)
return -ENOMEM;
+ parent_name = clk_hw_get_name(parent);
+
init.name = name;
init.flags = CLK_SET_RATE_PARENT;
- init.parent_names =
+ init.parent_names = _name;
init.num_parents = 1;
init.ops = _cpuclk_clk_ops;
+ cpuclk->alt_parent = alt_parent;
cpuclk->hw.init =
cpuclk->ctrl_base = ctx->reg_base + offset;
cpuclk->lock = >lock;
@@ -430,23 +438,8 @@ int __init exynos_register_cpu_clock(struct
samsung_clk_provider *ctx,
else
cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
- cpuclk->alt_parent = __clk_get_hw(__clk_lookup(alt_parent));
- if (!cpuclk->alt_parent) {
- pr_err("%s: could not lookup alternate parent %s\n",
- __func__, alt_parent);
- ret = -EINVAL;
- goto free_cpuclk;
- }
-
- parent_clk = __clk_lookup(parent);
- if (!parent_clk) {
- pr_err("%s: could not lookup parent clock %s\n",
- __func__, parent);
- ret = -EINVAL;
- goto free_cpuclk;
- }
- ret = clk_notifier_register(parent_clk, >clk_nb);
+ ret = clk_notifier_register(parent->clk, >clk_nb);
if (ret) {
pr_err("%s: failed to register clock notifier for %s\n",
__func__, name);
@@ -471,7 +464,7 @@ int __init exynos_register_cpu_clock(struct
samsung_clk_provider *ctx,
free_cpuclk_data:
kfree(cpuclk->cfg);
unregister_clk_nb:
- clk_notifier_unregister(parent_clk, >clk_nb);
+ clk_notifier_unregister(parent->clk, >clk_nb);
free_cpuclk:
kfree(cpuclk);
return ret;
diff --git a/drivers/clk/samsung/clk-cpu.h b/drivers/clk/samsung/clk-cpu.h
index ad38cc2..af74686 100644
--- a/drivers/clk/samsung/clk-cpu.h
+++ b/drivers/clk/samsung/clk-cpu.h
@@ -46,7 +46,7 @@ struct exynos_cpuclk_cfg_data {
*/
struct exynos_cpuclk {
struct clk_hw hw;
- struct clk_hw *alt_parent;
+ const struct clk_hw *alt_parent;
void __iomem*ctrl_base;
spinlock_t *lock;
const struct exynos_cpuclk_cfg_data *cfg;
@@ -62,9 +62,9 @@ struct exynos_cpuclk {
#define CLK_CPU_HAS_E5433_REGS_LAYOUT (1 << 2)
};
-extern int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
+int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
unsigned int lookup_id, const char *name,
- const char *parent, const char *alt_parent,
+ const struct clk_hw *parent, const struct clk_hw
*
[PATCH 2/3] clk: samsung: exynos5420/5250: Add IDs to the CPU parent clk definitions
Use non-zero clock IDs in definitions of the CPU parent clocks
for exynos5420, exynos5250 SoCs. This will allow us to reference
the parent clocks directly in the driver by cached struct clk_hw
pointers, rather than doing clk lookup by name.
Signed-off-by: Sylwester Nawrocki
---
drivers/clk/samsung/clk-exynos5250.c | 4 ++--
drivers/clk/samsung/clk-exynos5420.c | 11 ++-
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos5250.c
b/drivers/clk/samsung/clk-exynos5250.c
index 931c70a..7bcff76 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -253,14 +253,14 @@ static const struct samsung_mux_clock
exynos5250_mux_clks[] __initconst = {
/*
* CMU_CPU
*/
- MUX_F(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
+ MUX_F(CLK_MOUT_APLL, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
CLK_SET_RATE_PARENT, 0),
MUX(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1),
/*
* CMU_CORE
*/
- MUX(0, "mout_mpll", mout_mpll_p, SRC_CORE1, 8, 1),
+ MUX(CLK_MOUT_MPLL, "mout_mpll", mout_mpll_p, SRC_CORE1, 8, 1),
/*
* CMU_TOP
diff --git a/drivers/clk/samsung/clk-exynos5420.c
b/drivers/clk/samsung/clk-exynos5420.c
index f76ebd6..d07cee2 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -596,13 +596,14 @@ static const struct samsung_gate_clock
exynos5420_gate_clks[] __initconst = {
static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
MUX(0, "mout_user_pclk66_gpio", mout_user_pclk66_gpio_p,
SRC_TOP7, 4, 1),
- MUX(0, "mout_mspll_kfc", mout_mspll_cpu_p, SRC_TOP7, 8, 2),
- MUX(0, "mout_mspll_cpu", mout_mspll_cpu_p, SRC_TOP7, 12, 2),
-
- MUX_F(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
+ MUX(CLK_MOUT_MSPLL_KFC, "mout_mspll_kfc", mout_mspll_cpu_p,
+ SRC_TOP7, 8, 2),
+ MUX(CLK_MOUT_MSPLL_CPU, "mout_mspll_cpu", mout_mspll_cpu_p,
+ SRC_TOP7, 12, 2),
+ MUX_F(CLK_MOUT_APLL, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
CLK_SET_RATE_PARENT | CLK_RECALC_NEW_RATES, 0),
MUX(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1),
- MUX_F(0, "mout_kpll", mout_kpll_p, SRC_KFC, 0, 1,
+ MUX_F(CLK_MOUT_KPLL, "mout_kpll", mout_kpll_p, SRC_KFC, 0, 1,
CLK_SET_RATE_PARENT | CLK_RECALC_NEW_RATES, 0),
MUX(0, "mout_kfc", mout_kfc_p, SRC_KFC, 16, 1),
--
2.7.4
[PATCH 1/3] clk: samsung: Add clk ID definitions for the CPU parent clocks
Add clock ID definitions for the CPU parent clocks for SoCs which don't have such definitions yet. This will allow us to reference the parent clocks directly by cached struct clk_hw pointers in the clock provider, rather than doing clk lookup by name. Signed-off-by: Sylwester Nawrocki --- include/dt-bindings/clock/exynos5250.h | 4 +++- include/dt-bindings/clock/exynos5420.h | 5 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/include/dt-bindings/clock/exynos5250.h b/include/dt-bindings/clock/exynos5250.h index bc8a3c5..e259cc0 100644 --- a/include/dt-bindings/clock/exynos5250.h +++ b/include/dt-bindings/clock/exynos5250.h @@ -172,8 +172,10 @@ #define CLK_MOUT_GPLL 1025 #define CLK_MOUT_ACLK200_DISP1_SUB 1026 #define CLK_MOUT_ACLK300_DISP1_SUB 1027 +#define CLK_MOUT_APLL 1028 +#define CLK_MOUT_MPLL 1029 /* must be greater than maximal clock id */ -#define CLK_NR_CLKS1028 +#define CLK_NR_CLKS1030 #endif /* _DT_BINDINGS_CLOCK_EXYNOS_5250_H */ diff --git a/include/dt-bindings/clock/exynos5420.h b/include/dt-bindings/clock/exynos5420.h index ff917c8..9fffc6c 100644 --- a/include/dt-bindings/clock/exynos5420.h +++ b/include/dt-bindings/clock/exynos5420.h @@ -231,6 +231,11 @@ #define CLK_MOUT_SCLK_SPLL 660 #define CLK_MOUT_MX_MSPLL_CCORE_PHY661 #define CLK_MOUT_SW_ACLK_G3D 662 +#define CLK_MOUT_APLL 663 +#define CLK_MOUT_MSPLL_CPU 664 +#define CLK_MOUT_KPLL 665 +#define CLK_MOUT_MSPLL_KFC 666 + /* divider clocks */ #define CLK_DOUT_PIXEL 768 -- 2.7.4
Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values
On 8/21/20 18:13, Łukasz Stelmach wrote:
Check return values in prepare_dma() and s3c64xx_spi_config() and
propagate errors upwards.
Signed-off-by: Łukasz Stelmach
---
drivers/spi/spi-s3c64xx.c | 47 ---
1 file changed, 39 insertions(+), 8 deletions(-)
@@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
dma->direction, DMA_PREP_INTERRUPT);
+ if (!desc) {
+ dev_err(>pdev->dev, "unable to prepare %s scatterlist",
+ dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
+ return -ENOMEM;
+ }
desc->callback = s3c64xx_spi_dmacb;
desc->callback_param = dma;
dma->cookie = dmaengine_submit(desc);
+ ret = dma_submit_error(dma->cookie);
+ if (ret) {
+ dev_err(>pdev->dev, "DMA submission failed");
+ return -EIO;
Just return the error value from dma_submit_error() here?
[PATCH v3] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
In the .set_rate callback for some PLLs there is a loop polling state
of the PLL lock bit and it may become an endless loop when something
goes wrong with the PLL. For some PLLs there is already code for polling
with a timeout but it uses the ktime API, which doesn't work in some
conditions when the set_rate op is called, in particular during
initialization of the clk provider before the clocksource initialization
has completed. Hence the ktime API cannot be used to reliably detect
the PLL locking timeout.
This patch adds a common helper function for busy waiting on the PLL lock
bit with timeout detection.
Actual PLL lock time depends on the P divider value, the VCO frequency
and a constant PLL type specific LOCK_FACTOR and can be calculated as
lock_time = Pdiv * LOCK_FACTOR / VCO_freq
Common timeout value of 10 ms is used for all the PLLs with an assumption
that maximum possible value of Pdiv is 64, maximum possible LOCK_FACTOR
value is 3000 and minimum VCO frequency is 24 MHz.
Signed-off-by: Sylwester Nawrocki
---
Changes for v3:
- use busy-loop with udelay() instead of ktime API
Changes for v2:
- use common readl_relaxed_poll_timeout_atomic() macro
---
drivers/clk/samsung/clk-pll.c | 94 ---
1 file changed, 34 insertions(+), 60 deletions(-)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index ac70ad7..c83d261 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -15,7 +15,7 @@
#include "clk.h"
#include "clk-pll.h"
-#define PLL_TIMEOUT_MS 10
+#define PLL_TIMEOUT_US 1U
struct samsung_clk_pll {
struct clk_hw hw;
@@ -63,6 +63,25 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
return rate_table[i - 1].rate;
}
+static int samsung_pll_lock_wait(struct samsung_clk_pll *pll,
+unsigned int reg_mask)
+{
+ int i;
+
+ /* Wait until the PLL is in steady locked state */
+ for (i = 0; i < PLL_TIMEOUT_US / 2; i++) {
+ if (readl_relaxed(pll->con_reg) & reg_mask)
+ return 0;
+
+ udelay(2);
+ cpu_relax();
+ }
+
+ pr_err("Could not lock PLL %s\n", clk_hw_get_name(>hw));
+
+ return -ETIMEDOUT;
+}
+
static int samsung_pll3xxx_enable(struct clk_hw *hw)
{
struct samsung_clk_pll *pll = to_clk_pll(hw);
@@ -241,12 +260,9 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw,
unsigned long drate,
writel_relaxed(tmp, pll->con_reg);
/* Wait until the PLL is locked if it is enabled. */
- if (tmp & BIT(pll->enable_offs)) {
- do {
- cpu_relax();
- tmp = readl_relaxed(pll->con_reg);
- } while (!(tmp & BIT(pll->lock_offs)));
- }
+ if (tmp & BIT(pll->enable_offs))
+ return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
+
return 0;
}
@@ -318,7 +334,7 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw,
unsigned long drate,
unsigned long parent_rate)
{
struct samsung_clk_pll *pll = to_clk_pll(hw);
- u32 tmp, pll_con0, pll_con1;
+ u32 pll_con0, pll_con1;
const struct samsung_pll_rate_table *rate;
rate = samsung_get_pll_settings(pll, drate);
@@ -356,13 +372,8 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw,
unsigned long drate,
pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
writel_relaxed(pll_con1, pll->con_reg + 4);
- /* wait_lock_time */
- if (pll_con0 & BIT(pll->enable_offs)) {
- do {
- cpu_relax();
- tmp = readl_relaxed(pll->con_reg);
- } while (!(tmp & BIT(pll->lock_offs)));
- }
+ if (pll_con0 & BIT(pll->enable_offs))
+ return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
return 0;
}
@@ -437,7 +448,6 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw,
unsigned long drate,
struct samsung_clk_pll *pll = to_clk_pll(hw);
const struct samsung_pll_rate_table *rate;
u32 con0, con1;
- ktime_t start;
/* Get required rate settings from table */
rate = samsung_get_pll_settings(pll, drate);
@@ -489,20 +499,7 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw,
unsigned long drate,
writel_relaxed(con0, pll->con_reg);
/* Wait for locking. */
- start = ktime_get();
- while (!(readl_relaxed(pll->con_reg) & PLL45XX_LOCKED)) {
- ktime_t delta = ktime_sub(ktime_get(), start);
-
- if (ktime_to_ms(delta) > PLL_TIMEOUT_MS) {
- pr_err("%s: could not lock PLL %s\n",
- __func__, clk_
Re: [PATCH v2] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
On 11.08.2020 18:53, Tomasz Figa wrote: > Yeah... I should've thought about this. Interestingly enough, some of > the existing implementations in drivers/clk/samsung/clk-pll.c use the > ktime API. I guess they are lucky enough not to be called too early, > i.e. are not needed for the initialization of timers. In normal conditions nothing bad really happens, the loop exits as soon as the lock bit is asserted. Just the timeout condition will never be detected - if there is something wrong with the PLL's setup and it never locks we will never stop polling. -- Regards, Sylwester
Re: [PATCH v2] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
Hi Tomasz, On 11.08.2020 14:59, Tomasz Figa wrote: > 2020年8月11日(火) 13:25 Sylwester Nawrocki : >> >> In the .set_rate callback for some PLLs there is a loop polling state >> of the PLL lock bit and it may become an endless loop when something >> goes wrong with the PLL. For some PLLs there is already (a duplicated) >> code for polling with timeout. This patch replaces that code with >> the readl_relaxed_poll_timeout_atomic() macro and moves it to a common >> helper function, which is then used for all the PLLs. The downside >> of switching to the common macro is that we drop the cpu_relax() call. > > Tbh. I'm not sure what effect was exactly expected from cpu_relax() in > the functions which already had timeout handling. Could someone shed > some light on this? > >> Using a common helper function rather than the macro directly allows >> to avoid repeating the error message in the code and to avoid the object >> code size increase due to inlining. >> >> Signed-off-by: Sylwester Nawrocki >> --- >> Changes for v2: >> - use common readl_relaxed_poll_timeout_atomic() macro >> --- >> drivers/clk/samsung/clk-pll.c | 92 >> +++ >> 1 file changed, 32 insertions(+), 60 deletions(-) >> >> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c >> index ac70ad7..c3c1efe 100644 >> --- a/drivers/clk/samsung/clk-pll.c >> +++ b/drivers/clk/samsung/clk-pll.c >> @@ -9,13 +9,14 @@ >> -#define PLL_TIMEOUT_MS 10 >> +#define PLL_TIMEOUT_US 1U > > I'm also wondering if 10ms is the universal value that would cover the > oldest PLLs as well, but my loose recollection is that they should > still lock much faster than that. Could you double check that in the > documentation? Thanks for your comments. The oldest PLLs have a hard coded 300 us waiting time for PLL lock and are not affected by the patch. I have checked some of the PLLs and maximum observed lock time was around 370 us and most of the time it was just a few us. We calculate the lock time in each set_rate op, in the oscillator cycle units, as a product of current P divider value and a constant PLL type specific LOCK_FACTOR. Maximum possible P value is 64, maximum possible LOCK_FACTOR is 3000. Assuming minimum VCO frequency of 24 MHz (which I think will usually be much higher than that) maximum lock time would be (64 x 3000) / 24 MHz = 8 ms. I think we can leave the current 10 ms value. But there is other issue, it seems we can't really use the ktime API in the set_rate callbacks, as these could be called early, before the clocksource is initialized and ktime doesn't work yet. Below trace is from a dump_stack() added to the samsung_pll_lock_wait() callback. The PLL rate setting is triggered by assigned-clock* properties in the clock supplier node. I think we need to switch to a simple udelay() loop, as is done in clk-tegra210 driver for instance. [0.00] Hardware name: Samsung Exynos (Flattened Device Tree) [0.00] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [0.00] [] (show_stack) from [] (dump_stack+0xac/0xd8) [0.00] [] (dump_stack) from [] (samsung_pll_lock_wait+0x14/0x174) [0.00] [] (samsung_pll_lock_wait) from [] (clk_change_rate+0x1a8/0x8ac) [0.00] [] (clk_change_rate) from [] (clk_core_set_rate_nolock+0x24c/0x268) [0.00] [] (clk_core_set_rate_nolock) from [] (clk_set_rate+0x30/0x64) [0.00] [] (clk_set_rate) from [] (of_clk_set_defaults+0x214/0x384) [0.00] [] (of_clk_set_defaults) from [] (of_clk_add_hw_provider+0x98/0xd8) [0.00] [] (of_clk_add_hw_provider) from [] (samsung_clk_of_add_provider+0x1c/0x30) [0.00] [] (samsung_clk_of_add_provider) from [] (exynos5250_clk_of_clk_init_driver+0x1f4/0x240) [0.00] [] (exynos5250_clk_of_clk_init_driver) from [] (of_clk_init+0x16c/0x218) [0.00] [] (of_clk_init) from [] (time_init+0x24/0x30) [0.00] [] (time_init) from [] (start_kernel+0x3b0/0x520) [0.00] [] (start_kernel) from [<>] (0x0) [0.00] samsung_pll_lock_wait: PLL fout_epll, lock time: 0 us, ret: 0 [0.00] Exynos5250: clock setup completed, armclk=17 [0.00] Switching to timer-based delay loop, resolution 41ns [0.00] clocksource: mct-frc: mask: 0x max_cycles: 0x, max_idle_ns: 79635851949 ns [0.03] sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns [0.32] genirq: irq_chip COMBINER did not update eff. affinity mask of irq 49 [0.000523] arch_timer: cp15 timer(s) running at 24.00MHz (virt). [0.000536] clocksource: arch_sys_counter: mask: 0xff max_cycles: 0x588fe9dc0, max_idle_ns: 440795202592 ns [0.000551] sched_clock: 56 bits at 24MHz, resolution 41ns, wraps every 4398046511097ns -- Regards, Sylwester
[PATCH v2 1/2] clk: samsung: exynos5420: Add definition of clock ID for mout_sw_aclk_g3d
This patch adds ID for the mout_sw_aclk_g3d (SW_CLKMUX_ACLK_G3D) clock, mostly for internal use in the CMU driver. It will allow to avoid the __clk_lookup() call when setting up the clock during the clock provider initialization. Signed-off-by: Sylwester Nawrocki --- include/dt-bindings/clock/exynos5420.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/clock/exynos5420.h b/include/dt-bindings/clock/exynos5420.h index 02d5ac4..ff917c8 100644 --- a/include/dt-bindings/clock/exynos5420.h +++ b/include/dt-bindings/clock/exynos5420.h @@ -230,6 +230,7 @@ #define CLK_MOUT_USER_MAU_EPLL 659 #define CLK_MOUT_SCLK_SPLL 660 #define CLK_MOUT_MX_MSPLL_CCORE_PHY661 +#define CLK_MOUT_SW_ACLK_G3D 662 /* divider clocks */ #define CLK_DOUT_PIXEL 768 -- 2.7.4
[PATCH v2 2/2] clk: samsung: exynos5420: Avoid __clk_lookup() calls when enabling clocks
This patch adds a clk ID to the mout_sw_aclk_g3d clk definition so related
clk pointer gets cached in the driver's private data and can be used
later instead of a __clk_lookup() call.
With that we have all clocks used in the clk_prepare_enable() calls in the
clk provider init callback cached in clk_data.hws[] and we can reference
the clk pointers directly rather than using __clk_lookup() with global names.
Signed-off-by: Sylwester Nawrocki
---
Changes for v2:
- added missing part of the patch lost during rebase of the previous version
---
drivers/clk/samsung/clk-exynos5420.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos5420.c
b/drivers/clk/samsung/clk-exynos5420.c
index bd62087..f76ebd6 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -712,8 +712,8 @@ static const struct samsung_mux_clock exynos5x_mux_clks[]
__initconst = {
SRC_TOP12, 8, 1),
MUX(0, "mout_sw_aclk266_g2d", mout_sw_aclk266_g2d_p,
SRC_TOP12, 12, 1),
- MUX_F(0, "mout_sw_aclk_g3d", mout_sw_aclk_g3d_p, SRC_TOP12, 16, 1,
- CLK_SET_RATE_PARENT, 0),
+ MUX_F(CLK_MOUT_SW_ACLK_G3D, "mout_sw_aclk_g3d", mout_sw_aclk_g3d_p,
+ SRC_TOP12, 16, 1, CLK_SET_RATE_PARENT, 0),
MUX(0, "mout_sw_aclk300_jpeg", mout_sw_aclk300_jpeg_p,
SRC_TOP12, 20, 1),
MUX(CLK_MOUT_SW_ACLK300, "mout_sw_aclk300_disp1",
@@ -1560,6 +1560,7 @@ static void __init exynos5x_clk_init(struct device_node
*np,
enum exynos5x_soc soc)
{
struct samsung_clk_provider *ctx;
+ struct clk_hw **hws;
if (np) {
reg_base = of_iomap(np, 0);
@@ -1649,17 +1650,18 @@ static void __init exynos5x_clk_init(struct device_node
*np,
exynos5x_subcmus);
}
+ hws = ctx->clk_data.hws;
/*
* Keep top part of G3D clock path enabled permanently to ensure
* that the internal busses get their clock regardless of the
* main G3D clock enablement status.
*/
- clk_prepare_enable(__clk_lookup("mout_sw_aclk_g3d"));
+ clk_prepare_enable(hws[CLK_MOUT_SW_ACLK_G3D]->clk);
/*
* Keep top BPLL mux enabled permanently to ensure that DRAM operates
* properly.
*/
- clk_prepare_enable(__clk_lookup("mout_bpll"));
+ clk_prepare_enable(hws[CLK_MOUT_BPLL]->clk);
samsung_clk_of_add_provider(np, ctx);
}
--
2.7.4
[PATCH 2/2] clk: samsung: exynos5420: Avoid __clk_lookup() calls when enabling clocks
This patch adds a clk ID to the mout_sw_aclk_g3d clk definition so related
clk pointer gets cached in the driver's private data and can be used
later instead of a __clk_lookup() call.
With that we have all clocks used in the clk_prepare_enable() calls in the
clk provider init callback cached in clk_data.hws[] and we can reference
the clk pointers directly rather than using __clk_lookup() with global names.
Signed-off-by: Sylwester Nawrocki
---
Depends on patch:
[PATCH v2] clk: samsung: Keep top BPLL mux on Exynos542x enabled
drivers/clk/samsung/clk-exynos5420.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos5420.c
b/drivers/clk/samsung/clk-exynos5420.c
index bd62087..06841a6 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -712,8 +712,8 @@ static const struct samsung_mux_clock exynos5x_mux_clks[]
__initconst = {
SRC_TOP12, 8, 1),
MUX(0, "mout_sw_aclk266_g2d", mout_sw_aclk266_g2d_p,
SRC_TOP12, 12, 1),
- MUX_F(0, "mout_sw_aclk_g3d", mout_sw_aclk_g3d_p, SRC_TOP12, 16, 1,
- CLK_SET_RATE_PARENT, 0),
+ MUX_F(CLK_MOUT_SW_ACLK_G3D, "mout_sw_aclk_g3d", mout_sw_aclk_g3d_p,
+ SRC_TOP12, 16, 1, CLK_SET_RATE_PARENT, 0),
MUX(0, "mout_sw_aclk300_jpeg", mout_sw_aclk300_jpeg_p,
SRC_TOP12, 20, 1),
MUX(CLK_MOUT_SW_ACLK300, "mout_sw_aclk300_disp1",
@@ -1649,17 +1649,18 @@ static void __init exynos5x_clk_init(struct device_node
*np,
exynos5x_subcmus);
}
+ hws = ctx->clk_data.hws;
/*
* Keep top part of G3D clock path enabled permanently to ensure
* that the internal busses get their clock regardless of the
* main G3D clock enablement status.
*/
- clk_prepare_enable(__clk_lookup("mout_sw_aclk_g3d"));
+ clk_prepare_enable(hws[CLK_MOUT_SW_ACLK_G3D]->clk);
/*
* Keep top BPLL mux enabled permanently to ensure that DRAM operates
* properly.
*/
- clk_prepare_enable(__clk_lookup("mout_bpll"));
+ clk_prepare_enable(hws[CLK_MOUT_BPLL]->clk);
samsung_clk_of_add_provider(np, ctx);
}
--
2.7.4
[PATCH 1/2] clk: samsung: exynos5420: Add definition of clock ID for mout_sw_aclk_g3d
This patch adds ID for the mout_sw_aclk_g3d (SW_CLKMUX_ACLK_G3D) clock, mostly for internal use in the CMU driver. It will allow to avoid the __clk_lookup() call when setting up the clock during the clock provider initialization. Signed-off-by: Sylwester Nawrocki --- include/dt-bindings/clock/exynos5420.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/clock/exynos5420.h b/include/dt-bindings/clock/exynos5420.h index 02d5ac4..ff917c8 100644 --- a/include/dt-bindings/clock/exynos5420.h +++ b/include/dt-bindings/clock/exynos5420.h @@ -230,6 +230,7 @@ #define CLK_MOUT_USER_MAU_EPLL 659 #define CLK_MOUT_SCLK_SPLL 660 #define CLK_MOUT_MX_MSPLL_CCORE_PHY661 +#define CLK_MOUT_SW_ACLK_G3D 662 /* divider clocks */ #define CLK_DOUT_PIXEL 768 -- 2.7.4
[PATCH v2] clk: samsung: Prevent potential endless loop in the PLL set_rate ops
In the .set_rate callback for some PLLs there is a loop polling state
of the PLL lock bit and it may become an endless loop when something
goes wrong with the PLL. For some PLLs there is already (a duplicated)
code for polling with timeout. This patch replaces that code with
the readl_relaxed_poll_timeout_atomic() macro and moves it to a common
helper function, which is then used for all the PLLs. The downside
of switching to the common macro is that we drop the cpu_relax() call.
Using a common helper function rather than the macro directly allows
to avoid repeating the error message in the code and to avoid the object
code size increase due to inlining.
Signed-off-by: Sylwester Nawrocki
---
Changes for v2:
- use common readl_relaxed_poll_timeout_atomic() macro
---
drivers/clk/samsung/clk-pll.c | 92 +++
1 file changed, 32 insertions(+), 60 deletions(-)
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index ac70ad7..c3c1efe 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -9,13 +9,14 @@
#include
#include
#include
+#include
#include
#include
#include
#include "clk.h"
#include "clk-pll.h"
-#define PLL_TIMEOUT_MS 10
+#define PLL_TIMEOUT_US 1U
struct samsung_clk_pll {
struct clk_hw hw;
@@ -63,6 +64,22 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
return rate_table[i - 1].rate;
}
+static int samsung_pll_lock_wait(struct samsung_clk_pll *pll,
+unsigned int reg_mask)
+{
+ u32 val;
+ int ret;
+
+ /* Wait until the PLL is in steady locked state */
+ ret = readl_relaxed_poll_timeout_atomic(pll->con_reg, val,
+ val & reg_mask, 0, PLL_TIMEOUT_US);
+ if (ret < 0)
+ pr_err("%s: Could not lock PLL %s\n",
+ __func__, clk_hw_get_name(>hw));
+
+ return ret;
+}
+
static int samsung_pll3xxx_enable(struct clk_hw *hw)
{
struct samsung_clk_pll *pll = to_clk_pll(hw);
@@ -241,12 +258,9 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw,
unsigned long drate,
writel_relaxed(tmp, pll->con_reg);
/* Wait until the PLL is locked if it is enabled. */
- if (tmp & BIT(pll->enable_offs)) {
- do {
- cpu_relax();
- tmp = readl_relaxed(pll->con_reg);
- } while (!(tmp & BIT(pll->lock_offs)));
- }
+ if (tmp & BIT(pll->enable_offs))
+ return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
+
return 0;
}
@@ -318,7 +332,7 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw,
unsigned long drate,
unsigned long parent_rate)
{
struct samsung_clk_pll *pll = to_clk_pll(hw);
- u32 tmp, pll_con0, pll_con1;
+ u32 pll_con0, pll_con1;
const struct samsung_pll_rate_table *rate;
rate = samsung_get_pll_settings(pll, drate);
@@ -356,13 +370,8 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw,
unsigned long drate,
pll_con1 |= rate->kdiv << PLL36XX_KDIV_SHIFT;
writel_relaxed(pll_con1, pll->con_reg + 4);
- /* wait_lock_time */
- if (pll_con0 & BIT(pll->enable_offs)) {
- do {
- cpu_relax();
- tmp = readl_relaxed(pll->con_reg);
- } while (!(tmp & BIT(pll->lock_offs)));
- }
+ if (pll_con0 & BIT(pll->enable_offs))
+ return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
return 0;
}
@@ -437,7 +446,6 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw,
unsigned long drate,
struct samsung_clk_pll *pll = to_clk_pll(hw);
const struct samsung_pll_rate_table *rate;
u32 con0, con1;
- ktime_t start;
/* Get required rate settings from table */
rate = samsung_get_pll_settings(pll, drate);
@@ -489,20 +497,7 @@ static int samsung_pll45xx_set_rate(struct clk_hw *hw,
unsigned long drate,
writel_relaxed(con0, pll->con_reg);
/* Wait for locking. */
- start = ktime_get();
- while (!(readl_relaxed(pll->con_reg) & PLL45XX_LOCKED)) {
- ktime_t delta = ktime_sub(ktime_get(), start);
-
- if (ktime_to_ms(delta) > PLL_TIMEOUT_MS) {
- pr_err("%s: could not lock PLL %s\n",
- __func__, clk_hw_get_name(hw));
- return -EFAULT;
- }
-
- cpu_relax();
- }
-
- return 0;
+ return samsung_pll_lock_wait(pll, PLL45XX_LOCKED);
}
static const struct clk_ops samsung_pll45xx_clk_ops = {
@@ -588,7 +583,6 @@ static int samsu
