diff mbox

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

Message ID HE1PR0401MB18663F18D156196AB570E174E6240@HE1PR0401MB1866.eurprd04.prod.outlook.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nipun Gupta June 30, 2016, 12:11 p.m. UTC 
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: Thursday, June 30, 2016 16:53
> To: Nipun Gupta <nipun.gupta@nxp.com>; will.deacon@arm.com; Stuart Yoder
> <stuart.yoder@nxp.com>; iommu@lists.linux-foundation.org; linux-arm-
> kernel@lists.infradead.org
> Cc: Bharat Bhushan <bharat.bhushan@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@nxp.com>
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@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@driverdev.osuosl.org ; linux-kernel@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;
> >  }
> >
> >
Add a helper macro to return if a device has a bus type of fsl_mc.
This makes the bus driver code more readable and provides a way for
drivers like the SMMU driver to easily check the bus type.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Signed-off-by: Bharat Bhushan <bharat.bhushan@nxp.com>
---
 drivers/staging/fsl-mc/bus/dprc-driver.c               | 4 ++--
 drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c | 2 +-
 drivers/staging/fsl-mc/bus/mc-allocator.c              | 6 +++---
 drivers/staging/fsl-mc/bus/mc-bus.c                    | 6 +++---
 drivers/staging/fsl-mc/include/mc.h                    | 7 +++++++
 5 files changed, 16 insertions(+), 9 deletions(-)

--
1.9.1
diff mbox

Patch

diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-mc/bus/dprc-driver.c
index 7fc4717..cd6d75a 100644
--- a/drivers/staging/fsl-mc/bus/dprc-driver.c
+++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
@@ -649,7 +649,7 @@  static int dprc_probe(struct fsl_mc_device *mc_dev)
                /*
                 * This is a child DPRC:
                 */
-               if (WARN_ON(parent_dev->bus != &fsl_mc_bus_type))
+               if (WARN_ON(!dev_is_fsl_mc(parent_dev)))
                        return -EINVAL;

                if (WARN_ON(mc_dev->obj_desc.region_count == 0))
@@ -681,7 +681,7 @@  static int dprc_probe(struct fsl_mc_device *mc_dev)
                 */
                struct irq_domain *mc_msi_domain;

-               if (WARN_ON(parent_dev->bus == &fsl_mc_bus_type))
+               if (WARN_ON(dev_is_fsl_mc(parent_dev)))
                        return -EINVAL;

                error = fsl_mc_find_msi_domain(parent_dev,
diff --git a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
index 720e2b0..d0c20d6 100644
--- a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
@@ -35,7 +35,7 @@  static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
        struct fsl_mc_device *mc_bus_dev;
        struct msi_domain_info *msi_info;

-       if (WARN_ON(dev->bus != &fsl_mc_bus_type))
+       if (WARN_ON(!dev_is_fsl_mc(dev)))
                return -EINVAL;

        mc_bus_dev = to_fsl_mc_device(dev);
diff --git a/drivers/staging/fsl-mc/bus/mc-allocator.c b/drivers/staging/fsl-mc/bus/mc-allocator.c
index fb08f22..9216c32 100644
--- a/drivers/staging/fsl-mc/bus/mc-allocator.c
+++ b/drivers/staging/fsl-mc/bus/mc-allocator.c
@@ -281,7 +281,7 @@  int __must_check fsl_mc_portal_allocate(struct fsl_mc_device *mc_dev,
        if (mc_dev->flags & FSL_MC_IS_DPRC) {
                mc_bus_dev = mc_dev;
        } else {
-               if (WARN_ON(mc_dev->dev.parent->bus != &fsl_mc_bus_type))
+               if (WARN_ON(!dev_is_fsl_mc(mc_dev->dev.parent)))
                        return error;

                mc_bus_dev = to_fsl_mc_device(mc_dev->dev.parent);
@@ -420,7 +420,7 @@  int __must_check fsl_mc_object_allocate(struct fsl_mc_device *mc_dev,
        if (WARN_ON(mc_dev->flags & FSL_MC_IS_DPRC))
                goto error;

-       if (WARN_ON(mc_dev->dev.parent->bus != &fsl_mc_bus_type))
+       if (WARN_ON(!dev_is_fsl_mc(mc_dev->dev.parent)))
                goto error;

        if (WARN_ON(pool_type == FSL_MC_POOL_DPMCP))
@@ -678,7 +678,7 @@  static int fsl_mc_allocator_probe(struct fsl_mc_device *mc_dev)
                return -EINVAL;

        mc_bus_dev = to_fsl_mc_device(mc_dev->dev.parent);
-       if (WARN_ON(mc_bus_dev->dev.bus != &fsl_mc_bus_type))
+       if (WARN_ON(!dev_is_fsl_mc(&mc_bus_dev->dev)))
                return -EINVAL;

        mc_bus = to_fsl_mc_bus(mc_bus_dev);
diff --git a/drivers/staging/fsl-mc/bus/mc-bus.c b/drivers/staging/fsl-mc/bus/mc-bus.c
index 4053643..098f07c 100644
--- a/drivers/staging/fsl-mc/bus/mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/mc-bus.c
@@ -207,11 +207,11 @@  static void fsl_mc_get_root_dprc(struct device *dev,
 {
        if (WARN_ON(!dev)) {
                *root_dprc_dev = NULL;
-       } else if (WARN_ON(dev->bus != &fsl_mc_bus_type)) {
+       } else if (WARN_ON(!dev_is_fsl_mc(dev))) {
                *root_dprc_dev = NULL;
        } else {
                *root_dprc_dev = dev;
-               while ((*root_dprc_dev)->parent->bus == &fsl_mc_bus_type)
+               while (dev_is_fsl_mc((*root_dprc_dev)->parent))
                        *root_dprc_dev = (*root_dprc_dev)->parent;
        }
 }
@@ -405,7 +405,7 @@  int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
        struct fsl_mc_bus *mc_bus = NULL;
        struct fsl_mc_device *parent_mc_dev;

-       if (parent_dev->bus == &fsl_mc_bus_type)
+       if (dev_is_fsl_mc(parent_dev))
                parent_mc_dev = to_fsl_mc_device(parent_dev);
        else
                parent_mc_dev = NULL;
diff --git a/drivers/staging/fsl-mc/include/mc.h b/drivers/staging/fsl-mc/include/mc.h
index ac7c1ce..91213f5 100644
--- a/drivers/staging/fsl-mc/include/mc.h
+++ b/drivers/staging/fsl-mc/include/mc.h
@@ -183,6 +183,13 @@  struct fsl_mc_device {
 #define to_fsl_mc_device(_dev) \
        container_of(_dev, struct fsl_mc_device, dev)

+#ifdef CONFIG_FSL_MC_BUS
+#define dev_is_fsl_mc(_dev) ((_dev)->bus == &fsl_mc_bus_type)
+#else
+/* If fsl-mc bus is not present device cannot belong to fsl-mc bus */
+#define dev_is_fsl_mc(_dev) (0)
+#endif
+
 /*
  * module_fsl_mc_driver() - Helper macro for drivers that don't do
  * anything special in module init/exit.  This eliminates a lot of