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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25356e61-2e53-483f-916e-5a3685b5e108@lunn.ch>
Date: Tue, 27 Aug 2024 14:34:45 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Wei Fang <wei.fang@....com>
Cc: Rob Herring <robh@...nel.org>,
	"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>,
	"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

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