Re: [PATCH v2 2/3] arm64: dts: exynos: gs101-pixel: add generic gs101-based Pixel support

["André Draszik" <andre.draszik@linaro.org>]

Hi Krzysztof,

On Sun, 2024-12-22 at 12:42 +0100, Krzysztof Kozlowski wrote:
> On 20/12/2024 12:27, André Draszik wrote:
> > In order to support Pixel 6 (Oriole), Pixel 6 Pro (Raven), and Pixel 6a
> > (Bluejay) correctly, we have to be able to distinguish them properly as
> > we add support for more features.
> > 
> > For example, Raven has a larger display. There are other differences,
> > like battery design capacity, etc.
> > 
> > To facilitate this, we create a generic gs101-based Pixel DT that can
> > work on any such gs101-based device. At the same time, we move the
> 
> No, whatever insanity Android has there, please don't populate it to
> upstream.
> 
> There is no such thing as "generic board" thus cannot be a
> "generic DTS".

I'll rephrase to gs101-based Pixel base board. Unless you have a better
suggestion.

> 
> > Oriole specific parts that we have at the moment (display) into an
> > overlay, making it easy to add support for Raven and Bluejay in a
> > similar way.
> > 
> > Note1:
> > Despite being an overlay, we instruct kbuild to create a merged
> > gs101-oriole.dtb and a gs101-oriole.dtbo. This way existing scripts can
> > keep working, but it also gives the option to just apply the overlay
> > before boot (e.g. by the bootloader).
> > 
> > Note2:
> > I've changed the simple-framebuffer node to specify the memory via
> > memory-region instead of reg, as that avoids unnecessary duplication
> > (of the size), and it avoids having to specify #address-cells
> > and #size-cells in the chosen node (and duplicating this in the DTSO),
> > which is otherwise necessary to keep dt_binding_check happy and DT
> > validation working in general.
> > 
> > Signed-off-by: André Draszik <andre.draszik@linaro.org>
> > ---
> > Note: MAINTAINERS doesn't need updating, it covers this whole directory
> > ---
> >  arch/arm64/boot/dts/exynos/google/Makefile         |  6 ++--
> >  .../arm64/boot/dts/exynos/google/gs101-oriole.dtso | 33 ++++++++++++++++++++++
> >  .../{gs101-oriole.dts => gs101-pixel-generic.dts}  | 24 +++++++---------
> >  3 files changed, 47 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/exynos/google/Makefile b/arch/arm64/boot/dts/exynos/google/Makefile
> > index 0a6d5e1fe4ee..6e6b5319212a 100644
> > --- a/arch/arm64/boot/dts/exynos/google/Makefile
> > +++ b/arch/arm64/boot/dts/exynos/google/Makefile
> > @@ -1,4 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -dtb-$(CONFIG_ARCH_EXYNOS) += \
> > -	gs101-oriole.dtb \
> > +dtb-$(CONFIG_ARCH_EXYNOS) += gs101-pixel-generic.dtb
> > +
> > +gs101-oriole-dtbs := gs101-pixel-generic.dtb gs101-oriole.dtbo
> > +dtb-$(CONFIG_ARCH_EXYNOS) += gs101-oriole.dtb
> > diff --git a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dtso b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dtso
> > new file mode 100644
> > index 000000000000..43572039cd07
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dtso
> > @@ -0,0 +1,33 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Oriole Device Tree
> > + *
> > + * Copyright 2021-2023 Google LLC
> > + * Copyright 2023-2024 Linaro Ltd
> > + */
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +
> > +&{/} {
> > +	model = "Oriole";
> > +	compatible = "google,gs101-oriole", "google,gs101-pixel", "google,gs101";
> 
> Boards are not overlays. Board equals DTB.

You're saying this should move into a dts instead of dtso?
There are numerous boards upstream which use this same dtso
approach.

There is a base board, and also different versions of it,
oriole being one of them.

[...]

> > 	};
> >  
> >  	chosen {
> > -		#address-cells = <2>;
> > -		#size-cells = <1>;
> > -		ranges;
> > -
> >  		/* Bootloader expects bootargs specified otherwise it crashes */
> >  		bootargs = "";
> >  		stdout-path = &serial_0;
> >  
> >  		/* Use display framebuffer as setup by bootloader */
> > -		framebuffer0: framebuffer@fac00000 {
> 
> I don't think this exists in current source. It does exist in thing I
> was applying, but then this does not make much sense: add a framebuffer
> which has to be changed, because it is not correct.
> 
> I'll drop the framebuffer patch.

The framebuffer makes sense for the oriole version. I can fully remove this
node from this base board here, and just leave it in the versions.
That said, can you please expand how having a 'status=disabled' framebuffer
node is different from any other nodes that are enabled or modified by
overlays?

Cheers,
Andre'