Re: [PATCH V15 00/15] irqchip: Add LoongArch-related irqchip drivers

[Jianmin Lv]

On 2022/7/17 下午6:02, Marc Zyngier wrote:
> On Sun, 17 Jul 2022 02:06:12 +0100,
> Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
> > On 2022/7/17 上午2:39, Marc Zyngier wrote:
> > > On Fri, 15 Jul 2022 08:05:36 +0100,
> > > Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
> > > > LoongArch is a new RISC ISA, which is a bit like MIPS or RISC-V.
> > > > LoongArch includes a reduced 32-bit version (LA32R), a standard 32-bit
> > > > version (LA32S) and a 64-bit version (LA64). LoongArch use ACPI as its
> > > > boot protocol LoongArch-specific interrupt controllers (similar to APIC)
> > > > are already added in the ACPI Specification 6.5(which may be published in
> > > > early June this year and the board is reviewing the draft).
> > > >
> > > > Currently, LoongArch based processors (e.g. Loongson-3A5000) can only
> > > > work together with LS7A chipsets. The irq chips in LoongArch computers
> > > > include CPUINTC (CPU Core Interrupt Controller), LIOINTC (Legacy I/O
> > > > Interrupt Controller), EIOINTC (Extended I/O Interrupt Controller),
> > > > HTVECINTC (Hyper-Transport Vector Interrupt Controller), PCH-PIC (Main
> > > > Interrupt Controller in LS7A chipset), PCH-LPC (LPC Interrupt Controller
> > > > in LS7A chipset) and PCH-MSI (MSI Interrupt Controller).
> > >
> > > [...]
> > >
> > > Compiling this series for loongarch with loongson3_defconfig on top of
> > > 5.19-rc3 results in the following:
> > >
> > > loongarch64-linux-ld: drivers/irqchip/irq-loongson-eiointc.o: in function `.L60':
> > > irq-loongson-eiointc.c:(.init.text+0x4c): undefined reference to `pch_msi_acpi_init'
> > > loongarch64-linux-ld: drivers/irqchip/irq-loongson-htvec.o: in function `pch_msi_parse_madt':
> > > irq-loongson-htvec.c:(.init.text+0x14): undefined reference to `pch_msi_acpi_init'
> > > make: *** [Makefile:1164: vmlinux] Error 1
> > >
> > > I *really* would have expected this series to be in a better shape
> > > after over 15 rounds, but it looks like I'm expecting too much. I
> > > haven't investigated the breakage, but this should (at the very least)
> > > pass the defconfig test and optional drivers not being selected.
> > >
> > > The corresponding MIPS configuration seems to build fine.
> > >
> > > 	M.
> >
> > Hi, Marc
> >
> > Sorry for that first, pch_msi_acpi_init is defined in pch_msi driver
> > which is compiled depend on CONFIG_PCI, and I test the patches each
> > time with PCI patches and other(or else, kernel can not be boot), so
> > I'm ok for testing the patches. The PCI patches has been accepted by
> > PCI maintainers and will be merged in this merge window.
>
> But each series *must* at the very least compile in isolation.
>
> > I don't know how to deal with this situation. Should I add *#ifdef
> > CONFIG_PCI* at position of calling pch_msi_acpi_init or some other
> > way?
>
> You could try something like this, which results in a kernel that
> fully links with defconfig and no additional patch:
>
> diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
> index ca468564fc85..4479d95867ec 100644
> --- a/arch/loongarch/include/asm/irq.h
> +++ b/arch/loongarch/include/asm/irq.h
> @@ -99,8 +99,17 @@ int htvec_acpi_init(struct irq_domain *parent,
>   					struct acpi_madt_ht_pic *acpi_htvec);
>   int pch_lpc_acpi_init(struct irq_domain *parent,
>   					struct acpi_madt_lpc_pic *acpi_pchlpc);
> +#if IS_ENABLED(CONFIG_LOONGSON_PCH_MSI)
>   int pch_msi_acpi_init(struct irq_domain *parent,
> -					struct acpi_madt_msi_pic *acpi_pchmsi);
> +		      struct acpi_madt_msi_pic *acpi_pchmsi);
> +#else
> +static inline int pch_msi_acpi_init(struct irq_domain *parent,
> +				    struct acpi_madt_msi_pic *acpi_pchmsi)
> +{
> +	return 0;
> +
> +}
> +#endif
>   int pch_pic_acpi_init(struct irq_domain *parent,
>   					struct acpi_madt_bio_pic *acpi_pchpic);
>   int find_pch_pic(u32 gsi);

Ok, thanks, I'll add this into that patch.


> But the other issue is that you seem to call this function from two
> different locations. This cannot be right, as there should be only one
> probe order, and not multiple.

As we described two IRQ models(Legacy and Extended) in this cover 
letter, the parent domain of MSI domain can be htvec domain(Legacy) or 
eiointc domain(Extended). In MADT, only one APIC(HTPIC for htvec or 
EIOPIC for eiointc) is allowed to pass into kernel, and then in the 
irqchip driver, only one kind APIC of them can be parsed from MADT, so 
we have to support two probe order for them.

> 	M.