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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <21eb5877-ebdd-7710-e445-a60cb6e31ad6@synopsys.com>
Date:   Fri, 3 Aug 2018 16:25:27 +0100
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Andrew Lunn <andrew@...n.ch>, Jose Abreu <Jose.Abreu@...opsys.com>
CC:     <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
        "Joao Pinto" <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>
Subject: Re: [PATCH v2 net-next 5/9] net: stmmac: Add MDIO related functions
 for XGMAC2

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@...opsys.com>
>> Cc: David S. Miller <davem@...emloft.net>
>> Cc: Joao Pinto <jpinto@...opsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@...com>
>> Cc: Alexandre Torgue <alexandre.torgue@...com>
>> Cc: Andrew Lunn <andrew@...n.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