[PATCH 0/4] vf610-zii-dev updates

[PATCH 0/4] vf610-zii-dev updates

[Russell King - ARM Linux]

Hi,

These patches update the DT for the ZII VF610 boards.

The first patch fixes complaints at boot about missing DMAs on rev C
boards, particularly for the SPI interface.  This is because edma1 is
not enabled.  This seems to be a regression from the 4.10 era.

The second patch fixes an interrupt storm during boot on rev B boards,
which causes boot to take 80+ seconds - this seems to be a long
standing issue since the DT description was first added.  The PTB28
pin is definitely GPIO 98, and GPIO 98 is definitely part of the
gpio3 block, not the gpio2 block.  Since GPIO 66 (which is the
corresponding GPIO in gpio2) is low, and the IRQ trigger is level-low,
this causes an interrupt storm.

The last two patches add an explicit description of the PHYs that are
actually connected to the switch - the 88e1545 is a quad PHY, and
without describing the MDIO bus, DSA assumes that any PHYs it can
discover are present for the switch.  As only the first three PHYs
are connected, this leads the 4th port to believe it is connected to
the 4th PHY when the fixed-link definition is (eventually) removed.

Head this off by providing the proper descriptions, and as we have
them, also describe the interrupts for these PHYs.

Note, however, that the interrupt description is not quite correct -
the 88e1545 PHYs all share one interrupt line, and there is a register
in the PHY which can be used to demux the interrupt to the specific
PHY.  However, in this description, we ignore the demux register, and
just share the interrupt between the PHYs.  That much is fine, but
the pinmuxing becomes problematical - if we describe the same pinmux
settings for each PHY for the interrupt line, the 2nd/3rd PHYs fail.
This has no known solution.  Suggestions welcome.

 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 34 ++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/vf610-zii-dev.dtsi      |  4 ++++
 2 files changed, 37 insertions(+), 1 deletion(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH 1/4] ARM: dts: vf610-zii-dev: enable edma1

[Russell King]

EDMA1 is required for the SPI controller used on the ZII boards to be
functional.  Enable EDMA1.

Fixes: 14c416336820 ("ARM: dts: vfxxx: Enable DMA for DSPI on Vybrid")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/boot/dts/vf610-zii-dev.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-zii-dev.dtsi b/arch/arm/boot/dts/vf610-zii-dev.dtsi
index 6b58d3a97992..aadd36db0092 100644
--- a/arch/arm/boot/dts/vf610-zii-dev.dtsi
+++ b/arch/arm/boot/dts/vf610-zii-dev.dtsi
@@ -96,6 +96,10 @@
 	status = "okay";
 };
 
+&edma1 {
+	status = "okay";
+};
+
 &esdhc1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_esdhc1>;
-- 
2.7.4

[PATCH 2/4] ARM: dts: vf610-zii-dev-rev-b: fix interrupt for GPIO expander

[Russell King]

The interrupt specification for the GPIO expander is wrong - the
expander is wired to PTB28, which is GPIO98.  GPIO98 is on gpio chip
3, not 2.

In addition, the device is missing a required property.  Interrupt
controllers must have the "interrupt-controller" property specified.
Add this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index acdf12ad0622..ede8649ba515 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -371,7 +371,8 @@
 		reg = <0x22>;
 		gpio-controller;
 		#gpio-cells = <2>;
-		interrupt-parent = <&gpio2>;
+		interrupt-controller;
+		interrupt-parent = <&gpio3>;
 		interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
 	};
 };
-- 
2.7.4

[PATCH 2/4] ARM: dts: vf610-zii-dev-rev-b: fix interrupt for GPIO expander

[Andrew Lunn]

On Wed, Dec 20, 2017 at 11:11:50PM +0000, Russell King wrote:
> The interrupt specification for the GPIO expander is wrong - the
> expander is wired to PTB28, which is GPIO98.  GPIO98 is on gpio chip
> 3, not 2.

Hi Russell

I'd also seen this interrupt storm. The whole interrupt architecture
for this expander does not look so good, so i just assumed it was a
design issue. Instead, it was me who probably made a typ0 :-(

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 3/4] ARM: dts: vf610-zii-dev-rev-b: add PHYs for switch2

[Russell King]

Switch 2 has an 88e1545 PHY behind it, which is a quad PHY.  Only the
first three PHYs are used, the remaining PHY is unused.  When we wire
up the SFF sockets in a later commit, the omission of this causes the
fourth PHY to be used for port 3.  Specifying the PHYs in DT avoids
the auto-probing of the bus, and discovery of this PHY.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index ede8649ba515..782b69a3acdf 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -255,16 +255,19 @@
 					port at 0 {
 						reg = <0>;
 						label = "lan6";
+						phy-handle = <&switch2phy0>;
 					};
 
 					port at 1 {
 						reg = <1>;
 						label = "lan7";
+						phy-handle = <&switch2phy1>;
 					};
 
 					port at 2 {
 						reg = <2>;
 						label = "lan8";
+						phy-handle = <&switch2phy2>;
 					};
 
 					port at 3 {
@@ -304,6 +307,20 @@
 						};
 					};
 				};
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					switch2phy0: phy at 0 {
+						reg = <0>;
+					};
+					switch2phy1: phy at 1 {
+						reg = <1>;
+					};
+					switch2phy2: phy at 2 {
+						reg = <2>;
+					};
+				};
 			};
 		};
 
-- 
2.7.4

[PATCH 3/4] ARM: dts: vf610-zii-dev-rev-b: add PHYs for switch2

[Andrew Lunn]

On Wed, Dec 20, 2017 at 11:11:55PM +0000, Russell King wrote:
> Switch 2 has an 88e1545 PHY behind it, which is a quad PHY.  Only the
> first three PHYs are used, the remaining PHY is unused.  When we wire
> up the SFF sockets in a later commit, the omission of this causes the
> fourth PHY to be used for port 3.  Specifying the PHYs in DT avoids
> the auto-probing of the bus, and discovery of this PHY.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 3/4] ARM: dts: vf610-zii-dev-rev-b: add PHYs for switch2

[Linus Walleij]

On Thu, Dec 21, 2017 at 12:11 AM, Russell King
<rmk+kernel@armlinux.org.uk> wrote:

> Switch 2 has an 88e1545 PHY behind it, which is a quad PHY.  Only the
> first three PHYs are used, the remaining PHY is unused.  When we wire
> up the SFF sockets in a later commit, the omission of this causes the
> fourth PHY to be used for port 3.  Specifying the PHYs in DT avoids
> the auto-probing of the bus, and discovery of this PHY.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Ah this is elegant and what Andrew requested me to do for
another switch as well. Makes perfect sense. FWIW:

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Russell King]

The 88e1545 PHY has its interrupts wired to the VF610, so we might as
well use them.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
This is certainly not correct, as all PHYs on this device share the
same interrupt line, but we can't specify the pinmux settings
individually on each PHY.  How should this be handled?
---
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 782b69a3acdf..d6786c5d0109 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -312,12 +312,20 @@
 					#size-cells = <0>;
 
 					switch2phy0: phy at 0 {
+						interrupt-parent = <&gpio0>;
+						interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
 						reg = <0>;
+						pinctrl-names = "default";
+						pinctrl-0 = <&pinctrl_mv88e1545>;
 					};
 					switch2phy1: phy at 1 {
+						interrupt-parent = <&gpio0>;
+						interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
 						reg = <1>;
 					};
 					switch2phy2: phy at 2 {
+						interrupt-parent = <&gpio0>;
+						interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
 						reg = <2>;
 					};
 				};
@@ -488,6 +496,12 @@
 		>;
 	};
 
+	pinctrl_mv88e1545: pinctrl-mv88e1545 {
+		fsl,pins = <
+			VF610_PAD_PTB0__GPIO_22		0x219d
+		>;
+	};
+
 	pinctrl_pca9554_22: pinctrl-pca95540-22 {
 		fsl,pins = <
 			VF610_PAD_PTB28__GPIO_98	0x219d
-- 
2.7.4

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Andrew Lunn]

On Wed, Dec 20, 2017 at 11:12:01PM +0000, Russell King wrote:
> The 88e1545 PHY has its interrupts wired to the VF610, so we might as
> well use them.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> This is certainly not correct, as all PHYs on this device share the
> same interrupt line, but we can't specify the pinmux settings
> individually on each PHY.  How should this be handled?

Hi Russell

You could put it as a hog on the gpio controller node.  However, i
don't think that is much better.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Linus Walleij]

On Thu, Dec 21, 2017 at 12:12 AM, Russell King
<rmk+kernel@armlinux.org.uk> wrote:

> The 88e1545 PHY has its interrupts wired to the VF610, so we might as
> well use them.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> This is certainly not correct, as all PHYs on this device share the
> same interrupt line, but we can't specify the pinmux settings
> individually on each PHY.  How should this be handled?

I do not know the details of the Marvell switch.

Sorry for any possible misunderstandings below.

What I did with the Realtek switch I was playing around
with was to create a separate irqchip, also in the device tree,
embedded inside the DSA switch, then referenced the
IRQs from that chip as 0, 1 .. n.

The patches are here:
DTS:
https://marc.info/?l=linux-netdev&m=150992420713391&w=2
Driver:
https://marc.info/?l=linux-netdev&m=150992421113393&w=2

Note that this RFC is wrong: it assigns the IRQs to ports
instead of PHYs, but the idea with an IRQchip inside the
DSA is pretty solid IMO. (I will rewrite it using your method
of a separate mdio bus node and phy-handle references.)

Anyway I was inspired to this model from certain PCI bridges that
contain an IRQ demuxer and thus instantiate an irqchip for
this, that is then part of the bridge itself.

Then for the pin control, I guess the irqchip inside the bridge
should be the entity taking the IRQ from the GPIO-backed
irq controller and also the pin control handle.  As pin control
handles are tied to Linux devices, that requires it
to be a device proper though. I don't know if it's possible
to properly spawn a device for this irqchip from the switch,
but I guess it is what I would try.

I hope this helps.

Yours,
Linus Walleij

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Andrew Lunn]

On Thu, Dec 21, 2017 at 01:32:21PM +0100, Linus Walleij wrote:
> On Thu, Dec 21, 2017 at 12:12 AM, Russell King
> <rmk+kernel@armlinux.org.uk> wrote:
> 
> > The 88e1545 PHY has its interrupts wired to the VF610, so we might as
> > well use them.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > This is certainly not correct, as all PHYs on this device share the
> > same interrupt line, but we can't specify the pinmux settings
> > individually on each PHY.  How should this be handled?
> 
> I do not know the details of the Marvell switch.

Hi Linus

The 88e1545 is a discreet quad PHY. It is connected to the switch, but
not integrated into the switch. All its interrupt handling is done
with a GPIO onto the Freescale processor, via a GPIO. There is nothing
DSA related here at all with respect to the interrupt. It is just a
normal GPIO interrupt. What is a bit odd is that it one shared
interrupt for all four PHYs.

What you described with an irqchip inside the switch is what we
actually do for the internal PHYs on Marvell devices. And it is what i
recommend for all DSA drivers. Expose standard IRQs, and let phylib
use them in its normal way.

	  Andrew

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Russell King - ARM Linux]

On Thu, Dec 21, 2017 at 02:40:58PM +0100, Andrew Lunn wrote:
> On Thu, Dec 21, 2017 at 01:32:21PM +0100, Linus Walleij wrote:
> > On Thu, Dec 21, 2017 at 12:12 AM, Russell King
> > <rmk+kernel@armlinux.org.uk> wrote:
> > 
> > > The 88e1545 PHY has its interrupts wired to the VF610, so we might as
> > > well use them.
> > >
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > > This is certainly not correct, as all PHYs on this device share the
> > > same interrupt line, but we can't specify the pinmux settings
> > > individually on each PHY.  How should this be handled?
> > 
> > I do not know the details of the Marvell switch.
> 
> Hi Linus
> 
> The 88e1545 is a discreet quad PHY. It is connected to the switch, but
> not integrated into the switch. All its interrupt handling is done
> with a GPIO onto the Freescale processor, via a GPIO. There is nothing
> DSA related here at all with respect to the interrupt. It is just a
> normal GPIO interrupt. What is a bit odd is that it one shared
> interrupt for all four PHYs.
> 
> What you described with an irqchip inside the switch is what we
> actually do for the internal PHYs on Marvell devices. And it is what i
> recommend for all DSA drivers. Expose standard IRQs, and let phylib
> use them in its normal way.

... and it has to be said that model doesn't work in this case,
because, although there is the possibility to demux the interrupt
any of the PHYs, you already need to be driving one of the PHYs.

It's not an interrupt controller itself (there's no possibility to
enable/disable individual interrupts from a PHY) so it doesn't make
sense.

What we have here is _really_ a shared interrupt between four
separate devices, and we need a way to sanely describe resources
shared between several device instances to pinmux.  Unfortunately,
it seems pinmux is designed around one device having exclusive use
of a resource, which makes it hard to describe shared interrupts in
DT.

Given that DT should be a description of the hardware, and should be
independent of the OS implementation, I'd say this is a pinmux bug,
because pinmux gets in the way of describing the hardware correctly.
;)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Linus Walleij]

On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:

> What we have here is _really_ a shared interrupt between four
> separate devices, and we need a way to sanely describe resources
> shared between several device instances to pinmux.  Unfortunately,
> it seems pinmux is designed around one device having exclusive use
> of a resource, which makes it hard to describe shared interrupts in
> DT.
>
> Given that DT should be a description of the hardware, and should be
> independent of the OS implementation, I'd say this is a pinmux bug,
> because pinmux gets in the way of describing the hardware correctly.
> ;)

Hm that would be annoying. But when I look at it I think it would
actually work. Did you try just assigning the same pin control
state to all the PHY's and see what happens?

Just set
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_mv88e1545>;

on all of them?

I don't think DTC would complain at least.

When I look at the driver subsystem, I don't see anything really
stopping you from doing that and even have three devices selecting
the same "default" pin control state. It seems it will just wind up
three times in pinctrl_select_state() and the first time it calls the pin
control driver to actually set it and the three other times it finds the
state is already correct and returns success.

So the way I read it actually several devices can reference the
same pin control state.

That is, unless there is something I missed. Which I often do...

If it happens to work we should probably put a blurb in the
DT binding that this is expected behaviour though.

Yours,
Linus Walleij

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Russell King - ARM Linux]

On Thu, Dec 21, 2017 at 11:53:47PM +0100, Linus Walleij wrote:
> On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> 
> > What we have here is _really_ a shared interrupt between four
> > separate devices, and we need a way to sanely describe resources
> > shared between several device instances to pinmux.  Unfortunately,
> > it seems pinmux is designed around one device having exclusive use
> > of a resource, which makes it hard to describe shared interrupts in
> > DT.
> >
> > Given that DT should be a description of the hardware, and should be
> > independent of the OS implementation, I'd say this is a pinmux bug,
> > because pinmux gets in the way of describing the hardware correctly.
> > ;)
> 
> Hm that would be annoying. But when I look at it I think it would
> actually work. Did you try just assigning the same pin control
> state to all the PHY's and see what happens?
> 
> Just set
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_mv88e1545>;
> 
> on all of them?

It was tried, DT was happy, but the kernel on boot complained because
pinctrl objected, which caused the drivers to fail to bind:

libphy: mdio: probed
vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio at 4!switch at 0!mdio:00; cannot claim for !mdio-mux!mdio at 4!switch at 0!mdio:01
vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio at 4!switch at 0!mdio:01) status -22
vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545  on device 40048000.iomuxc
Marvell 88E1545 !mdio-mux!mdio at 4!switch at 0!mdio:01: Error applying setting, reverse things back
Marvell 88E1545: probe of !mdio-mux!mdio at 4!switch at 0!mdio:01 failed with error -22
vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio at 4!switch at 0!mdio:00; cannot claim for !mdio-mux!mdio at 4!switch at 0!mdio:02
vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio at 4!switch at 0!mdio:02) status -22
vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545  on device 40048000.iomuxc
Marvell 88E1545 !mdio-mux!mdio at 4!switch at 0!mdio:02: Error applying setting, reverse things back
Marvell 88E1545: probe of !mdio-mux!mdio at 4!switch at 0!mdio:02 failed with error -22

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Florian Fainelli]

On 12/21/2017 04:14 PM, Russell King - ARM Linux wrote:
> On Thu, Dec 21, 2017 at 11:53:47PM +0100, Linus Walleij wrote:
>> On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>>
>>> What we have here is _really_ a shared interrupt between four
>>> separate devices, and we need a way to sanely describe resources
>>> shared between several device instances to pinmux.  Unfortunately,
>>> it seems pinmux is designed around one device having exclusive use
>>> of a resource, which makes it hard to describe shared interrupts in
>>> DT.
>>>
>>> Given that DT should be a description of the hardware, and should be
>>> independent of the OS implementation, I'd say this is a pinmux bug,
>>> because pinmux gets in the way of describing the hardware correctly.
>>> ;)
>>
>> Hm that would be annoying. But when I look at it I think it would
>> actually work. Did you try just assigning the same pin control
>> state to all the PHY's and see what happens?
>>
>> Just set
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_mv88e1545>;
>>
>> on all of them?
> 
> It was tried, DT was happy, but the kernel on boot complained because
> pinctrl objected, which caused the drivers to fail to bind:
> 
> libphy: mdio: probed
> vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio at 4!switch at 0!mdio:00; cannot claim for !mdio-mux!mdio at 4!switch at 0!mdio:01
> vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio at 4!switch at 0!mdio:01) status -22
> vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545  on device 40048000.iomuxc
> Marvell 88E1545 !mdio-mux!mdio at 4!switch at 0!mdio:01: Error applying setting, reverse things back
> Marvell 88E1545: probe of !mdio-mux!mdio at 4!switch at 0!mdio:01 failed with error -22
> vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio at 4!switch at 0!mdio:00; cannot claim for !mdio-mux!mdio at 4!switch at 0!mdio:02
> vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio at 4!switch at 0!mdio:02) status -22
> vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545  on device 40048000.iomuxc
> Marvell 88E1545 !mdio-mux!mdio at 4!switch at 0!mdio:02: Error applying setting, reverse things back
> Marvell 88E1545: probe of !mdio-mux!mdio at 4!switch at 0!mdio:02 failed with error -22
> 

You could also see it another way, because this is a quad PHY in a
single package, you could theoretically have a representation that
exposes a node container for the 4 PHYs, and that container node
requests the pinmux/pinctrl. Of course, this would not work with the
MDIO code which would not go one level down, and would expect the PHYs
to be at the same level as the container node...

Oh well.
-- 
Florian

[PATCH 4/4] ARM: dts: vf610-zii-dev-rev-b: add interrupts for 88e1545 PHY

[Russell King - ARM Linux]

On Thu, Dec 21, 2017 at 04:20:34PM -0800, Florian Fainelli wrote:
> On 12/21/2017 04:14 PM, Russell King - ARM Linux wrote:
> > On Thu, Dec 21, 2017 at 11:53:47PM +0100, Linus Walleij wrote:
> >> On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux
> >> <linux@armlinux.org.uk> wrote:
> >>
> >>> What we have here is _really_ a shared interrupt between four
> >>> separate devices, and we need a way to sanely describe resources
> >>> shared between several device instances to pinmux.  Unfortunately,
> >>> it seems pinmux is designed around one device having exclusive use
> >>> of a resource, which makes it hard to describe shared interrupts in
> >>> DT.
> >>>
> >>> Given that DT should be a description of the hardware, and should be
> >>> independent of the OS implementation, I'd say this is a pinmux bug,
> >>> because pinmux gets in the way of describing the hardware correctly.
> >>> ;)
> >>
> >> Hm that would be annoying. But when I look at it I think it would
> >> actually work. Did you try just assigning the same pin control
> >> state to all the PHY's and see what happens?
> >>
> >> Just set
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&pinctrl_mv88e1545>;
> >>
> >> on all of them?
> > 
> > It was tried, DT was happy, but the kernel on boot complained because
> > pinctrl objected, which caused the drivers to fail to bind:
> > 
> > libphy: mdio: probed
> > vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio at 4!switch at 0!mdio:00; cannot claim for !mdio-mux!mdio at 4!switch at 0!mdio:01
> > vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio at 4!switch at 0!mdio:01) status -22
> > vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545  on device 40048000.iomuxc
> > Marvell 88E1545 !mdio-mux!mdio at 4!switch at 0!mdio:01: Error applying setting, reverse things back
> > Marvell 88E1545: probe of !mdio-mux!mdio at 4!switch at 0!mdio:01 failed with error -22
> > vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio at 4!switch at 0!mdio:00; cannot claim for !mdio-mux!mdio at 4!switch at 0!mdio:02
> > vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio at 4!switch at 0!mdio:02) status -22
> > vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545  on device 40048000.iomuxc
> > Marvell 88E1545 !mdio-mux!mdio at 4!switch at 0!mdio:02: Error applying setting, reverse things back
> > Marvell 88E1545: probe of !mdio-mux!mdio at 4!switch at 0!mdio:02 failed with error -22
> > 
> 
> You could also see it another way, because this is a quad PHY in a
> single package, you could theoretically have a representation that
> exposes a node container for the 4 PHYs, and that container node
> requests the pinmux/pinctrl. Of course, this would not work with the
> MDIO code which would not go one level down, and would expect the PHYs
> to be at the same level as the container node...

It would actually - we have other devices that sit on buses that take
several addresses, and we describe the "first" main device or use MFD
for it.

For example, in the case of the TDA998x HDMI encoder, these are two
devices merged into one package - the HDMI encoder at one address, and
a TDA9950 at another address.  Both addresses are related, so if you
tie the address configuration pins, the offset is added to both base
addresses.  We represent the TDA998x in DT, and have the TDA998x
driver create a separate device itself for the TDA9950.

What we could do for any multi-package PHY is describe the first PHY
as a multi-package PHY in DT, extend the phy binding to include a PHY
package index, and have the PHY driver create the MDIO devices for
the other PHYs.  Eg,

switch {
...
	ports {
		port at 0 {
			reg = <0>;
			label = "lan6";
			phy-handle = <&switch2phy 0>;
		};
		port at 1 {
			reg = <1>;
			label = "lan6";
			phy-handle = <&switch2phy 1>;
		};
		port at 2 {
			reg = <2>;
			label = "lan6";
			phy-handle = <&switch2phy 2>;
		};
	};

	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		switch2phy: phy at 0 {
			compatible = "marvell,88e1545", "ethernet-phy-ieee802.3-c22";
			interrupt-parent = <&gpio0>;
			interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
			reg = <0>;
			pinctrl-names = "default";
			pinctrl-0 = <&pinctrl_mv88e1545>;
		};
	};
};

The "marvell,88e1545" driver would be responsible for creating the
PHY devices for the other MDIO bus addresses (iow 1 to 3.)

This would be an accurate respresentation of the hardware in DT,
probably more so than the trap we seem to have fallen into by
describing the individual PHYs - which we've fallen into because
that's how our current implementation requires us to describe them.

Since DT is supposed to be a hardware description, I think the question
we ought to ask is: if we were starting afresh, how would we describe
these packages that contain multiple PHYs?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH 0/4] vf610-zii-dev updates

[Russell King - ARM Linux]

On Wed, Dec 20, 2017 at 11:11:08PM +0000, Russell King - ARM Linux wrote:
> Hi,
> 
> These patches update the DT for the ZII VF610 boards.
> 
> The first patch fixes complaints at boot about missing DMAs on rev C
> boards, particularly for the SPI interface.  This is because edma1 is
> not enabled.  This seems to be a regression from the 4.10 era.
> 
> The second patch fixes an interrupt storm during boot on rev B boards,
> which causes boot to take 80+ seconds - this seems to be a long
> standing issue since the DT description was first added.  The PTB28
> pin is definitely GPIO 98, and GPIO 98 is definitely part of the
> gpio3 block, not the gpio2 block.  Since GPIO 66 (which is the
> corresponding GPIO in gpio2) is low, and the IRQ trigger is level-low,
> this causes an interrupt storm.
> 
> The last two patches add an explicit description of the PHYs that are
> actually connected to the switch - the 88e1545 is a quad PHY, and
> without describing the MDIO bus, DSA assumes that any PHYs it can
> discover are present for the switch.  As only the first three PHYs
> are connected, this leads the 4th port to believe it is connected to
> the 4th PHY when the fixed-link definition is (eventually) removed.
> 
> Head this off by providing the proper descriptions, and as we have
> them, also describe the interrupts for these PHYs.
> 
> Note, however, that the interrupt description is not quite correct -
> the 88e1545 PHYs all share one interrupt line, and there is a register
> in the PHY which can be used to demux the interrupt to the specific
> PHY.  However, in this description, we ignore the demux register, and
> just share the interrupt between the PHYs.  That much is fine, but
> the pinmuxing becomes problematical - if we describe the same pinmux
> settings for each PHY for the interrupt line, the 2nd/3rd PHYs fail.
> This has no known solution.  Suggestions welcome.
> 
>  arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 34 ++++++++++++++++++++++++++++++-
>  arch/arm/boot/dts/vf610-zii-dev.dtsi      |  4 ++++
>  2 files changed, 37 insertions(+), 1 deletion(-)

There's more stuff that isn't correct in the Vybrid DTS files...

When the SoC was first added, the vfxxx.dtsi had:

+                       adc0: adc at 4003b000 {
...
+                               status = "disabled";
+                       };
+                       adc1: adc at 400bb000 {
...
+                               status = "disabled";
+                       };

This default status remains today.

IIO hwmon support was added later:

+               iio-hwmon {
+                       compatible = "iio-hwmon";
+                       io-channels = <&adc0 16>, <&adc1 16>;
+               };

which is all fine and dandy, but iio-hwmon fails to probe unless it can
find _all_ the io-channels specified.

Given that the two ADC channels referenced by iio-hwmon default to being
disabled, it makes no sense for iio-hwmon to default to being enabled.

What's more is that if, say, adc0 is enabled by a board, the iio-hwmon
device still fails to be probed because it fails to get adc1.

As I see it, there's two possible solutions to this:
1. remove the default disabled status of the ADCs so both are always
   available, or
2. default iio-hwmon to disabled, and provide a label for this device
   so that a correct io-channels specification can be suppled for the
   board in question, and for iio-hwmon to be enabled where appropriate.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH 0/4] vf610-zii-dev updates

[Stefan Agner]

On 2017-12-22 15:16, Russell King - ARM Linux wrote:
> On Wed, Dec 20, 2017 at 11:11:08PM +0000, Russell King - ARM Linux wrote:
>> Hi,
>>
>> These patches update the DT for the ZII VF610 boards.
>>
>> The first patch fixes complaints at boot about missing DMAs on rev C
>> boards, particularly for the SPI interface.  This is because edma1 is
>> not enabled.  This seems to be a regression from the 4.10 era.
>>
>> The second patch fixes an interrupt storm during boot on rev B boards,
>> which causes boot to take 80+ seconds - this seems to be a long
>> standing issue since the DT description was first added.  The PTB28
>> pin is definitely GPIO 98, and GPIO 98 is definitely part of the
>> gpio3 block, not the gpio2 block.  Since GPIO 66 (which is the
>> corresponding GPIO in gpio2) is low, and the IRQ trigger is level-low,
>> this causes an interrupt storm.
>>
>> The last two patches add an explicit description of the PHYs that are
>> actually connected to the switch - the 88e1545 is a quad PHY, and
>> without describing the MDIO bus, DSA assumes that any PHYs it can
>> discover are present for the switch.  As only the first three PHYs
>> are connected, this leads the 4th port to believe it is connected to
>> the 4th PHY when the fixed-link definition is (eventually) removed.
>>
>> Head this off by providing the proper descriptions, and as we have
>> them, also describe the interrupts for these PHYs.
>>
>> Note, however, that the interrupt description is not quite correct -
>> the 88e1545 PHYs all share one interrupt line, and there is a register
>> in the PHY which can be used to demux the interrupt to the specific
>> PHY.  However, in this description, we ignore the demux register, and
>> just share the interrupt between the PHYs.  That much is fine, but
>> the pinmuxing becomes problematical - if we describe the same pinmux
>> settings for each PHY for the interrupt line, the 2nd/3rd PHYs fail.
>> This has no known solution.  Suggestions welcome.
>>
>>  arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 34 ++++++++++++++++++++++++++++++-
>>  arch/arm/boot/dts/vf610-zii-dev.dtsi      |  4 ++++
>>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> There's more stuff that isn't correct in the Vybrid DTS files...
> 
> When the SoC was first added, the vfxxx.dtsi had:
> 
> +                       adc0: adc at 4003b000 {
> ...
> +                               status = "disabled";
> +                       };
> +                       adc1: adc at 400bb000 {
> ...
> +                               status = "disabled";
> +                       };
> 
> This default status remains today.
> 
> IIO hwmon support was added later:
> 
> +               iio-hwmon {
> +                       compatible = "iio-hwmon";
> +                       io-channels = <&adc0 16>, <&adc1 16>;
> +               };
> 
> which is all fine and dandy, but iio-hwmon fails to probe unless it can
> find _all_ the io-channels specified.
> 
> Given that the two ADC channels referenced by iio-hwmon default to being
> disabled, it makes no sense for iio-hwmon to default to being enabled.
> 
> What's more is that if, say, adc0 is enabled by a board, the iio-hwmon
> device still fails to be probed because it fails to get adc1.
> 
> As I see it, there's two possible solutions to this:
> 1. remove the default disabled status of the ADCs so both are always
>    available, or
> 2. default iio-hwmon to disabled, and provide a label for this device
>    so that a correct io-channels specification can be suppled for the
>    board in question, and for iio-hwmon to be enabled where appropriate.

I prefer solution 2. Some Vybrid SoC include a Cortex-M4 core which
might access the ADCs too. In that case, the Linux device tree should
have them disabled.

--
Stefan

[PATCH 0/4] vf610-zii-dev updates

[Shawn Guo]

On Wed, Dec 20, 2017 at 11:11:08PM +0000, Russell King - ARM Linux wrote:
>  arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 34 ++++++++++++++++++++++++++++++-
>  arch/arm/boot/dts/vf610-zii-dev.dtsi      |  4 ++++
>  2 files changed, 37 insertions(+), 1 deletion(-)

Applied 1 - 3, thanks.