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

[Jianmin Lv]

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.