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

[Suman Anna <s-anna@ti.com>]

On 7/3/20 12:04 PM, Grzegorz Jaszczyk wrote:
> On Thu, 2 Jul 2020 at 19:54, Marc Zyngier <maz@kernel.org> wrote:
>>
>> On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
>>> From: David Lechner <david@lechnology.com>
>>>
>>> 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@lechnology.com>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Reviewed-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>> 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@ti.com>
>>>    */
>>>
>>> +#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
>