Re: [PATCH] net: macb: reject unsupported rgmii delays

[Helmut Grohne <helmut.grohne@intenta.de>]

Hi Vladimir,

On Wed, Jun 17, 2020 at 11:57:23AM +0200, Vladimir Oltean wrote:
> On Tue, 16 Jun 2020 at 11:00, Helmut Grohne <helmut.grohne@intenta.de> wrote:
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -514,7 +514,7 @@ static void macb_validate(struct phylink_config *config,
> >             state->interface != PHY_INTERFACE_MODE_RMII &&
> >             state->interface != PHY_INTERFACE_MODE_GMII &&
> >             state->interface != PHY_INTERFACE_MODE_SGMII &&
> > -           !phy_interface_mode_is_rgmii(state->interface)) {
> > +           state->interface != PHY_INTERFACE_MODE_RGMII_ID) {
> 
> I don't think this change is correct though?
> What if there were PCB traces in place, for whatever reason? Then the
> driver would need to accept a phy with rgmii-txid, rgmii-rxid or
> rgmii.

I must confess that after rereading the discussion and the other
discussions at
https://patchwork.ozlabs.org/project/netdev/patch/20190410005700.31582-19-olteanv@gmail.com/
and
https://patchwork.ozlabs.org/project/netdev/patch/20190413012822.30931-21-olteanv@gmail.com/,
this is less and less clear to me.

I think we can agree that the current definition of phy-mode is
confusing when it comes to rgmii delays. The documentation doesn't even
mention the possibility of using serpentines.

Your interpretation seems to be that this value is solely meant for the
PHY to operate on and that the MAC should not act upon it (at least the
delay part). That interpetation is consistent with previous discussions
and mostly consistent with the documentation. The phy-mode property is
documented in ethernet-controller.yaml, which suggests that it should
apply to the MAC somehow.

Following this interpretation, I think it would be good to also document
it in ethernet-phy.yaml.

Thank you for the review. I agree that the hunk should be dropped.

However, in the fixed-link case (below) the interpretation regarding
serpentines seems to be well-defined: If serpentines are present, both
MACs should specify rgmii.

> >                 bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> >                 return;
> >         }
> > @@ -694,6 +694,13 @@ static int macb_phylink_connect(struct macb *bp)
> >         struct phy_device *phydev;
> >         int ret;
> >
> > +       if (of_phy_is_fixed_link(dn) &&
> > +           phy_interface_mode_is_rgmii(bp->phy_interface) &&
> > +           bp->phy_interface != PHY_INTERFACE_MODE_RGMII) {
> > +               netdev_err(dev, "RGMII delays are not supported\n");
> > +               return -EINVAL;
> > +       }
> > +
> 
> Have you checked that this doesn't break any existing in-tree users?

It seems like all the in-tree users of this driver that do specify a
phy-mode, specify rmii.

If possible breakage is to be minimized, I'd be happy with using
netdev_warn instead. The major benefit here is getting a clear
indication of why things don't work. My emphasis is on getting something
to dmesg, not to make things fail.

So what should we prefer here? Failure or warning?

In the long run, it would be nice to refactor the way to denote delays
in a way that is consistently defined for MAC to PHY and MAC to MAC
connections while accounting for serpentines.

Helmut