| Message ID | 20250303-dp83826-fixes-v1-2-6901a04f262d@yoseli.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: dp83826: Enable strap reading and fix TX data voltage support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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
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.
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
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
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
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.
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;
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(+)