[PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Wei Fang]

As Rob pointed in another mail thread [1], the binding of tja11xx PHY
is completely broken, the schema cannot catch the error in the DTS. A
compatiable string must be needed if we want to add a custom propety.
So extract known PHY IDs from the tja11xx PHY drivers and convert them
into supported compatible string list to fix the broken binding issue.

[1]: https://lore.kernel.org/netdev/31058f49-bac5-49a9-a422-c43b121bf049@kernel.org/T/

Fixes: 52b2fe4535ad ("dt-bindings: net: tja11xx: add nxp,refclk_in property")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 .../devicetree/bindings/net/nxp,tja11xx.yaml  | 50 +++++++++++++------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
index 85bfa45f5122..c2a1835863e1 100644
--- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
+++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
@@ -14,8 +14,41 @@ maintainers:
 description:
   Bindings for NXP TJA11xx automotive PHYs
 
+properties:
+  compatible:
+    enum:
+      - ethernet-phy-id0180.dc40
+      - ethernet-phy-id0180.dd00
+      - ethernet-phy-id0180.dc80
+      - ethernet-phy-id001b.b010
+      - ethernet-phy-id001b.b031
+
 allOf:
   - $ref: ethernet-phy.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - ethernet-phy-id0180.dc40
+              - ethernet-phy-id0180.dd00
+    then:
+      properties:
+        nxp,rmii-refclk-in:
+          type: boolean
+          description: |
+            The REF_CLK is provided for both transmitted and received data
+            in RMII mode. This clock signal is provided by the PHY and is
+            typically derived from an external 25MHz crystal. Alternatively,
+            a 50MHz clock signal generated by an external oscillator can be
+            connected to pin REF_CLK. A third option is to connect a 25MHz
+            clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
+            as input or output according to the actual circuit connection.
+            If present, indicates that the REF_CLK will be configured as
+            interface reference clock input when RMII mode enabled.
+            If not present, the REF_CLK will be configured as interface
+            reference clock output when RMII mode enabled.
+            Only supported on TJA1100 and TJA1101.
 
 patternProperties:
   "^ethernet-phy@[0-9a-f]+$":
@@ -32,22 +65,6 @@ patternProperties:
         description:
           The ID number for the child PHY. Should be +1 of parent PHY.
 
-      nxp,rmii-refclk-in:
-        type: boolean
-        description: |
-          The REF_CLK is provided for both transmitted and received data
-          in RMII mode. This clock signal is provided by the PHY and is
-          typically derived from an external 25MHz crystal. Alternatively,
-          a 50MHz clock signal generated by an external oscillator can be
-          connected to pin REF_CLK. A third option is to connect a 25MHz
-          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
-          as input or output according to the actual circuit connection.
-          If present, indicates that the REF_CLK will be configured as
-          interface reference clock input when RMII mode enabled.
-          If not present, the REF_CLK will be configured as interface
-          reference clock output when RMII mode enabled.
-          Only supported on TJA1100 and TJA1101.
-
     required:
       - reg
 
@@ -60,6 +77,7 @@ examples:
         #size-cells = <0>;
 
         tja1101_phy0: ethernet-phy@4 {
+            compatible = "ethernet-phy-id0180.dc40";
             reg = <0x4>;
             nxp,rmii-refclk-in;
         };
-- 
2.34.1

Re: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Andrew Lunn]

> +properties:
> +  compatible:
> +    enum:
> +      - ethernet-phy-id0180.dc40
> +      - ethernet-phy-id0180.dd00
> +      - ethernet-phy-id0180.dc80
> +      - ethernet-phy-id001b.b010
> +      - ethernet-phy-id001b.b031

This shows the issues with using a compatible. The driver has:

#define PHY_ID_TJA_1120                 0x001BB031

                PHY_ID_MATCH_MODEL(PHY_ID_TJA_1120),

which means the lowest nibble is ignored. The driver will quite happy
also probe for hardware using 001b.b030, 001b.b032, 001b.b033,
... 001b.b03f

Given you are inside NXP, do any of these exist? Was 001b.b030 too
broken it never left the QA lab? Are there any hardware issues which
might result in a new silicon stepping?

Does ethernet-phy-id0180.dc41 exist? etc.

	Andrew

RE: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Wei Fang]

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ethernet-phy-id0180.dc40
> > +      - ethernet-phy-id0180.dd00
> > +      - ethernet-phy-id0180.dc80
> > +      - ethernet-phy-id001b.b010
> > +      - ethernet-phy-id001b.b031
> 
> This shows the issues with using a compatible. The driver has:
> 
> #define PHY_ID_TJA_1120                 0x001BB031
> 
>                 PHY_ID_MATCH_MODEL(PHY_ID_TJA_1120),
> 
> which means the lowest nibble is ignored. The driver will quite happy also probe
> for hardware using 001b.b030, 001b.b032, 001b.b033, ... 001b.b03f
> 
> Given you are inside NXP, do any of these exist? Was 001b.b030 too broken it
> never left the QA lab? Are there any hardware issues which might result in a
> new silicon stepping?

Yes, some of the revisions do exist, but the driver should be compatible with
these different revisions.

For 001b.b030, I don't think it is broken, based on the latest data sheet of
TJA1120 (Rev 0.6 26 January 2023), the PHY ID is 001b.b030. I don't know
why it is defined as 001b.b031 in the driver, it may be a typo.
> 
> Does ethernet-phy-id0180.dc41 exist? etc.
I think other TJA PHYs should also have different revisions.

Because the driver ignores the lowest nibble of the PHY ID, I think it is fine to
define the lowest nibble of the PHY ID in these compatible strings as 0, and
there is no need to list all revisions. And I don't know which revisions exist,
because I haven't found or have no permission to download some PHY data
sheets. I think what I can do is to modify "ethernet-phy-id001b.b031" to
"ethernet-phy-id001b.b030".

Re: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Andrew Lunn]

On Tue, Sep 03, 2024 at 02:17:04AM +0000, Wei Fang wrote:
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - ethernet-phy-id0180.dc40
> > > +      - ethernet-phy-id0180.dd00
> > > +      - ethernet-phy-id0180.dc80
> > > +      - ethernet-phy-id001b.b010
> > > +      - ethernet-phy-id001b.b031
> > 
> > This shows the issues with using a compatible. The driver has:
> > 
> > #define PHY_ID_TJA_1120                 0x001BB031
> > 
> >                 PHY_ID_MATCH_MODEL(PHY_ID_TJA_1120),
> > 
> > which means the lowest nibble is ignored. The driver will quite happy also probe
> > for hardware using 001b.b030, 001b.b032, 001b.b033, ... 001b.b03f
> > 
> > Given you are inside NXP, do any of these exist? Was 001b.b030 too broken it
> > never left the QA lab? Are there any hardware issues which might result in a
> > new silicon stepping?
> 
> Yes, some of the revisions do exist, but the driver should be compatible with
> these different revisions.
> 
> For 001b.b030, I don't think it is broken, based on the latest data sheet of
> TJA1120 (Rev 0.6 26 January 2023), the PHY ID is 001b.b030. I don't know
> why it is defined as 001b.b031 in the driver, it may be a typo.

More likely, the board Radu Pirea has does have a device with this ID.

> > 
> > Does ethernet-phy-id0180.dc41 exist? etc.
> I think other TJA PHYs should also have different revisions.
> 
> Because the driver ignores the lowest nibble of the PHY ID, I think it is fine to
> define the lowest nibble of the PHY ID in these compatible strings as 0, and
> there is no need to list all revisions. And I don't know which revisions exist,
> because I haven't found or have no permission to download some PHY data
> sheets. I think what I can do is to modify "ethernet-phy-id001b.b031" to
> "ethernet-phy-id001b.b030".

You have to be careful here. Stating a compatible forces the PHY
ID. So if the compatible is "ethernet-phy-id001b.b031", but the board
actually has a "ethernet-phy-id001b.b030". phydev->phy_id is going to
be set to 0x001bb031. Any behaviour in the driver which look at that
revision nibble is then going to be wrong.

Maybe, now, today, that does not matter, because the driver never
looks at the revision. But it does mean developers might put the wrong
compatible in DT. And then when you do need to add code looking at the
revision, it does not always work, because there are some boards with
the wrong compatible in DT.

Listing all possible compatibles suggests to developers they need to
be careful and use the correct value. Or add a comment in the DT
bindings that not using a compatible is probably safer.

	Andrew

RE: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Wei Fang]

> On Tue, Sep 03, 2024 at 02:17:04AM +0000, Wei Fang wrote:
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - ethernet-phy-id0180.dc40
> > > > +      - ethernet-phy-id0180.dd00
> > > > +      - ethernet-phy-id0180.dc80
> > > > +      - ethernet-phy-id001b.b010
> > > > +      - ethernet-phy-id001b.b031
> > >
> > > This shows the issues with using a compatible. The driver has:
> > >
> > > #define PHY_ID_TJA_1120                 0x001BB031
> > >
> > >                 PHY_ID_MATCH_MODEL(PHY_ID_TJA_1120),
> > >
> > > which means the lowest nibble is ignored. The driver will quite
> > > happy also probe for hardware using 001b.b030, 001b.b032, 001b.b033,
> > > ... 001b.b03f
> > >
> > > Given you are inside NXP, do any of these exist? Was 001b.b030 too
> > > broken it never left the QA lab? Are there any hardware issues which
> > > might result in a new silicon stepping?
> >
> > Yes, some of the revisions do exist, but the driver should be
> > compatible with these different revisions.
> >
> > For 001b.b030, I don't think it is broken, based on the latest data
> > sheet of
> > TJA1120 (Rev 0.6 26 January 2023), the PHY ID is 001b.b030. I don't
> > know why it is defined as 001b.b031 in the driver, it may be a typo.
> 
> More likely, the board Radu Pirea has does have a device with this ID.
> 
> > >
> > > Does ethernet-phy-id0180.dc41 exist? etc.
> > I think other TJA PHYs should also have different revisions.
> >
> > Because the driver ignores the lowest nibble of the PHY ID, I think it
> > is fine to define the lowest nibble of the PHY ID in these compatible
> > strings as 0, and there is no need to list all revisions. And I don't
> > know which revisions exist, because I haven't found or have no
> > permission to download some PHY data sheets. I think what I can do is
> > to modify "ethernet-phy-id001b.b031" to "ethernet-phy-id001b.b030".
> 
> You have to be careful here. Stating a compatible forces the PHY ID. So if the
> compatible is "ethernet-phy-id001b.b031", but the board actually has a
> "ethernet-phy-id001b.b030". phydev->phy_id is going to be set to 0x001bb031.
> Any behaviour in the driver which look at that revision nibble is then going to be
> wrong.
> 
> Maybe, now, today, that does not matter, because the driver never looks at the
> revision. But it does mean developers might put the wrong compatible in DT.
> And then when you do need to add code looking at the revision, it does not
> always work, because there are some boards with the wrong compatible in DT.
> 
> Listing all possible compatibles suggests to developers they need to be careful
> and use the correct value. Or add a comment in the DT bindings that not using a
> compatible is probably safer.
> 
Thanks for the suggestion, I will try my best to check more data sheets and list all
the PHY IDs I know. I can't guarantee that I can list all the PHY IDs, but I will update
it in the future when I get more information.

Re: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Rob Herring]

On Mon, Sep 02, 2024 at 02:33:52PM +0800, Wei Fang wrote:
> As Rob pointed in another mail thread [1], the binding of tja11xx PHY
> is completely broken, the schema cannot catch the error in the DTS. A
> compatiable string must be needed if we want to add a custom propety.
> So extract known PHY IDs from the tja11xx PHY drivers and convert them
> into supported compatible string list to fix the broken binding issue.
> 
> [1]: https://lore.kernel.org/netdev/31058f49-bac5-49a9-a422-c43b121bf049@kernel.org/T/
> 
> Fixes: 52b2fe4535ad ("dt-bindings: net: tja11xx: add nxp,refclk_in property")
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  .../devicetree/bindings/net/nxp,tja11xx.yaml  | 50 +++++++++++++------
>  1 file changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> index 85bfa45f5122..c2a1835863e1 100644
> --- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> @@ -14,8 +14,41 @@ maintainers:
>  description:
>    Bindings for NXP TJA11xx automotive PHYs
>  
> +properties:
> +  compatible:
> +    enum:
> +      - ethernet-phy-id0180.dc40
> +      - ethernet-phy-id0180.dd00
> +      - ethernet-phy-id0180.dc80
> +      - ethernet-phy-id001b.b010
> +      - ethernet-phy-id001b.b031
> +
>  allOf:
>    - $ref: ethernet-phy.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ethernet-phy-id0180.dc40
> +              - ethernet-phy-id0180.dd00
> +    then:
> +      properties:
> +        nxp,rmii-refclk-in:
> +          type: boolean
> +          description: |
> +            The REF_CLK is provided for both transmitted and received data
> +            in RMII mode. This clock signal is provided by the PHY and is
> +            typically derived from an external 25MHz crystal. Alternatively,
> +            a 50MHz clock signal generated by an external oscillator can be
> +            connected to pin REF_CLK. A third option is to connect a 25MHz
> +            clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
> +            as input or output according to the actual circuit connection.
> +            If present, indicates that the REF_CLK will be configured as
> +            interface reference clock input when RMII mode enabled.
> +            If not present, the REF_CLK will be configured as interface
> +            reference clock output when RMII mode enabled.
> +            Only supported on TJA1100 and TJA1101.
>  
>  patternProperties:
>    "^ethernet-phy@[0-9a-f]+$":
> @@ -32,22 +65,6 @@ patternProperties:
>          description:
>            The ID number for the child PHY. Should be +1 of parent PHY.
>  
> -      nxp,rmii-refclk-in:
> -        type: boolean
> -        description: |
> -          The REF_CLK is provided for both transmitted and received data
> -          in RMII mode. This clock signal is provided by the PHY and is
> -          typically derived from an external 25MHz crystal. Alternatively,
> -          a 50MHz clock signal generated by an external oscillator can be
> -          connected to pin REF_CLK. A third option is to connect a 25MHz
> -          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
> -          as input or output according to the actual circuit connection.
> -          If present, indicates that the REF_CLK will be configured as
> -          interface reference clock input when RMII mode enabled.
> -          If not present, the REF_CLK will be configured as interface
> -          reference clock output when RMII mode enabled.
> -          Only supported on TJA1100 and TJA1101.
> -
>      required:
>        - reg
>  
> @@ -60,6 +77,7 @@ examples:
>          #size-cells = <0>;
>  
>          tja1101_phy0: ethernet-phy@4 {
> +            compatible = "ethernet-phy-id0180.dc40";
>              reg = <0x4>;
>              nxp,rmii-refclk-in;

Are child phy devices optional? Either way, would be good to show a 
child device. IOW, make examples as complete as possible showing 
optional properties/nodes.

Rob

RE: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Wei Fang]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2024年9月4日 22:57
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; krzk+dt@kernel.org;
> conor+dt@kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
> hkallweit1@gmail.com; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding
> 
> On Mon, Sep 02, 2024 at 02:33:52PM +0800, Wei Fang wrote:
> > As Rob pointed in another mail thread [1], the binding of tja11xx PHY
> > is completely broken, the schema cannot catch the error in the DTS. A
> > compatiable string must be needed if we want to add a custom propety.
> > So extract known PHY IDs from the tja11xx PHY drivers and convert them
> > into supported compatible string list to fix the broken binding issue.
> >
> > [1]:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fnetdev%2F31058f49-bac5-49a9-a422-c43b121bf049%40kernel
> .o
> >
> rg%2FT%2F&data=05%7C02%7Cwei.fang%40nxp.com%7C5616407288034aee9
> cf508dc
> >
> ccf1e1c8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6386105864
> 643564
> >
> 75%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI
> >
> 6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=TxuYnwBHxDKIdA5qxds
> 8sb4fzyi
> > Tk80cqyM4X0mk5PY%3D&reserved=0
> >
> > Fixes: 52b2fe4535ad ("dt-bindings: net: tja11xx: add nxp,refclk_in
> > property")
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  .../devicetree/bindings/net/nxp,tja11xx.yaml  | 50
> > +++++++++++++------
> >  1 file changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > index 85bfa45f5122..c2a1835863e1 100644
> > --- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -14,8 +14,41 @@ maintainers:
> >  description:
> >    Bindings for NXP TJA11xx automotive PHYs
> >
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ethernet-phy-id0180.dc40
> > +      - ethernet-phy-id0180.dd00
> > +      - ethernet-phy-id0180.dc80
> > +      - ethernet-phy-id001b.b010
> > +      - ethernet-phy-id001b.b031
> > +
> >  allOf:
> >    - $ref: ethernet-phy.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - ethernet-phy-id0180.dc40
> > +              - ethernet-phy-id0180.dd00
> > +    then:
> > +      properties:
> > +        nxp,rmii-refclk-in:
> > +          type: boolean
> > +          description: |
> > +            The REF_CLK is provided for both transmitted and received
> data
> > +            in RMII mode. This clock signal is provided by the PHY and is
> > +            typically derived from an external 25MHz crystal.
> Alternatively,
> > +            a 50MHz clock signal generated by an external oscillator can
> be
> > +            connected to pin REF_CLK. A third option is to connect a
> 25MHz
> > +            clock to pin CLK_IN_OUT. So, the REF_CLK should be
> configured
> > +            as input or output according to the actual circuit connection.
> > +            If present, indicates that the REF_CLK will be configured as
> > +            interface reference clock input when RMII mode enabled.
> > +            If not present, the REF_CLK will be configured as interface
> > +            reference clock output when RMII mode enabled.
> > +            Only supported on TJA1100 and TJA1101.
> >
> >  patternProperties:
> >    "^ethernet-phy@[0-9a-f]+$":
> > @@ -32,22 +65,6 @@ patternProperties:
> >          description:
> >            The ID number for the child PHY. Should be +1 of parent PHY.
> >
> > -      nxp,rmii-refclk-in:
> > -        type: boolean
> > -        description: |
> > -          The REF_CLK is provided for both transmitted and received data
> > -          in RMII mode. This clock signal is provided by the PHY and is
> > -          typically derived from an external 25MHz crystal. Alternatively,
> > -          a 50MHz clock signal generated by an external oscillator can be
> > -          connected to pin REF_CLK. A third option is to connect a 25MHz
> > -          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
> > -          as input or output according to the actual circuit connection.
> > -          If present, indicates that the REF_CLK will be configured as
> > -          interface reference clock input when RMII mode enabled.
> > -          If not present, the REF_CLK will be configured as interface
> > -          reference clock output when RMII mode enabled.
> > -          Only supported on TJA1100 and TJA1101.
> > -
> >      required:
> >        - reg
> >
> > @@ -60,6 +77,7 @@ examples:
> >          #size-cells = <0>;
> >
> >          tja1101_phy0: ethernet-phy@4 {
> > +            compatible = "ethernet-phy-id0180.dc40";
> >              reg = <0x4>;
> >              nxp,rmii-refclk-in;
> 
> Are child phy devices optional? Either way, would be good to show a child device.
> IOW, make examples as complete as possible showing optional
> properties/nodes.
> 
The "nxp,rmii-refclk-in" property is only applicable to TJA1100/TJA1101 PHYs, which
do not have a child phy device.