From: Peter Griffin <peter.griffin@linaro.org>
To: Sam Protsenko <semen.protsenko@linaro.org>
Cc: Chanwoo Choi <chanwoo@kernel.org>,
robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
mturquette@baylibre.com, conor+dt@kernel.org, sboyd@kernel.org,
tomasz.figa@gmail.com, s.nawrocki@samsung.com,
linus.walleij@linaro.org, wim@linux-watchdog.org,
linux@roeck-us.net, catalin.marinas@arm.com, will@kernel.org,
arnd@arndb.de, olof@lixom.net, gregkh@linuxfoundation.org,
cw00.choi@samsung.com, tudor.ambarus@linaro.org,
andre.draszik@linaro.org, saravanak@google.com,
willmcvicker@google.com, soc@kernel.org,
devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-clk@vger.kernel.org,
linux-gpio@vger.kernel.org, linux-watchdog@vger.kernel.org,
kernel-team@android.com, linux-serial@vger.kernel.org
Subject: Re: [PATCH v3 10/20] clk: samsung: clk-gs101: Add cmu_top registers, plls, mux and gates
Date: Fri, 1 Dec 2023 13:59:31 +0000 [thread overview]
Message-ID: <CADrjBPpCNc6RASB8p1MmaLuR46b_MsACyyGFLrnozW0_9oRQ3g@mail.gmail.com> (raw)
In-Reply-To: <CAPLW+4m+n-U4cAkJZTeCsoE_e6r1j8srYmVjSLawPWwHi6SEAw@mail.gmail.com>
Hi Sam,
On Wed, 8 Nov 2023 at 17:33, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Tue, Nov 7, 2023 at 7:57 AM Peter Griffin <peter.griffin@linaro.org> wrote:
> >
> > 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.
> >
>
> 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.
>
> > >
> > > > + 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
> >
>
> 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
>
> > >
> > > (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
> >
>
> Ditto.
>
> > Will fix in V4
> >
> > regards,
> >
> > Peter.
next prev parent reply other threads:[~2023-12-01 13:59 UTC|newest]
Thread overview: 100+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-11 18:48 [PATCH v3 00/20] Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board Peter Griffin
2023-10-11 18:48 ` [PATCH v3 01/20] dt-bindings: soc: samsung: exynos-pmu: Add gs101 compatible Peter Griffin
2023-10-11 18:54 ` Sam Protsenko
2023-10-11 18:48 ` [PATCH v3 02/20] dt-bindings: clock: Add Google gs101 clock management unit bindings Peter Griffin
2023-10-11 21:48 ` William McVicker
2023-10-12 6:07 ` Krzysztof Kozlowski
2023-10-12 8:56 ` Peter Griffin
2023-10-12 9:36 ` Krzysztof Kozlowski
2023-10-12 10:45 ` Peter Griffin
2023-10-12 11:33 ` Krzysztof Kozlowski
2023-10-12 16:41 ` William McVicker
2023-10-11 22:55 ` Sam Protsenko
2023-10-12 6:11 ` Krzysztof Kozlowski
2023-10-12 10:15 ` Peter Griffin
2023-10-12 10:20 ` Krzysztof Kozlowski
2023-10-12 10:39 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 03/20] dt-bindings: soc: google: exynos-sysreg: add dedicated SYSREG compatibles to GS101 Peter Griffin
2023-10-11 22:56 ` Sam Protsenko
2023-10-16 13:36 ` Rob Herring
2023-10-19 13:10 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 04/20] dt-bindings: watchdog: Document Google gs101 & gs201 watchdog bindings Peter Griffin
2023-10-11 22:57 ` Sam Protsenko
2023-10-12 10:56 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 05/20] dt-bindings: arm: google: Add bindings for Google ARM platforms Peter Griffin
2023-10-11 23:06 ` Sam Protsenko
2023-10-12 11:19 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 06/20] dt-bindings: pinctrl: samsung: add google,gs101-pinctrl compatible Peter Griffin
2023-10-11 23:10 ` Sam Protsenko
2023-10-16 13:41 ` Rob Herring
2023-11-07 12:18 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 07/20] dt-bindings: pinctrl: samsung: add gs101-wakeup-eint compatible Peter Griffin
2023-10-12 6:13 ` Krzysztof Kozlowski
2023-10-11 18:48 ` [PATCH v3 08/20] dt-bindings: serial: samsung: Add google-gs101-uart compatible Peter Griffin
2023-10-11 23:13 ` Sam Protsenko
2023-10-11 18:48 ` [PATCH v3 09/20] clk: samsung: clk-pll: Add support for pll_{0516,0517,518} Peter Griffin
2023-10-11 21:49 ` William McVicker
2023-10-11 23:19 ` Sam Protsenko
2023-10-12 11:50 ` Peter Griffin
[not found] ` <ef25ed87-f065-4a75-9e57-1f1073d9c805@kernel.org>
2023-10-17 20:39 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 10/20] clk: samsung: clk-gs101: Add cmu_top registers, plls, mux and gates Peter Griffin
2023-10-11 21:50 ` William McVicker
2023-10-12 0:06 ` Sam Protsenko
2023-10-12 12:06 ` Peter Griffin
2023-10-12 12:24 ` Krzysztof Kozlowski
2023-10-12 13:52 ` Peter Griffin
[not found] ` <aae4e6cd-dcfc-442d-9ed7-d5a73c419ba8@kernel.org>
2023-11-07 13:57 ` Peter Griffin
2023-11-08 17:33 ` Sam Protsenko
2023-12-01 13:59 ` Peter Griffin [this message]
2023-10-11 18:48 ` [PATCH v3 11/20] clk: samsung: clk-gs101: add CMU_APM support Peter Griffin
2023-10-11 21:50 ` William McVicker
2023-10-12 0:10 ` Sam Protsenko
2023-10-11 18:48 ` [PATCH v3 12/20] clk: samsung: clk-gs101: Add support for CMU_MISC clock unit Peter Griffin
2023-10-11 21:51 ` William McVicker
2023-10-12 0:12 ` Sam Protsenko
2023-10-12 16:02 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 13/20] pinctrl: samsung: Add filter selection support for alive banks Peter Griffin
2023-10-11 21:51 ` William McVicker
2023-10-11 22:47 ` Sam Protsenko
2023-10-20 13:54 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 14/20] pinctrl: samsung: Add gs101 SoC pinctrl configuration Peter Griffin
2023-10-11 21:52 ` William McVicker
2023-10-11 21:53 ` William McVicker
2023-10-12 5:59 ` Sam Protsenko
2023-11-08 13:43 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 15/20] watchdog: s3c2410_wdt: Add support for Google tensor SoCs Peter Griffin
2023-10-11 21:20 ` Guenter Roeck
2023-10-17 21:26 ` Peter Griffin
2023-10-12 2:32 ` Sam Protsenko
2023-10-17 21:39 ` Peter Griffin
2023-10-12 6:22 ` Krzysztof Kozlowski
2023-10-11 18:48 ` [PATCH v3 16/20] tty: serial: samsung: Add gs101 compatible and SoC data Peter Griffin
2023-10-11 21:54 ` William McVicker
2023-10-12 5:38 ` Sam Protsenko
2023-10-12 6:07 ` Arnd Bergmann
2023-10-20 21:47 ` Peter Griffin
2023-10-12 6:26 ` Krzysztof Kozlowski
2023-10-12 14:03 ` Peter Griffin
2023-10-12 14:10 ` Krzysztof Kozlowski
2023-10-11 18:48 ` [PATCH v3 17/20] arm64: dts: google: Add initial Google gs101 SoC support Peter Griffin
2023-10-11 21:55 ` William McVicker
2023-10-12 6:40 ` Krzysztof Kozlowski
2023-11-24 23:22 ` Peter Griffin
2023-11-28 8:58 ` Krzysztof Kozlowski
2023-10-12 6:44 ` Krzysztof Kozlowski
2023-11-24 23:53 ` Peter Griffin
2023-10-12 7:23 ` Sam Protsenko
2023-10-12 7:39 ` Krzysztof Kozlowski
2023-11-28 22:43 ` Peter Griffin
2023-10-11 18:48 ` [PATCH v3 18/20] arm64: dts: google: Add initial Oriole/pixel 6 board support Peter Griffin
2023-10-11 21:55 ` William McVicker
2023-10-12 6:44 ` Krzysztof Kozlowski
2023-10-12 7:40 ` Sam Protsenko
2023-10-11 18:48 ` [PATCH v3 19/20] arm64: defconfig: Enable Google Tensor SoC Peter Griffin
2023-10-11 21:56 ` William McVicker
2023-10-12 6:15 ` Sam Protsenko
2023-10-11 18:48 ` [PATCH v3 20/20] MAINTAINERS: add entry for " Peter Griffin
2023-10-12 6:02 ` Sam Protsenko
2023-10-11 21:58 ` [PATCH v3 00/20] Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board William McVicker
2023-10-11 22:51 ` Sam Protsenko
2023-10-12 6:28 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADrjBPpCNc6RASB8p1MmaLuR46b_MsACyyGFLrnozW0_9oRQ3g@mail.gmail.com \
--to=peter.griffin@linaro.org \
--cc=andre.draszik@linaro.org \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=chanwoo@kernel.org \
--cc=conor+dt@kernel.org \
--cc=cw00.choi@samsung.com \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=kernel-team@android.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=mturquette@baylibre.com \
--cc=olof@lixom.net \
--cc=robh+dt@kernel.org \
--cc=s.nawrocki@samsung.com \
--cc=saravanak@google.com \
--cc=sboyd@kernel.org \
--cc=semen.protsenko@linaro.org \
--cc=soc@kernel.org \
--cc=tomasz.figa@gmail.com \
--cc=tudor.ambarus@linaro.org \
--cc=will@kernel.org \
--cc=willmcvicker@google.com \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).