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

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

On Thu, Jun 18, 2020 at 11:05:26AM +0200, Helmut Grohne wrote:
> On Thu, Jun 18, 2020 at 10:45:54AM +0200, Russell King - ARM Linux admin wrote:
> > Why do we need that complexity?  If we decide that we can allow
> > phy-mode = "rgmii" and introduce new properties to control the
> > delay, then we just need:
> > 
> >   rgmii-tx-delay-ps = <nnn>;
> >   rgmii-rx-delay-ps = <nnn>;
> > 
> > specified in the MAC node (to be applied only at the MAC end) or
> > specified in the PHY node (to be applied only at the PHY end.)
> > In the normal case, this would be the standard delay value, but
> > in exceptional cases where supported, the delays can be arbitary.
> > We know there are PHYs out there which allow other delays.
> > 
> > This means each end is responsible for parsing these properties in
> > its own node and applying them - or raising an error if they can't
> > be supported.
> 
> Thank you. That makes a lot more sense while keeping the (imo) important
> properties of my proposal:
>  * It is backwards compatible. These properties override delays
>    specified inside phy-mode. Otherwise the vague phy-mode meaning is
>    retained.
>  * The ambiguity is resolved. It is always clear where delays should be
>    configure and the properties properly account for possible PCB
>    traces.
> 
> It also resolves my original problem. If support for these properties is
> added to macb_main.c, it would simply check that both delays are 0 as
> internal delays are not supported by the hardware.  When I would have
> attempted to configure an rx delay, it would have nicely errored out.

I think we'd want a helper or two to do the parsing and return the
delays, something like:

#define PHY_RGMII_DELAY_PS_NONE	0
#define PHY_RGMII_DELAY_PS_STD	1500

/* @np here should be the MAC node */
int of_mac_get_delays(struct device_node *np,
		      phy_interface_mode interface,
		      u32 *tx_delay_ps, u32 *rx_delay_ps)
{
	*tx_delay_ps = PHY_RGMII_DELAY_PS_NONE;
	*rx_delay_ps = PHY_RGMII_DELAY_PS_NONE;

	if (!np)
		return 0;

	if (interface == PHY_INTERFACE_MODE_RGMII) {
		of_property_read_u32(np, "rgmii-tx-delay-ps", tx_delay_ps);
		of_property_read_u32(np, "rgmii-rx-delay-ps", rx_delay_ps);
	}

	return 0;
}

/* @np here should be the PHY node */
int of_phy_get_delays(struct device_node *np,
		      phy_interface_mode interface,
		      u32 *tx_delay_ps, u32 *rx_delay_ps)
{
	switch (interface) {
	case PHY_INTERFACE_MODE_RGMII_ID:
		*tx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		*rx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		return 0;

	case PHY_INTERFACE_MODE_RGMII_RXID:
		*tx_delay_ps = PHY_RGMII_DELAY_PS_NONE;
		*rx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		return 0;

	case PHY_INTERFACE_MODE_RGMII_TXID:
		*tx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		*rx_delay_ps = PHY_RGMII_DELAY_PS_NONE;
		return 0;

	default:
		return of_mac_get_delays(np, interface,
					 tx_delay_ps, rx_delay_ps);
	}
}

as a first cut - validation left up to the user of these.  At least
we then have an easy interface for PHY drivers to use, for example:

static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
{
	u32 tx_delay_ps, rx_delay_ps;
	int err;

	err = of_phy_get_delays(phydev->mdio.dev.of_node,
				phydev->interface, &tx_delay_ps,
				&rx_delay_ps);
	if (err)
		return err;

	mscr = 0;
	if (tx_delay_ps == PHY_RGMII_DELAY_PS_STD)
		mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;
	else if (tx_delay_ps != PHY_RGMII_DELAY_PS_NONE)
		/* ... log an error to kernel log */
		return -EINVAL;

	if (rx_delay_ps == PHY_RGMII_DELAY_PS_STD)
		mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
	else if (rx_delay_ps != PHY_RGMII_DELAY_PS_NONE)
		/* ... log an error to kernel log */
		return -EINVAL;

	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
				MII_88E1121_PHY_MSCR_REG,
				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
}

> How can we achieve wider consensus on this and put it into the dt
> specification? Should there be drivers supporting these first?

Provide an illustration of the idea in code form for consideration? ;)

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