[PATCH mvebu-dt v2 0/6] Turris Omnia device-tree changes

[PATCH mvebu-dt v2 0/6] Turris Omnia device-tree changes

[Marek Behún]

Hi Gregory,

v2 of series with changes for Turris Omnia device tree.
It applies on mvebu-dt branch.

Changes from v1:
- added patch which adds description for switch interrupt
- removed patch adding ethernet-phy interrupt: the PHY interrupt is
  asserted by level low, but the GPIO expander driver supports only
  edge rising/falling, and even then it may not be correct when an
  interrupt storm occurs. So keep polling the PHY
- added Andrew's Reviewed-by tags

Marek

Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org

Marek Behún (6):
  ARM: dts: turris-omnia: enable HW buffer management
  ARM: dts: turris-omnia: add comphy handle to eth2
  ARM: dts: turris-omnia: describe switch interrupt
  ARM: dts: turris-omnia: add SFP node
  ARM: dts: turris-omnia: add LED controller node
  ARM: dts: turris-omnia: update ethernet-phy node and handle name

 arch/arm/boot/dts/armada-385-turris-omnia.dts | 162 +++++++++++++++++-
 1 file changed, 157 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH mvebu-dt v2 1/6] ARM: dts: turris-omnia: enable HW buffer management

[Marek Behún]

The buffer manager is available on Turris Omnia but needs to be
described in device-tree to be used.

Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index 768b6c5d2129..b6bd73d8f2ba 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -84,12 +84,23 @@ pcie@3,0 {
 	};
 };
 
+&bm {
+	status = "okay";
+};
+
+&bm_bppi {
+	status = "okay";
+};
+
 /* Connected to 88E6176 switch, port 6 */
 &eth0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&ge0_rgmii_pins>;
 	status = "okay";
 	phy-mode = "rgmii";
+	buffer-manager = <&bm>;
+	bm,pool-long = <0>;
+	bm,pool-short = <3>;
 
 	fixed-link {
 		speed = <1000>;
@@ -103,6 +114,9 @@ &eth1 {
 	pinctrl-0 = <&ge1_rgmii_pins>;
 	status = "okay";
 	phy-mode = "rgmii";
+	buffer-manager = <&bm>;
+	bm,pool-long = <1>;
+	bm,pool-short = <3>;
 
 	fixed-link {
 		speed = <1000>;
@@ -115,6 +129,9 @@ &eth2 {
 	status = "okay";
 	phy-mode = "sgmii";
 	phy = <&phy1>;
+	buffer-manager = <&bm>;
+	bm,pool-long = <2>;
+	bm,pool-short = <3>;
 };
 
 &i2c0 {
-- 
2.26.2

[PATCH mvebu-dt v2 2/6] ARM: dts: turris-omnia: add comphy handle to eth2

[Marek Behún]

The eth2 controller on Turris Omnia is connected to SerDes. For SFP to
be able to switch between 1G ans 2.5G modes the comphy link has to be
defined.

Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: f3a6a9f3704a ("ARM: dts: add description for Armada 38x ...")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index b6bd73d8f2ba..9de26c78ec4c 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -129,6 +129,7 @@ &eth2 {
 	status = "okay";
 	phy-mode = "sgmii";
 	phy = <&phy1>;
+	phys = <&comphy5 2>;
 	buffer-manager = <&bm>;
 	bm,pool-long = <2>;
 	bm,pool-short = <3>;
-- 
2.26.2

Re: [PATCH mvebu-dt v2 2/6] ARM: dts: turris-omnia: add comphy handle to eth2

[Andreas Färber]

On 14.11.20 19:32, Marek Behún wrote:
> The eth2 controller on Turris Omnia is connected to SerDes. For SFP to
> be able to switch between 1G ans 2.5G modes the comphy link has to be

Typo "ans" is still present in v2, despite pointed out by Andrew.

> defined.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: f3a6a9f3704a ("ARM: dts: add description for Armada 38x ...")
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  arch/arm/boot/dts/armada-385-turris-omnia.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> index b6bd73d8f2ba..9de26c78ec4c 100644
> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> @@ -129,6 +129,7 @@ &eth2 {
>  	status = "okay";
>  	phy-mode = "sgmii";
>  	phy = <&phy1>;
> +	phys = <&comphy5 2>;
>  	buffer-manager = <&bm>;
>  	bm,pool-long = <2>;
>  	bm,pool-short = <3>;

Property matches what I've come up with,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

[PATCH mvebu-dt v2 3/6] ARM: dts: turris-omnia: describe switch interrupt

[Marek Behún]

Describe switch interrupt for Turris Omnia so that the CPU does not have
to poll the switch. We also need to to set mpp45 pin to gpio function
for this.

Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index 9de26c78ec4c..7ccebf7d1757 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -260,13 +260,18 @@ phy1: phy@1 {
 
 	/* Switch MV88E6176 at address 0x10 */
 	switch@10 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&swint_pins>;
 		compatible = "marvell,mv88e6085";
 		#address-cells = <1>;
 		#size-cells = <0>;
-		dsa,member = <0 0>;
 
+		dsa,member = <0 0>;
 		reg = <0x10>;
 
+		interrupt-parent = <&gpio1>;
+		interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+
 		ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -319,6 +324,11 @@ pcawan_pins: pcawan-pins {
 		marvell,function = "gpio";
 	};
 
+	swint_pins: swint-pins {
+		marvell,pins = "mpp45";
+		marvell,function = "gpio";
+	};
+
 	spi0cs0_pins: spi0cs0-pins {
 		marvell,pins = "mpp25";
 		marvell,function = "spi0";
-- 
2.26.2

[PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Marek Behún]

Turris Omnia has a SFP cage that, together with WAN PHY is connected to
eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
it disabled. Until phylink has support for such multiplexor we will
leave it to U-Boot to enable SFP and disable WAN PHY at boot time
depending on whether a SFP module is present.

Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index 7ccebf7d1757..14c21cddef72 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -82,6 +82,22 @@ pcie@3,0 {
 			};
 		};
 	};
+
+	sfp: sfp {
+		compatible = "sff,sfp";
+		i2c-bus = <&sfp_i2c>;
+		tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
+		tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
+		rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
+		los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
+		mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
+
+		/*
+		 * For now this has to be enabled at boot time by U-Boot when
+		 * a SFP module is present.
+		 */
+		status = "disabled";
+	};
 };
 
 &bm {
@@ -130,6 +146,7 @@ &eth2 {
 	phy-mode = "sgmii";
 	phy = <&phy1>;
 	phys = <&comphy5 2>;
+	sfp = <&sfp>;
 	buffer-manager = <&bm>;
 	bm,pool-long = <2>;
 	bm,pool-short = <3>;
@@ -195,7 +212,7 @@ i2c@3 {
 			/* routed to PCIe2 connector (CN62A) */
 		};
 
-		i2c@4 {
+		sfp_i2c: i2c@4 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <4>;
-- 
2.26.2

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Andreas Färber]

Hi Marek,

On 14.11.20 19:32, Marek Behún wrote:
> Turris Omnia has a SFP cage that, together with WAN PHY is connected to

"an SFP"
Comma missing after PHY (or drop before together).

> eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
> it disabled. Until phylink has support for such multiplexor we will
> leave it to U-Boot to enable SFP and disable WAN PHY at boot time
> depending on whether a SFP module is present.

multiplexor vs. multiplexer may be a British thing? Thunderbird
underlines it fwiw.

> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  arch/arm/boot/dts/armada-385-turris-omnia.dts | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> index 7ccebf7d1757..14c21cddef72 100644
> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> @@ -82,6 +82,22 @@ pcie@3,0 {
>  			};
>  		};
>  	};
> +
> +	sfp: sfp {
> +		compatible = "sff,sfp";
> +		i2c-bus = <&sfp_i2c>;
> +		tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
> +		tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
> +		rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
> +		los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
> +		mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
> +
> +		/*
> +		 * For now this has to be enabled at boot time by U-Boot when
> +		 * a SFP module is present.
> +		 */
> +		status = "disabled";
> +	};
>  };
>  
>  &bm {
> @@ -130,6 +146,7 @@ &eth2 {
>  	phy-mode = "sgmii";
>  	phy = <&phy1>;
>  	phys = <&comphy5 2>;
> +	sfp = <&sfp>;
>  	buffer-manager = <&bm>;
>  	bm,pool-long = <2>;
>  	bm,pool-short = <3>;
> @@ -195,7 +212,7 @@ i2c@3 {
>  			/* routed to PCIe2 connector (CN62A) */
>  		};
>  
> -		i2c@4 {
> +		sfp_i2c: i2c@4 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			reg = <4>;

Matches what I've come up with,

Reviewed-by: Andreas Färber <afaerber@suse.de>

However, I also needed to set managed = "in-band-status" when enabling
SFP node and removing phy property. Shall we prepare it with its default
value of "auto" and add a comment? (unlike disabled -> okay,
in-band-status is longer than auto, so not sure whether it helps U-Boot,
but it may help humans.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Russell King - ARM Linux admin]

On Sat, Nov 14, 2020 at 10:36:09PM +0100, Andreas Färber wrote:
> Hi Marek,
> 
> On 14.11.20 19:32, Marek Behún wrote:
> > Turris Omnia has a SFP cage that, together with WAN PHY is connected to
> 
> "an SFP"
> Comma missing after PHY (or drop before together).
> 
> > eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
> > it disabled. Until phylink has support for such multiplexor we will
> > leave it to U-Boot to enable SFP and disable WAN PHY at boot time
> > depending on whether a SFP module is present.
> 
> multiplexor vs. multiplexer may be a British thing? Thunderbird
> underlines it fwiw.

Why doesn't someone who has a Turris Omnia come up with the support
needed for this board, such as sending a patch to add support for
this multiplexer?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Andreas Färber]

On 14.11.20 22:42, Russell King - ARM Linux admin wrote:
> On Sat, Nov 14, 2020 at 10:36:09PM +0100, Andreas Färber wrote:
>> Hi Marek,
>>
>> On 14.11.20 19:32, Marek Behún wrote:
>>> Turris Omnia has a SFP cage that, together with WAN PHY is connected to
>>
>> "an SFP"
>> Comma missing after PHY (or drop before together).
>>
>>> eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
>>> it disabled. Until phylink has support for such multiplexor we will
>>> leave it to U-Boot to enable SFP and disable WAN PHY at boot time
>>> depending on whether a SFP module is present.
>>
>> multiplexor vs. multiplexer may be a British thing? Thunderbird
>> underlines it fwiw.
> 
> Why doesn't someone who has a Turris Omnia come up with the support
> needed for this board, such as sending a patch to add support for
> this multiplexer?

Since you have me in To: Because I've already spent over a week on other
patches just to get a 2.5G SFP recognized, which still doesn't work.

Cheers,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Marek Behún]

On Sat, 14 Nov 2020 21:42:04 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Sat, Nov 14, 2020 at 10:36:09PM +0100, Andreas Färber wrote:
> > Hi Marek,
> > 
> > On 14.11.20 19:32, Marek Behún wrote:  
> > > Turris Omnia has a SFP cage that, together with WAN PHY is connected to  
> > 
> > "an SFP"
> > Comma missing after PHY (or drop before together).
> >   
> > > eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
> > > it disabled. Until phylink has support for such multiplexor we will
> > > leave it to U-Boot to enable SFP and disable WAN PHY at boot time
> > > depending on whether a SFP module is present.  
> > 
> > multiplexor vs. multiplexer may be a British thing? Thunderbird
> > underlines it fwiw.  
> 
> Why doesn't someone who has a Turris Omnia come up with the support
> needed for this board, such as sending a patch to add support for
> this multiplexer?
> 

Russell, I have this planned, but it is a bit complicated.
We discussed this maybe 1 or 2 years ago.

The thing is that phylink does not support such a thing nor is there a
simple way to add such support to it.

One problem we discussed last time is the correct DT binding. Should it
be something like this?
  eth2 {
    phy-mode = "sgmii";

    multiplexer {
      gpio = <&mod_def0_gpio>;

      option@0 {
        reg = <0>;
        phy-handle = <&phy1>;
      };

      option@1 {
        reg = <1>;
        sfp = <&sfp>;
        managed = "in-band-status";
      };
    };
  };

But who should handle this? Phylink, SFP, or maybe this should be
handled generically in OF / fwnode subsystem? I.e. a change in GPIO
value could change device's properties/children?

Also the &mod_def0_gpio is already used by the sfp node. Can this
multiplexor node also refer to it?

Marek

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Russell King - ARM Linux admin]

On Sat, Nov 14, 2020 at 11:55:50PM +0100, Marek Behún wrote:
> On Sat, 14 Nov 2020 21:42:04 +0000
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Sat, Nov 14, 2020 at 10:36:09PM +0100, Andreas Färber wrote:
> > > Hi Marek,
> > > 
> > > On 14.11.20 19:32, Marek Behún wrote:  
> > > > Turris Omnia has a SFP cage that, together with WAN PHY is connected to  
> > > 
> > > "an SFP"
> > > Comma missing after PHY (or drop before together).
> > >   
> > > > eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
> > > > it disabled. Until phylink has support for such multiplexor we will
> > > > leave it to U-Boot to enable SFP and disable WAN PHY at boot time
> > > > depending on whether a SFP module is present.  
> > > 
> > > multiplexor vs. multiplexer may be a British thing? Thunderbird
> > > underlines it fwiw.  
> > 
> > Why doesn't someone who has a Turris Omnia come up with the support
> > needed for this board, such as sending a patch to add support for
> > this multiplexer?
> > 
> 
> Russell, I have this planned, but it is a bit complicated.
> We discussed this maybe 1 or 2 years ago.
> 
> The thing is that phylink does not support such a thing nor is there a
> simple way to add such support to it.

What I'm saying is that hacking around this problem doesn't actually 
solve it, and phylink is not going to suddenly grow support for this.
It's not something I'm planning to work on adding support for.

> One problem we discussed last time is the correct DT binding. Should it
> be something like this?
>   eth2 {
>     phy-mode = "sgmii";
> 
>     multiplexer {
>       gpio = <&mod_def0_gpio>;
> 
>       option@0 {
>         reg = <0>;
>         phy-handle = <&phy1>;
>       };
> 
>       option@1 {
>         reg = <1>;
>         sfp = <&sfp>;
>         managed = "in-band-status";
>       };
>     };
>   };

See comments below.

> But who should handle this? Phylink, SFP, or maybe this should be
> handled generically in OF / fwnode subsystem? I.e. a change in GPIO
> value could change device's properties/children?

The DT folk will object to it being handled in the OF specific code, it
seems very obvious to me that would be the case. The OF specific code
is really just a library to parse the DT descriptions, it has very
little complexity to it, and it makes no decisions about what to parse
depending on external inputs. Also, it's technically infeasible when
you consider what it would take to implement it at that level.

Let's say for argument that it was implemented in the OF specific code.
What would it take? Well, when the GPIO changes state, we would have to
cause the code parsing this eth2 node to be re-run. The only way to do
that today would be to unbind the driver for eth2 and rebind it. That
will mean that anything setup with eth2 (e.g. interface renaming) would
be lost. It would also be very noisy in the kernel log, and would likely
take quite some time to process. The OF specific code would need to know
to redirect a lookup of "managed", "sfp", "phy" etc in the eth2 node
into the relative sub-node.

This is clearly not a feasible idea for something that dynamically
changes.

It doesn't make sense to try to do any of this at the SFP layer. The SFP
layer just concerns itself with the SFP cage, the inserted module, and
nothing more. It doesn't have any knowledge of what it is connected to
on the host side, nor should it.

That leaves phylink, which is told via the SFP layer when a module is
inserted or removed. It isn't told in real-time as the MOD_DEF0 signal
changes state however, only after the module has been successfully
detected. This leads to a race condition where phylink can think that
a module is not inserted, but it really is, and the link comes up on
your PHY, but that PHY is no longer connected to the ethernet
controller because MOD_DEF0 has changed state.

The only way around that is to have phylink check the state of MOD_DEF0
itself each time _before_ it uses the results of the on-board PHY.
However, phylink can't know the state of MOD_DEF0 (see below.)

> Also the &mod_def0_gpio is already used by the sfp node. Can this
> multiplexor node also refer to it?

The GPIO library does not allow multiple users of a single GPIO, so
this would not work.

I know this is not an easy problem to solve, but I don't think hacking
around the problem in the way that is being done in this patch set is
really helping.

Having an ethernet node with both a SFP cage and a phy will cause
problems when a SFP is detected containing a PHY. The two struct
phy_device pointers, one that is maintained by phylink and another in
struct net_device, will both be overwritten with the SFP PHY, losing
the on-board PHY. This will result in a memory and resource leak at
minimum.

At minimum, the first thing that needs to happen in phylink for this
is we split the pointer for the on-board PHY from the SFP module PHY,
so we can keep track of both. We then need to deal with the
struct net_device phy pointer, setting it as appropriate depending
on whether a module is inserted. That should get us to a point where
your proposed DT patch containing both the SFP and PHY can be used,
and the kernel will make the right decision, except for the MOD_DEF0
issue I mention above. Solving that is going to be hard, because it
seems specific to your platform, and I don't have an idea that would
be feasible at the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Marek Behún]

On Sat, 14 Nov 2020 22:36:09 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Hi Marek,
> 
> On 14.11.20 19:32, Marek Behún wrote:
> > Turris Omnia has a SFP cage that, together with WAN PHY is connected to  
> 
> "an SFP"
> Comma missing after PHY (or drop before together).
> 
> > eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
> > it disabled. Until phylink has support for such multiplexor we will
> > leave it to U-Boot to enable SFP and disable WAN PHY at boot time
> > depending on whether a SFP module is present.  
> 
> multiplexor vs. multiplexer may be a British thing? Thunderbird
> underlines it fwiw.
> 
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> > Cc: Jason Cooper <jason@lakedaemon.net>
> > Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> > Cc: Andreas Färber <afaerber@suse.de>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > ---
> >  arch/arm/boot/dts/armada-385-turris-omnia.dts | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > index 7ccebf7d1757..14c21cddef72 100644
> > --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > @@ -82,6 +82,22 @@ pcie@3,0 {
> >  			};
> >  		};
> >  	};
> > +
> > +	sfp: sfp {
> > +		compatible = "sff,sfp";
> > +		i2c-bus = <&sfp_i2c>;
> > +		tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
> > +		tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
> > +		rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
> > +		los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
> > +		mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
> > +
> > +		/*
> > +		 * For now this has to be enabled at boot time by U-Boot when
> > +		 * a SFP module is present.
> > +		 */
> > +		status = "disabled";
> > +	};
> >  };
> >  
> >  &bm {
> > @@ -130,6 +146,7 @@ &eth2 {
> >  	phy-mode = "sgmii";
> >  	phy = <&phy1>;
> >  	phys = <&comphy5 2>;
> > +	sfp = <&sfp>;
> >  	buffer-manager = <&bm>;
> >  	bm,pool-long = <2>;
> >  	bm,pool-short = <3>;
> > @@ -195,7 +212,7 @@ i2c@3 {
> >  			/* routed to PCIe2 connector (CN62A) */
> >  		};
> >  
> > -		i2c@4 {
> > +		sfp_i2c: i2c@4 {
> >  			#address-cells = <1>;
> >  			#size-cells = <0>;
> >  			reg = <4>;  
> 
> Matches what I've come up with,
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> However, I also needed to set managed = "in-band-status" when enabling
> SFP node and removing phy property. Shall we prepare it with its default
> value of "auto" and add a comment? (unlike disabled -> okay,
> in-band-status is longer than auto, so not sure whether it helps U-Boot,
> but it may help humans.

The idea is that for now when U-Boot detects that SFP is present, it
shall change the device tree accordingly:
  remove phy-handle
  add managed = in-band-status
  enable sfp node

This is the only way to support this reasobaly until the multiplexer is
somehow supported by kernel.

Marek

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Andreas Färber]

On 14.11.20 23:57, Marek Behún wrote:
> On Sat, 14 Nov 2020 22:36:09 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Hi Marek,
>>
>> On 14.11.20 19:32, Marek Behún wrote:
>>> Turris Omnia has a SFP cage that, together with WAN PHY is connected to  
>>
>> "an SFP"
>> Comma missing after PHY (or drop before together).
>>
>>> eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
>>> it disabled. Until phylink has support for such multiplexor we will
>>> leave it to U-Boot to enable SFP and disable WAN PHY at boot time
>>> depending on whether a SFP module is present.  
>>
>> multiplexor vs. multiplexer may be a British thing? Thunderbird
>> underlines it fwiw.
>>
>>>
>>> Signed-off-by: Marek Behún <kabel@kernel.org>
>>> Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
>>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> Cc: Jason Cooper <jason@lakedaemon.net>
>>> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
>>> Cc: Andreas Färber <afaerber@suse.de>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>  arch/arm/boot/dts/armada-385-turris-omnia.dts | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
>>> index 7ccebf7d1757..14c21cddef72 100644
>>> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
>>> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
>>> @@ -82,6 +82,22 @@ pcie@3,0 {
>>>  			};
>>>  		};
>>>  	};
>>> +
>>> +	sfp: sfp {
>>> +		compatible = "sff,sfp";
>>> +		i2c-bus = <&sfp_i2c>;
>>> +		tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
>>> +		tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
>>> +		rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
>>> +		los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
>>> +		mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
>>> +
>>> +		/*
>>> +		 * For now this has to be enabled at boot time by U-Boot when
>>> +		 * a SFP module is present.
>>> +		 */
>>> +		status = "disabled";
>>> +	};
>>>  };
>>>  
>>>  &bm {
>>> @@ -130,6 +146,7 @@ &eth2 {
>>>  	phy-mode = "sgmii";
>>>  	phy = <&phy1>;
>>>  	phys = <&comphy5 2>;
>>> +	sfp = <&sfp>;
>>>  	buffer-manager = <&bm>;
>>>  	bm,pool-long = <2>;
>>>  	bm,pool-short = <3>;
>>> @@ -195,7 +212,7 @@ i2c@3 {
>>>  			/* routed to PCIe2 connector (CN62A) */
>>>  		};
>>>  
>>> -		i2c@4 {
>>> +		sfp_i2c: i2c@4 {
>>>  			#address-cells = <1>;
>>>  			#size-cells = <0>;
>>>  			reg = <4>;  
>>
>> Matches what I've come up with,
>>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>
>> However, I also needed to set managed = "in-band-status" when enabling
>> SFP node and removing phy property. Shall we prepare it with its default
>> value of "auto" and add a comment? (unlike disabled -> okay,
>> in-band-status is longer than auto, so not sure whether it helps U-Boot,
>> but it may help humans.
> 
> The idea is that for now when U-Boot detects that SFP is present, it
> shall change the device tree accordingly:
>   remove phy-handle
>   add managed = in-band-status
>   enable sfp node
> 
> This is the only way to support this reasobaly until the multiplexer is
> somehow supported by kernel.

I do understand the idea. My point was that you added a 4-line comment
about status property further above, but no comments about phy-handle
nor managed properties down here.

It might also be a good idea to explain in a comment why they are
mutually exclusive (mod-def0, multiplexer).

Have you done any debugging as to why we can't just leave the sfp node
enabled? Does it toggle mod-def0-gpios on probe even if no SFP is
physically present on i2c? Maybe it can be simplified over in sfp code?

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Marek Behún]

On Sun, 15 Nov 2020 00:16:48 +0100
Andreas Färber <afaerber@suse.de> wrote:

> I do understand the idea. My point was that you added a 4-line comment
> about status property further above, but no comments about phy-handle
> nor managed properties down here.
> 
> It might also be a good idea to explain in a comment why they are
> mutually exclusive (mod-def0, multiplexer).
> 
> Have you done any debugging as to why we can't just leave the sfp node
> enabled?

Current phylink code will, in such situation, try to connect both phy
and sfp. It will try to configure settings from both, depending on when
last interrupt came from PHY and when SFP module state changed. It will
break.

Also if there is another PHY in the SFP module, it won't be able to get
connected.

It is simpler to leave sfp node disabled and get full support for this
configuration into kernel first.

> Does it toggle mod-def0-gpios on probe even if no SFP is
> physically present on i2c? Maybe it can be simplified over in sfp code?

mod-deg0-gpio is not toggled by kernel. It is an input gpio. Its state
determines whether sfp module is present.

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Marek Behún]

Andreas, is the version below satisfactory?
Marek

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index 7ccebf7d1757..f7498543c9ad 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -82,6 +82,23 @@ pcie@3,0 {
 			};
 		};
 	};
+
+	sfp: sfp {
+		compatible = "sff,sfp";
+		i2c-bus = <&sfp_i2c>;
+		tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
+		tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
+		rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
+		los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
+		mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
+
+		/*
+		  * For now this has to be enabled at boot time by U-Boot when
+		  * a SFP module is present. Read more in the comment in the
+		  * eth2 node below.
+		  */
+		status = "disabled";
+	};
 };
 
 &bm {
@@ -126,10 +143,20 @@ fixed-link {
 
 /* WAN port */
 &eth2 {
+	/*
+	  * eth2 is connected via a multiplexor to both the SFP cage and to
+	  * ethernet-phy@1. The multiplexor switches the signal to SFP cage when
+	  * a SFP module is present, as determined by the mode-def0 GPIO.
+	  *
+	  * Until kernel supports this configuration properly, in case SFP module
+	  * is present, U-Boot has to enable the sfp node above, remove phy
+	  * handle and add managed = "in-band-status" property.
+	  */
 	status = "okay";
 	phy-mode = "sgmii";
 	phy = <&phy1>;
 	phys = <&comphy5 2>;
+	sfp = <&sfp>;
 	buffer-manager = <&bm>;
 	bm,pool-long = <2>;
 	bm,pool-short = <3>;
@@ -195,7 +222,7 @@ i2c@3 {
 			/* routed to PCIe2 connector (CN62A) */
 		};
 
-		i2c@4 {
+		sfp_i2c: i2c@4 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <4>;

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Andreas Färber]

On 14.11.20 22:36, Andreas Färber wrote:
> Hi Marek,
> 
> On 14.11.20 19:32, Marek Behún wrote:
>> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
>> index 7ccebf7d1757..14c21cddef72 100644
>> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
>> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
>> @@ -82,6 +82,22 @@ pcie@3,0 {
>>  			};
>>  		};
>>  	};
>> +
>> +	sfp: sfp {
>> +		compatible = "sff,sfp";
>> +		i2c-bus = <&sfp_i2c>;
>> +		tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
>> +		tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
>> +		rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
>> +		los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
>> +		mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
>> +
>> +		/*
>> +		 * For now this has to be enabled at boot time by U-Boot when
>> +		 * a SFP module is present.
>> +		 */
>> +		status = "disabled";
>> +	};
>>  };
>>  
>>  &bm {
>> @@ -130,6 +146,7 @@ &eth2 {
>>  	phy-mode = "sgmii";
>>  	phy = <&phy1>;
>>  	phys = <&comphy5 2>;
>> +	sfp = <&sfp>;
>>  	buffer-manager = <&bm>;
>>  	bm,pool-long = <2>;
>>  	bm,pool-short = <3>;
>> @@ -195,7 +212,7 @@ i2c@3 {
>>  			/* routed to PCIe2 connector (CN62A) */
>>  		};
>>  
>> -		i2c@4 {
>> +		sfp_i2c: i2c@4 {
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  			reg = <4>;
> 
> Matches what I've come up with,

Actually one minor diff: As I had pointed out to you privately before, I
set maximum-power-milliwatt to its max of 2000.

Does your hardware only support the default of 1000, or are you still
unsure of the limit that you've omitted it here? Not sure if it makes a
difference in practice, but it does show up in dmesg.

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH mvebu-dt v2 4/6] ARM: dts: turris-omnia: add SFP node

[Marek Behún]

> Actually one minor diff: As I had pointed out to you privately before, I
> set maximum-power-milliwatt to its max of 2000.
> 
> Does your hardware only support the default of 1000, or are you still
> unsure of the limit that you've omitted it here? Not sure if it makes a
> difference in practice, but it does show up in dmesg.

I am unsure, I have to ask HW guys what power is guaranteed.

Marek

[PATCH mvebu-dt v2 5/6] ARM: dts: turris-omnia: add LED controller node

[Marek Behún]

Linux now has incomplete support for the LED controller on Turris Omnia:
it can set brightness and colors for each LED.

The controller can also put these LEDs into HW controlled mode, in which
the LEDs are controlled by HW: for example the WAN LED is connected via
MCU to the WAN PHY LED pin.

The driver does not support these HW controlled modes yet, and on probe
puts the LEDs into SW controlled mode.

Add node describing the LED controller, but disable it for now.

Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index 14c21cddef72..df53cf925db6 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -12,6 +12,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
 #include "armada-385.dtsi"
 
 / {
@@ -172,6 +173,112 @@ i2c@0 {
 			/* STM32F0 command interface at address 0x2a */
 			/* leds device (in STM32F0) at address 0x2b */
 
+			led-controller@2b {
+				compatible = "cznic,turris-omnia-leds";
+				reg = <0x2b>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				/*
+				 * The driver does not support HW control mode
+				 * for the LEDs yet. Disable the LEDs for now.
+				 *
+				 * Also LED functions are not stable yet:
+				 * - there are 3 LEDs connected via MCU to PCIe
+				 *   ports. One of these ports supports mSATA.
+				 *   There is no mSATA nor PCIe function.
+				 *   For now we use LED_FUNCITION_WLAN, since
+				 *   in most cases users have wifi cards in
+				 *   these slots
+				 * - there are 2 LEDs dedicated for user: A and
+				 *   B. Again there is no such function defined.
+				 *   For now we use LED_FUNCTION_DEBUG
+				 */
+				status = "disabled";
+
+				multi-led@0 {
+					reg = <0x0>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_DEBUG;
+					function-enumerator = <2>;
+				};
+
+				multi-led@1 {
+					reg = <0x1>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_DEBUG;
+					function-enumerator = <1>;
+				};
+
+				multi-led@2 {
+					reg = <0x2>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_WLAN;
+					function-enumerator = <3>;
+				};
+
+				multi-led@3 {
+					reg = <0x3>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_WLAN;
+					function-enumerator = <2>;
+				};
+
+				multi-led@4 {
+					reg = <0x4>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_WLAN;
+					function-enumerator = <1>;
+				};
+
+				multi-led@5 {
+					reg = <0x5>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_WAN;
+				};
+
+				multi-led@6 {
+					reg = <0x6>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_LAN;
+					function-enumerator = <4>;
+				};
+
+				multi-led@7 {
+					reg = <0x7>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_LAN;
+					function-enumerator = <3>;
+				};
+
+				multi-led@8 {
+					reg = <0x8>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_LAN;
+					function-enumerator = <2>;
+				};
+
+				multi-led@9 {
+					reg = <0x9>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_LAN;
+					function-enumerator = <1>;
+				};
+
+				multi-led@a {
+					reg = <0xa>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_LAN;
+					function-enumerator = <0>;
+				};
+
+				multi-led@b {
+					reg = <0xb>;
+					color = <LED_COLOR_ID_RGB>;
+					function = LED_FUNCTION_POWER;
+				};
+			};
+
 			eeprom@54 {
 				compatible = "atmel,24c64";
 				reg = <0x54>;
-- 
2.26.2

Re: [PATCH mvebu-dt v2 5/6] ARM: dts: turris-omnia: add LED controller node

[Andreas Färber]

Hi Marek,

On 14.11.20 19:32, Marek Behún wrote:
> Linux now has incomplete support for the LED controller on Turris Omnia:
> it can set brightness and colors for each LED.
> 
> The controller can also put these LEDs into HW controlled mode, in which
> the LEDs are controlled by HW: for example the WAN LED is connected via
> MCU to the WAN PHY LED pin.
> 
> The driver does not support these HW controlled modes yet, and on probe
> puts the LEDs into SW controlled mode.
> 
> Add node describing the LED controller, but disable it for now.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  arch/arm/boot/dts/armada-385-turris-omnia.dts | 107 ++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> index 14c21cddef72..df53cf925db6 100644
> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> @@ -12,6 +12,7 @@
>  
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
>  #include "armada-385.dtsi"
>  
>  / {
> @@ -172,6 +173,112 @@ i2c@0 {
>  			/* STM32F0 command interface at address 0x2a */
>  			/* leds device (in STM32F0) at address 0x2b */

Update and move this comment now that the node documents the address?

>  
> +			led-controller@2b {
> +				compatible = "cznic,turris-omnia-leds";
> +				reg = <0x2b>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				/*
> +				 * The driver does not support HW control mode
> +				 * for the LEDs yet. Disable the LEDs for now.
> +				 *
> +				 * Also LED functions are not stable yet:
> +				 * - there are 3 LEDs connected via MCU to PCIe
> +				 *   ports. One of these ports supports mSATA.
> +				 *   There is no mSATA nor PCIe function.
> +				 *   For now we use LED_FUNCITION_WLAN, since

FUNCTION

> +				 *   in most cases users have wifi cards in
> +				 *   these slots

Doesn't U-Boot detect mSATA and switches SerDes configuration? You could
then have it set LED_FUNCTION_DISK in case of mSATA detected.

I recently didn't find any DT binding for the netdev LED trigger, but
you could set trigger-sources to associate the LEDS with PCIe nodes even
if unused. Same for the LAN LEDs and switch port nodes, if you give them
labels.

> +				 * - there are 2 LEDs dedicated for user: A and
> +				 *   B. Again there is no such function defined.
> +				 *   For now we use LED_FUNCTION_DEBUG

I'd suggest the more neutral LED_FUNCTION_INDICATOR.

Cheers,
Andreas

> +				 */
> +				status = "disabled";
> +
> +				multi-led@0 {
> +					reg = <0x0>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_DEBUG;
> +					function-enumerator = <2>;
> +				};
> +
> +				multi-led@1 {
> +					reg = <0x1>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_DEBUG;
> +					function-enumerator = <1>;
> +				};
> +
> +				multi-led@2 {
> +					reg = <0x2>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_WLAN;
> +					function-enumerator = <3>;
> +				};
> +
> +				multi-led@3 {
> +					reg = <0x3>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_WLAN;
> +					function-enumerator = <2>;
> +				};
> +
> +				multi-led@4 {
> +					reg = <0x4>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_WLAN;
> +					function-enumerator = <1>;
> +				};
> +
> +				multi-led@5 {
> +					reg = <0x5>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_WAN;
> +				};
> +
> +				multi-led@6 {
> +					reg = <0x6>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_LAN;
> +					function-enumerator = <4>;
> +				};
> +
> +				multi-led@7 {
> +					reg = <0x7>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_LAN;
> +					function-enumerator = <3>;
> +				};
> +
> +				multi-led@8 {
> +					reg = <0x8>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_LAN;
> +					function-enumerator = <2>;
> +				};
> +
> +				multi-led@9 {
> +					reg = <0x9>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_LAN;
> +					function-enumerator = <1>;
> +				};
> +
> +				multi-led@a {
> +					reg = <0xa>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_LAN;
> +					function-enumerator = <0>;
> +				};
> +
> +				multi-led@b {
> +					reg = <0xb>;
> +					color = <LED_COLOR_ID_RGB>;
> +					function = LED_FUNCTION_POWER;
> +				};
> +			};
> +
>  			eeprom@54 {
>  				compatible = "atmel,24c64";
>  				reg = <0x54>;
> 


-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH mvebu-dt v2 5/6] ARM: dts: turris-omnia: add LED controller node

[Marek Behún]

On Sat, 14 Nov 2020 22:58:45 +0100
Andreas Färber <afaerber@suse.de> wrote:

Hi Andreas,

> >  			/* STM32F0 command interface at address 0x2a */
> >  			/* leds device (in STM32F0) at address 0x2b */  
> 
> Update and move this comment now that the node documents the address?

Sounds reasonable.

> > +				 *   in most cases users have wifi cards in
> > +				 *   these slots  
> 
> Doesn't U-Boot detect mSATA and switches SerDes configuration? You could
> then have it set LED_FUNCTION_DISK in case of mSATA detected.

Yes, but this also needs to be changed by u-boot. And current version
of MCU firmware on Omnia doesn't connect the mSATA/PCI3 LED when in HW
controlled mode, so the LED has to be blinked in software anyway, for
now.

Another problem is that user can put non wireless/disk PCIe device into
this slot. What should the LED function be then? ...

> I recently didn't find any DT binding for the netdev LED trigger, but
> you could set trigger-sources to associate the LEDS with PCIe nodes even
> if unused. Same for the LAN LEDs and switch port nodes, if you give them
> labels.

I am also working with Pavel in LED subsystem. Trigger-sources does not
work currently - you can put it in device tree, but the drivers ignore
it. I am currently also working on transparent HW offloading of LED
triggers: if you set netdev trigger for lan1 LED, the system will
just enable HW controlled mode on Omnia, instead of blinking this LED
in software.

So please wait till this is done.

BTW, another issue is the devicename and function part of LED:
The `linux,default-trigger` property is deprecated in favor of
`function`.
So somehow the system should enable netdev trigger on the LED is
trigger-source points to a network device and function is compatible
with netdev trigger. What should this functions be? We have:
  LED_FUNCTION_ACTIVITY
  LED_FUNCTION_RX
  LED_FUNCTION_TX
  LED_FUNCTION_WAN
  LED_FUNCTION_LAN

Jacek thinks that
  LED_FUNCTION_ACTIVITY should be used for system activity trigger
  LED_FUNCTION_RX/TX    on uart
  LED_FUNCTION_LAN      on a network device

But I and Pavel think that if the LED_FUNCTION_ACTIVITY is used, the
trigger should be selected depending on trigger-source:
- if it points to a network device, "netdev"
- if it points to a block device, a potential "blkdev" trigger which
  does not exist now
- ...
Also RX/TX should be IMO used this way: for the netdev trigger you can
use whether it should blink only on rx, only on tx, or on both.

Please look at:
https://www.spinics.net/lists/linux-leds/msg16632.html

> 
> > +				 * - there are 2 LEDs dedicated for user: A and
> > +				 *   B. Again there is no such function defined.
> > +				 *   For now we use LED_FUNCTION_DEBUG  
> 
> I'd suggest the more neutral LED_FUNCTION_INDICATOR.

Hmm, that sounds reasonable.

Thanks.

Marek

[PATCH mvebu-dt v2 6/6] ARM: dts: turris-omnia: update ethernet-phy node and handle name

[Marek Behún]

Use property name `phy-handle` instead of the deprecated `phy` to
connect eth2 to the PHY.
Rename the node from "phy@1" to "ethernet-phy@1", since "phy@1" is
incorrect according to device-tree bindings documentation.
Also remove the "ethernet-phy-id0141.0DD1" compatible string, it is not
needed. Kernel can read the PHY identifier itself.

Signed-off-by: Marek Behún <kabel@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index df53cf925db6..694d69798685 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -145,7 +145,7 @@ fixed-link {
 &eth2 {
 	status = "okay";
 	phy-mode = "sgmii";
-	phy = <&phy1>;
+	phy-handle = <&phy1>;
 	phys = <&comphy5 2>;
 	sfp = <&sfp>;
 	buffer-manager = <&bm>;
@@ -374,9 +374,9 @@ &mdio {
 	pinctrl-0 = <&mdio_pins>;
 	status = "okay";
 
-	phy1: phy@1 {
+	phy1: ethernet-phy@1 {
 		status = "okay";
-		compatible = "ethernet-phy-id0141.0DD1", "ethernet-phy-ieee802.3-c22";
+		compatible = "ethernet-phy-ieee802.3-c22";
 		reg = <1>;
 
 		/* irq is connected to &pcawan pin 7 */
-- 
2.26.2

Re: [PATCH mvebu-dt v2 6/6] ARM: dts: turris-omnia: update ethernet-phy node and handle name

[Andreas Färber]

Hi Marek,

On 14.11.20 19:32, Marek Behún wrote:
> Use property name `phy-handle` instead of the deprecated `phy` to
> connect eth2 to the PHY.
> Rename the node from "phy@1" to "ethernet-phy@1", since "phy@1" is
> incorrect according to device-tree bindings documentation.
> Also remove the "ethernet-phy-id0141.0DD1" compatible string, it is not
> needed. Kernel can read the PHY identifier itself.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  arch/arm/boot/dts/armada-385-turris-omnia.dts | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> index df53cf925db6..694d69798685 100644
> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> @@ -145,7 +145,7 @@ fixed-link {
>  &eth2 {
>  	status = "okay";
>  	phy-mode = "sgmii";
> -	phy = <&phy1>;
> +	phy-handle = <&phy1>;
>  	phys = <&comphy5 2>;
>  	sfp = <&sfp>;
>  	buffer-manager = <&bm>;
> @@ -374,9 +374,9 @@ &mdio {
>  	pinctrl-0 = <&mdio_pins>;
>  	status = "okay";
>  
> -	phy1: phy@1 {
> +	phy1: ethernet-phy@1 {

This one I had noticed in the DT binding and verified that mainline
U-Boot does not rely on it. So ACK for this.

>  		status = "okay";

Unrelated: This property is theoretically superfluous, as unlike eth2
this node is new and doesn't overwrite a pre-existing property.

I believe in my testing overriding with status = "disabled" was not
enough to get the SFP to work, I needed to comment out the referencing
phy(-handle) property.

> -		compatible = "ethernet-phy-id0141.0DD1", "ethernet-phy-ieee802.3-c22";
> +		compatible = "ethernet-phy-ieee802.3-c22";

Does it do any harm to leave it in though?

>  		reg = <1>;
>  
>  		/* irq is connected to &pcawan pin 7 */

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Re: [PATCH mvebu-dt v2 6/6] ARM: dts: turris-omnia: update ethernet-phy node and handle name

[Marek Behún]

On Sat, 14 Nov 2020 23:04:41 +0100
Andreas Färber <afaerber@suse.de> wrote:

Hi Andreas,

> > -	phy1: phy@1 {
> > +	phy1: ethernet-phy@1 {  
> 
> This one I had noticed in the DT binding and verified that mainline
> U-Boot does not rely on it. So ACK for this.
> 
> >  		status = "okay";  
> 
> Unrelated: This property is theoretically superfluous, as unlike eth2
> this node is new and doesn't overwrite a pre-existing property.

Yes, status = "okay" is not needed if there isn't status = "disabled"
before (somewhere in include files). Maybe I will send a patch removing
all unneeded status="okay" properties in the future.

> I believe in my testing overriding with status = "disabled" was not
> enough to get the SFP to work, I needed to comment out the referencing
> phy(-handle) property.

Yes, as I wrote in reply to your question in patch 4/6, U-Boot needs to:
- remove phy-handle property
- add managed = in-band-status property
- enable sfp node

> 
> > -		compatible = "ethernet-phy-id0141.0DD1", "ethernet-phy-ieee802.3-c22";
> > +		compatible = "ethernet-phy-ieee802.3-c22";  
> 
> Does it do any harm to leave it in though?

No, but lets remove it anyway :)

Marek