[PATCH 0/2] net: phy: dp83826: Enable strap reading and fix TX data voltage support

[PATCH 0/2] net: phy: dp83826: Enable strap reading and fix TX data voltage support

[Jean-Michel Hautbois]

Hi all,

Please find a small series to ensure correct operation across different
setups.

- Patch 1/2 fixes the TX data voltage initialization issue when
  CONFIG_OF_MDIO is disabled by setting default values for cfg_dac_minus
  and cfg_dac_plus to prevent PHY init failures.

- Patch 2/2 adds strap reading support during probe to configure RMII
  mode, MDIX, and auto-negotiation in line with hardware defaults.

Thanks for your feedbacks !

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
---
Jean-Michel Hautbois (2):
      net: phy: dp83826: Fix TX data voltage support
      net: phy: dp83826: Add support for straps reading

 drivers/net/phy/dp83822.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
---
base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
change-id: 20250303-dp83826-fixes-a854b9b0aabf

Best regards,
-- 
Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>

[PATCH 1/2] net: phy: dp83826: Fix TX data voltage support

[Jean-Michel Hautbois]

When CONFIG_OF_MDIO is not set, the cfg_dac_minus and cfg_dac_plus are
not set in dp83826_of_init(). This leads to a bad behavior in
dp83826_config_init: the phy initialization fails, after
MII_DP83826_VOD_CFG1 and MII_DP83826_VOD_CFG2 are set.

Fix it by setting the default value for both variables.

Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
---
 drivers/net/phy/dp83822.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 6599feca1967d705331d6e354205a2485ea962f2..88c49e8fe13e20e97191cddcd0885a6e075ae326 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -854,6 +854,10 @@ static int dp83822_of_init(struct phy_device *phydev)
 
 static void dp83826_of_init(struct phy_device *phydev)
 {
+	struct dp83822_private *dp83822 = phydev->priv;
+
+	dp83822->cfg_dac_minus = DP83826_CFG_DAC_MINUS_DEFAULT;
+	dp83822->cfg_dac_plus = DP83826_CFG_DAC_PLUS_DEFAULT;
 }
 #endif /* CONFIG_OF_MDIO */
 

-- 
2.39.5

[PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Jean-Michel Hautbois]

When the DP83826 is probed, read the straps, and apply the default
settings expected. The MDI-X is not yet supported, but still read the
strap.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
---
 drivers/net/phy/dp83822.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index 88c49e8fe13e20e97191cddcd0885a6e075ae326..5023f276b8818a5f7d9785fc53f77d59264ab4a4 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -197,6 +197,7 @@ struct dp83822_private {
 	bool set_gpio2_clk_out;
 	u32 gpio2_clk_out;
 	bool led_pin_enable[DP83822_MAX_LED_PINS];
+	int sor1;
 };
 
 static int dp83822_config_wol(struct phy_device *phydev,
@@ -620,6 +621,7 @@ static int dp83822_config_init(struct phy_device *phydev)
 static int dp8382x_config_rmii_mode(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
+	struct dp83822_private *dp83822 = phydev->priv;
 	const char *of_val;
 	int ret;
 
@@ -636,6 +638,17 @@ static int dp8382x_config_rmii_mode(struct phy_device *phydev)
 			ret = -EINVAL;
 		}
 
+		if (ret)
+			return ret;
+	} else {
+		if (dp83822->sor1 & BIT(5)) {
+			ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_RCSR,
+					       DP83822_RMII_MODE_SEL);
+		} else {
+			ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_RCSR,
+						 DP83822_RMII_MODE_SEL);
+		}
+
 		if (ret)
 			return ret;
 	}
@@ -888,6 +901,48 @@ static int dp83822_read_straps(struct phy_device *phydev)
 	return 0;
 }
 
+static int dp83826_read_straps(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_SOR1);
+	if (val < 0)
+		return val;
+
+	phydev_dbg(phydev, "SOR1 strap register: 0x%04x\n", val);
+
+	/* Bit 10: MDIX mode */
+	if (val & BIT(10))
+		phydev_dbg(phydev, "MDIX mode enabled\n");
+
+	/* Bit 9: auto-MDIX disable */
+	if (val & BIT(9))
+		phydev_dbg(phydev, "Auto-MDIX disabled\n");
+
+	/* Bit 8: RMII */
+	if (val & BIT(8)) {
+		phydev_dbg(phydev, "RMII mode enabled\n");
+		phydev->interface = PHY_INTERFACE_MODE_RMII;
+	}
+
+	/* Bit 5: Slave mode */
+	if (val & BIT(5))
+		phydev_dbg(phydev, "RMII slave mode enabled\n");
+
+	/* Bit 0: autoneg disable */
+	if (val & BIT(0)) {
+		phydev_dbg(phydev, "Auto-negotiation disabled\n");
+		phydev->autoneg = AUTONEG_DISABLE;
+		phydev->speed = SPEED_100;
+		phydev->duplex = DUPLEX_FULL;
+	}
+
+	dp83822->sor1 = val;
+
+	return 0;
+}
+
 static int dp8382x_probe(struct phy_device *phydev)
 {
 	struct dp83822_private *dp83822;
@@ -935,6 +990,10 @@ static int dp83826_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	ret = dp83826_read_straps(phydev);
+	if (ret)
+		return ret;
+
 	dp83826_of_init(phydev);
 
 	return 0;

-- 
2.39.5

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Andrew Lunn]

On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
> When the DP83826 is probed, read the straps, and apply the default
> settings expected. The MDI-X is not yet supported, but still read the
> strap.

What about backwards compatibility? I expect this changes the
behaviour of the device, potentially introducing regressions?  Please
add an explanation of why this is safe.

    Andrew

---
pw-bot: cr

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Russell King (Oracle)]

On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
> +	/* Bit 10: MDIX mode */
> +	if (val & BIT(10))
> +		phydev_dbg(phydev, "MDIX mode enabled\n");
> +
> +	/* Bit 9: auto-MDIX disable */
> +	if (val & BIT(9))
> +		phydev_dbg(phydev, "Auto-MDIX disabled\n");
> +
> +	/* Bit 8: RMII */
> +	if (val & BIT(8)) {
> +		phydev_dbg(phydev, "RMII mode enabled\n");
> +		phydev->interface = PHY_INTERFACE_MODE_RMII;
> +	}

Do all users of this PHY driver support having phydev->interface
changed?

> +
> +	/* Bit 5: Slave mode */
> +	if (val & BIT(5))
> +		phydev_dbg(phydev, "RMII slave mode enabled\n");
> +
> +	/* Bit 0: autoneg disable */
> +	if (val & BIT(0)) {
> +		phydev_dbg(phydev, "Auto-negotiation disabled\n");
> +		phydev->autoneg = AUTONEG_DISABLE;
> +		phydev->speed = SPEED_100;
> +		phydev->duplex = DUPLEX_FULL;
> +	}

This doesn't force phylib to disallow autoneg.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Jean-Michel Hautbois]

Hi Russel,

On 03/03/2025 18:25, Russell King (Oracle) wrote:
> On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
>> +	/* Bit 10: MDIX mode */
>> +	if (val & BIT(10))
>> +		phydev_dbg(phydev, "MDIX mode enabled\n");
>> +
>> +	/* Bit 9: auto-MDIX disable */
>> +	if (val & BIT(9))
>> +		phydev_dbg(phydev, "Auto-MDIX disabled\n");
>> +
>> +	/* Bit 8: RMII */
>> +	if (val & BIT(8)) {
>> +		phydev_dbg(phydev, "RMII mode enabled\n");
>> +		phydev->interface = PHY_INTERFACE_MODE_RMII;
>> +	}
> 
> Do all users of this PHY driver support having phydev->interface
> changed?
> 

I don't know, what is the correct way to know and do it ?
Other phys did something similar (bcm84881_read_status is an example I 
took).

>> +
>> +	/* Bit 5: Slave mode */
>> +	if (val & BIT(5))
>> +		phydev_dbg(phydev, "RMII slave mode enabled\n");
>> +
>> +	/* Bit 0: autoneg disable */
>> +	if (val & BIT(0)) {
>> +		phydev_dbg(phydev, "Auto-negotiation disabled\n");
>> +		phydev->autoneg = AUTONEG_DISABLE;
>> +		phydev->speed = SPEED_100;
>> +		phydev->duplex = DUPLEX_FULL;
>> +	}
> 
> This doesn't force phylib to disallow autoneg.
> 

Is it needed to call phy_lookup_setting() or something else ?

Thanks for your feedback,
JM

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Jean-Michel Hautbois]

Hi Andrew,

On 03/03/2025 18:20, Andrew Lunn wrote:
> On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
>> When the DP83826 is probed, read the straps, and apply the default
>> settings expected. The MDI-X is not yet supported, but still read the
>> strap.
> 
> What about backwards compatibility? I expect this changes the
> behaviour of the device, potentially introducing regressions?  Please
> add an explanation of why this is safe.

I am not certain it is safe. As far as I know that if straps are used on 
the hardware, then it should be used, and if the behavior has to be 
different, then userspace can change it (or any other way). Am I wrong ?

How could we make is safer, though ? We somehow need to read those ?

Thanks,
JM

> 
>      Andrew
> 
> ---
> pw-bot: cr

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Andrew Lunn]

On Mon, Mar 03, 2025 at 06:37:28PM +0100, Jean-Michel Hautbois wrote:
> Hi Andrew,
> 
> On 03/03/2025 18:20, Andrew Lunn wrote:
> > On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
> > > When the DP83826 is probed, read the straps, and apply the default
> > > settings expected. The MDI-X is not yet supported, but still read the
> > > strap.
> > 
> > What about backwards compatibility? I expect this changes the
> > behaviour of the device, potentially introducing regressions?  Please
> > add an explanation of why this is safe.
> 
> I am not certain it is safe. As far as I know that if straps are used on the
> hardware, then it should be used, and if the behavior has to be different,
> then userspace can change it (or any other way). Am I wrong ?

First off, what does the datasheet say about these straps?

Straps generally configure the hardware, without software being
involved. It seems odd you need software to read the straps and apply
them.

Why do you need this? What is your use case. As you said, user space
can configure all this, so why are you not doing that?

	Andrew

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Jean-Michel Hautbois]

Hi Andrew,

On 03/03/2025 18:50, Andrew Lunn wrote:
> On Mon, Mar 03, 2025 at 06:37:28PM +0100, Jean-Michel Hautbois wrote:
>> Hi Andrew,
>>
>> On 03/03/2025 18:20, Andrew Lunn wrote:
>>> On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
>>>> When the DP83826 is probed, read the straps, and apply the default
>>>> settings expected. The MDI-X is not yet supported, but still read the
>>>> strap.
>>>
>>> What about backwards compatibility? I expect this changes the
>>> behaviour of the device, potentially introducing regressions?  Please
>>> add an explanation of why this is safe.
>>
>> I am not certain it is safe. As far as I know that if straps are used on the
>> hardware, then it should be used, and if the behavior has to be different,
>> then userspace can change it (or any other way). Am I wrong ?
> 
> First off, what does the datasheet say about these straps?
> 
> Straps generally configure the hardware, without software being
> involved. It seems odd you need software to read the straps and apply
> them.
> 
> Why do you need this? What is your use case. As you said, user space
> can configure all this, so why are you not doing that?
> 
Mmmh, now that you say that, it makes me think that it could probably be 
configured indeed...
The HW uses the straps, and it disables autoneg and fixes a 100Mbps / 
Full duplex link on the board I have.
But, event if HW does this, autoneg still starts when ip link sets the 
device up.

Maybe should I call ethtool before ip link and see if it does the trick.

Thanks,
JM

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Russell King (Oracle)]

On Mon, Mar 03, 2025 at 06:35:04PM +0100, Jean-Michel Hautbois wrote:
> Hi Russel,
> 
> On 03/03/2025 18:25, Russell King (Oracle) wrote:
> > On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
> > > +	/* Bit 10: MDIX mode */
> > > +	if (val & BIT(10))
> > > +		phydev_dbg(phydev, "MDIX mode enabled\n");
> > > +
> > > +	/* Bit 9: auto-MDIX disable */
> > > +	if (val & BIT(9))
> > > +		phydev_dbg(phydev, "Auto-MDIX disabled\n");
> > > +
> > > +	/* Bit 8: RMII */
> > > +	if (val & BIT(8)) {
> > > +		phydev_dbg(phydev, "RMII mode enabled\n");
> > > +		phydev->interface = PHY_INTERFACE_MODE_RMII;
> > > +	}
> > 
> > Do all users of this PHY driver support having phydev->interface
> > changed?
> > 
> 
> I don't know, what is the correct way to know and do it ?
> Other phys did something similar (bcm84881_read_status is an example I
> took).

That's currently known to only be used in a SFP, and therefore it uses
phylink, and therefore changing ->interface is supported (phylink's
design is to support this.)

> > > +
> > > +	/* Bit 5: Slave mode */
> > > +	if (val & BIT(5))
> > > +		phydev_dbg(phydev, "RMII slave mode enabled\n");
> > > +
> > > +	/* Bit 0: autoneg disable */
> > > +	if (val & BIT(0)) {
> > > +		phydev_dbg(phydev, "Auto-negotiation disabled\n");
> > > +		phydev->autoneg = AUTONEG_DISABLE;
> > > +		phydev->speed = SPEED_100;
> > > +		phydev->duplex = DUPLEX_FULL;
> > > +	}
> > 
> > This doesn't force phylib to disallow autoneg.
> 
> Is it needed to call phy_lookup_setting() or something else ?

Have a look at phy_ethtool_ksettings_set(), there's some clues in
there about how to prevent autoneg being enabled.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: dp83826: Add support for straps reading

[Jean-Michel Hautbois]

Hello Andrew,

On 03/03/2025 18:50, Andrew Lunn wrote:
> On Mon, Mar 03, 2025 at 06:37:28PM +0100, Jean-Michel Hautbois wrote:
>> Hi Andrew,
>>
>> On 03/03/2025 18:20, Andrew Lunn wrote:
>>> On Mon, Mar 03, 2025 at 06:05:52PM +0100, Jean-Michel Hautbois wrote:
>>>> When the DP83826 is probed, read the straps, and apply the default
>>>> settings expected. The MDI-X is not yet supported, but still read the
>>>> strap.
>>>
>>> What about backwards compatibility? I expect this changes the
>>> behaviour of the device, potentially introducing regressions?  Please
>>> add an explanation of why this is safe.
>>
>> I am not certain it is safe. As far as I know that if straps are used on the
>> hardware, then it should be used, and if the behavior has to be different,
>> then userspace can change it (or any other way). Am I wrong ?
> 
> First off, what does the datasheet say about these straps?
> 
> Straps generally configure the hardware, without software being
> involved. It seems odd you need software to read the straps and apply
> them.
> 
> Why do you need this? What is your use case. As you said, user space
> can configure all this, so why are you not doing that?

Thanks you for your remarks, indeed, ip link set + ethtool -s works fine 
to force the link mode wanted. Event the master/slave can be specified, 
so there is no need for this patch 2/2.

Thanks !
JM

Re: [PATCH 1/2] net: phy: dp83826: Fix TX data voltage support

[Jean-Michel Hautbois]

Hi there !

On 3/3/25 6:05 PM, Jean-Michel Hautbois wrote:
> When CONFIG_OF_MDIO is not set, the cfg_dac_minus and cfg_dac_plus are
> not set in dp83826_of_init(). This leads to a bad behavior in
> dp83826_config_init: the phy initialization fails, after
> MII_DP83826_VOD_CFG1 and MII_DP83826_VOD_CFG2 are set.
> 
> Fix it by setting the default value for both variables.
> 
> Fixes: d1d77120bc28 ("net: phy: dp83826: support TX data voltage tuning")
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
> ---
>   drivers/net/phy/dp83822.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> index 6599feca1967d705331d6e354205a2485ea962f2..88c49e8fe13e20e97191cddcd0885a6e075ae326 100644
> --- a/drivers/net/phy/dp83822.c
> +++ b/drivers/net/phy/dp83822.c
> @@ -854,6 +854,10 @@ static int dp83822_of_init(struct phy_device *phydev)
>   
>   static void dp83826_of_init(struct phy_device *phydev)
>   {
> +	struct dp83822_private *dp83822 = phydev->priv;
> +
> +	dp83822->cfg_dac_minus = DP83826_CFG_DAC_MINUS_DEFAULT;
> +	dp83822->cfg_dac_plus = DP83826_CFG_DAC_PLUS_DEFAULT;
>   }
>   #endif /* CONFIG_OF_MDIO */
>   
> 

Gentle ping to know if this patch is ok (patch 2/2 is not) and if so, 
should I repost a v2 maybe ?

Thanks !
JM