diff mbox series

[v5,13/17] irqchip/riscv-intc: Add ACPI support for AIA

Message ID 20240501121742.1215792-14-sunilvl@ventanamicro.com
State New
Headers show
Series RISC-V: ACPI: Add external interrupt controller support | expand

Commit Message

Sunil V L May 1, 2024, 12:17 p.m. UTC
The RINTC subtype structure in MADT also has information about other
interrupt controllers. Save this information and provide interfaces to
retrieve them when required by corresponding drivers.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/irq.h     |  35 +++++++++++
 drivers/irqchip/irq-riscv-intc.c | 102 ++++++++++++++++++++++++++++++-
 2 files changed, 135 insertions(+), 2 deletions(-)

Comments

Thomas Gleixner May 23, 2024, 9:47 p.m. UTC | #1
On Wed, May 01 2024 at 17:47, Sunil V L wrote:
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index 9e71c4428814..af7a2f78f0ee 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -249,14 +249,105 @@ IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>  IRQCHIP_DECLARE(andes, "andestech,cpu-intc", riscv_intc_init);
>  
>  #ifdef CONFIG_ACPI
> +struct rintc_data {
> +	u32 ext_intc_id;
> +	unsigned long hart_id;
> +	u64 imsic_addr;
> +	u32 imsic_size;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

> +};
> +
> +static u32 nr_rintc;
> +static struct rintc_data *rintc_acpi_data[NR_CPUS];
> +
> +int acpi_get_intc_index_hartid(u32 index, unsigned long *hartid)

Why int? All of these functions have strictly boolean return values:
success = true, fail = false, no?

Either bool or get rid of the pointer and let the function return
either the real hart id or an invalid one.

> +{
> +	if (index >= nr_rintc)
> +		return -1;
> +
> +	*hartid = rintc_acpi_data[index]->hart_id;
> +	return 0;

I.e.

	return index >= nr_rintc ? rintc_acpi_data[index]->hart_id : INVALID_HART_ID;

> +int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> +			if (idx == j) {
> +				*hartid = rintc_acpi_data[i]->hart_id;
> +				return 0;
> +			}
> +			j++;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
> +void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id)
> +			j++;
> +	}
> +
> +	*nr_contexts = j;
> +}
> +
> +int acpi_get_plic_context(u8 id, u32 idx, int *context_id)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> +			if (idx == j) {
> +				*context_id = IDC_CONTEXT_ID(rintc_acpi_data[i]->ext_intc_id);
> +				return 0;
> +			}
> +
> +			j++;
> +		}
> +	}

So that's the third incarnation of the same loop with the truly self
explaining variable and argument names.

    j is actually the index of the context which is associated to a
    given PLIC ID.

    idx is the context index to search for

Right? So why can't these things be named in a way which makes the
intent of the code clear?

Also why are all the arguments u8/u32? There is no hardware
involved. Simple 'unsigned int' is just fine and the u8/u32 is not bying
you anything here.

Aside of that these ugly macros can be completely avoided and the code
can be written without a copy & pasta orgy.

struct rintc_data {
	union {
		u32		ext_intc_id;
                struct {
                	u32	context_id	: 16,
                        			:  8,
                        	aplic_plic_id	:  8;
                }
        };
	unsigned long		hart_id;
	u64			imsic_addr;
	u32			imsic_size;
};

#define for_each_matching_plic(_plic, _plic_id)				\
	for (_plic = 0; _plic < nr_rintc; _plict++)			\
        	if (rintc_acpi_data[_plic]->aplic_plic_id != _plic_id)	\
                	continue;					\
                else

unsigned int acpi_get_plic_nr_contexts(unsigned int plic_id)
{
	unsigned int nctx = 0;

	for_each_matching_plic(plic, plic_id)
		nctx++;

	return nctx;
}

static struct rintc_data *get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
{
	unsigned int ctxt = 0;

	for_each_matching_plic(plic, plic_id) {
        	if (ctxt == ctxt_idx)
                	return rintc_acpi_data + plic;
        }
        return NULL;
}

unsigned long acpi_get_ext_intc_parent_hartid(unsigned int plic_id, unsigned int ctxt_idx)
{
        struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);

        return data ? data->hart_id : INVALID_HART_ID;
}

unsigned int acpi_get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
{
        struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);

        return data ? data->context_id : INVALID_CONTEXT;
}

Or something like that. Hmm?

> +int acpi_get_imsic_mmio_info(u32 index, struct resource *res)
> +{
> +	if (index >= nr_rintc)
> +		return -1;
> +
> +	res->start = rintc_acpi_data[index]->imsic_addr;
> +	res->end = res->start + rintc_acpi_data[index]->imsic_size - 1;
> +	res->flags = IORESOURCE_MEM;
> +	return 0;
> +}
> +
> +static struct fwnode_handle *ext_entc_get_gsi_domain_id(u32 gsi)
> +{
> +	return riscv_acpi_get_gsi_domain_id(gsi);
> +}

This wrapper is required because using riscv_acpi_get_gsi_domain_id()
directly is too obvious, right?

>  static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  				       const unsigned long end)
>  {
> -	struct fwnode_handle *fn;
>  	struct acpi_madt_rintc *rintc;
> +	struct fwnode_handle *fn;
> +	int rc;
>  
>  	rintc = (struct acpi_madt_rintc *)header;
> +	rintc_acpi_data[nr_rintc] = kzalloc(sizeof(*rintc_acpi_data[0]), GFP_KERNEL);
> +	if (!rintc_acpi_data[nr_rintc])
> +		return -ENOMEM;
> +
> +	rintc_acpi_data[nr_rintc]->ext_intc_id = rintc->ext_intc_id;
> +	rintc_acpi_data[nr_rintc]->hart_id = rintc->hart_id;
> +	rintc_acpi_data[nr_rintc]->imsic_addr = rintc->imsic_addr;
> +	rintc_acpi_data[nr_rintc]->imsic_size = rintc->imsic_size;
> +	nr_rintc++;
>  
>  	/*
>  	 * The ACPI MADT will have one INTC for each CPU (or HART)
> @@ -273,7 +364,14 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  		return -ENOMEM;
>  	}
>  
> -	return riscv_intc_init_common(fn, &riscv_intc_chip);
> +	rc = riscv_intc_init_common(fn, &riscv_intc_chip);
> +	if (rc) {
> +		irq_domain_free_fwnode(fn);
> +		return rc;
> +	}

This looks like a completely unrelated bug fix. Please don't mix functional
changes and fixes.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 44a0b128c602..6bd578b1ffc9 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -25,9 +25,22 @@  enum riscv_irqchip_type {
 	ACPI_RISCV_IRQCHIP_APLIC	= 0x03,
 };
 
+/*
+ * 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)
+
 int riscv_acpi_get_gsi_info(struct fwnode_handle *fwnode, u32 *gsi_base,
 			    u32 *id, u32 *nr_irqs, u32 *nr_idcs);
 struct fwnode_handle *riscv_acpi_get_gsi_domain_id(u32 gsi);
+int __init acpi_get_intc_index_hartid(u32 index, unsigned long *hartid);
+int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid);
+void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts);
+int acpi_get_plic_context(u8 id, u32 idx, int *context_id);
+int __init acpi_get_imsic_mmio_info(u32 index, struct resource *res);
 
 #else
 static inline int riscv_acpi_get_gsi_info(struct fwnode_handle *fwnode, u32 *gsi_base,
@@ -36,6 +49,28 @@  static inline int riscv_acpi_get_gsi_info(struct fwnode_handle *fwnode, u32 *gsi
 	return 0;
 }
 
+static inline int __init acpi_get_intc_index_hartid(u32 index, unsigned long *hartid)
+{
+	return -EINVAL;
+}
+
+static inline int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid)
+{
+	return -EINVAL;
+}
+
+static inline void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts) { }
+
+static inline int acpi_get_plic_context(u8 id, u32 idx, int *context_id)
+{
+	return -EINVAL;
+}
+
+static inline int __init acpi_get_imsic_mmio_info(u32 index, struct resource *res)
+{
+	return 0;
+}
+
 #endif /* CONFIG_ACPI */
 
 #endif /* _ASM_RISCV_IRQ_H */
diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
index 9e71c4428814..af7a2f78f0ee 100644
--- a/drivers/irqchip/irq-riscv-intc.c
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -249,14 +249,105 @@  IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
 IRQCHIP_DECLARE(andes, "andestech,cpu-intc", riscv_intc_init);
 
 #ifdef CONFIG_ACPI
+struct rintc_data {
+	u32 ext_intc_id;
+	unsigned long hart_id;
+	u64 imsic_addr;
+	u32 imsic_size;
+};
+
+static u32 nr_rintc;
+static struct rintc_data *rintc_acpi_data[NR_CPUS];
+
+int acpi_get_intc_index_hartid(u32 index, unsigned long *hartid)
+{
+	if (index >= nr_rintc)
+		return -1;
+
+	*hartid = rintc_acpi_data[index]->hart_id;
+	return 0;
+}
+
+int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid)
+{
+	int i, j = 0;
+
+	for (i = 0; i < nr_rintc; i++) {
+		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
+			if (idx == j) {
+				*hartid = rintc_acpi_data[i]->hart_id;
+				return 0;
+			}
+			j++;
+		}
+	}
+
+	return -1;
+}
+
+void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts)
+{
+	int i, j = 0;
+
+	for (i = 0; i < nr_rintc; i++) {
+		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id)
+			j++;
+	}
+
+	*nr_contexts = j;
+}
+
+int acpi_get_plic_context(u8 id, u32 idx, int *context_id)
+{
+	int i, j = 0;
+
+	for (i = 0; i < nr_rintc; i++) {
+		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
+			if (idx == j) {
+				*context_id = IDC_CONTEXT_ID(rintc_acpi_data[i]->ext_intc_id);
+				return 0;
+			}
+
+			j++;
+		}
+	}
+
+	return -1;
+}
+
+int acpi_get_imsic_mmio_info(u32 index, struct resource *res)
+{
+	if (index >= nr_rintc)
+		return -1;
+
+	res->start = rintc_acpi_data[index]->imsic_addr;
+	res->end = res->start + rintc_acpi_data[index]->imsic_size - 1;
+	res->flags = IORESOURCE_MEM;
+	return 0;
+}
+
+static struct fwnode_handle *ext_entc_get_gsi_domain_id(u32 gsi)
+{
+	return riscv_acpi_get_gsi_domain_id(gsi);
+}
 
 static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
 				       const unsigned long end)
 {
-	struct fwnode_handle *fn;
 	struct acpi_madt_rintc *rintc;
+	struct fwnode_handle *fn;
+	int rc;
 
 	rintc = (struct acpi_madt_rintc *)header;
+	rintc_acpi_data[nr_rintc] = kzalloc(sizeof(*rintc_acpi_data[0]), GFP_KERNEL);
+	if (!rintc_acpi_data[nr_rintc])
+		return -ENOMEM;
+
+	rintc_acpi_data[nr_rintc]->ext_intc_id = rintc->ext_intc_id;
+	rintc_acpi_data[nr_rintc]->hart_id = rintc->hart_id;
+	rintc_acpi_data[nr_rintc]->imsic_addr = rintc->imsic_addr;
+	rintc_acpi_data[nr_rintc]->imsic_size = rintc->imsic_size;
+	nr_rintc++;
 
 	/*
 	 * The ACPI MADT will have one INTC for each CPU (or HART)
@@ -273,7 +364,14 @@  static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
 		return -ENOMEM;
 	}
 
-	return riscv_intc_init_common(fn, &riscv_intc_chip);
+	rc = riscv_intc_init_common(fn, &riscv_intc_chip);
+	if (rc) {
+		irq_domain_free_fwnode(fn);
+		return rc;
+	}
+
+	acpi_set_irq_model(ACPI_IRQ_MODEL_RINTC, ext_entc_get_gsi_domain_id);
+	return 0;
 }
 
 IRQCHIP_ACPI_DECLARE(riscv_intc, ACPI_MADT_TYPE_RINTC, NULL,