Re: [PATCH v3 10/20] clk: samsung: clk-gs101: Add cmu_top registers, plls, mux and gates

[Peter Griffin <peter.griffin@linaro.org>]

Hi Chanwoo,

Thanks for your review!

On Wed, 18 Oct 2023 at 17:51, Chanwoo Choi <chanwoo@kernel.org> wrote:
>
> Hi Peter,
>
> On 23. 10. 12. 03:48, Peter Griffin wrote:
> > 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)
>
> > +
> > +/* 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.

>
> > +         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.

>
> > +
> > +     /* 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)
>
> > +     /* 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

>
> (snip)
>
> > +     /* 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

Will fix in V4

regards,

Peter.