[PATCH v2 0/2] riscv-imsic/aplic: Save and restore the registers

[PATCH v2 0/2] riscv-imsic/aplic: Save and restore the registers

[Nick Hu]

Since the APLIC and IMSIC may be powered down when the CPUs enter a deep
sleep state, it is necessary to implement save and restore functions to
ensure their states are correctly preserved and restored.

Changes in V2:
- Address the compile error from kernel test robot when
  CONFIG_RISCV_M_MODE is enabled.
- Apply the suggestion from Anup [1]

Link:
- [1] https://lore.kernel.org/linux-riscv/20250709025516.28051-1-nick.hu@sifive.com/

Nick Hu (2):
  irqchip/riscv-imsic: Restore the IMSIC registers
  irqchip/riscv-aplic: Save and restore APLIC registers

 drivers/irqchip/irq-riscv-aplic-direct.c |  11 ++
 drivers/irqchip/irq-riscv-aplic-main.c   | 158 ++++++++++++++++++++++-
 drivers/irqchip/irq-riscv-aplic-main.h   |  11 ++
 drivers/irqchip/irq-riscv-imsic-early.c  |  41 ++++--
 4 files changed, 212 insertions(+), 9 deletions(-)

-- 
2.17.1


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

[PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers

[Nick Hu]

When the system woken up from the low power state, the IMSIC might be in
the reset state. Therefore adding the CPU PM callbacks to restore the
IMSIC register when the cpu resume from the low power state.

Signed-off-by: Nick Hu <nick.hu@sifive.com>
Reviewed-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
Reviewed-by: Cyan Yang <cyan.yang@sifive.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 drivers/irqchip/irq-riscv-imsic-early.c | 40 ++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
index d9ae87808651..62bcbcae8bd4 100644
--- a/drivers/irqchip/irq-riscv-imsic-early.c
+++ b/drivers/irqchip/irq-riscv-imsic-early.c
@@ -7,6 +7,7 @@
 #define pr_fmt(fmt) "riscv-imsic: " fmt
 #include <linux/acpi.h>
 #include <linux/cpu.h>
+#include <linux/cpu_pm.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -109,14 +110,8 @@ static void imsic_handle_irq(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static int imsic_starting_cpu(unsigned int cpu)
+static void imsic_restore(void)
 {
-	/* Mark per-CPU IMSIC state as online */
-	imsic_state_online();
-
-	/* Enable per-CPU parent interrupt */
-	enable_percpu_irq(imsic_parent_irq, irq_get_trigger_type(imsic_parent_irq));
-
 	/* Setup IPIs */
 	imsic_ipi_starting_cpu();
 
@@ -128,6 +123,19 @@ static int imsic_starting_cpu(unsigned int cpu)
 
 	/* Enable local interrupt delivery */
 	imsic_local_delivery(true);
+}
+
+static int imsic_starting_cpu(unsigned int cpu)
+{
+	/* Mark per-CPU IMSIC state as online */
+	imsic_state_online();
+
+	/* Enable per-CPU parent interrupt */
+	enable_percpu_irq(imsic_parent_irq,
+			  irq_get_trigger_type(imsic_parent_irq));
+
+	/* Restore the imsic reg */
+	imsic_restore();
 
 	return 0;
 }
@@ -143,6 +151,22 @@ static int imsic_dying_cpu(unsigned int cpu)
 	return 0;
 }
 
+static int imsic_pm_notifier(struct notifier_block *self, unsigned long cmd, void *v)
+{
+	switch (cmd) {
+	case CPU_PM_EXIT:
+		/* Restore the imsic reg */
+		imsic_restore();
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block imsic_pm_notifier_block = {
+	.notifier_call = imsic_pm_notifier,
+};
+
 static int __init imsic_early_probe(struct fwnode_handle *fwnode)
 {
 	struct irq_domain *domain;
@@ -180,7 +204,7 @@ static int __init imsic_early_probe(struct fwnode_handle *fwnode)
 	cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_IMSIC_STARTING, "irqchip/riscv/imsic:starting",
 			  imsic_starting_cpu, imsic_dying_cpu);
 
-	return 0;
+	return cpu_pm_register_notifier(&imsic_pm_notifier_block);
 }
 
 static int __init imsic_early_dt_init(struct device_node *node, struct device_node *parent)
-- 
2.17.1


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

Re: [PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers

[Nutty Liu]

On 8/6/2025 4:27 PM, Nick Hu wrote:
> When the system woken up from the low power state, the IMSIC might be in
> the reset state. Therefore adding the CPU PM callbacks to restore the
> IMSIC register when the cpu resume from the low power state.
>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> Reviewed-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> Reviewed-by: Cyan Yang <cyan.yang@sifive.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>
> ---
>   drivers/irqchip/irq-riscv-imsic-early.c | 40 ++++++++++++++++++++-----
>   1 file changed, 32 insertions(+), 8 deletions(-)
Reviewed-by: Nutty Liu <liujingqi@lanxincomputing.com>

Thanks,
Nutty
> diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> index d9ae87808651..62bcbcae8bd4 100644
> --- a/drivers/irqchip/irq-riscv-imsic-early.c
> +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> @@ -7,6 +7,7 @@
>   #define pr_fmt(fmt) "riscv-imsic: " fmt
>   #include <linux/acpi.h>
>   #include <linux/cpu.h>
> +#include <linux/cpu_pm.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
>   #include <linux/irq.h>
> @@ -109,14 +110,8 @@ static void imsic_handle_irq(struct irq_desc *desc)
>   	chained_irq_exit(chip, desc);
>   }
>   
> -static int imsic_starting_cpu(unsigned int cpu)
> +static void imsic_restore(void)
>   {
> -	/* Mark per-CPU IMSIC state as online */
> -	imsic_state_online();
> -
> -	/* Enable per-CPU parent interrupt */
> -	enable_percpu_irq(imsic_parent_irq, irq_get_trigger_type(imsic_parent_irq));
> -
>   	/* Setup IPIs */
>   	imsic_ipi_starting_cpu();
>   
> @@ -128,6 +123,19 @@ static int imsic_starting_cpu(unsigned int cpu)
>   
>   	/* Enable local interrupt delivery */
>   	imsic_local_delivery(true);
> +}
> +
> +static int imsic_starting_cpu(unsigned int cpu)
> +{
> +	/* Mark per-CPU IMSIC state as online */
> +	imsic_state_online();
> +
> +	/* Enable per-CPU parent interrupt */
> +	enable_percpu_irq(imsic_parent_irq,
> +			  irq_get_trigger_type(imsic_parent_irq));
> +
> +	/* Restore the imsic reg */
> +	imsic_restore();
>   
>   	return 0;
>   }
> @@ -143,6 +151,22 @@ static int imsic_dying_cpu(unsigned int cpu)
>   	return 0;
>   }
>   
> +static int imsic_pm_notifier(struct notifier_block *self, unsigned long cmd, void *v)
> +{
> +	switch (cmd) {
> +	case CPU_PM_EXIT:
> +		/* Restore the imsic reg */
> +		imsic_restore();
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block imsic_pm_notifier_block = {
> +	.notifier_call = imsic_pm_notifier,
> +};
> +
>   static int __init imsic_early_probe(struct fwnode_handle *fwnode)
>   {
>   	struct irq_domain *domain;
> @@ -180,7 +204,7 @@ static int __init imsic_early_probe(struct fwnode_handle *fwnode)
>   	cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_IMSIC_STARTING, "irqchip/riscv/imsic:starting",
>   			  imsic_starting_cpu, imsic_dying_cpu);
>   
> -	return 0;
> +	return cpu_pm_register_notifier(&imsic_pm_notifier_block);
>   }
>   
>   static int __init imsic_early_dt_init(struct device_node *node, struct device_node *parent)

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

Re: [PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers

[Thomas Gleixner]

On Wed, Aug 06 2025 at 16:27, Nick Hu wrote:

"Restore the IMSIC registers" does not tell me anything useful. When are
they restored?

> When the system woken up from the low power state, the IMSIC might be in

is woken up from a low power state

> the reset state.

The real important information is:

   When the system enters a low power state the IMSIC might be reset,
   but on exit nothing restores the registers, which prevents interrupt
   delivery.

Or something like that.

> Therefore adding the CPU PM callbacks to restore the IMSIC register
> when the cpu resume from the low power state.

This is not a valid sentence.

  Solve this by registering a CPU power management notifier, which
  restores the IMSIC on exit.

Or such.

See

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

for further explanation.

> diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> index d9ae87808651..62bcbcae8bd4 100644
> --- a/drivers/irqchip/irq-riscv-imsic-early.c
> +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> @@ -7,6 +7,7 @@
>  #define pr_fmt(fmt) "riscv-imsic: " fmt
>  #include <linux/acpi.h>
>  #include <linux/cpu.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/interrupt.h>

This does neither apply against Linus tree nor against tip 

> -static int imsic_starting_cpu(unsigned int cpu)
> +static void imsic_restore(void)

This function is used for both setup _and_ restore, so naming it
restore() is misleading at best.

>  {
> -	/* Mark per-CPU IMSIC state as online */
> -	imsic_state_online();
> -
> -	/* Enable per-CPU parent interrupt */
> -	enable_percpu_irq(imsic_parent_irq, irq_get_trigger_type(imsic_parent_irq));
> -
>  	/* Setup IPIs */
>  	imsic_ipi_starting_cpu();
>  
> @@ -128,6 +123,19 @@ static int imsic_starting_cpu(unsigned int cpu)
>  
>  	/* Enable local interrupt delivery */
>  	imsic_local_delivery(true);
> +}
> +
> +static int imsic_starting_cpu(unsigned int cpu)
> +{
> +	/* Mark per-CPU IMSIC state as online */
> +	imsic_state_online();
> +
> +	/* Enable per-CPU parent interrupt */
> +	enable_percpu_irq(imsic_parent_irq,
> +			  irq_get_trigger_type(imsic_parent_irq));

No line break required. You have 100 characters.

> +
> +	/* Restore the imsic reg */

One IMSIC register? Can you please write proper sentences and write
words out? This is not twitter. Also use IMSIC uppercase as the rest of
the code does in the comments.

Thanks,

        tglx

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

Re: [PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers

[Nick Hu]

On Wed, Aug 6, 2025 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Aug 06 2025 at 16:27, Nick Hu wrote:
>
> "Restore the IMSIC registers" does not tell me anything useful. When are
> they restored?
>
I’ll update it to: “Preserve the IMSIC states after PM”

> > When the system woken up from the low power state, the IMSIC might be in
>
> is woken up from a low power state
>
> > the reset state.
>
> The real important information is:
>
>    When the system enters a low power state the IMSIC might be reset,
>    but on exit nothing restores the registers, which prevents interrupt
>    delivery.
>
> Or something like that.
>
> > Therefore adding the CPU PM callbacks to restore the IMSIC register
> > when the cpu resume from the low power state.
>
> This is not a valid sentence.
>
>   Solve this by registering a CPU power management notifier, which
>   restores the IMSIC on exit.
>
> Or such.
>
> See
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> for further explanation.
>
I'll correct it in the next version. Thanks.

> > diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
> > index d9ae87808651..62bcbcae8bd4 100644
> > --- a/drivers/irqchip/irq-riscv-imsic-early.c
> > +++ b/drivers/irqchip/irq-riscv-imsic-early.c
> > @@ -7,6 +7,7 @@
> >  #define pr_fmt(fmt) "riscv-imsic: " fmt
> >  #include <linux/acpi.h>
> >  #include <linux/cpu.h>
> > +#include <linux/cpu_pm.h>
> >  #include <linux/interrupt.h>
>
> This does neither apply against Linus tree nor against tip
>
> > -static int imsic_starting_cpu(unsigned int cpu)
> > +static void imsic_restore(void)
>
> This function is used for both setup _and_ restore, so naming it
> restore() is misleading at best.
>
Will rename it to imsic_regs_init()

> >  {
> > -     /* Mark per-CPU IMSIC state as online */
> > -     imsic_state_online();
> > -
> > -     /* Enable per-CPU parent interrupt */
> > -     enable_percpu_irq(imsic_parent_irq, irq_get_trigger_type(imsic_parent_irq));
> > -
> >       /* Setup IPIs */
> >       imsic_ipi_starting_cpu();
> >
> > @@ -128,6 +123,19 @@ static int imsic_starting_cpu(unsigned int cpu)
> >
> >       /* Enable local interrupt delivery */
> >       imsic_local_delivery(true);
> > +}
> > +
> > +static int imsic_starting_cpu(unsigned int cpu)
> > +{
> > +     /* Mark per-CPU IMSIC state as online */
> > +     imsic_state_online();
> > +
> > +     /* Enable per-CPU parent interrupt */
> > +     enable_percpu_irq(imsic_parent_irq,
> > +                       irq_get_trigger_type(imsic_parent_irq));
>
> No line break required. You have 100 characters.
>
> > +
> > +     /* Restore the imsic reg */
>
> One IMSIC register? Can you please write proper sentences and write
> words out? This is not twitter. Also use IMSIC uppercase as the rest of
> the code does in the comments.
>
Sorry, I’ll be more careful.

> Thanks,
>
>         tglx

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

[PATCH v2 2/2] irqchip/riscv-aplic: Save and restore APLIC registers

[Nick Hu]

The APLIC may be powered down when the CPUs enter a deep sleep state.
Therefore adding the APLIC save and restore functions to save and
restore the states of APLIC.

Signed-off-by: Nick Hu <nick.hu@sifive.com>
Reviewed-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
Reviewed-by: Cyan Yang <cyan.yang@sifive.com>
---
 drivers/irqchip/irq-riscv-aplic-direct.c |  11 ++
 drivers/irqchip/irq-riscv-aplic-main.c   | 158 ++++++++++++++++++++++-
 drivers/irqchip/irq-riscv-aplic-main.h   |  11 ++
 3 files changed, 179 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-direct.c b/drivers/irqchip/irq-riscv-aplic-direct.c
index 205ad61d15e4..61b9ac2e1b7b 100644
--- a/drivers/irqchip/irq-riscv-aplic-direct.c
+++ b/drivers/irqchip/irq-riscv-aplic-direct.c
@@ -8,6 +8,7 @@
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/cpu.h>
+#include <linux/cpumask.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
@@ -171,6 +172,16 @@ static void aplic_idc_set_delivery(struct aplic_idc *idc, bool en)
 	writel(de, idc->regs + APLIC_IDC_IDELIVERY);
 }
 
+void aplic_direct_restore(struct aplic_priv *priv)
+{
+	struct aplic_direct *direct =
+			container_of(priv, struct aplic_direct, priv);
+	int cpu;
+
+	for_each_cpu(cpu, &direct->lmask)
+		aplic_idc_set_delivery(per_cpu_ptr(&aplic_idcs, cpu), true);
+}
+
 static int aplic_direct_dying_cpu(unsigned int cpu)
 {
 	if (aplic_direct_parent_irq)
diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
index 93e7c51f944a..91fe3305934d 100644
--- a/drivers/irqchip/irq-riscv-aplic-main.c
+++ b/drivers/irqchip/irq-riscv-aplic-main.c
@@ -12,10 +12,143 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/printk.h>
+#include <linux/syscore_ops.h>
 
 #include "irq-riscv-aplic-main.h"
 
+static LIST_HEAD(aplics);
+
+static void aplic_restore(struct aplic_priv *priv)
+{
+	int i;
+	u32 clrip;
+
+	writel(priv->saved_domaincfg, priv->regs + APLIC_DOMAINCFG);
+#ifdef CONFIG_RISCV_M_MODE
+	writel(priv->saved_msiaddr, priv->regs + APLIC_xMSICFGADDR);
+	writel(priv->saved_msiaddrh, priv->regs + APLIC_xMSICFGADDRH);
+#endif
+	for (i = 1; i <= priv->nr_irqs; i++) {
+		writel(priv->saved_sourcecfg[i - 1],
+		       priv->regs + APLIC_SOURCECFG_BASE +
+		       (i - 1) * sizeof(u32));
+		writel(priv->saved_target[i - 1],
+		       priv->regs + APLIC_TARGET_BASE +
+		       (i - 1) * sizeof(u32));
+	}
+
+	for (i = 0; i <= priv->nr_irqs; i += 32) {
+		writel(-1U, priv->regs + APLIC_CLRIE_BASE +
+			    (i / 32) * sizeof(u32));
+		writel(priv->saved_ie[i / 32],
+		       priv->regs + APLIC_SETIE_BASE +
+		       (i / 32) * sizeof(u32));
+	}
+
+	if (priv->nr_idcs) {
+		aplic_direct_restore(priv);
+	} else {
+		/* Re-trigger the interrupts */
+		for (i = 0; i <= priv->nr_irqs; i += 32) {
+			clrip = readl(priv->regs + APLIC_CLRIP_BASE +
+				      (i / 32) * sizeof(u32));
+			writel(clrip, priv->regs + APLIC_SETIP_BASE +
+				      (i / 32) * sizeof(u32));
+		}
+	}
+}
+
+static void aplic_save(struct aplic_priv *priv)
+{
+	int i;
+
+	for (i = 1; i <= priv->nr_irqs; i++) {
+		priv->saved_target[i - 1] = readl(priv->regs +
+						  APLIC_TARGET_BASE +
+						  (i - 1) * sizeof(u32));
+	}
+
+	for (i = 0; i <= priv->nr_irqs; i += 32) {
+		priv->saved_ie[i / 32] = readl(priv->regs +
+					       APLIC_SETIE_BASE +
+					       (i / 32) * sizeof(u32));
+	}
+}
+
+static int aplic_syscore_suspend(void)
+{
+	struct aplic_priv *priv;
+
+	list_for_each_entry(priv, &aplics, head) {
+		aplic_save(priv);
+	}
+
+	return 0;
+}
+
+static void aplic_syscore_resume(void)
+{
+	struct aplic_priv *priv;
+
+	list_for_each_entry(priv, &aplics, head) {
+		aplic_restore(priv);
+	}
+}
+
+static struct syscore_ops aplic_syscore_ops = {
+	.suspend = aplic_syscore_suspend,
+	.resume = aplic_syscore_resume,
+};
+
+static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct aplic_priv *priv = container_of(nb, struct aplic_priv, genpd_nb);
+
+	switch (action) {
+	case GENPD_NOTIFY_PRE_OFF:
+		aplic_save(priv);
+		break;
+	case GENPD_NOTIFY_ON:
+		aplic_restore(priv);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static void aplic_remove(void *data)
+{
+	struct aplic_priv *priv = data;
+
+	list_del(&priv->head);
+	dev_pm_genpd_remove_notifier(priv->dev);
+}
+
+static int aplic_add(struct device *dev, struct aplic_priv *priv)
+{
+	int ret;
+
+	list_add(&priv->head, &aplics);
+	/* Add genpd notifier */
+	priv->genpd_nb.notifier_call = aplic_pm_notifier;
+	ret = dev_pm_genpd_add_notifier(dev, &priv->genpd_nb);
+	if (ret && ret != -ENODEV && ret != -EOPNOTSUPP) {
+		list_del(&priv->head);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, aplic_remove, priv);
+	if (ret)
+		return ret;
+
+	return devm_pm_runtime_enable(dev);
+}
+
 void aplic_irq_unmask(struct irq_data *d)
 {
 	struct aplic_priv *priv = irq_data_get_irq_chip_data(d);
@@ -59,6 +192,7 @@ int aplic_irq_set_type(struct irq_data *d, unsigned int type)
 	sourcecfg = priv->regs + APLIC_SOURCECFG_BASE;
 	sourcecfg += (d->hwirq - 1) * sizeof(u32);
 	writel(val, sourcecfg);
+	priv->saved_sourcecfg[d->hwirq - 1] = val;
 
 	return 0;
 }
@@ -95,6 +229,8 @@ void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode)
 		valh |= FIELD_PREP(APLIC_xMSICFGADDRH_HHXS, priv->msicfg.hhxs);
 		writel(val, priv->regs + APLIC_xMSICFGADDR);
 		writel(valh, priv->regs + APLIC_xMSICFGADDRH);
+		priv->saved_msiaddr = val;
+		priv->saved_msiaddrh = valh;
 	}
 #endif
 
@@ -106,6 +242,7 @@ void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode)
 	writel(val, priv->regs + APLIC_DOMAINCFG);
 	if (readl(priv->regs + APLIC_DOMAINCFG) != val)
 		dev_warn(priv->dev, "unable to write 0x%x in domaincfg\n", val);
+	priv->saved_domaincfg = val;
 }
 
 static void aplic_init_hw_irqs(struct aplic_priv *priv)
@@ -176,7 +313,24 @@ int aplic_setup_priv(struct aplic_priv *priv, struct device *dev, void __iomem *
 	/* Setup initial state APLIC interrupts */
 	aplic_init_hw_irqs(priv);
 
-	return 0;
+	/* For power management */
+	priv->saved_target = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
+					  GFP_KERNEL);
+	if (!priv->saved_target)
+		return -ENOMEM;
+
+	priv->saved_sourcecfg = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
+					     GFP_KERNEL);
+	if (!priv->saved_sourcecfg)
+		return -ENOMEM;
+
+	priv->saved_ie = devm_kzalloc(dev,
+				      DIV_ROUND_UP(priv->nr_irqs, 32) * sizeof(u32),
+				      GFP_KERNEL);
+	if (!priv->saved_ie)
+		return -ENOMEM;
+
+	return aplic_add(dev, priv);
 }
 
 static int aplic_probe(struct platform_device *pdev)
@@ -209,6 +363,8 @@ static int aplic_probe(struct platform_device *pdev)
 	if (rc)
 		dev_err_probe(dev, rc, "failed to setup APLIC in %s mode\n",
 			      msi_mode ? "MSI" : "direct");
+	else
+		register_syscore_ops(&aplic_syscore_ops);
 
 #ifdef CONFIG_ACPI
 	if (!acpi_disabled)
diff --git a/drivers/irqchip/irq-riscv-aplic-main.h b/drivers/irqchip/irq-riscv-aplic-main.h
index b0ad8cde69b1..f27d5ff1c741 100644
--- a/drivers/irqchip/irq-riscv-aplic-main.h
+++ b/drivers/irqchip/irq-riscv-aplic-main.h
@@ -24,6 +24,7 @@ struct aplic_msicfg {
 };
 
 struct aplic_priv {
+	struct list_head	head;
 	struct device		*dev;
 	u32			gsi_base;
 	u32			nr_irqs;
@@ -31,6 +32,15 @@ struct aplic_priv {
 	u32			acpi_aplic_id;
 	void __iomem		*regs;
 	struct aplic_msicfg	msicfg;
+	struct notifier_block	genpd_nb;
+	u32 *saved_target;
+	u32 *saved_sourcecfg;
+	u32 *saved_ie;
+	u32 saved_domaincfg;
+#ifdef CONFIG_RISCV_M_MODE
+	u32 saved_msiaddr;
+	u32 saved_msiaddrh;
+#endif
 };
 
 void aplic_irq_unmask(struct irq_data *d);
@@ -39,6 +49,7 @@ int aplic_irq_set_type(struct irq_data *d, unsigned int type);
 int aplic_irqdomain_translate(struct irq_fwspec *fwspec, u32 gsi_base,
 			      unsigned long *hwirq, unsigned int *type);
 void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode);
+void aplic_direct_restore(struct aplic_priv *priv);
 int aplic_setup_priv(struct aplic_priv *priv, struct device *dev, void __iomem *regs);
 int aplic_direct_setup(struct device *dev, void __iomem *regs);
 #ifdef CONFIG_RISCV_APLIC_MSI
-- 
2.17.1


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

Re: [PATCH v2 2/2] irqchip/riscv-aplic: Save and restore APLIC registers

[Nutty Liu]

On 8/6/2025 4:27 PM, Nick Hu wrote:
> The APLIC may be powered down when the CPUs enter a deep sleep state.
> Therefore adding the APLIC save and restore functions to save and
> restore the states of APLIC.
>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> Reviewed-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> Reviewed-by: Cyan Yang <cyan.yang@sifive.com>
> ---
>   drivers/irqchip/irq-riscv-aplic-direct.c |  11 ++
>   drivers/irqchip/irq-riscv-aplic-main.c   | 158 ++++++++++++++++++++++-
>   drivers/irqchip/irq-riscv-aplic-main.h   |  11 ++
>   3 files changed, 179 insertions(+), 1 deletion(-)
Reviewed-by: Nutty Liu <liujingqi@lanxincomputing.com>

Thanks,
Nutty
> diff --git a/drivers/irqchip/irq-riscv-aplic-direct.c b/drivers/irqchip/irq-riscv-aplic-direct.c
> index 205ad61d15e4..61b9ac2e1b7b 100644
> --- a/drivers/irqchip/irq-riscv-aplic-direct.c
> +++ b/drivers/irqchip/irq-riscv-aplic-direct.c
> @@ -8,6 +8,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/bitops.h>
>   #include <linux/cpu.h>
> +#include <linux/cpumask.h>
>   #include <linux/interrupt.h>
>   #include <linux/irqchip.h>
>   #include <linux/irqchip/chained_irq.h>
> @@ -171,6 +172,16 @@ static void aplic_idc_set_delivery(struct aplic_idc *idc, bool en)
>   	writel(de, idc->regs + APLIC_IDC_IDELIVERY);
>   }
>   
> +void aplic_direct_restore(struct aplic_priv *priv)
> +{
> +	struct aplic_direct *direct =
> +			container_of(priv, struct aplic_direct, priv);
> +	int cpu;
> +
> +	for_each_cpu(cpu, &direct->lmask)
> +		aplic_idc_set_delivery(per_cpu_ptr(&aplic_idcs, cpu), true);
> +}
> +
>   static int aplic_direct_dying_cpu(unsigned int cpu)
>   {
>   	if (aplic_direct_parent_irq)
> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
> index 93e7c51f944a..91fe3305934d 100644
> --- a/drivers/irqchip/irq-riscv-aplic-main.c
> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
> @@ -12,10 +12,143 @@
>   #include <linux/of.h>
>   #include <linux/of_irq.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/printk.h>
> +#include <linux/syscore_ops.h>
>   
>   #include "irq-riscv-aplic-main.h"
>   
> +static LIST_HEAD(aplics);
> +
> +static void aplic_restore(struct aplic_priv *priv)
> +{
> +	int i;
> +	u32 clrip;
> +
> +	writel(priv->saved_domaincfg, priv->regs + APLIC_DOMAINCFG);
> +#ifdef CONFIG_RISCV_M_MODE
> +	writel(priv->saved_msiaddr, priv->regs + APLIC_xMSICFGADDR);
> +	writel(priv->saved_msiaddrh, priv->regs + APLIC_xMSICFGADDRH);
> +#endif
> +	for (i = 1; i <= priv->nr_irqs; i++) {
> +		writel(priv->saved_sourcecfg[i - 1],
> +		       priv->regs + APLIC_SOURCECFG_BASE +
> +		       (i - 1) * sizeof(u32));
> +		writel(priv->saved_target[i - 1],
> +		       priv->regs + APLIC_TARGET_BASE +
> +		       (i - 1) * sizeof(u32));
> +	}
> +
> +	for (i = 0; i <= priv->nr_irqs; i += 32) {
> +		writel(-1U, priv->regs + APLIC_CLRIE_BASE +
> +			    (i / 32) * sizeof(u32));
> +		writel(priv->saved_ie[i / 32],
> +		       priv->regs + APLIC_SETIE_BASE +
> +		       (i / 32) * sizeof(u32));
> +	}
> +
> +	if (priv->nr_idcs) {
> +		aplic_direct_restore(priv);
> +	} else {
> +		/* Re-trigger the interrupts */
> +		for (i = 0; i <= priv->nr_irqs; i += 32) {
> +			clrip = readl(priv->regs + APLIC_CLRIP_BASE +
> +				      (i / 32) * sizeof(u32));
> +			writel(clrip, priv->regs + APLIC_SETIP_BASE +
> +				      (i / 32) * sizeof(u32));
> +		}
> +	}
> +}
> +
> +static void aplic_save(struct aplic_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 1; i <= priv->nr_irqs; i++) {
> +		priv->saved_target[i - 1] = readl(priv->regs +
> +						  APLIC_TARGET_BASE +
> +						  (i - 1) * sizeof(u32));
> +	}
> +
> +	for (i = 0; i <= priv->nr_irqs; i += 32) {
> +		priv->saved_ie[i / 32] = readl(priv->regs +
> +					       APLIC_SETIE_BASE +
> +					       (i / 32) * sizeof(u32));
> +	}
> +}
> +
> +static int aplic_syscore_suspend(void)
> +{
> +	struct aplic_priv *priv;
> +
> +	list_for_each_entry(priv, &aplics, head) {
> +		aplic_save(priv);
> +	}
> +
> +	return 0;
> +}
> +
> +static void aplic_syscore_resume(void)
> +{
> +	struct aplic_priv *priv;
> +
> +	list_for_each_entry(priv, &aplics, head) {
> +		aplic_restore(priv);
> +	}
> +}
> +
> +static struct syscore_ops aplic_syscore_ops = {
> +	.suspend = aplic_syscore_suspend,
> +	.resume = aplic_syscore_resume,
> +};
> +
> +static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct aplic_priv *priv = container_of(nb, struct aplic_priv, genpd_nb);
> +
> +	switch (action) {
> +	case GENPD_NOTIFY_PRE_OFF:
> +		aplic_save(priv);
> +		break;
> +	case GENPD_NOTIFY_ON:
> +		aplic_restore(priv);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static void aplic_remove(void *data)
> +{
> +	struct aplic_priv *priv = data;
> +
> +	list_del(&priv->head);
> +	dev_pm_genpd_remove_notifier(priv->dev);
> +}
> +
> +static int aplic_add(struct device *dev, struct aplic_priv *priv)
> +{
> +	int ret;
> +
> +	list_add(&priv->head, &aplics);
> +	/* Add genpd notifier */
> +	priv->genpd_nb.notifier_call = aplic_pm_notifier;
> +	ret = dev_pm_genpd_add_notifier(dev, &priv->genpd_nb);
> +	if (ret && ret != -ENODEV && ret != -EOPNOTSUPP) {
> +		list_del(&priv->head);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, aplic_remove, priv);
> +	if (ret)
> +		return ret;
> +
> +	return devm_pm_runtime_enable(dev);
> +}
> +
>   void aplic_irq_unmask(struct irq_data *d)
>   {
>   	struct aplic_priv *priv = irq_data_get_irq_chip_data(d);
> @@ -59,6 +192,7 @@ int aplic_irq_set_type(struct irq_data *d, unsigned int type)
>   	sourcecfg = priv->regs + APLIC_SOURCECFG_BASE;
>   	sourcecfg += (d->hwirq - 1) * sizeof(u32);
>   	writel(val, sourcecfg);
> +	priv->saved_sourcecfg[d->hwirq - 1] = val;
>   
>   	return 0;
>   }
> @@ -95,6 +229,8 @@ void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode)
>   		valh |= FIELD_PREP(APLIC_xMSICFGADDRH_HHXS, priv->msicfg.hhxs);
>   		writel(val, priv->regs + APLIC_xMSICFGADDR);
>   		writel(valh, priv->regs + APLIC_xMSICFGADDRH);
> +		priv->saved_msiaddr = val;
> +		priv->saved_msiaddrh = valh;
>   	}
>   #endif
>   
> @@ -106,6 +242,7 @@ void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode)
>   	writel(val, priv->regs + APLIC_DOMAINCFG);
>   	if (readl(priv->regs + APLIC_DOMAINCFG) != val)
>   		dev_warn(priv->dev, "unable to write 0x%x in domaincfg\n", val);
> +	priv->saved_domaincfg = val;
>   }
>   
>   static void aplic_init_hw_irqs(struct aplic_priv *priv)
> @@ -176,7 +313,24 @@ int aplic_setup_priv(struct aplic_priv *priv, struct device *dev, void __iomem *
>   	/* Setup initial state APLIC interrupts */
>   	aplic_init_hw_irqs(priv);
>   
> -	return 0;
> +	/* For power management */
> +	priv->saved_target = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> +					  GFP_KERNEL);
> +	if (!priv->saved_target)
> +		return -ENOMEM;
> +
> +	priv->saved_sourcecfg = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> +					     GFP_KERNEL);
> +	if (!priv->saved_sourcecfg)
> +		return -ENOMEM;
> +
> +	priv->saved_ie = devm_kzalloc(dev,
> +				      DIV_ROUND_UP(priv->nr_irqs, 32) * sizeof(u32),
> +				      GFP_KERNEL);
> +	if (!priv->saved_ie)
> +		return -ENOMEM;
> +
> +	return aplic_add(dev, priv);
>   }
>   
>   static int aplic_probe(struct platform_device *pdev)
> @@ -209,6 +363,8 @@ static int aplic_probe(struct platform_device *pdev)
>   	if (rc)
>   		dev_err_probe(dev, rc, "failed to setup APLIC in %s mode\n",
>   			      msi_mode ? "MSI" : "direct");
> +	else
> +		register_syscore_ops(&aplic_syscore_ops);
>   
>   #ifdef CONFIG_ACPI
>   	if (!acpi_disabled)
> diff --git a/drivers/irqchip/irq-riscv-aplic-main.h b/drivers/irqchip/irq-riscv-aplic-main.h
> index b0ad8cde69b1..f27d5ff1c741 100644
> --- a/drivers/irqchip/irq-riscv-aplic-main.h
> +++ b/drivers/irqchip/irq-riscv-aplic-main.h
> @@ -24,6 +24,7 @@ struct aplic_msicfg {
>   };
>   
>   struct aplic_priv {
> +	struct list_head	head;
>   	struct device		*dev;
>   	u32			gsi_base;
>   	u32			nr_irqs;
> @@ -31,6 +32,15 @@ struct aplic_priv {
>   	u32			acpi_aplic_id;
>   	void __iomem		*regs;
>   	struct aplic_msicfg	msicfg;
> +	struct notifier_block	genpd_nb;
> +	u32 *saved_target;
> +	u32 *saved_sourcecfg;
> +	u32 *saved_ie;
> +	u32 saved_domaincfg;
> +#ifdef CONFIG_RISCV_M_MODE
> +	u32 saved_msiaddr;
> +	u32 saved_msiaddrh;
> +#endif
>   };
>   
>   void aplic_irq_unmask(struct irq_data *d);
> @@ -39,6 +49,7 @@ int aplic_irq_set_type(struct irq_data *d, unsigned int type);
>   int aplic_irqdomain_translate(struct irq_fwspec *fwspec, u32 gsi_base,
>   			      unsigned long *hwirq, unsigned int *type);
>   void aplic_init_hw_global(struct aplic_priv *priv, bool msi_mode);
> +void aplic_direct_restore(struct aplic_priv *priv);
>   int aplic_setup_priv(struct aplic_priv *priv, struct device *dev, void __iomem *regs);
>   int aplic_direct_setup(struct device *dev, void __iomem *regs);
>   #ifdef CONFIG_RISCV_APLIC_MSI

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

Re: [PATCH v2 2/2] irqchip/riscv-aplic: Save and restore APLIC registers

[Thomas Gleixner]

On Wed, Aug 06 2025 at 16:27, Nick Hu wrote:
> The APLIC may be powered down when the CPUs enter a deep sleep state.
> Therefore adding the APLIC save and restore functions to save and
> restore the states of APLIC.

Same comments vs. subject and change log.

>  
> +void aplic_direct_restore(struct aplic_priv *priv)
> +{
> +	struct aplic_direct *direct =
> +			container_of(priv, struct aplic_direct, priv);

No line break required.

> +	int cpu;
> +
> +	for_each_cpu(cpu, &direct->lmask)
> +		aplic_idc_set_delivery(per_cpu_ptr(&aplic_idcs, cpu), true);
> +static LIST_HEAD(aplics);
> +
> +static void aplic_restore(struct aplic_priv *priv)
> +{
> +	int i;
> +	u32 clrip;

See the documentation I linked you to and look for the chapter about
variable declarations.

> +	writel(priv->saved_domaincfg, priv->regs + APLIC_DOMAINCFG);
> +#ifdef CONFIG_RISCV_M_MODE
> +	writel(priv->saved_msiaddr, priv->regs + APLIC_xMSICFGADDR);
> +	writel(priv->saved_msiaddrh, priv->regs + APLIC_xMSICFGADDRH);
> +#endif
> +	for (i = 1; i <= priv->nr_irqs; i++) {
> +		writel(priv->saved_sourcecfg[i - 1],
> +		       priv->regs + APLIC_SOURCECFG_BASE +
> +		       (i - 1) * sizeof(u32));
> +		writel(priv->saved_target[i - 1],
> +		       priv->regs + APLIC_TARGET_BASE +
> +		       (i - 1) * sizeof(u32));

Sorry, but this is incomprehensible garbage.

Why are you starting with i = 1 when you subtract 1 from i at every
usage site?

        u32 __iomem *regs = priv->regs;
        for (i = 0; i < priv->nr_irqs; i++, regs++) {
        	writel(priv->saved_sourcecfg[i], regs + APLIC_SOURCECFG_BASE);        
        	writel(priv->saved_target[i], regs + APLIC_TARGET_BASE);        
	}

See?

> +	}
> +
> +	for (i = 0; i <= priv->nr_irqs; i += 32) {

Off by one. This needs to be i < priv->nr_irqs, no?

> +		writel(-1U, priv->regs + APLIC_CLRIE_BASE +
> +			    (i / 32) * sizeof(u32));
> +		writel(priv->saved_ie[i / 32],
> +		       priv->regs + APLIC_SETIE_BASE +
> +		       (i / 32) * sizeof(u32));
> +	}
> +
> +	if (priv->nr_idcs) {
> +		aplic_direct_restore(priv);
> +	} else {
> +		/* Re-trigger the interrupts */
> +		for (i = 0; i <= priv->nr_irqs; i += 32) {

Same here

> +			clrip = readl(priv->regs + APLIC_CLRIP_BASE +
> +				      (i / 32) * sizeof(u32));
> +			writel(clrip, priv->regs + APLIC_SETIP_BASE +
> +				      (i / 32) * sizeof(u32));
> +		}

And this can properly be combined into:

        u32 __iomem *regs = priv->regs;

        for (i = 0; i < priv->nr_irqs; i += 32, regs++) {
		writel(-1U, regs + APLIC_CLRIE_BASE);
                writel(priv->saved_ie[i / 32], regs + APLIC_SETIE_BASE);

                if (!priv->nr_idcs)
                        writel(readl(regs + APLOC_CLRIP_BASE), regs + APLIC_SETIP_BASE);
	}

        if (priv->nr_idcs)
		aplic_direct_restore(priv);

No?

> +}
> +
> +static void aplic_save(struct aplic_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 1; i <= priv->nr_irqs; i++) {
> +		priv->saved_target[i - 1] = readl(priv->regs +
> +						  APLIC_TARGET_BASE +
> +						  (i - 1) * sizeof(u32));
> +	}

Oh well...

> +
> +	for (i = 0; i <= priv->nr_irqs; i += 32) {

Off by one again ...

> +		priv->saved_ie[i / 32] = readl(priv->regs +
> +					       APLIC_SETIE_BASE +
> +					       (i / 32) * sizeof(u32));
> +	}
> +}
> +
> +static int aplic_syscore_suspend(void)
> +{
> +	struct aplic_priv *priv;
> +
> +	list_for_each_entry(priv, &aplics, head) {
> +		aplic_save(priv);
> +	}

Superflous brackets

> +
> +	return 0;
> +}
> +
> +static void aplic_syscore_resume(void)
> +{
> +	struct aplic_priv *priv;
> +
> +	list_for_each_entry(priv, &aplics, head) {
> +		aplic_restore(priv);
> +	}

Ditto

> +}
> +
> +static struct syscore_ops aplic_syscore_ops = {
> +	.suspend = aplic_syscore_suspend,
> +	.resume = aplic_syscore_resume,

See documentation about struct definitions and initializers

> +};
> +
> +static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct aplic_priv *priv = container_of(nb, struct aplic_priv, genpd_nb);
> +
> +	switch (action) {
> +	case GENPD_NOTIFY_PRE_OFF:
> +		aplic_save(priv);
> +		break;
> +	case GENPD_NOTIFY_ON:
> +		aplic_restore(priv);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static void aplic_remove(void *data)
> +{
> +	struct aplic_priv *priv = data;
> +
> +	list_del(&priv->head);

How is this list operation serialized?

> +	dev_pm_genpd_remove_notifier(priv->dev);
> +}
> +
> +static int aplic_add(struct device *dev, struct aplic_priv *priv)
> +{
> +	int ret;
> +
> +	list_add(&priv->head, &aplics);
> +	/* Add genpd notifier */
> +	priv->genpd_nb.notifier_call = aplic_pm_notifier;
> +	ret = dev_pm_genpd_add_notifier(dev, &priv->genpd_nb);
> +	if (ret && ret != -ENODEV && ret != -EOPNOTSUPP) {

What is this magic about? Lacks explanation and rationale.

> +		list_del(&priv->head);
> +		return ret;

...

> +	/* For power management */
> +	priv->saved_target = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> +					  GFP_KERNEL);
> +	if (!priv->saved_target)
> +		return -ENOMEM;
> +
> +	priv->saved_sourcecfg = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> +					     GFP_KERNEL);
> +	if (!priv->saved_sourcecfg)
> +		return -ENOMEM;
> +
> +	priv->saved_ie = devm_kzalloc(dev,
> +				      DIV_ROUND_UP(priv->nr_irqs, 32) * sizeof(u32),
> +				      GFP_KERNEL);
> +	if (!priv->saved_ie)
> +		return -ENOMEM;

Seriously? You allocate all of this seperately? Ever heard about the
concept of data structures?

struct savedregs {
	u32	target;
        u32	source;
        u32	ie;
};

        priv->savedregs = devm_kcalloc(dev, priv->nr_irqs, sizeof(*priv->savedregs, GFP_KERNEL);
    
Yes, I know you don't need as many ie registers, but that's not a
problem at all. You just need to write/read to/from index 0, 32, 64 ...

And your restore muck boils down to a single loop:

        u32 __iomem *regs = priv->regs;
        for (i = 0; i < priv->nr_irqs; i++, regs++) {
        	writel(priv->savedregs->source[i], regs + APLIC_SOURCECFG_BASE);        
        	writel(priv->savedregs->target[i], regs + APLIC_TARGET_BASE);

                if (i % 32)
                	continue;

		writel(-1U, regs + APLIC_CLRIE_BASE);
                writel(priv->saved_ie[i], regs + APLIC_SETIE_BASE);
                if (!priv->nr_idcs)
                        writel(readl(regs + APLOC_CLRIP_BASE), regs + APLIC_SETIP_BASE);
	}

That's too comprehensible, right?

>  struct aplic_priv {
> +	struct list_head	head;

Why needs this to be at the top?

There is no reason at all. Not that it matters much as this data
structure is a trainwreck vs. cacheline efficiency anyway.

>  	struct device		*dev;
>  	u32			gsi_base;
>  	u32			nr_irqs;
> @@ -31,6 +32,15 @@ struct aplic_priv {
>  	u32			acpi_aplic_id;
>  	void __iomem		*regs;
>  	struct aplic_msicfg	msicfg;
> +	struct notifier_block	genpd_nb;
> +	u32 *saved_target;

So you aligned @head and @genpd_nb, but then you use random formatting
to turn this into visual clutter.

I'm amazed that this lot has received three Reviewed-by tags and nobody
found even the slightest issue with it. Oh well.

Thanks,

        tglx


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

Re: [PATCH v2 2/2] irqchip/riscv-aplic: Save and restore APLIC registers

[Nick Hu]

On Wed, Aug 6, 2025 at 8:52 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Aug 06 2025 at 16:27, Nick Hu wrote:
> > The APLIC may be powered down when the CPUs enter a deep sleep state.
> > Therefore adding the APLIC save and restore functions to save and
> > restore the states of APLIC.
>
> Same comments vs. subject and change log.
>
> >
> > +void aplic_direct_restore(struct aplic_priv *priv)
> > +{
> > +     struct aplic_direct *direct =
> > +                     container_of(priv, struct aplic_direct, priv);
>
> No line break required.
>
> > +     int cpu;
> > +
> > +     for_each_cpu(cpu, &direct->lmask)
> > +             aplic_idc_set_delivery(per_cpu_ptr(&aplic_idcs, cpu), true);
> > +static LIST_HEAD(aplics);
> > +
> > +static void aplic_restore(struct aplic_priv *priv)
> > +{
> > +     int i;
> > +     u32 clrip;
>
> See the documentation I linked you to and look for the chapter about
> variable declarations.
>
> > +     writel(priv->saved_domaincfg, priv->regs + APLIC_DOMAINCFG);
> > +#ifdef CONFIG_RISCV_M_MODE
> > +     writel(priv->saved_msiaddr, priv->regs + APLIC_xMSICFGADDR);
> > +     writel(priv->saved_msiaddrh, priv->regs + APLIC_xMSICFGADDRH);
> > +#endif
> > +     for (i = 1; i <= priv->nr_irqs; i++) {
> > +             writel(priv->saved_sourcecfg[i - 1],
> > +                    priv->regs + APLIC_SOURCECFG_BASE +
> > +                    (i - 1) * sizeof(u32));
> > +             writel(priv->saved_target[i - 1],
> > +                    priv->regs + APLIC_TARGET_BASE +
> > +                    (i - 1) * sizeof(u32));
>
> Sorry, but this is incomprehensible garbage.
>
> Why are you starting with i = 1 when you subtract 1 from i at every
> usage site?
>
The AIA specification states, “The number zero is not a valid
interrupt identity number at an APLIC,”
so I thought it was intuitive to start with i = 1 up to
`priv->nr_irqs`. However, you are right, I should start
from 0 so the index doesn’t need to be offset by 1 in every iteration.

>         u32 __iomem *regs = priv->regs;
>         for (i = 0; i < priv->nr_irqs; i++, regs++) {
>                 writel(priv->saved_sourcecfg[i], regs + APLIC_SOURCECFG_BASE);
>                 writel(priv->saved_target[i], regs + APLIC_TARGET_BASE);
>         }
>
> See?
>
> > +     }
> > +
> > +     for (i = 0; i <= priv->nr_irqs; i += 32) {
>
> Off by one. This needs to be i < priv->nr_irqs, no?
>
We need to preserve bits 1 through priv->nr_irqs of the SETIE
register, so this should be i <= priv->nr_irqs.

> > +             writel(-1U, priv->regs + APLIC_CLRIE_BASE +
> > +                         (i / 32) * sizeof(u32));
> > +             writel(priv->saved_ie[i / 32],
> > +                    priv->regs + APLIC_SETIE_BASE +
> > +                    (i / 32) * sizeof(u32));
> > +     }
> > +
> > +     if (priv->nr_idcs) {
> > +             aplic_direct_restore(priv);
> > +     } else {
> > +             /* Re-trigger the interrupts */
> > +             for (i = 0; i <= priv->nr_irqs; i += 32) {
>
> Same here
>
> > +                     clrip = readl(priv->regs + APLIC_CLRIP_BASE +
> > +                                   (i / 32) * sizeof(u32));
> > +                     writel(clrip, priv->regs + APLIC_SETIP_BASE +
> > +                                   (i / 32) * sizeof(u32));
> > +             }
>
> And this can properly be combined into:
>
>         u32 __iomem *regs = priv->regs;
>
>         for (i = 0; i < priv->nr_irqs; i += 32, regs++) {
>                 writel(-1U, regs + APLIC_CLRIE_BASE);
>                 writel(priv->saved_ie[i / 32], regs + APLIC_SETIE_BASE);
>
>                 if (!priv->nr_idcs)
>                         writel(readl(regs + APLOC_CLRIP_BASE), regs + APLIC_SETIP_BASE);
>         }
>
>         if (priv->nr_idcs)
>                 aplic_direct_restore(priv);
>
> No?
>
I'll update it in the next version.

> > +}
> > +
> > +static void aplic_save(struct aplic_priv *priv)
> > +{
> > +     int i;
> > +
> > +     for (i = 1; i <= priv->nr_irqs; i++) {
> > +             priv->saved_target[i - 1] = readl(priv->regs +
> > +                                               APLIC_TARGET_BASE +
> > +                                               (i - 1) * sizeof(u32));
> > +     }
>
> Oh well...
>
> > +
> > +     for (i = 0; i <= priv->nr_irqs; i += 32) {
>
> Off by one again ...
>
> > +             priv->saved_ie[i / 32] = readl(priv->regs +
> > +                                            APLIC_SETIE_BASE +
> > +                                            (i / 32) * sizeof(u32));
> > +     }
> > +}
> > +
> > +static int aplic_syscore_suspend(void)
> > +{
> > +     struct aplic_priv *priv;
> > +
> > +     list_for_each_entry(priv, &aplics, head) {
> > +             aplic_save(priv);
> > +     }
>
> Superflous brackets
>
> > +
> > +     return 0;
> > +}
> > +
> > +static void aplic_syscore_resume(void)
> > +{
> > +     struct aplic_priv *priv;
> > +
> > +     list_for_each_entry(priv, &aplics, head) {
> > +             aplic_restore(priv);
> > +     }
>
> Ditto
>
> > +}
> > +
> > +static struct syscore_ops aplic_syscore_ops = {
> > +     .suspend = aplic_syscore_suspend,
> > +     .resume = aplic_syscore_resume,
>
> See documentation about struct definitions and initializers
>
> > +};
> > +
> > +static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
> > +{
> > +     struct aplic_priv *priv = container_of(nb, struct aplic_priv, genpd_nb);
> > +
> > +     switch (action) {
> > +     case GENPD_NOTIFY_PRE_OFF:
> > +             aplic_save(priv);
> > +             break;
> > +     case GENPD_NOTIFY_ON:
> > +             aplic_restore(priv);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void aplic_remove(void *data)
> > +{
> > +     struct aplic_priv *priv = data;
> > +
> > +     list_del(&priv->head);
>
> How is this list operation serialized?
>
I'll add a lock to prevent the race from binding/unbinding the driver
at runtime, Thanks.

> > +     dev_pm_genpd_remove_notifier(priv->dev);
> > +}
> > +
> > +static int aplic_add(struct device *dev, struct aplic_priv *priv)
> > +{
> > +     int ret;
> > +
> > +     list_add(&priv->head, &aplics);
> > +     /* Add genpd notifier */
> > +     priv->genpd_nb.notifier_call = aplic_pm_notifier;
> > +     ret = dev_pm_genpd_add_notifier(dev, &priv->genpd_nb);
> > +     if (ret && ret != -ENODEV && ret != -EOPNOTSUPP) {
>
> What is this magic about? Lacks explanation and rationale.
>
I’ll add the explanation:
It is okay when the platforms doesn't enable CONFIG_PM_GENERIC_DOMAINS
or define the
`power-domains` property in the APLIC DT node.

> > +             list_del(&priv->head);
> > +             return ret;
>
> ...
>
> > +     /* For power management */
> > +     priv->saved_target = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> > +                                       GFP_KERNEL);
> > +     if (!priv->saved_target)
> > +             return -ENOMEM;
> > +
> > +     priv->saved_sourcecfg = devm_kzalloc(dev, priv->nr_irqs * sizeof(u32),
> > +                                          GFP_KERNEL);
> > +     if (!priv->saved_sourcecfg)
> > +             return -ENOMEM;
> > +
> > +     priv->saved_ie = devm_kzalloc(dev,
> > +                                   DIV_ROUND_UP(priv->nr_irqs, 32) * sizeof(u32),
> > +                                   GFP_KERNEL);
> > +     if (!priv->saved_ie)
> > +             return -ENOMEM;
>
> Seriously? You allocate all of this seperately? Ever heard about the
> concept of data structures?
>
> struct savedregs {
>         u32     target;
>         u32     source;
>         u32     ie;
> };
>
>         priv->savedregs = devm_kcalloc(dev, priv->nr_irqs, sizeof(*priv->savedregs, GFP_KERNEL);
>
> Yes, I know you don't need as many ie registers, but that's not a
> problem at all. You just need to write/read to/from index 0, 32, 64 ...
>
> And your restore muck boils down to a single loop:
>
>         u32 __iomem *regs = priv->regs;
>         for (i = 0; i < priv->nr_irqs; i++, regs++) {
>                 writel(priv->savedregs->source[i], regs + APLIC_SOURCECFG_BASE);
>                 writel(priv->savedregs->target[i], regs + APLIC_TARGET_BASE);
>
>                 if (i % 32)
>                         continue;
>
>                 writel(-1U, regs + APLIC_CLRIE_BASE);
>                 writel(priv->saved_ie[i], regs + APLIC_SETIE_BASE);
>                 if (!priv->nr_idcs)
>                         writel(readl(regs + APLOC_CLRIP_BASE), regs + APLIC_SETIP_BASE);
>         }
>
> That's too comprehensible, right?
>
I can update it to:
         u32 __iomem *regs = priv->regs;
         for (i = 0; i <= priv->nr_irqs; i++) {
                 if (i < priv->nr_irqs) {
                         writel(priv->savedregs->source[i], regs +
APLIC_SOURCECFG_BASE + i);
                         writel(priv->savedregs->target[i], regs +
APLIC_TARGET_BASE + i);
                 }

                 if (i % 32)
                         continue;

                 writel(-1U, regs + APLIC_CLRIE_BASE + i / 32);
                 writel(priv->savedregs->ie[i], regs +
APLIC_SETIE_BASE + i  / 32);
                 if (!priv->nr_idcs)
                         writel(readl(regs + APLOC_CLRIP_BASE + i /
32), regs + APLIC_SETIP_BASE + i / 32);
         }

> >  struct aplic_priv {
> > +     struct list_head        head;
>
> Why needs this to be at the top?
>
> There is no reason at all. Not that it matters much as this data
> structure is a trainwreck vs. cacheline efficiency anyway.
>
Does it make sense to you if I put the hot data together ? Like:
        struct aplic_source_regs {
                u32                     *target;
                u32                     *sourcecfg;
                u32                     *ie;
        };

        struct aplic_saved_regs {
                u32                     domaincfg;
        #ifdef CONFIG_RISCV_M_MODE
                u32                     msiaddr;
                u32                     msiaddrh;
        #endif
                struct aplic_source_regs    *srcs
        };

        struct aplic_priv {
                struct list_head        head;
                struct aplic_saved_regs saved_regs;
                void __iomem            *regs;
                u32                     nr_irqs;
                u32                     nr_idcs;
                u32                     gsi_base;
                u32                     acpi_aplic_id;
                struct device           *dev;
                struct aplic_msicfg     msicfg;
                struct notifier_block   genpd_nb;
        };

Or any other suggestion?

> >       struct device           *dev;
> >       u32                     gsi_base;
> >       u32                     nr_irqs;
> > @@ -31,6 +32,15 @@ struct aplic_priv {
> >       u32                     acpi_aplic_id;
> >       void __iomem            *regs;
> >       struct aplic_msicfg     msicfg;
> > +     struct notifier_block   genpd_nb;
> > +     u32 *saved_target;
>
> So you aligned @head and @genpd_nb, but then you use random formatting
> to turn this into visual clutter.
>
Sorry about that.

> I'm amazed that this lot has received three Reviewed-by tags and nobody
> found even the slightest issue with it. Oh well.
>
> Thanks,
>
>         tglx
>

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