[PATCH 00/24] Add basic ACPI support for RISC-V

[PATCH 00/24] Add basic ACPI support for RISC-V

[Sunil V L]

This patch series enables the basic ACPI infrastructure for RISC-V.
Supporting external interrupt controllers is in progress and hence it is
tested using polling based HVC SBI console and RAM disk.

The series depends on Anup's IPI improvement series.
https://github.com/avpatel/linux/commits/riscv_ipi_imp_v17

These changes are available at
https://github.com/vlsunil/linux/commits/acpi_b1_us_review_ipi17

Testing:
1) Build Qemu with ACPI support using below branch
https://github.com/vlsunil/qemu/tree/acpi_b1_us_review

2) Build EDK2 as per instructions in
https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support

3) Build Linux after enabling SBI HVC and SBI earlycon
CONFIG_RISCV_SBI_V01=y
CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
CONFIG_HVC_RISCV_SBI=y

4) Build buildroot.

Run with below command.
qemu-system-riscv64   -nographic \
-drive file=Build/RiscVVirtQemu/RELEASE_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1 \
-machine virt,acpi=on -smp 16 -m 2G \
-kernel arch/riscv/boot/Image \
-initrd buildroot/output/images/rootfs.cpio \
-append "root=/dev/ram ro console=hvc0 earlycon=sbi"

Jisheng Zhang (1):
  riscv: move sbi_init() earlier before jump_label_init()

Sunil V L (23):
  ACPICA: MADT: Add RISC-V INTC interrupt controller
  ACPICA: Add structure definitions for RISC-V RHCT
  RISC-V: ACPI: Add empty headers to enable ACPI core
  RISC-V: ACPI: Add basic functions to build ACPI core
  RISC-V: ACPI: Add PCI functions to build ACPI core
  RISC-V: ACPI: Enable ACPI build infrastructure
  ACPI: Enable ACPI_PROCESSOR for RISC-V
  ACPI: OSL: Make should_use_kmap() 0 for RISC-V.
  ACPI: processor_core: RISC-V: Enable mapping processor to the hartid
  RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support
  RISC-V: ACPI: smpboot: Create wrapper smp_setup()
  RISC-V: ACPI: smpboot: Add ACPI support in smp_setup()
  RISC-V: ACPI: smpboot: Add function to retrieve the hartid
  clocksource/timer-riscv: Refactor riscv_timer_init_dt()
  RISC-V: ACPI: clocksource/timer-riscv: Add ACPI support
  ACPI: RISC-V: drivers/acpi: Add RHCT related code
  RISC-V: ACPI: time.c: Add ACPI support for time_init()
  RISC-V: ACPI: cpufeature: Add ACPI support in riscv_fill_hwcap()
  RISC-V: ACPI: cpu: Enable cpuinfo for ACPI systems
  RISC-V: ACPI: Add ACPI initialization in setup_arch()
  RISC-V: ACPI: Enable ACPI in defconfig
  MAINTAINERS: Add entry for drivers/acpi/riscv
  Documentation/kernel-parameters.txt: Add RISC-V for ACPI parameter

 .../admin-guide/kernel-parameters.txt         |   6 +-
 MAINTAINERS                                   |   7 +
 arch/riscv/Kconfig                            |   5 +
 arch/riscv/configs/defconfig                  |   4 +
 arch/riscv/include/asm/acenv.h                |  17 ++
 arch/riscv/include/asm/acpi.h                 |  87 +++++++++
 arch/riscv/include/asm/cpu.h                  |   9 +
 arch/riscv/kernel/Makefile                    |   3 +
 arch/riscv/kernel/acpi.c                      | 178 ++++++++++++++++++
 arch/riscv/kernel/cpu.c                       |  36 +++-
 arch/riscv/kernel/cpufeature.c                |  45 ++++-
 arch/riscv/kernel/pci.c                       | 173 +++++++++++++++++
 arch/riscv/kernel/setup.c                     |  21 ++-
 arch/riscv/kernel/smpboot.c                   |  99 +++++++++-
 arch/riscv/kernel/time.c                      |  25 ++-
 drivers/acpi/Kconfig                          |   2 +-
 drivers/acpi/Makefile                         |   2 +
 drivers/acpi/osl.c                            |   2 +-
 drivers/acpi/processor_core.c                 |  28 +++
 drivers/acpi/riscv/Makefile                   |   2 +
 drivers/acpi/riscv/rhct.c                     |  93 +++++++++
 drivers/clocksource/timer-riscv.c             |  88 ++++-----
 drivers/irqchip/irq-riscv-intc.c              |  79 ++++++--
 include/acpi/actbl2.h                         |  69 ++++++-
 24 files changed, 994 insertions(+), 86 deletions(-)
 create mode 100644 arch/riscv/include/asm/acenv.h
 create mode 100644 arch/riscv/include/asm/acpi.h
 create mode 100644 arch/riscv/include/asm/cpu.h
 create mode 100644 arch/riscv/kernel/acpi.c
 create mode 100644 arch/riscv/kernel/pci.c
 create mode 100644 drivers/acpi/riscv/Makefile
 create mode 100644 drivers/acpi/riscv/rhct.c

-- 
2.38.0

[PATCH 01/24] riscv: move sbi_init() earlier before jump_label_init()

[Sunil V L]

From: Jisheng Zhang <jszhang@kernel.org>

We call jump_label_init() in setup_arch() is to use static key
mechanism earlier, but riscv jump label relies on the sbi functions,
If we enable static key before sbi_init(), the code path looks like:
  static_branch_enable()
    ..
      arch_jump_label_transform()
        patch_text_nosync()
          flush_icache_range()
            flush_icache_all()
              sbi_remote_fence_i() for CONFIG_RISCV_SBI case
                __sbi_rfence()

Since sbi isn't initialized, so NULL deference! Here is a typical
panic log:

[    0.000000] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[    0.000000] Oops [#1]
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc7+ #79
[    0.000000] Hardware name: riscv-virtio,qemu (DT)
[    0.000000] epc : 0x0
[    0.000000]  ra : sbi_remote_fence_i+0x1e/0x26
[    0.000000] epc : 0000000000000000 ra : ffffffff80005826 sp : ffffffff80c03d50
[    0.000000]  gp : ffffffff80ca6178 tp : ffffffff80c0ad80 t0 : 6200000000000000
[    0.000000]  t1 : 0000000000000000 t2 : 62203a6b746e6972 s0 : ffffffff80c03d60
[    0.000000]  s1 : ffffffff80001af6 a0 : 0000000000000000 a1 : 0000000000000000
[    0.000000]  a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
[    0.000000]  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000080200
[    0.000000]  s2 : ffffffff808b3e48 s3 : ffffffff808bf698 s4 : ffffffff80cb2818
[    0.000000]  s5 : 0000000000000001 s6 : ffffffff80c9c345 s7 : ffffffff80895aa0
[    0.000000]  s8 : 0000000000000001 s9 : 000000000000007f s10: 0000000000000000
[    0.000000]  s11: 0000000000000000 t3 : ffffffff80824d08 t4 : 0000000000000022
[    0.000000]  t5 : 000000000000003d t6 : 0000000000000000
[    0.000000] status: 0000000000000100 badaddr: 0000000000000000 cause: 000000000000000c
[    0.000000] ---[ end trace 0000000000000000 ]---
[    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---

Fix this issue by moving sbi_init() earlier before jump_label_init()

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/kernel/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 86acd690d529..4335f08ffaf2 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -270,6 +270,7 @@ void __init setup_arch(char **cmdline_p)
 	*cmdline_p = boot_command_line;
 
 	early_ioremap_setup();
+	sbi_init();
 	jump_label_init();
 	parse_early_param();
 
@@ -287,7 +288,6 @@ void __init setup_arch(char **cmdline_p)
 	misc_mem_init();
 
 	init_resources();
-	sbi_init();
 
 #ifdef CONFIG_KASAN
 	kasan_init();
-- 
2.38.0

[PATCH 02/24] ACPICA: MADT: Add RISC-V INTC interrupt controller

[Sunil V L]

The ECR to add RISC-V INTC interrupt controller is approved by
the UEFI forum and will be availabl in the next revision of
the ACPI specification.

This patch is not yet merged in ACPICA but a PR is raised.

ACPICA PR: https://github.com/acpica/acpica/pull/804
Reference: Mantis ID: 2348

Cc: Robert Moore <robert.moore@intel.com>
Cc: acpica-devel@lists.linuxfoundation.org
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 include/acpi/actbl2.h | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index b2973dbe37ee..c432fd15db65 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -891,7 +891,8 @@ enum acpi_madt_type {
 	ACPI_MADT_TYPE_MSI_PIC = 21,
 	ACPI_MADT_TYPE_BIO_PIC = 22,
 	ACPI_MADT_TYPE_LPC_PIC = 23,
-	ACPI_MADT_TYPE_RESERVED = 24,	/* 24 to 0x7F are reserved */
+	ACPI_MADT_TYPE_RINTC = 24,
+	ACPI_MADT_TYPE_RESERVED = 25,	/* 25 to 0x7F are reserved */
 	ACPI_MADT_TYPE_OEM_RESERVED = 0x80	/* 0x80 to 0xFF are reserved for OEM use */
 };
 
@@ -1250,6 +1251,24 @@ enum acpi_madt_lpc_pic_version {
 	ACPI_MADT_LPC_PIC_VERSION_RESERVED = 2	/* 2 and greater are reserved */
 };
 
+/* 24: RISC-V INTC */
+struct acpi_madt_rintc {
+	struct acpi_subtable_header header;
+	u8 version;
+	u8 reserved;
+	u32 flags;
+	u64 hart_id;
+	u32 uid;		/* ACPI processor UID */
+};
+
+/* Values for RISC-V INTC Version field above */
+
+enum acpi_madt_rintc_version {
+	ACPI_MADT_RINTC_VERSION_NONE = 0,
+	ACPI_MADT_RINTC_VERSION_V1 = 1,
+	ACPI_MADT_RINTC_VERSION_RESERVED = 2	/* 2 and greater are reserved */
+};
+
 /* 80: OEM data */
 
 struct acpi_madt_oem_data {
-- 
2.38.0

Re: [PATCH 02/24] ACPICA: MADT: Add RISC-V INTC interrupt controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

On Mon, Jan 30, 2023 at 11:52:03PM +0530, Sunil V L wrote:
> The ECR to add RISC-V INTC interrupt controller is approved by
> the UEFI forum and will be availabl in the next revision of

nit: available

> the ACPI specification.
> 
> This patch is not yet merged in ACPICA but a PR is raised.
> 
> ACPICA PR: https://github.com/acpica/acpica/pull/804

I had a quick check with git grep, and as this doesn't appear to be a
regular pattern in the history, so could you please make this a regular
Link: trailer?

Cheers,
Conor.

> Reference: Mantis ID: 2348
> 
> Cc: Robert Moore <robert.moore@intel.com>
> Cc: acpica-devel@lists.linuxfoundation.org
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  include/acpi/actbl2.h | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index b2973dbe37ee..c432fd15db65 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -891,7 +891,8 @@ enum acpi_madt_type {
>  	ACPI_MADT_TYPE_MSI_PIC = 21,
>  	ACPI_MADT_TYPE_BIO_PIC = 22,
>  	ACPI_MADT_TYPE_LPC_PIC = 23,
> -	ACPI_MADT_TYPE_RESERVED = 24,	/* 24 to 0x7F are reserved */
> +	ACPI_MADT_TYPE_RINTC = 24,
> +	ACPI_MADT_TYPE_RESERVED = 25,	/* 25 to 0x7F are reserved */
>  	ACPI_MADT_TYPE_OEM_RESERVED = 0x80	/* 0x80 to 0xFF are reserved for OEM use */
>  };
>  
> @@ -1250,6 +1251,24 @@ enum acpi_madt_lpc_pic_version {
>  	ACPI_MADT_LPC_PIC_VERSION_RESERVED = 2	/* 2 and greater are reserved */
>  };
>  
> +/* 24: RISC-V INTC */
> +struct acpi_madt_rintc {
> +	struct acpi_subtable_header header;
> +	u8 version;
> +	u8 reserved;
> +	u32 flags;
> +	u64 hart_id;
> +	u32 uid;		/* ACPI processor UID */
> +};
> +
> +/* Values for RISC-V INTC Version field above */
> +
> +enum acpi_madt_rintc_version {
> +	ACPI_MADT_RINTC_VERSION_NONE = 0,
> +	ACPI_MADT_RINTC_VERSION_V1 = 1,
> +	ACPI_MADT_RINTC_VERSION_RESERVED = 2	/* 2 and greater are reserved */
> +};
> +
>  /* 80: OEM data */
>  
>  struct acpi_madt_oem_data {
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 02/24] ACPICA: MADT: Add RISC-V INTC interrupt controller

[Sunil V L]

On Wed, Feb 08, 2023 at 07:59:00PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:03PM +0530, Sunil V L wrote:
> > The ECR to add RISC-V INTC interrupt controller is approved by
> > the UEFI forum and will be availabl in the next revision of
> 
> nit: available
> 
Thanks!

> > the ACPI specification.
> > 
> > This patch is not yet merged in ACPICA but a PR is raised.
> > 
> > ACPICA PR: https://github.com/acpica/acpica/pull/804
> 
> I had a quick check with git grep, and as this doesn't appear to be a
> regular pattern in the history, so could you please make this a regular
> Link: trailer?
> 
This patch should be merged in acpica repo first and then we will get
this in standard format. Until then, it exists to allow other
patches get reviewed. I believe by the time we get all other patches
reviewed, acpica will have this merged.

Thanks!
Sunil

[PATCH 03/24] ACPICA: Add structure definitions for RISC-V RHCT

[Sunil V L]

RISC-V Hart Capabilities Table (RHCT) is a new static table.
The ECR to add RHCT is approved by the UEFI forum and will be
available in the next version of the ACPI spec.

This patch is not yet merged in ACPICA but a PR is raised.

ACPICA PR: https://github.com/acpica/acpica/pull/804
Reference: Mantis: 2349

Cc: Robert Moore <robert.moore@intel.com>
Cc: acpica-devel@lists.linuxfoundation.org
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 include/acpi/actbl2.h | 48 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index c432fd15db65..86bb79fdfa62 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -47,6 +47,7 @@
 #define ACPI_SIG_PRMT           "PRMT"	/* Platform Runtime Mechanism Table */
 #define ACPI_SIG_RASF           "RASF"	/* RAS Feature table */
 #define ACPI_SIG_RGRT           "RGRT"	/* Regulatory Graphics Resource Table */
+#define ACPI_SIG_RHCT           "RHCT"	/* RISC-V Hart Capabilities Table */
 #define ACPI_SIG_SBST           "SBST"	/* Smart Battery Specification Table */
 #define ACPI_SIG_SDEI           "SDEI"	/* Software Delegated Exception Interface Table */
 #define ACPI_SIG_SDEV           "SDEV"	/* Secure Devices table */
@@ -2604,6 +2605,53 @@ enum acpi_rgrt_image_type {
 	ACPI_RGRT_TYPE_RESERVED = 2	/* 2 and greater are reserved */
 };
 
+/*******************************************************************************
+ *
+ * RHCT - RISC-V Hart Capabilities Table
+ *        Version 1
+ *
+ ******************************************************************************/
+
+struct acpi_table_rhct {
+	struct acpi_table_header header;	/* Common ACPI table header */
+	u32 reserved;
+	u64 time_base_freq;
+	u32 node_count;
+	u32 node_offset;
+};
+
+/*
+ * RHCT subtables
+ */
+struct acpi_rhct_node_header {
+	u16 type;
+	u16 length;
+	u16 revision;
+};
+
+/* Values for RHCT subtable Type above */
+
+enum acpi_rhct_node_type {
+	ACPI_RHCT_NODE_TYPE_ISA_STRING = 0x0000,
+	ACPI_RHCT_NODE_TYPE_HART_INFO = 0xFFFF,
+};
+
+/*
+ * RHCT node specific subtables
+ */
+
+/* ISA string node structure */
+struct acpi_rhct_isa_string {
+	u16 isa_length;
+	char isa[];
+};
+
+/* Hart Info node structure */
+struct acpi_rhct_hart_info {
+	u16 num_offsets;
+	u32 uid;		/* ACPI processor UID */
+};
+
 /*******************************************************************************
  *
  * SBST - Smart Battery Specification Table
-- 
2.38.0

[PATCH 04/24] RISC-V: ACPI: Add empty headers to enable ACPI core

[Sunil V L]

Few header files are required unconditionally by ACPI core.
So add empty header files for now and update it when needed.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acenv.h | 17 +++++++++++++++++
 arch/riscv/include/asm/cpu.h   |  9 +++++++++
 2 files changed, 26 insertions(+)
 create mode 100644 arch/riscv/include/asm/acenv.h
 create mode 100644 arch/riscv/include/asm/cpu.h

diff --git a/arch/riscv/include/asm/acenv.h b/arch/riscv/include/asm/acenv.h
new file mode 100644
index 000000000000..bbc38ecdf753
--- /dev/null
+++ b/arch/riscv/include/asm/acenv.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * RISC-V specific ACPICA environments and implementation
+ *
+ * Copyright (C) 2014, Linaro Ltd.
+ *   Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *   Author: Graeme Gregory <graeme.gregory@linaro.org>
+ * Copyright (C) 2023, Ventana Micro Systems Inc.
+ *   Author: Sunil V L <sunilvl@ventanamicro.com>
+ */
+
+#ifndef _ASM_ACENV_H
+#define _ASM_ACENV_H
+
+/* It is required unconditionally by ACPI core, update it when needed. */
+
+#endif /* _ASM_ACENV_H */
diff --git a/arch/riscv/include/asm/cpu.h b/arch/riscv/include/asm/cpu.h
new file mode 100644
index 000000000000..51ec1a89a7a9
--- /dev/null
+++ b/arch/riscv/include/asm/cpu.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2014 ARM Ltd.
+ * Copyright (C) 2023 Ventana Micro Systems Inc.
+ */
+#ifndef __ASM_CPU_H
+#define __ASM_CPU_H
+
+#endif /* __ASM_CPU_H */
-- 
2.38.0

Re: [PATCH 04/24] RISC-V: ACPI: Add empty headers to enable ACPI core

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

On Mon, Jan 30, 2023 at 11:52:05PM +0530, Sunil V L wrote:
> Few header files are required unconditionally by ACPI core.

nit: A few. Without the article this has a different meaning.

> So add empty header files for now and update it when needed.

s/ and update it when needed// ;)

> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/include/asm/acenv.h | 17 +++++++++++++++++
>  arch/riscv/include/asm/cpu.h   |  9 +++++++++
>  2 files changed, 26 insertions(+)
>  create mode 100644 arch/riscv/include/asm/acenv.h
>  create mode 100644 arch/riscv/include/asm/cpu.h
> 
> diff --git a/arch/riscv/include/asm/acenv.h b/arch/riscv/include/asm/acenv.h
> new file mode 100644
> index 000000000000..bbc38ecdf753
> --- /dev/null
> +++ b/arch/riscv/include/asm/acenv.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * RISC-V specific ACPICA environments and implementation
> + *
> + * Copyright (C) 2014, Linaro Ltd.
> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> + * Copyright (C) 2023, Ventana Micro Systems Inc.
> + *   Author: Sunil V L <sunilvl@ventanamicro.com>

More out of idle curiosity than anything else, but what, may I ask, is
copyrightable about 2 empty files?
Triply so given there are !3! contributors to said empty file!

> + */
> +
> +#ifndef _ASM_ACENV_H
> +#define _ASM_ACENV_H
> +
> +/* It is required unconditionally by ACPI core, update it when needed. */

How come this file gets a comment and the other doesn't?
Also the comment doesn't really make much sense to me in a vacuum, and
ideally would read like:
"This header is required unconditionally by the ACPI core"
That is, if a comment is really even needed.

Cheers,
Conor.

> +
> +#endif /* _ASM_ACENV_H */
> diff --git a/arch/riscv/include/asm/cpu.h b/arch/riscv/include/asm/cpu.h
> new file mode 100644
> index 000000000000..51ec1a89a7a9
> --- /dev/null
> +++ b/arch/riscv/include/asm/cpu.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2014 ARM Ltd.
> + * Copyright (C) 2023 Ventana Micro Systems Inc.
> + */
> +#ifndef __ASM_CPU_H
> +#define __ASM_CPU_H
> +
> +#endif /* __ASM_CPU_H */
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 05/24] RISC-V: ACPI: Add basic functions to build ACPI core

[Sunil V L]

Introduce acpi.c and its related header files to provide
fundamental needs of extern variables and functions for ACPI core.
	- asm/acpi.h for arch specific variables and functions needed by
	  ACPI driver core;
	- acpi.c - Add function to initialize ACPI tables.
	- acpi.c for RISC-V related ACPI implementation for ACPI driver
	  core;

Code is mostly leveraged from ARM64.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h |  60 ++++++++++++
 arch/riscv/kernel/Makefile    |   2 +
 arch/riscv/kernel/acpi.c      | 178 ++++++++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 arch/riscv/include/asm/acpi.h
 create mode 100644 arch/riscv/kernel/acpi.c

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
new file mode 100644
index 000000000000..8b9babaf3f25
--- /dev/null
+++ b/arch/riscv/include/asm/acpi.h
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *	Author: Al Stone <al.stone@linaro.org>
+ *	Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *	Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *
+ *  Copyright (C) 2021-2023, Ventana Micro Systems Inc.
+ *	Author: Sunil V L <sunilvl@ventanamicro.com>
+ */
+
+#ifndef _ASM_ACPI_H
+#define _ASM_ACPI_H
+
+/* Basic configuration for ACPI */
+#ifdef CONFIG_ACPI
+
+/* ACPI table mapping after acpi_permanent_mmap is set */
+void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_ioremap acpi_os_ioremap
+
+#define acpi_strict 1   /* No out-of-spec workarounds on RISC-V */
+extern int acpi_disabled;
+extern int acpi_noirq;
+extern int acpi_pci_disabled;
+static inline void disable_acpi(void)
+{
+	acpi_disabled = 1;
+	acpi_pci_disabled = 1;
+	acpi_noirq = 1;
+}
+
+static inline void enable_acpi(void)
+{
+	acpi_disabled = 0;
+	acpi_pci_disabled = 0;
+	acpi_noirq = 0;
+}
+
+/*
+ * The ACPI processor driver for ACPI core code needs this macro
+ * to find out this cpu was already mapped (mapping from CPU hardware
+ * ID to CPU logical ID) or not.
+ */
+#define cpu_physical_id(cpu) cpuid_to_hartid_map(cpu)
+
+/*
+ * It's used from ACPI core in kdump to boot UP system with SMP kernel.
+ * Since MADT must provide at least one IMSIC structure for AIA
+ * initialization, CPU will be always available in MADT on RISC-V.
+ */
+static inline bool acpi_has_cpu_in_madt(void)
+{
+	return true;
+}
+
+static inline void arch_fix_phys_package_id(int num, u32 slot) { }
+#endif
+
+#endif /*_ASM_ACPI_H*/
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 67f542be1bea..f979dc8cf47d 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -90,3 +90,5 @@ obj-$(CONFIG_EFI)		+= efi.o
 obj-$(CONFIG_COMPAT)		+= compat_syscall_table.o
 obj-$(CONFIG_COMPAT)		+= compat_signal.o
 obj-$(CONFIG_COMPAT)		+= compat_vdso/
+
+obj-$(CONFIG_ACPI)              += acpi.o
diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
new file mode 100644
index 000000000000..9c1aa57bf4f7
--- /dev/null
+++ b/arch/riscv/kernel/acpi.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  RISC-V Specific Low-Level ACPI Boot Support
+ *
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *	Author: Al Stone <al.stone@linaro.org>
+ *	Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *	Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *	Author: Tomasz Nowicki <tomasz.nowicki@linaro.org>
+ *	Author: Naresh Bhat <naresh.bhat@linaro.org>
+ *
+ *  Copyright (C) 2021-2023, Ventana Micro Systems Inc.
+ *	Author: Sunil V L <sunilvl@ventanamicro.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/efi.h>
+#include <linux/efi-bgrt.h>
+#include <linux/io.h>
+#include <linux/of_fdt.h>
+#include <linux/serial_core.h>
+
+int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
+int acpi_disabled = 1;
+EXPORT_SYMBOL(acpi_disabled);
+
+int acpi_pci_disabled = 1;	/* skip ACPI PCI scan and IRQ initialization */
+EXPORT_SYMBOL(acpi_pci_disabled);
+
+static bool param_acpi_off __initdata;
+static bool param_acpi_on __initdata;
+static bool param_acpi_force __initdata;
+
+static int __init parse_acpi(char *arg)
+{
+	if (!arg)
+		return -EINVAL;
+
+	/* "acpi=off" disables both ACPI table parsing and interpreter */
+	if (strcmp(arg, "off") == 0)
+		param_acpi_off = true;
+	else if (strcmp(arg, "on") == 0) /* prefer ACPI over DT */
+		param_acpi_on = true;
+	else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
+		param_acpi_force = true;
+	else
+		return -EINVAL;	/* Core will print when we return error */
+
+	return 0;
+}
+early_param("acpi", parse_acpi);
+
+/*
+ * __acpi_map_table() will be called before page_init(), so early_ioremap()
+ * or early_memremap() should be called here to for ACPI table mapping.
+ */
+void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size)
+{
+	if (!size)
+		return NULL;
+
+	return early_memremap(phys, size);
+}
+
+void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
+{
+	if (!map || !size)
+		return;
+
+	early_memunmap(map, size);
+}
+
+void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+	return memremap(phys, size, MEMREMAP_WB);
+}
+
+/*
+ * acpi_fadt_sanity_check() - Check FADT presence and carry out sanity
+ *			      checks on it
+ *
+ * Return 0 on success,  <0 on failure
+ */
+static int __init acpi_fadt_sanity_check(void)
+{
+	struct acpi_table_header *table;
+	struct acpi_table_fadt *fadt;
+	acpi_status status;
+	int ret = 0;
+
+	/*
+	 * FADT is required on riscv; retrieve it to check its presence
+	 * and carry out revision and ACPI HW reduced compliancy tests
+	 */
+	status = acpi_get_table(ACPI_SIG_FADT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		const char *msg = acpi_format_exception(status);
+
+		pr_err("Failed to get FADT table, %s\n", msg);
+		return -ENODEV;
+	}
+
+	fadt = (struct acpi_table_fadt *)table;
+
+	/*
+	 * TODO: Should we add FADT version checks?
+	 */
+
+	if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) {
+		pr_err("FADT not ACPI hardware reduced compliant\n");
+		ret = -EINVAL;
+	}
+
+	/*
+	 * acpi_get_table() creates FADT table mapping that
+	 * should be released after parsing and before resuming boot
+	 */
+	acpi_put_table(table);
+	return ret;
+}
+
+/*
+ * acpi_boot_table_init() called from setup_arch(), always.
+ *	1. find RSDP and get its address, and then find XSDT
+ *	2. extract all tables and checksums them all
+ *	3. check ACPI FADT revision
+ *	4. check ACPI FADT HW reduced flag
+ *
+ * We can parse ACPI boot-time tables such as MADT after
+ * this function is called.
+ *
+ * On return ACPI is enabled if either:
+ *
+ * - ACPI tables are initialized and sanity checks passed
+ * - acpi=force was passed in the command line and ACPI was not disabled
+ *   explicitly through acpi=off command line parameter
+ *
+ * ACPI is disabled on function return otherwise
+ */
+void __init acpi_boot_table_init(void)
+{
+	/*
+	 * Enable ACPI instead of device tree unless
+	 * - ACPI has been disabled explicitly (acpi=off), or
+	 * - firmware has not populated ACPI ptr in EFI system table
+	 */
+
+	if (param_acpi_off || (efi.acpi20 == EFI_INVALID_TABLE_ADDR))
+		goto done;
+	/*
+	 * ACPI is disabled at this point. Enable it in order to parse
+	 * the ACPI tables and carry out sanity checks
+	 */
+	enable_acpi();
+
+	/*
+	 * If ACPI tables are initialized and FADT sanity checks passed,
+	 * leave ACPI enabled and carry on booting; otherwise disable ACPI
+	 * on initialization error.
+	 * If acpi=force was passed on the command line it forces ACPI
+	 * to be enabled even if its initialization failed.
+	 */
+	if (acpi_table_init() || acpi_fadt_sanity_check()) {
+		pr_err("Failed to init ACPI tables\n");
+		if (!param_acpi_force)
+			disable_acpi();
+	}
+
+done:
+	if (acpi_disabled) {
+		if (earlycon_acpi_spcr_enable)
+			early_init_dt_scan_chosen_stdout();
+	} else {
+		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
+		if (IS_ENABLED(CONFIG_ACPI_BGRT))
+			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
+	}
+}
-- 
2.38.0

Re: [PATCH 05/24] RISC-V: ACPI: Add basic functions to build ACPI core

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On Mon, Jan 30, 2023 at 11:52:06PM +0530, Sunil V L wrote:
> Introduce acpi.c and its related header files to provide
> fundamental needs of extern variables and functions for ACPI core.
> 	- asm/acpi.h for arch specific variables and functions needed by
> 	  ACPI driver core;
> 	- acpi.c - Add function to initialize ACPI tables.
> 	- acpi.c for RISC-V related ACPI implementation for ACPI driver
> 	  core;
> 
> Code is mostly leveraged from ARM64.



> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c

> + * __acpi_map_table() will be called before page_init(), so early_ioremap()

rg "\bpage_init\("
arch/riscv/kernel/acpi.c
54: * __acpi_map_table() will be called before page_init(), so early_ioremap()

arch/arm64/kernel/acpi.c
86: * __acpi_map_table() will be called before page_init(), so early_ioremap()

This function doesn't appear to exist, perhaps what you are looking for is
paging_init()?

> + * or early_memremap() should be called here to for ACPI table mapping.
> + */
> +void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size)
> +{
> +	if (!size)
> +		return NULL;
> +
> +	return early_memremap(phys, size);
> +}

> +void __init acpi_boot_table_init(void)
> +{
> +	/*
> +	 * Enable ACPI instead of device tree unless
> +	 * - ACPI has been disabled explicitly (acpi=off), or
> +	 * - firmware has not populated ACPI ptr in EFI system table
> +	 */
> +
> +	if (param_acpi_off || (efi.acpi20 == EFI_INVALID_TABLE_ADDR))

There's an extraneous set of () around the second item here.

> +		goto done;
> +	/*

A small nit: a newline before opening the comment block here please!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 05/24] RISC-V: ACPI: Add basic functions to build ACPI core

[Sunil V L]

On Wed, Feb 08, 2023 at 08:58:31PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:06PM +0530, Sunil V L wrote:
> > Introduce acpi.c and its related header files to provide
> > fundamental needs of extern variables and functions for ACPI core.
> > 	- asm/acpi.h for arch specific variables and functions needed by
> > 	  ACPI driver core;
> > 	- acpi.c - Add function to initialize ACPI tables.
> > 	- acpi.c for RISC-V related ACPI implementation for ACPI driver
> > 	  core;
> > 
> > Code is mostly leveraged from ARM64.
> 
> 
> 
> > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> 
> > + * __acpi_map_table() will be called before page_init(), so early_ioremap()
> 
> rg "\bpage_init\("
> arch/riscv/kernel/acpi.c
> 54: * __acpi_map_table() will be called before page_init(), so early_ioremap()
> 
> arch/arm64/kernel/acpi.c
> 86: * __acpi_map_table() will be called before page_init(), so early_ioremap()
> 
> This function doesn't appear to exist, perhaps what you are looking for is
> paging_init()?
> 
Yes, will update.

> > + * or early_memremap() should be called here to for ACPI table mapping.
> > + */
> > +void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size)
> > +{
> > +	if (!size)
> > +		return NULL;
> > +
> > +	return early_memremap(phys, size);
> > +}
> 
> > +void __init acpi_boot_table_init(void)
> > +{
> > +	/*
> > +	 * Enable ACPI instead of device tree unless
> > +	 * - ACPI has been disabled explicitly (acpi=off), or
> > +	 * - firmware has not populated ACPI ptr in EFI system table
> > +	 */
> > +
> > +	if (param_acpi_off || (efi.acpi20 == EFI_INVALID_TABLE_ADDR))
> 
> There's an extraneous set of () around the second item here.
> 
Okay.
> > +		goto done;
> > +	/*
> 
> A small nit: a newline before opening the comment block here please!

Thanks, will fix these in the next revision.
-Sunil

[PATCH 06/24] RISC-V: ACPI: Add PCI functions to build ACPI core

[Sunil V L]

When CONFIG_PCI is enabled, ACPI core expects few arch
functions related to PCI. Add those functions so that
ACPI core gets build. These are levraged from arm64.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/Makefile |   1 +
 arch/riscv/kernel/pci.c    | 173 +++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+)
 create mode 100644 arch/riscv/kernel/pci.c

diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index f979dc8cf47d..e9d37639751d 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -92,3 +92,4 @@ obj-$(CONFIG_COMPAT)		+= compat_signal.o
 obj-$(CONFIG_COMPAT)		+= compat_vdso/
 
 obj-$(CONFIG_ACPI)              += acpi.o
+obj-$(CONFIG_PCI)               += pci.o
diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c
new file mode 100644
index 000000000000..3388af3a67a0
--- /dev/null
+++ b/arch/riscv/kernel/pci.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Code borrowed from ARM64
+ *
+ * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
+ * Copyright (C) 2014 ARM Ltd.
+ * Copyright (C) 2022-2023 Ventana Micro System Inc.
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+#ifdef CONFIG_ACPI
+
+/*
+ * raw_pci_read/write - Platform-specific PCI config space access.
+ */
+int raw_pci_read(unsigned int domain, unsigned int bus,
+		  unsigned int devfn, int reg, int len, u32 *val)
+{
+	struct pci_bus *b = pci_find_bus(domain, bus);
+
+	if (!b)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return b->ops->read(b, devfn, reg, len, val);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int bus,
+		unsigned int devfn, int reg, int len, u32 val)
+{
+	struct pci_bus *b = pci_find_bus(domain, bus);
+
+	if (!b)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return b->ops->write(b, devfn, reg, len, val);
+}
+
+
+struct acpi_pci_generic_root_info {
+	struct acpi_pci_root_info	common;
+	struct pci_config_window	*cfg;	/* config space mapping */
+};
+
+int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	struct acpi_device *adev = to_acpi_device(cfg->parent);
+	struct acpi_pci_root *root = acpi_driver_data(adev);
+
+	return root->segment;
+}
+
+static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
+{
+	struct resource_entry *entry, *tmp;
+	int status;
+
+	status = acpi_pci_probe_root_resources(ci);
+	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
+		if (!(entry->res->flags & IORESOURCE_WINDOW))
+			resource_list_destroy_entry(entry);
+	}
+	return status;
+}
+
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static struct pci_config_window *
+pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
+{
+	struct device *dev = &root->device->dev;
+	struct resource *bus_res = &root->secondary;
+	u16 seg = root->segment;
+	const struct pci_ecam_ops *ecam_ops;
+	struct resource cfgres;
+	struct acpi_device *adev;
+	struct pci_config_window *cfg;
+	int ret;
+
+	ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
+	if (ret) {
+		dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
+		return NULL;
+	}
+
+	adev = acpi_resource_consumer(&cfgres);
+	if (adev)
+		dev_info(dev, "ECAM area %pR reserved by %s\n", &cfgres,
+			 dev_name(&adev->dev));
+	else
+		dev_warn(dev, FW_BUG "ECAM area %pR not reserved in ACPI namespace\n",
+			 &cfgres);
+
+	cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops);
+	if (IS_ERR(cfg)) {
+		dev_err(dev, "%04x:%pR error %ld mapping ECAM\n", seg, bus_res,
+			PTR_ERR(cfg));
+		return NULL;
+	}
+
+	return cfg;
+}
+
+/* release_info: free resources allocated by init_info */
+static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
+{
+	struct acpi_pci_generic_root_info *ri;
+
+	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
+	pci_ecam_free(ri->cfg);
+	kfree(ci->ops);
+	kfree(ri);
+}
+
+
+/* Interface called from ACPI code to setup PCI host controller */
+struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+{
+	struct acpi_pci_generic_root_info *ri;
+	struct pci_bus *bus, *child;
+	struct acpi_pci_root_ops *root_ops;
+	struct pci_host_bridge *host;
+
+	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
+	if (!ri)
+		return NULL;
+
+	root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
+	if (!root_ops) {
+		kfree(ri);
+		return NULL;
+	}
+
+	ri->cfg = pci_acpi_setup_ecam_mapping(root);
+	if (!ri->cfg) {
+		kfree(ri);
+		kfree(root_ops);
+		return NULL;
+	}
+
+	root_ops->release_info = pci_acpi_generic_release_info;
+	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
+	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
+	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
+	if (!bus)
+		return NULL;
+
+	/* If we must preserve the resource configuration, claim now */
+	host = pci_find_host_bridge(bus);
+	if (host->preserve_config)
+		pci_bus_claim_resources(bus);
+
+	/*
+	 * Assign whatever was left unassigned. If we didn't claim above,
+	 * this will reassign everything.
+	 */
+	pci_assign_unassigned_root_bus_resources(bus);
+
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	return bus;
+}
+
+#endif
-- 
2.38.0

Re: [PATCH 06/24] RISC-V: ACPI: Add PCI functions to build ACPI core

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6736 bytes --]

On Mon, Jan 30, 2023 at 11:52:07PM +0530, Sunil V L wrote:
> When CONFIG_PCI is enabled, ACPI core expects few arch
> functions related to PCI. Add those functions so that
> ACPI core gets build. These are levraged from arm64.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/kernel/Makefile |   1 +
>  arch/riscv/kernel/pci.c    | 173 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 174 insertions(+)
>  create mode 100644 arch/riscv/kernel/pci.c

> diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c
> new file mode 100644
> index 000000000000..3388af3a67a0
> --- /dev/null
> +++ b/arch/riscv/kernel/pci.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Code borrowed from ARM64
> + *
> + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
> + * Copyright (C) 2014 ARM Ltd.
> + * Copyright (C) 2022-2023 Ventana Micro System Inc.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +#ifdef CONFIG_ACPI

Quickly checking against ARM64, they do not wrap the read/write
functions in this ifdef, so why do we need to do so?

> +/*
> + * raw_pci_read/write - Platform-specific PCI config space access.
> + */
> +int raw_pci_read(unsigned int domain, unsigned int bus,
> +		  unsigned int devfn, int reg, int len, u32 *val)
> +{
> +	struct pci_bus *b = pci_find_bus(domain, bus);
> +
> +	if (!b)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	return b->ops->read(b, devfn, reg, len, val);

A newline before the return would be appreciated by my eyes :)

> +}
> +
> +int raw_pci_write(unsigned int domain, unsigned int bus,
> +		unsigned int devfn, int reg, int len, u32 val)

Also, both read and write functions here appear to have incorrect
alignment on the second lines.

> +{
> +	struct pci_bus *b = pci_find_bus(domain, bus);
> +
> +	if (!b)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	return b->ops->write(b, devfn, reg, len, val);
> +}
> +
> +

Extra newline here too, looks to be exactly where you deleted the numa
stuff from arm64 ;)

> +struct acpi_pci_generic_root_info {
> +	struct acpi_pci_root_info	common;
> +	struct pci_config_window	*cfg;	/* config space mapping */
> +};
> +
> +int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct acpi_device *adev = to_acpi_device(cfg->parent);
> +	struct acpi_pci_root *root = acpi_driver_data(adev);
> +
> +	return root->segment;
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)

Rhetorical question perhaps, but what does "ci" mean?

> +{
> +	struct resource_entry *entry, *tmp;
> +	int status;
> +
> +	status = acpi_pci_probe_root_resources(ci);
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		if (!(entry->res->flags & IORESOURCE_WINDOW))
> +			resource_list_destroy_entry(entry);
> +	}
> +	return status;

Perhaps that extra newline from above could migrate down to the line
above the return here.

> +}
> +
> +/*
> + * Lookup the bus range for the domain in MCFG, and set up config space
> + * mapping.
> + */
> +static struct pci_config_window *
> +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)

This all fits on 1 line.

> +{
> +	struct device *dev = &root->device->dev;
> +	struct resource *bus_res = &root->secondary;
> +	u16 seg = root->segment;
> +	const struct pci_ecam_ops *ecam_ops;
> +	struct resource cfgres;
> +	struct acpi_device *adev;
> +	struct pci_config_window *cfg;
> +	int ret;
> +
> +	ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
> +	if (ret) {
> +		dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
> +		return NULL;
> +	}
> +
> +	adev = acpi_resource_consumer(&cfgres);
> +	if (adev)
> +		dev_info(dev, "ECAM area %pR reserved by %s\n", &cfgres,
> +			 dev_name(&adev->dev));
> +	else
> +		dev_warn(dev, FW_BUG "ECAM area %pR not reserved in ACPI namespace\n",
> +			 &cfgres);
> +
> +	cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops);
> +	if (IS_ERR(cfg)) {
> +		dev_err(dev, "%04x:%pR error %ld mapping ECAM\n", seg, bus_res,
> +			PTR_ERR(cfg));
> +		return NULL;
> +	}
> +
> +	return cfg;
> +}
> +
> +/* release_info: free resources allocated by init_info */

The fact that you haven't picked a consistent comment style for this
functions really bothers my OCD. Yes, it may be copy-paste from arm64,
but since this is "new code" I don't think there's harm in at least
*starting* with something that looks cohesive.

> +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_generic_root_info *ri;
> +
> +	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> +	pci_ecam_free(ri->cfg);
> +	kfree(ci->ops);
> +	kfree(ri);
> +}
> +
> +

Extra newline here.

> +/* Interface called from ACPI code to setup PCI host controller */
> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> +{
> +	struct acpi_pci_generic_root_info *ri;
> +	struct pci_bus *bus, *child;
> +	struct acpi_pci_root_ops *root_ops;
> +	struct pci_host_bridge *host;
> +
> +	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
> +	if (!ri)
> +		return NULL;
> +
> +	root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
> +	if (!root_ops) {
> +		kfree(ri);
> +		return NULL;
> +	}
> +
> +	ri->cfg = pci_acpi_setup_ecam_mapping(root);
> +	if (!ri->cfg) {
> +		kfree(ri);
> +		kfree(root_ops);
> +		return NULL;
> +	}
> +
> +	root_ops->release_info = pci_acpi_generic_release_info;
> +	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> +	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
> +	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> +	if (!bus)
> +		return NULL;
> +
> +	/* If we must preserve the resource configuration, claim now */
> +	host = pci_find_host_bridge(bus);
> +	if (host->preserve_config)
> +		pci_bus_claim_resources(bus);
> +
> +	/*
> +	 * Assign whatever was left unassigned. If we didn't claim above,
> +	 * this will reassign everything.
> +	 */
> +	pci_assign_unassigned_root_bus_resources(bus);
> +
> +	list_for_each_entry(child, &bus->children, node)
> +		pcie_bus_configure_settings(child);
> +
> +	return bus;
> +}

Anyways, this does look to be "leveraged from arm64" as you say and I
only had minor nits to comment about...
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 06/24] RISC-V: ACPI: Add PCI functions to build ACPI core

[Sunil V L]

On Wed, Feb 08, 2023 at 09:26:57PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:07PM +0530, Sunil V L wrote:
> > When CONFIG_PCI is enabled, ACPI core expects few arch
> > functions related to PCI. Add those functions so that
> > ACPI core gets build. These are levraged from arm64.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/kernel/Makefile |   1 +
> >  arch/riscv/kernel/pci.c    | 173 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 174 insertions(+)
> >  create mode 100644 arch/riscv/kernel/pci.c
> 
> > diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c
> > new file mode 100644
> > index 000000000000..3388af3a67a0
> > --- /dev/null
> > +++ b/arch/riscv/kernel/pci.c
> > @@ -0,0 +1,173 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Code borrowed from ARM64
> > + *
> > + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
> > + * Copyright (C) 2014 ARM Ltd.
> > + * Copyright (C) 2022-2023 Ventana Micro System Inc.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mm.h>
> > +#include <linux/pci.h>
> > +#include <linux/pci-acpi.h>
> > +#include <linux/pci-ecam.h>
> > +
> > +#ifdef CONFIG_ACPI
> 
> Quickly checking against ARM64, they do not wrap the read/write
> functions in this ifdef, so why do we need to do so?
> 
I didn't find any callers other than ACPI. But let me keep them outside so
that we are consistent.

> > +/*
> > + * raw_pci_read/write - Platform-specific PCI config space access.
> > + */
> > +int raw_pci_read(unsigned int domain, unsigned int bus,
> > +		  unsigned int devfn, int reg, int len, u32 *val)
> > +{
> > +	struct pci_bus *b = pci_find_bus(domain, bus);
> > +
> > +	if (!b)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +	return b->ops->read(b, devfn, reg, len, val);
> 
> A newline before the return would be appreciated by my eyes :)
> 
Okay.

> > +}
> > +
> > +int raw_pci_write(unsigned int domain, unsigned int bus,
> > +		unsigned int devfn, int reg, int len, u32 val)
> 
> Also, both read and write functions here appear to have incorrect
> alignment on the second lines.
> 
Okay.

> > +{
> > +	struct pci_bus *b = pci_find_bus(domain, bus);
> > +
> > +	if (!b)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +	return b->ops->write(b, devfn, reg, len, val);
> > +}
> > +
> > +
> 
> Extra newline here too, looks to be exactly where you deleted the numa
> stuff from arm64 ;)
> 
Okay.

> > +struct acpi_pci_generic_root_info {
> > +	struct acpi_pci_root_info	common;
> > +	struct pci_config_window	*cfg;	/* config space mapping */
> > +};
> > +
> > +int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
> > +{
> > +	struct pci_config_window *cfg = bus->sysdata;
> > +	struct acpi_device *adev = to_acpi_device(cfg->parent);
> > +	struct acpi_pci_root *root = acpi_driver_data(adev);
> > +
> > +	return root->segment;
> > +}
> > +
> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> 
> Rhetorical question perhaps, but what does "ci" mean?
>
I don't know either :-). I guess since there is one more generic
ri, this is named as ci.

> > +{
> > +	struct resource_entry *entry, *tmp;
> > +	int status;
> > +
> > +	status = acpi_pci_probe_root_resources(ci);
> > +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> > +		if (!(entry->res->flags & IORESOURCE_WINDOW))
> > +			resource_list_destroy_entry(entry);
> > +	}
> > +	return status;
> 
> Perhaps that extra newline from above could migrate down to the line
> above the return here.
>
Okay.

> > +}
> > +
> > +/*
> > + * Lookup the bus range for the domain in MCFG, and set up config space
> > + * mapping.
> > + */
> > +static struct pci_config_window *
> > +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> 
> This all fits on 1 line.
> 
It actually goes beyond 80 characters, right?

> > +{
> > +	struct device *dev = &root->device->dev;
> > +	struct resource *bus_res = &root->secondary;
> > +	u16 seg = root->segment;
> > +	const struct pci_ecam_ops *ecam_ops;
> > +	struct resource cfgres;
> > +	struct acpi_device *adev;
> > +	struct pci_config_window *cfg;
> > +	int ret;
> > +
> > +	ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
> > +	if (ret) {
> > +		dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
> > +		return NULL;
> > +	}
> > +
> > +	adev = acpi_resource_consumer(&cfgres);
> > +	if (adev)
> > +		dev_info(dev, "ECAM area %pR reserved by %s\n", &cfgres,
> > +			 dev_name(&adev->dev));
> > +	else
> > +		dev_warn(dev, FW_BUG "ECAM area %pR not reserved in ACPI namespace\n",
> > +			 &cfgres);
> > +
> > +	cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops);
> > +	if (IS_ERR(cfg)) {
> > +		dev_err(dev, "%04x:%pR error %ld mapping ECAM\n", seg, bus_res,
> > +			PTR_ERR(cfg));
> > +		return NULL;
> > +	}
> > +
> > +	return cfg;
> > +}
> > +
> > +/* release_info: free resources allocated by init_info */
> 
> The fact that you haven't picked a consistent comment style for this
> functions really bothers my OCD. Yes, it may be copy-paste from arm64,
> but since this is "new code" I don't think there's harm in at least
> *starting* with something that looks cohesive.
> 
Agree. Will try to fix them in the next revision.

> > +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> > +{
> > +	struct acpi_pci_generic_root_info *ri;
> > +
> > +	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> > +	pci_ecam_free(ri->cfg);
> > +	kfree(ci->ops);
> > +	kfree(ri);
> > +}
> > +
> > +
> 
> Extra newline here.
>
Okay.
 
> > +/* Interface called from ACPI code to setup PCI host controller */
> > +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> > +{
> > +	struct acpi_pci_generic_root_info *ri;
> > +	struct pci_bus *bus, *child;
> > +	struct acpi_pci_root_ops *root_ops;
> > +	struct pci_host_bridge *host;
> > +
> > +	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
> > +	if (!ri)
> > +		return NULL;
> > +
> > +	root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
> > +	if (!root_ops) {
> > +		kfree(ri);
> > +		return NULL;
> > +	}
> > +
> > +	ri->cfg = pci_acpi_setup_ecam_mapping(root);
> > +	if (!ri->cfg) {
> > +		kfree(ri);
> > +		kfree(root_ops);
> > +		return NULL;
> > +	}
> > +
> > +	root_ops->release_info = pci_acpi_generic_release_info;
> > +	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> > +	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
> > +	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> > +	if (!bus)
> > +		return NULL;
> > +
> > +	/* If we must preserve the resource configuration, claim now */
> > +	host = pci_find_host_bridge(bus);
> > +	if (host->preserve_config)
> > +		pci_bus_claim_resources(bus);
> > +
> > +	/*
> > +	 * Assign whatever was left unassigned. If we didn't claim above,
> > +	 * this will reassign everything.
> > +	 */
> > +	pci_assign_unassigned_root_bus_resources(bus);
> > +
> > +	list_for_each_entry(child, &bus->children, node)
> > +		pcie_bus_configure_settings(child);
> > +
> > +	return bus;
> > +}
> 
> Anyways, this does look to be "leveraged from arm64" as you say and I
> only had minor nits to comment about...
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
Thanks!
Sunil

Re: [PATCH 06/24] RISC-V: ACPI: Add PCI functions to build ACPI core

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Mon, Feb 13, 2023 at 08:53:01PM +0530, Sunil V L wrote:
> On Wed, Feb 08, 2023 at 09:26:57PM +0000, Conor Dooley wrote:
> > On Mon, Jan 30, 2023 at 11:52:07PM +0530, Sunil V L wrote:

> > > +/*
> > > + * Lookup the bus range for the domain in MCFG, and set up config space
> > > + * mapping.
> > > + */
> > > +static struct pci_config_window *
> > > +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> > 
> > This all fits on 1 line.
> > 
> It actually goes beyond 80 characters, right?

100 chars is the limit :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 06/24] RISC-V: ACPI: Add PCI functions to build ACPI core

[Jessica Clarke]

On 30 Jan 2023, at 18:22, Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> When CONFIG_PCI is enabled, ACPI core expects few arch
> functions related to PCI. Add those functions so that
> ACPI core gets build. These are levraged from arm64.

Presumably this is pretty generic and applies to anything without x86
weirdness. Copying all this supposedly architecture specific code
that’s really generic seems like bad practice to me as an outsider.
Should this not be unifying the two in a shared location as has been
done for other subsystems?

Jess

> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
> arch/riscv/kernel/Makefile |   1 +
> arch/riscv/kernel/pci.c    | 173 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 174 insertions(+)
> create mode 100644 arch/riscv/kernel/pci.c
> 
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index f979dc8cf47d..e9d37639751d 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -92,3 +92,4 @@ obj-$(CONFIG_COMPAT)		+= compat_signal.o
> obj-$(CONFIG_COMPAT)		+= compat_vdso/
> 
> obj-$(CONFIG_ACPI)              += acpi.o
> +obj-$(CONFIG_PCI)               += pci.o
> diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c
> new file mode 100644
> index 000000000000..3388af3a67a0
> --- /dev/null
> +++ b/arch/riscv/kernel/pci.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Code borrowed from ARM64
> + *
> + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
> + * Copyright (C) 2014 ARM Ltd.
> + * Copyright (C) 2022-2023 Ventana Micro System Inc.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +#ifdef CONFIG_ACPI
> +
> +/*
> + * raw_pci_read/write - Platform-specific PCI config space access.
> + */
> +int raw_pci_read(unsigned int domain, unsigned int bus,
> +		  unsigned int devfn, int reg, int len, u32 *val)
> +{
> +	struct pci_bus *b = pci_find_bus(domain, bus);
> +
> +	if (!b)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	return b->ops->read(b, devfn, reg, len, val);
> +}
> +
> +int raw_pci_write(unsigned int domain, unsigned int bus,
> +		unsigned int devfn, int reg, int len, u32 val)
> +{
> +	struct pci_bus *b = pci_find_bus(domain, bus);
> +
> +	if (!b)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	return b->ops->write(b, devfn, reg, len, val);
> +}
> +
> +
> +struct acpi_pci_generic_root_info {
> +	struct acpi_pci_root_info	common;
> +	struct pci_config_window	*cfg;	/* config space mapping */
> +};
> +
> +int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct acpi_device *adev = to_acpi_device(cfg->parent);
> +	struct acpi_pci_root *root = acpi_driver_data(adev);
> +
> +	return root->segment;
> +}
> +
> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> +{
> +	struct resource_entry *entry, *tmp;
> +	int status;
> +
> +	status = acpi_pci_probe_root_resources(ci);
> +	resource_list_for_each_entry_safe(entry, tmp, &ci->resources) {
> +		if (!(entry->res->flags & IORESOURCE_WINDOW))
> +			resource_list_destroy_entry(entry);
> +	}
> +	return status;
> +}
> +
> +/*
> + * Lookup the bus range for the domain in MCFG, and set up config space
> + * mapping.
> + */
> +static struct pci_config_window *
> +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> +{
> +	struct device *dev = &root->device->dev;
> +	struct resource *bus_res = &root->secondary;
> +	u16 seg = root->segment;
> +	const struct pci_ecam_ops *ecam_ops;
> +	struct resource cfgres;
> +	struct acpi_device *adev;
> +	struct pci_config_window *cfg;
> +	int ret;
> +
> +	ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops);
> +	if (ret) {
> +		dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res);
> +		return NULL;
> +	}
> +
> +	adev = acpi_resource_consumer(&cfgres);
> +	if (adev)
> +		dev_info(dev, "ECAM area %pR reserved by %s\n", &cfgres,
> +			 dev_name(&adev->dev));
> +	else
> +		dev_warn(dev, FW_BUG "ECAM area %pR not reserved in ACPI namespace\n",
> +			 &cfgres);
> +
> +	cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops);
> +	if (IS_ERR(cfg)) {
> +		dev_err(dev, "%04x:%pR error %ld mapping ECAM\n", seg, bus_res,
> +			PTR_ERR(cfg));
> +		return NULL;
> +	}
> +
> +	return cfg;
> +}
> +
> +/* release_info: free resources allocated by init_info */
> +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> +{
> +	struct acpi_pci_generic_root_info *ri;
> +
> +	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> +	pci_ecam_free(ri->cfg);
> +	kfree(ci->ops);
> +	kfree(ri);
> +}
> +
> +
> +/* Interface called from ACPI code to setup PCI host controller */
> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> +{
> +	struct acpi_pci_generic_root_info *ri;
> +	struct pci_bus *bus, *child;
> +	struct acpi_pci_root_ops *root_ops;
> +	struct pci_host_bridge *host;
> +
> +	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
> +	if (!ri)
> +		return NULL;
> +
> +	root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
> +	if (!root_ops) {
> +		kfree(ri);
> +		return NULL;
> +	}
> +
> +	ri->cfg = pci_acpi_setup_ecam_mapping(root);
> +	if (!ri->cfg) {
> +		kfree(ri);
> +		kfree(root_ops);
> +		return NULL;
> +	}
> +
> +	root_ops->release_info = pci_acpi_generic_release_info;
> +	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> +	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
> +	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> +	if (!bus)
> +		return NULL;
> +
> +	/* If we must preserve the resource configuration, claim now */
> +	host = pci_find_host_bridge(bus);
> +	if (host->preserve_config)
> +		pci_bus_claim_resources(bus);
> +
> +	/*
> +	 * Assign whatever was left unassigned. If we didn't claim above,
> +	 * this will reassign everything.
> +	 */
> +	pci_assign_unassigned_root_bus_resources(bus);
> +
> +	list_for_each_entry(child, &bus->children, node)
> +		pcie_bus_configure_settings(child);
> +
> +	return bus;
> +}
> +
> +#endif
> -- 
> 2.38.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 06/24] RISC-V: ACPI: Add PCI functions to build ACPI core

[Sunil V L]

On Mon, Feb 13, 2023 at 05:26:13PM +0000, Jessica Clarke wrote:
> On 30 Jan 2023, at 18:22, Sunil V L <sunilvl@ventanamicro.com> wrote:
> > 
> > When CONFIG_PCI is enabled, ACPI core expects few arch
> > functions related to PCI. Add those functions so that
> > ACPI core gets build. These are levraged from arm64.
> 
> Presumably this is pretty generic and applies to anything without x86
> weirdness. Copying all this supposedly architecture specific code
> that’s really generic seems like bad practice to me as an outsider.
> Should this not be unifying the two in a shared location as has been
> done for other subsystems?

Make sense. Let me add these functions in a common location pci-acpi.c
for RISC-V. Other architectures can migrate to this in future.

Thanks,
Sunil

[PATCH 07/24] RISC-V: ACPI: Enable ACPI build infrastructure

[Sunil V L]

Enable build infrastructure to add ACPI support for
RISC-V.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/Kconfig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d153e1cd890b..f664350679bc 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -12,6 +12,9 @@ config 32BIT
 
 config RISCV
 	def_bool y
+	select ACPI_GENERIC_GSI if ACPI
+	select ACPI_MCFG if (ACPI && PCI)
+	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ARCH_CLOCKSOURCE_INIT
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
 	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
@@ -598,6 +601,7 @@ config EFI_STUB
 config EFI
 	bool "UEFI runtime support"
 	depends on OF && !XIP_KERNEL
+	select ARCH_SUPPORTS_ACPI if 64BIT
 	select LIBFDT
 	select UCS2_STRING
 	select EFI_PARAMS_FROM_FDT
@@ -703,3 +707,4 @@ source "drivers/cpufreq/Kconfig"
 endmenu # "CPU Power Management"
 
 source "arch/riscv/kvm/Kconfig"
+source "drivers/acpi/Kconfig"
-- 
2.38.0

Re: [PATCH 07/24] RISC-V: ACPI: Enable ACPI build infrastructure

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Mon, Jan 30, 2023 at 11:52:08PM +0530, Sunil V L wrote:
> Enable build infrastructure to add ACPI support for
> RISC-V.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/Kconfig | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d153e1cd890b..f664350679bc 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -12,6 +12,9 @@ config 32BIT
>  
>  config RISCV
>  	def_bool y
> +	select ACPI_GENERIC_GSI if ACPI
> +	select ACPI_MCFG if (ACPI && PCI)

These brackets are not needed, right?

> +	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>  	select ARCH_CLOCKSOURCE_INIT
>  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
>  	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
> @@ -598,6 +601,7 @@ config EFI_STUB
>  config EFI
>  	bool "UEFI runtime support"
>  	depends on OF && !XIP_KERNEL
> +	select ARCH_SUPPORTS_ACPI if 64BIT
>  	select LIBFDT
>  	select UCS2_STRING
>  	select EFI_PARAMS_FROM_FDT
> @@ -703,3 +707,4 @@ source "drivers/cpufreq/Kconfig"
>  endmenu # "CPU Power Management"
>  
>  source "arch/riscv/kvm/Kconfig"
> +source "drivers/acpi/Kconfig"

For consistency with the rest of the file, a newline before drivers
would be in order here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 07/24] RISC-V: ACPI: Enable ACPI build infrastructure

[Sunil V L]

On Wed, Feb 08, 2023 at 09:31:44PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:08PM +0530, Sunil V L wrote:
> > Enable build infrastructure to add ACPI support for
> > RISC-V.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/Kconfig | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index d153e1cd890b..f664350679bc 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -12,6 +12,9 @@ config 32BIT
> >  
> >  config RISCV
> >  	def_bool y
> > +	select ACPI_GENERIC_GSI if ACPI
> > +	select ACPI_MCFG if (ACPI && PCI)
> 
> These brackets are not needed, right?
> 
Okay.

> > +	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> >  	select ARCH_CLOCKSOURCE_INIT
> >  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
> >  	select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
> > @@ -598,6 +601,7 @@ config EFI_STUB
> >  config EFI
> >  	bool "UEFI runtime support"
> >  	depends on OF && !XIP_KERNEL
> > +	select ARCH_SUPPORTS_ACPI if 64BIT
> >  	select LIBFDT
> >  	select UCS2_STRING
> >  	select EFI_PARAMS_FROM_FDT
> > @@ -703,3 +707,4 @@ source "drivers/cpufreq/Kconfig"
> >  endmenu # "CPU Power Management"
> >  
> >  source "arch/riscv/kvm/Kconfig"
> > +source "drivers/acpi/Kconfig"
> 
> For consistency with the rest of the file, a newline before drivers
> would be in order here.
>
Sure.

Thanks
Sunil 

[PATCH 08/24] ACPI: Enable ACPI_PROCESSOR for RISC-V

[Sunil V L]

The ACPI processor driver is not currently enabled for RISC-V.
Enable it

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 ccbeab9500ec..b44ac8e55b54 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -281,7 +281,7 @@ config ACPI_CPPC_LIB
 
 config ACPI_PROCESSOR
 	tristate "Processor"
-	depends on X86 || IA64 || ARM64 || LOONGARCH
+	depends on X86 || IA64 || ARM64 || LOONGARCH || RISCV
 	select ACPI_PROCESSOR_IDLE
 	select ACPI_CPU_FREQ_PSS if X86 || IA64 || LOONGARCH
 	select THERMAL
-- 
2.38.0

[PATCH 09/24] ACPI: OSL: Make should_use_kmap() 0 for RISC-V.

[Sunil V L]

Without this, if the tables are larger than 4K,
acpi_map() will fail.

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

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3269a888fb7a..f725813d0cce 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -276,7 +276,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 	return NULL;
 }
 
-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
+#if defined(CONFIG_IA64) || defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
 /* ioremap will take care of cache attributes */
 #define should_use_kmap(pfn)   0
 #else
-- 
2.38.0

[PATCH 10/24] ACPI: processor_core: RISC-V: Enable mapping processor to the hartid

[Sunil V L]

processor_core needs arch specific functions to map the ACPI ID
to the physical ID. In RISC-V platforms, hartid is the physical id
and RINTC structure in MADT provides this mapping. Add arch specific
function to get this mapping from RINTC.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h |  3 +++
 drivers/acpi/processor_core.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 8b9babaf3f25..c5cb9f96d404 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -15,6 +15,9 @@
 /* Basic configuration for ACPI */
 #ifdef CONFIG_ACPI
 
+typedef u64 phys_cpuid_t;
+#define PHYS_CPUID_INVALID INVALID_HARTID
+
 /* ACPI table mapping after acpi_permanent_mmap is set */
 void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
 #define acpi_os_ioremap acpi_os_ioremap
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 2ac48cda5b20..889f495b3481 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -106,6 +106,31 @@ static int map_gicc_mpidr(struct acpi_subtable_header *entry,
 	return -EINVAL;
 }
 
+/*
+ * Retrieve the RISC-V hartid for the processor
+ */
+static int map_rintc_hartid(struct acpi_subtable_header *entry,
+		int device_declaration, u32 acpi_id, phys_cpuid_t *hartid)
+{
+	struct acpi_madt_rintc *rintc =
+	    container_of(entry, struct acpi_madt_rintc, header);
+
+	if (!(rintc->flags & ACPI_MADT_ENABLED))
+		return -ENODEV;
+
+	/* device_declaration means Device object in DSDT, in the
+	 * RISC-V, logical processors are required to
+	 * have a Processor Device object in the DSDT, so we should
+	 * check device_declaration here
+	 */
+	if (device_declaration && (rintc->uid == acpi_id)) {
+		*hartid = rintc->hart_id;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static phys_cpuid_t map_madt_entry(struct acpi_table_madt *madt,
 				   int type, u32 acpi_id)
 {
@@ -136,6 +161,9 @@ static phys_cpuid_t map_madt_entry(struct acpi_table_madt *madt,
 		} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
 			if (!map_gicc_mpidr(header, type, acpi_id, &phys_id))
 				break;
+		} else if (header->type == ACPI_MADT_TYPE_RINTC) {
+			if (!map_rintc_hartid(header, type, acpi_id, &phys_id))
+				break;
 		}
 		entry += header->length;
 	}
-- 
2.38.0

[PATCH 11/24] RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support

[Sunil V L]

Add support for initializing the RISC-V INTC driver on ACPI based
platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/irqchip/irq-riscv-intc.c | 79 +++++++++++++++++++++++++++-----
 1 file changed, 67 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index f229e3e66387..044ec92fcba7 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -6,6 +6,7 @@
  */
 
 #define pr_fmt(fmt) "riscv-intc: " fmt
+#include <linux/acpi.h>
 #include <linux/atomic.h>
 #include <linux/bits.h>
 #include <linux/cpu.h>
@@ -112,6 +113,30 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
 	return intc_domain->fwnode;
 }
 
+static int __init riscv_intc_init_common(struct fwnode_handle *fn)
+{
+	int rc;
+
+	intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
+					       &riscv_intc_domain_ops, NULL);
+	if (!intc_domain) {
+		pr_err("unable to add IRQ domain\n");
+		return -ENXIO;
+	}
+
+	rc = set_handle_irq(&riscv_intc_irq);
+	if (rc) {
+		pr_err("failed to set irq handler\n");
+		return rc;
+	}
+
+	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
+
+	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
+
+	return 0;
+}
+
 static int __init riscv_intc_init(struct device_node *node,
 				  struct device_node *parent)
 {
@@ -133,24 +158,54 @@ static int __init riscv_intc_init(struct device_node *node,
 	if (riscv_hartid_to_cpuid(hartid) != smp_processor_id())
 		return 0;
 
-	intc_domain = irq_domain_add_linear(node, BITS_PER_LONG,
-					    &riscv_intc_domain_ops, NULL);
-	if (!intc_domain) {
-		pr_err("unable to add IRQ domain\n");
-		return -ENXIO;
-	}
-
-	rc = set_handle_irq(&riscv_intc_irq);
+	rc = riscv_intc_init_common(of_node_to_fwnode(node));
 	if (rc) {
-		pr_err("failed to set irq handler\n");
+		pr_err("failed to initialize INTC\n");
 		return rc;
 	}
 
-	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
+	return 0;
+}
 
-	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
+IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
+
+#ifdef CONFIG_ACPI
+
+static int __init
+riscv_intc_acpi_init(union acpi_subtable_headers *header,
+		     const unsigned long end)
+{
+	int rc;
+	struct fwnode_handle *fn;
+	struct acpi_madt_rintc *rintc;
+
+	rintc = (struct acpi_madt_rintc *)header;
+
+	/*
+	 * The ACPI MADT will have one INTC for each CPU (or HART)
+	 * so riscv_intc_acpi_init() function will be called once
+	 * for each INTC. We only need to do INTC initialization
+	 * for the INTC belonging to the boot CPU (or boot HART).
+	 */
+	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
+		return 0;
+
+	fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
+	WARN_ON(fn == NULL);
+	if (!fn) {
+		pr_err("unable to allocate INTC FW node\n");
+		return -1;
+	}
+
+	rc = riscv_intc_init_common(fn);
+	if (rc) {
+		pr_err("failed to initialize INTC\n");
+		return rc;
+	}
 
 	return 0;
 }
 
-IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
+IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC,
+		     NULL, 1, riscv_intc_acpi_init);
+#endif
-- 
2.38.0

Re: [PATCH 11/24] RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support

[Jessica Clarke]

On 30 Jan 2023, at 18:22, Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> Add support for initializing the RISC-V INTC driver on ACPI based
> platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
> drivers/irqchip/irq-riscv-intc.c | 79 +++++++++++++++++++++++++++-----
> 1 file changed, 67 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index f229e3e66387..044ec92fcba7 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -6,6 +6,7 @@
>  */
> 
> #define pr_fmt(fmt) "riscv-intc: " fmt
> +#include <linux/acpi.h>
> #include <linux/atomic.h>
> #include <linux/bits.h>
> #include <linux/cpu.h>
> @@ -112,6 +113,30 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
> 	return intc_domain->fwnode;
> }
> 
> +static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> +{
> +	int rc;
> +
> +	intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> +					       &riscv_intc_domain_ops, NULL);
> +	if (!intc_domain) {
> +		pr_err("unable to add IRQ domain\n");
> +		return -ENXIO;
> +	}
> +
> +	rc = set_handle_irq(&riscv_intc_irq);
> +	if (rc) {
> +		pr_err("failed to set irq handler\n");
> +		return rc;
> +	}
> +
> +	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> +
> +	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> +
> +	return 0;
> +}
> +
> static int __init riscv_intc_init(struct device_node *node,
> 				  struct device_node *parent)
> {
> @@ -133,24 +158,54 @@ static int __init riscv_intc_init(struct device_node *node,
> 	if (riscv_hartid_to_cpuid(hartid) != smp_processor_id())
> 		return 0;
> 
> -	intc_domain = irq_domain_add_linear(node, BITS_PER_LONG,
> -					    &riscv_intc_domain_ops, NULL);
> -	if (!intc_domain) {
> -		pr_err("unable to add IRQ domain\n");
> -		return -ENXIO;
> -	}
> -
> -	rc = set_handle_irq(&riscv_intc_irq);
> +	rc = riscv_intc_init_common(of_node_to_fwnode(node));
> 	if (rc) {
> -		pr_err("failed to set irq handler\n");
> +		pr_err("failed to initialize INTC\n");
> 		return rc;
> 	}
> 
> -	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> +	return 0;
> +}
> 
> -	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> +
> +#ifdef CONFIG_ACPI
> +
> +static int __init
> +riscv_intc_acpi_init(union acpi_subtable_headers *header,
> +		     const unsigned long end)
> +{
> +	int rc;
> +	struct fwnode_handle *fn;
> +	struct acpi_madt_rintc *rintc;
> +
> +	rintc = (struct acpi_madt_rintc *)header;
> +
> +	/*
> +	 * The ACPI MADT will have one INTC for each CPU (or HART)
> +	 * so riscv_intc_acpi_init() function will be called once
> +	 * for each INTC. We only need to do INTC initialization
> +	 * for the INTC belonging to the boot CPU (or boot HART).
> +	 */
> +	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
> +		return 0;

Why are we carrying forward this mess to ACPI? The DT bindings are
awful and a complete pain to deal with, as evidenced by how both Linux
and FreeBSD have to go out of their way to do special things to only
look at one of the many copies of the same thing.

Jess

> +
> +	fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
> +	WARN_ON(fn == NULL);
> +	if (!fn) {
> +		pr_err("unable to allocate INTC FW node\n");
> +		return -1;
> +	}
> +
> +	rc = riscv_intc_init_common(fn);
> +	if (rc) {
> +		pr_err("failed to initialize INTC\n");
> +		return rc;
> +	}
> 
> 	return 0;
> }
> 
> -IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> +IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC,
> +		     NULL, 1, riscv_intc_acpi_init);
> +#endif
> -- 
> 2.38.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 11/24] RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support

[Sunil V L]

Hi Jessica,

On Mon, Jan 30, 2023 at 11:38:49PM +0000, Jessica Clarke wrote:
> On 30 Jan 2023, at 18:22, Sunil V L <sunilvl@ventanamicro.com> wrote:
> > 
> > Add support for initializing the RISC-V INTC driver on ACPI based
> > platforms.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> > drivers/irqchip/irq-riscv-intc.c | 79 +++++++++++++++++++++++++++-----
> > 1 file changed, 67 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > index f229e3e66387..044ec92fcba7 100644
> > --- a/drivers/irqchip/irq-riscv-intc.c
> > +++ b/drivers/irqchip/irq-riscv-intc.c
> > @@ -6,6 +6,7 @@
> >  */
> > 
> > #define pr_fmt(fmt) "riscv-intc: " fmt
> > +#include <linux/acpi.h>
> > #include <linux/atomic.h>
> > #include <linux/bits.h>
> > #include <linux/cpu.h>
> > @@ -112,6 +113,30 @@ static struct fwnode_handle *riscv_intc_hwnode(void)
> > 	return intc_domain->fwnode;
> > }
> > 
> > +static int __init riscv_intc_init_common(struct fwnode_handle *fn)
> > +{
> > +	int rc;
> > +
> > +	intc_domain = irq_domain_create_linear(fn, BITS_PER_LONG,
> > +					       &riscv_intc_domain_ops, NULL);
> > +	if (!intc_domain) {
> > +		pr_err("unable to add IRQ domain\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	rc = set_handle_irq(&riscv_intc_irq);
> > +	if (rc) {
> > +		pr_err("failed to set irq handler\n");
> > +		return rc;
> > +	}
> > +
> > +	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> > +
> > +	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> > +
> > +	return 0;
> > +}
> > +
> > static int __init riscv_intc_init(struct device_node *node,
> > 				  struct device_node *parent)
> > {
> > @@ -133,24 +158,54 @@ static int __init riscv_intc_init(struct device_node *node,
> > 	if (riscv_hartid_to_cpuid(hartid) != smp_processor_id())
> > 		return 0;
> > 
> > -	intc_domain = irq_domain_add_linear(node, BITS_PER_LONG,
> > -					    &riscv_intc_domain_ops, NULL);
> > -	if (!intc_domain) {
> > -		pr_err("unable to add IRQ domain\n");
> > -		return -ENXIO;
> > -	}
> > -
> > -	rc = set_handle_irq(&riscv_intc_irq);
> > +	rc = riscv_intc_init_common(of_node_to_fwnode(node));
> > 	if (rc) {
> > -		pr_err("failed to set irq handler\n");
> > +		pr_err("failed to initialize INTC\n");
> > 		return rc;
> > 	}
> > 
> > -	riscv_set_intc_hwnode_fn(riscv_intc_hwnode);
> > +	return 0;
> > +}
> > 
> > -	pr_info("%d local interrupts mapped\n", BITS_PER_LONG);
> > +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> > +
> > +#ifdef CONFIG_ACPI
> > +
> > +static int __init
> > +riscv_intc_acpi_init(union acpi_subtable_headers *header,
> > +		     const unsigned long end)
> > +{
> > +	int rc;
> > +	struct fwnode_handle *fn;
> > +	struct acpi_madt_rintc *rintc;
> > +
> > +	rintc = (struct acpi_madt_rintc *)header;
> > +
> > +	/*
> > +	 * The ACPI MADT will have one INTC for each CPU (or HART)
> > +	 * so riscv_intc_acpi_init() function will be called once
> > +	 * for each INTC. We only need to do INTC initialization
> > +	 * for the INTC belonging to the boot CPU (or boot HART).
> > +	 */
> > +	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
> > +		return 0;
> 
> Why are we carrying forward this mess to ACPI? The DT bindings are
> awful and a complete pain to deal with, as evidenced by how both Linux
> and FreeBSD have to go out of their way to do special things to only
> look at one of the many copies of the same thing.
>

Local interrupt controller structures are per-cpu in any architecture.
So, there will be multiple such structures. It is upto the OS to choose
one of them. What is the issue here?

The RISC-V DT code is selecting the one which is corresponding to the boot
cpu. While in ACPI we can choose any one, I think it is better to
follow the DT code to keep it similar and boot cpu is always guaranteed
to be available.

Thanks!
Sunil

Re: [PATCH 11/24] RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

Hey Sunil,

On Mon, Jan 30, 2023 at 11:52:12PM +0530, Sunil V L wrote:
> Add support for initializing the RISC-V INTC driver on ACPI based
> platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

> +static int __init
> +riscv_intc_acpi_init(union acpi_subtable_headers *header,
> +		     const unsigned long end)
> +{
> +	int rc;
> +	struct fwnode_handle *fn;
> +	struct acpi_madt_rintc *rintc;
> +
> +	rintc = (struct acpi_madt_rintc *)header;
> +
> +	/*
> +	 * The ACPI MADT will have one INTC for each CPU (or HART)
> +	 * so riscv_intc_acpi_init() function will be called once
> +	 * for each INTC. We only need to do INTC initialization
> +	 * for the INTC belonging to the boot CPU (or boot HART).
> +	 */
> +	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
> +		return 0;
> +
> +	fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
> +	WARN_ON(fn == NULL);

Is there a reason that you do not just check this as !fn?

> +	if (!fn) {

This is a repeated check from the WARN_ON(), no?

> +		pr_err("unable to allocate INTC FW node\n");

Why do you need a WARN_ON() & the pr_err() here?

> +		return -1;

Why not an actual ERRNO?

Cheers,
Conor.

> +	}
> +
> +	rc = riscv_intc_init_common(fn);
> +	if (rc) {
> +		pr_err("failed to initialize INTC\n");
> +		return rc;
> +	}
>  
>  	return 0;
>  }
>  
> -IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> +IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC,
> +		     NULL, 1, riscv_intc_acpi_init);
> +#endif
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 11/24] RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support

[Sunil V L]

On Wed, Feb 08, 2023 at 09:49:40PM +0000, Conor Dooley wrote:
> Hey Sunil,
> 
> On Mon, Jan 30, 2023 at 11:52:12PM +0530, Sunil V L wrote:
> > Add support for initializing the RISC-V INTC driver on ACPI based
> > platforms.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> 
> > +static int __init
> > +riscv_intc_acpi_init(union acpi_subtable_headers *header,
> > +		     const unsigned long end)
> > +{
> > +	int rc;
> > +	struct fwnode_handle *fn;
> > +	struct acpi_madt_rintc *rintc;
> > +
> > +	rintc = (struct acpi_madt_rintc *)header;
> > +
> > +	/*
> > +	 * The ACPI MADT will have one INTC for each CPU (or HART)
> > +	 * so riscv_intc_acpi_init() function will be called once
> > +	 * for each INTC. We only need to do INTC initialization
> > +	 * for the INTC belonging to the boot CPU (or boot HART).
> > +	 */
> > +	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
> > +		return 0;
> > +
> > +	fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
> > +	WARN_ON(fn == NULL);
> 
> Is there a reason that you do not just check this as !fn?
> 
> > +	if (!fn) {
> 
> This is a repeated check from the WARN_ON(), no?
> 
> > +		pr_err("unable to allocate INTC FW node\n");
> 
> Why do you need a WARN_ON() & the pr_err() here?
> 
You are right. Will remove the WARN_ON.

> > +		return -1;
> 
> Why not an actual ERRNO?
>
Okay. 

Thanks,
Sunil

[PATCH 12/24] RISC-V: ACPI: smpboot: Create wrapper smp_setup()

[Sunil V L]

smp_setup() currently assumes DT based platforms. To enable ACPI,
first make this as a wrapper function and move existing code to
a separate DT specific function.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/smpboot.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 00b53913d4c6..26214ddefaa4 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -70,7 +70,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
-void __init setup_smp(void)
+static void __init of_parse_and_init_cpus(void)
 {
 	struct device_node *dn;
 	unsigned long hart;
@@ -116,6 +116,11 @@ void __init setup_smp(void)
 	}
 }
 
+void __init setup_smp(void)
+{
+	of_parse_and_init_cpus();
+}
+
 static int start_secondary_cpu(int cpu, struct task_struct *tidle)
 {
 	if (cpu_ops[cpu]->cpu_start)
-- 
2.38.0

Re: [PATCH 12/24] RISC-V: ACPI: smpboot: Create wrapper smp_setup()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On Mon, Jan 30, 2023 at 11:52:13PM +0530, Sunil V L wrote:
> Subject: RISC-V: ACPI: smpboot: Create wrapper smp_setup()

As I pointed out the other day, this one of the patches that really
doesn't warrant an ACPI: prefix.

With that fixed:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> smp_setup() currently assumes DT based platforms. To enable ACPI,
> first make this as a wrapper function and move existing code to
> a separate DT specific function.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/kernel/smpboot.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 00b53913d4c6..26214ddefaa4 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -70,7 +70,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  	}
>  }
>  
> -void __init setup_smp(void)
> +static void __init of_parse_and_init_cpus(void)
>  {
>  	struct device_node *dn;
>  	unsigned long hart;
> @@ -116,6 +116,11 @@ void __init setup_smp(void)
>  	}
>  }
>  
> +void __init setup_smp(void)
> +{
> +	of_parse_and_init_cpus();
> +}
> +
>  static int start_secondary_cpu(int cpu, struct task_struct *tidle)
>  {
>  	if (cpu_ops[cpu]->cpu_start)
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 13/24] RISC-V: ACPI: smpboot: Add ACPI support in smp_setup()

[Sunil V L]

Add function to parse the RINTC structure in
the MADT table and create the required initializations to
enable SMP boot on ACPI based platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h |  7 ++++
 arch/riscv/kernel/smpboot.c   | 73 ++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index c5cb9f96d404..d1f1e53ec657 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -58,6 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void)
 }
 
 static inline void arch_fix_phys_package_id(int num, u32 slot) { }
+
+#ifdef CONFIG_ACPI_NUMA
+int acpi_numa_get_nid(unsigned int cpu);
+#else
+static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
+#endif /* CONFIG_ACPI_NUMA */
+
 #endif
 
 #endif /*_ASM_ACPI_H*/
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 26214ddefaa4..e48cf88d0bc1 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/acpi.h>
 #include <linux/arch_topology.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -70,6 +71,73 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 }
 
+#ifdef CONFIG_ACPI
+static unsigned int cpu_count = 1;
+
+static int __init
+acpi_parse_rintc(union acpi_subtable_headers *header,
+			     const unsigned long end)
+{
+	unsigned long hart;
+	bool found_boot_cpu = false;
+
+	struct acpi_madt_rintc *processor;
+
+	processor = (struct acpi_madt_rintc *)header;
+
+	/* RINTC entry which has !ACPI_MADT_ENABLED is not enabled so skip */
+	if (!(processor->flags & ACPI_MADT_ENABLED))
+		return 0;
+
+	hart = processor->hart_id;
+	if (hart < 0)
+		return 0;
+	if (hart == cpuid_to_hartid_map(0)) {
+		BUG_ON(found_boot_cpu);
+		found_boot_cpu = 1;
+		early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
+		return 0;
+	}
+	if (cpu_count >= NR_CPUS) {
+		pr_warn("Invalid cpuid [%d] for hartid [%lu]\n",
+			cpu_count, hart);
+		return 0;
+	}
+
+	cpuid_to_hartid_map(cpu_count) = hart;
+	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
+	cpu_count++;
+	return 0;
+}
+
+static void __init acpi_parse_and_init_cpus(void)
+{
+	int cpuid;
+
+	cpu_set_ops(0);
+	/*
+	 * do a walk of MADT to determine how many CPUs
+	 * we have including disabled CPUs, and get information
+	 * we need for SMP init.
+	 */
+	acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC,
+				      acpi_parse_rintc, 0);
+
+	/*
+	 * NUMA - TODO
+	 */
+
+	for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
+		if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
+			cpu_set_ops(cpuid);
+			set_cpu_possible(cpuid, true);
+		}
+	}
+}
+#else
+#define acpi_parse_and_init_cpus(...)	do { } while (0)
+#endif
+
 static void __init of_parse_and_init_cpus(void)
 {
 	struct device_node *dn;
@@ -118,7 +186,10 @@ static void __init of_parse_and_init_cpus(void)
 
 void __init setup_smp(void)
 {
-	of_parse_and_init_cpus();
+	if (acpi_disabled)
+		of_parse_and_init_cpus();
+	else
+		acpi_parse_and_init_cpus();
 }
 
 static int start_secondary_cpu(int cpu, struct task_struct *tidle)
-- 
2.38.0

Re: [PATCH 13/24] RISC-V: ACPI: smpboot: Add ACPI support in smp_setup()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4761 bytes --]

On Mon, Jan 30, 2023 at 11:52:14PM +0530, Sunil V L wrote:
> Add function to parse the RINTC structure in
> the MADT table and create the required initializations to
> enable SMP boot on ACPI based platforms.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/include/asm/acpi.h |  7 ++++
>  arch/riscv/kernel/smpboot.c   | 73 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index c5cb9f96d404..d1f1e53ec657 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -58,6 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void)
>  }
>  
>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> +
> +#ifdef CONFIG_ACPI_NUMA
> +int acpi_numa_get_nid(unsigned int cpu);
> +#else
> +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> +#endif /* CONFIG_ACPI_NUMA */
> +
>  #endif
>  
>  #endif /*_ASM_ACPI_H*/
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 26214ddefaa4..e48cf88d0bc1 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2017 SiFive
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/arch_topology.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -70,6 +71,73 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  	}
>  }
>  
> +#ifdef CONFIG_ACPI
> +static unsigned int cpu_count = 1;
> +
> +static int __init
> +acpi_parse_rintc(union acpi_subtable_headers *header,
> +			     const unsigned long end)

This all fits on one line. And also avoids the checkpatch complaint from
what you have currently done...

> +{
> +	unsigned long hart;
> +	bool found_boot_cpu = false;
> +
> +	struct acpi_madt_rintc *processor;
> +
> +	processor = (struct acpi_madt_rintc *)header;

Why not combine the above two lines?

> +	/* RINTC entry which has !ACPI_MADT_ENABLED is not enabled so skip */

This comment is a bit -ENOPARSE. Please reword it in a way that is
understandable to mere mortals like myself.

> +	if (!(processor->flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	hart = processor->hart_id;
> +	if (hart < 0)
> +		return 0;

Newline here please

> +	if (hart == cpuid_to_hartid_map(0)) {
> +		BUG_ON(found_boot_cpu);
> +		found_boot_cpu = 1;

This is a bool, why not assign a bool value to it so it looks more
intentional? I know this is copied from the dt code, but that should
really be on too IMO.

> +		early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
> +		return 0;
> +	}

And a newline here too...

> +	if (cpu_count >= NR_CPUS) {
> +		pr_warn("Invalid cpuid [%d] for hartid [%lu]\n",
> +			cpu_count, hart);
> +		return 0;
> +	}
> +
> +	cpuid_to_hartid_map(cpu_count) = hart;
> +	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
> +	cpu_count++;

...and also here please!

> +	return 0;
> +}
> +
> +static void __init acpi_parse_and_init_cpus(void)
> +{
> +	int cpuid;
> +
> +	cpu_set_ops(0);

While I'm at it suggesting newline additions, adding them before
comments would be great too.

> +	/*
> +	 * do a walk of MADT to determine how many CPUs
> +	 * we have including disabled CPUs, and get information
> +	 * we need for SMP init.
> +	 */
> +	acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC,
> +				      acpi_parse_rintc, 0);
> +
> +	/*
> +	 * NUMA - TODO
> +	 */

TODO before merging, or TODO at some indeterminate point in the future?

Anyways, this is all nits & this largely seem to resemble the dt code,
so with the nits fixed (and an s/ACPI: // in $subject):
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> +	for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
> +		if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
> +			cpu_set_ops(cpuid);
> +			set_cpu_possible(cpuid, true);
> +		}
> +	}
> +}
> +#else
> +#define acpi_parse_and_init_cpus(...)	do { } while (0)
> +#endif
> +
>  static void __init of_parse_and_init_cpus(void)
>  {
>  	struct device_node *dn;
> @@ -118,7 +186,10 @@ static void __init of_parse_and_init_cpus(void)
>  
>  void __init setup_smp(void)
>  {
> -	of_parse_and_init_cpus();
> +	if (acpi_disabled)
> +		of_parse_and_init_cpus();
> +	else
> +		acpi_parse_and_init_cpus();
>  }
>  
>  static int start_secondary_cpu(int cpu, struct task_struct *tidle)
> -- 
> 2.38.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 13/24] RISC-V: ACPI: smpboot: Add ACPI support in smp_setup()

[Sunil V L]

On Wed, Feb 08, 2023 at 10:10:14PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:14PM +0530, Sunil V L wrote:
> > Add function to parse the RINTC structure in
> > the MADT table and create the required initializations to
> > enable SMP boot on ACPI based platforms.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/acpi.h |  7 ++++
> >  arch/riscv/kernel/smpboot.c   | 73 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index c5cb9f96d404..d1f1e53ec657 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -58,6 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void)
> >  }
> >  
> >  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> > +
> > +#ifdef CONFIG_ACPI_NUMA
> > +int acpi_numa_get_nid(unsigned int cpu);
> > +#else
> > +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > +#endif /* CONFIG_ACPI_NUMA */
> > +
> >  #endif
> >  
> >  #endif /*_ASM_ACPI_H*/
> > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> > index 26214ddefaa4..e48cf88d0bc1 100644
> > --- a/arch/riscv/kernel/smpboot.c
> > +++ b/arch/riscv/kernel/smpboot.c
> > @@ -8,6 +8,7 @@
> >   * Copyright (C) 2017 SiFive
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/arch_topology.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> > @@ -70,6 +71,73 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> >  	}
> >  }
> >  
> > +#ifdef CONFIG_ACPI
> > +static unsigned int cpu_count = 1;
> > +
> > +static int __init
> > +acpi_parse_rintc(union acpi_subtable_headers *header,
> > +			     const unsigned long end)
> 
> This all fits on one line. And also avoids the checkpatch complaint from
> what you have currently done...
> 
Okay.

> > +{
> > +	unsigned long hart;
> > +	bool found_boot_cpu = false;
> > +
> > +	struct acpi_madt_rintc *processor;
> > +
> > +	processor = (struct acpi_madt_rintc *)header;
> 
> Why not combine the above two lines?
> 
> > +	/* RINTC entry which has !ACPI_MADT_ENABLED is not enabled so skip */
> 
> This comment is a bit -ENOPARSE. Please reword it in a way that is
> understandable to mere mortals like myself.
>
Okay, let me try :-).
 
> > +	if (!(processor->flags & ACPI_MADT_ENABLED))
> > +		return 0;
> > +
> > +	hart = processor->hart_id;
> > +	if (hart < 0)
> > +		return 0;
> 
> Newline here please
> 
> > +	if (hart == cpuid_to_hartid_map(0)) {
> > +		BUG_ON(found_boot_cpu);
> > +		found_boot_cpu = 1;
> 
> This is a bool, why not assign a bool value to it so it looks more
> intentional? I know this is copied from the dt code, but that should
> really be on too IMO.
>
Okay.
 
> > +		early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
> > +		return 0;
> > +	}
> 
> And a newline here too...
>
Okay. 
> > +	if (cpu_count >= NR_CPUS) {
> > +		pr_warn("Invalid cpuid [%d] for hartid [%lu]\n",
> > +			cpu_count, hart);
> > +		return 0;
> > +	}
> > +
> > +	cpuid_to_hartid_map(cpu_count) = hart;
> > +	early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
> > +	cpu_count++;
> 
> ...and also here please!
>
Okay.

> > +	return 0;
> > +}
> > +
> > +static void __init acpi_parse_and_init_cpus(void)
> > +{
> > +	int cpuid;
> > +
> > +	cpu_set_ops(0);
> 
> While I'm at it suggesting newline additions, adding them before
> comments would be great too.
>
Okay.
 
> > +	/*
> > +	 * do a walk of MADT to determine how many CPUs
> > +	 * we have including disabled CPUs, and get information
> > +	 * we need for SMP init.
> > +	 */
> > +	acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC,
> > +				      acpi_parse_rintc, 0);
> > +
> > +	/*
> > +	 * NUMA - TODO
> > +	 */
> 
> TODO before merging, or TODO at some indeterminate point in the future?
>
Will remove this comment. We need to add NUMA after basic support is
done.
 
> Anyways, this is all nits & this largely seem to resemble the dt code,
> so with the nits fixed (and an s/ACPI: // in $subject):
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
Thanks!
Sunil

[PATCH 14/24] RISC-V: ACPI: smpboot: Add function to retrieve the hartid

[Sunil V L]

hartid is in RINTC structuire in MADT table. Instead of parsing
the ACPI table every time we need for a cpu, cache it and provide
a function to read it.

This is similar to acpi_get_madt_gicc() in arm64.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h | 14 +++++++++++++-
 arch/riscv/kernel/smpboot.c   | 21 +++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index d1f1e53ec657..69a880b7257a 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -65,6 +65,18 @@ int acpi_numa_get_nid(unsigned int cpu);
 static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
 #endif /* CONFIG_ACPI_NUMA */
 
-#endif
+struct acpi_madt_rintc *acpi_get_madt_rintc(int cpu);
+struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
+static inline u32 get_acpi_id_for_cpu(int cpu)
+{
+	return	acpi_cpu_get_madt_rintc(cpu)->uid;
+}
+#else
+static inline u32 get_acpi_id_for_cpu(int cpu)
+{
+	return -1;
+}
+
+#endif /* CONFIG_ACPI */
 
 #endif /*_ASM_ACPI_H*/
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index e48cf88d0bc1..3a8b7a9eb5ac 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -73,6 +73,25 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 
 #ifdef CONFIG_ACPI
 static unsigned int cpu_count = 1;
+static unsigned int intc_count;
+static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
+
+struct acpi_madt_rintc *acpi_get_madt_rintc(int cpu)
+{
+	return &cpu_madt_rintc[cpu];
+}
+
+struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
+{
+	int i;
+
+	for (i = 0; i < NR_CPUS; i++) {
+		if (riscv_hartid_to_cpuid(cpu_madt_rintc[i].hart_id) == cpu)
+			return &cpu_madt_rintc[i];
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(acpi_cpu_get_madt_rintc);
 
 static int __init
 acpi_parse_rintc(union acpi_subtable_headers *header,
@@ -92,6 +111,8 @@ acpi_parse_rintc(union acpi_subtable_headers *header,
 	hart = processor->hart_id;
 	if (hart < 0)
 		return 0;
+
+	cpu_madt_rintc[intc_count++] = *processor;
 	if (hart == cpuid_to_hartid_map(0)) {
 		BUG_ON(found_boot_cpu);
 		found_boot_cpu = 1;
-- 
2.38.0

Re: [PATCH 14/24] RISC-V: ACPI: smpboot: Add function to retrieve the hartid

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3189 bytes --]

Hey Sunil, Drew,

@drew, a question below that I'm sorta aiming at you...

On Mon, Jan 30, 2023 at 11:52:15PM +0530, Sunil V L wrote:
> hartid is in RINTC structuire in MADT table. Instead of parsing

Nit: missing articles before RINTC and MADT. Also typo "structure".

Perhaps you'd benefit from a spell checker in your git editor.

> the ACPI table every time we need for a cpu, cache it and provide
> a function to read it.
> 
> This is similar to acpi_get_madt_gicc() in arm64.

-ENOTFOUND, do you mean acpi_cpu_get_madt_gicc()?

> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/include/asm/acpi.h | 14 +++++++++++++-
>  arch/riscv/kernel/smpboot.c   | 21 +++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index d1f1e53ec657..69a880b7257a 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -65,6 +65,18 @@ int acpi_numa_get_nid(unsigned int cpu);
>  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
>  #endif /* CONFIG_ACPI_NUMA */
>  
> -#endif
> +struct acpi_madt_rintc *acpi_get_madt_rintc(int cpu);
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> +static inline u32 get_acpi_id_for_cpu(int cpu)
> +{
> +	return	acpi_cpu_get_madt_rintc(cpu)->uid;
> +}
> +#else
> +static inline u32 get_acpi_id_for_cpu(int cpu)
> +{
> +	return -1;
> +}
> +
> +#endif /* CONFIG_ACPI */
>  
>  #endif /*_ASM_ACPI_H*/
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index e48cf88d0bc1..3a8b7a9eb5ac 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -73,6 +73,25 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  
>  #ifdef CONFIG_ACPI
>  static unsigned int cpu_count = 1;
> +static unsigned int intc_count;
> +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> +
> +struct acpi_madt_rintc *acpi_get_madt_rintc(int cpu)
> +{
> +	return &cpu_madt_rintc[cpu];
> +}
> +
> +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> +{
> +	int i;

Since we are C11 now, you don't even need to declare this outside of the
loop, right?

> +
> +	for (i = 0; i < NR_CPUS; i++) {

@drew, perhaps you know since you were fiddling not too long ago with
cpumask stuff - at what point does for_each_possible_cpu() become
usable?
I had a bit of a poke & couldn't immediately tell if it'd be okay to use
it here.

> +		if (riscv_hartid_to_cpuid(cpu_madt_rintc[i].hart_id) == cpu)
> +			return &cpu_madt_rintc[i];
> +	}
> +	return NULL;

Another nit: newline before return please :)

> +}
> +EXPORT_SYMBOL_GPL(acpi_cpu_get_madt_rintc);
>  
>  static int __init
>  acpi_parse_rintc(union acpi_subtable_headers *header,
> @@ -92,6 +111,8 @@ acpi_parse_rintc(union acpi_subtable_headers *header,
>  	hart = processor->hart_id;
>  	if (hart < 0)
>  		return 0;
> +
> +	cpu_madt_rintc[intc_count++] = *processor;
>  	if (hart == cpuid_to_hartid_map(0)) {
>  		BUG_ON(found_boot_cpu);
>  		found_boot_cpu = 1;
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 14/24] RISC-V: ACPI: smpboot: Add function to retrieve the hartid

[Sunil V L]

On Thu, Feb 09, 2023 at 08:30:40PM +0000, Conor Dooley wrote:
> Hey Sunil, Drew,
> 
> @drew, a question below that I'm sorta aiming at you...
> 
> On Mon, Jan 30, 2023 at 11:52:15PM +0530, Sunil V L wrote:
> > hartid is in RINTC structuire in MADT table. Instead of parsing
> 
> Nit: missing articles before RINTC and MADT. Also typo "structure".
> 
> Perhaps you'd benefit from a spell checker in your git editor.
> 
Okay.

> > the ACPI table every time we need for a cpu, cache it and provide
> > a function to read it.
> > 
> > This is similar to acpi_get_madt_gicc() in arm64.
> 
> -ENOTFOUND, do you mean acpi_cpu_get_madt_gicc()?
>
Yes. Will update.

> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/acpi.h | 14 +++++++++++++-
> >  arch/riscv/kernel/smpboot.c   | 21 +++++++++++++++++++++
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index d1f1e53ec657..69a880b7257a 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -65,6 +65,18 @@ int acpi_numa_get_nid(unsigned int cpu);
> >  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> >  #endif /* CONFIG_ACPI_NUMA */
> >  
> > -#endif
> > +struct acpi_madt_rintc *acpi_get_madt_rintc(int cpu);
> > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
> > +static inline u32 get_acpi_id_for_cpu(int cpu)
> > +{
> > +	return	acpi_cpu_get_madt_rintc(cpu)->uid;
> > +}
> > +#else
> > +static inline u32 get_acpi_id_for_cpu(int cpu)
> > +{
> > +	return -1;
> > +}
> > +
> > +#endif /* CONFIG_ACPI */
> >  
> >  #endif /*_ASM_ACPI_H*/
> > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> > index e48cf88d0bc1..3a8b7a9eb5ac 100644
> > --- a/arch/riscv/kernel/smpboot.c
> > +++ b/arch/riscv/kernel/smpboot.c
> > @@ -73,6 +73,25 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> >  
> >  #ifdef CONFIG_ACPI
> >  static unsigned int cpu_count = 1;
> > +static unsigned int intc_count;
> > +static struct acpi_madt_rintc cpu_madt_rintc[NR_CPUS];
> > +
> > +struct acpi_madt_rintc *acpi_get_madt_rintc(int cpu)
> > +{
> > +	return &cpu_madt_rintc[cpu];
> > +}
> > +
> > +struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > +{
> > +	int i;
> 
> Since we are C11 now, you don't even need to declare this outside of the
> loop, right?
>
Okay.
 
> > +
> > +	for (i = 0; i < NR_CPUS; i++) {
> 
> @drew, perhaps you know since you were fiddling not too long ago with
> cpumask stuff - at what point does for_each_possible_cpu() become
> usable?
> I had a bit of a poke & couldn't immediately tell if it'd be okay to use
> it here.
>
It should be possible. Thanks!
 
> > +		if (riscv_hartid_to_cpuid(cpu_madt_rintc[i].hart_id) == cpu)
> > +			return &cpu_madt_rintc[i];
> > +	}
> > +	return NULL;
> 
> Another nit: newline before return please :)
>
Sure.

Thanks,
Sunil 

[PATCH 15/24] clocksource/timer-riscv: Refactor riscv_timer_init_dt()

[Sunil V L]

Refactor the timer init function such that few things can be shared by
both DT and ACPI based platforms.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/clocksource/timer-riscv.c | 79 +++++++++++++++----------------
 1 file changed, 37 insertions(+), 42 deletions(-)

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 1b4b36df5484..4016c065a01c 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -119,61 +119,28 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int __init riscv_timer_init_dt(struct device_node *n)
+static int __init riscv_timer_init_common(void)
 {
-	int cpuid, error;
-	unsigned long hartid;
-	struct device_node *child;
-	struct irq_domain *domain;
-
-	error = riscv_of_processor_hartid(n, &hartid);
-	if (error < 0) {
-		pr_warn("Not valid hartid for node [%pOF] error = [%lu]\n",
-			n, hartid);
-		return error;
-	}
-
-	cpuid = riscv_hartid_to_cpuid(hartid);
-	if (cpuid < 0) {
-		pr_warn("Invalid cpuid for hartid [%lu]\n", hartid);
-		return cpuid;
-	}
-
-	if (cpuid != smp_processor_id())
-		return 0;
+	int error;
+	struct fwnode_handle *intc_fwnode = riscv_get_intc_hwnode();
+	struct irq_domain *domain = NULL;
 
-	child = of_find_compatible_node(NULL, NULL, "riscv,timer");
-	if (child) {
-		riscv_timer_cannot_wake_cpu = of_property_read_bool(child,
-					"riscv,timer-cannot-wake-cpu");
-		of_node_put(child);
-	}
 
-	domain = NULL;
-	child = of_get_compatible_child(n, "riscv,cpu-intc");
-	if (!child) {
-		pr_err("Failed to find INTC node [%pOF]\n", n);
-		return -ENODEV;
-	}
-	domain = irq_find_host(child);
-	of_node_put(child);
+	domain = irq_find_matching_fwnode(intc_fwnode, DOMAIN_BUS_ANY);
 	if (!domain) {
-		pr_err("Failed to find IRQ domain for node [%pOF]\n", n);
+		pr_err("Failed to find INTC node [%pfwP]\n", intc_fwnode);
 		return -ENODEV;
 	}
 
 	riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER);
 	if (!riscv_clock_event_irq) {
-		pr_err("Failed to map timer interrupt for node [%pOF]\n", n);
-		return -ENODEV;
+		pr_err("Failed to map timer interrupt for node [%pfwP]\n",
+			intc_fwnode);
 	}
 
-	pr_info("%s: Registering clocksource cpuid [%d] hartid [%lu]\n",
-	       __func__, cpuid, hartid);
 	error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
 	if (error) {
-		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
-		       error, cpuid);
+		pr_err("clocksource register failed [%d]\n", error);
 		return error;
 	}
 
@@ -199,7 +166,35 @@ static int __init riscv_timer_init_dt(struct device_node *n)
 		static_branch_enable(&riscv_sstc_available);
 	}
 
+	pr_info("timer registered using %s\n",
+		(static_branch_likely(&riscv_sstc_available)) ?
+		"RISC-V Sstc" : "RISC-V SBI");
+
 	return error;
 }
 
+static int __init riscv_timer_init_dt(struct device_node *n)
+{
+	int cpuid, error;
+	unsigned long hartid;
+
+	error = riscv_of_processor_hartid(n, &hartid);
+	if (error < 0) {
+		pr_warn("Not valid hartid for node [%pOF] error = [%lu]\n",
+			n, hartid);
+		return error;
+	}
+
+	cpuid = riscv_hartid_to_cpuid(hartid);
+	if (cpuid < 0) {
+		pr_warn("Invalid cpuid for hartid [%lu]\n", hartid);
+		return cpuid;
+	}
+
+	if (cpuid != smp_processor_id())
+		return 0;
+
+	return riscv_timer_init_common();
+}
+
 TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
-- 
2.38.0

Re: [PATCH 15/24] clocksource/timer-riscv: Refactor riscv_timer_init_dt()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

Hey Sunil,

On Mon, Jan 30, 2023 at 11:52:16PM +0530, Sunil V L wrote:
> Refactor the timer init function such that few things can be shared by
> both DT and ACPI based platforms.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

What did Anup do here? He's not author or co-author, so the SoB chain
looks incorrect.

> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  drivers/clocksource/timer-riscv.c | 79 +++++++++++++++----------------
>  1 file changed, 37 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 1b4b36df5484..4016c065a01c 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -119,61 +119,28 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int __init riscv_timer_init_dt(struct device_node *n)
> +static int __init riscv_timer_init_common(void)
>  {
> -	int cpuid, error;
> -	unsigned long hartid;
> -	struct device_node *child;
> -	struct irq_domain *domain;
> -
> -	error = riscv_of_processor_hartid(n, &hartid);
> -	if (error < 0) {
> -		pr_warn("Not valid hartid for node [%pOF] error = [%lu]\n",
> -			n, hartid);
> -		return error;
> -	}
> -
> -	cpuid = riscv_hartid_to_cpuid(hartid);
> -	if (cpuid < 0) {
> -		pr_warn("Invalid cpuid for hartid [%lu]\n", hartid);
> -		return cpuid;
> -	}
> -
> -	if (cpuid != smp_processor_id())
> -		return 0;
> +	int error;
> +	struct fwnode_handle *intc_fwnode = riscv_get_intc_hwnode();
> +	struct irq_domain *domain = NULL;
>  
> -	child = of_find_compatible_node(NULL, NULL, "riscv,timer");
> -	if (child) {
> -		riscv_timer_cannot_wake_cpu = of_property_read_bool(child,
> -					"riscv,timer-cannot-wake-cpu");
> -		of_node_put(child);
> -	}

Uhh, where did this code go?
Unless I've badly missed something, this has vanished in the patch.

>  
> -	domain = NULL;
> -	child = of_get_compatible_child(n, "riscv,cpu-intc");
> -	if (!child) {
> -		pr_err("Failed to find INTC node [%pOF]\n", n);
> -		return -ENODEV;
> -	}
> -	domain = irq_find_host(child);
> -	of_node_put(child);
> +	domain = irq_find_matching_fwnode(intc_fwnode, DOMAIN_BUS_ANY);
>  	if (!domain) {
> -		pr_err("Failed to find IRQ domain for node [%pOF]\n", n);
> +		pr_err("Failed to find INTC node [%pfwP]\n", intc_fwnode);
>  		return -ENODEV;
>  	}
>  
>  	riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER);
>  	if (!riscv_clock_event_irq) {
> -		pr_err("Failed to map timer interrupt for node [%pOF]\n", n);
> -		return -ENODEV;
> +		pr_err("Failed to map timer interrupt for node [%pfwP]\n",
> +			intc_fwnode);
>  	}
>  
> -	pr_info("%s: Registering clocksource cpuid [%d] hartid [%lu]\n",
> -	       __func__, cpuid, hartid);
>  	error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
>  	if (error) {
> -		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> -		       error, cpuid);
> +		pr_err("clocksource register failed [%d]\n", error);

If you're changing this, s/register/registration/ to be grammatically
correct I suppose.

>  		return error;
>  	}
>  
> @@ -199,7 +166,35 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>  		static_branch_enable(&riscv_sstc_available);
>  	}
>  
> +	pr_info("timer registered using %s\n",
> +		(static_branch_likely(&riscv_sstc_available)) ?
> +		"RISC-V Sstc" : "RISC-V SBI");

Why is this needed? Isn't there a print like 3 lines above here that
says "Timer interrupt in S-mode is available via sstc extension"?

> +
>  	return error;
>  }
>  
> +static int __init riscv_timer_init_dt(struct device_node *n)
> +{
> +	int cpuid, error;
> +	unsigned long hartid;
> +
> +	error = riscv_of_processor_hartid(n, &hartid);
> +	if (error < 0) {
> +		pr_warn("Not valid hartid for node [%pOF] error = [%lu]\n",

While you're already moving this, may as well fix the grammar IMO.
s/Not valid/Invalid/

Cheers,
Conor.

> +			n, hartid);
> +		return error;
> +	}
> +
> +	cpuid = riscv_hartid_to_cpuid(hartid);
> +	if (cpuid < 0) {
> +		pr_warn("Invalid cpuid for hartid [%lu]\n", hartid);
> +		return cpuid;
> +	}
> +
> +	if (cpuid != smp_processor_id())
> +		return 0;
> +
> +	return riscv_timer_init_common();
> +}
> +
>  TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 15/24] clocksource/timer-riscv: Refactor riscv_timer_init_dt()

[Sunil V L]

Hi Conor,

On Thu, Feb 09, 2023 at 08:54:11PM +0000, Conor Dooley wrote:
> Hey Sunil,
> 
> On Mon, Jan 30, 2023 at 11:52:16PM +0530, Sunil V L wrote:
> > Refactor the timer init function such that few things can be shared by
> > both DT and ACPI based platforms.
> > 
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> 
> What did Anup do here? He's not author or co-author, so the SoB chain
> looks incorrect.
> 
When I wanted to refactor, I realized Anup had done similar in his branch for
a different purpose. So, I took some of his changes and I added SOB. Let me
add him in co-developed-by:

> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  drivers/clocksource/timer-riscv.c | 79 +++++++++++++++----------------
> >  1 file changed, 37 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > index 1b4b36df5484..4016c065a01c 100644
> > --- a/drivers/clocksource/timer-riscv.c
> > +++ b/drivers/clocksource/timer-riscv.c
> > @@ -119,61 +119,28 @@ static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -static int __init riscv_timer_init_dt(struct device_node *n)
> > +static int __init riscv_timer_init_common(void)
> >  {
> > -	int cpuid, error;
> > -	unsigned long hartid;
> > -	struct device_node *child;
> > -	struct irq_domain *domain;
> > -
> > -	error = riscv_of_processor_hartid(n, &hartid);
> > -	if (error < 0) {
> > -		pr_warn("Not valid hartid for node [%pOF] error = [%lu]\n",
> > -			n, hartid);
> > -		return error;
> > -	}
> > -
> > -	cpuid = riscv_hartid_to_cpuid(hartid);
> > -	if (cpuid < 0) {
> > -		pr_warn("Invalid cpuid for hartid [%lu]\n", hartid);
> > -		return cpuid;
> > -	}
> > -
> > -	if (cpuid != smp_processor_id())
> > -		return 0;
> > +	int error;
> > +	struct fwnode_handle *intc_fwnode = riscv_get_intc_hwnode();
> > +	struct irq_domain *domain = NULL;
> >  
> > -	child = of_find_compatible_node(NULL, NULL, "riscv,timer");
> > -	if (child) {
> > -		riscv_timer_cannot_wake_cpu = of_property_read_bool(child,
> > -					"riscv,timer-cannot-wake-cpu");
> > -		of_node_put(child);
> > -	}
> 
> Uhh, where did this code go?
> Unless I've badly missed something, this has vanished in the patch.
> 
Oops! Anup's patch had moved this to a separate DT timer node instead of
cpu node which I didn't realize. Thanks for catching this. Will fix it.

> >  
> > -	domain = NULL;
> > -	child = of_get_compatible_child(n, "riscv,cpu-intc");
> > -	if (!child) {
> > -		pr_err("Failed to find INTC node [%pOF]\n", n);
> > -		return -ENODEV;
> > -	}
> > -	domain = irq_find_host(child);
> > -	of_node_put(child);
> > +	domain = irq_find_matching_fwnode(intc_fwnode, DOMAIN_BUS_ANY);
> >  	if (!domain) {
> > -		pr_err("Failed to find IRQ domain for node [%pOF]\n", n);
> > +		pr_err("Failed to find INTC node [%pfwP]\n", intc_fwnode);
> >  		return -ENODEV;
> >  	}
> >  
> >  	riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER);
> >  	if (!riscv_clock_event_irq) {
> > -		pr_err("Failed to map timer interrupt for node [%pOF]\n", n);
> > -		return -ENODEV;
> > +		pr_err("Failed to map timer interrupt for node [%pfwP]\n",
> > +			intc_fwnode);
> >  	}
> >  
> > -	pr_info("%s: Registering clocksource cpuid [%d] hartid [%lu]\n",
> > -	       __func__, cpuid, hartid);
> >  	error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
> >  	if (error) {
> > -		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> > -		       error, cpuid);
> > +		pr_err("clocksource register failed [%d]\n", error);
> 
> If you're changing this, s/register/registration/ to be grammatically
> correct I suppose.
> 
Sure.
> >  		return error;
> >  	}
> >  
> > @@ -199,7 +166,35 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> >  		static_branch_enable(&riscv_sstc_available);
> >  	}
> >  
> > +	pr_info("timer registered using %s\n",
> > +		(static_branch_likely(&riscv_sstc_available)) ?
> > +		"RISC-V Sstc" : "RISC-V SBI");
> 
> Why is this needed? Isn't there a print like 3 lines above here that
> says "Timer interrupt in S-mode is available via sstc extension"?
> 
Yes, will remove it.

> > +
> >  	return error;
> >  }
> >  
> > +static int __init riscv_timer_init_dt(struct device_node *n)
> > +{
> > +	int cpuid, error;
> > +	unsigned long hartid;
> > +
> > +	error = riscv_of_processor_hartid(n, &hartid);
> > +	if (error < 0) {
> > +		pr_warn("Not valid hartid for node [%pOF] error = [%lu]\n",
> 
> While you're already moving this, may as well fix the grammar IMO.
> s/Not valid/Invalid/
>
Okay

Thanks!
Sunil 

[PATCH 16/24] RISC-V: ACPI: clocksource/timer-riscv: Add ACPI support

[Sunil V L]

timer-riscv driver needs to get the timebase-frequency from
RISC-V Hart Capabilities Table (RHCT) on ACPI platforms. Add
support to read the information from RHCT.

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

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 4016c065a01c..8079666753a6 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -10,6 +10,7 @@
 
 #define pr_fmt(fmt) "riscv-timer: " fmt
 
+#include <linux/acpi.h>
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
 #include <linux/cpu.h>
@@ -198,3 +199,11 @@ static int __init riscv_timer_init_dt(struct device_node *n)
 }
 
 TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
+
+#ifdef CONFIG_ACPI
+static int __init riscv_timer_acpi_init(struct acpi_table_header *table)
+{
+	return riscv_timer_init_common();
+}
+TIMER_ACPI_DECLARE(aclint_mtimer, ACPI_SIG_RHCT, riscv_timer_acpi_init);
+#endif
-- 
2.38.0

Re: [PATCH 16/24] RISC-V: ACPI: clocksource/timer-riscv: Add ACPI support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Mon, Jan 30, 2023 at 11:52:17PM +0530, Sunil V L wrote:
> timer-riscv driver needs to get the timebase-frequency from
> RISC-V Hart Capabilities Table (RHCT) on ACPI platforms. Add
> support to read the information from RHCT.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  drivers/clocksource/timer-riscv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 4016c065a01c..8079666753a6 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -10,6 +10,7 @@
>  
>  #define pr_fmt(fmt) "riscv-timer: " fmt
>  
> +#include <linux/acpi.h>
>  #include <linux/clocksource.h>
>  #include <linux/clockchips.h>
>  #include <linux/cpu.h>
> @@ -198,3 +199,11 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>  }
>  
>  TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
> +
> +#ifdef CONFIG_ACPI
> +static int __init riscv_timer_acpi_init(struct acpi_table_header *table)
> +{
> +	return riscv_timer_init_common();

I feel like I need to ask as it was deleted in the previous patch, how
does ACPI determine whether the arch timer can wake the CPUs?

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 16/24] RISC-V: ACPI: clocksource/timer-riscv: Add ACPI support

[Sunil V L]

On Thu, Feb 09, 2023 at 08:58:25PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:17PM +0530, Sunil V L wrote:
> > timer-riscv driver needs to get the timebase-frequency from
> > RISC-V Hart Capabilities Table (RHCT) on ACPI platforms. Add
> > support to read the information from RHCT.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  drivers/clocksource/timer-riscv.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > index 4016c065a01c..8079666753a6 100644
> > --- a/drivers/clocksource/timer-riscv.c
> > +++ b/drivers/clocksource/timer-riscv.c
> > @@ -10,6 +10,7 @@
> >  
> >  #define pr_fmt(fmt) "riscv-timer: " fmt
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/clocksource.h>
> >  #include <linux/clockchips.h>
> >  #include <linux/cpu.h>
> > @@ -198,3 +199,11 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> >  }
> >  
> >  TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
> > +
> > +#ifdef CONFIG_ACPI
> > +static int __init riscv_timer_acpi_init(struct acpi_table_header *table)
> > +{
> > +	return riscv_timer_init_common();
> 
> I feel like I need to ask as it was deleted in the previous patch, how
> does ACPI determine whether the arch timer can wake the CPUs?
> 
We have plans to add a flag in RHCT. But that still needs approval.

Thanks
Sunil

[PATCH 17/24] ACPI: RISC-V: drivers/acpi: Add RHCT related code

[Sunil V L]

RHCT is a new table defined for RISC-V to communicate the
features of the CPU to the OS. Create a new architecture folder
in drivers/acpi and add RHCT parsing code.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h |  5 ++
 drivers/acpi/Makefile         |  2 +
 drivers/acpi/riscv/Makefile   |  2 +
 drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+)
 create mode 100644 drivers/acpi/riscv/Makefile
 create mode 100644 drivers/acpi/riscv/rhct.c

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 69a880b7257a..4fec9ea3b874 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -71,11 +71,16 @@ static inline u32 get_acpi_id_for_cpu(int cpu)
 {
 	return	acpi_cpu_get_madt_rintc(cpu)->uid;
 }
+int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const char **isa);
 #else
 static inline u32 get_acpi_id_for_cpu(int cpu)
 {
 	return -1;
 }
+int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const char **isa)
+{
+	return -1;
+}
 
 #endif /* CONFIG_ACPI */
 
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index feb36c0b9446..3fc5a0d54f6e 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -131,3 +131,5 @@ obj-y				+= dptf/
 obj-$(CONFIG_ARM64)		+= arm64/
 
 obj-$(CONFIG_ACPI_VIOT)		+= viot.o
+
+obj-$(CONFIG_RISCV)		+= riscv/
diff --git a/drivers/acpi/riscv/Makefile b/drivers/acpi/riscv/Makefile
new file mode 100644
index 000000000000..8b3b126e0b94
--- /dev/null
+++ b/drivers/acpi/riscv/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-y 	+= rhct.o
diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
new file mode 100644
index 000000000000..7c64be64d9d9
--- /dev/null
+++ b/drivers/acpi/riscv/rhct.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022-2023, Ventana Micro Systems Inc
+ *	Author: Sunil V L <sunilvl@ventanamicro.com>
+ *
+ */
+
+#define pr_fmt(fmt)	"ACPI: RHCT: " fmt
+
+#include <linux/acpi.h>
+
+static void acpi_rhct_warn_missing(void)
+{
+	pr_warn_once("No RHCT table found\n");
+}
+
+static struct acpi_table_header *acpi_get_rhct(void)
+{
+	static struct acpi_table_header *rhct;
+	acpi_status status;
+
+	/*
+	 * RHCT will be used at runtime on every CPU, so we
+	 * don't need to call acpi_put_table() to release the table mapping.
+	 */
+	if (!rhct) {
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
+		if (ACPI_FAILURE(status))
+			acpi_rhct_warn_missing();
+	}
+
+	return rhct;
+}
+
+/*
+ * During early boot, the caller should call acpi_get_table() and pass its pointer to
+ * these functions(and free up later). At run time, since this table can be used
+ * multiple times, pass NULL so that the table remains in memory
+ */
+int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int acpi_cpu_id, const char **isa)
+{
+	struct acpi_rhct_node_header *node, *ref_node, *end;
+	struct acpi_table_rhct *rhct;
+	struct acpi_rhct_hart_info *hart_info;
+	struct acpi_rhct_isa_string *isa_node;
+	u32 *hart_info_node_offset;
+	int i, j;
+
+	if (acpi_disabled) {
+		pr_debug("%s: acpi is disabled\n", __func__);
+		return -1;
+	}
+
+	if (!table) {
+		rhct = (struct acpi_table_rhct *)acpi_get_rhct();
+		if (!rhct)
+			return -ENOENT;
+	} else {
+		rhct = (struct acpi_table_rhct *)table;
+	}
+
+	node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
+	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
+
+	for (i = 0; i < rhct->node_count; i++) {
+		if (node >= end)
+			break;
+		switch (node->type) {
+		case ACPI_RHCT_NODE_TYPE_HART_INFO:
+			hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node,
+					sizeof(struct acpi_rhct_node_header));
+			hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info,
+					sizeof(struct acpi_rhct_hart_info));
+			if (acpi_cpu_id != hart_info->uid)
+				break;
+			for (j = 0; j < hart_info->num_offsets; j++) {
+				ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
+						rhct, hart_info_node_offset[j]);
+				if (ref_node->type == ACPI_RHCT_NODE_TYPE_ISA_STRING) {
+					isa_node = ACPI_ADD_PTR(struct acpi_rhct_isa_string,
+							ref_node,
+							sizeof(struct acpi_rhct_node_header));
+					*isa = isa_node->isa;
+					return 0;
+				}
+			}
+			break;
+		}
+		node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length);
+	}
+
+	return -1;
+}
-- 
2.38.0

[PATCH 18/24] RISC-V: ACPI: time.c: Add ACPI support for time_init()

[Sunil V L]

On ACPI based platforms, timer related information is
available in RHCT. Add ACPI based probe support to the
timer initialization.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/time.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 1cf21db4fcc7..e49b897fc657 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/acpi.h>
 #include <linux/of_clk.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
@@ -18,17 +19,29 @@ EXPORT_SYMBOL_GPL(riscv_timebase);
 void __init time_init(void)
 {
 	struct device_node *cpu;
+	struct acpi_table_rhct *rhct;
+	acpi_status status;
 	u32 prop;
 
-	cpu = of_find_node_by_path("/cpus");
-	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
-		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
-	of_node_put(cpu);
-	riscv_timebase = prop;
+	if (acpi_disabled) {
+		cpu = of_find_node_by_path("/cpus");
+		if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
+			panic("RISC-V system with no 'timebase-frequency' in DTS\n");
+		of_node_put(cpu);
+		riscv_timebase = prop;
+	} else {
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, (struct acpi_table_header **)&rhct);
+		if (ACPI_FAILURE(status))
+			panic("RISC-V ACPI system with no RHCT table\n");
+		riscv_timebase = rhct->time_base_freq;
+		acpi_put_table((struct acpi_table_header *)rhct);
+	}
 
 	lpj_fine = riscv_timebase / HZ;
 
-	of_clk_init(NULL);
+	if (acpi_disabled)
+		of_clk_init(NULL);
+
 	timer_probe();
 
 	tick_setup_hrtimer_broadcast();
-- 
2.38.0

[PATCH 19/24] RISC-V: ACPI: cpufeature: Add ACPI support in riscv_fill_hwcap()

[Sunil V L]

On ACPI based systems, the information about the hart
like ISA, extesions supported are defined in RISC-V Hart
Capabilities Table (RHCT). Enable filling up hwcap structure
based on the information in RHCT.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/cpufeature.c | 45 ++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 93e45560af30..c10177c608f8 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -6,12 +6,14 @@
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <linux/ctype.h>
 #include <linux/libfdt.h>
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <asm/acpi.h>
 #include <asm/alternative.h>
 #include <asm/cacheflush.h>
 #include <asm/errata_list.h>
@@ -21,6 +23,7 @@
 #include <asm/processor.h>
 #include <asm/smp.h>
 #include <asm/switch_to.h>
+#include <linux/of_device.h>
 
 #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
 
@@ -93,7 +96,10 @@ void __init riscv_fill_hwcap(void)
 	char print_str[NUM_ALPHA_EXTS + 1];
 	int i, j, rc;
 	unsigned long isa2hwcap[26] = {0};
+	struct acpi_table_header *rhct;
+	acpi_status status;
 	unsigned long hartid;
+	unsigned int cpu;
 
 	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
 	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
@@ -106,18 +112,38 @@ void __init riscv_fill_hwcap(void)
 
 	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
 
-	for_each_of_cpu_node(node) {
+	if (!acpi_disabled) {
+
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
+		if (ACPI_FAILURE(status))
+			return;
+	}
+
+	for_each_possible_cpu(cpu) {
 		unsigned long this_hwcap = 0;
 		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
 		const char *temp;
 
-		rc = riscv_of_processor_hartid(node, &hartid);
-		if (rc < 0)
-			continue;
+		if (acpi_disabled) {
+			node = of_cpu_device_node_get(cpu);
+			if (node) {
+				rc = riscv_of_processor_hartid(node, &hartid);
+				if (rc < 0)
+					continue;
 
-		if (of_property_read_string(node, "riscv,isa", &isa)) {
-			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
-			continue;
+				if (of_property_read_string(node, "riscv,isa", &isa)) {
+					pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
+					continue;
+				}
+				of_node_put(node);
+			}
+		} else {
+			rc = acpi_get_riscv_isa(rhct, get_acpi_id_for_cpu(cpu), &isa);
+			if (rc < 0) {
+				pr_warn("Unable to get ISA for the hart - %d\n",
+						cpu);
+				continue;
+			}
 		}
 
 		temp = isa;
@@ -248,6 +274,11 @@ void __init riscv_fill_hwcap(void)
 			bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
 	}
 
+#ifdef CONFIG_ACPI
+	if (!acpi_disabled)
+		acpi_put_table((struct acpi_table_header *)rhct);
+#endif
+
 	/* We don't support systems with F but without D, so mask those out
 	 * here. */
 	if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {
-- 
2.38.0

Re: [PATCH 19/24] RISC-V: ACPI: cpufeature: Add ACPI support in riscv_fill_hwcap()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

On Mon, Jan 30, 2023 at 11:52:20PM +0530, Sunil V L wrote:
> On ACPI based systems, the information about the hart
> like ISA, extesions supported are defined in RISC-V Hart
> Capabilities Table (RHCT). Enable filling up hwcap structure
> based on the information in RHCT.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 45 ++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 93e45560af30..c10177c608f8 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -6,12 +6,14 @@
>   * Copyright (C) 2017 SiFive
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/bitmap.h>
>  #include <linux/ctype.h>
>  #include <linux/libfdt.h>
>  #include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <asm/acpi.h>
>  #include <asm/alternative.h>
>  #include <asm/cacheflush.h>
>  #include <asm/errata_list.h>
> @@ -21,6 +23,7 @@
>  #include <asm/processor.h>
>  #include <asm/smp.h>
>  #include <asm/switch_to.h>
> +#include <linux/of_device.h>

Is there a reason this header is not added with the other linux ones?

>  
>  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
>  
> @@ -93,7 +96,10 @@ void __init riscv_fill_hwcap(void)
>  	char print_str[NUM_ALPHA_EXTS + 1];
>  	int i, j, rc;
>  	unsigned long isa2hwcap[26] = {0};
> +	struct acpi_table_header *rhct;
> +	acpi_status status;
>  	unsigned long hartid;
> +	unsigned int cpu;
>  
>  	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
>  	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> @@ -106,18 +112,38 @@ void __init riscv_fill_hwcap(void)
>  
>  	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
>  
> -	for_each_of_cpu_node(node) {
> +	if (!acpi_disabled) {
> +

Extraneous blank line.

> +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> +		if (ACPI_FAILURE(status))
> +			return;
> +	}
> +
> +	for_each_possible_cpu(cpu) {
>  		unsigned long this_hwcap = 0;
>  		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
>  		const char *temp;
>  
> -		rc = riscv_of_processor_hartid(node, &hartid);
> -		if (rc < 0)
> -			continue;
> +		if (acpi_disabled) {
> +			node = of_cpu_device_node_get(cpu);
> +			if (node) {
> +				rc = riscv_of_processor_hartid(node, &hartid);
> +				if (rc < 0)
> +					continue;
>  
> -		if (of_property_read_string(node, "riscv,isa", &isa)) {
> -			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> -			continue;
> +				if (of_property_read_string(node, "riscv,isa", &isa)) {
> +					pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> +					continue;
> +				}
> +				of_node_put(node);
> +			}
> +		} else {
> +			rc = acpi_get_riscv_isa(rhct, get_acpi_id_for_cpu(cpu), &isa);
> +			if (rc < 0) {
> +				pr_warn("Unable to get ISA for the hart - %d\n",
> +						cpu);

The alignment here is wrong, but the whole thing fits on a single line.

> +				continue;
> +			}
>  		}
>  
>  		temp = isa;
> @@ -248,6 +274,11 @@ void __init riscv_fill_hwcap(void)
>  			bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
>  	}
>  
> +#ifdef CONFIG_ACPI

Is this guard actually needed, or is acpi_put_table() always available?

Cheers,
Conor.

> +	if (!acpi_disabled)
> +		acpi_put_table((struct acpi_table_header *)rhct);
> +#endif
> +
>  	/* We don't support systems with F but without D, so mask those out
>  	 * here. */
>  	if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 19/24] RISC-V: ACPI: cpufeature: Add ACPI support in riscv_fill_hwcap()

[Sunil V L]

On Thu, Feb 09, 2023 at 09:47:52PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:20PM +0530, Sunil V L wrote:
> > On ACPI based systems, the information about the hart
> > like ISA, extesions supported are defined in RISC-V Hart
> > Capabilities Table (RHCT). Enable filling up hwcap structure
> > based on the information in RHCT.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/kernel/cpufeature.c | 45 ++++++++++++++++++++++++++++------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 93e45560af30..c10177c608f8 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -6,12 +6,14 @@
> >   * Copyright (C) 2017 SiFive
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/bitmap.h>
> >  #include <linux/ctype.h>
> >  #include <linux/libfdt.h>
> >  #include <linux/log2.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <asm/acpi.h>
> >  #include <asm/alternative.h>
> >  #include <asm/cacheflush.h>
> >  #include <asm/errata_list.h>
> > @@ -21,6 +23,7 @@
> >  #include <asm/processor.h>
> >  #include <asm/smp.h>
> >  #include <asm/switch_to.h>
> > +#include <linux/of_device.h>
> 
> Is there a reason this header is not added with the other linux ones?
> 
No, let me move it.

> >  
> >  #define NUM_ALPHA_EXTS ('z' - 'a' + 1)
> >  
> > @@ -93,7 +96,10 @@ void __init riscv_fill_hwcap(void)
> >  	char print_str[NUM_ALPHA_EXTS + 1];
> >  	int i, j, rc;
> >  	unsigned long isa2hwcap[26] = {0};
> > +	struct acpi_table_header *rhct;
> > +	acpi_status status;
> >  	unsigned long hartid;
> > +	unsigned int cpu;
> >  
> >  	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
> >  	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> > @@ -106,18 +112,38 @@ void __init riscv_fill_hwcap(void)
> >  
> >  	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
> >  
> > -	for_each_of_cpu_node(node) {
> > +	if (!acpi_disabled) {
> > +
> 
> Extraneous blank line.
> 
Okay

> > +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> > +		if (ACPI_FAILURE(status))
> > +			return;
> > +	}
> > +
> > +	for_each_possible_cpu(cpu) {
> >  		unsigned long this_hwcap = 0;
> >  		DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> >  		const char *temp;
> >  
> > -		rc = riscv_of_processor_hartid(node, &hartid);
> > -		if (rc < 0)
> > -			continue;
> > +		if (acpi_disabled) {
> > +			node = of_cpu_device_node_get(cpu);
> > +			if (node) {
> > +				rc = riscv_of_processor_hartid(node, &hartid);
> > +				if (rc < 0)
> > +					continue;
> >  
> > -		if (of_property_read_string(node, "riscv,isa", &isa)) {
> > -			pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> > -			continue;
> > +				if (of_property_read_string(node, "riscv,isa", &isa)) {
> > +					pr_warn("Unable to find \"riscv,isa\" devicetree entry\n");
> > +					continue;
> > +				}
> > +				of_node_put(node);
> > +			}
> > +		} else {
> > +			rc = acpi_get_riscv_isa(rhct, get_acpi_id_for_cpu(cpu), &isa);
> > +			if (rc < 0) {
> > +				pr_warn("Unable to get ISA for the hart - %d\n",
> > +						cpu);
> 
> The alignment here is wrong, but the whole thing fits on a single line.
> 
Okay

> > +				continue;
> > +			}
> >  		}
> >  
> >  		temp = isa;
> > @@ -248,6 +274,11 @@ void __init riscv_fill_hwcap(void)
> >  			bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> >  	}
> >  
> > +#ifdef CONFIG_ACPI
> 
> Is this guard actually needed, or is acpi_put_table() always available?
> 
It can be removed.

Thanks,
Sunil

> Cheers,
> Conor.
> 
> > +	if (!acpi_disabled)
> > +		acpi_put_table((struct acpi_table_header *)rhct);
> > +#endif
> > +
> >  	/* We don't support systems with F but without D, so mask those out
> >  	 * here. */
> >  	if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {
> > -- 
> > 2.38.0
> > 

[PATCH 20/24] RISC-V: ACPI: cpu: Enable cpuinfo for ACPI systems

[Sunil V L]

On ACPI based platforms, few details like ISA need to be read
from the ACPI table. Enable cpuinfo on ACPI based systems.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/cpu.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 1b9a5a66e55a..bd6c0fcfe4ce 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2012 Regents of the University of California
  */
 
+#include <linux/acpi.h>
 #include <linux/cpu.h>
 #include <linux/init.h>
 #include <linux/seq_file.h>
@@ -256,26 +257,47 @@ static void c_stop(struct seq_file *m, void *v)
 {
 }
 
+#ifdef CONFIG_ACPI
+void acpi_print_hart_info(struct seq_file *m,
+			  unsigned long cpu)
+{
+	const char *isa;
+
+	if (!acpi_get_riscv_isa(NULL, get_acpi_id_for_cpu(cpu), &isa))
+		print_isa(m, isa);
+
+}
+#endif
+
 static int c_show(struct seq_file *m, void *v)
 {
 	unsigned long cpu_id = (unsigned long)v - 1;
-	struct device_node *node = of_get_cpu_node(cpu_id, NULL);
 	struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
+	struct device_node *node;
 	const char *compat, *isa;
 
 	seq_printf(m, "processor\t: %lu\n", cpu_id);
 	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
-	if (!of_property_read_string(node, "riscv,isa", &isa))
-		print_isa(m, isa);
+
+	if (acpi_disabled) {
+		node = of_get_cpu_node(cpu_id, NULL);
+		if (!of_property_read_string(node, "riscv,isa", &isa))
+			print_isa(m, isa);
+		if (!of_property_read_string(node, "compatible", &compat)
+		    && strcmp(compat, "riscv"))
+			seq_printf(m, "uarch\t\t: %s\n", compat);
+		of_node_put(node);
+	}
+#ifdef CONFIG_ACPI
+	else
+		acpi_print_hart_info(m, cpu_id);
+#endif
+
 	print_mmu(m);
-	if (!of_property_read_string(node, "compatible", &compat)
-	    && strcmp(compat, "riscv"))
-		seq_printf(m, "uarch\t\t: %s\n", compat);
 	seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid);
 	seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid);
 	seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid);
 	seq_puts(m, "\n");
-	of_node_put(node);
 
 	return 0;
 }
-- 
2.38.0

Re: [PATCH 20/24] RISC-V: ACPI: cpu: Enable cpuinfo for ACPI systems

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

On Mon, Jan 30, 2023 at 11:52:21PM +0530, Sunil V L wrote:
> On ACPI based platforms, few details like ISA need to be read
> from the ACPI table. Enable cpuinfo on ACPI based systems.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/kernel/cpu.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 1b9a5a66e55a..bd6c0fcfe4ce 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2012 Regents of the University of California
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/cpu.h>
>  #include <linux/init.h>
>  #include <linux/seq_file.h>
> @@ -256,26 +257,47 @@ static void c_stop(struct seq_file *m, void *v)
>  {
>  }
>  
> +#ifdef CONFIG_ACPI
> +void acpi_print_hart_info(struct seq_file *m,
> +			  unsigned long cpu)

Surely this fits on one line?

> +{
> +	const char *isa;
> +
> +	if (!acpi_get_riscv_isa(NULL, get_acpi_id_for_cpu(cpu), &isa))
> +		print_isa(m, isa);

Do you really need to guard this function? Aren't there nop'ed versions
of acpi_get_riscv_isa() and get_acpi_id_for_cpu() in acpi.h?

IMO, basically any use of ifdeffery you can cleanly remove from a c file
is a worthwhile change.

> +

Extra blank line here FYI.

> +}
> +#endif
> +
>  static int c_show(struct seq_file *m, void *v)
>  {
>  	unsigned long cpu_id = (unsigned long)v - 1;
> -	struct device_node *node = of_get_cpu_node(cpu_id, NULL);
>  	struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
> +	struct device_node *node;
>  	const char *compat, *isa;
>  
>  	seq_printf(m, "processor\t: %lu\n", cpu_id);
>  	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
> -	if (!of_property_read_string(node, "riscv,isa", &isa))
> -		print_isa(m, isa);
> +
> +	if (acpi_disabled) {
> +		node = of_get_cpu_node(cpu_id, NULL);
> +		if (!of_property_read_string(node, "riscv,isa", &isa))
> +			print_isa(m, isa);
> +		if (!of_property_read_string(node, "compatible", &compat)
> +		    && strcmp(compat, "riscv"))
                    ^^ this should be on the line above
TBH the whole series is in need of a checkpatch --strict run IMO,
there's a bunch of coding style issues throughout.

> +			seq_printf(m, "uarch\t\t: %s\n", compat);
> +		of_node_put(node);
> +	}
> +#ifdef CONFIG_ACPI
> +	else
> +		acpi_print_hart_info(m, cpu_id);

Delete the ifdeffery here too please :)

Cheers,
Conor.

> +#endif
> +
>  	print_mmu(m);
> -	if (!of_property_read_string(node, "compatible", &compat)
> -	    && strcmp(compat, "riscv"))
> -		seq_printf(m, "uarch\t\t: %s\n", compat);
>  	seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid);
>  	seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid);
>  	seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid);
>  	seq_puts(m, "\n");
> -	of_node_put(node);
>  
>  	return 0;
>  }
> -- 
> 2.38.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 20/24] RISC-V: ACPI: cpu: Enable cpuinfo for ACPI systems

[Sunil V L]

On Thu, Feb 09, 2023 at 09:13:37PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:21PM +0530, Sunil V L wrote:
> > On ACPI based platforms, few details like ISA need to be read
> > from the ACPI table. Enable cpuinfo on ACPI based systems.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/kernel/cpu.c | 36 +++++++++++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 1b9a5a66e55a..bd6c0fcfe4ce 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (C) 2012 Regents of the University of California
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/cpu.h>
> >  #include <linux/init.h>
> >  #include <linux/seq_file.h>
> > @@ -256,26 +257,47 @@ static void c_stop(struct seq_file *m, void *v)
> >  {
> >  }
> >  
> > +#ifdef CONFIG_ACPI
> > +void acpi_print_hart_info(struct seq_file *m,
> > +			  unsigned long cpu)
> 
> Surely this fits on one line?
> 
Okay

> > +{
> > +	const char *isa;
> > +
> > +	if (!acpi_get_riscv_isa(NULL, get_acpi_id_for_cpu(cpu), &isa))
> > +		print_isa(m, isa);
> 
> Do you really need to guard this function? Aren't there nop'ed versions
> of acpi_get_riscv_isa() and get_acpi_id_for_cpu() in acpi.h?
> 
> IMO, basically any use of ifdeffery you can cleanly remove from a c file
> is a worthwhile change.
> 
You are right. Let me remove ifdef.

> > +
> 
> Extra blank line here FYI.
> 
> > +}
> > +#endif
> > +
> >  static int c_show(struct seq_file *m, void *v)
> >  {
> >  	unsigned long cpu_id = (unsigned long)v - 1;
> > -	struct device_node *node = of_get_cpu_node(cpu_id, NULL);
> >  	struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
> > +	struct device_node *node;
> >  	const char *compat, *isa;
> >  
> >  	seq_printf(m, "processor\t: %lu\n", cpu_id);
> >  	seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
> > -	if (!of_property_read_string(node, "riscv,isa", &isa))
> > -		print_isa(m, isa);
> > +
> > +	if (acpi_disabled) {
> > +		node = of_get_cpu_node(cpu_id, NULL);
> > +		if (!of_property_read_string(node, "riscv,isa", &isa))
> > +			print_isa(m, isa);
> > +		if (!of_property_read_string(node, "compatible", &compat)
> > +		    && strcmp(compat, "riscv"))
>                     ^^ this should be on the line above
> TBH the whole series is in need of a checkpatch --strict run IMO,
> there's a bunch of coding style issues throughout.
> 
I just moved this line as is. Sure, let me fix it. Thanks.

> > +			seq_printf(m, "uarch\t\t: %s\n", compat);
> > +		of_node_put(node);
> > +	}
> > +#ifdef CONFIG_ACPI
> > +	else
> > +		acpi_print_hart_info(m, cpu_id);
> 
> Delete the ifdeffery here too please :)
> 
Okay

Thanks,
Sunil

[PATCH 21/24] RISC-V: ACPI: Add ACPI initialization in setup_arch()

[Sunil V L]

Initialize ACPI tables for RISC-V during boot.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/setup.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 4335f08ffaf2..5b4ad1baf664 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -8,6 +8,7 @@
  *  Nick Kossifidis <mick@ics.forth.gr>
  */
 
+#include <linux/acpi.h>
 #include <linux/init.h>
 #include <linux/mm.h>
 #include <linux/memblock.h>
@@ -276,14 +277,22 @@ void __init setup_arch(char **cmdline_p)
 
 	efi_init();
 	paging_init();
+
+	/* Parse the ACPI tables for possible boot-time configuration */
+	acpi_boot_table_init();
+	if (acpi_disabled) {
 #if IS_ENABLED(CONFIG_BUILTIN_DTB)
-	unflatten_and_copy_device_tree();
+		unflatten_and_copy_device_tree();
 #else
-	if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
-		unflatten_device_tree();
-	else
-		pr_err("No DTB found in kernel mappings\n");
+		if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
+			unflatten_device_tree();
+		else
+			pr_err("No DTB found in kernel mappings\n");
 #endif
+	} else {
+		early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)));
+	}
+
 	early_init_fdt_scan_reserved_mem();
 	misc_mem_init();
 
-- 
2.38.0

Re: [PATCH 21/24] RISC-V: ACPI: Add ACPI initialization in setup_arch()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On Mon, Jan 30, 2023 at 11:52:22PM +0530, Sunil V L wrote:
> Initialize ACPI tables for RISC-V during boot.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/kernel/setup.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 4335f08ffaf2..5b4ad1baf664 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -8,6 +8,7 @@
>   *  Nick Kossifidis <mick@ics.forth.gr>
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/init.h>
>  #include <linux/mm.h>
>  #include <linux/memblock.h>
> @@ -276,14 +277,22 @@ void __init setup_arch(char **cmdline_p)
>  
>  	efi_init();
>  	paging_init();
> +
> +	/* Parse the ACPI tables for possible boot-time configuration */
> +	acpi_boot_table_init();
> +	if (acpi_disabled) {
>  #if IS_ENABLED(CONFIG_BUILTIN_DTB)

I only poked it with a stick, but I think this `#if IS_ENABLED()` can
be changed to a normal `if (IS_ENABLED())` while you're already
modifying this code.

> -	unflatten_and_copy_device_tree();
> +		unflatten_and_copy_device_tree();
>  #else
> -	if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> -		unflatten_device_tree();
> -	else
> -		pr_err("No DTB found in kernel mappings\n");
> +		if (early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa))))
> +			unflatten_device_tree();
> +		else
> +			pr_err("No DTB found in kernel mappings\n");
>  #endif
> +	} else {
> +		early_init_dt_verify(__va(XIP_FIXUP(dtb_early_pa)));
> +	}
> +
>  	early_init_fdt_scan_reserved_mem();
>  	misc_mem_init();
>  
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 21/24] RISC-V: ACPI: Add ACPI initialization in setup_arch()

[Sunil V L]

On Thu, Feb 09, 2023 at 09:53:59PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:22PM +0530, Sunil V L wrote:
> > Initialize ACPI tables for RISC-V during boot.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/kernel/setup.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 4335f08ffaf2..5b4ad1baf664 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -8,6 +8,7 @@
> >   *  Nick Kossifidis <mick@ics.forth.gr>
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/init.h>
> >  #include <linux/mm.h>
> >  #include <linux/memblock.h>
> > @@ -276,14 +277,22 @@ void __init setup_arch(char **cmdline_p)
> >  
> >  	efi_init();
> >  	paging_init();
> > +
> > +	/* Parse the ACPI tables for possible boot-time configuration */
> > +	acpi_boot_table_init();
> > +	if (acpi_disabled) {
> >  #if IS_ENABLED(CONFIG_BUILTIN_DTB)
> 
> I only poked it with a stick, but I think this `#if IS_ENABLED()` can
> be changed to a normal `if (IS_ENABLED())` while you're already
> modifying this code.
> 
Sure,

Thanks
Sunil

[PATCH 22/24] RISC-V: ACPI: Enable ACPI in defconfig

[Sunil V L]

Add support to build ACPI subsystem in defconfig.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/configs/defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index 128dcf4c0814..8ce06fb0dde8 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -218,3 +218,7 @@ CONFIG_RCU_EQS_DEBUG=y
 # CONFIG_FTRACE is not set
 # CONFIG_RUNTIME_TESTING_MENU is not set
 CONFIG_MEMTEST=y
+CONFIG_ARCH_SUPPORTS_ACPI=y
+CONFIG_ACPI=y
+CONFIG_ACPI_MCFG=y
+# CONFIG_PCI_QUIRKS is not set
-- 
2.38.0

Re: [PATCH 22/24] RISC-V: ACPI: Enable ACPI in defconfig

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Hey Sunil,

Two quick comments while I think of them..

On Mon, Jan 30, 2023 at 11:52:23PM +0530, Sunil V L wrote:
> RISC-V: ACPI: Enable ACPI in defconfig

btw, about half of this series redundantly puts "ACPI:" or "RISC-V:
ACPI:" into $subject. None of commits that mention ACPI after the last :
should mention ACPI in the prefix IMO, it's just noise.

For example, this one should be something like:
RISC-V: enable ACPI in defconfig

> Add support to build ACPI subsystem in defconfig.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/configs/defconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> index 128dcf4c0814..8ce06fb0dde8 100644
> --- a/arch/riscv/configs/defconfig
> +++ b/arch/riscv/configs/defconfig
> @@ -218,3 +218,7 @@ CONFIG_RCU_EQS_DEBUG=y
>  # CONFIG_FTRACE is not set
>  # CONFIG_RUNTIME_TESTING_MENU is not set
>  CONFIG_MEMTEST=y
> +CONFIG_ARCH_SUPPORTS_ACPI=y

This needs to go into the arch Kconfig file, where it will be selected.
Check what arm64 does if you are not sure what I mean.

Hopefully I'll get a chance to look at the rest of this this week
sometime,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 22/24] RISC-V: ACPI: Enable ACPI in defconfig

[Sunil V L]

Hi Conor,
On Mon, Jan 30, 2023 at 11:47:35PM +0000, Conor Dooley wrote:
> Hey Sunil,
> 
> Two quick comments while I think of them..
> 

Sure. Thank you!

> On Mon, Jan 30, 2023 at 11:52:23PM +0530, Sunil V L wrote:
> > RISC-V: ACPI: Enable ACPI in defconfig
> 
> btw, about half of this series redundantly puts "ACPI:" or "RISC-V:
> ACPI:" into $subject. None of commits that mention ACPI after the last :
> should mention ACPI in the prefix IMO, it's just noise.
> 
> For example, this one should be something like:
> RISC-V: enable ACPI in defconfig
> 

I agree. Will update in the next version. I added ACPI in begining so that
I could quickly identify them along with other patches in my branch.

> > Add support to build ACPI subsystem in defconfig.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/configs/defconfig | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > index 128dcf4c0814..8ce06fb0dde8 100644
> > --- a/arch/riscv/configs/defconfig
> > +++ b/arch/riscv/configs/defconfig
> > @@ -218,3 +218,7 @@ CONFIG_RCU_EQS_DEBUG=y
> >  # CONFIG_FTRACE is not set
> >  # CONFIG_RUNTIME_TESTING_MENU is not set
> >  CONFIG_MEMTEST=y
> > +CONFIG_ARCH_SUPPORTS_ACPI=y
> 
> This needs to go into the arch Kconfig file, where it will be selected.
> Check what arm64 does if you are not sure what I mean.
> 

Yes, I have added in "[PATCH 07/24] RISC-V: ACPI: Enable ACPI build
infrastructure". But forgot to remove here. Thanks!. Will update it when
I send next revision.

> Hopefully I'll get a chance to look at the rest of this this week
> sometime,
> Conor.
> 

Thanks!
Sunil

[PATCH 23/24] MAINTAINERS: Add entry for drivers/acpi/riscv

[Sunil V L]

ACPI defines few RISC-V specific tables which need
parsing code added in drivers/acpi/riscv. Add maintainer
entries for this newly created folder.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8a5c25c20d00..b14ceb917a81 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -450,6 +450,13 @@ S:	Orphan
 F:	drivers/platform/x86/wmi.c
 F:	include/uapi/linux/wmi.h
 
+ACPI FOR RISC-V (ACPI/riscv)
+M:	Sunil V L <sunilvl@ventanamicro.com>
+L:	linux-acpi@vger.kernel.org
+L:	linux-riscv@lists.infradead.org
+S:	Maintained
+F:	drivers/acpi/riscv
+
 ACRN HYPERVISOR SERVICE MODULE
 M:	Fei Li <fei1.li@intel.com>
 L:	acrn-dev@lists.projectacrn.org (subscribers-only)
-- 
2.38.0

Re: [PATCH 23/24] MAINTAINERS: Add entry for drivers/acpi/riscv

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

On Mon, Jan 30, 2023 at 11:52:24PM +0530, Sunil V L wrote:
> ACPI defines few RISC-V specific tables which need
> parsing code added in drivers/acpi/riscv. Add maintainer
> entries for this newly created folder.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8a5c25c20d00..b14ceb917a81 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -450,6 +450,13 @@ S:	Orphan
>  F:	drivers/platform/x86/wmi.c
>  F:	include/uapi/linux/wmi.h
>  
> +ACPI FOR RISC-V (ACPI/riscv)
> +M:	Sunil V L <sunilvl@ventanamicro.com>
> +L:	linux-acpi@vger.kernel.org
> +L:	linux-riscv@lists.infradead.org
> +S:	Maintained

Supported, no?

> +F:	drivers/acpi/riscv
> +
>  ACRN HYPERVISOR SERVICE MODULE
>  M:	Fei Li <fei1.li@intel.com>
>  L:	acrn-dev@lists.projectacrn.org (subscribers-only)
> -- 
> 2.38.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 23/24] MAINTAINERS: Add entry for drivers/acpi/riscv

[Sunil V L]

On Thu, Feb 09, 2023 at 09:54:42PM +0000, Conor Dooley wrote:
> On Mon, Jan 30, 2023 at 11:52:24PM +0530, Sunil V L wrote:
> > ACPI defines few RISC-V specific tables which need
> > parsing code added in drivers/acpi/riscv. Add maintainer
> > entries for this newly created folder.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  MAINTAINERS | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 8a5c25c20d00..b14ceb917a81 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -450,6 +450,13 @@ S:	Orphan
> >  F:	drivers/platform/x86/wmi.c
> >  F:	include/uapi/linux/wmi.h
> >  
> > +ACPI FOR RISC-V (ACPI/riscv)
> > +M:	Sunil V L <sunilvl@ventanamicro.com>
> > +L:	linux-acpi@vger.kernel.org
> > +L:	linux-riscv@lists.infradead.org
> > +S:	Maintained
> 
> Supported, no?
> 
I prefer to keep it same as ARM.

[PATCH 24/24] Documentation/kernel-parameters.txt: Add RISC-V for ACPI parameter

[Sunil V L]

With ACPI support added for RISC-V, this kernel parameter also
supported on RISC-V. Hence, update the documentation.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6cfa6e3996cf..d9795418aaf8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1,9 +1,9 @@
-	acpi=		[HW,ACPI,X86,ARM64]
+	acpi=		[HW,ACPI,X86,ARM64,RISC-V]
 			Advanced Configuration and Power Interface
 			Format: { force | on | off | strict | noirq | rsdt |
 				  copy_dsdt }
 			force -- enable ACPI if default was off
-			on -- enable ACPI but allow fallback to DT [arm64]
+			on -- enable ACPI but allow fallback to DT [arm64,riscv]
 			off -- disable ACPI if default was on
 			noirq -- do not use ACPI for IRQ routing
 			strict -- Be less tolerant of platforms that are not
@@ -12,6 +12,8 @@
 			copy_dsdt -- copy DSDT to memory
 			For ARM64, ONLY "acpi=off", "acpi=on" or "acpi=force"
 			are available
+			For RISC-V, ONLY "acpi=off", "acpi=on" or "acpi=force"
+			are available
 
 			See also Documentation/power/runtime_pm.rst, pci=noacpi
 
-- 
2.38.0

Re: [PATCH 24/24] Documentation/kernel-parameters.txt: Add RISC-V for ACPI parameter

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On Mon, Jan 30, 2023 at 11:52:25PM +0530, Sunil V L wrote:
>  			For ARM64, ONLY "acpi=off", "acpi=on" or "acpi=force"
>  			are available
> +			For RISC-V, ONLY "acpi=off", "acpi=on" or "acpi=force"
> +			are available
>  

Something repetitive here. What about "For ARM64 and RISC-V, the
available options are only "acpi=off", "acpi=on", and "acpi=force""?

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 24/24] Documentation/kernel-parameters.txt: Add RISC-V for ACPI parameter

[Sunil V L]

On Thu, Feb 09, 2023 at 09:02:37AM +0700, Bagas Sanjaya wrote:
> On Mon, Jan 30, 2023 at 11:52:25PM +0530, Sunil V L wrote:
> >  			For ARM64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> >  			are available
> > +			For RISC-V, ONLY "acpi=off", "acpi=on" or "acpi=force"
> > +			are available
> >  
> 
> Something repetitive here. What about "For ARM64 and RISC-V, the
> available options are only "acpi=off", "acpi=on", and "acpi=force""?
> 
> Thanks.
> 
Hi Sanjaya,
-Sunil

Re: [PATCH 00/24] Add basic ACPI support for RISC-V

[Rafael J. Wysocki]

On Mon, Jan 30, 2023 at 7:22 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> This patch series enables the basic ACPI infrastructure for RISC-V.
> Supporting external interrupt controllers is in progress and hence it is
> tested using polling based HVC SBI console and RAM disk.
>
> The series depends on Anup's IPI improvement series.
> https://github.com/avpatel/linux/commits/riscv_ipi_imp_v17
>
> These changes are available at
> https://github.com/vlsunil/linux/commits/acpi_b1_us_review_ipi17
>
> Testing:
> 1) Build Qemu with ACPI support using below branch
> https://github.com/vlsunil/qemu/tree/acpi_b1_us_review
>
> 2) Build EDK2 as per instructions in
> https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support
>
> 3) Build Linux after enabling SBI HVC and SBI earlycon
> CONFIG_RISCV_SBI_V01=y
> CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
> CONFIG_HVC_RISCV_SBI=y
>
> 4) Build buildroot.
>
> Run with below command.
> qemu-system-riscv64   -nographic \
> -drive file=Build/RiscVVirtQemu/RELEASE_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1 \
> -machine virt,acpi=on -smp 16 -m 2G \
> -kernel arch/riscv/boot/Image \
> -initrd buildroot/output/images/rootfs.cpio \
> -append "root=/dev/ram ro console=hvc0 earlycon=sbi"
>
> Jisheng Zhang (1):
>   riscv: move sbi_init() earlier before jump_label_init()
>
> Sunil V L (23):
>   ACPICA: MADT: Add RISC-V INTC interrupt controller
>   ACPICA: Add structure definitions for RISC-V RHCT
>   RISC-V: ACPI: Add empty headers to enable ACPI core
>   RISC-V: ACPI: Add basic functions to build ACPI core
>   RISC-V: ACPI: Add PCI functions to build ACPI core
>   RISC-V: ACPI: Enable ACPI build infrastructure
>   ACPI: Enable ACPI_PROCESSOR for RISC-V
>   ACPI: OSL: Make should_use_kmap() 0 for RISC-V.
>   ACPI: processor_core: RISC-V: Enable mapping processor to the hartid
>   RISC-V: ACPI: irqchip/riscv-intc: Add ACPI support
>   RISC-V: ACPI: smpboot: Create wrapper smp_setup()
>   RISC-V: ACPI: smpboot: Add ACPI support in smp_setup()
>   RISC-V: ACPI: smpboot: Add function to retrieve the hartid
>   clocksource/timer-riscv: Refactor riscv_timer_init_dt()
>   RISC-V: ACPI: clocksource/timer-riscv: Add ACPI support
>   ACPI: RISC-V: drivers/acpi: Add RHCT related code
>   RISC-V: ACPI: time.c: Add ACPI support for time_init()
>   RISC-V: ACPI: cpufeature: Add ACPI support in riscv_fill_hwcap()
>   RISC-V: ACPI: cpu: Enable cpuinfo for ACPI systems
>   RISC-V: ACPI: Add ACPI initialization in setup_arch()
>   RISC-V: ACPI: Enable ACPI in defconfig
>   MAINTAINERS: Add entry for drivers/acpi/riscv
>   Documentation/kernel-parameters.txt: Add RISC-V for ACPI parameter

The series looks fine to me from the ACPI perspective, so please feel
free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to it and route it via RISC-V.

Thanks!

Re: [PATCH 00/24] Add basic ACPI support for RISC-V

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

Hey Sunil,

On Mon, Jan 30, 2023 at 11:52:01PM +0530, Sunil V L wrote:
> This patch series enables the basic ACPI infrastructure for RISC-V.
> Supporting external interrupt controllers is in progress and hence it is
> tested using polling based HVC SBI console and RAM disk.
> 
> The series depends on Anup's IPI improvement series.
> https://github.com/avpatel/linux/commits/riscv_ipi_imp_v17

In the future, please provide links to patchsets rather than "random"
git trees.

> Jisheng Zhang (1):
>   riscv: move sbi_init() earlier before jump_label_init()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What has this patch got to do with your series? Just something that was
sitting in your tree?

If you need this, it'd be ideal if you would submit *with* the R-b tags
it appears to have had by v6 [1] & add the reason that you need to move
it to the commit message.
In Jisheng's series that was obvious, but this is a significantly larger
series and it is hard to spot your reasoning for it.

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 00/24] Add basic ACPI support for RISC-V

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

On Wed, Feb 08, 2023 at 06:28:15PM +0000, Conor Dooley wrote:
> Hey Sunil,
> 
> On Mon, Jan 30, 2023 at 11:52:01PM +0530, Sunil V L wrote:
> > This patch series enables the basic ACPI infrastructure for RISC-V.
> > Supporting external interrupt controllers is in progress and hence it is
> > tested using polling based HVC SBI console and RAM disk.
> > 
> > The series depends on Anup's IPI improvement series.
> > https://github.com/avpatel/linux/commits/riscv_ipi_imp_v17
> 
> In the future, please provide links to patchsets rather than "random"
> git trees.
> 
> > Jisheng Zhang (1):
> >   riscv: move sbi_init() earlier before jump_label_init()
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> What has this patch got to do with your series? Just something that was
> sitting in your tree?
> 
> If you need this, it'd be ideal if you would submit *with* the R-b tags
> it appears to have had by v6 [1] & add the reason that you need to move
> it to the commit message.
> In Jisheng's series that was obvious, but this is a significantly larger
> series and it is hard to spot your reasoning for it.

Apologies, I forgot to provide the link!
https://lore.kernel.org/all/20220821140918.3613-1-jszhang@kernel.org/

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 00/24] Add basic ACPI support for RISC-V

[Sunil V L]

Hi Conor,

On Wed, Feb 08, 2023 at 06:50:39PM +0000, Conor Dooley wrote:
> On Wed, Feb 08, 2023 at 06:28:15PM +0000, Conor Dooley wrote:
> > Hey Sunil,
> > 
> > On Mon, Jan 30, 2023 at 11:52:01PM +0530, Sunil V L wrote:
> > > This patch series enables the basic ACPI infrastructure for RISC-V.
> > > Supporting external interrupt controllers is in progress and hence it is
> > > tested using polling based HVC SBI console and RAM disk.
> > > 
> > > The series depends on Anup's IPI improvement series.
> > > https://github.com/avpatel/linux/commits/riscv_ipi_imp_v17
> > 
> > In the future, please provide links to patchsets rather than "random"
> > git trees.
> > 
> > > Jisheng Zhang (1):
> > >   riscv: move sbi_init() earlier before jump_label_init()
> >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > What has this patch got to do with your series? Just something that was
> > sitting in your tree?
> > 
> > If you need this, it'd be ideal if you would submit *with* the R-b tags
> > it appears to have had by v6 [1] & add the reason that you need to move
> > it to the commit message.
> > In Jisheng's series that was obvious, but this is a significantly larger
> > series and it is hard to spot your reasoning for it.
> 
> Apologies, I forgot to provide the link!
> https://lore.kernel.org/all/20220821140918.3613-1-jszhang@kernel.org/
> 

First of all, thank you very much for your detailed review!!. Will address
your comments in the next revision of the series I am preparing to send this
week.

This patch from Jisheng is required to enable ACPI for the same reason.
sbi_init() should be called before jump_label_init(). Otherwise, I hit a
panic like below.

[   0.000000] efi: seeding entropy pool
 [   0.000000] sbi_remote_fence_i: Enter for cpu_mask = 0000000000000000
 [   0.000000] Kernel panic - not syncing: __sbi_rfence is called before sbi_init!!!
 [   0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.19.0-rc3-00079-gd3ee951eeea1-dirty #3
 [   0.000000] Call Trace:
 [   0.000000] [<ffffffff80005900>] dump_backtrace+0x1c/0x24
 [   0.000000] [<ffffffff8072a83e>] show_stack+0x2c/0x38
 [   0.000000] [<ffffffff8072fe7c>] dump_stack_lvl+0x40/0x58
 [   0.000000] [<ffffffff8072fea8>] dump_stack+0x14/0x1c
 [   0.000000] [<ffffffff8072ae4a>] panic+0x106/0x2c6
 [   0.000000] [<ffffffff8072a964>] sbi_remote_fence_i+0x32/0x4a
 [   0.000000] [<ffffffff80009c22>] flush_icache_all+0x1a/0x44
 [   0.000000] [<ffffffff8000622a>] patch_text_nosync+0x1e/0x2a
 [   0.000000] [<ffffffff800084da>] arch_jump_label_transform+0x48/0xd0
 [   0.000000] [<ffffffff801013ee>] __jump_label_update+0x82/0xd4
 [   0.000000] [<ffffffff801014bc>] jump_label_update+0x7c/0xca
 [   0.000000] [<ffffffff80101b5c>] static_key_enable_cpuslocked+0x70/0x9c
 [   0.000000] [<ffffffff80101b9e>] static_key_enable+0x16/0x24
 [   0.000000] [<ffffffff80730c16>] crng_set_ready+0x18/0x20
 [   0.000000] [<ffffffff80022f06>] execute_in_process_context+0x3e/0x92
 [   0.000000] [<ffffffff80730dc8>] _credit_init_bits+0x9c/0x140
 [   0.000000] [<ffffffff80827024>] add_bootloader_randomness+0x3e/0x48
 [   0.000000] [<ffffffff8082c434>] efi_config_parse_tables+0x114/0x21c
 [   0.000000] [<ffffffff8082dd18>] efi_init+0x11e/0x22a
 [   0.000000] [<ffffffff80803256>] setup_arch+0xc8/0x5fa
 [   0.000000] [<ffffffff80800716>] start_kernel+0x88/0x74e
 [   0.000000] ---[ end Kernel panic - not syncing: __sbi_rfence is called before sbi_init!!! ]---


Yes, I missed V6 of Jishang. Will update it. I will also use public
patchset linkgs.

Thanks!
Sunil