Re: [PATCH v2 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2
> > 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 ? I would fix this now. It probably cannot cause issues now, but it is wrong. You are going to have to fix it some time, so why not now? Andrew
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
>> Cc: David S. Miller
>> Cc: Joao Pinto
>> Cc: Giuseppe Cavallaro
>> Cc: Alexandre Torgue
>> Cc: Andrew Lunn
>> ---
>> 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
>> #include
>>
>> +#include "dwxgmac2.h"
>> #include "stmmac.h"
>>
>> #define MII_BUSY 0x0001
>> @@ -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 0x 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, 1))
>> +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, 1))
>> +return -EBUSY;
>> +
>> +/* Read the data from the MII data register */
>> +return readl(priv->ioaddr + mii_data) & GENMASK(15, 0);
>> +}
> Andrew
Re: [PATCH v2 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2
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
> Cc: David S. Miller
> Cc: Joao Pinto
> Cc: Giuseppe Cavallaro
> Cc: Alexandre Torgue
> Cc: Andrew Lunn
> ---
> 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
> #include
>
> +#include "dwxgmac2.h"
> #include "stmmac.h"
>
> #define MII_BUSY 0x0001
> @@ -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 0x 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, 1))
> + 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, 1))
> + return -EBUSY;
> +
> + /* Read the data from the MII data register */
> + return readl(priv->ioaddr + mii_data) & GENMASK(15, 0);
> +}
Andrew
