Re: [PATCH V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support

[Marc Zyngier <maz@kernel.org>]

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.