Messages in this thread | | | Date | Fri, 9 Jul 2021 13:26:58 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next] net: phy: intel-xway: Add RGMII internal delay configuration |
| |
On Fri, Jul 09, 2021 at 01:57:26PM +0200, Martin Schiller wrote: > +static int xway_gphy_of_reg_init(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + int delay_size = ARRAY_SIZE(xway_internal_delay); > + s32 rx_int_delay; > + s32 tx_int_delay; > + int err = 0; > + int val; > + > + if (phy_interface_is_rgmii(phydev)) { > + val = phy_read(phydev, XWAY_MDIO_MIICTRL); > + if (val < 0) > + return val; > + } > + > + /* Existing behavior was to use default pin strapping delay in rgmii > + * mode, but rgmii should have meant no delay. Warn existing users. > + */ > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { > + const u16 txskew = (val & XWAY_MDIO_MIICTRL_TXSKEW_MASK) >> > + XWAY_MDIO_MIICTRL_TXSKEW_SHIFT; > + const u16 rxskew = (val & XWAY_MDIO_MIICTRL_RXSKEW_MASK) >> > + XWAY_MDIO_MIICTRL_RXSKEW_SHIFT; > + > + if (txskew > 0 || rxskew > 0) > + phydev_warn(phydev, > + "PHY has delays (e.g. via pin strapping), but phy-mode = 'rgmii'\n" > + "Should be 'rgmii-id' to use internal delays txskew:%x rxskew:%x\n", > + txskew, rxskew); > + } > + > + /* RX delay *must* be specified if internal delay of RX is used. */ > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > + rx_int_delay = phy_get_internal_delay(phydev, dev, > + &xway_internal_delay[0], > + delay_size, true); > + > + if (rx_int_delay < 0) { > + phydev_err(phydev, "rx-internal-delay-ps must be specified\n"); > + return rx_int_delay; > + } > + > + val &= ~XWAY_MDIO_MIICTRL_RXSKEW_MASK; > + val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT; > + } > + > + /* TX delay *must* be specified if internal delay of TX is used. */ > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + tx_int_delay = phy_get_internal_delay(phydev, dev, > + &xway_internal_delay[0], > + delay_size, false); > + > + if (tx_int_delay < 0) { > + phydev_err(phydev, "tx-internal-delay-ps must be specified\n"); > + return tx_int_delay; > + } > + > + val &= ~XWAY_MDIO_MIICTRL_TXSKEW_MASK; > + val |= tx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT; > + } > + > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) > + err = phy_write(phydev, XWAY_MDIO_MIICTRL, val); > + > + return err; > +}
Please reconsider the above. Maybe something like the following would be better:
u16 mask = 0; int val = 0;
if (!phy_interface_is_rgmii(phydev)) return;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { u16 txskew, rxskew;
val = phy_read(phydev, XWAY_MDIO_MIICTRL); if (val < 0) return val;
txskew = (val & XWAY_MDIO_MIICTRL_TXSKEW_MASK) >> XWAY_MDIO_MIICTRL_TXSKEW_SHIFT; rxskew = (val & XWAY_MDIO_MIICTRL_RXSKEW_MASK) >> XWAY_MDIO_MIICTRL_RXSKEW_SHIFT;
if (txskew > 0 || rxskew > 0) phydev_warn(phydev, "PHY has delays (e.g. via pin strapping), but phy-mode = 'rgmii'\n" "Should be 'rgmii-id' to use internal delays txskew:%x rxskew:%x\n", txskew, rxskew); return; }
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { ... mask |= XWAY_MDIO_MIICTRL_RXSKEW_MASK; val |= rx_int_delay << XWAY_MDIO_MIICTRL_RXSKEW_SHIFT; }
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { ... mask |= XWAY_MDIO_MIICTRL_TXSKEW_MASK; val |= rx_int_delay << XWAY_MDIO_MIICTRL_TXSKEW_SHIFT; }
return phy_modify(phydev, XWAY_MDIO_MIICTRL, mask, val);
Using phy_modify() has the advantage that the read-modify-write is done as a locked transaction on the bus, meaning that it is atomic. There isn't a high cost to writing functions in a way that makes use of that as can be seen from the above.
Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |