Re: [PATCHv3 4/6] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops

[Suman Anna]

On 7/3/20 12:04 PM, Grzegorz Jaszczyk wrote:
> On Thu, 2 Jul 2020 at 19:54, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
> > > From: David Lechner <david@xxxxxxxxxxxxxx>
> > >
> > > This implements the irq_get_irqchip_state and irq_set_irqchip_state
> > > callbacks for the TI PRUSS INTC driver. The set callback can be used
> > > by drivers to "kick" a PRU by enabling a PRU system event.
> >
> > "enabling"? That'd be unmasking an interrupt, which isn't what this
> > does. "injecting", maybe?
>
> Yes "injecting" is much better.
>
> > > Example:
> > >       irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, true);
> >
> > Nice example.
> >
> > What this example does explain is how you are actually going to kick
> > a PRU via this interface. For that to happen, you'd have to have on
> > the Linux side an interrupt that is actually routed to a PRU.
>
> Correct.
>
> > And from what I have understood of the previous patches, this can't
> > be the case. What didi I miss?
>
> The hwirq's handled by this driver are so called system events in
> PRUSS nomenclature. This driver is responsible for the entire mapping
> of those system events to PRUSS specific channels which are next
> mapped to host_irq (patch #6 https://lkml.org/lkml/2020/7/2/612).
> There are 8 host_irqs that are routed to the main cpu (running Linux)
> and they are called host_intr0..host_intr7 (were seen in previous
> patches of this series). But there are other "host_interrupts" that
> are routed not to the main CPU but to PRU cores and this driver is
> responsible for creating proper mapping (system
> event/channel/host_irq) for them, and allowing to kick PRU via the
> introduced interface.
>
> It is worth noting that the PRUSS is quite flexible and allows various
> things e.g.:
> - map any of 160/64 internal system events to any of the 20/10 channels
> - map any of the 20/10 channels to any of the 20/10 host interrupts.
>
> So e.g. it is possible to map e.g. system event 17 to the main CPU
> (through e.g. channel 1 which is the next map to e.g. host_intr0). Or
> (exclusively) map the same system event 17 to PRU core (through e.g.
> channel 1 which is the next map to PRU0).

Grzegorz has explained in detail, the short summary is we trigger a 
system event, and the mapping for that event ensures the interrupt is 
routed at the desired processor.

regards
Suman

> > > Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>
> > > Signed-off-by: Suman Anna <s-anna@xxxxxx>
> > > Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@xxxxxxxxxx>
> > > Reviewed-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > > ---
> > > v2->v3:
> > > - Get rid of unnecessary pruss_intc_check_write() and use
> > >    pruss_intc_write_reg directly.
> > > v1->v2:
> > > - https://patchwork.kernel.org/patch/11069769/
> > > ---
> > >   drivers/irqchip/irq-pruss-intc.c | 43
> > > ++++++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 41 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-pruss-intc.c
> > > b/drivers/irqchip/irq-pruss-intc.c
> > > index 49c936f..19b3d38 100644
> > > --- a/drivers/irqchip/irq-pruss-intc.c
> > > +++ b/drivers/irqchip/irq-pruss-intc.c
> > > @@ -7,6 +7,7 @@
> > >    *   Suman Anna <s-anna@xxxxxx>
> > >    */
> > >
> > > +#include <linux/interrupt.h>
> > >   #include <linux/irq.h>
> > >   #include <linux/irqchip/chained_irq.h>
> > >   #include <linux/irqdomain.h>
> > > @@ -39,8 +40,7 @@
> > >   #define PRU_INTC_HIEISR              0x0034
> > >   #define PRU_INTC_HIDISR              0x0038
> > >   #define PRU_INTC_GPIR                0x0080
> > > -#define PRU_INTC_SRSR0               0x0200
> > > -#define PRU_INTC_SRSR1               0x0204
> > > +#define PRU_INTC_SRSR(x)     (0x0200 + (x) * 4)
> > >   #define PRU_INTC_SECR0               0x0280
> > >   #define PRU_INTC_SECR1               0x0284
> > >   #define PRU_INTC_ESR0                0x0300
> > > @@ -145,6 +145,43 @@ static void pruss_intc_irq_relres(struct irq_data
> > > *data)
> > >        module_put(THIS_MODULE);
> > >   }
> > >
> > > +static int pruss_intc_irq_get_irqchip_state(struct irq_data *data,
> > > +                                         enum irqchip_irq_state which,
> > > +                                         bool *state)
> > > +{
> > > +     struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
> > > +     u32 reg, mask, srsr;
> > > +
> > > +     if (which != IRQCHIP_STATE_PENDING)
> > > +             return -EINVAL;
> > > +
> > > +     reg = PRU_INTC_SRSR(data->hwirq / 32);
> >
> > I assume the register file scales as more interrupts are added in the
> > subsequent patch?
> Yes, after I will move part of the next patch to patch #2 as you
> suggested it will stop being confusing.
>
> Thank you,
> Grzegorz