Skip to contentSkip to conversation

fsl-mc: Added header file to provide fsl-mc bus iommu group creation

RFC PATCH New Nipun Gupta 2 participants 8 messages Jun 29, 2016 – Jul 1, 2016

Subsystems (2)
Staging IOMMU
lore
Share & tools
lore

Nipun Gupta

author
#0 lore raw

RFC PATCH 1/2 fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Added a header file fsl_mc_smmu.h to provide basic support of creating an IOMMU group for a fsl-mc type device and also provide helper macro to get the stream ID of fsl-mc tyoe device.

Signed-off-by Nipun Gupta <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45 ++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +45 -0
new file mode 100644
index 0000000..9dff5ba
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Nipun Gupta <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _FSL_MC_SMMU_H_
+#define _FSL_MC_SMMU_H_
+
+#include <../drivers/staging/fsl-mc/include/mc.h>
+
+/* Macro to get the MC device ICID (Stream ID) */
+#define fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid)
+
+/* Macro to get the container device of a MC device */
+#define fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \
+	FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent))
+
+/* Macro to check if a device is a container device */
+#define is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC)
+
+static struct iommu_group *fslmc_device_group(struct device *dev)
+{
+	struct device *cont_dev = fslmc_cont_dev(dev);
+	struct iommu_group *group;
+
+	/* Container device is responsible for creating the iommu group */
+	if (is_cont_dev(dev)) {
+		group = iommu_group_alloc();
+
+		if (IS_ERR(group))
+			return NULL;
+	} else {
+		get_device(cont_dev);
+		group = iommu_group_get(cont_dev);
+		put_device(cont_dev);
+	}
+
+	return group;
+}
+
+#endif /* _FSL_MC_SMMU_H_ */
Signature · 1 line
1.9.1
author
#1 lore raw

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

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 <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/iommu/arm-smmu.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
drivers/iommu/arm-smmu.c +43 -0
index ab365ec..28d5dc8 100644
@@ -47,6 +47,9 @@
 
 #include <linux/amba/bus.h>
 
+/* Include path will be changed once FSL-MC support is out from staging */
+#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)) {
+		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);
+}
+
+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);
+	}
+
+	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;
 }
Signature · 1 line
1.9.1
reviewer
#2 lore raw

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

Quoted 14 lines · Nipun Gupta

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 <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/iommu/arm-smmu.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
drivers/iommu/arm-smmu.c +0 -0
index ab365ec..28d5dc8 100644

drivers/iommu/arm-smmu.c line 50

Quoted 4 lines
drivers/iommu/arm-smmu.c +1 -0
@@ -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?

drivers/iommu/arm-smmu.c static struct device_node *dev_get_dev_node(struct device *dev) · line 453

Quoted 10 lines
drivers/iommu/arm-smmu.c +3 -0
+#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.

Quoted 8 lines
drivers/iommu/arm-smmu.c +4 -0
+		while (dev_is_fsl_mc(dev))
+			dev = dev->parent;
+	}
+
 	return dev->of_node;
 }

drivers/iommu/arm-smmu.c static int arm_smmu_init_platform_device(struct device *dev, · line 1457

Quoted 7 lines
drivers/iommu/arm-smmu.c +4 -0
@@ -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.

Quoted 7 lines
drivers/iommu/arm-smmu.c +7 -0
+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) {

drivers/iommu/arm-smmu.c static int arm_smmu_init_platform_device(struct device *dev, · line 1475

Quoted 10 lines
drivers/iommu/arm-smmu.c +10 -0
+		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?

Robin.

Quoted 37 lines
drivers/iommu/arm-smmu.c +12 -0
+	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;
 }
author
#3 lore raw

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

Quoted 28 lines

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Thursday, June 30, 2016 16:53
To: Nipun Gupta <[email protected]>; will.deacon at arm.com; Stuart Yoder <[email protected]>; iommu at lists.linux-foundation.org; linux-arm- kernel at lists.infradead.org
Cc: Bharat Bhushan <[email protected]>
Subject: Re: [RFC PATCH 2/2] iommu/arm-smmu: Add support for the fsl-mc bus

Quoted 16 lines · Nipun Gupta

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 <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/iommu/arm-smmu.c | 43
+++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
drivers/iommu/arm-smmu.c index renamed from drivers/iommu/arm-smmu.c +0 -0

ab365ec..28d5dc8 100644

drivers/iommu/arm-smmu.c +0 -0
@@ -47,6 +47,9 @@
Quoted 6 lines

drivers/iommu/arm-smmu.c

Quoted 4 lines
drivers/iommu/arm-smmu.c +2 -0
 #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 :)

Quoted 8 lines

drivers/iommu/arm-smmu.c

Quoted 7 lines
drivers/iommu/arm-smmu.c +2 -1
+#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)

Quoted 7 lines

drivers/iommu/arm-smmu.c static struct device_node *dev_get_dev_node(struct device *dev) · line 453

Quoted 4 lines
drivers/iommu/arm-smmu.c +1 -0
 		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.

Quoted 13 lines

drivers/iommu/arm-smmu.c static int arm_smmu_init_platform_device(struct

Quoted 9 lines
drivers/iommu/arm-smmu.c +4 -0
+		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,

Quoted 3 lines
drivers/iommu/arm-smmu.c +0 -0
 	return 0;
 }
Quoted 6 lines

drivers/iommu/arm-smmu.c static int arm_smmu_init_platform_device(struct device *dev, · line 1457

Quoted 3 lines
drivers/iommu/arm-smmu.c +3 -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?

Quoted 18 lines
Quoted 7 lines
drivers/iommu/arm-smmu.c +6 -0
+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);

drivers/iommu/arm-smmu.c static int arm_smmu_init_platform_device(struct device *dev, · line 1476

Quoted 10 lines
drivers/iommu/arm-smmu.c +10 -0
+	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);

Quoted 6 lines

drivers/iommu/arm-smmu.c static int arm_smmu_init_platform_device(struct · line 1473

Quoted 1 line
drivers/iommu/arm-smmu.c +1 -0
+	}

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

Quoted 32 lines

Robin.

Quoted 29 lines
drivers/iommu/arm-smmu.c +7 -0
+	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
Quoted 8 lines

drivers/iommu/arm-smmu.c static int __init arm_smmu_init(void) · line 2150

Quoted 8 lines
drivers/iommu/arm-smmu.c +4 -0
+#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 <[email protected]>
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>

reviewer
#4 lore raw

RFC PATCH 1/2 fsl-mc: Added header file to provide fsl-mc bus iommu group creation

In reply to Nipun Gupta
Quoted 23 lines · Nipun Gupta

Added a header file fsl_mc_smmu.h to provide basic support of creating an IOMMU group for a fsl-mc type device and also provide helper macro to get the stream ID of fsl-mc tyoe device.

Signed-off-by Nipun Gupta <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45 ++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +6 -0
new file mode 100644
index 0000000..9dff5ba
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Nipun Gupta <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 16

Quoted 10 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +10 -0
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _FSL_MC_SMMU_H_
+#define _FSL_MC_SMMU_H_
+
+#include <../drivers/staging/fsl-mc/include/mc.h>
+
+/* Macro to get the MC device ICID (Stream ID) */
+#define fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid)

Is the the full 15-bit ICID from the MC's point of view, just the 7 bits that are actually routed to the SMMU, or the actual stream ID seen by the SMMU? None of those three are necessarily the same, and unless it's the third then I don't see the point of patches adding incomplete code which isn't going to work.

Quoted 16 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +16 -0
+/* Macro to get the container device of a MC device */
+#define fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \
+	FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent))
+
+/* Macro to check if a device is a container device */
+#define is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC)
+
+static struct iommu_group *fslmc_device_group(struct device *dev)
+{
+	struct device *cont_dev = fslmc_cont_dev(dev);
+	struct iommu_group *group;
+
+	/* Container device is responsible for creating the iommu group */
+	if (is_cont_dev(dev)) {
+		group = iommu_group_alloc();
+

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 43

Quoted 10 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +10 -0
+		if (IS_ERR(group))
+			return NULL;
+	} else {
+		get_device(cont_dev);
+		group = iommu_group_get(cont_dev);
+		put_device(cont_dev);
+	}
+
+	return group;
+}

In isolation, though, this part seems perfectly reasonable.

Robin.

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +2 -0
+
+#endif /* _FSL_MC_SMMU_H_ */
author
#5 lore raw

RFC PATCH 1/2 fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 45 lines

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Thursday, June 30, 2016 16:26
To: Nipun Gupta <[email protected]>; will.deacon at arm.com; Stuart Yoder <[email protected]>; iommu at lists.linux-foundation.org; linux-arm- kernel at lists.infradead.org
Cc: Bharat Bhushan <[email protected]>
Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 32 lines · Nipun Gupta

Added a header file fsl_mc_smmu.h to provide basic support of creating an IOMMU group for a fsl-mc type device and also provide helper macro to get the stream ID of fsl-mc tyoe device.

Signed-off-by Nipun Gupta <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45
++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +13 -0
new file mode 100644
index 0000000..9dff5ba
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Nipun Gupta <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _FSL_MC_SMMU_H_
+#define _FSL_MC_SMMU_H_
+
+#include <../drivers/staging/fsl-mc/include/mc.h>
Quoted 8 lines

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 16

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +3 -0
+
+/* Macro to get the MC device ICID (Stream ID) */ #define
+fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid)

Is the the full 15-bit ICID from the MC's point of view, just the 7 bits that are actually routed to the SMMU, or the actual stream ID seen by the SMMU? None of those three are necessarily the same, and unless it's the third then I don't see the point of patches adding incomplete code which isn't going to work.

Hi Robin,

It's the second one where just 7 bits are maintained in the 'fsl_mc_device--->icid' which is programmed in SMMU. Right, once the SMR masking support is there then only these patches would make more sense. We have sent these patches to get some early comments and to have you guys well informed beforehand about the changes which we require in the SMMU driver specific to fsl-mc bus :).

Thanks,
Nipun

Quoted 34 lines
Quoted 16 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +15 -0
+/* Macro to get the container device of a MC device */ #define
+fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \
+	FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent))
+
+/* Macro to check if a device is a container device */ #define
+is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC)
+
+static struct iommu_group *fslmc_device_group(struct device *dev) {
+	struct device *cont_dev = fslmc_cont_dev(dev);
+	struct iommu_group *group;
+
+	/* Container device is responsible for creating the iommu group */
+	if (is_cont_dev(dev)) {
+		group = iommu_group_alloc();
+

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 43

Quoted 10 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +10 -0
+		if (IS_ERR(group))
+			return NULL;
+	} else {
+		get_device(cont_dev);
+		group = iommu_group_get(cont_dev);
+		put_device(cont_dev);
+	}
+
+	return group;
+}

In isolation, though, this part seems perfectly reasonable.

Robin.

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +2 -0
+
+#endif /* _FSL_MC_SMMU_H_ */
reviewer
#6 lore raw

RFC PATCH 1/2 fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 54 lines · Nipun Gupta
Quoted 45 lines

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Thursday, June 30, 2016 16:26
To: Nipun Gupta <[email protected]>; will.deacon at arm.com; Stuart Yoder <[email protected]>; iommu at lists.linux-foundation.org; linux-arm- kernel at lists.infradead.org
Cc: Bharat Bhushan <[email protected]>
Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 32 lines · Nipun Gupta

Added a header file fsl_mc_smmu.h to provide basic support of creating an IOMMU group for a fsl-mc type device and also provide helper macro to get the stream ID of fsl-mc tyoe device.

Signed-off-by Nipun Gupta <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45
++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +13 -0
new file mode 100644
index 0000000..9dff5ba
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Nipun Gupta <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _FSL_MC_SMMU_H_
+#define _FSL_MC_SMMU_H_
+
+#include <../drivers/staging/fsl-mc/include/mc.h>
Quoted 8 lines

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 16

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +3 -0
+
+/* Macro to get the MC device ICID (Stream ID) */ #define
+fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid)

Is the the full 15-bit ICID from the MC's point of view, just the 7 bits that are actually routed to the SMMU, or the actual stream ID seen by the SMMU? None of those three are necessarily the same, and unless it's the third then I don't see the point of patches adding incomplete code which isn't going to work.

Quoted 4 lines

Hi Robin,

It's the second one where just 7 bits are maintained in the 'fsl_mc_device--->icid' which is programmed in SMMU. Right, once the SMR masking support is there then only these patches would make more sense. We have sent these patches to get some early comments and to have you guys well informed beforehand about the changes which we require in the SMMU driver specific to fsl-mc bus :).

Indeed, it's just kinda hard to review incomplete code, especially when it's the parts which don't exist yet which are the most significant ;)

Either way, I'm deep in the middle of reworking SMMUv2 generic bindings[1] based on the latest iteration of the core code and iommu_fwspec[2], which removes the need for nearly all the bus-specific business within the driver (it's getting too big to be 4.8 material now, but I'll have something to post shortly). In that context, if the fsl-mc driver could process an "iommu-map" property on the MC node to plumb the ICID-to-stream-ID relationship into of_xlate, and inherit bus->iommu_ops from its parent bus, there shouldn't be any need to directly touch the SMMU driver at all.

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/14303

Quoted 39 lines

Thanks,
Nipun

Quoted 34 lines
Quoted 16 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +15 -0
+/* Macro to get the container device of a MC device */ #define
+fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \
+	FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent))
+
+/* Macro to check if a device is a container device */ #define
+is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC)
+
+static struct iommu_group *fslmc_device_group(struct device *dev) {
+	struct device *cont_dev = fslmc_cont_dev(dev);
+	struct iommu_group *group;
+
+	/* Container device is responsible for creating the iommu group */
+	if (is_cont_dev(dev)) {
+		group = iommu_group_alloc();
+

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 43

Quoted 10 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +10 -0
+		if (IS_ERR(group))
+			return NULL;
+	} else {
+		get_device(cont_dev);
+		group = iommu_group_get(cont_dev);
+		put_device(cont_dev);
+	}
+
+	return group;
+}

In isolation, though, this part seems perfectly reasonable.

Robin.

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +2 -0
+
+#endif /* _FSL_MC_SMMU_H_ */
author
#7 lore raw

RFC PATCH 1/2 fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 80 lines

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Thursday, June 30, 2016 19:44
To: Nipun Gupta <[email protected]>; will.deacon at arm.com; Stuart Yoder <[email protected]>; iommu at lists.linux-foundation.org; linux-arm- kernel at lists.infradead.org
Cc: Bharat Bhushan <[email protected]>
Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 45 lines · Nipun Gupta
Quoted 45 lines

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Thursday, June 30, 2016 16:26
To: Nipun Gupta <[email protected]>; will.deacon at arm.com; Stuart Yoder <[email protected]>; iommu at lists.linux-foundation.org; linux-arm- kernel at lists.infradead.org
Cc: Bharat Bhushan <[email protected]>
Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation

Quoted 32 lines · Nipun Gupta

Added a header file fsl_mc_smmu.h to provide basic support of creating an IOMMU group for a fsl-mc type device and also provide helper macro to get the stream ID of fsl-mc tyoe device.

Signed-off-by Nipun Gupta <[email protected]>
Signed-off-by Bharat Bhushan <[email protected]>
---
 drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45
++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +13 -0
new file mode 100644
index 0000000..9dff5ba
@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Nipun Gupta <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _FSL_MC_SMMU_H_
+#define _FSL_MC_SMMU_H_
+
+#include <../drivers/staging/fsl-mc/include/mc.h>
Quoted 8 lines
Quoted 8 lines

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 16

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +3 -0
+
+/* Macro to get the MC device ICID (Stream ID) */ #define
+fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid)

Is the the full 15-bit ICID from the MC's point of view, just the 7 bits that are actually routed to the SMMU, or the actual stream ID seen by the SMMU? None of those three are necessarily the same, and unless it's the third then I don't see the point of patches adding incomplete

code which isn't going to work.

Quoted 5 lines

Hi Robin,

It's the second one where just 7 bits are maintained in the 'fsl_mc_device--- icid' which is programmed in SMMU. Right, once the SMR masking support is

there then only these patches would make more sense.

Quoted 1 line

We have sent these patches to get some early comments and to have you guys

well informed beforehand about the changes which we require in the SMMU driver specific to fsl-mc bus :).

Indeed, it's just kinda hard to review incomplete code, especially when it's the parts which don't exist yet which are the most significant ;)

Quoted 8 lines

Either way, I'm deep in the middle of reworking SMMUv2 generic bindings[1] based on the latest iteration of the core code and iommu_fwspec[2], which removes the need for nearly all the bus-specific business within the driver (it's getting too big to be 4.8 material now, but I'll have something to post shortly). In that context, if the fsl-mc driver could process an "iommu-map" property on the MC node to plumb the ICID-to-stream-ID relationship into of_xlate, and inherit bus->iommu_ops from its parent bus, there shouldn't be any need to directly touch the SMMU driver at all.

Hi Robin,

One thing I would like to bring to your notice is that with our SoC the container devices gets dynamically created by MC (using linux command line). Once the container is created, the ICID is provided by MC to the devices. Structure is somewhat the following:

							      		  ---- device 1 (ID 1)
				  ---- Container device 1 (ID 1)-----------	|---- device 2 (ID 1)
				|					  ---- device 3 (ID 1) ...
	MC bus/device------------	|---- Container device 2 (ID 2) ...
	(has a DT node)  	  ---- Container device 3 (ID 3) ...

Here the MC device node which is the SMMU master mode is present in the device tree, but the Container devices are created dynamically with ICID's provided to them by the MC (again at run-time). A single iommu_group is required to be created per container, which will have the container device and all of its child devices added into that group.

On lines of your suggestion we can call the of_xlate callback of the MC device whenever the devices are created. This shall make API 'arm_smmu_init_fslmc_device' redundant from http://article.gmane.org/gmane.linux.kernel.iommu/14323 (patch 2/2). Also looking at the your older patch-set for SMMU-v2 if we use of_xlate, I think for fsl-mc bus support there may be few changes required to facilitate addition of all the devices within the container into the single iommu_group & also setting up the 'dev->archdata.iommu' for the fsl-mc non-container devices. I need to have a closer look at the iommu_fwspec[2] to see if these other changes required to support fsl-mc bus can also be minimized/removed.

We are eagerly waiting for your patches on SMMU-v2 for the new exciting stuff :).

Thanks,
Nipun

Quoted 45 lines

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/14303

Quoted 39 lines

Thanks,
Nipun

Quoted 34 lines
Quoted 16 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +15 -0
+/* Macro to get the container device of a MC device */ #define
+fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \
+	FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent))
+
+/* Macro to check if a device is a container device */ #define
+is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC)
+
+static struct iommu_group *fslmc_device_group(struct device *dev) {
+	struct device *cont_dev = fslmc_cont_dev(dev);
+	struct iommu_group *group;
+
+	/* Container device is responsible for creating the iommu group */
+	if (is_cont_dev(dev)) {
+		group = iommu_group_alloc();
+

drivers/staging/fsl-mc/include/fsl_mc_smmu.h line 43

Quoted 10 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +10 -0
+		if (IS_ERR(group))
+			return NULL;
+	} else {
+		get_device(cont_dev);
+		group = iommu_group_get(cont_dev);
+		put_device(cont_dev);
+	}
+
+	return group;
+}

In isolation, though, this part seems perfectly reasonable.

Robin.

Quoted 3 lines
drivers/staging/fsl-mc/include/fsl_mc_smmu.h new file +2 -0
+
+#endif /* _FSL_MC_SMMU_H_ */