diff mbox series

[1/2] dt-bindings: arm: fsl: rename gw7905 to gw75xx

Message ID 20240522215043.3747651-1-tharvey@gateworks.com (mailing list archive)
State New, archived
Headers show
Series [1/2] dt-bindings: arm: fsl: rename gw7905 to gw75xx | expand

Commit Message

Tim Harvey May 22, 2024, 9:50 p.m. UTC
The GW7905 was renamed to GW7500 before production release.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski May 23, 2024, 7:02 a.m. UTC | #1
On 22/05/2024 23:50, Tim Harvey wrote:
> The GW7905 was renamed to GW7500 before production release.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.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.

Best regards,
Krzysztof
Conor Dooley May 23, 2024, 2:47 p.m. UTC | #2
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@gateworks.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.
Tim Harvey May 23, 2024, 11:04 p.m. UTC | #3
On Thu, May 23, 2024 at 7:47 AM Conor Dooley <conor@kernel.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@gateworks.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?

Best Regards,

Tim
Conor Dooley May 24, 2024, 6:40 p.m. UTC | #4
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@kernel.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@gateworks.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.
Krzysztof Kozlowski May 25, 2024, 6:34 p.m. UTC | #5
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@kernel.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@gateworks.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.

Best regards,
Krzysztof
Tim Harvey May 25, 2024, 7:58 p.m. UTC | #6
On Sat, May 25, 2024 at 11:34 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.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@kernel.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@gateworks.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.

Best Regards,

Tim
Rob Herring May 28, 2024, 3:58 p.m. UTC | #7
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@linaro.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@kernel.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@gateworks.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.

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.

Rob

[1] https://lore.kernel.org/all/20240521-board-ids-v3-0-e6c71d05f4d2@quicinc.com/
[2] https://lists.linaro.org/archives/list/boot-architecture@lists.linaro.org/thread/DZCZSOCRH5BN7YOXEL2OQKSDIY7DCW2M/
Tim Harvey May 28, 2024, 4:23 p.m. UTC | #8
On Tue, May 28, 2024 at 8:58 AM Rob Herring <robh@kernel.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@linaro.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@kernel.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@gateworks.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.
>
> 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.

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?

Best Regards,

Tim

> Rob
>
> [1] https://lore.kernel.org/all/20240521-board-ids-v3-0-e6c71d05f4d2@quicinc.com/
> [2] https://lists.linaro.org/archives/list/boot-architecture@lists.linaro.org/thread/DZCZSOCRH5BN7YOXEL2OQKSDIY7DCW2M/
Conor Dooley May 30, 2024, 4:54 p.m. UTC | #9
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@kernel.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@linaro.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@kernel.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@gateworks.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.
Tim Harvey May 30, 2024, 5:36 p.m. UTC | #10
On Thu, May 30, 2024 at 9:54 AM Conor Dooley <conor@kernel.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@kernel.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@linaro.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@kernel.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@gateworks.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

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:
$ ls -1 arch/arm/boot/dts/nxp/imx/imx6*gw*.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw51xx.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw52xx.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw53xx.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw54xx.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw551x.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw552x.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw553x.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw560x.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw5903.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw5904.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw5907.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw5910.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw5912.dts
arch/arm/boot/dts/nxp/imx/imx6dl-gw5913.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw51xx.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw52xx.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw53xx.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5400-a.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw54xx.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw551x.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw552x.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw553x.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw560x.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5903.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5904.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5907.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5910.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5912.dts
arch/arm/boot/dts/nxp/imx/imx6q-gw5913.dts

$ git grep gw,imx6q-gw5 Documentation/devicetree/bindings/arm/fsl.yaml
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw51xx
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw52xx
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw53xx
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5400-a
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw54xx
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw551x
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw552x
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw553x
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw560x
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5903
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5904
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5907
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5910
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5912
Documentation/devicetree/bindings/arm/fsl.yaml:              - gw,imx6q-gw5913

So the formula we use here at Gateworks is the first number is the
product family (5=imx6q/dl ventana, 7=imx8m/n/p venice), the next
digit (or digits) is the baseboard and if the 9xx model numbers are
1-off designs that will never have variations.

So here is what currently exists for the Venice family:
$ git grep gw,imx8 Documentation/devicetree/bindings/arm/fsl.yaml
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mm-gw71xx-0x       # i.MX8MM Gateworks Development Kit
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mm-gw72xx-0x       # i.MX8MM Gateworks Development Kit
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mm-gw73xx-0x       # i.MX8MM Gateworks Development Kit
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mm-gw7901          # i.MX8MM Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mm-gw7902          # i.MX8MM Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mm-gw7903          # i.MX8MM Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gw,imx8mn-gw7902          # i.MX8MM Gateworks Board
$ git grep gateworks,imx8 Documentation/devicetree/bindings/arm/fsl.yaml
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mm-gw7904
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mm-gw7905-0x # i.MX8MM Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mp-gw71xx-2x # i.MX8MP Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mp-gw72xx-2x # i.MX8MP Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mp-gw73xx-2x # i.MX8MP Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mp-gw74xx   # i.MX8MP Gateworks Board
Documentation/devicetree/bindings/arm/fsl.yaml:              -
gateworks,imx8mp-gw7905-2x # i.MX8MP Gateworks Board

All that I'm trying to do is correct the fact that during the point I
had boards in the lab and created the device-tree and the point at
which we shipped the boards it was decided that the GW7905 should not
just be a 1-off board and that we wanted to make it a more widely
available baseboard with room for changes thus it was renamed from
GW7905 to GW7500 with expected GW7501, GW7502, GW7503 revisions
coming.

My whole point here is that in reference to the product naming scheme
I'm not doing anything different than what has been accepted before.
We have our reasons for naming this board combination
'gateworks,imx8mp-gw75xx-2x' instead of the original name
'gateworks,imx8mp-gw7905-2x'. Isn't the whole point that this is a
vendor defined string? I get that it is a string that should not
change but this name change was made before product was shipped so it
seems fair to make the change. If gateworks changed the company name
we would not be allowed to 'change' the existing strings but could
create a new one and start using it (perfect example was when someone
(not us!) decided the vendor name abbreviation of 'gw' should be
replaced with 'gateworks'.

I could stick with the idea that 'gateworks,imx8mp-gw7905-2x' is set
in stone and leave it there and just create a new
'gateworks,imx8mp-gw75xx-2x' but I hate to just duplicate the actual
dts files as that is just a waste and the reason for the 2nd patch of
this series.

Looking for guidance...

Best Regards,

Tim

>
> > > 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.
Rob Herring May 31, 2024, 2:10 p.m. UTC | #11
On Thu, May 30, 2024 at 12:36 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Thu, May 30, 2024 at 9:54 AM Conor Dooley <conor@kernel.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@kernel.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@linaro.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@kernel.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@gateworks.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
Rob Herring May 31, 2024, 2:13 p.m. UTC | #12
On Wed, May 22, 2024 at 4:50 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> The GW7905 was renamed to GW7500 before production release.

Maybe some summary of the discussion and how this changed from one-off
to wider availability.

>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>
Tim Harvey Aug. 13, 2024, 4:22 p.m. UTC | #13
On Fri, May 31, 2024 at 7:13 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, May 22, 2024 at 4:50 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > The GW7905 was renamed to GW7500 before production release.
>
> Maybe some summary of the discussion and how this changed from one-off
> to wider availability.
>
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Rob Herring <robh@kernel.org>

Hi Rob,

What is the status of this patch? I'm not clear what tree the
Documentation/devicetree/bindings go through.

Best regards,

Tim
Krzysztof Kozlowski Aug. 13, 2024, 4:34 p.m. UTC | #14
On 13/08/2024 18:22, Tim Harvey wrote:
> On Fri, May 31, 2024 at 7:13 AM Rob Herring <robh@kernel.org> wrote:
>>
>> On Wed, May 22, 2024 at 4:50 PM Tim Harvey <tharvey@gateworks.com> wrote:
>>>
>>> The GW7905 was renamed to GW7500 before production release.
>>
>> Maybe some summary of the discussion and how this changed from one-off
>> to wider availability.
>>
>>>
>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>> ---
>>>  Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> Hi Rob,
> 
> What is the status of this patch? I'm not clear what tree the
> Documentation/devicetree/bindings go through.

Always via given subsystem. Which subsystem is here? Maintainers should
tell you - ARM Freescale/NXP.

See also:
https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79

Best regards,
Krzysztof
Tim Harvey Aug. 13, 2024, 5:10 p.m. UTC | #15
On Tue, Aug 13, 2024 at 9:34 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/08/2024 18:22, Tim Harvey wrote:
> > On Fri, May 31, 2024 at 7:13 AM Rob Herring <robh@kernel.org> wrote:
> >>
> >> On Wed, May 22, 2024 at 4:50 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >>>
> >>> The GW7905 was renamed to GW7500 before production release.
> >>
> >> Maybe some summary of the discussion and how this changed from one-off
> >> to wider availability.
> >>
> >>>
> >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> Reviewed-by: Rob Herring <robh@kernel.org>
> >
> > Hi Rob,
> >
> > What is the status of this patch? I'm not clear what tree the
> > Documentation/devicetree/bindings go through.
>
> Always via given subsystem. Which subsystem is here? Maintainers should
> tell you - ARM Freescale/NXP.
>
> See also:
> https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79
>
> Best regards,
> Krzysztof
>

Krzysztof, thanks - that makes sense.

Shawn, what is the status of this series [1]

Best Regards,

Tim
1 - https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=855146&state=%2A&archive=both
Rob Herring Aug. 19, 2024, 1:17 p.m. UTC | #16
On Tue, Aug 13, 2024 at 11:10 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Tue, Aug 13, 2024 at 9:34 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 13/08/2024 18:22, Tim Harvey wrote:
> > > On Fri, May 31, 2024 at 7:13 AM Rob Herring <robh@kernel.org> wrote:
> > >>
> > >> On Wed, May 22, 2024 at 4:50 PM Tim Harvey <tharvey@gateworks.com> wrote:
> > >>>
> > >>> The GW7905 was renamed to GW7500 before production release.
> > >>
> > >> Maybe some summary of the discussion and how this changed from one-off
> > >> to wider availability.
> > >>
> > >>>
> > >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/arm/fsl.yaml | 4 ++--
> > >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> Reviewed-by: Rob Herring <robh@kernel.org>
> > >
> > > Hi Rob,
> > >
> > > What is the status of this patch? I'm not clear what tree the
> > > Documentation/devicetree/bindings go through.
> >
> > Always via given subsystem. Which subsystem is here? Maintainers should
> > tell you - ARM Freescale/NXP.
> >
> > See also:
> > https://elixir.bootlin.com/linux/v6.11-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L79
> >
> > Best regards,
> > Krzysztof
> >
>
> Krzysztof, thanks - that makes sense.
>
> Shawn, what is the status of this series [1]

No maintainer is going to look at 3 month old patches and may or may
not see some reply on it. Generally, if there's any discussion on a
patch, I assume a new version will be the result and move on. So
resend your patch.

Rob
diff mbox series

Patch

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
               - gateworks,imx8mm-gw7904
-              - gateworks,imx8mm-gw7905-0x # i.MX8MM Gateworks Board
               - gw,imx8mm-gw71xx-0x       # i.MX8MM Gateworks Development Kit
               - gw,imx8mm-gw72xx-0x       # i.MX8MM Gateworks Development Kit
               - gw,imx8mm-gw73xx-0x       # i.MX8MM Gateworks Development Kit
@@ -1055,7 +1055,7 @@  properties:
               - gateworks,imx8mp-gw72xx-2x # i.MX8MP Gateworks Board
               - gateworks,imx8mp-gw73xx-2x # i.MX8MP Gateworks Board
               - gateworks,imx8mp-gw74xx   # i.MX8MP Gateworks Board
-              - gateworks,imx8mp-gw7905-2x # i.MX8MP Gateworks Board
+              - gateworks,imx8mp-gw75xx-2x # i.MX8MP Gateworks Board
               - skov,imx8mp-skov-revb-hdmi # SKOV i.MX8MP climate control without panel
               - skov,imx8mp-skov-revb-lt6 # SKOV i.MX8MP climate control with 7” panel
               - skov,imx8mp-skov-revb-mi1010ait-1cp1 # SKOV i.MX8MP climate control with 10.1" panel