Re: [PATCH 18/21] arm64: dts: google: Add initial Google gs101 SoC support

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 06/10/2023 18:33, William McVicker wrote:
> On 10/06/2023, Arnd Bergmann wrote:
>> On Fri, Oct 6, 2023, at 08:06, Krzysztof Kozlowski wrote:
>>> On 06/10/2023 01:19, William McVicker wrote:
>>>> On 10/05/2023, Krzysztof Kozlowski wrote:
>>>>> On 05/10/2023 21:23, Greg KH wrote:
>>>>
>>>> Being able to include SERIAL_SAMSUNG and SERIAL_MSM without all the vendor> specific drivers that ARCH_EXYNOS and ARCH_QCOM select is very
>>> valuable for
>>>> debugging early boot issues.
>>>
>>> Really? How related? The drivers are independent. You describe some
>>> out-of-tree development process which we never needed for upstream work.
>>> And we did here quite a lot of upstream, specially if you look at ARCH_QCOM.
>>
>> Right: in general, all drivers are independent of the platform
>> besides the typical 'depends on ARCH_FOO || COMPILE_TEST' dependency,
>> but I think it's worth mentioning the known exceptions, so Greg and
>> Will can take that fight to the respective places rather than
>> discussing it in the platform submission:
>>
>> - Some subsystems are considered 'special' and the maintainers
>>   prefer the drivers to be automatically selected based on the
>>   ARCH_* settings instead of having user-visible options. This is
>>   traditionally true for large chunks of drivers/irqchip,
>>   drivers/clocksource and drivers/pinctrl, though it has gotten
>>   better over time on all of them.
>>
>> - Some older 32-bit platforms are still not as modular as we'd
>>   like them to be, especially the StrongARM (ARMv4) platforms that
>>   require a custom kernel build, and some of ARMv4T and ARMv5
>>   boards that are still missing DT support. These tend to require
>>   drivers they directly link to from board code, so disabling
>>   the drivers would cause a link failure until this gets
>>   cleaned up.
>>
>> - A couple of drivers are force-enabled based on the ARCH_*
>>   options because booting without these drivers would risk
>>   permanent damage to hardware, e.g. in overtemp or overcurrent
>>   scenarios.
>>
>> - ACPI based platforms require the PCI host bridge driver to
>>   be built-in rather than a loadable module because ACPI
>>   needs to probe PCI devices during early boot.
>>
>> - Some subsystems (notably drivers/gpu/, but others as well)
>>   have an excessive number of 'select' statements, so you
>>   end up surprise-enabling a number of additional drivers
>>   and subsystems by enabling certain less important platform
>>   specific drivers.
>>
>>       Arnd
> 
> So if the argument is that the existing upstream Exynos platforms are required
> to have these drivers built-in to the kernel to boot:
>     COMMON_CLK_SAMSUNG
>     CLKSRC_EXYNOS_MCT
>     EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
>     EXYNOS_PMU
>     PINCTRL
>     PINCTRL_EXYNOS
>     PM_GENERIC_DOMAINS if PM
>     SOC_SAMSUNG
> 
> ...then that is understandable and we can work to fix that.
> 
> My last question then is -- why do we need a new ARCH_GOOGLE_TENSOR config in
> the platform Kconfig? For example, I don't really like this:
> 
> diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
> index 76a494e95027..4c8f173c4dec 100644
> --- a/drivers/clk/samsung/Kconfig
> +++ b/drivers/clk/samsung/Kconfig
> @@ -13,6 +13,7 @@ config COMMON_CLK_SAMSUNG
>         select EXYNOS_5420_COMMON_CLK if ARM && SOC_EXYNOS5420
>         select EXYNOS_ARM64_COMMON_CLK if ARM64 && ARCH_EXYNOS
>         select TESLA_FSD_COMMON_CLK if ARM64 && ARCH_TESLA_FSD
> +       select GOOGLE_GS101_COMMON_CLK if ARM64 && ARCH_GOOGLE_TENSOR
> 
> What happens when we have GOOGLE_GS101_COMMON_CLK, GOOGLE_GS201_COMMON_CLK, and
> so on? 

Nothing happens... or happens anything you wish. Did you read the
motivation why this was created like this?


> How are we going to pick the right driver when e have a generic
> ARCH_GOOGLE_TENSOR config?

You do not have to pick. You select ARCH_GOOGLE_TENSOR and proper pick
is done by you. Nothing to do more.

> Ideally, we should have one Exynos clock driver that
> can detect what hardware is running (using the DT) to determine what it needs

It's already like this. We're done.

> to do. If you really want to compile out the other vendor's clock drivers using
> some configs, then we should do that with SOC_GS101, SOC_GS201, SOC_TESLA_FSD

Whether you call it SOC or ARCH it is the same. We organized it as ARCH.

> configs (not ideal though). With that approach, we could drop the platform
> ARCH_GOOGLE_TENSOR config and create an SOC_GS101 config that can be used for
> things like the COMMON_CLK_SAMSUNG driver (for now) and building the GS101 dtb.

There is no need for this. ARCH does exactly the same.

Best regards,
Krzysztof