Message-ID: <CAAhV-H6CJ-aF2s9D4QJ5zYZGt_=C4QHFCzKByT6urcFMgWv+4g@mail.gmail.com>
Date:   Sun, 17 Jul 2022 22:08:14 +0800
From:   Huacai Chen <chenhuacai@...nel.org>
To:     Jianmin Lv <lvjianmin@...ngson.cn>
Cc:     Marc Zyngier <maz@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev,
        Hanjun Guo <guohanjun@...wei.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V15 00/15] irqchip: Add LoongArch-related irqchip drivers

Hi, Marc, Jianmin,

On Sun, Jul 17, 2022 at 7:29 PM Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>
>
>
> On 2022/7/17 下午6:02, Marc Zyngier wrote:
> > On Sun, 17 Jul 2022 02:06:12 +0100,
> > Jianmin Lv <lvjianmin@...ngson.cn> wrote:
> >>
> >>
> >>
> >> On 2022/7/17 上午2:39, Marc Zyngier wrote:
> >>> On Fri, 15 Jul 2022 08:05:36 +0100,
> >>> Jianmin Lv <lvjianmin@...ngson.cn> 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.

I have an idea but I don't know whether it is acceptable: Marc gives
an Acked-by for the whole series, then this irqchip series goes
through the loongarch tree together with the PCI patches, then we
don't need other hacks except the ACPI definitions.

Huacai
>
> >       M.
> >
>
>

Powered by blists - more mailing lists