Message-ID: <3fb55030-52d5-4e61-896f-c15a4f45ae8b@arm.com>
Date: Wed, 5 Nov 2025 18:20:38 +0000
From: Robin Murphy <robin.murphy@....com>
To: Charan Teja Kalla <charan.kalla@....qualcomm.com>, will@...nel.org,
 joro@...tes.org, robh@...nel.org, dmitry.baryshkov@....qualcomm.com,
 konrad.dybcio@....qualcomm.com, bjorn.andersson@....qualcomm.com,
 bod@...nel.org, conor+dt@...nel.org, krzk+dt@...nel.org,
 saravanak@...gle.com, prakash.gupta@....qualcomm.com,
 vikash.garodia@....qualcomm.com
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH 6/6] of: use correct iommu-map parsing logic from of_iommu
 layer

On 2025-11-04 8:51 am, Charan Teja Kalla wrote:
> Use of_map_id_or_funcid() that takes #iommu-cells into account, while
> keeping the compatibility with the existing DTs, which assume single
> argument. A callback function is used to get the of_phandle arguments,
> that contains iommu controller and its cells, and is called into the
> vendor specific driver through of_iommu_xlate().
> 
> When 'args_count' passed in of_phandle_args is '1', it is expected that
> of_map_id_or_funcid() fills the translated id into the of_phandle
> arguments which then called into of_iommu_xlate().
> 
> When 'args_count' > 1, as it is complex to establish a defined relation
> between the input id and output, of_iommu layer handles through the
> below relation:
> For platform devices, 'rid' defined in the iommu-map tuple indicates a
> function, through a bit position, which is compared against passed input
> 'id' that represents a bitmap of functions represented by the device.
> 
> For others, 'rid' is compared against the input 'id' as an integer value.

Sorry, I struggle to comprehend what this is trying to achieve... 
of_iommu doesn't have a clue what phandle arguments represent, only the 
relevant IOMMU driver does. How are we asking the IOMMU driver to map an 
input ID represented by an offset into a range, to an appropriate output 
specifier, without passing all that information to a suitable driver 
operation? Why would that have any dependency on bus types? And where 
has this notion of Linux platform devices having some sort of inherently 
standard bitmap suddenly sprung from?

Thanks,
Robin.

> When matched, of_iommu_xlate() is used to call into vendor specific
> iommu driver.
> 
> Signed-off-by: Charan Teja Kalla <charan.kalla@....qualcomm.com>
> ---
>   drivers/iommu/of_iommu.c | 59 +++++++++++++++++++++++++++++++++-------
>   drivers/of/base.c        |  4 +--
>   include/linux/of.h       | 15 ++++++++++
>   3 files changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 6b989a62def2..9575e37ad844 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -16,6 +16,7 @@
>   #include <linux/pci.h>
>   #include <linux/slab.h>
>   #include <linux/fsl/mc.h>
> +#include <linux/platform_device.h>
>   
>   #include "iommu-priv.h"
>   
> @@ -41,22 +42,60 @@ static int of_iommu_xlate(struct device *dev,
>   	return ret;
>   }
>   
> +/**
> + * of_iommu_map_xlate - Translate IOMMU mapping and release node reference.
> + *
> + * @id: bitmap for function id parsing(platform devices) or base id.
> + * @id_base: checked against @id
> + * @arg: Device to configure.
> + * @iommu_spec: IOMMU specifier from device tree.
> + *
> + * Wrapper function that calls of_iommu_xlate() to configure the IOMMU
> + * mapping for a device, then releases the device tree node reference.
> + * This is used as a callback function for of_map_id_or_funcid().
> + *
> + * When the cell_count > 1, the relation between input id(@id) and the
> + * output specifier is complex to define, like linear case when cell_count = 1,
> + * Handle such cases where linear relation can't be established between
> + * the @id and the output with the below relation:
> + * a) For platform devices, @id_base represents the function id, which is
> + * compared against the input @id, all maintained in bitmap relation.
> + * b) For other devices, it performs exact ID matching.
> + *
> + * Return: 0 on success, -EAGAIN if ID doesn't match
> + * and standard negative error code on failure.
> + */
> +static int of_iommu_map_xlate(u32 id, u32 id_base,
> +			void *arg, struct of_phandle_args *iommu_spec)
> +{
> +	struct device *dev = arg;
> +	int ret;
> +
> +	if (iommu_spec->args_count > 1) {
> +		if (dev_is_platform(dev)) {     /* case a. */
> +			if (!(BIT(id_base - 1) & id))
> +				return -EAGAIN;
> +		} else if (id_base != id) {     /* case b. */
> +			return -EAGAIN;
> +		}
> +
> +	}
> +
> +	ret = of_iommu_xlate(dev, iommu_spec);
> +	of_node_put(iommu_spec->np);
> +	return ret;
> +}
> +
>   static int of_iommu_configure_dev_id(struct device_node *master_np,
>   				     struct device *dev,
>   				     const u32 *id)
>   {
>   	struct of_phandle_args iommu_spec = { .args_count = 1 };
> -	int err;
>   
> -	err = of_map_id(master_np, *id, "iommu-map",
> -			 "iommu-map-mask", &iommu_spec.np,
> -			 iommu_spec.args);
> -	if (err)
> -		return err;
> -
> -	err = of_iommu_xlate(dev, &iommu_spec);
> -	of_node_put(iommu_spec.np);
> -	return err;
> +	return of_map_id_or_funcid(master_np, *id, "iommu-map",
> +			"iommu-map-mask", &iommu_spec.np,
> +			 iommu_spec.args, dev, &iommu_spec,
> +			 of_iommu_map_xlate);
>   }
>   
>   static int of_iommu_configure_dev(struct device_node *master_np,
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index cd221c003dd5..b5554c0b0bc5 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2181,7 +2181,7 @@ static int of_map_id_cell_count(const __be32 *map, const char *map_name,
>    * @id: input id that is given for translation.
>    * @arg: argument passed to the @fn.
>    * @pargs: filled by the contents of @map and pass to the @fn.
> - * @fn: Callback function that operated on @dev and @fn.
> + * @fn: Callback function that operates on @arg.
>    *
>    * The function populates the phandle args structure with the node and
>    * specifier cells, then invokes the callback function. The callback is
> @@ -2233,7 +2233,7 @@ static int of_map_id_untranslated(const __be32 *map, u32 cell_count, u32 id,
>    *
>    * Return: 0 on success or a standard error code on failure.
>    */
> -static int of_map_id_or_funcid(const struct device_node *np, u32 id,
> +int of_map_id_or_funcid(const struct device_node *np, u32 id,
>   		const char *map_name, const char *map_mask_name,
>   		struct device_node **target, u32 *id_out,
>   		void *arg, struct of_phandle_args *pargs,
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f8d976205ff4..7935ff5ac09e 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -410,6 +410,12 @@ extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
>   extern int of_alias_get_id(const struct device_node *np, const char *stem);
>   extern int of_alias_get_highest_id(const char *stem);
>   
> +extern int of_map_id_or_funcid(const struct device_node *np, u32 id,
> +	       const char *map_name, const char *map_mask_name,
> +	       struct device_node **target, u32 *id_out,
> +	       void *arg, struct of_phandle_args *pargs,
> +	       of_map_id_cb fn);
> +
>   bool of_machine_compatible_match(const char *const *compats);
>   
>   /**
> @@ -909,6 +915,15 @@ static inline int of_map_id(const struct device_node *np, u32 id,
>   	return -EINVAL;
>   }
>   
> +static inline int of_map_id_or_funcid(const struct device_node *np, u32 id,
> +	       const char *map_name, const char *map_mask_name,
> +	       struct device_node **target, u32 *id_out,
> +	       void *arg, struct of_phandle_args *pargs,
> +	       of_map_id_cb fn)
> +{
> +	return -EINVAL;
> +}
> +
>   static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np)
>   {
>   	return PHYS_ADDR_MAX;

Powered by blists - more mailing lists