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

The DMA (BCDMA/PKTDMA and their rings/flows) events are under the INTA's
supervision as unmapped events in AM64.

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.

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@ti.com>

---
 drivers/irqchip/irq-ti-sci-inta.c | 72 +++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 4 deletions(-)

-- 
Peter

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

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.

> 

> 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@ti.com>

> ---

>  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.
-- 
Jazz is not dead. It just smells funny...

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@ti.com>

>> ---

>>  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

On 2020-09-30 11:01, Peter Ujfalusi wrote:
> 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'.


Is "unmapped events" an official TI wording, described as such in the 
TRM?

> 

> 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.


OK, so they are not "unmapped". They are just permanently mapped, aren't 
they?

> 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.


If they truly don't require a mapping, why is this patch actually 
mapping
them with the INTA as a source? Your explanation doesn't make much sense
to me.

> 

> 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...


I still believe the term "unmapped event" doesn't describe what we have
here. These events seems, at least from what the patch does, internally
generated by the INTA.

Irrespective of this, my other comments still stand.

         M.
-- 
Jazz is not dead. It just smells funny...

On 30/09/2020 16.13, Marc Zyngier wrote:
> On 2020-09-30 11:01, Peter Ujfalusi wrote:

>> 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'.

> 

> Is "unmapped events" an official TI wording, described as such in the TRM?


Yes, this is the exact term.

>>

>> 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.

> 

> OK, so they are not "unmapped". They are just permanently mapped, aren't

> they?


The reason why these events are called as unmapped events is because
they are _not_ contained within the SoC level global event map -> they
are not mapped events -> unmapped events.
SW should be treating the unmapped event list as global events using
separate address mapping.
The mapped and unmapped event map space exists in parallel if we put it
this way.

>> 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.

> 

> If they truly don't require a mapping, why is this patch actually mapping

> them with the INTA as a source? Your explanation doesn't make much sense

> to me.


In DMSS lots of optimization went into the HW and one of them was to
remove the Output Event Steering Registers (OESR) from the DMAs and it's
rings. Well, not remove, but not add them as we have new DMAs.
Now INTA have a section to handle the unmapped events via the new
Unmapped Event Mapping Block.
This provides similar functionality as OESR did when they were present
on the parent IP. It also have the possibility to directly do unmapped
to virtual interrupt mapping, skipping the unmapped -> global event ->
local event flow.

The new INTA in DMSS supports the original global event to local event
handling (for ringacc for example) and it introduced these new unmapped
events.

To configure either type (mapped or unmapped global events) we have a
single API from SYSFW.
For mapped events the src_id is the source device identification (ring,
DMA channel, etc).
For unmapped events the src_id is the device identification number of
the INTA where the unmapped event is handled.

For mapped events we have OESR register in the source peripheral's
register space, this does not exists for unmapped event sources.

With the "ti,unmapped-event-sources" points to the devices from where
these unmapped events are originating and the inta driver will use the
appropriate src_id for SYSFW so client drivers do not need to know and
care about the differences between the two INTA.

>> 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...

> 

> I still believe the term "unmapped event" doesn't describe what we have

> here. These events seems, at least from what the patch does, internally

> generated by the INTA.


The INTA driver is ti-sci-inta, it relies on SYSFW interface solely, the
hardware is abstracted by the interrupt API: TISCI_MSG_RM_IRQ_SET and
TISCI_MSG_RM_IRQ_RELEASE.

But I do agree that the lack of OESR in the new DMA components and the
unmapped events can be viewed and implemented in a different way.

> Irrespective of this, my other comments still stand.

> 

>         M.


- Péter

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

Marc,

On 01/10/2020 14.42, Peter Ujfalusi wrote:
> 

> 

> On 30/09/2020 16.13, Marc Zyngier wrote:

>> On 2020-09-30 11:01, Peter Ujfalusi wrote:

>>> 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'.

>>

>> Is "unmapped events" an official TI wording, described as such in the TRM?

> 

> Yes, this is the exact term.

> 

>>>

>>> 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.

>>

>> OK, so they are not "unmapped". They are just permanently mapped, aren't

>> they?

> 

> The reason why these events are called as unmapped events is because

> they are _not_ contained within the SoC level global event map -> they

> are not mapped events -> unmapped events.

> SW should be treating the unmapped event list as global events using

> separate address mapping.

> The mapped and unmapped event map space exists in parallel if we put it

> this way.


The design of irqchip/irq-ti-sci-inta.c, soc/ti/ti_sci_inta_msi.c and
irqchip/irq-ti-sci-intr.c created to handle the interrupt needs present
in K3 devices with NAVSS.
DMSS of newer K3 devices extends and simplifies the NAVSS components and
a big part of that change was done with the INTA and DMAs.
System Firmware also changed to adopt to these changes.

As an example, let's assume that we want an interrupt from a ring.
The high level of the events in this case are:

NAVSS:
1.1 ring generates an internal signal (up or down)
1.2 the ringacc will send a (mapped) Global Event to INTA
1.3 When INTA receives the global event, it will signal it's outgoing
    VINT to INTR
1.4 INTR will trigger a host interrupt.

DMSS
1.1 ring generates an internal signal (up or down)
1.2 the DMA (ring is now part of the DMA) will send an unmapped event to
    INTA
1.3 When INTA receives the unmapped event, it will send a (mapped)
    Global Event to itself
1.4 When INTA receives the global event, it will signal it's outgoing
    VINT to INTR
1.5 INTR will trigger a host interrupt.

The API from sysfw is the same to configure the global events and VINT,
but we need to use the INTA's tisci device identification number to let
sysfw know that the Global event number can be programmed into INTA's
unmapped event steering register. The DMA no longer have this register,
it sends unmapped event to INTA.

The unmapped event number is fixed per sources, they will arrive at the
specific unmapped event configuration register of INTA.

INTA itself does not know which source are allocated to be used by
Linux, the allocation is for the DMA resources and only the DMA driver
knows which channels, rings, flows can be used and can ask the INTA MSI
domain to create interrupts for those.

By handling the ti,unmapped-event-sources the INTA driver can make
decision on the correct tisci dev_id to be used for the sysfw API to
where the global event must be configured and the client drivers does
not need to know how things are working under the hood.

There are components in DMSS which use is exactly how they worked within
NAVSS, they are not using unmapped events. Ringacc comes to mind first.

I can add a comment block to explain the nature of unmapped events and
the reason why we need to do what we do.

Would this be acceptable?

- Péter

>>> 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.

>>

>> If they truly don't require a mapping, why is this patch actually mapping

>> them with the INTA as a source? Your explanation doesn't make much sense

>> to me.

> 

> In DMSS lots of optimization went into the HW and one of them was to

> remove the Output Event Steering Registers (OESR) from the DMAs and it's

> rings. Well, not remove, but not add them as we have new DMAs.

> Now INTA have a section to handle the unmapped events via the new

> Unmapped Event Mapping Block.

> This provides similar functionality as OESR did when they were present

> on the parent IP. It also have the possibility to directly do unmapped

> to virtual interrupt mapping, skipping the unmapped -> global event ->

> local event flow.

> 

> The new INTA in DMSS supports the original global event to local event

> handling (for ringacc for example) and it introduced these new unmapped

> events.

> 

> To configure either type (mapped or unmapped global events) we have a

> single API from SYSFW.

> For mapped events the src_id is the source device identification (ring,

> DMA channel, etc).

> For unmapped events the src_id is the device identification number of

> the INTA where the unmapped event is handled.

> 

> For mapped events we have OESR register in the source peripheral's

> register space, this does not exists for unmapped event sources.

> 

> With the "ti,unmapped-event-sources" points to the devices from where

> these unmapped events are originating and the inta driver will use the

> appropriate src_id for SYSFW so client drivers do not need to know and

> care about the differences between the two INTA.

> 

>>> 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...

>>

>> I still believe the term "unmapped event" doesn't describe what we have

>> here. These events seems, at least from what the patch does, internally

>> generated by the INTA.

> 

> The INTA driver is ti-sci-inta, it relies on SYSFW interface solely, the

> hardware is abstracted by the interrupt API: TISCI_MSG_RM_IRQ_SET and

> TISCI_MSG_RM_IRQ_RELEASE.

> 

> But I do agree that the lack of OESR in the new DMA components and the

> unmapped events can be viewed and implemented in a different way.

> 

>> Irrespective of this, my other comments still stand.

>>

>>         M.

> 

> - Péter

> 

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.

> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

> 

> 

> _______________________________________________

> linux-arm-kernel mailing list

> linux-arm-kernel@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

> 


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

On 2020-10-09 09:58, Peter Ujfalusi wrote:
> Marc,

> 


[...]

> The design of irqchip/irq-ti-sci-inta.c, soc/ti/ti_sci_inta_msi.c and

> irqchip/irq-ti-sci-intr.c created to handle the interrupt needs present

> in K3 devices with NAVSS.

> DMSS of newer K3 devices extends and simplifies the NAVSS components 

> and

> a big part of that change was done with the INTA and DMAs.

> System Firmware also changed to adopt to these changes.

> 

> As an example, let's assume that we want an interrupt from a ring.

> The high level of the events in this case are:

> 

> NAVSS:

> 1.1 ring generates an internal signal (up or down)

> 1.2 the ringacc will send a (mapped) Global Event to INTA

> 1.3 When INTA receives the global event, it will signal it's outgoing

>     VINT to INTR

> 1.4 INTR will trigger a host interrupt.

> 

> DMSS

> 1.1 ring generates an internal signal (up or down)

> 1.2 the DMA (ring is now part of the DMA) will send an unmapped event 

> to

>     INTA

> 1.3 When INTA receives the unmapped event, it will send a (mapped)

>     Global Event to itself

> 1.4 When INTA receives the global event, it will signal it's outgoing

>     VINT to INTR

> 1.5 INTR will trigger a host interrupt.

> 

> The API from sysfw is the same to configure the global events and VINT,

> but we need to use the INTA's tisci device identification number to let

> sysfw know that the Global event number can be programmed into INTA's

> unmapped event steering register. The DMA no longer have this register,

> it sends unmapped event to INTA.

> 

> The unmapped event number is fixed per sources, they will arrive at the

> specific unmapped event configuration register of INTA.

> 

> INTA itself does not know which source are allocated to be used by

> Linux, the allocation is for the DMA resources and only the DMA driver

> knows which channels, rings, flows can be used and can ask the INTA MSI

> domain to create interrupts for those.

> 

> By handling the ti,unmapped-event-sources the INTA driver can make

> decision on the correct tisci dev_id to be used for the sysfw API to

> where the global event must be configured and the client drivers does

> not need to know how things are working under the hood.

> 

> There are components in DMSS which use is exactly how they worked 

> within

> NAVSS, they are not using unmapped events. Ringacc comes to mind first.

> 

> I can add a comment block to explain the nature of unmapped events and

> the reason why we need to do what we do.

> 

> Would this be acceptable?


That'd be useful, as long as it is shorter than the above.

         M.
-- 
Jazz is not dead. It just smells funny...

On 12/10/2020 10.31, Marc Zyngier wrote:
> On 2020-10-09 09:58, Peter Ujfalusi wrote:

>> Marc,

>>

> 

> [...]

> 

>> The design of irqchip/irq-ti-sci-inta.c, soc/ti/ti_sci_inta_msi.c and

>> irqchip/irq-ti-sci-intr.c created to handle the interrupt needs present

>> in K3 devices with NAVSS.

>> DMSS of newer K3 devices extends and simplifies the NAVSS components and

>> a big part of that change was done with the INTA and DMAs.

>> System Firmware also changed to adopt to these changes.

>>

>> As an example, let's assume that we want an interrupt from a ring.

>> The high level of the events in this case are:

>>

>> NAVSS:

>> 1.1 ring generates an internal signal (up or down)

>> 1.2 the ringacc will send a (mapped) Global Event to INTA

>> 1.3 When INTA receives the global event, it will signal it's outgoing

>>     VINT to INTR

>> 1.4 INTR will trigger a host interrupt.

>>

>> DMSS

>> 1.1 ring generates an internal signal (up or down)

>> 1.2 the DMA (ring is now part of the DMA) will send an unmapped event to

>>     INTA

>> 1.3 When INTA receives the unmapped event, it will send a (mapped)

>>     Global Event to itself

>> 1.4 When INTA receives the global event, it will signal it's outgoing

>>     VINT to INTR

>> 1.5 INTR will trigger a host interrupt.

>>

>> The API from sysfw is the same to configure the global events and VINT,

>> but we need to use the INTA's tisci device identification number to let

>> sysfw know that the Global event number can be programmed into INTA's

>> unmapped event steering register. The DMA no longer have this register,

>> it sends unmapped event to INTA.

>>

>> The unmapped event number is fixed per sources, they will arrive at the

>> specific unmapped event configuration register of INTA.

>>

>> INTA itself does not know which source are allocated to be used by

>> Linux, the allocation is for the DMA resources and only the DMA driver

>> knows which channels, rings, flows can be used and can ask the INTA MSI

>> domain to create interrupts for those.

>>

>> By handling the ti,unmapped-event-sources the INTA driver can make

>> decision on the correct tisci dev_id to be used for the sysfw API to

>> where the global event must be configured and the client drivers does

>> not need to know how things are working under the hood.

>>

>> There are components in DMSS which use is exactly how they worked within

>> NAVSS, they are not using unmapped events. Ringacc comes to mind first.

>>

>> I can add a comment block to explain the nature of unmapped events and

>> the reason why we need to do what we do.

>>

>> Would this be acceptable?

> 

> That'd be useful, as long as it is shorter than the above.


OK, I will send an update next week after I'm back.


- Péter

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

Marc,

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.


Looks like I forgot to reply on the other comments...

> 

>>

>> 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@ti.com>

>> ---

>>  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.


It is silly to my taste as well...

> 

>> +    u32 *dev_ids_for_unmapped;

> 

> unmapped_dev_ids?


OK, I will came up with more reasobnable names.

> 

>>  };

>>

>>  #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.


Documentation/devicetree/bindings/arm/keystone/ti,k3-sci-common.yaml
defines the ti,sci-dev-id as u32.
I will store them in u16 array in ti_sci_inta_get_unmapped_sources()

> 

>> +            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.


OK.

> 

>> +            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.


- Péter

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