[PATCH v3 0/6] mtk_iommu: Specify phandles to infracfg and pericfg

[PATCH v3 0/6] mtk_iommu: Specify phandles to infracfg and pericfg

[AngeloGioacchino Del Regno]

The IOMMU has registers in the infracfg and/or pericfg iospaces: as
for the currently supported SoCs, MT2712 and MT8173 need a phandle to
infracfg, while MT8195 needs one to pericfg.

Before this change, the driver was checking for a SoC-specific infra/peri
compatible but, sooner or later, these lists are going to grow a lot...
...and this is why it was chosen to add phandles (as it was done with
some other drivers already - look at mtk-pm-domains, mt8192-afe

Please note that, while it was necessary to update the devicetrees for
MT8173 and MT2712e, there was no update for MT8195 because there is no
IOMMU node in there yet.

Changes in v3:
 - Different squashing of dt-bindings patches (sorry for misunderstanding!)
 - Removed legacy devicetree print

Changes in v2:
 - Squashed dt-bindings patches as suggested by Matthias
 - Removed quotes from infra/peri phandle refs
 - Changed dev_warn to dev_info in patches [2/7], [3/7]

AngeloGioacchino Del Regno (6):
  dt-bindings: iommu: mediatek: Add mediatek,infracfg phandle
  iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg
  arm64: dts: mediatek: mt8173: Add mediatek,infracfg phandle for IOMMU
  arm64: dts: mediatek: mt2712e: Add mediatek,infracfg phandle for IOMMU
  dt-bindings: iommu: mediatek: Add mediatek,pericfg phandle
  iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

 .../bindings/iommu/mediatek,iommu.yaml        | 30 +++++++++
 arch/arm64/boot/dts/mediatek/mt2712e.dtsi     |  2 +
 arch/arm64/boot/dts/mediatek/mt8173.dtsi      |  1 +
 drivers/iommu/mtk_iommu.c                     | 61 +++++++++++--------
 4 files changed, 70 insertions(+), 24 deletions(-)

-- 
2.35.1

[PATCH v3 1/6] dt-bindings: iommu: mediatek: Add mediatek,infracfg phandle

[AngeloGioacchino Del Regno]

Add property "mediatek,infracfg" to let the mtk_iommu driver retrieve
a phandle to the infracfg syscon instead of performing a per-soc
compatible lookup in the entire devicetree and set it as a required
property for MT2712 and MT8173.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../bindings/iommu/mediatek,iommu.yaml           | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
index 2ae3bbad7f1a..4142a568b293 100644
--- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
+++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
@@ -101,6 +101,10 @@ properties:
     items:
       - const: bclk
 
+  mediatek,infracfg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle to the mediatek infracfg syscon
+
   mediatek,larbs:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     minItems: 1
@@ -167,6 +171,18 @@ allOf:
       required:
         - power-domains
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt2712-m4u
+              - mediatek,mt8173-m4u
+
+    then:
+      required:
+        - mediatek,infracfg
+
   - if: # The IOMMUs don't have larbs.
       not:
         properties:
-- 
2.35.1

[PATCH v3 2/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg

[AngeloGioacchino Del Regno]

This driver will get support for more SoCs and the list of infracfg
compatibles is expected to grow: in order to prevent getting this
situation out of control and see a long list of compatible strings,
add support to retrieve a handle to infracfg's regmap through a
new "mediatek,infracfg" phandle.

In order to keep retrocompatibility with older devicetrees, the old
way is kept in place.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/mtk_iommu.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index bb9dd92c9898..90685946fcbe 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1140,22 +1140,32 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
 
 	if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE)) {
-		switch (data->plat_data->m4u_plat) {
-		case M4U_MT2712:
-			p = "mediatek,mt2712-infracfg";
-			break;
-		case M4U_MT8173:
-			p = "mediatek,mt8173-infracfg";
-			break;
-		default:
-			p = NULL;
+		infracfg = syscon_regmap_lookup_by_phandle(dev->of_node, "mediatek,infracfg");
+		if (IS_ERR(infracfg)) {
+			/*
+			 * Legacy devicetrees will not specify a phandle to
+			 * mediatek,infracfg: in that case, we use the older
+			 * way to retrieve a syscon to infra.
+			 *
+			 * This is for retrocompatibility purposes only, hence
+			 * no more compatibles shall be added to this.
+			 */
+			switch (data->plat_data->m4u_plat) {
+			case M4U_MT2712:
+				p = "mediatek,mt2712-infracfg";
+				break;
+			case M4U_MT8173:
+				p = "mediatek,mt8173-infracfg";
+				break;
+			default:
+				p = NULL;
+			}
+
+			infracfg = syscon_regmap_lookup_by_compatible(p);
+			if (IS_ERR(infracfg))
+				return PTR_ERR(infracfg);
 		}
 
-		infracfg = syscon_regmap_lookup_by_compatible(p);
-
-		if (IS_ERR(infracfg))
-			return PTR_ERR(infracfg);
-
 		ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
 		if (ret)
 			return ret;
-- 
2.35.1

[PATCH v3 3/6] arm64: dts: mediatek: mt8173: Add mediatek,infracfg phandle for IOMMU

[AngeloGioacchino Del Regno]

The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
new way to retrieve a syscon to that:
even though the old way is retained, it has been deprecated and the
driver will write a message in kmsg advertising to use the phandle
way instead.

For this reason, assign the right phandle to mediatek,infracfg in
the iommu node.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 40d7b47fc52e..825a3c670373 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -588,6 +588,7 @@ iommu: iommu@10205000 {
 			interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>;
 			clocks = <&infracfg CLK_INFRA_M4U>;
 			clock-names = "bclk";
+			mediatek,infracfg = <&infracfg>;
 			mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
 					 <&larb3>, <&larb4>, <&larb5>;
 			#iommu-cells = <1>;
-- 
2.35.1

[PATCH v3 4/6] arm64: dts: mediatek: mt2712e: Add mediatek,infracfg phandle for IOMMU

[AngeloGioacchino Del Regno]

The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
new way to retrieve a syscon to that:
even though the old way is retained, it has been deprecated and the
driver will write a message in kmsg advertising to use the phandle
way instead.

For this reason, assign the right phandle to mediatek,infracfg in
the iommu node.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
index 623eb3beabf2..4797537cb368 100644
--- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
@@ -329,6 +329,7 @@ iommu0: iommu@10205000 {
 		interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_LOW>;
 		clocks = <&infracfg CLK_INFRA_M4U>;
 		clock-names = "bclk";
+		mediatek,infracfg = <&infracfg>;
 		mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
 				 <&larb3>, <&larb6>;
 		#iommu-cells = <1>;
@@ -346,6 +347,7 @@ iommu1: iommu@1020a000 {
 		interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_LOW>;
 		clocks = <&infracfg CLK_INFRA_M4U>;
 		clock-names = "bclk";
+		mediatek,infracfg = <&infracfg>;
 		mediatek,larbs = <&larb4>, <&larb5>, <&larb7>;
 		#iommu-cells = <1>;
 	};
-- 
2.35.1

[PATCH v3 5/6] dt-bindings: iommu: mediatek: Add mediatek,pericfg phandle

[AngeloGioacchino Del Regno]

Add property "mediatek,pericfg" to let the mtk_iommu driver retrieve
a phandle to the infracfg syscon instead of performing a per-soc
compatible lookup in the entire devicetree and set it as a required
property for MT8195's infra IOMMU.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../devicetree/bindings/iommu/mediatek,iommu.yaml  | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
index 4142a568b293..d5e3272a54e8 100644
--- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
+++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
@@ -116,6 +116,10 @@ properties:
       Refer to bindings/memory-controllers/mediatek,smi-larb.yaml. It must sort
       according to the local arbiter index, like larb0, larb1, larb2...
 
+  mediatek,pericfg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle to the mediatek pericfg syscon
+
   '#iommu-cells':
     const: 1
     description: |
@@ -183,6 +187,16 @@ allOf:
       required:
         - mediatek,infracfg
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mediatek,mt8195-iommu-infra
+
+    then:
+      required:
+        - mediatek,pericfg
+
   - if: # The IOMMUs don't have larbs.
       not:
         properties:
-- 
2.35.1

[PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[AngeloGioacchino Del Regno]

On some SoCs (of which only MT8195 is supported at the time of writing),
the "R" and "W" (I/O) enable bits for the IOMMUs are in the pericfg_ao
register space and not in the IOMMU space: as it happened already with
infracfg, it is expected that this list will grow.

Instead of specifying pericfg compatibles on a per-SoC basis, following
what was done with infracfg, let's lookup the syscon by phandle instead.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/mtk_iommu.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 90685946fcbe..0ea0848581e9 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -138,6 +138,8 @@
 /* PM and clock always on. e.g. infra iommu */
 #define PM_CLK_AO			BIT(15)
 #define IFA_IOMMU_PCIE_SUPPORT		BIT(16)
+/* IOMMU I/O (r/w) is enabled using PERICFG_IOMMU_1 register */
+#define HAS_PERI_IOMMU1_REG		BIT(17)
 
 #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
 				((((pdata)->flags) & (mask)) == (_x))
@@ -187,7 +189,6 @@ struct mtk_iommu_plat_data {
 	u32			flags;
 	u32			inv_sel_reg;
 
-	char			*pericfg_comp_str;
 	struct list_head	*hw_list;
 	unsigned int		iova_region_nr;
 	const struct mtk_iommu_iova_region	*iova_region;
@@ -1218,14 +1219,16 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 			goto out_runtime_disable;
 		}
 	} else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
-		   data->plat_data->pericfg_comp_str) {
-		infracfg = syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
-		if (IS_ERR(infracfg)) {
-			ret = PTR_ERR(infracfg);
-			goto out_runtime_disable;
+		   MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_PERI_IOMMU1_REG)) {
+		data->pericfg = syscon_regmap_lookup_by_phandle(dev->of_node, "mediatek,pericfg");
+		if (IS_ERR(data->pericfg)) {
+			p = "mediatek,mt8195-pericfg_ao";
+			data->pericfg = syscon_regmap_lookup_by_compatible(p);
+			if (IS_ERR(data->pericfg)) {
+				ret = PTR_ERR(data->pericfg);
+				goto out_runtime_disable;
+			}
 		}
-
-		data->pericfg = infracfg;
 	}
 
 	platform_set_drvdata(pdev, data);
@@ -1484,8 +1487,8 @@ static const struct mtk_iommu_plat_data mt8192_data = {
 static const struct mtk_iommu_plat_data mt8195_data_infra = {
 	.m4u_plat	  = M4U_MT8195,
 	.flags            = WR_THROT_EN | DCM_DISABLE | STD_AXI_MODE | PM_CLK_AO |
-			    MTK_IOMMU_TYPE_INFRA | IFA_IOMMU_PCIE_SUPPORT,
-	.pericfg_comp_str = "mediatek,mt8195-pericfg_ao",
+			    HAS_PERI_IOMMU1_REG | MTK_IOMMU_TYPE_INFRA |
+			    IFA_IOMMU_PCIE_SUPPORT,
 	.inv_sel_reg      = REG_MMU_INV_SEL_GEN2,
 	.banks_num	  = 5,
 	.banks_enable     = {true, false, false, false, true},
-- 
2.35.1

Re: [PATCH v3 1/6] dt-bindings: iommu: mediatek: Add mediatek,infracfg phandle

[Rob Herring]

On Thu, 09 Jun 2022 12:07:57 +0200, AngeloGioacchino Del Regno wrote:
> Add property "mediatek,infracfg" to let the mtk_iommu driver retrieve
> a phandle to the infracfg syscon instead of performing a per-soc
> compatible lookup in the entire devicetree and set it as a required
> property for MT2712 and MT8173.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../bindings/iommu/mediatek,iommu.yaml           | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/mediatek,iommu.example.dtb: iommu@10205000: 'mediatek,infracfg' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 2/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg

[Miles Chen]

> This driver will get support for more SoCs and the list of infracfg
> compatibles is expected to grow: in order to prevent getting this
> situation out of control and see a long list of compatible strings,
> add support to retrieve a handle to infracfg's regmap through a
> new "mediatek,infracfg" phandle.

I also like the idea of using mediatek,infracfg instead of a list of
platform compatible strings.

Reviewed-by: Miles Chen <miles.chen@mediatek.com> 

> 
> In order to keep retrocompatibility with older devicetrees, the old
> way is kept in place.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v3 3/6] arm64: dts: mediatek: mt8173: Add mediatek,infracfg phandle for IOMMU

[Miles Chen]

> The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
> new way to retrieve a syscon to that:
> even though the old way is retained, it has been deprecated and the
> driver will write a message in kmsg advertising to use the phandle
> way instead.
> 
> For this reason, assign the right phandle to mediatek,infracfg in
> the iommu node.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Miles Chen <miles.chen@mediatek.com> 

> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 40d7b47fc52e..825a3c670373 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -588,6 +588,7 @@ iommu: iommu@10205000 {
>  			interrupts = <GIC_SPI 139 IRQ_TYPE_LEVEL_LOW>;
>  			clocks = <&infracfg CLK_INFRA_M4U>;
>  			clock-names = "bclk";
> +			mediatek,infracfg = <&infracfg>;
>  			mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
>  					 <&larb3>, <&larb4>, <&larb5>;
>  			#iommu-cells = <1>;
> -- 
> 2.35.1
> 
> 

Re: [PATCH v3 4/6] arm64: dts: mediatek: mt2712e: Add mediatek,infracfg phandle for IOMMU

[Miles Chen]

> The IOMMU driver now looks for the "mediatek,infracfg" phandle as a
> new way to retrieve a syscon to that:
> even though the old way is retained, it has been deprecated and the
> driver will write a message in kmsg advertising to use the phandle
> way instead.
> 
> For this reason, assign the right phandle to mediatek,infracfg in
> the iommu node.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Miles Chen <miles.chen@mediatek.com> 

> ---
>  arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> index 623eb3beabf2..4797537cb368 100644
> --- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> @@ -329,6 +329,7 @@ iommu0: iommu@10205000 {
>  		interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_LOW>;
>  		clocks = <&infracfg CLK_INFRA_M4U>;
>  		clock-names = "bclk";
> +		mediatek,infracfg = <&infracfg>;
>  		mediatek,larbs = <&larb0>, <&larb1>, <&larb2>,
>  				 <&larb3>, <&larb6>;
>  		#iommu-cells = <1>;
> @@ -346,6 +347,7 @@ iommu1: iommu@1020a000 {
>  		interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_LOW>;
>  		clocks = <&infracfg CLK_INFRA_M4U>;
>  		clock-names = "bclk";
> +		mediatek,infracfg = <&infracfg>;
>  		mediatek,larbs = <&larb4>, <&larb5>, <&larb7>;
>  		#iommu-cells = <1>;
>  	};
> -- 
> 2.35.1
> 
> 

Re: [PATCH v3 2/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg

[Yong Wu]

On Thu, 2022-06-09 at 12:07 +0200, AngeloGioacchino Del Regno wrote:
> This driver will get support for more SoCs and the list of infracfg
> compatibles is expected to grow: in order to prevent getting this
> situation out of control and see a long list of compatible strings,
> add support to retrieve a handle to infracfg's regmap through a
> new "mediatek,infracfg" phandle.
> 
> In order to keep retrocompatibility with older devicetrees, the old
> way is kept in place.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>

Retitle to: iommu/mediatek: Xxx, then

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

> ---
>  drivers/iommu/mtk_iommu.c | 38 ++++++++++++++++++++++++-------------
> -
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index bb9dd92c9898..90685946fcbe 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1140,22 +1140,32 @@ static int mtk_iommu_probe(struct
> platform_device *pdev)
>  	data->protect_base = ALIGN(virt_to_phys(protect),
> MTK_PROTECT_PA_ALIGN);
>  
>  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE)) {
> -		switch (data->plat_data->m4u_plat) {
> -		case M4U_MT2712:
> -			p = "mediatek,mt2712-infracfg";
> -			break;
> -		case M4U_MT8173:
> -			p = "mediatek,mt8173-infracfg";
> -			break;
> -		default:
> -			p = NULL;
> +		infracfg = syscon_regmap_lookup_by_phandle(dev-
> >of_node, "mediatek,infracfg");
> +		if (IS_ERR(infracfg)) {
> +			/*
> +			 * Legacy devicetrees will not specify a
> phandle to
> +			 * mediatek,infracfg: in that case, we use the
> older
> +			 * way to retrieve a syscon to infra.
> +			 *
> +			 * This is for retrocompatibility purposes
> only, hence
> +			 * no more compatibles shall be added to this.
> +			 */
> +			switch (data->plat_data->m4u_plat) {
> +			case M4U_MT2712:
> +				p = "mediatek,mt2712-infracfg";
> +				break;
> +			case M4U_MT8173:
> +				p = "mediatek,mt8173-infracfg";
> +				break;
> +			default:
> +				p = NULL;
> +			}
> +
> +			infracfg =
> syscon_regmap_lookup_by_compatible(p);
> +			if (IS_ERR(infracfg))
> +				return PTR_ERR(infracfg);
>  		}
>  
> -		infracfg = syscon_regmap_lookup_by_compatible(p);
> -
> -		if (IS_ERR(infracfg))
> -			return PTR_ERR(infracfg);
> -
>  		ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
>  		if (ret)
>  			return ret;

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[Yong Wu]

On Thu, 2022-06-09 at 12:08 +0200, AngeloGioacchino Del Regno wrote:
> On some SoCs (of which only MT8195 is supported at the time of
> writing),
> the "R" and "W" (I/O) enable bits for the IOMMUs are in the
> pericfg_ao
> register space and not in the IOMMU space: as it happened already
> with
> infracfg, it is expected that this list will grow.

Currently I don't see the list will grow. As commented before, In the
lastest SoC, The IOMMU enable bits for IOMMU will be in ATF, rather
than in this pericfg register region. In this case, Is this patch
unnecessary? or we could add this patch when there are 2 SoCs use this
setting at least?  what's your opinion?

> 
> Instead of specifying pericfg compatibles on a per-SoC basis,
> following
> what was done with infracfg, let's lookup the syscon by phandle
> instead.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
>  drivers/iommu/mtk_iommu.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 90685946fcbe..0ea0848581e9 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -138,6 +138,8 @@
>  /* PM and clock always on. e.g. infra iommu */
>  #define PM_CLK_AO			BIT(15)
>  #define IFA_IOMMU_PCIE_SUPPORT		BIT(16)
> +/* IOMMU I/O (r/w) is enabled using PERICFG_IOMMU_1 register */
> +#define HAS_PERI_IOMMU1_REG		BIT(17)
>  
>  #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
>  				((((pdata)->flags) & (mask)) == (_x))
> @@ -187,7 +189,6 @@ struct mtk_iommu_plat_data {
>  	u32			flags;
>  	u32			inv_sel_reg;
>  
> -	char			*pericfg_comp_str;
>  	struct list_head	*hw_list;
>  	unsigned int		iova_region_nr;
>  	const struct mtk_iommu_iova_region	*iova_region;
> @@ -1218,14 +1219,16 @@ static int mtk_iommu_probe(struct
> platform_device *pdev)
>  			goto out_runtime_disable;
>  		}
>  	} else if (MTK_IOMMU_IS_TYPE(data->plat_data,
> MTK_IOMMU_TYPE_INFRA) &&
> -		   data->plat_data->pericfg_comp_str) {
> -		infracfg = syscon_regmap_lookup_by_compatible(data-
> >plat_data->pericfg_comp_str);
> -		if (IS_ERR(infracfg)) {
> -			ret = PTR_ERR(infracfg);
> -			goto out_runtime_disable;
> +		   MTK_IOMMU_HAS_FLAG(data->plat_data,
> HAS_PERI_IOMMU1_REG)) {
> +		data->pericfg = syscon_regmap_lookup_by_phandle(dev-
> >of_node, "mediatek,pericfg");
> +		if (IS_ERR(data->pericfg)) {
> +			p = "mediatek,mt8195-pericfg_ao";
> +			data->pericfg =
> syscon_regmap_lookup_by_compatible(p);

Upstream doesn't have the mt8195 iommu node currently, thus We don't
need to recover for the previous dts case. right?

> +			if (IS_ERR(data->pericfg)) {
> +				ret = PTR_ERR(data->pericfg);
> +				goto out_runtime_disable;
> +			}
>  		}
> -
> -		data->pericfg = infracfg;
>  	}
>  
>  	platform_set_drvdata(pdev, data);
> @@ -1484,8 +1487,8 @@ static const struct mtk_iommu_plat_data
> mt8192_data = {
>  static const struct mtk_iommu_plat_data mt8195_data_infra = {
>  	.m4u_plat	  = M4U_MT8195,
>  	.flags            = WR_THROT_EN | DCM_DISABLE | STD_AXI_MODE |
> PM_CLK_AO |
> -			    MTK_IOMMU_TYPE_INFRA |
> IFA_IOMMU_PCIE_SUPPORT,
> -	.pericfg_comp_str = "mediatek,mt8195-pericfg_ao",
> +			    HAS_PERI_IOMMU1_REG | MTK_IOMMU_TYPE_INFRA
> |
> +			    IFA_IOMMU_PCIE_SUPPORT,
>  	.inv_sel_reg      = REG_MMU_INV_SEL_GEN2,
>  	.banks_num	  = 5,
>  	.banks_enable     = {true, false, false, false, true},

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[AngeloGioacchino Del Regno]

Il 13/06/22 07:32, Yong Wu ha scritto:
> On Thu, 2022-06-09 at 12:08 +0200, AngeloGioacchino Del Regno wrote:
>> On some SoCs (of which only MT8195 is supported at the time of
>> writing),
>> the "R" and "W" (I/O) enable bits for the IOMMUs are in the
>> pericfg_ao
>> register space and not in the IOMMU space: as it happened already
>> with
>> infracfg, it is expected that this list will grow.
> 
> Currently I don't see the list will grow. As commented before, In the
> lastest SoC, The IOMMU enable bits for IOMMU will be in ATF, rather
> than in this pericfg register region. In this case, Is this patch
> unnecessary? or we could add this patch when there are 2 SoCs use this
> setting at least?  what's your opinion?
> 

Perhaps I've misunderstood... besides, can you please check if there's any
other SoC (not just chromebooks, also smartphone SoCs) that need this logic?

Thanks,
Angelo

Re: [PATCH v3 2/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to infracfg

[Matthias Brugger]

On 09/06/2022 12:07, AngeloGioacchino Del Regno wrote:
> This driver will get support for more SoCs and the list of infracfg
> compatibles is expected to grow: in order to prevent getting this
> situation out of control and see a long list of compatible strings,
> add support to retrieve a handle to infracfg's regmap through a
> new "mediatek,infracfg" phandle.
> 
> In order to keep retrocompatibility with older devicetrees, the old
> way is kept in place.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/iommu/mtk_iommu.c | 38 ++++++++++++++++++++++++--------------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index bb9dd92c9898..90685946fcbe 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1140,22 +1140,32 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>   	data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
>   
>   	if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE)) {
> -		switch (data->plat_data->m4u_plat) {
> -		case M4U_MT2712:
> -			p = "mediatek,mt2712-infracfg";
> -			break;
> -		case M4U_MT8173:
> -			p = "mediatek,mt8173-infracfg";
> -			break;
> -		default:
> -			p = NULL;
> +		infracfg = syscon_regmap_lookup_by_phandle(dev->of_node, "mediatek,infracfg");
> +		if (IS_ERR(infracfg)) {
> +			/*
> +			 * Legacy devicetrees will not specify a phandle to
> +			 * mediatek,infracfg: in that case, we use the older
> +			 * way to retrieve a syscon to infra.
> +			 *
> +			 * This is for retrocompatibility purposes only, hence
> +			 * no more compatibles shall be added to this.
> +			 */
> +			switch (data->plat_data->m4u_plat) {
> +			case M4U_MT2712:
> +				p = "mediatek,mt2712-infracfg";
> +				break;
> +			case M4U_MT8173:
> +				p = "mediatek,mt8173-infracfg";
> +				break;
> +			default:
> +				p = NULL;
> +			}
> +
> +			infracfg = syscon_regmap_lookup_by_compatible(p);
> +			if (IS_ERR(infracfg))
> +				return PTR_ERR(infracfg);
>   		}
>   
> -		infracfg = syscon_regmap_lookup_by_compatible(p);
> -
> -		if (IS_ERR(infracfg))
> -			return PTR_ERR(infracfg);
> -
>   		ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
>   		if (ret)
>   			return ret;

Re: [PATCH v3 5/6] dt-bindings: iommu: mediatek: Add mediatek,pericfg phandle

[Matthias Brugger]

On 09/06/2022 12:08, AngeloGioacchino Del Regno wrote:
> Add property "mediatek,pericfg" to let the mtk_iommu driver retrieve
> a phandle to the infracfg syscon instead of performing a per-soc
> compatible lookup in the entire devicetree and set it as a required
> property for MT8195's infra IOMMU.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewd-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   .../devicetree/bindings/iommu/mediatek,iommu.yaml  | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
> index 4142a568b293..d5e3272a54e8 100644
> --- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml
> @@ -116,6 +116,10 @@ properties:
>         Refer to bindings/memory-controllers/mediatek,smi-larb.yaml. It must sort
>         according to the local arbiter index, like larb0, larb1, larb2...
>   
> +  mediatek,pericfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle to the mediatek pericfg syscon
> +
>     '#iommu-cells':
>       const: 1
>       description: |
> @@ -183,6 +187,16 @@ allOf:
>         required:
>           - mediatek,infracfg
>   
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mediatek,mt8195-iommu-infra
> +
> +    then:
> +      required:
> +        - mediatek,pericfg
> +
>     - if: # The IOMMUs don't have larbs.
>         not:
>           properties:

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[Matthias Brugger]

On 09/06/2022 12:08, AngeloGioacchino Del Regno wrote:
> On some SoCs (of which only MT8195 is supported at the time of writing),
> the "R" and "W" (I/O) enable bits for the IOMMUs are in the pericfg_ao
> register space and not in the IOMMU space: as it happened already with
> infracfg, it is expected that this list will grow.
> 
> Instead of specifying pericfg compatibles on a per-SoC basis, following
> what was done with infracfg, let's lookup the syscon by phandle instead.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/iommu/mtk_iommu.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 90685946fcbe..0ea0848581e9 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -138,6 +138,8 @@
>   /* PM and clock always on. e.g. infra iommu */
>   #define PM_CLK_AO			BIT(15)
>   #define IFA_IOMMU_PCIE_SUPPORT		BIT(16)
> +/* IOMMU I/O (r/w) is enabled using PERICFG_IOMMU_1 register */
> +#define HAS_PERI_IOMMU1_REG		BIT(17)

 From what I can see MTK_IOMMU_TYPE_INFRA is only set in MT8195 which uses 
pericfg. So we don't need a new flag here. For me the flag name 
MTK_IOMMU_TYPE_INFRA was confusing as it has nothing to do with the use of 
infracfg. I'll hijack this patch to provide some feedback on the actual code, 
please see below.

>   
>   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
>   				((((pdata)->flags) & (mask)) == (_x))
> @@ -187,7 +189,6 @@ struct mtk_iommu_plat_data {
>   	u32			flags;
>   	u32			inv_sel_reg;
>   
> -	char			*pericfg_comp_str;
>   	struct list_head	*hw_list;
>   	unsigned int		iova_region_nr;
>   	const struct mtk_iommu_iova_region	*iova_region;
> @@ -1218,14 +1219,16 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>   			goto out_runtime_disable;
>   		}
>   	} else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
> -		   data->plat_data->pericfg_comp_str) {

Check for pericfg_comp_str is not needed, we only have one platform that uses 
MTK_IOMMU_TYPE_INFRA.

> -		infracfg = syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);

We can do something like this to make the code clearer:
data->pericfg = 
syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
		if (IS_ERR(data->pericfg)) {

Using infracfg variable here is confusing as it has nothing to do with infracfg 
used with HAS_4GB_MODE flag.

Regards,
Matthias

> -		if (IS_ERR(infracfg)) {
> -			ret = PTR_ERR(infracfg);
> -			goto out_runtime_disable;
> +		   MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_PERI_IOMMU1_REG)) {
> +		data->pericfg = syscon_regmap_lookup_by_phandle(dev->of_node, "mediatek,pericfg");
> +		if (IS_ERR(data->pericfg)) {
> +			p = "mediatek,mt8195-pericfg_ao";
> +			data->pericfg = syscon_regmap_lookup_by_compatible(p);
> +			if (IS_ERR(data->pericfg)) {
> +				ret = PTR_ERR(data->pericfg);
> +				goto out_runtime_disable;
> +			}
>   		}
> -
> -		data->pericfg = infracfg;
>   	}
>   
>   	platform_set_drvdata(pdev, data);
> @@ -1484,8 +1487,8 @@ static const struct mtk_iommu_plat_data mt8192_data = {
>   static const struct mtk_iommu_plat_data mt8195_data_infra = {
>   	.m4u_plat	  = M4U_MT8195,
>   	.flags            = WR_THROT_EN | DCM_DISABLE | STD_AXI_MODE | PM_CLK_AO |
> -			    MTK_IOMMU_TYPE_INFRA | IFA_IOMMU_PCIE_SUPPORT,
> -	.pericfg_comp_str = "mediatek,mt8195-pericfg_ao",
> +			    HAS_PERI_IOMMU1_REG | MTK_IOMMU_TYPE_INFRA |
> +			    IFA_IOMMU_PCIE_SUPPORT,
>   	.inv_sel_reg      = REG_MMU_INV_SEL_GEN2,
>   	.banks_num	  = 5,
>   	.banks_enable     = {true, false, false, false, true},

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[AngeloGioacchino Del Regno]

Il 15/06/22 14:09, Matthias Brugger ha scritto:
> 
> 
> On 09/06/2022 12:08, AngeloGioacchino Del Regno wrote:
>> On some SoCs (of which only MT8195 is supported at the time of writing),
>> the "R" and "W" (I/O) enable bits for the IOMMUs are in the pericfg_ao
>> register space and not in the IOMMU space: as it happened already with
>> infracfg, it is expected that this list will grow.
>>
>> Instead of specifying pericfg compatibles on a per-SoC basis, following
>> what was done with infracfg, let's lookup the syscon by phandle instead.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/iommu/mtk_iommu.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>> index 90685946fcbe..0ea0848581e9 100644
>> --- a/drivers/iommu/mtk_iommu.c
>> +++ b/drivers/iommu/mtk_iommu.c
>> @@ -138,6 +138,8 @@
>>   /* PM and clock always on. e.g. infra iommu */
>>   #define PM_CLK_AO            BIT(15)
>>   #define IFA_IOMMU_PCIE_SUPPORT        BIT(16)
>> +/* IOMMU I/O (r/w) is enabled using PERICFG_IOMMU_1 register */
>> +#define HAS_PERI_IOMMU1_REG        BIT(17)
> 
>  From what I can see MTK_IOMMU_TYPE_INFRA is only set in MT8195 which uses pericfg. 
> So we don't need a new flag here. For me the flag name MTK_IOMMU_TYPE_INFRA was 
> confusing as it has nothing to do with the use of infracfg. I'll hijack this patch 
> to provide some feedback on the actual code, please see below.
> 
>>   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)    \
>>                   ((((pdata)->flags) & (mask)) == (_x))
>> @@ -187,7 +189,6 @@ struct mtk_iommu_plat_data {
>>       u32            flags;
>>       u32            inv_sel_reg;
>> -    char            *pericfg_comp_str;
>>       struct list_head    *hw_list;
>>       unsigned int        iova_region_nr;
>>       const struct mtk_iommu_iova_region    *iova_region;
>> @@ -1218,14 +1219,16 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>>               goto out_runtime_disable;
>>           }
>>       } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
>> -           data->plat_data->pericfg_comp_str) {
> 
> Check for pericfg_comp_str is not needed, we only have one platform that uses 
> MTK_IOMMU_TYPE_INFRA.
> 

Fair enough. I agree.

>> -        infracfg = 
>> syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
> 
> We can do something like this to make the code clearer:
> data->pericfg = syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
>          if (IS_ERR(data->pericfg)) {
> 
> Using infracfg variable here is confusing as it has nothing to do with infracfg 
> used with HAS_4GB_MODE flag.

Yes Matthias, using the infracfg variable is confusing - that's why I changed that
already....

> 
> Regards,
> Matthias
> 
>> -        if (IS_ERR(infracfg)) {
>> -            ret = PTR_ERR(infracfg);
>> -            goto out_runtime_disable;
>> +           MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_PERI_IOMMU1_REG)) {



>> +        data->pericfg = syscon_regmap_lookup_by_phandle(dev->of_node, 
>> "mediatek,pericfg");

....Here, where I'm assigning directly to data->pericfg :-P

By the way, since it was only about one platform, my intention was to remove the
pericfg_comp_str from struct iommu_plat_data (as you can see), but then, with the
current code, I had to assign .....


>> +        if (IS_ERR(data->pericfg)) {
>> +            p = "mediatek,mt8195-pericfg_ao";

...the string to 'p', because otherwise it would go over 100 columns.

In any case, I just checked and, apparently, MT8195 is really the one and only SoC
that needs this pericfg register to be managed by Linux... even the latest and
greatest smartphone chip (Dimensity 9000, MT6983) doesn't need this (at least,
from what I can read on a downstream kernel).

On an afterthought, perhaps the best idea is to just leave this as it is and, as
you proposed, avoid using that confusing infracfg variable, without adding the
pericfg handle at all.

After all, it's just one single SoC.

I'll send a new version soon!

Cheers,
Angelo

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[Yong Wu]

On Mon, 2022-06-13 at 10:13 +0200, AngeloGioacchino Del Regno wrote:
> Il 13/06/22 07:32, Yong Wu ha scritto:
> > On Thu, 2022-06-09 at 12:08 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > On some SoCs (of which only MT8195 is supported at the time of
> > > writing),
> > > the "R" and "W" (I/O) enable bits for the IOMMUs are in the
> > > pericfg_ao
> > > register space and not in the IOMMU space: as it happened already
> > > with
> > > infracfg, it is expected that this list will grow.
> > 
> > Currently I don't see the list will grow. As commented before, In
> > the
> > lastest SoC, The IOMMU enable bits for IOMMU will be in ATF, rather
> > than in this pericfg register region. In this case, Is this patch
> > unnecessary? or we could add this patch when there are 2 SoCs use
> > this
> > setting at least?  what's your opinion?
> > 
> 
> Perhaps I've misunderstood... besides, can you please check if
> there's any
> other SoC (not just chromebooks, also smartphone SoCs) that need this
> logic?

As far as I know, SmartPhone SoCs don't enable the infra iommu until
now. they don't have this logic. I don't object this patch, I think we
could add it when at least 2 SoCs need this.

Thanks very much for help improving here.

> 
> Thanks,
> Angelo
> 
> 

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[AngeloGioacchino Del Regno]

Il 16/06/22 08:30, Yong Wu ha scritto:
> On Mon, 2022-06-13 at 10:13 +0200, AngeloGioacchino Del Regno wrote:
>> Il 13/06/22 07:32, Yong Wu ha scritto:
>>> On Thu, 2022-06-09 at 12:08 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>> On some SoCs (of which only MT8195 is supported at the time of
>>>> writing),
>>>> the "R" and "W" (I/O) enable bits for the IOMMUs are in the
>>>> pericfg_ao
>>>> register space and not in the IOMMU space: as it happened already
>>>> with
>>>> infracfg, it is expected that this list will grow.
>>>
>>> Currently I don't see the list will grow. As commented before, In
>>> the
>>> lastest SoC, The IOMMU enable bits for IOMMU will be in ATF, rather
>>> than in this pericfg register region. In this case, Is this patch
>>> unnecessary? or we could add this patch when there are 2 SoCs use
>>> this
>>> setting at least?  what's your opinion?
>>>
>>
>> Perhaps I've misunderstood... besides, can you please check if
>> there's any
>> other SoC (not just chromebooks, also smartphone SoCs) that need this
>> logic?
> 
> As far as I know, SmartPhone SoCs don't enable the infra iommu until
> now. they don't have this logic. I don't object this patch, I think we
> could add it when at least 2 SoCs need this.
> 
> Thanks very much for help improving here.
> 

Many thanks for checking that! Now that everything is clear, I can safely
go on with pushing a v4 of this series.

Thanks again!
Angelo

Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[Matthias Brugger]

On 15/06/2022 14:28, AngeloGioacchino Del Regno wrote:
> Il 15/06/22 14:09, Matthias Brugger ha scritto:
>>
>>
>> On 09/06/2022 12:08, AngeloGioacchino Del Regno wrote:
>>> On some SoCs (of which only MT8195 is supported at the time of writing),
>>> the "R" and "W" (I/O) enable bits for the IOMMUs are in the pericfg_ao
>>> register space and not in the IOMMU space: as it happened already with
>>> infracfg, it is expected that this list will grow.
>>>
>>> Instead of specifying pericfg compatibles on a per-SoC basis, following
>>> what was done with infracfg, let's lookup the syscon by phandle instead.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   drivers/iommu/mtk_iommu.c | 23 +++++++++++++----------
>>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 90685946fcbe..0ea0848581e9 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -138,6 +138,8 @@
>>>   /* PM and clock always on. e.g. infra iommu */
>>>   #define PM_CLK_AO            BIT(15)
>>>   #define IFA_IOMMU_PCIE_SUPPORT        BIT(16)
>>> +/* IOMMU I/O (r/w) is enabled using PERICFG_IOMMU_1 register */
>>> +#define HAS_PERI_IOMMU1_REG        BIT(17)
>>
>>  From what I can see MTK_IOMMU_TYPE_INFRA is only set in MT8195 which uses 
>> pericfg. So we don't need a new flag here. For me the flag name 
>> MTK_IOMMU_TYPE_INFRA was confusing as it has nothing to do with the use of 
>> infracfg. I'll hijack this patch to provide some feedback on the actual code, 
>> please see below.
>>
>>>   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)    \
>>>                   ((((pdata)->flags) & (mask)) == (_x))
>>> @@ -187,7 +189,6 @@ struct mtk_iommu_plat_data {
>>>       u32            flags;
>>>       u32            inv_sel_reg;
>>> -    char            *pericfg_comp_str;
>>>       struct list_head    *hw_list;
>>>       unsigned int        iova_region_nr;
>>>       const struct mtk_iommu_iova_region    *iova_region;
>>> @@ -1218,14 +1219,16 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>>>               goto out_runtime_disable;
>>>           }
>>>       } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
>>> -           data->plat_data->pericfg_comp_str) {
>>
>> Check for pericfg_comp_str is not needed, we only have one platform that uses 
>> MTK_IOMMU_TYPE_INFRA.
>>
> 
> Fair enough. I agree.
> 
>>> -        infracfg = 
>>> syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
>>
>> We can do something like this to make the code clearer:
>> data->pericfg = 
>> syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
>>          if (IS_ERR(data->pericfg)) {
>>
>> Using infracfg variable here is confusing as it has nothing to do with 
>> infracfg used with HAS_4GB_MODE flag.
> 
> Yes Matthias, using the infracfg variable is confusing - that's why I changed that
> already....
> 
>>
>> Regards,
>> Matthias
>>
>>> -        if (IS_ERR(infracfg)) {
>>> -            ret = PTR_ERR(infracfg);
>>> -            goto out_runtime_disable;
>>> +           MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_PERI_IOMMU1_REG)) {
> 
> 
> 
>>> +        data->pericfg = syscon_regmap_lookup_by_phandle(dev->of_node, 
>>> "mediatek,pericfg");
> 
> ....Here, where I'm assigning directly to data->pericfg :-P
> 

Uuuups, sorry, did look too much on the existing code and not enough on your patch.

> By the way, since it was only about one platform, my intention was to remove the
> pericfg_comp_str from struct iommu_plat_data (as you can see), but then, with the
> current code, I had to assign .....
> 
> 
>>> +        if (IS_ERR(data->pericfg)) {
>>> +            p = "mediatek,mt8195-pericfg_ao";
> 
> ...the string to 'p', because otherwise it would go over 100 columns.
> 
> In any case, I just checked and, apparently, MT8195 is really the one and only SoC
> that needs this pericfg register to be managed by Linux... even the latest and
> greatest smartphone chip (Dimensity 9000, MT6983) doesn't need this (at least,
> from what I can read on a downstream kernel).
> 
> On an afterthought, perhaps the best idea is to just leave this as it is and, as
> you proposed, avoid using that confusing infracfg variable, without adding the
> pericfg handle at all.

Either this or get also rid of the pericfg_comp_str in the _plat_data. I'm 
unemotional about this :)

Regards,
Matthias

> 
> After all, it's just one single SoC.
> 
> I'll send a new version soon!
> 
> Cheers,
> Angelo
>