Re: [PATCH v3 -next 2/3] ACPI: RISC-V: Add LPI driver
From: Sunil V L
Date: Mon Mar 18 2024 - 04:43:04 EST
On Sun, Mar 17, 2024 at 10:33:15PM -0700, Drew Fustini wrote:
> On Thu, Jan 18, 2024 at 11:59:29AM +0530, Sunil V L wrote:
> > Enable Low Power Idle (LPI) based cpuidle driver for RISC-V platforms.
> > It depends on SBI HSM calls for idle state transitions.
> >
> > Signed-off-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/riscv/Makefile | 3 +-
> > drivers/acpi/riscv/cpuidle.c | 81 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 83 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/acpi/riscv/cpuidle.c
> >
> > diff --git a/drivers/acpi/riscv/Makefile b/drivers/acpi/riscv/Makefile
> > index 8b3b126e0b94..7309d92dd477 100644
> > --- a/drivers/acpi/riscv/Makefile
> > +++ b/drivers/acpi/riscv/Makefile
> > @@ -1,2 +1,3 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > -obj-y += rhct.o
> > +obj-y += rhct.o
> > +obj-$(CONFIG_ACPI_PROCESSOR_IDLE) += cpuidle.o
> > diff --git a/drivers/acpi/riscv/cpuidle.c b/drivers/acpi/riscv/cpuidle.c
> > new file mode 100644
> > index 000000000000..624f9bbdb58c
> > --- /dev/null
> > +++ b/drivers/acpi/riscv/cpuidle.c
> > @@ -0,0 +1,81 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2024, Ventana Micro Systems Inc
> > + * Author: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx>
> > + *
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <acpi/processor.h>
> > +#include <linux/cpu_pm.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/suspend.h>
> > +#include <asm/cpuidle.h>
> > +#include <asm/sbi.h>
> > +#include <asm/suspend.h>
> > +
> > +#define RISCV_FFH_LPI_TYPE_MASK GENMASK_ULL(63, 60)
> > +#define RISCV_FFH_LPI_RSVD_MASK GENMASK_ULL(59, 32)
> > +
> > +#define RISCV_FFH_LPI_TYPE_SBI BIT_ULL(60)
> > +
> > +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;
> > +
> > + if (!riscv_sbi_hsm_is_supported())
> > + return -ENODEV;
> > +
> > + 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) != 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;
>
> It seems that acpi_lpi_state.address is u64, so shouldn't state be u64
> instead of u32?
>
SBI suspend state is only 32 bits represented by lower 32 bits of
lpi->address.
Thanks,
Sunil