| Message ID | 20180827200344.16158-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | ARM: NSP updates to support switch interrupts/SFP | expand |
> @@ -210,6 +228,17 @@ > reg = <4>; > }; > > + port@5 { > + label = "sfp"; > + phy-mode = "sgmii"; > + reg = <5>; > + sfp = <&sfp>; > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; Hi Florian You might want to add a comment about why you are using fixed-link and sgmii, which seems very odd. Is it even correct? Andrew
On Mon, Aug 27, 2018 at 01:52:42PM -0700, Florian Fainelli wrote: > On 08/27/2018 01:35 PM, Andrew Lunn wrote: > >> @@ -210,6 +228,17 @@ > >> reg = <4>; > >> }; > >> > >> + port@5 { > >> + label = "sfp"; > >> + phy-mode = "sgmii"; > >> + reg = <5>; > >> + sfp = <&sfp>; > >> + fixed-link { > >> + speed = <1000>; > >> + full-duplex; > >> + }; > > > > Hi Florian > > > > You might want to add a comment about why you are using fixed-link and > > sgmii, which seems very odd. Is it even correct? > > Probably not, this is kind of left over from before adding the sfp > phandle, but if I do remove it, and I can see the DSA slave network > device fail to initialize, likely because we destroy the PHYLINK instance. > > AFAIR, when we talked about this with Russell, I did not see why we had > to comment out the following: > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 962c4fd338ba..f3ae16dbf8d8 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1227,7 +1227,7 @@ static int dsa_slave_phy_setup(struct net_device > *slave_dev) > netdev_err(slave_dev, > "failed to connect to port %d: %d\n", > dp->index, ret); > - phylink_destroy(dp->pl); > + //phylink_destroy(dp->pl); > return ret; > } > } > > maybe you know? Hi Florian I didn't need anything like this for the mv88e6xxx. I had patches merged in -rc1 to make SFF work connected to the mv88e6390. The DT change was not merged, but it is here: https://patchwork.ozlabs.org/patch/955635/ + port@9 { + reg = <9>; + label = "sff2"; + phy-mode = "sgmii"; + managed = "in-band-status"; + sfp = <&sff2>; + }; Andrew
On 08/27/2018 01:52 PM, Florian Fainelli wrote: > On 08/27/2018 01:35 PM, Andrew Lunn wrote: >>> @@ -210,6 +228,17 @@ >>> reg = <4>; >>> }; >>> >>> + port@5 { >>> + label = "sfp"; >>> + phy-mode = "sgmii"; >>> + reg = <5>; >>> + sfp = <&sfp>; >>> + fixed-link { >>> + speed = <1000>; >>> + full-duplex; >>> + }; >> >> Hi Florian >> >> You might want to add a comment about why you are using fixed-link and >> sgmii, which seems very odd. Is it even correct? > > Probably not, this is kind of left over from before adding the sfp > phandle, but if I do remove it, and I can see the DSA slave network > device fail to initialize, likely because we destroy the PHYLINK instance. > > AFAIR, when we talked about this with Russell, I did not see why we had > to comment out the following: > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 962c4fd338ba..f3ae16dbf8d8 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1227,7 +1227,7 @@ static int dsa_slave_phy_setup(struct net_device > *slave_dev) > netdev_err(slave_dev, > "failed to connect to port %d: %d\n", > dp->index, ret); > - phylink_destroy(dp->pl); > + //phylink_destroy(dp->pl); > return ret; > } > } > > maybe you know? Stupid question, if we have a "sfp" phandle, must one also specify a managed = "in-band-status" property? Under what circumstances are not these two things implying one another (SFF maybe)? That would explain why the code path taken from phylink_of_phy_connect() would not return 0 and we would indeed fail to connect to the built-in DSA MDIO bus.
On 08/27/2018 02:09 PM, Andrew Lunn wrote: > On Mon, Aug 27, 2018 at 01:52:42PM -0700, Florian Fainelli wrote: >> On 08/27/2018 01:35 PM, Andrew Lunn wrote: >>>> @@ -210,6 +228,17 @@ >>>> reg = <4>; >>>> }; >>>> >>>> + port@5 { >>>> + label = "sfp"; >>>> + phy-mode = "sgmii"; >>>> + reg = <5>; >>>> + sfp = <&sfp>; >>>> + fixed-link { >>>> + speed = <1000>; >>>> + full-duplex; >>>> + }; >>> >>> Hi Florian >>> >>> You might want to add a comment about why you are using fixed-link and >>> sgmii, which seems very odd. Is it even correct? >> >> Probably not, this is kind of left over from before adding the sfp >> phandle, but if I do remove it, and I can see the DSA slave network >> device fail to initialize, likely because we destroy the PHYLINK instance. >> >> AFAIR, when we talked about this with Russell, I did not see why we had >> to comment out the following: >> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 962c4fd338ba..f3ae16dbf8d8 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -1227,7 +1227,7 @@ static int dsa_slave_phy_setup(struct net_device >> *slave_dev) >> netdev_err(slave_dev, >> "failed to connect to port %d: %d\n", >> dp->index, ret); >> - phylink_destroy(dp->pl); >> + //phylink_destroy(dp->pl); >> return ret; >> } >> } >> >> maybe you know? > > Hi Florian > > I didn't need anything like this for the mv88e6xxx. I had patches > merged in -rc1 to make SFF work connected to the mv88e6390. The DT > change was not merged, but it is here: > > https://patchwork.ozlabs.org/patch/955635/ > > + port@9 { > + reg = <9>; > + label = "sff2"; > + phy-mode = "sgmii"; > + managed = "in-band-status"; ^===== Yes that is what I was missing, thanks Andrew! Still not 100% sure why having a "sfp" phandle is not enough, but I suppose there are problematic cases like the ZII Devel Rev. B where we have a SFF and we are not able to auto-negotiate the fiber connection.
> > Hi Florian > > > > I didn't need anything like this for the mv88e6xxx. I had patches > > merged in -rc1 to make SFF work connected to the mv88e6390. The DT > > change was not merged, but it is here: > > > > https://patchwork.ozlabs.org/patch/955635/ > > > > + port@9 { > > + reg = <9>; > > + label = "sff2"; > > + phy-mode = "sgmii"; > > + managed = "in-band-status"; > > ^===== > > Yes that is what I was missing, thanks Andrew! Still not 100% sure why > having a "sfp" phandle is not enough, but I suppose there are > problematic cases like the ZII Devel Rev. B where we have a SFF and we > are not able to auto-negotiate the fiber connection. ZII Devel Rev. B is actually broken, should not work, but does somehow. The SFF 3 and 4 are connected to switch ports which cannot do 1000Base-X. They are using something like SGMII. So the link partner needs to be very forgiving. But it happens that the link partners i'm testing against are forgiving. SFF 1 and 2 are generally not populated. If they are, i think you need to remove a resistor, to make them work. But they are then connected to a switch port which does use 1000Base-X. Now, since ZII devel B is technically broken, i would not be too unhappy if "sfp" phandle implies managed = "in-band-status" by default, so long as we can still use fixed-link somehow. Andrew
On Mon, Aug 27, 2018 at 01:03:42PM -0700, Florian Fainelli wrote: > Enable the SFP connected to port 5 of the switch and wire up all GPIOs > to the SFP cage. Because of a hardware limitation of the i2c controller > on the iProc SoCs which prevents large i2c (> 256 bytes) transactions to > work, we use the i2c-gpio interface instead, which does not have that > limitation. This allows us to read the SFP module EEPROM, which would > not be possible otherwise since it exceeds that size during a single > read transfer. We shouldn't exceed 256 bytes, since 256 bytes is the "page" size of the EEPROM. The most that we read in one block is either ETH_MODULE_SFF_8079_LEN or (ETH_MODULE_SFF_8472_LEN - ETH_MODULE_SFF_8079_LEN), both of which result in no more than 256 byte reads.
On 08/27/2018 03:26 PM, Russell King - ARM Linux wrote: > On Mon, Aug 27, 2018 at 01:03:42PM -0700, Florian Fainelli wrote: >> Enable the SFP connected to port 5 of the switch and wire up all GPIOs >> to the SFP cage. Because of a hardware limitation of the i2c controller >> on the iProc SoCs which prevents large i2c (> 256 bytes) transactions to >> work, we use the i2c-gpio interface instead, which does not have that >> limitation. This allows us to read the SFP module EEPROM, which would >> not be possible otherwise since it exceeds that size during a single >> read transfer. > > We shouldn't exceed 256 bytes, since 256 bytes is the "page" size > of the EEPROM. The most that we read in one block is either > ETH_MODULE_SFF_8079_LEN or (ETH_MODULE_SFF_8472_LEN - > ETH_MODULE_SFF_8079_LEN), both of which result in no more than 256 > byte reads. You are right, I got things mixed up here, the controller's limitation is actually 63 bytes per transfer, I will be rewording the commit message accordingly.