Received-SPF: pass (google.com: domain of linux-kernel+bounces-63573-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <7b985e81cc0e7f8f3dab939a097ebeecd78f9477.camel@osasysteme.de>
Subject: Re: [PATCH v2] net: phy: dp83822: Fix RGMII TX delay configuration
From: Tim Pambor <tp@osasysteme.de>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, 
 Russell King <linux@armlinux.org.uk>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
 <kuba@kernel.org>, Dan Murphy <dmurphy@ti.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
Date: Tue, 13 Feb 2024 14:07:01 +0100
In-Reply-To: <63d59bff272f572c94f97000475c4ac3e157c013.camel@redhat.com>
References: <20240204101128.49336-1-tp@osasysteme.de>
	 <63d59bff272f572c94f97000475c4ac3e157c013.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3-1 
Precedence: bulk
MIME-Version: 1.0

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.
> >=20
> > Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf=C2=A0p. 69
> >=20
> > Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed
> > internal delay")
> > Signed-off-by: Tim Pambor <tp@osasysteme.de>
> > ---
> > Changes in v2:
> > =C2=A0 - Further cleanup of RGMII configuration
>=20
> This overall makes the change more invasive, I'm unsure is in the
> direction pointed by Russell
>=20
> > =C2=A0 - Check for errors setting DP83822_RGMII_MODE_EN
> > ---
> > =C2=A0drivers/net/phy/dp83822.c | 41 +++++++++++++---------------------=
-
> > ----
> > =C2=A01 file changed, 14 insertions(+), 27 deletions(-)
> >=20
> > 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)
> > =C2=A0{
> > =C2=A0	struct dp83822_private *dp83822 =3D phydev->priv;
> > =C2=A0	struct device *dev =3D &phydev->mdio.dev;
> > -	int rgmii_delay;
> > -	s32 rx_int_delay;
> > -	s32 tx_int_delay;
> > +	int rcsr_mask =3D DP83822_RGMII_MODE_EN;
> > +	int rcsr =3D 0;
> > =C2=A0	int err =3D 0;
> > =C2=A0	int bmcr;
> > =C2=A0
> > =C2=A0	if (phy_interface_is_rgmii(phydev)) {
> > -		rx_int_delay =3D phy_get_internal_delay(phydev, dev,
> > NULL, 0,
> > -						=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 true);
> > +		rcsr |=3D DP83822_RGMII_MODE_EN;
> > =C2=A0
> > -		if (rx_int_delay <=3D 0)
> > -			rgmii_delay =3D 0;
> > -		else
> > -			rgmii_delay =3D 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 |=3D DP83822_RX_CLK_SHIFT;
> > =C2=A0
> > -		tx_int_delay =3D phy_get_internal_delay(phydev, dev,
> > NULL, 0,
> > -						=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 false);
> > -		if (tx_int_delay <=3D 0)
> > -			rgmii_delay &=3D ~DP83822_TX_CLK_SHIFT;
> > -		else
> > -			rgmii_delay |=3D 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) <=3D 0)
> > +			rcsr |=3D DP83822_TX_CLK_SHIFT;
> > =C2=A0
> > -		if (rgmii_delay) {
> > -			err =3D phy_set_bits_mmd(phydev,
> > DP83822_DEVADDR,
> > -					=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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 |=3D DP83822_RX_CLK_SHIFT |
> > DP83822_TX_CLK_SHIFT;
>=20
> 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.

>=20
> It does not look correct to me.
>=20
> Cheers,
>=20
> Paolo
>=20