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

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

The hierarchy model is constructed by parsing irq contronler structures
in MADT. Some controllers((e.g. LIOINTC, HTVECINTC, EIOINTC and PCH-LPC)
are hardcodingly connected to their parents, so their irqdomins are
separately routed to their parents in a fixed way. Some controllers
(e.g. PCH-PIC and PCH-MSI) could be routed to different parents for different
CPU. The firmware will config EIOINTC for the newer CPU and config HTVECINTC
for old CPU in MADT. By this way, PCH-PIC and PCH-MSI irqdomain can only be
routed one parent irqdomin: HTVECINTC or EIOINTC.


Example of irqchip topology in a system with  two chipsets:

 +------------------------------------------------------------+
 |                                                            |
 |                     +------------------+                   |
 |                     |      CPUINTC     |                   |
 |                     +------------------+                   |
 |                     ^                  ^                   |
 |                     |                  |                   |
 |          +----------+                  +----------+        |
 |          | EIOINTC 0|                  | EIOINTC 1|        |
 |          +----------+                  +----------+        |
 |          ^          ^                  ^          ^        |
 |          |          |                  |          |        |
 | +----------+   +----------+   +----------+    +----------+ |
 | | PCH-PIC 0|   | PCH-MSI 0|   | PCH-PIC 1|    | PCH-MSI 1| |
 | +----------+   +----------+   +----------+    +----------+ |
 |                                                            |
 |                                                            |
 +------------------------------------------------------------+

For systems with two chipsets, there are tow group(consists of EIOINTC, PCH-PIC and PCH-MSI) irqdomains, 
and each group has same node id. So we defined a structure to mantain the relation of node and it's parent irqdomain.

struct acpi_vector_group {
        int node;
	int pci_segment;
        struct irq_domain *parent;
};

The initialization and use of acpi_vector_group array are following:

1 Entry of struct acpi_vector_group array initialization:

By parsing MCFG, the node id（from bit44-47 of Base Address）and pci segment are extracted. And from MADT, we have the node id of each EIOINTC.

entry.node = node id of pci segment
entry.pci_segment = pci segment (only for msi irqdomain)

By matching node id of entry and EIOINTC to set parent.

entry.parent = EIOINTC irqdomain(node id of EIOINTC == node id of pci segment)

2 Get parent irqdomain for PCH-PIC:

From MADT, we have the node id of each PCH-PIC(from bit44-47 of Base Address).
if (node of entry i == node of PCH-PIC)
	return entrys[i].parent;

3 Get parent irqdomain for PCH-MSI of pci segment:

	return entrys[i].parent; (i is the index of msi irqdomain)

4 How to select a correct irqdomain to map irq for a device?
For devices using legacy irq behind PCH-PIC, GSI is used to select correct PCH-PIC irqdomain.
For devices using msi irq behind PCH-MSI, the pci segmen of the device is used to select correct PCH-MSI irqdomain.

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.

V11 -> RFC:
1, Refactored the way to build irqchip hierarchy topology.

RFC -> RFC V2:
1, Move all IO-interrupt related code to driver/irqchip from arch directory.
2. Add description for an example of two chipsets system.

RFC V2 -> RFC V3:
1, Add support for multiple GSI domains
2, Use ACPI_GENERIC_GSI for GSI handling
3, Drop suspend-resume stuff
4, Export fwnode handles instead of irq domain handles

RFC V3 -> V12:
1, Address patch attributions of the patch series

V12 -> V13
1 Based on 5.19-rc2
2 Remove arch specified gsi code
3 Split some 'common' code into the various drivers where they belong.
4 Allow acpi_gsi_to_irq() to have an arch-specific fallback

V13 -> V14
1 Add LoongArch-specified APICs definition
2 Use the way in CPUINTC driver to call pch-pic and pch-msi entry
3 Fix compiling and regression issue for OF path

V14 -> V15
1 Expose fwnode_handle of CPUINTC domain instead of using get_xxx_irq() for CPUINTC driver
2 Fix EIOINTC driver: delete parent_data referencing and fix set_affinity bug
3 Use acpi_disabled for DT and ACPI runtime code path
4 Fix return type of arch-specific acpi_gsi_to_irq fallback 
5 Fix compile bug tested by kernel test robot

Huacai Chen (9):
  ACPICA: MADT: Add LoongArch APICs support
  irqchip: Add Loongson PCH LPC controller support
  irqchip: remove COMPILE_TEST for pch-pic and pch-msi
  irqchip/loongson-pch-pic: Add ACPI init support
  irqchip/loongson-pch-msi: Add ACPI init support
  irqchip/loongson-htvec: Add ACPI init support
  irqchip/loongson-liointc: Add ACPI init support
  irqchip: Add Loongson Extended I/O interrupt controller support
  irqchip: Add LoongArch CPU interrupt controller support

Jianmin Lv (4):
  genirq/generic_chip: export irq_unmap_generic_chip
  LoongArch: Use ACPI_GENERIC_GSI for gsi handling
  LoongArch: prepare to support multiple pch-pic and pch-msi irqdomain
  irqchip / ACPI: Introduce ACPI_IRQ_MODEL_LPIC for LoongArch

Marc Zyngier (2):
  APCI: irq: Add support for multiple GSI domains
  ACPI: irq: Allow acpi_gsi_to_irq() to have an arch-specific fallback

 arch/loongarch/Kconfig                      |   1 +
 arch/loongarch/include/asm/irq.h            |  46 ++--
 arch/loongarch/kernel/acpi.c                |  65 -----
 arch/loongarch/kernel/irq.c                 |  57 +++-
 arch/loongarch/kernel/time.c                |  14 +-
 arch/mips/include/asm/mach-loongson64/irq.h |   2 +-
 drivers/acpi/bus.c                          |   3 +
 drivers/acpi/irq.c                          |  58 ++--
 drivers/irqchip/Kconfig                     |  32 ++-
 drivers/irqchip/Makefile                    |   3 +
 drivers/irqchip/irq-gic-v3.c                |  18 +-
 drivers/irqchip/irq-gic.c                   |  18 +-
 drivers/irqchip/irq-loongarch-cpu.c         | 157 +++++++++++
 drivers/irqchip/irq-loongson-eiointc.c      | 395 ++++++++++++++++++++++++++++
 drivers/irqchip/irq-loongson-htvec.c        | 145 +++++++---
 drivers/irqchip/irq-loongson-liointc.c      | 226 ++++++++++------
 drivers/irqchip/irq-loongson-pch-lpc.c      | 208 +++++++++++++++
 drivers/irqchip/irq-loongson-pch-msi.c      | 131 ++++++---
 drivers/irqchip/irq-loongson-pch-pic.c      | 178 ++++++++++---
 include/acpi/actbl2.h                       | 127 ++++++++-
 include/linux/acpi.h                        |   4 +-
 include/linux/cpuhotplug.h                  |   1 +
 include/linux/irq.h                         |   1 +
 kernel/irq/generic-chip.c                   |   2 +-
 24 files changed, 1576 insertions(+), 316 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

-- 
1.8.3.1

On Fri, 15 Jul 2022 08:05:36 +0100,
Jianmin Lv <lvjianmin@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 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.

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

On 2022/7/17 上午2:39, Marc Zyngier wrote:
> On Fri, 15 Jul 2022 08:05:36 +0100,
> Jianmin Lv <lvjianmin@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 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.

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?

Thanks sincerely.

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

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.

	M.

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

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

On Sun, 17 Jul 2022 12:29:05 +0100,
Jianmin Lv <lvjianmin@loongson.cn> wrote:
> 
> 
> 
> On 2022/7/17 下午6:02, Marc Zyngier wrote:
> > 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.

Do you really have the two variants in the wild? Or is this just
because this is a possibility?

	M.

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

On 2022/7/17 下午10:49, Marc Zyngier wrote:
> On Sun, 17 Jul 2022 12:29:05 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>>
>>
>> On 2022/7/17 下午6:02, Marc Zyngier wrote:
>>> 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.
> 
> Do you really have the two variants in the wild? Or is this just
> because this is a possibility?
> 

Currently, there are not CPUs(used for PC and server) based on LoongArch 
shipped with only HTPIC, but with both HTPIC and EIOPIC, we just want to 
provide two choices for designers(but obviously, EIOPIC may be enough 
currently). Do you think we don't have to do like this, yes? If so, 
maybe we don't have to support ACPI-way entry for htvec currently, and 
do the work in future if required.

> 	M.
> 

On Mon, 18 Jul 2022 02:07:21 +0100,
Jianmin Lv <lvjianmin@loongson.cn> wrote:
> 
> 
> 
> On 2022/7/17 下午10:49, Marc Zyngier wrote:
> > On Sun, 17 Jul 2022 12:29:05 +0100,
> > Jianmin Lv <lvjianmin@loongson.cn> wrote:
> >> 
> >> 
> >> 
> >> On 2022/7/17 下午6:02, Marc Zyngier wrote:
> >>> 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.
> > 
> > Do you really have the two variants in the wild? Or is this just
> > because this is a possibility?
> > 
> 
> Currently, there are not CPUs(used for PC and server) based on
> LoongArch shipped with only HTPIC, but with both HTPIC and EIOPIC, we
> just want to provide two choices for designers(but obviously, EIOPIC
> may be enough currently). Do you think we don't have to do like this,
> yes? If so, maybe we don't have to support ACPI-way entry for htvec
> currently, and do the work in future if required.

If the existing HW is only following the 'Extended' model, then I'd
suggest you only support this for now. It has two effects:

- it simplifies the current code, making it more maintainable and
  easier to reason about

- it sends the message to integrators that 'Extended' is the correct
  model, and that it is what they should support

Now, we don't have much time left to get this series into -next (I
will be closing the tree to new features this week, and only queue
fixes).

So whatever you need to do, please do it quickly so that we can have
at least some of this in 5.20.

Thanks,

	M.

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

On 2022/7/18 下午2:39, Marc Zyngier wrote:
> On Mon, 18 Jul 2022 02:07:21 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>
>>
>>
>> On 2022/7/17 下午10:49, Marc Zyngier wrote:
>>> On Sun, 17 Jul 2022 12:29:05 +0100,
>>> Jianmin Lv <lvjianmin@loongson.cn> wrote:
>>>>
>>>>
>>>>
>>>> On 2022/7/17 下午6:02, Marc Zyngier wrote:
>>>>> 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.
>>>
>>> Do you really have the two variants in the wild? Or is this just
>>> because this is a possibility?
>>>
>>
>> Currently, there are not CPUs(used for PC and server) based on
>> LoongArch shipped with only HTPIC, but with both HTPIC and EIOPIC, we
>> just want to provide two choices for designers(but obviously, EIOPIC
>> may be enough currently). Do you think we don't have to do like this,
>> yes? If so, maybe we don't have to support ACPI-way entry for htvec
>> currently, and do the work in future if required.
> 
> If the existing HW is only following the 'Extended' model, then I'd
> suggest you only support this for now. It has two effects:
> 
> - it simplifies the current code, making it more maintainable and
>    easier to reason about
> 
> - it sends the message to integrators that 'Extended' is the correct
>    model, and that it is what they should support
> 
> Now, we don't have much time left to get this series into -next (I
> will be closing the tree to new features this week, and only queue
> fixes).
> 
> So whatever you need to do, please do it quickly so that we can have
> at least some of this in 5.20.
> 
> Thanks,
> 
> 	M.
> 

Ok, Marc, thanks for your suggestion, got it, I'll remove 'Legacy' mode 
support and send next version as soon as possible.

Hi, Jianmin and Marc,

On Mon, Jul 18, 2022 at 4:29 PM Jianmin Lv <lvjianmin@loongson.cn> wrote:
>
>
>
> On 2022/7/18 下午2:39, Marc Zyngier wrote:
> > On Mon, 18 Jul 2022 02:07:21 +0100,
> > Jianmin Lv <lvjianmin@loongson.cn> wrote:
> >>
> >>
> >>
> >> On 2022/7/17 下午10:49, Marc Zyngier wrote:
> >>> On Sun, 17 Jul 2022 12:29:05 +0100,
> >>> Jianmin Lv <lvjianmin@loongson.cn> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2022/7/17 下午6:02, Marc Zyngier wrote:
> >>>>> 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.
> >>>
> >>> Do you really have the two variants in the wild? Or is this just
> >>> because this is a possibility?
> >>>
> >>
> >> Currently, there are not CPUs(used for PC and server) based on
> >> LoongArch shipped with only HTPIC, but with both HTPIC and EIOPIC, we
> >> just want to provide two choices for designers(but obviously, EIOPIC
> >> may be enough currently). Do you think we don't have to do like this,
> >> yes? If so, maybe we don't have to support ACPI-way entry for htvec
> >> currently, and do the work in future if required.
> >
> > If the existing HW is only following the 'Extended' model, then I'd
> > suggest you only support this for now. It has two effects:
> >
> > - it simplifies the current code, making it more maintainable and
> >    easier to reason about
> >
> > - it sends the message to integrators that 'Extended' is the correct
> >    model, and that it is what they should support
> >
> > Now, we don't have much time left to get this series into -next (I
> > will be closing the tree to new features this week, and only queue
> > fixes).
> >
> > So whatever you need to do, please do it quickly so that we can have
> > at least some of this in 5.20.
> >
> > Thanks,
> >
> >       M.
> >
>
> Ok, Marc, thanks for your suggestion, got it, I'll remove 'Legacy' mode
> support and send next version as soon as possible.
I think keeping the "Legacy" mode is faster than removing it for now
to keep up with the merge window, since it is already here and doesn't
need to modify.

Huacai
>
>

Hi, Marc, Jianmin,

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

On Sun, 17 Jul 2022 15:08:14 +0100,
Huacai Chen <chenhuacai@kernel.org> wrote:
> 
> Hi, Marc, Jianmin,
> 
> 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.

Not sure how this solves the original problem. PCI should never be
mandatory (it is actually super useful to be able to build a very
small kernel without too many drivers), and there shouldn't be
configurations where the kernel doesn't build.

It is also my own responsibility to merge these things, and I'd rather
not delegate this, specially as it touches a bunch of other
subsystems.

Thanks,

	M.

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

Hi, Marc,

On Sun, Jul 17, 2022 at 10:43 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 17 Jul 2022 15:08:14 +0100,
> Huacai Chen <chenhuacai@kernel.org> wrote:
> >
> > Hi, Marc, Jianmin,
> >
> > 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.
>
> Not sure how this solves the original problem. PCI should never be
> mandatory (it is actually super useful to be able to build a very
> small kernel without too many drivers), and there shouldn't be
> configurations where the kernel doesn't build.
Now, the pci-loongson controller code (A) is in the PCI tree, the pci
enablement code (B) is in the LoongArch tree, and the irqchip code (C)
is in the irqchip tree. If the order for upstream is (A) --> (B) -->
(C), there will be no build error. My above idea is to make sure the
order of (B) and (C) is controlled in the same tree. PCI/MSI is a
mandatory requirement for LoongArch, so I want to avoid some
unnecessary #ifdefs.

>
> It is also my own responsibility to merge these things, and I'd rather
> not delegate this, specially as it touches a bunch of other
> subsystems.
I know, this is reasonable. Then if we can control the order of
(A)(B)(C) in three trees, the build error can be avoided in the
linux-next tree.

Huacai

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

On Mon, 18 Jul 2022 03:38:09 +0100,
Huacai Chen <chenhuacai@kernel.org> wrote:
> 
> Hi, Marc,
> 
> On Sun, Jul 17, 2022 at 10:43 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Sun, 17 Jul 2022 15:08:14 +0100,
> > Huacai Chen <chenhuacai@kernel.org> wrote:
> > >
> > > Hi, Marc, Jianmin,
> > >
> > > 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.
> >
> > Not sure how this solves the original problem. PCI should never be
> > mandatory (it is actually super useful to be able to build a very
> > small kernel without too many drivers), and there shouldn't be
> > configurations where the kernel doesn't build.
> Now, the pci-loongson controller code (A) is in the PCI tree, the pci
> enablement code (B) is in the LoongArch tree, and the irqchip code (C)
> is in the irqchip tree. If the order for upstream is (A) --> (B) -->
> (C), there will be no build error. My above idea is to make sure the
> order of (B) and (C) is controlled in the same tree. PCI/MSI is a
> mandatory requirement for LoongArch, so I want to avoid some
> unnecessary #ifdefs.
>
> >
> > It is also my own responsibility to merge these things, and I'd rather
> > not delegate this, specially as it touches a bunch of other
> > subsystems.
> I know, this is reasonable. Then if we can control the order of
> (A)(B)(C) in three trees, the build error can be avoided in the
> linux-next tree.

This would require stable branches between all three trees, as we
don't control the *order* of the merges. I'd have to carry (A) and (B)
as part of (C), which is really over the top.

Just queue a patch to remove the #ifdef once we're at -rc1 and that
things have settled down. This will be simpler for everyone, and will
allow everyone to have a clean tree without dragging tons of extra
patches.

	M.

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

Hi, Marc,

On Mon, Jul 18, 2022 at 2:43 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 18 Jul 2022 03:38:09 +0100,
> Huacai Chen <chenhuacai@kernel.org> wrote:
> >
> > Hi, Marc,
> >
> > On Sun, Jul 17, 2022 at 10:43 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Sun, 17 Jul 2022 15:08:14 +0100,
> > > Huacai Chen <chenhuacai@kernel.org> wrote:
> > > >
> > > > Hi, Marc, Jianmin,
> > > >
> > > > 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.
> > >
> > > Not sure how this solves the original problem. PCI should never be
> > > mandatory (it is actually super useful to be able to build a very
> > > small kernel without too many drivers), and there shouldn't be
> > > configurations where the kernel doesn't build.
> > Now, the pci-loongson controller code (A) is in the PCI tree, the pci
> > enablement code (B) is in the LoongArch tree, and the irqchip code (C)
> > is in the irqchip tree. If the order for upstream is (A) --> (B) -->
> > (C), there will be no build error. My above idea is to make sure the
> > order of (B) and (C) is controlled in the same tree. PCI/MSI is a
> > mandatory requirement for LoongArch, so I want to avoid some
> > unnecessary #ifdefs.
> >
> > >
> > > It is also my own responsibility to merge these things, and I'd rather
> > > not delegate this, specially as it touches a bunch of other
> > > subsystems.
> > I know, this is reasonable. Then if we can control the order of
> > (A)(B)(C) in three trees, the build error can be avoided in the
> > linux-next tree.
>
> This would require stable branches between all three trees, as we
> don't control the *order* of the merges. I'd have to carry (A) and (B)
> as part of (C), which is really over the top.
>
> Just queue a patch to remove the #ifdef once we're at -rc1 and that
> things have settled down. This will be simpler for everyone, and will
> allow everyone to have a clean tree without dragging tons of extra
> patches.
OK, I agree with your decision.

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