Re: [PATCH v2 3/3] of: Respect #{iommu,msi}-cells in maps

[Rob Herring <robh@kernel.org>]

On Wed, Dec 10, 2025 at 03:57:56PM +0530, Vijayanand Jitta wrote:
> On 12/10/2025 1:47 AM, Rob Herring wrote:
> > On Thu, Dec 04, 2025 at 03:25:30PM +0530, Vijayanand Jitta wrote:
> >> From: Robin Murphy <robin.murphy@arm.com>
> >>
> >> So far our parsing of {iommu,msi}-map properites has always blindly
> >> asusmed that the output specifiers will always have exactly 1 cell.
> >> This typically does happen to be the case, but is not actually enforced
> >> (and the PCI msi-map binding even explicitly states support for 0 or 1
> >> cells) - as a result we've now ended up with dodgy DTs out in the field
> >> which depend on this behaviour to map a 1-cell specifier for a 2-cell
> >> provider, despite that being bogus per the bindings themselves.
> >>
> >> Since there is some potential use in being able to map at least single
> >> input IDs to multi-cell output specifiers (and properly support 0-cell
> >> outputs as well), add support for properly parsing and using the target
> >> nodes' #cells values, albeit with the unfortunate complication of still
> >> having to work around expectations of the old behaviour too.
> >>
> >> Since there are multi-cell output specifiers, the callers of of_map_id()
> >> may need to get the exact cell output value for further processing.
> >> Added support for that part --charan
> >>
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> Signed-off-by: Vijayanand Jitta <vijayanand.jitta@oss.qualcomm.com>
> >> ---
> >>  drivers/iommu/of_iommu.c |   3 +-
> >>  drivers/of/base.c        | 107 ++++++++++++++++++++++++++++++---------
> >>  include/linux/of.h       |  17 ++++---
> >>  3 files changed, 94 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> >> index eac62bc441c5..48759cf1d900 100644
> >> --- a/drivers/iommu/of_iommu.c
> >> +++ b/drivers/iommu/of_iommu.c
> >> @@ -45,10 +45,11 @@ 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 };
> >> +	struct of_phandle_args iommu_spec = {};
> >>  	struct of_map_id_arg arg = {
> >>  		.target = &iommu_spec.np,
> >>  		.id_out = iommu_spec.args,
> >> +		.map_cells = &iommu_spec.args_count,
> >>  	};
> >>  	int err;
> >>  
> >> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> index b8f78a9e6a09..68a7d6ddba66 100644
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -2045,11 +2045,30 @@ int of_find_last_cache_level(unsigned int cpu)
> >>  	return cache_level;
> >>  }
> >>  
> >> +/*
> >> + * Some DTs have an iommu-map targeting a 2-cell IOMMU node while
> >> + * specifying only 1 cell. Fortunately they all consist of length == 1
> >> + * entries with the same target, so check for that pattern.
> > 
> > Can you show what a bad entry looks like here.
> > 
> 
> Sure, will add an example in the comments. Basically it would look like below.
> 
> for iommu with iommu-cells  = <2>;
> 
> Device having below iommu-map property.
> 
> iommu-map = <0x0000  &smmu  0x0000  0x1>,
>             <0x0100  &smmu  0x0100  0x1>;
> 
> >> + */
> >> +static bool of_check_bad_map(const __be32 *map, int len)
> >> +{
> >> +	__be32 phandle = map[1];
> >> +
> >> +	if (len % 4)
> >> +		return false;
> >> +	for (int i = 0; i < len; i += 4) {
> >> +		if (map[i + 1] != phandle || map[i + 3] != cpu_to_be32(1))
> > 
> > Why does the IOMMU arg cell have to be 1? The description said 'same 
> > target', but it is just all have an IOMMU cell value of 1?
> > 
> 
> Here, the check is for length argument to be 1. This is to maintain backward
> compatibility as mentioned above, as all such bad entries right now have
> length as 1.

You say length and I think arg/cell length, not that the cell value 
contains a length. That's because generally cell args are provider 
defined and specific. So just say the 2nd cell has a value of 1 and 
leave out that's a length.

[...]

> >> @@ -1455,7 +1456,7 @@ static inline int of_map_msi_id(const struct device_node *np, u32 id,
> >>  		.id_out = id_out,
> >>  	};
> >>  
> >> -	return of_map_id(np, id, "msi-map", "msi-map-mask", &arg);
> >> +	return of_map_id(np, id, "msi-map", "#msi-cells", "msi-map-mask", &arg);
> > 
> > There are cases of no #msi-cells and we default to 0 cells in that case. 
> > Do you maintain that?
> > 
> > Rob
> 
> Thanks for pointing this, I see this case of no #msi-cells is not covered. Will
> add it in next revision.  Also, IIUC shouldn't we set default cells to '1' to
> maintain backward compatibility of of_map_id in this case ? No ?

The only default is 0. Perhaps msi-map is never used if there are 0 
cells? IDK, you tell me.

Rob