Messages in this thread | | | Date | Wed, 29 Jun 2022 12:20:54 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support |
| |
On Mon, 27 Jun 2022 12:39:50 +0100, Jianmin Lv <lvjianmin@loongson.cn> wrote: > > From: Huacai Chen <chenhuacai@loongson.cn> > > We are preparing to add new Loongson (based on LoongArch, not compatible > with old MIPS-based Loongson) support. LoongArch use ACPI other than DT > as its boot protocol, so add ACPI init support.
Drop this paragraph.
> > PCH-PIC/PCH-MSI stands for "Interrupt Controller" that described in > Section 5 of "Loongson 7A1000 Bridge User Manual". For more information > please refer Documentation/loongarch/irq-chip-model.rst. > > For systems with two chipsets, there are two related pch irqdomains, > each of which has the same node id as its parent irqdomain. So we > defined a structure to mantain the relation of node and it's parent > irqdomain. > > struct acpi_vector_group { > int node; > struct irq_domain *parent; > }; > > The parent irqdomain will be set for node id in parent irqdomain driver > before pch irqdomain is created. > > Co-developed-by: Jianmin Lv <lvjianmin@loongson.cn> > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > --- > arch/loongarch/include/asm/irq.h | 11 +- > arch/loongarch/kernel/irq.c | 2 +- > drivers/irqchip/irq-loongson-pch-pic.c | 200 ++++++++++++++++++++++++++++----- > 3 files changed, 179 insertions(+), 34 deletions(-) > > diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h > index 48c0ce4..a444dc6 100644 > --- a/arch/loongarch/include/asm/irq.h > +++ b/arch/loongarch/include/asm/irq.h > @@ -48,6 +48,12 @@ static inline bool on_irq_stack(int cpu, unsigned long sp) > #define MAX_IO_PICS 2 > #define NR_IRQS (64 + (256 * MAX_IO_PICS)) > > +struct acpi_vector_group { > + int node; > + struct irq_domain *parent; > +}; > +extern struct acpi_vector_group pch_group[MAX_IO_PICS]; > + > #define CORES_PER_EIO_NODE 4 > > #define LOONGSON_CPU_UART0_VEC 10 /* CPU UART0 */ > @@ -108,8 +114,7 @@ int pch_lpc_acpi_init(struct irq_domain *parent, > struct acpi_madt_lpc_pic *acpi_pchlpc); > struct irq_domain *pch_msi_acpi_init(struct irq_domain *parent, > struct acpi_madt_msi_pic *acpi_pchmsi); > -struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent, > - struct acpi_madt_bio_pic *acpi_pchpic); > +int find_pch_pic(u32 gsi); > > extern struct acpi_madt_lio_pic *acpi_liointc; > extern struct acpi_madt_eio_pic *acpi_eiointc[MAX_IO_PICS]; > @@ -123,7 +128,7 @@ struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent, > extern struct irq_domain *liointc_domain; > extern struct fwnode_handle *pch_lpc_handle; > extern struct irq_domain *pch_msi_domain[MAX_IO_PICS]; > -extern struct irq_domain *pch_pic_domain[MAX_IO_PICS]; > +extern struct fwnode_handle *pch_pic_handle[MAX_IO_PICS]; > > extern irqreturn_t loongson3_ipi_interrupt(int irq, void *dev); > > diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c > index 07d6059..f2115be 100644 > --- a/arch/loongarch/kernel/irq.c > +++ b/arch/loongarch/kernel/irq.c > @@ -28,7 +28,7 @@ > struct irq_domain *cpu_domain; > struct irq_domain *liointc_domain; > struct irq_domain *pch_msi_domain[MAX_IO_PICS]; > -struct irq_domain *pch_pic_domain[MAX_IO_PICS]; > +struct acpi_vector_group pch_group[MAX_IO_PICS]; > > /* > * 'what should we do if we get a hw irq event on an illegal vector'. > diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c > index a4eb8a2..170a5b9 100644 > --- a/drivers/irqchip/irq-loongson-pch-pic.c > +++ b/drivers/irqchip/irq-loongson-pch-pic.c > @@ -33,13 +33,40 @@ > #define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG) > #define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG) > > +static int nr_pics; > + > struct pch_pic { > void __iomem *base; > struct irq_domain *pic_domain; > + struct fwnode_handle *domain_handle; > u32 ht_vec_base; > raw_spinlock_t pic_lock; > + u32 gsi_end; > + u32 gsi_base; > }; > > +static struct pch_pic *pch_pic_priv[2];
Why 2? Is that related to MAX_IO_PICS being 2?
> +struct fwnode_handle *pch_pic_handle[MAX_IO_PICS]; > + > +int find_pch_pic(u32 gsi) > +{ > + int i; > + > + /* Find the PCH_PIC that manages this GSI. */ > + for (i = 0; i < MAX_IO_PICS; i++) { > + struct pch_pic *priv = pch_pic_priv[i]; > + > + if (!priv) > + return -1; > + > + if (gsi >= priv->gsi_base && gsi <= priv->gsi_end) > + return i; > + } > + > + pr_err("ERROR: Unable to locate PCH_PIC for GSI %d\n", gsi); > + return -1; > +} > + > static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit) > { > u32 reg; > @@ -139,6 +166,24 @@ static void pch_pic_ack_irq(struct irq_data *d) > .irq_set_type = pch_pic_set_type, > }; > > +static int pch_pic_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + struct pch_pic *priv = d->host_data; > + struct device_node *of_node = to_of_node(fwspec->fwnode); > + > + if (fwspec->param_count < 1) > + return -EINVAL; > + if (of_node) > + *hwirq += priv->ht_vec_base; > + else > + *hwirq = fwspec->param[0] - priv->gsi_base; > + *type = IRQ_TYPE_NONE;
Have you tested this on MIPS HW? This looks like a regression on the OF path, as it used irq_domain_translate_twocell().
> + > + return 0; > +} > static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq, > unsigned int nr_irqs, void *arg) > { > @@ -149,13 +194,13 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq, > struct irq_fwspec parent_fwspec; > struct pch_pic *priv = domain->host_data; > > - err = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type); > + err = pch_pic_domain_translate(domain, fwspec, &hwirq, &type); > if (err) > return err; > > parent_fwspec.fwnode = domain->parent->fwnode; > parent_fwspec.param_count = 1; > - parent_fwspec.param[0] = hwirq + priv->ht_vec_base; > + parent_fwspec.param[0] = hwirq; > > err = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); > if (err) > @@ -170,7 +215,7 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq, > } > > static const struct irq_domain_ops pch_pic_domain_ops = { > - .translate = irq_domain_translate_twocell, > + .translate = pch_pic_domain_translate, > .alloc = pch_pic_alloc, > .free = irq_domain_free_irqs_parent, > }; > @@ -180,7 +225,7 @@ static void pch_pic_reset(struct pch_pic *priv) > int i; > > for (i = 0; i < PIC_COUNT; i++) { > - /* Write vectored ID */ > + /* Write vector ID */ > writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i)); > /* Hardcode route to HT0 Lo */ > writeb(1, priv->base + PCH_INT_ROUTE(i)); > @@ -198,50 +243,41 @@ static void pch_pic_reset(struct pch_pic *priv) > } > } > > -static int pch_pic_of_init(struct device_node *node, > - struct device_node *parent) > +static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base, > + struct irq_domain *parent_domain, struct fwnode_handle *domain_handle, > + u32 gsi_base) > { > + int vec_count; > struct pch_pic *priv; > - struct irq_domain *parent_domain; > - int err; > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > raw_spin_lock_init(&priv->pic_lock); > - priv->base = of_iomap(node, 0); > - if (!priv->base) { > - err = -ENOMEM; > + priv->base = ioremap(addr, size); > + if (!priv->base) > goto free_priv; > - } > > - parent_domain = irq_find_host(parent); > - if (!parent_domain) { > - pr_err("Failed to find the parent domain\n"); > - err = -ENXIO; > - goto iounmap_base; > - } > + priv->domain_handle = domain_handle; > > - if (of_property_read_u32(node, "loongson,pic-base-vec", > - &priv->ht_vec_base)) { > - pr_err("Failed to determine pic-base-vec\n"); > - err = -EINVAL; > - goto iounmap_base; > - } > + priv->ht_vec_base = vec_base;
Why isn't this pre-filled on the OF path as it isn't relevant to ACPI?
> + vec_count = ((readq(priv->base) >> 48) & 0xff) + 1;
Is that valid on the old HW?
> + priv->gsi_base = gsi_base;
Why isn't this pre-filled on the ACPI path as it isn't relevant to OF?
> + priv->gsi_end = gsi_base + vec_count - 1;
I don't think this needs to be ACPI specific. Just track the vector count, which applies to both ACPI and DT.
> > priv->pic_domain = irq_domain_create_hierarchy(parent_domain, 0, > - PIC_COUNT, > - of_node_to_fwnode(node), > - &pch_pic_domain_ops, > - priv); > + vec_count, priv->domain_handle, > + &pch_pic_domain_ops, priv); > + > if (!priv->pic_domain) { > pr_err("Failed to create IRQ domain\n"); > - err = -ENOMEM; > goto iounmap_base; > } > > pch_pic_reset(priv); > + pch_pic_handle[nr_pics] = domain_handle; > + pch_pic_priv[nr_pics++] = priv; > > return 0; > > @@ -250,7 +286,111 @@ static int pch_pic_of_init(struct device_node *node, > free_priv: > kfree(priv); > > - return err; > + return -EINVAL; > +} > + > +#ifdef CONFIG_OF > + > +static int pch_pic_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + int err, vec_base; > + struct resource res; > + struct irq_domain *parent_domain; > + > + if (of_address_to_resource(node, 0, &res)) > + return -EINVAL; > + > + parent_domain = irq_find_host(parent); > + if (!parent_domain) { > + pr_err("Failed to find the parent domain\n"); > + return -ENXIO; > + } > + > + if (of_property_read_u32(node, "loongson,pic-base-vec", &vec_base)) { > + pr_err("Failed to determine pic-base-vec\n"); > + return -EINVAL; > + } > + > + err = pch_pic_init(res.start, resource_size(&res), vec_base, > + parent_domain, of_node_to_fwnode(node), 0); > + if (err < 0) > + return err; > + > + return 0; > } > > IRQCHIP_DECLARE(pch_pic, "loongson,pch-pic-1.0", pch_pic_of_init); > + > +#endif > + > +#ifdef CONFIG_ACPI > +static int __init > +lpcintc_parse_madt(union acpi_subtable_headers *header, > + const unsigned long end) > +{ > + struct acpi_madt_lpc_pic *lpcintc_entry = (struct acpi_madt_lpc_pic *)header; > + > + return pch_lpc_acpi_init(pch_pic_priv[0]->pic_domain, lpcintc_entry); > +} > + > +static int __init acpi_cascade_irqdomain_init(void) > +{ > + acpi_table_parse_madt(ACPI_MADT_TYPE_LPC_PIC, > + lpcintc_parse_madt, 0); > + return 0; > +}
Missing blank line between functions
> +int __init pch_pic_init_irqdomain(struct irq_domain *parent, > + struct acpi_madt_bio_pic *acpi_pchpic) > +{ > + int ret, vec_base; > + struct fwnode_handle *domain_handle; > + > + if (!acpi_pchpic) > + return -EINVAL; > + > + vec_base = acpi_pchpic->gsi_base - GSI_MIN_PCH_IRQ; > + > + domain_handle = irq_domain_alloc_fwnode((phys_addr_t *)acpi_pchpic); > + if (!domain_handle) { > + pr_err("Unable to allocate domain handle\n"); > + return -ENOMEM; > + } > + > + ret = pch_pic_init(acpi_pchpic->address, acpi_pchpic->size, > + vec_base, parent, domain_handle, acpi_pchpic->gsi_base); > + if (ret == 0 && acpi_pchpic->id == 0) > + acpi_cascade_irqdomain_init(); > + > + return ret; > +} > + > +struct irq_domain *acpi_get_pch_parent(int node) > +{ > + int i; > + > + for (i = 0; i < MAX_IO_PICS; i++) { > + if (node == pch_group[i].node) > + return pch_group[i].parent; > + } > + return NULL; > +} > + > +static int __init pchintc_parse_madt(union acpi_subtable_headers *header, > + const unsigned long end) > +{ > + struct acpi_madt_bio_pic *pch_pic_entry = (struct acpi_madt_bio_pic *)header; > + > + return pch_pic_init_irqdomain(acpi_get_pch_parent((pch_pic_entry->address >> 44) & 0xf), > + pch_pic_entry); > +} > + > +static int __init pch_pic_acpi_init(void) > +{ > + acpi_table_parse_madt(ACPI_MADT_TYPE_BIO_PIC,
Where is this defined? It only appears in the documentation, and nowhere else...
> + pchintc_parse_madt, 0); > + > + return 0; > +} > +early_initcall(pch_pic_acpi_init);
Why can't you use IRQCHIP_ACPI_DECLARE here? This is terribly fragile, and will eventually break. I really don't want to rely on this.
M.
-- Without deviation from the norm, progress is not possible.
| |