[RFC PATCH 2/2] iommu/arm-smmu: Add support for the fsl-mc bus

[Nipun Gupta]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy at arm.com]
> Sent: Thursday, June 30, 2016 16:53
> To: Nipun Gupta <nipun.gupta at nxp.com>; will.deacon at arm.com; Stuart Yoder
> <stuart.yoder at nxp.com>; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org
> Cc: Bharat Bhushan <bharat.bhushan at nxp.com>
> Subject: Re: [RFC PATCH 2/2] iommu/arm-smmu: Add support for the fsl-mc bus
> 
> On 29/06/16 18:14, Nipun Gupta wrote:
> > Implement bus specific support for the fsl-mc bus including
> > registering the arm_smmu_ops and bus specific smmu device init.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta at nxp.com>
> > Signed-off-by: Bharat Bhushan <bharat.bhushan at nxp.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 43
> > +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> > ab365ec..28d5dc8 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -47,6 +47,9 @@
> >
> >  #include <linux/amba/bus.h>
> >
> > +/* Include path will be changed once FSL-MC support is out from
> > +staging */
> 
> How about we do that first?

Agree. We are working on that part :)

> 
> > +#include <../drivers/staging/fsl-mc/include/fsl_mc_smmu.h>
> > +
> >  #include "io-pgtable.h"
> >
> >  /* Maximum number of stream IDs assigned to a single device */ @@
> > -447,6 +450,11 @@ static struct device_node *dev_get_dev_node(struct
> device *dev)
> >  		return bus->bridge->parent->of_node;
> >  	}
> >
> > +	if (dev_is_fsl_mc(dev)) {
> 
> Where is dev_is_fsl_mc() defined? I don't see it anywhere in mainline, here, or in
> -next.

My Bad. We have provided a patch on the devel at driverdev.osuosl.org ; linux-kernel at vger.kernel.org to have the macro dev_is_fsl_mc defined. Please see the attached patch.

> 
> > +		while (dev_is_fsl_mc(dev))
> > +			dev = dev->parent;
> > +	}
> > +
> >  	return dev->of_node;
> >  }
> >
> > @@ -1443,6 +1451,32 @@ static int arm_smmu_init_platform_device(struct
> device *dev,
> >  	return 0;
> >  }
> >
> > +static void __arm_smmu_release_fslmc_iommudata(void *data) {
> > +	kfree(data);
> > +}
> 
> There's already a suitable callback for this; nobody needs an exact duplicate of
> it.

I see '__arm_smmu_release_pci_iommudata' similarly defined for PCI bus.
We will probably need it renaming to '__arm_smmu_release_iommudata' to have a better readable code. Seems okay?

> 
> > +static int arm_smmu_init_fslmc_device(struct device *dev,
> > +				      struct iommu_group *group)
> > +{
> > +	struct arm_smmu_master_cfg *cfg;
> > +
> > +	cfg = iommu_group_get_iommudata(group);
> > +	if (!cfg) {
> > +		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> > +		if (!cfg)
> > +			return -ENOMEM;
> > +
> > +		cfg->streamids[0] = fslmc_dev_streamid(dev);
> > +		cfg->num_streamids = 1;
> > +
> > +		iommu_group_set_iommudata(group, cfg,
> > +
> __arm_smmu_release_fslmc_iommudata);
> > +	}
> 
> So the group gets the ID of the container device (assuming that's the first one
> added), and the subsequent IDs are just ignored and left to go wrong? Or is it
> inherently guaranteed that all devices in the same container are programmed
> with the same ID?

Yeah latter one is correct!! Each container is associated with an ID which is common for all the devices. Container device gets added first and is responsible for creating the iommu group and the master CFG.

Thanks,
Nipun

> 
> Robin.
> 
> > +	return 0;
> > +}
> > +
> >  static int arm_smmu_add_device(struct device *dev)  {
> >  	struct iommu_group *group;
> > @@ -1467,6 +1501,8 @@ static struct iommu_group
> > *arm_smmu_device_group(struct device *dev)
> >
> >  	if (dev_is_pci(dev))
> >  		group = pci_device_group(dev);
> > +	else if (dev_is_fsl_mc(dev))
> > +		group = fslmc_device_group(dev);
> >  	else
> >  		group = generic_device_group(dev);
> >
> > @@ -1475,6 +1511,8 @@ static struct iommu_group
> > *arm_smmu_device_group(struct device *dev)
> >
> >  	if (dev_is_pci(dev))
> >  		ret = arm_smmu_init_pci_device(to_pci_dev(dev), group);
> > +	else if (dev_is_fsl_mc(dev))
> > +		ret = arm_smmu_init_fslmc_device(dev, group);
> >  	else
> >  		ret = arm_smmu_init_platform_device(dev, group);
> >
> > @@ -2102,6 +2140,11 @@ static int __init arm_smmu_init(void)
> >  	}
> >  #endif
> >
> > +#ifdef CONFIG_FSL_MC_BUS
> > +	if (!iommu_present(&fsl_mc_bus_type))
> > +		bus_set_iommu(&fsl_mc_bus_type, &arm_smmu_ops); #endif
> > +
> >  	return 0;
> >  }
> >
> >

-------------- next part --------------
An embedded message was scrubbed...
From: Nipun Gupta <nipun.gupta at nxp.com>
Subject: [PATCH v2] fsl-mc: add helper macro to determine if a device is of fsl_mc type
Date: Wed, 29 Jun 2016 17:14:39 +0000
Size: 9077
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160630/563688cd/attachment.mht>