* [PATCH v2] net: phy: dp83822: Fix RGMII TX delay configuration
@ 2024-02-04 10:11 Tim Pambor
2024-02-08 10:12 ` Paolo Abeni
0 siblings, 1 reply; 3+ messages in thread
From: Tim Pambor @ 2024-02-04 10:11 UTC (permalink / raw)
To: Andrew Lunn
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Dan Murphy, netdev, linux-kernel,
Tim Pambor
The logic for enabling the TX clock shift is inverse of enabling the RX
clock shift. The TX clock shift is disabled when DP83822_TX_CLK_SHIFT is
set. Correct the current behavior and always write the delay configuration
to ensure consistent delay settings regardless of bootloader configuration.
Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf p. 69
Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed internal delay")
Signed-off-by: Tim Pambor <tp@osasysteme.de>
---
Changes in v2:
- Further cleanup of RGMII configuration
- Check for errors setting DP83822_RGMII_MODE_EN
---
drivers/net/phy/dp83822.c | 41 +++++++++++++--------------------------
1 file changed, 14 insertions(+), 27 deletions(-)
diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index b7cb71817780..1b2c34a97396 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -380,42 +380,29 @@ static int dp83822_config_init(struct phy_device *phydev)
{
struct dp83822_private *dp83822 = phydev->priv;
struct device *dev = &phydev->mdio.dev;
- int rgmii_delay;
- s32 rx_int_delay;
- s32 tx_int_delay;
+ int rcsr_mask = DP83822_RGMII_MODE_EN;
+ int rcsr = 0;
int err = 0;
int bmcr;
if (phy_interface_is_rgmii(phydev)) {
- rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
- true);
+ rcsr |= DP83822_RGMII_MODE_EN;
- if (rx_int_delay <= 0)
- rgmii_delay = 0;
- else
- rgmii_delay = DP83822_RX_CLK_SHIFT;
+ /* Set DP83822_RX_CLK_SHIFT to enable rx clk internal delay */
+ if (phy_get_internal_delay(phydev, dev, NULL, 0, true) > 0)
+ rcsr |= DP83822_RX_CLK_SHIFT;
- tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
- false);
- if (tx_int_delay <= 0)
- rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
- else
- rgmii_delay |= DP83822_TX_CLK_SHIFT;
+ /* Set DP83822_TX_CLK_SHIFT to disable tx clk internal delay */
+ if (phy_get_internal_delay(phydev, dev, NULL, 0, false) <= 0)
+ rcsr |= DP83822_TX_CLK_SHIFT;
- if (rgmii_delay) {
- err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
- MII_DP83822_RCSR, rgmii_delay);
- if (err)
- return err;
- }
-
- phy_set_bits_mmd(phydev, DP83822_DEVADDR,
- MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
- } else {
- phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
- MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
+ rcsr_mask |= DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT;
}
+ err = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RCSR, rcsr_mask, rcsr);
+ if (err < 0)
+ return err;
+
if (dp83822->fx_enabled) {
err = phy_modify(phydev, MII_DP83822_CTRL_2,
DP83822_FX_ENABLE, 1);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] net: phy: dp83822: Fix RGMII TX delay configuration
2024-02-04 10:11 [PATCH v2] net: phy: dp83822: Fix RGMII TX delay configuration Tim Pambor
@ 2024-02-08 10:12 ` Paolo Abeni
2024-02-13 13:07 ` Tim Pambor
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Abeni @ 2024-02-08 10:12 UTC (permalink / raw)
To: Tim Pambor, Andrew Lunn
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Dan Murphy, netdev, linux-kernel
On Sun, 2024-02-04 at 11:11 +0100, Tim Pambor wrote:
> The logic for enabling the TX clock shift is inverse of enabling the RX
> clock shift. The TX clock shift is disabled when DP83822_TX_CLK_SHIFT is
> set. Correct the current behavior and always write the delay configuration
> to ensure consistent delay settings regardless of bootloader configuration.
>
> Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf p. 69
>
> Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed internal delay")
> Signed-off-by: Tim Pambor <tp@osasysteme.de>
> ---
> Changes in v2:
> - Further cleanup of RGMII configuration
This overall makes the change more invasive, I'm unsure is in the
direction pointed by Russell
> - Check for errors setting DP83822_RGMII_MODE_EN
> ---
> drivers/net/phy/dp83822.c | 41 +++++++++++++--------------------------
> 1 file changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> index b7cb71817780..1b2c34a97396 100644
> --- a/drivers/net/phy/dp83822.c
> +++ b/drivers/net/phy/dp83822.c
> @@ -380,42 +380,29 @@ static int dp83822_config_init(struct phy_device *phydev)
> {
> struct dp83822_private *dp83822 = phydev->priv;
> struct device *dev = &phydev->mdio.dev;
> - int rgmii_delay;
> - s32 rx_int_delay;
> - s32 tx_int_delay;
> + int rcsr_mask = DP83822_RGMII_MODE_EN;
> + int rcsr = 0;
> int err = 0;
> int bmcr;
>
> if (phy_interface_is_rgmii(phydev)) {
> - rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
> - true);
> + rcsr |= DP83822_RGMII_MODE_EN;
>
> - if (rx_int_delay <= 0)
> - rgmii_delay = 0;
> - else
> - rgmii_delay = DP83822_RX_CLK_SHIFT;
> + /* Set DP83822_RX_CLK_SHIFT to enable rx clk internal delay */
> + if (phy_get_internal_delay(phydev, dev, NULL, 0, true) > 0)
> + rcsr |= DP83822_RX_CLK_SHIFT;
>
> - tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
> - false);
> - if (tx_int_delay <= 0)
> - rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
> - else
> - rgmii_delay |= DP83822_TX_CLK_SHIFT;
> + /* Set DP83822_TX_CLK_SHIFT to disable tx clk internal delay */
> + if (phy_get_internal_delay(phydev, dev, NULL, 0, false) <= 0)
> + rcsr |= DP83822_TX_CLK_SHIFT;
>
> - if (rgmii_delay) {
> - err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> - MII_DP83822_RCSR, rgmii_delay);
> - if (err)
> - return err;
> - }
> -
> - phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> - MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
> - } else {
> - phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
> - MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
> + rcsr_mask |= DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT;
It looks like there is a change of behavior here. The current code
clear the DP83822_RGMII_MODE_EN, while this patch will clear
DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT, leaving
DP83822_RGMII_MODE_EN unmodified.
It does not look correct to me.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] net: phy: dp83822: Fix RGMII TX delay configuration
2024-02-08 10:12 ` Paolo Abeni
@ 2024-02-13 13:07 ` Tim Pambor
0 siblings, 0 replies; 3+ messages in thread
From: Tim Pambor @ 2024-02-13 13:07 UTC (permalink / raw)
To: Paolo Abeni
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Dan Murphy, netdev, linux-kernel
Hi Paolo,
On Thu, 2024-02-08 at 11:12 +0100, Paolo Abeni wrote:
> On Sun, 2024-02-04 at 11:11 +0100, Tim Pambor wrote:
> > The logic for enabling the TX clock shift is inverse of enabling
> > the RX
> > clock shift. The TX clock shift is disabled when
> > DP83822_TX_CLK_SHIFT is
> > set. Correct the current behavior and always write the delay
> > configuration
> > to ensure consistent delay settings regardless of bootloader
> > configuration.
> >
> > Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf p. 69
> >
> > Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed
> > internal delay")
> > Signed-off-by: Tim Pambor <tp@osasysteme.de>
> > ---
> > Changes in v2:
> > - Further cleanup of RGMII configuration
>
> This overall makes the change more invasive, I'm unsure is in the
> direction pointed by Russell
>
> > - Check for errors setting DP83822_RGMII_MODE_EN
> > ---
> > drivers/net/phy/dp83822.c | 41 +++++++++++++----------------------
> > ----
> > 1 file changed, 14 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> > index b7cb71817780..1b2c34a97396 100644
> > --- a/drivers/net/phy/dp83822.c
> > +++ b/drivers/net/phy/dp83822.c
> > @@ -380,42 +380,29 @@ static int dp83822_config_init(struct
> > phy_device *phydev)
> > {
> > struct dp83822_private *dp83822 = phydev->priv;
> > struct device *dev = &phydev->mdio.dev;
> > - int rgmii_delay;
> > - s32 rx_int_delay;
> > - s32 tx_int_delay;
> > + int rcsr_mask = DP83822_RGMII_MODE_EN;
> > + int rcsr = 0;
> > int err = 0;
> > int bmcr;
> >
> > if (phy_interface_is_rgmii(phydev)) {
> > - rx_int_delay = phy_get_internal_delay(phydev, dev,
> > NULL, 0,
> > - true);
> > + rcsr |= DP83822_RGMII_MODE_EN;
> >
> > - if (rx_int_delay <= 0)
> > - rgmii_delay = 0;
> > - else
> > - rgmii_delay = DP83822_RX_CLK_SHIFT;
> > + /* Set DP83822_RX_CLK_SHIFT to enable rx clk
> > internal delay */
> > + if (phy_get_internal_delay(phydev, dev, NULL, 0,
> > true) > 0)
> > + rcsr |= DP83822_RX_CLK_SHIFT;
> >
> > - tx_int_delay = phy_get_internal_delay(phydev, dev,
> > NULL, 0,
> > - false);
> > - if (tx_int_delay <= 0)
> > - rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
> > - else
> > - rgmii_delay |= DP83822_TX_CLK_SHIFT;
> > + /* Set DP83822_TX_CLK_SHIFT to disable tx clk
> > internal delay */
> > + if (phy_get_internal_delay(phydev, dev, NULL, 0,
> > false) <= 0)
> > + rcsr |= DP83822_TX_CLK_SHIFT;
> >
> > - if (rgmii_delay) {
> > - err = phy_set_bits_mmd(phydev,
> > DP83822_DEVADDR,
> > - MII_DP83822_RCSR,
> > rgmii_delay);
> > - if (err)
> > - return err;
> > - }
> > -
> > - phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> > - MII_DP83822_RCSR,
> > DP83822_RGMII_MODE_EN);
> > - } else {
> > - phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
> > - MII_DP83822_RCSR,
> > DP83822_RGMII_MODE_EN);
> > + rcsr_mask |= DP83822_RX_CLK_SHIFT |
> > DP83822_TX_CLK_SHIFT;
>
> It looks like there is a change of behavior here. The current code
> clear the DP83822_RGMII_MODE_EN, while this patch will clear
> DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT, leaving
> DP83822_RGMII_MODE_EN unmodified.
The behavior of DP83822_RGMII_MODE_EN should not have changed.
DP83822_RGMII_MODE_EN is always set in rcsr_mask and if the interface
is RGMII, DP83822_RX_CLK_SHIFT and DP83822_TX_CLK_SHIFT are added to
the mask as well.
I also have tested this patch on hardware with a DP83822 phy in RGMII
mode.
>
> It does not look correct to me.
>
> Cheers,
>
> Paolo
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-13 13:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-04 10:11 [PATCH v2] net: phy: dp83822: Fix RGMII TX delay configuration Tim Pambor
2024-02-08 10:12 ` Paolo Abeni
2024-02-13 13:07 ` Tim Pambor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox