Messages in this thread | | | From | Martin Blumenstingl <> | Date | Fri, 9 Jul 2021 20:31:59 +0200 | Subject | Re: [PATCH net-next v3] net: phy: intel-xway: Add RGMII internal delay configuration |
| |
Hi Martin,
overall this is looking good. A few comments below - I think none of them is a "must change" in my opinion.
On Fri, Jul 9, 2021 at 6:42 PM Martin Schiller <ms@dev.tdt.de> wrote: > > This adds the posibility to configure the RGMII RX/TX clock skew via typo: posibility -> possibility
[...] > +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK GENMASK(14, 12) > +#define XWAY_MDIO_MIICTRL_RXSKEW_SHIFT 12 if you use - FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, rxskew); (as for example [0] does) - and FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val); below then you can drop the _SHIFT #define this is purely cosmetic though, so nothing which blocks this from being merged
> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK GENMASK(10, 8) > +#define XWAY_MDIO_MIICTRL_TXSKEW_SHIFT 8 same as above
[...] > +#if IS_ENABLED(CONFIG_OF_MDIO) is there any particular reason why we need to guard this with CONFIG_OF_MDIO? The dp83822 driver does not use this #if either (as far as I understand at least)
[...] > +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); Some people in the kernel community are working on automatically detecting and fixing signedness issues. I am not sure if they would find this at some point suggesting that it can be an "unsigned int".
> + s32 rx_int_delay; > + s32 tx_int_delay; xway_gphy14_config_aneg() below defines two variables in one line, so to be consistent this would be: s32 rx_int_delay, tx_int_delay; another option is to just re-use one "int_delay" variable (as it seems that they're both used in different code-paths).
> + u16 mask = 0; I think this should be dropped and the phy_modify() call below should read: return phy_modify(phydev, XWAY_MDIO_MIICTRL, XWAY_MDIO_MIICTRL_RXSKEW_MASK | XWAY_MDIO_MIICTRL_TXSKEW_MASK, val); For rgmii-txid the RX delay might be provided by the MAC or PCB trace length so the PHY should not add any RX delay. Similarly for rgmii-rxid the TX delay might be provided by the MAC or PCB trace length so the PHY should not add any TX delay. That means we always need to mask the RX and TX skew bits, regardless of what we're setting later on (as phy_modify is only called for one of: rgmii-id, rgmii-txid, rgmii-rxid).
[...] > + 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], I think above line can be simplified as: xway_internal_delay,
[...] > + 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], same as above
Best regards, Martin
[0] https://elixir.bootlin.com/linux/v5.13/source/drivers/net/phy/dp83867.c#L438
| |