diff mbox

[RFC] ARM: dts: add support for Turris Omnia

Message ID 20161105203841.9661-1-uwe@kleine-koenig.org
State New
Headers show

Commit Message

Uwe Kleine-König Nov. 5, 2016, 8:38 p.m. UTC
This machine is an open hardware router by cz.nic driven by a
Marvell Armada 385.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---

Hello,

the following components are working:

 - WAN port
 - eMMC
 - UART0
 - USB

Still missing is support for the switch. Wireless fails to probe, didn't
debug this up to now. SFP is untested as is UART1.

The device tree on the device doesn't specify a board compatible, I added
"turris,omnia". Do I need to "register" turris in vendor-prefixes.txt for that?
@Tomas+Martin: Is this correct at all, or should I better reference cz.nic?

Best regards
Uwe

---
 arch/arm/boot/dts/Makefile                    |   1 +
 arch/arm/boot/dts/armada-385-turris-omnia.dts | 246 ++++++++++++++++++++++++++
 2 files changed, 247 insertions(+)
 create mode 100644 arch/arm/boot/dts/armada-385-turris-omnia.dts

Comments

Andrew Lunn Nov. 5, 2016, 9:04 p.m. UTC | #1
> +&mdio {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mdio_pins>;
> +	status = "okay";
> +
> +	phy1: phy@1 {
> +		status = "okay";
> +		compatible = "marvell,88E1514", "marvell,88E1510", "ethernet-phy-ieee802.3-c22";
> +		reg = <1>;
> +	};

phy.txt says:

- compatible: Compatible list, may contain
  "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for
  PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45
  specifications. If neither of these are specified, the default is to
  assume clause 22.

  If the phy's identifier is known then the list may contain an entry
  of the form: "ethernet-phy-idAAAA.BBBB" where
     AAAA - The value of the 16 bit Phy Identifier 1 register as
            4 hex digits. This is the chip vendor OUI bits 3:18
     BBBB - The value of the 16 bit Phy Identifier 2 register as
            4 hex digits. This is the chip vendor OUI bits 19:24,
            followed by 10 bits of a vendor specific ID.

  The compatible list should not contain other values than those
  listed here.

Please don't list the "marvell,*" names.

       Andrew
Andrew Lunn Nov. 5, 2016, 9:23 p.m. UTC | #2
> Still missing is support for the switch.

Is it a Marvell Switch? armada-370-rd.dts would be a good start for
the old binding? vf610-zii-dev-rev-b.dts uses the new binding.

> SFP is untested as is UART1.

UART would be unusual. They are normally i2c.

> Do I need to "register" turris in vendor-prefixes.txt for that?

Yes please.

    Andrew
Uwe Kleine-König Nov. 5, 2016, 9:27 p.m. UTC | #3
Hello Andrew,

On Sat, Nov 05, 2016 at 10:23:26PM +0100, Andrew Lunn wrote:
> > Still missing is support for the switch.
> 
> Is it a Marvell Switch? armada-370-rd.dts would be a good start for
> the old binding? vf610-zii-dev-rev-b.dts uses the new binding.

Yeah, a 88E6176. I already try to understand vf610-zii-dev-rev-b.dts. Do
you know if this driver works for the 88E6176?

> > SFP is untested as is UART1.
> 
> UART would be unusual. They are normally i2c.

I wanted to say: SFP is untested, and UART1 is untested too. Yes, SFP is
connected via i2c.

> > Do I need to "register" turris in vendor-prefixes.txt for that?
> 
> Yes please.

OK, will wait for Martin to comment what we want there. cznic or turris.

Thanks
Uwe
Andrew Lunn Nov. 5, 2016, 9:37 p.m. UTC | #4
On Sat, Nov 05, 2016 at 10:27:49PM +0100, Uwe Kleine-König wrote:
> Hello Andrew,
> 
> On Sat, Nov 05, 2016 at 10:23:26PM +0100, Andrew Lunn wrote:
> > > Still missing is support for the switch.
> > 
> > Is it a Marvell Switch? armada-370-rd.dts would be a good start for
> > the old binding? vf610-zii-dev-rev-b.dts uses the new binding.
> 
> Yeah, a 88E6176. I already try to understand vf610-zii-dev-rev-b.dts. Do
> you know if this driver works for the 88E6176?

Yes it does. 

The vf610-zii-dev-rev-b is a bit complex because it has three
switches, and an mdio mux. You should be able to transplant the
switch0: switch0@0 part into the mdio node.

	 Andrew
Uwe Kleine-König Nov. 5, 2016, 10:08 p.m. UTC | #5
On Sat, Nov 05, 2016 at 10:04:41PM +0100, Andrew Lunn wrote:
> > +&mdio {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&mdio_pins>;
> > +	status = "okay";
> > +
> > +	phy1: phy@1 {
> > +		status = "okay";
> > +		compatible = "marvell,88E1514", "marvell,88E1510", "ethernet-phy-ieee802.3-c22";
> > +		reg = <1>;
> > +	};
> 
> phy.txt says:
> 
> - compatible: Compatible list, may contain
>   "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for
>   PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45
>   specifications. If neither of these are specified, the default is to
>   assume clause 22.
> 
>   If the phy's identifier is known then the list may contain an entry
>   of the form: "ethernet-phy-idAAAA.BBBB" where
>      AAAA - The value of the 16 bit Phy Identifier 1 register as
>             4 hex digits. This is the chip vendor OUI bits 3:18
>      BBBB - The value of the 16 bit Phy Identifier 2 register as
>             4 hex digits. This is the chip vendor OUI bits 19:24,
>             followed by 10 bits of a vendor specific ID.
> 
>   The compatible list should not contain other values than those
>   listed here.
> 
> Please don't list the "marvell,*" names.

Will do for v2. arch/arm/boot/dts/keystone-* needs fixing in this
regard, too.

Best regards
Uwe
Andrew Lunn Nov. 6, 2016, 10:19 a.m. UTC | #6
> Will do for v2. arch/arm/boot/dts/keystone-* needs fixing in this
> regard, too.

There is a whitelist of compatible strings which are ignored, for
backwards compatibility. See of_mdiobus_child_is_phy(). It would be
nice to fix keystone, but it is not required.

     Andrew
Uwe Kleine-König Nov. 6, 2016, 7:32 p.m. UTC | #7
Hello,

I just noticed that I dropped the other recipents from the conversation.
I readded them also adding some context.

> > On Sun, Nov 06, 2016 at 05:28:09PM +0100, Andrew Lunn wrote:
> > > On Sun, Nov 06, 2016 at 11:45:34AM +0100, Uwe Kleine-König wrote:
> > > > +     switch@0 {
> > > > +             compatible = "marvell,mv88e6176", "marvell,mv88e6085";
> > > 
> > > All currently supported switches are compatible with the mv88e6085, in
> > > terms of probing. During the probe it can read an ID register to find
> > > out what specific switch it is, so you don't need additional details
> > > here. So please drop the marvell,mv88e6176, it will never be used.
> > 
> > That's what I know from several imx devices, for example look at
> > 
> > $ git grep imx25 arch/arm/boot/dts/imx25.dtsi
> > 
> > There are several instances of imx25-something,
> > imx$earliersoc-something. Given there is a good reason for this, I
> > wonder why it's different here.
> 
> Possibly because you cannot easily tell the variants apart using ID
> registers in the devices register space? For the switch it is very
> easy, port register 3 is the ID. It only becomes an issue when probe
> cannot find this register. There is a new generation mv88e6390 which
> i'm currently adding support for which has moved the port
> registers. So i need to add a new compatible string so probe knows
> where to look.

Even if you cannot easily distinguish between an "fsl,imx35-cspi" and an
"fsl,imx27-cspi" by inspecting hardware registers, it would be enough to
write in imx25.dtsi

	compatible = "fsl,imx35-cspi";

still we're writing

	compatible = "fsl,imx25-cspi", "fsl,imx35-cspi";

because it never hurts and is helpful when later some differences are
found and it documents the situation more accurately.

I wonder what the dt people have to say here.

> > > From what you say here, the switch is in mulit-chip mode, at address
> > > 0x10. So set the reg property to <0x10>.
> > 
> > When using 0x10 I get
> > 
> > 	mv88e6085 f1072004.mdio-mi:10: switch 0x176 probed: Marvell 88E6176, revision 1
> 
> Great.
> 
> > 
> > so now I have to find out how to use this.
> 
> Just use them as normal interfaces. ip addr add 10.42.42.42/24 dev
> lan4, brctrl addif br0 lan2, ethtool -S lan1 etc.
> 
> The whole idea is that they are just normal Linux network interfaces.

That's how I expected things to be, but

	uwe@omnia:~$ dmesg | tail
	...
	[ 2164.644589] libphy: mdio_driver_register: mv88e6085
	[ 2164.649823] mv88e6085 f1072004.mdio-mi:10: switch 0x176 probed: Marvell 88E6176, revision 1
	uwe@omnia:~$ ls /sys/class/net/
	eth0  eth1  eth2  lo

there are no additional interfaces. I will debug a bit further.

Best regards
Uwe
Martin Strbacka Nov. 7, 2016, 7:41 a.m. UTC | #8
On 5.11.2016 22:27, Uwe Kleine-König wrote:
>>> Do I need to "register" turris in vendor-prefixes.txt for that?
>> > 
>> > Yes please.
> OK, will wait for Martin to comment what we want there. cznic or turris.

Hello,

please use CZ.NIC in this case. Turris Omnia is a model name.

Thanks for your work!

Martin
tomas.hlavacek@nic.cz Nov. 14, 2016, 12:23 p.m. UTC | #9
Hello Uwe and all!

On Sat, Nov 5, 2016 at 9:38 PM, Uwe Kleine-König 
<uwe@kleine-koenig.org> wrote:
> This machine is an open hardware router by cz.nic driven by a
> Marvell Armada 385.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> 
> Hello,
> 
> the following components are working:
> 
>  - WAN port
>  - eMMC

But I not not sure about DDR50 mode. At least with kernel 4.4, that we 
use in production, we had to limit to SDR50 to overcome I/O errors and 
communication instability, if I can remember it correctly. So it might 
need more testing with the current kernel.

> 
>  - UART0
>  - USB

It is worth noting that the USB 2.0 interface of Armada 385 is wired to 
USB pinout of the last (right-most) PCIe connector. The two USB3 
interfaces routed through SERDES 1 and 3 go directly to the external 
USB connectors.

> 
> Still missing is support for the switch. Wireless fails to probe, 
> didn't
> debug this up to now. SFP is untested as is UART1.

Actually SFP is connected to SGMII interface of eth1, which is routed 
through SERDES 5. The SGMII line is shared between the SFP and metallic 
PHY 88E1514. There is a autonomous high-speed switch connected to the 
SFPDET signal from SFP cage. It disconnects the metallic SFP and 
connects SGMII to SFP once the module is connected.

The SFP is also connected to the I2C mux port 4 and to GPIO expander 
for reading/driving SFPDET, LOS, TXFLT, TXDIS signals:

&i2c0 {
        pinctrl-names = "default";
        pinctrl-0 = <&i2c0_pins>;
        status = "okay";
        clock-frequency = <100000>;

        i2cmux@70 {
                compatible = "nxp,pca9547";
                #address-cells = <1>;
                #size-cells = <0>;
                reg = <0x70>;
                status = "okay";

...

                i2c@7 {
                        /* SFP+ GPIO expander */
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <7>;

                        sfpgpio: gpio@71 {
                                compatible = "nxp,pca9538";
                                reg = <0x71>;
                                interrupt-parent = <&gpio1>;
                                interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
                                gpio-controller;
                                #gpio-cells = <2>;
                        };
                };
	};
};

We have our proprietary support hacked onto mvneta driver for 
disconnecting PHY on the fly. It is a bit nasty, so I suggest to ignore 
SFP in this DTS altogether and let's wait till "phylink based SFP 
module support" or something alike hits upstream, so we can base the 
SFP support on solid code; unless somebody has a better idea, of course.

> 
> 
> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts 
> b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> new file mode 100644
> index 000000000000..d3cd8a4d713d
> --- /dev/null
> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> @@ -0,0 +1,246 @@
...
> +
> +			/* USB part of the eSATA/USB 2.0 port */

This comment is perhaps some error inherited from my development DTS. 
We do not have any eSATA, perhaps PCIe/USB 2.0 slot.

> 
> +			usb@58000 {
> +				status = "okay";
> +			};
> +
> +
> +&eth0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&ge0_rgmii_pins>;
> +	status = "okay";
> +	phy-mode = "rgmii-id";
> +
> +	fixed-link {
> +		speed = <1000>;
> +		full-duplex;
> +	};
> +};
> +
> +&eth1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&ge1_rgmii_pins>;
> +	status = "okay";
> +	phy-mode = "rgmii-id";
> +
> +	fixed-link {
> +		speed = <1000>;
> +		full-duplex;
> +	};
> +};
> +

Actually eth0 and eth1 (both are RGMII) are connected to the 88E6176 
switch. The problem is that from what I have read so far the switch can 
not operate in DSA mode with two CPU ports. We currently operate the 
switch in "normal mode" with the eth0 and eth1 set to fixed-link 
1000/full and with proprietary driver (derived from OpenWRT switch 
drivers). I would say that these records for eth0 and eth1 are 
therefore redundant, because it does nothing without the switch support 
and it would most likely change once we have DSA driver (using only 
eth0).

Tomas
Andrew Lunn Nov. 14, 2016, 1:10 p.m. UTC | #10
> Actually SFP is connected to SGMII interface of eth1, which is
> routed through SERDES 5.

You say eth1 here. Yet lower down you say got eth0 and eth1 are
connected to the switch?

> We have our proprietary support hacked onto mvneta driver for
> disconnecting PHY on the fly. It is a bit nasty, so I suggest to
> ignore SFP in this DTS altogether and let's wait till "phylink based
> SFP module support" or something alike hits upstream, so we can base
> the SFP support on solid code;

It would be great if you could work on getting the phylink patches
into mainline. It is something i have wanted to do for a long time,
but it is too low down on my priority list to get to. The code is high
quality, so i don't think there will be too many issues. It probably
just needs splitting up into smaller batches, submitting, and working
on any comments.

> Actually eth0 and eth1 (both are RGMII) are connected to the 88E6176
> switch. The problem is that from what I have read so far the switch
> can not operate in DSA mode with two CPU ports.

Again, this is something i wanted to do, and i did have a prototype at
one point. But again, not enough time. If you have resources to work
on this, i can find my code, explain my ideas, and let you complete
it.

	Andrew
tomas.hlavacek@nic.cz Nov. 14, 2016, 2:59 p.m. UTC | #11
Hi Andrew!

(Sorry for re-posting the previous e-mail, that most likely didn't get 
through.)

On Mon, Nov 14, 2016 at 2:10 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>  Actually SFP is connected to SGMII interface of eth1, which is
>>  routed through SERDES 5.
> 
> You say eth1 here. Yet lower down you say got eth0 and eth1 are
> connected to the switch?

Oh sorry, this was a NIC name based on probing order derived from base 
address of NIC registers:

	eth1: ethernet@30000 - probes as eth0
	eth2: ethernet@34000 - probes as eth1
	eth0: ethernet@70000 - probes as eth2

It is a bit confusing. I meant eth2 in DTS. Sorry.

> 
> 
>>  We have our proprietary support hacked onto mvneta driver for
>>  disconnecting PHY on the fly. It is a bit nasty, so I suggest to
>>  ignore SFP in this DTS altogether and let's wait till "phylink based
>>  SFP module support" or something alike hits upstream, so we can base
>>  the SFP support on solid code;
> 
> It would be great if you could work on getting the phylink patches
> into mainline. It is something i have wanted to do for a long time,
> but it is too low down on my priority list to get to. The code is high
> quality, so i don't think there will be too many issues. It probably
> just needs splitting up into smaller batches, submitting, and working
> on any comments.

That is exactly what I thought when I saw the patches for the first 
time. I will try to merge the patches to the current kernel and see 
what happens. I still need to learn a lot about PHY subsystem.

> 
> 
>>  Actually eth0 and eth1 (both are RGMII) are connected to the 88E6176
>>  switch. The problem is that from what I have read so far the switch
>>  can not operate in DSA mode with two CPU ports.
> 
> Again, this is something i wanted to do, and i did have a prototype at
> one point. But again, not enough time. If you have resources to work
> on this, i can find my code, explain my ideas, and let you complete
> it.

I am definitely interested, though I didn't have time to read and 
absorb DSA yet, but I definitely want to try to hack 88E6176 support. I 
would be really grateful if you can provide some pointers and/or code 
to start from.

Thanks,
Tomas
Uwe Kleine-König Nov. 14, 2016, 8:16 p.m. UTC | #12
Hello Tomas,

On Mon, Nov 14, 2016 at 01:23:05PM +0100, tomas.hlavacek@nic.cz wrote:
> On Sat, Nov 5, 2016 at 9:38 PM, Uwe Kleine-König <uwe@kleine-koenig.org>
> wrote:
> > This machine is an open hardware router by cz.nic driven by a
> > Marvell Armada 385.
> > 
> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > ---
> > 
> > Hello,
> > 
> > the following components are working:
> > 
> >  - WAN port
> >  - eMMC
> 
> But I not not sure about DDR50 mode. At least with kernel 4.4, that we use
> in production, we had to limit to SDR50 to overcome I/O errors and
> communication instability, if I can remember it correctly. So it might need
> more testing with the current kernel.

I didn't test that extensively, but the eMMC serves my rootfs and I
didn't had any problems so far.

> > Still missing is support for the switch. Wireless fails to probe, didn't
> > debug this up to now. SFP is untested as is UART1.
> 
> Actually SFP is connected to SGMII interface of eth1, which is routed
> through SERDES 5. The SGMII line is shared between the SFP and metallic PHY
> 88E1514. There is a autonomous high-speed switch connected to the SFPDET
> signal from SFP cage. It disconnects the metallic SFP and connects SGMII to
> SFP once the module is connected.
> 
> The SFP is also connected to the I2C mux port 4 and to GPIO expander for
> reading/driving SFPDET, LOS, TXFLT, TXDIS signals:
> 
> &i2c0 {
>        pinctrl-names = "default";
>        pinctrl-0 = <&i2c0_pins>;
>        status = "okay";
>        clock-frequency = <100000>;
> 
>        i2cmux@70 {
>                compatible = "nxp,pca9547";
>                #address-cells = <1>;
>                #size-cells = <0>;
>                reg = <0x70>;
>                status = "okay";
> 
> ...
> 
>                i2c@7 {
>                        /* SFP+ GPIO expander */
>                        #address-cells = <1>;
>                        #size-cells = <0>;
>                        reg = <7>;
> 
>                        sfpgpio: gpio@71 {
>                                compatible = "nxp,pca9538";
>                                reg = <0x71>;
>                                interrupt-parent = <&gpio1>;
>                                interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
>                                gpio-controller;
>                                #gpio-cells = <2>;
>                        };

I have authored a nearly identical snippet, mine looks as follows:

+               i2c@7 {
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+                       reg = <7>;
+
+                       pcawan: gpio@71 {
+                               compatible = "nxp,pca9538";
+                               reg = <0x71>;
+
+                               pinctrl-names = "default";
+                               pinctrl-0 = <&pcawan_pins>;
+
+                               interrupt-parent = <&gpio1>;
+                               interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
+
+                               gpio-controller;
+                               #gpio-cells = <2>;
+
+                               interrupt-controller;
+                               #interrupt-cells = <2>;
+                       };
+               };

The interrupt-controller part doesn't seem to work though, at least

+               interrupt-parent = <&pcawan>;
+               interrupts = <7 IRQ_TYPE_LEVEL_LOW>;

in the phy node gives an error.


>                };
> 	};
> };
> 
> We have our proprietary support hacked onto mvneta driver for disconnecting
> PHY on the fly. It is a bit nasty, so I suggest to ignore SFP in this DTS
> altogether and let's wait till "phylink based SFP module support" or
> something alike hits upstream, so we can base the SFP support on solid code;
> unless somebody has a better idea, of course.
> 
> > 
> > 
> > diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > new file mode 100644
> > index 000000000000..d3cd8a4d713d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
> > @@ -0,0 +1,246 @@
> ...
> > +
> > +			/* USB part of the eSATA/USB 2.0 port */
> 
> This comment is perhaps some error inherited from my development DTS. We do
> not have any eSATA, perhaps PCIe/USB 2.0 slot.

oh right. I changed it for v3.
 
> > 
> > +			usb@58000 {
> > +				status = "okay";
> > +			};
> > +
> > +
> > +&eth0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&ge0_rgmii_pins>;
> > +	status = "okay";
> > +	phy-mode = "rgmii-id";
> > +
> > +	fixed-link {
> > +		speed = <1000>;
> > +		full-duplex;
> > +	};
> > +};
> > +
> > +&eth1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&ge1_rgmii_pins>;
> > +	status = "okay";
> > +	phy-mode = "rgmii-id";
> > +
> > +	fixed-link {
> > +		speed = <1000>;
> > +		full-duplex;
> > +	};
> > +};
> > +
> 
> Actually eth0 and eth1 (both are RGMII) are connected to the 88E6176 switch.
> The problem is that from what I have read so far the switch can not operate
> in DSA mode with two CPU ports. We currently operate the switch in "normal
> mode" with the eth0 and eth1 set to fixed-link 1000/full and with
> proprietary driver (derived from OpenWRT switch drivers). I would say that
> these records for eth0 and eth1 are therefore redundant, because it does
> nothing without the switch support and it would most likely change once we
> have DSA driver (using only eth0).

Right. They do nothing currently. In my local tree I have a
specification for the switch which allows to read the phy registers of
the lan ports, but communication isn't possible yet. For this AFAIK I
need at least one of these. I mailed a few iterations with Andrew here,
but no success so far. Also dropping one cpu port from the definition
didn't help.

Best regards
Uwe
Andrew Lunn Nov. 14, 2016, 8:28 p.m. UTC | #13
> 
> +               i2c@7 {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       reg = <7>;
> +
> +                       pcawan: gpio@71 {
> +                               compatible = "nxp,pca9538";
> +                               reg = <0x71>;
> +
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&pcawan_pins>;
> +
> +                               interrupt-parent = <&gpio1>;
> +                               interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
> +
> +                               gpio-controller;
> +                               #gpio-cells = <2>;
> +
> +                               interrupt-controller;
> +                               #interrupt-cells = <2>;
> +                       };
> +               };
> 
> The interrupt-controller part doesn't seem to work though, at least
> 
> +               interrupt-parent = <&pcawan>;
> +               interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> 
> in the phy node gives an error.

Interrupts don't seem to work very well with the nxp,pca9538. Which
is probably why it is disabled by default.

   Andrew
tomas.hlavacek@nic.cz Nov. 19, 2016, 8:09 p.m. UTC | #14
Hello Uwe!

On Mon, Nov 14, 2016 at 9:28 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> 
>>  +               i2c@7 {
>>  +                       #address-cells = <1>;
>>  +                       #size-cells = <0>;
>>  +                       reg = <7>;
>>  +
>>  +                       pcawan: gpio@71 {
>>  +                               compatible = "nxp,pca9538";
>>  +                               reg = <0x71>;
>>  +
>>  +                               pinctrl-names = "default";
>>  +                               pinctrl-0 = <&pcawan_pins>;
>>  +
>>  +                               interrupt-parent = <&gpio1>;
>>  +                               interrupts = <14 
>> IRQ_TYPE_LEVEL_LOW>;
>>  +
>>  +                               gpio-controller;
>>  +                               #gpio-cells = <2>;
>>  +
>>  +                               interrupt-controller;
>>  +                               #interrupt-cells = <2>;
>>  +                       };
>>  +               };
>> 
>>  The interrupt-controller part doesn't seem to work though, at least
>> 
>>  +               interrupt-parent = <&pcawan>;
>>  +               interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
>> 
>>  in the phy node gives an error.
> 
> Interrupts don't seem to work very well with the nxp,pca9538. Which
> is probably why it is disabled by default.

I was thinking about this issue and I can remember that there was an 
earlier prototype that had a shared interrupt line from PHY (88E1514) 
and from the PCA9538. In this case we needed to specifically disable 
the interrupt of the PHY to release the interrupt line (which needed a 
hack into PHY driver code). The IRQ from PHY is connected as an 
ordinary input to PCA9538 in later board prototype. And the same holds 
for the production version.

Do you have CZ11NIC13 or older board revision?

Tomas
Uwe Kleine-König Nov. 20, 2016, 8:30 p.m. UTC | #15
Hello Tomas,

On Sat, Nov 19, 2016 at 09:09:07PM +0100, tomas.hlavacek@nic.cz wrote:
> On Mon, Nov 14, 2016 at 9:28 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > Interrupts don't seem to work very well with the nxp,pca9538. Which
> > is probably why it is disabled by default.
> 
> I was thinking about this issue and I can remember that there was an earlier
> prototype that had a shared interrupt line from PHY (88E1514) and from the
> PCA9538. In this case we needed to specifically disable the interrupt of the
> PHY to release the interrupt line (which needed a hack into PHY driver
> code). The IRQ from PHY is connected as an ordinary input to PCA9538 in
> later board prototype. And the same holds for the production version.

That would explain why I see an "irq but nobody cared" message when
booting the original system.

This isn't the problem I meant though. When adding interrupt-parent =
<&pcawan>; interrupts = <7 IRQ_TYPE_LEVEL_LOW>; to the phy node I get an
error saying that there is no irq domain associated with this device.
 
> Do you have CZ11NIC13 or older board revision?

CZ11NIC12 is indicated on my board.

Best regards
Uwe
tomas.hlavacek@nic.cz Nov. 22, 2016, 9:59 p.m. UTC | #16
Hi Uwe!

On Sun, Nov 20, 2016 at 9:30 PM, Uwe Kleine-König 
<uwe@kleine-koenig.org> wrote:
> Hello Tomas,
> 
> On Sat, Nov 19, 2016 at 09:09:07PM +0100, tomas.hlavacek@nic.cz wrote:
>>  On Mon, Nov 14, 2016 at 9:28 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>  > Interrupts don't seem to work very well with the nxp,pca9538. 
>> Which
>>  > is probably why it is disabled by default.
>> 
>>  I was thinking about this issue and I can remember that there was 
>> an earlier
>>  prototype that had a shared interrupt line from PHY (88E1514) and 
>> from the
>>  PCA9538. In this case we needed to specifically disable the 
>> interrupt of the
>>  PHY to release the interrupt line (which needed a hack into PHY 
>> driver
>>  code). The IRQ from PHY is connected as an ordinary input to 
>> PCA9538 in
>>  later board prototype. And the same holds for the production 
>> version.
> 
> That would explain why I see an "irq but nobody cared" message when
> booting the original system.
> 
> This isn't the problem I meant though. When adding interrupt-parent =
> <&pcawan>; interrupts = <7 IRQ_TYPE_LEVEL_LOW>; to the phy node I get 
> an
> error saying that there is no irq domain associated with this device.
> 
>>  Do you have CZ11NIC13 or older board revision?
> 
> CZ11NIC12 is indicated on my board.

:-( Well, this board version has wrongly matched length of some 
differential pairs, IRQ from 88E1514 is connected differently, there 
are slight differences in power supplies and (if I am not mistaken) 
something changed in RTC support circuitry. It looks like a huge 
mistake on our side.

Anyway I took your patch and tried few things:
- clean up comments
- add pca9538 interrupt-controller
- remove rtc disable (WFM with CZ11NIC13, which is the production board)
- add MBUS mem regions for CESA
- add IRQ for 88E1514 PHY - and there is a problem:

It seems that libphy is probed before pca9538 and we end up with:
[    4.217550] libphy: orion_mdio_bus: probed
[    4.221777] irq: no irq domain found for 
/soc/internal-regs/i2c@11000/i2cmux@70/i2c@7/gpio@71 !

Any clue where to look in order to defer probing libphy or at least 
orion_mdio_bus?

I'll post my version of the patch without the PHY IRQ (therefore 
polling will kick in).

Thanks,
Tomas
tomas.hlavacek@nic.cz Nov. 23, 2016, 12:27 a.m. UTC | #17
Hi Uwe!

On Tue, Nov 22, 2016 at 10:59 PM, tomas.hlavacek@nic.cz wrote:
> Anyway I took your patch and tried few things:
> - add pca9538 interrupt-controller
> - add IRQ for 88E1514 PHY - and there is a problem:
...

I thought it over and if I am not mistaken this is not going to work 
anyway, because pca9538 driver causes the GPIO driver to set 
IRQ_NESTED_THREAD, so we can not simply use one of the GPIO expander 
pins as IRQ source for 88E1514, because request_irq() on it will fail 
ultimately.

Tomas
Andrew Lunn Nov. 23, 2016, 1:39 a.m. UTC | #18
On Wed, Nov 23, 2016 at 01:27:31AM +0100, tomas.hlavacek@nic.cz wrote:
> Hi Uwe!
> 
> On Tue, Nov 22, 2016 at 10:59 PM, tomas.hlavacek@nic.cz wrote:
> >Anyway I took your patch and tried few things:
> >- add pca9538 interrupt-controller
> >- add IRQ for 88E1514 PHY - and there is a problem:
> ...
> 
> I thought it over and if I am not mistaken this is not going to work
> anyway, because pca9538 driver causes the GPIO driver to set
> IRQ_NESTED_THREAD, so we can not simply use one of the GPIO expander
> pins as IRQ source for 88E1514, because request_irq() on it will
> fail ultimately.

Actually, the phylib now does uses threaded IRQs, since i implemented
interrupt support for the mv88e6xxx driver. Its interrupts require
MDIO transactions, so have to be threaded.

However, i don't think using interrupts on the pca9538 are
reliable. Interrupt support is compile time optional for that
driver. It is not clear to me if distributions do compile the driver
with interrupts enabled. So it could be the probe fails with OpenWRT,
Debian, etc...

	Andrew
Andrew Lunn Nov. 23, 2016, 2:59 p.m. UTC | #19
> >CZ11NIC12 is indicated on my board.
> 
> :-( Well, this board version has wrongly matched length of some
> differential pairs, IRQ from 88E1514 is connected differently, there
> are slight differences in power supplies and (if I am not mistaken)
> something changed in RTC support circuitry. It looks like a huge
> mistake on our side.

Hi Tomas

Would these problems also explain why the Ethernet links to the switch
don't work? Maybe the differential pairs?

> It seems that libphy is probed before pca9538 and we end up with:
> [    4.217550] libphy: orion_mdio_bus: probed
> [    4.221777] irq: no irq domain found for
> /soc/internal-regs/i2c@11000/i2cmux@70/i2c@7/gpio@71 !
> 
> Any clue where to look in order to defer probing libphy or at least
> orion_mdio_bus?

I think there is a known phylib problem here. Somewhere in the call
chain there is a void function, so the EPROBE_DEFFER gets
discarded. But i could be remembering this wrongly.

      Andrew
Uwe Kleine-König Nov. 23, 2016, 6:36 p.m. UTC | #20
Hello Andrew,

On 11/23/2016 03:59 PM, Andrew Lunn wrote:
>>> CZ11NIC12 is indicated on my board.
>>
>> :-( Well, this board version has wrongly matched length of some
>> differential pairs, IRQ from 88E1514 is connected differently, there
>> are slight differences in power supplies and (if I am not mistaken)
>> something changed in RTC support circuitry. It looks like a huge
>> mistake on our side.
> 
> Hi Tomas
> 
> Would these problems also explain why the Ethernet links to the switch
> don't work? Maybe the differential pairs?

no this is not the problem. When booting the OpenWRT based system I can
communicate with the device via the switch. (Didn't test deeply, but my
Laptop got a dhcp lease from the Turris Omnia and I can ping it.) But
this convinces me, that the hardware is good enough.

If you have any ideas what I can try to make the switch work or help to
diagnose the problem, just let me know.

Best regards
Uwe
tomas.hlavacek@nic.cz Nov. 23, 2016, 10:45 p.m. UTC | #21
Hi Andrew!

On Wed, Nov 23, 2016 at 3:59 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>  >CZ11NIC12 is indicated on my board.
>> 
>>  :-( Well, this board version has wrongly matched length of some
>>  differential pairs, IRQ from 88E1514 is connected differently, there
>>  are slight differences in power supplies and (if I am not mistaken)
>>  something changed in RTC support circuitry. It looks like a huge
>>  mistake on our side.
> 
> Hi Tomas
> 
> Would these problems also explain why the Ethernet links to the switch
> don't work? Maybe the differential pairs?

I do not think so. The ethernet links to the switch are RGMII, not 
differential pairs. Differential pair is used only for the eth2 to link 
either SFP+ or 88E1514 (via a high-speed switch that selects one or 
another). So the problems with differential pairs affect only WAN 
interface.

> 
> 
>>  It seems that libphy is probed before pca9538 and we end up with:
>>  [    4.217550] libphy: orion_mdio_bus: probed
>>  [    4.221777] irq: no irq domain found for
>>  /soc/internal-regs/i2c@11000/i2cmux@70/i2c@7/gpio@71 !
>> 
>>  Any clue where to look in order to defer probing libphy or at least
>>  orion_mdio_bus?
> 
> I think there is a known phylib problem here. Somewhere in the call
> chain there is a void function, so the EPROBE_DEFFER gets
> discarded. But i could be remembering this wrongly.

Oh yes, I thought that and I tried to find exactly this type of problem 
yesterday, but I didn't succeed. But I think that we agreed that we are 
going to stick with PHY polling rather then experimenting with 
unreliable IRQ over the GPIO expander, so we can leave this unresolved.

I will look into the I2C mux concerns, fix the remaining comments 
regarding my version and test RTC more extensively - Uwe's board is 
still not ticking, mine does, so we have to rule out that it is a 
common problem.

Tomas
diff mbox

Patch

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index befcd2619902..f1d3b9ff257e 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -920,6 +920,7 @@  dtb-$(CONFIG_MACH_ARMADA_38X) += \
 	armada-385-db-ap.dtb \
 	armada-385-linksys-caiman.dtb \
 	armada-385-linksys-cobra.dtb \
+	armada-385-turris-omnia.dtb \
 	armada-388-clearfog.dtb \
 	armada-388-db.dtb \
 	armada-388-gp.dtb \
diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
new file mode 100644
index 000000000000..d3cd8a4d713d
--- /dev/null
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -0,0 +1,246 @@ 
+/*
+ * Device Tree file for the Turris Omnia
+ * Schematic available at https://www.turris.cz/doc/_media/rtrom01-schema.pdf
+ *
+ * Copyright (C) 2016 Uwe Kleine-König <uwe@kleine-koenig.org>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is licensed under the terms of the GNU General Public
+ *     License version 2.  This program is licensed "as is" without
+ *     any warranty of any kind, whether express or implied.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include "armada-385.dtsi"
+
+/ {
+	model = "Turris Omnia";
+	compatible = "turris,omnia", "marvell,armada385", "marvell,armada380";
+
+	chosen {
+		stdout-path = &uart0;
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0x00000000 0x40000000>; /* 1024 MB */
+	};
+
+	soc {
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000
+			  MBUS_ID(0x01, 0x1d) 0 0xfff00000 0x100000>;
+
+		internal-regs {
+
+			/* USB part of the eSATA/USB 2.0 port */
+			usb@58000 {
+				status = "okay";
+			};
+
+			sata@a8000 {
+				status = "okay";
+			};
+
+			sdhci@d8000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&sdhci_pins>;
+				status = "okay";
+
+				bus-width = <8>;
+				no-1-8-v;
+				non-removable;
+			};
+
+			usb3@f0000 {
+				status = "okay";
+			};
+
+			usb3@f8000 {
+				status = "okay";
+			};
+		};
+
+		pcie-controller {
+			status = "okay";
+
+			pcie@1,0 {
+				/* Port 0, Lane 0 */
+				status = "okay";
+			};
+
+			pcie@2,0 {
+				/* Port 2, Lane 0 */
+				status = "okay";
+			};
+
+			pcie@3,0 {
+				/* Port 3, Lane 0 */
+				status = "okay";
+			};
+		};
+	};
+};
+
+&eth0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ge0_rgmii_pins>;
+	status = "okay";
+	phy-mode = "rgmii-id";
+
+	fixed-link {
+		speed = <1000>;
+		full-duplex;
+	};
+};
+
+&eth1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ge1_rgmii_pins>;
+	status = "okay";
+	phy-mode = "rgmii-id";
+
+	fixed-link {
+		speed = <1000>;
+		full-duplex;
+	};
+};
+
+/* WAN port */
+&eth2 {
+	status = "okay";
+	phy-mode = "sgmii";
+	phy = <&phy1>;
+};
+
+&i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c0_pins>;
+	status = "okay";
+
+	i2cmux@70 {
+		compatible = "nxp,pca9547";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x70>;
+		status = "okay";
+
+		i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+			status = "okay";
+
+			/* STM32F0 at address 0x2a */
+			/* leds device at address 0x2b */
+
+			eeprom@54 {
+				/* holds configuration about RAM, evaluated by bootloader */
+				compatible = "at,24c64";
+				reg = <0x54>;
+			};
+		};
+
+		i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+
+			/* ATSHA204A at address 0x64 */
+		};
+
+		i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+
+			/* exposed on pin header */
+		};
+	};
+};
+
+&mdio {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mdio_pins>;
+	status = "okay";
+
+	phy1: phy@1 {
+		status = "okay";
+		compatible = "marvell,88E1514", "marvell,88E1510", "ethernet-phy-ieee802.3-c22";
+		reg = <1>;
+	};
+};
+
+&pinctrl {
+	spi0cs1_pins: spi0-pins-0cs1 {
+		marvell,pins = "mpp26";
+		marvell,function = "spi0";
+	};
+};
+
+&spi0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi0_pins &spi0cs1_pins>;
+	status = "okay";
+
+	spi-nor@0 {
+		compatible = "spansion,s25fl164k", "jedec,spi-nor";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0>;
+		spi-max-frequency = <40000000>;
+
+		partition@0 {
+			reg = <0x0 0x00100000>;
+			label = "U-Boot";
+		};
+
+		partition@1 {
+			reg = <0x00100000 0x00700000>;
+			label = "Rescue system";
+		};
+	};
+
+	/* @1 is on pin header */
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_pins>;
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>;
+	status = "okay";
+};