Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 08 Jun 2022 10:12:29 +0100
Message-ID: <871qvzr01u.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Aswath Govindraju <a-govindraju@ti.com>
Cc:     Vignesh Raghavendra <vigneshr@ti.com>, Nishanth Menon <nm@ti.com>,
        Tero Kristo <kristo@kernel.org>,
        Santosh Shilimkar <ssantosh@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] irqchip/ti-sci-intr: Add support for system suspend/resume PM
In-Reply-To: <0733f59a-497a-351b-0e97-26a2875a5352@ti.com>
References: <20220607061912.12222-1-a-govindraju@ti.com>
        <87sfohyma5.wl-maz@kernel.org>
        <0733f59a-497a-351b-0e97-26a2875a5352@ti.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 08 Jun 2022 09:48:20 +0100,
Aswath Govindraju <a-govindraju@ti.com> wrote:
> 
> 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.

You're missing the point: the ti_sci_resource structure is managed by
this resource allocator, and it isn't exactly rocket science to add
the required context to it, and then get it to restore that context on
resume.

This would actually give a sense of purpose to this stuff, which is
otherwise pretty useless.

	M.

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