[PATCH v3 net-next 0/2] make PHY output RMII reference clock

[PATCH v3 net-next 0/2] make PHY output RMII reference clock

[Wei Fang]

The TJA11xx PHYs have the capability to provide 50MHz reference clock
in RMII mode and output on REF_CLK pin. Therefore, add the new property
"nxp,phy-output-refclk" to support this feature. This property is only
available for PHYs which use nxp-c45-tja11xx driver, such as TJA1103,
TJA1104, TJA1120 and TJA1121.

---
V3 change:
Removed the patch 2 from the V2 patch set.
v2 Link: https://lore.kernel.org/netdev/20240823-jersey-conducive-70863dd6fd27@spud/T/
---

Wei Fang (2):
  dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property
  net: phy: c45-tja11xx: add support for outputing RMII reference clock

 .../devicetree/bindings/net/nxp,tja11xx.yaml  |  6 ++++
 drivers/net/phy/nxp-c45-tja11xx.c             | 29 +++++++++++++++++--
 drivers/net/phy/nxp-c45-tja11xx.h             |  1 +
 3 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Wei Fang]

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@nxp.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(+)

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:
+        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

[PATCH v3 net-next 2/2] net: phy: c45-tja11xx: add support for outputing RMII reference clock

[Wei Fang]

For TJA11xx PHYs, they have the capability to output 50MHz reference
clock on REF_CLK pin in RMII mode, which is called "revRMII" mode in
the PHY data sheet.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
V2 changes:
1. Change tihe property name.
2. Modify the subject and commit message.
V3 changes:
No changes.
---
 drivers/net/phy/nxp-c45-tja11xx.c | 29 +++++++++++++++++++++++++++--
 drivers/net/phy/nxp-c45-tja11xx.h |  1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 5af5ade4fc64..880d4ca883a8 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/mii.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/processor.h>
 #include <linux/property.h>
@@ -185,6 +186,8 @@
 
 #define NXP_C45_SKB_CB(skb)	((struct nxp_c45_skb_cb *)(skb)->cb)
 
+#define TJA11XX_REVERSE_MODE		BIT(0)
+
 struct nxp_c45_phy;
 
 struct nxp_c45_skb_cb {
@@ -1510,6 +1513,7 @@ static int nxp_c45_get_delays(struct phy_device *phydev)
 
 static int nxp_c45_set_phy_mode(struct phy_device *phydev)
 {
+	struct nxp_c45_phy *priv = phydev->priv;
 	int ret;
 
 	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_ABILITIES);
@@ -1561,8 +1565,13 @@ static int nxp_c45_set_phy_mode(struct phy_device *phydev)
 			phydev_err(phydev, "rmii mode not supported\n");
 			return -EINVAL;
 		}
-		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_MII_BASIC_CONFIG,
-			      MII_BASIC_CONFIG_RMII);
+
+		if (priv->flags & TJA11XX_REVERSE_MODE)
+			phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_MII_BASIC_CONFIG,
+				      MII_BASIC_CONFIG_RMII | MII_BASIC_CONFIG_REV);
+		else
+			phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_MII_BASIC_CONFIG,
+				      MII_BASIC_CONFIG_RMII);
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 		if (!(ret & SGMII_ABILITY)) {
@@ -1623,6 +1632,20 @@ static int nxp_c45_get_features(struct phy_device *phydev)
 	return genphy_c45_pma_read_abilities(phydev);
 }
 
+static int nxp_c45_parse_dt(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct nxp_c45_phy *priv = phydev->priv;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (of_property_read_bool(node, "nxp,phy-output-refclk"))
+		priv->flags |= TJA11XX_REVERSE_MODE;
+
+	return 0;
+}
+
 static int nxp_c45_probe(struct phy_device *phydev)
 {
 	struct nxp_c45_phy *priv;
@@ -1642,6 +1665,8 @@ static int nxp_c45_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	nxp_c45_parse_dt(phydev);
+
 	mutex_init(&priv->ptp_lock);
 
 	phy_abilities = phy_read_mmd(phydev, MDIO_MMD_VEND1,
diff --git a/drivers/net/phy/nxp-c45-tja11xx.h b/drivers/net/phy/nxp-c45-tja11xx.h
index f364fca68f0b..8b5fc383752b 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.h
+++ b/drivers/net/phy/nxp-c45-tja11xx.h
@@ -28,6 +28,7 @@ struct nxp_c45_phy {
 	int extts_index;
 	bool extts;
 	struct nxp_c45_macsec *macsec;
+	u32 flags;
 };
 
 #if IS_ENABLED(CONFIG_MACSEC)
-- 
2.34.1

Re: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Rob Herring]

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@nxp.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.

If you want/need custom properties, then you must have a compatible 
string.

> 
> 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.

> +        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
> 

RE: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Wei Fang]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2024年8月26日 23:50
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; krzk+dt@kernel.org; conor+dt@kernel.org;
> andrew@lunn.ch; f.fainelli@gmail.com; hkallweit1@gmail.com;
> linux@armlinux.org.uk; Andrei Botila (OSS) <andrei.botila@oss.nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; imx@lists.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@nxp.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
> >

Re: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Andrew Lunn]

> > 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:

This is where we get into the differences between how the kernel
actually works, and how the tools work. The kernel does not need a
compatible, it reads the ID registers and uses that to load the
driver. You can optionally have a compatible with the contents of the
ID registers, and that will force the kernel to ignore the ID in the
hardware and load a specific driver.

The DT tools however require a compatible in order to match the node
in the blob to the binding in a .yaml file. Without the compatible,
the binding is not imposed, which is why you will never see an error.

So in the example, include a compatible, using the real ID.

For a real DT blob, you need to decide if you want to include a
compatible or not. The downside is that it forces the ID. It is not
unknown for board manufacturers to replace a PHY with another pin
compatible PHY. Without a compatible, the kernel will load the correct
driver, based on the ID. With a compatible it will keep using the same
driver, which is probably wrong for the hardware.

Does the PHY use the lower nibble to indicate the revision? Using a
compatible will also override the revision. So the driver cannot even
trust the revision if there is a compatible.

	Andrew

Re: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Krzysztof Kozlowski]

On 27/08/2024 05:25, Wei Fang wrote:
>> -----Original Message-----
>> From: Rob Herring <robh@kernel.org>
>> Sent: 2024年8月26日 23:50
>> To: Wei Fang <wei.fang@nxp.com>
>> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com; krzk+dt@kernel.org; conor+dt@kernel.org;
>> andrew@lunn.ch; f.fainelli@gmail.com; hkallweit1@gmail.com;
>> linux@armlinux.org.uk; Andrei Botila (OSS) <andrei.botila@oss.nxp.com>;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org; imx@lists.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@nxp.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

Make an intentional error in your DTS and then validate that the schema
catches it. That's the challenge.

Best regards,
Krzysztof

RE: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2024年8月27日 20:35
> To: Wei Fang <wei.fang@nxp.com>
> Cc: Rob Herring <robh@kernel.org>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> krzk+dt@kernel.org; conor+dt@kernel.org; f.fainelli@gmail.com;
> hkallweit1@gmail.com; linux@armlinux.org.uk; Andrei Botila (OSS)
> <andrei.botila@oss.nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add
> "nxp,phy-output-refclk" property
> 
> > > 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:
> 
> This is where we get into the differences between how the kernel actually
> works, and how the tools work. The kernel does not need a compatible, it
> reads the ID registers and uses that to load the driver. You can optionally have
> a compatible with the contents of the ID registers, and that will force the
> kernel to ignore the ID in the hardware and load a specific driver.
> 
> The DT tools however require a compatible in order to match the node in the
> blob to the binding in a .yaml file. Without the compatible, the binding is not
> imposed, which is why you will never see an error.
> 
> So in the example, include a compatible, using the real ID.
> 
> For a real DT blob, you need to decide if you want to include a compatible or
> not. The downside is that it forces the ID. It is not unknown for board
> manufacturers to replace a PHY with another pin compatible PHY. Without a
> compatible, the kernel will load the correct driver, based on the ID. With a
> compatible it will keep using the same driver, which is probably wrong for the
> hardware.
> 
> Does the PHY use the lower nibble to indicate the revision? Using a compatible
> will also override the revision. So the driver cannot even trust the revision if
> there is a compatible.
> 
Many thanks for the detailed explanation, currently both nxp-tja11xx and
nxp-c45-tja11xx drivers use PHY_ID_MATCH_MODEL() to match the PHY
driver, so the lower nibble is ignored.

RE: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add "nxp,phy-output-refclk" property

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2024年8月27日 21:27
> To: Wei Fang <wei.fang@nxp.com>; Rob Herring <robh@kernel.org>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; krzk+dt@kernel.org; conor+dt@kernel.org;
> andrew@lunn.ch; f.fainelli@gmail.com; hkallweit1@gmail.com;
> linux@armlinux.org.uk; Andrei Botila (OSS) <andrei.botila@oss.nxp.com>;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v3 net-next 1/2] dt-bindings: net: tja11xx: add
> "nxp,phy-output-refclk" property
> 
> On 27/08/2024 05:25, Wei Fang wrote:
> >> -----Original Message-----
> >> From: Rob Herring <robh@kernel.org>
> >> Sent: 2024年8月26日 23:50
> >> To: Wei Fang <wei.fang@nxp.com>
> >> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> >> pabeni@redhat.com; krzk+dt@kernel.org; conor+dt@kernel.org;
> >> andrew@lunn.ch; f.fainelli@gmail.com; hkallweit1@gmail.com;
> >> linux@armlinux.org.uk; Andrei Botila (OSS)
> >> <andrei.botila@oss.nxp.com>; netdev@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> imx@lists.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@nxp.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
> 
> Make an intentional error in your DTS and then validate that the schema
> catches it. That's the challenge.
> 
Thanks, got it, I will fix the issue first.