Re: [PATCH v2 00/12] arm64: Kconfig: Update ARCH_EXYNOS select configs

[Olof Johansson]

On Thu, Sep 30, 2021 at 11:03 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Thu, Sep 30, 2021 at 10:36 PM Olof Johansson <olof@xxxxxxxxx> wrote:
> >
> > On Thu, Sep 30, 2021 at 10:24 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski
> > > > > <krzysztof.kozlowski@xxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On 29/09/2021 01:56, Will McVicker wrote:
> > > > > > > This is v2 of the series of patches that modularizes a number of core
> > > > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have
> > > > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS
> > > > > > > series of "select XXX". This includes setting the following configs as
> > > > > > > tristate:
> > > > > > >
> > > > > > >  * COMMON_CLK_SAMSUNG
> > > > > > >  * EXYNOS_ARM64_COMMON_CLK
> > > > > > >  * PINCTRL_SAMSUNG
> > > > > > >  * PINCTRL_EXYNOS
> > > > > > >  * EXYNOS_PMU_ARM64
> > > > > > >  * EXYNOS_PM_DOMAINS
> > > > > > >
> > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM
> > > > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively.
> > > > > > > The reason for these new configs is because we are not able to easily
> > > > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on
> > > > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up
> > > > > > > the ARM and ARM64 portions into two separate configs.
> > > > > > >
> > > > > > > Overall, these drivers didn't require much refactoring and converted to
> > > > > > > modules relatively easily. However, due to my lack of exynos hardware, I
> > > > > > > was not able to boot test these changes. I'm mostly concerned about the
> > > > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm
> > > > > > > requesting help for testing these changes on the respective hardware.
> > > > > > >
> > > > > >
> > > > > > These are all not tested at all? In such case, since these are not
> > > > > > trivial changes, please mark the series as RFT.
> > > > > >
> > > > > > I will not be able to test these for some days, so it must wait.
> > > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Krzysztof
> > > > >
> > > > > +Cc Arnd and Olof,
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > To avoid the scrambled conversation from the first patchset, I'm going
> > > > > to address all your general questions here in the cover letter thread
> > > > > so that it's easier for everyone to follow and reference in the
> > > > > future.
> > > >
> > > > This patchset shouldn't go in.
> > > >
> > > > GKI is a fantastic effort, since it finally seems like Google has the
> > > > backbone to put pressure on the vendors to upstream all their stuff.
> > > >
> > > > This patcheset dilutes and undermines all of that by opening up a
> > > > truck-size loophole, reducing the impact of GKI, and overall removes
> > > > leverage to get vendors to do the right thing.
> > > >
> > > > It's against our interest as a community to have this happen, since
> > > > there's no other reasonably justifiable reason to do this.
> > >
> > > Oolf, Geert, Krzysztof, Arnd,
> >
> > So close.
>
> I'm sorry, it's pretty late here and I'm sleepy and messed it up.

This email thread will be here in the morning too, this is the last
reply I will make tonight myself.

> > > I skimmed through the emails and you all make a lot of good points.
> >
> > I skimmed through this email and I think it adds a lot of new
> > complexity and fragility to solve a problem that doesn't really exist
> > for upstream, adding yet more config parameter combinations to build
> > and test for.
>
> How is this not an upstream problem? Having a minimal kernel with as
> many drivers as modules is of interest to upstream. And what's the
> complexity in having a config to easily disable a bunch of configs?
> The new config gives a clear config against which new
> platforms/systems should be developed against.

The approach for general kernel upstream has been to have the built-in
drivers needed to reach rootfs and runtime load the rest from there.
For downstream embedded kernels, the right answer is to just exclude
the drivers you don't need.

I agree, some of this is probably useful work but
1) It was posted as a burden on the maintainers of the legacy
platforms to test and make sure it's not broken
2) It regresses distro configs, which we do care about
3) Based on the above, it's unclear whether the people pushing for
this patchset cares about the existing platforms, they're looking to
solve a different problem

I didn't go back and look at who and where, but the person who started
justifying this work with making it easier for out-of-tree vendors to
integrate their non-contributed code killed this patchset.

Would the out-of-tree argument on its own kill it? No, but the above
complexities/cost, plus the actual intent behind the patchset did.

> > A much more valuable approach would be to work towards being able to
> > free up memory by un-probed drivers at the end of boot. That would
> > possibly benefit all platforms on all architectures.
>
> Sure it would help memory after boot, but it won't help with size on
> "disk", kernel load time, etc. And some of the devices have very tight
> boot requirements. Think battery operated outdoor cameras for example.

I would question the choices made if someone ships a camera with a
generic kernel (without a generic filesystem which at that point also
needs sufficient storage such that the kernel image size becomes a
secondary consideration). Once your storage is that constrained the
balance shifts towards a custom minimal config.


-Olof