| Message ID | 20240212165043.26961-3-johan+linaro@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: enable GICv3 ITS for PCIe | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 12/02/2024 17:50, Johan Hovold wrote: > Whether the 'msi-map-mask' property is needed or not depends on how the > MSI interrupts are mapped and it should therefore not be described as > required. I could imagine that on all devices the interrupts are mapped in a way you need to provide msi-map-mask. IOW, can there be a Qualcomm platform without msi-map-mask? Best regards, Krzysztof
On Wed, Feb 14, 2024 at 01:01:20PM +0100, Krzysztof Kozlowski wrote: > On 12/02/2024 17:50, Johan Hovold wrote: > > Whether the 'msi-map-mask' property is needed or not depends on how the > > MSI interrupts are mapped and it should therefore not be described as > > required. > > I could imagine that on all devices the interrupts are mapped in a way > you need to provide msi-map-mask. IOW, can there be a Qualcomm platform > without msi-map-mask? I don't have access to the documentation so I'll leave that for you guys to determine. I do note that the downstream DT does not use it and that we have a new devicetree in linux-next which also does not have it: https://lore.kernel.org/r/20240125-topic-sm8650-upstream-pcie-its-v1-1-cb506deeb43e@linaro.org But at least the latter looks like an omission that should be fixed. Johan
On 14/02/2024 13:54, Johan Hovold wrote: > On Wed, Feb 14, 2024 at 01:01:20PM +0100, Krzysztof Kozlowski wrote: >> On 12/02/2024 17:50, Johan Hovold wrote: >>> Whether the 'msi-map-mask' property is needed or not depends on how the >>> MSI interrupts are mapped and it should therefore not be described as >>> required. >> >> I could imagine that on all devices the interrupts are mapped in a way >> you need to provide msi-map-mask. IOW, can there be a Qualcomm platform >> without msi-map-mask? > > I don't have access to the documentation so I'll leave that for you guys > to determine. I do note that the downstream DT does not use it and that > we have a new devicetree in linux-next which also does not have it: > > https://lore.kernel.org/r/20240125-topic-sm8650-upstream-pcie-its-v1-1-cb506deeb43e@linaro.org > > But at least the latter looks like an omission that should be fixed. Hm, either that or the mask for sm8450 was not needed as well. Anyway, thanks for explanation, appreciated! Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Wed, Feb 14, 2024 at 02:38:57PM +0100, Krzysztof Kozlowski wrote: > On 14/02/2024 13:54, Johan Hovold wrote: > > On Wed, Feb 14, 2024 at 01:01:20PM +0100, Krzysztof Kozlowski wrote: > >> On 12/02/2024 17:50, Johan Hovold wrote: > >>> Whether the 'msi-map-mask' property is needed or not depends on how the > >>> MSI interrupts are mapped and it should therefore not be described as > >>> required. > >> > >> I could imagine that on all devices the interrupts are mapped in a way > >> you need to provide msi-map-mask. IOW, can there be a Qualcomm platform > >> without msi-map-mask? > > > > I don't have access to the documentation so I'll leave that for you guys > > to determine. I do note that the downstream DT does not use it and that > > we have a new devicetree in linux-next which also does not have it: > > > > https://lore.kernel.org/r/20240125-topic-sm8650-upstream-pcie-its-v1-1-cb506deeb43e@linaro.org > > > > But at least the latter looks like an omission that should be fixed. > > Hm, either that or the mask for sm8450 was not needed as well. Anyway, > thanks for explanation, appreciated! > msi-map-mask is definitely needed as it would allow all the devices under the same bus to reuse the MSI identifier. Currently, excluding this property will not cause any issue since there is a single device under each bus. But we cannot assume that is going to be the case on all boards. I will submit a patch to fix SM8650. - Mani
On Fri, Feb 16, 2024 at 10:24:06PM +0530, Manivannan Sadhasivam wrote: > On Wed, Feb 14, 2024 at 02:38:57PM +0100, Krzysztof Kozlowski wrote: > > On 14/02/2024 13:54, Johan Hovold wrote: > > > On Wed, Feb 14, 2024 at 01:01:20PM +0100, Krzysztof Kozlowski wrote: > > >> On 12/02/2024 17:50, Johan Hovold wrote: > > >>> Whether the 'msi-map-mask' property is needed or not depends on how the > > >>> MSI interrupts are mapped and it should therefore not be described as > > >>> required. > > >> > > >> I could imagine that on all devices the interrupts are mapped in a way > > >> you need to provide msi-map-mask. IOW, can there be a Qualcomm platform > > >> without msi-map-mask? > > > > > > I don't have access to the documentation so I'll leave that for you guys > > > to determine. I do note that the downstream DT does not use it and that > > > we have a new devicetree in linux-next which also does not have it: > > > > > > https://lore.kernel.org/r/20240125-topic-sm8650-upstream-pcie-its-v1-1-cb506deeb43e@linaro.org > > > > > > But at least the latter looks like an omission that should be fixed. > > > > Hm, either that or the mask for sm8450 was not needed as well. Anyway, > > thanks for explanation, appreciated! > > msi-map-mask is definitely needed as it would allow all the devices under the > same bus to reuse the MSI identifier. Currently, excluding this property will > not cause any issue since there is a single device under each bus. But we cannot > assume that is going to be the case on all boards. Are you saying that there is never a use case for an identity mapping? Just on Qualcomm hardware or in general? It looks like we have a fairly large number of mainline devicetrees that do use an identity mapping here (i.e. do not specify 'msi-map-mask') and the binding document also has an explicit example of this. Documentation/devicetree/bindings/pci/pci-msi.txt > I will submit a patch to fix SM8650. Johan
On Tue, Feb 20, 2024 at 08:41:25AM +0100, Johan Hovold wrote: > On Fri, Feb 16, 2024 at 10:24:06PM +0530, Manivannan Sadhasivam wrote: > > msi-map-mask is definitely needed as it would allow all the devices under the > > same bus to reuse the MSI identifier. Currently, excluding this property will > > not cause any issue since there is a single device under each bus. But we cannot > > assume that is going to be the case on all boards. > > Are you saying that there is never a use case for an identity mapping? > Just on Qualcomm hardware or in general? > > It looks like we have a fairly large number of mainline devicetrees that > do use an identity mapping here (i.e. do not specify 'msi-map-mask') and > the binding document also has an explicit example of this. > > Documentation/devicetree/bindings/pci/pci-msi.txt The above should have said "linear mapping" as the msi-base is not always identical to the rid-base, but you get the point. Johan
On Tue, Feb 20, 2024 at 08:41:25AM +0100, Johan Hovold wrote: > On Fri, Feb 16, 2024 at 10:24:06PM +0530, Manivannan Sadhasivam wrote: > > On Wed, Feb 14, 2024 at 02:38:57PM +0100, Krzysztof Kozlowski wrote: > > > On 14/02/2024 13:54, Johan Hovold wrote: > > > > On Wed, Feb 14, 2024 at 01:01:20PM +0100, Krzysztof Kozlowski wrote: > > > >> On 12/02/2024 17:50, Johan Hovold wrote: > > > >>> Whether the 'msi-map-mask' property is needed or not depends on how the > > > >>> MSI interrupts are mapped and it should therefore not be described as > > > >>> required. > > > >> > > > >> I could imagine that on all devices the interrupts are mapped in a way > > > >> you need to provide msi-map-mask. IOW, can there be a Qualcomm platform > > > >> without msi-map-mask? > > > > > > > > I don't have access to the documentation so I'll leave that for you guys > > > > to determine. I do note that the downstream DT does not use it and that > > > > we have a new devicetree in linux-next which also does not have it: > > > > > > > > https://lore.kernel.org/r/20240125-topic-sm8650-upstream-pcie-its-v1-1-cb506deeb43e@linaro.org > > > > > > > > But at least the latter looks like an omission that should be fixed. > > > > > > Hm, either that or the mask for sm8450 was not needed as well. Anyway, > > > thanks for explanation, appreciated! > > > > msi-map-mask is definitely needed as it would allow all the devices under the > > same bus to reuse the MSI identifier. Currently, excluding this property will > > not cause any issue since there is a single device under each bus. But we cannot > > assume that is going to be the case on all boards. > > Are you saying that there is never a use case for an identity mapping? > Just on Qualcomm hardware or in general? > > It looks like we have a fairly large number of mainline devicetrees that > do use an identity mapping here (i.e. do not specify 'msi-map-mask') and > the binding document also has an explicit example of this. > > Documentation/devicetree/bindings/pci/pci-msi.txt I don't know how other platforms supposed to work without this property for more than one devices. Maybe they were not tested enough? But for sure, Qcom SoCs require either per device MSI identifier or msi-map-mask. - Mani
On Wed, Feb 21, 2024 at 10:56:07AM +0530, Manivannan Sadhasivam wrote: > On Tue, Feb 20, 2024 at 08:41:25AM +0100, Johan Hovold wrote: > > On Fri, Feb 16, 2024 at 10:24:06PM +0530, Manivannan Sadhasivam wrote: > > > msi-map-mask is definitely needed as it would allow all the devices under the > > > same bus to reuse the MSI identifier. Currently, excluding this property will > > > not cause any issue since there is a single device under each bus. But we cannot > > > assume that is going to be the case on all boards. > > > > Are you saying that there is never a use case for an identity mapping? > > Just on Qualcomm hardware or in general? > > > > It looks like we have a fairly large number of mainline devicetrees that > > do use an identity mapping here (i.e. do not specify 'msi-map-mask') and > > the binding document also has an explicit example of this. > > > > Documentation/devicetree/bindings/pci/pci-msi.txt > > I don't know how other platforms supposed to work without this property for more > than one devices. Maybe they were not tested enough? Seems a bit far fetched since it's also an example in the binding. In fact, only the two Qualcomm platforms that you added 'msi-map-mask' for use it. > But for sure, Qcom SoCs require either per device MSI identifier or > msi-map-mask. But isn't the mapping set up by the boot firmware and can differ between platforms? The mapping on sc8280xp looks quite different from sm8450/sm8650: msi-map = <0x0 &gic_its 0x5981 0x1>, <0x100 &gic_its 0x5980 0x1>; msi-map-mask = <0xff00>; Here it's obvious that the mask is needed, whereas for sc8280xp: msi-map = <0x0 &its 0xa0000 0x10000>; it's not obvious what the mask should be. In fact, it looks like Qualcomm intended a linear mapping here as the length is 0x10000 and they left out the mask. And after digging through the X13s ACPI tables, this is indeed how the hardware is configured, which means that we should not use a 'msi-map-mask' property for sc8280xp and that this patch is correct. Johan
On Wed, Feb 21, 2024 at 11:30:58AM +0100, Johan Hovold wrote: > On Wed, Feb 21, 2024 at 10:56:07AM +0530, Manivannan Sadhasivam wrote: > > On Tue, Feb 20, 2024 at 08:41:25AM +0100, Johan Hovold wrote: > > > On Fri, Feb 16, 2024 at 10:24:06PM +0530, Manivannan Sadhasivam wrote: > > > > > msi-map-mask is definitely needed as it would allow all the devices under the > > > > same bus to reuse the MSI identifier. Currently, excluding this property will > > > > not cause any issue since there is a single device under each bus. But we cannot > > > > assume that is going to be the case on all boards. > > > > > > Are you saying that there is never a use case for an identity mapping? > > > Just on Qualcomm hardware or in general? > > > > > > It looks like we have a fairly large number of mainline devicetrees that > > > do use an identity mapping here (i.e. do not specify 'msi-map-mask') and > > > the binding document also has an explicit example of this. > > > > > > Documentation/devicetree/bindings/pci/pci-msi.txt > > > > I don't know how other platforms supposed to work without this property for more > > than one devices. Maybe they were not tested enough? > > Seems a bit far fetched since it's also an example in the binding. > > In fact, only the two Qualcomm platforms that you added 'msi-map-mask' > for use it. > > > But for sure, Qcom SoCs require either per device MSI identifier or > > msi-map-mask. > > But isn't the mapping set up by the boot firmware and can differ between > platforms? > > The mapping on sc8280xp looks quite different from sm8450/sm8650: > > msi-map = <0x0 &gic_its 0x5981 0x1>, > <0x100 &gic_its 0x5980 0x1>; > msi-map-mask = <0xff00>; > > Here it's obvious that the mask is needed, whereas for sc8280xp: > > msi-map = <0x0 &its 0xa0000 0x10000>; > > it's not obvious what the mask should be. In fact, it looks like > Qualcomm intended a linear mapping here as the length is 0x10000 and > they left out the mask. > > And after digging through the X13s ACPI tables, this is indeed how the > hardware is configured, which means that we should not use a > 'msi-map-mask' property for sc8280xp and that this patch is correct. > Right. Confirmed the same with the hw team. On Qcom SoCs ITS mapping is relatively similar to SMMU stream IDs. So on SM8450 and other mobile targets making use of SMMUv2, only 128 SIDs are available, hence only 128 MSI identifiers. But on SC8280XP and other similar ones, SMMUv3 is used, so there are 65536 SIDs available and also the MSI identifiers. So yes, this SoC indeed supports linear mapping of MSI identifiers and so the mask is not required. Thanks! - Mani
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml index 5eda4e72f681..b28517047db2 100644 --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml @@ -146,7 +146,6 @@ anyOf: - "#interrupt-cells" - required: - msi-map - - msi-map-mask allOf: - $ref: /schemas/pci/pci-bus.yaml#
Whether the 'msi-map-mask' property is needed or not depends on how the MSI interrupts are mapped and it should therefore not be described as required. Note that the current schema fails to detect omissions of the mask property if the internal MSI controller properties are also present. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 1 - 1 file changed, 1 deletion(-)