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

[Jianmin Lv <lvjianmin@loongson.cn>]

On 2022/6/30 下午3:22, Marc Zyngier wrote:
> On Thu, 30 Jun 2022 05:36:40 +0100,
> Jianmin Lv <lvjianmin@loongson.cn> 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.
>