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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 12/10/2023 14:06, Peter Griffin wrote:
> Hi Sam,
> 
> Thanks for the review.
> 
> On Thu, 12 Oct 2023 at 01:07, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
>>
>> On Wed, Oct 11, 2023 at 1:49 PM Peter Griffin <peter.griffin@xxxxxxxxxx> 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.
>>> +
>>
>> Why is that new option needed? From the look of it, it could be just a
>> part of EXYNOS_ARM64_COMMON_CLK. Like clk-exynos850 or
>> clk-exynosautov9. Is there any particular feature that makes it SoC
>> special?
> 
> No, it could also be added to EXYNOS_ARM64_COMMON_CLK. I was following
> the example set by TESLA_FSD which is another custom Exynos based chipset
> that added its own config option.
> 
> Krzysztof do you have any preference on this?

Usually there is only one image for several boards so long time ago we
stopped adding per-SoC Kconfig entries. This has its own ARCH_xxx, just
like Tesla, thus having separate Kconfig for all Google Tensor clock
drivers makes sense. Maybe it should be just called a bit differently,
e.g. GOOGLE_TENSOR_COMMON_CLK



Best regards,
Krzysztof