Re: [PATCH net-next v3 1/9] net: phy: micrel: move phy reg offsets to common header

[Vladimir Oltean]

On Wed, May 26, 2021 at 06:30:29AM +0200, Oleksij Rempel wrote:
> From: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
>  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;