Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210709164216.18561-1-ms@dev.tdt.de>
In-Reply-To: <20210709164216.18561-1-ms@dev.tdt.de>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Fri, 9 Jul 2021 20:31:59 +0200
Message-ID: <CAFBinCCw9+oCV==1DrNFU6Lu02h3OyZu9wM=78RKGMCZU6ObEA@mail.gmail.com>
Subject: Re: [PATCH net-next v3] net: phy: intel-xway: Add RGMII internal
 delay configuration
To:     Martin Schiller <ms@dev.tdt.de>
Cc:     Hauke Mehrtens <hauke@hauke-m.de>, f.fainelli@gmail.com,
        andrew@lunn.ch, hkallweit1@gmail.com, linux@armlinux.org.uk,
        davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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