[RFC PATCH v1 00/21] RISC-V: ACPI: Add external interrupt controller support

[RFC PATCH v1 00/21] RISC-V: ACPI: Add external interrupt controller support

[Sunil V L]

This series adds support for the below ECRs approved by ASWG recently.
1) MADT - https://drive.google.com/file/d/1oMGPyOD58JaPgMl1pKasT-VKsIKia7zR/view?usp=sharing
2) RHCT - https://drive.google.com/file/d/1sKbOa8m1UZw1JkquZYe3F1zQBN1xXsaf/view?usp=sharing

The series primarily adds below features.

1) ACPI support for external interrupt controller drivers (IMSIC, APLIC and PLIC).
2) Get CBO block sizes from RHCT.
3) Set timer_can_not_wakeup in timer driver based on the flag in RHCT.

PCI ACPI related functions are migrated from arm64 to common file
so that we don't need to duplicate them for RISC-V.

It uses software node framework to create the fwnode for the interrupt
controllers. This helps in keeping the actual drivers code mostly common
for DT and ACPI.

This series is based on Anup's AIA v7 series. The first 2 ACPICA
patches in this series will be merged via ACPICA release process. PATCH3 is a
fix patch. These patches are included in this series only to enable build.

To test the series,

1) Qemu should be built using the riscv_acpi_b2_v1_plic branch at
https://github.com/vlsunil/qemu.git

2) EDK2 should be built using the instructions at:
https://github.com/tianocore/edk2/blob/master/OvmfPkg/RiscVVirt/README.md

3) Build Linux using this series on top of Anup's AIA v7 series.

Run Qemu:
qemu-system-riscv64 \
 -M virt,pflash0=pflash0,pflash1=pflash1,aia=aplic-imsic \
 -m 2G -smp 8 \
 -serial mon:stdio \
 -device virtio-gpu-pci -full-screen \
 -device qemu-xhci \
 -device usb-kbd \
 -blockdev node-name=pflash0,driver=file,read-only=on,filename=RISCV_VIRT_CODE.fd \
 -blockdev node-name=pflash1,driver=file,filename=RISCV_VIRT_VARS.fd \
 -netdev user,id=net0 -device virtio-net-pci,netdev=net0 \
 -kernel arch/riscv/boot/Image \
 -initrd rootfs.cpio \
 -append "root=/dev/ram ro console=ttyS0 rootwait earlycon=uart8250,mmio,0x10000000"

To boot with APLIC only, use aia=aplic.
To boot with PLIC, remove aia= option.

This series is also available in riscv_acpi_b2_v1 brach at
https://github.com/vlsunil/linux.git

Based-on: 20230802150018.327079-1-apatel@ventanamicro.com
(https://lore.kernel.org/lkml/20230802150018.327079-1-apatel@ventanamicro.com/)


Anup Patel (1):
  swnode: Add support to create early during boot

Sunil V L (20):
  ACPICA: MADT: Add RISC-V external interrupt controllers
  ACPICA: RHCT: Add flags, CMO and MMU nodes
  RISC-V: ACPI: Fix acpi_os_ioremap to return iomem address
  RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping
  arm64: PCI: Migrate ACPI related functions to pci-acpi.c
  RISC-V: ACPI: Implement PCI related functionality
  RISC-V: Kconfig: Select ECAM and MCFG
  RISC-V: ACPI: RHCT: Add function to get CBO block sizes
  RISC-V: cacheflush: Initialize CBO variables on ACPI systems
  clocksource/timer-riscv: ACPI: Add timer_cannot_wakeup_cpu
  irqchip/riscv-intc: Use swnode framework to create fwnode
  irqchip/riscv-imsic-early: Add ACPI support
  ACPI: bus: Add acpi_riscv_init function
  ACPI: RISC-V: Create IMSIC platform device
  ACPI: Add APLIC IRQ model for RISC-V
  ACPI: RISC-V: Create APLIC platform device
  irqchip/irq-riscv-aplic-msi: Add ACPI support
  ACPI: bus: Add PLIC IRQ model
  RISC-V: ACPI: Create PLIC platform device
  irqchip/sifive-plic: Add GSI conversion support

 Documentation/riscv/acpi.rst            |  33 ++
 arch/arm64/kernel/pci.c                 | 193 ---------
 arch/riscv/Kconfig                      |   3 +
 arch/riscv/include/asm/acpi.h           |  21 +-
 arch/riscv/kernel/acpi.c                | 120 +++++-
 arch/riscv/mm/cacheflush.c              |  37 +-
 drivers/acpi/bus.c                      |   7 +
 drivers/acpi/riscv/Makefile             |   2 +-
 drivers/acpi/riscv/init.c               |  16 +
 drivers/acpi/riscv/init.h               |   6 +
 drivers/acpi/riscv/irqchip.c            | 507 ++++++++++++++++++++++++
 drivers/acpi/riscv/rhct.c               |  61 +++
 drivers/base/swnode.c                   | 117 +++++-
 drivers/clocksource/timer-riscv.c       |   4 +
 drivers/irqchip/irq-riscv-aplic-msi.c   |  14 +-
 drivers/irqchip/irq-riscv-imsic-early.c |  28 ++
 drivers/irqchip/irq-riscv-imsic-state.c |  33 +-
 drivers/irqchip/irq-riscv-intc.c        |  12 +-
 drivers/irqchip/irq-sifive-plic.c       |  16 +
 drivers/pci/pci-acpi.c                  | 182 +++++++++
 include/acpi/actbl2.h                   |  76 +++-
 include/linux/acpi.h                    |   8 +
 include/linux/property.h                |   3 +
 23 files changed, 1248 insertions(+), 251 deletions(-)
 create mode 100644 drivers/acpi/riscv/init.c
 create mode 100644 drivers/acpi/riscv/init.h
 create mode 100644 drivers/acpi/riscv/irqchip.c

-- 
2.39.2

[RFC PATCH v1 01/21] ACPICA: MADT: Add RISC-V external interrupt controllers

[Sunil V L]

ACPICA commit 8c048cee4ea7b9ded8db3e1b3b9c14e21e084a2c

This adds 3 different external interrupt controller
definitions in MADT for RISC-V.

 1) RISC-V PLIC is a platform interrupt controller for
    handling wired interrupt in a RISC-V systems.

 2) RISC-V IMSIC is MSI interrupt controller to
    support MSI interrupts.

 3) RISC-V APLIC has dual functionality. First it can
    act like PLIC and direct all wired interrupts to
    the CPU which doesn't have MSI controller. Second,
    when the CPU has MSI controller (IMSIC), it will
    act as a converter from wired interrupts to MSI.

Update the existing RINTC structure also to support
these external interrupt controllers.

This codefirst ECR is approved by UEFI forum and will
be part of next ACPI spec version.

Link: https://github.com/acpica/acpica/commit/8c048cee
Co-developed-by: Haibo Xu <haibo1.xu@intel.com>
Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/acpi/actbl2.h | 50 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 0029336775a9..280ab4c7f77a 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -893,7 +893,10 @@ enum acpi_madt_type {
 	ACPI_MADT_TYPE_BIO_PIC = 22,
 	ACPI_MADT_TYPE_LPC_PIC = 23,
 	ACPI_MADT_TYPE_RINTC = 24,
-	ACPI_MADT_TYPE_RESERVED = 25,	/* 25 to 0x7F are reserved */
+	ACPI_MADT_TYPE_IMSIC = 25,
+	ACPI_MADT_TYPE_APLIC = 26,
+	ACPI_MADT_TYPE_PLIC = 27,
+	ACPI_MADT_TYPE_RESERVED = 28,	/* 28 to 0x7F are reserved */
 	ACPI_MADT_TYPE_OEM_RESERVED = 0x80	/* 0x80 to 0xFF are reserved for OEM use */
 };
 
@@ -1261,6 +1264,9 @@ struct acpi_madt_rintc {
 	u32 flags;
 	u64 hart_id;
 	u32 uid;		/* ACPI processor UID */
+	u32 ext_intc_id;	/* External INTC Id */
+	u64 imsic_addr;		/* IMSIC base address */
+	u32 imsic_size;		/* IMSIC size */
 };
 
 /* Values for RISC-V INTC Version field above */
@@ -1271,6 +1277,48 @@ enum acpi_madt_rintc_version {
 	ACPI_MADT_RINTC_VERSION_RESERVED = 2	/* 2 and greater are reserved */
 };
 
+/* 25: RISC-V IMSIC */
+struct acpi_madt_imsic {
+	struct acpi_subtable_header header;
+	u8 version;
+	u8 reserved;
+	u32 flags;
+	u16 num_ids;
+	u16 num_guest_ids;
+	u8 guest_index_bits;
+	u8 hart_index_bits;
+	u8 group_index_bits;
+	u8 group_index_shift;
+};
+
+/* 26: RISC-V APLIC */
+struct acpi_madt_aplic {
+	struct acpi_subtable_header header;
+	u8 version;
+	u8 id;
+	u32 flags;
+	u8 hw_id[8];
+	u16 num_idcs;
+	u16 num_sources;
+	u32 gsi_base;
+	u64 base_addr;
+	u32 size;
+};
+
+/* 27: RISC-V PLIC */
+struct acpi_madt_plic {
+	struct acpi_subtable_header header;
+	u8 version;
+	u8 id;
+	u8 hw_id[8];
+	u16 num_irqs;
+	u16 max_prio;
+	u32 flags;
+	u32 size;
+	u64 base_addr;
+	u32 gsi_base;
+};
+
 /* 80: OEM data */
 
 struct acpi_madt_oem_data {
-- 
2.39.2

[RFC PATCH v1 02/21] ACPICA: RHCT: Add flags, CMO and MMU nodes

[Sunil V L]

ACPICA commit 2eded5a6a13d892b7dc3be6096e7b1e8d4407600

Update RHCT table with below details.

 1) Add additional structure to describe the Cache Management
    Operation (CMO) related information.

 2) Add structure to describe MMU type.

 3) Convert the current reserved field to flags and define
    a flag to indicate timer capability.

This codefirst ECR is approved by UEFI forum and will
be part of next ACPI spec version.

Link: https://github.com/acpica/acpica/commit/2eded5a6
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/acpi/actbl2.h | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 280ab4c7f77a..3751ae69432f 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -2778,12 +2778,15 @@ enum acpi_rgrt_image_type {
 
 struct acpi_table_rhct {
 	struct acpi_table_header header;	/* Common ACPI table header */
-	u32 reserved;
+	u32 flags;		/* RHCT flags */
 	u64 time_base_freq;
 	u32 node_count;
 	u32 node_offset;
 };
 
+/* RHCT Flags */
+
+#define ACPI_RHCT_TIMER_CANNOT_WAKEUP_CPU       (1)
 /*
  * RHCT subtables
  */
@@ -2797,6 +2800,9 @@ struct acpi_rhct_node_header {
 
 enum acpi_rhct_node_type {
 	ACPI_RHCT_NODE_TYPE_ISA_STRING = 0x0000,
+	ACPI_RHCT_NODE_TYPE_CMO = 0x0001,
+	ACPI_RHCT_NODE_TYPE_MMU = 0x0002,
+	ACPI_RHCT_NODE_TYPE_RESERVED = 0x0003,
 	ACPI_RHCT_NODE_TYPE_HART_INFO = 0xFFFF,
 };
 
@@ -2810,6 +2816,24 @@ struct acpi_rhct_isa_string {
 	char isa[];
 };
 
+struct acpi_rhct_cmo_node {
+	u8 reserved;		/* Must be zero */
+	u8 cbom_size;		/* CBOM size in powerof 2 */
+	u8 cbop_size;		/* CBOP size in powerof 2 */
+	u8 cboz_size;		/* CBOZ size in powerof 2 */
+};
+
+struct acpi_rhct_mmu_node {
+	u8 reserved;		/* Must be zero */
+	u8 mmu_type;		/* Virtual Address Scheme */
+};
+
+enum acpi_rhct_mmu_type {
+	ACPI_RHCT_MMU_TYPE_SV39 = 0,
+	ACPI_RHCT_MMU_TYPE_SV48 = 1,
+	ACPI_RHCT_MMU_TYPE_SV57 = 2
+};
+
 /* Hart Info node structure */
 struct acpi_rhct_hart_info {
 	u16 num_offsets;
-- 
2.39.2

[RFC PATCH v1 03/21] RISC-V: ACPI: Fix acpi_os_ioremap to return iomem address

[Sunil V L]

acpi_os_ioremap() currently is a wrapper to memremap() on
RISC-V. But the callers of acpi_os_ioremap() expect it to
return __iomem address and hence sparse tool reports a new
warning. Fix this issue by type casting to  __iomem type.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202307230357.egcTAefj-lkp@intel.com/
Fixes: a91a9ffbd3a5 ("RISC-V: Add support to build the ACPI core")
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h | 2 +-
 arch/riscv/kernel/acpi.c      | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index f71ce21ff684..d5604d2073bc 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -19,7 +19,7 @@ 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);
+void __iomem *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 */
diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
index 5ee03ebab80e..56cb2c986c48 100644
--- a/arch/riscv/kernel/acpi.c
+++ b/arch/riscv/kernel/acpi.c
@@ -215,9 +215,9 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
 	early_iounmap(map, size);
 }
 
-void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 {
-	return memremap(phys, size, MEMREMAP_WB);
+	return (void __iomem *)memremap(phys, size, MEMREMAP_WB);
 }
 
 #ifdef CONFIG_PCI
-- 
2.39.2

Re: [RFC PATCH v1 03/21] RISC-V: ACPI: Fix acpi_os_ioremap to return iomem address

[Andrew Jones]

On Thu, Aug 03, 2023 at 11:28:58PM +0530, Sunil V L wrote:
> acpi_os_ioremap() currently is a wrapper to memremap() on
> RISC-V. But the callers of acpi_os_ioremap() expect it to
> return __iomem address and hence sparse tool reports a new
> warning. Fix this issue by type casting to  __iomem type.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202307230357.egcTAefj-lkp@intel.com/
> Fixes: a91a9ffbd3a5 ("RISC-V: Add support to build the ACPI core")
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/include/asm/acpi.h | 2 +-
>  arch/riscv/kernel/acpi.c      | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index f71ce21ff684..d5604d2073bc 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -19,7 +19,7 @@ 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);
> +void __iomem *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 */
> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> index 5ee03ebab80e..56cb2c986c48 100644
> --- a/arch/riscv/kernel/acpi.c
> +++ b/arch/riscv/kernel/acpi.c
> @@ -215,9 +215,9 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
>  	early_iounmap(map, size);
>  }
>  
> -void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> +void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
>  {
> -	return memremap(phys, size, MEMREMAP_WB);
> +	return (void __iomem *)memremap(phys, size, MEMREMAP_WB);
>  }
>  
>  #ifdef CONFIG_PCI
> -- 
> 2.39.2
>

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

[RFC PATCH v1 04/21] RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping

[Sunil V L]

Enhance the acpi_os_ioremap() to support opregions in MMIO
space. Also, have strict checks using EFI memory map
to allow remapping the RAM similar to arm64.

Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Alexandre Ghiti <alexghiti@rivosinc.com>

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/Kconfig       |  1 +
 arch/riscv/kernel/acpi.c | 86 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 318f62a0a187..e19f32c12a68 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -38,6 +38,7 @@ config RISCV
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_HAS_VDSO_DATA
+	select ARCH_KEEP_MEMBLOCK
 	select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
 	select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
 	select ARCH_STACKWALK
diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
index 56cb2c986c48..aa4433bca6d9 100644
--- a/arch/riscv/kernel/acpi.c
+++ b/arch/riscv/kernel/acpi.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/pci.h>
 #include <linux/efi.h>
+#include <linux/memblock.h>
 
 int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
 int acpi_disabled = 1;
@@ -217,7 +218,90 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
 
 void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 {
-	return (void __iomem *)memremap(phys, size, MEMREMAP_WB);
+	efi_memory_desc_t *md, *region = NULL;
+	pgprot_t prot;
+
+	if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP)))
+		return NULL;
+
+	for_each_efi_memory_desc(md) {
+		u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
+
+		if (phys < md->phys_addr || phys >= end)
+			continue;
+
+		if (phys + size > end) {
+			pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n");
+			return NULL;
+		}
+		region = md;
+		break;
+	}
+
+	/*
+	 * It is fine for AML to remap regions that are not represented in the
+	 * EFI memory map at all, as it only describes normal memory, and MMIO
+	 * regions that require a virtual mapping to make them accessible to
+	 * the EFI runtime services.
+	 */
+	prot = PAGE_KERNEL_IO;
+	if (region) {
+		switch (region->type) {
+		case EFI_LOADER_CODE:
+		case EFI_LOADER_DATA:
+		case EFI_BOOT_SERVICES_CODE:
+		case EFI_BOOT_SERVICES_DATA:
+		case EFI_CONVENTIONAL_MEMORY:
+		case EFI_PERSISTENT_MEMORY:
+			if (memblock_is_map_memory(phys) ||
+			    !memblock_is_region_memory(phys, size)) {
+				pr_warn(FW_BUG "requested region covers kernel memory @ %p\n",
+					&phys);
+				return NULL;
+			}
+
+			/*
+			 * Mapping kernel memory is permitted if the region in
+			 * question is covered by a single memblock with the
+			 * NOMAP attribute set: this enables the use of ACPI
+			 * table overrides passed via initramfs.
+			 * This particular use case only requires read access.
+			 */
+			fallthrough;
+
+		case EFI_RUNTIME_SERVICES_CODE:
+			/*
+			 * This would be unusual, but not problematic per se,
+			 * as long as we take care not to create a writable
+			 * mapping for executable code.
+			 */
+			prot = PAGE_KERNEL_RO;
+			break;
+
+		case EFI_ACPI_RECLAIM_MEMORY:
+			/*
+			 * ACPI reclaim memory is used to pass firmware tables
+			 * and other data that is intended for consumption by
+			 * the OS only, which may decide it wants to reclaim
+			 * that memory and use it for something else. We never
+			 * do that, but we usually add it to the linear map
+			 * anyway, in which case we should use the existing
+			 * mapping.
+			 */
+			if (memblock_is_map_memory(phys))
+				return (void __iomem *)__va(phys);
+			fallthrough;
+
+		default:
+			if (region->attribute & EFI_MEMORY_WB)
+				prot = PAGE_KERNEL;
+			else if ((region->attribute & EFI_MEMORY_WC) ||
+				 (region->attribute & EFI_MEMORY_WT))
+				prot = pgprot_writecombine(PAGE_KERNEL);
+		}
+	}
+
+	return ioremap_prot(phys, size, pgprot_val(prot));
 }
 
 #ifdef CONFIG_PCI
-- 
2.39.2

Re: [RFC PATCH v1 04/21] RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:28:59PM +0530, Sunil V L wrote:
> Enhance the acpi_os_ioremap() to support opregions in MMIO
> space. Also, have strict checks using EFI memory map
> to allow remapping the RAM similar to arm64.
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Alexandre Ghiti <alexghiti@rivosinc.com>

You may use --cc to the command line when forming patches.

Also we usually consider Cc: as a part of the tag block, meaning no blank line
should be here.

> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>

...

>  #include <linux/io.h>
>  #include <linux/pci.h>
>  #include <linux/efi.h>
> +#include <linux/memblock.h>

Can you squeeze it to have some order, like to be after io.h (taking into
account given context)?

...

> +			if (memblock_is_map_memory(phys) ||
> +			    !memblock_is_region_memory(phys, size)) {
> +				pr_warn(FW_BUG "requested region covers kernel memory @ %p\n",
> +					&phys);

How %p can be useful here (it's mangled), but also wouldn't this give a hint to
an attacker about the kernel memory location and diminish the KASLR protection?
(IIRC after boot we always have the same salt for the mangling the pointers when
 printing, so at least theoretically it might be possible to bruteforce the
 printing algo to give a clue about the kernel address.)

> +				return NULL;
> +			}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 04/21] RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping

[Sunil V L]

On Fri, Aug 04, 2023 at 08:47:53AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:28:59PM +0530, Sunil V L wrote:
> > Enhance the acpi_os_ioremap() to support opregions in MMIO
> > space. Also, have strict checks using EFI memory map
> > to allow remapping the RAM similar to arm64.
> > 
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> You may use --cc to the command line when forming patches.
> 
> Also we usually consider Cc: as a part of the tag block, meaning no blank line
> should be here.
> 
Thanks!, Andy. I specifically wanted Ard and Alex to look at this patch
and hence added Cc. I was not aware that no blank line should be there.
Thanks for letting me know.

> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> 
> ...
> 
> >  #include <linux/io.h>
> >  #include <linux/pci.h>
> >  #include <linux/efi.h>
> > +#include <linux/memblock.h>
> 
> Can you squeeze it to have some order, like to be after io.h (taking into
> account given context)?
> 
Yeah, sure. Let me update in next version.

> ...
> 
> > +			if (memblock_is_map_memory(phys) ||
> > +			    !memblock_is_region_memory(phys, size)) {
> > +				pr_warn(FW_BUG "requested region covers kernel memory @ %p\n",
> > +					&phys);
> 
> How %p can be useful here (it's mangled), but also wouldn't this give a hint to
> an attacker about the kernel memory location and diminish the KASLR protection?
> (IIRC after boot we always have the same salt for the mangling the pointers when
>  printing, so at least theoretically it might be possible to bruteforce the
>  printing algo to give a clue about the kernel address.)
> 
Okay. This is copied from arm64 version. But I think this is a good
point. Let me just print the warning message without address in the next
version.

Thanks!
Sunil

Re: [RFC PATCH v1 04/21] RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping

[Andrew Jones]

On Thu, Aug 03, 2023 at 11:28:59PM +0530, Sunil V L wrote:
> Enhance the acpi_os_ioremap() to support opregions in MMIO
> space. Also, have strict checks using EFI memory map
> to allow remapping the RAM similar to arm64.
> 
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/Kconfig       |  1 +
>  arch/riscv/kernel/acpi.c | 86 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 318f62a0a187..e19f32c12a68 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -38,6 +38,7 @@ config RISCV
>  	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>  	select ARCH_HAS_UBSAN_SANITIZE_ALL
>  	select ARCH_HAS_VDSO_DATA
> +	select ARCH_KEEP_MEMBLOCK
>  	select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
>  	select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
>  	select ARCH_STACKWALK
> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
> index 56cb2c986c48..aa4433bca6d9 100644
> --- a/arch/riscv/kernel/acpi.c
> +++ b/arch/riscv/kernel/acpi.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/pci.h>
>  #include <linux/efi.h>
> +#include <linux/memblock.h>
>  
>  int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
>  int acpi_disabled = 1;
> @@ -217,7 +218,90 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
>  
>  void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
>  {
> -	return (void __iomem *)memremap(phys, size, MEMREMAP_WB);
> +	efi_memory_desc_t *md, *region = NULL;
> +	pgprot_t prot;
> +
> +	if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP)))
> +		return NULL;
> +
> +	for_each_efi_memory_desc(md) {
> +		u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
> +
> +		if (phys < md->phys_addr || phys >= end)
> +			continue;
> +
> +		if (phys + size > end) {
> +			pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n");
> +			return NULL;
> +		}
> +		region = md;
> +		break;
> +	}
> +
> +	/*
> +	 * It is fine for AML to remap regions that are not represented in the
> +	 * EFI memory map at all, as it only describes normal memory, and MMIO
> +	 * regions that require a virtual mapping to make them accessible to
> +	 * the EFI runtime services.
> +	 */
> +	prot = PAGE_KERNEL_IO;
> +	if (region) {
> +		switch (region->type) {
> +		case EFI_LOADER_CODE:
> +		case EFI_LOADER_DATA:
> +		case EFI_BOOT_SERVICES_CODE:
> +		case EFI_BOOT_SERVICES_DATA:
> +		case EFI_CONVENTIONAL_MEMORY:
> +		case EFI_PERSISTENT_MEMORY:
> +			if (memblock_is_map_memory(phys) ||
> +			    !memblock_is_region_memory(phys, size)) {
> +				pr_warn(FW_BUG "requested region covers kernel memory @ %p\n",
> +					&phys);
> +				return NULL;
> +			}
> +
> +			/*
> +			 * Mapping kernel memory is permitted if the region in
> +			 * question is covered by a single memblock with the
> +			 * NOMAP attribute set: this enables the use of ACPI
> +			 * table overrides passed via initramfs.
> +			 * This particular use case only requires read access.
> +			 */
> +			fallthrough;
> +
> +		case EFI_RUNTIME_SERVICES_CODE:
> +			/*
> +			 * This would be unusual, but not problematic per se,
> +			 * as long as we take care not to create a writable
> +			 * mapping for executable code.
> +			 */
> +			prot = PAGE_KERNEL_RO;
> +			break;
> +
> +		case EFI_ACPI_RECLAIM_MEMORY:
> +			/*
> +			 * ACPI reclaim memory is used to pass firmware tables
> +			 * and other data that is intended for consumption by
> +			 * the OS only, which may decide it wants to reclaim
> +			 * that memory and use it for something else. We never
> +			 * do that, but we usually add it to the linear map
> +			 * anyway, in which case we should use the existing
> +			 * mapping.
> +			 */
> +			if (memblock_is_map_memory(phys))
> +				return (void __iomem *)__va(phys);
> +			fallthrough;
> +
> +		default:
> +			if (region->attribute & EFI_MEMORY_WB)
> +				prot = PAGE_KERNEL;
> +			else if ((region->attribute & EFI_MEMORY_WC) ||
> +				 (region->attribute & EFI_MEMORY_WT))
> +				prot = pgprot_writecombine(PAGE_KERNEL);
> +		}
> +	}
> +
> +	return ioremap_prot(phys, size, pgprot_val(prot));
>  }
>  
>  #ifdef CONFIG_PCI
> -- 
> 2.39.2
>

Such a faithful port of arm64's function that it begs the question as to
whether or not we should refactor and share. Anyway, other than Andy's
comments, LGTM

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

Thanks,
drew

[RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Sunil V L]

The functions defined in arm64 for ACPI support are required
for RISC-V also. To avoid duplication, copy these functions
to common location.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/arm64/kernel/pci.c | 193 ----------------------------------------
 drivers/pci/pci-acpi.c  | 182 +++++++++++++++++++++++++++++++++++++
 2 files changed, 182 insertions(+), 193 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 2276689b5411..fd9a7bed83ce 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -6,30 +6,7 @@
  * Copyright (C) 2014 ARM Ltd.
  */
 
-#include <linux/acpi.h>
-#include <linux/init.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/mm.h>
-#include <linux/of_pci.h>
-#include <linux/of_platform.h>
 #include <linux/pci.h>
-#include <linux/pci-acpi.h>
-#include <linux/pci-ecam.h>
-#include <linux/slab.h>
-
-#ifdef CONFIG_ACPI
-/*
- * Try to assign the IRQ number when probing a new device
- */
-int pcibios_alloc_irq(struct pci_dev *dev)
-{
-	if (!acpi_disabled)
-		acpi_pci_irq_enable(dev);
-
-	return 0;
-}
-#endif
 
 /*
  * raw_pci_read/write - Platform-specific PCI config space access.
@@ -63,173 +40,3 @@ int pcibus_to_node(struct pci_bus *bus)
 EXPORT_SYMBOL(pcibus_to_node);
 
 #endif
-
-#ifdef CONFIG_ACPI
-
-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;
-}
-
-int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
-{
-	struct pci_config_window *cfg;
-	struct acpi_device *adev;
-	struct device *bus_dev;
-
-	if (acpi_disabled)
-		return 0;
-
-	cfg = bridge->bus->sysdata;
-
-	/*
-	 * On Hyper-V there is no corresponding ACPI device for a root bridge,
-	 * therefore ->parent is set as NULL by the driver. And set 'adev' as
-	 * NULL in this case because there is no proper ACPI device.
-	 */
-	if (!cfg->parent)
-		adev = NULL;
-	else
-		adev = to_acpi_device(cfg->parent);
-
-	bus_dev = &bridge->bus->dev;
-
-	ACPI_COMPANION_SET(&bridge->dev, adev);
-	set_dev_node(bus_dev, acpi_get_node(acpi_device_handle(adev)));
-
-	return 0;
-}
-
-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;
-}
-
-void pcibios_add_bus(struct pci_bus *bus)
-{
-	acpi_pci_add_bus(bus);
-}
-
-void pcibios_remove_bus(struct pci_bus *bus)
-{
-	acpi_pci_remove_bus(bus);
-}
-
-#endif
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a05350a4e49c..d6b2d64b8237 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -15,6 +15,7 @@
 #include <linux/pci_hotplug.h>
 #include <linux/module.h>
 #include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
 #include <linux/rwsem.h>
@@ -1517,4 +1518,185 @@ static int __init acpi_pci_init(void)
 
 	return 0;
 }
+
 arch_initcall(acpi_pci_init);
+
+#if defined(CONFIG_ARM64)
+/*
+ * Try to assign the IRQ number when probing a new device
+ */
+int pcibios_alloc_irq(struct pci_dev *dev)
+{
+	if (!acpi_disabled)
+		acpi_pci_irq_enable(dev);
+
+	return 0;
+}
+
+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;
+}
+
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	struct pci_config_window *cfg;
+	struct acpi_device *adev;
+	struct device *bus_dev;
+
+	if (acpi_disabled)
+		return 0;
+
+	cfg = bridge->bus->sysdata;
+
+	/*
+	 * On Hyper-V there is no corresponding ACPI device for a root bridge,
+	 * therefore ->parent is set as NULL by the driver. And set 'adev' as
+	 * NULL in this case because there is no proper ACPI device.
+	 */
+	if (!cfg->parent)
+		adev = NULL;
+	else
+		adev = to_acpi_device(cfg->parent);
+
+	bus_dev = &bridge->bus->dev;
+
+	ACPI_COMPANION_SET(&bridge->dev, adev);
+	set_dev_node(bus_dev, acpi_get_node(acpi_device_handle(adev)));
+
+	return 0;
+}
+
+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;
+}
+
+void pcibios_add_bus(struct pci_bus *bus)
+{
+	acpi_pci_add_bus(bus);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+	acpi_pci_remove_bus(bus);
+}
+
+#endif
-- 
2.39.2

Re: [RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:29:00PM +0530, Sunil V L wrote:
> The functions defined in arm64 for ACPI support are required
> for RISC-V also. To avoid duplication, copy these functions
> to common location.

...

>  }
> +

Stray change.

>  arch_initcall(acpi_pci_init);
> +
> +#if defined(CONFIG_ARM64)

...

> +	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;

Can be

	cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops);
	ret = PTR_ERR_OR_ZERO(cfg);
	if (ret) {
		dev_err(dev, "%04x:%pR error %d mapping ECAM\n", seg, bus_res, ret);

but as far as I understand this is in the original code like this, so consider
as a suggestion for further cleanups.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Sunil V L]

On Fri, Aug 04, 2023 at 08:53:14AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:29:00PM +0530, Sunil V L wrote:
> > The functions defined in arm64 for ACPI support are required
> > for RISC-V also. To avoid duplication, copy these functions
> > to common location.
> 
> ...
> 
> >  }
> > +
> 
> Stray change.
> 
Let me remove this in next version.

> >  arch_initcall(acpi_pci_init);
> > +
> > +#if defined(CONFIG_ARM64)
> 
> ...
> 
> > +	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;
> 
> Can be
> 
> 	cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops);
> 	ret = PTR_ERR_OR_ZERO(cfg);
> 	if (ret) {
> 		dev_err(dev, "%04x:%pR error %d mapping ECAM\n", seg, bus_res, ret);
> 
> but as far as I understand this is in the original code like this, so consider
> as a suggestion for further cleanups.
>

Good suggestion!. Sure, we can cleanup as a follow on patch.

Thanks,
Sunil 

Re: [RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Bjorn Helgaas]

On Thu, Aug 03, 2023 at 11:29:00PM +0530, Sunil V L wrote:
> The functions defined in arm64 for ACPI support are required
> for RISC-V also. To avoid duplication, copy these functions
> to common location.

I assume this is a "move" (not a copy) and the code being moved isn't
being changed.

If so,

  Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/arm64/kernel/pci.c | 193 ----------------------------------------
>  drivers/pci/pci-acpi.c  | 182 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 182 insertions(+), 193 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 2276689b5411..fd9a7bed83ce 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -6,30 +6,7 @@
>   * Copyright (C) 2014 ARM Ltd.
>   */
>  
> -#include <linux/acpi.h>
> -#include <linux/init.h>
> -#include <linux/io.h>
> -#include <linux/kernel.h>
> -#include <linux/mm.h>
> -#include <linux/of_pci.h>
> -#include <linux/of_platform.h>
>  #include <linux/pci.h>
> -#include <linux/pci-acpi.h>
> -#include <linux/pci-ecam.h>
> -#include <linux/slab.h>
> -
> -#ifdef CONFIG_ACPI
> -/*
> - * Try to assign the IRQ number when probing a new device
> - */
> -int pcibios_alloc_irq(struct pci_dev *dev)
> -{
> -	if (!acpi_disabled)
> -		acpi_pci_irq_enable(dev);
> -
> -	return 0;
> -}
> -#endif
>  
>  /*
>   * raw_pci_read/write - Platform-specific PCI config space access.
> @@ -63,173 +40,3 @@ int pcibus_to_node(struct pci_bus *bus)
>  EXPORT_SYMBOL(pcibus_to_node);
>  
>  #endif
> -
> -#ifdef CONFIG_ACPI
> -
> -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;
> -}
> -
> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> -{
> -	struct pci_config_window *cfg;
> -	struct acpi_device *adev;
> -	struct device *bus_dev;
> -
> -	if (acpi_disabled)
> -		return 0;
> -
> -	cfg = bridge->bus->sysdata;
> -
> -	/*
> -	 * On Hyper-V there is no corresponding ACPI device for a root bridge,
> -	 * therefore ->parent is set as NULL by the driver. And set 'adev' as
> -	 * NULL in this case because there is no proper ACPI device.
> -	 */
> -	if (!cfg->parent)
> -		adev = NULL;
> -	else
> -		adev = to_acpi_device(cfg->parent);
> -
> -	bus_dev = &bridge->bus->dev;
> -
> -	ACPI_COMPANION_SET(&bridge->dev, adev);
> -	set_dev_node(bus_dev, acpi_get_node(acpi_device_handle(adev)));
> -
> -	return 0;
> -}
> -
> -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;
> -}
> -
> -void pcibios_add_bus(struct pci_bus *bus)
> -{
> -	acpi_pci_add_bus(bus);
> -}
> -
> -void pcibios_remove_bus(struct pci_bus *bus)
> -{
> -	acpi_pci_remove_bus(bus);
> -}
> -
> -#endif
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a05350a4e49c..d6b2d64b8237 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -15,6 +15,7 @@
>  #include <linux/pci_hotplug.h>
>  #include <linux/module.h>
>  #include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_qos.h>
>  #include <linux/rwsem.h>
> @@ -1517,4 +1518,185 @@ static int __init acpi_pci_init(void)
>  
>  	return 0;
>  }
> +
>  arch_initcall(acpi_pci_init);
> +
> +#if defined(CONFIG_ARM64)
> +/*
> + * Try to assign the IRQ number when probing a new device
> + */
> +int pcibios_alloc_irq(struct pci_dev *dev)
> +{
> +	if (!acpi_disabled)
> +		acpi_pci_irq_enable(dev);
> +
> +	return 0;
> +}
> +
> +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;
> +}
> +
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct pci_config_window *cfg;
> +	struct acpi_device *adev;
> +	struct device *bus_dev;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	cfg = bridge->bus->sysdata;
> +
> +	/*
> +	 * On Hyper-V there is no corresponding ACPI device for a root bridge,
> +	 * therefore ->parent is set as NULL by the driver. And set 'adev' as
> +	 * NULL in this case because there is no proper ACPI device.
> +	 */
> +	if (!cfg->parent)
> +		adev = NULL;
> +	else
> +		adev = to_acpi_device(cfg->parent);
> +
> +	bus_dev = &bridge->bus->dev;
> +
> +	ACPI_COMPANION_SET(&bridge->dev, adev);
> +	set_dev_node(bus_dev, acpi_get_node(acpi_device_handle(adev)));
> +
> +	return 0;
> +}
> +
> +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;
> +}
> +
> +void pcibios_add_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_add_bus(bus);
> +}
> +
> +void pcibios_remove_bus(struct pci_bus *bus)
> +{
> +	acpi_pci_remove_bus(bus);
> +}
> +
> +#endif
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Sunil V L]

On Mon, Aug 07, 2023 at 05:41:52PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 03, 2023 at 11:29:00PM +0530, Sunil V L wrote:
> > The functions defined in arm64 for ACPI support are required
> > for RISC-V also. To avoid duplication, copy these functions
> > to common location.
> 
> I assume this is a "move" (not a copy) and the code being moved isn't
> being changed.
> 
Hi Bjorn,

Thank you very much for the review!. Yes, it is a move as is. Let me
update the commit message in next version.

Thanks!
Sunil
> If so,
> 
>   Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/arm64/kernel/pci.c | 193 ----------------------------------------
> >  drivers/pci/pci-acpi.c  | 182 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 182 insertions(+), 193 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 2276689b5411..fd9a7bed83ce 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -6,30 +6,7 @@
> >   * Copyright (C) 2014 ARM Ltd.
> >   */
> >  
> > -#include <linux/acpi.h>
> > -#include <linux/init.h>
> > -#include <linux/io.h>
> > -#include <linux/kernel.h>
> > -#include <linux/mm.h>
> > -#include <linux/of_pci.h>
> > -#include <linux/of_platform.h>
> >  #include <linux/pci.h>
> > -#include <linux/pci-acpi.h>
> > -#include <linux/pci-ecam.h>
> > -#include <linux/slab.h>
> > -
> > -#ifdef CONFIG_ACPI
> > -/*
> > - * Try to assign the IRQ number when probing a new device
> > - */
> > -int pcibios_alloc_irq(struct pci_dev *dev)
> > -{
> > -	if (!acpi_disabled)
> > -		acpi_pci_irq_enable(dev);
> > -
> > -	return 0;
> > -}
> > -#endif
> >  
> >  /*
> >   * raw_pci_read/write - Platform-specific PCI config space access.
> > @@ -63,173 +40,3 @@ int pcibus_to_node(struct pci_bus *bus)
> >  EXPORT_SYMBOL(pcibus_to_node);
> >  
> >  #endif
> > -
> > -#ifdef CONFIG_ACPI
> > -
> > -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;
> > -}
> > -
> > -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> > -{
> > -	struct pci_config_window *cfg;
> > -	struct acpi_device *adev;
> > -	struct device *bus_dev;
> > -
> > -	if (acpi_disabled)
> > -		return 0;
> > -
> > -	cfg = bridge->bus->sysdata;
> > -
> > -	/*
> > -	 * On Hyper-V there is no corresponding ACPI device for a root bridge,
> > -	 * therefore ->parent is set as NULL by the driver. And set 'adev' as
> > -	 * NULL in this case because there is no proper ACPI device.
> > -	 */
> > -	if (!cfg->parent)
> > -		adev = NULL;
> > -	else
> > -		adev = to_acpi_device(cfg->parent);
> > -
> > -	bus_dev = &bridge->bus->dev;
> > -
> > -	ACPI_COMPANION_SET(&bridge->dev, adev);
> > -	set_dev_node(bus_dev, acpi_get_node(acpi_device_handle(adev)));
> > -
> > -	return 0;
> > -}
> > -
> > -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;
> > -}
> > -
> > -void pcibios_add_bus(struct pci_bus *bus)
> > -{
> > -	acpi_pci_add_bus(bus);
> > -}
> > -
> > -void pcibios_remove_bus(struct pci_bus *bus)
> > -{
> > -	acpi_pci_remove_bus(bus);
> > -}
> > -
> > -#endif
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index a05350a4e49c..d6b2d64b8237 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/pci_hotplug.h>
> >  #include <linux/module.h>
> >  #include <linux/pci-acpi.h>
> > +#include <linux/pci-ecam.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pm_qos.h>
> >  #include <linux/rwsem.h>
> > @@ -1517,4 +1518,185 @@ static int __init acpi_pci_init(void)
> >  
> >  	return 0;
> >  }
> > +
> >  arch_initcall(acpi_pci_init);
> > +
> > +#if defined(CONFIG_ARM64)
> > +/*
> > + * Try to assign the IRQ number when probing a new device
> > + */
> > +int pcibios_alloc_irq(struct pci_dev *dev)
> > +{
> > +	if (!acpi_disabled)
> > +		acpi_pci_irq_enable(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +}
> > +
> > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> > +{
> > +	struct pci_config_window *cfg;
> > +	struct acpi_device *adev;
> > +	struct device *bus_dev;
> > +
> > +	if (acpi_disabled)
> > +		return 0;
> > +
> > +	cfg = bridge->bus->sysdata;
> > +
> > +	/*
> > +	 * On Hyper-V there is no corresponding ACPI device for a root bridge,
> > +	 * therefore ->parent is set as NULL by the driver. And set 'adev' as
> > +	 * NULL in this case because there is no proper ACPI device.
> > +	 */
> > +	if (!cfg->parent)
> > +		adev = NULL;
> > +	else
> > +		adev = to_acpi_device(cfg->parent);
> > +
> > +	bus_dev = &bridge->bus->dev;
> > +
> > +	ACPI_COMPANION_SET(&bridge->dev, adev);
> > +	set_dev_node(bus_dev, acpi_get_node(acpi_device_handle(adev)));
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +}
> > +
> > +void pcibios_add_bus(struct pci_bus *bus)
> > +{
> > +	acpi_pci_add_bus(bus);
> > +}
> > +
> > +void pcibios_remove_bus(struct pci_bus *bus)
> > +{
> > +	acpi_pci_remove_bus(bus);
> > +}
> > +
> > +#endif
> > -- 
> > 2.39.2
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Andy Shevchenko]

On Tue, Aug 08, 2023 at 10:22:10AM +0530, Sunil V L wrote:
> On Mon, Aug 07, 2023 at 05:41:52PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 03, 2023 at 11:29:00PM +0530, Sunil V L wrote:
> > > The functions defined in arm64 for ACPI support are required
> > > for RISC-V also. To avoid duplication, copy these functions
> > > to common location.
> > 
> > I assume this is a "move" (not a copy) and the code being moved isn't
> > being changed.
> 
> Thank you very much for the review!. Yes, it is a move as is. Let me
> update the commit message in next version.

Try to play with -M -C

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 05/21] arm64: PCI: Migrate ACPI related functions to pci-acpi.c

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 05:41:52PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 03, 2023 at 11:29:00PM +0530, Sunil V L wrote:
> > The functions defined in arm64 for ACPI support are required
> > for RISC-V also. To avoid duplication, copy these functions
> > to common location.
> 
> I assume this is a "move" (not a copy) and the code being moved isn't
> being changed.

Not sure is proper -M -C will help to recognize that on the patch generation.
Maybe -M 50 (or another carefully chosen percentage) can help.

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH v1 06/21] RISC-V: ACPI: Implement PCI related functionality

[Sunil V L]

Replace the dummy implementation for PCI related functions
with actual implementation.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/kernel/acpi.c | 32 +++++++++++++++-----------------
 drivers/pci/pci-acpi.c   |  2 +-
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
index aa4433bca6d9..01022c5802ec 100644
--- a/arch/riscv/kernel/acpi.c
+++ b/arch/riscv/kernel/acpi.c
@@ -307,29 +307,27 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 #ifdef CONFIG_PCI
 
 /*
- * These interfaces are defined just to enable building ACPI core.
- * TODO: Update it with actual implementation when external interrupt
- * controller support is added in RISC-V 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)
+int raw_pci_read(unsigned int domain, unsigned int bus,
+		 unsigned int devfn, int reg, int len, u32 *val)
 {
-	return PCIBIOS_DEVICE_NOT_FOUND;
-}
+	struct pci_bus *b = pci_find_bus(domain, bus);
 
-int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
-		  int reg, int len, u32 val)
-{
-	return PCIBIOS_DEVICE_NOT_FOUND;
-}
+	if (!b)
+		return PCIBIOS_DEVICE_NOT_FOUND;
 
-int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
-{
-	return -1;
+	return b->ops->read(b, devfn, reg, len, val);
 }
 
-struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+int raw_pci_write(unsigned int domain, unsigned int bus,
+		  unsigned int devfn, int reg, int len, u32 val)
 {
-	return NULL;
+	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);
 }
 #endif	/* CONFIG_PCI */
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index d6b2d64b8237..5af6188cdbe0 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1521,7 +1521,7 @@ static int __init acpi_pci_init(void)
 
 arch_initcall(acpi_pci_init);
 
-#if defined(CONFIG_ARM64)
+#if defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
 /*
  * Try to assign the IRQ number when probing a new device
  */
-- 
2.39.2

[RFC PATCH v1 07/21] RISC-V: Kconfig: Select ECAM and MCFG

[Sunil V L]

Enable ECAM and MCFG to support PCIe on ACPI based
platforms.

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

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index e19f32c12a68..79dc35e89d5f 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -13,6 +13,7 @@ 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_DMA_DEFAULT_COHERENT
 	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
@@ -147,6 +148,7 @@ config RISCV
 	select OF_EARLY_FLATTREE
 	select OF_IRQ
 	select PCI_DOMAINS_GENERIC if PCI
+	select PCI_ECAM if (ACPI && PCI)
 	select PCI_MSI if PCI
 	select RISCV_ALTERNATIVE if !XIP_KERNEL
 	select RISCV_APLIC
-- 
2.39.2

[RFC PATCH v1 08/21] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Sunil V L]

CBO related block size in ACPI is provided by RHCT. Add
support to read the CMO node in RHCT to get this information.

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

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index d5604d2073bc..0c4e8b35103e 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -66,6 +66,8 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
 		       unsigned int cpu, const char **isa);
 
 static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
+int acpi_get_cbo_block_size(struct acpi_table_header *table, unsigned int cpu, u32 *cbom_size,
+			    u32 *cboz_size, u32 *cbop_size);
 #else
 static inline void acpi_init_rintc_map(void) { }
 static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
@@ -79,6 +81,13 @@ static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
 	return -EINVAL;
 }
 
+static inline int acpi_get_cbo_block_size(struct acpi_table_header *table,
+					  unsigned int cpu, u32 *cbom_size,
+					  u32 *cboz_size, u32 *cbop_size)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_ACPI */
 
 #endif /*_ASM_ACPI_H*/
diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
index b280b3e9c7d9..07309525f277 100644
--- a/drivers/acpi/riscv/rhct.c
+++ b/drivers/acpi/riscv/rhct.c
@@ -81,3 +81,64 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
 
 	return -1;
 }
+
+/*
+ * 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_cbo_block_size(struct acpi_table_header *table, unsigned int cpu,
+			    u32 *cbom_size, u32 *cboz_size, u32 *cbop_size)
+{
+	struct acpi_rhct_node_header *node, *ref_node, *end;
+	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
+	u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info);
+	struct acpi_rhct_hart_info *hart_info;
+	struct acpi_rhct_cmo_node *cmo_node;
+	struct acpi_table_rhct *rhct;
+	u32 *hart_info_node_offset;
+	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+
+	BUG_ON(acpi_disabled);
+
+	if (!table) {
+		rhct = (struct acpi_table_rhct *)acpi_get_rhct();
+		if (!rhct)
+			return -ENOENT;
+	} else {
+		rhct = (struct acpi_table_rhct *)table;
+	}
+
+	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
+
+	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
+	     node < end;
+	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {
+		if (node->type == ACPI_RHCT_NODE_TYPE_HART_INFO) {
+			hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node, size_hdr);
+			hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo);
+			if (acpi_cpu_id != hart_info->uid)
+				continue;
+
+			for (int i = 0; i < hart_info->num_offsets; i++) {
+				ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
+							rhct, hart_info_node_offset[i]);
+				if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
+					cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
+								ref_node, size_hdr);
+					if (cbom_size)
+						*cbom_size = 1 << cmo_node->cbom_size;
+
+					if (cboz_size)
+						*cboz_size = 1 << cmo_node->cboz_size;
+
+					if (cbop_size)
+						*cbop_size = 1 << cmo_node->cbop_size;
+					return 0;
+				}
+			}
+		}
+	}
+
+	return -ENOENT;
+}
-- 
2.39.2

Re: [RFC PATCH v1 08/21] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:29:03PM +0530, Sunil V L wrote:
> CBO related block size in ACPI is provided by RHCT. Add
> support to read the CMO node in RHCT to get this information.

...

> +	if (!table) {

Why not positive conditional?

> +		rhct = (struct acpi_table_rhct *)acpi_get_rhct();
> +		if (!rhct)
> +			return -ENOENT;
> +	} else {
> +		rhct = (struct acpi_table_rhct *)table;
> +	}

...

> +	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);

> +

Blank line here is not needed.

> +	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
> +	     node < end;
> +	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {

> +			for (int i = 0; i < hart_info->num_offsets; i++) {
> +				ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
> +							rhct, hart_info_node_offset[i]);
> +				if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
> +					cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
> +								ref_node, size_hdr);
> +					if (cbom_size)
> +						*cbom_size = 1 << cmo_node->cbom_size;
> +
> +					if (cboz_size)
> +						*cboz_size = 1 << cmo_node->cboz_size;
> +
> +					if (cbop_size)
> +						*cbop_size = 1 << cmo_node->cbop_size;

BIT() in all three cases?

But how you guarantee it will not overflow? I mean who prevents cboX_size to be
bigger than 30 (note also that 31 in your case is Undefined Behaviour in
accordance with the C standard).

> +					return 0;
> +				}
> +			}

> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 08/21] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Sunil V L]

On Fri, Aug 04, 2023 at 09:00:45AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:29:03PM +0530, Sunil V L wrote:
> > CBO related block size in ACPI is provided by RHCT. Add
> > support to read the CMO node in RHCT to get this information.
> 
> ...
> 
> > +	if (!table) {
> 
> Why not positive conditional?
> 
Sure.

> > +		rhct = (struct acpi_table_rhct *)acpi_get_rhct();
> > +		if (!rhct)
> > +			return -ENOENT;
> > +	} else {
> > +		rhct = (struct acpi_table_rhct *)table;
> > +	}
> 
> ...
> 
> > +	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
> 
> > +
> 
> Blank line here is not needed.
> 
Okay.

> > +	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
> > +	     node < end;
> > +	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {
> 
> > +			for (int i = 0; i < hart_info->num_offsets; i++) {
> > +				ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
> > +							rhct, hart_info_node_offset[i]);
> > +				if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
> > +					cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
> > +								ref_node, size_hdr);
> > +					if (cbom_size)
> > +						*cbom_size = 1 << cmo_node->cbom_size;
> > +
> > +					if (cboz_size)
> > +						*cboz_size = 1 << cmo_node->cboz_size;
> > +
> > +					if (cbop_size)
> > +						*cbop_size = 1 << cmo_node->cbop_size;
> 
> BIT() in all three cases?
>
Sure.
 
> But how you guarantee it will not overflow? I mean who prevents cboX_size to be
> bigger than 30 (note also that 31 in your case is Undefined Behaviour in
> accordance with the C standard).
> 
Good catch!. Let me add a check that the value is not more than 30.

Thanks!
Sunil

[RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Sunil V L]

Using new interface to get the CBO block size information in
RHCT, initialize the variables on ACPI platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/mm/cacheflush.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index fbc59b3f69f2..63bb56819b37 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -4,6 +4,8 @@
  */
 
 #include <linux/of.h>
+#include <linux/acpi.h>
+#include <asm/acpi.h>
 #include <asm/cacheflush.h>
 
 #ifdef CONFIG_SMP
@@ -131,15 +133,38 @@ void __init riscv_init_cbo_blocksizes(void)
 	unsigned long cbom_hartid, cboz_hartid;
 	u32 cbom_block_size = 0, cboz_block_size = 0;
 	struct device_node *node;
+	struct acpi_table_header *rhct;
+	acpi_status status;
+	unsigned int cpu;
+
+	if (!acpi_disabled) {
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
+		if (ACPI_FAILURE(status))
+			return;
+	}
 
-	for_each_of_cpu_node(node) {
-		/* set block-size for cbom and/or cboz extension if available */
-		cbo_get_block_size(node, "riscv,cbom-block-size",
-				   &cbom_block_size, &cbom_hartid);
-		cbo_get_block_size(node, "riscv,cboz-block-size",
-				   &cboz_block_size, &cboz_hartid);
+	for_each_possible_cpu(cpu) {
+		if (acpi_disabled) {
+			node = of_cpu_device_node_get(cpu);
+			if (!node) {
+				pr_warn("Unable to find cpu node\n");
+				continue;
+			}
+
+			/* set block-size for cbom and/or cboz extension if available */
+			cbo_get_block_size(node, "riscv,cbom-block-size",
+					   &cbom_block_size, &cbom_hartid);
+			cbo_get_block_size(node, "riscv,cboz-block-size",
+					   &cboz_block_size, &cboz_hartid);
+		} else {
+			acpi_get_cbo_block_size(rhct, cpu, &cbom_block_size,
+						&cboz_block_size, NULL);
+		}
 	}
 
+	if (!acpi_disabled && rhct)
+		acpi_put_table((struct acpi_table_header *)rhct);
+
 	if (cbom_block_size)
 		riscv_cbom_block_size = cbom_block_size;
 
-- 
2.39.2

Re: [RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> Using new interface to get the CBO block size information in
> RHCT, initialize the variables on ACPI platforms.

...

>  #include <linux/of.h>
> +#include <linux/acpi.h>

Can you keep it sorted (to some extent)?

> +#include <asm/acpi.h>

What do you need this for?

>  #include <asm/cacheflush.h>

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Sunil V L]

On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> > Using new interface to get the CBO block size information in
> > RHCT, initialize the variables on ACPI platforms.
> 
> ...
> 
> >  #include <linux/of.h>
> > +#include <linux/acpi.h>
> 
> Can you keep it sorted (to some extent)?
> 
Sure.

> > +#include <asm/acpi.h>
> 
> What do you need this for?
> 
> >  #include <asm/cacheflush.h>
> 
When CONFIG_ACPI is disabled, this include is required to get
acpi_get_cbo_block_size().

Thanks,
Sunil

Re: [RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> > > Using new interface to get the CBO block size information in
> > > RHCT, initialize the variables on ACPI platforms.

...

> > > +#include <asm/acpi.h>
> > 
> > What do you need this for?
> > 
> > >  #include <asm/cacheflush.h>
> > 
> When CONFIG_ACPI is disabled, this include is required to get
> acpi_get_cbo_block_size().

How is it useful without ACPI being enabled?  If it's indeed
(in which I do not believe), better to make sure you have it
avaiable independently on CONFIG_ACPI. Otherwise, just put
#ifdef CONFIG_ACPI around the call.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Conor Dooley]

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

On Fri, Aug 04, 2023 at 05:59:51PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> > On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:
> > > > Using new interface to get the CBO block size information in
> > > > RHCT, initialize the variables on ACPI platforms.
> 
> ...
> 
> > > > +#include <asm/acpi.h>
> > > 
> > > What do you need this for?
> > > 
> > > >  #include <asm/cacheflush.h>
> > > 
> > When CONFIG_ACPI is disabled, this include is required to get
> > acpi_get_cbo_block_size().
> 
> How is it useful without ACPI being enabled?

It is not, as evidenced by the `return -EINVAL;`.

> If it's indeed
> (in which I do not believe), better to make sure you have it
> avaiable independently on CONFIG_ACPI. Otherwise, just put
> #ifdef CONFIG_ACPI around the call.

Let's not litter the code with ifdeffery please where it can be easily
avoided.

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

Re: [RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 04:19:27PM +0100, Conor Dooley wrote:
> On Fri, Aug 04, 2023 at 05:59:51PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> > > On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:

...

> > > > > +#include <asm/acpi.h>
> > > > 
> > > > What do you need this for?
> > > > 
> > > > >  #include <asm/cacheflush.h>
> > > > 
> > > When CONFIG_ACPI is disabled, this include is required to get
> > > acpi_get_cbo_block_size().
> > 
> > How is it useful without ACPI being enabled?
> 
> It is not, as evidenced by the `return -EINVAL;`.
> 
> > If it's indeed
> > (in which I do not believe), better to make sure you have it
> > avaiable independently on CONFIG_ACPI. Otherwise, just put
> > #ifdef CONFIG_ACPI around the call.
> 
> Let's not litter the code with ifdeffery please where it can be easily
> avoided.

Including asm/acpi.h looks to me as a "let's avoid it with a hack that it
is uglier than ifdeffery". Sorry, but ifdeffery for ACPI, with all my full
agreement with the statement that it's not good, is the correct way to fix
this.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 09/21] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 07:52:56PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 04:19:27PM +0100, Conor Dooley wrote:
> > On Fri, Aug 04, 2023 at 05:59:51PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 04, 2023 at 02:50:34PM +0530, Sunil V L wrote:
> > > > On Fri, Aug 04, 2023 at 08:56:29AM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Aug 03, 2023 at 11:29:04PM +0530, Sunil V L wrote:

...

> > > > > > +#include <asm/acpi.h>
> > > > > 
> > > > > What do you need this for?
> > > > > 
> > > > > >  #include <asm/cacheflush.h>
> > > > > 
> > > > When CONFIG_ACPI is disabled, this include is required to get
> > > > acpi_get_cbo_block_size().
> > > 
> > > How is it useful without ACPI being enabled?
> > 
> > It is not, as evidenced by the `return -EINVAL;`.
> > 
> > > If it's indeed
> > > (in which I do not believe), better to make sure you have it
> > > avaiable independently on CONFIG_ACPI. Otherwise, just put
> > > #ifdef CONFIG_ACPI around the call.
> > 
> > Let's not litter the code with ifdeffery please where it can be easily
> > avoided.
> 
> Including asm/acpi.h looks to me as a "let's avoid it with a hack that it
> is uglier than ifdeffery". Sorry, but ifdeffery for ACPI, with all my full
> agreement with the statement that it's not good, is the correct way to fix
> this.

On the other hand this is an arch code and I see precedents of using the
headers together, alas, it seems not better to me that ugly ifdeffery.

So, I leave it to the respective maintainers to decide.

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH v1 10/21] clocksource/timer-riscv: ACPI: Add timer_cannot_wakeup_cpu

[Sunil V L]

The timer capability to wakeup the cpu irrespective of its
idle state is provided by the flag in RHCT. Update the timer
code to set this flag.

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

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index da3071b387eb..50198657230e 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -212,6 +212,10 @@ 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)
 {
+	struct acpi_table_rhct *rhct = (struct acpi_table_rhct *)table;
+
+	riscv_timer_cannot_wake_cpu = rhct->flags & ACPI_RHCT_TIMER_CANNOT_WAKEUP_CPU;
+
 	return riscv_timer_init_common();
 }
 
-- 
2.39.2

[RFC PATCH v1 11/21] swnode: Add support to create early during boot

[Sunil V L]

From: Anup Patel <apatel@ventanamicro.com>

swnode framework can be used to create fwnode for interrupt
controllers. This helps in keeping the drivers same for both
DT and ACPI. To enable this, enhance the swnode framework so
that it can be created early during boot without dependency
on sysfs.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Co-developed-by: Sunil V L <sunilvl@ventanamicro.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/base/swnode.c    | 117 +++++++++++++++++++++++++++++++++------
 include/linux/property.h |   3 +
 2 files changed, 103 insertions(+), 17 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 1886995a0b3a..43f191a38980 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #include "base.h"
 
@@ -21,6 +22,7 @@ struct swnode {
 
 	/* hierarchy */
 	struct ida child_ids;
+	struct list_head early;
 	struct list_head entry;
 	struct list_head children;
 	struct swnode *parent;
@@ -32,6 +34,9 @@ struct swnode {
 static DEFINE_IDA(swnode_root_ids);
 static struct kset *swnode_kset;
 
+static DEFINE_SPINLOCK(swnode_early_lock);
+static LIST_HEAD(swnode_early_list);
+
 #define kobj_to_swnode(_kobj_) container_of(_kobj_, struct swnode, kobj)
 
 static const struct fwnode_operations software_node_ops;
@@ -73,6 +78,17 @@ software_node_to_swnode(const struct software_node *node)
 	if (!node)
 		return NULL;
 
+	spin_lock(&swnode_early_lock);
+
+	list_for_each_entry(swnode, &swnode_early_list, early) {
+		if (swnode->node == node) {
+			spin_unlock(&swnode_early_lock);
+			return swnode;
+		}
+	}
+
+	spin_unlock(&swnode_early_lock);
+
 	spin_lock(&swnode_kset->list_lock);
 
 	list_for_each_entry(k, &swnode_kset->list, entry) {
@@ -698,6 +714,19 @@ software_node_find_by_name(const struct software_node *parent, const char *name)
 	if (!name)
 		return NULL;
 
+	spin_lock(&swnode_early_lock);
+
+	list_for_each_entry(swnode, &swnode_early_list, early) {
+		if (parent == swnode->node->parent && swnode->node->name &&
+		    !strcmp(name, swnode->node->name)) {
+			kobject_get(&swnode->kobj);
+			spin_unlock(&swnode_early_lock);
+			return swnode->node;
+		}
+	}
+
+	spin_unlock(&swnode_early_lock);
+
 	spin_lock(&swnode_kset->list_lock);
 
 	list_for_each_entry(k, &swnode_kset->list, entry) {
@@ -742,10 +771,16 @@ static void software_node_free(const struct software_node *node)
 	kfree(node);
 }
 
-static void software_node_release(struct kobject *kobj)
+static void software_node_release_common(struct kobject *kobj, bool early)
 {
 	struct swnode *swnode = kobj_to_swnode(kobj);
 
+	if (early) {
+		spin_lock(&swnode_early_lock);
+		list_del(&swnode->early);
+		spin_unlock(&swnode_early_lock);
+	}
+
 	if (swnode->parent) {
 		ida_simple_remove(&swnode->parent->child_ids, swnode->id);
 		list_del(&swnode->entry);
@@ -760,6 +795,20 @@ static void software_node_release(struct kobject *kobj)
 	kfree(swnode);
 }
 
+static void software_node_release(struct kobject *kobj)
+{
+	software_node_release_common(kobj, false);
+}
+
+static void software_node_release_early(struct kobject *kobj)
+{
+	software_node_release_common(kobj, true);
+}
+
+static const struct kobj_type software_node_type_early = {
+	.release = software_node_release_early
+};
+
 static const struct kobj_type software_node_type = {
 	.release = software_node_release,
 	.sysfs_ops = &kobj_sysfs_ops,
@@ -767,7 +816,7 @@ static const struct kobj_type software_node_type = {
 
 static struct fwnode_handle *
 swnode_register(const struct software_node *node, struct swnode *parent,
-		unsigned int allocated)
+		unsigned int allocated, unsigned int early)
 {
 	struct swnode *swnode;
 	int ret;
@@ -786,21 +835,39 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 	swnode->id = ret;
 	swnode->node = node;
 	swnode->parent = parent;
-	swnode->kobj.kset = swnode_kset;
+	swnode->kobj.kset = (!early) ? swnode_kset : NULL;
 	fwnode_init(&swnode->fwnode, &software_node_ops);
 
 	ida_init(&swnode->child_ids);
+	INIT_LIST_HEAD(&swnode->early);
 	INIT_LIST_HEAD(&swnode->entry);
 	INIT_LIST_HEAD(&swnode->children);
 
-	if (node->name)
-		ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
-					   parent ? &parent->kobj : NULL,
-					   "%s", node->name);
-	else
-		ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
-					   parent ? &parent->kobj : NULL,
-					   "node%d", swnode->id);
+	if (early) {
+		ret = 0;
+		kobject_init(&swnode->kobj, &software_node_type_early);
+		swnode->kobj.parent = parent ? &parent->kobj : NULL;
+		if (node->name)
+			ret = kobject_set_name(&swnode->kobj,
+					       "%s", node->name);
+		else
+			ret = kobject_set_name(&swnode->kobj,
+					       "node%d", swnode->id);
+		if (!ret) {
+			spin_lock(&swnode_early_lock);
+			list_add_tail(&swnode->early, &swnode_early_list);
+			spin_unlock(&swnode_early_lock);
+		}
+	} else {
+		if (node->name)
+			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
+						   parent ? &parent->kobj : NULL,
+						   "%s", node->name);
+		else
+			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
+						   parent ? &parent->kobj : NULL,
+						   "node%d", swnode->id);
+	}
 	if (ret) {
 		kobject_put(&swnode->kobj);
 		return ERR_PTR(ret);
@@ -815,7 +882,8 @@ swnode_register(const struct software_node *node, struct swnode *parent,
 	if (parent)
 		list_add_tail(&swnode->entry, &parent->children);
 
-	kobject_uevent(&swnode->kobj, KOBJ_ADD);
+	if (!early)
+		kobject_uevent(&swnode->kobj, KOBJ_ADD);
 	return &swnode->fwnode;
 }
 
@@ -892,7 +960,7 @@ int software_node_register(const struct software_node *node)
 	if (node->parent && !parent)
 		return -EINVAL;
 
-	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0));
+	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0));
 }
 EXPORT_SYMBOL_GPL(software_node_register);
 
@@ -910,9 +978,10 @@ void software_node_unregister(const struct software_node *node)
 }
 EXPORT_SYMBOL_GPL(software_node_unregister);
 
-struct fwnode_handle *
-fwnode_create_software_node(const struct property_entry *properties,
-			    const struct fwnode_handle *parent)
+static struct fwnode_handle *
+fwnode_create_software_node_common(const struct property_entry *properties,
+				   const struct fwnode_handle *parent,
+				   bool early)
 {
 	struct fwnode_handle *fwnode;
 	struct software_node *node;
@@ -931,12 +1000,26 @@ fwnode_create_software_node(const struct property_entry *properties,
 
 	node->parent = p ? p->node : NULL;
 
-	fwnode = swnode_register(node, p, 1);
+	fwnode = swnode_register(node, p, 1, early);
 	if (IS_ERR(fwnode))
 		software_node_free(node);
 
 	return fwnode;
 }
+
+struct fwnode_handle *
+fwnode_create_software_node_early(const struct property_entry *properties,
+				  const struct fwnode_handle *parent)
+{
+	return fwnode_create_software_node_common(properties, parent, true);
+}
+
+struct fwnode_handle *
+fwnode_create_software_node(const struct property_entry *properties,
+			    const struct fwnode_handle *parent)
+{
+	return fwnode_create_software_node_common(properties, parent, false);
+}
 EXPORT_SYMBOL_GPL(fwnode_create_software_node);
 
 void fwnode_remove_software_node(struct fwnode_handle *fwnode)
diff --git a/include/linux/property.h b/include/linux/property.h
index 8c3c6685a2ae..7137338bfabb 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -503,6 +503,9 @@ void software_node_unregister_node_group(const struct software_node **node_group
 int software_node_register(const struct software_node *node);
 void software_node_unregister(const struct software_node *node);
 
+struct fwnode_handle *
+fwnode_create_software_node_early(const struct property_entry *properties,
+				  const struct fwnode_handle *parent);
 struct fwnode_handle *
 fwnode_create_software_node(const struct property_entry *properties,
 			    const struct fwnode_handle *parent);
-- 
2.39.2

Re: [RFC PATCH v1 11/21] swnode: Add support to create early during boot

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote:
> From: Anup Patel <apatel@ventanamicro.com>
> 
> swnode framework can be used to create fwnode for interrupt
> controllers.

Why? What is this for?
Can you elaborate? This commit message is poorly written...

And why firmware node is not enough for ACPI case?
I assume the fwnode in DT case is already provided by OF.

> This helps in keeping the drivers same for both
> DT and ACPI. To enable this, enhance the swnode framework so
> that it can be created early during boot without dependency
> on sysfs.

...

> -	swnode->kobj.kset = swnode_kset;
> +	swnode->kobj.kset = (!early) ? swnode_kset : NULL;

Too many unneeded characters. Why parentheses? Why negative check?

...

> +	if (early) {
> +		ret = 0;
> +		kobject_init(&swnode->kobj, &software_node_type_early);
> +		swnode->kobj.parent = parent ? &parent->kobj : NULL;
> +		if (node->name)
> +			ret = kobject_set_name(&swnode->kobj,
> +					       "%s", node->name);
> +		else
> +			ret = kobject_set_name(&swnode->kobj,
> +					       "node%d", swnode->id);
> +		if (!ret) {
> +			spin_lock(&swnode_early_lock);
> +			list_add_tail(&swnode->early, &swnode_early_list);
> +			spin_unlock(&swnode_early_lock);
> +		}
> +	} else {
> +		if (node->name)
> +			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> +						   parent ? &parent->kobj : NULL,

This looks like have a duplication.

> +						   "%s", node->name);
> +		else
> +			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> +						   parent ? &parent->kobj : NULL,
> +						   "node%d", swnode->id);
> +	}

Maybe it's possible to refactor this piece to be more compact?

...

> -	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0));
> +	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0));

In one case you use boolean, here is unsigned int for early flag, why is the
inconsistency added?

...

> -struct fwnode_handle *
> -fwnode_create_software_node(const struct property_entry *properties,
> -			    const struct fwnode_handle *parent)
> +static struct fwnode_handle *
> +fwnode_create_software_node_common(const struct property_entry *properties,
> +				   const struct fwnode_handle *parent,
> +				   bool early)

Why would you need this API in early stages?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 11/21] swnode: Add support to create early during boot

[Sunil V L]

Hi Andy,

On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote:
> > From: Anup Patel <apatel@ventanamicro.com>
> > 
> > swnode framework can be used to create fwnode for interrupt
> > controllers.
> 
> Why? What is this for?
> Can you elaborate? This commit message is poorly written...
> 
> And why firmware node is not enough for ACPI case?
> I assume the fwnode in DT case is already provided by OF.
> 
Thanks a lot for the review!.

You are right, OF provides the fwnode for irqchip drivers. However, for
ACPI case, it is typically created using irq_domain_alloc_named_fwnode
or irq_domain_alloc_fwnode since these are not ACPI devices in the
namespace but from MADT. The fwnode created using
irq_domain_alloc_fwnode() is a simple one which doesn't support properties
similar to the one created by OF framework or software node framework.
Hence, lot of data from the MADT structures need to be cached as
separate structures in the drivers and also would need several ifdefs to
check for ACPI and some amount of code duplication is also required due
to the way DT driver gets the information vs ACPI.

The beauty of software node framework is, it supports adding properties
and also is a supported fwnode type in __irq_domain_create(). So, if we
can create the fwnode for these irqchip using software node, we can
attach the same properties and the actual irqchip driver which uses the
fwnode doesn't need to have any ACPI vs DT checks. Same driver will work
seamlessly on both DT and ACPI platforms.  But the challenge is,
currently swnode expects to be created with sysfs which won't be
available during early boot when irqchip drivers need to be probed. So,
adding support to create without dependency on sysfs help us to reuse
the same framework for irqchip use case also.

Apologies for not descriptive in the commit message. Please let us know
your feedback on this approach.

> > This helps in keeping the drivers same for both
> > DT and ACPI. To enable this, enhance the swnode framework so
> > that it can be created early during boot without dependency
> > on sysfs.
> 
> ...
> 
> > -	swnode->kobj.kset = swnode_kset;
> > +	swnode->kobj.kset = (!early) ? swnode_kset : NULL;
> 
> Too many unneeded characters. Why parentheses? Why negative check?
> 
Sure, will update in next version.

> ...
> 
> > +	if (early) {
> > +		ret = 0;
> > +		kobject_init(&swnode->kobj, &software_node_type_early);
> > +		swnode->kobj.parent = parent ? &parent->kobj : NULL;
> > +		if (node->name)
> > +			ret = kobject_set_name(&swnode->kobj,
> > +					       "%s", node->name);
> > +		else
> > +			ret = kobject_set_name(&swnode->kobj,
> > +					       "node%d", swnode->id);
> > +		if (!ret) {
> > +			spin_lock(&swnode_early_lock);
> > +			list_add_tail(&swnode->early, &swnode_early_list);
> > +			spin_unlock(&swnode_early_lock);
> > +		}
> > +	} else {
> > +		if (node->name)
> > +			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> > +						   parent ? &parent->kobj : NULL,
> 
> This looks like have a duplication.
> 
> > +						   "%s", node->name);
> > +		else
> > +			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> > +						   parent ? &parent->kobj : NULL,
> > +						   "node%d", swnode->id);
> > +	}
> 
> Maybe it's possible to refactor this piece to be more compact?
> 
The issue is, kobject_init_and_add() expects sysfs. Let me try to
compact this in next version. Thanks!
> ...
> 
> > -	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0));
> > +	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0));
> 
> In one case you use boolean, here is unsigned int for early flag, why is the
> inconsistency added?
> 
Yeah, it should be bool. Let me fix it in next version.

> ...
> 
> > -struct fwnode_handle *
> > -fwnode_create_software_node(const struct property_entry *properties,
> > -			    const struct fwnode_handle *parent)
> > +static struct fwnode_handle *
> > +fwnode_create_software_node_common(const struct property_entry *properties,
> > +				   const struct fwnode_handle *parent,
> > +				   bool early)
> 
> Why would you need this API in early stages?
> 
Hope I answered the question above. 

Thanks!
Sunil

Re: [RFC PATCH v1 11/21] swnode: Add support to create early during boot

[Marc Zyngier]

On Fri, 04 Aug 2023 09:11:05 +0100,
Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> Hi Andy,
> 
> On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote:
> > > From: Anup Patel <apatel@ventanamicro.com>
> > > 
> > > swnode framework can be used to create fwnode for interrupt
> > > controllers.
> > 
> > Why? What is this for?
> > Can you elaborate? This commit message is poorly written...
> > 
> > And why firmware node is not enough for ACPI case?
> > I assume the fwnode in DT case is already provided by OF.
> > 
> Thanks a lot for the review!.
> 
> You are right, OF provides the fwnode for irqchip drivers. However, for
> ACPI case, it is typically created using irq_domain_alloc_named_fwnode
> or irq_domain_alloc_fwnode since these are not ACPI devices in the
> namespace but from MADT. The fwnode created using
> irq_domain_alloc_fwnode() is a simple one which doesn't support properties
> similar to the one created by OF framework or software node framework.
> Hence, lot of data from the MADT structures need to be cached as
> separate structures in the drivers and also would need several ifdefs to
> check for ACPI and some amount of code duplication is also required due
> to the way DT driver gets the information vs ACPI.
> 
> The beauty of software node framework is, it supports adding properties
> and also is a supported fwnode type in __irq_domain_create().

There is no beauty here. Only some extra bloat that we do not need.

DT and ACPI exposes very different attributes. One describe the HW,
the other one describe an OS abstraction. Pretending that you can
summon both into the same infrastructure is a fallacy. You'll just end
up with the cross product of both infrastructure, and pollute the rest
of the kernel with pointless cruft.

> So, if we
> can create the fwnode for these irqchip using software node, we can
> attach the same properties and the actual irqchip driver which uses the
> fwnode doesn't need to have any ACPI vs DT checks. Same driver will work
> seamlessly on both DT and ACPI platforms.  But the challenge is,
> currently swnode expects to be created with sysfs which won't be
> available during early boot when irqchip drivers need to be probed. So,
> adding support to create without dependency on sysfs help us to reuse
> the same framework for irqchip use case also.

That's another fallacy.

Most irqchips *DO NOT* need to be probed early. Only the root
irqchip. Given that this series is about *secondary* interrupt
controllers, they absolutely don't need to be probed early.

To be clear: I do not intend to merge anything that:

- invents yet another way to "abstract" firmware interfaces

- adds more "early probe" hacks for non-primary interrupt controllers

I have already said that in response to Anup's AIA series, and this
equally applies to this series.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH v1 11/21] swnode: Add support to create early during boot

[Sunil V L]

On Tue, Aug 08, 2023 at 02:17:22PM +0100, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 09:11:05 +0100,
> Sunil V L <sunilvl@ventanamicro.com> wrote:
> > 
> > Hi Andy,
> > 
> > On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote:
> > > > From: Anup Patel <apatel@ventanamicro.com>
> > > > 
> > > > swnode framework can be used to create fwnode for interrupt
> > > > controllers.
> > > 
> > > Why? What is this for?
> > > Can you elaborate? This commit message is poorly written...
> > > 
> > > And why firmware node is not enough for ACPI case?
> > > I assume the fwnode in DT case is already provided by OF.
> > > 
> > Thanks a lot for the review!.
> > 
> > You are right, OF provides the fwnode for irqchip drivers. However, for
> > ACPI case, it is typically created using irq_domain_alloc_named_fwnode
> > or irq_domain_alloc_fwnode since these are not ACPI devices in the
> > namespace but from MADT. The fwnode created using
> > irq_domain_alloc_fwnode() is a simple one which doesn't support properties
> > similar to the one created by OF framework or software node framework.
> > Hence, lot of data from the MADT structures need to be cached as
> > separate structures in the drivers and also would need several ifdefs to
> > check for ACPI and some amount of code duplication is also required due
> > to the way DT driver gets the information vs ACPI.
> > 
> > The beauty of software node framework is, it supports adding properties
> > and also is a supported fwnode type in __irq_domain_create().
> 
> There is no beauty here. Only some extra bloat that we do not need.
> 
> DT and ACPI exposes very different attributes. One describe the HW,
> the other one describe an OS abstraction. Pretending that you can
> summon both into the same infrastructure is a fallacy. You'll just end
> up with the cross product of both infrastructure, and pollute the rest
> of the kernel with pointless cruft.
> 
Hi Marc,

Thank you very much for the feedback!. Sure, let me revert this approach
and do as you recommended in next version.

> > So, if we
> > can create the fwnode for these irqchip using software node, we can
> > attach the same properties and the actual irqchip driver which uses the
> > fwnode doesn't need to have any ACPI vs DT checks. Same driver will work
> > seamlessly on both DT and ACPI platforms.  But the challenge is,
> > currently swnode expects to be created with sysfs which won't be
> > available during early boot when irqchip drivers need to be probed. So,
> > adding support to create without dependency on sysfs help us to reuse
> > the same framework for irqchip use case also.
> 
> That's another fallacy.
> 
> Most irqchips *DO NOT* need to be probed early. Only the root
> irqchip. Given that this series is about *secondary* interrupt
> controllers, they absolutely don't need to be probed early.
>
Since we created swnode for root irqchip also in this approach, we had
to support early creation. With your feedback, this is no longer
required.

> To be clear: I do not intend to merge anything that:
> 
> - invents yet another way to "abstract" firmware interfaces
> 
> - adds more "early probe" hacks for non-primary interrupt controllers
> 
> I have already said that in response to Anup's AIA series, and this
> equally applies to this series.
>
In Anup's AIA v7 series, he has made non-primary controller drivers as
platform drivers which are not probed early.

Thanks,
Sunil

Re: [RFC PATCH v1 11/21] swnode: Add support to create early during boot

[Marc Zyngier]

On Thu, 03 Aug 2023 18:59:06 +0100,
Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> From: Anup Patel <apatel@ventanamicro.com>
> 
> swnode framework can be used to create fwnode for interrupt
> controllers. This helps in keeping the drivers same for both
> DT and ACPI. To enable this, enhance the swnode framework so
> that it can be created early during boot without dependency
> on sysfs.

Where is this coming from? We have had common abstractions for
irqchips for a very long time. We can create fwnodes any odd way we
want, but why oh why should we invent another method for that?

We already have multiple architectures such as arm64 and loongarch
already having both DT and ACPI. Do they need another level of
"abstraction"? No. Why is risc-v so special?

	M.

-- 
Without deviation from the norm, progress is not possible.

[RFC PATCH v1 12/21] irqchip/riscv-intc: Use swnode framework to create fwnode

[Sunil V L]

By using swnode framework, all data from ACPI tables can
be populated as properties of the swnode. This simplifies
the driver code and removes the need for ACPI vs DT checks.
Use this framework for RISC-V INTC driver.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 Documentation/riscv/acpi.rst     | 21 +++++++++++++++
 arch/riscv/include/asm/acpi.h    |  1 +
 drivers/acpi/riscv/Makefile      |  2 +-
 drivers/acpi/riscv/irqchip.c     | 46 ++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-riscv-intc.c | 12 ++++-----
 5 files changed, 75 insertions(+), 7 deletions(-)
 create mode 100644 drivers/acpi/riscv/irqchip.c

diff --git a/Documentation/riscv/acpi.rst b/Documentation/riscv/acpi.rst
index 9870a282815b..e2406546bc16 100644
--- a/Documentation/riscv/acpi.rst
+++ b/Documentation/riscv/acpi.rst
@@ -8,3 +8,24 @@ The ISA string parsing rules for ACPI are defined by `Version ASCIIDOC
 Conversion, 12/2022 of the RISC-V specifications, as defined by tag
 "riscv-isa-release-1239329-2023-05-23" (commit 1239329
 ) <https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-1239329-2023-05-23>`_
+
+Interrupt Controller Drivers
+=======
+
+ACPI drivers for RISC-V interrupt controllers use software node framework to
+create the fwnode for the interrupt controllers. Below properties are
+additionally required for some firmware nodes apart from the properties
+defined by the device tree bindings for these interrupt controllers. The
+properties are created using the data in MADT table.
+
+1) RISC-V Interrupt Controller (INTC)
+-----------
+``hartid`` - Hart ID of the hart this interrupt controller belongs to.
+
+``riscv,imsic-addr`` - Physical base address of the Incoming MSI Controller
+(IMSIC) MMIO region of this hart.
+
+``riscv,imsic-size`` - Size in bytes of the IMSIC MMIO region of this hart.
+
+``riscv,ext-intc-id`` - The unique ID of the external interrupts connected
+to this hart.
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 0c4e8b35103e..0ac2df2dd194 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -68,6 +68,7 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
 static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
 int acpi_get_cbo_block_size(struct acpi_table_header *table, unsigned int cpu, u32 *cbom_size,
 			    u32 *cboz_size, u32 *cbop_size);
+struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc);
 #else
 static inline void acpi_init_rintc_map(void) { }
 static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
diff --git a/drivers/acpi/riscv/Makefile b/drivers/acpi/riscv/Makefile
index 8b3b126e0b94..8b664190d172 100644
--- a/drivers/acpi/riscv/Makefile
+++ b/drivers/acpi/riscv/Makefile
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y 	+= rhct.o
+obj-y 	+= rhct.o irqchip.o
diff --git a/drivers/acpi/riscv/irqchip.c b/drivers/acpi/riscv/irqchip.c
new file mode 100644
index 000000000000..36f066a2cad5
--- /dev/null
+++ b/drivers/acpi/riscv/irqchip.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023, Ventana Micro Systems Inc
+ *	Author: Sunil V L <sunilvl@ventanamicro.com>
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/fwnode.h>
+#include <linux/irqdomain.h>
+#include <linux/list.h>
+#include <linux/property.h>
+
+struct riscv_irqchip_list {
+	struct fwnode_handle *fwnode;
+	struct list_head list;
+};
+
+LIST_HEAD(rintc_list);
+
+struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc)
+{
+	struct property_entry props[6] = {};
+	struct fwnode_handle *fwnode;
+	struct riscv_irqchip_list *rintc_element;
+
+	props[0] = PROPERTY_ENTRY_U64("hartid", rintc->hart_id);
+	props[1] = PROPERTY_ENTRY_U32("riscv,ext-intc-id", rintc->ext_intc_id);
+	props[2] = PROPERTY_ENTRY_U64("riscv,imsic-addr", rintc->imsic_addr);
+	props[3] = PROPERTY_ENTRY_U32("riscv,imsic-size", rintc->imsic_size);
+	props[4] = PROPERTY_ENTRY_U32("#interrupt-cells", 1);
+
+	fwnode = fwnode_create_software_node_early(props, NULL);
+	if (fwnode) {
+		rintc_element = kzalloc(sizeof(*rintc_element), GFP_KERNEL);
+		if (!rintc_element) {
+			fwnode_remove_software_node(fwnode);
+			return NULL;
+		}
+
+		rintc_element->fwnode = fwnode;
+		list_add_tail(&rintc_element->list, &rintc_list);
+	}
+
+	return fwnode;
+}
diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index 1a0fc87152c5..1ef9cada1ed3 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -203,6 +203,12 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
 
 	rintc = (struct acpi_madt_rintc *)header;
 
+	fn = acpi_rintc_create_irqchip_fwnode(rintc);
+	if (!fn) {
+		pr_err("unable to create INTC FW node\n");
+		return -ENOMEM;
+	}
+
 	/*
 	 * The ACPI MADT will have one INTC for each CPU (or HART)
 	 * so riscv_intc_acpi_init() function will be called once
@@ -212,12 +218,6 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
 	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
 		return 0;
 
-	fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
-	if (!fn) {
-		pr_err("unable to allocate INTC FW node\n");
-		return -ENOMEM;
-	}
-
 	return riscv_intc_init_common(fn);
 }
 
-- 
2.39.2

Re: [RFC PATCH v1 12/21] irqchip/riscv-intc: Use swnode framework to create fwnode

[Conor Dooley]

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

Hey Sunil,

On Thu, Aug 03, 2023 at 11:29:07PM +0530, Sunil V L wrote:
> By using swnode framework, all data from ACPI tables can
> be populated as properties of the swnode. This simplifies
> the driver code and removes the need for ACPI vs DT checks.
> Use this framework for RISC-V INTC driver.

btw, you are permitted to use more than 60 characters in a commit
message...

> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  Documentation/riscv/acpi.rst     | 21 +++++++++++++++
>  arch/riscv/include/asm/acpi.h    |  1 +
>  drivers/acpi/riscv/Makefile      |  2 +-
>  drivers/acpi/riscv/irqchip.c     | 46 ++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-riscv-intc.c | 12 ++++-----
>  5 files changed, 75 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/acpi/riscv/irqchip.c
> 
> diff --git a/Documentation/riscv/acpi.rst b/Documentation/riscv/acpi.rst
> index 9870a282815b..e2406546bc16 100644
> --- a/Documentation/riscv/acpi.rst
> +++ b/Documentation/riscv/acpi.rst
> @@ -8,3 +8,24 @@ The ISA string parsing rules for ACPI are defined by `Version ASCIIDOC
>  Conversion, 12/2022 of the RISC-V specifications, as defined by tag
>  "riscv-isa-release-1239329-2023-05-23" (commit 1239329
>  ) <https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-1239329-2023-05-23>`_
> +
> +Interrupt Controller Drivers
> +=======
> +
> +ACPI drivers for RISC-V interrupt controllers use software node framework to
> +create the fwnode for the interrupt controllers. Below properties are
> +additionally required for some firmware nodes apart from the properties
> +defined by the device tree bindings for these interrupt controllers. The
> +properties are created using the data in MADT table.

I don't really understand this text, specifically what you are getting
at w/ the dependency on devicetree properties. What exactly does "apart
from the properties defined by the devicetree bindings" mean?

Is there prior art for this kind of "ACPI needs swnodes that look
vaguely similar to devicetree" for other interrupt controllers?

Thanks,
Conor.

> +1) RISC-V Interrupt Controller (INTC)
> +-----------
> +``hartid`` - Hart ID of the hart this interrupt controller belongs to.
> +
> +``riscv,imsic-addr`` - Physical base address of the Incoming MSI Controller
> +(IMSIC) MMIO region of this hart.
> +
> +``riscv,imsic-size`` - Size in bytes of the IMSIC MMIO region of this hart.
> +
> +``riscv,ext-intc-id`` - The unique ID of the external interrupts connected
> +to this hart.
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index 0c4e8b35103e..0ac2df2dd194 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -68,6 +68,7 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
>  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
>  int acpi_get_cbo_block_size(struct acpi_table_header *table, unsigned int cpu, u32 *cbom_size,
>  			    u32 *cboz_size, u32 *cbop_size);
> +struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc);
>  #else
>  static inline void acpi_init_rintc_map(void) { }
>  static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> diff --git a/drivers/acpi/riscv/Makefile b/drivers/acpi/riscv/Makefile
> index 8b3b126e0b94..8b664190d172 100644
> --- a/drivers/acpi/riscv/Makefile
> +++ b/drivers/acpi/riscv/Makefile
> @@ -1,2 +1,2 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -obj-y 	+= rhct.o
> +obj-y 	+= rhct.o irqchip.o
> diff --git a/drivers/acpi/riscv/irqchip.c b/drivers/acpi/riscv/irqchip.c
> new file mode 100644
> index 000000000000..36f066a2cad5
> --- /dev/null
> +++ b/drivers/acpi/riscv/irqchip.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023, Ventana Micro Systems Inc
> + *	Author: Sunil V L <sunilvl@ventanamicro.com>
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/fwnode.h>
> +#include <linux/irqdomain.h>
> +#include <linux/list.h>
> +#include <linux/property.h>
> +
> +struct riscv_irqchip_list {
> +	struct fwnode_handle *fwnode;
> +	struct list_head list;
> +};
> +
> +LIST_HEAD(rintc_list);
> +
> +struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc)
> +{
> +	struct property_entry props[6] = {};
> +	struct fwnode_handle *fwnode;
> +	struct riscv_irqchip_list *rintc_element;
> +
> +	props[0] = PROPERTY_ENTRY_U64("hartid", rintc->hart_id);
> +	props[1] = PROPERTY_ENTRY_U32("riscv,ext-intc-id", rintc->ext_intc_id);
> +	props[2] = PROPERTY_ENTRY_U64("riscv,imsic-addr", rintc->imsic_addr);
> +	props[3] = PROPERTY_ENTRY_U32("riscv,imsic-size", rintc->imsic_size);
> +	props[4] = PROPERTY_ENTRY_U32("#interrupt-cells", 1);
> +
> +	fwnode = fwnode_create_software_node_early(props, NULL);
> +	if (fwnode) {
> +		rintc_element = kzalloc(sizeof(*rintc_element), GFP_KERNEL);
> +		if (!rintc_element) {
> +			fwnode_remove_software_node(fwnode);
> +			return NULL;
> +		}
> +
> +		rintc_element->fwnode = fwnode;
> +		list_add_tail(&rintc_element->list, &rintc_list);
> +	}
> +
> +	return fwnode;
> +}
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index 1a0fc87152c5..1ef9cada1ed3 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -203,6 +203,12 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  
>  	rintc = (struct acpi_madt_rintc *)header;
>  
> +	fn = acpi_rintc_create_irqchip_fwnode(rintc);
> +	if (!fn) {
> +		pr_err("unable to create INTC FW node\n");
> +		return -ENOMEM;
> +	}
> +
>  	/*
>  	 * The ACPI MADT will have one INTC for each CPU (or HART)
>  	 * so riscv_intc_acpi_init() function will be called once
> @@ -212,12 +218,6 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  	if (riscv_hartid_to_cpuid(rintc->hart_id) != smp_processor_id())
>  		return 0;
>  
> -	fn = irq_domain_alloc_named_fwnode("RISCV-INTC");
> -	if (!fn) {
> -		pr_err("unable to allocate INTC FW node\n");
> -		return -ENOMEM;
> -	}
> -
>  	return riscv_intc_init_common(fn);
>  }
>  
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> 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: [RFC PATCH v1 12/21] irqchip/riscv-intc: Use swnode framework to create fwnode

[Sunil V L]

On Tue, Aug 08, 2023 at 09:31:49AM +0100, Conor Dooley wrote:
> Hey Sunil,
> 
> On Thu, Aug 03, 2023 at 11:29:07PM +0530, Sunil V L wrote:
> > By using swnode framework, all data from ACPI tables can
> > be populated as properties of the swnode. This simplifies
> > the driver code and removes the need for ACPI vs DT checks.
> > Use this framework for RISC-V INTC driver.
> 
> btw, you are permitted to use more than 60 characters in a commit
> message...
> 
Sure.

> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  Documentation/riscv/acpi.rst     | 21 +++++++++++++++
> >  arch/riscv/include/asm/acpi.h    |  1 +
> >  drivers/acpi/riscv/Makefile      |  2 +-
> >  drivers/acpi/riscv/irqchip.c     | 46 ++++++++++++++++++++++++++++++++
> >  drivers/irqchip/irq-riscv-intc.c | 12 ++++-----
> >  5 files changed, 75 insertions(+), 7 deletions(-)
> >  create mode 100644 drivers/acpi/riscv/irqchip.c
> > 
> > diff --git a/Documentation/riscv/acpi.rst b/Documentation/riscv/acpi.rst
> > index 9870a282815b..e2406546bc16 100644
> > --- a/Documentation/riscv/acpi.rst
> > +++ b/Documentation/riscv/acpi.rst
> > @@ -8,3 +8,24 @@ The ISA string parsing rules for ACPI are defined by `Version ASCIIDOC
> >  Conversion, 12/2022 of the RISC-V specifications, as defined by tag
> >  "riscv-isa-release-1239329-2023-05-23" (commit 1239329
> >  ) <https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-isa-release-1239329-2023-05-23>`_
> > +
> > +Interrupt Controller Drivers
> > +=======
> > +
> > +ACPI drivers for RISC-V interrupt controllers use software node framework to
> > +create the fwnode for the interrupt controllers. Below properties are
> > +additionally required for some firmware nodes apart from the properties
> > +defined by the device tree bindings for these interrupt controllers. The
> > +properties are created using the data in MADT table.
> 
> I don't really understand this text, specifically what you are getting
> at w/ the dependency on devicetree properties. What exactly does "apart
> from the properties defined by the devicetree bindings" mean?
> 
> Is there prior art for this kind of "ACPI needs swnodes that look
> vaguely similar to devicetree" for other interrupt controllers?
>
Never mind. This will not be required with Marc's feedback.

Thanks,
Sunil 

[RFC PATCH v1 13/21] irqchip/riscv-imsic-early: Add ACPI support

[Sunil V L]

Add support to probe the IMSIC early driver on ACPI
based RISC-V platforms.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h           |  6 +++
 drivers/acpi/riscv/irqchip.c            | 57 +++++++++++++++++++++++++
 drivers/irqchip/irq-riscv-imsic-early.c | 28 ++++++++++++
 drivers/irqchip/irq-riscv-imsic-state.c | 33 +++++++++++---
 4 files changed, 119 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 0ac2df2dd194..6dde3d63dc0e 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -69,6 +69,8 @@ static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
 int acpi_get_cbo_block_size(struct acpi_table_header *table, unsigned int cpu, u32 *cbom_size,
 			    u32 *cboz_size, u32 *cbop_size);
 struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc);
+struct fwnode_handle *acpi_imsic_create_fwnode(struct acpi_madt_imsic *imsic);
+struct fwnode_handle *acpi_riscv_get_msi_fwnode(struct device *dev);
 #else
 static inline void acpi_init_rintc_map(void) { }
 static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
@@ -89,6 +91,10 @@ static inline int acpi_get_cbo_block_size(struct acpi_table_header *table,
 	return -EINVAL;
 }
 
+static inline struct fwnode_handle *acpi_riscv_get_msi_fwnode(struct device *dev)
+{
+	return NULL;
+}
 #endif /* CONFIG_ACPI */
 
 #endif /*_ASM_ACPI_H*/
diff --git a/drivers/acpi/riscv/irqchip.c b/drivers/acpi/riscv/irqchip.c
index 36f066a2cad5..6e15d45cb229 100644
--- a/drivers/acpi/riscv/irqchip.c
+++ b/drivers/acpi/riscv/irqchip.c
@@ -18,6 +18,8 @@ struct riscv_irqchip_list {
 
 LIST_HEAD(rintc_list);
 
+static struct fwnode_handle *imsic_acpi_fwnode;
+
 struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc)
 {
 	struct property_entry props[6] = {};
@@ -44,3 +46,58 @@ struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *r
 
 	return fwnode;
 }
+
+static struct fwnode_handle *acpi_imsic_get_rintc_fwnode(u32 idx)
+{
+	struct riscv_irqchip_list *rintc_element;
+	struct fwnode_handle *fwnode;
+	struct list_head *i, *tmp;
+	unsigned int j = 0;
+
+	list_for_each_safe(i, tmp, &rintc_list) {
+		rintc_element = list_entry(i, struct riscv_irqchip_list, list);
+		fwnode = rintc_element->fwnode;
+
+		if (j == idx)
+			return fwnode;
+
+		j++;
+	}
+
+	return NULL;
+}
+
+struct fwnode_handle *acpi_imsic_create_fwnode(struct acpi_madt_imsic *imsic)
+{
+	struct property_entry props[8] = {};
+	struct software_node_ref_args *refs;
+	struct fwnode_handle *parent_fwnode;
+	unsigned int nr_rintc, i;
+
+	props[0] = PROPERTY_ENTRY_U32("riscv,guest-index-bits", imsic->guest_index_bits);
+	props[1] = PROPERTY_ENTRY_U32("riscv,hart-index-bits", imsic->hart_index_bits);
+	props[2] = PROPERTY_ENTRY_U32("riscv,group-index-bits", imsic->group_index_bits);
+	props[3] = PROPERTY_ENTRY_U32("riscv,group-index-shift", imsic->group_index_shift);
+	props[4] = PROPERTY_ENTRY_U32("riscv,num-ids", imsic->num_ids);
+	props[5] = PROPERTY_ENTRY_U32("riscv,num-guest-ids", imsic->num_guest_ids);
+
+	nr_rintc = list_count_nodes(&rintc_list);
+	refs = kcalloc(nr_rintc, sizeof(*refs), GFP_KERNEL);
+	if (!refs)
+		return NULL;
+
+	for (i = 0; i < nr_rintc; i++) {
+		parent_fwnode = acpi_imsic_get_rintc_fwnode(i);
+		refs[i] = SOFTWARE_NODE_REFERENCE(to_software_node(parent_fwnode), RV_IRQ_EXT);
+	}
+	props[6] = PROPERTY_ENTRY_REF_ARRAY_LEN("interrupts-extended", refs, nr_rintc);
+
+	imsic_acpi_fwnode = fwnode_create_software_node_early(props, NULL);
+
+	return imsic_acpi_fwnode;
+}
+
+struct fwnode_handle *acpi_riscv_get_msi_fwnode(struct device *dev)
+{
+	return imsic_acpi_fwnode;
+}
diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
index 1de89ce1ec2f..93f4d748ca6d 100644
--- a/drivers/irqchip/irq-riscv-imsic-early.c
+++ b/drivers/irqchip/irq-riscv-imsic-early.c
@@ -12,6 +12,7 @@
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include <linux/spinlock.h>
 #include <linux/smp.h>
 
@@ -256,3 +257,30 @@ static int __init imsic_early_dt_init(struct device_node *node,
 	return 0;
 }
 IRQCHIP_DECLARE(riscv_imsic, "riscv,imsics", imsic_early_dt_init);
+
+#ifdef CONFIG_ACPI
+static int __init imsic_early_acpi_init(union acpi_subtable_headers *header,
+					const unsigned long end)
+{
+	struct fwnode_handle *fwnode;
+	int rc;
+
+	/*
+	 * There should be only one IMSIC node.
+	 */
+	fwnode = acpi_imsic_create_fwnode((struct acpi_madt_imsic *)header);
+	if (!fwnode) {
+		pr_err("unable to create IMSIC FW node\n");
+		return -ENOMEM;
+	}
+
+	rc = imsic_early_probe(fwnode);
+	if (!rc)
+		pci_msi_register_fwnode_provider(&acpi_riscv_get_msi_fwnode);
+
+	return rc;
+}
+
+IRQCHIP_ACPI_DECLARE(riscv_imsic, ACPI_MADT_TYPE_IMSIC,
+		     NULL, 1, imsic_early_acpi_init);
+#endif
diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
index 412b5b919dcc..d0e09e51e8ae 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.c
+++ b/drivers/irqchip/irq-riscv-imsic-state.c
@@ -225,15 +225,38 @@ static int __init imsic_get_parent_hartid(struct fwnode_handle *fwnode,
 	return riscv_get_intc_hartid(parent.fwnode, hartid);
 }
 
+static int __init imsic_acpi_get_mmio_resource(struct fwnode_handle *fwnode,
+					       u32 index, struct resource *res)
+{
+	int rc;
+	struct fwnode_reference_args parent;
+	u64 base;
+	u32 size;
+
+	rc = fwnode_property_get_reference_args(fwnode,
+						"interrupts-extended", NULL,
+						0, index, &parent);
+	if (rc)
+		return rc;
+
+	rc = fwnode_property_read_u64_array(parent.fwnode, "riscv,imsic-addr",
+					    &base, 1);
+	rc = fwnode_property_read_u32_array(parent.fwnode, "riscv,imsic-size",
+					    &size, 1);
+	if (!rc) {
+		res->start = base;
+		res->end = res->start + size - 1;
+	}
+
+	return 0;
+}
+
 static int __init imsic_get_mmio_resource(struct fwnode_handle *fwnode,
 					  u32 index, struct resource *res)
 {
-	/*
-	 * Currently, only OF fwnode is support so extend this function
-	 * for other types of fwnode for ACPI support.
-	 */
 	if (!is_of_node(fwnode))
-		return -EINVAL;
+		return imsic_acpi_get_mmio_resource(fwnode, index, res);
+
 	return of_address_to_resource(to_of_node(fwnode), index, res);
 }
 
-- 
2.39.2

[RFC PATCH v1 14/21] ACPI: bus: Add acpi_riscv_init function

[Sunil V L]

Add a new function for RISC-V to do any architecture
specific initialization. This function will be used
to create platform devices like APLIC, IMSIC MSI controller,
RISC-V IOMMU etc.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/acpi/bus.c          |  1 +
 drivers/acpi/riscv/Makefile |  2 +-
 drivers/acpi/riscv/init.c   | 12 ++++++++++++
 include/linux/acpi.h        |  6 ++++++
 4 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 drivers/acpi/riscv/init.c

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 2fc2b43a4ed3..9a8c16170a4b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1388,6 +1388,7 @@ static int __init acpi_init(void)
 
 	pci_mmcfg_late_init();
 	acpi_arm_init();
+	acpi_riscv_init();
 	acpi_viot_early_init();
 	acpi_hest_init();
 	acpi_ghes_init();
diff --git a/drivers/acpi/riscv/Makefile b/drivers/acpi/riscv/Makefile
index 8b664190d172..3433a19c421d 100644
--- a/drivers/acpi/riscv/Makefile
+++ b/drivers/acpi/riscv/Makefile
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y 	+= rhct.o irqchip.o
+obj-y 	+= rhct.o irqchip.o init.o
diff --git a/drivers/acpi/riscv/init.c b/drivers/acpi/riscv/init.c
new file mode 100644
index 000000000000..b5807bbdb171
--- /dev/null
+++ b/drivers/acpi/riscv/init.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023, Ventana Micro Systems Inc
+ *	Author: Sunil V L <sunilvl@ventanamicro.com>
+ *
+ */
+
+#include <linux/acpi.h>
+
+void __init acpi_riscv_init(void)
+{
+}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 641dc4843987..d16739928888 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1511,6 +1511,12 @@ void acpi_arm_init(void);
 static inline void acpi_arm_init(void) { }
 #endif
 
+#ifdef CONFIG_RISCV
+void acpi_riscv_init(void);
+#else
+static inline void acpi_riscv_init(void) { }
+#endif
+
 #ifdef CONFIG_ACPI_PCC
 void acpi_init_pcc(void);
 #else
-- 
2.39.2

[RFC PATCH v1 15/21] ACPI: RISC-V: Create IMSIC platform device

[Sunil V L]

The MSI controller functionality of the RISC-V IMSIC is
probed as a platform device by the driver. So, create the
IMSIC platform device if the IMSIC was discovered in MADT
during early IMSIC driver probe.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/acpi/riscv/init.c    |  2 ++
 drivers/acpi/riscv/init.h    |  4 ++++
 drivers/acpi/riscv/irqchip.c | 23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+)
 create mode 100644 drivers/acpi/riscv/init.h

diff --git a/drivers/acpi/riscv/init.c b/drivers/acpi/riscv/init.c
index b5807bbdb171..be61c08ea385 100644
--- a/drivers/acpi/riscv/init.c
+++ b/drivers/acpi/riscv/init.c
@@ -6,7 +6,9 @@
  */
 
 #include <linux/acpi.h>
+#include "init.h"
 
 void __init acpi_riscv_init(void)
 {
+	riscv_acpi_imsic_platform_init();
 }
diff --git a/drivers/acpi/riscv/init.h b/drivers/acpi/riscv/init.h
new file mode 100644
index 000000000000..a2f72bb294d3
--- /dev/null
+++ b/drivers/acpi/riscv/init.h
@@ -0,0 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <linux/init.h>
+
+void __init riscv_acpi_imsic_platform_init(void);
diff --git a/drivers/acpi/riscv/irqchip.c b/drivers/acpi/riscv/irqchip.c
index 6e15d45cb229..ca96bf109cf7 100644
--- a/drivers/acpi/riscv/irqchip.c
+++ b/drivers/acpi/riscv/irqchip.c
@@ -9,7 +9,10 @@
 #include <linux/fwnode.h>
 #include <linux/irqdomain.h>
 #include <linux/list.h>
+#include <linux/msi.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
+#include "../../../drivers/pci/pci.h"
 
 struct riscv_irqchip_list {
 	struct fwnode_handle *fwnode;
@@ -101,3 +104,23 @@ struct fwnode_handle *acpi_riscv_get_msi_fwnode(struct device *dev)
 {
 	return imsic_acpi_fwnode;
 }
+
+void __init riscv_acpi_imsic_platform_init(void)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	if (!acpi_riscv_get_msi_fwnode(NULL)) {
+		pci_no_msi();
+		return;
+	}
+
+	pdev = platform_device_alloc("riscv-imsic", 0);
+	if (!pdev)
+		return;
+
+	pdev->dev.fwnode = acpi_riscv_get_msi_fwnode(NULL);
+	ret = platform_device_add(pdev);
+	if (ret)
+		platform_device_put(pdev);
+}
-- 
2.39.2

[RFC PATCH v1 16/21] ACPI: Add APLIC IRQ model for RISC-V

[Sunil V L]

Add the IRQ model for RISC-V APLIC so that acpi_set_irq_model
can use this for RISC-V.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/acpi/bus.c   | 3 +++
 include/linux/acpi.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 9a8c16170a4b..dd9d80630bf6 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1132,6 +1132,9 @@ static int __init acpi_bus_init_irq(void)
 	case ACPI_IRQ_MODEL_LPIC:
 		message = "LPIC";
 		break;
+	case ACPI_IRQ_MODEL_APLIC:
+		message = "APLIC";
+		break;
 	default:
 		pr_info("Unknown interrupt routing model\n");
 		return -ENODEV;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d16739928888..698d120a1bd2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -95,6 +95,7 @@ enum acpi_irq_model_id {
 	ACPI_IRQ_MODEL_PLATFORM,
 	ACPI_IRQ_MODEL_GIC,
 	ACPI_IRQ_MODEL_LPIC,
+	ACPI_IRQ_MODEL_APLIC,
 	ACPI_IRQ_MODEL_COUNT
 };
 
-- 
2.39.2

[RFC PATCH v1 17/21] ACPI: RISC-V: Create APLIC platform device

[Sunil V L]

Since APLIC needs to be a platform device, probe the
MADT and create platform devices for each APLIC in the
system. Use software node framework for the fwnode
which allows to create properties and hence the
actual irqchip driver doesn't need to do anything
different for ACPI vs DT.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 Documentation/riscv/acpi.rst |   6 ++
 drivers/acpi/riscv/init.c    |   1 +
 drivers/acpi/riscv/init.h    |   1 +
 drivers/acpi/riscv/irqchip.c | 183 +++++++++++++++++++++++++++++++++++
 4 files changed, 191 insertions(+)

diff --git a/Documentation/riscv/acpi.rst b/Documentation/riscv/acpi.rst
index e2406546bc16..9ea9008288ea 100644
--- a/Documentation/riscv/acpi.rst
+++ b/Documentation/riscv/acpi.rst
@@ -29,3 +29,9 @@ properties are created using the data in MADT table.
 
 ``riscv,ext-intc-id`` - The unique ID of the external interrupts connected
 to this hart.
+
+2) RISC-V Advanced Platform Level Interrupt Controller (APLIC)
+-----------
+
+``riscv,gsi-base`` - The global system interrupt number where this APLIC’s
+interrupt inputs start.
diff --git a/drivers/acpi/riscv/init.c b/drivers/acpi/riscv/init.c
index be61c08ea385..ee747211174f 100644
--- a/drivers/acpi/riscv/init.c
+++ b/drivers/acpi/riscv/init.c
@@ -11,4 +11,5 @@
 void __init acpi_riscv_init(void)
 {
 	riscv_acpi_imsic_platform_init();
+	riscv_acpi_aplic_platform_init();
 }
diff --git a/drivers/acpi/riscv/init.h b/drivers/acpi/riscv/init.h
index a2f72bb294d3..17bcf0baaadb 100644
--- a/drivers/acpi/riscv/init.h
+++ b/drivers/acpi/riscv/init.h
@@ -2,3 +2,4 @@
 #include <linux/init.h>
 
 void __init riscv_acpi_imsic_platform_init(void);
+void __init riscv_acpi_aplic_platform_init(void);
diff --git a/drivers/acpi/riscv/irqchip.c b/drivers/acpi/riscv/irqchip.c
index ca96bf109cf7..7fb7befdb303 100644
--- a/drivers/acpi/riscv/irqchip.c
+++ b/drivers/acpi/riscv/irqchip.c
@@ -23,6 +23,8 @@ LIST_HEAD(rintc_list);
 
 static struct fwnode_handle *imsic_acpi_fwnode;
 
+LIST_HEAD(aplic_list);
+
 struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc)
 {
 	struct property_entry props[6] = {};
@@ -124,3 +126,184 @@ void __init riscv_acpi_imsic_platform_init(void)
 	if (ret)
 		platform_device_put(pdev);
 }
+
+/*
+ * The ext_intc_id format is as follows:
+ * Bits [31:24] APLIC/PLIC ID
+ * Bits [15:0] APLIC IDC ID / PLIC S-Mode Context ID for this hart
+ */
+#define APLIC_PLIC_ID(x) ((x) >> 24)
+#define IDC_CONTEXT_ID(x) ((x) & 0x0000ffff)
+
+static struct fwnode_handle *acpi_ext_intc_get_rintc_fwnode(u8 aplic_plic_id, u16 index)
+{
+	struct riscv_irqchip_list *rintc_element;
+	struct fwnode_handle *fwnode;
+	struct list_head *i, *tmp;
+	u32 id;
+	int rc;
+
+	list_for_each_safe(i, tmp, &rintc_list) {
+		rintc_element = list_entry(i, struct riscv_irqchip_list, list);
+		fwnode = rintc_element->fwnode;
+		rc = fwnode_property_read_u32_array(fwnode, "riscv,ext-intc-id", &id, 1);
+		if (rc)
+			continue;
+
+		if ((APLIC_PLIC_ID(id) == aplic_plic_id) && (IDC_CONTEXT_ID(id) == index))
+			return fwnode;
+	}
+
+	return NULL;
+}
+
+static struct fwnode_handle *acpi_aplic_create_fwnode(struct acpi_madt_aplic *aplic)
+{
+	struct fwnode_handle *fwnode, *parent_fwnode;
+	struct riscv_irqchip_list *aplic_element;
+	struct software_node_ref_args *refs;
+	struct property_entry props[8] = {};
+	unsigned int i;
+
+	props[0] = PROPERTY_ENTRY_U32("riscv,gsi-base", aplic->gsi_base);
+	props[1] = PROPERTY_ENTRY_U32("riscv,num-sources", aplic->num_sources);
+	props[2] = PROPERTY_ENTRY_U32("riscv,num-idcs", aplic->num_idcs);
+	props[3] = PROPERTY_ENTRY_U32("riscv,aplic-id", aplic->id);
+	props[4] = PROPERTY_ENTRY_U64("riscv,aplic-base", aplic->base_addr);
+	props[5] = PROPERTY_ENTRY_U32("riscv,aplic-size", aplic->size);
+	if (aplic->num_idcs) {
+		refs = kcalloc(aplic->num_idcs, sizeof(*refs), GFP_KERNEL);
+		if (!refs)
+			return NULL;
+
+		for (i = 0; i < aplic->num_idcs; i++) {
+			parent_fwnode = acpi_ext_intc_get_rintc_fwnode(aplic->id, i);
+			refs[i] = SOFTWARE_NODE_REFERENCE(to_software_node(parent_fwnode),
+							  RV_IRQ_EXT);
+		}
+		props[6] = PROPERTY_ENTRY_REF_ARRAY_LEN("interrupts-extended",
+							refs, aplic->num_idcs);
+	} else {
+		props[6] = PROPERTY_ENTRY_BOOL("msi-parent");
+	}
+
+	fwnode = fwnode_create_software_node_early(props, NULL);
+
+	if (fwnode) {
+		aplic_element = kzalloc(sizeof(*aplic_element), GFP_KERNEL);
+		if (!aplic_element) {
+			fwnode_remove_software_node(fwnode);
+			return NULL;
+		}
+
+		aplic_element->fwnode = fwnode;
+		list_add_tail(&aplic_element->list, &aplic_list);
+	}
+
+	return fwnode;
+}
+
+static struct fwnode_handle *aplic_get_gsi_domain_id(u32 gsi)
+{
+	struct riscv_irqchip_list *aplic_element;
+	struct fwnode_handle *fwnode;
+	struct list_head *i, *tmp;
+	u32 gsi_base;
+	u32 nr_irqs;
+	int rc;
+
+	list_for_each_safe(i, tmp, &aplic_list) {
+		aplic_element = list_entry(i, struct riscv_irqchip_list, list);
+		fwnode = aplic_element->fwnode;
+		rc = fwnode_property_read_u32_array(fwnode, "riscv,gsi-base", &gsi_base, 1);
+		if (!rc) {
+			rc = fwnode_property_read_u32_array(fwnode, "riscv,num-sources",
+							    &nr_irqs, 1);
+			if (!rc && (gsi >= gsi_base && gsi < gsi_base + nr_irqs))
+				return fwnode;
+		}
+	}
+
+	return NULL;
+}
+
+static u32 __init aplic_gsi_to_irq(u32 gsi)
+{
+	return acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
+}
+
+static int __init aplic_create_platform_device(struct fwnode_handle *fwnode)
+{
+	struct platform_device *pdev;
+	u32 aplic_size, aplic_id;
+	struct resource *res;
+	u64 aplic_base;
+	int ret;
+
+	if (!fwnode)
+		return -ENODEV;
+
+	ret = fwnode_property_read_u64_array(fwnode, "riscv,aplic-base", &aplic_base, 1);
+	if (ret)
+		return -ENODEV;
+
+	ret = fwnode_property_read_u32_array(fwnode, "riscv,aplic-size", &aplic_size, 1);
+	if (ret)
+		return -ENODEV;
+
+	ret = fwnode_property_read_u32_array(fwnode, "riscv,aplic-id", &aplic_id, 1);
+	if (ret)
+		return -ENODEV;
+
+	pdev = platform_device_alloc("riscv-aplic", aplic_id);
+	if (!pdev)
+		return -ENOMEM;
+
+	res = kcalloc(1, sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		ret = -ENOMEM;
+		goto dev_put;
+	}
+
+	res->start = aplic_base;
+	res->end = res->start + aplic_size - 1;
+	res->flags = IORESOURCE_MEM;
+	ret = platform_device_add_resources(pdev, res, 1);
+	/*
+	 * Resources are duplicated in platform_device_add_resources,
+	 * free their allocated memory
+	 */
+	kfree(res);
+
+	pdev->dev.fwnode = fwnode;
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto dev_put;
+
+	return 0;
+
+dev_put:
+	platform_device_put(pdev);
+	return ret;
+}
+
+static int __init aplic_parse_madt(union acpi_subtable_headers *header,
+				   const unsigned long end)
+{
+	struct acpi_madt_aplic *aplic = (struct acpi_madt_aplic *)header;
+	struct fwnode_handle *fwnode;
+
+	fwnode = acpi_aplic_create_fwnode(aplic);
+	if (fwnode)
+		aplic_create_platform_device(fwnode);
+
+	return 0;
+}
+
+void __init riscv_acpi_aplic_platform_init(void)
+{
+	if (acpi_table_parse_madt(ACPI_MADT_TYPE_APLIC, aplic_parse_madt, 0) > 0) {
+		acpi_set_irq_model(ACPI_IRQ_MODEL_APLIC, aplic_get_gsi_domain_id);
+		acpi_set_gsi_to_irq_fallback(aplic_gsi_to_irq);
+	}
+}
-- 
2.39.2

[RFC PATCH v1 18/21] irqchip/irq-riscv-aplic-msi: Add ACPI support

[Sunil V L]

Search and configure the MSI domain for the APLIC
on ACPI based systems.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/irqchip/irq-riscv-aplic-msi.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
index 086d00e0429e..1948444c9e0c 100644
--- a/drivers/irqchip/irq-riscv-aplic-msi.c
+++ b/drivers/irqchip/irq-riscv-aplic-msi.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/smp.h>
+#include <asm/acpi.h>
 
 #include "irq-riscv-aplic-main.h"
 
@@ -178,6 +179,8 @@ static void aplic_msi_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 int aplic_msi_setup(struct device *dev, void __iomem *regs)
 {
 	const struct imsic_global_config *imsic_global;
+	struct irq_domain *msi_domain = NULL;
+	struct fwnode_handle *msi_fwnode;
 	struct irq_domain *irqdomain;
 	struct aplic_priv *priv;
 	struct aplic_msicfg *mc;
@@ -261,8 +264,17 @@ int aplic_msi_setup(struct device *dev, void __iomem *regs)
 		 * IMSIC and the IMSIC MSI domains are created later through
 		 * the platform driver probing so we set it explicitly here.
 		 */
-		if (is_of_node(dev->fwnode))
+		if (is_of_node(dev->fwnode)) {
 			of_msi_configure(dev, to_of_node(dev->fwnode));
+		} else {
+			msi_fwnode = acpi_riscv_get_msi_fwnode(dev);
+			if (msi_fwnode)
+				msi_domain = irq_find_matching_fwnode(msi_fwnode,
+								      DOMAIN_BUS_PLATFORM_MSI);
+
+			if (msi_domain)
+				dev_set_msi_domain(dev, msi_domain);
+		}
 	}
 
 	/* Create irq domain instance for the APLIC MSI-mode */
-- 
2.39.2

[RFC PATCH v1 19/21] ACPI: bus: Add PLIC IRQ model

[Sunil V L]

Add the IRQ model for RISC-V PLIC so that acpi_set_irq_model
can use this for RISC-V.

Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/acpi/bus.c   | 3 +++
 include/linux/acpi.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index dd9d80630bf6..68b8d38ec48a 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1135,6 +1135,9 @@ static int __init acpi_bus_init_irq(void)
 	case ACPI_IRQ_MODEL_APLIC:
 		message = "APLIC";
 		break;
+	case ACPI_IRQ_MODEL_PLIC:
+		message = "PLIC";
+		break;
 	default:
 		pr_info("Unknown interrupt routing model\n");
 		return -ENODEV;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 698d120a1bd2..31f080df179d 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -96,6 +96,7 @@ enum acpi_irq_model_id {
 	ACPI_IRQ_MODEL_GIC,
 	ACPI_IRQ_MODEL_LPIC,
 	ACPI_IRQ_MODEL_APLIC,
+	ACPI_IRQ_MODEL_PLIC,
 	ACPI_IRQ_MODEL_COUNT
 };
 
-- 
2.39.2

[RFC PATCH v1 20/21] RISC-V: ACPI: Create PLIC platform device

[Sunil V L]

Since PLIC needs to be a platform device, probe the
MADT and create platform devices for each PLIC in the
system. Use software node framework for the fwnode
which allows to create properties and hence the
actual irqchip driver doesn't need to do anything
different for ACPI vs DT.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Co-developed-by: Haibo Xu <haibo1.xu@intel.com>
Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
---
 Documentation/riscv/acpi.rst  |   6 ++
 arch/riscv/include/asm/acpi.h |   3 +
 drivers/acpi/riscv/init.c     |   1 +
 drivers/acpi/riscv/init.h     |   1 +
 drivers/acpi/riscv/irqchip.c  | 198 ++++++++++++++++++++++++++++++++++
 5 files changed, 209 insertions(+)

diff --git a/Documentation/riscv/acpi.rst b/Documentation/riscv/acpi.rst
index 9ea9008288ea..007483dfddc1 100644
--- a/Documentation/riscv/acpi.rst
+++ b/Documentation/riscv/acpi.rst
@@ -35,3 +35,9 @@ to this hart.
 
 ``riscv,gsi-base`` - The global system interrupt number where this APLIC’s
 interrupt inputs start.
+
+3) RISC-V Platform Level Interrupt Controller (PLIC)
+-----------
+
+``riscv,gsi-base`` - The global system interrupt number where this PLIC’s
+interrupt inputs start.
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 6dde3d63dc0e..163b8eefa744 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -71,6 +71,7 @@ int acpi_get_cbo_block_size(struct acpi_table_header *table, unsigned int cpu, u
 struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc);
 struct fwnode_handle *acpi_imsic_create_fwnode(struct acpi_madt_imsic *imsic);
 struct fwnode_handle *acpi_riscv_get_msi_fwnode(struct device *dev);
+int acpi_plic_get_context_id(u8 plic_id, u16 idx);
 #else
 static inline void acpi_init_rintc_map(void) { }
 static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
@@ -95,6 +96,8 @@ static inline struct fwnode_handle *acpi_riscv_get_msi_fwnode(struct device *dev
 {
 	return NULL;
 }
+
+static inline int acpi_plic_get_context_id(u8 plic_id, u16 idx) { return idx; }
 #endif /* CONFIG_ACPI */
 
 #endif /*_ASM_ACPI_H*/
diff --git a/drivers/acpi/riscv/init.c b/drivers/acpi/riscv/init.c
index ee747211174f..cc733ea9ef1d 100644
--- a/drivers/acpi/riscv/init.c
+++ b/drivers/acpi/riscv/init.c
@@ -12,4 +12,5 @@ void __init acpi_riscv_init(void)
 {
 	riscv_acpi_imsic_platform_init();
 	riscv_acpi_aplic_platform_init();
+	riscv_acpi_plic_platform_init();
 }
diff --git a/drivers/acpi/riscv/init.h b/drivers/acpi/riscv/init.h
index 17bcf0baaadb..b4b305d83b3a 100644
--- a/drivers/acpi/riscv/init.h
+++ b/drivers/acpi/riscv/init.h
@@ -3,3 +3,4 @@
 
 void __init riscv_acpi_imsic_platform_init(void);
 void __init riscv_acpi_aplic_platform_init(void);
+void __init riscv_acpi_plic_platform_init(void);
diff --git a/drivers/acpi/riscv/irqchip.c b/drivers/acpi/riscv/irqchip.c
index 7fb7befdb303..cb70f0f2294b 100644
--- a/drivers/acpi/riscv/irqchip.c
+++ b/drivers/acpi/riscv/irqchip.c
@@ -25,6 +25,8 @@ static struct fwnode_handle *imsic_acpi_fwnode;
 
 LIST_HEAD(aplic_list);
 
+LIST_HEAD(plic_list);
+
 struct fwnode_handle *acpi_rintc_create_irqchip_fwnode(struct acpi_madt_rintc *rintc)
 {
 	struct property_entry props[6] = {};
@@ -307,3 +309,199 @@ void __init riscv_acpi_aplic_platform_init(void)
 		acpi_set_gsi_to_irq_fallback(aplic_gsi_to_irq);
 	}
 }
+
+static int acpi_plic_get_nr_contexts(u8 plic_id)
+{
+	struct riscv_irqchip_list *rintc_element;
+	struct fwnode_handle *fwnode;
+	struct list_head *i, *tmp;
+	u32 id;
+	int rc, nr_contexts = 0;
+
+	list_for_each_safe(i, tmp, &rintc_list) {
+		rintc_element = list_entry(i, struct riscv_irqchip_list, list);
+		fwnode = rintc_element->fwnode;
+		rc = fwnode_property_read_u32_array(fwnode, "riscv,ext-intc-id", &id, 1);
+		if (rc)
+			continue;
+
+		if (APLIC_PLIC_ID(id) == plic_id)
+			nr_contexts++;
+	}
+
+	return nr_contexts * 2;
+}
+
+int acpi_plic_get_context_id(u8 plic_id, u16 idx)
+{
+	struct riscv_irqchip_list *rintc_element;
+	struct fwnode_handle *fwnode;
+	struct list_head *i, *tmp;
+	u32 id;
+	int rc, nr_contexts = -1;
+
+	list_for_each_safe(i, tmp, &rintc_list) {
+		rintc_element = list_entry(i, struct riscv_irqchip_list, list);
+		fwnode = rintc_element->fwnode;
+		rc = fwnode_property_read_u32_array(fwnode, "riscv,ext-intc-id", &id, 1);
+		if (rc)
+			continue;
+
+		if (APLIC_PLIC_ID(id) == plic_id)
+			nr_contexts++;
+		if (nr_contexts == idx)
+			return IDC_CONTEXT_ID(id);
+	}
+
+	return -1;
+}
+
+static struct fwnode_handle *acpi_plic_create_fwnode(struct acpi_madt_plic *plic)
+{
+	struct fwnode_handle *fwnode, *parent_fwnode;
+	struct riscv_irqchip_list *plic_element;
+	struct software_node_ref_args *refs;
+	struct property_entry props[8] = {};
+	int nr_contexts;
+
+	props[0] = PROPERTY_ENTRY_U32("riscv,gsi-base", plic->gsi_base);
+	props[1] = PROPERTY_ENTRY_U32("riscv,ndev", plic->num_irqs);
+	props[2] = PROPERTY_ENTRY_U32("riscv,max_prio", plic->max_prio);
+	props[3] = PROPERTY_ENTRY_U8("riscv,plic-id", plic->id);
+	props[4] = PROPERTY_ENTRY_U64("riscv,plic-base", plic->base_addr);
+	props[5] = PROPERTY_ENTRY_U32("riscv,plic-size", plic->size);
+
+	nr_contexts = acpi_plic_get_nr_contexts(plic->id);
+	if (nr_contexts) {
+		refs = kcalloc(nr_contexts, sizeof(*refs), GFP_KERNEL);
+		for (int i = 0; i < nr_contexts / 2; i++) {
+			int context_id = acpi_plic_get_context_id(plic->id, i);
+
+			parent_fwnode = acpi_ext_intc_get_rintc_fwnode(plic->id, context_id);
+			refs[2 * i] = SOFTWARE_NODE_REFERENCE(to_software_node(parent_fwnode), 0);
+			refs[2 * i + 1] = SOFTWARE_NODE_REFERENCE(to_software_node(parent_fwnode),
+								  RV_IRQ_EXT);
+		}
+		props[6] = PROPERTY_ENTRY_REF_ARRAY_LEN("interrupts-extended", refs, nr_contexts);
+	}
+
+	fwnode = fwnode_create_software_node_early(props, NULL);
+
+	if (fwnode) {
+		plic_element = kzalloc(sizeof(*plic_element), GFP_KERNEL);
+		if (!plic_element) {
+			fwnode_remove_software_node(fwnode);
+			return NULL;
+		}
+
+		plic_element->fwnode = fwnode;
+		list_add_tail(&plic_element->list, &plic_list);
+	}
+
+	return fwnode;
+}
+
+static struct fwnode_handle *plic_get_gsi_domain_id(u32 gsi)
+{
+	struct riscv_irqchip_list *plic_element;
+	struct fwnode_handle *fwnode;
+	struct list_head *i, *tmp;
+	u32 gsi_base;
+	u32 nr_irqs;
+	int rc;
+
+	list_for_each_safe(i, tmp, &plic_list) {
+		plic_element = list_entry(i, struct riscv_irqchip_list, list);
+		fwnode = plic_element->fwnode;
+		rc = fwnode_property_read_u32_array(fwnode, "riscv,gsi-base", &gsi_base, 1);
+		if (!rc) {
+			rc = fwnode_property_read_u32_array(fwnode, "riscv,ndev", &nr_irqs, 1);
+			if (!rc && (gsi >= gsi_base && gsi < gsi_base + nr_irqs))
+				return fwnode;
+		}
+	}
+
+	return NULL;
+}
+
+static u32 plic_gsi_to_irq(u32 gsi)
+{
+	return acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
+}
+
+static int __init plic_create_platform_device(struct fwnode_handle *fwnode)
+{
+	struct platform_device *pdev;
+	u32 plic_size;
+	u8  plic_id;
+	struct resource *res;
+	u64 plic_base;
+	int ret;
+
+	if (!fwnode)
+		return -ENODEV;
+
+	ret = fwnode_property_read_u64_array(fwnode, "riscv,plic-base", &plic_base, 1);
+	if (ret)
+		return -ENODEV;
+
+	ret = fwnode_property_read_u32_array(fwnode, "riscv,plic-size", &plic_size, 1);
+	if (ret)
+		return -ENODEV;
+
+	ret = fwnode_property_read_u8_array(fwnode, "riscv,plic-id", &plic_id, 1);
+	if (ret)
+		return -ENODEV;
+
+	pdev = platform_device_alloc("riscv-plic", plic_id);
+	if (!pdev)
+		return -ENOMEM;
+
+	res = kcalloc(1, sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		ret = -ENOMEM;
+		goto dev_put;
+	}
+
+	res->start = plic_base;
+	res->end = res->start + plic_size - 1;
+	res->flags = IORESOURCE_MEM;
+	ret = platform_device_add_resources(pdev, res, 1);
+	/*
+	 * Resources are duplicated in platform_device_add_resources,
+	 * free their allocated memory
+	 */
+	kfree(res);
+
+	pdev->dev.fwnode = fwnode;
+	ret = platform_device_add(pdev);
+	if (ret)
+		goto dev_put;
+
+	return 0;
+
+dev_put:
+	platform_device_put(pdev);
+	return ret;
+}
+
+static int __init plic_parse_madt(union acpi_subtable_headers *header,
+				  const unsigned long end)
+{
+	struct acpi_madt_plic *plic = (struct acpi_madt_plic *)header;
+	struct fwnode_handle *fwnode;
+
+	fwnode = acpi_plic_create_fwnode(plic);
+	if (fwnode)
+		plic_create_platform_device(fwnode);
+
+	return 0;
+}
+
+void __init riscv_acpi_plic_platform_init(void)
+{
+	if (acpi_table_parse_madt(ACPI_MADT_TYPE_PLIC, plic_parse_madt, 0) > 0) {
+		acpi_set_irq_model(ACPI_IRQ_MODEL_PLIC, plic_get_gsi_domain_id);
+		acpi_set_gsi_to_irq_fallback(plic_gsi_to_irq);
+	}
+}
-- 
2.39.2

Re: [RFC PATCH v1 20/21] RISC-V: ACPI: Create PLIC platform device

[Conor Dooley]

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

On Thu, Aug 03, 2023 at 11:29:15PM +0530, Sunil V L wrote:
> Since PLIC needs to be a platform device

For the unwashed, why does the PLCI need to be a platform device?
(And while you're at that, please try to make use of the extra ~20
characters per line that you can use here.)

> probe the
> MADT and create platform devices for each PLIC in the
> system. Use software node framework for the fwnode
> which allows to create properties and hence the
> actual irqchip driver doesn't need to do anything
> different for ACPI vs DT.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Co-developed-by: Haibo Xu <haibo1.xu@intel.com>
> Signed-off-by: Haibo Xu <haibo1.xu@intel.com>

> +static struct fwnode_handle *acpi_plic_create_fwnode(struct acpi_madt_plic *plic)
> +{
> +	struct fwnode_handle *fwnode, *parent_fwnode;
> +	struct riscv_irqchip_list *plic_element;
> +	struct software_node_ref_args *refs;
> +	struct property_entry props[8] = {};
> +	int nr_contexts;
> +
> +	props[0] = PROPERTY_ENTRY_U32("riscv,gsi-base", plic->gsi_base);
> +	props[1] = PROPERTY_ENTRY_U32("riscv,ndev", plic->num_irqs);
> +	props[2] = PROPERTY_ENTRY_U32("riscv,max_prio", plic->max_prio);

My OCD wants to know why this gets an _ but the others have a -.

> +	props[3] = PROPERTY_ENTRY_U8("riscv,plic-id", plic->id);
> +	props[4] = PROPERTY_ENTRY_U64("riscv,plic-base", plic->base_addr);
> +	props[5] = PROPERTY_ENTRY_U32("riscv,plic-size", plic->size);

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

Re: [RFC PATCH v1 20/21] RISC-V: ACPI: Create PLIC platform device

[Anup Patel]

On Tue, Aug 8, 2023 at 2:12 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Aug 03, 2023 at 11:29:15PM +0530, Sunil V L wrote:
> > Since PLIC needs to be a platform device
>
> For the unwashed, why does the PLCI need to be a platform device?
> (And while you're at that, please try to make use of the extra ~20
> characters per line that you can use here.)

As suggested by Marc Z, only timers and IPIs need to be probed early.
Everything else needs to be a platform device. The devlink feature of
Linux DD framework ensures that platform devices are probed in the
right order based on the interdependency.

The PATCH5 of the v7 AIA series converts the PLIC driver into a
platform driver. This works perfectly fine.

>
> > probe the
> > MADT and create platform devices for each PLIC in the
> > system. Use software node framework for the fwnode
> > which allows to create properties and hence the
> > actual irqchip driver doesn't need to do anything
> > different for ACPI vs DT.
> >
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > Co-developed-by: Haibo Xu <haibo1.xu@intel.com>
> > Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
>
> > +static struct fwnode_handle *acpi_plic_create_fwnode(struct acpi_madt_plic *plic)
> > +{
> > +     struct fwnode_handle *fwnode, *parent_fwnode;
> > +     struct riscv_irqchip_list *plic_element;
> > +     struct software_node_ref_args *refs;
> > +     struct property_entry props[8] = {};
> > +     int nr_contexts;
> > +
> > +     props[0] = PROPERTY_ENTRY_U32("riscv,gsi-base", plic->gsi_base);
> > +     props[1] = PROPERTY_ENTRY_U32("riscv,ndev", plic->num_irqs);
> > +     props[2] = PROPERTY_ENTRY_U32("riscv,max_prio", plic->max_prio);
>
> My OCD wants to know why this gets an _ but the others have a -.
>
> > +     props[3] = PROPERTY_ENTRY_U8("riscv,plic-id", plic->id);
> > +     props[4] = PROPERTY_ENTRY_U64("riscv,plic-base", plic->base_addr);
> > +     props[5] = PROPERTY_ENTRY_U32("riscv,plic-size", plic->size);
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup

Re: [RFC PATCH v1 20/21] RISC-V: ACPI: Create PLIC platform device

[Conor Dooley]

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

On Tue, Aug 08, 2023 at 04:27:16PM +0530, Anup Patel wrote:
> On Tue, Aug 8, 2023 at 2:12 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Aug 03, 2023 at 11:29:15PM +0530, Sunil V L wrote:
> > > Since PLIC needs to be a platform device
> >
> > For the unwashed, why does the PLCI need to be a platform device?
> > (And while you're at that, please try to make use of the extra ~20
> > characters per line that you can use here.)
> 
> As suggested by Marc Z, only timers and IPIs need to be probed early.
> Everything else needs to be a platform device. The devlink feature of
> Linux DD framework ensures that platform devices are probed in the
> right order based on the interdependency.
> 
> The PATCH5 of the v7 AIA series converts the PLIC driver into a
> platform driver. This works perfectly fine.

To be clear, I want the explanation of why the "PLIC needs to be a
platform device" to be in the commit message.

Thanks,
Conor.

> 
> >
> > > probe the
> > > MADT and create platform devices for each PLIC in the
> > > system. Use software node framework for the fwnode
> > > which allows to create properties and hence the
> > > actual irqchip driver doesn't need to do anything
> > > different for ACPI vs DT.
> > >
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > Co-developed-by: Haibo Xu <haibo1.xu@intel.com>
> > > Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> >
> > > +static struct fwnode_handle *acpi_plic_create_fwnode(struct acpi_madt_plic *plic)
> > > +{
> > > +     struct fwnode_handle *fwnode, *parent_fwnode;
> > > +     struct riscv_irqchip_list *plic_element;
> > > +     struct software_node_ref_args *refs;
> > > +     struct property_entry props[8] = {};
> > > +     int nr_contexts;
> > > +
> > > +     props[0] = PROPERTY_ENTRY_U32("riscv,gsi-base", plic->gsi_base);
> > > +     props[1] = PROPERTY_ENTRY_U32("riscv,ndev", plic->num_irqs);
> > > +     props[2] = PROPERTY_ENTRY_U32("riscv,max_prio", plic->max_prio);
> >
> > My OCD wants to know why this gets an _ but the others have a -.
> >
> > > +     props[3] = PROPERTY_ENTRY_U8("riscv,plic-id", plic->id);
> > > +     props[4] = PROPERTY_ENTRY_U64("riscv,plic-base", plic->base_addr);
> > > +     props[5] = PROPERTY_ENTRY_U32("riscv,plic-size", plic->size);

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

Re: [RFC PATCH v1 20/21] RISC-V: ACPI: Create PLIC platform device

[Sunil V L]

Hi Conor,

On Tue, Aug 08, 2023 at 09:41:45AM +0100, Conor Dooley wrote:
> On Thu, Aug 03, 2023 at 11:29:15PM +0530, Sunil V L wrote:
> > Since PLIC needs to be a platform device
> 
> For the unwashed, why does the PLCI need to be a platform device?
> (And while you're at that, please try to make use of the extra ~20
> characters per line that you can use here.)
>
Sure, let me add more details and use extra characters.
 
> > probe the
> > MADT and create platform devices for each PLIC in the
> > system. Use software node framework for the fwnode
> > which allows to create properties and hence the
> > actual irqchip driver doesn't need to do anything
> > different for ACPI vs DT.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > Co-developed-by: Haibo Xu <haibo1.xu@intel.com>
> > Signed-off-by: Haibo Xu <haibo1.xu@intel.com>
> 
> > +static struct fwnode_handle *acpi_plic_create_fwnode(struct acpi_madt_plic *plic)
> > +{
> > +	struct fwnode_handle *fwnode, *parent_fwnode;
> > +	struct riscv_irqchip_list *plic_element;
> > +	struct software_node_ref_args *refs;
> > +	struct property_entry props[8] = {};
> > +	int nr_contexts;
> > +
> > +	props[0] = PROPERTY_ENTRY_U32("riscv,gsi-base", plic->gsi_base);
> > +	props[1] = PROPERTY_ENTRY_U32("riscv,ndev", plic->num_irqs);
> > +	props[2] = PROPERTY_ENTRY_U32("riscv,max_prio", plic->max_prio);
> 
> My OCD wants to know why this gets an _ but the others have a -.
> 
It should be -. But with Marc's recommendation, this is no longer
required.

Thanks!
Sunil

[RFC PATCH v1 21/21] irqchip/sifive-plic: Add GSI conversion support

[Sunil V L]

ACPI uses flat Global System Interrupt Vector space and
hence the GSI number needs to be converted to corresponding
local IRQ number of the interrupt controller. Add a new
gsi_base field in the priv structure to handle this which
will be 0 on DT based systems.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/irqchip/irq-sifive-plic.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 72d6e06ef52b..1c22070e9cdd 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -68,6 +68,7 @@ struct plic_priv {
 	unsigned long plic_quirks;
 	unsigned int nr_irqs;
 	unsigned long *prio_save;
+	u32 gsi_base;
 };
 
 struct plic_handler {
@@ -314,6 +315,10 @@ static int plic_irq_domain_translate(struct irq_domain *d,
 {
 	struct plic_priv *priv = d->host_data;
 
+	/* For DT, gsi_base is always zero. */
+	if (fwspec->param[0] >= priv->gsi_base)
+		fwspec->param[0] = fwspec->param[0] - priv->gsi_base;
+
 	if (test_bit(PLIC_QUIRK_EDGE_INTERRUPT, &priv->plic_quirks))
 		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
 
@@ -453,6 +458,17 @@ static int plic_probe(struct platform_device *pdev)
 		return -EIO;
 	}
 
+	/*
+	 * Find out GSI base number
+	 *
+	 * Note: DT does not define "riscv,gsi-base" property so GSI
+	 * base is always zero for DT.
+	 */
+	rc = fwnode_property_read_u32_array(dev->fwnode, "riscv,gsi-base",
+					    &priv->gsi_base, 1);
+	if (rc)
+		priv->gsi_base = 0;
+
 	rc = fwnode_property_read_u32_array(dev->fwnode, "riscv,ndev",
 						&nr_irqs, 1);
 	if (rc) {
-- 
2.39.2