Le 12/07/2016 à 17:57, Andrew Lunn a écrit :
>> +#define LPA_FIBER_1000HALF 0x40
>> +#define LPA_FIBER_1000FULL 0x20
>> +
>> +#define LPA_PAUSE_FIBER 0x180
>> +#define LPA_PAUSE_ASYM_FIBER 0x100
>> +
>> +#define ADVERTISE_FIBER_1000HALF 0x40
>> +#define ADVERTISE_FIBER_1000FULL 0x20
>> +
>> +#define ADVERTISE_PAUSE_FIBER 0x180
>> +#define ADVERTISE_PAUSE_ASYM_FIBER 0x100
>
> Are these standardised anywhere? If they are following a standard,
> they should be put into include/uapi/linux/mii.h.
[snip]
>> + if (ethadv & ADVERTISED_1000baseT_Full)
>> + result |= ADVERTISE_FIBER_1000FULL;
>> +
>> + if ((ethadv & ADVERTISE_PAUSE_ASYM) && (ethadv & ADVERTISE_PAUSE_CAP))
>> + result |= LPA_PAUSE_ASYM_FIBER;
>> + else if (ethadv & ADVERTISE_PAUSE_CAP)
>> + result |= (ADVERTISE_PAUSE_FIBER
>> + & (~ADVERTISE_PAUSE_ASYM_FIBER));
>> +
>> + return result;
>> +}
>
> If these values are standardised, i think this function should be
> moved into the generic code. If however, this is Marvell specific,
> keep it here.
>
I don't find any standard about this, I think it should be Marvell specific.
>> +static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
>> +{
>> + u32 result = 0;
>> +
>> + if (ethadv & ADVERTISED_1000baseT_Half)
>> + result |= ADVERTISE_FIBER_1000HALF;
>
> Dumb question: Does 1000baseT_Half even make sense for fibre? Can you
> do half duplex? Would that not mean you have a single fibre, both
> ends are using the same laser frequency, and you are doing some form
> of CSMA/CD?
It's strange, I agree, but the register about that exists in the datasheet and
the value is not fixed.
In practice, I don't have a component to test this case correctly.
>> *
>> * 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...
>> +/* marvell_read_fiber_status
>> + *
>> + * Some Marvell's phys have two modes: fiber and copper.
>> + * Both need status checked.
>> + * Description:
>> + * First, check the fiber link and status.
>> + * If the fiber link is down, check the copper link and status which
>> + * will be the default value if both link are down.
>> + */
>> +static int marvell_read_fiber_status(struct phy_device *phydev)
>
> The name is a bit confusing. I would probably use
> marvell_read_copper_fiber_status() making it clear it reads both.
You're right.
>> +
>> + return marvell_read_status(phydev);
>> +
>> +error:
>> + phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>> + return err;
>> +}
>> +
>> +/* marvell_suspend_fiber
>> + *
>> + * Some Marvell's phys have two modes: fiber and copper.
>> + * Both need to be suspended
>> + */
>> +static int marvell_suspend_fiber(struct phy_device *phydev)
>> +{
>> + int err;
>> +
>> + /* Suspend the fiber mode first */
>> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
>> + if (err < 0)
>> + goto error;
>> +
>> + err = genphy_suspend(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_suspend(phydev);
>> +
>> +error:
>> + phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
>> + return err;
>> +}
>
> 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.
>> +
>> +/* 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.
Each interface are resumed / suspended once by these functions.
>
>> @@ -1130,6 +1416,11 @@ static u64 marvell_get_stat(struct phy_device
>> *phydev, int i)
>> int err, oldpage, val;
>> u64 ret;
>>
>> + if (!(phydev->supported & SUPPORTED_FIBRE)) {
>> + if (strstr(marvell_hw_stats[i].string, "fiber"))
>> + return 0;
>
> I think a better solution is for marvell_get_sset_count() to return 2
> or 3 depending on phydev->supported & SUPPORTED_FIBRE.
Ok.
Thanks for all of your comments, I will fix that. However, I need some
additional answers to do that entirely. :)
Regards.
Charles-Antoine Couret
