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

From: Thomas Gleixner <tglx@linutronix.de>
To: Nick Hu <nick.hu@sifive.com>,
	anup@brainfault.org, Alexandre Ghiti <alex@ghiti.fr>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Cc: Nick Hu <nick.hu@sifive.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>
Subject: Re: [PATCH v2 1/2] irqchip/riscv-imsic: Restore the IMSIC registers
Date: Wed, 06 Aug 2025 14:08:16 +0200	[thread overview]
Message-ID: <878qjwejpr.ffs@tglx> (raw)
In-Reply-To: <20250806082726.8835-2-nick.hu@sifive.com>

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