Re: [PATCH net-next 17/17] dt-bindings: net: dsa: Add documentation for NXP SJA1105 driver

[Vladimir Oltean]

On 4/3/19 12:38 AM, Andrew Lunn wrote:
> > +Optional properties:
> > +
> > +- sja1105,mac-mode, sja1105,phy-mode: Boolean properties that can be assigned
> > +  under each port node that is MII or RMII (has no effect for RGMII).  By
> > +  default (unless otherwise specified) a port is configured as MAC if it is
> > +  driving a PHY (phy-handle is present) or as PHY if it is PHY-less (fixed-link
> > +  specified, presumably because it is connected to a MAC).  These properties
> > +  are required in the case where SJA1105 ports are at both ends of an MII/RMII
> > +  PHY-less setup. One end would need to have sja1105,mac-mode, while the other
> > +  sja1105,phy-mode.
>
> Hi Vladimir
>
> phy-mode has a well known meaning, indicating mii, gmii, sqmii, rmii,
> rxaud, etc.
>
> The meaning here is quite different. To maybe avoid confusion, could
> we flip the name around?
>
> sja1105,mode-mac and sja1105,mode-phy?

Hi Andrew,

I agree the clash of words is confusing.
Which one of "sja1105,mode-phy", "sja1105,type-phy", "sja1105,role-phy" 
sounds easier to digest?

> > +			port@4 {
> > +				/* Internal port connected to eth2 */
> > +				ethernet = <&enet2>;
> > +				phy-mode = "rgmii";
> > +				reg = <4>;
> > +				/* Implicit "sja1105,phy-mode;" */
> > +				fixed-link {
> > +					speed = <1000>;
> > +					full-duplex;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
>
>
> > +&enet2 {
> > +	phy-connection-type = "rgmii";
>
> You don't see this used much, phy-mode is preferred. Do you have a
> reason for this?

No particular reason for using phy-connection-type. It is just a 
copy-pasta from the (not yet submitted) ls1021a-tsn.dts that I'm using 
to test the driver on.
The other LS1021A DTS files were already using this notation, so that's 
how it got there. But the gianfar driver works just as well with 
phy-mode, so maybe I will send another patch for converting the existing 
notations when I submit the new board DTS too.

> Also, you have the MAC using RGMII and the port using RGMII. Neither
> is inserting delays. That implies the delays are added by the track
> layout of the PCB. It would be good to add a comment about
> this. Anybody copying this using a design without the delays via PCB
> are probably going to get it wrong to start with and not realise they
> need to change one end to RGMII-ID.

You've hit the nail on the head with this excellent comment.
So the second-generation switches (P/Q/R/S) have addressed this hardware 
limitation and do provide some tunable delay lines for RGMII.
But on the LS1021A-TSN, which is using a first-generation T chip, the 
RGMII delays for correct sample/hold times are obtained through ~50cm 
copper wires, since the LS1021 cannot apply them internally either.

> https://www.nxp.com/docs/en/data-sheet/SJA1105.pdf
>
> Section 6.2.3 RGMII signaling and encoding
>
> Note that RGMII requires an external delay of between 1.5 ns and 2 ns
> on TXC and RXC.
>
> So it sounds like the switch only supports PHY_INTERFACE_MODE_RGMII.
>
> If the port is in MAC mode, you should pass this phy-mode to the PHY
> when you connect to it. The PHY can then add the delay if needed.
>
> If however, the port is in PHY mode, and it is asked to do RGMII other
> than PHY_INTERFACE_MODE_RGMII, you should report an error. It cannot
> do it.

I expect that next week I will be able to get a reworked LS1021A-TSN 
board with a second-generation switch, and the delay wires removed.
Using that, I can at least add support for the tunable delay lines in a 
fashion similar to what you're suggesting.

But since you brought this up, let me point out some complications I see:
* I think the way this works is that phy_attach_direct populates 
phydev->interface based on the MAC's DT bindings. Then the PHY driver is 
supposed to apply internal delays based on that. But this shadows the 
MAC's own ability to add internal delays based on the same DT bindings. 
You told me to pass the internal delay settings to the PHY if 
"sja1105,mode-mac" is (implicitly or explicitly) specified, or otherwise 
("sja1105,mode-phy"), take the internal delay settings and apply them 
myself (if the switch revision supports this). But in the latter case, 
should I mask off the RGMII_*ID modes and convert everything to RGMII 
when doing the of_phy_connect (to ensure that at the other end nobody 
will add the internal delays twice)? I know the theory but I have never 
actually seen a MAC driver apply the internal delay settings. And even 
in my case I'm a bit surprised that there isn't a more generic mechanism 
to specify this and that I have to rely on custom DT bindings.
* The 2 groups of devices (E, T, P, Q) and (R, S) are pin-compatible 
with one another, and in principle hot-swappable. Initially I had 
explicit "compatible" properties for each device model, but with the 
code for device_id detection, this became a bit redundant so I just made 
everything "nxp,sja1105". But with the different RGMII internal delay 
capabilities, and later on with SGMII support (R, S), some differences 
at the DT level will become apparent. For example my T -> Q swap on the 
LS1021A-TSN board will require a DT change for the internal delay 
settings. How should I handle the "compatible" property for this driver?

> Thanks
>       Andrew

Thank you,
-Vladimir