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!
