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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fcb3203ea82d1180a6e471f22e39e817@dev.tdt.de>
Date:   Mon, 12 Jul 2021 08:13:48 +0200
From:   Martin Schiller <ms@....tdt.de>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     Hauke Mehrtens <hauke@...ke-m.de>, f.fainelli@...il.com,
        andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
        davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
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@....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