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

Huacai Chen posted 10 patches 4 years, 1 month ago
There is a newer version of this series
drivers/irqchip/Kconfig                |  38 +++-
drivers/irqchip/Makefile               |   3 +
drivers/irqchip/irq-loongarch-cpu.c    |  92 ++++++++
drivers/irqchip/irq-loongson-eiointc.c | 372 +++++++++++++++++++++++++++++++++
drivers/irqchip/irq-loongson-htvec.c   | 146 ++++++++++---
drivers/irqchip/irq-loongson-liointc.c | 204 +++++++++++-------
drivers/irqchip/irq-loongson-pch-lpc.c | 225 ++++++++++++++++++++
drivers/irqchip/irq-loongson-pch-msi.c | 128 ++++++++----
drivers/irqchip/irq-loongson-pch-pic.c | 155 +++++++++++---
include/linux/cpuhotplug.h             |   1 +
10 files changed, 1177 insertions(+), 187 deletions(-)
create mode 100644 drivers/irqchip/irq-loongarch-cpu.c
create mode 100644 drivers/irqchip/irq-loongson-eiointc.c
create mode 100644 drivers/irqchip/irq-loongson-pch-lpc.c
[PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers
Posted by Huacai Chen 4 years, 1 month ago
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

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);

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@loongson.cn/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.

Attention: CPUINTC is CSR.ECFG/CSR.ESTAT and its interrupt controller
described in Section 7.4 of "LoongArch Reference Manual, Vol 1"; LIOINTC
is "Legacy I/O Interrupts" described in Section 11.1 of "Loongson 3A5000
Processor Reference Manual"; EIOINTC is "Extended I/O Interrupts" described
in Section 11.2 of "Loongson 3A5000 Processor Reference Manual"; HTVECINTC
is "HyperTransport Interrupts" described in Section 14.3 of "Loongson 3A5000
Processor Reference Manual"; PCH-PIC/PCH-MSI is "Interrupt Controller"
described in Section 5 of "Loongson 7A1000 Bridge User Manual"; PCH-LPC
is "LPC Interrupts" described in Section 24.3 of "Loongson 7A1000 Bridge
User Manual".

V1 -> V2:
1, Remove queued patches;
2, Move common logic of DT/ACPI probing to common functions;
3, Split .suspend()/.resume() functions to separate patches.

V2 -> V3:
1, Fix a bug for loongson-pch-pic probe;
2, Some minor improvements for LPC controller.

V3 -> V4:
1, Rework the CPU interrupt controller driver;
2, Some minor improvements for other controllers.

V4 -> V5:
1, Add a description of LoonArch's IRQ model;
2, Support multiple EIOINTCs in one system;
3, Some minor improvements for other controllers.

V5 -> V6:
1, Attach a fwnode to CPUINTC irq domain;
2, Use raw spinlock instead of generic spinlock;
3, Improve the method of restoring EIOINTC state;
4, Update documentation, comments and commit messages.

V6 -> V7:
1, Fix build warnings reported by kernel test robot.

V7 -> V8:
1, Add arguments sanity checking for irqchip init functions;
2, Support Loongson-3C5000 (One NUMA Node includes 4 EIOINTC Node).

V8 -> V9:
1, Rebase on 5.17-rc5;
2, Update cover letter;
3, Some small improvements.

V9 -> V10:
1, Rebase on 5.17-rc6;
2, Fix build warnings reported by kernel test robot.

V10 -> V11:
1, Rebase on 5.18-rc4;
2, Fix irq affinity setting for EIOINTC;
3, Fix hwirq allocation failure for EIOINTC.

Huacai Chen:
 irqchip: Adjust Kconfig for Loongson.
 irqchip/loongson-pch-pic: Add ACPI init support.
 irqchip/loongson-pch-pic: Add suspend/resume support.
 irqchip/loongson-pch-msi: Add ACPI init support.
 irqchip/loongson-htvec: Add ACPI init support.
 irqchip/loongson-htvec: Add suspend/resume support.
 irqchip/loongson-liointc: Add ACPI init support. 
 irqchip: Add LoongArch CPU interrupt controller support.
 irqchip: Add Loongson Extended I/O interrupt controller.
 irqchip: Add Loongson PCH LPC controller support.

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/irqchip/Kconfig                |  38 +++-
 drivers/irqchip/Makefile               |   3 +
 drivers/irqchip/irq-loongarch-cpu.c    |  92 ++++++++
 drivers/irqchip/irq-loongson-eiointc.c | 372 +++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-loongson-htvec.c   | 146 ++++++++++---
 drivers/irqchip/irq-loongson-liointc.c | 204 +++++++++++-------
 drivers/irqchip/irq-loongson-pch-lpc.c | 225 ++++++++++++++++++++
 drivers/irqchip/irq-loongson-pch-msi.c | 128 ++++++++----
 drivers/irqchip/irq-loongson-pch-pic.c | 155 +++++++++++---
 include/linux/cpuhotplug.h             |   1 +
 10 files changed, 1177 insertions(+), 187 deletions(-)
 create mode 100644 drivers/irqchip/irq-loongarch-cpu.c
 create mode 100644 drivers/irqchip/irq-loongson-eiointc.c
 create mode 100644 drivers/irqchip/irq-loongson-pch-lpc.c
--
2.27.0

Re: [PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers
Posted by Marc Zyngier 4 years, 1 month ago
On Sat, 30 Apr 2022 09:53:34 +0100,
Huacai Chen <chenhuacai@loongson.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 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@loongson.cn/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.
Re: [PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers
Posted by Jiaxun Yang 4 years ago

在 2022/5/5 16:58, Marc Zyngier 写道:
>> 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.
Just my two cents here.

Those drivers have such a topology just because this was my design to
handle irqchip differences between RS780E and LS7A for MIPS-era Loongson.

TBH, for LoongArch-era Loongson, they should be handled by the same driver,
cuz the topology behind them just looks like GIC PPI SPI and MSI for Arm 
GIC.

PCH PIC and eiointc in combination relays interrupts from peripherals 
just like SPI.
liointc is doing the PPI job. They are not separated modules in 
hardware, they are
interlocked.

The system should be treated as a whole, pretty much like how we see 
Arm's GIC. The
topology will last forever for every ACPI enabled LoongArch PC.

I see no reason they should be described separately. Adding complicities to
ACPI bindings brings no benefit. Changing ACPI binding which is already in
final draft stage can only leave us with chaos.

Disclaimer: I don't have any connections with Loongson currently. I'm 
standing
at contributor's approach to make statement above.

Thanks.
- Jiaxun
Re: [PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers
Posted by Marc Zyngier 4 years ago
+ Jianmin Lv

On Fri, 20 May 2022 16:04:28 +0100,
Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> 
> 
> 在 2022/5/5 16:58, Marc Zyngier 写道:
> >> 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.
> Just my two cents here.
> 
> Those drivers have such a topology just because this was my design to
> handle irqchip differences between RS780E and LS7A for MIPS-era Loongson.
> 
> TBH, for LoongArch-era Loongson, they should be handled by the same driver,
> cuz the topology behind them just looks like GIC PPI SPI and MSI for
> Arm GIC.
> 
> PCH PIC and eiointc in combination relays interrupts from
> peripherals just like SPI.  liointc is doing the PPI job. They are
> not separated modules in hardware, they are interlocked.

That was my impression too, but I keep getting pushback on that. I
wouldn't mind leaving the existing drivers for MIPS only and get new
ones for Loongson if that made things clearer.

> The system should be treated as a whole, pretty much like how we see
> Arm's GIC. The topology will last forever for every ACPI enabled
> LoongArch PC.
> 
> I see no reason they should be described separately. Adding complicities to
> ACPI bindings brings no benefit. Changing ACPI binding which is already in
> final draft stage can only leave us with chaos.

OK. So how do we move forward? You seem to have a good grasp on how
this should be structured, so can you work with Jianmin Lv to make
some progress on this?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
Re: [PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers
Posted by Huacai Chen 3 years, 12 months ago
Hi, Marc,

On Thu, Jun 9, 2022 at 8:01 PM Marc Zyngier <maz@kernel.org> wrote:
>
> + Jianmin Lv
>
> On Fri, 20 May 2022 16:04:28 +0100,
> Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> >
> >
> >
> > 在 2022/5/5 16:58, Marc Zyngier 写道:
> > >> 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.
> > Just my two cents here.
> >
> > Those drivers have such a topology just because this was my design to
> > handle irqchip differences between RS780E and LS7A for MIPS-era Loongson.
> >
> > TBH, for LoongArch-era Loongson, they should be handled by the same driver,
> > cuz the topology behind them just looks like GIC PPI SPI and MSI for
> > Arm GIC.
> >
> > PCH PIC and eiointc in combination relays interrupts from
> > peripherals just like SPI.  liointc is doing the PPI job. They are
> > not separated modules in hardware, they are interlocked.
>
> That was my impression too, but I keep getting pushback on that. I
> wouldn't mind leaving the existing drivers for MIPS only and get new
> ones for Loongson if that made things clearer.
>
> > The system should be treated as a whole, pretty much like how we see
> > Arm's GIC. The topology will last forever for every ACPI enabled
> > LoongArch PC.
> >
> > I see no reason they should be described separately. Adding complicities to
> > ACPI bindings brings no benefit. Changing ACPI binding which is already in
> > final draft stage can only leave us with chaos.
>
> OK. So how do we move forward? You seem to have a good grasp on how
> this should be structured, so can you work with Jianmin Lv to make
> some progress on this?
Thank you for helping us to move this forward, and I have some
considerations below:
1, Jianmin Lv and I have both made some effort on the irqchip driver
design, but Jianmin more or less lacks some community experience, so
he has made some mistakes as most new-comers. I think he will improve
in the future.
2, And then, maybe we can temporarily ignore those new-comers'
mistakes and focus on the key problem. I think the key problem is that
our irqchip topology is fixed in hardware (not re-organizable, which
can nearly be treated as a whole, as Jiaxun described before), and the
ACPI spec is frozen in late 2021.
3, To solve the key problem, Jianmin and I propose two different
designs. My solution is in V11 and previous versions: initialize the
root irqchip in the arch code, and return the root domain, then the
downstream irqchip takes the root domain as its parent to initialize
itself, and so on. While Jianmin's design is in RFC versions: the arch
code only calls irqchip_init() which causes it to initialize the root
irqchip, and the root irqchip driver explicitly calls its downstream
irqchips' initialization. I think this is the main difference between
them.
4, There is coupling between arch and driver in my solution, and there
is coupling between one driver and some other drivers in Jianmin's
solution. Both of the two solutions are not perfect (or we can even
say they are ugly). Which one do you think is a little better?


Huacai

>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.
Re: [PATCH V11 00/10] irqchip: Add LoongArch-related irqchip drivers
Posted by Jianmin Lv 4 years, 1 month ago
On 2022/5/5 下午11:58, Marc Zyngier wrote:
> On Sat, 30 Apr 2022 09:53:34 +0100,
> Huacai Chen <chenhuacai@loongson.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 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).
> 

Sorry, the link of mantis needs login, LoongArch-specific interrupt 
controllers related content has been added into ACPI spec 6.5 draft, 
working group review of which has been done, and the board will start a 
30-day legal review period, so spec 6.5 publication will be at some time 
in early Jun.

>>
>> 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.
> 

I completely agree with you about topology scaling, and we will change 
this according to LoongArch hardware feature as following:

- Delete fixed hierarchy code in arch file, and only call irqchip_init() 
to init irqchip controllers.
- Use IRQCHIP_ACPI_DECLARE to declare top-level core pic.
- For some controllers(lpc pic, lio pic, eio pic and ht pic) 
hardcodingly casecaded to parent one in LoongArch hardware,  we'll 
initialize them (if they are found in MADT) in their parent directly.
- For some controllers(bio pic and msi pic) cascading to different 
parent for different CPU, we'll initialize them (if they are found in 
MADT) in their candidate parent in term of CPU hardware feature 
information(e.g. support eio pic )

By this way, the irqchip hierarchy topology will be scaled automatically.

>>
>> 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@loongson.cn/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.
> 

I understand your viewpoint, we'll change it and make it to be more 
reasonable, thanks very much for your suggestion again, and please 
checkout if what I said above is in line with your opinion.