Hi Florian,
On 06-08-2018 16:25, Florian Fainelli wrote:
> On August 6, 2018 12:59:54 AM PDT, Jose Abreu <jose.ab...@synopsys.com> wrote:
>> 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?
> You can always extract the code that scans a MDIO bus into a helper function
> and make it parametrized with a callback of some kind. In that case I would
> be fine with you open coding the MDIO bus scan to find out if there is an
> address >= 4.
Sorry but I dont' think thats the best solution because
of_mdiobus_register() already scans the bus. Duplicating this
should be avoided, no?
Note all of this is probably never needed because stmmac just
picks the first phy it finds, if phy_addr is not specified ...
>
>>>> + /* 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?
> Because this is all about transactions, the HW must be in a state that it
> will be able to perform that transaction correctly. So here for instance if
> you needed to support a C45 transaction you would have to clear that bit for
> that particular PHY address. Since you don't appear to support those yet then
> yes the code appears fine though it would not hurt if you did clear all other
> PHY's c22 bits to make it clear what this does.
I can't test C45 right now but I will in a near future. In that
case then we will need to support C22 and C45 so I may want to
only set one bit for a specific phy that only supports c22.
Thanks and Best Regards,
Jose Miguel Abreu
>
