[PATCH 0/2] arm64: dts: marvell: PCIe related fixes for Armada 7K/8K

[PATCH 0/2] arm64: dts: marvell: PCIe related fixes for Armada 7K/8K

[Thomas Petazzoni]

Hello,

Here are two patches doing exactly the same fix, one for the master
CP110 and one for the slave CP110. The only reason for splitting the
fix into two patches is that the "Fixes:" tag is different in the two
patches. Indeed, the CP110 master DT and the CP110 slave DT were
introduced at different times (master CP110 in Linux 4.7, slave CP110
in Linux 4.9).

Thanks!

Thomas

Thomas Petazzoni (2):
  arm64: dts: marvell: fix interrupt-map property for Armada CP110
    master PCIe controller
  arm64: dts: marvell: fix interrupt-map property for Armada CP110 slave
    PCIe controller

 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 +++---
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi  | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.13.5

[PATCH 1/2] arm64: dts: marvell: fix interrupt-map property for Armada CP110 master PCIe controller

[Thomas Petazzoni]

The interrupt-map property used in the description of the Marvell
Armada 7K/8K PCIe controllers has a bogus extraneous 0 that causes the
interrupt conversion to not be done properly. This causes the PCIe PME
and AER root port service drivers to fail their initialization:

[    5.019900] genirq: Setting trigger mode 7 for irq 114 failed (irq_chip_set_type_parent+0x0/0x30)
[    5.028821] pcie_pme: probe of 0001:00:00.0:pcie001 failed with error -22
[    5.035687] genirq: Setting trigger mode 7 for irq 114 failed (irq_chip_set_type_parent+0x0/0x30)
[    5.044614] aer: probe of 0001:00:00.0:pcie002 failed with error -22

This problem exists since the Device Tree description of the master
CP110 was added to the kernel.

Fixes: 728dacc7f4dd5 ("arm64: dts: marvell: initial DT description of Armada 7K/8K CP110 master")
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
index 8263a8a504a8..f2aa2a81de4d 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
@@ -336,7 +336,7 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf6000000 0  0xf6000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			num-lanes = <1>;
 			clocks = <&cpm_clk 1 13>;
@@ -362,7 +362,7 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf7000000 0  0xf7000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 			interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
@@ -389,7 +389,7 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xf8000000 0  0xf8000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cpm_icu ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 			interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
-- 
2.13.5

[EXT] [PATCH 1/2] arm64: dts: marvell: fix interrupt-map property for Armada CP110 master PCIe controller

[Yehuda Yitschak]

Hi Thomas 

> -----Original Message-----
> From: Thomas Petazzoni [mailto:thomas.petazzoni at free-electrons.com]
> Sent: Thursday, September 28, 2017 15:46
> To: Jason Cooper; Andrew Lunn; Sebastian Hesselbarth; Gregory Clement
> Cc: Nadav Haklai; Hanna Hawa; Yehuda Yitschak; Antoine Tenart; Miqu?l
> Raynal; linux-arm-kernel at lists.infradead.org; Thomas Petazzoni
> Subject: [EXT] [PATCH 1/2] arm64: dts: marvell: fix interrupt-map property
> for Armada CP110 master PCIe controller
> 
> External Email
> 
> ----------------------------------------------------------------------
> The interrupt-map property used in the description of the Marvell Armada
> 7K/8K PCIe controllers has a bogus extraneous 0 that causes the interrupt
> conversion to not be done properly. This causes the PCIe PME and AER root
> port service drivers to fail their initialization:
> 
> [    5.019900] genirq: Setting trigger mode 7 for irq 114 failed
> (irq_chip_set_type_parent+0x0/0x30)
> [    5.028821] pcie_pme: probe of 0001:00:00.0:pcie001 failed with error -22
> [    5.035687] genirq: Setting trigger mode 7 for irq 114 failed
> (irq_chip_set_type_parent+0x0/0x30)
> [    5.044614] aer: probe of 0001:00:00.0:pcie002 failed with error -22
> 
> This problem exists since the Device Tree description of the master
> CP110 was added to the kernel.

The initial version referenced the GIC interrupt controller. 
Maybe this issue was introduced during the switch to the ICU  ?
I am not sure but does the GIC has the same I think the GIC <address/size-cells> as the ICU ?

> 
> Fixes: 728dacc7f4dd5 ("arm64: dts: marvell: initial DT description of Armada
> 7K/8K CP110 master")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> index 8263a8a504a8..f2aa2a81de4d 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110-master.dtsi
> @@ -336,7 +336,7 @@
>  				/* non-prefetchable memory */
>  				0x82000000 0 0xf6000000 0  0xf6000000 0
> 0xf00000>;
>  			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR
> 22 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-map = <0 0 0 0 &cpm_icu ICU_GRP_NSR 22
> +IRQ_TYPE_LEVEL_HIGH>;
>  			interrupts = <ICU_GRP_NSR 22
> IRQ_TYPE_LEVEL_HIGH>;
>  			num-lanes = <1>;
>  			clocks = <&cpm_clk 1 13>;
> @@ -362,7 +362,7 @@
>  				/* non-prefetchable memory */
>  				0x82000000 0 0xf7000000 0  0xf7000000 0
> 0xf00000>;
>  			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR
> 24 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-map = <0 0 0 0 &cpm_icu ICU_GRP_NSR 24
> +IRQ_TYPE_LEVEL_HIGH>;
>  			interrupts = <ICU_GRP_NSR 24
> IRQ_TYPE_LEVEL_HIGH>;
> 
>  			num-lanes = <1>;
> @@ -389,7 +389,7 @@
>  				/* non-prefetchable memory */
>  				0x82000000 0 0xf8000000 0  0xf8000000 0
> 0xf00000>;
>  			interrupt-map-mask = <0 0 0 0>;
> -			interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR
> 23 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-map = <0 0 0 0 &cpm_icu ICU_GRP_NSR 23
> +IRQ_TYPE_LEVEL_HIGH>;
>  			interrupts = <ICU_GRP_NSR 23
> IRQ_TYPE_LEVEL_HIGH>;
> 
>  			num-lanes = <1>;
> --
> 2.13.5

[EXT] [PATCH 1/2] arm64: dts: marvell: fix interrupt-map property for Armada CP110 master PCIe controller

[Thomas Petazzoni]

Hello,

On Thu, 28 Sep 2017 12:52:07 +0000, Yehuda Yitschak wrote:

> The initial version referenced the GIC interrupt controller. 
> Maybe this issue was introduced during the switch to the ICU  ?

No, it was not introduced by the switch to the ICU. The switch to the
ICU looked like this:

-                       interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
-                       interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
+                       interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+                       interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;

As you can see there was already a bogus "0" after &gic.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[EXT] [PATCH 1/2] arm64: dts: marvell: fix interrupt-map property for Armada CP110 master PCIe controller

[Yehuda Yitschak]

Yes...but 

That zero you removed is the "parent unit address" according to the "interrupt-map" documentation 

	parent unit address  - The unit address in the domain of the interrupt parent. The number of 32-bit
			            cells required to specify this address is described by the #address-cells property of the node
			           pointed to by the interrupt-parent field.

Now,  the gic has #address-cells =  <0x1>
And the icu has  #address-cells = <0x0>

So when switching to ICU, the parent unit address was no longer needed and should have been removed 

Best Regards

Yehuda


> -----Original Message-----
> From: Thomas Petazzoni [mailto:thomas.petazzoni at free-electrons.com]
> Sent: Thursday, September 28, 2017 15:57
> To: Yehuda Yitschak
> Cc: Jason Cooper; Andrew Lunn; Sebastian Hesselbarth; Gregory Clement;
> Nadav Haklai; Hanna Hawa; Antoine Tenart; Miqu?l Raynal; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [EXT] [PATCH 1/2] arm64: dts: marvell: fix interrupt-map
> property for Armada CP110 master PCIe controller
> 
> Hello,
> 
> On Thu, 28 Sep 2017 12:52:07 +0000, Yehuda Yitschak wrote:
> 
> > The initial version referenced the GIC interrupt controller.
> > Maybe this issue was introduced during the switch to the ICU  ?
> 
> No, it was not introduced by the switch to the ICU. The switch to the ICU
> looked like this:
> 
> -                       interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 34
> IRQ_TYPE_LEVEL_HIGH>;
> -                       interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
> +                       interrupt-map = <0 0 0 0 &cpm_icu 0 ICU_GRP_NSR 24
> IRQ_TYPE_LEVEL_HIGH>;
> +                       interrupts = <ICU_GRP_NSR 24
> + IRQ_TYPE_LEVEL_HIGH>;
> 
> As you can see there was already a bogus "0" after &gic.
> 
> Best regards,
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[EXT] [PATCH 1/2] arm64: dts: marvell: fix interrupt-map property for Armada CP110 master PCIe controller

[Thomas Petazzoni]

Hello,

On Thu, 28 Sep 2017 13:18:40 +0000, Yehuda Yitschak wrote:

> That zero you removed is the "parent unit address" according to the "interrupt-map" documentation 
> 
> 	parent unit address  - The unit address in the domain of the interrupt parent. The number of 32-bit
> 			            cells required to specify this address is described by the #address-cells property of the node
> 			           pointed to by the interrupt-parent field.
> 
> Now,  the gic has #address-cells =  <0x1>
> And the icu has  #address-cells = <0x0>
> 
> So when switching to ICU, the parent unit address was no longer needed and should have been removed 

Indeed, you are completely right. I'll adjust the patch accordingly and
resend. Thanks for spotting this!

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 2/2] arm64: dts: marvell: fix interrupt-map property for Armada CP110 slave PCIe controller

[Thomas Petazzoni]

The interrupt-map property used in the description of the Marvell
Armada 7K/8K PCIe controllers has a bogus extraneous 0 that causes the
interrupt conversion to not be done properly. This causes the PCIe PME
and AER root port service drivers to fail their initialization:

[    5.019900] genirq: Setting trigger mode 7 for irq 114 failed (irq_chip_set_type_parent+0x0/0x30)
[    5.028821] pcie_pme: probe of 0001:00:00.0:pcie001 failed with error -22
[    5.035687] genirq: Setting trigger mode 7 for irq 114 failed (irq_chip_set_type_parent+0x0/0x30)
[    5.044614] aer: probe of 0001:00:00.0:pcie002 failed with error -22

This problem exists since the Device Tree description of the slave
CP110 was added to the kernel.

Fixes: 4eef78a0091b5 ("arm64: dts: marvell: add description for the slave CP110 in Armada 8K")
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
index b71ee6c83668..4fe70323abb3 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110-slave.dtsi
@@ -335,7 +335,7 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfa000000 0  0xfa000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
 			num-lanes = <1>;
 			clocks = <&cps_clk 1 13>;
@@ -361,7 +361,7 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfb000000 0  0xfb000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 			interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
@@ -388,7 +388,7 @@
 				/* non-prefetchable memory */
 				0x82000000 0 0xfc000000 0  0xfc000000 0 0xf00000>;
 			interrupt-map-mask = <0 0 0 0>;
-			interrupt-map = <0 0 0 0 &cps_icu 0 ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-map = <0 0 0 0 &cps_icu ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 			interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
 
 			num-lanes = <1>;
-- 
2.13.5