From: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
To: Maxime Chevallier <maxime.chevallier@bootlin.com>,
Andrew Lunn <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Paolo Abeni <pabeni@redhat.com>, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
Andrew Lunn <andrew@lunn.ch>,
Heiner Kallweit <hkallweit1@gmail.com>,
Russell King <linux@armlinux.org.uk>
Cc: kevin-kw.huang@airoha.com, macpaul.lin@mediatek.com,
matthias.bgg@gmail.com, kernel@collabora.com,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v3 5/6] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver
Date: Wed, 20 May 2026 14:46:22 +0200 [thread overview]
Message-ID: <d61f709524326a4f9943188fcabb5746a1b11e3d.camel@collabora.com> (raw)
In-Reply-To: <e77b4028-c201-4dd4-9214-721bdf67e976@bootlin.com>
Hi Maxime,
On Tue, 2026-05-12 at 12:06 +0200, Maxime Chevallier wrote:
> Hi :)
>
> This looks good, I just have very minimal comments
>
> On 5/12/26 06:33, Louis-Alexis Eyraud wrote:
> > From: AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com>
> >
> > Introduce a driver for the Airoha AN8801R Series Gigabit Ethernet
> > PHY; this currently supports setting up PHY LEDs, 10/100M, 1000M
> > speeds, and Wake on LAN and PHY interrupts.
> >
> > Signed-off-by: AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com>
> > Signed-off-by: Louis-Alexis Eyraud
> > <louisalexis.eyraud@collabora.com>
>
> [...]
>
> > +static u32 an8801r_led_blink_ms_to_hw(unsigned long req_ms)
> > +{
> > + u32 req_ns, regval;
> > +
> > + if (req_ms > AN8801_MAX_PERIOD_MS)
> > + req_ms = AN8801_MAX_PERIOD_MS;
> > +
> > + req_ns = req_ms * 1000000;
>
> Use NSEC_PER_MSEC :)
I'll fix this in the next version.
>
> > +
> > + /* Round to the nearest period unit... */
> > + regval = req_ns + (AN8801_PERIOD_UNIT / 2);
> > +
> > + /* ...and now divide by the full period */
> > + regval >>= AN8801_PERIOD_SHIFT;
> > +
> > + return regval;
> > +}
> > +
>
> [...]
>
> > +static int an8801r_led_hw_control_set(struct phy_device *phydev,
> > u8 index,
> > + unsigned long rules)
> > +{
> > + u16 on = 0, blink = 0;
> > + int ret;
> > +
> > + if (index >= AN8801R_NUM_LEDS)
> > + return -EINVAL;
> > +
> > + ret = an8801r_led_trig_to_hw(rules, &on, &blink);
> > + if (ret)
> > + return ret;
> > +
> > + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_ON_CTRL(index),
> > + LED_ON_EVT_MASK, on);
> > + if (ret)
> > + return ret;
> > +
> > + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_BLINK_CTRL(index),
> > + LED_BLINK_EVT_MASK, blink);
> > +
> > + if (ret)
> > + return ret;
>
> Extra newline before the if()
I'll fix this in the next version too.
>
> > +
> > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> > LED_ON_CTRL(index),
> > + LED_ON_EN, on | blink ? LED_ON_EN :
> > 0);
> > +}
> > +
>
> [...]
>
> > +static int an8801r_rgmii_rxdelay(struct phy_device *phydev, bool
> > enable,
> > + u16 delay_steps)
> > +{
> > + u32 reg_val;
> > +
> > + if (delay_steps > RGMII_DELAY_STEP_MASK)
> > + return -EINVAL;
> > +
> > + if (enable) {
> > + reg_val = delay_steps & RGMII_DELAY_STEP_MASK;
> > +
> > + /* Set align bit to add extra offset for RX delay
> > */
> > + reg_val |= RGMII_RXDELAY_ALIGN;
> > +
> > + /* Set force mode bit to enable RX delay
> > insertion */
> > + reg_val |= RGMII_RXDELAY_FORCE_MODE;
> > + } else {
> > + reg_val = 0;
> > + }
> > +
> > + return an8801_buckpbus_reg_write(phydev,
> > AN8801_BPBUS_REG_RXDLY_STEP,
> > + reg_val);
> > +}
> > +
> > +static int an8801r_rgmii_txdelay(struct phy_device *phydev, bool
> > enable,
> > + u16 delay_steps)
> > +{
> > + u32 reg_val;
> > +
> > + if (delay_steps > RGMII_DELAY_STEP_MASK)
> > + return -EINVAL;
> > +
> > + if (enable) {
> > + reg_val = delay_steps & RGMII_DELAY_STEP_MASK;
>
> Is this bitwise and needed, as you have the check above ?
Indeed, it is not needed, so I'll remove this masking operation here
and in an8801r_rgmii_rxdelay too.
>
> > +
> > + /* Set force mode bit to enable TX delay
> > insertion */
> > + reg_val |= RGMII_TXDELAY_FORCE_MODE;
> > + } else {
> > + reg_val = 0;
> > + }
> > +
> > + return an8801_buckpbus_reg_write(phydev,
> > AN8801_BPBUS_REG_TXDLY_STEP,
> > + reg_val);
> > +}
> > +
> > +static int an8801r_rgmii_delay_config(struct phy_device *phydev)
> > +{
> > + bool enable_delay;
> > + u16 delay_step;
> > + int ret;
> > +
> > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > + enable_delay = true;
> > + delay_step = AN8801_RGMII_TXDELAY_DEFAULT;
> > + } else {
> > + enable_delay = false;
> > + delay_step = RGMII_DELAY_NO_STEP;
> > + }
> > +
> > + ret = an8801r_rgmii_txdelay(phydev, enable_delay,
> > delay_step);
> > + if (ret)
> > + return ret;
> > +
> > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> > + enable_delay = true;
> > + delay_step = AN8801_RGMII_RXDELAY_DEFAULT;
>
> Is it correct that AN8801_RGMII_RXDELAY_DEFAULT expands to
> RGMII_DELAY_NO_STEP ? feels strange, but it may simply be how the HW
> is
> made :)
As I replied in an earlier comment ([1]), for the inserted RX delay,
when the RGMII_RXDELAY_ALIGN bit is set, the "no step" value is then
the closest setting to the 2ns value.
I'll improve the AN8801_RGMII_RXDELAY_DEFAULT comment to say that it
corresponds to the 1.992ns delay value when the align bit is set and -
0.008ns otherwise.
[1]:https://lore.kernel.org/linux-mediatek/199e674cfdbccad104db761964611b1d6352f9f3.camel@collabora.com/
Best regards,
Louis-Alexis
>
> Thanks,
>
> Maxime
next prev parent reply other threads:[~2026-05-20 12:47 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-12 4:33 [PATCH net-next v3 0/6] Introduce Airoha AN8801R series Gigabit Ethernet PHY driver Louis-Alexis Eyraud
2026-05-12 4:33 ` [PATCH net-next v3 1/6] dt-bindings: net: Add support for Airoha AN8801/R GbE PHY Louis-Alexis Eyraud
2026-05-12 4:33 ` [PATCH net-next v3 2/6] net: phy: Add Airoha phy library for shared code Louis-Alexis Eyraud
2026-05-12 12:43 ` Andrew Lunn
2026-05-12 4:33 ` [PATCH net-next v3 3/6] net: phy: air_phy_lib: Factorize BuckPBus register accessors Louis-Alexis Eyraud
2026-05-12 12:45 ` Andrew Lunn
2026-05-12 4:33 ` [PATCH net-next v3 4/6] net: phy: Rename Airoha common " Louis-Alexis Eyraud
2026-05-12 12:45 ` Andrew Lunn
2026-05-12 4:33 ` [PATCH net-next v3 5/6] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver Louis-Alexis Eyraud
2026-05-12 10:06 ` Maxime Chevallier
2026-05-20 12:46 ` Louis-Alexis Eyraud [this message]
2026-05-12 4:33 ` [PATCH net-next v3 6/6] net: phy: air_an8801: ensure maximum available speed link use Louis-Alexis Eyraud
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=d61f709524326a4f9943188fcabb5746a1b11e3d.camel@collabora.com \
--to=louisalexis.eyraud@collabora.com \
--cc=andrew+netdev@lunn.ch \
--cc=andrew@lunn.ch \
--cc=angelogioacchino.delregno@collabora.com \
--cc=conor+dt@kernel.org \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=hkallweit1@gmail.com \
--cc=kernel@collabora.com \
--cc=kevin-kw.huang@airoha.com \
--cc=krzk+dt@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux@armlinux.org.uk \
--cc=macpaul.lin@mediatek.com \
--cc=matthias.bgg@gmail.com \
--cc=maxime.chevallier@bootlin.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=robh@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 read-only IMAP folder(s) and NNTP newsgroup(s).