[PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Parikshit Pareek]

Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
This series introduce the due support for associated interconnect, and
setting of the due interconnect-bandwidth. Setting due interconnect
bandwidth is needed to avoid the issues like [1], caused by not having
due clock votes(indirectly dependent upon interconnect bandwidth).

Parikshit Pareek (3):
  dt-bindings: arm-smmu: Add interconnect for qcom SMMUs
  arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU
  iommu/arm-smmu-qcom: Add support for the interconnect

 .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         |  4 ++++
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c    | 16 ++++++++++++++
 3 files changed, 42 insertions(+)

-- 
2.17.1

[PATCH 1/3] dt-bindings: arm-smmu: Add interconnect for qcom SMMUs

[Parikshit Pareek]

There are certain SMMUs on qcom SoCs, which need to set interconnect-
bandwidth, before accessing any MIMO mapped HW registers, and accessing
RAM during page table walk. Hence introduce the due bindings for
interconnects.

Reported-by: Eric Chanudet <echanude@redhat.com>
Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
---
 .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
index ba677d401e24..75e00789d8c2 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
@@ -327,6 +327,28 @@ allOf:
             - description: interface clock required to access smmu's registers
                 through the TCU's programming interface.
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              qcom,sa8775p-smmu-500
+    then:
+      properties:
+        interconnects:
+          minItems: 1
+          maxItems: 1
+
+        interconnect-names:
+          minItems: 1
+          items:
+            - const: tbu_mc
+
+        icc_bw:
+          $ref: /schemas/types.yaml#/definitions/int32
+          description:
+            An integer expressing the interconnect bandwidth(MBps) to be set.
+
   - if:
       properties:
         compatible:
-- 
2.17.1

Re: [PATCH 1/3] dt-bindings: arm-smmu: Add interconnect for qcom SMMUs

[Konrad Dybcio]

On 9.06.2023 07:41, Parikshit Pareek wrote:
> There are certain SMMUs on qcom SoCs, which need to set interconnect-
> bandwidth, before accessing any MIMO mapped HW registers, and accessing
> RAM during page table walk. Hence introduce the due bindings for
> interconnects.
> 
> Reported-by: Eric Chanudet <echanude@redhat.com>
> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index ba677d401e24..75e00789d8c2 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -327,6 +327,28 @@ allOf:
>              - description: interface clock required to access smmu's registers
>                  through the TCU's programming interface.
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              qcom,sa8775p-smmu-500
> +    then:
> +      properties:
> +        interconnects:
This isn't specific to SA8775P.. I believe we could make it SMMU-generic..

> +          minItems: 1
> +          maxItems: 1
> +
> +        interconnect-names:
> +          minItems: 1
> +          items:
> +            - const: tbu_mc
> +
> +        icc_bw:
No underscores in property names.
> +          $ref: /schemas/types.yaml#/definitions/int32
Can't we use OPP tables? They'd also allow for specifying required-opps.

Konrad
> +          description:
> +            An integer expressing the interconnect bandwidth(MBps) to be set.
> +
>    - if:
>        properties:
>          compatible:

Re: [PATCH 1/3] dt-bindings: arm-smmu: Add interconnect for qcom SMMUs

[Krzysztof Kozlowski]

On 09/06/2023 07:41, Parikshit Pareek wrote:
> There are certain SMMUs on qcom SoCs, which need to set interconnect-
> bandwidth, before accessing any MIMO mapped HW registers, and accessing
> RAM during page table walk. Hence introduce the due bindings for
> interconnects.
> 
> Reported-by: Eric Chanudet <echanude@redhat.com>

What is reported here exactly? What is the bug?

> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index ba677d401e24..75e00789d8c2 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -327,6 +327,28 @@ allOf:
>              - description: interface clock required to access smmu's registers
>                  through the TCU's programming interface.
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              qcom,sa8775p-smmu-500
> +    then:
> +      properties:
> +        interconnects:
> +          minItems: 1

Drop minItems

> +          maxItems: 1
> +
> +        interconnect-names:
> +          minItems: 1

??? Drop

> +          items:
> +            - const: tbu_mc

Anyway, properties must be defined in top-level. In if block you only
customize them.

> +
> +        icc_bw:
> +          $ref: /schemas/types.yaml#/definitions/int32

No, for multiple reasons. First - do not define properties in if: block.
Second, does not look like description of hardware. I actually don't
understand what is this for. :(

Best regards,
Krzysztof

[PATCH 2/3] arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU

[Parikshit Pareek]

Introduce the interconnect, connecting PCIe SMMU to the memory. This
is accessed during memory mapped IO access of smmu registers, and
during page table walks.

Reported-by: Eric Chanudet <echanude@redhat.com>
Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index b130136acffe..ea3c37019c46 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -2137,6 +2137,10 @@
 				     <GIC_SPI 639 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>;
+			interconnects = <&pcie_anoc MASTER_PCIE_0 QCOM_ICC_TAG_ALWAYS
+					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
+			interconnect-names = "tbu_mc";
+			icc_bw = <250>;
 		};
 
 		intc: interrupt-controller@17a00000 {
-- 
2.17.1

Re: [PATCH 2/3] arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU

[Krzysztof Kozlowski]

On 09/06/2023 07:41, Parikshit Pareek wrote:
> Introduce the interconnect, connecting PCIe SMMU to the memory. This
> is accessed during memory mapped IO access of smmu registers, and
> during page table walks.
> 
> Reported-by: Eric Chanudet <echanude@redhat.com>
> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index b130136acffe..ea3c37019c46 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -2137,6 +2137,10 @@
>  				     <GIC_SPI 639 IRQ_TYPE_LEVEL_HIGH>,
>  				     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
>  				     <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>;
> +			interconnects = <&pcie_anoc MASTER_PCIE_0 QCOM_ICC_TAG_ALWAYS
> +					&mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "tbu_mc";
> +			icc_bw = <250>;

Why 250? Why it cannot change during system run depending on the needs?


Best regards,
Krzysztof

[PATCH 3/3] iommu/arm-smmu-qcom: Add support for the interconnect

[Parikshit Pareek]

Introduce support to detect the interconnect, and set its bandwidth.
For certain targets, we need to set the bandwidth of interconnect,
connecting smmu to memory. This is accessed during memory mapped IO
access to smmu registers, and during page tables walks.

Reported-by: Eric Chanudet <echanude@redhat.com>
Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index c71afda79d64..6961d564869b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/of_device.h>
 #include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/interconnect.h>
 
 #include "arm-smmu.h"
 #include "arm-smmu-qcom.h"
@@ -549,6 +550,8 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
 {
 	const struct device_node *np = smmu->dev->of_node;
 	const struct of_device_id *match;
+	struct icc_path *icc_path;
+	int ret, icc_bw;
 
 #ifdef CONFIG_ACPI
 	if (np == NULL) {
@@ -558,6 +561,19 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
 	}
 #endif
 
+	icc_path = devm_of_icc_get(smmu->dev, "tbu_mc");
+	if (IS_ERR(icc_path))
+		return (struct arm_smmu_device *)icc_path;
+
+	ret = of_property_read_u32(np, "icc_bw", &icc_bw);
+
+	/*if interconnect exists, check for the  bandwidth value*/
+	if (icc_path && !ret) {
+		ret = icc_set_bw(icc_path, 0, MBps_to_icc(icc_bw));
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	match = of_match_node(qcom_smmu_impl_of_match, np);
 	if (match)
 		return qcom_smmu_create(smmu, match->data);
-- 
2.17.1

Re: [PATCH 3/3] iommu/arm-smmu-qcom: Add support for the interconnect

[Konrad Dybcio]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]



On 9.06.2023 07:52, Parikshit Pareek wrote:
> Introduce support to detect the interconnect, and set its bandwidth.
> For certain targets, we need to set the bandwidth of interconnect,
> connecting smmu to memory. This is accessed during memory mapped IO
> access to smmu registers, and during page tables walks.
> 
> Reported-by: Eric Chanudet <echanude@redhat.com>
> Signed-off-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> ---
Quite recently, I've been toying with this too.. I coded it in
a way that allows it to be reused by other impls and uses OPP APIs.
Please take a look at the attached patch.

Konrad
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index c71afda79d64..6961d564869b 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -8,6 +8,7 @@
>  #include <linux/delay.h>
>  #include <linux/of_device.h>
>  #include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/interconnect.h>
>  
>  #include "arm-smmu.h"
>  #include "arm-smmu-qcom.h"
> @@ -549,6 +550,8 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
>  {
>  	const struct device_node *np = smmu->dev->of_node;
>  	const struct of_device_id *match;
> +	struct icc_path *icc_path;
> +	int ret, icc_bw;
>  
>  #ifdef CONFIG_ACPI
>  	if (np == NULL) {
> @@ -558,6 +561,19 @@ struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
>  	}
>  #endif
>  
> +	icc_path = devm_of_icc_get(smmu->dev, "tbu_mc");
> +	if (IS_ERR(icc_path))
> +		return (struct arm_smmu_device *)icc_path;
> +
> +	ret = of_property_read_u32(np, "icc_bw", &icc_bw);
> +
> +	/*if interconnect exists, check for the  bandwidth value*/
> +	if (icc_path && !ret) {
> +		ret = icc_set_bw(icc_path, 0, MBps_to_icc(icc_bw));
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
>  	match = of_match_node(qcom_smmu_impl_of_match, np);
>  	if (match)
>  		return qcom_smmu_create(smmu, match->data);

[-- Attachment #2: 0001-smmu-opp-wip.patch --]
[-- Type: text/x-patch, Size: 2616 bytes --]

From 8112d8f8b9a43178af3b203e3fcf94cbd24510a4 Mon Sep 17 00:00:00 2001
From: Konrad Dybcio <konrad.dybcio@linaro.org>
Date: Tue, 30 May 2023 23:39:56 +0200
Subject: [PATCH] smmu opp wip

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 41 ++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index a86acd76c1df..03448bf8ae7e 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -32,6 +32,7 @@
 #include <linux/of_device.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/ratelimit.h>
 #include <linux/slab.h>
@@ -1970,7 +1971,10 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,
 {
 	const struct arm_smmu_match_data *data;
 	struct device *dev = smmu->dev;
+	struct dev_pm_opp *opp;
 	bool legacy_binding;
+	unsigned int bw = 0;
+	int ret;
 
 	if (of_property_read_u32(dev->of_node, "#global-interrupts", global_irqs))
 		return dev_err_probe(dev, -ENODEV,
@@ -1998,7 +2002,25 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,
 	if (of_dma_is_coherent(dev->of_node))
 		smmu->features |= ARM_SMMU_FEAT_COHERENT_WALK;
 
-	return 0;
+	/* It's fine to omit the OPP table */
+	if (!dev_pm_opp_of_get_opp_desc_node(dev))
+		return 0;
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret)
+		return ret;
+
+	ret = dev_pm_opp_of_find_icc_paths(dev, NULL);
+	if (ret)
+		return ret;
+
+	opp = dev_pm_opp_find_bw_ceil(dev, &bw, 0);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	dev_pm_opp_set_opp(dev, opp);
+
+	return ret;
 }
 
 static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
@@ -2224,8 +2246,20 @@ static void arm_smmu_device_remove(struct platform_device *pdev)
 static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
 {
 	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+	struct dev_pm_opp *opp;
+	unsigned int bw = 0;
 	int ret;
 
+	if (PTR_ERR(dev_pm_opp_get_opp_table(dev)) < 0)
+		goto skip_opp;
+
+	opp = dev_pm_opp_find_bw_ceil(dev, &bw, 0);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	dev_pm_opp_set_opp(dev, opp);
+
+skip_opp:
 	ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
 	if (ret)
 		return ret;
@@ -2241,6 +2275,11 @@ static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
 
 	clk_bulk_disable(smmu->num_clks, smmu->clks);
 
+	if (PTR_ERR(dev_pm_opp_get_opp_table(dev)) < 0)
+		return 0;
+
+	dev_pm_opp_set_opp(dev, NULL);
+
 	return 0;
 }
 
-- 
2.41.0

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Konrad Dybcio]

On 9.06.2023 07:41, Parikshit Pareek wrote:
> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
> This series introduce the due support for associated interconnect, and
> setting of the due interconnect-bandwidth. Setting due interconnect
> bandwidth is needed to avoid the issues like [1], caused by not having
> due clock votes(indirectly dependent upon interconnect bandwidth).

[1] ???

Konrad
> 
> Parikshit Pareek (3):
>   dt-bindings: arm-smmu: Add interconnect for qcom SMMUs
>   arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU
>   iommu/arm-smmu-qcom: Add support for the interconnect
> 
>  .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi         |  4 ++++
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c    | 16 ++++++++++++++
>  3 files changed, 42 insertions(+)
> 

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Parikshit Pareek]

On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
> 
> 
> On 9.06.2023 07:41, Parikshit Pareek wrote:
> > Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
> > This series introduce the due support for associated interconnect, and
> > setting of the due interconnect-bandwidth. Setting due interconnect
> > bandwidth is needed to avoid the issues like [1], caused by not having
> > due clock votes(indirectly dependent upon interconnect bandwidth).
> 
> [1] ???

My bad. Intended to mention following:
https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/

Regards,
Parikshit Pareek
> 
> Konrad
> > 
> > Parikshit Pareek (3):
> >   dt-bindings: arm-smmu: Add interconnect for qcom SMMUs
> >   arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU
> >   iommu/arm-smmu-qcom: Add support for the interconnect
> > 
> >  .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
> >  arch/arm64/boot/dts/qcom/sa8775p.dtsi         |  4 ++++
> >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c    | 16 ++++++++++++++
> >  3 files changed, 42 insertions(+)
> > 

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Robin Murphy]

On 2023-06-09 13:56, Parikshit Pareek wrote:
> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
>>
>>
>> On 9.06.2023 07:41, Parikshit Pareek wrote:
>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
>>> This series introduce the due support for associated interconnect, and
>>> setting of the due interconnect-bandwidth. Setting due interconnect
>>> bandwidth is needed to avoid the issues like [1], caused by not having
>>> due clock votes(indirectly dependent upon interconnect bandwidth).
>>
>> [1] ???
> 
> My bad. Intended to mention following:
> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/

This sounds super-dodgy - do you really have to rely on configuration of 
the interconnect path from the SMMU's pagetable walker to RAM to keep a 
completely different interconnect path clocked for the CPU to access 
SMMU registers? You can't just request the programming interface clock 
directly like on other SoCs?

Thanks,
Robin.

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Konrad Dybcio]

On 9.06.2023 16:45, Robin Murphy wrote:
> On 2023-06-09 13:56, Parikshit Pareek wrote:
>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
>>>
>>>
>>> On 9.06.2023 07:41, Parikshit Pareek wrote:
>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
>>>> This series introduce the due support for associated interconnect, and
>>>> setting of the due interconnect-bandwidth. Setting due interconnect
>>>> bandwidth is needed to avoid the issues like [1], caused by not having
>>>> due clock votes(indirectly dependent upon interconnect bandwidth).
>>>
>>> [1] ???
>>
>> My bad. Intended to mention following:
>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/
> 
> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs?
On Qualcomm platforms, particularly so with the more recent ones, some
clocks are managed by various remote cores. Half of what the interconnect
infra does on these SoCs is telling one such core to change the internally
managed clock's rate based on the requested bw.

Konrad
> 
> Thanks,
> Robin.

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Dmitry Baryshkov]

On Fri, 9 Jun 2023 at 17:52, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 9.06.2023 16:45, Robin Murphy wrote:
> > On 2023-06-09 13:56, Parikshit Pareek wrote:
> >> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
> >>>
> >>>
> >>> On 9.06.2023 07:41, Parikshit Pareek wrote:
> >>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
> >>>> This series introduce the due support for associated interconnect, and
> >>>> setting of the due interconnect-bandwidth. Setting due interconnect
> >>>> bandwidth is needed to avoid the issues like [1], caused by not having
> >>>> due clock votes(indirectly dependent upon interconnect bandwidth).
> >>>
> >>> [1] ???
> >>
> >> My bad. Intended to mention following:
> >> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/
> >
> > This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs?
> On Qualcomm platforms, particularly so with the more recent ones, some
> clocks are managed by various remote cores. Half of what the interconnect
> infra does on these SoCs is telling one such core to change the internally
> managed clock's rate based on the requested bw.

But enabling PCIe interconnect to keep SMMU working sounds strange to
me too. Does the fault come from some outstanding PCIe transaction?


-- 
With best wishes
Dmitry

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Robin Murphy]

On 2023-06-09 15:56, Dmitry Baryshkov wrote:
> On Fri, 9 Jun 2023 at 17:52, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 9.06.2023 16:45, Robin Murphy wrote:
>>> On 2023-06-09 13:56, Parikshit Pareek wrote:
>>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 9.06.2023 07:41, Parikshit Pareek wrote:
>>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
>>>>>> This series introduce the due support for associated interconnect, and
>>>>>> setting of the due interconnect-bandwidth. Setting due interconnect
>>>>>> bandwidth is needed to avoid the issues like [1], caused by not having
>>>>>> due clock votes(indirectly dependent upon interconnect bandwidth).
>>>>>
>>>>> [1] ???
>>>>
>>>> My bad. Intended to mention following:
>>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/
>>>
>>> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs?
>> On Qualcomm platforms, particularly so with the more recent ones, some
>> clocks are managed by various remote cores. Half of what the interconnect
>> infra does on these SoCs is telling one such core to change the internally
>> managed clock's rate based on the requested bw.
> 
> But enabling PCIe interconnect to keep SMMU working sounds strange to
> me too. Does the fault come from some outstanding PCIe transaction?

The "Injecting instruction/data abort to VM 3" message from the 
hypervisor implies that it is the access to SMMU_CR0 from 
arm_smmu_shutdown() that's blown up. I can even believe that the SMMU 
shares some clocks with the PCIe interconnect, given that its TBU must 
be *in* that path from PCIe to memory, at least. However I would 
instinctively expect the abstraction layers above to have some notion of 
distinct votes for "CPU wants to access SMMU" vs. "SMMU/PCIe wants to 
access RAM", given that the latter is liable to need to enable more than 
the former if the clock/power gating is as fine-grained as previous SoCs 
seem to have been. But maybe my hunch is wrong and this time 
everything's just in one big clock domain. I don't know. I'm just here 
to ask questions to establish whether this really is the most correct 
abstraction or just a lazy bodge to avoid doing the proper thing in some 
other driver.

Thanks,
Robin.

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Shazad Hussain]

Hi,

On 6/9/2023 9:09 PM, Robin Murphy wrote:
> On 2023-06-09 15:56, Dmitry Baryshkov wrote:
>> On Fri, 9 Jun 2023 at 17:52, Konrad Dybcio <konrad.dybcio@linaro.org> 
>> wrote:
>>>
>>>
>>>
>>> On 9.06.2023 16:45, Robin Murphy wrote:
>>>> On 2023-06-09 13:56, Parikshit Pareek wrote:
>>>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
>>>>>>
>>>>>>
>>>>>> On 9.06.2023 07:41, Parikshit Pareek wrote:
>>>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth 
>>>>>>> to be
>>>>>>> This series introduce the due support for associated 
>>>>>>> interconnect, and
>>>>>>> setting of the due interconnect-bandwidth. Setting due interconnect
>>>>>>> bandwidth is needed to avoid the issues like [1], caused by not 
>>>>>>> having
>>>>>>> due clock votes(indirectly dependent upon interconnect bandwidth).
>>>>>>
>>>>>> [1] ???
>>>>>
>>>>> My bad. Intended to mention following:
>>>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/
>>>>
>>>> This sounds super-dodgy - do you really have to rely on 
>>>> configuration of the interconnect path from the SMMU's pagetable 
>>>> walker to RAM to keep a completely different interconnect path 
>>>> clocked for the CPU to access SMMU registers? You can't just request 
>>>> the programming interface clock directly like on other SoCs?
>>> On Qualcomm platforms, particularly so with the more recent ones, some
>>> clocks are managed by various remote cores. Half of what the 
>>> interconnect
>>> infra does on these SoCs is telling one such core to change the 
>>> internally
>>> managed clock's rate based on the requested bw.
>>
>> But enabling PCIe interconnect to keep SMMU working sounds strange to
>> me too. Does the fault come from some outstanding PCIe transaction?
> 
> The "Injecting instruction/data abort to VM 3" message from the 
> hypervisor implies that it is the access to SMMU_CR0 from 
> arm_smmu_shutdown() that's blown up. I can even believe that the SMMU 
> shares some clocks with the PCIe interconnect, given that its TBU must 
> be *in* that path from PCIe to memory, at least. However I would 
> instinctively expect the abstraction layers above to have some notion of 
> distinct votes for "CPU wants to access SMMU" vs. "SMMU/PCIe wants to 
> access RAM", given that the latter is liable to need to enable more than 
> the former if the clock/power gating is as fine-grained as previous SoCs 
> seem to have been. But maybe my hunch is wrong and this time 
> everything's just in one big clock domain. I don't know. I'm just here 
> to ask questions to establish whether this really is the most correct 
> abstraction or just a lazy bodge to avoid doing the proper thing in some 
> other driver.
> 
> Thanks,
> Robin.

For this platform to access the SMMU_CR0 we need to have pcie_tcu_clk
enabled and in order to do so we have to have interconnect vote from
MASTER_PCIE_[0/1] -> SLAVE_ANOC_PCIE_GEM_NOC so that AOP/RPMH can enable
aggre_noc_pcie_sf_clk_src which in turns enables bulk of clocks of which
pcie_tcu_clk is one.

    ---
   |RAM|
  ------------       -----      -----------       ----------
| GEMNOC     |<----| TBU |----| PCIE ANOC |<----| pcie_0/1 |
  ------------       -----      -----------       ----------
    ^      ^           ^
    |      |           |
    |      v           v
   ---   -----------------
  |CPU| |PCIE TCU (smmuv2)|
   ---   -----------------

I think this should be the right driver to implement this to have a sync
with vote/unvote of the clock while the smmu register is being accessed
in arm_smmu_shutdown() right !

-Shazad

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Robin Murphy]

On 2023-06-09 15:52, Konrad Dybcio wrote:
> 
> 
> On 9.06.2023 16:45, Robin Murphy wrote:
>> On 2023-06-09 13:56, Parikshit Pareek wrote:
>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 9.06.2023 07:41, Parikshit Pareek wrote:
>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
>>>>> This series introduce the due support for associated interconnect, and
>>>>> setting of the due interconnect-bandwidth. Setting due interconnect
>>>>> bandwidth is needed to avoid the issues like [1], caused by not having
>>>>> due clock votes(indirectly dependent upon interconnect bandwidth).
>>>>
>>>> [1] ???
>>>
>>> My bad. Intended to mention following:
>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/
>>
>> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs?
> On Qualcomm platforms, particularly so with the more recent ones, some
> clocks are managed by various remote cores. Half of what the interconnect
> infra does on these SoCs is telling one such core to change the internally
> managed clock's rate based on the requested bw.

That much I get, it just seems like an arse-backwards design decision if 
it's really necessary to pretend the SMMU needs to access memory in 
order for the CPU to be able to access the SMMU. The respective SMMU 
interfaces are functionally independent of each other - even if it is 
the case in the integration that both interfaces and/or the internal TCU 
clock do happen to be driven synchronously from the same parent clock - 
and in any sane interconnect the CPU->SMMU and SMMU->RAM routes would be 
completely different and not intersect at all.

Thanks,
Robin.

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Konrad Dybcio]

On 9.06.2023 17:07, Robin Murphy wrote:
> On 2023-06-09 15:52, Konrad Dybcio wrote:
>>
>>
>> On 9.06.2023 16:45, Robin Murphy wrote:
>>> On 2023-06-09 13:56, Parikshit Pareek wrote:
>>>> On Fri, Jun 09, 2023 at 10:52:26AM +0200, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 9.06.2023 07:41, Parikshit Pareek wrote:
>>>>>> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
>>>>>> This series introduce the due support for associated interconnect, and
>>>>>> setting of the due interconnect-bandwidth. Setting due interconnect
>>>>>> bandwidth is needed to avoid the issues like [1], caused by not having
>>>>>> due clock votes(indirectly dependent upon interconnect bandwidth).
>>>>>
>>>>> [1] ???
>>>>
>>>> My bad. Intended to mention following:
>>>> https://lore.kernel.org/linux-arm-msm/20230418165224.vmok75fwcjqdxspe@echanude/
>>>
>>> This sounds super-dodgy - do you really have to rely on configuration of the interconnect path from the SMMU's pagetable walker to RAM to keep a completely different interconnect path clocked for the CPU to access SMMU registers? You can't just request the programming interface clock directly like on other SoCs?
>> On Qualcomm platforms, particularly so with the more recent ones, some
>> clocks are managed by various remote cores. Half of what the interconnect
>> infra does on these SoCs is telling one such core to change the internally
>> managed clock's rate based on the requested bw.
> 
> That much I get, it just seems like an arse-backwards design decision if it's really necessary to pretend the SMMU needs to access memory in order for the CPU to be able to access the SMMU. The respective SMMU interfaces are functionally independent of each other - even if it is the case in the integration that both interfaces and/or the internal TCU clock do happen to be driven synchronously from the same parent clock - and in any sane interconnect the CPU->SMMU and SMMU->RAM routes would be completely different and not intersect at all.
Well, it's not the first time we stumble into a.. peculiar.. design decision on
these SoCs.. That said, we can't do much about it now..

On older SoCs, some interconnect paths were strongly associated with specific
TBUs which were responsible for specific SID ranges..

In this specific case, it looks like SIDs 0x000-0x3ff should correspond to
PCIE0 and 0x400-0x7ff to PCIE1. But the line isn't drawn very clearly this
time around, so maybe there's some internal spaghetti.

Konrad
> 
> Thanks,
> Robin.

Re: [PATCH 0/3] arm64: dts: qcom: sa8775p: Add interconnect to SMMU

[Manivannan Sadhasivam]

On Fri, Jun 09, 2023 at 11:11:39AM +0530, Parikshit Pareek wrote:
> Some qcom SoCs have SMMUs, which need the interconnect bandwidth to be
> This series introduce the due support for associated interconnect, and
> setting of the due interconnect-bandwidth. Setting due interconnect
> bandwidth is needed to avoid the issues like [1], caused by not having
> due clock votes(indirectly dependent upon interconnect bandwidth).
> 

As discussed offline, once you enable the PCIe RC driver which votes for this
interconnect path (pcie-mem) like other platforms [1], then you do not need this
series. This interconnect path belongs to the PCIe RC controller. So it is the
responsibility of the PCIe RC driver to vote for this path and that's what the
driver is already doing.

- Mani

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp.dtsi#n1767

> Parikshit Pareek (3):
>   dt-bindings: arm-smmu: Add interconnect for qcom SMMUs
>   arm64: dts: qcom: sa8775p: Add interconnect to PCIe SMMU
>   iommu/arm-smmu-qcom: Add support for the interconnect
> 
>  .../devicetree/bindings/iommu/arm,smmu.yaml   | 22 +++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi         |  4 ++++
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c    | 16 ++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> -- 
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்