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(-)
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,
--
2.40.1On 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
Hi Thomas,
On Thu, May 23, 2024 at 11:47:06PM +0200, Thomas Gleixner wrote:
> 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
>
Sure, thanks!
> > +};
> > +
> > +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.
>
Sure. I just tried to keep it similar to the parent function. But let me
go with your suggestion in the next revision.
> > +{
> > + 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;
>
Sure.
> > +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?
>
Nice!. Yes, this is better. Thanks a lot for the suggestion. Let me
update in the next revision.
> > +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?
>
:-). Let me remove it.
> > 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.
>
Makes sense. Let me create separate patch.
Thanks a lot for the review!
Sunil
© 2016 - 2025 Red Hat, Inc.