[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Marc Zyngier]

[+Mark]

On 12/10/17 23:24, Ard Biesheuvel wrote:
> On 12 October 2017 at 22:34, Rob Herring <robh+dt at kernel.org> wrote:
>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>> <ard.biesheuvel at linaro.org> wrote:
>>> The Socionext Synquacer SoC's implementation of GICv3 has a so-called
>>> 'pre-ITS', which maps 32-bit writes targeted at a separate window of
>>> size '4 << device_id_bits' onto writes to GITS_TRANSLATER with device
>>> ID taken from bits [device_id_bits + 1:2] of the window offset.
>>> Writes that target GITS_TRANSLATER directly are reported as originating
>>> from device ID #0.
>>>
>>> So add a workaround for this. Given that this breaks isolation, clear
>>> the IRQ_DOMAIN_FLAG_MSI_REMAP flag as well.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>  3 files changed, 72 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>>> index 4c29cdab0ea5..0798a61bbf99 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>>> @@ -75,6 +75,10 @@ These nodes must have the following properties:
>>>  - reg: Specifies the base physical address and size of the ITS
>>>    registers.
>>>
>>> +Optional:
>>> +- socionext,synquacer-pre-its: (u32, u32) tuple describing the host address
>>> +  and size of the pre-ITS window, as implemented on the Socionext Synquacer SoC
>>
>> Sounds like a separate h/w block to me and addresses should be in
>> "reg". I would suggest you define a separate node for the pre-its
>> block and then use of_find_compatible_node() from the GIC driver to
>> retrieve the node and whatever you need from it. Or do an SoC specific
>> compatible string for the GIC and let that imply whatever information
>> you need. Then the next quirk doesn't need a DT update.
>>
> 
> For my understanding, you mean either
> 
>     gic: interrupt-controller at 30000000 {
>         compatible = "arm,gic-v3";
>         ...
> 
>         its: gic-its at 30020000 {
>             compatible = "arm,gic-v3-its";
>             reg = <0x0 0x30020000 0x0 0x20000>;
>             #msi-cells = <1>;
>             msi-controller;
>         };
>     };
> 
>     preits at 58000000 {
>         compatible = "socionext,synquacer-pre-its";
>         reg = <0x0 0x58000000 0x0 0x200000>;
>         msi-slave = <&its>;
>     };
> 
> or
> 
>     gic: interrupt-controller at 30000000 {
>         compatible = "arm,gic-v3";
>         ...
> 
>         its: gic-its at 30020000 {
>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>             reg = <0x0 0x30020000 0x0 0x20000>,
>                   <0x0 0x58000000 0x0 0x200000>;
>             #msi-cells = <1>;
>             msi-controller;
>         };
>     };
> 
> right?
> 
> Marc, what do you think? IIRC we did discuss option #2 at some point,
> but I don't remember if/why we rejected it.

I dislike #2 because these registers are not part of the regular ITS,
and would get in the way of potential extensions of the ITS (I don't
know of any, but just in case...).

I also dislike #1 as the "msi-slave" part is both ugly and confusing
(are we writing to the ITS? to the pre-ITS?).

How about putting the pre-its node *inside* the its node itself? It
makes it obvious which pre-ITS corresponds to which ITS, and it leaves
the ITS regs untouched.

Thoughts?

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