Hi Andrew,
On 01-08-2018 16:08, Andrew Lunn wrote:
> 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.
The organization of addr reg is the following:
DA [25:21] | PA [20:16] | RA [15:0]
DA is Device Address, PA is Port Address and RA is Register Address.
>
>> + } 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?
Only ports 0 to 3 can be configured as C22 ports, according to
databook. This for MDIO bus trough controller.
>
>> + 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.
Ok.
>
>> + 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?
The GENMASK is needed, notice how we set more values into
mii_data (clk_csr, bit(18), cmd), this is not cleared by XGMAC2
upon the completion of the operation ...
>
>> /**
>> * 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.
Makes sense! Thanks!
Thanks and Best Regards,
Jose Miguel Abreu
>
> Andrew
