Re: [PATCH v2 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Aug 03, 2018 at 03:56:07PM +0100, Jose Abreu wrote:
> Add the MDIO related funcionalities for the new IP block XGMAC2.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> ---
> Changes from v1:
> 	- Remove C45 support (Andrew)
> 	- Add define for bits (Andrew)
> 	- Remove uneeded cast (Andrew)
> 	- Use different callbacks instead of if's (Andrew)
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 101 +++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 5df1a608e566..9bbdb78d3315 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -29,6 +29,7 @@
>  #include <linux/phy.h>
>  #include <linux/slab.h>
>  
> +#include "dwxgmac2.h"
>  #include "stmmac.h"
>  
>  #define MII_BUSY 0x00000001
> @@ -39,6 +40,96 @@
>  #define MII_GMAC4_WRITE			(1 << MII_GMAC4_GOC_SHIFT)
>  #define MII_GMAC4_READ			(3 << MII_GMAC4_GOC_SHIFT)
>  
> +/* XGMAC defines */
> +#define MII_XGMAC_SADDR			BIT(18)
> +#define MII_XGMAC_CMD_SHIFT		16
> +#define MII_XGMAC_WRITE			(1 << MII_XGMAC_CMD_SHIFT)
> +#define MII_XGMAC_READ			(3 << MII_XGMAC_CMD_SHIFT)
> +#define MII_XGMAC_BUSY			BIT(22)
> +
> +static int stmmac_xgmac2_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
> +{
> +	struct net_device *ndev = bus->priv;
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	unsigned int mii_address = priv->hw->mii.addr;
> +	unsigned int mii_data = priv->hw->mii.data;
> +	u32 tmp, addr, value = MII_XGMAC_BUSY;
> +
> +	if (phyreg & MII_ADDR_C45) {
> +		return -EOPNOTSUPP;
> +	} else {
> +		if (phyaddr >= 4)
> +			return -ENODEV;
> +
> +		/* Set port as Clause 22 */
> +		tmp = readl(priv->ioaddr + XGMAC_MDIO_C22P);
> +		tmp |= BIT(phyaddr);
> +		writel(tmp, priv->ioaddr + XGMAC_MDIO_C22P);

Hi Jose

Maybe put this into a helper? You do repeat it twice.

> +
> +		addr = (phyaddr << 16) | (phyreg & 0x1f);

You could use GENMASK(4, 0) here. That was the point i was trying to
make earlier. But i actually find 0x1f, and 0xffff easier to read.

> +	}
> +
> +	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> +		& priv->hw->mii.clk_csr_mask;
> +	value |= MII_XGMAC_SADDR | MII_XGMAC_READ;
> +
> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> +		return -EBUSY;

Probably you want to wait for the bus to be idle before you change the
mode to C22. Some PHYs can do both C22 and C45, e.g. EEE registers can
be in C45 space, while the rest are in C22.

> +
> +	writel(addr, priv->ioaddr + mii_address);
> +	writel(value, priv->ioaddr + mii_data);
> +
> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> +		return -EBUSY;
> +
> +	/* Read the data from the MII data register */
> +	return readl(priv->ioaddr + mii_data) & GENMASK(15, 0);
> +}

  Andrew