Message-ID: <20240530-powwow-outpour-ca48b1f22a3e@spud>
Date: Thu, 30 May 2024 17:54:19 +0100
From: Conor Dooley <conor@...nel.org>
To: Tim Harvey <tharvey@...eworks.com>
Cc: Rob Herring <robh@...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 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?

> > You haven't documented the other 2 though.
> >
> > How do "all GW75** use the same device-tree", but then there are 3
> > possible DTs for just 1 board?
> >
> > Selecting a DT is not a unique problem. We don't need unique
> > solutions. There's the QCom board-id proposal[1] and OS provided DT[2]
> > which are addressing similar issues.
> >
> 
> Hi Rob,
> 
> I'm not sure those links are really able to address all needs. I see
> some similarity with the concept of a board-id taking the place of the
> don't-cares used in our names but not the concept of marrying a
> baseboard to a SOM with the two different boards creating a named
> combination (both which may have some don't cares). The Gateworks
> Venice product family of boards (imx8m{m,n,p}-gw7***-*x) boards have
> been in the kernel for quite some time now as has been the U-Boot code
> that determines the device tree using a baseboard model number
> combined with a SOM model number.
> 
> A baseboard with an model of GW7301 (programmed into an EERPOM at mfg
> time) gets coupled with a SOM with the model of GW7000 and this uses a
> device-tree of gw73xx-0x (prepended by the SoC name of imx8mm, imx8mn,
> imx8mp). The don't care's here and the naming convention has been
> chosen by us, the board manufacturer, leaving enough significant
> digits for component subloads that was desired at the time. So a
> GW7300 and a GW7301 are the same schematic, they just have some
> different loading options.

By "loading options" do you mean "we changed the supplier for the 12v
barrel jack" rather than something that is actually visible to an OS?

> I really don't understand the issue here. A board was originally named
> gw7905 when I brought up the prototype in the lab and created its
> device-tree but between then and when it shipped it got moved to the
> more generic 'family' of gw75xx baseboards which get coupled with a
> SOM. I already have a gw71xx, gw72xx, gw73xx out there for years that
> function this way.
> 
> Device trees describe hardware using a name... the name changed :(
> 
> Quite simply there are no boards out there with a GW7905 in the EEPROM
> that need to be supported... they all have a GW7500 programmed in them
> (and some may in the future have a GW7501, GW7502, etc).
> 
> Is the problem here the fact that I use don't-cares in the names or
> the fact that a name changed?

IMO, getting hung up on the name changing before a released product is
rather silly, but straightening out what exactly your selection method
is worthwhile.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists