Re: [PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers

[Marc Zyngier]

On Sat, 30 Apr 2022 09:53:34 +0100,
Huacai Chen <chenhuacai@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 next revision of ACPI Specification (current
> revision is 6.4).
> 
> 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).
> 
> CPUINTC is a per-core controller (in CPU), LIOINTC/EIOINTC/HTVECINTC are
> per-package controllers (in CPU), while PCH-PIC/PCH-LPC/PCH-MSI are all
> controllers out of CPU (i.e., in chipsets). These controllers (in other
> words, irqchips) are linked in a hierarchy, and there are two models of
> hierarchy (legacy model and extended model).
> 
> Legacy IRQ model:
> 
> In this model, the IPI (Inter-Processor Interrupt) and CPU Local Timer
> interrupt go to CPUINTC directly, CPU UARTS interrupts go to LIOINTC,
> while all other devices interrupts go to PCH-PIC/PCH-LPC/PCH-MSI and
> gathered by HTVECINTC, and then go to LIOINTC, and then CPUINTC.
> 
>  +---------------------------------------------+
>  |                                             |
>  |    +-----+     +---------+     +-------+    |
>  |    | IPI | --> | CPUINTC | <-- | Timer |    |
>  |    +-----+     +---------+     +-------+    |
>  |                     ^                       |
>  |                     |                       |
>  |                +---------+     +-------+    |
>  |                | LIOINTC | <-- | UARTs |    |
>  |                +---------+     +-------+    |
>  |                     ^                       |
>  |                     |                       |
>  |               +-----------+                 |
>  |               | HTVECINTC |                 |
>  |               +-----------+                 |
>  |                ^         ^                  |
>  |                |         |                  |
>  |          +---------+ +---------+            |
>  |          | PCH-PIC | | PCH-MSI |            |
>  |          +---------+ +---------+            |
>  |            ^     ^           ^              |
>  |            |     |           |              |
>  |    +---------+ +---------+ +---------+      |
>  |    | PCH-LPC | | Devices | | Devices |      |
>  |    +---------+ +---------+ +---------+      |
>  |         ^                                   |
>  |         |                                   |
>  |    +---------+                              |
>  |    | Devices |                              |
>  |    +---------+                              |
>  |                                             |
>  |                                             |
>  +---------------------------------------------+
> 
> Extended IRQ model:
> 
> In this model, the IPI (Inter-Processor Interrupt) and CPU Local Timer
> interrupt go to CPUINTC directly, CPU UARTS interrupts go to LIOINTC,
> while all other devices interrupts go to PCH-PIC/PCH-LPC/PCH-MSI and
> gathered by EIOINTC, and then go to to CPUINTC directly.
> 
>  +--------------------------------------------------------+
>  |                                                        |
>  |         +-----+     +---------+     +-------+          |
>  |         | IPI | --> | CPUINTC | <-- | Timer |          |
>  |         +-----+     +---------+     +-------+          |
>  |                      ^       ^                         |
>  |                      |       |                         |
>  |               +---------+ +---------+     +-------+    |
>  |               | EIOINTC | | LIOINTC | <-- | UARTs |    |
>  |               +---------+ +---------+     +-------+    |
>  |                ^       ^                               |
>  |                |       |                               |
>  |         +---------+ +---------+                        |
>  |         | PCH-PIC | | PCH-MSI |                        |
>  |         +---------+ +---------+                        |
>  |           ^     ^           ^                          |
>  |           |     |           |                          |
>  |   +---------+ +---------+ +---------+                  |
>  |   | PCH-LPC | | Devices | | Devices |                  |
>  |   +---------+ +---------+ +---------+                  |
>  |        ^                                               |
>  |        |                                               |
>  |   +---------+                                          |
>  |   | Devices |                                          |
>  |   +---------+                                          |
>  |                                                        |
>  |                                                        |
>  +--------------------------------------------------------+
> 
> This patchset adds some irqchip drivers for LoongArch, it is preparing
> to add LoongArch support in mainline kernel, we can see a snapshot here:
> https://github.com/loongson/linux/tree/loongarch-next
> 
> Cross-compile tool chain to build kernel:
> https://github.com/loongson/build-tools/releases/latest/download/loongarch64-clfs-20211202-cross-tools.tar.xz
> 
> A CLFS-based Linux distro:
> https://github.com/loongson/build-tools/releases/latest/download/loongarch64-clfs-system-2021-12-02.tar.bz2
> 
> Loongson and LoongArch documentations:
> https://github.com/loongson/LoongArch-Documentation
> 
> LoongArch-specific interrupt controllers:
> https://mantis.uefi.org/mantis/view.php?id=2203

Nothing to see here, unfortunately (you need a login to access this
information).

> 
> LoongArch use ACPI, but ACPI tables cannot describe the hierarchy of
> irqchips, so we initilize the irqchip subsystem in this way (from arch
> code):
> 
>   cpu_domain = loongarch_cpu_irq_init();
>   liointc_domain = liointc_acpi_init(cpu_domain, acpi_liointc);
>   eiointc_domain = eiointc_acpi_init(cpu_domain, acpi_eiointc);
>   pch_pic_domain = pch_pic_acpi_init(eiointc_domain, acpi_pchpic);
>   pch_msi_domain = pch_msi_acpi_init(eiointc_domain, acpi_pchmsi);

I said no to this in the past, and I'm going to reiterate: this is
*not* acceptable. This obviously doesn't scale and isn't manageable in
the long run. Hardcoding the topology and the probing order in the
kernel code has repeatedly proved to be a disaster, and yet you refuse
to take any input from past experience. This is pretty worrying.

>
> Upstream irqchip init function return an irqdomain, and this domain
> will be used by downstream irqchips as their parent domains. For more
> infomation please refer:
> https://lore.kernel.org/linux-arch/20210927064300.624279-11-chenhuacai@xxxxxxxxxxx/T/#u
> 
> Q: Why we don't declare irqchips in ACPI DSDT where hierarchy is possible?
> A: This is answered in V8 of this series as below:
> 
>   - There are several kinds of irq chips(e.g. pchpic、eiointc、cpuintc)
>   for LoongArch. SCI interrupt (Fixed hardware is implemented for
>   LoongArch in pch such as LS7A1000, and SCI interrupt is used for fixed
>   event handling.) is an irq input of pch irq chip which routes
>   interrupts to cpu as following irq chips path:
> 
>   sci interrupt->|pchpic| ->|eiointc|->|cpuintc|
> 
>   sci_interrupt will be transferred from gsi to irq through
>   acpi_gsi_to_irq in acpi_enable_subsystem called from acpi_bus_init
>   before acpi_scan_init where acpi device namespace is created, so we
>   should build pch irq domain and related upstream irq domains before
>   acpi_bus_init.
> 
>   - PCI bus enumeration is executed from acpi_scan_init, and
>   pci_set_msi_domain will be called for setting msi_domain of enumerated
>   pci device. In pci_set_msi_domain, msi domain may be got through
>   pcibios_device_add, fdt, iort(used for arm64) or inheriting from host
>   bridge domain. And in each way, the msi domain needs to be found by
>   calling irq_find_matching_fwnode(fwnode, DOMAIN_BUS_PCI_MSI) to match
>   one from the registered msi domain before. So we build the msi domain
>   as x86 and arm64 before acpi_scan_init. The msi domain is hierarchic
>   as following:
> 
>   msi interrupt->|msipic| ->|eiointc|->|cpuintc|
> 
>   - Yes, a driver can be deferred probed when get -EPROBE_DEFER on
>   probing, but both sci interrupt transfer and pci bus enumeration are
>   common code (not private driver for LoongArch).
> 
>   So, declaring pic devices in DSDT for seems not suitable, we can only
>   select the X86-like way which is a bit ugly.

ACPI *can* describe these topologies if you teach it. ARM managed to
do it with IORT, which is part of the ACPI specification. Given that
you are starting from scratch and that your bindings aren't even in
the official ACPI spec, there is zero reason not to do the right
thing.

Until then, I'm not interested in reviewing more of these patches.

Thanks,

	M.

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