| Message ID | 20171227221446.18459-6-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <antoine.tenart@free-electrons.com> > --- > 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 = <&cps_comphy0 1>; > }; > > +&cps_eth2 { > + /* CPS Lane 5 */ > + status = "okay"; > + phy-mode = "2500base-x"; > + /* Generic PHY, providing serdes lanes */ > + phys = <&cps_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.
Hi Russell, On Wed, Dec 27, 2017 at 10:24:01PM +0000, Russell King - ARM Linux wrote: > On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: > > > > +&cps_eth2 { > > + /* CPS Lane 5 */ > > + status = "okay"; > > + phy-mode = "2500base-x"; > > + /* Generic PHY, providing serdes lanes */ > > + phys = <&cps_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
On Wed, Dec 27, 2017 at 11:42:52PM +0100, Antoine Tenart wrote: > Hi Russell, > > On Wed, Dec 27, 2017 at 10:24:01PM +0000, Russell King - ARM Linux wrote: > > On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: > > > > > > +&cps_eth2 { > > > + /* CPS Lane 5 */ > > > + status = "okay"; > > > + phy-mode = "2500base-x"; > > > + /* Generic PHY, providing serdes lanes */ > > > + phys = <&cps_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.
On Wed, Dec 27, 2017 at 10:24:01PM +0000, 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 <antoine.tenart@free-electrons.com> > > --- > > 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 = <&cps_comphy0 1>; > > }; > > > > +&cps_eth2 { > > + /* CPS Lane 5 */ > > + status = "okay"; > > + phy-mode = "2500base-x"; > > + /* Generic PHY, providing serdes lanes */ > > + phys = <&cps_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
Hi Russell, On Wed, Dec 27, 2017 at 11:20:00PM +0000, 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
Hi Andrew, On Thu, Dec 28, 2017 at 08:46:23AM +0100, Andrew Lunn wrote: > On Wed, Dec 27, 2017 at 10:24:01PM +0000, Russell King - ARM Linux wrote: > > On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: > > > > > > +&cps_eth2 { > > > + /* CPS Lane 5 */ > > > + status = "okay"; > > > + phy-mode = "2500base-x"; > > > + /* Generic PHY, providing serdes lanes */ > > > + phys = <&cps_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
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 +0000, Russell King - ARM Linux wrote: >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: >>>> >>>> +&cps_eth2 { >>>> + /* CPS Lane 5 */ >>>> + status = "okay"; >>>> + phy-mode = "2500base-x"; >>>> + /* Generic PHY, providing serdes lanes */ >>>> + phys = <&cps_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?
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 +0000, Russell King - ARM Linux wrote: > >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: > >>>> > >>>> +&cps_eth2 { > >>>> + /* CPS Lane 5 */ > >>>> + status = "okay"; > >>>> + phy-mode = "2500base-x"; > >>>> + /* Generic PHY, providing serdes lanes */ > >>>> + phys = <&cps_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
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 +0000, Russell King - ARM Linux wrote: > > >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: > > >>>> > > >>>> +&cps_eth2 { > > >>>> + /* CPS Lane 5 */ > > >>>> + status = "okay"; > > >>>> + phy-mode = "2500base-x"; > > >>>> + /* Generic PHY, providing serdes lanes */ > > >>>> + phys = <&cps_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.
On Thu, Dec 28, 2017 at 11:04:16AM +0100, Antoine Tenart wrote: > Hi Russell, > > On Wed, Dec 27, 2017 at 11:20:00PM +0000, 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.
Hi Russell, 2017-12-28 19:46 GMT+01:00 Russell King - ARM Linux <linux@armlinux.org.uk>: > 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 +0000, Russell King - ARM Linux wrote: >> > >>> On Wed, Dec 27, 2017 at 11:14:45PM +0100, Antoine Tenart wrote: >> > >>>> >> > >>>> +&cps_eth2 { >> > >>>> + /* CPS Lane 5 */ >> > >>>> + status = "okay"; >> > >>>> + phy-mode = "2500base-x"; >> > >>>> + /* Generic PHY, providing serdes lanes */ >> > >>>> + phys = <&cps_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
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.)
Hi Russell, On Thu, Dec 28, 2017 at 06:59:21PM +0000, 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
Hi Russell and Stefan, 2017-12-29 12:38 GMT+01:00 Russell King - ARM Linux <linux@armlinux.org.uk>: > 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
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>; 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>; 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 be done by grep alone due to the includes etc. I think the only possible way would be to build all DT files, then reverse them back to dts and search those for the mvpp2 compatible strings, and then manually check each one. > 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. It's not though - there are SFP modules that are SGMII and we have no access to the PHY onboard, so the only way we know what they're doing is from the inband status sent as part of the SGMII in-band configuration. So, even when using phylink, we need the in-band stuff to work, and so we need those link IRQs. There's also additional complexities around Cisco SGMII and "extended" SGMII concerning the flow control settings - in Cisco SGMII, there are no bits in the 16-bit control word for communicating the flow control to the MAC. In extended SGMII (which appears in some Marvell devices) you can configure flow control to appear in the 16-bit control word, and in some cases, also EEE. When implemented correctly by the MAC, phylink supports the "Cisco" method when it knows that in-band AN is being used along with a PHY - it knows to read the settings from the MAC but combine the flow control with what has been read from the PHY. If this is not done, we're likely to end up with the link partner believing that FC is supported (eg, because the PHY has defaulted to advertising FC) but the local MAC having FC disabled. Note that there's another quirk as far as SGMII goes - some PHYs will not pass data until their "negotiation" (iow, passing and acknowledgement of the SGMII control word by the MAC) has completed. Disabling SGMII "AN" on the MAC causes some SGMII PHYs to apparently be in "link up" state but with no traffic flow possible in either direction. This is a particularly important point if using phylib - the temptation is to use phylib to pass the results of AN to the MAC for SGMII and disable AN on the MAC, but this is, in fact, wrong for the reason set out in this paragraph. There are bits present that allow AN bypass if it doesn't complete in a certain time, but that's an entirely separate issue - especially when there's SGMII PHYs that we have no access to! Sorting out these nuances over the life of phylink so far has been "interesting".
Hi Russell, 2017-12-30 18:31 GMT+01:00 Russell King - ARM Linux <linux@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. > > 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>; > 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>; > 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 be done by grep alone due to the > includes etc. I think the only possible way would be to build all DT > files, then reverse them back to dts and search those for the mvpp2 > compatible strings, and then manually check each one. > Thanks for detailed explanation. I agree with you - for the link IRQ, the 'managed' property should be used. IMO this change should be a part of mvpp2 -> phylink conversion patchset. Best regards, Marcin >> 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. > > It's not though - there are SFP modules that are SGMII and we have no > access to the PHY onboard, so the only way we know what they're doing > is from the inband status sent as part of the SGMII in-band > configuration. So, even when using phylink, we need the in-band > stuff to work, and so we need those link IRQs. > > There's also additional complexities around Cisco SGMII and "extended" > SGMII concerning the flow control settings - in Cisco SGMII, there > are no bits in the 16-bit control word for communicating the flow > control to the MAC. In extended SGMII (which appears in some Marvell > devices) you can configure flow control to appear in the 16-bit > control word, and in some cases, also EEE. When implemented correctly > by the MAC, phylink supports the "Cisco" method when it knows that > in-band AN is being used along with a PHY - it knows to read the > settings from the MAC but combine the flow control with what has been > read from the PHY. If this is not done, we're likely to end up with > the link partner believing that FC is supported (eg, because the PHY > has defaulted to advertising FC) but the local MAC having FC disabled. > > Note that there's another quirk as far as SGMII goes - some PHYs will > not pass data until their "negotiation" (iow, passing and acknowledgement > of the SGMII control word by the MAC) has completed. Disabling SGMII > "AN" on the MAC causes some SGMII PHYs to apparently be in "link up" > state but with no traffic flow possible in either direction. This is > a particularly important point if using phylib - the temptation is to > use phylib to pass the results of AN to the MAC for SGMII and disable > AN on the MAC, but this is, in fact, wrong for the reason set out in > this paragraph. > > There are bits present that allow AN bypass if it doesn't complete in > a certain time, but that's an entirely separate issue - especially > when there's SGMII PHYs that we have no access to! > > Sorting out these nuances over the life of phylink so far has been > "interesting". > There's ton of quirks here and there, thanks for the efforts, which help to sort them out. Marcin
> -----Original Message----- > From: Marcin Wojtas [mailto:mw@semihalf.com] > Sent: Monday, January 01, 2018 12:18 PM > To: Russell King - ARM Linux <linux@armlinux.org.uk> > Cc: Stefan Chulski <stefanc@marvell.com>; Thomas Petazzoni > <thomas.petazzoni@free-electrons.com>; Andrew Lunn <andrew@lunn.ch>; > Florian Fainelli <f.fainelli@gmail.com>; Yan Markman > <ymarkman@marvell.com>; Jason Cooper <jason@lakedaemon.net>; netdev > <netdev@vger.kernel.org>; Antoine Tenart <antoine.tenart@free- > electrons.com>; linux-kernel@vger.kernel.org; kishon@ti.com; Nadav Haklai > <nadavh@marvell.com>; Miquèl Raynal <miquel.raynal@free-electrons.com>; > Gregory Clément <gregory.clement@free-electrons.com>; David S. Miller > <davem@davemloft.net>; linux-arm-kernel@lists.infradead.org; Sebastian > Hesselbarth <sebastian.hesselbarth@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 > <linux@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.
On Mon, Jan 01, 2018 at 10:35:25AM +0000, 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.
> > > -----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.
On Wed, Jan 03, 2018 at 05:00:47PM +0000, 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.)
Russell, 2018-01-03 18:54 GMT+01:00 Russell King - ARM Linux <linux@armlinux.org.uk>: > On Wed, Jan 03, 2018 at 05:00:47PM +0000, 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
> 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
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 = <&cps_comphy0 1>; }; +&cps_eth2 { + /* CPS Lane 5 */ + status = "okay"; + phy-mode = "2500base-x"; + /* Generic PHY, providing serdes lanes */ + phys = <&cps_comphy5 2>; +}; + &cps_pinctrl { cps_spi1_pins: spi1-pins { marvell,pins = "mpp12", "mpp13", "mpp14", "mpp15", "mpp16";
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 <antoine.tenart@free-electrons.com> --- arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 8 ++++++++ 1 file changed, 8 insertions(+)