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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Jun 16, 2020 at 09:49:56AM +0200, Helmut Grohne wrote:
> The macb driver does not support configuring rgmii delays. At least for
> the Zynq GEM, delays are not supported by the hardware at all. However,
> the driver happily accepts and ignores any such delays.
> 
> When operating in a mac to phy connection, the delay setting applies to
> the phy. Since the MAC does not support delays, the phy must provide
> them and the only supported mode is rgmii-id.  However, in a fixed mac
> to mac connection, the delay applies to the mac itself. Therefore the
> only supported rgmii mode is rgmii.

This seems incorrect - see the phy documentation in
Documentation/networking/phy.rst:

* PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
  internal delay by itself, it assumes that either the Ethernet MAC (if capable
  or the PCB traces) insert the correct 1.5-2ns delay

* PHY_INTERFACE_MODE_RGMII_TXID: the PHY should insert an internal delay
  for the transmit data lines (TXD[3:0]) processed by the PHY device

* PHY_INTERFACE_MODE_RGMII_RXID: the PHY should insert an internal delay
  for the receive data lines (RXD[3:0]) processed by the PHY device

* PHY_INTERFACE_MODE_RGMII_ID: the PHY should insert internal delays for
  both transmit AND receive data lines from/to the PHY device

Note that PHY_INTERFACE_MODE_RGMII, the delay can be added by _either_
the MAC or by PCB trace routing.

The individual RGMII delay modes are more about what the PHY itself is
asked to do with respect to inserting delays, so I don't think your
patch makes sense.

In any case...

> Link: https://lore.kernel.org/netdev/20200610081236.GA31659@laureti-dev/
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5b9d7c60eebc..bee5bf65e8b3 100644
> --- 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) {

Here you reject everything except PHY_INTERFACE_MODE_RGMII_ID.

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

but here you reject everything except PHY_INTERFACE_MODE_RGMII.  These
can't both be right.  If you start with PHY_INTERFACE_MODE_RGMII, and
have a fixed link, you'll have PHY_INTERFACE_MODE_RGMII passed into
the validate function, which will then fail.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!