[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Ard Biesheuvel]

Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
based ACPI devices, replacing the dummy fwnode with the one populated by
the ACPI device core.

This allows the SynQuacer ACPI tables to publish a device node such
as

    Device (ITS0) {
      Name (_HID, "SCX0005")
      Name (_ADR, 0x30020000)
      Name (_DSD, Package ()  // _DSD: Device-Specific Data
      {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
          Package (2) {
            "socionext,synquacer-pre-its",
            Package () { 0x58000000, 0x200000 }
          },
        }
      })
    }

which will trigger the existing quirk that replaces the doorbell
address with the appropriate address in the pre-ITS frame.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Marc, Lorenzo,

I am aware that this patch may be seen as controversial, but I would like to
propose it nonetheless. The reason is that this is the only thing standing in
the way of full ACPI support in Socionext SynQuacer based platforms.

The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
ARM designing it, and the reason we need DT/ACPI based quirks in the first
place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
one (as they designed the IP)

Please take this into consideration when reviewing this patch,

Thanks,
Ard.

 drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06f025fd5726..a63973baf08a 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3517,3 +3517,42 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 
 	return 0;
 }
+
+#if defined(CONFIG_SOCIONEXT_SYNQUACER_PREITS) && defined(CONFIG_ACPI)
+static acpi_status __init acpi_its_device_probe (acpi_handle handle,
+						 u32 depth, void *context,
+						 void **ret)
+{
+	struct acpi_device *adev;
+	unsigned long long phys_base;
+	struct its_node *its;
+	acpi_status status;
+	int err;
+
+	err = acpi_bus_get_device(handle, &adev);
+	if (err)
+		return AE_CTRL_TERMINATE;
+
+	status = acpi_evaluate_integer(handle, "_ADR", NULL, &phys_base);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	list_for_each_entry(its, &its_nodes, entry)
+		if (its->phys_base == phys_base) {
+			irq_domain_free_fwnode(its->fwnode_handle);
+			its->fwnode_handle = &adev->fwnode;
+			its_enable_quirk_socionext_synquacer(its);
+			break;
+		}
+
+	return AE_CTRL_TERMINATE;
+}
+
+static int __init acpi_its_device_probe_init(void)
+{
+	if (!acpi_disabled)
+		acpi_get_devices("SCX0005", acpi_its_device_probe, NULL, NULL);
+	return 0;
+}
+subsys_initcall_sync(acpi_its_device_probe_init);
+#endif
-- 
2.11.0

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Ard Biesheuvel]

On 13 February 2018 at 14:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
> based ACPI devices, replacing the dummy fwnode with the one populated by
> the ACPI device core.
>
> This allows the SynQuacer ACPI tables to publish a device node such
> as
>
>     Device (ITS0) {
>       Name (_HID, "SCX0005")
>       Name (_ADR, 0x30020000)
>       Name (_DSD, Package ()  // _DSD: Device-Specific Data
>       {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>           Package (2) {
>             "socionext,synquacer-pre-its",
>             Package () { 0x58000000, 0x200000 }
>           },
>         }
>       })
>     }
>
> which will trigger the existing quirk that replaces the doorbell
> address with the appropriate address in the pre-ITS frame.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Marc, Lorenzo,
>
> I am aware that this patch may be seen as controversial, but I would like to
> propose it nonetheless. The reason is that this is the only thing standing in
> the way of full ACPI support in Socionext SynQuacer based platforms.
>
> The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
> ARM designing it, and the reason we need DT/ACPI based quirks in the first
> place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
> one (as they designed the IP)
>
> Please take this into consideration when reviewing this patch,
>

Ping?

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Marc Zyngier]

Hi Ard,

On 13/02/18 14:11, Ard Biesheuvel wrote:
> Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
> based ACPI devices, replacing the dummy fwnode with the one populated by
> the ACPI device core.
> 
> This allows the SynQuacer ACPI tables to publish a device node such
> as
> 
>     Device (ITS0) {
>       Name (_HID, "SCX0005")
>       Name (_ADR, 0x30020000)
>       Name (_DSD, Package ()  // _DSD: Device-Specific Data
>       {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>           Package (2) {
>             "socionext,synquacer-pre-its",
>             Package () { 0x58000000, 0x200000 }
>           },
>         }
>       })
>     }
> 
> which will trigger the existing quirk that replaces the doorbell
> address with the appropriate address in the pre-ITS frame.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Marc, Lorenzo,
> 
> I am aware that this patch may be seen as controversial, but I would like to
> propose it nonetheless. The reason is that this is the only thing standing in
> the way of full ACPI support in Socionext SynQuacer based platforms.
> 
> The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
> ARM designing it, and the reason we need DT/ACPI based quirks in the first
> place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
> one (as they designed the IP)

That's odd. A bit of archaeology shows that ARM indeed designed a
pre-ITS, but that one doesn't break isolation at all (it still has a
single doorbell). So whatever creative changes Socionext applied to that
piece of IP (assuming this is the same IP), they didn't really
understand the far reaching impact it has.

> 
> Please take this into consideration when reviewing this patch,
> 
> Thanks,
> Ard.
> 
>  drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..a63973baf08a 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3517,3 +3517,42 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  
>  	return 0;
>  }
> +
> +#if defined(CONFIG_SOCIONEXT_SYNQUACER_PREITS) && defined(CONFIG_ACPI)
> +static acpi_status __init acpi_its_device_probe (acpi_handle handle,
> +						 u32 depth, void *context,
> +						 void **ret)
> +{
> +	struct acpi_device *adev;
> +	unsigned long long phys_base;
> +	struct its_node *its;
> +	acpi_status status;
> +	int err;
> +
> +	err = acpi_bus_get_device(handle, &adev);
> +	if (err)
> +		return AE_CTRL_TERMINATE;
> +
> +	status = acpi_evaluate_integer(handle, "_ADR", NULL, &phys_base);
> +	if (ACPI_FAILURE(status))
> +		return status;
> +
> +	list_for_each_entry(its, &its_nodes, entry)
> +		if (its->phys_base == phys_base) {
> +			irq_domain_free_fwnode(its->fwnode_handle);

That line scares me. What about irq domains that are hold a pointer to
this handle? its_init_domain() uses it to construct the LPI domain, and
it is now pointing to some free memory.

You'd need to reassign all the domains that match this fwnode before
freeing it.

> +			its->fwnode_handle = &adev->fwnode;
> +			its_enable_quirk_socionext_synquacer(its);
> +			break;
> +		}
> +
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static int __init acpi_its_device_probe_init(void)
> +{
> +	if (!acpi_disabled)
> +		acpi_get_devices("SCX0005", acpi_its_device_probe, NULL, NULL);
> +	return 0;
> +}
> +subsys_initcall_sync(acpi_its_device_probe_init);
> +#endif
> 

Is there any chance that MSIs could be allocated before this kicks in?
If that happens, we're in trouble...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Ard Biesheuvel]

On 26 February 2018 at 10:18, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Ard,
>
> On 13/02/18 14:11, Ard Biesheuvel wrote:
>> Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
>> based ACPI devices, replacing the dummy fwnode with the one populated by
>> the ACPI device core.
>>
>> This allows the SynQuacer ACPI tables to publish a device node such
>> as
>>
>>     Device (ITS0) {
>>       Name (_HID, "SCX0005")
>>       Name (_ADR, 0x30020000)
>>       Name (_DSD, Package ()  // _DSD: Device-Specific Data
>>       {
>>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>         Package () {
>>           Package (2) {
>>             "socionext,synquacer-pre-its",
>>             Package () { 0x58000000, 0x200000 }
>>           },
>>         }
>>       })
>>     }
>>
>> which will trigger the existing quirk that replaces the doorbell
>> address with the appropriate address in the pre-ITS frame.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> Marc, Lorenzo,
>>
>> I am aware that this patch may be seen as controversial, but I would like to
>> propose it nonetheless. The reason is that this is the only thing standing in
>> the way of full ACPI support in Socionext SynQuacer based platforms.
>>
>> The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
>> ARM designing it, and the reason we need DT/ACPI based quirks in the first
>> place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
>> one (as they designed the IP)
>
> That's odd. A bit of archaeology shows that ARM indeed designed a
> pre-ITS, but that one doesn't break isolation at all (it still has a
> single doorbell). So whatever creative changes Socionext applied to that
> piece of IP (assuming this is the same IP), they didn't really
> understand the far reaching impact it has.
>

OK, thanks for digging that up. All the information I have on this
topic is second hand, and I had no reason to assume their account of
the history was inaccurate.

>>
>> Please take this into consideration when reviewing this patch,
>>
>> Thanks,
>> Ard.
>>
>>  drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..a63973baf08a 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -3517,3 +3517,42 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>
>>       return 0;
>>  }
>> +
>> +#if defined(CONFIG_SOCIONEXT_SYNQUACER_PREITS) && defined(CONFIG_ACPI)
>> +static acpi_status __init acpi_its_device_probe (acpi_handle handle,
>> +                                              u32 depth, void *context,
>> +                                              void **ret)
>> +{
>> +     struct acpi_device *adev;
>> +     unsigned long long phys_base;
>> +     struct its_node *its;
>> +     acpi_status status;
>> +     int err;
>> +
>> +     err = acpi_bus_get_device(handle, &adev);
>> +     if (err)
>> +             return AE_CTRL_TERMINATE;
>> +
>> +     status = acpi_evaluate_integer(handle, "_ADR", NULL, &phys_base);
>> +     if (ACPI_FAILURE(status))
>> +             return status;
>> +
>> +     list_for_each_entry(its, &its_nodes, entry)
>> +             if (its->phys_base == phys_base) {
>> +                     irq_domain_free_fwnode(its->fwnode_handle);
>
> That line scares me. What about irq domains that are hold a pointer to
> this handle? its_init_domain() uses it to construct the LPI domain, and
> it is now pointing to some free memory.
>
> You'd need to reassign all the domains that match this fwnode before
> freeing it.
>

OK, I can iterate over the domains using irq_find_matching_fwspec()
and update the handles one by one. Not pretty, but that is a lost
cause anyway for this patch.

>> +                     its->fwnode_handle = &adev->fwnode;
>> +                     its_enable_quirk_socionext_synquacer(its);
>> +                     break;
>> +             }
>> +
>> +     return AE_CTRL_TERMINATE;
>> +}
>> +
>> +static int __init acpi_its_device_probe_init(void)
>> +{
>> +     if (!acpi_disabled)
>> +             acpi_get_devices("SCX0005", acpi_its_device_probe, NULL, NULL);
>> +     return 0;
>> +}
>> +subsys_initcall_sync(acpi_its_device_probe_init);
>> +#endif
>>
>
> Is there any chance that MSIs could be allocated before this kicks in?
> If that happens, we're in trouble...

This SoC does not have any MSI capable platform devices, so the only
consumers are PCI devices, and PCI drivers are registered as a
device_initcal().

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Marc Zyngier]

On 26/02/18 11:09, Ard Biesheuvel wrote:
> On 26 February 2018 at 10:18, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> Hi Ard,
>>
>> On 13/02/18 14:11, Ard Biesheuvel wrote:
>>> Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
>>> based ACPI devices, replacing the dummy fwnode with the one populated by
>>> the ACPI device core.
>>>
>>> This allows the SynQuacer ACPI tables to publish a device node such
>>> as
>>>
>>>     Device (ITS0) {
>>>       Name (_HID, "SCX0005")
>>>       Name (_ADR, 0x30020000)
>>>       Name (_DSD, Package ()  // _DSD: Device-Specific Data
>>>       {
>>>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>>         Package () {
>>>           Package (2) {
>>>             "socionext,synquacer-pre-its",
>>>             Package () { 0x58000000, 0x200000 }
>>>           },
>>>         }
>>>       })
>>>     }
>>>
>>> which will trigger the existing quirk that replaces the doorbell
>>> address with the appropriate address in the pre-ITS frame.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> Marc, Lorenzo,
>>>
>>> I am aware that this patch may be seen as controversial, but I would like to
>>> propose it nonetheless. The reason is that this is the only thing standing in
>>> the way of full ACPI support in Socionext SynQuacer based platforms.
>>>
>>> The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
>>> ARM designing it, and the reason we need DT/ACPI based quirks in the first
>>> place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
>>> one (as they designed the IP)
>>
>> That's odd. A bit of archaeology shows that ARM indeed designed a
>> pre-ITS, but that one doesn't break isolation at all (it still has a
>> single doorbell). So whatever creative changes Socionext applied to that
>> piece of IP (assuming this is the same IP), they didn't really
>> understand the far reaching impact it has.
>>
> 
> OK, thanks for digging that up. All the information I have on this
> topic is second hand, and I had no reason to assume their account of
> the history was inaccurate.

No worries. There is a short description of the PITS in the integration
guide, but I don't think that's available to the mere mortal, unfortunately.

> 
>>>
>>> Please take this into consideration when reviewing this patch,
>>>
>>> Thanks,
>>> Ard.
>>>
>>>  drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..a63973baf08a 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -3517,3 +3517,42 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>>
>>>       return 0;
>>>  }
>>> +
>>> +#if defined(CONFIG_SOCIONEXT_SYNQUACER_PREITS) && defined(CONFIG_ACPI)
>>> +static acpi_status __init acpi_its_device_probe (acpi_handle handle,
>>> +                                              u32 depth, void *context,
>>> +                                              void **ret)
>>> +{
>>> +     struct acpi_device *adev;
>>> +     unsigned long long phys_base;
>>> +     struct its_node *its;
>>> +     acpi_status status;
>>> +     int err;
>>> +
>>> +     err = acpi_bus_get_device(handle, &adev);
>>> +     if (err)
>>> +             return AE_CTRL_TERMINATE;
>>> +
>>> +     status = acpi_evaluate_integer(handle, "_ADR", NULL, &phys_base);
>>> +     if (ACPI_FAILURE(status))
>>> +             return status;
>>> +
>>> +     list_for_each_entry(its, &its_nodes, entry)
>>> +             if (its->phys_base == phys_base) {
>>> +                     irq_domain_free_fwnode(its->fwnode_handle);
>>
>> That line scares me. What about irq domains that are hold a pointer to
>> this handle? its_init_domain() uses it to construct the LPI domain, and
>> it is now pointing to some free memory.
>>
>> You'd need to reassign all the domains that match this fwnode before
>> freeing it.
>>
> 
> OK, I can iterate over the domains using irq_find_matching_fwspec()
> and update the handles one by one. Not pretty, but that is a lost
> cause anyway for this patch.

Yeah, that should do the trick.

> 
>>> +                     its->fwnode_handle = &adev->fwnode;
>>> +                     its_enable_quirk_socionext_synquacer(its);
>>> +                     break;
>>> +             }
>>> +
>>> +     return AE_CTRL_TERMINATE;
>>> +}
>>> +
>>> +static int __init acpi_its_device_probe_init(void)
>>> +{
>>> +     if (!acpi_disabled)
>>> +             acpi_get_devices("SCX0005", acpi_its_device_probe, NULL, NULL);
>>> +     return 0;
>>> +}
>>> +subsys_initcall_sync(acpi_its_device_probe_init);
>>> +#endif
>>>
>>
>> Is there any chance that MSIs could be allocated before this kicks in?
>> If that happens, we're in trouble...
> 
> This SoC does not have any MSI capable platform devices, so the only
> consumers are PCI devices, and PCI drivers are registered as a
> device_initcal().

Ideally, we'd only do the reasignment if domain->mapcount is zero.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Lorenzo Pieralisi]

On Tue, Feb 13, 2018 at 02:11:18PM +0000, Ard Biesheuvel wrote:
> Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
> based ACPI devices, replacing the dummy fwnode with the one populated by
> the ACPI device core.
> 
> This allows the SynQuacer ACPI tables to publish a device node such
> as
> 
>     Device (ITS0) {
>       Name (_HID, "SCX0005")
>       Name (_ADR, 0x30020000)

You can't have both _HID and _ADR (ACPI 6.2 - 6.1 - page 321) and
I do not think _ADR is the correct binding to solve this problem either
(_ADR can only be used for enumerable busses).

>       Name (_DSD, Package ()  // _DSD: Device-Specific Data
>       {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>           Package (2) {
>             "socionext,synquacer-pre-its",
>             Package () { 0x58000000, 0x200000 }
>           },
>         }
>       })
>     }
> 
> which will trigger the existing quirk that replaces the doorbell
> address with the appropriate address in the pre-ITS frame.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Marc, Lorenzo,
> 
> I am aware that this patch may be seen as controversial, but I would like to
> propose it nonetheless. The reason is that this is the only thing standing in
> the way of full ACPI support in Socionext SynQuacer based platforms.

I question whether these platforms should have upstream and long-term
ACPI support(dependency) - that's where the controversy is (aka if you
allow one quirk you allow them all), I do not want to add a dependency
to the ITS ACPI support for a platform that may well/is likely to be
short-lived.

> The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
> ARM designing it, and the reason we need DT/ACPI based quirks in the first
> place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
> one (as they designed the IP)
> 
> Please take this into consideration when reviewing this patch,
> 
> Thanks,
> Ard.
> 
>  drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..a63973baf08a 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3517,3 +3517,42 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  
>  	return 0;
>  }
> +
> +#if defined(CONFIG_SOCIONEXT_SYNQUACER_PREITS) && defined(CONFIG_ACPI)
> +static acpi_status __init acpi_its_device_probe (acpi_handle handle,
> +						 u32 depth, void *context,
> +						 void **ret)
> +{
> +	struct acpi_device *adev;
> +	unsigned long long phys_base;
> +	struct its_node *its;
> +	acpi_status status;
> +	int err;
> +
> +	err = acpi_bus_get_device(handle, &adev);
> +	if (err)
> +		return AE_CTRL_TERMINATE;
> +
> +	status = acpi_evaluate_integer(handle, "_ADR", NULL, &phys_base);
> +	if (ACPI_FAILURE(status))
> +		return status;

I do not think using _ADR is correct here, the phys_base should be
described as a _CRS (or you avoided using _CRS for resources
conflicts ? Still, I do not think that using _ADR is right).

> +	list_for_each_entry(its, &its_nodes, entry)
> +		if (its->phys_base == phys_base) {
> +			irq_domain_free_fwnode(its->fwnode_handle);
> +			its->fwnode_handle = &adev->fwnode;
> +			its_enable_quirk_socionext_synquacer(its);
> +			break;
> +		}

I think this is wrong. Why do you need to replace the fwnode at all
(and how does this work with IORT ?) ?

I understand you want to have a uniform DT/ACPI quirk handling (and stash
the fwnode so that you can read a _DSD out of it with the fwnode_ API)
but still, that does not justify swapping the IRQ domain fwnode handle.

> +
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static int __init acpi_its_device_probe_init(void)
> +{
> +	if (!acpi_disabled)
> +		acpi_get_devices("SCX0005", acpi_its_device_probe, NULL, NULL);
> +	return 0;
> +}
> +subsys_initcall_sync(acpi_its_device_probe_init);

That's a subsys_initcall_sync just because you need the interpreter up and
running right ?

Thanks,
Lorenzo

[RFC PATCH] irqchip/gic-v3-its: apply ACPI device based quirks

[Ard Biesheuvel]

On 26 February 2018 at 11:51, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, Feb 13, 2018 at 02:11:18PM +0000, Ard Biesheuvel wrote:
>> Reapply the SynQuacer quirk for ITS frames that are matched by 'SCX0005'
>> based ACPI devices, replacing the dummy fwnode with the one populated by
>> the ACPI device core.
>>
>> This allows the SynQuacer ACPI tables to publish a device node such
>> as
>>
>>     Device (ITS0) {
>>       Name (_HID, "SCX0005")
>>       Name (_ADR, 0x30020000)
>
> You can't have both _HID and _ADR (ACPI 6.2 - 6.1 - page 321) and
> I do not think _ADR is the correct binding to solve this problem either
> (_ADR can only be used for enumerable busses).
>

OK.

>>       Name (_DSD, Package ()  // _DSD: Device-Specific Data
>>       {
>>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>         Package () {
>>           Package (2) {
>>             "socionext,synquacer-pre-its",
>>             Package () { 0x58000000, 0x200000 }
>>           },
>>         }
>>       })
>>     }
>>
>> which will trigger the existing quirk that replaces the doorbell
>> address with the appropriate address in the pre-ITS frame.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> Marc, Lorenzo,
>>
>> I am aware that this patch may be seen as controversial, but I would like to
>> propose it nonetheless. The reason is that this is the only thing standing in
>> the way of full ACPI support in Socionext SynQuacer based platforms.
>
> I question whether these platforms should have upstream and long-term
> ACPI support(dependency) - that's where the controversy is (aka if you
> allow one quirk you allow them all), I do not want to add a dependency
> to the ITS ACPI support for a platform that may well/is likely to be
> short-lived.
>

I understand.
>> The pre-ITS is a monstrosity, but as it turns out, Socionext had help from
>> ARM designing it, and the reason we need DT/ACPI based quirks in the first
>> place is that the IIDR of this GICv3 implementation is simply the ARM Ltd.
>> one (as they designed the IP)
>>
>> Please take this into consideration when reviewing this patch,
>>
>> Thanks,
>> Ard.
>>
>>  drivers/irqchip/irq-gic-v3-its.c | 39 ++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..a63973baf08a 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -3517,3 +3517,42 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>>
>>       return 0;
>>  }
>> +
>> +#if defined(CONFIG_SOCIONEXT_SYNQUACER_PREITS) && defined(CONFIG_ACPI)
>> +static acpi_status __init acpi_its_device_probe (acpi_handle handle,
>> +                                              u32 depth, void *context,
>> +                                              void **ret)
>> +{
>> +     struct acpi_device *adev;
>> +     unsigned long long phys_base;
>> +     struct its_node *its;
>> +     acpi_status status;
>> +     int err;
>> +
>> +     err = acpi_bus_get_device(handle, &adev);
>> +     if (err)
>> +             return AE_CTRL_TERMINATE;
>> +
>> +     status = acpi_evaluate_integer(handle, "_ADR", NULL, &phys_base);
>> +     if (ACPI_FAILURE(status))
>> +             return status;
>
> I do not think using _ADR is correct here, the phys_base should be
> described as a _CRS (or you avoided using _CRS for resources
> conflicts ? Still, I do not think that using _ADR is right).
>

No, there is no resource conflict afaik. I just wasn't aware that _ADR
is inappropriate here.

>> +     list_for_each_entry(its, &its_nodes, entry)
>> +             if (its->phys_base == phys_base) {
>> +                     irq_domain_free_fwnode(its->fwnode_handle);
>> +                     its->fwnode_handle = &adev->fwnode;
>> +                     its_enable_quirk_socionext_synquacer(its);
>> +                     break;
>> +             }
>
> I think this is wrong. Why do you need to replace the fwnode at all
> (and how does this work with IORT ?) ?
>
> I understand you want to have a uniform DT/ACPI quirk handling (and stash
> the fwnode so that you can read a _DSD out of it with the fwnode_ API)
> but still, that does not justify swapping the IRQ domain fwnode handle.
>

Fair enough. I am looking into whether it is feasible to instantiate
the ITS node later (and not describe it at all in the MADT)

>> +
>> +     return AE_CTRL_TERMINATE;
>> +}
>> +
>> +static int __init acpi_its_device_probe_init(void)
>> +{
>> +     if (!acpi_disabled)
>> +             acpi_get_devices("SCX0005", acpi_its_device_probe, NULL, NULL);
>> +     return 0;
>> +}
>> +subsys_initcall_sync(acpi_its_device_probe_init);
>
> That's a subsys_initcall_sync just because you need the interpreter up and
> running right ?
>

Because it is the earliest initcall level where ACPI devices have been
instantiated.