| Message ID | 20230619081633.589703-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: microchip: ksz9477: follow errata sheet when applying fixups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
> static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port) > { > + u16 cr; > + > + /* Errata document says the PHY must be configured to 100Mbps > + * with auto-neg disabled before configuring the PHY MMD > + * registers. > + */ > + ksz_pread16(dev, port, REG_PORT_PHY_CTRL, &cr); > + ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL, > + PORT_SPEED_100MBIT | PORT_FULL_DUPLEX); > + For this fix, these are fine. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Looking at the values of PORT_SPEED_100MBIT and PORT_FULL_DUPLEX, they are identical to BMCR_SPEED100 and BMCR_FULLDPLX. In fact, it looks like for 9477 this is a standard BMCR. Please could you add a follow up patch which replaces these #defines with the standard ones in include/uapi/linux/mii.h. The code is then more understandable by people who know the standard defines. Thanks Andrew
On Mon, 2023-06-19 at 10:16 +0200, Rasmus Villemoes wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you recognize the sender > and know the content is safe. > > The errata sheets for both ksz9477 and ksz9567 begin with > > IMPORTANT NOTE > > Multiple errata workarounds in this document call for changing PHY > registers for each PHY port. PHY registers 0x0 to 0x1F are in the > address range 0xN100 to 0xN13F, while indirect (MMD) PHY registers > are accessed via the PHY MMD Setup Register and the PHY MMD Data > Register. > > Before configuring the PHY MMD registers, it is necessary to set > the > PHY to 100 Mbps speed with auto-negotiation disabled by writing to > register 0xN100-0xN101. After writing the MMD registers, and after > all errata workarounds that involve PHY register settings, write > register 0xN100-0xN101 again to enable and restart auto- > negotiation. > > Without that explicit auto-neg restart, we do sometimes have problems > establishing link. > > Rather than writing back the hardcoded 0x1340 value the errata sheet > suggests (which likely just corresponds to the most common strap > configuration), restore the original value, setting the > PORT_AUTO_NEG_RESTART bit if PORT_AUTO_NEG_ENABLE is set. > > Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata > workarounds") > Cc: stable@vger.kernel.org > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > While I do believe this is a fix, I don't think it's post-rc7 > material, hence targeting net-next with cc stable. I don't think this will apply to net-next as the relevant code has been moved to the Micrel PHY driver and removed from this one in the following commits, and effectively the same change to disable autoneg before the register writes and re-enable afterwards was incorporated: commit 26dd2974c5b5caef358784530c9e72715adc8f5b Author: Robert Hancock <robert.hancock@calian.com> Date: Mon Jun 5 09:39:42 2023 -0600 net: phy: micrel: Move KSZ9477 errata fixes to PHY driver commit 6068e6d7ba5001dfb96bb8b7b92e2ed2a5877786 Author: Robert Hancock <robert.hancock@calian.com> Date: Mon Jun 5 09:39:43 2023 -0600 net: dsa: microchip: remove KSZ9477 PHY errata handling However, your patch may be reasonable to apply to -rc7 or stable as a more targeted change for those releases. > > drivers/net/dsa/microchip/ksz9477.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz9477.c > b/drivers/net/dsa/microchip/ksz9477.c > index bf13d47c26cf..9a712ea71ee7 100644 > --- a/drivers/net/dsa/microchip/ksz9477.c > +++ b/drivers/net/dsa/microchip/ksz9477.c > @@ -902,6 +902,16 @@ static void ksz9477_port_mmd_write(struct > ksz_device *dev, int port, > > static void ksz9477_phy_errata_setup(struct ksz_device *dev, int > port) > { > + u16 cr; > + > + /* Errata document says the PHY must be configured to 100Mbps > + * with auto-neg disabled before configuring the PHY MMD > + * registers. > + */ > + ksz_pread16(dev, port, REG_PORT_PHY_CTRL, &cr); > + ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL, > + PORT_SPEED_100MBIT | PORT_FULL_DUPLEX); > + > /* Apply PHY settings to address errata listed in > * KSZ9477, KSZ9897, KSZ9896, KSZ9567, KSZ8565 > * Silicon Errata and Data Sheet Clarification documents: > @@ -943,6 +953,13 @@ static void ksz9477_phy_errata_setup(struct > ksz_device *dev, int port) > ksz9477_port_mmd_write(dev, port, 0x1c, 0x1d, 0xe7ff); > ksz9477_port_mmd_write(dev, port, 0x1c, 0x1e, 0xefff); > ksz9477_port_mmd_write(dev, port, 0x1c, 0x20, 0xeeee); > + > + /* Restore PHY CTRL register, restart auto-negotiation if > + * enabled in the original value. > + */ > + if (cr & PORT_AUTO_NEG_ENABLE) > + cr |= PORT_AUTO_NEG_RESTART; > + ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL, cr); > } > > void ksz9477_get_caps(struct ksz_device *dev, int port, > -- > 2.37.2 >
On Mon, Jun 19, 2023 at 10:16:32AM +0200, Rasmus Villemoes wrote: > The errata sheets for both ksz9477 and ksz9567 begin with > > IMPORTANT NOTE > > Multiple errata workarounds in this document call for changing PHY > registers for each PHY port. PHY registers 0x0 to 0x1F are in the > address range 0xN100 to 0xN13F, while indirect (MMD) PHY registers > are accessed via the PHY MMD Setup Register and the PHY MMD Data > Register. > > Before configuring the PHY MMD registers, it is necessary to set the > PHY to 100 Mbps speed with auto-negotiation disabled by writing to > register 0xN100-0xN101. After writing the MMD registers, and after > all errata workarounds that involve PHY register settings, write > register 0xN100-0xN101 again to enable and restart auto-negotiation. > > Without that explicit auto-neg restart, we do sometimes have problems > establishing link. > > Rather than writing back the hardcoded 0x1340 value the errata sheet > suggests (which likely just corresponds to the most common strap > configuration), restore the original value, setting the > PORT_AUTO_NEG_RESTART bit if PORT_AUTO_NEG_ENABLE is set. > > Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata workarounds") > Cc: stable@vger.kernel.org > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > While I do believe this is a fix, I don't think it's post-rc7 > material, hence targeting net-next with cc stable. Hi Rasmus, unfortunately this does not seem to apply to net-next. Please consider rebasing and reposting. Please include Andrew's Reviewed-by tag unless there are substantial changes (seems unlikely).
On 19/06/2023 19.31, Robert Hancock wrote: > On Mon, 2023-06-19 at 10:16 +0200, Rasmus Villemoes wrote: >> >> Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata >> workarounds") >> Cc: stable@vger.kernel.org >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> --- >> While I do believe this is a fix, I don't think it's post-rc7 >> material, hence targeting net-next with cc stable. > > I don't think this will apply to net-next as the relevant code has been > moved to the Micrel PHY driver and removed from this one in the > following commits, Ah, sorry about that. This code hadn't been touched in a very long time, so I didn't actually think to check -next. and effectively the same change to disable autoneg > before the register writes and re-enable afterwards was incorporated: Yes, except it seems to always enable autoneg, even if the phy was strapped otherwise. That's not a problem for our use case. > However, your patch may be reasonable to apply to -rc7 or stable as a > more targeted change for those releases. Well, yes, it could be backported, but then there'd be a (simple) conflict when net-next is to be merged in the next window. So I don't think it's worth it. Unless the net maintainers decide otherwise, please just drop this patch. Rasmus
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index bf13d47c26cf..9a712ea71ee7 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -902,6 +902,16 @@ static void ksz9477_port_mmd_write(struct ksz_device *dev, int port, static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port) { + u16 cr; + + /* Errata document says the PHY must be configured to 100Mbps + * with auto-neg disabled before configuring the PHY MMD + * registers. + */ + ksz_pread16(dev, port, REG_PORT_PHY_CTRL, &cr); + ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL, + PORT_SPEED_100MBIT | PORT_FULL_DUPLEX); + /* Apply PHY settings to address errata listed in * KSZ9477, KSZ9897, KSZ9896, KSZ9567, KSZ8565 * Silicon Errata and Data Sheet Clarification documents: @@ -943,6 +953,13 @@ static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port) ksz9477_port_mmd_write(dev, port, 0x1c, 0x1d, 0xe7ff); ksz9477_port_mmd_write(dev, port, 0x1c, 0x1e, 0xefff); ksz9477_port_mmd_write(dev, port, 0x1c, 0x20, 0xeeee); + + /* Restore PHY CTRL register, restart auto-negotiation if + * enabled in the original value. + */ + if (cr & PORT_AUTO_NEG_ENABLE) + cr |= PORT_AUTO_NEG_RESTART; + ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL, cr); } void ksz9477_get_caps(struct ksz_device *dev, int port,
The errata sheets for both ksz9477 and ksz9567 begin with IMPORTANT NOTE Multiple errata workarounds in this document call for changing PHY registers for each PHY port. PHY registers 0x0 to 0x1F are in the address range 0xN100 to 0xN13F, while indirect (MMD) PHY registers are accessed via the PHY MMD Setup Register and the PHY MMD Data Register. Before configuring the PHY MMD registers, it is necessary to set the PHY to 100 Mbps speed with auto-negotiation disabled by writing to register 0xN100-0xN101. After writing the MMD registers, and after all errata workarounds that involve PHY register settings, write register 0xN100-0xN101 again to enable and restart auto-negotiation. Without that explicit auto-neg restart, we do sometimes have problems establishing link. Rather than writing back the hardcoded 0x1340 value the errata sheet suggests (which likely just corresponds to the most common strap configuration), restore the original value, setting the PORT_AUTO_NEG_RESTART bit if PORT_AUTO_NEG_ENABLE is set. Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata workarounds") Cc: stable@vger.kernel.org Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- While I do believe this is a fix, I don't think it's post-rc7 material, hence targeting net-next with cc stable. drivers/net/dsa/microchip/ksz9477.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)