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.5On 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!
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
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!
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.