From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Sat, 14 Oct 2017 10:13:13 +0100 Subject: [PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS In-Reply-To: (Ard Biesheuvel's message of "Fri, 13 Oct 2017 21:38:02 +0100") References: <20171012183247.23679-1-ard.biesheuvel@linaro.org> <20171012183247.23679-3-ard.biesheuvel@linaro.org> Message-ID: <86po9qks7q.fsf@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Oct 13 2017 at 9:38:02 pm BST, Ard Biesheuvel wrote: > On 13 October 2017 at 19:50, Rob Herring wrote: >> On Fri, Oct 13, 2017 at 4:47 AM, Marc Zyngier wrote: >>> [+Mark] >>> >>> On 12/10/17 23:24, Ard Biesheuvel wrote: >>>> On 12 October 2017 at 22:34, Rob Herring wrote: >>>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel >>>>> 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 >>>>>> --- >>>>>> 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?). >> >> Why do you need this? Are you ever going to have multiple pre-its's? > > The question is really whether we are ever going to have multiple > ITSes, in which case we will need to identify which ITS this single > pre-ITS is associated with, given that it can only target one. And > while I would prefer zero pre-ITSes in all cases [as would Marc, I'm > sure], having a pre-ITS for each ITS frame is not unimaginable either. > So yes, we need to record this information in one way or the other. Multi-ITS systems are out there. The box I'm using for a large part of my GICv4 testing has 8 of them. It is quite common to place ITSs close to the PCIe RC, and multi-socket systems usually have at least one per socket. Hopefully the pre-ITS concept is a one-off nightmare, something we'll laugh about in two years from now. Maybe. Thanks, M. -- Jazz is not dead. It just smells funny. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS Date: Sat, 14 Oct 2017 10:13:13 +0100 Message-ID: <86po9qks7q.fsf@arm.com> References: <20171012183247.23679-1-ard.biesheuvel@linaro.org> <20171012183247.23679-3-ard.biesheuvel@linaro.org> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: (Ard Biesheuvel's message of "Fri, 13 Oct 2017 21:38:02 +0100") Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ard Biesheuvel Cc: Rob Herring , Robin Murphy , "linux-arm-kernel@lists.infradead.org" , Daniel Thompson , Leif Lindholm , Graeme Gregory , Catalin Marinas , Will Deacon , "devicetree@vger.kernel.org" , Mark Rutland List-Id: devicetree@vger.kernel.org On Fri, Oct 13 2017 at 9:38:02 pm BST, Ard Biesheuvel wrote: > On 13 October 2017 at 19:50, Rob Herring wrote: >> On Fri, Oct 13, 2017 at 4:47 AM, Marc Zyngier wrote: >>> [+Mark] >>> >>> On 12/10/17 23:24, Ard Biesheuvel wrote: >>>> On 12 October 2017 at 22:34, Rob Herring wrote: >>>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel >>>>> 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 >>>>>> --- >>>>>> 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@30000000 { >>>> compatible = "arm,gic-v3"; >>>> ... >>>> >>>> its: gic-its@30020000 { >>>> compatible = "arm,gic-v3-its"; >>>> reg = <0x0 0x30020000 0x0 0x20000>; >>>> #msi-cells = <1>; >>>> msi-controller; >>>> }; >>>> }; >>>> >>>> preits@58000000 { >>>> compatible = "socionext,synquacer-pre-its"; >>>> reg = <0x0 0x58000000 0x0 0x200000>; >>>> msi-slave = <&its>; >>>> }; >>>> >>>> or >>>> >>>> gic: interrupt-controller@30000000 { >>>> compatible = "arm,gic-v3"; >>>> ... >>>> >>>> its: gic-its@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?). >> >> Why do you need this? Are you ever going to have multiple pre-its's? > > The question is really whether we are ever going to have multiple > ITSes, in which case we will need to identify which ITS this single > pre-ITS is associated with, given that it can only target one. And > while I would prefer zero pre-ITSes in all cases [as would Marc, I'm > sure], having a pre-ITS for each ITS frame is not unimaginable either. > So yes, we need to record this information in one way or the other. Multi-ITS systems are out there. The box I'm using for a large part of my GICv4 testing has 8 of them. It is quite common to place ITSs close to the PCIe RC, and multi-socket systems usually have at least one per socket. Hopefully the pre-ITS concept is a one-off nightmare, something we'll laugh about in two years from now. Maybe. Thanks, M. -- Jazz is not dead. It just smells funny. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html