> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: 2024年8月26日 23:50
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> andrew@xxxxxxx; f.fainelli@xxxxxxxxx; hkallweit1@xxxxxxxxx;
> linux@xxxxxxxxxxxxxxx; Andrei Botila (OSS) <andrei.botila@xxxxxxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > 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
> >
