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

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

[Aswath Govindraju]

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;
+}
+
 /**
  * ti_sci_intr_alloc_parent_irq() - Allocate parent IRQ
  * @domain:	Pointer to the interrupt router IRQ domain
@@ -173,6 +211,9 @@ static int ti_sci_intr_alloc_parent_irq(struct irq_domain *domain,
 	if (err)
 		goto err_msg;
 
+	/* Adding out_irq <-> hwirq to mapping */
+	ti_sci_intr_add_irq(intr, hwirq, out_irq);
+
 	return out_irq;
 
 err_msg:
@@ -221,6 +262,26 @@ static const struct irq_domain_ops ti_sci_intr_irq_domain_ops = {
 	.translate	= ti_sci_intr_irq_domain_translate,
 };
 
+/**
+ * ti_sci_intr_initialize_mappingg - Initialize the out_irq <-> hwirq mapping table
+ * @intr:	IRQ domain corresponding to Interrupt Router
+ */
+static int ti_sci_intr_initialize_mapping(struct ti_sci_intr_irq_domain *intr)
+{
+	int i;
+	int mapping_len = intr->out_irqs->desc->num + intr->out_irqs->desc->num_sec;
+
+	intr->mapping = devm_kzalloc(intr->dev, mapping_len * sizeof(u32), GFP_KERNEL);
+	if (!intr->mapping)
+		return -ENOMEM;
+
+	/* Set all the elements in the array to max value of u32 */
+	for (i = 0; i < mapping_len; i++)
+		intr->mapping[i] = 0xFFFFFFFF;
+
+	return 0;
+}
+
 static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
 {
 	struct irq_domain *parent_domain, *domain;
@@ -246,6 +307,8 @@ static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	intr->dev = dev;
+	platform_set_drvdata(pdev, intr);
+
 	ret = of_property_read_u32(dev_of_node(dev), "ti,intr-trigger-type",
 				   &intr->type);
 	if (ret) {
@@ -273,6 +336,10 @@ static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
 		return PTR_ERR(intr->out_irqs);
 	}
 
+	ret = ti_sci_intr_initialize_mapping(intr);
+	if (ret)
+		return ret;
+
 	domain = irq_domain_add_hierarchy(parent_domain, 0, 0, dev_of_node(dev),
 					  &ti_sci_intr_irq_domain_ops, intr);
 	if (!domain) {
@@ -285,6 +352,46 @@ static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int ti_sci_intr_restore_irqs(struct ti_sci_intr_irq_domain *intr)
+{
+	int i, err = 0;
+	u16 start = intr->out_irqs->desc->start;
+	u16 num = intr->out_irqs->desc->num;
+	u16 start_sec = intr->out_irqs->desc->start_sec;
+	u16 num_sec = intr->out_irqs->desc->num_sec;
+
+	for (i = 0; i < num + num_sec; i++) {
+		if (intr->mapping[i] == 0xFFFFFFFF)
+			continue;
+
+		if (i < num)
+			err = intr->sci->ops.rm_irq_ops.set_irq(intr->sci,
+								intr->ti_sci_id, intr->mapping[i],
+								intr->ti_sci_id, i + start);
+
+		else
+			err = intr->sci->ops.rm_irq_ops.set_irq(intr->sci,
+								intr->ti_sci_id, intr->mapping[i],
+								intr->ti_sci_id, i + start_sec);
+
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused ti_sci_intr_resume(struct device *dev)
+{
+	struct ti_sci_intr_irq_domain *intr = dev_get_drvdata(dev);
+
+	return ti_sci_intr_restore_irqs(intr);
+}
+
+static const struct dev_pm_ops ti_sci_intr_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, ti_sci_intr_resume)
+};
+
 static const struct of_device_id ti_sci_intr_irq_domain_of_match[] = {
 	{ .compatible = "ti,sci-intr", },
 	{ /* sentinel */ },
@@ -295,6 +402,7 @@ static struct platform_driver ti_sci_intr_irq_domain_driver = {
 	.probe = ti_sci_intr_irq_domain_probe,
 	.driver = {
 		.name = "ti-sci-intr",
+		.pm = &ti_sci_intr_dev_pm_ops,
 		.of_match_table = ti_sci_intr_irq_domain_of_match,
 	},
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Aswath Govindraju]

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




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[kernel test robot]

Hi Aswath,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tip/irq/core]
[also build test WARNING on v5.19-rc1 next-20220608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aswath-Govindraju/irqchip-ti-sci-intr-Add-support-for-system-suspend-resume-PM/20220607-142656
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git cdb4913293897dde0df522ed5789ba016f3b9157
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20220609/202206090635.fkrDWqjX-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c41c5fa0992f5a0a3298d6933ed84349f1f48b58
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aswath-Govindraju/irqchip-ti-sci-intr-Add-support-for-system-suspend-resume-PM/20220607-142656
        git checkout c41c5fa0992f5a0a3298d6933ed84349f1f48b58
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/irqchip/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/irqchip/irq-ti-sci-intr.c:270: warning: expecting prototype for ti_sci_intr_initialize_mappingg(). Prototype was for ti_sci_intr_initialize_mapping() instead


vim +270 drivers/irqchip/irq-ti-sci-intr.c

   264	
   265	/**
   266	 * ti_sci_intr_initialize_mappingg - Initialize the out_irq <-> hwirq mapping table
   267	 * @intr:	IRQ domain corresponding to Interrupt Router
   268	 */
   269	static int ti_sci_intr_initialize_mapping(struct ti_sci_intr_irq_domain *intr)
 > 270	{
   271		int i;
   272		int mapping_len = intr->out_irqs->desc->num + intr->out_irqs->desc->num_sec;
   273	
   274		intr->mapping = devm_kzalloc(intr->dev, mapping_len * sizeof(u32), GFP_KERNEL);
   275		if (!intr->mapping)
   276			return -ENOMEM;
   277	
   278		/* Set all the elements in the array to max value of u32 */
   279		for (i = 0; i < mapping_len; i++)
   280			intr->mapping[i] = 0xFFFFFFFF;
   281	
   282		return 0;
   283	}
   284	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[kernel test robot]

Hi Aswath,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tip/irq/core]
[also build test WARNING on v5.19-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aswath-Govindraju/irqchip-ti-sci-intr-Add-support-for-system-suspend-resume-PM/20220607-142656
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git cdb4913293897dde0df522ed5789ba016f3b9157
config: arm64-randconfig-r020-20220607 (https://download.01.org/0day-ci/archive/20220609/202206092226.YcJ2jTZb-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b92436efcb7813fc481b30f2593a4907568d917a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/c41c5fa0992f5a0a3298d6933ed84349f1f48b58
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aswath-Govindraju/irqchip-ti-sci-intr-Add-support-for-system-suspend-resume-PM/20220607-142656
        git checkout c41c5fa0992f5a0a3298d6933ed84349f1f48b58
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/irqchip/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/irqchip/irq-ti-sci-intr.c:270: warning: expecting prototype for ti_sci_intr_initialize_mappingg(). Prototype was for ti_sci_intr_initialize_mapping() instead


vim +270 drivers/irqchip/irq-ti-sci-intr.c

   264	
   265	/**
   266	 * ti_sci_intr_initialize_mappingg - Initialize the out_irq <-> hwirq mapping table
   267	 * @intr:	IRQ domain corresponding to Interrupt Router
   268	 */
   269	static int ti_sci_intr_initialize_mapping(struct ti_sci_intr_irq_domain *intr)
 > 270	{
   271		int i;
   272		int mapping_len = intr->out_irqs->desc->num + intr->out_irqs->desc->num_sec;
   273	
   274		intr->mapping = devm_kzalloc(intr->dev, mapping_len * sizeof(u32), GFP_KERNEL);
   275		if (!intr->mapping)
   276			return -ENOMEM;
   277	
   278		/* Set all the elements in the array to max value of u32 */
   279		for (i = 0; i < mapping_len; i++)
   280			intr->mapping[i] = 0xFFFFFFFF;
   281	
   282		return 0;
   283	}
   284	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thomas Richard]

On 6/8/22 11:12, Marc Zyngier wrote:
> 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.

Hi Marc,

I'm interested to continue the work to upstream this patch.
I read all the thread, but I think I'm also missing the point.

Do you mean the ti_sci_intr_irq_domain struct is not the right place to
save the mappings, and it shall not be done in this irqchip driver?
The context should be saved and restored from the ti_sci driver using
the ti_sci_resource struct?

Am I on the right track?

Best Regards,

Thomas

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On Tue, 31 Oct 2023 16:00:59 +0000,
Thomas Richard <thomas.richard@bootlin.com> wrote:
> 
> On 6/8/22 11:12, Marc Zyngier wrote:
> > On Wed, 08 Jun 2022 09:48:20 +0100,
> > Aswath Govindraju <a-govindraju@ti.com> wrote:
> >>
> >> 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.
> 
> Hi Marc,
> 
> I'm interested to continue the work to upstream this patch.
> I read all the thread, but I think I'm also missing the point.
> 
> Do you mean the ti_sci_intr_irq_domain struct is not the right place to
> save the mappings, and it shall not be done in this irqchip driver?
> The context should be saved and restored from the ti_sci driver using
> the ti_sci_resource struct?

That's exactly my point. There is already a large body of code that is
aware of the resource allocation (the ti_sci driver), which is common
to a whole lot of subsystem that will need some form of save/restore.

It seems natural to put the save/restore aspects inside that driver,
unless there is some other reasons not to do so (which haven't so far
been explained).

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel