Message-ID: <CAL_JsqKHs_cBJWrS6+tzoUZ5fESAsim2uasHmFRg1CT7BOjDWQ@mail.gmail.com>
Date: Fri, 31 May 2024 09:10:54 -0500
From: Rob Herring <robh@...nel.org>
To: Tim Harvey <tharvey@...eworks.com>
Cc: Conor Dooley <conor@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, 
	Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Li Yang <leoyang.li@....com>, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] dt-bindings: arm: fsl: rename gw7905 to gw75xx

On Thu, May 30, 2024 at 12:36 PM Tim Harvey <tharvey@...eworks.com> wrote:
>
> On Thu, May 30, 2024 at 9:54 AM Conor Dooley <conor@...nel.org> wrote:
> >
> > On Tue, May 28, 2024 at 09:23:10AM -0700, Tim Harvey wrote:
> > > On Tue, May 28, 2024 at 8:58 AM Rob Herring <robh@...nel.org> wrote:
> > > >
> > > > On Sat, May 25, 2024 at 12:58:18PM -0700, Tim Harvey wrote:
> > > > > On Sat, May 25, 2024 at 11:34 AM Krzysztof Kozlowski
> > > > > <krzysztof.kozlowski@...aro.org> wrote:
> > > > > >
> > > > > > On 24/05/2024 20:40, Conor Dooley wrote:
> > > > > > > On Thu, May 23, 2024 at 04:04:50PM -0700, Tim Harvey wrote:
> > > > > > >> On Thu, May 23, 2024 at 7:47 AM Conor Dooley <conor@...nel.org> wrote:
> > > > > > >>>
> > > > > > >>> On Thu, May 23, 2024 at 09:02:46AM +0200, Krzysztof Kozlowski wrote:
> > > > > > >>>> On 22/05/2024 23:50, Tim Harvey wrote:
> > > > > > >>>>> The GW7905 was renamed to GW7500 before production release.
> > > > > > >>>>>
> > > > > > >>>>> Signed-off-by: Tim Harvey <tharvey@...eworks.com>
> > > > > > >>>>> ---
> > > > > > >>>>>  Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
> > > > > > >>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >>>>>
> > > > > > >>>>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > > > >>>>> index 0027201e19f8..d8bc295079e3 100644
> > > > > > >>>>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > > > >>>>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> > > > > > >>>>> @@ -920,8 +920,8 @@ properties:
> > > > > > >>>>>                - fsl,imx8mm-ddr4-evk       # i.MX8MM DDR4 EVK Board
> > > > > > >>>>>                - fsl,imx8mm-evk            # i.MX8MM EVK Board
> > > > > > >>>>>                - fsl,imx8mm-evkb           # i.MX8MM EVKB Board
> > > > > > >>>>> +              - gateworks,imx8mm-gw75xx-0x # i.MX8MM Gateworks Board
> > > > > > >>>>
> > > > > > >>>> That's not even equivalent. You 7500 != 75xx.
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>>>>                - gateworks,imx8mm-gw7904
> > > > > > >>>>> -              - gateworks,imx8mm-gw7905-0x # i.MX8MM Gateworks Board
> > > > > > >>>>
> > > > > > >>>> Compatibles do not change. It's just a string. Fixed string.
> > > > > > >>>
> > > > > > >>> I think there's justification here for removing it, per the commit
> > > > > > >>> message, the rename happened before the device was available to
> > > > > > >>> customers.
> > > > > > >>> Additionally, I think we can give people that upstream things before they're
> > > > > > >>> publicly available a bit of slack, otherwise we're just discouraging
> > > > > > >>> people from upstreaming early.
> > > > > > >>
> > > > > > >> Hi Conor,
> > > > > > >>
> > > > > > >> Thanks for understanding - that's exactly what happened. I'm in the
> > > > > > >> habit of submitting patches early and often and it's no fun when
> > > > > > >> something like a silly product name gets changed and breaks all the
> > > > > > >> hard work.
> > > > > > >>
> > > > > > >> The board model number is stored in an EEPROM at manufacturing time
> > > > > > >> and that EEPROM model is used to build a dt name. So instead of GW7905
> > > > > > >> which would be a one-off custom design it was decided to change the
> > > > > > >> product to a GW75xx. The difference between GW7500 and GW75xx is
> > > > > > >> because we subload components on boards between GW7500/GW7501/GW7502
> > > > > > >> etc but the dt is the same.
> > > > > > >>
> > > > > > >> If there is resistance to a patch that renames it then I guess I'll
> > > > > > >> have to submit a patch that removes the obsolete board, then adds back
> > > > > > >> the same board under a different name. Shall I do that?
> > > > > > >
> > > > > > > I think this patch is fine - other than the inconsistency that Krzysztof
> > > > > > > pointed out between the "renamed to gw7500" and the "gw75xx" in the new
> > > > > > > compatible.
> > > > > >
> > > > > > I am not a fan of renaming compatibles because of marketing change,
> > > > > > because compatible does not have to reflect the marketing name, but
> > > > > > there was already precedent from Qualcomm which I did not nak, so fine
> > > > > > here as well. Double wildcard 75xx is however a bit worrying.
> > > > > >
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > Thanks for understanding. The double-wildcard is again a marketing
> > > > > tool. All GW75** use the same device-tree by design. The boot firmware
> > > > > that chooses the device-tree understands this and for a GW7521 for
> > > > > example would look for gw7521 first, gw752x next, gw75xx last.
> >
> > When it is doing this matching, does it actually apply a wildcard, or
> > does it look for "x"? IOW, if your eeprom said "gw7521" and there were
> > no devicetrees matching "gw7521" but there was one with "gw7500" would
> > it match?
>
> I attempt to explain the algorithm used in the comment of the U-Boot
> code used by U-Boot to both choose the DTB it uses as well as the
> bootscript to choose the DTB used for Linux when booting Linux:
> https://elixir.bootlin.com/u-boot/latest/source/board/gateworks/venice/eeprom.c#L164
>
> Consider a GW7001-F.1 SOM married to a GW7201-G.2 Baseboard (the
> letter at len-2 is the PCB revision and the number is a BOM revision
> and those are the full model strings in the EEPROM's of the boards):
> fdt_file1=imx8mm-venice-gw72xx-0x-g2f1.dtb
> fdt_file2=imx8mm-venice-gw72xx-0x-g2f.dtb
> fdt_file3=imx8mm-venice-gw72xx-0x-gf.dtb
> fdt_file4=imx8mm-venice-gw72xx-0x.dtb

These all have the same root compatible I'm guessing? Based on recent
discussions around firmware picking a DTB, we really want that based
on compatibles, not filenames. After all, compatible is all about most
specific to least specific matching. Maybe it doesn't matter for your
products. However, that's likely the direction we're going and it may
make your life easier to align with that.

> The script that loads the fdt for booting Linux looks for fdt_file1
> first, then fdt_file2, etc etc so that if needed a 'more specific' dtb
> could exist with base-board and som-revision fixups. However to answer
> your question the algorithm already assumes that baseboards have 2
> digits worth of don't care and match specifically an 'x' for that.
> Including the SOM and Baseboard PCB revision and BOM revision was a
> safeguard that has never been needed so in practice we always just end
> up with the last most generic dtb above which is the case of
> 'imx8mm-venice-gw72xx-0x.dtb' case means a 'gw72xx' baseboard married
> to a 'gw700x' SOM. The 'gw70' is removed from the dtb name as all
> Venice SOM's start are GW70**.
>
> So in my mind the dt for the baseboard above is called
> 'imx8mm-venice-gw72xx' where 'xx' is part of the name but does have
> implied meaning. It certainly helps our customers know that the last
> two digits of a baseboard are don't-cares.
>
> This is nothing new... I did this for the imx6 based Gateworks Ventana
> boards as well most of which have been upstream since Linux 4.x:

Based on this, I'm going to say what you have is fine and let's move on.

We'll all probably forget this and you'll have to remind us the next
time we see wildcards.

Rob

Powered by blists - more mailing lists