Message-ID:
 <PAXPR04MB8510228822AFFD8BD9F7414388942@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Tue, 27 Aug 2024 03:25:12 +0000
From: Wei Fang <wei.fang@....com>
To: Rob Herring <robh@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"andrew@...n.ch" <andrew@...n.ch>, "f.fainelli@...il.com"
	<f.fainelli@...il.com>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>, "Andrei Botila (OSS)"
	<andrei.botila@....nxp.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: RE: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add
 "nxp,phy-output-refclk" property

> -----Original Message-----
> From: Rob Herring <robh@...nel.org>
> Sent: 2024年8月26日 23:50
> To: Wei Fang <wei.fang@....com>
> Cc: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; krzk+dt@...nel.org; conor+dt@...nel.org;
> andrew@...n.ch; f.fainelli@...il.com; hkallweit1@...il.com;
> linux@...linux.org.uk; Andrei Botila (OSS) <andrei.botila@....nxp.com>;
> netdev@...r.kernel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; imx@...ts.linux.dev
> Subject: Re: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add
> "nxp,phy-output-refclk" property
> 
> On Mon, Aug 26, 2024 at 01:26:59PM +0800, Wei Fang wrote:
> > Per the RMII specification, the REF_CLK is sourced from MAC to PHY or
> > from an external source. But for TJA11xx PHYs, they support to output
> > a 50MHz RMII reference clock on REF_CLK pin. Previously the
> > "nxp,rmii-refclk-in" was added to indicate that in RMII mode, if this
> > property present, REF_CLK is input to the PHY, otherwise it is output.
> > This seems inappropriate now. Because according to the RMII
> > specification, the REF_CLK is originally input, so there is no need to
> > add an additional "nxp,rmii-refclk-in" property to declare that
> > REF_CLK is input.
> > Unfortunately, because the "nxp,rmii-refclk-in" property has been
> > added for a while, and we cannot confirm which DTS use the TJA1100 and
> > TJA1101 PHYs, changing it to switch polarity will cause an ABI break.
> > But fortunately, this property is only valid for TJA1100 and TJA1101.
> > For TJA1103/TJA1104/TJA1120/TJA1121 PHYs, this property is invalid
> > because they use the nxp-c45-tja11xx driver, which is a different
> > driver from TJA1100/TJA1101. Therefore, for PHYs using nxp-c45-tja11xx
> > driver, add "nxp,phy-output-refclk" property to support outputting
> > RMII reference clock on REF_CLK pin.
> >
> > Signed-off-by: Wei Fang <wei.fang@....com>
> > ---
> > V2 changes:
> > 1. Change the property name from "nxp,reverse-mode" to
> > "nxp,phy-output-refclk".
> > 2. Simplify the description of the property.
> > 3. Modify the subject and commit message.
> > V3 changes:
> > 1. Keep the "nxp,rmii-refclk-in" property for TJA1100 and TJA1101.
> > 2. Rephrase the commit message and subject.
> > ---
> >  Documentation/devicetree/bindings/net/nxp,tja11xx.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> 
> This binding is completely broken. I challenge you to make it report any errors.
> Those issues need to be addressed before you add more properties.
> 
Sorry, I'm not sure I fully understand what you mean, do you mean I need
to move the "nxp,rmii-refclk-in" property out of the patternProperties?
Just like below?
+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,28 +71,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.

> If you want/need custom properties, then you must have a compatible string.
> 
I looked at the binding documentation of other PHYs and there doesn't seem to
be any precedent for doing this. Is this a newly added dt-binding rule?

There is another question. For PHY, usually its compatible string is either
"ethernet-phy-ieee802.3-c45" or "ethernet-phy-ieee802.3-c22". If I want to
add a custom property to TJA11xx PHY, can I use these generic compatible
strings? As shown below:

allOf:
   - $ref: ethernet-phy.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ethernet-phy-ieee802.3-c22
+    then:
+      properties:
+        nxp,phy-output-refclk: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ethernet-phy-ieee802.3-c45
+    then:
+      properties:
+        nxp,rmii-refclk-in: false

> >
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > index 85bfa45f5122..f775036a7521 100644
> > --- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -48,6 +48,12 @@ patternProperties:
> >            reference clock output when RMII mode enabled.
> >            Only supported on TJA1100 and TJA1101.
> >
> > +      nxp,phy-output-refclk:
> 
> Why not "nxp,rmii-refclk-out" if this is just the reverse of the existing property.
> 
This is a historical issue. At first, I defined "nxp, reverse-mode" to replace the
existing "nxp, rmii-refclk-in" property, but Andrew suggested that I refer to other
PHY drivers and use a more generic name, so I referred to adin PHY and changed
it to "nxp, phy-output-refclk". Of course, under the premise of retaining the
"nxp,rmii-refclk-in" property, using "nxp,rmii-refclk-out" seems more appropriate.

> > +        type: boolean
> > +        description: |
> > +          Enable 50MHz RMII reference clock output on REF_CLK pin.
> This
> > +          property is only applicable to nxp-c45-tja11xx driver.
> > +
> >      required:
> >        - reg
> >
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists