Re: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt support.
On Jul 10, 2012, at 4:39 AM, Sethi Varun-B16395 wrote:
>
>
>> -Original Message-
>> From: Kumar Gala [mailto:ga...@kernel.crashing.org]
>> Sent: Tuesday, July 10, 2012 7:17 AM
>> To: Wood Scott-B07421
>> Cc: Sethi Varun-B16395; Hamciuc Bogdan-BHAMCIU1; linuxppc-
>> d...@lists.ozlabs.org
>> Subject: Re: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt
>> support.
>>
>>
>> On Jul 9, 2012, at 3:22 PM, Scott Wood wrote:
>>
>>> On 07/09/2012 02:03 PM, Kumar Gala wrote:
>>>>
>>>> On Jul 9, 2012, at 3:47 AM, Varun Sethi wrote:
>>>>
>>>>> +int mpic_err_int_init(struct mpic *mpic, irq_hw_number_t irqnum) {
>>>>
>>>> Why can't we do this during mpic_init() time?
>>>
>>> Are you willing to hardcode that IRQ 16 is the error interrupt,
>>> without waiting to see an intspec?
>>
>> I'm torn, but the bit of code in mpic_host_xlate that calls
>> mpic_err_int_init() is just ugly.
>>
>> We could consider it similar to how we assume IPIs.
> [Sethi Varun-B16395] I don't understand this point.
>
> -Varun
Just that we don't parse the .dts to get the IPI interrupts. They are just
assumed as part of being OpenPIC. So, I was suggesting if you set the
MPIC_FSL_HAS_EIMR flag, we just assume the IRQ #, rather than parsing the .dts.
- k
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt support.
> -Original Message-
> From: Kumar Gala [mailto:ga...@kernel.crashing.org]
> Sent: Tuesday, July 10, 2012 7:17 AM
> To: Wood Scott-B07421
> Cc: Sethi Varun-B16395; Hamciuc Bogdan-BHAMCIU1; linuxppc-
> d...@lists.ozlabs.org
> Subject: Re: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt
> support.
>
>
> On Jul 9, 2012, at 3:22 PM, Scott Wood wrote:
>
> > On 07/09/2012 02:03 PM, Kumar Gala wrote:
> >>
> >> On Jul 9, 2012, at 3:47 AM, Varun Sethi wrote:
> >>
> >>> +int mpic_err_int_init(struct mpic *mpic, irq_hw_number_t irqnum) {
> >>
> >> Why can't we do this during mpic_init() time?
> >
> > Are you willing to hardcode that IRQ 16 is the error interrupt,
> > without waiting to see an intspec?
>
> I'm torn, but the bit of code in mpic_host_xlate that calls
> mpic_err_int_init() is just ugly.
>
> We could consider it similar to how we assume IPIs.
[Sethi Varun-B16395] I don't understand this point.
-Varun
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt support.
>
> > + u32 eisr, eimr;
> > + int errint;
> > + unsigned int cascade_irq;
> > +
> > + eisr = fsl_mpic_err_read(mpic->err_regs, eisr_offset);
> > + eimr = fsl_mpic_err_read(mpic->err_regs, eimr_offset);
> > +
> > + if (!(eisr & ~eimr))
> > + return IRQ_NONE;
> > +
> > + while (eisr) {
> > + errint = __builtin_clz(eisr);
> > + cascade_irq = irq_linear_revmap(mpic->irqhost,
> > +mpic->err_int_vecs[errint]);
> > + WARN_ON(cascade_irq == NO_IRQ);
> > + if (cascade_irq != NO_IRQ) {
> > + generic_handle_irq(cascade_irq);
> > + } else {
> > + eimr |= 1 << (31 - errint);
> > + fsl_mpic_err_write(mpic->err_regs, eimr_offset, eimr);
> > + }
> > + eisr &= ~(1 << (31 - errint));
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +int mpic_err_int_init(struct mpic *mpic, irq_hw_number_t irqnum) {
>
> Why can't we do this during mpic_init() time?
>
[Sethi Varun-B16395] Don't want to hard code the error interrupt number.
> > + unsigned int virq;
> > + unsigned int offset = MPIC_ERR_INT_EIMR;
>
> remove offset, just use MPIC_ERR_INT_EIMR in mpic_err_write
>
> > + int ret;
> > +
> > + virq = irq_create_mapping(mpic->irqhost, irqnum);
> > + if (virq == NO_IRQ) {
> > + pr_err("Error interrupt setup failed\n");
> > + return -ENOSPC;
> > + }
> > +
> > + fsl_mpic_err_write(mpic->err_regs, offset, ~0);
>
> Add a comment about what this line is doing
>
[Sethi Varun-B16395] We are masking all the error interrupts here. I
Will add a comment for this.
> > +
> > + ret = request_irq(virq, fsl_error_int_handler, IRQF_NO_THREAD,
> > + "mpic-error-int", mpic);
>
> Hmm, should we be using irq_set_chained_handler() instead of request_irq
>
> > + if (ret) {
> > + pr_err("Failed to register error interrupt handler\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > index 61c7225..7002ef3 100644
> > --- a/arch/powerpc/sysdev/mpic.c
> > +++ b/arch/powerpc/sysdev/mpic.c
> > @@ -1026,6 +1026,9 @@ static int mpic_host_map(struct irq_domain *h,
> unsigned int virq,
> > return 0;
> > }
> >
> > + if (mpic_map_error_int(mpic, virq, hw))
> > + return 0;
> > +
> > if (hw >= mpic->num_sources)
> > return -EINVAL;
> >
> > @@ -1085,7 +1088,24 @@ static int mpic_host_xlate(struct irq_domain *h,
> struct device_node *ct,
> > */
> > switch (intspec[2]) {
> > case 0:
> > - case 1: /* no EISR/EIMR support for now, treat as shared IRQ
> */
> > + break;
> > + case 1:
> > + if (!(mpic->flags & MPIC_FSL_HAS_EIMR))
> > + break;
> > +
> > + if (intspec[3] >= ARRAY_SIZE(mpic->err_int_vecs))
> > + return -EINVAL;
> > +
> > + if (!mpic->err_int_config_done) {
> > + int ret;
> > + ret = mpic_err_int_init(mpic, intspec[0]);
> > + if (ret)
> > + return ret;
> > + mpic->err_int_config_done = 1;
> > + }
> > +
> > + *out_hwirq = mpic->err_int_vecs[intspec[3]];
> > +
> > break;
> > case 2:
> > if (intspec[0] >= ARRAY_SIZE(mpic->ipi_vecs)) @@ -
> 1302,6 +1322,8 @@
> > struct mpic * __init mpic_alloc(struct device_node *node,
> > mpic_map(mpic, mpic->paddr, &mpic->tmregs, MPIC_INFO(TIMER_BASE),
> > 0x1000);
> >
> > if (mpic->flags & MPIC_FSL) {
> > + u32 brr1, version;
> > +
> > /*
> > * Yes, Freescale really did put global registers in the
> > * magic per-cpu area -- and they don't even show up in the
> @@
> > -1309,6 +1331,17 @@ struct mpic * __init mpic_alloc(struct device_node
> *node,
> > */
> > mpic_map(mpic, mpic->paddr, &mpic->thiscpuregs,
> > MPIC_CPU_THISBASE, 0x1000);
> > +
> > + brr1 = _mpic_read(mpic->reg_type, &mpic->thiscpuregs,
> > + MPIC_FSL_BRR1);
> > + version = brr1 & MPIC_FSL_BRR1_VER;
> > +
> > + /* Error interrupt mask register (EIMR) is required for
> > +* handling individual device error interrupts. EIMR
> > +* was added in MPIC version 4.1.
> > +*/
> > + if (version >= 0x401)
> > + mpic_setup_error_int(mpic, intvec_top - 12);
>
> Would really like not to have this magic 12, but a comment would be nice
> if we keep it where the 12 comes from
>
[Sethi Varun-B16395]Obtaining vector numbers beyond ipi and timers for the
error interrupts.
Wi
Re: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt support.
On Jul 9, 2012, at 3:22 PM, Scott Wood wrote:
> On 07/09/2012 02:03 PM, Kumar Gala wrote:
>>
>> On Jul 9, 2012, at 3:47 AM, Varun Sethi wrote:
>>
>>> +int mpic_err_int_init(struct mpic *mpic, irq_hw_number_t irqnum)
>>> +{
>>
>> Why can't we do this during mpic_init() time?
>
> Are you willing to hardcode that IRQ 16 is the error interrupt, without
> waiting to see an intspec?
I'm torn, but the bit of code in mpic_host_xlate that calls mpic_err_int_init()
is just ugly.
We could consider it similar to how we assume IPIs.
>>> + ret = request_irq(virq, fsl_error_int_handler, IRQF_NO_THREAD,
>>> + "mpic-error-int", mpic);
>>
>> Hmm, should we be using irq_set_chained_handler() instead of request_irq
>
> As I said last time, "that's how Varun initially did it and I asked him
> to change it, because it gets a lot trickier to get things right, and I
> didn't see what it was buying us." That original patch had locking
> problems as a result.
>
> Using the chained handler mechanism puts the responsibility on us to do
> a lot of the generic stuff that's already perfectly well implemented in
> generic code. We're implementing a cascade, not a new flow.
Makes sense.
- k
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt support.
On 07/09/2012 02:03 PM, Kumar Gala wrote:
>
> On Jul 9, 2012, at 3:47 AM, Varun Sethi wrote:
>
>> +int mpic_err_int_init(struct mpic *mpic, irq_hw_number_t irqnum)
>> +{
>
> Why can't we do this during mpic_init() time?
Are you willing to hardcode that IRQ 16 is the error interrupt, without
waiting to see an intspec?
>> +ret = request_irq(virq, fsl_error_int_handler, IRQF_NO_THREAD,
>> +"mpic-error-int", mpic);
>
> Hmm, should we be using irq_set_chained_handler() instead of request_irq
As I said last time, "that's how Varun initially did it and I asked him
to change it, because it gets a lot trickier to get things right, and I
didn't see what it was buying us." That original patch had locking
problems as a result.
Using the chained handler mechanism puts the responsibility on us to do
a lot of the generic stuff that's already perfectly well implemented in
generic code. We're implementing a cascade, not a new flow.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3 v2] powerpc/mpic: FSL MPIC error interrupt support.
On Jul 9, 2012, at 3:47 AM, Varun Sethi wrote: > All SOC device error interrupts are muxed and delivered to the core as a > single > MPIC error interrupt. Currently all the device drivers requiring access to > device > errors have to register for the MPIC error interrupt as a shared interrupt. > > With this patch we add interrupt demuxing capability in the mpic driver, > allowing > device drivers to register for their individual error interrupts. This is > achieved > by handling error interrupts in a cascaded fashion. > > MPIC error interrupt is handled by the "error_int_handler", which > subsequently demuxes > it using the EISR and delivers it to the respective drivers. > > The error interrupt capability is dependent on the MPIC EIMR register, which > was > introduced in FSL MPIC version 4.1 (P4080 rev2). So, error interrupt demuxing > capability > is dependent on the MPIC version and can be used for versions >= 4.1. > > Signed-off-by: Varun Sethi > Signed-off-by: Bogdan Hamciuc > [In the initial version of the patch we were using handle_simple_irq > as the handler for cascaded error interrupts, this resulted > in issues in case of threaded isrs (with RT kernel). This issue was > debugged by Bogdan and decision was taken to use the handle_level_irq > handler] > --- > arch/powerpc/include/asm/mpic.h| 17 > arch/powerpc/sysdev/Makefile |2 +- > arch/powerpc/sysdev/fsl_mpic_err.c | 157 > arch/powerpc/sysdev/mpic.c | 35 - > arch/powerpc/sysdev/mpic.h | 22 + > 5 files changed, 231 insertions(+), 2 deletions(-) > create mode 100644 arch/powerpc/sysdev/fsl_mpic_err.c > > diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h > index e14d35d..71b42b9 100644 > --- a/arch/powerpc/include/asm/mpic.h > +++ b/arch/powerpc/include/asm/mpic.h > @@ -114,10 +114,17 @@ > #define MPIC_FSL_BRR1 0x0 > #define MPIC_FSL_BRR1_VER 0x > > +/* > + * Error interrupt registers > + */ > + > + > #define MPIC_MAX_IRQ_SOURCES 2048 > #define MPIC_MAX_CPUS 32 > #define MPIC_MAX_ISU 32 > > +#define MPIC_MAX_ERR 32 > + > /* > * Tsi108 implementation of MPIC has many differences from the original one > */ > @@ -270,6 +277,7 @@ struct mpic > struct irq_chip hc_ipi; > #endif > struct irq_chip hc_tm; > + struct irq_chip hc_err; > const char *name; > /* Flags */ > unsigned intflags; > @@ -283,6 +291,8 @@ struct mpic > /* vector numbers used for internal sources (ipi/timers) */ > unsigned intipi_vecs[4]; > unsigned inttimer_vecs[8]; > + /* vector numbers used for FSL MPIC error interrupts */ > + unsigned interr_int_vecs[MPIC_MAX_ERR]; > > /* Spurious vector to program into unused sources */ > unsigned intspurious_vec; > @@ -306,6 +316,11 @@ struct mpic > struct mpic_reg_bankcpuregs[MPIC_MAX_CPUS]; > struct mpic_reg_bankisus[MPIC_MAX_ISU]; > > + /* ioremap'ed base for error interrupt registers */ > + u32 __iomem *err_regs; > + /* error interrupt config */ > + u32 err_int_config_done; > + > /* Protected sources */ > unsigned long *protected; > > @@ -370,6 +385,8 @@ struct mpic > #define MPIC_NO_RESET 0x4000 > /* Freescale MPIC (compatible includes "fsl,mpic") */ > #define MPIC_FSL 0x8000 > +/* Freescale MPIC supports EIMR (error interrupt mask register)*/ > +#define MPIC_FSL_HAS_EIMR0x0001 > > /* MPIC HW modification ID */ > #define MPIC_REGSET_MASK 0xf000 > diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile > index 1bd7ecb..a57600b 100644 > --- a/arch/powerpc/sysdev/Makefile > +++ b/arch/powerpc/sysdev/Makefile > @@ -15,7 +15,7 @@ obj-$(CONFIG_PPC_DCR_NATIVE)+= dcr-low.o > obj-$(CONFIG_PPC_PMI) += pmi.o > obj-$(CONFIG_U3_DART) += dart_iommu.o > obj-$(CONFIG_MMIO_NVRAM) += mmio_nvram.o > -obj-$(CONFIG_FSL_SOC)+= fsl_soc.o > +obj-$(CONFIG_FSL_SOC)+= fsl_soc.o fsl_mpic_err.o > obj-$(CONFIG_FSL_PCI) += fsl_pci.o $(fsl-msi-obj-y) > obj-$(CONFIG_FSL_PMC) += fsl_pmc.o > obj-$(CONFIG_FSL_LBC) += fsl_lbc.o > diff --git a/arch/powerpc/sysdev/fsl_mpic_err.c > b/arch/powerpc/sysdev/fsl_mpic_err.c > new file mode 100644 > index 000..f2d28f2 > --- /dev/null > +++ b/arch/powerpc/sysdev/fsl_mpic_err.c > @@ -0,0 +1,157 @@ > +/* > + * Copyright (C) 2012 Freescale Semiconductor, Inc. > + * > + * Author: Varun Sethi > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; version 2 of the
