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

[William McVicker <willmcvicker@xxxxxxxxxx>]

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? How are we going to pick the right driver when we have a generic
ARCH_GOOGLE_TENSOR config? Ideally, we should have one Exynos clock driver that
can detect what hardware is running (using the DT) to determine what it needs
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
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.

Let me know your thoughts.

Thanks,
Will