irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum

RFC In review Shameerali Kolothum Thodi 3 participants 7 messages Jan 24, 2017 1 message not in the archive

Subsystems (3)

Device tree bindings irqchip drivers Arm generic interrupt controller drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Shameerali Kolothum Thodi

author

2017-01-24 13:42 UTC #1 lore raw

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

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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

---
 .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt         | 6 ++++++
 1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt +6 -0

index 4c29cda..84af301 100644
@@ -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.

Signature · 13 lines

1.9.1








--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland

2017-01-24 13:52 UTC #2 lore raw

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.

Quoted 16 lines · 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 <[email protected]>
---
 .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt         | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt +0 -0
index 4c29cda..84af301 100644

Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt These nodes must have the following properties: · line 82

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt +5 -0

@@ -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.

Shameerali Kolothum Thodi

author

2017-01-24 14:00 UTC #3 lore raw

Hi Mark,

Quoted 10 lines

-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
Sent: Tuesday, January 24, 2017 1:52 PM
To: Shameerali Kolothum Thodi
Cc: [email protected]; [email protected]; linux- [email protected]; Linuxarm; [email protected]; John Garry; Guohanjun (Hanjun Guo)
Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
hisilicon 161010801 erratum

Quoted 7 lines

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.

Quoted 34 lines

Quoted 10 lines · 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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
.../devicetree/bindings/interrupt-controller/arm,gic-v3.txt
| 6 ++++++
Quoted 4 lines
 1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
v3.tx
Quoted 2 lines
t b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
v3.tx
Quoted 4 lines
t
index 4c29cda..84af301 100644
---
a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
v3.tx
Quoted 2 lines
t
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-
v
Quoted 5 lines
+++ 3.txt
Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt +0 -0
@@ -75,6 +75,12 @@ These nodes must have the following properties:
 - reg: Specifies the base physical address and size of the ITS
   registers.
 

Quoted 11 lines

Review These nodes must have the following properties: · line 85
Quoted 8 lines
(quoted diff) +8 -0
+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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Mark Rutland

2017-01-24 14:28 UTC #4 lore raw

Quoted 18 lines · Shameerali Kolothum Thodi

Quoted 6 lines
-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum Thodi wrote:
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8 -0
+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?

Quoted 5 lines

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Shameerali Kolothum Thodi

author

2017-01-24 15:13 UTC #5 lore raw

Quoted 43 lines

-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
Sent: Tuesday, January 24, 2017 2:29 PM
To: Shameerali Kolothum Thodi
Cc: [email protected]; [email protected]; linux- [email protected]; Linuxarm; [email protected]; John Garry; Guohanjun (Hanjun Guo)
Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
hisilicon 161010801 erratum
Quoted 8 lines · Shameerali Kolothum Thodi
Quoted 8 lines
-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
Quoted 2 lines · Shameerali Kolothum Thodi
+Optional
+- hisilicon,erratum-161010801 : A boolean property. Indicates
the
Quoted 11 lines
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+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
Quoted 2 lines
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.
Quoted 2 lines
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.

Quoted 7 lines

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Marc Zyngier

2017-01-24 15:28 UTC #6 lore raw

Quoted 60 lines · Shameerali Kolothum Thodi

Quoted 43 lines
-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
Sent: Tuesday, January 24, 2017 2:29 PM
To: Shameerali Kolothum Thodi
Cc: [email protected]; [email protected]; linux- [email protected]; Linuxarm; [email protected]; John Garry; Guohanjun (Hanjun Guo)
Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for
hisilicon 161010801 erratum
Quoted 8 lines · Shameerali Kolothum Thodi
Quoted 8 lines
-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
Quoted 2 lines · Shameerali Kolothum Thodi
+Optional
+- hisilicon,erratum-161010801 : A boolean property. Indicates
the
Quoted 11 lines
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+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
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 7 lines
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:

Quoted 5 lines

"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.

Quoted 1 line

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.

Signature · 5 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Shameerali Kolothum Thodi

author

2017-01-24 15:42 UTC #7 lore raw

Quoted 87 lines

-----Original Message-----
From: Marc Zyngier [mailto:[email protected]]
Sent: Tuesday, January 24, 2017 3:28 PM
To: Shameerali Kolothum Thodi; Mark Rutland
Cc: [email protected]; [email protected]; Linuxarm; [email protected]; John Garry; Guohanjun (Hanjun Guo)
Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum
Quoted 12 lines · Shameerali Kolothum Thodi
Quoted 12 lines
-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
Sent: Tuesday, January 24, 2017 2:29 PM
To: Shameerali Kolothum Thodi
Cc: [email protected]; [email protected]; linux- [email protected]; Linuxarm; [email protected]; John Garry; Guohanjun (Hanjun Guo)
Subject: Re: [RFC 1/4] irqchip, gicv3-its: Add device tree binding for hisilicon 161010801 erratum
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines · Shameerali Kolothum Thodi
Quoted 4 lines
-----Original Message-----
From: Mark Rutland [mailto:[email protected]]
On Tue, Jan 24, 2017 at 01:42:56PM +0000, Shameerali Kolothum
Thodi
Quoted 15 lines
Quoted 15 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines · wrote:
+Optional
+- hisilicon,erratum-161010801 : A boolean property. Indicates
the
Quoted 10 lines
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+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?
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
On this platforms the ITS doorbell deviceID information is embedded
in
Quoted 2 lines
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.
Quoted 1 line
The way it modifies this is by comparing against a SYS_CTRL
register
Quoted 16 lines
Quoted 11 lines
Quoted 1 line
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.
Quoted 5 lines
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:
Quoted 1 line
-multiplexing the device ID onto the address bus (which is what GIC-
500 provides
Quoted 2 lines
a mechanism for), or
-extending the data value to 64 bits, providing the device ID in the
upper bits,
Quoted 1 line
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.

Quoted 6 lines

Quoted 1 line
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