Hi Jose
> +static int stmmac_xgmac2_mdio_read(struct stmmac_priv *priv, int phyaddr,
> + int phyreg)
> +{
> + unsigned int mii_address = priv->hw->mii.addr;
> + unsigned int mii_data = priv->hw->mii.data;
> + u32 tmp, addr, value = MII_XGMAC_BUSY;
> + int data;
> +
> + if (phyreg & MII_ADDR_C45) {
> + addr = ((phyreg >> 16) & 0x1f) << 21;
> + addr |= (phyaddr << 16) | (phyreg & 0xffff);
Do you need to tell the hardware this is a C45 transfer? Normally an
extra bit needs setting somewhere.
> + } else {
> + if (phyaddr >= 4)
> + return -ENODEV;
Can the MDIO bus be external? If so, is there a reason why there
cannot be a PHY at addresses > 4. So maybe there is an Ethernet
switch, which needs lots of addresses? And C45 can have devices > 4
but C22 cannot?
> + writel(~0x0, priv->ioaddr + 0x220);
> + addr = (phyaddr << 16) | (phyreg & 0x1f);
> + }
> +
> + value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
> + & priv->hw->mii.clk_csr_mask;
> + value |= BIT(18);
Please add a #define for this bit.
> + value |= MII_XGMAC_READ;
> +
> + if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> + !(tmp & MII_XGMAC_BUSY), 100, 10000))
> + return -EBUSY;
> +
> + 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 */
> + data = (int)readl(priv->ioaddr + mii_data) & GENMASK(15, 0);
Is the cast needed? And why use GENMASK here, but not in all the other
places you have masks in this code?
> /**
> * stmmac_mdio_read
> * @bus: points to the mii_bus structure
> @@ -59,6 +141,9 @@ static int stmmac_mdio_read(struct mii_bus *bus, int
> phyaddr, int phyreg)
> int data;
> u32 value = MII_BUSY;
>
> + if (priv->plat->has_xgmac)
> + return stmmac_xgmac2_mdio_read(priv, phyaddr, phyreg);
It would be cleaner to instead do this in stmmac_mdio_register() when
setting new_bus->read.
Andrew
