Re: [PATCH net-next v3] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

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@xxxxxxxxxx> 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