Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Andrew Lunn]

> Do i need to change Ethtool application and submit along with 
> downshift driver patch (i.e. ethtool.c, phy.c, cpsw.c and mscc.c changes) ? 

Yes, please submit two patchset. One patchset for ethtool and its man
page and a second patchset for all the kernel changes.

 Thanks
Andrew

Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Andrew,

On Tue, Oct 18, 2016 at 12:58:20PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
> 
> 
> > > In fact, this looks a lot like netdev features (e.g: checksum
> > > offload), and there seems to be some commonality here between at
> > > least Marvell and Microsemi (for the faster link down reporting),
> > > so maybe we should start adding PHY features similar to netdev
> > > features?
> 
> > Sure.
> >
> > I would like add one flag in phy_device structure:
> > u64 phy_features;
> >
> > In phy_driver structure, i would like to add 2 function pointer as
> >
> > int (*phy_featues_set)(struct phy_device *phydev);
> > int (*phy_featues_get)(struct phy_device *phydev);
> >
> > All the PHY specific features i.e. Fast link failure -2, Downshift, 
> > Loopback etc
> > are the case in feature_set/feature_get functions.
> 
> Please follow how the ethertool features are implemented. So you need
> to extend net/core/ethtool.c to pass these call down, etc.
> 

Sure. I will do.
Do i need to change Ethtool application and submit along with 
downshift driver patch (i.e. ethtool.c, phy.c, cpsw.c and mscc.c changes) ? 
>Andrew

---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Andrew Lunn]

> > In fact, this looks a lot like netdev features (e.g: checksum
> > offload), and there seems to be some commonality here between at
> > least Marvell and Microsemi (for the faster link down reporting),
> > so maybe we should start adding PHY features similar to netdev
> > features?

> Sure. 
> 
> I would like add one flag in phy_device structure:
> u64 phy_features;
> 
> In phy_driver structure, i would like to add 2 function pointer as
> 
> int (*phy_featues_set)(struct phy_device *phydev);
> int (*phy_featues_get)(struct phy_device *phydev);
> 
> All the PHY specific features i.e. Fast link failure -2, Downshift, Loopback 
> etc
> are the case in feature_set/feature_get functions.

Please follow how the ethertool features are implemented. So you need
to extend net/core/ethtool.c to pass these call down, etc.

   Andrew

Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Florian,

Thank you for review comments.

On Mon, Oct 17, 2016 at 05:51:11AM -0700, Florian Fainelli wrote:
> EXTERNAL EMAIL
> 
> 
> On October 17, 2016 1:13:14 AM PDT, Raju Lakkaraju 
>  wrote:
> >Hi Andrew,
> >
> >Thank you for code review and comments.
> >
> >On Fri, Oct 14, 2016 at 02:02:28PM +0200, Andrew Lunn wrote:
> >> EXTERNAL EMAIL
> >>
> >>
> >> > On Fri, Oct 14, 2016 at 05:10:33PM +0530, Raju Lakkaraju wrote:
> >> > From: Raju Lakkaraju 
> >> >
> >> > VSC8531 Fast Link Failure 2 feature enables the PHY to indicate the
> >> > onset of a potential link failure in < 100 usec for 100BASE-TX
> >> > operation. FLF2 is supported through the MDINT (active low) pin.
> >>
> >> Is the MDINT pin specific to this feature, or a general interrupt
> >pin?
> >>
> >
> >MDINT pin is general interrupt. MDINT pin share the interrupt with
> >FLF2 along with another 13 interrupts.
> >
> >> Device tree is used to describe the hardware. It should not really
> >> describe software or configuration. But the borders are a bit
> >> fluffly. Signal edge rates is near to hardware. This is a lot more
> >> towards configuration. So i'm not sure a device tree property is the
> >> correct way to describe this.
> >>
> >> This is also a feature i know other PHYs support. The Marvell PHY has
> >> a "Metro Ethernet" extension which allows it to report link failures
> >> for 1000BASE-T in 10, 20 or 40ms, instead of the usual 750ms. So we
> >> need a generic solution other PHYs can implement.
> >>
> >> As with cable testing, i think it should be an ethtool option.
> >
> >I agree with you.
> >I thought this is one time initialization either enable or disable.
> >if customer need this feature, they can enable in DT.
> >Do you want me to implement through IOCTL instead of Device tree?
> >Do you have any other suggestions?
> 
> As indicated in the other email about speed downshift, we may want to utilize 
> ethtool's ability to modify tunable parameters (small integer, boolean, 
> values) and extend it to cover features offered by PHYs in a way that an user 
> can dynamically turn these features on or off.
> 
> In fact, this looks a lot like netdev features (e.g: checksum offload), and 
> there seems to be some commonality here between at least Marvell and 
> Microsemi (for the faster link down reporting), so maybe we should start 
> adding PHY features similar to netdev features?
> 

Sure. 

I would like add one flag in phy_device structure:
u64 phy_features;

In phy_driver structure, i would like to add 2 function pointer as

int (*phy_featues_set)(struct phy_device *phydev);
int (*phy_featues_get)(struct phy_device *phydev);

All the PHY specific features i.e. Fast link failure -2, Downshift, Loopback etc
are the case in feature_set/feature_get functions.

Is it ok?

> --
> Florian

---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Florian Fainelli]

On October 17, 2016 1:13:14 AM PDT, Raju Lakkaraju 
 wrote:
>Hi Andrew,
>
>Thank you for code review and comments.
>
>On Fri, Oct 14, 2016 at 02:02:28PM +0200, Andrew Lunn wrote:
>> EXTERNAL EMAIL
>> 
>> 
>> > On Fri, Oct 14, 2016 at 05:10:33PM +0530, Raju Lakkaraju wrote:
>> > From: Raju Lakkaraju 
>> >
>> > VSC8531 Fast Link Failure 2 feature enables the PHY to indicate the
>> > onset of a potential link failure in < 100 usec for 100BASE-TX
>> > operation. FLF2 is supported through the MDINT (active low) pin.
>> 
>> Is the MDINT pin specific to this feature, or a general interrupt
>pin?
>> 
>
>MDINT pin is general interrupt. MDINT pin share the interrupt with
>FLF2 along with another 13 interrupts.
>
>> Device tree is used to describe the hardware. It should not really
>> describe software or configuration. But the borders are a bit
>> fluffly. Signal edge rates is near to hardware. This is a lot more
>> towards configuration. So i'm not sure a device tree property is the
>> correct way to describe this.
>> 
>> This is also a feature i know other PHYs support. The Marvell PHY has
>> a "Metro Ethernet" extension which allows it to report link failures
>> for 1000BASE-T in 10, 20 or 40ms, instead of the usual 750ms. So we
>> need a generic solution other PHYs can implement.
>> 
>> As with cable testing, i think it should be an ethtool option.
>
>I agree with you.
>I thought this is one time initialization either enable or disable.
>if customer need this feature, they can enable in DT.
>Do you want me to implement through IOCTL instead of Device tree?
>Do you have any other suggestions?

As indicated in the other email about speed downshift, we may want to utilize 
ethtool's ability to modify tunable parameters (small integer, boolean, values) 
and extend it to cover features offered by PHYs in a way that an user can 
dynamically turn these features on or off.

In fact, this looks a lot like netdev features (e.g: checksum offload), and 
there seems to be some commonality here between at least Marvell and Microsemi 
(for the faster link down reporting), so maybe we should start adding PHY 
features similar to netdev features?

-- 
Florian

Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Raju Lakkaraju]

Hi Andrew,

Thank you for code review and comments.

On Fri, Oct 14, 2016 at 02:02:28PM +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL
> 
> 
> > On Fri, Oct 14, 2016 at 05:10:33PM +0530, Raju Lakkaraju wrote:
> > From: Raju Lakkaraju 
> >
> > VSC8531 Fast Link Failure 2 feature enables the PHY to indicate the
> > onset of a potential link failure in < 100 usec for 100BASE-TX
> > operation. FLF2 is supported through the MDINT (active low) pin.
> 
> Is the MDINT pin specific to this feature, or a general interrupt pin?
> 

MDINT pin is general interrupt. MDINT pin share the interrupt with
FLF2 along with another 13 interrupts.

> Device tree is used to describe the hardware. It should not really
> describe software or configuration. But the borders are a bit
> fluffly. Signal edge rates is near to hardware. This is a lot more
> towards configuration. So i'm not sure a device tree property is the
> correct way to describe this.
> 
> This is also a feature i know other PHYs support. The Marvell PHY has
> a "Metro Ethernet" extension which allows it to report link failures
> for 1000BASE-T in 10, 20 or 40ms, instead of the usual 750ms. So we
> need a generic solution other PHYs can implement.
> 
> As with cable testing, i think it should be an ethtool option.

I agree with you.
I thought this is one time initialization either enable or disable.
if customer need this feature, they can enable in DT.
Do you want me to implement through IOCTL instead of Device tree?
Do you have any other suggestions?

> 
>Andrew

---
Thanks,
Raju.

Re: [PATCH net-next 2/2] net: phy: Add Fast Link Failure - 2 set driver for Microsemi PHYs.

[Andrew Lunn]

> On Fri, Oct 14, 2016 at 05:10:33PM +0530, Raju Lakkaraju wrote:
> From: Raju Lakkaraju 
> 
> VSC8531 Fast Link Failure 2 feature enables the PHY to indicate the
> onset of a potential link failure in < 100 usec for 100BASE-TX
> operation. FLF2 is supported through the MDINT (active low) pin.

Is the MDINT pin specific to this feature, or a general interrupt pin?

Device tree is used to describe the hardware. It should not really
describe software or configuration. But the borders are a bit
fluffly. Signal edge rates is near to hardware. This is a lot more
towards configuration. So i'm not sure a device tree property is the
correct way to describe this.

This is also a feature i know other PHYs support. The Marvell PHY has
a "Metro Ethernet" extension which allows it to report link failures
for 1000BASE-T in 10, 20 or 40ms, instead of the usual 750ms. So we
need a generic solution other PHYs can implement.

As with cable testing, i think it should be an ethtool option.

   Andrew