[PATCH 0/3] ARM: NSP updates to support switch interrupts/SFP

[PATCH 0/3] ARM: NSP updates to support switch interrupts/SFP

[Florian Fainelli]

Hi all,

This patch series updates the ARM NSP DTS and BCM958625HR in order to
support the SFP connected to port 5 on these reference boards.

I will be submitting the functional changes to drivers/net/dsa/b53 once
net-next opens back up, but this is largely independent from getting
these 3 patches out.

Thank you

Florian Fainelli (3):
  ARM: dts: NSP: Enable SFP on bcm958625hr
  dt-bindings: net: dsa: Document B53 SRAB interrupts and registers
  ARM: dts: NSP: Wire up switch interrupts

 .../devicetree/bindings/net/dsa/b53.txt       | 23 ++++++++++++++
 arch/arm/boot/dts/bcm-nsp.dtsi                | 31 ++++++++++++++++++-
 arch/arm/boot/dts/bcm958625hr.dts             | 29 +++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Florian Fainelli]

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/boot/dts/bcm958625hr.dts | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/bcm958625hr.dts b/arch/arm/boot/dts/bcm958625hr.dts
index ea3fc194f8f3..05751c895a0e 100644
--- a/arch/arm/boot/dts/bcm958625hr.dts
+++ b/arch/arm/boot/dts/bcm958625hr.dts
@@ -58,6 +58,24 @@
 		open-source;
 		priority = <200>;
 	};
+
+	/* Hardware I2C block has limitations on number of bytes it can
+	 * transfer, which breaks with SFP/SFF modules.
+	 */
+	i2c1: i2c1 {
+		compatible = "i2c-gpio";
+		sda-gpios = <&gpioa 5 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+		scl-gpios = <&gpioa 4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+	};
+
+	sfp: sfp {
+		compatible = "sff,sfp";
+		i2c-bus = <&i2c1>;
+		mod-def0-gpios = <&gpioa 28 GPIO_ACTIVE_LOW>;
+		los-gpios = <&gpioa 24 GPIO_ACTIVE_HIGH>;
+		tx-fault-gpios = <&gpioa 30 GPIO_ACTIVE_HIGH>;
+		tx-disable-gpios = <&gpioa 26 GPIO_ACTIVE_HIGH>;
+	};
 };
 
 &amac0 {
@@ -210,6 +228,17 @@
 			reg = <4>;
 		};
 
+		port@5 {
+			label = "sfp";
+			phy-mode = "sgmii";
+			reg = <5>;
+			sfp = <&sfp>;
+			fixed-link {
+				speed = <1000>;
+				full-duplex;
+			};
+		};
+
 		port@8 {
 			ethernet = <&amac2>;
 			label = "cpu";
-- 
2.17.1

Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Andrew Lunn]

> @@ -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

Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Florian Fainelli]

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?
-- 
Florian

Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Andrew Lunn]

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

Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Andrew Lunn]

> > 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

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH 1/3] ARM: dts: NSP: Enable SFP on bcm958625hr

[Russell King - ARM Linux]

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

[Florian Fainelli]

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

[PATCH 2/3] dt-bindings: net: dsa: Document B53 SRAB interrupts and registers

[Florian Fainelli]

Document the Broadcom roboswitch Switch Register Access Block interrupt
lines and additional register base addresses for port mux configuration
and SGMII status/configuration registers.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../devicetree/bindings/net/dsa/b53.txt       | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/b53.txt b/Documentation/devicetree/bindings/net/dsa/b53.txt
index 1811e1972a7a..5f1029e853b8 100644
--- a/Documentation/devicetree/bindings/net/dsa/b53.txt
+++ b/Documentation/devicetree/bindings/net/dsa/b53.txt
@@ -46,6 +46,29 @@ Required properties:
       "brcm,bcm6328-switch"
       "brcm,bcm6368-switch" and the mandatory "brcm,bcm63xx-switch"
 
+Required properties for BCM585xx/586xx/88312 SoCs:
+
+ - reg: a total of 3 register base addresses, the first one must be the
+   Switch Register Access block base, the second is the port 5/4 mux
+   configuration register and the third one is the SGMII configuration
+   and status register base address.
+
+ - interrupts: a total of 13 interrupts must be specified, in the following
+   order: port 0-5, 7-8 link status change, then the integrated PHY interrupt,
+   then the timestamping interrupt and the sleep timer interrupts for ports
+   5,7,8.
+
+Optional properties for BCM585xx/586xx/88312 SoCs:
+
+  - reg-names: a total of 3 names matching the 3 base register address, must
+    be "srab", "mux_config" and "sgmii_config";
+
+  - interrupt-names: a total of 13 names matching the 13 interrupts specified
+    must be: "link_state_p0", "link_state_p1", "link_state_p2",
+    "link_state_p3", "link_state_p4", "link_state_p5", "link_state_p7",
+    "link_state_p8", "phy", "ts", "imp_sleep_timer_p5", "imp_sleep_timer_p7",
+    "imp_sleep_timer_p8"
+
 See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
 required and optional properties.
 
-- 
2.17.1

[PATCH 3/3] ARM: dts: NSP: Wire up switch interrupts

[Florian Fainelli]

The Switch Register Access Block (SRAB) has one interrupt for link state
change on each ports (0-5, 7-8) a PHY interrupt, timestamping interrupt
and sleep timer interrupts for each management ports (5,7,8). Wire those
up so we can utilize them to speed up link resolution.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/boot/dts/bcm-nsp.dtsi | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi
index 09ba85046322..ec869a80f9ba 100644
--- a/arch/arm/boot/dts/bcm-nsp.dtsi
+++ b/arch/arm/boot/dts/bcm-nsp.dtsi
@@ -377,7 +377,36 @@
 
 		srab: srab@36000 {
 			compatible = "brcm,nsp-srab";
-			reg = <0x36000 0x1000>;
+			reg = <0x36000 0x1000>,
+			      <0x3f308 0x8>,
+			      <0x3f410 0xc>;
+			reg-names = "srab", "mux_config", "sgmii";
+			interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "link_state_p0",
+					  "link_state_p1",
+					  "link_state_p2",
+					  "link_state_p3",
+					  "link_state_p4",
+					  "link_state_p5",
+					  "link_state_p7",
+					  "link_state_p8",
+					  "phy",
+					  "ts",
+					  "imp_sleep_timer_p5",
+					  "imp_sleep_timer_p7",
+					  "imp_sleep_timer_p8";
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
2.17.1