[RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Shameerali Kolothum Thodi]

This erratum describes the limitation of certain HiSilicon platforms
to support the SMMU mappings for MSI transactions and on those platforms
the MSI transactions has to be bypassed by SMMU. The IIDR register of the
GICv3 ITS on these platforms are not properly populated to differentiate
the hardware, hence describe it in device tree.

Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
---
 .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt         | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index 4c29cda..84af301 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -75,6 +75,12 @@ These nodes must have the following properties:
 - reg: Specifies the base physical address and size of the ITS
   registers.

+Optional
+- hisilicon,erratum-161010801 : A boolean property. Indicates the presence of
+  erratum 161010801, which says that these platforms doesn't support  SMMU
+  mapping for MSI transactions and those transactions has to be bypassed
+  by SMMU.
+
 The main GIC node must contain the appropriate #address-cells,
 #size-cells and ranges properties for the reg property of all ITS
 nodes.
-- 
1.9.1

Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Mark Rutland]

Hi,

I see this wasn't Cc'd to LAKML, unlike the cover letter, and patch 3
(which isn't threaded against the cover letter).

Please use a consistent Cc list, with patches in-reply to the cover
letter.

On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum Thodi wrote:
> This erratum describes the limitation of certain HiSilicon platforms
> to support the SMMU mappings for MSI transactions and on those platforms
> the MSI transactions has to be bypassed by SMMU. The IIDR register of the
> GICv3 ITS on these platforms are not properly populated to differentiate
> the hardware, hence describe it in device tree.
> 
> Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> ---
>  .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt         | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> index 4c29cda..84af301 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> @@ -75,6 +75,12 @@ These nodes must have the following properties:
>  - reg: Specifies the base physical address and size of the ITS
>    registers.
> 
> +Optional
> +- hisilicon,erratum-161010801 : A boolean property. Indicates the presence of
> +  erratum 161010801, which says that these platforms doesn't support  SMMU
> +  mapping for MSI transactions and those transactions has to be bypassed
> +  by SMMU.

What exactly is meant by "doesn't support SMMU mapping" here? What
precisely is the problem in HW?

Thanks,
Mark.

RE: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Shameerali Kolothum Thodi]

Hi Mark,

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Tuesday, January 24, 2017 1:52 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier@arm.com; will.deacon@arm.com; linux-
> kernel@vger.kernel.org; Linuxarm; devicetree@vger.kernel.org; John
> Garry; Guohanjun (Hanjun Guo)
> Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
> hisilicon 161010801 erratum
> 
> Hi,
> 
> I see this wasn't Cc'd to LAKML, unlike the cover letter, and patch 3
> (which isn't threaded against the cover letter).
> 
> Please use a consistent Cc list, with patches in-reply to the cover
> letter.

Apologies for the inconsistencies. 

> On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum Thodi
> wrote:
> > This erratum describes the limitation of certain HiSilicon platforms
> > to support the SMMU mappings for MSI transactions and on those
> > platforms the MSI transactions has to be bypassed by SMMU. The IIDR
> > register of the
> > GICv3 ITS on these platforms are not properly populated to
> > differentiate the hardware, hence describe it in device tree.
> >
> > Signed-off-by: shameer <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
> v3.tx
> > t
> > b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
> v3.tx
> > t
> > index 4c29cda..84af301 100644
> > ---
> > a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
> v3.tx
> > t
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
> v
> > +++ 3.txt
> > @@ -75,6 +75,12 @@ These nodes must have the following properties:
> >  - reg: Specifies the base physical address and size of the ITS
> >    registers.
> >
> > +Optional
> > +- hisilicon,erratum-161010801 : A boolean property. Indicates the
> > +presence of
> > +  erratum 161010801, which says that these platforms doesn't support
> > +SMMU
> > +  mapping for MSI transactions and those transactions has to be
> > +bypassed
> > +  by SMMU.
> 
> What exactly is meant by "doesn't support SMMU mapping" here? What
> precisely is the problem in HW?

On this platforms the ITS doorbell deviceID information is embedded in the MSI 
payload. To do this, the PCIe controller differentiates the MSI payload and 
DMA payload and modifies the MSI payload to add the deviceID information.
The way it modifies this is by comparing against a SYS_CTRL register which
is configured by UEFI with the ITS doorbell phys address.

Hence if EP is configured with a SMMU translated Doorbell address, the PCIe
RC cannot differentiate the MSI payload and ITS will fail to generate the 
Interrupt.

Hope I am clear.

Thanks,
Shameer

Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Mark Rutland]

On Tue, Jan 24, 2017 at 02:00:30PM +0000, Shameerali Kolothum Thodi wrote:
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]

> > On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum Thodi
> > wrote:

> > > +Optional
> > > +- hisilicon,erratum-161010801 : A boolean property. Indicates the
> > > +presence of
> > > +  erratum 161010801, which says that these platforms doesn't support
> > > +SMMU
> > > +  mapping for MSI transactions and those transactions has to be
> > > +bypassed
> > > +  by SMMU.
> > 
> > What exactly is meant by "doesn't support SMMU mapping" here? What
> > precisely is the problem in HW?
> 
> On this platforms the ITS doorbell deviceID information is embedded in the MSI 
> payload. To do this, the PCIe controller differentiates the MSI payload and 
> DMA payload and modifies the MSI payload to add the deviceID information.
> The way it modifies this is by comparing against a SYS_CTRL register which
> is configured by UEFI with the ITS doorbell phys address.

Ok. Some part of this will need to go in the binding description.

How does this interact with translations via the SMMU?

Do writes matching this address:

(a) always bypass translation.
(b) get translated after modification.
(c) other?

Thanks,
Mark.

RE: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: Tuesday, January 24, 2017 2:29 PM
> To: Shameerali Kolothum Thodi
> Cc: marc.zyngier@arm.com; will.deacon@arm.com; linux-
> kernel@vger.kernel.org; Linuxarm; devicetree@vger.kernel.org; John
> Garry; Guohanjun (Hanjun Guo)
> Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
> hisilicon 161010801 erratum
> 
> On Tue, Jan 24, 2017 at 02:00:30PM +0000, Shameerali Kolothum Thodi
> wrote:
> > > -----Original Message-----
> > > From: Mark Rutland [mailto:mark.rutland@arm.com]
> 
> > > On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum Thodi
> > > wrote:
> 
> > > > +Optional
> > > > +- hisilicon,erratum-161010801 : A boolean property. Indicates
> the
> > > > +presence of
> > > > +  erratum 161010801, which says that these platforms doesn't
> > > > +support SMMU
> > > > +  mapping for MSI transactions and those transactions has to be
> > > > +bypassed
> > > > +  by SMMU.
> > >
> > > What exactly is meant by "doesn't support SMMU mapping" here? What
> > > precisely is the problem in HW?
> >
> > On this platforms the ITS doorbell deviceID information is embedded
> in
> > the MSI payload. To do this, the PCIe controller differentiates the
> > MSI payload and DMA payload and modifies the MSI payload to add the
> deviceID information.
> > The way it modifies this is by comparing against a SYS_CTRL register
> > which is configured by UEFI with the ITS doorbell phys address.
> 
> Ok. Some part of this will need to go in the binding description.
> 
> How does this interact with translations via the SMMU?
> 
> Do writes matching this address:
> 
> (a) always bypass translation.
> (b) get translated after modification.
> (c) other?

PCIe RC has a configuration setting to enable/disable SMMU
bypass for PCIe MSI write and with this patch series we
are using the disable mode. So it bypasses SMMU always for
MSI but not for DMA. 

As per our SoC engineers this implementation seems to be based on an earlier
version of GIC spec earlier version the GIC spec(Document 
number:PRD03-GENC-010745 18.0) where it says:

"Implementations may choose to transform writes to GITS_TRANSLATER by either:
 -multiplexing the device ID onto the address bus (which is what GIC-500 provides
  a mechanism for), or
 -extending the data value to 64 bits, providing the device ID in the upper bits,
  and transforming the access to become a 64-bit write"

Though I can't find the same in latest GIC spec.

Thanks,
Shameer

Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Marc Zyngier]

On 24/01/17 15:13, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Mark Rutland [mailto:mark.rutland@arm.com]
>> Sent: Tuesday, January 24, 2017 2:29 PM
>> To: Shameerali Kolothum Thodi
>> Cc: marc.zyngier@arm.com; will.deacon@arm.com; linux-
>> kernel@vger.kernel.org; Linuxarm; devicetree@vger.kernel.org; John
>> Garry; Guohanjun (Hanjun Guo)
>> Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
>> hisilicon 161010801 erratum
>>
>> On Tue, Jan 24, 2017 at 02:00:30PM +0000, Shameerali Kolothum Thodi
>> wrote:
>>>> -----Original Message-----
>>>> From: Mark Rutland [mailto:mark.rutland@arm.com]
>>
>>>> On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum Thodi
>>>> wrote:
>>
>>>>> +Optional
>>>>> +- hisilicon,erratum-161010801 : A boolean property. Indicates
>> the
>>>>> +presence of
>>>>> +  erratum 161010801, which says that these platforms doesn't
>>>>> +support SMMU
>>>>> +  mapping for MSI transactions and those transactions has to be
>>>>> +bypassed
>>>>> +  by SMMU.
>>>>
>>>> What exactly is meant by "doesn't support SMMU mapping" here? What
>>>> precisely is the problem in HW?
>>>
>>> On this platforms the ITS doorbell deviceID information is embedded
>> in
>>> the MSI payload. To do this, the PCIe controller differentiates the
>>> MSI payload and DMA payload and modifies the MSI payload to add the
>> deviceID information.
>>> The way it modifies this is by comparing against a SYS_CTRL register
>>> which is configured by UEFI with the ITS doorbell phys address.
>>
>> Ok. Some part of this will need to go in the binding description.
>>
>> How does this interact with translations via the SMMU?
>>
>> Do writes matching this address:
>>
>> (a) always bypass translation.
>> (b) get translated after modification.
>> (c) other?
> 
> PCIe RC has a configuration setting to enable/disable SMMU
> bypass for PCIe MSI write and with this patch series we
> are using the disable mode. So it bypasses SMMU always for
> MSI but not for DMA. 
> 
> As per our SoC engineers this implementation seems to be based on an earlier
> version of GIC spec earlier version the GIC spec(Document 
> number:PRD03-GENC-010745 18.0) where it says:
> 
> "Implementations may choose to transform writes to GITS_TRANSLATER by either:
>  -multiplexing the device ID onto the address bus (which is what GIC-500 provides
>   a mechanism for), or
>  -extending the data value to 64 bits, providing the device ID in the upper bits,
>   and transforming the access to become a 64-bit write"

Crucially, that should be done by performing the up-scaling just as the
write reaches the ITS translation register, and *not* when the write
leaves the RC. If you up-scale it early, you end-up in this silly situation.

> Though I can't find the same in latest GIC spec.

Because that's not an architecture feature, but an implement decision.
And whatever the implementation does, it should be invisible to SW.
Unfortunately, bypassing the SMMU is not exactly invisible...

Thanks,

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

RE: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Tuesday, January 24, 2017 3:28 PM
> To: Shameerali Kolothum Thodi; Mark Rutland
> Cc: will.deacon@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> devicetree@vger.kernel.org; John Garry; Guohanjun (Hanjun Guo)
> Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
> hisilicon 161010801 erratum
> 
> On 24/01/17 15:13, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Mark Rutland [mailto:mark.rutland@arm.com]
> >> Sent: Tuesday, January 24, 2017 2:29 PM
> >> To: Shameerali Kolothum Thodi
> >> Cc: marc.zyngier@arm.com; will.deacon@arm.com; linux-
> >> kernel@vger.kernel.org; Linuxarm; devicetree@vger.kernel.org; John
> >> Garry; Guohanjun (Hanjun Guo)
> >> Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding
> >> for hisilicon 161010801 erratum
> >>
> >> On Tue, Jan 24, 2017 at 02:00:30PM +0000, Shameerali Kolothum Thodi
> >> wrote:
> >>>> -----Original Message-----
> >>>> From: Mark Rutland [mailto:mark.rutland@arm.com]
> >>
> >>>> On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum
> Thodi
> >>>> wrote:
> >>
> >>>>> +Optional
> >>>>> +- hisilicon,erratum-161010801 : A boolean property. Indicates
> >> the
> >>>>> +presence of
> >>>>> +  erratum 161010801, which says that these platforms doesn't
> >>>>> +support SMMU
> >>>>> +  mapping for MSI transactions and those transactions has to be
> >>>>> +bypassed
> >>>>> +  by SMMU.
> >>>>
> >>>> What exactly is meant by "doesn't support SMMU mapping" here? What
> >>>> precisely is the problem in HW?
> >>>
> >>> On this platforms the ITS doorbell deviceID information is embedded
> >> in
> >>> the MSI payload. To do this, the PCIe controller differentiates the
> >>> MSI payload and DMA payload and modifies the MSI payload to add the
> >> deviceID information.
> >>> The way it modifies this is by comparing against a SYS_CTRL
> register
> >>> which is configured by UEFI with the ITS doorbell phys address.
> >>
> >> Ok. Some part of this will need to go in the binding description.
> >>
> >> How does this interact with translations via the SMMU?
> >>
> >> Do writes matching this address:
> >>
> >> (a) always bypass translation.
> >> (b) get translated after modification.
> >> (c) other?
> >
> > PCIe RC has a configuration setting to enable/disable SMMU bypass for
> > PCIe MSI write and with this patch series we are using the disable
> > mode. So it bypasses SMMU always for MSI but not for DMA.
> >
> > As per our SoC engineers this implementation seems to be based on an
> > earlier version of GIC spec earlier version the GIC spec(Document
> > number:PRD03-GENC-010745 18.0) where it says:
> >
> > "Implementations may choose to transform writes to GITS_TRANSLATER by
> either:
> >  -multiplexing the device ID onto the address bus (which is what GIC-
> 500 provides
> >   a mechanism for), or
> >  -extending the data value to 64 bits, providing the device ID in the
> upper bits,
> >   and transforming the access to become a 64-bit write"
> 
> Crucially, that should be done by performing the up-scaling just as the
> write reaches the ITS translation register, and *not* when the write
> leaves the RC. If you up-scale it early, you end-up in this silly
> situation.
> 
> > Though I can't find the same in latest GIC spec.
> 
> Because that's not an architecture feature, but an implement decision.
> And whatever the implementation does, it should be invisible to SW.
> Unfortunately, bypassing the SMMU is not exactly invisible...
> 

Totally agree. Unfortunately this is the way our implementation is :(

Thanks,
Shameer