From: Martin Schiller <ms@dev.tdt.de>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
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
Subject: Re: [PATCH net-next v3] net: phy: intel-xway: Add RGMII internal delay configuration
Date: Mon, 12 Jul 2021 08:13:48 +0200 [thread overview]
Message-ID: <fcb3203ea82d1180a6e471f22e39e817@dev.tdt.de> (raw)
In-Reply-To: <CAFBinCCw9+oCV==1DrNFU6Lu02h3OyZu9wM=78RKGMCZU6ObEA@mail.gmail.com>
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
next prev parent reply other threads:[~2021-07-12 6:14 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-09 16:42 [PATCH net-next v3] net: phy: intel-xway: Add RGMII internal delay configuration Martin Schiller
2021-07-09 18:31 ` Martin Blumenstingl
2021-07-12 6:13 ` Martin Schiller [this message]
2021-07-13 13:06 ` Andrew Lunn
2021-07-17 16:38 ` Hauke Mehrtens
2021-07-19 8:14 ` Martin Schiller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fcb3203ea82d1180a6e471f22e39e817@dev.tdt.de \
--to=ms@dev.tdt.de \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=hauke@hauke-m.de \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=martin.blumenstingl@googlemail.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).