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

[William McVicker <willmcvicker@google.com>]

On 10/07/2023, Krzysztof Kozlowski wrote:
> 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?
> 

Okay, we can figure that out the gs201 specifics when the time comes.

> 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.

Okay, sounds good. Thanks for the responses.

Regards,
Will

> 
> Best regards,
> Krzysztof
>