Hi Andrew,
Le 13/07/2016 à 15:26, Andrew Lunn a écrit :
>>>> *
>>>> * Generic status code does not detect Fiber correctly!
>>>> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device
>>>> *phydev)
>>>> int lpa;
>>>> int lpagb;
>>>> int status = 0;
>>>> + int page, fiber;
>>>>
>>>> - /* Update the link, but return if there
>>>> + /* Detect and update the link, but return if there
>>>> * was an error */
>>>> - err = genphy_update_link(phydev);
>>>> - if (err)
>>>> - return err;
>>>> + page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
>>>> + if (page == MII_M1111_FIBER)
>>>> + fiber = 1;
>>>> + else
>>>> + fiber = 0;
>>>
>>> This read is expensive, since the MDIO bus is slow. It would be better
>>> just to pass fibre as a parameter.
>>
>> But this function is used for other Marvell's phy, without fiber link for
>> example.
>> And this function should has only the struct phy_device as parameter.
>>
>> I don't have idea to avoid that, without create a custom function for that
>> which would be very similar to this function.
>> Or used a phy_device field for that? I think it's awful idea...
>
> So i would have
>
> static int marvell_read_status_page(struct phy_device *phydev, int page)
> {}
>
> basically doing what you have above, but without the read.
>
> static int marvell_read_status(struct phy_device *phydev)
> {
> if (phydev->supported & SUPPORTED_FIBRE) {
> marvell_read_status_page(phydev, MII_M1111_FIBER);
> if (phydev->link)
> return;
>
> return marvell_read_status_page(phydev, MII_M1111_COPPER);
> }
Oh I see. Thank you!
>
>>> I think it would be better to look for SUPPORTED_FIBRE in
>>> drv->features, rather than have two different functions.
>>>
>>> In fact, i would do that in general, rather than add your _fibre()
>>> functions.
>>
>> So, you suggest to do that in genphy_* functions or create marvell_*
>> functions with this condition?
>> I'm agree with the second suggestion.
>
> The second.
I'm working on this.
It's done for _resume and _suspend. It will be done for _status.
But, for aneg or ethtool concerned, I think adding these functions is better.
>>>> +
>>>> +/* marvell_resume_fiber
>>>> + *
>>>> + * Some Marvell's phys have two modes: fiber and copper.
>>>> + * Both need to be resumed
>>>> + */
>>>> +static int marvell_resume_fiber(struct phy_device *phydev)
>>>> +{
>>>> + int err;
>>>> +
>>>> + /* Resume the fiber mode first */
>>>> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
>>>> + if (err < 0)
>>>> + goto error;
>>>> +
>>>> + err = genphy_resume(phydev);
>>>> + if (err < 0)
>>>> + goto error;
>>>> +
>>>> + /* Then, the copper link */
>>>> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>>>> + if (err < 0)
>>>> + goto error;
>>>> +
>>>> + return genphy_resume(phydev);
>>>
>>> Should it be resumed twice? Or just once at the end? Same question
>>> for suspend.
>>
>> I don't understand your question.
>
> You call genphy_resume(phydev) twice. Once is sufficient.
Yes, but it's normal because each interface could be suspended or resumed
independently.
genphy_* functions use BMCR register which are identical between fiber and
copper link. But each link has its own register to change.
Thank you.
Regards.
Charles-Antoine Couret
