[PATCH 2/2] ARM: versatile: fix MMC/SD interrupt assignment

[PATCH 2/2] ARM: versatile: fix MMC/SD interrupt assignment

[Linus Walleij]

Commit 0976c946a610d06e907335b7a3afa6db046f8e1b
"arm/versatile: Fix versatile irq specifications"
has an off-by-one error on the Versatile AB that has
been regressing the Versatile AB hardware for some time.

However it seems like the interrupt assignments have
never been correct and I have now adjusted them according
to the specification. The masks for the valid interrupts
made it impossible to assign the right SIC interrupt
for the MMCI, so I went in and fixed these to correspond
to the specifications, and added references if anyone
wants to double-check.

Due to the Versatile PB including the Versatile AB
as a base DTS file, we need to override and correct
some values to correspond to the actual changes in the
hardware.

For the Versatile PB I don't think the IRQ line
assignment for MMCI has ever been correct for either of
the two MMCI blocks. It would be nice if someone with the
physical PB board could test this.

Patch tested on the Versatile AB, QEMU for Versatile AB
and QEMU for Versatile PB.

Cc: Rob Herring <robh@kernel.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: stable at vger.kernel.org
Fixes: 0976c946a610 ("arm/versatile: Fix versatile irq specifications")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ARM SoC people: please apply this directly for fixes if
you find it OK.
---
 arch/arm/boot/dts/versatile-ab.dts | 10 +++++++---
 arch/arm/boot/dts/versatile-pb.dts | 20 +++++++++++++++++++-
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
index 01f40197ea13..3279bf1a17a1 100644
--- a/arch/arm/boot/dts/versatile-ab.dts
+++ b/arch/arm/boot/dts/versatile-ab.dts
@@ -110,7 +110,11 @@
 			interrupt-parent = <&vic>;
 			interrupts = <31>; /* Cascaded to vic */
 			clear-mask = <0xffffffff>;
-			valid-mask = <0xffc203f8>;
+			/*
+			 * Valid interrupt lines mask according to
+			 * table 4-36 page 4-50 of ARM DUI 0225D
+			 */
+			valid-mask = <0x0760031b>;
 		};
 
 		dma at 10130000 {
@@ -266,8 +270,8 @@
 			};
 			mmc at 5000 {
 				compatible = "arm,pl180", "arm,primecell";
-				reg = < 0x5000 0x1000>;
-				interrupts-extended = <&vic 22 &sic 2>;
+				reg = <0x5000 0x1000>;
+				interrupts-extended = <&vic 22 &sic 1>;
 				clocks = <&xtal24mhz>, <&pclk>;
 				clock-names = "mclk", "apb_pclk";
 			};
diff --git a/arch/arm/boot/dts/versatile-pb.dts b/arch/arm/boot/dts/versatile-pb.dts
index b83137f66034..33a8eb28374e 100644
--- a/arch/arm/boot/dts/versatile-pb.dts
+++ b/arch/arm/boot/dts/versatile-pb.dts
@@ -5,6 +5,16 @@
 	compatible = "arm,versatile-pb";
 
 	amba {
+		/* The Versatile PB is using more SIC IRQ lines than the AB */
+		sic: intc at 10003000 {
+			clear-mask = <0xffffffff>;
+			/*
+			 * Valid interrupt lines mask according to
+			 * figure 3-30 page 3-74 of ARM DUI 0224B
+			 */
+			valid-mask = <0x7fe003ff>;
+		};
+
 		gpio2: gpio at 101e6000 {
 			compatible = "arm,pl061", "arm,primecell";
 			reg = <0x101e6000 0x1000>;
@@ -67,6 +77,13 @@
 		};
 
 		fpga {
+			mmc at 5000 {
+				/*
+				 * Overrides the interrupt assignment from
+				 * the Versatile AB board file.
+				 */
+				interrupts-extended = <&sic 22 &sic 23>;
+			};
 			uart at 9000 {
 				compatible = "arm,pl011", "arm,primecell";
 				reg = <0x9000 0x1000>;
@@ -86,7 +103,8 @@
 			mmc at b000 {
 				compatible = "arm,pl180", "arm,primecell";
 				reg = <0xb000 0x1000>;
-				interrupts-extended = <&vic 23 &sic 2>;
+				interrupt-parent = <&sic>;
+				interrupts = <1>, <2>;
 				clocks = <&xtal24mhz>, <&pclk>;
 				clock-names = "mclk", "apb_pclk";
 			};
-- 
2.4.3

[PATCH 2/2] ARM: versatile: fix MMC/SD interrupt assignment

[Rob Herring]

On Tue, Jan 5, 2016 at 2:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Commit 0976c946a610d06e907335b7a3afa6db046f8e1b
> "arm/versatile: Fix versatile irq specifications"
> has an off-by-one error on the Versatile AB that has
> been regressing the Versatile AB hardware for some time.
>
> However it seems like the interrupt assignments have
> never been correct and I have now adjusted them according
> to the specification. The masks for the valid interrupts
> made it impossible to assign the right SIC interrupt
> for the MMCI, so I went in and fixed these to correspond
> to the specifications, and added references if anyone
> wants to double-check.
>
> Due to the Versatile PB including the Versatile AB
> as a base DTS file, we need to override and correct
> some values to correspond to the actual changes in the
> hardware.
>
> For the Versatile PB I don't think the IRQ line
> assignment for MMCI has ever been correct for either of
> the two MMCI blocks. It would be nice if someone with the
> physical PB board could test this.
>
> Patch tested on the Versatile AB, QEMU for Versatile AB
> and QEMU for Versatile PB.
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: stable at vger.kernel.org
> Fixes: 0976c946a610 ("arm/versatile: Fix versatile irq specifications")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ARM SoC people: please apply this directly for fixes if
> you find it OK.
> ---
>  arch/arm/boot/dts/versatile-ab.dts | 10 +++++++---
>  arch/arm/boot/dts/versatile-pb.dts | 20 +++++++++++++++++++-
>  2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
> index 01f40197ea13..3279bf1a17a1 100644
> --- a/arch/arm/boot/dts/versatile-ab.dts
> +++ b/arch/arm/boot/dts/versatile-ab.dts
> @@ -110,7 +110,11 @@
>                         interrupt-parent = <&vic>;
>                         interrupts = <31>; /* Cascaded to vic */
>                         clear-mask = <0xffffffff>;
> -                       valid-mask = <0xffc203f8>;
> +                       /*
> +                        * Valid interrupt lines mask according to
> +                        * table 4-36 page 4-50 of ARM DUI 0225D
> +                        */
> +                       valid-mask = <0x0760031b>;

I never really liked valid-mask in the first place. Valid interrupts
are the ones specified by devices and we don't need this extra data.
If we do, then *every* interrupt controller needs this property.
Perhaps the driver should just ignore it. But you've already done the
work here, so this is okay too.

Rob

[PATCH 2/2] ARM: versatile: fix MMC/SD interrupt assignment

[Linus Walleij]

On Wed, Jan 6, 2016 at 12:43 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 5, 2016 at 2:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
>> index 01f40197ea13..3279bf1a17a1 100644
>> --- a/arch/arm/boot/dts/versatile-ab.dts
>> +++ b/arch/arm/boot/dts/versatile-ab.dts
>> @@ -110,7 +110,11 @@
>>                         interrupt-parent = <&vic>;
>>                         interrupts = <31>; /* Cascaded to vic */
>>                         clear-mask = <0xffffffff>;
>> -                       valid-mask = <0xffc203f8>;
>> +                       /*
>> +                        * Valid interrupt lines mask according to
>> +                        * table 4-36 page 4-50 of ARM DUI 0225D
>> +                        */
>> +                       valid-mask = <0x0760031b>;
>
> I never really liked valid-mask in the first place. Valid interrupts
> are the ones specified by devices and we don't need this extra data.
> If we do, then *every* interrupt controller needs this property.
> Perhaps the driver should just ignore it. But you've already done the
> work here, so this is okay too.

It's a migrated feature from the boardfile days as it happens.

Before commit d7057e1de8d6c180eb2ecd90b0cab9d1a8a4d8ab
"ARM: integrator: delete non-devicetree boot path"
the Integrator/CP was doing this:

static void __init intcp_init_irq(void)
{
       u32 pic_mask, cic_mask, sic_mask;

       /* These masks are for the HW IRQ registers */
       pic_mask = ~((~0u) << (11 - 0));
       pic_mask |= (~((~0u) << (29 - 22))) << 22;
       cic_mask = ~((~0u) << (1 + IRQ_CIC_END - IRQ_CIC_START));
       sic_mask = ~((~0u) << (1 + IRQ_SIC_END - IRQ_SIC_START));

       /*
        * Disable all interrupt sources
        */
       writel(0xffffffff, INTCP_VA_PIC_BASE + IRQ_ENABLE_CLEAR);
       writel(0xffffffff, INTCP_VA_PIC_BASE + FIQ_ENABLE_CLEAR);
       writel(0xffffffff, INTCP_VA_CIC_BASE + IRQ_ENABLE_CLEAR);
       writel(0xffffffff, INTCP_VA_CIC_BASE + FIQ_ENABLE_CLEAR);
       writel(sic_mask, INTCP_VA_SIC_BASE + IRQ_ENABLE_CLEAR);
       writel(sic_mask, INTCP_VA_SIC_BASE + FIQ_ENABLE_CLEAR);

       fpga_irq_init(INTCP_VA_PIC_BASE, "PIC", IRQ_PIC_START,
                     -1, pic_mask, NULL);

       fpga_irq_init(INTCP_VA_CIC_BASE, "CIC", IRQ_CIC_START,
                     -1, cic_mask, NULL);

       fpga_irq_init(INTCP_VA_SIC_BASE, "SIC", IRQ_SIC_START,
                     IRQ_CP_CPPLDINT, sic_mask, NULL);
(...)

The clear-mask and valid-mask:s found in
arch/arm/boot/dts/integratorcp.dts comes from this for example.
And that is a modernization of code that was always there.

I think it's because the Integrator will crash if you try to even
clear a non-existing interrupt by writing a '1' in the wrong bit,
and that is why we have the clear-mask. And valid-mask is
to avoid even mapping one of these non-existing IRQs.
The issue doesn't seem to exist on the Versatile (I tried
with 0xffffffff and it worked) and probably not on Integrator
QEMU either, but on the real hardware I've experienced it.

So the FPGA interrupt controller is sensitive stuff ... and it
seemed natural to preserve this overzealous code
(and bindings).

Yours,
Linus Walleij

[PATCH 2/2] ARM: versatile: fix MMC/SD interrupt assignment

[Rob Herring]

On Thu, Jan 7, 2016 at 3:58 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jan 6, 2016 at 12:43 AM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Jan 5, 2016 at 2:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
>>> index 01f40197ea13..3279bf1a17a1 100644
>>> --- a/arch/arm/boot/dts/versatile-ab.dts
>>> +++ b/arch/arm/boot/dts/versatile-ab.dts
>>> @@ -110,7 +110,11 @@
>>>                         interrupt-parent = <&vic>;
>>>                         interrupts = <31>; /* Cascaded to vic */
>>>                         clear-mask = <0xffffffff>;
>>> -                       valid-mask = <0xffc203f8>;
>>> +                       /*
>>> +                        * Valid interrupt lines mask according to
>>> +                        * table 4-36 page 4-50 of ARM DUI 0225D
>>> +                        */
>>> +                       valid-mask = <0x0760031b>;
>>
>> I never really liked valid-mask in the first place. Valid interrupts
>> are the ones specified by devices and we don't need this extra data.
>> If we do, then *every* interrupt controller needs this property.
>> Perhaps the driver should just ignore it. But you've already done the
>> work here, so this is okay too.
>
> It's a migrated feature from the boardfile days as it happens.

[...]

> I think it's because the Integrator will crash if you try to even
> clear a non-existing interrupt by writing a '1' in the wrong bit,
> and that is why we have the clear-mask. And valid-mask is
> to avoid even mapping one of these non-existing IRQs.
> The issue doesn't seem to exist on the Versatile (I tried
> with 0xffffffff and it worked) and probably not on Integrator
> QEMU either, but on the real hardware I've experienced it.
>
> So the FPGA interrupt controller is sensitive stuff ... and it
> seemed natural to preserve this overzealous code
> (and bindings).

Okay, well this is certainly a better reason than I had heard before.

Acked-by: Rob Herring <robh@kernel.org>

Rob

[PATCH 2/2] ARM: versatile: fix MMC/SD interrupt assignment

[Olof Johansson]

On Tue, Jan 05, 2016 at 09:59:30AM +0100, Linus Walleij wrote:
> Commit 0976c946a610d06e907335b7a3afa6db046f8e1b
> "arm/versatile: Fix versatile irq specifications"
> has an off-by-one error on the Versatile AB that has
> been regressing the Versatile AB hardware for some time.
> 
> However it seems like the interrupt assignments have
> never been correct and I have now adjusted them according
> to the specification. The masks for the valid interrupts
> made it impossible to assign the right SIC interrupt
> for the MMCI, so I went in and fixed these to correspond
> to the specifications, and added references if anyone
> wants to double-check.
> 
> Due to the Versatile PB including the Versatile AB
> as a base DTS file, we need to override and correct
> some values to correspond to the actual changes in the
> hardware.
> 
> For the Versatile PB I don't think the IRQ line
> assignment for MMCI has ever been correct for either of
> the two MMCI blocks. It would be nice if someone with the
> physical PB board could test this.
> 
> Patch tested on the Versatile AB, QEMU for Versatile AB
> and QEMU for Versatile PB.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: stable at vger.kernel.org
> Fixes: 0976c946a610 ("arm/versatile: Fix versatile irq specifications")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ARM SoC people: please apply this directly for fixes if
> you find it OK.

Applied to fixes. Seems like Rob's comments were concerns but not
blockers, so picking up the fix seems to make sense.


-Olof