Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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. -- Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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. -- Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
> > 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
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
> > 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
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
+ 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.
--
Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
+ 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.
--
Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
> >> + 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 = <>;
+ };
Andrew
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
> >> + 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 = <>;
+ };
Andrew
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
>>> + 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.
--
Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
>>> + 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.
--
Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
>> +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?
--
Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
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 = <>;
>> +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?
--
Florian
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
> @@ -210,6 +228,17 @@
> reg = <4>;
> };
>
> + port@5 {
> + label = "sfp";
> + phy-mode = "sgmii";
> + reg = <5>;
> + 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
Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr
> @@ -210,6 +228,17 @@
> reg = <4>;
> };
>
> + port@5 {
> + label = "sfp";
> + phy-mode = "sgmii";
> + reg = <5>;
> + 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
