Hi Sam,

On Wed, 8 Nov 2023 at 17:33, Sam Protsenko [off-list ref] wrote:

On Tue, Nov 7, 2023 at 7:57 AM Peter Griffin [off-list ref] wrote:

quoted

Hi Chanwoo,

Thanks for your review!

On Wed, 18 Oct 2023 at 17:51, Chanwoo Choi [off-list ref] wrote:

quoted

Hi Peter,

On 23. 10. 12. 03:48, Peter Griffin wrote:

quoted

CMU_TOP is the top level clock management unit which contains PLLs, muxes
and gates that feed the other clock management units.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/clk/samsung/Kconfig     |    9 +
 drivers/clk/samsung/Makefile    |    2 +
 drivers/clk/samsung/clk-gs101.c | 1551 +++++++++++++++++++++++++++++++
 3 files changed, 1562 insertions(+)
 create mode 100644 drivers/clk/samsung/clk-gs101.c

diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
index 76a494e95027..14362ec9c543 100644
--- a/drivers/clk/samsung/Kconfig
+++ b/drivers/clk/samsung/Kconfig

@@ -12,6 +12,7 @@ config COMMON_CLK_SAMSUNG
      select EXYNOS_5410_COMMON_CLK if ARM && SOC_EXYNOS5410
      select EXYNOS_5420_COMMON_CLK if ARM && SOC_EXYNOS5420
      select EXYNOS_ARM64_COMMON_CLK if ARM64 && ARCH_EXYNOS
+     select GOOGLE_GS101_COMMON_CLK if ARM64 && ARCH_GOOGLE_TENSOR
      select TESLA_FSD_COMMON_CLK if ARM64 && ARCH_TESLA_FSD

 config S3C64XX_COMMON_CLK

@@ -95,6 +96,14 @@ config EXYNOS_CLKOUT
        status of the certains clocks from SoC, but it could also be tied to
        other devices as an input clock.

+config GOOGLE_GS101_COMMON_CLK
+     bool "Google gs101 clock controller support" if COMPILE_TEST
+     depends on COMMON_CLK_SAMSUNG
+     depends on EXYNOS_ARM64_COMMON_CLK
+     help
+       Support for the clock controller present on the Google gs101 SoC.
+       Choose Y here only if you build for this SoC.
+

(snip)

quoted

+
+/* gs101 */
+static const struct samsung_mux_clock cmu_top_mux_clks[] __initconst = {
+     /* CMU_TOP_PURECLKCOMP */
+     MUX(CLK_MOUT_SHARED0_PLL, "mout_shared0_pll", mout_shared0_pll_p,
+         PLL_CON0_PLL_SHARED0, 4, 1),
+     MUX(CLK_MOUT_SHARED1_PLL, "mout_shared1_pll", mout_shared1_pll_p,
+         PLL_CON0_PLL_SHARED1, 4, 1),
+     MUX(CLK_MOUT_SHARED2_PLL, "mout_shared2_pll", mout_shared2_pll_p,
+         PLL_CON0_PLL_SHARED2, 4, 1),
+     MUX(CLK_MOUT_SHARED3_PLL, "mout_shared3_pll", mout_shared3_pll_p,
+         PLL_CON0_PLL_SHARED3, 4, 1),
+     MUX(CLK_MOUT_SPARE_PLL, "mout_spare_pll", mout_spare_pll_p,
+         PLL_CON0_PLL_SPARE, 4, 1),
+
+     /* BUS0 */
+     MUX(CLK_MOUT_BUS0_BUS, "mout_cmu_bus0_bus", mout_cmu_bus0_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_BUS0_BUS, 0, 2),
+     MUX(CLK_MOUT_CMU_BOOST, "mout_cmu_boost", mout_cmu_cmu_boost_p,

In order to keep the consistent naming style,
I think that need to change from 'mout_cmu_boost' to 'mout_cmu_cmu_boost'.

Yes, that's a good point, and a good spot! Will fix it in v4.

Why do we need cmu_cmu part at all? From the look of it, renaming all
*_cmu_cmu_* clocks to just cmu wouldn't cause any naming conflicts. So
I don't see any benefit of double cmu prefix really.

The benefit is consistency, as Chanwoo pointed out.

quoted

quoted

quoted

+         CLK_CON_MUX_MUX_CLKCMU_CMU_BOOST, 0, 2),
+
+     /* BUS1 */
+     MUX(CLK_MOUT_BUS1_BUS, "mout_cmu_bus1_bus", mout_cmu_bus1_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_BUS1_BUS, 0, 2),
+
+     /* BUS2 */
+     MUX(CLK_MOUT_BUS2_BUS, "mout_cmu_bus2_bus", mout_cmu_bus2_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_BUS2_BUS, 0, 2),
+
+     /* CORE */
+     MUX(CLK_MOUT_CORE_BUS, "mout_cmu_core_bus", mout_cmu_core_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),
+
+     /* EH */
+     MUX(CLK_MOUT_EH_BUS, "mout_cmu_eh_bus", mout_cmu_eh_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),

'mout_cmu_core_bus' and 'mout_cmu_eh_bus' uses the same register/shift/width information.
I think it should be modified by changing the regiter or changing the shift/width information.

It should be using the CLK_CON_MUX_MUX_CLKCMU_EH_BUS register.
Will fix it in v4.

quoted

quoted

+
+     /* CPUCL{0,1,2,} */
+     MUX(CLK_MOUT_CPUCL2_SWITCH, "mout_cmu_cpucl2_switch", mout_cmu_cpucl2_switch_p,
+         CLK_CON_MUX_MUX_CLKCMU_CPUCL2_SWITCH, 0, 2),
+
+     MUX(CLK_MOUT_CPUCL1_SWITCH, "mout_cmu_cpucl1_switch", mout_cmu_cpucl1_switch_p,
+         CLK_CON_MUX_MUX_CLKCMU_CPUCL1_SWITCH, 0, 2),
+
+     MUX(CLK_MOUT_CPUCL0_SWITCH, "mout_cmu_cpucl0_switch", mout_cmu_cpucl0_switch_p,
+         CLK_CON_MUX_MUX_CLKCMU_CPUCL0_SWITCH, 0, 2),
+
+     MUX(CLK_MOUT_CPUCL0_DBG, "mout_cmu_cpucl0_dbg", mout_cmu_cpucl0_dbg_p,
+         CLK_CON_DIV_CLKCMU_CPUCL0_DBG, 0, 2),
+
+     MUX(CLK_MOUT_CMU_HPM, "mout_cmu_hpm", mout_cmu_hpm_p,
+         CLK_CON_MUX_MUX_CLKCMU_HPM, 0, 2),

(snip)

quoted

+     /* PDP */
+     MUX(CLK_MOUT_PDP_BUS, "mout_cmu_pdp_bus", mout_cmu_pdp_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_PDP_BUS, 0, 2),
+
+     /* PDP */
+     MUX(CLK_MOUT_PDP_VRA, "mout_cmu_pdp_vra", mout_cmu_pdp_vra_p,
+         CLK_CON_MUX_MUX_CLKCMU_PDP_VRA, 0, 2),
+
+     /* IPP */
+     MUX(CLK_MOUT_IPP_BUS, "mout_cmu_ipp_bus", mout_cmu_ipp_bus_p,
+         CLK_CON_MUX_MUX_CLKCMU_IPP_BUS, 0, 2),
+
+     /* G3AA */
+     MUX(CLK_MOUT_G3AA, "mout_cmu_g3aa", mout_cmu_g3aa_p,
+         CLK_CON_MUX_MUX_CLKCMU_G3AA_G3AA, 0, 2),

I think that need to change the mux name and mux parent name
because other mux name use the twice word according to the register name
even if use the same work such as 'mout_cmu_g2d_g2d', 'mout_cmu_mcsc_mcsc' and 'mout_cmu_mfc_mfc'.
- mout_cmu_g3aa -> mout_cmu_g3aa_g3aa
- mout_cmu_g3aa_p -> mount_cmu_g3aa_g3aa_p

Will fix in v4

That consistent name duplication, while not causing any conflicts when
being removed, looks suspicious to me. That's probably some internal
scheme which doesn't make much sense for us and doesn't bring any
value, in terms of clock drivers. Maybe it'll be better to instead get
rid of such duplication throughout the driver, at least for clock name
strings? I mention this, because that's what I did in clk-exynos850.
With the only exception being the main domain clocks, which basically
enables/disables the whole unit internally, e.g.

    GATE(CLK_GOUT_G3D_CMU_G3D_PCLK, "gout_g3d_cmu_g3d_pclk", ...

which "G3D domain gate clock that enables/disables G3D", or something
like that. But clk-exynos850 doesn't have any duplicating bits like
"cmu_cmu" or "g3d_g3d". And the reason why I did that is I wanted
those clock names appear short and nice in device tree, as there were
no benefits in those duplicating bits.

I did start out doing something similar, but decided it was a bad
idea. The value of consistent naming and being able to more easily
cross reference and verify correctness with the TRM for me far
outweighs the advantage of having a shorter name. There are literally
thousands of clocks we need to expose in this SoC to have everything
fully modelled and having these various "exceptions" where we change
the naming scheme of the clock because of a duplicated word IMO isn't
helpful.

Peter

quoted

quoted

(snip)

quoted

+     /* CSIS */
+     GATE(CLK_GOUT_CSIS, "gout_cmu_csis_bus", "mout_cmu_csis_bus",
+          CLK_CON_GAT_GATE_CLKCMU_CSIS_BUS, 21, 0, 0),
+     /* PDP */
+     GATE(CLK_GOUT_PDP_BUS, "gout_cmu_pdp_bus", "mout_cmu_pdp_bus",
+          CLK_CON_GAT_GATE_CLKCMU_PDP_BUS, 21, 0, 0),
+
+     GATE(CLK_GOUT_PDP_VRA, "gout_cmu_pdp_vra", "mout_cmu_pdp_vra",
+          CLK_CON_GAT_GATE_CLKCMU_PDP_BUS, 21, 0, 0),
+
+     /* IPP */
+     GATE(CLK_GOUT_IPP_BUS, "gout_cmu_ipp_bus", "mout_cmu_ipp_bus",
+          CLK_CON_GAT_GATE_CLKCMU_IPP_BUS, 21, 0, 0),
+     /* G3AA */
+     GATE(CLK_GOUT_G3AA, "gout_cmu_g3aa", "mout_cmu_g3aa",
+          CLK_CON_MUX_MUX_CLKCMU_G3AA_G3AA, 21, 0, 0),

ditto.
gout_cmu_g3aa -> gout_cmu_g3aa_g3aa
mout_cmu_g3aa -> mout_cmu_g3aa_g3aa

Ditto.

quoted

Will fix in V4

regards,

Peter.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel