| Message ID | 20240501121742.1215792-14-sunilvl@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,01/17] arm64: PCI: Migrate ACPI related functions to pci-acpi.c | expand |
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
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
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,
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(-)