Re: [PATCH V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support

From: Jianmin Lv
Date: Thu Jun 30 2022 - 04:38:13 EST




On 2022/6/30 下午3:22, Marc Zyngier wrote:
On Thu, 30 Jun 2022 05:36:40 +0100,
Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:



[...]

+static int __init pch_pic_acpi_init(void)
+{
+ acpi_table_parse_madt(ACPI_MADT_TYPE_BIO_PIC,

Where is this defined? It only appears in the documentation, and
nowhere else...


It's defined in the patch to ACPICA(which has been merged, please to
see
https://github.com/acpica/acpica/commit/1dc530059a3e6202e941e6a9478cf30f092bfb47).
the patch will be synchronized to linux kernel by maintainer of ACPICA.

How can I take this patch upstream if it doesn't even compile? Please
make this patch part of your series. There are tons of patches that
need Acks from the ACPI maintainers, this is only one of them.


Ok, I'll put the patch into my series.




+ pchintc_parse_madt, 0);
+
+ return 0;
+}
+early_initcall(pch_pic_acpi_init);

Why can't you use IRQCHIP_ACPI_DECLARE here? This is terribly fragile,
and will eventually break. I really don't want to rely on this.


In early time, the change here is implemented using
IRQCHIP_ACPI_DECLARE, but we found that calling order(during
irqchip_init) of the entry declared using IRQCHIP_ACPI_DECLARE is
depended on the compiling order(driver order in Makefile) of the
driver. For removing the dependency to the compiling order, the new
way here is used(I looked into ARM, it seems that GIC driver uses
IRQCHIP_ACPI_DECLARE, and ITS driver uses early_initcall too.).

It's not quite the same. The ITS part that uses early_initcall isn't
an actual driver (it only registers a domain, and nothing else).


Ok, thanks, I got it, I don't understand ARM's irqchips well.

There is also no guarantee that the initcalls at the same priority
will execute in any order, and you already have at least two such
initcalls (pch_pic_acpi_init, pch_msi_acpi_init, and I haven't quite
understood how the rest is probed yet).


Yes, agree, but pch_pic and pch_msi are independent, and there is no
dependencies between them, so there is no requirement on calling order for the two entries.

One possibility would be to drive the whole probing from the root
interrupt controller, which is similar to what GICv3 does for the
actual ITS driver. You already do this sort of stuff in the CPUINTC
driver, so adding to this shouldn't be too hard.


Ok, thanks for your suggestion, I'll try to change it as the way in the CPUINTC driver, it seems that way is better.

M.