Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 08:34:14 +0100
Message-ID: <87bkue36h5.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Jianmin Lv <lvjianmin@loongson.cn>
Cc:     Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
        Hanjun Guo <guohanjun@huawei.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>,
        Huacai Chen <chenhuacai@loongson.cn>
Subject: Re: [PATCH V12 04/10] irqchip: create library file for LoongArch irqchip driver
In-Reply-To: <7fa780d5-406d-6112-e4b4-b56ad291353f@loongson.cn>
References: <1655273250-23495-1-git-send-email-lvjianmin@loongson.cn>
        <1655273250-23495-5-git-send-email-lvjianmin@loongson.cn>
        <877d5dga4k.wl-maz@kernel.org>
        <7fa780d5-406d-6112-e4b4-b56ad291353f@loongson.cn>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 20 Jun 2022 04:12:10 +0100,
Jianmin Lv <lvjianmin@loongson.cn> wrote:
>=20
>=20
>=20
> On 2022/6/19 =E4=B8=8A=E5=8D=881:22, Marc Zyngier wrote:
> > On Wed, 15 Jun 2022 07:07:24 +0100,
> > Jianmin Lv <lvjianmin@loongson.cn> wrote:
> >>=20
> >> 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.
> >>=20
> >> 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
> >>=20
> >> 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)			+=3D irq-ls1x.o
> >>   obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+=3D irq-ti-sci-intr.o
> >>   obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+=3D irq-ti-sci-inta.o
> >>   obj-$(CONFIG_TI_PRUSS_INTC)		+=3D irq-pruss-intc.o
> >> -obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+=3D irq-loongarch-cpu.o
> >> +obj-$(CONFIG_IRQ_LOONGARCH_CPU)		+=3D irq-loongarch-cpu.o irq-loongar=
ch-pic-common.o
> >>   obj-$(CONFIG_LOONGSON_LIOINTC)		+=3D irq-loongson-liointc.o
> >>   obj-$(CONFIG_LOONGSON_HTPIC)		+=3D irq-loongson-htpic.o
> >>   obj-$(CONFIG_LOONGSON_HTVEC)		+=3D irq-loongson-htvec.o
> >> diff --git a/drivers/irqchip/irq-loongarch-pic-common.c b/drivers/irqc=
hip/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];
> >=20
> > Why aren't these in individual drivers, and then have accessors to
> > retrieve them?
> >=20
>=20
> Ok, I'll try to put them in individual drivers.
>=20
>=20
> >> +
> >> +static int find_pch_pic(u32 gsi)
> >> +{
> >> +	int i, start, end;
> >> +
> >> +	/* Find the PCH_PIC that manages this GSI. */
> >> +	for (i =3D 0; i < MAX_IO_PICS; i++) {
> >> +		struct acpi_madt_bio_pic *irq_cfg =3D acpi_pchpic[i];
> >> +
> >> +		if (!irq_cfg)
> >> +			return -1;
> >> +
> >> +		start =3D irq_cfg->gsi_base;
> >> +		end   =3D irq_cfg->gsi_base + irq_cfg->size;
> >> +		if (gsi >=3D start && gsi < end)
> >> +			return i;
> >> +	}
> >> +
> >> +	pr_err("ERROR: Unable to locate PCH_PIC for GSI %d\n", gsi);
> >> +	return -1;
> >> +}
> >=20
> > Same thing. This really should be in the PCH driver, and be called by
> > lpic_get_gsi_domain().
> >=20
>=20
> Ok, I'll try to put them in PCH driver.
>=20
>=20
> >> +
> >> +struct fwnode_handle *lpic_get_gsi_domain_id(u32 gsi)
> >> +{
> >> +	int id;
> >> +	struct fwnode_handle *domain_handle =3D NULL;
> >> +
> >> +	switch (gsi) {
> >> +	case GSI_MIN_CPU_IRQ ... GSI_MAX_CPU_IRQ:
> >> +		if (liointc_handle)
> >> +			domain_handle =3D liointc_handle;
> >> +		break;
> >> +
> >> +	case GSI_MIN_LPC_IRQ ... GSI_MAX_LPC_IRQ:
> >> +		if (pch_lpc_handle)
> >> +			domain_handle =3D pch_lpc_handle;
> >> +		break;
> >> +
> >> +	case GSI_MIN_PCH_IRQ ... GSI_MAX_PCH_IRQ:
> >> +		id =3D find_pch_pic(gsi);
> >> +		if (id >=3D 0 && pch_pic_handle[id])
> >> +			domain_handle =3D 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 =3D (header->length - sizeof(struct acpi_table_mcfg)) /
> >> +					sizeof(struct acpi_mcfg_allocation);
> >> +	mcfg =3D (struct acpi_table_mcfg *)header;
> >> +	mptr =3D (struct acpi_mcfg_allocation *) &mcfg[1];
> >> +
> >> +	for (i =3D 0; i < n; i++, mptr++)
> >> +		vector_group[mptr->pci_segment].node =3D (mptr->address >> 44) & 0x=
f;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +void __init init_vector_parent_group(void)
> >> +{
> >> +	acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
> >> +}
> >=20
> > 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.
> >=20
>=20
>=20
> 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.
>=20
> 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).
>=20
> 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().

That'd probably be better. This really is arch-specific stuff, and not
interrupt-controller related.

	M.

--=20
Without deviation from the norm, progress is not possible.