RE: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Stefan Chulski]

> Sorry, I find this very confusing.
> 
> It seems we have some people telling me that when there's no PHY described
> in DT, we use this link interrupt, and have a functional network interface
> (presumably at whatever speed.)

I give you couple of examples when we have functional network interface with
link interrupt:

1) 10G XLG mode without PHY - since XLG doesn't have auto negation mechanism
2) SGMII with in-band  auto negation
3) RGMII with auto negation done previously by boot loader or by change default 
fixed
speed same as speed on cable.

Of course correct way to do it in RGMII mode is use values provided by 
phylink/phylib.

Stefan.

> 
> I can't see this working from what you describe - what you describe basically
> tells me that when in-band autonegotiation is disabled, and we have no PHY in
> the kernel, then effectively we are in fixed-link mode - since we need to know
> what speed, duplex and flow control settings to use.
> 
> So, this means that mvpp2 should be enforcing the presence of a fixed-link
> description in DT if there is no PHY node at the moment, but it doesn't.
> 
> Instead, it looks to me like the speed and duplex settings are inherited from 
> the
> boot loader or whatever was running before - I can't find anything that
> configures MVPP2_GMAC_AUTONEG_CONFIG in this case.  That seems quite a
> mess.
> 
> Maybe I'm missing something, but I don't see how mvpp2 can be converted to
> phylink given this without causing some kind of regression, unless someone can
> be much clearer about exactly what kinds of link mvpp2 supports and how they
> work (which is basically what I asked back in
> October.)
> 
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up

Re: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Marcin Wojtas]

Russell,

2018-01-03 18:54 GMT+01:00 Russell King - ARM Linux :
> On Wed, Jan 03, 2018 at 05:00:47PM +, Stefan Chulski wrote:
>> > > > -Original Message-
>> > > > Hi Russell,
>> > > >
>> > > > Indeed. RGMII MAC behaves same way, although it shouldn't be named
>> > > > as 'in- band' to be on par with the specifications. Anyway - this
>> > > > one is rather a stub for being able to work with ACPI, so once the
>> > > > MDIO bus works there, this will be out of any concerns.
>> > >
>> > > Hi Marcin,
>> > >
>> > > This is correct.
>> > > "in-band" supported only for SGMII mode.
>> > > IRQ link interrupt depend on "in-band"' auto negation only if "in-band"'
>> > enabled.
>> > > But IRQ link interrupt could be triggered with "in-band", "out-band" or 
>> > > with
>> > specific fixed speed/duplex/flow_contol.
>> >
>> > Hi Stefan,
>> >
>> > How does this work in RGMII mode - is this handled by the PP2 polling the 
>> > PHY
>> > to get the speed, duplex and flow control settings?
>> >
>>
>> IRQ interrupt doesn't handled speed, duplex and flow control settings.
>> It's just raised if number of criterions met:
>> 1) Physical signal detected by MAC
>> 2) MAC auto negotiation succeeded(valid only auto negotiation enabled in MAC 
>> and "in-band" bypass disabled)
>>
>> So if auto negotiation mechanism disabled in MAC or bypassed, link status 
>> would changes to up and IRQ interrupt be triggered.
>>
>> In case of RGMII mode obviously we don't have "in-band" auto negotiation and 
>> "out-band" cannot be used in Kernel(due to missed locks).
>> So auto negotiation should be disabled on MAC level and 
>> speed/duplex/flow_contol would be negotiate by PHY.
>> phylink/phylib infrastructure should provide speed/duplex/flow_contol(that 
>> were agreed between PHY's) to ppv2 driver.
>
> Sorry, I find this very confusing.
>
> It seems we have some people telling me that when there's no PHY
> described in DT, we use this link interrupt, and have a functional
> network interface (presumably at whatever speed.)
>
> I can't see this working from what you describe - what you describe
> basically tells me that when in-band autonegotiation is disabled, and we
> have no PHY in the kernel, then effectively we are in fixed-link mode -
> since we need to know what speed, duplex and flow control settings to
> use.
>
> So, this means that mvpp2 should be enforcing the presence of a
> fixed-link description in DT if there is no PHY node at the moment, but
> it doesn't.
>
> Instead, it looks to me like the speed and duplex settings are inherited
> from the boot loader or whatever was running before - I can't find
> anything that configures MVPP2_GMAC_AUTONEG_CONFIG in this case.  That
> seems quite a mess.

Just 3 cents from me about the RGMII + link IRQ, which is only a
temporary solution for ACPI. It works basing on the results of UEFI HW
PHY polling and inherited GMAC settings. Once this MDIO bus / PHY
handling lands, this configuration will be out of any question and
usage in the pp2 driver.

Marcin

>
> Maybe I'm missing something, but I don't see how mvpp2 can be converted
> to phylink given this without causing some kind of regression, unless
> someone can be much clearer about exactly what kinds of link mvpp2
> supports and how they work (which is basically what I asked back in
> October.)
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
> According to speedtest.net: 8.21Mbps down 510kbps up

Re: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Wed, Jan 03, 2018 at 05:00:47PM +, Stefan Chulski wrote:
> > > > -Original Message-
> > > > Hi Russell,
> > > >
> > > > Indeed. RGMII MAC behaves same way, although it shouldn't be named
> > > > as 'in- band' to be on par with the specifications. Anyway - this
> > > > one is rather a stub for being able to work with ACPI, so once the
> > > > MDIO bus works there, this will be out of any concerns.
> > >
> > > Hi Marcin,
> > >
> > > This is correct.
> > > "in-band" supported only for SGMII mode.
> > > IRQ link interrupt depend on "in-band"' auto negation only if "in-band"'
> > enabled.
> > > But IRQ link interrupt could be triggered with "in-band", "out-band" or 
> > > with
> > specific fixed speed/duplex/flow_contol.
> > 
> > Hi Stefan,
> > 
> > How does this work in RGMII mode - is this handled by the PP2 polling the 
> > PHY
> > to get the speed, duplex and flow control settings?
> > 
> 
> IRQ interrupt doesn't handled speed, duplex and flow control settings.
> It's just raised if number of criterions met:
> 1) Physical signal detected by MAC
> 2) MAC auto negotiation succeeded(valid only auto negotiation enabled in MAC 
> and "in-band" bypass disabled)
> 
> So if auto negotiation mechanism disabled in MAC or bypassed, link status 
> would changes to up and IRQ interrupt be triggered.
> 
> In case of RGMII mode obviously we don't have "in-band" auto negotiation and 
> "out-band" cannot be used in Kernel(due to missed locks).
> So auto negotiation should be disabled on MAC level and 
> speed/duplex/flow_contol would be negotiate by PHY. 
> phylink/phylib infrastructure should provide speed/duplex/flow_contol(that 
> were agreed between PHY's) to ppv2 driver.

Sorry, I find this very confusing.

It seems we have some people telling me that when there's no PHY
described in DT, we use this link interrupt, and have a functional
network interface (presumably at whatever speed.)

I can't see this working from what you describe - what you describe
basically tells me that when in-band autonegotiation is disabled, and we
have no PHY in the kernel, then effectively we are in fixed-link mode -
since we need to know what speed, duplex and flow control settings to
use.

So, this means that mvpp2 should be enforcing the presence of a
fixed-link description in DT if there is no PHY node at the moment, but
it doesn't.

Instead, it looks to me like the speed and duplex settings are inherited
from the boot loader or whatever was running before - I can't find
anything that configures MVPP2_GMAC_AUTONEG_CONFIG in this case.  That
seems quite a mess.

Maybe I'm missing something, but I don't see how mvpp2 can be converted
to phylink given this without causing some kind of regression, unless
someone can be much clearer about exactly what kinds of link mvpp2
supports and how they work (which is basically what I asked back in
October.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

RE: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Stefan Chulski]

> > > -Original Message-
> > > Hi Russell,
> > >
> > > Indeed. RGMII MAC behaves same way, although it shouldn't be named
> > > as 'in- band' to be on par with the specifications. Anyway - this
> > > one is rather a stub for being able to work with ACPI, so once the
> > > MDIO bus works there, this will be out of any concerns.
> >
> > Hi Marcin,
> >
> > This is correct.
> > "in-band" supported only for SGMII mode.
> > IRQ link interrupt depend on "in-band"' auto negation only if "in-band"'
> enabled.
> > But IRQ link interrupt could be triggered with "in-band", "out-band" or with
> specific fixed speed/duplex/flow_contol.
> 
> Hi Stefan,
> 
> How does this work in RGMII mode - is this handled by the PP2 polling the PHY
> to get the speed, duplex and flow control settings?
> 

IRQ interrupt doesn't handled speed, duplex and flow control settings.
It's just raised if number of criterions met:
1) Physical signal detected by MAC
2) MAC auto negotiation succeeded(valid only auto negotiation enabled in MAC 
and "in-band" bypass disabled)

So if auto negotiation mechanism disabled in MAC or bypassed, link status would 
changes to up and IRQ interrupt be triggered.

In case of RGMII mode obviously we don't have "in-band" auto negotiation and 
"out-band" cannot be used in Kernel(due to missed locks).
So auto negotiation should be disabled on MAC level and 
speed/duplex/flow_contol would be negotiate by PHY. 
phylink/phylib infrastructure should provide speed/duplex/flow_contol(that were 
agreed between PHY's) to ppv2 driver.

Stefan.

Re: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Mon, Jan 01, 2018 at 10:35:25AM +, Stefan Chulski wrote:
> 
> > -Original Message-
> > Hi Russell,
> > 
> > Indeed. RGMII MAC behaves same way, although it shouldn't be named as 'in-
> > band' to be on par with the specifications. Anyway - this one is rather a 
> > stub for
> > being able to work with ACPI, so once the MDIO bus works there, this will be
> > out of any concerns.
> 
> Hi Marcin,
> 
> This is correct.
> "in-band" supported only for SGMII mode.
> IRQ link interrupt depend on "in-band"' auto negation only if "in-band"' 
> enabled.
> But IRQ link interrupt could be triggered with "in-band", "out-band" or with 
> specific fixed speed/duplex/flow_contol.

Hi Stefan,

How does this work in RGMII mode - is this handled by the PP2 polling
the PHY to get the speed, duplex and flow control settings?

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

RE: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Stefan Chulski]

> -Original Message-
> From: Marcin Wojtas [mailto:m...@semihalf.com]
> Sent: Monday, January 01, 2018 12:18 PM
> To: Russell King - ARM Linux <li...@armlinux.org.uk>
> Cc: Stefan Chulski <stef...@marvell.com>; Thomas Petazzoni
> <thomas.petazz...@free-electrons.com>; Andrew Lunn <and...@lunn.ch>;
> Florian Fainelli <f.faine...@gmail.com>; Yan Markman
> <ymark...@marvell.com>; Jason Cooper <ja...@lakedaemon.net>; netdev
> <netdev@vger.kernel.org>; Antoine Tenart <antoine.tenart@free-
> electrons.com>; linux-ker...@vger.kernel.org; kis...@ti.com; Nadav Haklai
> <nad...@marvell.com>; Miquèl Raynal <miquel.ray...@free-electrons.com>;
> Gregory Clément <gregory.clem...@free-electrons.com>; David S. Miller
> <da...@davemloft.net>; linux-arm-ker...@lists.infradead.org; Sebastian
> Hesselbarth <sebastian.hesselba...@gmail.com>
> Subject: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the
> fourth network interface
> 
> External Email
> 
> --
> Hi Russell,
> 
> 2017-12-30 18:31 GMT+01:00 Russell King - ARM Linux
> <li...@armlinux.org.uk>:
> > Hi Marcin,
> >
> > On Sat, Dec 30, 2017 at 05:34:23PM +0100, Marcin Wojtas wrote:
> >> Yes, I already split the series and will send first one right away. I
> >> will be followed by MDIO bus / PHY handling proposal, including the
> >> bits related to phylink. I'm looking forward to your opinion on that
> >> once sent.
> >
> > I'm looking forward to the patches. :)
> >
> >> This my understanding of how the PP2 HW works in terms of signalling
> >> the link interrupt:
> >>
> >> The full in-band management, similar to mvneta is supported only in
> >> the SGMII mode (1G, not sure how it looks like in 2.5G mode). Such
> >> handling is not yet implemented in the mvpp2.c
> >>
> >> 10G:
> >> The XGMII MAC (XLG) is capable of generating link status change
> >> interrupt upon information provided from the reconciliation layer
> >> (RS) of the interface.
> >>
> >> 2.5G/1G SGMII:
> >> Apart from the in-band management, the MAC is also capable of
> >> generating IRQ during link-status change.
> >>
> >> 1G RGMII:
> >> I was a bit surprised, but checked on my own - the link change IRQ
> >> can be generated here as well.
> >>
> >> In addition to above the clause 22 PHYs can be automatically polled
> >> via SMI bus and provide complete information about link status,
> >> speed, etc., reflecting it directly in GMAC status registers.
> >> However, this feature had to be disabled, in order not to conflict
> >> with SW PHY management of the phylib.
> >>
> >> Stefan, is above correct?
> >
> > This sounds very much like mvneta's 'managed = "in-band"' mode.
> 
> Indeed. RGMII MAC behaves same way, although it shouldn't be named as 'in-
> band' to be on par with the specifications. Anyway - this one is rather a 
> stub for
> being able to work with ACPI, so once the MDIO bus works there, this will be
> out of any concerns.

Hi Marcin,

This is correct.
"in-band" supported only for SGMII mode.
IRQ link interrupt depend on "in-band"' auto negation only if "in-band"' 
enabled.
But IRQ link interrupt could be triggered with "in-band", "out-band" or with 
specific fixed speed/duplex/flow_contol.

Best Regards.

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Marcin Wojtas]

Hi Russell,

2017-12-30 18:31 GMT+01:00 Russell King - ARM Linux :
> Hi Marcin,
>
> On Sat, Dec 30, 2017 at 05:34:23PM +0100, Marcin Wojtas wrote:
>> Yes, I already split the series and will send first one right away. I
>> will be followed by MDIO bus / PHY handling proposal, including the
>> bits related to phylink. I'm looking forward to your opinion on that
>> once sent.
>
> I'm looking forward to the patches. :)
>
>> This my understanding of how the PP2 HW works in terms of signalling
>> the link interrupt:
>>
>> The full in-band management, similar to mvneta is supported only in
>> the SGMII mode (1G, not sure how it looks like in 2.5G mode). Such
>> handling is not yet implemented in the mvpp2.c
>>
>> 10G:
>> The XGMII MAC (XLG) is capable of generating link status change
>> interrupt upon information provided from the reconciliation layer (RS)
>> of the interface.
>>
>> 2.5G/1G SGMII:
>> Apart from the in-band management, the MAC is also capable of
>> generating IRQ during link-status change.
>>
>> 1G RGMII:
>> I was a bit surprised, but checked on my own - the link change IRQ can
>> be generated here as well.
>>
>> In addition to above the clause 22 PHYs can be automatically polled
>> via SMI bus and provide complete information about link status, speed,
>> etc., reflecting it directly in GMAC status registers. However, this
>> feature had to be disabled, in order not to conflict with SW PHY
>> management of the phylib.
>>
>> Stefan, is above correct?
>
> This sounds very much like mvneta's 'managed = "in-band"' mode.

Indeed. RGMII MAC behaves same way, although it shouldn't be named as
'in-band' to be on par with the specifications. Anyway - this one is
rather a stub for being able to work with ACPI, so once the MDIO bus
works there, this will be out of any concerns.

>
> Having done some research earlier this month on the "2.5G SGMII" I have
> a number of comments about this:
>
> 1. Beware of "SGMII" being used as a generic term for single lane serdes
>based ethernet. Marvell seem to use this for 802.3z BASE-X in their
>code, but it is not. SGMII is a modification of 802.3z BASE-X by
>Cisco.  This leads to some confusion!
>
> 2. For Cisco SGMII running at 2.5G, PHYs such as those from Xilinx do not
>support the speed bits, because the speed is defined to be 2.5G.  IOW,
>they do not support 250Mbps or 25Mbps speeds by data replication as is
>done with 100Mbps and 10Mbps over 1G SGMII.
>
> 3. There is also 1000BASE-X upclocked to 2.5G speeds, which mvneta and
>mvpp2 both support by appropriate configuration of the comphy.  I've
>already tested this with 4.3Mbps Fiberchannel SFPs between clearfog
>and mcbin - but needing devmem2 to reconfigure the clearfog comphy.

As for 3. I tested 2.5G link on Clearfog (FreeBSD) and Armada 8040
(UEFI), so yes - the comphy is a crucial component here.

>
>> > If my guessing is correct, I have to wonder why mvpp2 invented a
>> > different way to represent this from mvneta?  This makes it much more
>> > difficult to convert mvpp2 to phylink, and it also makes it difficult
>> > to add SFP support ignoring the phylink issue (since there is no phy
>> > handle there either.)
>>
>> Doesn't SFP require the fwnode handle to the sfp node? This is what I
>> understand at least from the phylink_register_sfp.
>
> Yes, internally within phylink.  What I'm concerned about is the
> following disparity between mvneta and mvpp2 - I'll try to explain it
> more clearly with DT examples:
>
> 1.1. mvneta phy
>  {
> phy = <>;
> phy-mode = "whatever";
> };
> 1.2. mvneta fixed-link
>  {
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
> 1.3. mvneta in-band
>  {
> phy-mode = "sgmii";
> managed = "in-band-status";
> };
> 2.1. mvpp2 phy
>  {
> phy = <>;
> phy-mode = "whatever";
> };
> 2.2. mvpp2 fixed-link
>  {
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
> 2.3. mvpp2 in-band (guess)
>  {
> phy-mode = "sgmii";
> };
>
> In both cases, the representation for phy and fixed-link mode are the
> same, but the in-band are different.  In mvneta in-band, the generic
> "managed" property must be specified as specified by
> Documentation/devicetree/bindings/net/ethernet.txt.  However, for mvpp2,
> this mode is currently selected by omission of both a "phy" property and
> a "fixed-link" sub-node/property - and that goes against the description
> of the "managed" property in the ethernet.txt binding doc.
>
> Phylink won't recognise the mvpp2's style of "in-band" because phylink,
> being a piece of generic code, is written to follow the generic binding
> 

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

Hi Marcin,

On Sat, Dec 30, 2017 at 05:34:23PM +0100, Marcin Wojtas wrote:
> Yes, I already split the series and will send first one right away. I
> will be followed by MDIO bus / PHY handling proposal, including the
> bits related to phylink. I'm looking forward to your opinion on that
> once sent.

I'm looking forward to the patches. :)

> This my understanding of how the PP2 HW works in terms of signalling
> the link interrupt:
> 
> The full in-band management, similar to mvneta is supported only in
> the SGMII mode (1G, not sure how it looks like in 2.5G mode). Such
> handling is not yet implemented in the mvpp2.c
> 
> 10G:
> The XGMII MAC (XLG) is capable of generating link status change
> interrupt upon information provided from the reconciliation layer (RS)
> of the interface.
> 
> 2.5G/1G SGMII:
> Apart from the in-band management, the MAC is also capable of
> generating IRQ during link-status change.
> 
> 1G RGMII:
> I was a bit surprised, but checked on my own - the link change IRQ can
> be generated here as well.
> 
> In addition to above the clause 22 PHYs can be automatically polled
> via SMI bus and provide complete information about link status, speed,
> etc., reflecting it directly in GMAC status registers. However, this
> feature had to be disabled, in order not to conflict with SW PHY
> management of the phylib.
> 
> Stefan, is above correct?

This sounds very much like mvneta's 'managed = "in-band"' mode.

Having done some research earlier this month on the "2.5G SGMII" I have
a number of comments about this:

1. Beware of "SGMII" being used as a generic term for single lane serdes
   based ethernet. Marvell seem to use this for 802.3z BASE-X in their
   code, but it is not. SGMII is a modification of 802.3z BASE-X by
   Cisco.  This leads to some confusion!

2. For Cisco SGMII running at 2.5G, PHYs such as those from Xilinx do not
   support the speed bits, because the speed is defined to be 2.5G.  IOW,
   they do not support 250Mbps or 25Mbps speeds by data replication as is
   done with 100Mbps and 10Mbps over 1G SGMII.

3. There is also 1000BASE-X upclocked to 2.5G speeds, which mvneta and
   mvpp2 both support by appropriate configuration of the comphy.  I've
   already tested this with 4.3Mbps Fiberchannel SFPs between clearfog
   and mcbin - but needing devmem2 to reconfigure the clearfog comphy.

> > If my guessing is correct, I have to wonder why mvpp2 invented a
> > different way to represent this from mvneta?  This makes it much more
> > difficult to convert mvpp2 to phylink, and it also makes it difficult
> > to add SFP support ignoring the phylink issue (since there is no phy
> > handle there either.)
> 
> Doesn't SFP require the fwnode handle to the sfp node? This is what I
> understand at least from the phylink_register_sfp.

Yes, internally within phylink.  What I'm concerned about is the
following disparity between mvneta and mvpp2 - I'll try to explain it
more clearly with DT examples:

1.1. mvneta phy
 {
phy = <>;
phy-mode = "whatever";
};
1.2. mvneta fixed-link
 {
fixed-link {
speed = <1000>;
full-duplex;
};
};
1.3. mvneta in-band
 {
phy-mode = "sgmii";
managed = "in-band-status";
};
2.1. mvpp2 phy
 {
phy = <>;
phy-mode = "whatever";
};
2.2. mvpp2 fixed-link
 {
fixed-link {
speed = <1000>;
full-duplex;
};
};
2.3. mvpp2 in-band (guess)
 {
phy-mode = "sgmii";
};

In both cases, the representation for phy and fixed-link mode are the
same, but the in-band are different.  In mvneta in-band, the generic
"managed" property must be specified as specified by
Documentation/devicetree/bindings/net/ethernet.txt.  However, for mvpp2,
this mode is currently selected by omission of both a "phy" property and
a "fixed-link" sub-node/property - and that goes against the description
of the "managed" property in the ethernet.txt binding doc.

Phylink won't recognise the mvpp2's style of "in-band" because phylink,
being a piece of generic code, is written to follow the generic binding
documentation, rather than accomodating driver's individual quirks.

So, if what I think is correct (basically what I've said above) there is
a problem converting mvpp2 to use phylink - any existing DT files that
use the "2.3 mvpp2 in-band" method instantly break, and I think that's
what Antoine referred to when I picked out that the previous patches
avoided using phylink when there was no "phy" node present.

However, I haven't spotted anything using the 2.3 method, but it's not
that easy to find the lack of a property amongst the maze of .dts*
files - trying to track down which use mvpp2 and which do not specify
a phy or fixed-link node can't 

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Marcin Wojtas]

Hi Russell and Stefan,

2017-12-29 12:38 GMT+01:00 Russell King - ARM Linux :
> On Fri, Dec 29, 2017 at 12:12:15PM +0100, Marcin Wojtas wrote:
>> Hi Russell,
>>
>> I see that I misspelled your email address, hence the series remained 
>> unnoticed:
>> https://lkml.org/lkml/2017/12/18/216
>>
>> In terms of the phylink support, I think the most important are:
>> * 3/8
>> https://lkml.org/lkml/2017/12/18/211
>> * 7/8
>> https://lkml.org/lkml/2017/12/18/207
>>
>> I think the way of obtaining PHY fwnode and connecting it from the
>> latter patch could be incorporated to the phylink code. Although I
>> didn't get much feedback, the whole ACPI-handling of MDIO bus and the
>> PHYs touch ACPI specification and I expect it a slower to get merged.
>> Hence my idea is following:
>> * Send v2 with ACPI supporting link-irq only in mvpp2.c
>> * Extract MDIO bus handling for ACPI and propose PHY handling
>> modifications in phylink.
>>
>> This way we may push the two things forwards in more efficient way.
>> I'm looking forward to your opinion.
>
> Agreed - as we have very few users of phylink at the moment (they're
> mostly all in external trees) we can easily change the phylink
> interfaces.  The first step is solving the ACPI representation of the
> MDIO bus and attached devices, and until that is settled, not much can
> be done.
>
> However, it seems to me that the issues of adding ACPI to mvpp2 vs
> adding phylink to mvpp2 are two entirely separate problems that don't
> really conflict with each other - since the "phy" problem afflicts
> both.
>

Yes, I already split the series and will send first one right away. I
will be followed by MDIO bus / PHY handling proposal, including the
bits related to phylink. I'm looking forward to your opinion on that
once sent.

> However, I'm not sure what this "link-irq" thing is that you talk
> about (and I suspect it's one of the things that I've been trying for
> months to find out about from Antoine when he says that there's stuff
> that mvpp2 supports that phylink doesn't.)  So, I'm left to guess, and
> I guess it's the mvpp2-variant of mvneta's in-band autonegotiation.
> Continuing to guess from the mvpp2 phylink conversion patch, this mvpp2
> variant is selected by not providing a phy handle in DT, whereas
> mvneta's variant is selected using the ethernet-standard property
> 'managed = "in-band-status"'.

This my understanding of how the PP2 HW works in terms of signalling
the link interrupt:

The full in-band management, similar to mvneta is supported only in
the SGMII mode (1G, not sure how it looks like in 2.5G mode). Such
handling is not yet implemented in the mvpp2.c

10G:
The XGMII MAC (XLG) is capable of generating link status change
interrupt upon information provided from the reconciliation layer (RS)
of the interface.

2.5G/1G SGMII:
Apart from the in-band management, the MAC is also capable of
generating IRQ during link-status change.

1G RGMII:
I was a bit surprised, but checked on my own - the link change IRQ can
be generated here as well.

In addition to above the clause 22 PHYs can be automatically polled
via SMI bus and provide complete information about link status, speed,
etc., reflecting it directly in GMAC status registers. However, this
feature had to be disabled, in order not to conflict with SW PHY
management of the phylib.

Stefan, is above correct?

>
> If my guessing is correct, I have to wonder why mvpp2 invented a
> different way to represent this from mvneta?  This makes it much more
> difficult to convert mvpp2 to phylink, and it also makes it difficult
> to add SFP support ignoring the phylink issue (since there is no phy
> handle there either.)

Doesn't SFP require the fwnode handle to the sfp node? This is what I
understand at least from the phylink_register_sfp.

Anyway, once the phylink is introduced in mvpp2.c, its presence will
simply be detected by port->phylink pointer. In such case the link IRQ
will no be used. In longer perspective, link IRQ should be used only
by ACPI and once MDIO bus is supported in generic way in this world,
it could remain as the 'last resort' option.

Best regards,
Marcin

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Antoine Tenart]

Hi Russell,

On Thu, Dec 28, 2017 at 06:59:21PM +, Russell King - ARM Linux wrote:
> On Thu, Dec 28, 2017 at 11:04:16AM +0100, Antoine Tenart wrote:
> > 
> > That's not what I remembered. You had some valid points, and others
> > related to PHY modes the driver wasn't supporting before the phylink
> > transition. My understanding of this was that you wanted a full
> > featured support while I only wanted to convert the already supported
> > modes.
> 
> You are mistaken - you can get a full refresher on where things were
> at via https://patchwork.kernel.org/patch/9963971/

I read it again and still have the same feeling. There's been a
misunderstanding at some point. Anyway, let's move forward :)

> 1. I asked for details about what mvpp2.c supports that phylink does
>not (as you indicated that there were certain things that mvpp2
>supports that phylink does not.)  I'm still awaiting a response.

I don't remember PHY modes supported in the PPv2 driver that aren't
supported in PHYLINK. I think this point is the main misunderstanding. I
thought you wanted me to support modes unsupported in the PPv2 driver
before. But you explained quite well what these comments were about
below.

So I guess this point is resolved (aka I'll have to take your comments
into account for the v2).

> 2. 25th Sept, you indicated that you would get someone to test
>an issue related to in-band AN. No results of that testing have
>been forthcoming.

That's right. I asked someone to make a test, but did not get an answer.
And because the PHYLINK patch stalled on my side I kinda forget about
it. I'll try again to have this test made.

> I am not after a full featured support, what I'm after is ensuring
> that phylink is (a) used correctly and (b) implementations using it
> are correct.  Part of that is ensuring that users don't introduce
> unexpected failure conditions.
> 
> So, when you do this in the validate() callback:
> 
>  +   phylink_set(mask, 1000baseX_Full);
> 
> and then do this in the mac_config() callback:
> 
>  +   if (!phy_interface_mode_is_rgmii(port->phy_interface) &&
>  +   port->phy_interface != PHY_INTERFACE_MODE_SGMII)
>  +   return;
> 
> and this in the link_state() callback:
> 
>  +   if (!phy_interface_mode_is_rgmii(port->phy_interface) &&
>  +   port->phy_interface != PHY_INTERFACE_MODE_SGMII)
>  +   return 0;
> 
> the result is that phylink thinks that you support 1000base-X modes,
> and it will call mac_config() asking for 1000base-X, but you silently
> ignore that, leaving the hardware configured in whatever state it was.
> That leads to a silent failure as far as the user is concerned.
> 
> So, if you do not intend to support 1000base-X initially, don't
> allow it in the validate callback until you do.
> 
> It gets worse, because the return in link_state() means that phylink
> thinks that the link is up if it has requested 1000base-X, which it
> won't be unless you've properly configured it.
> 
> It's this kind of unreliability that I was concerned about in your
> patch.  I'm not demanding "full featured implementation" but I do
> want you to use it correctly.

Thanks for the detailed explanations!

> > > What I'm most concerned about, given the bindings for comphy that
> > > have been merged, is that Free Electrons is pushing forward seemingly
> > > with no regard to the requirement that the serdes lanes are dynamically
> > > reconfigurable, and that's a basic requirement for SFP, and for the
> > > 88x3310 PHYs on the Macchiatobin platform.
> > 
> > The main idea behind the comphy driver is to provide a way to
> > reconfigure the serdes lanes at runtime. Could you develop what are
> > blocking points to properly support SFP, regarding the current comphy
> > support?
> 
> If it supports serdes lane mode reconfiguration (iow, switching between
> 1000base-X, 2500base-X, SGMII, 10G-KR), then that's all that's required.

It does, and the PPv2 driver already ask the COMPHY driver to perform
these reconfigurations (when using the 10G/1G interface on the mcbin for
example).

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Fri, Dec 29, 2017 at 12:12:15PM +0100, Marcin Wojtas wrote:
> Hi Russell,
> 
> I see that I misspelled your email address, hence the series remained 
> unnoticed:
> https://lkml.org/lkml/2017/12/18/216
> 
> In terms of the phylink support, I think the most important are:
> * 3/8
> https://lkml.org/lkml/2017/12/18/211
> * 7/8
> https://lkml.org/lkml/2017/12/18/207
> 
> I think the way of obtaining PHY fwnode and connecting it from the
> latter patch could be incorporated to the phylink code. Although I
> didn't get much feedback, the whole ACPI-handling of MDIO bus and the
> PHYs touch ACPI specification and I expect it a slower to get merged.
> Hence my idea is following:
> * Send v2 with ACPI supporting link-irq only in mvpp2.c
> * Extract MDIO bus handling for ACPI and propose PHY handling
> modifications in phylink.
> 
> This way we may push the two things forwards in more efficient way.
> I'm looking forward to your opinion.

Agreed - as we have very few users of phylink at the moment (they're
mostly all in external trees) we can easily change the phylink
interfaces.  The first step is solving the ACPI representation of the
MDIO bus and attached devices, and until that is settled, not much can
be done.

However, it seems to me that the issues of adding ACPI to mvpp2 vs
adding phylink to mvpp2 are two entirely separate problems that don't
really conflict with each other - since the "phy" problem afflicts
both.

However, I'm not sure what this "link-irq" thing is that you talk
about (and I suspect it's one of the things that I've been trying for
months to find out about from Antoine when he says that there's stuff
that mvpp2 supports that phylink doesn't.)  So, I'm left to guess, and
I guess it's the mvpp2-variant of mvneta's in-band autonegotiation.
Continuing to guess from the mvpp2 phylink conversion patch, this mvpp2
variant is selected by not providing a phy handle in DT, whereas
mvneta's variant is selected using the ethernet-standard property
'managed = "in-band-status"'.

If my guessing is correct, I have to wonder why mvpp2 invented a
different way to represent this from mvneta?  This makes it much more
difficult to convert mvpp2 to phylink, and it also makes it difficult
to add SFP support ignoring the phylink issue (since there is no phy
handle there either.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Marcin Wojtas]

Hi Russell,

2017-12-28 19:46 GMT+01:00 Russell King - ARM Linux :
> On Thu, Dec 28, 2017 at 07:27:39PM +0100, Antoine Tenart wrote:
>> Hi Florian,
>>
>> On Thu, Dec 28, 2017 at 07:02:09AM -0800, Florian Fainelli wrote:
>> > On 12/28/2017 02:05 AM, Antoine Tenart wrote:
>> > > On Thu, Dec 28, 2017 at 08:46:23AM +0100, Andrew Lunn wrote:
>> > >> On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux 
>> > >> wrote:
>> > >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
>> > 
>> >  +_eth2 {
>> >  +  /* CPS Lane 5 */
>> >  +  status = "okay";
>> >  +  phy-mode = "2500base-x";
>> >  +  /* Generic PHY, providing serdes lanes */
>> >  +  phys = <_comphy5 2>;
>> >  +};
>> >  +
>> > >>>
>> > >>> This is wrong.  This lane is connected to a SFP cage which can support
>> > >>> more than just 2500base-X.  Tying it in this way to 2500base-X means
>> > >>> that this port does not support conenctions at 1000base-X, despite
>> > >>> that's one of the most popular and more standardised speeds.
>> > >>>
>> > >>
>> > >> I agree with Russell here. SFP modules are hot pluggable, and support
>> > >> a range of interface modes. You need to query what the SFP module is
>> > >> in order to know how to configure the SERDES interface. The phylink
>> > >> infrastructure does that for you.
>> > >
>> > > Sure, I understand. We'll be able to support such interfaces only when
>> > > the phylink PPv2 support lands in.
>> >
>> > Should we expect PHYLINK support to make it as the first patch in your
>> > v2 of this patch series, or is someone else doing that?
>>
>> No, the phylink patch conflicts with Marcin's ACPI series and we agreed
>> to let him get his series merged first. And I will probably work on a
>> few other topics before having the chance to work on it. So it'll
>> probably be me doing that, but not right now.
>
> ACPI is going to be a problem with phylink for a while.  There's patches
> queued in net-next which convert phylink and SFP mostly to the fwnode
> and property based systems, but phylib and i2c do not seem to have the
> necessary bits to be able to deal with those.
>
> Specifically, in DT we have "of_find_i2c_adapter_by_node()" but afaics
> there is no equivalent in ACPI - which means in an ACPI based system
> we have no way to determine the I2C bus associated with a SFP socket,
> which is a rather fundamental issue for SFP modules.
>
> For phylib side, there's "of_phy_attach()" but again there is no
> equivalent in ACPI. This should not be that much of a problem, because
> network drivers using the DT phylib calls (eg, "of_phy_connect()") are
> already restricted by this. That may have been solved by Marcin's
> series, but I've not seen it to know.
>

I see that I misspelled your email address, hence the series remained unnoticed:
https://lkml.org/lkml/2017/12/18/216

In terms of the phylink support, I think the most important are:
* 3/8
https://lkml.org/lkml/2017/12/18/211
* 7/8
https://lkml.org/lkml/2017/12/18/207

I think the way of obtaining PHY fwnode and connecting it from the
latter patch could be incorporated to the phylink code. Although I
didn't get much feedback, the whole ACPI-handling of MDIO bus and the
PHYs touch ACPI specification and I expect it a slower to get merged.
Hence my idea is following:
* Send v2 with ACPI supporting link-irq only in mvpp2.c
* Extract MDIO bus handling for ACPI and propose PHY handling
modifications in phylink.

This way we may push the two things forwards in more efficient way.
I'm looking forward to your opinion.

Best regards,
Marcin

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Thu, Dec 28, 2017 at 11:04:16AM +0100, Antoine Tenart wrote:
> Hi Russell,
> 
> On Wed, Dec 27, 2017 at 11:20:00PM +, Russell King - ARM Linux wrote:
> > On Wed, Dec 27, 2017 at 11:42:52PM +0100, Antoine Tenart wrote:
> > > 
> > > What do you suggest to describe this in the dt, to enable a port using
> > > the current PPv2 driver?
> > 
> > I don't - I'm merely pointing out that you're bodging support for the
> > SFP cage rather than productively discussing phylink for mvpp2.
> > 
> > As far as I remember, the discussion stalled at this point:
> > 
> > - You think there's modes that mvpp2 supports that are not supportable
> >   if you use phylink.
> > 
> > - I've described what phylink supports, and I've asked you for details
> >   about what you can't support.
> 
> That's not what I remembered. You had some valid points, and others
> related to PHY modes the driver wasn't supporting before the phylink
> transition. My understanding of this was that you wanted a full
> featured support while I only wanted to convert the already supported
> modes.

You are mistaken - you can get a full refresher on where things were
at via https://patchwork.kernel.org/patch/9963971/

There are two points in that thread where discussion stopped awaiting
input:

1. I asked for details about what mvpp2.c supports that phylink does
   not (as you indicated that there were certain things that mvpp2
   supports that phylink does not.)  I'm still awaiting a response.

2. 25th Sept, you indicated that you would get someone to test
   an issue related to in-band AN. No results of that testing have
   been forthcoming.

Consequently, the ball is in your court on both these issues.

I am not after a full featured support, what I'm after is ensuring
that phylink is (a) used correctly and (b) implementations using it
are correct.  Part of that is ensuring that users don't introduce
unexpected failure conditions.

So, when you do this in the validate() callback:

 +   phylink_set(mask, 1000baseX_Full);

and then do this in the mac_config() callback:

 +   if (!phy_interface_mode_is_rgmii(port->phy_interface) &&
 +   port->phy_interface != PHY_INTERFACE_MODE_SGMII)
 +   return;

and this in the link_state() callback:

 +   if (!phy_interface_mode_is_rgmii(port->phy_interface) &&
 +   port->phy_interface != PHY_INTERFACE_MODE_SGMII)
 +   return 0;

the result is that phylink thinks that you support 1000base-X modes,
and it will call mac_config() asking for 1000base-X, but you silently
ignore that, leaving the hardware configured in whatever state it was.
That leads to a silent failure as far as the user is concerned.

So, if you do not intend to support 1000base-X initially, don't
allow it in the validate callback until you do.

It gets worse, because the return in link_state() means that phylink
thinks that the link is up if it has requested 1000base-X, which it
won't be unless you've properly configured it.

It's this kind of unreliability that I was concerned about in your
patch.  I'm not demanding "full featured implementation" but I do
want you to use it correctly.

> You're probably right about not wanting this dt patch. The non-dt
> patches still are relevant regardless of phylink being supported in the
> PPv2 driver. I'll send a v2 without the dt parts.

Thanks.

> > What I'm most concerned about, given the bindings for comphy that
> > have been merged, is that Free Electrons is pushing forward seemingly
> > with no regard to the requirement that the serdes lanes are dynamically
> > reconfigurable, and that's a basic requirement for SFP, and for the
> > 88x3310 PHYs on the Macchiatobin platform.
> 
> The main idea behind the comphy driver is to provide a way to
> reconfigure the serdes lanes at runtime. Could you develop what are
> blocking points to properly support SFP, regarding the current comphy
> support?

If it supports serdes lane mode reconfiguration (iow, switching between
1000base-X, 2500base-X, SGMII, 10G-KR), then that's all that's required.
Note that you need comphy to switch between SGMII / 10G-KR to support
the 88x3310 fully too.

Having looked deeper at this, it seems it does have the capability of
doing what's required, sorry for the noise.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Thu, Dec 28, 2017 at 07:27:39PM +0100, Antoine Tenart wrote:
> Hi Florian,
> 
> On Thu, Dec 28, 2017 at 07:02:09AM -0800, Florian Fainelli wrote:
> > On 12/28/2017 02:05 AM, Antoine Tenart wrote:
> > > On Thu, Dec 28, 2017 at 08:46:23AM +0100, Andrew Lunn wrote:
> > >> On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
> > >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
> >   
> >  +_eth2 {
> >  +  /* CPS Lane 5 */
> >  +  status = "okay";
> >  +  phy-mode = "2500base-x";
> >  +  /* Generic PHY, providing serdes lanes */
> >  +  phys = <_comphy5 2>;
> >  +};
> >  +
> > >>>
> > >>> This is wrong.  This lane is connected to a SFP cage which can support
> > >>> more than just 2500base-X.  Tying it in this way to 2500base-X means
> > >>> that this port does not support conenctions at 1000base-X, despite
> > >>> that's one of the most popular and more standardised speeds.
> > >>>
> > >>
> > >> I agree with Russell here. SFP modules are hot pluggable, and support
> > >> a range of interface modes. You need to query what the SFP module is
> > >> in order to know how to configure the SERDES interface. The phylink
> > >> infrastructure does that for you.
> > > 
> > > Sure, I understand. We'll be able to support such interfaces only when
> > > the phylink PPv2 support lands in.
> > 
> > Should we expect PHYLINK support to make it as the first patch in your
> > v2 of this patch series, or is someone else doing that?
> 
> No, the phylink patch conflicts with Marcin's ACPI series and we agreed
> to let him get his series merged first. And I will probably work on a
> few other topics before having the chance to work on it. So it'll
> probably be me doing that, but not right now.

ACPI is going to be a problem with phylink for a while.  There's patches
queued in net-next which convert phylink and SFP mostly to the fwnode
and property based systems, but phylib and i2c do not seem to have the
necessary bits to be able to deal with those.

Specifically, in DT we have "of_find_i2c_adapter_by_node()" but afaics
there is no equivalent in ACPI - which means in an ACPI based system
we have no way to determine the I2C bus associated with a SFP socket,
which is a rather fundamental issue for SFP modules.

For phylib side, there's "of_phy_attach()" but again there is no
equivalent in ACPI. This should not be that much of a problem, because
network drivers using the DT phylib calls (eg, "of_phy_connect()") are
already restricted by this. That may have been solved by Marcin's
series, but I've not seen it to know.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Antoine Tenart]

Hi Florian,

On Thu, Dec 28, 2017 at 07:02:09AM -0800, Florian Fainelli wrote:
> On 12/28/2017 02:05 AM, Antoine Tenart wrote:
> > On Thu, Dec 28, 2017 at 08:46:23AM +0100, Andrew Lunn wrote:
> >> On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
> >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
>   
>  +_eth2 {
>  +/* CPS Lane 5 */
>  +status = "okay";
>  +phy-mode = "2500base-x";
>  +/* Generic PHY, providing serdes lanes */
>  +phys = <_comphy5 2>;
>  +};
>  +
> >>>
> >>> This is wrong.  This lane is connected to a SFP cage which can support
> >>> more than just 2500base-X.  Tying it in this way to 2500base-X means
> >>> that this port does not support conenctions at 1000base-X, despite
> >>> that's one of the most popular and more standardised speeds.
> >>>
> >>
> >> I agree with Russell here. SFP modules are hot pluggable, and support
> >> a range of interface modes. You need to query what the SFP module is
> >> in order to know how to configure the SERDES interface. The phylink
> >> infrastructure does that for you.
> > 
> > Sure, I understand. We'll be able to support such interfaces only when
> > the phylink PPv2 support lands in.
> 
> Should we expect PHYLINK support to make it as the first patch in your
> v2 of this patch series, or is someone else doing that?

No, the phylink patch conflicts with Marcin's ACPI series and we agreed
to let him get his series merged first. And I will probably work on a
few other topics before having the chance to work on it. So it'll
probably be me doing that, but not right now.

This isn't an issue regarding the PPv2 and PHY patches of this series,
but yes we probably won't get the fourth network interface supported on
the mcbin during this release.

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Florian Fainelli]

On 12/28/2017 02:05 AM, Antoine Tenart wrote:
> Hi Andrew,
> 
> On Thu, Dec 28, 2017 at 08:46:23AM +0100, Andrew Lunn wrote:
>> On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
>>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
  
 +_eth2 {
 +  /* CPS Lane 5 */
 +  status = "okay";
 +  phy-mode = "2500base-x";
 +  /* Generic PHY, providing serdes lanes */
 +  phys = <_comphy5 2>;
 +};
 +
>>>
>>> This is wrong.  This lane is connected to a SFP cage which can support
>>> more than just 2500base-X.  Tying it in this way to 2500base-X means
>>> that this port does not support conenctions at 1000base-X, despite
>>> that's one of the most popular and more standardised speeds.
>>>
>>
>> I agree with Russell here. SFP modules are hot pluggable, and support
>> a range of interface modes. You need to query what the SFP module is
>> in order to know how to configure the SERDES interface. The phylink
>> infrastructure does that for you.
> 
> Sure, I understand. We'll be able to support such interfaces only when
> the phylink PPv2 support lands in.

Should we expect PHYLINK support to make it as the first patch in your
v2 of this patch series, or is someone else doing that?
-- 
Florian

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Antoine Tenart]

Hi Andrew,

On Thu, Dec 28, 2017 at 08:46:23AM +0100, Andrew Lunn wrote:
> On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
> > On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
> > >  
> > > +_eth2 {
> > > + /* CPS Lane 5 */
> > > + status = "okay";
> > > + phy-mode = "2500base-x";
> > > + /* Generic PHY, providing serdes lanes */
> > > + phys = <_comphy5 2>;
> > > +};
> > > +
> > 
> > This is wrong.  This lane is connected to a SFP cage which can support
> > more than just 2500base-X.  Tying it in this way to 2500base-X means
> > that this port does not support conenctions at 1000base-X, despite
> > that's one of the most popular and more standardised speeds.
> > 
> 
> I agree with Russell here. SFP modules are hot pluggable, and support
> a range of interface modes. You need to query what the SFP module is
> in order to know how to configure the SERDES interface. The phylink
> infrastructure does that for you.

Sure, I understand. We'll be able to support such interfaces only when
the phylink PPv2 support lands in.

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Antoine Tenart]

Hi Russell,

On Wed, Dec 27, 2017 at 11:20:00PM +, Russell King - ARM Linux wrote:
> On Wed, Dec 27, 2017 at 11:42:52PM +0100, Antoine Tenart wrote:
> > 
> > What do you suggest to describe this in the dt, to enable a port using
> > the current PPv2 driver?
> 
> I don't - I'm merely pointing out that you're bodging support for the
> SFP cage rather than productively discussing phylink for mvpp2.
> 
> As far as I remember, the discussion stalled at this point:
> 
> - You think there's modes that mvpp2 supports that are not supportable
>   if you use phylink.
> 
> - I've described what phylink supports, and I've asked you for details
>   about what you can't support.

That's not what I remembered. You had some valid points, and others
related to PHY modes the driver wasn't supporting before the phylink
transition. My understanding of this was that you wanted a full
featured support while I only wanted to convert the already supported
modes.

> Unfortunately, no details have been forthcoming, and no further
> discussion has occurred - the ball is entirely in your court to
> progress this issue since I requested information from you and that
> is where things seem to have stalled.
> 
> The result is that, with your patch, you're locking the port to 2.5G
> speeds, meaning that only 4.3Mbps Fibrechannel SFPs can be used with
> the port, and it can only be used with another device that supports
> 2.5G speeds.  You can't use a copper RJ45 module, and you can't use
> a standard 1000base-X module either in this configuration.

You're probably right about not wanting this dt patch. The non-dt
patches still are relevant regardless of phylink being supported in the
PPv2 driver. I'll send a v2 without the dt parts.

Regarding the phylink patch it's stalled for now as I have other
priorities, but I do agree it's a topic that needs to be worked on for a
proper support. I initially made a patch to be nice as it was mentioned
on a previous series, but given the feedback I got I decided to delay it
until my other tasks were completed.

So let's delay the fourth interface support on the mcbin for now.

> What I'm most concerned about, given the bindings for comphy that
> have been merged, is that Free Electrons is pushing forward seemingly
> with no regard to the requirement that the serdes lanes are dynamically
> reconfigurable, and that's a basic requirement for SFP, and for the
> 88x3310 PHYs on the Macchiatobin platform.

The main idea behind the comphy driver is to provide a way to
reconfigure the serdes lanes at runtime. Could you develop what are
blocking points to properly support SFP, regarding the current comphy
support?

Thanks,
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Andrew Lunn]

On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
> > This patch enables the fourth network interface on the Marvell
> > Macchiatobin. It is configured in the 2500Base-X PHY mode.
> > 
> > Signed-off-by: Antoine Tenart 
> > ---
> >  arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts 
> > b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> > index b3350827ee55..c51efd511324 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> > +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> > @@ -279,6 +279,14 @@
> > phys = <_comphy0 1>;
> >  };
> >  
> > +_eth2 {
> > +   /* CPS Lane 5 */
> > +   status = "okay";
> > +   phy-mode = "2500base-x";
> > +   /* Generic PHY, providing serdes lanes */
> > +   phys = <_comphy5 2>;
> > +};
> > +
> 
> This is wrong.  This lane is connected to a SFP cage which can support
> more than just 2500base-X.  Tying it in this way to 2500base-X means
> that this port does not support conenctions at 1000base-X, despite
> that's one of the most popular and more standardised speeds.
> 

Hi Antoine

I agree with Russell here. SFP modules are hot pluggable, and support
a range of interface modes. You need to query what the SFP module is
in order to know how to configure the SERDES interface. The phylink
infrastructure does that for you.

   Andrew

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Wed, Dec 27, 2017 at 11:42:52PM +0100, Antoine Tenart wrote:
> Hi Russell,
> 
> On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
> > On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
> > >  
> > > +_eth2 {
> > > + /* CPS Lane 5 */
> > > + status = "okay";
> > > + phy-mode = "2500base-x";
> > > + /* Generic PHY, providing serdes lanes */
> > > + phys = <_comphy5 2>;
> > > +};
> > > +
> > 
> > This is wrong.  This lane is connected to a SFP cage which can support
> > more than just 2500base-X.  Tying it in this way to 2500base-X means
> > that this port does not support conenctions at 1000base-X, despite
> > that's one of the most popular and more standardised speeds.
> 
> What do you suggest to describe this in the dt, to enable a port using
> the current PPv2 driver?

I don't - I'm merely pointing out that you're bodging support for the
SFP cage rather than productively discussing phylink for mvpp2.

As far as I remember, the discussion stalled at this point:

- You think there's modes that mvpp2 supports that are not supportable
  if you use phylink.

- I've described what phylink supports, and I've asked you for details
  about what you can't support.

Unfortunately, no details have been forthcoming, and no further
discussion has occurred - the ball is entirely in your court to
progress this issue since I requested information from you and that
is where things seem to have stalled.

The result is that, with your patch, you're locking the port to 2.5G
speeds, meaning that only 4.3Mbps Fibrechannel SFPs can be used with
the port, and it can only be used with another device that supports
2.5G speeds.  You can't use a copper RJ45 module, and you can't use
a standard 1000base-X module either in this configuration.

What I'm most concerned about, given the bindings for comphy that
have been merged, is that Free Electrons is pushing forward seemingly
with no regard to the requirement that the serdes lanes are dynamically
reconfigurable, and that's a basic requirement for SFP, and for the
88x3310 PHYs on the Macchiatobin platform.

So, my question to you is: what is Free Electrons plans to properly
support the ethernet ports on the Macchiatobin platform?


For those on the Cc list who don't know, phylink is part of full support
for SFP and SFP+ cages, sponsored (in terms of hardware including SFP
modules) by SolidRun on both SolidRun's Clearfog and Macchiatobin
platforms, supporting a wide range of SFP modules including:

- 1G Optical ethernet modules (duplex and bidi modules)
- 10/100/1G RJ45 modules
- 10G SFP+ modules
- 2.5Gbase-X using 4.3Mbps Fibrechannel modules
- Direct attach cables

There is work ongoing between Florian, Andrew and myself to switch DSA
to phylink, and have SFP modules working with both Marvell and Broadcom
DSA switches.

Phylink and SFP was already merged into mainline, and has been usable
(provided that the network driver is converted to phylink rather than
phylib) since 4.14-rc1.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Antoine Tenart]

Hi Russell,

On Wed, Dec 27, 2017 at 10:24:01PM +, Russell King - ARM Linux wrote:
> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
> >  
> > +_eth2 {
> > +   /* CPS Lane 5 */
> > +   status = "okay";
> > +   phy-mode = "2500base-x";
> > +   /* Generic PHY, providing serdes lanes */
> > +   phys = <_comphy5 2>;
> > +};
> > +
> 
> This is wrong.  This lane is connected to a SFP cage which can support
> more than just 2500base-X.  Tying it in this way to 2500base-X means
> that this port does not support conenctions at 1000base-X, despite
> that's one of the most popular and more standardised speeds.

What do you suggest to describe this in the dt, to enable a port using
the current PPv2 driver?

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Russell King - ARM Linux]

On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote:
> This patch enables the fourth network interface on the Marvell
> Macchiatobin. It is configured in the 2500Base-X PHY mode.
> 
> Signed-off-by: Antoine Tenart 
> ---
>  arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts 
> b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> index b3350827ee55..c51efd511324 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> @@ -279,6 +279,14 @@
>   phys = <_comphy0 1>;
>  };
>  
> +_eth2 {
> + /* CPS Lane 5 */
> + status = "okay";
> + phy-mode = "2500base-x";
> + /* Generic PHY, providing serdes lanes */
> + phys = <_comphy5 2>;
> +};
> +

This is wrong.  This lane is connected to a SFP cage which can support
more than just 2500base-X.  Tying it in this way to 2500base-X means
that this port does not support conenctions at 1000base-X, despite
that's one of the most popular and more standardised speeds.

So, I feel I have to NAK this patch in its current form.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface

[Antoine Tenart]

This patch enables the fourth network interface on the Marvell
Macchiatobin. It is configured in the 2500Base-X PHY mode.

Signed-off-by: Antoine Tenart 
---
 arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts 
b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
index b3350827ee55..c51efd511324 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
@@ -279,6 +279,14 @@
phys = <_comphy0 1>;
 };
 
+_eth2 {
+   /* CPS Lane 5 */
+   status = "okay";
+   phy-mode = "2500base-x";
+   /* Generic PHY, providing serdes lanes */
+   phys = <_comphy5 2>;
+};
+
 _pinctrl {
cps_spi1_pins: spi1-pins {
marvell,pins = "mpp12", "mpp13", "mpp14", "mpp15", "mpp16";
-- 
2.14.3