Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

[Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>]

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