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

[Marc Zyngier <maz@kernel.org>]

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.

	M.

-- 
Without deviation from the norm, progress is not possible.