Hi Chanwoo,
Thanks for your review!
On Wed, 18 Oct 2023 at 17:51, Chanwoo Choi <chanwoo@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
