On 08/07/2018 06:35 AM, Jose Abreu wrote:
> 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?
I suggested you extract the code into a helper to avoid the duplication.
>
> Note all of this is probably never needed because stmmac just
> picks the first phy it finds, if phy_addr is not specified ...
No, it's much more involved than that, there are several modes in which
MDIO on stmmac can be instantiated:
- platform data instantiation, which uses a loop and finds the first
available address
- Device Tree which will scan the MDIO bus' node child nodes and create
PHYs for valid nodes
It feels a bit late to check for PHY address >= 4 during the first read,
because it won't be obvious to somehow why this fails, but if that
fulfills your needs, keep it that way.
>
>>
>>>>> + /* 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.
We are really talking about adding something like:
reg &= ~(MASK);
before setting the bit, anyhow, if you want to do that later on when you
add C45 support, be my guest...
--
Florian
