| Message ID | 1451984370-16932-1-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 20f12758c9a837e9cafd7ced59f0b4c7a3961281 |
| Headers | show |
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@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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@10130000 { @@ -266,8 +270,8 @@ }; mmc@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@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@101e6000 { compatible = "arm,pl061", "arm,primecell"; reg = <0x101e6000 0x1000>; @@ -67,6 +77,13 @@ }; fpga { + mmc@5000 { + /* + * Overrides the interrupt assignment from + * the Versatile AB board file. + */ + interrupts-extended = <&sic 22 &sic 23>; + }; uart@9000 { compatible = "arm,pl011", "arm,primecell"; reg = <0x9000 0x1000>; @@ -86,7 +103,8 @@ mmc@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"; };
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@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(-) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html