linux-samsung-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver
@ 2021-09-14 15:56 Sam Protsenko
  2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
                   ` (5 more replies)
  0 siblings, 6 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

This patch series provides the implementation for Exynos850 clock
driver, its documentation and corresponding changes for Samsung clock
infrastructure:
  - Adds new PLL types used in Exynos850 SoC, following TRM
  - Enables bus clock for each registered CMU, if it's provided

I tried to follow already established design for Samsung clock drivers
(getting most insights from Exynos7 and Exynos5433 clock drivers), and
integrate the driver in existing infrastructure. The whole driver was
implemented from scratch, using mostly TRM.

For now only basic clocks are implemented, including next blocks:
  - CMU_TOP
  - CMU_PERI
  - CMU_CORE
  - CMU_HSI

Some CMUs are still not implemented, but that can be added in future,
when the need arises. The driver also lacks CLKOUT support, PM ops and
automatic clocks control (using Q-Channel protocol). All that can be
added independently later.

Implemented clock tree was tested via UART and MMC drivers, and using
DebugFS clk support (e.g. using 'clk_summary' file). In order to keep
all clocks running I added 'clk_ignore_unused' kernel param in my local
tree, and defined CLOCK_ALLOW_WRITE_DEBUGFS in clk.c for actually
testing clocks via DebugFS.

Sam Protsenko (6):
  clk: samsung: Enable bus clock on init
  clk: samsung: clk-pll: Implement pll0822x PLL type
  clk: samsung: clk-pll: Implement pll0831x PLL type
  dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  dt-bindings: clock: Document Exynos850 CMU bindings
  clk: samsung: Introduce Exynos850 clock driver

 .../clock/samsung,exynos850-clock.yaml        | 190 +++++
 drivers/clk/samsung/Makefile                  |   1 +
 drivers/clk/samsung/clk-exynos850.c           | 700 ++++++++++++++++++
 drivers/clk/samsung/clk-pll.c                 | 196 +++++
 drivers/clk/samsung/clk-pll.h                 |   2 +
 drivers/clk/samsung/clk.c                     |  13 +
 include/dt-bindings/clock/exynos850.h         |  72 ++
 7 files changed, 1174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
 create mode 100644 drivers/clk/samsung/clk-exynos850.c
 create mode 100644 include/dt-bindings/clock/exynos850.h

-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
@ 2021-09-14 15:56 ` Sam Protsenko
  2021-09-15  8:21   ` Krzysztof Kozlowski
  2021-09-15 12:51   ` Sylwester Nawrocki
  2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

By default if bus clock has no users its "enable count" value is 0. It
might be actually running if it's already enabled in bootloader, but
then in some cases it can be disabled by mistake. For example, such case
was observed when dw_mci_probe() enabled bus clock, then failed to do
something and disabled that bus clock on error path. After that even
attempt to read the 'clk_summary' file in DebugFS freezed forever, as
CMU bus clock ended up being disabled and it wasn't possible to access
CMU registers anymore.

To avoid such cases, CMU driver must increment the ref count for that
bus clock by running clk_prepare_enable(). There is already existing
'.clk_name' field in struct samsung_cmu_info, exactly for that reason.
It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
support for runtime PM"). But the clock is actually enabled only in
Exynos5433 clock driver. Let's mimic what is done there in generic
samsung_cmu_register_one() function, so other drivers can benefit from
that `.clk_name' field. As was described above, it might be helpful not
only for PM reasons, but also to prevent possible erroneous clock gating
on error paths.

Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
flag for corresponding gate clocks. But that might be not very good
design decision, as we might still want to disable that bus clock, e.g.
on PM suspend.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/samsung/clk.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 1949ae7851b2..da65149fa502 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
 
 	ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
 
+	/* Keep bus clock running, so it's possible to access CMU registers */
+	if (cmu->clk_name) {
+		struct clk *bus_clk;
+
+		bus_clk = __clk_lookup(cmu->clk_name);
+		if (bus_clk) {
+			clk_prepare_enable(bus_clk);
+		} else {
+			pr_err("%s: could not find bus clock %s\n", __func__,
+			       cmu->clk_name);
+		}
+	}
+
 	if (cmu->pll_clks)
 		samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks,
 			reg_base);
-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type
  2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
  2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
@ 2021-09-14 15:56 ` Sam Protsenko
  2021-09-15  8:24   ` Krzysztof Kozlowski
  2021-09-15 15:59   ` Chanwoo Choi
  2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

pll0822x PLL is used in Exynos850 SoC for top-level integer PLLs. The
code was derived from very similar pll35xx type, with next differences:

1. Lock time for pll0822x is 150*P_DIV, when for pll35xx it's 270*P_DIV
2. It's not suggested in Exynos850 TRM that S_DIV change doesn't require
   performing PLL lock procedure (which is done in pll35xx
   implementation)

When defining pll0822x type, CON3 register offset should be provided as
a "con" parameter of PLL() macro, like this:

    PLL(pll_0822x, 0, "fout_shared0_pll", "oscclk",
        PLL_LOCKTIME_PLL_SHARED0, PLL_CON3_PLL_SHARED0,
        exynos850_shared0_pll_rates),

To define PLL rates table, one can use PLL_35XX_RATE() macro, e.g.:

    PLL_35XX_RATE(26 * MHZ, 1600 * MHZ, 800, 13, 0)

as it's completely appropriate for pl0822x type and there is no sense in
duplicating that.

If bit #1 (MANUAL_PLL_CTRL) is not set in CON1 register, it won't be
possible to set new rate, with next error showing in kernel log:

    Could not lock PLL fout_shared1_pll

That can happen for example if bootloader clears that bit beforehand.
PLL driver doesn't account for that, so if MANUAL_PLL_CTRL bit was
cleared, it's assumed it was done for a reason and it shouldn't be
possible to change that PLL's rate at all.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/samsung/clk-pll.c | 91 +++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |  1 +
 2 files changed, 92 insertions(+)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 5873a9354b50..03131b149c0b 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -415,6 +415,89 @@ static const struct clk_ops samsung_pll36xx_clk_min_ops = {
 	.recalc_rate = samsung_pll36xx_recalc_rate,
 };
 
+/*
+ * PLL0822x Clock Type
+ */
+/* Maximum lock time can be 150 * PDIV cycles */
+#define PLL0822X_LOCK_FACTOR		(150)
+
+#define PLL0822X_MDIV_MASK		(0x3FF)
+#define PLL0822X_PDIV_MASK		(0x3F)
+#define PLL0822X_SDIV_MASK		(0x7)
+#define PLL0822X_MDIV_SHIFT		(16)
+#define PLL0822X_PDIV_SHIFT		(8)
+#define PLL0822X_SDIV_SHIFT		(0)
+#define PLL0822X_LOCK_STAT_SHIFT	(29)
+#define PLL0822X_ENABLE_SHIFT		(31)
+
+static unsigned long samsung_pll0822x_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 mdiv, pdiv, sdiv, pll_con3;
+	u64 fvco = parent_rate;
+
+	pll_con3 = readl_relaxed(pll->con_reg);
+	mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL0822X_MDIV_MASK;
+	pdiv = (pll_con3 >> PLL0822X_PDIV_SHIFT) & PLL0822X_PDIV_MASK;
+	sdiv = (pll_con3 >> PLL0822X_SDIV_SHIFT) & PLL0822X_SDIV_MASK;
+
+	fvco *= mdiv;
+	do_div(fvco, (pdiv << sdiv));
+
+	return (unsigned long)fvco;
+}
+
+static int samsung_pll0822x_set_rate(struct clk_hw *hw, unsigned long drate,
+				     unsigned long prate)
+{
+	const struct samsung_pll_rate_table *rate;
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 pll_con3;
+
+	/* Get required rate settings from table */
+	rate = samsung_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, clk_hw_get_name(hw));
+		return -EINVAL;
+	}
+
+	/* Change PLL PMS values */
+	pll_con3 = readl_relaxed(pll->con_reg);
+	pll_con3 &= ~((PLL0822X_MDIV_MASK << PLL0822X_MDIV_SHIFT) |
+			(PLL0822X_PDIV_MASK << PLL0822X_PDIV_SHIFT) |
+			(PLL0822X_SDIV_MASK << PLL0822X_SDIV_SHIFT));
+	pll_con3 |= (rate->mdiv << PLL0822X_MDIV_SHIFT) |
+			(rate->pdiv << PLL0822X_PDIV_SHIFT) |
+			(rate->sdiv << PLL0822X_SDIV_SHIFT);
+
+	/* Set PLL lock time */
+	writel_relaxed(rate->pdiv * PLL0822X_LOCK_FACTOR,
+			pll->lock_reg);
+
+	/* Write PMS values */
+	writel_relaxed(pll_con3, pll->con_reg);
+
+	/* Wait for PLL lock if the PLL is enabled */
+	if (pll_con3 & BIT(pll->enable_offs))
+		return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
+
+	return 0;
+}
+
+static const struct clk_ops samsung_pll0822x_clk_ops = {
+	.recalc_rate = samsung_pll0822x_recalc_rate,
+	.round_rate = samsung_pll_round_rate,
+	.set_rate = samsung_pll0822x_set_rate,
+	.enable = samsung_pll3xxx_enable,
+	.disable = samsung_pll3xxx_disable,
+};
+
+static const struct clk_ops samsung_pll0822x_clk_min_ops = {
+	.recalc_rate = samsung_pll0822x_recalc_rate,
+};
+
 /*
  * PLL45xx Clock Type
  */
@@ -1296,6 +1379,14 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 		else
 			init.ops = &samsung_pll35xx_clk_ops;
 		break;
+	case pll_0822x:
+		pll->enable_offs = PLL0822X_ENABLE_SHIFT;
+		pll->lock_offs = PLL0822X_LOCK_STAT_SHIFT;
+		if (!pll->rate_table)
+			init.ops = &samsung_pll0822x_clk_min_ops;
+		else
+			init.ops = &samsung_pll0822x_clk_ops;
+		break;
 	case pll_4500:
 		init.ops = &samsung_pll45xx_clk_min_ops;
 		break;
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index 79e41c226b90..213e94a97f23 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -36,6 +36,7 @@ enum samsung_pll_type {
 	pll_1451x,
 	pll_1452x,
 	pll_1460x,
+	pll_0822x,
 };
 
 #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x PLL type
  2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
  2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
  2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko
@ 2021-09-14 15:56 ` Sam Protsenko
  2021-09-15  8:26   ` Krzysztof Kozlowski
  2021-09-15 16:11   ` Chanwoo Choi
  2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

pll0831x PLL is used in Exynos850 SoC for top-level fractional PLLs. The
code was derived from very similar pll36xx type, with next differences:

1. Lock time for pll0831x is 500*P_DIV, when for pll36xx it's 3000*P_DIV
2. It's not suggested in Exynos850 TRM that S_DIV change doesn't require
   performing PLL lock procedure (which is done in pll36xx
   implementation)
3. The offset from PMS-values register to K-value register is 0x8 for
   pll0831x, when for pll36xx it's 0x4

When defining pll0831x type, CON3 register offset should be provided as
a "con" parameter of PLL() macro, like this:

    PLL(pll_0831x, 0, "fout_mmc_pll", "oscclk",
        PLL_LOCKTIME_PLL_MMC, PLL_CON3_PLL_MMC, pll0831x_26mhz_tbl),

To define PLL rates table, one can use PLL_36XX_RATE() macro, e.g.:

    PLL_36XX_RATE(26 * MHZ, 799999877, 31, 1, 0, -15124)

as it's completely appropriate for pl0831x type and there is no sense in
duplicating that.

If bit #1 (MANUAL_PLL_CTRL) is not set in CON1 register, it won't be
possible to set new rate, with next error showing in kernel log:

    Could not lock PLL fout_mmc_pll

That can happen for example if bootloader clears that bit beforehand.
PLL driver doesn't account for that, so if MANUAL_PLL_CTRL bit was
cleared, it's assumed it was done for a reason and it shouldn't be
possible to change that PLL's rate at all.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/samsung/clk-pll.c | 105 ++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |   1 +
 2 files changed, 106 insertions(+)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 03131b149c0b..83d1b03647db 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -498,6 +498,103 @@ static const struct clk_ops samsung_pll0822x_clk_min_ops = {
 	.recalc_rate = samsung_pll0822x_recalc_rate,
 };
 
+/*
+ * PLL0831x Clock Type
+ */
+/* Maximum lock time can be 500 * PDIV cycles */
+#define PLL0831X_LOCK_FACTOR		(500)
+
+#define PLL0831X_KDIV_MASK		(0xFFFF)
+#define PLL0831X_MDIV_MASK		(0x1FF)
+#define PLL0831X_PDIV_MASK		(0x3F)
+#define PLL0831X_SDIV_MASK		(0x7)
+#define PLL0831X_MDIV_SHIFT		(16)
+#define PLL0831X_PDIV_SHIFT		(8)
+#define PLL0831X_SDIV_SHIFT		(0)
+#define PLL0831X_KDIV_SHIFT		(0)
+#define PLL0831X_LOCK_STAT_SHIFT	(29)
+#define PLL0831X_ENABLE_SHIFT		(31)
+
+static unsigned long samsung_pll0831x_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 mdiv, pdiv, sdiv, pll_con3, pll_con5;
+	s16 kdiv;
+	u64 fvco = parent_rate;
+
+	pll_con3 = readl_relaxed(pll->con_reg);
+	pll_con5 = readl_relaxed(pll->con_reg + 8);
+	mdiv = (pll_con3 >> PLL0831X_MDIV_SHIFT) & PLL0831X_MDIV_MASK;
+	pdiv = (pll_con3 >> PLL0831X_PDIV_SHIFT) & PLL0831X_PDIV_MASK;
+	sdiv = (pll_con3 >> PLL0831X_SDIV_SHIFT) & PLL0831X_SDIV_MASK;
+	kdiv = (s16)((pll_con5 >> PLL0831X_KDIV_SHIFT) & PLL0831X_KDIV_MASK);
+
+	fvco *= (mdiv << 16) + kdiv;
+	do_div(fvco, (pdiv << sdiv));
+	fvco >>= 16;
+
+	return (unsigned long)fvco;
+}
+
+static int samsung_pll0831x_set_rate(struct clk_hw *hw, unsigned long drate,
+				     unsigned long parent_rate)
+{
+	const struct samsung_pll_rate_table *rate;
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 pll_con3, pll_con5;
+
+	/* Get required rate settings from table */
+	rate = samsung_get_pll_settings(pll, drate);
+	if (!rate) {
+		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
+			drate, clk_hw_get_name(hw));
+		return -EINVAL;
+	}
+
+	pll_con3 = readl_relaxed(pll->con_reg);
+	pll_con5 = readl_relaxed(pll->con_reg + 8);
+
+	/* Change PLL PMSK values */
+	pll_con3 &= ~((PLL0831X_MDIV_MASK << PLL0831X_MDIV_SHIFT) |
+			(PLL0831X_PDIV_MASK << PLL0831X_PDIV_SHIFT) |
+			(PLL0831X_SDIV_MASK << PLL0831X_SDIV_SHIFT));
+	pll_con3 |= (rate->mdiv << PLL0831X_MDIV_SHIFT) |
+			(rate->pdiv << PLL0831X_PDIV_SHIFT) |
+			(rate->sdiv << PLL0831X_SDIV_SHIFT);
+	pll_con5 &= ~(PLL0831X_KDIV_MASK << PLL0831X_KDIV_SHIFT);
+	/*
+	 * kdiv is 16-bit 2's complement (s16), but stored as unsigned int.
+	 * Cast it to u16 to avoid leading 0xffff's in case of negative value.
+	 */
+	pll_con5 |= ((u16)rate->kdiv << PLL0831X_KDIV_SHIFT);
+
+	/* Set PLL lock time */
+	writel_relaxed(rate->pdiv * PLL0831X_LOCK_FACTOR, pll->lock_reg);
+
+	/* Write PMSK values */
+	writel_relaxed(pll_con3, pll->con_reg);
+	writel_relaxed(pll_con5, pll->con_reg + 8);
+
+	/* Wait for PLL lock if the PLL is enabled */
+	if (pll_con3 & BIT(pll->enable_offs))
+		return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
+
+	return 0;
+}
+
+static const struct clk_ops samsung_pll0831x_clk_ops = {
+	.recalc_rate = samsung_pll0831x_recalc_rate,
+	.set_rate = samsung_pll0831x_set_rate,
+	.round_rate = samsung_pll_round_rate,
+	.enable = samsung_pll3xxx_enable,
+	.disable = samsung_pll3xxx_disable,
+};
+
+static const struct clk_ops samsung_pll0831x_clk_min_ops = {
+	.recalc_rate = samsung_pll0831x_recalc_rate,
+};
+
 /*
  * PLL45xx Clock Type
  */
@@ -1407,6 +1504,14 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 		else
 			init.ops = &samsung_pll36xx_clk_ops;
 		break;
+	case pll_0831x:
+		pll->enable_offs = PLL0831X_ENABLE_SHIFT;
+		pll->lock_offs = PLL0831X_LOCK_STAT_SHIFT;
+		if (!pll->rate_table)
+			init.ops = &samsung_pll0831x_clk_min_ops;
+		else
+			init.ops = &samsung_pll0831x_clk_ops;
+		break;
 	case pll_6552:
 	case pll_6552_s3c2416:
 		init.ops = &samsung_pll6552_clk_ops;
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index 213e94a97f23..a739f2b7ae80 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -37,6 +37,7 @@ enum samsung_pll_type {
 	pll_1452x,
 	pll_1460x,
 	pll_0822x,
+	pll_0831x,
 };
 
 #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
                   ` (2 preceding siblings ...)
  2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko
@ 2021-09-14 15:56 ` Sam Protsenko
  2021-09-15  8:27   ` Krzysztof Kozlowski
                     ` (2 more replies)
  2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
  2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
  5 siblings, 3 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

Clock controller driver is designed to have separate instances for each
particular CMU. So clock IDs in this bindings header also start from 1
for each CMU.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 include/dt-bindings/clock/exynos850.h | 72 +++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 include/dt-bindings/clock/exynos850.h

diff --git a/include/dt-bindings/clock/exynos850.h b/include/dt-bindings/clock/exynos850.h
new file mode 100644
index 000000000000..2f0a7f619627
--- /dev/null
+++ b/include/dt-bindings/clock/exynos850.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 Linaro Ltd.
+ * Author: Sam Protsenko <semen.protsenko@linaro.org>
+ *
+ * Device Tree binding constants for Exynos850 clock controller.
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_EXYNOS_850_H
+#define _DT_BINDINGS_CLOCK_EXYNOS_850_H
+
+/* CMU_TOP */
+#define DOUT_HSI_BUS			1
+#define DOUT_HSI_MMC_CARD		2
+#define DOUT_HSI_USB20DRD		3
+#define DOUT_PERI_BUS			4
+#define DOUT_PERI_UART			5
+#define DOUT_PERI_IP			6
+#define DOUT_CORE_BUS			7
+#define DOUT_CORE_CCI			8
+#define DOUT_CORE_MMC_EMBD		9
+#define DOUT_CORE_SSS			10
+#define TOP_NR_CLK			11
+
+/* CMU_HSI */
+#define GOUT_USB_RTC_CLK		1
+#define GOUT_USB_REF_CLK		2
+#define GOUT_USB_PHY_REF_CLK		3
+#define GOUT_USB_PHY_ACLK		4
+#define GOUT_USB_BUS_EARLY_CLK		5
+#define GOUT_GPIO_HSI_PCLK		6
+#define GOUT_MMC_CARD_ACLK		7
+#define GOUT_MMC_CARD_SDCLKIN		8
+#define GOUT_SYSREG_HSI_PCLK		9
+#define HSI_NR_CLK			10
+
+/* CMU_PERI */
+#define GOUT_GPIO_PERI_PCLK		1
+#define GOUT_HSI2C0_IPCLK		2
+#define GOUT_HSI2C0_PCLK		3
+#define GOUT_HSI2C1_IPCLK		4
+#define GOUT_HSI2C1_PCLK		5
+#define GOUT_HSI2C2_IPCLK		6
+#define GOUT_HSI2C2_PCLK		7
+#define GOUT_I2C0_PCLK			8
+#define GOUT_I2C1_PCLK			9
+#define GOUT_I2C2_PCLK			10
+#define GOUT_I2C3_PCLK			11
+#define GOUT_I2C4_PCLK			12
+#define GOUT_I2C5_PCLK			13
+#define GOUT_I2C6_PCLK			14
+#define GOUT_MCT_PCLK			15
+#define GOUT_PWM_MOTOR_PCLK		16
+#define GOUT_SPI0_IPCLK			17
+#define GOUT_SPI0_PCLK			18
+#define GOUT_SYSREG_PERI_PCLK		19
+#define GOUT_UART_IPCLK			20
+#define GOUT_UART_PCLK			21
+#define GOUT_WDT0_PCLK			22
+#define GOUT_WDT1_PCLK			23
+#define PERI_NR_CLK			24
+
+/* CMU_CORE */
+#define GOUT_CCI_ACLK			1
+#define GOUT_GIC_CLK			2
+#define GOUT_MMC_EMBD_ACLK		3
+#define GOUT_MMC_EMBD_SDCLKIN		4
+#define GOUT_SSS_ACLK			5
+#define GOUT_SSS_PCLK			6
+#define CORE_NR_CLK			7
+
+#endif /* _DT_BINDINGS_CLOCK_EXYNOS_850_H */
-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings
  2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
                   ` (3 preceding siblings ...)
  2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
@ 2021-09-14 15:56 ` Sam Protsenko
  2021-09-14 21:35   ` Rob Herring
                     ` (2 more replies)
  2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
  5 siblings, 3 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

Provide dt-schema documentation for Exynos850 SoC clock controller.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 .../clock/samsung,exynos850-clock.yaml        | 190 ++++++++++++++++++
 1 file changed, 190 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
new file mode 100644
index 000000000000..b69ba4125421
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
@@ -0,0 +1,190 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/samsung,exynos850-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung Exynos850 SoC clock controller
+
+maintainers:
+  - Sam Protsenko <semen.protsenko@linaro.org>
+  - Chanwoo Choi <cw00.choi@samsung.com>
+  - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
+  - Sylwester Nawrocki <s.nawrocki@samsung.com>
+  - Tomasz Figa <tomasz.figa@gmail.com>
+
+description: |
+  Exynos850 clock controller is comprised of several CMU units, generating
+  clocks for different domains. Those CMU units are modeled as separate device
+  tree nodes, and might depend on each other. Root clocks in that clock tree are
+  two external clocks:: OSCCLK (26 MHz) and RTCCLK (32768 Hz). Those external
+  clocks must be defined as fixed-rate clocks in dts.
+
+  CMU_TOP is a top-level CMU, where all base clocks are prepared using PLLs and
+  dividers; all other leaf clocks (other CMUs) are usually derived from CMU_TOP.
+
+  Each clock is assigned an identifier and client nodes can use this identifier
+  to specify the clock which they consume. All clocks that available for usage
+  in clock consumer nodes are defined as preprocessor macros in
+  'dt-bindings/clock/exynos850.h' header.
+
+properties:
+  compatible:
+    enum:
+      - samsung,exynos850-cmu-top
+      - samsung,exynos850-cmu-core
+      - samsung,exynos850-cmu-hsi
+      - samsung,exynos850-cmu-peri
+
+  clocks:
+    minItems: 1
+    maxItems: 5
+
+  clock-names:
+    minItems: 1
+    maxItems: 5
+
+  "#clock-cells":
+    const: 1
+
+  reg:
+    maxItems: 1
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos850-cmu-top
+
+    then:
+      properties:
+        clocks:
+          items:
+            - description: External reference clock (26 MHz)
+
+        clock-names:
+          items:
+            - const: oscclk
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos850-cmu-core
+
+    then:
+      properties:
+        clocks:
+          items:
+            - description: External reference clock (26 MHz)
+            - description: CMU_CORE bus clock (from CMU_TOP)
+            - description: CCI clock (from CMU_TOP)
+            - description: eMMC clock (from CMU_TOP)
+            - description: SSS clock (from CMU_TOP)
+
+        clock-names:
+          items:
+            - const: oscclk
+            - const: dout_core_bus
+            - const: dout_core_cci
+            - const: dout_core_mmc_embd
+            - const: dout_core_sss
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos850-cmu-hsi
+
+    then:
+      properties:
+        clocks:
+          items:
+            - description: External reference clock (26 MHz)
+            - description: External RTC clock (32768 Hz)
+            - description: CMU_HSI bus clock (from CMU_TOP)
+            - description: SD card clock (from CMU_TOP)
+            - description: "USB 2.0 DRD clock (from CMU_TOP)"
+
+        clock-names:
+          items:
+            - const: oscclk
+            - const: rtcclk
+            - const: dout_hsi_bus
+            - const: dout_hsi_mmc_card
+            - const: dout_hsi_usb20drd
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos850-cmu-peri
+
+    then:
+      properties:
+        clocks:
+          items:
+            - description: External reference clock (26 MHz)
+            - description: CMU_PERI bus clock (from CMU_TOP)
+            - description: UART clock (from CMU_TOP)
+            - description: Parent clock for HSI2C and SPI (from CMU_TOP)
+
+        clock-names:
+          items:
+            - const: oscclk
+            - const: dout_peri_bus
+            - const: dout_peri_uart
+            - const: dout_peri_ip
+
+required:
+  - compatible
+  - "#clock-cells"
+  - clocks
+  - clock-names
+  - reg
+
+additionalProperties: false
+
+examples:
+  # Clock controller node for CMU_PERI
+  - |
+    #include <dt-bindings/clock/exynos850.h>
+
+    cmu_peri: clock-controller@10030000 {
+        compatible = "samsung,exynos850-cmu-peri";
+        reg = <0x10030000 0x8000>;
+        #clock-cells = <1>;
+
+        clocks = <&oscclk>, <&cmu_top DOUT_PERI_BUS>,
+                 <&cmu_top DOUT_PERI_UART>,
+                 <&cmu_top DOUT_PERI_IP>;
+        clock-names = "oscclk", "dout_peri_bus",
+                      "dout_peri_uart", "dout_peri_ip";
+    };
+
+  # External reference clock (should be provided in particular board DTS)
+  - |
+    oscclk: clock-oscclk {
+        compatible = "fixed-clock";
+        #clock-cells = <0>;
+        clock-output-names = "oscclk";
+        clock-frequency = <26000000>;
+    };
+
+  # UART controller node that consumes the clock generated by CMU_PERI
+  - |
+    #include <dt-bindings/clock/exynos850.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    serial_0: serial@13820000 {
+        compatible = "samsung,exynos850-uart";
+        reg = <0x13820000 0x100>;
+        interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&uart0_pins>;
+        clocks = <&cmu_peri GOUT_UART_PCLK>, <&cmu_peri GOUT_UART_IPCLK>;
+        clock-names = "uart", "clk_uart_baud0";
+    };
+
+...
-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
                   ` (4 preceding siblings ...)
  2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
@ 2021-09-14 15:56 ` Sam Protsenko
  2021-09-15  8:59   ` Krzysztof Kozlowski
                     ` (2 more replies)
  5 siblings, 3 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-14 15:56 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

This is the initial implementation adding only basic clocks like UART,
MMC, I2C and corresponding parent clocks. Design is influenced by
Exynos7 and Exynos5433 clock drivers.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/samsung/Makefile        |   1 +
 drivers/clk/samsung/clk-exynos850.c | 700 ++++++++++++++++++++++++++++
 2 files changed, 701 insertions(+)
 create mode 100644 drivers/clk/samsung/clk-exynos850.c

diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index 028b2e27a37e..c46cf11e4d0b 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos5433.o
 obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
 obj-$(CONFIG_EXYNOS_CLKOUT)	+= clk-exynos-clkout.o
 obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
+obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
 obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
 obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
 obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
new file mode 100644
index 000000000000..1028caa2102e
--- /dev/null
+++ b/drivers/clk/samsung/clk-exynos850.c
@@ -0,0 +1,700 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 Linaro Ltd.
+ * Author: Sam Protsenko <semen.protsenko@linaro.org>
+ *
+ * Common Clock Framework support for Exynos850 SoC.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <dt-bindings/clock/exynos850.h>
+
+#include "clk.h"
+
+/* Gate register bits */
+#define GATE_MANUAL		BIT(20)
+#define GATE_ENABLE_HWACG	BIT(28)
+
+/* Gate register offsets range */
+#define GATE_OFF_START		0x2000
+#define GATE_OFF_END		0x2fff
+
+/**
+ * exynos850_init_clocks - Set clocks initial configuration
+ * @np:			CMU device tree node with "reg" property (CMU addr)
+ * @reg_offs:		Register offsets array for clocks to init
+ * @reg_offs_len:	Number of register offsets in reg_offs array
+ *
+ * Set manual control mode for all gate clocks.
+ */
+static void __init exynos850_init_clocks(struct device_node *np,
+		const unsigned long *reg_offs, size_t reg_offs_len)
+{
+	const __be32 *regaddr_p;
+	u64 regaddr;
+	u32 base;
+	size_t i;
+
+	/* Get the base address ("reg" property in dts) */
+	regaddr_p = of_get_address(np, 0, NULL, NULL);
+	if (!regaddr_p)
+		panic("%s: failed to get reg regaddr\n", __func__);
+
+	regaddr = of_translate_address(np, regaddr_p);
+	if (regaddr == OF_BAD_ADDR || !regaddr)
+		panic("%s: bad reg regaddr\n", __func__);
+
+	base = (u32)regaddr;
+
+	for (i = 0; i < reg_offs_len; ++i) {
+		void __iomem *reg;
+		u32 val;
+
+		/* Modify only gate clock registers */
+		if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
+			continue;
+
+		reg = ioremap(base + reg_offs[i], 4);
+		val = ioread32(reg);
+		val |= GATE_MANUAL;
+		val &= ~GATE_ENABLE_HWACG;
+		iowrite32(val, reg);
+		iounmap(reg);
+	}
+}
+
+/* Register Offset definitions for CMU_TOP (0x120e0000) */
+#define PLL_LOCKTIME_PLL_MMC			0x0000
+#define PLL_LOCKTIME_PLL_SHARED0		0x0004
+#define PLL_LOCKTIME_PLL_SHARED1		0x0008
+#define PLL_CON0_PLL_MMC			0x0100
+#define PLL_CON3_PLL_MMC			0x010c
+#define PLL_CON0_PLL_SHARED0			0x0140
+#define PLL_CON3_PLL_SHARED0			0x014c
+#define PLL_CON0_PLL_SHARED1			0x0180
+#define PLL_CON3_PLL_SHARED1			0x018c
+#define CLK_CON_MUX_MUX_CLKCMU_CORE_BUS		0x1014
+#define CLK_CON_MUX_MUX_CLKCMU_CORE_CCI		0x1018
+#define CLK_CON_MUX_MUX_CLKCMU_CORE_MMC_EMBD	0x101c
+#define CLK_CON_MUX_MUX_CLKCMU_CORE_SSS		0x1020
+#define CLK_CON_MUX_MUX_CLKCMU_HSI_BUS		0x103c
+#define CLK_CON_MUX_MUX_CLKCMU_HSI_MMC_CARD	0x1040
+#define CLK_CON_MUX_MUX_CLKCMU_HSI_USB20DRD	0x1044
+#define CLK_CON_MUX_MUX_CLKCMU_PERI_BUS		0x1070
+#define CLK_CON_MUX_MUX_CLKCMU_PERI_IP		0x1074
+#define CLK_CON_MUX_MUX_CLKCMU_PERI_UART	0x1078
+#define CLK_CON_DIV_CLKCMU_CORE_BUS		0x1820
+#define CLK_CON_DIV_CLKCMU_CORE_CCI		0x1824
+#define CLK_CON_DIV_CLKCMU_CORE_MMC_EMBD	0x1828
+#define CLK_CON_DIV_CLKCMU_CORE_SSS		0x182c
+#define CLK_CON_DIV_CLKCMU_HSI_BUS		0x1848
+#define CLK_CON_DIV_CLKCMU_HSI_MMC_CARD		0x184c
+#define CLK_CON_DIV_CLKCMU_HSI_USB20DRD		0x1850
+#define CLK_CON_DIV_CLKCMU_PERI_BUS		0x187c
+#define CLK_CON_DIV_CLKCMU_PERI_IP		0x1880
+#define CLK_CON_DIV_CLKCMU_PERI_UART		0x1884
+#define CLK_CON_DIV_PLL_SHARED0_DIV2		0x188c
+#define CLK_CON_DIV_PLL_SHARED0_DIV3		0x1890
+#define CLK_CON_DIV_PLL_SHARED0_DIV4		0x1894
+#define CLK_CON_DIV_PLL_SHARED1_DIV2		0x1898
+#define CLK_CON_DIV_PLL_SHARED1_DIV3		0x189c
+#define CLK_CON_DIV_PLL_SHARED1_DIV4		0x18a0
+#define CLK_CON_GAT_GATE_CLKCMU_CORE_BUS	0x201c
+#define CLK_CON_GAT_GATE_CLKCMU_CORE_CCI	0x2020
+#define CLK_CON_GAT_GATE_CLKCMU_CORE_MMC_EMBD	0x2024
+#define CLK_CON_GAT_GATE_CLKCMU_CORE_SSS	0x2028
+#define CLK_CON_GAT_GATE_CLKCMU_HSI_BUS		0x2044
+#define CLK_CON_GAT_GATE_CLKCMU_HSI_MMC_CARD	0x2048
+#define CLK_CON_GAT_GATE_CLKCMU_HSI_USB20DRD	0x204c
+#define CLK_CON_GAT_GATE_CLKCMU_PERI_BUS	0x2080
+#define CLK_CON_GAT_GATE_CLKCMU_PERI_IP		0x2084
+#define CLK_CON_GAT_GATE_CLKCMU_PERI_UART	0x2088
+
+static const unsigned long top_clk_regs[] __initconst = {
+	PLL_LOCKTIME_PLL_MMC,
+	PLL_LOCKTIME_PLL_SHARED0,
+	PLL_LOCKTIME_PLL_SHARED1,
+	PLL_CON0_PLL_MMC,
+	PLL_CON3_PLL_MMC,
+	PLL_CON0_PLL_SHARED0,
+	PLL_CON3_PLL_SHARED0,
+	PLL_CON0_PLL_SHARED1,
+	PLL_CON3_PLL_SHARED1,
+	CLK_CON_MUX_MUX_CLKCMU_CORE_BUS,
+	CLK_CON_MUX_MUX_CLKCMU_CORE_CCI,
+	CLK_CON_MUX_MUX_CLKCMU_CORE_MMC_EMBD,
+	CLK_CON_MUX_MUX_CLKCMU_CORE_SSS,
+	CLK_CON_MUX_MUX_CLKCMU_HSI_BUS,
+	CLK_CON_MUX_MUX_CLKCMU_HSI_MMC_CARD,
+	CLK_CON_MUX_MUX_CLKCMU_HSI_USB20DRD,
+	CLK_CON_MUX_MUX_CLKCMU_PERI_BUS,
+	CLK_CON_MUX_MUX_CLKCMU_PERI_IP,
+	CLK_CON_MUX_MUX_CLKCMU_PERI_UART,
+	CLK_CON_DIV_CLKCMU_CORE_BUS,
+	CLK_CON_DIV_CLKCMU_CORE_CCI,
+	CLK_CON_DIV_CLKCMU_CORE_MMC_EMBD,
+	CLK_CON_DIV_CLKCMU_CORE_SSS,
+	CLK_CON_DIV_CLKCMU_HSI_BUS,
+	CLK_CON_DIV_CLKCMU_HSI_MMC_CARD,
+	CLK_CON_DIV_CLKCMU_HSI_USB20DRD,
+	CLK_CON_DIV_CLKCMU_PERI_BUS,
+	CLK_CON_DIV_CLKCMU_PERI_IP,
+	CLK_CON_DIV_CLKCMU_PERI_UART,
+	CLK_CON_DIV_PLL_SHARED0_DIV2,
+	CLK_CON_DIV_PLL_SHARED0_DIV3,
+	CLK_CON_DIV_PLL_SHARED0_DIV4,
+	CLK_CON_DIV_PLL_SHARED1_DIV2,
+	CLK_CON_DIV_PLL_SHARED1_DIV3,
+	CLK_CON_DIV_PLL_SHARED1_DIV4,
+	CLK_CON_GAT_GATE_CLKCMU_CORE_BUS,
+	CLK_CON_GAT_GATE_CLKCMU_CORE_CCI,
+	CLK_CON_GAT_GATE_CLKCMU_CORE_MMC_EMBD,
+	CLK_CON_GAT_GATE_CLKCMU_CORE_SSS,
+	CLK_CON_GAT_GATE_CLKCMU_HSI_BUS,
+	CLK_CON_GAT_GATE_CLKCMU_HSI_MMC_CARD,
+	CLK_CON_GAT_GATE_CLKCMU_HSI_USB20DRD,
+	CLK_CON_GAT_GATE_CLKCMU_PERI_BUS,
+	CLK_CON_GAT_GATE_CLKCMU_PERI_IP,
+	CLK_CON_GAT_GATE_CLKCMU_PERI_UART,
+};
+
+/*
+ * Do not provide PLL tables to core PLLs, as MANUAL_PLL_CTRL bit is not set
+ * for those PLLs by default, so set_rate operation would fail.
+ */
+static const struct samsung_pll_clock top_pll_clks[] __initconst = {
+	/* CMU_TOP_PURECLKCOMP */
+	PLL(pll_0822x, 0, "fout_shared0_pll", "oscclk",
+	    PLL_LOCKTIME_PLL_SHARED0, PLL_CON3_PLL_SHARED0,
+	    NULL),
+	PLL(pll_0822x, 0, "fout_shared1_pll", "oscclk",
+	    PLL_LOCKTIME_PLL_SHARED1, PLL_CON3_PLL_SHARED1,
+	    NULL),
+	PLL(pll_0831x, 0, "fout_mmc_pll", "oscclk",
+	    PLL_LOCKTIME_PLL_MMC, PLL_CON3_PLL_MMC, NULL),
+};
+
+/* List of parent clocks for Muxes in CMU_TOP */
+PNAME(mout_shared0_pll_p)	= { "oscclk", "fout_shared0_pll" };
+PNAME(mout_shared1_pll_p)	= { "oscclk", "fout_shared1_pll" };
+PNAME(mout_mmc_pll_p)		= { "oscclk", "fout_mmc_pll" };
+/* List of parent clocks for Muxes in CMU_TOP: for CMU_CORE */
+PNAME(mout_core_bus_p)		= { "dout_shared1_div2", "dout_shared0_div3",
+				    "dout_shared1_div3", "dout_shared0_div4" };
+PNAME(mout_core_cci_p)		= { "dout_shared0_div2", "dout_shared1_div2",
+				    "dout_shared0_div3", "dout_shared1_div3" };
+PNAME(mout_core_mmc_embd_p)	= { "oscclk", "dout_shared0_div2",
+				    "dout_shared1_div2", "dout_shared0_div3",
+				    "dout_shared1_div3", "mout_mmc_pll",
+				    "oscclk", "oscclk" };
+PNAME(mout_core_sss_p)		= { "dout_shared0_div3", "dout_shared1_div3",
+				    "dout_shared0_div4", "dout_shared1_div4" };
+/* List of parent clocks for Muxes in CMU_TOP: for CMU_HSI */
+PNAME(mout_hsi_bus_p)		= { "dout_shared0_div2", "dout_shared1_div2" };
+PNAME(mout_hsi_mmc_card_p)	= { "oscclk", "dout_shared0_div2",
+				    "dout_shared1_div2", "dout_shared0_div3",
+				    "dout_shared1_div3", "mout_mmc_pll",
+				    "oscclk", "oscclk" };
+PNAME(mout_hsi_usb20drd_p)	= { "oscclk", "dout_shared0_div4",
+				    "dout_shared1_div4", "oscclk" };
+/* List of parent clocks for Muxes in CMU_TOP: for CMU_PERI */
+PNAME(mout_peri_bus_p)		= { "dout_shared0_div4", "dout_shared1_div4" };
+PNAME(mout_peri_uart_p)		= { "oscclk", "dout_shared0_div4",
+				    "dout_shared1_div4", "oscclk" };
+PNAME(mout_peri_ip_p)		= { "oscclk", "dout_shared0_div4",
+				    "dout_shared1_div4", "oscclk" };
+
+static const struct samsung_mux_clock top_mux_clks[] __initconst = {
+	/* CMU_TOP_PURECLKCOMP */
+	MUX(0, "mout_shared0_pll", mout_shared0_pll_p,
+	    PLL_CON0_PLL_SHARED0, 4, 1),
+	MUX(0, "mout_shared1_pll", mout_shared1_pll_p,
+	    PLL_CON0_PLL_SHARED1, 4, 1),
+	MUX(0, "mout_mmc_pll", mout_mmc_pll_p,
+	    PLL_CON0_PLL_MMC, 4, 1),
+
+	/* CORE */
+	MUX(0, "mout_core_bus", mout_core_bus_p,
+	    CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),
+	MUX(0, "mout_core_cci", mout_core_cci_p,
+	    CLK_CON_MUX_MUX_CLKCMU_CORE_CCI, 0, 2),
+	MUX(0, "mout_core_mmc_embd", mout_core_mmc_embd_p,
+	    CLK_CON_MUX_MUX_CLKCMU_CORE_MMC_EMBD, 0, 3),
+	MUX(0, "mout_core_sss", mout_core_sss_p,
+	    CLK_CON_MUX_MUX_CLKCMU_CORE_SSS, 0, 2),
+
+	/* HSI */
+	MUX(0, "mout_hsi_bus", mout_hsi_bus_p,
+	    CLK_CON_MUX_MUX_CLKCMU_HSI_BUS, 0, 1),
+	MUX(0, "mout_hsi_mmc_card", mout_hsi_mmc_card_p,
+	    CLK_CON_MUX_MUX_CLKCMU_HSI_MMC_CARD, 0, 3),
+	MUX(0, "mout_hsi_usb20drd", mout_hsi_usb20drd_p,
+	    CLK_CON_MUX_MUX_CLKCMU_HSI_USB20DRD, 0, 2),
+
+	/* PERI */
+	MUX(0, "mout_peri_bus", mout_peri_bus_p,
+	    CLK_CON_MUX_MUX_CLKCMU_PERI_BUS, 0, 1),
+	MUX(0, "mout_peri_uart", mout_peri_uart_p,
+	    CLK_CON_MUX_MUX_CLKCMU_PERI_UART, 0, 2),
+	MUX(0, "mout_peri_ip", mout_peri_ip_p,
+	    CLK_CON_MUX_MUX_CLKCMU_PERI_IP, 0, 2),
+};
+
+static const struct samsung_div_clock top_div_clks[] __initconst = {
+	/* CMU_TOP_PURECLKCOMP */
+	DIV(0, "dout_shared0_div3", "mout_shared0_pll",
+	    CLK_CON_DIV_PLL_SHARED0_DIV3, 0, 2),
+	DIV(0, "dout_shared0_div2", "mout_shared0_pll",
+	    CLK_CON_DIV_PLL_SHARED0_DIV2, 0, 1),
+	DIV(0, "dout_shared1_div3", "mout_shared1_pll",
+	    CLK_CON_DIV_PLL_SHARED1_DIV3, 0, 2),
+	DIV(0, "dout_shared1_div2", "mout_shared1_pll",
+	    CLK_CON_DIV_PLL_SHARED1_DIV2, 0, 1),
+	DIV(0, "dout_shared0_div4", "dout_shared0_div2",
+	    CLK_CON_DIV_PLL_SHARED0_DIV4, 0, 1),
+	DIV(0, "dout_shared1_div4", "dout_shared1_div2",
+	    CLK_CON_DIV_PLL_SHARED1_DIV4, 0, 1),
+
+	/* CORE */
+	DIV(DOUT_CORE_BUS, "dout_core_bus", "gout_core_bus",
+	    CLK_CON_DIV_CLKCMU_CORE_BUS, 0, 4),
+	DIV(DOUT_CORE_CCI, "dout_core_cci", "gout_core_cci",
+	    CLK_CON_DIV_CLKCMU_CORE_CCI, 0, 4),
+	DIV(DOUT_CORE_MMC_EMBD, "dout_core_mmc_embd", "gout_core_mmc_embd",
+	    CLK_CON_DIV_CLKCMU_CORE_MMC_EMBD, 0, 9),
+	DIV(DOUT_CORE_SSS, "dout_core_sss", "gout_core_sss",
+	    CLK_CON_DIV_CLKCMU_CORE_SSS, 0, 4),
+
+	/* HSI */
+	DIV(DOUT_HSI_BUS, "dout_hsi_bus", "gout_hsi_bus",
+	    CLK_CON_DIV_CLKCMU_HSI_BUS, 0, 4),
+	DIV(DOUT_HSI_MMC_CARD, "dout_hsi_mmc_card", "gout_hsi_mmc_card",
+	    CLK_CON_DIV_CLKCMU_HSI_MMC_CARD, 0, 9),
+	DIV(DOUT_HSI_USB20DRD, "dout_hsi_usb20drd", "gout_hsi_usb20drd",
+	    CLK_CON_DIV_CLKCMU_HSI_USB20DRD, 0, 4),
+
+	/* PERI */
+	DIV(DOUT_PERI_BUS, "dout_peri_bus", "gout_peri_bus",
+	    CLK_CON_DIV_CLKCMU_PERI_BUS, 0, 4),
+	DIV(DOUT_PERI_UART, "dout_peri_uart", "gout_peri_uart",
+	    CLK_CON_DIV_CLKCMU_PERI_UART, 0, 4),
+	DIV(DOUT_PERI_IP, "dout_peri_ip", "gout_peri_ip",
+	    CLK_CON_DIV_CLKCMU_PERI_IP, 0, 4),
+};
+
+static const struct samsung_gate_clock top_gate_clks[] __initconst = {
+	/* CORE */
+	GATE(0, "gout_core_bus", "mout_core_bus",
+	     CLK_CON_GAT_GATE_CLKCMU_CORE_BUS, 21, 0, 0),
+	GATE(0, "gout_core_cci", "mout_core_cci",
+	     CLK_CON_GAT_GATE_CLKCMU_CORE_CCI, 21, 0, 0),
+	GATE(0, "gout_core_mmc_embd", "mout_core_mmc_embd",
+	     CLK_CON_GAT_GATE_CLKCMU_CORE_MMC_EMBD, 21, 0, 0),
+	GATE(0, "gout_core_sss", "mout_core_sss",
+	     CLK_CON_GAT_GATE_CLKCMU_CORE_SSS, 21, 0, 0),
+
+	/* HSI */
+	GATE(0, "gout_hsi_bus", "mout_hsi_bus",
+	     CLK_CON_GAT_GATE_CLKCMU_HSI_BUS, 21, 0, 0),
+	GATE(0, "gout_hsi_mmc_card", "mout_hsi_mmc_card",
+	     CLK_CON_GAT_GATE_CLKCMU_HSI_MMC_CARD, 21, 0, 0),
+	GATE(0, "gout_hsi_usb20drd", "mout_hsi_usb20drd",
+	     CLK_CON_GAT_GATE_CLKCMU_HSI_USB20DRD, 21, 0, 0),
+
+	/* PERI */
+	GATE(0, "gout_peri_bus", "mout_peri_bus",
+	     CLK_CON_GAT_GATE_CLKCMU_PERI_BUS, 21, 0, 0),
+	GATE(0, "gout_peri_uart", "mout_peri_uart",
+	     CLK_CON_GAT_GATE_CLKCMU_PERI_UART, 21, 0, 0),
+	GATE(0, "gout_peri_ip", "mout_peri_ip",
+	     CLK_CON_GAT_GATE_CLKCMU_PERI_IP, 21, 0, 0),
+};
+
+static const struct samsung_cmu_info top_cmu_info __initconst = {
+	.pll_clks		= top_pll_clks,
+	.nr_pll_clks		= ARRAY_SIZE(top_pll_clks),
+	.mux_clks		= top_mux_clks,
+	.nr_mux_clks		= ARRAY_SIZE(top_mux_clks),
+	.div_clks		= top_div_clks,
+	.nr_div_clks		= ARRAY_SIZE(top_div_clks),
+	.gate_clks		= top_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(top_gate_clks),
+	.nr_clk_ids		= TOP_NR_CLK,
+	.clk_regs		= top_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(top_clk_regs),
+};
+
+static void __init exynos850_cmu_top_init(struct device_node *np)
+{
+	exynos850_init_clocks(np, top_clk_regs, ARRAY_SIZE(top_clk_regs));
+	samsung_cmu_register_one(np, &top_cmu_info);
+}
+
+CLK_OF_DECLARE(exynos850_cmu_top, "samsung,exynos850-cmu-top",
+	       exynos850_cmu_top_init);
+
+/* Register Offset definitions for CMU_HSI (0x13400000) */
+#define PLL_CON0_MUX_CLKCMU_HSI_BUS_USER			0x0600
+#define PLL_CON0_MUX_CLKCMU_HSI_MMC_CARD_USER			0x0610
+#define PLL_CON0_MUX_CLKCMU_HSI_USB20DRD_USER			0x0620
+#define CLK_CON_MUX_MUX_CLK_HSI_RTC				0x1000
+#define CLK_CON_GAT_HSI_USB20DRD_TOP_I_RTC_CLK__ALV		0x2008
+#define CLK_CON_GAT_HSI_USB20DRD_TOP_I_REF_CLK_50		0x200c
+#define CLK_CON_GAT_HSI_USB20DRD_TOP_I_PHY_REFCLK_26		0x2010
+#define CLK_CON_GAT_GOUT_HSI_GPIO_HSI_PCLK			0x2018
+#define CLK_CON_GAT_GOUT_HSI_MMC_CARD_I_ACLK			0x2024
+#define CLK_CON_GAT_GOUT_HSI_MMC_CARD_SDCLKIN			0x2028
+#define CLK_CON_GAT_GOUT_HSI_SYSREG_HSI_PCLK			0x2038
+#define CLK_CON_GAT_GOUT_HSI_USB20DRD_TOP_ACLK_PHYCTRL_20	0x203c
+#define CLK_CON_GAT_GOUT_HSI_USB20DRD_TOP_BUS_CLK_EARLY		0x2040
+
+static const unsigned long hsi_clk_regs[] __initconst = {
+	PLL_CON0_MUX_CLKCMU_HSI_BUS_USER,
+	PLL_CON0_MUX_CLKCMU_HSI_MMC_CARD_USER,
+	PLL_CON0_MUX_CLKCMU_HSI_USB20DRD_USER,
+	CLK_CON_MUX_MUX_CLK_HSI_RTC,
+	CLK_CON_GAT_HSI_USB20DRD_TOP_I_RTC_CLK__ALV,
+	CLK_CON_GAT_HSI_USB20DRD_TOP_I_REF_CLK_50,
+	CLK_CON_GAT_HSI_USB20DRD_TOP_I_PHY_REFCLK_26,
+	CLK_CON_GAT_GOUT_HSI_GPIO_HSI_PCLK,
+	CLK_CON_GAT_GOUT_HSI_MMC_CARD_I_ACLK,
+	CLK_CON_GAT_GOUT_HSI_MMC_CARD_SDCLKIN,
+	CLK_CON_GAT_GOUT_HSI_SYSREG_HSI_PCLK,
+	CLK_CON_GAT_GOUT_HSI_USB20DRD_TOP_ACLK_PHYCTRL_20,
+	CLK_CON_GAT_GOUT_HSI_USB20DRD_TOP_BUS_CLK_EARLY,
+};
+
+/* List of parent clocks for Muxes in CMU_PERI */
+PNAME(mout_hsi_bus_user_p)	= { "oscclk", "dout_hsi_bus" };
+PNAME(mout_hsi_mmc_card_user_p)	= { "oscclk", "dout_hsi_mmc_card" };
+PNAME(mout_hsi_usb20drd_user_p)	= { "oscclk", "dout_hsi_usb20drd" };
+PNAME(mout_hsi_rtc_p)		= { "rtcclk", "oscclk" };
+
+static const struct samsung_mux_clock hsi_mux_clks[] __initconst = {
+	MUX(0, "mout_hsi_bus_user", mout_hsi_bus_user_p,
+	    PLL_CON0_MUX_CLKCMU_HSI_BUS_USER, 4, 1),
+	MUX_F(0, "mout_hsi_mmc_card_user", mout_hsi_mmc_card_user_p,
+	      PLL_CON0_MUX_CLKCMU_HSI_MMC_CARD_USER, 4, 1,
+	      CLK_SET_RATE_PARENT, 0),
+	MUX(0, "mout_hsi_usb20drd_user", mout_hsi_usb20drd_user_p,
+	    PLL_CON0_MUX_CLKCMU_HSI_USB20DRD_USER, 4, 1),
+	MUX(0, "mout_hsi_rtc", mout_hsi_rtc_p,
+	    CLK_CON_MUX_MUX_CLK_HSI_RTC, 0, 1),
+};
+
+static const struct samsung_gate_clock hsi_gate_clks[] __initconst = {
+	GATE(GOUT_USB_RTC_CLK, "gout_usb_rtc", "mout_hsi_rtc",
+	     CLK_CON_GAT_HSI_USB20DRD_TOP_I_RTC_CLK__ALV, 21, 0, 0),
+	GATE(GOUT_USB_REF_CLK, "gout_usb_ref", "mout_hsi_usb20drd_user",
+	     CLK_CON_GAT_HSI_USB20DRD_TOP_I_REF_CLK_50, 21, 0, 0),
+	GATE(GOUT_USB_PHY_REF_CLK, "gout_usb_phy_ref", "oscclk",
+	     CLK_CON_GAT_HSI_USB20DRD_TOP_I_PHY_REFCLK_26, 21, 0, 0),
+	GATE(GOUT_GPIO_HSI_PCLK, "gout_gpio_hsi_pclk", "mout_hsi_bus_user",
+	     CLK_CON_GAT_GOUT_HSI_GPIO_HSI_PCLK, 21, 0, 0),
+	GATE(GOUT_MMC_CARD_ACLK, "gout_mmc_card_aclk", "mout_hsi_bus_user",
+	     CLK_CON_GAT_GOUT_HSI_MMC_CARD_I_ACLK, 21, 0, 0),
+	GATE(GOUT_MMC_CARD_SDCLKIN, "gout_mmc_card_sdclkin",
+				    "mout_hsi_mmc_card_user",
+	     CLK_CON_GAT_GOUT_HSI_MMC_CARD_SDCLKIN, 21, CLK_SET_RATE_PARENT, 0),
+	GATE(GOUT_SYSREG_HSI_PCLK, "gout_sysreg_hsi_pclk", "mout_hsi_bus_user",
+	     CLK_CON_GAT_GOUT_HSI_SYSREG_HSI_PCLK, 21, 0, 0),
+	GATE(GOUT_USB_PHY_ACLK, "gout_usb_phy_aclk", "mout_hsi_bus_user",
+	     CLK_CON_GAT_GOUT_HSI_USB20DRD_TOP_ACLK_PHYCTRL_20, 21, 0, 0),
+	GATE(GOUT_USB_BUS_EARLY_CLK, "gout_usb_bus_early", "mout_hsi_bus_user",
+	     CLK_CON_GAT_GOUT_HSI_USB20DRD_TOP_BUS_CLK_EARLY, 21, 0, 0),
+};
+
+static const struct samsung_cmu_info hsi_cmu_info __initconst = {
+	.mux_clks		= hsi_mux_clks,
+	.nr_mux_clks		= ARRAY_SIZE(hsi_mux_clks),
+	.gate_clks		= hsi_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(hsi_gate_clks),
+	.nr_clk_ids		= HSI_NR_CLK,
+	.clk_regs		= hsi_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(hsi_clk_regs),
+	.clk_name		= "dout_hsi_bus",
+};
+
+static void __init exynos850_cmu_hsi_init(struct device_node *np)
+{
+	exynos850_init_clocks(np, hsi_clk_regs, ARRAY_SIZE(hsi_clk_regs));
+	samsung_cmu_register_one(np, &hsi_cmu_info);
+}
+
+CLK_OF_DECLARE(exynos850_cmu_hsi, "samsung,exynos850-cmu-hsi",
+	       exynos850_cmu_hsi_init);
+
+/* Register Offset definitions for CMU_PERI (0x10030000) */
+#define PLL_CON0_MUX_CLKCMU_PERI_BUS_USER	0x0600
+#define PLL_CON0_MUX_CLKCMU_PERI_HSI2C_USER	0x0610
+#define PLL_CON0_MUX_CLKCMU_PERI_SPI_USER	0x0620
+#define PLL_CON0_MUX_CLKCMU_PERI_UART_USER	0x0630
+#define CLK_CON_DIV_DIV_CLK_PERI_HSI2C_0	0x1800
+#define CLK_CON_DIV_DIV_CLK_PERI_HSI2C_1	0x1804
+#define CLK_CON_DIV_DIV_CLK_PERI_HSI2C_2	0x1808
+#define CLK_CON_DIV_DIV_CLK_PERI_SPI_0		0x180c
+#define CLK_CON_GAT_GATE_CLK_PERI_HSI2C_0	0x200c
+#define CLK_CON_GAT_GATE_CLK_PERI_HSI2C_1	0x2010
+#define CLK_CON_GAT_GATE_CLK_PERI_HSI2C_2	0x2014
+#define CLK_CON_GAT_GOUT_PERI_GPIO_PERI_PCLK	0x2020
+#define CLK_CON_GAT_GOUT_PERI_HSI2C_0_IPCLK	0x2024
+#define CLK_CON_GAT_GOUT_PERI_HSI2C_0_PCLK	0x2028
+#define CLK_CON_GAT_GOUT_PERI_HSI2C_1_IPCLK	0x202c
+#define CLK_CON_GAT_GOUT_PERI_HSI2C_1_PCLK	0x2030
+#define CLK_CON_GAT_GOUT_PERI_HSI2C_2_IPCLK	0x2034
+#define CLK_CON_GAT_GOUT_PERI_HSI2C_2_PCLK	0x2038
+#define CLK_CON_GAT_GOUT_PERI_I2C_0_PCLK	0x203c
+#define CLK_CON_GAT_GOUT_PERI_I2C_1_PCLK	0x2040
+#define CLK_CON_GAT_GOUT_PERI_I2C_2_PCLK	0x2044
+#define CLK_CON_GAT_GOUT_PERI_I2C_3_PCLK	0x2048
+#define CLK_CON_GAT_GOUT_PERI_I2C_4_PCLK	0x204c
+#define CLK_CON_GAT_GOUT_PERI_I2C_5_PCLK	0x2050
+#define CLK_CON_GAT_GOUT_PERI_I2C_6_PCLK	0x2054
+#define CLK_CON_GAT_GOUT_PERI_MCT_PCLK		0x205c
+#define CLK_CON_GAT_GOUT_PERI_PWM_MOTOR_PCLK	0x2064
+#define CLK_CON_GAT_GOUT_PERI_SPI_0_IPCLK	0x209c
+#define CLK_CON_GAT_GOUT_PERI_SPI_0_PCLK	0x20a0
+#define CLK_CON_GAT_GOUT_PERI_SYSREG_PERI_PCLK	0x20a4
+#define CLK_CON_GAT_GOUT_PERI_UART_IPCLK	0x20a8
+#define CLK_CON_GAT_GOUT_PERI_UART_PCLK		0x20ac
+#define CLK_CON_GAT_GOUT_PERI_WDT_0_PCLK	0x20b0
+#define CLK_CON_GAT_GOUT_PERI_WDT_1_PCLK	0x20b4
+
+static const unsigned long peri_clk_regs[] __initconst = {
+	PLL_CON0_MUX_CLKCMU_PERI_BUS_USER,
+	PLL_CON0_MUX_CLKCMU_PERI_HSI2C_USER,
+	PLL_CON0_MUX_CLKCMU_PERI_SPI_USER,
+	PLL_CON0_MUX_CLKCMU_PERI_UART_USER,
+	CLK_CON_DIV_DIV_CLK_PERI_HSI2C_0,
+	CLK_CON_DIV_DIV_CLK_PERI_HSI2C_1,
+	CLK_CON_DIV_DIV_CLK_PERI_HSI2C_2,
+	CLK_CON_DIV_DIV_CLK_PERI_SPI_0,
+	CLK_CON_GAT_GATE_CLK_PERI_HSI2C_0,
+	CLK_CON_GAT_GATE_CLK_PERI_HSI2C_1,
+	CLK_CON_GAT_GATE_CLK_PERI_HSI2C_2,
+	CLK_CON_GAT_GOUT_PERI_GPIO_PERI_PCLK,
+	CLK_CON_GAT_GOUT_PERI_HSI2C_0_IPCLK,
+	CLK_CON_GAT_GOUT_PERI_HSI2C_0_PCLK,
+	CLK_CON_GAT_GOUT_PERI_HSI2C_1_IPCLK,
+	CLK_CON_GAT_GOUT_PERI_HSI2C_1_PCLK,
+	CLK_CON_GAT_GOUT_PERI_HSI2C_2_IPCLK,
+	CLK_CON_GAT_GOUT_PERI_HSI2C_2_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_0_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_1_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_2_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_3_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_4_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_5_PCLK,
+	CLK_CON_GAT_GOUT_PERI_I2C_6_PCLK,
+	CLK_CON_GAT_GOUT_PERI_MCT_PCLK,
+	CLK_CON_GAT_GOUT_PERI_PWM_MOTOR_PCLK,
+	CLK_CON_GAT_GOUT_PERI_SPI_0_IPCLK,
+	CLK_CON_GAT_GOUT_PERI_SPI_0_PCLK,
+	CLK_CON_GAT_GOUT_PERI_SYSREG_PERI_PCLK,
+	CLK_CON_GAT_GOUT_PERI_UART_IPCLK,
+	CLK_CON_GAT_GOUT_PERI_UART_PCLK,
+	CLK_CON_GAT_GOUT_PERI_WDT_0_PCLK,
+	CLK_CON_GAT_GOUT_PERI_WDT_1_PCLK,
+};
+
+/* List of parent clocks for Muxes in CMU_PERI */
+PNAME(mout_peri_bus_user_p)	= { "oscclk", "dout_peri_bus" };
+PNAME(mout_peri_uart_user_p)	= { "oscclk", "dout_peri_uart" };
+PNAME(mout_peri_hsi2c_user_p)	= { "oscclk", "dout_peri_ip" };
+PNAME(mout_peri_spi_user_p)	= { "oscclk", "dout_peri_ip" };
+
+static const struct samsung_mux_clock peri_mux_clks[] __initconst = {
+	MUX(0, "mout_peri_bus_user", mout_peri_bus_user_p,
+	    PLL_CON0_MUX_CLKCMU_PERI_BUS_USER, 4, 1),
+	MUX(0, "mout_peri_uart_user", mout_peri_uart_user_p,
+	    PLL_CON0_MUX_CLKCMU_PERI_UART_USER, 4, 1),
+	MUX(0, "mout_peri_hsi2c_user", mout_peri_hsi2c_user_p,
+	    PLL_CON0_MUX_CLKCMU_PERI_HSI2C_USER, 4, 1),
+	MUX(0, "mout_peri_spi_user", mout_peri_spi_user_p,
+	    PLL_CON0_MUX_CLKCMU_PERI_SPI_USER, 4, 1),
+};
+
+static const struct samsung_div_clock peri_div_clks[] __initconst = {
+	DIV(0, "dout_peri_hsi2c0", "gout_peri_hsi2c0",
+	    CLK_CON_DIV_DIV_CLK_PERI_HSI2C_0, 0, 5),
+	DIV(0, "dout_peri_hsi2c1", "gout_peri_hsi2c1",
+	    CLK_CON_DIV_DIV_CLK_PERI_HSI2C_1, 0, 5),
+	DIV(0, "dout_peri_hsi2c2", "gout_peri_hsi2c2",
+	    CLK_CON_DIV_DIV_CLK_PERI_HSI2C_2, 0, 5),
+	DIV(0, "dout_peri_spi0", "mout_peri_spi_user",
+	    CLK_CON_DIV_DIV_CLK_PERI_SPI_0, 0, 5),
+};
+
+static const struct samsung_gate_clock peri_gate_clks[] __initconst = {
+	GATE(0, "gout_peri_hsi2c0", "mout_peri_hsi2c_user",
+	     CLK_CON_GAT_GATE_CLK_PERI_HSI2C_0, 21, 0, 0),
+	GATE(0, "gout_peri_hsi2c1", "mout_peri_hsi2c_user",
+	     CLK_CON_GAT_GATE_CLK_PERI_HSI2C_1, 21, 0, 0),
+	GATE(0, "gout_peri_hsi2c2", "mout_peri_hsi2c_user",
+	     CLK_CON_GAT_GATE_CLK_PERI_HSI2C_2, 21, 0, 0),
+	GATE(GOUT_HSI2C0_IPCLK, "gout_hsi2c0_ipclk", "dout_peri_hsi2c0",
+	     CLK_CON_GAT_GOUT_PERI_HSI2C_0_IPCLK, 21, 0, 0),
+	GATE(GOUT_HSI2C0_PCLK, "gout_hsi2c0_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_HSI2C_0_PCLK, 21, 0, 0),
+	GATE(GOUT_HSI2C1_IPCLK, "gout_hsi2c1_ipclk", "dout_peri_hsi2c1",
+	     CLK_CON_GAT_GOUT_PERI_HSI2C_1_IPCLK, 21, 0, 0),
+	GATE(GOUT_HSI2C1_PCLK, "gout_hsi2c1_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_HSI2C_1_PCLK, 21, 0, 0),
+	GATE(GOUT_HSI2C2_IPCLK, "gout_hsi2c2_ipclk", "dout_peri_hsi2c2",
+	     CLK_CON_GAT_GOUT_PERI_HSI2C_2_IPCLK, 21, 0, 0),
+	GATE(GOUT_HSI2C2_PCLK, "gout_hsi2c2_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_HSI2C_2_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C0_PCLK, "gout_i2c0_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_0_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C1_PCLK, "gout_i2c1_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_1_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C2_PCLK, "gout_i2c2_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_2_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C3_PCLK, "gout_i2c3_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_3_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C4_PCLK, "gout_i2c4_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_4_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C5_PCLK, "gout_i2c5_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_5_PCLK, 21, 0, 0),
+	GATE(GOUT_I2C6_PCLK, "gout_i2c6_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_I2C_6_PCLK, 21, 0, 0),
+	GATE(GOUT_MCT_PCLK, "gout_mct_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_MCT_PCLK, 21, 0, 0),
+	GATE(GOUT_PWM_MOTOR_PCLK, "gout_pwm_motor_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_PWM_MOTOR_PCLK, 21, 0, 0),
+	GATE(GOUT_SPI0_IPCLK, "gout_spi0_ipclk", "dout_peri_spi0",
+	     CLK_CON_GAT_GOUT_PERI_SPI_0_IPCLK, 21, 0, 0),
+	GATE(GOUT_SPI0_PCLK, "gout_spi0_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_SPI_0_PCLK, 21, 0, 0),
+	GATE(GOUT_SYSREG_PERI_PCLK, "gout_sysreg_peri_pclk",
+				    "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_SYSREG_PERI_PCLK, 21, 0, 0),
+	GATE(GOUT_UART_IPCLK, "gout_uart_ipclk", "mout_peri_uart_user",
+	     CLK_CON_GAT_GOUT_PERI_UART_IPCLK, 21, 0, 0),
+	GATE(GOUT_UART_PCLK, "gout_uart_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_UART_PCLK, 21, 0, 0),
+	GATE(GOUT_WDT0_PCLK, "gout_wdt0_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_WDT_0_PCLK, 21, 0, 0),
+	GATE(GOUT_WDT1_PCLK, "gout_wdt1_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_WDT_1_PCLK, 21, 0, 0),
+	GATE(GOUT_GPIO_PERI_PCLK, "gout_gpio_peri_pclk", "mout_peri_bus_user",
+	     CLK_CON_GAT_GOUT_PERI_GPIO_PERI_PCLK, 21, 0, 0),
+};
+
+static const struct samsung_cmu_info peri_cmu_info __initconst = {
+	.mux_clks		= peri_mux_clks,
+	.nr_mux_clks		= ARRAY_SIZE(peri_mux_clks),
+	.div_clks		= peri_div_clks,
+	.nr_div_clks		= ARRAY_SIZE(peri_div_clks),
+	.gate_clks		= peri_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(peri_gate_clks),
+	.nr_clk_ids		= PERI_NR_CLK,
+	.clk_regs		= peri_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(peri_clk_regs),
+	.clk_name		= "dout_peri_bus",
+};
+
+static void __init exynos850_cmu_peri_init(struct device_node *np)
+{
+	exynos850_init_clocks(np, peri_clk_regs, ARRAY_SIZE(peri_clk_regs));
+	samsung_cmu_register_one(np, &peri_cmu_info);
+}
+
+CLK_OF_DECLARE(exynos850_cmu_peri, "samsung,exynos850-cmu-peri",
+	       exynos850_cmu_peri_init);
+
+/* Register Offset definitions for CMU_CORE (0x12000000) */
+#define PLL_CON0_MUX_CLKCMU_CORE_BUS_USER	0x0600
+#define PLL_CON0_MUX_CLKCMU_CORE_CCI_USER	0x0610
+#define PLL_CON0_MUX_CLKCMU_CORE_MMC_EMBD_USER	0x0620
+#define PLL_CON0_MUX_CLKCMU_CORE_SSS_USER	0x0630
+#define CLK_CON_MUX_MUX_CLK_CORE_GIC		0x1000
+#define CLK_CON_DIV_DIV_CLK_CORE_BUSP		0x1800
+#define CLK_CON_GAT_GOUT_CORE_CCI_550_ACLK	0x2038
+#define CLK_CON_GAT_GOUT_CORE_GIC_CLK		0x2040
+#define CLK_CON_GAT_GOUT_CORE_MMC_EMBD_I_ACLK	0x20e8
+#define CLK_CON_GAT_GOUT_CORE_MMC_EMBD_SDCLKIN	0x20ec
+#define CLK_CON_GAT_GOUT_CORE_SSS_I_ACLK	0x2128
+#define CLK_CON_GAT_GOUT_CORE_SSS_I_PCLK	0x212c
+
+static const unsigned long core_clk_regs[] __initconst = {
+	PLL_CON0_MUX_CLKCMU_CORE_BUS_USER,
+	PLL_CON0_MUX_CLKCMU_CORE_CCI_USER,
+	PLL_CON0_MUX_CLKCMU_CORE_MMC_EMBD_USER,
+	PLL_CON0_MUX_CLKCMU_CORE_SSS_USER,
+	CLK_CON_MUX_MUX_CLK_CORE_GIC,
+	CLK_CON_DIV_DIV_CLK_CORE_BUSP,
+	CLK_CON_GAT_GOUT_CORE_CCI_550_ACLK,
+	CLK_CON_GAT_GOUT_CORE_GIC_CLK,
+	CLK_CON_GAT_GOUT_CORE_MMC_EMBD_I_ACLK,
+	CLK_CON_GAT_GOUT_CORE_MMC_EMBD_SDCLKIN,
+	CLK_CON_GAT_GOUT_CORE_SSS_I_ACLK,
+	CLK_CON_GAT_GOUT_CORE_SSS_I_PCLK,
+};
+
+/* List of parent clocks for Muxes in CMU_CORE */
+PNAME(mout_core_bus_user_p)		= { "oscclk", "dout_core_bus" };
+PNAME(mout_core_cci_user_p)		= { "oscclk", "dout_core_cci" };
+PNAME(mout_core_mmc_embd_user_p)	= { "oscclk", "dout_core_mmc_embd" };
+PNAME(mout_core_sss_user_p)		= { "oscclk", "dout_core_sss" };
+PNAME(mout_core_gic_p)			= { "dout_core_busp", "oscclk" };
+
+static const struct samsung_mux_clock core_mux_clks[] __initconst = {
+	MUX(0, "mout_core_bus_user", mout_core_bus_user_p,
+	    PLL_CON0_MUX_CLKCMU_CORE_BUS_USER, 4, 1),
+	MUX(0, "mout_core_cci_user", mout_core_cci_user_p,
+	    PLL_CON0_MUX_CLKCMU_CORE_CCI_USER, 4, 1),
+	MUX_F(0, "mout_core_mmc_embd_user", mout_core_mmc_embd_user_p,
+	      PLL_CON0_MUX_CLKCMU_CORE_MMC_EMBD_USER, 4, 1,
+	      CLK_SET_RATE_PARENT, 0),
+	MUX(0, "mout_core_sss_user", mout_core_sss_user_p,
+	    PLL_CON0_MUX_CLKCMU_CORE_SSS_USER, 4, 1),
+	MUX(0, "mout_core_gic", mout_core_gic_p,
+	    CLK_CON_MUX_MUX_CLK_CORE_GIC, 0, 1),
+};
+
+static const struct samsung_div_clock core_div_clks[] __initconst = {
+	DIV(0, "dout_core_busp", "mout_core_bus_user",
+	    CLK_CON_DIV_DIV_CLK_CORE_BUSP, 0, 2),
+};
+
+static const struct samsung_gate_clock core_gate_clks[] __initconst = {
+	GATE(GOUT_CCI_ACLK, "gout_cci_aclk", "mout_core_cci_user",
+	     CLK_CON_GAT_GOUT_CORE_CCI_550_ACLK, 21, 0, 0),
+	GATE(GOUT_GIC_CLK, "gout_gic_clk", "mout_core_gic",
+	     CLK_CON_GAT_GOUT_CORE_GIC_CLK, 21, 0, 0),
+	GATE(GOUT_MMC_EMBD_ACLK, "gout_mmc_embd_aclk", "dout_core_busp",
+	     CLK_CON_GAT_GOUT_CORE_MMC_EMBD_I_ACLK, 21, 0, 0),
+	GATE(GOUT_MMC_EMBD_SDCLKIN, "gout_mmc_embd_sdclkin",
+	     "mout_core_mmc_embd_user", CLK_CON_GAT_GOUT_CORE_MMC_EMBD_SDCLKIN,
+	     21, CLK_SET_RATE_PARENT, 0),
+	GATE(GOUT_SSS_ACLK, "gout_sss_aclk", "mout_core_sss_user",
+	     CLK_CON_GAT_GOUT_CORE_SSS_I_ACLK, 21, 0, 0),
+	GATE(GOUT_SSS_PCLK, "gout_sss_pclk", "dout_core_busp",
+	     CLK_CON_GAT_GOUT_CORE_SSS_I_PCLK, 21, 0, 0),
+};
+
+static const struct samsung_cmu_info core_cmu_info __initconst = {
+	.mux_clks		= core_mux_clks,
+	.nr_mux_clks		= ARRAY_SIZE(core_mux_clks),
+	.div_clks		= core_div_clks,
+	.nr_div_clks		= ARRAY_SIZE(core_div_clks),
+	.gate_clks		= core_gate_clks,
+	.nr_gate_clks		= ARRAY_SIZE(core_gate_clks),
+	.nr_clk_ids		= CORE_NR_CLK,
+	.clk_regs		= core_clk_regs,
+	.nr_clk_regs		= ARRAY_SIZE(core_clk_regs),
+	.clk_name		= "dout_core_bus",
+};
+
+static void __init exynos850_cmu_core_init(struct device_node *np)
+{
+	exynos850_init_clocks(np, core_clk_regs, ARRAY_SIZE(core_clk_regs));
+	samsung_cmu_register_one(np, &core_cmu_info);
+}
+
+CLK_OF_DECLARE(exynos850_cmu_core, "samsung,exynos850-cmu-core",
+	       exynos850_cmu_core_init);
-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings
  2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
@ 2021-09-14 21:35   ` Rob Herring
  2021-09-15  8:28   ` Krzysztof Kozlowski
  2021-09-15 16:47   ` Chanwoo Choi
  2 siblings, 0 replies; 38+ messages in thread
From: Rob Herring @ 2021-09-14 21:35 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Tom Gall, devicetree, Stephen Boyd, linux-clk, Tomasz Figa,
	Michael Turquette, Amit Pundir, Sumit Semwal, John Stultz,
	linux-arm-kernel, linux-kernel, Chanwoo Choi, Ryu Euiyoul,
	Rob Herring, linux-samsung-soc, Krzysztof Kozlowski,
	Paweł Chmiel, Sylwester Nawrocki

On Tue, 14 Sep 2021 18:56:06 +0300, Sam Protsenko wrote:
> Provide dt-schema documentation for Exynos850 SoC clock controller.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  .../clock/samsung,exynos850-clock.yaml        | 190 ++++++++++++++++++
>  1 file changed, 190 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/clock/samsung,exynos850-clock.example.dt.yaml:0:0: /example-2/serial@13820000: failed to match any schema with compatible: ['samsung,exynos850-uart']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1528063

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
@ 2021-09-15  8:21   ` Krzysztof Kozlowski
  2021-10-06 10:46     ` Sam Protsenko
  2021-09-15 12:51   ` Sylwester Nawrocki
  1 sibling, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-15  8:21 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 14/09/2021 17:56, Sam Protsenko wrote:
> By default if bus clock has no users its "enable count" value is 0. It
> might be actually running if it's already enabled in bootloader, but
> then in some cases it can be disabled by mistake. For example, such case
> was observed when dw_mci_probe() enabled bus clock, then failed to do
> something and disabled that bus clock on error path. After that even
> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
> CMU bus clock ended up being disabled and it wasn't possible to access
> CMU registers anymore.
> 
> To avoid such cases, CMU driver must increment the ref count for that
> bus clock by running clk_prepare_enable(). There is already existing
> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> support for runtime PM"). But the clock is actually enabled only in
> Exynos5433 clock driver. Let's mimic what is done there in generic
> samsung_cmu_register_one() function, so other drivers can benefit from
> that `.clk_name' field. As was described above, it might be helpful not
> only for PM reasons, but also to prevent possible erroneous clock gating
> on error paths.
> 
> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
> flag for corresponding gate clocks. But that might be not very good
> design decision, as we might still want to disable that bus clock, e.g.
> on PM suspend.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/samsung/clk.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 1949ae7851b2..da65149fa502 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>  
>  	ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
>  
> +	/* Keep bus clock running, so it's possible to access CMU registers */
> +	if (cmu->clk_name) {
> +		struct clk *bus_clk;
> +
> +		bus_clk = __clk_lookup(cmu->clk_name);
> +		if (bus_clk) {
> +			clk_prepare_enable(bus_clk);
> +		} else {
> +			pr_err("%s: could not find bus clock %s\n", __func__,
> +			       cmu->clk_name);
> +		}
> +	}
> +

Solving this problem in generic way makes sense but your solution is
insufficient. You skipped suspend/resume paths and in such case you
should remove the Exynos5433-specific code.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type
  2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko
@ 2021-09-15  8:24   ` Krzysztof Kozlowski
  2021-09-15 15:59   ` Chanwoo Choi
  1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-15  8:24 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 14/09/2021 17:56, Sam Protsenko wrote:
> pll0822x PLL is used in Exynos850 SoC for top-level integer PLLs. The
> code was derived from very similar pll35xx type, with next differences:
> 
> 1. Lock time for pll0822x is 150*P_DIV, when for pll35xx it's 270*P_DIV
> 2. It's not suggested in Exynos850 TRM that S_DIV change doesn't require
>    performing PLL lock procedure (which is done in pll35xx
>    implementation)
> 
> When defining pll0822x type, CON3 register offset should be provided as
> a "con" parameter of PLL() macro, like this:
> 
>     PLL(pll_0822x, 0, "fout_shared0_pll", "oscclk",
>         PLL_LOCKTIME_PLL_SHARED0, PLL_CON3_PLL_SHARED0,
>         exynos850_shared0_pll_rates),
> 
> To define PLL rates table, one can use PLL_35XX_RATE() macro, e.g.:
> 
>     PLL_35XX_RATE(26 * MHZ, 1600 * MHZ, 800, 13, 0)
> 
> as it's completely appropriate for pl0822x type and there is no sense in
> duplicating that.
> 
> If bit #1 (MANUAL_PLL_CTRL) is not set in CON1 register, it won't be
> possible to set new rate, with next error showing in kernel log:
> 
>     Could not lock PLL fout_shared1_pll
> 
> That can happen for example if bootloader clears that bit beforehand.
> PLL driver doesn't account for that, so if MANUAL_PLL_CTRL bit was
> cleared, it's assumed it was done for a reason and it shouldn't be
> possible to change that PLL's rate at all.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/samsung/clk-pll.c | 91 +++++++++++++++++++++++++++++++++++
>  drivers/clk/samsung/clk-pll.h |  1 +
>  2 files changed, 92 insertions(+)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x PLL type
  2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko
@ 2021-09-15  8:26   ` Krzysztof Kozlowski
  2021-09-15 16:11   ` Chanwoo Choi
  1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-15  8:26 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 14/09/2021 17:56, Sam Protsenko wrote:
> pll0831x PLL is used in Exynos850 SoC for top-level fractional PLLs. The
> code was derived from very similar pll36xx type, with next differences:
> 
> 1. Lock time for pll0831x is 500*P_DIV, when for pll36xx it's 3000*P_DIV
> 2. It's not suggested in Exynos850 TRM that S_DIV change doesn't require
>    performing PLL lock procedure (which is done in pll36xx
>    implementation)
> 3. The offset from PMS-values register to K-value register is 0x8 for
>    pll0831x, when for pll36xx it's 0x4
> 
> When defining pll0831x type, CON3 register offset should be provided as
> a "con" parameter of PLL() macro, like this:
> 
>     PLL(pll_0831x, 0, "fout_mmc_pll", "oscclk",
>         PLL_LOCKTIME_PLL_MMC, PLL_CON3_PLL_MMC, pll0831x_26mhz_tbl),
> 
> To define PLL rates table, one can use PLL_36XX_RATE() macro, e.g.:
> 
>     PLL_36XX_RATE(26 * MHZ, 799999877, 31, 1, 0, -15124)
> 
> as it's completely appropriate for pl0831x type and there is no sense in
> duplicating that.
> 
> If bit #1 (MANUAL_PLL_CTRL) is not set in CON1 register, it won't be
> possible to set new rate, with next error showing in kernel log:
> 
>     Could not lock PLL fout_mmc_pll
> 
> That can happen for example if bootloader clears that bit beforehand.
> PLL driver doesn't account for that, so if MANUAL_PLL_CTRL bit was
> cleared, it's assumed it was done for a reason and it shouldn't be
> possible to change that PLL's rate at all.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/samsung/clk-pll.c | 105 ++++++++++++++++++++++++++++++++++
>  drivers/clk/samsung/clk-pll.h |   1 +
>  2 files changed, 106 insertions(+)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
@ 2021-09-15  8:27   ` Krzysztof Kozlowski
  2021-09-15 16:37   ` Chanwoo Choi
  2021-09-21 21:10   ` Rob Herring
  2 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-15  8:27 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 14/09/2021 17:56, Sam Protsenko wrote:
> Clock controller driver is designed to have separate instances for each
> particular CMU. So clock IDs in this bindings header also start from 1
> for each CMU.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  include/dt-bindings/clock/exynos850.h | 72 +++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 include/dt-bindings/clock/exynos850.h
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings
  2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
  2021-09-14 21:35   ` Rob Herring
@ 2021-09-15  8:28   ` Krzysztof Kozlowski
  2021-10-05 11:48     ` Sam Protsenko
  2021-09-15 16:47   ` Chanwoo Choi
  2 siblings, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-15  8:28 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 14/09/2021 17:56, Sam Protsenko wrote:
> Provide dt-schema documentation for Exynos850 SoC clock controller.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  .../clock/samsung,exynos850-clock.yaml        | 190 ++++++++++++++++++
>  1 file changed, 190 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> new file mode 100644
> index 000000000000..b69ba4125421
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> @@ -0,0 +1,190 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/samsung,exynos850-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung Exynos850 SoC clock controller
> +
> +maintainers:
> +  - Sam Protsenko <semen.protsenko@linaro.org>
> +  - Chanwoo Choi <cw00.choi@samsung.com>
> +  - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> +  - Sylwester Nawrocki <s.nawrocki@samsung.com>
> +  - Tomasz Figa <tomasz.figa@gmail.com>
> +
> +description: |
> +  Exynos850 clock controller is comprised of several CMU units, generating
> +  clocks for different domains. Those CMU units are modeled as separate device
> +  tree nodes, and might depend on each other. Root clocks in that clock tree are
> +  two external clocks:: OSCCLK (26 MHz) and RTCCLK (32768 Hz). Those external
> +  clocks must be defined as fixed-rate clocks in dts.
> +
> +  CMU_TOP is a top-level CMU, where all base clocks are prepared using PLLs and
> +  dividers; all other leaf clocks (other CMUs) are usually derived from CMU_TOP.
> +
> +  Each clock is assigned an identifier and client nodes can use this identifier
> +  to specify the clock which they consume. All clocks that available for usage
> +  in clock consumer nodes are defined as preprocessor macros in
> +  'dt-bindings/clock/exynos850.h' header.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - samsung,exynos850-cmu-top
> +      - samsung,exynos850-cmu-core
> +      - samsung,exynos850-cmu-hsi
> +      - samsung,exynos850-cmu-peri
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 5
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 5
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-top
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-core
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_CORE bus clock (from CMU_TOP)
> +            - description: CCI clock (from CMU_TOP)
> +            - description: eMMC clock (from CMU_TOP)
> +            - description: SSS clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: dout_core_bus
> +            - const: dout_core_cci
> +            - const: dout_core_mmc_embd
> +            - const: dout_core_sss
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-hsi
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: External RTC clock (32768 Hz)
> +            - description: CMU_HSI bus clock (from CMU_TOP)
> +            - description: SD card clock (from CMU_TOP)
> +            - description: "USB 2.0 DRD clock (from CMU_TOP)"
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: rtcclk
> +            - const: dout_hsi_bus
> +            - const: dout_hsi_mmc_card
> +            - const: dout_hsi_usb20drd
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-peri
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_PERI bus clock (from CMU_TOP)
> +            - description: UART clock (from CMU_TOP)
> +            - description: Parent clock for HSI2C and SPI (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: dout_peri_bus
> +            - const: dout_peri_uart
> +            - const: dout_peri_ip
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  # Clock controller node for CMU_PERI
> +  - |
> +    #include <dt-bindings/clock/exynos850.h>
> +
> +    cmu_peri: clock-controller@10030000 {
> +        compatible = "samsung,exynos850-cmu-peri";
> +        reg = <0x10030000 0x8000>;
> +        #clock-cells = <1>;
> +
> +        clocks = <&oscclk>, <&cmu_top DOUT_PERI_BUS>,
> +                 <&cmu_top DOUT_PERI_UART>,
> +                 <&cmu_top DOUT_PERI_IP>;
> +        clock-names = "oscclk", "dout_peri_bus",
> +                      "dout_peri_uart", "dout_peri_ip";
> +    };
> +
> +  # External reference clock (should be provided in particular board DTS)
> +  - |
> +    oscclk: clock-oscclk {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-output-names = "oscclk";
> +        clock-frequency = <26000000>;
> +    };

Skip ossclk - it's trivial and not related to these bindings.

> +
> +  # UART controller node that consumes the clock generated by CMU_PERI
> +  - |
> +    #include <dt-bindings/clock/exynos850.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    serial_0: serial@13820000 {
> +        compatible = "samsung,exynos850-uart";
> +        reg = <0x13820000 0x100>;
> +        interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&uart0_pins>;
> +        clocks = <&cmu_peri GOUT_UART_PCLK>, <&cmu_peri GOUT_UART_IPCLK>;
> +        clock-names = "uart", "clk_uart_baud0";

The same, skip it because it is trivial and common with all clock providers.

Also Rob's robot checker complains about it.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
@ 2021-09-15  8:59   ` Krzysztof Kozlowski
  2021-10-05 11:29     ` Sam Protsenko
  2021-09-15 13:07   ` Sylwester Nawrocki
  2021-09-15 18:04   ` Chanwoo Choi
  2 siblings, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-15  8:59 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 14/09/2021 17:56, Sam Protsenko wrote:
> This is the initial implementation adding only basic clocks like UART,
> MMC, I2C and corresponding parent clocks. Design is influenced by
> Exynos7 and Exynos5433 clock drivers.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/samsung/Makefile        |   1 +
>  drivers/clk/samsung/clk-exynos850.c | 700 ++++++++++++++++++++++++++++
>  2 files changed, 701 insertions(+)
>  create mode 100644 drivers/clk/samsung/clk-exynos850.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 028b2e27a37e..c46cf11e4d0b 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos5433.o
>  obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
>  obj-$(CONFIG_EXYNOS_CLKOUT)	+= clk-exynos-clkout.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
>  obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
>  obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
>  obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> new file mode 100644
> index 000000000000..1028caa2102e
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos850.c
> @@ -0,0 +1,700 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Linaro Ltd.
> + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * Common Clock Framework support for Exynos850 SoC.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include <dt-bindings/clock/exynos850.h>
> +
> +#include "clk.h"
> +
> +/* Gate register bits */
> +#define GATE_MANUAL		BIT(20)
> +#define GATE_ENABLE_HWACG	BIT(28)
> +
> +/* Gate register offsets range */
> +#define GATE_OFF_START		0x2000
> +#define GATE_OFF_END		0x2fff
> +
> +/**
> + * exynos850_init_clocks - Set clocks initial configuration
> + * @np:			CMU device tree node with "reg" property (CMU addr)
> + * @reg_offs:		Register offsets array for clocks to init
> + * @reg_offs_len:	Number of register offsets in reg_offs array
> + *
> + * Set manual control mode for all gate clocks.
> + */
> +static void __init exynos850_init_clocks(struct device_node *np,
> +		const unsigned long *reg_offs, size_t reg_offs_len)
> +{
> +	const __be32 *regaddr_p;
> +	u64 regaddr;
> +	u32 base;
> +	size_t i;
> +
> +	/* Get the base address ("reg" property in dts) */
> +	regaddr_p = of_get_address(np, 0, NULL, NULL);
> +	if (!regaddr_p)
> +		panic("%s: failed to get reg regaddr\n", __func__);
> +
> +	regaddr = of_translate_address(np, regaddr_p);
> +	if (regaddr == OF_BAD_ADDR || !regaddr)
> +		panic("%s: bad reg regaddr\n", __func__);
> +
> +	base = (u32)regaddr;
> +
> +	for (i = 0; i < reg_offs_len; ++i) {
> +		void __iomem *reg;
> +		u32 val;
> +
> +		/* Modify only gate clock registers */
> +		if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
> +			continue;
> +
> +		reg = ioremap(base + reg_offs[i], 4);

You first translate the address to CPU physical address and then apply
offset. This should be equivalent to one of_iomap() of entire range and
iterate starting from the base pointer.  IOW, I don't get why you have
to map each register instead of mapping entire SFR/IO range?

> +		val = ioread32(reg);
> +		val |= GATE_MANUAL;
> +		val &= ~GATE_ENABLE_HWACG;
> +		iowrite32(val, reg);

All other drivers use readl/writel, so how about keeping it consistent?

Rest looks good but I did not verify the numbers :)

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
  2021-09-15  8:21   ` Krzysztof Kozlowski
@ 2021-09-15 12:51   ` Sylwester Nawrocki
  2021-10-06 11:18     ` Sam Protsenko
  1 sibling, 1 reply; 38+ messages in thread
From: Sylwester Nawrocki @ 2021-09-15 12:51 UTC (permalink / raw)
  To: Sam Protsenko, Krzysztof Kozlowski, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc, Michael Turquette, Stephen Boyd, Rob Herring

Hi,

On 14.09.2021 17:56, Sam Protsenko wrote:
> By default if bus clock has no users its "enable count" value is 0. It
> might be actually running if it's already enabled in bootloader, but
> then in some cases it can be disabled by mistake. For example, such case
> was observed when dw_mci_probe() enabled bus clock, then failed to do
> something and disabled that bus clock on error path. After that even
> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
> CMU bus clock ended up being disabled and it wasn't possible to access
> CMU registers anymore.
> 
> To avoid such cases, CMU driver must increment the ref count for that
> bus clock by running clk_prepare_enable(). There is already existing
> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> support for runtime PM"). But the clock is actually enabled only in
> Exynos5433 clock driver. Let's mimic what is done there in generic
> samsung_cmu_register_one() function, so other drivers can benefit from
> that `.clk_name' field. As was described above, it might be helpful not
> only for PM reasons, but also to prevent possible erroneous clock gating
> on error paths.
> 
> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
> flag for corresponding gate clocks. But that might be not very good
> design decision, as we might still want to disable that bus clock, e.g.
> on PM suspend.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/samsung/clk.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index 1949ae7851b2..da65149fa502 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>  
>  	ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
>  
> +	/* Keep bus clock running, so it's possible to access CMU registers */
> +	if (cmu->clk_name) {
> +		struct clk *bus_clk;
> +
> +		bus_clk = __clk_lookup(cmu->clk_name);
> +		if (bus_clk) {
> +			clk_prepare_enable(bus_clk);
> +		} else {
> +			pr_err("%s: could not find bus clock %s\n", __func__,
> +			       cmu->clk_name);
> +		}
> +	}
> +
>  	if (cmu->pll_clks)
>  		samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks,
>  			reg_base);

I would suggest to implement runtime PM ops in your driver instead, even though
those would initially only contain single clk enable/disable. Things like 
the clk_summary will work then thanks to runtime PM support in the clk core 
(see clk_pm_runtime_* calls).
We could also make common runtime PM suspend/resume helpers but I wouldn't focus
on that too much now, it could well be done later.
And please avoid introducing new __clk_lookup() calls.

-- 
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
  2021-09-15  8:59   ` Krzysztof Kozlowski
@ 2021-09-15 13:07   ` Sylwester Nawrocki
  2021-10-05 11:36     ` Sam Protsenko
  2021-09-15 18:04   ` Chanwoo Choi
  2 siblings, 1 reply; 38+ messages in thread
From: Sylwester Nawrocki @ 2021-09-15 13:07 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc, Michael Turquette, Stephen Boyd, Rob Herring,
	Tomasz Figa, Chanwoo Choi, Paweł Chmiel,
	Krzysztof Kozlowski

On 14.09.2021 17:56, Sam Protsenko wrote:
> +static void __init exynos850_cmu_top_init(struct device_node *np)
> +{
> +	exynos850_init_clocks(np, top_clk_regs, ARRAY_SIZE(top_clk_regs));
> +	samsung_cmu_register_one(np, &top_cmu_info);
> +}
> +
> +CLK_OF_DECLARE(exynos850_cmu_top, "samsung,exynos850-cmu-top",
> +	       exynos850_cmu_top_init);

Was there anything preventing you from making it a platform driver instead?

-- 
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type
  2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko
  2021-09-15  8:24   ` Krzysztof Kozlowski
@ 2021-09-15 15:59   ` Chanwoo Choi
  1 sibling, 0 replies; 38+ messages in thread
From: Chanwoo Choi @ 2021-09-15 15:59 UTC (permalink / raw)
  To: Sam Protsenko, Krzysztof Kozlowski, Sylwester Nawrocki,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> pll0822x PLL is used in Exynos850 SoC for top-level integer PLLs. The
> code was derived from very similar pll35xx type, with next differences:
> 
> 1. Lock time for pll0822x is 150*P_DIV, when for pll35xx it's 270*P_DIV
> 2. It's not suggested in Exynos850 TRM that S_DIV change doesn't require
>     performing PLL lock procedure (which is done in pll35xx
>     implementation)
> 
> When defining pll0822x type, CON3 register offset should be provided as
> a "con" parameter of PLL() macro, like this:
> 
>      PLL(pll_0822x, 0, "fout_shared0_pll", "oscclk",
>          PLL_LOCKTIME_PLL_SHARED0, PLL_CON3_PLL_SHARED0,
>          exynos850_shared0_pll_rates),
> 
> To define PLL rates table, one can use PLL_35XX_RATE() macro, e.g.:
> 
>      PLL_35XX_RATE(26 * MHZ, 1600 * MHZ, 800, 13, 0)
> 
> as it's completely appropriate for pl0822x type and there is no sense in
> duplicating that.
> 
> If bit #1 (MANUAL_PLL_CTRL) is not set in CON1 register, it won't be
> possible to set new rate, with next error showing in kernel log:
> 
>      Could not lock PLL fout_shared1_pll
> 
> That can happen for example if bootloader clears that bit beforehand.
> PLL driver doesn't account for that, so if MANUAL_PLL_CTRL bit was
> cleared, it's assumed it was done for a reason and it shouldn't be
> possible to change that PLL's rate at all.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/clk/samsung/clk-pll.c | 91 +++++++++++++++++++++++++++++++++++
>   drivers/clk/samsung/clk-pll.h |  1 +
>   2 files changed, 92 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index 5873a9354b50..03131b149c0b 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -415,6 +415,89 @@ static const struct clk_ops samsung_pll36xx_clk_min_ops = {
>   	.recalc_rate = samsung_pll36xx_recalc_rate,
>   };
>   
> +/*
> + * PLL0822x Clock Type
> + */
> +/* Maximum lock time can be 150 * PDIV cycles */
> +#define PLL0822X_LOCK_FACTOR		(150)
> +
> +#define PLL0822X_MDIV_MASK		(0x3FF)
> +#define PLL0822X_PDIV_MASK		(0x3F)
> +#define PLL0822X_SDIV_MASK		(0x7)
> +#define PLL0822X_MDIV_SHIFT		(16)
> +#define PLL0822X_PDIV_SHIFT		(8)
> +#define PLL0822X_SDIV_SHIFT		(0)
> +#define PLL0822X_LOCK_STAT_SHIFT	(29)
> +#define PLL0822X_ENABLE_SHIFT		(31)
> +
> +static unsigned long samsung_pll0822x_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	u32 mdiv, pdiv, sdiv, pll_con3;
> +	u64 fvco = parent_rate;
> +
> +	pll_con3 = readl_relaxed(pll->con_reg);
> +	mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL0822X_MDIV_MASK;
> +	pdiv = (pll_con3 >> PLL0822X_PDIV_SHIFT) & PLL0822X_PDIV_MASK;
> +	sdiv = (pll_con3 >> PLL0822X_SDIV_SHIFT) & PLL0822X_SDIV_MASK;
> +
> +	fvco *= mdiv;
> +	do_div(fvco, (pdiv << sdiv));
> +
> +	return (unsigned long)fvco;
> +}
> +
> +static int samsung_pll0822x_set_rate(struct clk_hw *hw, unsigned long drate,
> +				     unsigned long prate)
> +{
> +	const struct samsung_pll_rate_table *rate;
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	u32 pll_con3;
> +
> +	/* Get required rate settings from table */
> +	rate = samsung_get_pll_settings(pll, drate);
> +	if (!rate) {
> +		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
> +			drate, clk_hw_get_name(hw));
> +		return -EINVAL;
> +	}
> +
> +	/* Change PLL PMS values */
> +	pll_con3 = readl_relaxed(pll->con_reg);
> +	pll_con3 &= ~((PLL0822X_MDIV_MASK << PLL0822X_MDIV_SHIFT) |
> +			(PLL0822X_PDIV_MASK << PLL0822X_PDIV_SHIFT) |
> +			(PLL0822X_SDIV_MASK << PLL0822X_SDIV_SHIFT));
> +	pll_con3 |= (rate->mdiv << PLL0822X_MDIV_SHIFT) |
> +			(rate->pdiv << PLL0822X_PDIV_SHIFT) |
> +			(rate->sdiv << PLL0822X_SDIV_SHIFT);
> +
> +	/* Set PLL lock time */
> +	writel_relaxed(rate->pdiv * PLL0822X_LOCK_FACTOR,
> +			pll->lock_reg);
> +
> +	/* Write PMS values */
> +	writel_relaxed(pll_con3, pll->con_reg);
> +
> +	/* Wait for PLL lock if the PLL is enabled */
> +	if (pll_con3 & BIT(pll->enable_offs))
> +		return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops samsung_pll0822x_clk_ops = {
> +	.recalc_rate = samsung_pll0822x_recalc_rate,
> +	.round_rate = samsung_pll_round_rate,
> +	.set_rate = samsung_pll0822x_set_rate,
> +	.enable = samsung_pll3xxx_enable,
> +	.disable = samsung_pll3xxx_disable,
> +};
> +
> +static const struct clk_ops samsung_pll0822x_clk_min_ops = {
> +	.recalc_rate = samsung_pll0822x_recalc_rate,
> +};
> +
>   /*
>    * PLL45xx Clock Type
>    */
> @@ -1296,6 +1379,14 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
>   		else
>   			init.ops = &samsung_pll35xx_clk_ops;
>   		break;
> +	case pll_0822x:
> +		pll->enable_offs = PLL0822X_ENABLE_SHIFT;
> +		pll->lock_offs = PLL0822X_LOCK_STAT_SHIFT;
> +		if (!pll->rate_table)
> +			init.ops = &samsung_pll0822x_clk_min_ops;
> +		else
> +			init.ops = &samsung_pll0822x_clk_ops;
> +		break;
>   	case pll_4500:
>   		init.ops = &samsung_pll45xx_clk_min_ops;
>   		break;
> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
> index 79e41c226b90..213e94a97f23 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -36,6 +36,7 @@ enum samsung_pll_type {
>   	pll_1451x,
>   	pll_1452x,
>   	pll_1460x,
> +	pll_0822x,
>   };
>   
>   #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
> 

Even if I have not Exynos850 TRM, it looks good to me. Thanks.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x PLL type
  2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko
  2021-09-15  8:26   ` Krzysztof Kozlowski
@ 2021-09-15 16:11   ` Chanwoo Choi
  1 sibling, 0 replies; 38+ messages in thread
From: Chanwoo Choi @ 2021-09-15 16:11 UTC (permalink / raw)
  To: Sam Protsenko, Krzysztof Kozlowski, Sylwester Nawrocki,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> pll0831x PLL is used in Exynos850 SoC for top-level fractional PLLs. The
> code was derived from very similar pll36xx type, with next differences:
> 
> 1. Lock time for pll0831x is 500*P_DIV, when for pll36xx it's 3000*P_DIV
> 2. It's not suggested in Exynos850 TRM that S_DIV change doesn't require
>     performing PLL lock procedure (which is done in pll36xx
>     implementation)
> 3. The offset from PMS-values register to K-value register is 0x8 for
>     pll0831x, when for pll36xx it's 0x4
> 
> When defining pll0831x type, CON3 register offset should be provided as
> a "con" parameter of PLL() macro, like this:
> 
>      PLL(pll_0831x, 0, "fout_mmc_pll", "oscclk",
>          PLL_LOCKTIME_PLL_MMC, PLL_CON3_PLL_MMC, pll0831x_26mhz_tbl),
> 
> To define PLL rates table, one can use PLL_36XX_RATE() macro, e.g.:
> 
>      PLL_36XX_RATE(26 * MHZ, 799999877, 31, 1, 0, -15124)
> 
> as it's completely appropriate for pl0831x type and there is no sense in
> duplicating that.
> 
> If bit #1 (MANUAL_PLL_CTRL) is not set in CON1 register, it won't be
> possible to set new rate, with next error showing in kernel log:
> 
>      Could not lock PLL fout_mmc_pll
> 
> That can happen for example if bootloader clears that bit beforehand.
> PLL driver doesn't account for that, so if MANUAL_PLL_CTRL bit was
> cleared, it's assumed it was done for a reason and it shouldn't be
> possible to change that PLL's rate at all.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/clk/samsung/clk-pll.c | 105 ++++++++++++++++++++++++++++++++++
>   drivers/clk/samsung/clk-pll.h |   1 +
>   2 files changed, 106 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index 03131b149c0b..83d1b03647db 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -498,6 +498,103 @@ static const struct clk_ops samsung_pll0822x_clk_min_ops = {
>   	.recalc_rate = samsung_pll0822x_recalc_rate,
>   };
>   
> +/*
> + * PLL0831x Clock Type
> + */
> +/* Maximum lock time can be 500 * PDIV cycles */
> +#define PLL0831X_LOCK_FACTOR		(500)
> +
> +#define PLL0831X_KDIV_MASK		(0xFFFF)
> +#define PLL0831X_MDIV_MASK		(0x1FF)
> +#define PLL0831X_PDIV_MASK		(0x3F)
> +#define PLL0831X_SDIV_MASK		(0x7)
> +#define PLL0831X_MDIV_SHIFT		(16)
> +#define PLL0831X_PDIV_SHIFT		(8)
> +#define PLL0831X_SDIV_SHIFT		(0)
> +#define PLL0831X_KDIV_SHIFT		(0)
> +#define PLL0831X_LOCK_STAT_SHIFT	(29)
> +#define PLL0831X_ENABLE_SHIFT		(31)
> +
> +static unsigned long samsung_pll0831x_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	u32 mdiv, pdiv, sdiv, pll_con3, pll_con5;
> +	s16 kdiv;
> +	u64 fvco = parent_rate;
> +
> +	pll_con3 = readl_relaxed(pll->con_reg);
> +	pll_con5 = readl_relaxed(pll->con_reg + 8);
> +	mdiv = (pll_con3 >> PLL0831X_MDIV_SHIFT) & PLL0831X_MDIV_MASK;
> +	pdiv = (pll_con3 >> PLL0831X_PDIV_SHIFT) & PLL0831X_PDIV_MASK;
> +	sdiv = (pll_con3 >> PLL0831X_SDIV_SHIFT) & PLL0831X_SDIV_MASK;
> +	kdiv = (s16)((pll_con5 >> PLL0831X_KDIV_SHIFT) & PLL0831X_KDIV_MASK);
> +
> +	fvco *= (mdiv << 16) + kdiv;
> +	do_div(fvco, (pdiv << sdiv));
> +	fvco >>= 16;
> +
> +	return (unsigned long)fvco;
> +}
> +
> +static int samsung_pll0831x_set_rate(struct clk_hw *hw, unsigned long drate,
> +				     unsigned long parent_rate)
> +{
> +	const struct samsung_pll_rate_table *rate;
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	u32 pll_con3, pll_con5;
> +
> +	/* Get required rate settings from table */
> +	rate = samsung_get_pll_settings(pll, drate);
> +	if (!rate) {
> +		pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__,
> +			drate, clk_hw_get_name(hw));
> +		return -EINVAL;
> +	}
> +
> +	pll_con3 = readl_relaxed(pll->con_reg);
> +	pll_con5 = readl_relaxed(pll->con_reg + 8);
> +
> +	/* Change PLL PMSK values */
> +	pll_con3 &= ~((PLL0831X_MDIV_MASK << PLL0831X_MDIV_SHIFT) |
> +			(PLL0831X_PDIV_MASK << PLL0831X_PDIV_SHIFT) |
> +			(PLL0831X_SDIV_MASK << PLL0831X_SDIV_SHIFT));
> +	pll_con3 |= (rate->mdiv << PLL0831X_MDIV_SHIFT) |
> +			(rate->pdiv << PLL0831X_PDIV_SHIFT) |
> +			(rate->sdiv << PLL0831X_SDIV_SHIFT);
> +	pll_con5 &= ~(PLL0831X_KDIV_MASK << PLL0831X_KDIV_SHIFT);
> +	/*
> +	 * kdiv is 16-bit 2's complement (s16), but stored as unsigned int.
> +	 * Cast it to u16 to avoid leading 0xffff's in case of negative value.
> +	 */
> +	pll_con5 |= ((u16)rate->kdiv << PLL0831X_KDIV_SHIFT);
> +
> +	/* Set PLL lock time */
> +	writel_relaxed(rate->pdiv * PLL0831X_LOCK_FACTOR, pll->lock_reg);
> +
> +	/* Write PMSK values */
> +	writel_relaxed(pll_con3, pll->con_reg);
> +	writel_relaxed(pll_con5, pll->con_reg + 8);
> +
> +	/* Wait for PLL lock if the PLL is enabled */
> +	if (pll_con3 & BIT(pll->enable_offs))
> +		return samsung_pll_lock_wait(pll, BIT(pll->lock_offs));
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops samsung_pll0831x_clk_ops = {
> +	.recalc_rate = samsung_pll0831x_recalc_rate,
> +	.set_rate = samsung_pll0831x_set_rate,
> +	.round_rate = samsung_pll_round_rate,
> +	.enable = samsung_pll3xxx_enable,
> +	.disable = samsung_pll3xxx_disable,
> +};
> +
> +static const struct clk_ops samsung_pll0831x_clk_min_ops = {
> +	.recalc_rate = samsung_pll0831x_recalc_rate,
> +};
> +
>   /*
>    * PLL45xx Clock Type
>    */
> @@ -1407,6 +1504,14 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
>   		else
>   			init.ops = &samsung_pll36xx_clk_ops;
>   		break;
> +	case pll_0831x:
> +		pll->enable_offs = PLL0831X_ENABLE_SHIFT;
> +		pll->lock_offs = PLL0831X_LOCK_STAT_SHIFT;
> +		if (!pll->rate_table)
> +			init.ops = &samsung_pll0831x_clk_min_ops;
> +		else
> +			init.ops = &samsung_pll0831x_clk_ops;
> +		break;
>   	case pll_6552:
>   	case pll_6552_s3c2416:
>   		init.ops = &samsung_pll6552_clk_ops;
> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
> index 213e94a97f23..a739f2b7ae80 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -37,6 +37,7 @@ enum samsung_pll_type {
>   	pll_1452x,
>   	pll_1460x,
>   	pll_0822x,
> +	pll_0831x,
>   };
>   
>   #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
  2021-09-15  8:27   ` Krzysztof Kozlowski
@ 2021-09-15 16:37   ` Chanwoo Choi
  2021-10-05 10:28     ` Sam Protsenko
  2021-09-21 21:10   ` Rob Herring
  2 siblings, 1 reply; 38+ messages in thread
From: Chanwoo Choi @ 2021-09-15 16:37 UTC (permalink / raw)
  To: Sam Protsenko, Krzysztof Kozlowski, Sylwester Nawrocki,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

Hi,

You don't add clock ids for the all defined clocks in clk-exynos850.c.
I recommend that add all clock ids for the defined clocks if possible.

If you want to change the parent clock of mux or change the clock rate
of div rate for some clocks, you have to touch the files as following:
- include/dt-bindings/clock/exynos850.h
- drivers/clk/samsung/clk-exynos850.c
- exynos850 dt files

If you define the clock ids for all clocks added to this patchset,
you can change the parent or rate by just editing the dt files.

But, I have no strongly objection about just keeping this patch.


On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> Clock controller driver is designed to have separate instances for each
> particular CMU. So clock IDs in this bindings header also start from 1
> for each CMU.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   include/dt-bindings/clock/exynos850.h | 72 +++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
>   create mode 100644 include/dt-bindings/clock/exynos850.h
> 
> diff --git a/include/dt-bindings/clock/exynos850.h b/include/dt-bindings/clock/exynos850.h
> new file mode 100644
> index 000000000000..2f0a7f619627
> --- /dev/null
> +++ b/include/dt-bindings/clock/exynos850.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 Linaro Ltd.
> + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * Device Tree binding constants for Exynos850 clock controller.
> + */
> +
> +#ifndef _DT_BINDINGS_CLOCK_EXYNOS_850_H
> +#define _DT_BINDINGS_CLOCK_EXYNOS_850_H
> +
> +/* CMU_TOP */
> +#define DOUT_HSI_BUS			1
> +#define DOUT_HSI_MMC_CARD		2
> +#define DOUT_HSI_USB20DRD		3
> +#define DOUT_PERI_BUS			4
> +#define DOUT_PERI_UART			5
> +#define DOUT_PERI_IP			6
> +#define DOUT_CORE_BUS			7
> +#define DOUT_CORE_CCI			8
> +#define DOUT_CORE_MMC_EMBD		9
> +#define DOUT_CORE_SSS			10
> +#define TOP_NR_CLK			11
> +
> +/* CMU_HSI */
> +#define GOUT_USB_RTC_CLK		1
> +#define GOUT_USB_REF_CLK		2
> +#define GOUT_USB_PHY_REF_CLK		3
> +#define GOUT_USB_PHY_ACLK		4
> +#define GOUT_USB_BUS_EARLY_CLK		5
> +#define GOUT_GPIO_HSI_PCLK		6
> +#define GOUT_MMC_CARD_ACLK		7
> +#define GOUT_MMC_CARD_SDCLKIN		8
> +#define GOUT_SYSREG_HSI_PCLK		9
> +#define HSI_NR_CLK			10
> +
> +/* CMU_PERI */
> +#define GOUT_GPIO_PERI_PCLK		1
> +#define GOUT_HSI2C0_IPCLK		2
> +#define GOUT_HSI2C0_PCLK		3
> +#define GOUT_HSI2C1_IPCLK		4
> +#define GOUT_HSI2C1_PCLK		5
> +#define GOUT_HSI2C2_IPCLK		6
> +#define GOUT_HSI2C2_PCLK		7
> +#define GOUT_I2C0_PCLK			8
> +#define GOUT_I2C1_PCLK			9
> +#define GOUT_I2C2_PCLK			10
> +#define GOUT_I2C3_PCLK			11
> +#define GOUT_I2C4_PCLK			12
> +#define GOUT_I2C5_PCLK			13
> +#define GOUT_I2C6_PCLK			14
> +#define GOUT_MCT_PCLK			15
> +#define GOUT_PWM_MOTOR_PCLK		16
> +#define GOUT_SPI0_IPCLK			17
> +#define GOUT_SPI0_PCLK			18
> +#define GOUT_SYSREG_PERI_PCLK		19
> +#define GOUT_UART_IPCLK			20
> +#define GOUT_UART_PCLK			21
> +#define GOUT_WDT0_PCLK			22
> +#define GOUT_WDT1_PCLK			23
> +#define PERI_NR_CLK			24
> +
> +/* CMU_CORE */
> +#define GOUT_CCI_ACLK			1
> +#define GOUT_GIC_CLK			2
> +#define GOUT_MMC_EMBD_ACLK		3
> +#define GOUT_MMC_EMBD_SDCLKIN		4
> +#define GOUT_SSS_ACLK			5
> +#define GOUT_SSS_PCLK			6
> +#define CORE_NR_CLK			7
> +
> +#endif /* _DT_BINDINGS_CLOCK_EXYNOS_850_H */
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings
  2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
  2021-09-14 21:35   ` Rob Herring
  2021-09-15  8:28   ` Krzysztof Kozlowski
@ 2021-09-15 16:47   ` Chanwoo Choi
  2 siblings, 0 replies; 38+ messages in thread
From: Chanwoo Choi @ 2021-09-15 16:47 UTC (permalink / raw)
  To: Sam Protsenko, Krzysztof Kozlowski, Sylwester Nawrocki,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> Provide dt-schema documentation for Exynos850 SoC clock controller.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   .../clock/samsung,exynos850-clock.yaml        | 190 ++++++++++++++++++
>   1 file changed, 190 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> new file mode 100644
> index 000000000000..b69ba4125421
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> @@ -0,0 +1,190 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/samsung,exynos850-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung Exynos850 SoC clock controller
> +
> +maintainers:
> +  - Sam Protsenko <semen.protsenko@linaro.org>
> +  - Chanwoo Choi <cw00.choi@samsung.com>
> +  - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> +  - Sylwester Nawrocki <s.nawrocki@samsung.com>
> +  - Tomasz Figa <tomasz.figa@gmail.com>
> +
> +description: |
> +  Exynos850 clock controller is comprised of several CMU units, generating
> +  clocks for different domains. Those CMU units are modeled as separate device
> +  tree nodes, and might depend on each other. Root clocks in that clock tree are
> +  two external clocks:: OSCCLK (26 MHz) and RTCCLK (32768 Hz). Those external
> +  clocks must be defined as fixed-rate clocks in dts.
> +
> +  CMU_TOP is a top-level CMU, where all base clocks are prepared using PLLs and
> +  dividers; all other leaf clocks (other CMUs) are usually derived from CMU_TOP.
> +
> +  Each clock is assigned an identifier and client nodes can use this identifier
> +  to specify the clock which they consume. All clocks that available for usage
> +  in clock consumer nodes are defined as preprocessor macros in
> +  'dt-bindings/clock/exynos850.h' header.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - samsung,exynos850-cmu-top
> +      - samsung,exynos850-cmu-core
> +      - samsung,exynos850-cmu-hsi
> +      - samsung,exynos850-cmu-peri
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 5
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 5
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-top
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-core
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_CORE bus clock (from CMU_TOP)
> +            - description: CCI clock (from CMU_TOP)
> +            - description: eMMC clock (from CMU_TOP)
> +            - description: SSS clock (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: dout_core_bus
> +            - const: dout_core_cci
> +            - const: dout_core_mmc_embd
> +            - const: dout_core_sss
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-hsi
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: External RTC clock (32768 Hz)
> +            - description: CMU_HSI bus clock (from CMU_TOP)
> +            - description: SD card clock (from CMU_TOP)
> +            - description: "USB 2.0 DRD clock (from CMU_TOP)"
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: rtcclk
> +            - const: dout_hsi_bus
> +            - const: dout_hsi_mmc_card
> +            - const: dout_hsi_usb20drd
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: samsung,exynos850-cmu-peri
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: External reference clock (26 MHz)
> +            - description: CMU_PERI bus clock (from CMU_TOP)
> +            - description: UART clock (from CMU_TOP)
> +            - description: Parent clock for HSI2C and SPI (from CMU_TOP)
> +
> +        clock-names:
> +          items:
> +            - const: oscclk
> +            - const: dout_peri_bus
> +            - const: dout_peri_uart
> +            - const: dout_peri_ip
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  # Clock controller node for CMU_PERI
> +  - |
> +    #include <dt-bindings/clock/exynos850.h>
> +
> +    cmu_peri: clock-controller@10030000 {
> +        compatible = "samsung,exynos850-cmu-peri";
> +        reg = <0x10030000 0x8000>;
> +        #clock-cells = <1>;
> +
> +        clocks = <&oscclk>, <&cmu_top DOUT_PERI_BUS>,
> +                 <&cmu_top DOUT_PERI_UART>,
> +                 <&cmu_top DOUT_PERI_IP>;
> +        clock-names = "oscclk", "dout_peri_bus",
> +                      "dout_peri_uart", "dout_peri_ip";
> +    };
> +
> +  # External reference clock (should be provided in particular board DTS)
> +  - |
> +    oscclk: clock-oscclk {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-output-names = "oscclk";
> +        clock-frequency = <26000000>;
> +    };
> +
> +  # UART controller node that consumes the clock generated by CMU_PERI
> +  - |
> +    #include <dt-bindings/clock/exynos850.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    serial_0: serial@13820000 {
> +        compatible = "samsung,exynos850-uart";
> +        reg = <0x13820000 0x100>;
> +        interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&uart0_pins>;
> +        clocks = <&cmu_peri GOUT_UART_PCLK>, <&cmu_peri GOUT_UART_IPCLK>;
> +        clock-names = "uart", "clk_uart_baud0";
> +    };
> +
> +...
> 

Looks good for very detailed description and example. Thanks.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
  2021-09-15  8:59   ` Krzysztof Kozlowski
  2021-09-15 13:07   ` Sylwester Nawrocki
@ 2021-09-15 18:04   ` Chanwoo Choi
  2021-09-15 22:00     ` Sam Protsenko
  2 siblings, 1 reply; 38+ messages in thread
From: Chanwoo Choi @ 2021-09-15 18:04 UTC (permalink / raw)
  To: Sam Protsenko, Krzysztof Kozlowski, Sylwester Nawrocki,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm-kernel, linux-clk, linux-kernel,
	linux-samsung-soc

Hi Sam,

On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> This is the initial implementation adding only basic clocks like UART,
> MMC, I2C and corresponding parent clocks. Design is influenced by
> Exynos7 and Exynos5433 clock drivers.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>   drivers/clk/samsung/Makefile        |   1 +
>   drivers/clk/samsung/clk-exynos850.c | 700 ++++++++++++++++++++++++++++
>   2 files changed, 701 insertions(+)
>   create mode 100644 drivers/clk/samsung/clk-exynos850.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 028b2e27a37e..c46cf11e4d0b 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos5433.o
>   obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
>   obj-$(CONFIG_EXYNOS_CLKOUT)	+= clk-exynos-clkout.o
>   obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
>   obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
>   obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
>   obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> new file mode 100644
> index 000000000000..1028caa2102e
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos850.c
> @@ -0,0 +1,700 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Linaro Ltd.
> + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> + *
> + * Common Clock Framework support for Exynos850 SoC.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include <dt-bindings/clock/exynos850.h>
> +
> +#include "clk.h"
> +
> +/* Gate register bits */
> +#define GATE_MANUAL		BIT(20)
> +#define GATE_ENABLE_HWACG	BIT(28)
> +
> +/* Gate register offsets range */
> +#define GATE_OFF_START		0x2000
> +#define GATE_OFF_END		0x2fff
> +
> +/**
> + * exynos850_init_clocks - Set clocks initial configuration
> + * @np:			CMU device tree node with "reg" property (CMU addr)
> + * @reg_offs:		Register offsets array for clocks to init
> + * @reg_offs_len:	Number of register offsets in reg_offs array
> + *
> + * Set manual control mode for all gate clocks.
> + */
> +static void __init exynos850_init_clocks(struct device_node *np,
> +		const unsigned long *reg_offs, size_t reg_offs_len)
> +{
> +	const __be32 *regaddr_p;
> +	u64 regaddr;
> +	u32 base;
> +	size_t i;
> +
> +	/* Get the base address ("reg" property in dts) */
> +	regaddr_p = of_get_address(np, 0, NULL, NULL);
> +	if (!regaddr_p)
> +		panic("%s: failed to get reg regaddr\n", __func__);
> +
> +	regaddr = of_translate_address(np, regaddr_p);
> +	if (regaddr == OF_BAD_ADDR || !regaddr)
> +		panic("%s: bad reg regaddr\n", __func__);
> +
> +	base = (u32)regaddr;
> +
> +	for (i = 0; i < reg_offs_len; ++i) {
> +		void __iomem *reg;
> +		u32 val;
> +
> +		/* Modify only gate clock registers */
> +		if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
> +			continue; > +
> +		reg = ioremap(base + reg_offs[i], 4);
> +		val = ioread32(reg);
> +		val |= GATE_MANUAL;
> +		val &= ~GATE_ENABLE_HWACG;
> +		iowrite32(val, reg);
> +		iounmap(reg);

I understand your intention for disabling HWACG.
But, it is not good to execute ioreamp/iounmap for each clock gate
register. I think that we need to consider the more pretty method
to initialize the clock register before clock registration.

[snip]

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-15 18:04   ` Chanwoo Choi
@ 2021-09-15 22:00     ` Sam Protsenko
  0 siblings, 0 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-09-15 22:00 UTC (permalink / raw)
  To: Chanwoo Choi, Krzysztof Kozlowski, Sylwester Nawrocki
  Cc: Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette, Ryu Euiyoul, Tom Gall,
	Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On Wed, 15 Sept 2021 at 21:05, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi Sam,
>
> On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> > This is the initial implementation adding only basic clocks like UART,
> > MMC, I2C and corresponding parent clocks. Design is influenced by
> > Exynos7 and Exynos5433 clock drivers.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >   drivers/clk/samsung/Makefile        |   1 +
> >   drivers/clk/samsung/clk-exynos850.c | 700 ++++++++++++++++++++++++++++
> >   2 files changed, 701 insertions(+)
> >   create mode 100644 drivers/clk/samsung/clk-exynos850.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index 028b2e27a37e..c46cf11e4d0b 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos5433.o
> >   obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
> >   obj-$(CONFIG_EXYNOS_CLKOUT) += clk-exynos-clkout.o
> >   obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos7.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos850.o
> >   obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
> >   obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
> >   obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> > new file mode 100644
> > index 000000000000..1028caa2102e
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-exynos850.c
> > @@ -0,0 +1,700 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2021 Linaro Ltd.
> > + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> > + *
> > + * Common Clock Framework support for Exynos850 SoC.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +
> > +#include <dt-bindings/clock/exynos850.h>
> > +
> > +#include "clk.h"
> > +
> > +/* Gate register bits */
> > +#define GATE_MANUAL          BIT(20)
> > +#define GATE_ENABLE_HWACG    BIT(28)
> > +
> > +/* Gate register offsets range */
> > +#define GATE_OFF_START               0x2000
> > +#define GATE_OFF_END         0x2fff
> > +
> > +/**
> > + * exynos850_init_clocks - Set clocks initial configuration
> > + * @np:                      CMU device tree node with "reg" property (CMU addr)
> > + * @reg_offs:                Register offsets array for clocks to init
> > + * @reg_offs_len:    Number of register offsets in reg_offs array
> > + *
> > + * Set manual control mode for all gate clocks.
> > + */
> > +static void __init exynos850_init_clocks(struct device_node *np,
> > +             const unsigned long *reg_offs, size_t reg_offs_len)
> > +{
> > +     const __be32 *regaddr_p;
> > +     u64 regaddr;
> > +     u32 base;
> > +     size_t i;
> > +
> > +     /* Get the base address ("reg" property in dts) */
> > +     regaddr_p = of_get_address(np, 0, NULL, NULL);
> > +     if (!regaddr_p)
> > +             panic("%s: failed to get reg regaddr\n", __func__);
> > +
> > +     regaddr = of_translate_address(np, regaddr_p);
> > +     if (regaddr == OF_BAD_ADDR || !regaddr)
> > +             panic("%s: bad reg regaddr\n", __func__);
> > +
> > +     base = (u32)regaddr;
> > +
> > +     for (i = 0; i < reg_offs_len; ++i) {
> > +             void __iomem *reg;
> > +             u32 val;
> > +
> > +             /* Modify only gate clock registers */
> > +             if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
> > +                     continue; > +
> > +             reg = ioremap(base + reg_offs[i], 4);
> > +             val = ioread32(reg);
> > +             val |= GATE_MANUAL;
> > +             val &= ~GATE_ENABLE_HWACG;
> > +             iowrite32(val, reg);
> > +             iounmap(reg);
>
> I understand your intention for disabling HWACG.
> But, it is not good to execute ioreamp/iounmap for each clock gate
> register. I think that we need to consider the more pretty method
> to initialize the clock register before clock registration.
>
> [snip]
>

Hi guys,

Thanks for the quick review! I'll address all your comments once I get
back from vacation (in two weeks), and will send v2.

> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
  2021-09-15  8:27   ` Krzysztof Kozlowski
  2021-09-15 16:37   ` Chanwoo Choi
@ 2021-09-21 21:10   ` Rob Herring
  2 siblings, 0 replies; 38+ messages in thread
From: Rob Herring @ 2021-09-21 21:10 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Chanwoo Choi, Krzysztof Kozlowski, Michael Turquette,
	Ryu Euiyoul, Sumit Semwal, linux-clk, Amit Pundir, Tom Gall,
	linux-kernel, linux-samsung-soc, Tomasz Figa, linux-arm-kernel,
	Paweł Chmiel, Sylwester Nawrocki, Stephen Boyd, Rob Herring,
	John Stultz, devicetree

On Tue, 14 Sep 2021 18:56:05 +0300, Sam Protsenko wrote:
> Clock controller driver is designed to have separate instances for each
> particular CMU. So clock IDs in this bindings header also start from 1
> for each CMU.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  include/dt-bindings/clock/exynos850.h | 72 +++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 include/dt-bindings/clock/exynos850.h
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-09-15 16:37   ` Chanwoo Choi
@ 2021-10-05 10:28     ` Sam Protsenko
  2021-10-06 10:49       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 38+ messages in thread
From: Sam Protsenko @ 2021-10-05 10:28 UTC (permalink / raw)
  To: Chanwoo Choi, Krzysztof Kozlowski, Sylwester Nawrocki
  Cc: Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette, Ryu Euiyoul, Tom Gall,
	Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On Wed, 15 Sept 2021 at 19:37, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi,
>
> You don't add clock ids for the all defined clocks in clk-exynos850.c.
> I recommend that add all clock ids for the defined clocks if possible.
>
> If you want to change the parent clock of mux or change the clock rate
> of div rate for some clocks, you have to touch the files as following:
> - include/dt-bindings/clock/exynos850.h
> - drivers/clk/samsung/clk-exynos850.c
> - exynos850 dt files
>
> If you define the clock ids for all clocks added to this patchset,
> you can change the parent or rate by just editing the dt files.
>

Hi Chanwoo,

I see your point. But I have intentionally omitted some clock ids,
which can't be / shouldn't be used by consumers in device tree.
Actually I took that idea from clk-exynos7.c.

Krzysztof, Sylwester: can you please advice if all clock ids should be
defined, or only those that are going to be used in dts clk consumers?
I don't mind reworking the patch, just want to be sure which design
approach we want to follow.

Thanks!

> But, I have no strongly objection about just keeping this patch.
>
>
> On 21. 9. 15. 오전 12:56, Sam Protsenko wrote:
> > Clock controller driver is designed to have separate instances for each
> > particular CMU. So clock IDs in this bindings header also start from 1
> > for each CMU.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >   include/dt-bindings/clock/exynos850.h | 72 +++++++++++++++++++++++++++
> >   1 file changed, 72 insertions(+)
> >   create mode 100644 include/dt-bindings/clock/exynos850.h
> >
> > diff --git a/include/dt-bindings/clock/exynos850.h b/include/dt-bindings/clock/exynos850.h
> > new file mode 100644
> > index 000000000000..2f0a7f619627
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/exynos850.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2021 Linaro Ltd.
> > + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> > + *
> > + * Device Tree binding constants for Exynos850 clock controller.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_CLOCK_EXYNOS_850_H
> > +#define _DT_BINDINGS_CLOCK_EXYNOS_850_H
> > +
> > +/* CMU_TOP */
> > +#define DOUT_HSI_BUS                 1
> > +#define DOUT_HSI_MMC_CARD            2
> > +#define DOUT_HSI_USB20DRD            3
> > +#define DOUT_PERI_BUS                        4
> > +#define DOUT_PERI_UART                       5
> > +#define DOUT_PERI_IP                 6
> > +#define DOUT_CORE_BUS                        7
> > +#define DOUT_CORE_CCI                        8
> > +#define DOUT_CORE_MMC_EMBD           9
> > +#define DOUT_CORE_SSS                        10
> > +#define TOP_NR_CLK                   11
> > +
> > +/* CMU_HSI */
> > +#define GOUT_USB_RTC_CLK             1
> > +#define GOUT_USB_REF_CLK             2
> > +#define GOUT_USB_PHY_REF_CLK         3
> > +#define GOUT_USB_PHY_ACLK            4
> > +#define GOUT_USB_BUS_EARLY_CLK               5
> > +#define GOUT_GPIO_HSI_PCLK           6
> > +#define GOUT_MMC_CARD_ACLK           7
> > +#define GOUT_MMC_CARD_SDCLKIN                8
> > +#define GOUT_SYSREG_HSI_PCLK         9
> > +#define HSI_NR_CLK                   10
> > +
> > +/* CMU_PERI */
> > +#define GOUT_GPIO_PERI_PCLK          1
> > +#define GOUT_HSI2C0_IPCLK            2
> > +#define GOUT_HSI2C0_PCLK             3
> > +#define GOUT_HSI2C1_IPCLK            4
> > +#define GOUT_HSI2C1_PCLK             5
> > +#define GOUT_HSI2C2_IPCLK            6
> > +#define GOUT_HSI2C2_PCLK             7
> > +#define GOUT_I2C0_PCLK                       8
> > +#define GOUT_I2C1_PCLK                       9
> > +#define GOUT_I2C2_PCLK                       10
> > +#define GOUT_I2C3_PCLK                       11
> > +#define GOUT_I2C4_PCLK                       12
> > +#define GOUT_I2C5_PCLK                       13
> > +#define GOUT_I2C6_PCLK                       14
> > +#define GOUT_MCT_PCLK                        15
> > +#define GOUT_PWM_MOTOR_PCLK          16
> > +#define GOUT_SPI0_IPCLK                      17
> > +#define GOUT_SPI0_PCLK                       18
> > +#define GOUT_SYSREG_PERI_PCLK                19
> > +#define GOUT_UART_IPCLK                      20
> > +#define GOUT_UART_PCLK                       21
> > +#define GOUT_WDT0_PCLK                       22
> > +#define GOUT_WDT1_PCLK                       23
> > +#define PERI_NR_CLK                  24
> > +
> > +/* CMU_CORE */
> > +#define GOUT_CCI_ACLK                        1
> > +#define GOUT_GIC_CLK                 2
> > +#define GOUT_MMC_EMBD_ACLK           3
> > +#define GOUT_MMC_EMBD_SDCLKIN                4
> > +#define GOUT_SSS_ACLK                        5
> > +#define GOUT_SSS_PCLK                        6
> > +#define CORE_NR_CLK                  7
> > +
> > +#endif /* _DT_BINDINGS_CLOCK_EXYNOS_850_H */
> >
>
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-15  8:59   ` Krzysztof Kozlowski
@ 2021-10-05 11:29     ` Sam Protsenko
  2021-10-06 12:50       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 38+ messages in thread
From: Sam Protsenko @ 2021-10-05 11:29 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On Wed, 15 Sept 2021 at 11:59, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 14/09/2021 17:56, Sam Protsenko wrote:
> > This is the initial implementation adding only basic clocks like UART,
> > MMC, I2C and corresponding parent clocks. Design is influenced by
> > Exynos7 and Exynos5433 clock drivers.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/clk/samsung/Makefile        |   1 +
> >  drivers/clk/samsung/clk-exynos850.c | 700 ++++++++++++++++++++++++++++
> >  2 files changed, 701 insertions(+)
> >  create mode 100644 drivers/clk/samsung/clk-exynos850.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index 028b2e27a37e..c46cf11e4d0b 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos5433.o
> >  obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o
> >  obj-$(CONFIG_EXYNOS_CLKOUT)  += clk-exynos-clkout.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos7.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos850.o
> >  obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o
> >  obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
> >  obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o
> > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> > new file mode 100644
> > index 000000000000..1028caa2102e
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-exynos850.c
> > @@ -0,0 +1,700 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2021 Linaro Ltd.
> > + * Author: Sam Protsenko <semen.protsenko@linaro.org>
> > + *
> > + * Common Clock Framework support for Exynos850 SoC.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +
> > +#include <dt-bindings/clock/exynos850.h>
> > +
> > +#include "clk.h"
> > +
> > +/* Gate register bits */
> > +#define GATE_MANUAL          BIT(20)
> > +#define GATE_ENABLE_HWACG    BIT(28)
> > +
> > +/* Gate register offsets range */
> > +#define GATE_OFF_START               0x2000
> > +#define GATE_OFF_END         0x2fff
> > +
> > +/**
> > + * exynos850_init_clocks - Set clocks initial configuration
> > + * @np:                      CMU device tree node with "reg" property (CMU addr)
> > + * @reg_offs:                Register offsets array for clocks to init
> > + * @reg_offs_len:    Number of register offsets in reg_offs array
> > + *
> > + * Set manual control mode for all gate clocks.
> > + */
> > +static void __init exynos850_init_clocks(struct device_node *np,
> > +             const unsigned long *reg_offs, size_t reg_offs_len)
> > +{
> > +     const __be32 *regaddr_p;
> > +     u64 regaddr;
> > +     u32 base;
> > +     size_t i;
> > +
> > +     /* Get the base address ("reg" property in dts) */
> > +     regaddr_p = of_get_address(np, 0, NULL, NULL);
> > +     if (!regaddr_p)
> > +             panic("%s: failed to get reg regaddr\n", __func__);
> > +
> > +     regaddr = of_translate_address(np, regaddr_p);
> > +     if (regaddr == OF_BAD_ADDR || !regaddr)
> > +             panic("%s: bad reg regaddr\n", __func__);
> > +
> > +     base = (u32)regaddr;
> > +
> > +     for (i = 0; i < reg_offs_len; ++i) {
> > +             void __iomem *reg;
> > +             u32 val;
> > +
> > +             /* Modify only gate clock registers */
> > +             if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
> > +                     continue;
> > +
> > +             reg = ioremap(base + reg_offs[i], 4);
>
> You first translate the address to CPU physical address and then apply
> offset. This should be equivalent to one of_iomap() of entire range and
> iterate starting from the base pointer.  IOW, I don't get why you have
> to map each register instead of mapping entire SFR/IO range?
>

Thanks, will do in v2.

> > +             val = ioread32(reg);
> > +             val |= GATE_MANUAL;
> > +             val &= ~GATE_ENABLE_HWACG;
> > +             iowrite32(val, reg);
>
> All other drivers use readl/writel, so how about keeping it consistent?
>

Ok. Though io* variants looks better to me (API names consistent with
ioremap/iounmap) :)

> Rest looks good but I did not verify the numbers :)
>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-09-15 13:07   ` Sylwester Nawrocki
@ 2021-10-05 11:36     ` Sam Protsenko
  2021-10-06 12:46       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 38+ messages in thread
From: Sam Protsenko @ 2021-10-05 11:36 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm Mailing List, linux-clk,
	Linux Kernel Mailing List, Linux Samsung SOC, Michael Turquette,
	Stephen Boyd, Rob Herring, Tomasz Figa, Chanwoo Choi,
	Paweł Chmiel, Krzysztof Kozlowski

On Wed, 15 Sept 2021 at 16:07, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> On 14.09.2021 17:56, Sam Protsenko wrote:
> > +static void __init exynos850_cmu_top_init(struct device_node *np)
> > +{
> > +     exynos850_init_clocks(np, top_clk_regs, ARRAY_SIZE(top_clk_regs));
> > +     samsung_cmu_register_one(np, &top_cmu_info);
> > +}
> > +
> > +CLK_OF_DECLARE(exynos850_cmu_top, "samsung,exynos850-cmu-top",
> > +            exynos850_cmu_top_init);
>
> Was there anything preventing you from making it a platform driver instead?
>

Can you please elaborate on benefits of adding platform driver? I
don't implement PM ops for now, and I can see that clk-exynos7.c does
not add platform driver as well... clk-exynos5433.c seems to use
platform_driver for PM ops only.

> --
> Regards,
> Sylwester

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings
  2021-09-15  8:28   ` Krzysztof Kozlowski
@ 2021-10-05 11:48     ` Sam Protsenko
  0 siblings, 0 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-10-05 11:48 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On Wed, 15 Sept 2021 at 11:28, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 14/09/2021 17:56, Sam Protsenko wrote:
> > Provide dt-schema documentation for Exynos850 SoC clock controller.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  .../clock/samsung,exynos850-clock.yaml        | 190 ++++++++++++++++++
> >  1 file changed, 190 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> > new file mode 100644
> > index 000000000000..b69ba4125421
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/samsung,exynos850-clock.yaml
> > @@ -0,0 +1,190 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/samsung,exynos850-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Samsung Exynos850 SoC clock controller
> > +
> > +maintainers:
> > +  - Sam Protsenko <semen.protsenko@linaro.org>
> > +  - Chanwoo Choi <cw00.choi@samsung.com>
> > +  - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> > +  - Sylwester Nawrocki <s.nawrocki@samsung.com>
> > +  - Tomasz Figa <tomasz.figa@gmail.com>
> > +
> > +description: |
> > +  Exynos850 clock controller is comprised of several CMU units, generating
> > +  clocks for different domains. Those CMU units are modeled as separate device
> > +  tree nodes, and might depend on each other. Root clocks in that clock tree are
> > +  two external clocks:: OSCCLK (26 MHz) and RTCCLK (32768 Hz). Those external
> > +  clocks must be defined as fixed-rate clocks in dts.
> > +
> > +  CMU_TOP is a top-level CMU, where all base clocks are prepared using PLLs and
> > +  dividers; all other leaf clocks (other CMUs) are usually derived from CMU_TOP.
> > +
> > +  Each clock is assigned an identifier and client nodes can use this identifier
> > +  to specify the clock which they consume. All clocks that available for usage
> > +  in clock consumer nodes are defined as preprocessor macros in
> > +  'dt-bindings/clock/exynos850.h' header.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - samsung,exynos850-cmu-top
> > +      - samsung,exynos850-cmu-core
> > +      - samsung,exynos850-cmu-hsi
> > +      - samsung,exynos850-cmu-peri
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 5
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: samsung,exynos850-cmu-top
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: External reference clock (26 MHz)
> > +
> > +        clock-names:
> > +          items:
> > +            - const: oscclk
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: samsung,exynos850-cmu-core
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: External reference clock (26 MHz)
> > +            - description: CMU_CORE bus clock (from CMU_TOP)
> > +            - description: CCI clock (from CMU_TOP)
> > +            - description: eMMC clock (from CMU_TOP)
> > +            - description: SSS clock (from CMU_TOP)
> > +
> > +        clock-names:
> > +          items:
> > +            - const: oscclk
> > +            - const: dout_core_bus
> > +            - const: dout_core_cci
> > +            - const: dout_core_mmc_embd
> > +            - const: dout_core_sss
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: samsung,exynos850-cmu-hsi
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: External reference clock (26 MHz)
> > +            - description: External RTC clock (32768 Hz)
> > +            - description: CMU_HSI bus clock (from CMU_TOP)
> > +            - description: SD card clock (from CMU_TOP)
> > +            - description: "USB 2.0 DRD clock (from CMU_TOP)"
> > +
> > +        clock-names:
> > +          items:
> > +            - const: oscclk
> > +            - const: rtcclk
> > +            - const: dout_hsi_bus
> > +            - const: dout_hsi_mmc_card
> > +            - const: dout_hsi_usb20drd
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: samsung,exynos850-cmu-peri
> > +
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: External reference clock (26 MHz)
> > +            - description: CMU_PERI bus clock (from CMU_TOP)
> > +            - description: UART clock (from CMU_TOP)
> > +            - description: Parent clock for HSI2C and SPI (from CMU_TOP)
> > +
> > +        clock-names:
> > +          items:
> > +            - const: oscclk
> > +            - const: dout_peri_bus
> > +            - const: dout_peri_uart
> > +            - const: dout_peri_ip
> > +
> > +required:
> > +  - compatible
> > +  - "#clock-cells"
> > +  - clocks
> > +  - clock-names
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  # Clock controller node for CMU_PERI
> > +  - |
> > +    #include <dt-bindings/clock/exynos850.h>
> > +
> > +    cmu_peri: clock-controller@10030000 {
> > +        compatible = "samsung,exynos850-cmu-peri";
> > +        reg = <0x10030000 0x8000>;
> > +        #clock-cells = <1>;
> > +
> > +        clocks = <&oscclk>, <&cmu_top DOUT_PERI_BUS>,
> > +                 <&cmu_top DOUT_PERI_UART>,
> > +                 <&cmu_top DOUT_PERI_IP>;
> > +        clock-names = "oscclk", "dout_peri_bus",
> > +                      "dout_peri_uart", "dout_peri_ip";
> > +    };
> > +
> > +  # External reference clock (should be provided in particular board DTS)
> > +  - |
> > +    oscclk: clock-oscclk {
> > +        compatible = "fixed-clock";
> > +        #clock-cells = <0>;
> > +        clock-output-names = "oscclk";
> > +        clock-frequency = <26000000>;
> > +    };
>
> Skip ossclk - it's trivial and not related to these bindings.
>
> > +
> > +  # UART controller node that consumes the clock generated by CMU_PERI
> > +  - |
> > +    #include <dt-bindings/clock/exynos850.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    serial_0: serial@13820000 {
> > +        compatible = "samsung,exynos850-uart";
> > +        reg = <0x13820000 0x100>;
> > +        interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&uart0_pins>;
> > +        clocks = <&cmu_peri GOUT_UART_PCLK>, <&cmu_peri GOUT_UART_IPCLK>;
> > +        clock-names = "uart", "clk_uart_baud0";
>
> The same, skip it because it is trivial and common with all clock providers.
>

Sure, will do in v2.

> Also Rob's robot checker complains about it.
>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-09-15  8:21   ` Krzysztof Kozlowski
@ 2021-10-06 10:46     ` Sam Protsenko
  2021-10-06 12:38       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 38+ messages in thread
From: Sam Protsenko @ 2021-10-06 10:46 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 14/09/2021 17:56, Sam Protsenko wrote:
> > By default if bus clock has no users its "enable count" value is 0. It
> > might be actually running if it's already enabled in bootloader, but
> > then in some cases it can be disabled by mistake. For example, such case
> > was observed when dw_mci_probe() enabled bus clock, then failed to do
> > something and disabled that bus clock on error path. After that even
> > attempt to read the 'clk_summary' file in DebugFS freezed forever, as
> > CMU bus clock ended up being disabled and it wasn't possible to access
> > CMU registers anymore.
> >
> > To avoid such cases, CMU driver must increment the ref count for that
> > bus clock by running clk_prepare_enable(). There is already existing
> > '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
> > It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> > support for runtime PM"). But the clock is actually enabled only in
> > Exynos5433 clock driver. Let's mimic what is done there in generic
> > samsung_cmu_register_one() function, so other drivers can benefit from
> > that `.clk_name' field. As was described above, it might be helpful not
> > only for PM reasons, but also to prevent possible erroneous clock gating
> > on error paths.
> >
> > Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
> > flag for corresponding gate clocks. But that might be not very good
> > design decision, as we might still want to disable that bus clock, e.g.
> > on PM suspend.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/clk/samsung/clk.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> > index 1949ae7851b2..da65149fa502 100644
> > --- a/drivers/clk/samsung/clk.c
> > +++ b/drivers/clk/samsung/clk.c
> > @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
> >
> >       ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
> >
> > +     /* Keep bus clock running, so it's possible to access CMU registers */
> > +     if (cmu->clk_name) {
> > +             struct clk *bus_clk;
> > +
> > +             bus_clk = __clk_lookup(cmu->clk_name);
> > +             if (bus_clk) {
> > +                     clk_prepare_enable(bus_clk);
> > +             } else {
> > +                     pr_err("%s: could not find bus clock %s\n", __func__,
> > +                            cmu->clk_name);
> > +             }
> > +     }
> > +
>
> Solving this problem in generic way makes sense but your solution is
> insufficient. You skipped suspend/resume paths and in such case you
> should remove the Exynos5433-specific code.
>

Keeping core bus clocks always running seems like a separate
independent feature to me (not related to suspend/resume). It's
mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
support for runtime PM") this way:

    "Also for each CMU there is one special parent clock, which has to
be enabled all the time when any access to CMU registers is being
done."

Why do you think suspend/resume paths have to be implemented along
with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not
implemented on my board yet and I can't test it.

If you are suggesting moving all stuff from exynos5433_cmu_probe()
into samsung_cmu_register_one(), it would take passing platform_device
there, and implementing all PM related operations. I guess it's not a
super easy task, as it would require converting clk-exynos7 to
platform_driver for instance, and re-testing everything on exynos5433
and exynos7 boards (which I don't have).

What do you say if I pull that code to clk-exynos850.c instead for v2?
Refactoring (merging stuff from exynos5433_cmu_probe() into
samsung_cmu_register_one() ) can be done later, when I add PM ops into
clk-exynos850.

> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-10-05 10:28     ` Sam Protsenko
@ 2021-10-06 10:49       ` Krzysztof Kozlowski
  2021-10-06 13:31         ` Sam Protsenko
  0 siblings, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-06 10:49 UTC (permalink / raw)
  To: Sam Protsenko, Chanwoo Choi, Sylwester Nawrocki
  Cc: Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette, Ryu Euiyoul, Tom Gall,
	Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On 05/10/2021 12:28, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 19:37, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>>
>> Hi,
>>
>> You don't add clock ids for the all defined clocks in clk-exynos850.c.
>> I recommend that add all clock ids for the defined clocks if possible.
>>
>> If you want to change the parent clock of mux or change the clock rate
>> of div rate for some clocks, you have to touch the files as following:
>> - include/dt-bindings/clock/exynos850.h
>> - drivers/clk/samsung/clk-exynos850.c
>> - exynos850 dt files
>>
>> If you define the clock ids for all clocks added to this patchset,
>> you can change the parent or rate by just editing the dt files.
>>
> 
> Hi Chanwoo,
> 
> I see your point. But I have intentionally omitted some clock ids,
> which can't be / shouldn't be used by consumers in device tree.
> Actually I took that idea from clk-exynos7.c.
> 
> Krzysztof, Sylwester: can you please advice if all clock ids should be
> defined, or only those that are going to be used in dts clk consumers?
> I don't mind reworking the patch, just want to be sure which design
> approach we want to follow.
> 

I would advise to define all clock IDs, unless the clock really, really
should not be used. Why do you think several clocks should not be used?
Have in mind it is not only about consumers but also clock reparenting
and assigning rates.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-09-15 12:51   ` Sylwester Nawrocki
@ 2021-10-06 11:18     ` Sam Protsenko
  2021-10-06 12:45       ` Krzysztof Kozlowski
  2021-10-09 18:49       ` Sylwester Nawrocki
  0 siblings, 2 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-10-06 11:18 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Krzysztof Kozlowski, Paweł Chmiel, Chanwoo Choi,
	Tomasz Figa, Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz,
	Amit Pundir, devicetree, linux-arm Mailing List, linux-clk,
	Linux Kernel Mailing List, Linux Samsung SOC, Michael Turquette,
	Stephen Boyd, Rob Herring

On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
>
> Hi,
>
> On 14.09.2021 17:56, Sam Protsenko wrote:
> > By default if bus clock has no users its "enable count" value is 0. It
> > might be actually running if it's already enabled in bootloader, but
> > then in some cases it can be disabled by mistake. For example, such case
> > was observed when dw_mci_probe() enabled bus clock, then failed to do
> > something and disabled that bus clock on error path. After that even
> > attempt to read the 'clk_summary' file in DebugFS freezed forever, as
> > CMU bus clock ended up being disabled and it wasn't possible to access
> > CMU registers anymore.
> >
> > To avoid such cases, CMU driver must increment the ref count for that
> > bus clock by running clk_prepare_enable(). There is already existing
> > '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
> > It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> > support for runtime PM"). But the clock is actually enabled only in
> > Exynos5433 clock driver. Let's mimic what is done there in generic
> > samsung_cmu_register_one() function, so other drivers can benefit from
> > that `.clk_name' field. As was described above, it might be helpful not
> > only for PM reasons, but also to prevent possible erroneous clock gating
> > on error paths.
> >
> > Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
> > flag for corresponding gate clocks. But that might be not very good
> > design decision, as we might still want to disable that bus clock, e.g.
> > on PM suspend.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/clk/samsung/clk.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> > index 1949ae7851b2..da65149fa502 100644
> > --- a/drivers/clk/samsung/clk.c
> > +++ b/drivers/clk/samsung/clk.c
> > @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
> >
> >       ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
> >
> > +     /* Keep bus clock running, so it's possible to access CMU registers */
> > +     if (cmu->clk_name) {
> > +             struct clk *bus_clk;
> > +
> > +             bus_clk = __clk_lookup(cmu->clk_name);
> > +             if (bus_clk) {
> > +                     clk_prepare_enable(bus_clk);
> > +             } else {
> > +                     pr_err("%s: could not find bus clock %s\n", __func__,
> > +                            cmu->clk_name);
> > +             }
> > +     }
> > +
> >       if (cmu->pll_clks)
> >               samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks,
> >                       reg_base);
>
> I would suggest to implement runtime PM ops in your driver instead, even though
> those would initially only contain single clk enable/disable. Things like
> the clk_summary will work then thanks to runtime PM support in the clk core
> (see clk_pm_runtime_* calls).

Can you please elaborate more? I don't see how adding PM ops would
solve the problem I'm trying to address, which is keeping core bus
clocks always running. For example, I'm looking at clk-exynos5433.c
implementation, which enables bus clock on resume path:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
static int __maybe_unused exynos5433_cmu_resume(struct device *dev)
{
    ...
    clk_prepare_enable(data->clk);
    ...
}
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

But that resume operation won't be called on driver init, because it
configures runtime PM like this:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
static int __init exynos5433_cmu_probe(struct platform_device *pdev)
{
    ...
    /*
     * Enable runtime PM here to allow the clock core using runtime PM
     * for the registered clocks. Additionally, we increase the runtime
     * PM usage count before registering the clocks, to prevent the
     * clock core from runtime suspending the device.
     */
    pm_runtime_get_noresume(dev);
    pm_runtime_set_active(dev);
    pm_runtime_enable(dev);
    ...
    pm_runtime_put_sync(dev);
    ...
}
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

When I tried to implement the same in my driver, only suspend function
is called during kernel startup.

Anyway, even clk-exynos5433.c driver (which also implements PM ops)
does the same for core bus clocks:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
static int __init exynos5433_cmu_probe(struct platform_device *pdev)
{
    ...
    if (info->clk_name)
        data->clk = clk_get(dev, info->clk_name);
    clk_prepare_enable(data->clk);
    ...
}
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

So it looks like separate feature to me. Not sure how that can be
implemented only by adding PM ops. Also, my board lacks PM support in
upstream kernel right now, so I probably won't be able to test PM ops
if I implement those, that's why I decided to skip it for now.

> We could also make common runtime PM suspend/resume helpers but I wouldn't focus
> on that too much now, it could well be done later.
> And please avoid introducing new __clk_lookup() calls.
>

The reason I used __clk_lookup() is that it's the only API that works
in that case. I tried to use clk_get(), but we lack 'struct dev'
pointer in samsung_cmu_register_one(), so when providing dev=NULL into
clk_get() it fails to get the clock. That's happening because
LIST_HEAD(clocks) is probably empty in clkdev.c. So this chain fails:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
clk_get()    // dev = NULL
  v
__clk_get_sys()
  v
clk_find_hw()
  v
clk_find()   // returns 0, because LIST_HEAD(clocks) is empty
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

I saw your patches which get rid of __clk_lookup() usage by accessing
ctx->clk_data.hws[], but that requires using clock index, not name.
'struct samsung_cmu_info' only stores bus clock name (.clk_name),
which seems logical to me, so we can't get away from using
__clk_lookup() in that case without refactoring 'struct
samsung_cmu_info' first.

All that said, I suggest next: I'll pull the code from this patch into
clk-exynos850.c, adding platform_driver registration there, so I can
actually use clk_get() for getting bus clocks. As for PM ops, I'd like
to skip it for now, if you don't mind, as I can't fully test those.
Otherwise please elaborate more on how PM ops can solve this problem.

Thanks!

> --
> Regards,
> Sylwester

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-10-06 10:46     ` Sam Protsenko
@ 2021-10-06 12:38       ` Krzysztof Kozlowski
  2021-10-06 13:29         ` Sam Protsenko
  0 siblings, 1 reply; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-06 12:38 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On 06/10/2021 12:46, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 14/09/2021 17:56, Sam Protsenko wrote:
>>> By default if bus clock has no users its "enable count" value is 0. It
>>> might be actually running if it's already enabled in bootloader, but
>>> then in some cases it can be disabled by mistake. For example, such case
>>> was observed when dw_mci_probe() enabled bus clock, then failed to do
>>> something and disabled that bus clock on error path. After that even
>>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
>>> CMU bus clock ended up being disabled and it wasn't possible to access
>>> CMU registers anymore.
>>>
>>> To avoid such cases, CMU driver must increment the ref count for that
>>> bus clock by running clk_prepare_enable(). There is already existing
>>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
>>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
>>> support for runtime PM"). But the clock is actually enabled only in
>>> Exynos5433 clock driver. Let's mimic what is done there in generic
>>> samsung_cmu_register_one() function, so other drivers can benefit from
>>> that `.clk_name' field. As was described above, it might be helpful not
>>> only for PM reasons, but also to prevent possible erroneous clock gating
>>> on error paths.
>>>
>>> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
>>> flag for corresponding gate clocks. But that might be not very good
>>> design decision, as we might still want to disable that bus clock, e.g.
>>> on PM suspend.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>>  drivers/clk/samsung/clk.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>>> index 1949ae7851b2..da65149fa502 100644
>>> --- a/drivers/clk/samsung/clk.c
>>> +++ b/drivers/clk/samsung/clk.c
>>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>>>
>>>       ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
>>>
>>> +     /* Keep bus clock running, so it's possible to access CMU registers */
>>> +     if (cmu->clk_name) {
>>> +             struct clk *bus_clk;
>>> +
>>> +             bus_clk = __clk_lookup(cmu->clk_name);
>>> +             if (bus_clk) {
>>> +                     clk_prepare_enable(bus_clk);
>>> +             } else {
>>> +                     pr_err("%s: could not find bus clock %s\n", __func__,
>>> +                            cmu->clk_name);
>>> +             }
>>> +     }
>>> +
>>
>> Solving this problem in generic way makes sense but your solution is
>> insufficient. You skipped suspend/resume paths and in such case you
>> should remove the Exynos5433-specific code.
>>
> 
> Keeping core bus clocks always running seems like a separate
> independent feature to me (not related to suspend/resume). It's
> mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> support for runtime PM") this way:
> 
>     "Also for each CMU there is one special parent clock, which has to
> be enabled all the time when any access to CMU registers is being
> done."
> 
> Why do you think suspend/resume paths have to be implemented along
> with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not
> implemented on my board yet and I can't test it.

You can skip the runtime PM, so keep your patch almost like it is now
(in respect to Sylwester's comment about __clk_lookup). However now the
Exynos5433 will enable the clk_name twice: here and in
exynos5433_cmu_probe().

If you keep this approach, you need to remove duplicated part in
exynos5433_cmu_probe()...

> 
> If you are suggesting moving all stuff from exynos5433_cmu_probe()
> into samsung_cmu_register_one(), it would take passing platform_device
> there, and implementing all PM related operations. I guess it's not a
> super easy task, as it would require converting clk-exynos7 to
> platform_driver for instance, and re-testing everything on exynos5433
> and exynos7 boards (which I don't have).
> 
> What do you say if I pull that code to clk-exynos850.c instead for v2?
> Refactoring (merging stuff from exynos5433_cmu_probe() into
> samsung_cmu_register_one() ) can be done later, when I add PM ops into
> clk-exynos850.
> 
>> Best regards,
>> Krzysztof


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-10-06 11:18     ` Sam Protsenko
@ 2021-10-06 12:45       ` Krzysztof Kozlowski
  2021-10-09 18:49       ` Sylwester Nawrocki
  1 sibling, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-06 12:45 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki
  Cc: Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC, Michael Turquette, Stephen Boyd, Rob Herring

On 06/10/2021 13:18, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> Hi,
>>
>> On 14.09.2021 17:56, Sam Protsenko wrote:
>>> By default if bus clock has no users its "enable count" value is 0. It
>>> might be actually running if it's already enabled in bootloader, but
>>> then in some cases it can be disabled by mistake. For example, such case
>>> was observed when dw_mci_probe() enabled bus clock, then failed to do
>>> something and disabled that bus clock on error path. After that even
>>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
>>> CMU bus clock ended up being disabled and it wasn't possible to access
>>> CMU registers anymore.
>>>
>>> To avoid such cases, CMU driver must increment the ref count for that
>>> bus clock by running clk_prepare_enable(). There is already existing
>>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
>>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
>>> support for runtime PM"). But the clock is actually enabled only in
>>> Exynos5433 clock driver. Let's mimic what is done there in generic
>>> samsung_cmu_register_one() function, so other drivers can benefit from
>>> that `.clk_name' field. As was described above, it might be helpful not
>>> only for PM reasons, but also to prevent possible erroneous clock gating
>>> on error paths.
>>>
>>> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
>>> flag for corresponding gate clocks. But that might be not very good
>>> design decision, as we might still want to disable that bus clock, e.g.
>>> on PM suspend.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>>  drivers/clk/samsung/clk.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>>> index 1949ae7851b2..da65149fa502 100644
>>> --- a/drivers/clk/samsung/clk.c
>>> +++ b/drivers/clk/samsung/clk.c
>>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>>>
>>>       ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
>>>
>>> +     /* Keep bus clock running, so it's possible to access CMU registers */
>>> +     if (cmu->clk_name) {
>>> +             struct clk *bus_clk;
>>> +
>>> +             bus_clk = __clk_lookup(cmu->clk_name);
>>> +             if (bus_clk) {
>>> +                     clk_prepare_enable(bus_clk);
>>> +             } else {
>>> +                     pr_err("%s: could not find bus clock %s\n", __func__,
>>> +                            cmu->clk_name);
>>> +             }
>>> +     }
>>> +
>>>       if (cmu->pll_clks)
>>>               samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks,
>>>                       reg_base);
>>
>> I would suggest to implement runtime PM ops in your driver instead, even though
>> those would initially only contain single clk enable/disable. Things like
>> the clk_summary will work then thanks to runtime PM support in the clk core
>> (see clk_pm_runtime_* calls).
> 
> Can you please elaborate more? I don't see how adding PM ops would
> solve the problem I'm trying to address, which is keeping core bus
> clocks always running. For example, I'm looking at clk-exynos5433.c
> implementation, which enables bus clock on resume path:
> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> static int __maybe_unused exynos5433_cmu_resume(struct device *dev)
> {
>     ...
>     clk_prepare_enable(data->clk);
>     ...
> }
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> 
> But that resume operation won't be called on driver init, because it
> configures runtime PM like this:

The device will get suspended (like you say) till the first usage, which
will resume it and thus make the clock enabled.

> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> static int __init exynos5433_cmu_probe(struct platform_device *pdev)
> {
>     ...
>     /*
>      * Enable runtime PM here to allow the clock core using runtime PM
>      * for the registered clocks. Additionally, we increase the runtime
>      * PM usage count before registering the clocks, to prevent the
>      * clock core from runtime suspending the device.
>      */
>     pm_runtime_get_noresume(dev);
>     pm_runtime_set_active(dev);
>     pm_runtime_enable(dev);
>     ...
>     pm_runtime_put_sync(dev);
>     ...
> }
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> 
> When I tried to implement the same in my driver, only suspend function
> is called during kernel startup.
> 
> Anyway, even clk-exynos5433.c driver (which also implements PM ops)
> does the same for core bus clocks:
> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> static int __init exynos5433_cmu_probe(struct platform_device *pdev)
> {
>     ...
>     if (info->clk_name)
>         data->clk = clk_get(dev, info->clk_name);
>     clk_prepare_enable(data->clk);
>     ...
> }
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> 
> So it looks like separate feature to me. Not sure how that can be
> implemented only by adding PM ops. Also, my board lacks PM support in
> upstream kernel right now, so I probably won't be able to test PM ops
> if I implement those, that's why I decided to skip it for now.

In general you need runtime PM to make a proper clock driver. You can
skip it, just like most of our early drivers skipped it, including
Exynos7, but it's not good in the long run. You might later hit for
example imprecise aborts when enumerating clocks (/sys/kernel/debug/clk)
or power domains.

To me it is fine with skipping runtime PM, but using platform driver now
seems good choice. When writing the code, use rather Exynos5433 as an
example, not Exynos7. The former was extensively developed and used for
mainline. The latter was only part of rather early bringup of platform
and lacks several features/drivers/DT.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-10-05 11:36     ` Sam Protsenko
@ 2021-10-06 12:46       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-06 12:46 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki
  Cc: Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz, Amit Pundir,
	devicetree, linux-arm Mailing List, linux-clk,
	Linux Kernel Mailing List, Linux Samsung SOC, Michael Turquette,
	Stephen Boyd, Rob Herring, Tomasz Figa, Chanwoo Choi,
	Paweł Chmiel

On 05/10/2021 13:36, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 16:07, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>>
>> On 14.09.2021 17:56, Sam Protsenko wrote:
>>> +static void __init exynos850_cmu_top_init(struct device_node *np)
>>> +{
>>> +     exynos850_init_clocks(np, top_clk_regs, ARRAY_SIZE(top_clk_regs));
>>> +     samsung_cmu_register_one(np, &top_cmu_info);
>>> +}
>>> +
>>> +CLK_OF_DECLARE(exynos850_cmu_top, "samsung,exynos850-cmu-top",
>>> +            exynos850_cmu_top_init);
>>
>> Was there anything preventing you from making it a platform driver instead?
>>
> 
> Can you please elaborate on benefits of adding platform driver? I
> don't implement PM ops for now, and I can see that clk-exynos7.c does
> not add platform driver as well... clk-exynos5433.c seems to use
> platform_driver for PM ops only.

I said it in response to patch 1, so just for the record:
Exynos7 is not the example you are looking for. :) Exynos5433 is.


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver
  2021-10-05 11:29     ` Sam Protsenko
@ 2021-10-06 12:50       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-06 12:50 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On 05/10/2021 13:29, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 11:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
> 
>>> +             val = ioread32(reg);
>>> +             val |= GATE_MANUAL;
>>> +             val &= ~GATE_ENABLE_HWACG;
>>> +             iowrite32(val, reg);
>>
>> All other drivers use readl/writel, so how about keeping it consistent?
>>
> 
> Ok. Though io* variants looks better to me (API names consistent with
> ioremap/iounmap) :)

The io* variants are for PCI I/O and I/O port. Since we know this is
MMIO, all drivers use regular readX/writeX, so let's keep it the same.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-10-06 12:38       ` Krzysztof Kozlowski
@ 2021-10-06 13:29         ` Sam Protsenko
  2021-10-08  6:50           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 38+ messages in thread
From: Sam Protsenko @ 2021-10-06 13:29 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On Wed, 6 Oct 2021 at 15:38, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 06/10/2021 12:46, Sam Protsenko wrote:
> > On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 14/09/2021 17:56, Sam Protsenko wrote:
> >>> By default if bus clock has no users its "enable count" value is 0. It
> >>> might be actually running if it's already enabled in bootloader, but
> >>> then in some cases it can be disabled by mistake. For example, such case
> >>> was observed when dw_mci_probe() enabled bus clock, then failed to do
> >>> something and disabled that bus clock on error path. After that even
> >>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
> >>> CMU bus clock ended up being disabled and it wasn't possible to access
> >>> CMU registers anymore.
> >>>
> >>> To avoid such cases, CMU driver must increment the ref count for that
> >>> bus clock by running clk_prepare_enable(). There is already existing
> >>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
> >>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> >>> support for runtime PM"). But the clock is actually enabled only in
> >>> Exynos5433 clock driver. Let's mimic what is done there in generic
> >>> samsung_cmu_register_one() function, so other drivers can benefit from
> >>> that `.clk_name' field. As was described above, it might be helpful not
> >>> only for PM reasons, but also to prevent possible erroneous clock gating
> >>> on error paths.
> >>>
> >>> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
> >>> flag for corresponding gate clocks. But that might be not very good
> >>> design decision, as we might still want to disable that bus clock, e.g.
> >>> on PM suspend.
> >>>
> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> >>> ---
> >>>  drivers/clk/samsung/clk.c | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> >>> index 1949ae7851b2..da65149fa502 100644
> >>> --- a/drivers/clk/samsung/clk.c
> >>> +++ b/drivers/clk/samsung/clk.c
> >>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
> >>>
> >>>       ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
> >>>
> >>> +     /* Keep bus clock running, so it's possible to access CMU registers */
> >>> +     if (cmu->clk_name) {
> >>> +             struct clk *bus_clk;
> >>> +
> >>> +             bus_clk = __clk_lookup(cmu->clk_name);
> >>> +             if (bus_clk) {
> >>> +                     clk_prepare_enable(bus_clk);
> >>> +             } else {
> >>> +                     pr_err("%s: could not find bus clock %s\n", __func__,
> >>> +                            cmu->clk_name);
> >>> +             }
> >>> +     }
> >>> +
> >>
> >> Solving this problem in generic way makes sense but your solution is
> >> insufficient. You skipped suspend/resume paths and in such case you
> >> should remove the Exynos5433-specific code.
> >>
> >
> > Keeping core bus clocks always running seems like a separate
> > independent feature to me (not related to suspend/resume). It's
> > mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
> > support for runtime PM") this way:
> >
> >     "Also for each CMU there is one special parent clock, which has to
> > be enabled all the time when any access to CMU registers is being
> > done."
> >
> > Why do you think suspend/resume paths have to be implemented along
> > with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not
> > implemented on my board yet and I can't test it.
>
> You can skip the runtime PM, so keep your patch almost like it is now
> (in respect to Sylwester's comment about __clk_lookup). However now the
> Exynos5433 will enable the clk_name twice: here and in
> exynos5433_cmu_probe().
>
> If you keep this approach, you need to remove duplicated part in
> exynos5433_cmu_probe()...
>

My patch is only touching samsung_cmu_register_one(), and
exynos5433_cmu_probe() doesn't call samsung_cmu_register_one(). So I
don't think there can be a problem there. Or I'm missing something?

samsung_cmu_register_one() is actually called from 5433 clk driver,
but only from CMUs registered with CLK_OF_DECLARE(), and those are not
setting .clk_name field, so my code is not affecting those either.

Real problem I can see is that I can't avoid using __clk_lookup() if I
implement that code in samsung_cmu_register_one(). Tried to do use
clk_get(NULL, ...) instead, but it doesn't work with 1st param (dev)
being NULL, because samsung_clk_register_*() functions don't register
clkdev (only samsung_clk_register_fixed_rate() does), hence
LIST_HEAD(clocks) is empty in clkdev.c, and clk_get() fails, when not
provided with actual 'dev' param, which in turn is not present in
samsung_cmu_register_one()...

About using platform_driver: as I can see from clk-exynos5433.c, only
CMUs which belong to Power Domains are registered as platform_driver.
Rest of CMUs are registered using CLK_OF_DECLARE(), thus they don't
get platform_device param. That makes it harder to avoid using
__clk_lookup() inside samsung_cmu_register_one().

All that said, I feel like correct way to implement this patch would be:
  1. Register all PD-capable CMUs as platform_driver in clk-exynos850
(all CMUs except CMU_TOP)
  2. Move bus clock enablement code from samsung_cmu_register_one() to
corresponding clk-exynos850 probe function

This way I would be able to use clk_get(dev, ...) instead of
__clk_lookup(), and that won't affect any existing code for sure. Code
will be more unified w.r.t. how it's done in clk-exynos5433, and
platform_device will be a foundation for implementing PM ops later.
Taking into account how much design decisions should be done for using
that in common code -- I'd say let's do that later, as a separate
refactoring activity.

Do you think that makes sense?

Thanks!

> >
> > If you are suggesting moving all stuff from exynos5433_cmu_probe()
> > into samsung_cmu_register_one(), it would take passing platform_device
> > there, and implementing all PM related operations. I guess it's not a
> > super easy task, as it would require converting clk-exynos7 to
> > platform_driver for instance, and re-testing everything on exynos5433
> > and exynos7 boards (which I don't have).
> >
> > What do you say if I pull that code to clk-exynos850.c instead for v2?
> > Refactoring (merging stuff from exynos5433_cmu_probe() into
> > samsung_cmu_register_one() ) can be done later, when I add PM ops into
> > clk-exynos850.
> >
> >> Best regards,
> >> Krzysztof
>
>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU
  2021-10-06 10:49       ` Krzysztof Kozlowski
@ 2021-10-06 13:31         ` Sam Protsenko
  0 siblings, 0 replies; 38+ messages in thread
From: Sam Protsenko @ 2021-10-06 13:31 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Chanwoo Choi, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette, Ryu Euiyoul, Tom Gall, Sumit Semwal,
	John Stultz, Amit Pundir, devicetree, linux-arm Mailing List,
	linux-clk, Linux Kernel Mailing List, Linux Samsung SOC

On Wed, 6 Oct 2021 at 13:49, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 05/10/2021 12:28, Sam Protsenko wrote:
> > On Wed, 15 Sept 2021 at 19:37, Chanwoo Choi <cwchoi00@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> You don't add clock ids for the all defined clocks in clk-exynos850.c.
> >> I recommend that add all clock ids for the defined clocks if possible.
> >>
> >> If you want to change the parent clock of mux or change the clock rate
> >> of div rate for some clocks, you have to touch the files as following:
> >> - include/dt-bindings/clock/exynos850.h
> >> - drivers/clk/samsung/clk-exynos850.c
> >> - exynos850 dt files
> >>
> >> If you define the clock ids for all clocks added to this patchset,
> >> you can change the parent or rate by just editing the dt files.
> >>
> >
> > Hi Chanwoo,
> >
> > I see your point. But I have intentionally omitted some clock ids,
> > which can't be / shouldn't be used by consumers in device tree.
> > Actually I took that idea from clk-exynos7.c.
> >
> > Krzysztof, Sylwester: can you please advice if all clock ids should be
> > defined, or only those that are going to be used in dts clk consumers?
> > I don't mind reworking the patch, just want to be sure which design
> > approach we want to follow.
> >
>
> I would advise to define all clock IDs, unless the clock really, really
> should not be used. Why do you think several clocks should not be used?
> Have in mind it is not only about consumers but also clock reparenting
> and assigning rates.
>

Thanks! Will be done in v2.

>
> Best regards,
> Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-10-06 13:29         ` Sam Protsenko
@ 2021-10-08  6:50           ` Krzysztof Kozlowski
  0 siblings, 0 replies; 38+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-08  6:50 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Sylwester Nawrocki, Paweł Chmiel, Chanwoo Choi, Tomasz Figa,
	Rob Herring, Stephen Boyd, Michael Turquette, Ryu Euiyoul,
	Tom Gall, Sumit Semwal, John Stultz, Amit Pundir, devicetree,
	linux-arm Mailing List, linux-clk, Linux Kernel Mailing List,
	Linux Samsung SOC

On 06/10/2021 15:29, Sam Protsenko wrote:
> On Wed, 6 Oct 2021 at 15:38, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 06/10/2021 12:46, Sam Protsenko wrote:
>>> On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@canonical.com> wrote:
>>>>
>>>> On 14/09/2021 17:56, Sam Protsenko wrote:
>>>>> By default if bus clock has no users its "enable count" value is 0. It
>>>>> might be actually running if it's already enabled in bootloader, but
>>>>> then in some cases it can be disabled by mistake. For example, such case
>>>>> was observed when dw_mci_probe() enabled bus clock, then failed to do
>>>>> something and disabled that bus clock on error path. After that even
>>>>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
>>>>> CMU bus clock ended up being disabled and it wasn't possible to access
>>>>> CMU registers anymore.
>>>>>
>>>>> To avoid such cases, CMU driver must increment the ref count for that
>>>>> bus clock by running clk_prepare_enable(). There is already existing
>>>>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
>>>>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
>>>>> support for runtime PM"). But the clock is actually enabled only in
>>>>> Exynos5433 clock driver. Let's mimic what is done there in generic
>>>>> samsung_cmu_register_one() function, so other drivers can benefit from
>>>>> that `.clk_name' field. As was described above, it might be helpful not
>>>>> only for PM reasons, but also to prevent possible erroneous clock gating
>>>>> on error paths.
>>>>>
>>>>> Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
>>>>> flag for corresponding gate clocks. But that might be not very good
>>>>> design decision, as we might still want to disable that bus clock, e.g.
>>>>> on PM suspend.
>>>>>
>>>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>>>> ---
>>>>>  drivers/clk/samsung/clk.c | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>>>>> index 1949ae7851b2..da65149fa502 100644
>>>>> --- a/drivers/clk/samsung/clk.c
>>>>> +++ b/drivers/clk/samsung/clk.c
>>>>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>>>>>
>>>>>       ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
>>>>>
>>>>> +     /* Keep bus clock running, so it's possible to access CMU registers */
>>>>> +     if (cmu->clk_name) {
>>>>> +             struct clk *bus_clk;
>>>>> +
>>>>> +             bus_clk = __clk_lookup(cmu->clk_name);
>>>>> +             if (bus_clk) {
>>>>> +                     clk_prepare_enable(bus_clk);
>>>>> +             } else {
>>>>> +                     pr_err("%s: could not find bus clock %s\n", __func__,
>>>>> +                            cmu->clk_name);
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>
>>>> Solving this problem in generic way makes sense but your solution is
>>>> insufficient. You skipped suspend/resume paths and in such case you
>>>> should remove the Exynos5433-specific code.
>>>>
>>>
>>> Keeping core bus clocks always running seems like a separate
>>> independent feature to me (not related to suspend/resume). It's
>>> mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
>>> support for runtime PM") this way:
>>>
>>>     "Also for each CMU there is one special parent clock, which has to
>>> be enabled all the time when any access to CMU registers is being
>>> done."
>>>
>>> Why do you think suspend/resume paths have to be implemented along
>>> with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not
>>> implemented on my board yet and I can't test it.
>>
>> You can skip the runtime PM, so keep your patch almost like it is now
>> (in respect to Sylwester's comment about __clk_lookup). However now the
>> Exynos5433 will enable the clk_name twice: here and in
>> exynos5433_cmu_probe().
>>
>> If you keep this approach, you need to remove duplicated part in
>> exynos5433_cmu_probe()...
>>
> 
> My patch is only touching samsung_cmu_register_one(), and
> exynos5433_cmu_probe() doesn't call samsung_cmu_register_one(). So I
> don't think there can be a problem there. Or I'm missing something?
> 
> samsung_cmu_register_one() is actually called from 5433 clk driver,
> but only from CMUs registered with CLK_OF_DECLARE(), and those are not
> setting .clk_name field, so my code is not affecting those either.

You are right.

> 
> Real problem I can see is that I can't avoid using __clk_lookup() if I
> implement that code in samsung_cmu_register_one(). Tried to do use
> clk_get(NULL, ...) instead, but it doesn't work with 1st param (dev)
> being NULL, because samsung_clk_register_*() functions don't register
> clkdev (only samsung_clk_register_fixed_rate() does), hence
> LIST_HEAD(clocks) is empty in clkdev.c, and clk_get() fails, when not
> provided with actual 'dev' param, which in turn is not present in
> samsung_cmu_register_one()...
> 
> About using platform_driver: as I can see from clk-exynos5433.c, only
> CMUs which belong to Power Domains are registered as platform_driver.
> Rest of CMUs are registered using CLK_OF_DECLARE(), thus they don't
> get platform_device param. That makes it harder to avoid using
> __clk_lookup() inside samsung_cmu_register_one().
> 
> All that said, I feel like correct way to implement this patch would be:
>   1. Register all PD-capable CMUs as platform_driver in clk-exynos850
> (all CMUs except CMU_TOP)
>   2. Move bus clock enablement code from samsung_cmu_register_one() to
> corresponding clk-exynos850 probe function
> 
> This way I would be able to use clk_get(dev, ...) instead of
> __clk_lookup(), and that won't affect any existing code for sure. Code
> will be more unified w.r.t. how it's done in clk-exynos5433, and
> platform_device will be a foundation for implementing PM ops later.
> Taking into account how much design decisions should be done for using
> that in common code -- I'd say let's do that later, as a separate
> refactoring activity.
> 
> Do you think that makes sense?

Yes, makes sense. Thank you!


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
  2021-10-06 11:18     ` Sam Protsenko
  2021-10-06 12:45       ` Krzysztof Kozlowski
@ 2021-10-09 18:49       ` Sylwester Nawrocki
  1 sibling, 0 replies; 38+ messages in thread
From: Sylwester Nawrocki @ 2021-10-09 18:49 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Krzysztof Kozlowski, Paweł Chmiel, Chanwoo Choi,
	Tomasz Figa, Ryu Euiyoul, Tom Gall, Sumit Semwal, John Stultz,
	Amit Pundir, devicetree, linux-arm Mailing List, linux-clk,
	Linux Kernel Mailing List, Linux Samsung SOC, Michael Turquette,
	Stephen Boyd, Rob Herring, Sylwester Nawrocki

On 06.10.2021 13:18, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 14.09.2021 17:56, Sam Protsenko wrote:
>>> By default if bus clock has no users its "enable count" value is 0. It
>>> might be actually running if it's already enabled in bootloader, but
>>> then in some cases it can be disabled by mistake. For example, such case
>>> was observed when dw_mci_probe() enabled bus clock, then failed to do
>>> something and disabled that bus clock on error path. After that even
>>> attempt to read the 'clk_summary' file in DebugFS freezed forever, as
>>> CMU bus clock ended up being disabled and it wasn't possible to access
>>> CMU registers anymore.
>>>
>>> To avoid such cases, CMU driver must increment the ref count for that
>>> bus clock by running clk_prepare_enable(). There is already existing
>>> '.clk_name' field in struct samsung_cmu_info, exactly for that reason.
>>> It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
>>> support for runtime PM"). But the clock is actually enabled only in
>>> Exynos5433 clock driver. Let's mimic what is done there in generic
>>> samsung_cmu_register_one() function, so other drivers can benefit from
>>> that `.clk_name' field. As was described above, it might be helpful not
>>> only for PM reasons, but also to prevent possible erroneous clock gating
>>> on error paths.

>>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>>> index 1949ae7851b2..da65149fa502 100644
>>> --- a/drivers/clk/samsung/clk.c
>>> +++ b/drivers/clk/samsung/clk.c
>>> @@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(
>>>
>>>        ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);
>>>
>>> +     /* Keep bus clock running, so it's possible to access CMU registers */
>>> +     if (cmu->clk_name) {
>>> +             struct clk *bus_clk;
>>> +
>>> +             bus_clk = __clk_lookup(cmu->clk_name);
>>> +             if (bus_clk) {
>>> +                     clk_prepare_enable(bus_clk);
>>> +             } else {
>>> +                     pr_err("%s: could not find bus clock %s\n", __func__,
>>> +                            cmu->clk_name);
>>> +             }
>>> +     }
>>> +
>>>        if (cmu->pll_clks)
>>>                samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks,
>>>                        reg_base);
>>
>> I would suggest to implement runtime PM ops in your driver instead, even though
>> those would initially only contain single clk enable/disable. Things like
>> the clk_summary will work then thanks to runtime PM support in the clk core
>> (see clk_pm_runtime_* calls).
> 
> Can you please elaborate more? I don't see how adding PM ops would
> solve the problem I'm trying to address, which is keeping core bus
> clocks always running. For example, I'm looking at clk-exynos5433.c

I missed the fact that there is usually a specific SFR sequence required
for disabling the CMU root (and APB) clock. We would need to figure out what
an exact sequence is for each CMU, similarly as is done in clk-exynos5433,
then keeping the CMU source clock always enabled shouldn't be required.
I'm fine with just enabling the APB clocks in probe() until proper CMU
suspend/resume support is added.

> implementation, which enables bus clock on resume path:
> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> static int __maybe_unused exynos5433_cmu_resume(struct device *dev)
> {
>      ...
>      clk_prepare_enable(data->clk);
>      ...
> }
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> 
> But that resume operation won't be called on driver init, because it
> configures runtime PM like this:
> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> static int __init exynos5433_cmu_probe(struct platform_device *pdev)
> {
>      ...
>      /*
>       * Enable runtime PM here to allow the clock core using runtime PM
>       * for the registered clocks. Additionally, we increase the runtime
>       * PM usage count before registering the clocks, to prevent the
>       * clock core from runtime suspending the device.
>       */
>      pm_runtime_get_noresume(dev);
>      pm_runtime_set_active(dev);
>      pm_runtime_enable(dev);
>      ...
>      pm_runtime_put_sync(dev);
>      ...
> }
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> 
> When I tried to implement the same in my driver, only suspend function
> is called during kernel startup.

I think some of the clocks supplied by a CMU need to be in use
(e.g. clk_prepare()) to get the resume op in the CMU driver invoked.

> Anyway, even clk-exynos5433.c driver (which also implements PM ops)
> does the same for core bus clocks:
> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> static int __init exynos5433_cmu_probe(struct platform_device *pdev)
> {
>      ...
>      if (info->clk_name)
>          data->clk = clk_get(dev, info->clk_name);
>      clk_prepare_enable(data->clk);
>      ...
> }
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

Enabling the clock corresponds with the pm_runtime_set_active() call you
pointed out above. Such pattern also ensures the clock will stay enabled
when CONFIG_PM_RUNTIME is disabled.

> So it looks like separate feature to me. Not sure how that can be
> implemented only by adding PM ops. Also, my board lacks PM support in
> upstream kernel right now, so I probably won't be able to test PM ops
> if I implement those, that's why I decided to skip it for now.

It is not really a separate feature, I think having the clocks permanently
enabled is not something we would like to end up with. It would need to be
revisited anyway when adding the power domains support.

>> We could also make common runtime PM suspend/resume helpers but I wouldn't 
>> focus on that too much now, it could well be done later.
>> And please avoid introducing new __clk_lookup() calls.
> 
> The reason I used __clk_lookup() is that it's the only API that works
> in that case. I tried to use clk_get(), but we lack 'struct dev'
> pointer in samsung_cmu_register_one(), so when providing dev=NULL into
> clk_get() it fails to get the clock. That's happening because
> LIST_HEAD(clocks) is probably empty in clkdev.c. So this chain fails:
> 
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> clk_get()    // dev = NULL
>    v
> __clk_get_sys()
>    v
> clk_find_hw()
>    v
> clk_find()   // returns 0, because LIST_HEAD(clocks) is empty
> <<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
> 
> I saw your patches which get rid of __clk_lookup() usage by accessing
> ctx->clk_data.hws[], but that requires using clock index, not name.
> 'struct samsung_cmu_info' only stores bus clock name (.clk_name),
> which seems logical to me, so we can't get away from using
> __clk_lookup() in that case without refactoring 'struct
> samsung_cmu_info' first.

You need device pointer to get the CMU input clocks as specified in DT.
clk_get with NULL device pointer and global clock name will now only work
on Samsung non-DT platforms, for DT-only SoCs we don't register clkdev
entries at all (see samsung_clk_register_alias()).

> All that said, I suggest next: I'll pull the code from this patch into
> clk-exynos850.c, adding platform_driver registration there, so I can
> actually use clk_get() for getting bus clocks. As for PM ops, I'd like
> to skip it for now, if you don't mind, as I can't fully test those.

Sounds good to me, thank you for working on this.

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2021-10-09 18:49 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
2021-09-15  8:21   ` Krzysztof Kozlowski
2021-10-06 10:46     ` Sam Protsenko
2021-10-06 12:38       ` Krzysztof Kozlowski
2021-10-06 13:29         ` Sam Protsenko
2021-10-08  6:50           ` Krzysztof Kozlowski
2021-09-15 12:51   ` Sylwester Nawrocki
2021-10-06 11:18     ` Sam Protsenko
2021-10-06 12:45       ` Krzysztof Kozlowski
2021-10-09 18:49       ` Sylwester Nawrocki
2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko
2021-09-15  8:24   ` Krzysztof Kozlowski
2021-09-15 15:59   ` Chanwoo Choi
2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko
2021-09-15  8:26   ` Krzysztof Kozlowski
2021-09-15 16:11   ` Chanwoo Choi
2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
2021-09-15  8:27   ` Krzysztof Kozlowski
2021-09-15 16:37   ` Chanwoo Choi
2021-10-05 10:28     ` Sam Protsenko
2021-10-06 10:49       ` Krzysztof Kozlowski
2021-10-06 13:31         ` Sam Protsenko
2021-09-21 21:10   ` Rob Herring
2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
2021-09-14 21:35   ` Rob Herring
2021-09-15  8:28   ` Krzysztof Kozlowski
2021-10-05 11:48     ` Sam Protsenko
2021-09-15 16:47   ` Chanwoo Choi
2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
2021-09-15  8:59   ` Krzysztof Kozlowski
2021-10-05 11:29     ` Sam Protsenko
2021-10-06 12:50       ` Krzysztof Kozlowski
2021-09-15 13:07   ` Sylwester Nawrocki
2021-10-05 11:36     ` Sam Protsenko
2021-10-06 12:46       ` Krzysztof Kozlowski
2021-09-15 18:04   ` Chanwoo Choi
2021-09-15 22:00     ` Sam Protsenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).