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 <[email protected]>
---
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
On Tue, 07 Jun 2022 07:19:12 +0100,
Aswath Govindraju <[email protected]> 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 <[email protected]>
> ---
> 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.
Hi Marc,
On 07/06/22 12:48, Marc Zyngier wrote:
> On Tue, 07 Jun 2022 07:19:12 +0100,
> Aswath Govindraju <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>
On Wed, 08 Jun 2022 09:48:20 +0100,
Aswath Govindraju <[email protected]> wrote:
>
> Hi Marc,
>
> On 07/06/22 12:48, Marc Zyngier wrote:
> > On Tue, 07 Jun 2022 07:19:12 +0100,
> > Aswath Govindraju <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.
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/[email protected]/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 <[email protected]>
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
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/[email protected]/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 <[email protected]>
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
On 6/8/22 11:12, Marc Zyngier wrote:
> On Wed, 08 Jun 2022 09:48:20 +0100,
> Aswath Govindraju <[email protected]> wrote:
>>
>> Hi Marc,
>>
>> On 07/06/22 12:48, Marc Zyngier wrote:
>>> On Tue, 07 Jun 2022 07:19:12 +0100,
>>> Aswath Govindraju <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
>
On Tue, 31 Oct 2023 16:00:59 +0000,
Thomas Richard <[email protected]> wrote:
>
> On 6/8/22 11:12, Marc Zyngier wrote:
> > On Wed, 08 Jun 2022 09:48:20 +0100,
> > Aswath Govindraju <[email protected]> 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.