Re: [PATCH v2 2/2] irqchip/ti-sci-inta: Add support for unmapped event handling

[Peter Ujfalusi]

On 30/09/2020 11.33, Marc Zyngier wrote:
> On 2020-09-30 08:45, Peter Ujfalusi wrote:
>> The DMA (BCDMA/PKTDMA and their rings/flows) events are under the INTA's
>> supervision as unmapped events in AM64.
> 
> What does "unmapped event" mean? An event that doesn't require a mapping?
> Or an internally generated event? Or a proxy event?
> 
>>
>> In order to keep the current SW stack working, the INTA driver must
>> replace
>> the dev_id with it's own when a request comes for BCDMA or PKTDMA
>> resources.
> 
> This seems to support my second option. Please clarify what you mean
> exactly.

In previous devices with NAVSS we had event registers in UDMAP and in rings.
In AM64 with DMSS these event registers have been removed from the DMAs
(rings are part of the DMAs).
The event configuration registers in DMSS are part of the INTA and they
under 'unmapped events'.

In essence the difference boils down to the fact that we do not
configure event numbers (to be consumed by INTA) for the events coming
from DMAs, but they already have their specific offsets within INTA's
unmapped region.
With this change in hardware we can not use the DMA's ti_sci device
identification number to configure these events to produce interrupts,
they are under INTA.

The only difference in ti_sci API is that for DMA related
interrupts/events we need to use the INTA's ti_sci device identification
number in place of the DMA component's.

I would say that I would describe the unmapped events with your first
option. They kind of not require mapping from the source to INTA. The
source of the event (DMA, ring) is pre-configured to send specific
events to target the unmapped event area at specific offsets.

But in the unmapped event register one can still define either:
- to send a global event (to be used as trigger or for other purposes)
- to generate interrupt (in a similar way as the IMAP registers were
doing in NAVSS).

I'm not sure if this answers your question...

- Péter

>>
>> Implement parsing of the optional "ti,unmapped-event-sources" phandle
>> array
>> to get the sci-dev-ids of the devices where the unmapped events
>> originate.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
>> ---
>>  drivers/irqchip/irq-ti-sci-inta.c | 72 +++++++++++++++++++++++++++++--
>>  1 file changed, 68 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-ti-sci-inta.c
>> b/drivers/irqchip/irq-ti-sci-inta.c
>> index bc863ef7998d..00f5b34863c5 100644
>> --- a/drivers/irqchip/irq-ti-sci-inta.c
>> +++ b/drivers/irqchip/irq-ti-sci-inta.c
>> @@ -85,6 +85,8 @@ struct ti_sci_inta_vint_desc {
>>   * @base:        Base address of the memory mapped IO registers
>>   * @pdev:        Pointer to platform device.
>>   * @ti_sci_id:        TI-SCI device identifier
>> + * @difu_cnt:        Number of TI-SCI device identifiers for unmapped
>> events
>> + * @dev_ids_for_unmapped: Pointer to an array of TI-SCI device
>> identifiers
>>   */
>>  struct ti_sci_inta_irq_domain {
>>      const struct ti_sci_handle *sci;
>> @@ -96,11 +98,33 @@ struct ti_sci_inta_irq_domain {
>>      void __iomem *base;
>>      struct platform_device *pdev;
>>      u32 ti_sci_id;
>> +
>> +    int difu_cnt;
> 
> I like silly acronyms as much as the next man, but please adopt
> sensible names. unmapped_cnt, or something.
> 
>> +    u32 *dev_ids_for_unmapped;
> 
> unmapped_dev_ids?
> 
>>  };
>>
>>  #define to_vint_desc(e, i) container_of(e, struct
>> ti_sci_inta_vint_desc, \
>>                      events[i])
>>
>> +static u16 ti_sci_inta_get_dev_id(struct ti_sci_inta_irq_domain *inta,
>> +                  u32 hwirq)
>> +{
>> +    u16 dev_id = HWIRQ_TO_DEVID(hwirq);
>> +    int i;
>> +
>> +    if (inta->difu_cnt == 0)
>> +        return dev_id;
>> +
>> +    for (i = 0; i < inta->difu_cnt; i++) {
>> +        if (dev_id == inta->dev_ids_for_unmapped[i]) {
>> +            dev_id = inta->ti_sci_id;
> 
> ti_sci_id is a u32, and you are implicitly casting it as u16.
> One of the two is wrong.
> 
>> +            break;
>> +        }
>> +    }
>> +
>> +    return dev_id;
>> +}
>> +
>>  /**
>>   * ti_sci_inta_irq_handler() - Chained IRQ handler for the vint irqs
>>   * @desc:    Pointer to irq_desc corresponding to the irq
>> @@ -251,7 +275,7 @@ static struct ti_sci_inta_event_desc
>> *ti_sci_inta_alloc_event(struct ti_sci_inta
>>      u16 dev_id, dev_index;
>>      int err;
>>
>> -    dev_id = HWIRQ_TO_DEVID(hwirq);
>> +    dev_id = ti_sci_inta_get_dev_id(inta, hwirq);
>>      dev_index = HWIRQ_TO_IRQID(hwirq);
>>
>>      event_desc = &vint_desc->events[free_bit];
>> @@ -352,14 +376,15 @@ static void ti_sci_inta_free_irq(struct
>> ti_sci_inta_event_desc *event_desc,
>>  {
>>      struct ti_sci_inta_vint_desc *vint_desc;
>>      struct ti_sci_inta_irq_domain *inta;
>> +    u16 dev_id;
>>
>>      vint_desc = to_vint_desc(event_desc, event_desc->vint_bit);
>>      inta = vint_desc->domain->host_data;
>> +    dev_id = ti_sci_inta_get_dev_id(inta, hwirq);
>>      /* free event irq */
>>      mutex_lock(&inta->vint_mutex);
>>      inta->sci->ops.rm_irq_ops.free_event_map(inta->sci,
>> -                         HWIRQ_TO_DEVID(hwirq),
>> -                         HWIRQ_TO_IRQID(hwirq),
>> +                         dev_id, HWIRQ_TO_IRQID(hwirq),
>>                           inta->ti_sci_id,
>>                           vint_desc->vint_id,
>>                           event_desc->global_event,
>> @@ -562,7 +587,6 @@ static void
>> ti_sci_inta_msi_set_desc(msi_alloc_info_t *arg,
>>      arg->desc = desc;
>>      arg->hwirq = TO_HWIRQ(pdev->id, desc->inta.dev_index);
>>  }
>> -
>>  static struct msi_domain_ops ti_sci_inta_msi_ops = {
>>      .set_desc    = ti_sci_inta_msi_set_desc,
>>  };
>> @@ -574,6 +598,42 @@ static struct msi_domain_info
>> ti_sci_inta_msi_domain_info = {
>>      .chip    = &ti_sci_inta_msi_irq_chip,
>>  };
>>
>> +static int ti_sci_inta_get_unmapped_sources(struct
>> ti_sci_inta_irq_domain *inta)
>> +{
>> +    struct device *dev = &inta->pdev->dev;
>> +    struct device_node *node = dev_of_node(dev);
>> +    struct of_phandle_iterator it;
>> +    int count, err, ret, i;
>> +
>> +    count = of_count_phandle_with_args(node,
>> "ti,unmapped-event-sources",
>> +                       NULL);
>> +    if (count <= 0)
>> +        return 0;
>> +
>> +    inta->dev_ids_for_unmapped = devm_kcalloc(dev, count,
>> +                    sizeof(*inta->dev_ids_for_unmapped),
>> +                    GFP_KERNEL);
>> +    if (!inta->dev_ids_for_unmapped)
>> +        return -ENOMEM;
>> +
>> +    i = 0;
>> +    of_for_each_phandle(&it, err, node, "ti,unmapped-event-sources",
>> +                NULL, 0) {
>> +        ret = of_property_read_u32(it.node, "ti,sci-dev-id",
>> +                       &inta->dev_ids_for_unmapped[i++]);
>> +        if (ret) {
>> +            dev_err(dev, "ti,sci-dev-id read failure for %s\n",
>> +                of_node_full_name(it.node));
> 
> Use the printk format specifier instead of of_node_full_name.
> 
>> +            of_node_put(it.node);
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    inta->difu_cnt = count;
>> +
>> +    return 0;
>> +}
>> +
>>  static int ti_sci_inta_irq_domain_probe(struct platform_device *pdev)
>>  {
>>      struct irq_domain *parent_domain, *domain, *msi_domain;
>> @@ -629,6 +689,10 @@ static int ti_sci_inta_irq_domain_probe(struct
>> platform_device *pdev)
>>      if (IS_ERR(inta->base))
>>          return PTR_ERR(inta->base);
>>
>> +    ret = ti_sci_inta_get_unmapped_sources(inta);
>> +    if (ret)
>> +        return ret;
>> +
>>      domain = irq_domain_add_linear(dev_of_node(dev),
>>                         ti_sci_get_num_resources(inta->vint),
>>                         &ti_sci_inta_irq_domain_ops, inta);
> 
>         M.


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki