Re: [PATCH V12 04/10] irqchip: create library file for LoongArch irqchip driver

[Jianmin Lv <lvjianmin@loongson.cn>]

On 2022/6/19 上午1:22, Marc Zyngier wrote:
> On Wed, 15 Jun 2022 07:07:24 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>> The library file contains following content:
>> - Implement acpi_get_gsi_domain_id callback.
>> - Implement initialization of vector group entries and APIs
>>    for building hierachy irqdomains.
>>
>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>> ---
>>   drivers/irqchip/Makefile                   |   2 +-
>>   drivers/irqchip/irq-loongarch-pic-common.c | 122 +++++++++++++++++++++++++++++
>>   drivers/irqchip/irq-loongarch-pic-common.h |  39 +++++++++
>>   3 files changed, 162 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/irqchip/irq-loongarch-pic-common.c
>>   create mode 100644 drivers/irqchip/irq-loongarch-pic-common.h
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 6894a13..2d0d871 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -103,7 +103,7 @@ obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
>>   obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
>>   obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
>>   obj-$(CONFIG_TI_PRUSS_INTC)		+= irq-pruss-intc.o
>> -obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+= irq-loongarch-cpu.o
>> +obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+= irq-loongarch-cpu.o irq-loongarch-pic-common.o
>>   obj-$(CONFIG_LOONGSON_LIOINTC)		+= irq-loongson-liointc.o
>>   obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
>>   obj-$(CONFIG_LOONGSON_HTVEC)		+= irq-loongson-htvec.o
>> diff --git a/drivers/irqchip/irq-loongarch-pic-common.c b/drivers/irqchip/irq-loongarch-pic-common.c
>> new file mode 100644
>> index 0000000..2f75362
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-loongarch-pic-common.c
>> @@ -0,0 +1,122 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2022 Loongson Limited, All Rights Reserved.
>> + */
>> +
>> +#include <linux/irq.h>
>> +#include <linux/pci.h>
>> +#include <linux/acpi.h>
>> +#include "irq-loongarch-pic-common.h"
>> +
>> +static struct acpi_vector_group vector_group[MAX_IO_PICS];
>> +
>> +struct acpi_madt_bio_pic *acpi_pchpic[MAX_IO_PICS];
>> +
>> +struct fwnode_handle *liointc_handle;
>> +struct fwnode_handle *pch_lpc_handle;
>> +struct fwnode_handle *pch_msi_handle[MAX_IO_PICS];
>> +struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
> 
> Why aren't these in individual drivers, and then have accessors to
> retrieve them?
> 

Ok, I'll try to put them in individual drivers.


>> +
>> +static int find_pch_pic(u32 gsi)
>> +{
>> +	int i, start, end;
>> +
>> +	/* Find the PCH_PIC that manages this GSI. */
>> +	for (i = 0; i < MAX_IO_PICS; i++) {
>> +		struct acpi_madt_bio_pic *irq_cfg = acpi_pchpic[i];
>> +
>> +		if (!irq_cfg)
>> +			return -1;
>> +
>> +		start = irq_cfg->gsi_base;
>> +		end   = irq_cfg->gsi_base + irq_cfg->size;
>> +		if (gsi >= start && gsi < end)
>> +			return i;
>> +	}
>> +
>> +	pr_err("ERROR: Unable to locate PCH_PIC for GSI %d\n", gsi);
>> +	return -1;
>> +}
> 
> Same thing. This really should be in the PCH driver, and be called by
> lpic_get_gsi_domain().
> 

Ok, I'll try to put them in PCH driver.


>> +
>> +struct fwnode_handle *lpic_get_gsi_domain_id(u32 gsi)
>> +{
>> +	int id;
>> +	struct fwnode_handle *domain_handle = NULL;
>> +
>> +	switch (gsi) {
>> +	case GSI_MIN_CPU_IRQ ... GSI_MAX_CPU_IRQ:
>> +		if (liointc_handle)
>> +			domain_handle = liointc_handle;
>> +		break;
>> +
>> +	case GSI_MIN_LPC_IRQ ... GSI_MAX_LPC_IRQ:
>> +		if (pch_lpc_handle)
>> +			domain_handle = pch_lpc_handle;
>> +		break;
>> +
>> +	case GSI_MIN_PCH_IRQ ... GSI_MAX_PCH_IRQ:
>> +		id = find_pch_pic(gsi);
>> +		if (id >= 0 && pch_pic_handle[id])
>> +			domain_handle = pch_pic_handle[id];
>> +
>> +		break;
>> +	}
>> +
>> +	return domain_handle;
>> +}
>> +
>> +static int pci_mcfg_parse(struct acpi_table_header *header)
>> +{
>> +	struct acpi_table_mcfg *mcfg;
>> +	struct acpi_mcfg_allocation *mptr;
>> +	int i, n;
>> +
>> +	if (header->length < sizeof(struct acpi_table_mcfg))
>> +		return -EINVAL;
>> +
>> +	n = (header->length - sizeof(struct acpi_table_mcfg)) /
>> +					sizeof(struct acpi_mcfg_allocation);
>> +	mcfg = (struct acpi_table_mcfg *)header;
>> +	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +
>> +	for (i = 0; i < n; i++, mptr++)
>> +		vector_group[mptr->pci_segment].node = (mptr->address >> 44) & 0xf;
>> +
>> +	return 0;
>> +}
>> +
>> +void __init init_vector_parent_group(void)
>> +{
>> +	acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
>> +}
> 
> I really don't think the PCI code should be anywhere near
> this. Frankly, this file looks like a dumping ground for totally
> unrelated stuff.
> 


For ARM, the msi domain of a pci device is matched by comparing PCI 
segment of it and PCI segment of msi domain's parent domain(its domain).
The PCI segment number here is as in MCFG and as returned by _SEG in the 
namespace.

For LoongArch, a similar way is used, but we don't use
IORT-like table, rather, we directly used PCI segment and Base 
address(node information is in bit44-47 of the address) in MCFG for it, 
so we need to get them by early parsing MCFG before creating MSI and PCH 
irqdomain(MSI and PCH irqdomain have the same node as their parent
irqdomain).

If the related code is not suitable to be here, Maybe I should put them 
in arch/loongarch/kernel/irq.c and call init_vector_parent_group before 
irqchip_init().


>> +
>> +void acpi_set_vector_parent(int node, struct irq_domain *parent)
>> +{
>> +	int i;
>> +
>> +	if (cpu_has_flatmode)
>> +		node = cpu_to_node(node * CORES_PER_EIO_NODE);
>> +
>> +	for (i = 0; i < MAX_IO_PICS; i++) {
>> +		if (node == vector_group[i].node) {
>> +			vector_group[i].parent = parent;
>> +			return;
>> +		}
>> +	}
>> +}
>> +
>> +struct irq_domain *acpi_get_msi_parent(int index)
>> +{
>> +	return vector_group[index].parent;
>> +}
>> +
>> +struct irq_domain *acpi_get_pch_parent(int node)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < MAX_IO_PICS; i++) {
>> +		if (node == vector_group[i].node)
>> +			return vector_group[i].parent;
>> +	}
>> +	return NULL;
>> +}
> 
> Same thing. There is nothing "common" here. All this should be split
> in the various drivers, where they belong.
> 

Ok, I'll try to put them in individual drivers. So the "common" file 
here will be not needed any more.


> 	M.
>