Re: [PATCH net-next v3 1/9] net: phy: micrel: move phy reg offsets to common header
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-05-26 22:01:40
Also in:
lkml
On Wed, May 26, 2021 at 06:30:29AM +0200, Oleksij Rempel wrote:
quoted hunk ↗ jump to hunk
From: Michael Grzeschik <redacted> 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 <redacted> 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;