Messages in this thread | | | Date | Mon, 12 Jul 2021 08:13:48 +0200 | From | Martin Schiller <> | Subject | Re: [PATCH net-next v3] net: phy: intel-xway: Add RGMII internal delay configuration |
| |
On 2021-07-09 20:31, Martin Blumenstingl wrote: > 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 >
Thanks. I'll fix that.
> [...] >> +#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 >
Thanks for the hint. I'll switch to the FIELD_... macros.
> [...] >> +#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) >
It makes no sense to retrieve properties from the device tree if we are compiling for a target that does not support a device tree. At least that is my understanding of this condition.
> [...] >> +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". >
OK, I'll change that.
>> + 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). >
I'll switch to use one "int_delay".
>> + 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). >
Yes, you are right. I'll change that as suggested.
> [...] >> + 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,
I've copied that from dp83869.c, but yes, I can change it.
> > [...] >> + 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 >
dito.
> > Best regards, > Martin > > > [0] > https://elixir.bootlin.com/linux/v5.13/source/drivers/net/phy/dp83867.c#L438
| |