On 03-08-2018 20:06, Florian Fainelli wrote:
> On 08/03/2018 08:50 AM, Jose Abreu wrote:
>> Add the MDIO related funcionalities for the new IP block XGMAC2.
>>
>> Signed-off-by: Jose Abreu <joab...@synopsys.com>
>> Cc: David S. Miller <da...@davemloft.net>
>> Cc: Joao Pinto <jpi...@synopsys.com>
>> Cc: Giuseppe Cavallaro <peppe.cavall...@st.com>
>> Cc: Alexandre Torgue <alexandre.tor...@st.com>
>> Cc: Andrew Lunn <and...@lunn.ch>
>> ---
>> +satic int stmmac_xgmac2_c22_format(struct stmmac_priv *priv, int phyaddr,
>> + int phyreg, u32 *hw_addr)
>> +{
>> + unsigned int mii_data = priv->hw->mii.data;
>> + u32 tmp;
>> +
>> + /* HW does not support C22 addr >= 4 */
>> + if (phyaddr >= 4)
>> + return -ENODEV;
> It would be nice if this could be moved at probe time so you don't have
> to wait until you connect to the PHY, read its PHY OUI and find out it
> has a MDIO address >= 4. Not a blocker, but something that could be
> improved further on.
>
> In premise you could even scan the MDIO bus' device tree node, and find
> that out ahead of time.
Oh, but I use phylib ... I only provide the read/write callbacks
so I think we should avoid duplicating the code that's already in
the phylib ... No?
>
>> + /* Wait until any existing MII operation is complete */
>> + if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
>> + !(tmp & MII_XGMAC_BUSY), 100, 10000))
>> + return -EBUSY;
>> +
>> + /* Set port as Clause 22 */
>> + tmp = readl(priv->ioaddr + XGMAC_MDIO_C22P);
>> + tmp |= BIT(phyaddr);
> Since the registers are being Read/Modify/Write here, don't you need to
> clear the previous address bits as well?
>
> You probably did not encounter any problems in your testing if you had
> only one PHY on the MDIO bus, but this is not something that is
> necessarily true, e.g: if you have an Ethernet switch, several MDIO bus
> addresses are going to be responding.
>
> Your MDIO bus implementation must be able to support one transaction
> with one PHY address and the next transaction with another PHY address ,
> etc...
>
> That is something that should be easy to fix and be resubmitted as part
> of v4.
I'm not following you here... I only set/unset the bit for the
corresponding phyaddr that phylib wants to read/write. Why would
I clear the remaining addresses?
Thanks and Best Regards,
Jose Miguel Abreu
