| Message ID | 20161114121351.10924-7-Zubair.Kakakhel@imgtec.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 14.11.2016 13:13, Zubair Lutfullah Kakakhel wrote: > The powerpc dts file does not have the xlnx,kind-of-intr property. > Instead of erroring out, give a warning instead. And attempt to > continue to probe the interrupt controller while assuming > kind-of-intr is 0x0 as a fall back. > > Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> > > --- > V6 -> V7 > Rebase to v4.9-rc5 > > V5 -> V6 > Rebase to v4.9-rc3 > > V5 new patch > --- > drivers/irqchip/irq-xilinx-intc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c > index 971c141..d330917 100644 > --- a/drivers/irqchip/irq-xilinx-intc.c > +++ b/drivers/irqchip/irq-xilinx-intc.c > @@ -179,8 +179,8 @@ static int __init xilinx_intc_of_init(struct device_node *intc, > > ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask); > if (ret < 0) { > - pr_err("irq-xilinx: unable to read xlnx,kind-of-intr\n"); > - goto err_alloc; > + pr_warn("irq-xilinx: unable to read xlnx,kind-of-intr\n"); > + irqc->intr_mask = 0; > } > > if (irqc->intr_mask >> nr_irq) > Acked-by: Michal Simek <michal.simek@xilinx.com> Thanks, Michal
On Mon, 14 Nov 2016, Zubair Lutfullah Kakakhel wrote: > The powerpc dts file does not have the xlnx,kind-of-intr property. > Instead of erroring out, give a warning instead. And attempt to > continue to probe the interrupt controller while assuming > kind-of-intr is 0x0 as a fall back. This is broken, really. On multiplatform kernels this will try to probe the chip no matter what. Powerpc already has: static const struct of_device_id xilinx_intc_match[] __initconst = { { .compatible = "xlnx,opb-intc-1.00.c", }, { .compatible = "xlnx,xps-intc-1.00.a", }, {} }; Unless I'm missing something important, then adding those compatible strings to the driver will just keep stuff working as expected instead of adding unsafe and broken heuristics. Thanks, tglx
Hi, On 11/18/2016 01:29 PM, Thomas Gleixner wrote: > On Mon, 14 Nov 2016, Zubair Lutfullah Kakakhel wrote: > >> The powerpc dts file does not have the xlnx,kind-of-intr property. >> Instead of erroring out, give a warning instead. And attempt to >> continue to probe the interrupt controller while assuming >> kind-of-intr is 0x0 as a fall back. > > This is broken, really. On multiplatform kernels this will try to probe the > chip no matter what. I'm not sure I understand why this driver will probe on multi-platform kernels if the compatible string isn't in the DT? > > Powerpc already has: > > static const struct of_device_id xilinx_intc_match[] __initconst = { > { .compatible = "xlnx,opb-intc-1.00.c", }, > { .compatible = "xlnx,xps-intc-1.00.a", }, > {} > }; > > Unless I'm missing something important, then adding those compatible > strings to the driver will just keep stuff working as expected instead of > adding unsafe and broken heuristics. > The last two lines of the driver already specify the compatible strings. " IRQCHIP_DECLARE(xilinx_intc_xps, "xlnx,xps-intc-1.00.a", xilinx_intc_of_init); IRQCHIP_DECLARE(xilinx_intc_opb, "xlnx,opb-intc-1.00.c", xilinx_intc_of_init); " I'll elaborate on the commit message. The DT node in arch/powerpc for this driver is intc_0: interrupt-controller@81800000 { #interrupt-cells = <0x2>; compatible = "xlnx,xps-intc-1.00.a"; interrupt-controller ; reg = < 0x81800000 0x10000 >; xlnx,num-intr-inputs = <0xc>; } ; The DT node in arch/microblaze for this driver is intc_0: interrupt-controller@81800000 { #interrupt-cells = <0x2>; compatible = "xlnx,xps-intc-1.00.a"; interrupt-controller ; reg = < 0x81800000 0x10000 >; xlnx,kind-of-intr = <0x100>; //<Missing from ppc>! xlnx,num-intr-inputs = <0x9>; } ; The PPC driver assumes the kind-of-intr value be 0x0 and doesn't specify it in DT. This patch makes that a fall back case. Instead of completely error-ing out. Regards, ZubairLK > Thanks, > > tglx > >
On 21/11/16 14:05, Zubair Lutfullah Kakakhel wrote: > Hi, > > On 11/18/2016 01:29 PM, Thomas Gleixner wrote: >> On Mon, 14 Nov 2016, Zubair Lutfullah Kakakhel wrote: >> >>> The powerpc dts file does not have the xlnx,kind-of-intr property. >>> Instead of erroring out, give a warning instead. And attempt to >>> continue to probe the interrupt controller while assuming >>> kind-of-intr is 0x0 as a fall back. >> >> This is broken, really. On multiplatform kernels this will try to probe the >> chip no matter what. > > I'm not sure I understand why this driver will probe on multi-platform kernels > if the compatible string isn't in the DT? > >> >> Powerpc already has: >> >> static const struct of_device_id xilinx_intc_match[] __initconst = { >> { .compatible = "xlnx,opb-intc-1.00.c", }, >> { .compatible = "xlnx,xps-intc-1.00.a", }, >> {} >> }; >> >> Unless I'm missing something important, then adding those compatible >> strings to the driver will just keep stuff working as expected instead of >> adding unsafe and broken heuristics. >> > > The last two lines of the driver already specify the compatible strings. > > " > IRQCHIP_DECLARE(xilinx_intc_xps, "xlnx,xps-intc-1.00.a", xilinx_intc_of_init); > IRQCHIP_DECLARE(xilinx_intc_opb, "xlnx,opb-intc-1.00.c", xilinx_intc_of_init); > " Is PPC actually using this infrastructure? It predates the whole IRQCHIP_DECLARE business by about a decade. You seem to have tested it using QEMU, so I assume it "just works", but I'd feel more reassured it you stated so... Thanks, M.
Hi, On 11/21/2016 02:17 PM, Marc Zyngier wrote: > On 21/11/16 14:05, Zubair Lutfullah Kakakhel wrote: >> Hi, >> >> On 11/18/2016 01:29 PM, Thomas Gleixner wrote: >>> On Mon, 14 Nov 2016, Zubair Lutfullah Kakakhel wrote: >>> >>>> The powerpc dts file does not have the xlnx,kind-of-intr property. >>>> Instead of erroring out, give a warning instead. And attempt to >>>> continue to probe the interrupt controller while assuming >>>> kind-of-intr is 0x0 as a fall back. >>> >>> This is broken, really. On multiplatform kernels this will try to probe the >>> chip no matter what. >> >> I'm not sure I understand why this driver will probe on multi-platform kernels >> if the compatible string isn't in the DT? >> >>> >>> Powerpc already has: >>> >>> static const struct of_device_id xilinx_intc_match[] __initconst = { >>> { .compatible = "xlnx,opb-intc-1.00.c", }, >>> { .compatible = "xlnx,xps-intc-1.00.a", }, >>> {} >>> }; >>> >>> Unless I'm missing something important, then adding those compatible >>> strings to the driver will just keep stuff working as expected instead of >>> adding unsafe and broken heuristics. >>> >> >> The last two lines of the driver already specify the compatible strings. >> >> " >> IRQCHIP_DECLARE(xilinx_intc_xps, "xlnx,xps-intc-1.00.a", xilinx_intc_of_init); >> IRQCHIP_DECLARE(xilinx_intc_opb, "xlnx,opb-intc-1.00.c", xilinx_intc_of_init); >> " > > Is PPC actually using this infrastructure? It predates the whole > IRQCHIP_DECLARE business by about a decade. You seem to have tested it > using QEMU, so I assume it "just works", but I'd feel more reassured it > you stated so... I didn't realize that it could have been an issue. I simply included <linux/irqchip.h> and called irqchip_init() in the platform code instead of the previous initialization. Patch 7/7 in this series does that. And yes I tested it on QEMU. And it does look like it 'just works'. Without this patch, the UART driver would revert to polling and there would be various error messages about no irq domain found. With this patch, the 'no irq domain found' messages disappeared and the uart driver did get an irq. Regards, ZubairLK > > Thanks, > > M. >
On Mon, 21 Nov 2016, Zubair Lutfullah Kakakhel wrote: > On 11/18/2016 01:29 PM, Thomas Gleixner wrote: > I'll elaborate on the commit message. > > The DT node in arch/powerpc for this driver is > > intc_0: interrupt-controller@81800000 { > #interrupt-cells = <0x2>; > compatible = "xlnx,xps-intc-1.00.a"; > interrupt-controller ; > reg = < 0x81800000 0x10000 >; > xlnx,num-intr-inputs = <0xc>; > } ; > > The DT node in arch/microblaze for this driver is > > intc_0: interrupt-controller@81800000 { > #interrupt-cells = <0x2>; > compatible = "xlnx,xps-intc-1.00.a"; > interrupt-controller ; > reg = < 0x81800000 0x10000 >; > xlnx,kind-of-intr = <0x100>; //<Missing from ppc>! > xlnx,num-intr-inputs = <0x9>; > } ; > > The PPC driver assumes the kind-of-intr value be 0x0 and doesn't specify it in > DT. > This patch makes that a fall back case. Instead of completely error-ing out. Ok. makes sense. I misread the changelog/patch. Thanks, tglx
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c index 971c141..d330917 100644 --- a/drivers/irqchip/irq-xilinx-intc.c +++ b/drivers/irqchip/irq-xilinx-intc.c @@ -179,8 +179,8 @@ static int __init xilinx_intc_of_init(struct device_node *intc, ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask); if (ret < 0) { - pr_err("irq-xilinx: unable to read xlnx,kind-of-intr\n"); - goto err_alloc; + pr_warn("irq-xilinx: unable to read xlnx,kind-of-intr\n"); + irqc->intr_mask = 0; } if (irqc->intr_mask >> nr_irq)
The powerpc dts file does not have the xlnx,kind-of-intr property. Instead of erroring out, give a warning instead. And attempt to continue to probe the interrupt controller while assuming kind-of-intr is 0x0 as a fall back. Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> --- V6 -> V7 Rebase to v4.9-rc5 V5 -> V6 Rebase to v4.9-rc3 V5 new patch --- drivers/irqchip/irq-xilinx-intc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)