Messages in this thread | | | Date | Thu, 27 May 2021 01:01:32 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v3 1/9] net: phy: micrel: move phy reg offsets to common header |
| |
On Wed, May 26, 2021 at 06:30:29AM +0200, Oleksij Rempel wrote: > From: Michael Grzeschik <m.grzeschik@pengutronix.de> > > Some micrel devices share the same PHY register defines. This patch > moves them to one common header so other drivers can reuse them. > And reuse generic MII_* defines where possible. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz8795.c | 119 ++++++++++++------------ > drivers/net/dsa/microchip/ksz8795_reg.h | 62 ------------ > drivers/net/ethernet/micrel/ksz884x.c | 105 +++------------------ > include/linux/micrel_phy.h | 13 +++ > 4 files changed, 88 insertions(+), 211 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > index ad509a57a945..ba065003623f 100644 > --- a/drivers/net/dsa/microchip/ksz8795.c > +++ b/drivers/net/dsa/microchip/ksz8795.c > @@ -15,6 +15,7 @@ > #include <linux/phy.h> > #include <linux/etherdevice.h> > #include <linux/if_bridge.h> > +#include <linux/micrel_phy.h> > #include <net/dsa.h> > #include <net/switchdev.h> > > @@ -731,88 +732,88 @@ static void ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val) > u8 p = phy; > > switch (reg) { > - case PHY_REG_CTRL: > + case MII_BMCR: > ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart); > ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed); > ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl); > if (restart & PORT_PHY_LOOPBACK) > - data |= PHY_LOOPBACK; > + data |= BMCR_LOOPBACK; > if (ctrl & PORT_FORCE_100_MBIT) > - data |= PHY_SPEED_100MBIT; > + data |= BMCR_SPEED100; > if (ksz_is_ksz88x3(dev)) { > if ((ctrl & PORT_AUTO_NEG_ENABLE)) > - data |= PHY_AUTO_NEG_ENABLE; > + data |= BMCR_ANENABLE; > } else { > if (!(ctrl & PORT_AUTO_NEG_DISABLE)) > - data |= PHY_AUTO_NEG_ENABLE; > + data |= BMCR_ANENABLE; > } > if (restart & PORT_POWER_DOWN) > - data |= PHY_POWER_DOWN; > + data |= BMCR_PDOWN; > if (restart & PORT_AUTO_NEG_RESTART) > - data |= PHY_AUTO_NEG_RESTART; > + data |= BMCR_ANRESTART; > if (ctrl & PORT_FORCE_FULL_DUPLEX) > - data |= PHY_FULL_DUPLEX; > + data |= BMCR_FULLDPLX; > if (speed & PORT_HP_MDIX) > - data |= PHY_HP_MDIX; > + data |= KSZ886X_BMCR_HP_MDIX; > if (restart & PORT_FORCE_MDIX) > - data |= PHY_FORCE_MDIX; > + data |= KSZ886X_BMCR_FORCE_MDI; > if (restart & PORT_AUTO_MDIX_DISABLE) > - data |= PHY_AUTO_MDIX_DISABLE; > + data |= KSZ886X_BMCR_DISABLE_AUTO_MDIX; > if (restart & PORT_TX_DISABLE) > - data |= PHY_TRANSMIT_DISABLE; > + data |= KSZ886X_BMCR_DISABLE_TRANSMIT; > if (restart & PORT_LED_OFF) > - data |= PHY_LED_DISABLE; > + data |= KSZ886X_BMCR_DISABLE_LED; > break;
I am deeply confused as to what this function is doing. It is reading the 8-bit port registers P_NEG_RESTART_CTRL, P_SPEED_STATUS and P_FORCE_CTRL and stitching them into a 16-bit "MII_BMCR"?
What layout does this control register even have? Seeing as this is the implementation of ksz_phy_read16(), I expect that MII_BMCR has the layout specified in clause 22.2.4.1?
But clause 22 says register 0.5 is "Unidirectional enable", not "PHY_HP_MDIX" (whatever that might be), and bits 0.4:0 are reserved and must be written as zero and ignored on read.
> - case PHY_REG_STATUS: > + case MII_BMSR: > ksz_pread8(dev, p, regs[P_LINK_STATUS], &link); > - data = PHY_100BTX_FD_CAPABLE | > - PHY_100BTX_CAPABLE | > - PHY_10BT_FD_CAPABLE | > - PHY_10BT_CAPABLE | > - PHY_AUTO_NEG_CAPABLE; > + data = BMSR_100FULL | > + BMSR_100HALF | > + BMSR_10FULL | > + BMSR_10HALF | > + BMSR_ANEGCAPABLE; > if (link & PORT_AUTO_NEG_COMPLETE) > - data |= PHY_AUTO_NEG_ACKNOWLEDGE; > + data |= BMSR_ANEGCOMPLETE; > if (link & PORT_STAT_LINK_GOOD) > - data |= PHY_LINK_STATUS; > + data |= BMSR_LSTATUS; > break; > - case PHY_REG_ID_1: > + case MII_PHYSID1: > data = KSZ8795_ID_HI; > break; > - case PHY_REG_ID_2: > + case MII_PHYSID2: > if (ksz_is_ksz88x3(dev)) > data = KSZ8863_ID_LO; > else > data = KSZ8795_ID_LO; > break; > - case PHY_REG_AUTO_NEGOTIATION: > + case MII_ADVERTISE: > ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl); > - data = PHY_AUTO_NEG_802_3; > + data = ADVERTISE_CSMA; > if (ctrl & PORT_AUTO_NEG_SYM_PAUSE) > - data |= PHY_AUTO_NEG_SYM_PAUSE; > + data |= ADVERTISE_PAUSE_CAP; > if (ctrl & PORT_AUTO_NEG_100BTX_FD) > - data |= PHY_AUTO_NEG_100BTX_FD; > + data |= ADVERTISE_100FULL; > if (ctrl & PORT_AUTO_NEG_100BTX) > - data |= PHY_AUTO_NEG_100BTX; > + data |= ADVERTISE_100HALF; > if (ctrl & PORT_AUTO_NEG_10BT_FD) > - data |= PHY_AUTO_NEG_10BT_FD; > + data |= ADVERTISE_10FULL; > if (ctrl & PORT_AUTO_NEG_10BT) > - data |= PHY_AUTO_NEG_10BT; > + data |= ADVERTISE_10HALF; > break; > - case PHY_REG_REMOTE_CAPABILITY: > + case MII_LPA: > ksz_pread8(dev, p, regs[P_REMOTE_STATUS], &link); > - data = PHY_AUTO_NEG_802_3; > + data = LPA_SLCT; > if (link & PORT_REMOTE_SYM_PAUSE) > - data |= PHY_AUTO_NEG_SYM_PAUSE; > + data |= LPA_PAUSE_CAP; > if (link & PORT_REMOTE_100BTX_FD) > - data |= PHY_AUTO_NEG_100BTX_FD; > + data |= LPA_100FULL; > if (link & PORT_REMOTE_100BTX) > - data |= PHY_AUTO_NEG_100BTX; > + data |= LPA_100HALF; > if (link & PORT_REMOTE_10BT_FD) > - data |= PHY_AUTO_NEG_10BT_FD; > + data |= LPA_10FULL; > if (link & PORT_REMOTE_10BT) > - data |= PHY_AUTO_NEG_10BT; > - if (data & ~PHY_AUTO_NEG_802_3) > - data |= PHY_REMOTE_ACKNOWLEDGE_NOT; > + data |= LPA_10HALF; > + if (data & ~LPA_SLCT) > + data |= LPA_LPACK; > break; > default: > processed = false;
| |