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

[Martin Schiller <ms@dev.tdt.de>]

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