Re: [PATCH net v3] net: phy: intel-xway: enable integrated led functions

[Tim Harvey]

On Thu, Feb 3, 2022 at 4:04 PM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> > Andrew,
> >
> > The issue is that in an ideal world where all parts adhere to the
> > JEDEC standards 2ns would be correct but that is not reality. In my
> > experience with the small embedded boards I help design and support
> > not about trace lengths as it would take inches to skew 0.5ns but
> > instead differences in setup/hold values for various MAC/PHY parts
> > that are likely not adhering the standards.
> >
> > Some examples from current boards I support:
> > - CN8030 MAC rgmii-rxid with intel-xway GPY111 PHY: we need to
> > configure this for rx_delay=1.5ns and tx_delay=0.5ns
>
> So i assume this is what broke for you. Your bootloader sets these
> delays, phy-type is rgmii-id, and since you don't have the properties
> for what exact delays you want it default to what 802.3 specifies,
> 2ns.
>
> I actually think the current behaviour of the driver is correct.  By
> saying phy-type = rgmii-id you are telling the PHY driver to insert
> the delays and that is what it is doing.
>
> In retrospect, i would say you had two choices to cleanly solve this.
>
> 1) Do exactly what the patch does, add properties to define what
> actual delay you want, when you don't want 2ns.
>
> 2) Have the bootloader set the delay, but also tell Linux you have set
> the phy mode including the delays, and it should not touch them. There
> is a well defined way to do this:
>
> PHY_INTERFACE_MODE_NA
>
> It has two main use cases. The first is that the MAC and PHY are
> integrated, there is no MII between them, phy-mode is completely
> unnecessary. This is the primary meaning.
>
> The second meaning is, something else has setup the phy mode, e.g. the
> bootloader. Don't touch it.
>
> This second meaning does not always work, since the driver might reset
> the PHY, and depending on the PHY, that might clear whatever the
> bootloader set. So it is not reliable.

Andrew,

I appreciate your suggestions.

The PHY_INTERRFACE_MODE_NA is a neat trick that I will remember but it
would only help with the rgmii delay issue and not the LED issue (this
patch). The GPY111 has some really nasty errata that is going to cause
me to have a very hackish work-around anyway and I will be disabling
the PHY driver to stay out of the way of that workaround so in this
case here I'm not looking for a solution.

>
> > - CN8030 MAC rgmii-rxid with dp83867 GPY111 PHY: we need to configure
> > this for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected)
> > - IMX8MM FEC MAC rgmii-id with intel-xway PHY: we need to configure
> > this for rx_delay=2ns and tx_delay=2.5ns
> > - IMX8MM FEC MAC rgmii-id with dp83867 PHY: we need to configure this
> > for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected)
> >
> > The two boards above have identical well matched trace-lengths between
> > the two PHY variant designs so you can see here that the intel-xway
> > GPY111 PHY needs an extra 0.5ps delay to avoid RX errors on the
> > far-off board.
>
> So a couple of ideas.
>
> Since GPY111 and dp83867 use different properties for the delays, just
> put both in DT. The PHY driver will look for the property it
> understands and ignore the other. So you can have different delays for
> different PHYs.

Yes, this is what I am currently doing but I'm not sure that would
ever be accepted as a dt change upstream and at some point when the
common property for the delays is supported in both PHY drivers that
will not work (as you point out below). I do a lot of dt fixups in
boot firmware already to deal with board revision's so I'll probably
deal with it that way at some point.

I believe I have made a good case why defaulting to 2ns delays does
not work for all RGMII MAC/PHY scenarios. If 2ns was all you needed
then all these PHY's would not have adjustable delays (its not just
about trace-lengths as they have to be off more than an insh for 0.5ps
and there are a ton of tiny embedded boards out there with delays
between 0.5 and 2.5ns).

As far as changing a driver to force a LED configuration with no dt
binding input (like this patch does) it feels wrong for exactly the
same reason - LED configuration for this PHY can be done via
pin-strapping and this driver now undoes that with this patch. I still
feel very strongly that a PHY driver should not change configuration
which may be based on pin-strapping or boot firmware changes when no
dt properties exist telling it to make that specific change. However
if my opinion is not the popular one I will stop bringing it up.

Best regards,

Tim


>
> We have started to standardize on the delay property. And new driver
> should be using rx-internal-delay-ps and tx-internal-delay-ps. If you
> have two drivers using the same property name, what i just suggested
> will not work. Can you control the address the PHY uses? Put the
> GPY111 on a different address to the dp83867. List them both in DT. If
> you don't have a phy-handle in DT, the FEC will go hunting on its MDIO
> bus and use the first PHY it finds. You can put the needed delay
> properties in DT for the specific PHY.
>
>         Andrew
>