[PATCH -next 0/2] RISC-V: ACPI: Add LPI support

[PATCH -next 0/2] RISC-V: ACPI: Add LPI support

[Sunil V L]

This series adds support for Low Power Idle (LPI) on ACPI based
platforms. 

LPI is described in the ACPI spec [1]. RISC-V FFH spec required to
enable this is available at [2].

[1] - https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html#lpi-low-power-idle-states 
[2] - https://github.com/riscv-non-isa/riscv-acpi-ffh/releases/download/v/riscv-ffh.pdf

Sunil V L (2):
  ACPI: Enable ACPI_PROCESSOR for RISC-V
  cpuidle: RISC-V: Add ACPI LPI support

 drivers/acpi/Kconfig                |  2 +-
 drivers/cpuidle/cpuidle-riscv-sbi.c | 78 +++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH -next 1/2] ACPI: Enable ACPI_PROCESSOR for RISC-V

[Sunil V L]

The ACPI processor driver is not currently enabled for RISC-V.
This is required to enable CPU related functionalities like
LPI and CPPC. Hence, enable ACPI_PROCESSOR for RISC-V.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/acpi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index f819e760ff19..9a920752171c 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -282,7 +282,7 @@ config ACPI_CPPC_LIB
 
 config ACPI_PROCESSOR
 	tristate "Processor"
-	depends on X86 || ARM64 || LOONGARCH
+	depends on X86 || ARM64 || LOONGARCH || RISCV
 	select ACPI_PROCESSOR_IDLE
 	select ACPI_CPU_FREQ_PSS if X86 || LOONGARCH
 	select THERMAL
-- 
2.34.1

[PATCH -next 2/2] cpuidle: RISC-V: Add ACPI LPI support

[Sunil V L]

Add required callbacks to support Low Power Idle (LPI) on ACPI based
RISC-V platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 78 +++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
index e8094fc92491..cea67a54ab39 100644
--- a/drivers/cpuidle/cpuidle-riscv-sbi.c
+++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
@@ -632,3 +632,81 @@ static int __init sbi_cpuidle_init(void)
 	return 0;
 }
 device_initcall(sbi_cpuidle_init);
+
+#ifdef CONFIG_ACPI_PROCESSOR_IDLE
+
+#include <linux/acpi.h>
+#include <acpi/processor.h>
+
+#define RISCV_FFH_LPI_TYPE_MASK		0x1000000000000000ULL
+#define RISCV_FFH_LPI_RSVD_MASK		0x0FFFFFFF00000000ULL
+
+static int acpi_cpu_init_idle(unsigned int cpu)
+{
+	int i;
+	struct acpi_lpi_state *lpi;
+	struct acpi_processor *pr = per_cpu(processors, cpu);
+
+	if (unlikely(!pr || !pr->flags.has_lpi))
+		return -EINVAL;
+
+	/*
+	 * The SBI HSM suspend function is only available when:
+	 * 1) SBI version is 0.3 or higher
+	 * 2) SBI HSM extension is available
+	 */
+	if (sbi_spec_version < sbi_mk_version(0, 3) ||
+	    !sbi_probe_extension(SBI_EXT_HSM)) {
+		pr_warn("HSM suspend not available\n");
+		return -EINVAL;
+	}
+
+	if (pr->power.count <= 1)
+		return -ENODEV;
+
+	for (i = 1; i < pr->power.count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i];
+
+		/* Validate Entry Method as per FFH spec.
+		 * bits[63:60] should be 0x1
+		 * bits[59:32] should be 0x0
+		 * bits[31:0] represent a SBI power_state
+		 */
+		if (!(lpi->address & RISCV_FFH_LPI_TYPE_MASK) ||
+		    (lpi->address & RISCV_FFH_LPI_RSVD_MASK)) {
+			pr_warn("Invalid LPI entry method %#llx\n", lpi->address);
+			return -EINVAL;
+		}
+
+		state = lpi->address;
+		if (!sbi_suspend_state_is_valid(state)) {
+			pr_warn("Invalid SBI power state %#x\n", state);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+int acpi_processor_ffh_lpi_probe(unsigned int cpu)
+{
+	return acpi_cpu_init_idle(cpu);
+}
+
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
+{
+	u32 state = lpi->address;
+
+	if (state & SBI_HSM_SUSP_NON_RET_BIT)
+		return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend,
+						   lpi->index,
+						   state);
+	else
+		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
+							     lpi->index,
+							     state);
+}
+
+#endif
-- 
2.34.1

Re: [PATCH -next 1/2] ACPI: Enable ACPI_PROCESSOR for RISC-V

[Andrew Jones]

On Thu, Jan 11, 2024 at 03:00:57PM +0530, Sunil V L wrote:
> The ACPI processor driver is not currently enabled for RISC-V.
> This is required to enable CPU related functionalities like
> LPI and CPPC. Hence, enable ACPI_PROCESSOR for RISC-V.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  drivers/acpi/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index f819e760ff19..9a920752171c 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -282,7 +282,7 @@ config ACPI_CPPC_LIB
>  
>  config ACPI_PROCESSOR
>  	tristate "Processor"
> -	depends on X86 || ARM64 || LOONGARCH
> +	depends on X86 || ARM64 || LOONGARCH || RISCV
>  	select ACPI_PROCESSOR_IDLE
>  	select ACPI_CPU_FREQ_PSS if X86 || LOONGARCH
>  	select THERMAL
> -- 
> 2.34.1
>

Hi Sunil,

Typically we'll want the Kconfig changes to come at the end of a series,
or squashed into the patch that adds support for it, otherwise there's
risk of build breakage during bisection. In this case, we're safe because
the two new functions (I looked ahead) have __weak versions when they're
not present.

Also, interestingly, it looks like this ancient line

 obj-$(CONFIG_ACPI_PROCESSOR)    += processor.o

in drivers/acpi/Makefile should be removed, since there's no
drivers/acpi/processor.c file. I guess the make process silently
filters object files which don't have corresponding source files?
Maybe we should write a Makefile analyzer to see what other lines
can be removed...

Anyway, for this patch, which I'd prefer to be swapped in order with
the other patch, or just squashed into the other patch,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH -next 2/2] cpuidle: RISC-V: Add ACPI LPI support

[Andrew Jones]

On Thu, Jan 11, 2024 at 03:00:58PM +0530, Sunil V L wrote:
> Add required callbacks to support Low Power Idle (LPI) on ACPI based
> RISC-V platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  drivers/cpuidle/cpuidle-riscv-sbi.c | 78 +++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> index e8094fc92491..cea67a54ab39 100644
> --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> @@ -632,3 +632,81 @@ static int __init sbi_cpuidle_init(void)
>  	return 0;
>  }
>  device_initcall(sbi_cpuidle_init);
> +
> +#ifdef CONFIG_ACPI_PROCESSOR_IDLE
> +
> +#include <linux/acpi.h>
> +#include <acpi/processor.h>
> +
> +#define RISCV_FFH_LPI_TYPE_MASK		0x1000000000000000ULL
> +#define RISCV_FFH_LPI_RSVD_MASK		0x0FFFFFFF00000000ULL

GENMASK might look nicer and the type mask is 0xF000000000000000ULL,
where 0x1000000000000000ULL means that the type is an SBI identifier.
We need both defined

#define RISCV_FFH_LPI_TYPE_MASK              0xF000000000000000ULL
#define RISCV_FFH_LPI_TYPE_SBI               0x1000000000000000ULL

as I point out below.

> +
> +static int acpi_cpu_init_idle(unsigned int cpu)
> +{
> +	int i;
> +	struct acpi_lpi_state *lpi;
> +	struct acpi_processor *pr = per_cpu(processors, cpu);
> +
> +	if (unlikely(!pr || !pr->flags.has_lpi))
> +		return -EINVAL;
> +
> +	/*
> +	 * The SBI HSM suspend function is only available when:
> +	 * 1) SBI version is 0.3 or higher
> +	 * 2) SBI HSM extension is available
> +	 */
> +	if (sbi_spec_version < sbi_mk_version(0, 3) ||
> +	    !sbi_probe_extension(SBI_EXT_HSM)) {
> +		pr_warn("HSM suspend not available\n");

The comment and these lines match what's done in sbi_cpuidle_init().
How about a static helper function to avoid duplication?

> +		return -EINVAL;
> +	}
> +
> +	if (pr->power.count <= 1)
> +		return -ENODEV;
> +
> +	for (i = 1; i < pr->power.count; i++) {
> +		u32 state;
> +
> +		lpi = &pr->power.lpi_states[i];
> +
> +		/* Validate Entry Method as per FFH spec.
> +		 * bits[63:60] should be 0x1
> +		 * bits[59:32] should be 0x0
> +		 * bits[31:0] represent a SBI power_state
                                        ^ an

> +		 */

Comment block needs opening wing (/*)

> +		if (!(lpi->address & RISCV_FFH_LPI_TYPE_MASK) ||

This should be (lpi->address & RISCV_FFH_LPI_TYPE_MASK) != RISCV_FFH_LPI_TYPE_SBI

> +		    (lpi->address & RISCV_FFH_LPI_RSVD_MASK)) {
> +			pr_warn("Invalid LPI entry method %#llx\n", lpi->address);
> +			return -EINVAL;
> +		}
> +
> +		state = lpi->address;
> +		if (!sbi_suspend_state_is_valid(state)) {
> +			pr_warn("Invalid SBI power state %#x\n", state);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> +{
> +	return acpi_cpu_init_idle(cpu);
> +}
> +
> +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
> +{
> +	u32 state = lpi->address;
> +
> +	if (state & SBI_HSM_SUSP_NON_RET_BIT)
> +		return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend,
> +						   lpi->index,
> +						   state);
> +	else
> +		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
> +							     lpi->index,
> +							     state);
> +}
> +
> +#endif
> -- 
> 2.34.1
>

Thanks,
drew

Re: [PATCH -next 1/2] ACPI: Enable ACPI_PROCESSOR for RISC-V

[Sunil V L]

On Thu, Jan 11, 2024 at 11:00:12AM +0100, Andrew Jones wrote:
> On Thu, Jan 11, 2024 at 03:00:57PM +0530, Sunil V L wrote:
> > The ACPI processor driver is not currently enabled for RISC-V.
> > This is required to enable CPU related functionalities like
> > LPI and CPPC. Hence, enable ACPI_PROCESSOR for RISC-V.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  drivers/acpi/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index f819e760ff19..9a920752171c 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -282,7 +282,7 @@ config ACPI_CPPC_LIB
> >  
> >  config ACPI_PROCESSOR
> >  	tristate "Processor"
> > -	depends on X86 || ARM64 || LOONGARCH
> > +	depends on X86 || ARM64 || LOONGARCH || RISCV
> >  	select ACPI_PROCESSOR_IDLE
> >  	select ACPI_CPU_FREQ_PSS if X86 || LOONGARCH
> >  	select THERMAL
> > -- 
> > 2.34.1
> >
> 
> Hi Sunil,
> 
> Typically we'll want the Kconfig changes to come at the end of a series,
> or squashed into the patch that adds support for it, otherwise there's
> risk of build breakage during bisection. In this case, we're safe because
> the two new functions (I looked ahead) have __weak versions when they're
> not present.
> 
Sure. Let me swap the order of the patches.

> Also, interestingly, it looks like this ancient line
> 
>  obj-$(CONFIG_ACPI_PROCESSOR)    += processor.o
> 
> in drivers/acpi/Makefile should be removed, since there's no
> drivers/acpi/processor.c file. I guess the make process silently
> filters object files which don't have corresponding source files?
> Maybe we should write a Makefile analyzer to see what other lines
> can be removed...
>
Interesting. 

Hi Rafael, any thoughts?
 
> Anyway, for this patch, which I'd prefer to be swapped in order with
> the other patch, or just squashed into the other patch,
>
I prefer to keep as 2 separate patches. I will swap the order.

> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
Thanks!
Sunil 

Re: [PATCH -next 2/2] cpuidle: RISC-V: Add ACPI LPI support

[Sunil V L]

On Thu, Jan 11, 2024 at 11:19:49AM +0100, Andrew Jones wrote:
> On Thu, Jan 11, 2024 at 03:00:58PM +0530, Sunil V L wrote:
> > Add required callbacks to support Low Power Idle (LPI) on ACPI based
> > RISC-V platforms.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  drivers/cpuidle/cpuidle-riscv-sbi.c | 78 +++++++++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> > 
> > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> > index e8094fc92491..cea67a54ab39 100644
> > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> > @@ -632,3 +632,81 @@ static int __init sbi_cpuidle_init(void)
> >  	return 0;
> >  }
> >  device_initcall(sbi_cpuidle_init);
> > +
> > +#ifdef CONFIG_ACPI_PROCESSOR_IDLE
> > +
> > +#include <linux/acpi.h>
> > +#include <acpi/processor.h>
> > +
> > +#define RISCV_FFH_LPI_TYPE_MASK		0x1000000000000000ULL
> > +#define RISCV_FFH_LPI_RSVD_MASK		0x0FFFFFFF00000000ULL
> 
> GENMASK might look nicer and the type mask is 0xF000000000000000ULL,
> where 0x1000000000000000ULL means that the type is an SBI identifier.
> We need both defined
> 
> #define RISCV_FFH_LPI_TYPE_MASK              0xF000000000000000ULL
> #define RISCV_FFH_LPI_TYPE_SBI               0x1000000000000000ULL
> 
Sure. Let me use GENMASK and define both MASK and SBI type.

> as I point out below.
> 
> > +
> > +static int acpi_cpu_init_idle(unsigned int cpu)
> > +{
> > +	int i;
> > +	struct acpi_lpi_state *lpi;
> > +	struct acpi_processor *pr = per_cpu(processors, cpu);
> > +
> > +	if (unlikely(!pr || !pr->flags.has_lpi))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * The SBI HSM suspend function is only available when:
> > +	 * 1) SBI version is 0.3 or higher
> > +	 * 2) SBI HSM extension is available
> > +	 */
> > +	if (sbi_spec_version < sbi_mk_version(0, 3) ||
> > +	    !sbi_probe_extension(SBI_EXT_HSM)) {
> > +		pr_warn("HSM suspend not available\n");
> 
> The comment and these lines match what's done in sbi_cpuidle_init().
> How about a static helper function to avoid duplication?
> 
Sure.

> > +		return -EINVAL;
> > +	}
> > +
> > +	if (pr->power.count <= 1)
> > +		return -ENODEV;
> > +
> > +	for (i = 1; i < pr->power.count; i++) {
> > +		u32 state;
> > +
> > +		lpi = &pr->power.lpi_states[i];
> > +
> > +		/* Validate Entry Method as per FFH spec.
> > +		 * bits[63:60] should be 0x1
> > +		 * bits[59:32] should be 0x0
> > +		 * bits[31:0] represent a SBI power_state
>                                         ^ an
> 
> > +		 */
> 
> Comment block needs opening wing (/*)
> 
Okay.

> > +		if (!(lpi->address & RISCV_FFH_LPI_TYPE_MASK) ||
> 
> This should be (lpi->address & RISCV_FFH_LPI_TYPE_MASK) != RISCV_FFH_LPI_TYPE_SBI
> 
Sure.

Let me send v2 in couple of days with these changes.

Thanks!
Sunil

> > +		    (lpi->address & RISCV_FFH_LPI_RSVD_MASK)) {
> > +			pr_warn("Invalid LPI entry method %#llx\n", lpi->address);
> > +			return -EINVAL;
> > +		}
> > +
> > +		state = lpi->address;
> > +		if (!sbi_suspend_state_is_valid(state)) {
> > +			pr_warn("Invalid SBI power state %#x\n", state);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> > +{
> > +	return acpi_cpu_init_idle(cpu);
> > +}
> > +
> > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
> > +{
> > +	u32 state = lpi->address;
> > +
> > +	if (state & SBI_HSM_SUSP_NON_RET_BIT)
> > +		return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend,
> > +						   lpi->index,
> > +						   state);
> > +	else
> > +		return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
> > +							     lpi->index,
> > +							     state);
> > +}
> > +
> > +#endif
> > -- 
> > 2.34.1
> >
> 
> Thanks,
> drew

Re: [PATCH -next 1/2] ACPI: Enable ACPI_PROCESSOR for RISC-V

[Sudeep Holla]

On Thu, Jan 11, 2024 at 11:00:12AM +0100, Andrew Jones wrote:

[...]

> Also, interestingly, it looks like this ancient line
>
>  obj-$(CONFIG_ACPI_PROCESSOR)    += processor.o
>
> in drivers/acpi/Makefile should be removed,

No

> since there's no drivers/acpi/processor.c file.

Correct, but ..

> I guess the make process silently filters object files which don't have
> corresponding source files?

May be, but I doubt if that is the case here.

processor.o is just aggregation of all processor_*.o and this will be
the processor.ko when built as a module.

--
Regards,
Sudeep

Re: Re: [PATCH -next 1/2] ACPI: Enable ACPI_PROCESSOR for RISC-V

[Andrew Jones]

On Thu, Jan 11, 2024 at 12:16:06PM +0000, Sudeep Holla wrote:
> On Thu, Jan 11, 2024 at 11:00:12AM +0100, Andrew Jones wrote:
> 
> [...]
> 
> > Also, interestingly, it looks like this ancient line
> >
> >  obj-$(CONFIG_ACPI_PROCESSOR)    += processor.o
> >
> > in drivers/acpi/Makefile should be removed,
> 
> No
> 
> > since there's no drivers/acpi/processor.c file.
> 
> Correct, but ..
> 
> > I guess the make process silently filters object files which don't have
> > corresponding source files?
> 
> May be, but I doubt if that is the case here.
> 
> processor.o is just aggregation of all processor_*.o and this will be
> the processor.ko when built as a module.

Oh, I see. I had tried looking for a processor.o after building, to see if
it was something like that, but it still didn't appear. It didn't occur to
me to also try ACPI_PROCESSOR as a module.

I'll go put my head in some sand now.

Thanks,
drew

Re: [PATCH -next 2/2] cpuidle: RISC-V: Add ACPI LPI support

[Anup Patel]

On Thu, Jan 11, 2024 at 3:01 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> Add required callbacks to support Low Power Idle (LPI) on ACPI based
> RISC-V platforms.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  drivers/cpuidle/cpuidle-riscv-sbi.c | 78 +++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>
> diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> index e8094fc92491..cea67a54ab39 100644
> --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> @@ -632,3 +632,81 @@ static int __init sbi_cpuidle_init(void)
>         return 0;
>  }
>  device_initcall(sbi_cpuidle_init);
> +
> +#ifdef CONFIG_ACPI_PROCESSOR_IDLE
> +
> +#include <linux/acpi.h>
> +#include <acpi/processor.h>
> +
> +#define RISCV_FFH_LPI_TYPE_MASK                0x1000000000000000ULL
> +#define RISCV_FFH_LPI_RSVD_MASK                0x0FFFFFFF00000000ULL
> +
> +static int acpi_cpu_init_idle(unsigned int cpu)
> +{
> +       int i;
> +       struct acpi_lpi_state *lpi;
> +       struct acpi_processor *pr = per_cpu(processors, cpu);
> +
> +       if (unlikely(!pr || !pr->flags.has_lpi))
> +               return -EINVAL;
> +
> +       /*
> +        * The SBI HSM suspend function is only available when:
> +        * 1) SBI version is 0.3 or higher
> +        * 2) SBI HSM extension is available
> +        */
> +       if (sbi_spec_version < sbi_mk_version(0, 3) ||
> +           !sbi_probe_extension(SBI_EXT_HSM)) {
> +               pr_warn("HSM suspend not available\n");
> +               return -EINVAL;
> +       }
> +
> +       if (pr->power.count <= 1)
> +               return -ENODEV;
> +
> +       for (i = 1; i < pr->power.count; i++) {
> +               u32 state;
> +
> +               lpi = &pr->power.lpi_states[i];
> +
> +               /* Validate Entry Method as per FFH spec.
> +                * bits[63:60] should be 0x1
> +                * bits[59:32] should be 0x0
> +                * bits[31:0] represent a SBI power_state
> +                */
> +               if (!(lpi->address & RISCV_FFH_LPI_TYPE_MASK) ||
> +                   (lpi->address & RISCV_FFH_LPI_RSVD_MASK)) {
> +                       pr_warn("Invalid LPI entry method %#llx\n", lpi->address);
> +                       return -EINVAL;
> +               }
> +
> +               state = lpi->address;
> +               if (!sbi_suspend_state_is_valid(state)) {
> +                       pr_warn("Invalid SBI power state %#x\n", state);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> +{
> +       return acpi_cpu_init_idle(cpu);
> +}
> +
> +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
> +{
> +       u32 state = lpi->address;
> +
> +       if (state & SBI_HSM_SUSP_NON_RET_BIT)
> +               return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend,
> +                                                  lpi->index,
> +                                                  state);
> +       else
> +               return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
> +                                                            lpi->index,
> +                                                            state);
> +}
> +
> +#endif

Lets keep the cpuidle-riscv-sbi.c driver focused on DT only. Instead,
I would suggest moving the required function from cpuidle-riscv-sbi.c
to arch/riscv and have a separate driver under driver/acpi/riscv for
LPI states.

Regards,
Anup

Re: [PATCH -next 2/2] cpuidle: RISC-V: Add ACPI LPI support

[Sunil V L]

On Fri, Jan 12, 2024 at 10:35:07AM +0530, Anup Patel wrote:
> On Thu, Jan 11, 2024 at 3:01 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > Add required callbacks to support Low Power Idle (LPI) on ACPI based
> > RISC-V platforms.
> >
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  drivers/cpuidle/cpuidle-riscv-sbi.c | 78 +++++++++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> > index e8094fc92491..cea67a54ab39 100644
> > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> > @@ -632,3 +632,81 @@ static int __init sbi_cpuidle_init(void)
> >         return 0;
> >  }
> >  device_initcall(sbi_cpuidle_init);
> > +
> > +#ifdef CONFIG_ACPI_PROCESSOR_IDLE
> > +
> > +#include <linux/acpi.h>
> > +#include <acpi/processor.h>
> > +
> > +#define RISCV_FFH_LPI_TYPE_MASK                0x1000000000000000ULL
> > +#define RISCV_FFH_LPI_RSVD_MASK                0x0FFFFFFF00000000ULL
> > +
> > +static int acpi_cpu_init_idle(unsigned int cpu)
> > +{
> > +       int i;
> > +       struct acpi_lpi_state *lpi;
> > +       struct acpi_processor *pr = per_cpu(processors, cpu);
> > +
> > +       if (unlikely(!pr || !pr->flags.has_lpi))
> > +               return -EINVAL;
> > +
> > +       /*
> > +        * The SBI HSM suspend function is only available when:
> > +        * 1) SBI version is 0.3 or higher
> > +        * 2) SBI HSM extension is available
> > +        */
> > +       if (sbi_spec_version < sbi_mk_version(0, 3) ||
> > +           !sbi_probe_extension(SBI_EXT_HSM)) {
> > +               pr_warn("HSM suspend not available\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (pr->power.count <= 1)
> > +               return -ENODEV;
> > +
> > +       for (i = 1; i < pr->power.count; i++) {
> > +               u32 state;
> > +
> > +               lpi = &pr->power.lpi_states[i];
> > +
> > +               /* Validate Entry Method as per FFH spec.
> > +                * bits[63:60] should be 0x1
> > +                * bits[59:32] should be 0x0
> > +                * bits[31:0] represent a SBI power_state
> > +                */
> > +               if (!(lpi->address & RISCV_FFH_LPI_TYPE_MASK) ||
> > +                   (lpi->address & RISCV_FFH_LPI_RSVD_MASK)) {
> > +                       pr_warn("Invalid LPI entry method %#llx\n", lpi->address);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               state = lpi->address;
> > +               if (!sbi_suspend_state_is_valid(state)) {
> > +                       pr_warn("Invalid SBI power state %#x\n", state);
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> > +{
> > +       return acpi_cpu_init_idle(cpu);
> > +}
> > +
> > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
> > +{
> > +       u32 state = lpi->address;
> > +
> > +       if (state & SBI_HSM_SUSP_NON_RET_BIT)
> > +               return CPU_PM_CPU_IDLE_ENTER_PARAM(sbi_suspend,
> > +                                                  lpi->index,
> > +                                                  state);
> > +       else
> > +               return CPU_PM_CPU_IDLE_ENTER_RETENTION_PARAM(sbi_suspend,
> > +                                                            lpi->index,
> > +                                                            state);
> > +}
> > +
> > +#endif
> 
> Lets keep the cpuidle-riscv-sbi.c driver focused on DT only. Instead,
> I would suggest moving the required function from cpuidle-riscv-sbi.c
> to arch/riscv and have a separate driver under driver/acpi/riscv for
> LPI states.
> 
Okay, sure. Let me send v2 with your suggestion.

Thanks,
Sunil