[PATCH v3 0/3] iommu: omap: Simplify few things

[PATCH v3 0/3] iommu: omap: Simplify few things

[Krzysztof Kozlowski]

Changes in v3:
- Rebase
- Link to v2: https://lore.kernel.org/r/20250501-syscon-phandle-args-iommu-v2-0-5a3cab296972@linaro.org

Changes in v2:
- Combine patches #1 and #2, add dev_err_probe() and drop the comment
  (resolving Robin's comment)
- New patch: iommu: mtk_iommu_v1: Simplify by dropping local 'mtk_mapping' variable
- Rb tag
- Link to resend of v1: https://lore.kernel.org/r/20250212-syscon-phandle-args-iommu-v1-0-c3fab486b426@linaro.org
- Link to v1: https://lore.kernel.org/r/20250111-syscon-phandle-args-iommu-v1-0-3767dee585a6@linaro.org

Few code simplifications without functional impact.  Not tested on
hardware.

Best regards,
Krzysztof

---
Krzysztof Kozlowski (3):
      iommu: omap: Drop redundant check if ti,syscon-mmuconfig exists
      iommu: omap: Use syscon_regmap_lookup_by_phandle_args
      iommu: mtk_iommu_v1: Simplify by dropping local 'mtk_mapping' variable

 drivers/iommu/mtk_iommu_v1.c |  4 +---
 drivers/iommu/omap-iommu.c   | 24 +++++-------------------
 2 files changed, 6 insertions(+), 22 deletions(-)
---
base-commit: 2ae2aaafb21454f4781c30734959cf223ab486ef
change-id: 20250111-syscon-phandle-args-iommu-2ea162b223a4

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

[PATCH v3 1/3] iommu: omap: Drop redundant check if ti,syscon-mmuconfig exists

[Krzysztof Kozlowski]

The syscon_regmap_lookup_by_phandle() will fail if property does not
exist, so doing of_property_read_bool() earlier is redundant.  Drop that
check and move error message to syscon_regmap_lookup_by_phandle() error
case while converting it to dev_err_probe().

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iommu/omap-iommu.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 3c62337f43c67720a15b67e8b610da7886f6f39c..4448c0a512137c79195112eea25d762266c77bc3 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1123,23 +1123,15 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
 					      struct omap_iommu *obj)
 {
 	struct device_node *np = pdev->dev.of_node;
-	int ret;
 
 	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
 		return 0;
 
-	if (!of_property_read_bool(np, "ti,syscon-mmuconfig")) {
-		dev_err(&pdev->dev, "ti,syscon-mmuconfig property is missing\n");
-		return -EINVAL;
-	}
-
 	obj->syscfg =
 		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
-	if (IS_ERR(obj->syscfg)) {
-		/* can fail with -EPROBE_DEFER */
-		ret = PTR_ERR(obj->syscfg);
-		return ret;
-	}
+	if (IS_ERR(obj->syscfg))
+		return dev_err_probe(&pdev->dev, PTR_ERR(obj->syscfg),
+				     "ti,syscon-mmuconfig property is missing\n");
 
 	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
 				       &obj->id)) {

-- 
2.48.1

Re: [PATCH v3 1/3] iommu: omap: Drop redundant check if ti,syscon-mmuconfig exists

[Robin Murphy]

On 2025-06-24 1:22 pm, Krzysztof Kozlowski wrote:
> The syscon_regmap_lookup_by_phandle() will fail if property does not
> exist, so doing of_property_read_bool() earlier is redundant.  Drop that
> check and move error message to syscon_regmap_lookup_by_phandle() error
> case while converting it to dev_err_probe().

Sure, may as well.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   drivers/iommu/omap-iommu.c | 14 +++-----------
>   1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 3c62337f43c67720a15b67e8b610da7886f6f39c..4448c0a512137c79195112eea25d762266c77bc3 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1123,23 +1123,15 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
>   					      struct omap_iommu *obj)
>   {
>   	struct device_node *np = pdev->dev.of_node;
> -	int ret;
>   
>   	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
>   		return 0;
>   
> -	if (!of_property_read_bool(np, "ti,syscon-mmuconfig")) {
> -		dev_err(&pdev->dev, "ti,syscon-mmuconfig property is missing\n");
> -		return -EINVAL;
> -	}
> -
>   	obj->syscfg =
>   		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
> -	if (IS_ERR(obj->syscfg)) {
> -		/* can fail with -EPROBE_DEFER */
> -		ret = PTR_ERR(obj->syscfg);
> -		return ret;
> -	}
> +	if (IS_ERR(obj->syscfg))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(obj->syscfg),
> +				     "ti,syscon-mmuconfig property is missing\n");
>   
>   	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
>   				       &obj->id)) {
> 

[PATCH v3 2/3] iommu: omap: Use syscon_regmap_lookup_by_phandle_args

[Krzysztof Kozlowski]

Use syscon_regmap_lookup_by_phandle_args() which is a wrapper over
syscon_regmap_lookup_by_phandle() combined with getting the syscon
argument.  Except simpler code this annotates within one line that given
phandle has arguments, so grepping for code would be easier.

There is also no real benefit in printing errors on missing syscon
argument, because this is done just too late: runtime check on
static/build-time data.  Dtschema and Devicetree bindings offer the
static/build-time check for this already.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iommu/omap-iommu.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 4448c0a512137c79195112eea25d762266c77bc3..30fdbabbc9c652990e6cd31d3c0a1a06633df9e6 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1127,18 +1127,12 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
 	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
 		return 0;
 
-	obj->syscfg =
-		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
+	obj->syscfg = syscon_regmap_lookup_by_phandle_args(np, "ti,syscon-mmuconfig",
+							   1, &obj->id);
 	if (IS_ERR(obj->syscfg))
 		return dev_err_probe(&pdev->dev, PTR_ERR(obj->syscfg),
 				     "ti,syscon-mmuconfig property is missing\n");
 
-	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
-				       &obj->id)) {
-		dev_err(&pdev->dev, "couldn't get the IOMMU instance id within subsystem\n");
-		return -EINVAL;
-	}
-
 	if (obj->id != 0 && obj->id != 1) {
 		dev_err(&pdev->dev, "invalid IOMMU instance id\n");
 		return -EINVAL;

-- 
2.48.1

[PATCH v3 3/3] iommu: mtk_iommu_v1: Simplify by dropping local 'mtk_mapping' variable

[Krzysztof Kozlowski]

Storing 'data->mapping' in local variable in
mtk_iommu_v1_probe_finalize() does not make the code easier to read.
Use the 'data->mapping' directly.

ARM64 W=1 builds also complain with:

  mtk_iommu_v1.c:512:28: error: variable 'mtk_mapping' set but not used [-Werror,-Wunused-but-set-variable]

but this is not being fixed here and 'data' still won't be used in such
compile test.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Old warning is changed now to:

  error: variable 'data' set but not used [-Werror,-Wunused-but-set-variable]
---
 drivers/iommu/mtk_iommu_v1.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 66824982e05fbfdda224276ad41b90f9d5f9ca4e..bbe3e9d901c69ac6405d9549a4481fc80f1adb80 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -509,14 +509,12 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev)
 
 static void mtk_iommu_v1_probe_finalize(struct device *dev)
 {
-	struct dma_iommu_mapping *mtk_mapping;
 	struct mtk_iommu_v1_data *data;
 	int err;
 
 	data        = dev_iommu_priv_get(dev);
-	mtk_mapping = data->mapping;
 
-	err = arm_iommu_attach_device(dev, mtk_mapping);
+	err = arm_iommu_attach_device(dev, data->mapping);
 	if (err)
 		dev_err(dev, "Can't create IOMMU mapping - DMA-OPS will not work\n");
 }

-- 
2.48.1

Re: [PATCH v3 3/3] iommu: mtk_iommu_v1: Simplify by dropping local 'mtk_mapping' variable

[Robin Murphy]

On 2025-06-24 1:22 pm, Krzysztof Kozlowski wrote:
> Storing 'data->mapping' in local variable in
> mtk_iommu_v1_probe_finalize() does not make the code easier to read.
> Use the 'data->mapping' directly.
> 
> ARM64 W=1 builds also complain with:
> 
>    mtk_iommu_v1.c:512:28: error: variable 'mtk_mapping' set but not used [-Werror,-Wunused-but-set-variable]
> 
> but this is not being fixed here and 'data' still won't be used in such
> compile test.

Sorry, I guess that's on me for doing the COMPILE_TEST stubs the lazy 
way... TBH I'm still not sure this driver hasn't completely bitrotted, 
but if we're touching it at all we should probably actually fix the 
warning, e.g.:

-#define arm_iommu_attach_device(...)   -ENODEV
+#define arm_iommu_attach_device(d, m)  ((void)m, -ENODEV)

or perhaps make it a less lazy static inline.

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Old warning is changed now to:
> 
>    error: variable 'data' set but not used [-Werror,-Wunused-but-set-variable]
> ---
>   drivers/iommu/mtk_iommu_v1.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
> index 66824982e05fbfdda224276ad41b90f9d5f9ca4e..bbe3e9d901c69ac6405d9549a4481fc80f1adb80 100644
> --- a/drivers/iommu/mtk_iommu_v1.c
> +++ b/drivers/iommu/mtk_iommu_v1.c
> @@ -509,14 +509,12 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev)
>   
>   static void mtk_iommu_v1_probe_finalize(struct device *dev)
>   {
> -	struct dma_iommu_mapping *mtk_mapping;
>   	struct mtk_iommu_v1_data *data;
>   	int err;
>   
>   	data        = dev_iommu_priv_get(dev);

And for the sake of a readability cleanup you may as well fold this into 
the declaration line as well. And then maybe also whack a __maybe_unused 
on it, for even less effort than the first silly thing I thought of 
above... :)

Thanks,
Robin.

> -	mtk_mapping = data->mapping;
>   
> -	err = arm_iommu_attach_device(dev, mtk_mapping);
> +	err = arm_iommu_attach_device(dev, data->mapping);
>   	if (err)
>   		dev_err(dev, "Can't create IOMMU mapping - DMA-OPS will not work\n");
>   }
> 

Re: [PATCH v3 0/3] iommu: omap: Simplify few things

[Joerg Roedel]

On Tue, Jun 24, 2025 at 02:22:29PM +0200, Krzysztof Kozlowski wrote:
> Krzysztof Kozlowski (3):
>       iommu: omap: Drop redundant check if ti,syscon-mmuconfig exists
>       iommu: omap: Use syscon_regmap_lookup_by_phandle_args
>       iommu: mtk_iommu_v1: Simplify by dropping local 'mtk_mapping' variable

Applied patches 1 and 2 to the ti/omap branch, thanks.