Re: [PATCH] irqchip/ti-sci-intr: Add support for system suspend/resume PM

[Aswath Govindraju <a-govindraju@ti.com>]

Hi Marc,

On 07/06/22 12:48, Marc Zyngier wrote:
> On Tue, 07 Jun 2022 07:19:12 +0100,
> Aswath Govindraju <a-govindraju@ti.com> wrote:
>>
>> Add support for system level suspend/resume power management. The
>> interrupt mappings are stored in an array and restored in the system level
>> resume routine. Struct ti_sci_resource_desc can have atmost 2 sets for
>> ranges. Therefore, the mapping array is also formatted such that it can
>> store two sets of ranges.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/irqchip/irq-ti-sci-intr.c | 108 ++++++++++++++++++++++++++++++
>>  1 file changed, 108 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>> index fe8fad22bcf9..a8fc6cfb96ca 100644
>> --- a/drivers/irqchip/irq-ti-sci-intr.c
>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>> @@ -25,6 +25,7 @@
>>   * @dev:	Struct device pointer.
>>   * @ti_sci_id:	TI-SCI device identifier
>>   * @type:	Specifies the trigger type supported by this Interrupt Router
>> + * @mapping:	Pointer to out_irq <-> hwirq mapping table
>>   */
>>  struct ti_sci_intr_irq_domain {
>>  	const struct ti_sci_handle *sci;
>> @@ -32,6 +33,7 @@ struct ti_sci_intr_irq_domain {
>>  	struct device *dev;
>>  	u32 ti_sci_id;
>>  	u32 type;
>> +	u32 *mapping;
>>  };
>>  
>>  static struct irq_chip ti_sci_intr_irq_chip = {
>> @@ -99,6 +101,23 @@ static int ti_sci_intr_xlate_irq(struct ti_sci_intr_irq_domain *intr, u32 irq)
>>  	return -ENOENT;
>>  }
>>  
>> +/**
>> + * ti_sci_intr_free_irq - Free the irq entry in the out_irq <-> hwirq mapping table
>> + * @intr:	IRQ domain corresponding to Interrupt Router
>> + * @out_irq:	Out irq number
>> + */
>> +static void ti_sci_intr_free_irq(struct ti_sci_intr_irq_domain *intr, u16 out_irq)
>> +{
>> +	u16 start = intr->out_irqs->desc->start;
>> +	u16 num = intr->out_irqs->desc->num;
>> +	u16 start_sec = intr->out_irqs->desc->start_sec;
>> +
>> +	if (out_irq < start + num)
>> +		intr->mapping[out_irq - start] = 0xFFFFFFFF;
>> +	else
>> +		intr->mapping[out_irq - start_sec + num] = 0xFFFFFFFF;
>> +}
>> +
>>  /**
>>   * ti_sci_intr_irq_domain_free() - Free the specified IRQs from the domain.
>>   * @domain:	Domain to which the irqs belong
>> @@ -118,11 +137,30 @@ static void ti_sci_intr_irq_domain_free(struct irq_domain *domain,
>>  	intr->sci->ops.rm_irq_ops.free_irq(intr->sci,
>>  					   intr->ti_sci_id, data->hwirq,
>>  					   intr->ti_sci_id, out_irq);
>> +	ti_sci_intr_free_irq(intr, out_irq);
>>  	ti_sci_release_resource(intr->out_irqs, out_irq);
>>  	irq_domain_free_irqs_parent(domain, virq, 1);
>>  	irq_domain_reset_irq_data(data);
>>  }
>>  
>> +/**
>> + * ti_sci_intr_add_irq - Add the irq entry in the out_irq <-> hwirq mapping table
>> + * @intr:	IRQ domain corresponding to Interrupt Router
>> + * @hwirq:	Input irq number
>> + * @out_irq:	Out irq number
>> + */
>> +static void ti_sci_intr_add_irq(struct ti_sci_intr_irq_domain *intr, u32 hwirq, u16 out_irq)
>> +{
>> +	u16 start = intr->out_irqs->desc->start;
>> +	u16 num = intr->out_irqs->desc->num;
>> +	u16 start_sec = intr->out_irqs->desc->start_sec;
>> +
>> +	if (out_irq < start + num)
>> +		intr->mapping[out_irq - start] = hwirq;
>> +	else
>> +		intr->mapping[out_irq - start_sec + num] = hwirq;
>> +}
> 
> I'll bite: you already have a full resource allocator that is used for
> all sort of things. Why isn't this cached by the resource allocator
> itself? Why is this an irqchip specific thing? I expect other users of
> the same API to have the same needs.
> 

As, the resource allocator does not have enough memory to save and
restore all the mappings corresponding various resources, this is being
done on the requester or consumer side.

-- 
Thanks,
Aswath

> 	M.
>