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

[Jose Abreu <Jose.Abreu@synopsys.com>]

Hi Andrew,

On 03-08-2018 16:20, Andrew Lunn wrote:
> 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.

Yes, makes sense.

>
>> +
>> +		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.

Less typing :D

>
>> +	}
>> +
>> +	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.

Ok but I can't test C45 right now so maybe leave that change to
when I can test it ?

Thanks and Best Regards,
Jose Miguel Abreu

>
>> +
>> +	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