Re: [PATCH v1] net: phy: micrel: add phy-mode support for the KSZ9031 PHY

[Philippe Schenker]

On Mon, 2020-04-06 at 15:37 +0200, Oleksij Rempel wrote:
> Hi,
> 
> On Mon, Apr 06, 2020 at 08:58:49AM +0000, Philippe Schenker wrote:
> > On Fri, 2020-04-03 at 10:18 +0200, Oleksij Rempel wrote:
> > > Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
> > > rgmii-rxid.
> > > 
> > > This PHY has an internal RX delay of 1.2ns and no delay for TX.
> > > 
> > > The pad skew registers allow to set the total TX delay to max
> > > 1.38ns
> > > and
> > > the total RX delay to max of 2.58ns (configurable 1.38ns + build
> > > in
> > > 1.2ns) and a minimal delay of 0ns.
> > 
> > This skew delay registers of the KSZ9031 are not meant for this
> > delay.
> 
> According to the documentation of the PHY [1], these registers should
> be
> used to tune the total delay of the circuit.

Yes you're right. I mixed it up with KSZ9131 that has a specific
register for that purpose and skew registers are meant only for PCB-
length adjustment.
> 
> > But I agree that it could make sense to implement phy-modes too for
> > this
> > PHY. I even thought myself about implementing it.
> > But I guess it is not a good thing to be able to set the same
> > registers in a chip in two different places in a DT. How is this
> > solved generally in linux?
> 
> In this case i would prefer to talk about several device tree
> properties
> describing the same thing. The phy-mode property will set a more
> generic
> defaults where the *-skew-ps properties cane be used to overwrite
> single
> or all pads.

I'm just afraid that one will read the documentation, put rgmii-id in
there and also try to adjust PCB-trace-length in skew registers...
That's why I'd at least throw a warning or error.
> 
> The current situation is:
> - we have a RGMII-RXID PHY (with internal not optional delay of 1.2ns)
> - which is configured in many (all?) devicetries as phy-mode="rgmii",
> not
>   "rgmii-rxid".
> 
> There are boards:
> - with default options. No extra delays are configured, so actually
> they
>   want to have a "rgmii-rxid", but configure as phy-mode="rgmii" 
> - configured by fixup (for example in i'MX6Q:  RGMII-ID, but in DT
>   configuration with phy-mode=rgmii)
> 
> All of this configurations are broken. This one is correct:
> 
> - configured by *-skew-ps property and using phy-mode=rgmii.

I agree.
> 
> > Another reasoning is that this will *only* work, if the PCB traces
> > are
> > length- matched. This leads me to the conclusion that throwing an
> > error so the PHY doesn't work if someone added e.g. 'rgmii-id' and
> > skew registers is a good thing.
> 
> So you mean, skew setting should only work with phy-mode="rgmii", and
> throw an error otherwise? Makes sense to me.

Yes, that's exactly my intention. But when I think about it a warning is
more appropriate than an error, but I guess that's up to the maintainer
to decide.
> 
> > But with that we would maybe break some boards... At least I would
> > throw
> > a warning in ksz9031_of_load_skew_values.
> > 
> > > According to the RGMII v2 specification the delay provided by PCB
> > > traces
> > 
> > As I understood, RGMII v1.3 demands delay by PCB traces (that is for
> > embedded mostly not possible). Whereas RGMII v2.0 demands de MAC to
> > add
> > the delay for TXC and the PHY for RXC.
> > 
> > I know its nitpicky but still can be confusing for someone trying to
> > understand that. Could you adjust that here?
> > 
> > > should be between 1.5ns and 2.0ns. As this PHY can provide max
> > > delay
> > > of
> > > only 1.38ns on the TX line, in RGMII-ID mode a symmetric delay of
> > > 1.38ns
> > > for both the RX and TX lines is chosen, even if the RX line could
> > > be
> > > configured with the 1.5ns according to the standard.
> > 
> > Why do you decided for a symmetric delay? I guess the hardware level
> > doesn't care if the input-stages of two different silicons don't
> > care if
> > the delay is symmetrical. I suggest to use a delay for RXC to get
> > the
> > RXC clock edge in the middle of the data lines.
> 
> Are there any technical justification to use both 1.38 or one 1.38 and
> other 2.0?

Yes, we should try to achieve the RGMII specs where possible. So I'd
delay RXC more than TXC.
> 
> Our HW expert suggest to use the middle of the RGMII recommended
> delay:
> 1.75ns. What is your opinion? So far ksz9031 provide not configurable
> 1.2ns and
> ksu9131 use 2.0ns (DLL based) delay. It looks like the "internal
> delay"
> interpretation has some valid range of numbers.

I would not only try to hit the middle from the specification but the
middle of the actual signal. With that I mean to have T_setup and T_hold
times about the same. Please see an RGMII timing diagram for the meaning
of those names[1].

I calculated the optimum delay in a worst case scenario. I took maximum
deviation of the clock (7.2ns - 8.8ns) and also skew of the MAC
(according to RGMII spec -500ps to 500ps). With my theory of hitting the
middle of the clock edges, this results in min/max delay times of an
added delay to RXC of 600ps (1.8ns in total) and for TXC we're stuck
with 1.38ns which is fine in most cases, but an optimal value here would
also be in the range your HW expert suggests. I'm glad we're resulting
in about the same values!

Ultimately I would set the registers in 'rgmii-
id' to:
MMD Register 2.4	0x70
MMD Register 2.5	0x7777
MMD Register 2.6	0x0
MMD Register 2.8	0x3F9

I'll send you these values for cross-check
validation.

Regards,
Philippe

[1] https://www.ti.com/lit/an/snla243/snla243.pdf