Re: [PATCH 21/24] irqchip: mips-gic: Support local interrupts

[Andrew Bresticker <abrestic@chromium.org>]

On Wed, Sep 17, 2014 at 10:40 AM, Andrew Bresticker
<abrestic@chromium.org> wrote:
> On Wed, Sep 17, 2014 at 2:50 AM, Qais Yousef <qais.yousef@imgtec.com> wrote:
>> On 09/16/2014 12:51 AM, Andrew Bresticker wrote:
>>>
>>> The MIPS GIC supports 7 local interrupts, 2 of which are the GIC
>>> local watchdog and count/compare timer.  The remainder are CPU
>>> interrupts which may optionally be re-routed through the GIC.
>>> GIC hardware IRQs 0-6 are now used for local interrupts while
>>> hardware IRQs 7+ are used for external (shared) interrupts.
>>>
>>> Note that the 5 CPU interrupts may not be re-routable through
>>> the GIC.  In that case mapping will fail and the vectors reported
>>> in C0_IntCtl should be used instead.  gic_get_c0_compare_int() and
>>> gic_get_c0_perfcount_int() will return the correct IRQ number to
>>> use for the C0 timer and perfcounter interrupts based on the
>>> routability of those interrupts through the GIC.
>>>
>>> Malta, SEAD-3, and the GIC clockevent driver have been updated
>>> to use local interrupts and the R4K clockevent driver has been
>>> updated to poll for C0 timer interrupts through the GIC when
>>> the GIC is present.
>>>
>>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>
>>> diff --git a/arch/mips/include/asm/mips-boards/maltaint.h
>>> b/arch/mips/include/asm/mips-boards/maltaint.h
>>> index bdd6f39..38b06a0 100644
>>> --- a/arch/mips/include/asm/mips-boards/maltaint.h
>>> +++ b/arch/mips/include/asm/mips-boards/maltaint.h
>>> @@ -10,6 +10,8 @@
>>>   #ifndef _MIPS_MALTAINT_H
>>>   #define _MIPS_MALTAINT_H
>>>   +#include <asm/gic.h>
>>> +
>>
>>
>> nit: I think gic.h should be split to driver/irqchip/irq-mips-gic.h for
>> private definitions and include/linux/irqchip/irq-mips-gic.h for
>> exported/public definitions.
>
> Yup, I was planning on doing this in the next series :).  Malta and
> the clockevent/clocksource driver need to get cleaned up first though,
> so that they don't have to use the private register definitions.
>
>>> diff --git a/drivers/irqchip/irq-mips-gic.c
>>> b/drivers/irqchip/irq-mips-gic.c
>>> index 6682a4e..3abe310 100644
>>> --- a/drivers/irqchip/irq-mips-gic.c
>>> +++ b/drivers/irqchip/irq-mips-gic.c
>
>>> @@ -95,12 +96,39 @@ cycle_t gic_read_compare(void)
>>>   }
>>>   #endif
>>>   +static bool gic_local_irq_is_routable(int intr)
>>> +{
>>> +       u32 vpe_ctl;
>>> +
>>> +       /* All local interrupts are routable in EIC mode. */
>>> +       if (cpu_has_veic)
>>> +               return true;
>>> +
>>> +       GICREAD(GIC_REG(VPE_LOCAL, GIC_VPE_CTL), vpe_ctl);
>>> +       switch (intr) {
>>> +       case GIC_LOCAL_INT_TIMER:
>>> +               return vpe_ctl & GIC_VPE_CTL_TIMER_RTBL_MSK;
>>> +       case GIC_LOCAL_INT_PERFCTR:
>>> +               return vpe_ctl & GIC_VPE_CTL_PERFCNT_RTBL_MSK;
>>> +       case GIC_LOCAL_INT_FDC:
>>> +               return vpe_ctl & GIC_VPE_CTL_FDC_RTBL_MSK;
>>> +       case GIC_LOCAL_INT_SWINT0:
>>> +       case GIC_LOCAL_INT_SWINT1:
>>> +               /*
>>> +                * SWINT{0,1} are not routable in non-EIC mode, regardless
>>> +                * of the setting of SWINT_ROUTABLE.
>>> +                */
>>> +               return false;
>>
>>
>> Hmm AFAIK they are routable. Actually from hard reset they're automatically
>> routed to vpe0 pin 0 which caught me a number of times when trying to use
>> software interrupt on hardware that has GIC. When setting software interrupt
>> I was seeing pin 0 going high too and thought it's a hardware bug for a
>> while.
>
> Interesting, the interAptiv User's Manual I have, in section 9.4.7.1,
> says: "Note that Software Interrupts from the VPE are routed
> internally by the CPU in vectored interrupt mode, and are only routed
> through the GIC when the GIC is in EIC mode, regardless of the
> GIC_VPEi_CTL register."  IIRC, there's a similar statement in the
> proAptiv manual as well.  I didn't play with the SWINT bits myself, so
> I wouldn't be surprised if that's wrong :).
>
>> I think all local interrupts should be masked at GIC initialisation except
>> for timer interrupt. I was preparing a set of patches for GIC but you beat
>> me into it :)
>
> If SWINT gets routed both through the GIC and directly to the CPU,
> then that's probably the best thing to do.  I suspect we'll also want
> to leave the performance counter interrupt unmasked too, since,
> unfortunately, both the HW perf event driver and oprofile do not use
> the percpu IRQ API.

Instead of leaving those two unmasked, perhaps the better thing to do
is to have a separate irq_chip which masks/unmasks on all VPEs.