Re: [RFC PATCH 06/11] drivers: iommu: make of_xlate() interface DT agnostic

From: Marek Szyprowski
Date: Wed Apr 20 2016 - 03:14:58 EST

Hi Lorenzo,

On 2016-04-19 13:30, Lorenzo Pieralisi wrote:
Hi Marek,

On Tue, Apr 19, 2016 at 10:28:02AM +0200, Marek Szyprowski wrote:
Hello,

On 2016-04-14 19:25, Lorenzo Pieralisi wrote:
On systems booting with ACPI, the IOMMU drivers require the same
kind of id mapping carried out with a DT tree through the of_xlate()
API in order to map devices identifiers to IOMMU ones.

On ACPI systems, since DT nodes are not present (ie struct
device.of_node == NULL), to identify the device requiring the translation
the struct device_node (and the structure used to pass translation
information - struct of_phandle_args - that contains a struct device_node)
cannot be used, so a generic translation structure to be used for IOMMU
mapping should be defined, based on the firmware agnostic fwnode_handle
type.

This patch mechanically refactors/renames the of_xlate API to make
it DT agnostic, by declaring a new type (struct iommu_fwspec), that
allows the kernel to pass a device identifier (fwnode - which can
represent either a DT node or an IOMMU FW node) and by changing the
of_xlate signature so that it does not take anymore the DT specific
of_phandle_args argument and replaces it with the DT agnostic
iommu_fwspec one.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
Cc: Matthias Brugger <matthias.bgg@xxxxxxxxx>
Cc: Will Deacon <will.deacon@xxxxxxx>
Cc: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
Cc: Rob Herring <robh+dt@xxxxxxxxxx>
Cc: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
Cc: Robin Murphy <robin.murphy@xxxxxxx>
Cc: Tomasz Nowicki <tn@xxxxxxxxxxxx>
Cc: Joerg Roedel <joro@xxxxxxxxxx>
Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
I'm not sure if this is the right approach, although I have not enough
knowledge on ACPI firmware. Do you plan to rewrite all subsystems to the
new "fwspec" based interface? Right now of_xlate is rather common
interface used by various subsystems. Maybe it will be much easier to
Yes, that's a valid concern, when you say "it is rather common" though,
it seems to me that the of_xlate footprint is still subsystem specific,
so this patch should be self-contained anyway (granted, doing this
conversion for a specific subsystem is questionable, I guess that what
you are asking is, if you do it for IOMMU, why would not you do it for
other subsystems ?).

I was curious if you want to replace of_xlate() interface in other subsystems
like clocks, power domains, regulators, etc. Each of_xlate interface is
specific to particular subsystem, but they all more or less follows the same
style, what makes it easier to understand the code.

It is an RFC for this specific reason.

just add acpi_xlate callback and plumb it to the generic code? Maybe later
when similar code will be in other subsystems and drivers, it can be
unified, having much more real use cases?
Yes, that's a possibility, it means adding yet another hook into
the IOMMU drivers, probably a simpler change than this one, I
posted this code as and RFC to see which direction we want to take
so further feedback is welcome we can then choose the best approach.

Thanks,
Lorenzo

---
drivers/iommu/arm-smmu.c | 12 +++++++-----
drivers/iommu/exynos-iommu.c | 11 +++++++----
drivers/iommu/mtk_iommu.c | 13 ++++++++-----
drivers/iommu/of_iommu.c | 20 ++++++++++++++++++--
include/linux/iommu.h | 24 ++++++++++++++++++++----
5 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6c42770..84bcff7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1440,18 +1440,20 @@ out_unlock:
return ret;
}
-static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+static int arm_smmu_fw_xlate(struct device *dev, struct iommu_fwspec *args)
{
struct arm_smmu_device *smmu;
- struct platform_device *smmu_pdev;
+ struct platform_device *smmu_pdev = NULL;
+
+ if (is_of_node(args->fwnode))
+ smmu_pdev = of_find_device_by_node(to_of_node(args->fwnode));
- smmu_pdev = of_find_device_by_node(args->np);
if (!smmu_pdev)
return -ENODEV;
smmu = platform_get_drvdata(smmu_pdev);
- return arm_smmu_add_dev_streamid(smmu, dev, args->args[0]);
+ return arm_smmu_add_dev_streamid(smmu, dev, args->param[0]);
}
static struct iommu_ops arm_smmu_ops = {
@@ -1468,7 +1470,7 @@ static struct iommu_ops arm_smmu_ops = {
.device_group = arm_smmu_device_group,
.domain_get_attr = arm_smmu_domain_get_attr,
.domain_set_attr = arm_smmu_domain_set_attr,
- .of_xlate = arm_smmu_of_xlate,
+ .fw_xlate = arm_smmu_fw_xlate,
.pgsize_bitmap = -1UL, /* Restricted during device attach */
};
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 5ecc86c..84ff5bb 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1250,13 +1250,16 @@ static void exynos_iommu_remove_device(struct device *dev)
iommu_group_remove_device(dev);
}
-static int exynos_iommu_of_xlate(struct device *dev,
- struct of_phandle_args *spec)
+static int exynos_iommu_fw_xlate(struct device *dev,
+ struct iommu_fwspec *args)
{
struct exynos_iommu_owner *owner = dev->archdata.iommu;
- struct platform_device *sysmmu = of_find_device_by_node(spec->np);
+ struct platform_device *sysmmu = NULL;
struct sysmmu_drvdata *data;
+ if (is_of_node(args->fwnode))
+ sysmmu = of_find_device_by_node(to_of_node(args->fwnode));
+
if (!sysmmu)
return -ENODEV;
@@ -1290,7 +1293,7 @@ static struct iommu_ops exynos_iommu_ops = {
.add_device = exynos_iommu_add_device,
.remove_device = exynos_iommu_remove_device,
.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
- .of_xlate = exynos_iommu_of_xlate,
+ .fw_xlate = exynos_iommu_fw_xlate,
};
static bool init_done;
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 929a66a..e08dc0a 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -436,20 +436,23 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev)
return data->m4u_group;
}
-static int mtk_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
+static int mtk_iommu_fw_xlate(struct device *dev, struct iommu_fwspec *args)
{
struct mtk_iommu_client_priv *head, *priv, *next;
struct platform_device *m4updev;
- if (args->args_count != 1) {
+ if (!is_of_node(args->fwnode))
+ return -ENODEV;
+
+ if (args->param_count != 1) {
dev_err(dev, "invalid #iommu-cells(%d) property for IOMMU\n",
- args->args_count);
+ args->param_count);
return -EINVAL;
}
if (!dev->archdata.iommu) {
/* Get the m4u device */
- m4updev = of_find_device_by_node(args->np);
+ m4updev = of_find_device_by_node(to_of_node(args->fwnode));
of_node_put(args->np);
if (WARN_ON(!m4updev))
return -EINVAL;
@@ -469,7 +472,7 @@ static int mtk_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
if (!priv)
goto err_free_mem;
- priv->mtk_m4u_id = args->args[0];
+ priv->mtk_m4u_id = args->param[0];
list_add_tail(&priv->client, &head->client);
return 0;
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 910826c..331dd78 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -135,6 +135,18 @@ struct iommu_ops *of_iommu_get_ops(struct device_node *np)
return ops;
}
+static void of_phandle_args_to_fwspec(struct of_phandle_args *iommu_data,
+ struct iommu_fwspec *fwspec)
+{
+ int i;
+
+ fwspec->fwnode = iommu_data->np ? &iommu_data->np->fwnode : NULL;
+ fwspec->param_count = iommu_data->args_count;
+
+ for (i = 0; i < iommu_data->args_count; i++)
+ fwspec->param[i] = iommu_data->args[i];
+}
+
static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
{
struct of_phandle_args *iommu_spec = data;
@@ -148,6 +160,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev,
struct device_node *master_np)
{
struct of_phandle_args iommu_spec;
+ struct iommu_fwspec fwspec;
struct device_node *np = NULL;
struct iommu_ops *ops = NULL;
int idx = 0;
@@ -170,8 +183,9 @@ struct iommu_ops *of_iommu_configure(struct device *dev,
iommu_spec.np = np;
iommu_spec.args_count = 1;
+ of_phandle_args_to_fwspec(&iommu_spec, &fwspec);
ops = of_iommu_get_ops(np);
- if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
+ if (!ops || !ops->fw_xlate || ops->fw_xlate(dev, &fwspec))
goto err_put_node;
of_node_put(np);
@@ -189,7 +203,9 @@ struct iommu_ops *of_iommu_configure(struct device *dev,
np = iommu_spec.np;
ops = of_iommu_get_ops(np);
- if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
+ of_phandle_args_to_fwspec(&iommu_spec, &fwspec);
+
+ if (!ops || !ops->fw_xlate || ops->fw_xlate(dev, &fwspec))
goto err_put_node;
of_node_put(np);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0bba25e..5184d81 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -85,6 +85,24 @@ struct iommu_domain {
void *iova_cookie;
};
+#define IOMMU_MAP_SPEC_PARAMS 16
+
+/**
+ * struct iommu_fwspec - generic IOMMU specifier structure
+ *
+ * @fwnode: Pointer to a firmware-specific descriptor
+ * @param_count: Number of device-specific parameters
+ * @param: Device-specific parameters
+ *
+ * This structure, directly modeled after of_phandle_args, is used to
+ * pass a device-specific description of an IOMMU mapping.
+ */
+struct iommu_fwspec {
+ struct fwnode_handle *fwnode;
+ int param_count;
+ u32 param[IOMMU_MAP_SPEC_PARAMS];
+};
+
enum iommu_cap {
IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA
transactions */
@@ -155,7 +173,7 @@ struct iommu_dm_region {
* @domain_window_disable: Disable a particular window for a domain
* @domain_set_windows: Set the number of windows for a domain
* @domain_get_windows: Return the number of windows for a domain
- * @of_xlate: add OF master IDs to iommu grouping
+ * @fw_xlate: add FW master IDs to iommu grouping
* @pgsize_bitmap: bitmap of supported page sizes
* @priv: per-instance data private to the iommu driver
*/
@@ -196,9 +214,7 @@ struct iommu_ops {
/* Get the number of windows per domain */
u32 (*domain_get_windows)(struct iommu_domain *domain);
-#ifdef CONFIG_OF_IOMMU
- int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
-#endif
+ int (*fw_xlate)(struct device *dev, struct iommu_fwspec *args);
unsigned long pgsize_bitmap;
void *priv;

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland