From: Joerg Roedel <[email protected]>
The 'struct exynos_iommu_owner' is an umbrella for multiple SYSMMU
instances attached to one master. As such all these instances are
handled the same, they are all configured with the same iommu_domain,
for example.
The IOMMU core code expects each device to have only one IOMMU
attached, so create the IOMMU-device for the umbrella instead of each
hardware SYSMMU.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/exynos-iommu.c | 96 +++++++++++++++++++++++++++---------
1 file changed, 73 insertions(+), 23 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 186ff5cc975c..86ecccbf0438 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -235,6 +235,8 @@ struct exynos_iommu_owner {
struct list_head controllers; /* list of sysmmu_drvdata.owner_node */
struct iommu_domain *domain; /* domain this device is attached */
struct mutex rpm_lock; /* for runtime pm of all sysmmus */
+
+ struct iommu_device iommu; /* IOMMU core handle */
};
/*
@@ -274,8 +276,6 @@ struct sysmmu_drvdata {
struct list_head owner_node; /* node for owner controllers list */
phys_addr_t pgtable; /* assigned page table structure */
unsigned int version; /* our version */
-
- struct iommu_device iommu; /* IOMMU core handle */
};
static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
@@ -625,18 +625,6 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
data->sysmmu = dev;
spin_lock_init(&data->lock);
- ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
- dev_name(data->sysmmu));
- if (ret)
- return ret;
-
- iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
- iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
-
- ret = iommu_device_register(&data->iommu);
- if (ret)
- return ret;
-
platform_set_drvdata(pdev, data);
__sysmmu_get_version(data);
@@ -1261,6 +1249,8 @@ static int exynos_iommu_add_device(struct device *dev)
}
iommu_group_put(group);
+ iommu_device_link(&owner->iommu, dev);
+
return 0;
}
@@ -1282,18 +1272,82 @@ static void exynos_iommu_remove_device(struct device *dev)
iommu_group_put(group);
}
}
+ iommu_device_unlink(&owner->iommu, dev);
iommu_group_remove_device(dev);
list_for_each_entry(data, &owner->controllers, owner_node)
device_link_del(data->link);
}
+static int exynos_iommu_device_init(struct exynos_iommu_owner *owner)
+{
+ static u32 counter = 0;
+ int ret;
+
+ /*
+ * Create a virtual IOMMU device. In reality it is an umbrella for a
+ * number of SYSMMU platform devices, but that also means that any
+ * master can have more than one real IOMMU device. This drivers handles
+ * all the real devices for one master synchronously, so they appear as
+ * one anyway.
+ */
+ ret = iommu_device_sysfs_add(&owner->iommu, NULL, NULL,
+ "sysmmu-owner-%d", counter++);
+ if (ret)
+ return ret;
+
+ iommu_device_set_ops(&owner->iommu, &exynos_iommu_ops);
+
+ return 0;
+}
+
+static void exynos_iommu_device_remove(struct exynos_iommu_owner *owner)
+{
+ iommu_device_set_ops(&owner->iommu, NULL);
+ iommu_device_sysfs_remove(&owner->iommu);
+}
+
+static int exynos_owner_init(struct device *dev)
+{
+ struct exynos_iommu_owner *owner = dev->archdata.iommu;
+ int ret;
+
+ if (owner)
+ return 0;
+
+ owner = kzalloc(sizeof(*owner), GFP_KERNEL);
+ if (!owner)
+ return -ENOMEM;
+
+ ret = exynos_iommu_device_init(owner);
+ if (ret)
+ goto out_free_owner;
+
+ ret = iommu_device_register(&owner->iommu);
+ if (ret)
+ goto out_remove_iommu_device;
+
+ INIT_LIST_HEAD(&owner->controllers);
+ mutex_init(&owner->rpm_lock);
+ dev->archdata.iommu = owner;
+
+ return 0;
+
+out_remove_iommu_device:
+ exynos_iommu_device_remove(owner);
+out_free_owner:
+ kfree(owner);
+
+ return ret;
+}
+
static int exynos_iommu_of_xlate(struct device *dev,
struct of_phandle_args *spec)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
struct platform_device *sysmmu = of_find_device_by_node(spec->np);
struct sysmmu_drvdata *data, *entry;
+ struct exynos_iommu_owner *owner;
+ int ret;
if (!sysmmu)
return -ENODEV;
@@ -1302,15 +1356,11 @@ static int exynos_iommu_of_xlate(struct device *dev,
if (!data)
return -ENODEV;
- if (!owner) {
- owner = kzalloc(sizeof(*owner), GFP_KERNEL);
- if (!owner)
- return -ENOMEM;
+ ret = exynos_owner_init(dev);
+ if (ret)
+ return ret;
- INIT_LIST_HEAD(&owner->controllers);
- mutex_init(&owner->rpm_lock);
- dev->archdata.iommu = owner;
- }
+ owner = dev->archdata.iommu;
list_for_each_entry(entry, &owner->controllers, owner_node)
if (entry == data)
--
2.17.1
Hi Joerg,
On 07.04.2020 20:37, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> The 'struct exynos_iommu_owner' is an umbrella for multiple SYSMMU
> instances attached to one master. As such all these instances are
> handled the same, they are all configured with the same iommu_domain,
> for example.
>
> The IOMMU core code expects each device to have only one IOMMU
> attached, so create the IOMMU-device for the umbrella instead of each
> hardware SYSMMU.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/exynos-iommu.c | 96 +++++++++++++++++++++++++++---------
> 1 file changed, 73 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 186ff5cc975c..86ecccbf0438 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -235,6 +235,8 @@ struct exynos_iommu_owner {
> struct list_head controllers; /* list of sysmmu_drvdata.owner_node */
> struct iommu_domain *domain; /* domain this device is attached */
> struct mutex rpm_lock; /* for runtime pm of all sysmmus */
> +
> + struct iommu_device iommu; /* IOMMU core handle */
> };
>
> /*
> @@ -274,8 +276,6 @@ struct sysmmu_drvdata {
> struct list_head owner_node; /* node for owner controllers list */
> phys_addr_t pgtable; /* assigned page table structure */
> unsigned int version; /* our version */
> -
> - struct iommu_device iommu; /* IOMMU core handle */
> };
>
> static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -625,18 +625,6 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
> data->sysmmu = dev;
> spin_lock_init(&data->lock);
>
> - ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
> - dev_name(data->sysmmu));
> - if (ret)
> - return ret;
> -
> - iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
> - iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
The iommu_device_set_fwnode() call is lost during this conversion, what breaks driver operation. Most of the above IOMMU fw calls you have moved to xlate function. I've checked briefly but it looks that there is a chicken-egg problem here. The owner structure is allocated and initialized from of_xlate(), which won't be called without linking the problem iommu structure with the fwnode first, what might be done only in sysmmu_probe(). I will check how to handle this in a different way.
> -
> - ret = iommu_device_register(&data->iommu);
> - if (ret)
> - return ret;
> -
> platform_set_drvdata(pdev, data);
>
> __sysmmu_get_version(data);
> @@ -1261,6 +1249,8 @@ static int exynos_iommu_add_device(struct device *dev)
> }
> iommu_group_put(group);
>
> + iommu_device_link(&owner->iommu, dev);
> +
> return 0;
> }
>
> @@ -1282,18 +1272,82 @@ static void exynos_iommu_remove_device(struct device *dev)
> iommu_group_put(group);
> }
> }
> + iommu_device_unlink(&owner->iommu, dev);
> iommu_group_remove_device(dev);
>
> list_for_each_entry(data, &owner->controllers, owner_node)
> device_link_del(data->link);
> }
>
> +static int exynos_iommu_device_init(struct exynos_iommu_owner *owner)
> +{
> + static u32 counter = 0;
> + int ret;
> +
> + /*
> + * Create a virtual IOMMU device. In reality it is an umbrella for a
> + * number of SYSMMU platform devices, but that also means that any
> + * master can have more than one real IOMMU device. This drivers handles
> + * all the real devices for one master synchronously, so they appear as
> + * one anyway.
> + */
> + ret = iommu_device_sysfs_add(&owner->iommu, NULL, NULL,
> + "sysmmu-owner-%d", counter++);
> + if (ret)
> + return ret;
> +
> + iommu_device_set_ops(&owner->iommu, &exynos_iommu_ops);
> +
> + return 0;
> +}
> +
> +static void exynos_iommu_device_remove(struct exynos_iommu_owner *owner)
> +{
> + iommu_device_set_ops(&owner->iommu, NULL);
> + iommu_device_sysfs_remove(&owner->iommu);
> +}
> +
> +static int exynos_owner_init(struct device *dev)
> +{
> + struct exynos_iommu_owner *owner = dev->archdata.iommu;
> + int ret;
> +
> + if (owner)
> + return 0;
> +
> + owner = kzalloc(sizeof(*owner), GFP_KERNEL);
> + if (!owner)
> + return -ENOMEM;
> +
> + ret = exynos_iommu_device_init(owner);
> + if (ret)
> + goto out_free_owner;
> +
> + ret = iommu_device_register(&owner->iommu);
> + if (ret)
> + goto out_remove_iommu_device;
> +
> + INIT_LIST_HEAD(&owner->controllers);
> + mutex_init(&owner->rpm_lock);
> + dev->archdata.iommu = owner;
> +
> + return 0;
> +
> +out_remove_iommu_device:
> + exynos_iommu_device_remove(owner);
> +out_free_owner:
> + kfree(owner);
> +
> + return ret;
> +}
> +
> static int exynos_iommu_of_xlate(struct device *dev,
> struct of_phandle_args *spec)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct platform_device *sysmmu = of_find_device_by_node(spec->np);
> struct sysmmu_drvdata *data, *entry;
> + struct exynos_iommu_owner *owner;
> + int ret;
>
> if (!sysmmu)
> return -ENODEV;
> @@ -1302,15 +1356,11 @@ static int exynos_iommu_of_xlate(struct device *dev,
> if (!data)
> return -ENODEV;
>
> - if (!owner) {
> - owner = kzalloc(sizeof(*owner), GFP_KERNEL);
> - if (!owner)
> - return -ENOMEM;
> + ret = exynos_owner_init(dev);
> + if (ret)
> + return ret;
>
> - INIT_LIST_HEAD(&owner->controllers);
> - mutex_init(&owner->rpm_lock);
> - dev->archdata.iommu = owner;
> - }
> + owner = dev->archdata.iommu;
>
> list_for_each_entry(entry, &owner->controllers, owner_node)
> if (entry == data)
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi again,
On 08.04.2020 14:23, Marek Szyprowski wrote:
> Hi Joerg,
>
> On 07.04.2020 20:37, Joerg Roedel wrote:
>> From: Joerg Roedel <[email protected]>
>>
>> The 'struct exynos_iommu_owner' is an umbrella for multiple SYSMMU
>> instances attached to one master. As such all these instances are
>> handled the same, they are all configured with the same iommu_domain,
>> for example.
>>
>> The IOMMU core code expects each device to have only one IOMMU
>> attached, so create the IOMMU-device for the umbrella instead of each
>> hardware SYSMMU.
>>
>> Signed-off-by: Joerg Roedel <[email protected]>
>> ---
>> drivers/iommu/exynos-iommu.c | 96 +++++++++++++++++++++++++++---------
>> 1 file changed, 73 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 186ff5cc975c..86ecccbf0438 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -235,6 +235,8 @@ struct exynos_iommu_owner {
>> struct list_head controllers; /* list of
>> sysmmu_drvdata.owner_node */
>> struct iommu_domain *domain; /* domain this device is
>> attached */
>> struct mutex rpm_lock; /* for runtime pm of all sysmmus */
>> +
>> + struct iommu_device iommu; /* IOMMU core handle */
>> };
>> /*
>> @@ -274,8 +276,6 @@ struct sysmmu_drvdata {
>> struct list_head owner_node; /* node for owner controllers
>> list */
>> phys_addr_t pgtable; /* assigned page table structure */
>> unsigned int version; /* our version */
>> -
>> - struct iommu_device iommu; /* IOMMU core handle */
>> };
>> static struct exynos_iommu_domain *to_exynos_domain(struct
>> iommu_domain *dom)
>> @@ -625,18 +625,6 @@ static int exynos_sysmmu_probe(struct
>> platform_device *pdev)
>> data->sysmmu = dev;
>> spin_lock_init(&data->lock);
>> - ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
>> - dev_name(data->sysmmu));
>> - if (ret)
>> - return ret;
>> -
>> - iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
>> - iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
>
> The iommu_device_set_fwnode() call is lost during this conversion,
> what breaks driver operation. Most of the above IOMMU fw calls you
> have moved to xlate function. I've checked briefly but it looks that
> there is a chicken-egg problem here. The owner structure is allocated
> and initialized from of_xlate(), which won't be called without linking
> the problem iommu structure with the fwnode first, what might be done
> only in sysmmu_probe(). I will check how to handle this in a different
> way.
I've played with this a bit and it looks that won't be easy to make it
working on ARM 32bit.
I need a place to initialize properly all the structures for the given
master (IOMMU client device, the one which performs DMA operations).
I tried to move all the initialization from xlate() to device_probe(),
but such approach doesn't work.
On ARM32bit exynos_iommu_device_probe() is called by the device core and
IOMMU framework very early, before the Exynos SYSMMU platform devices
(controllers) are probed yet. Even iommu class is not yet initialized
that time, so returning anything successful from it causes following
NULL ptr dereference:
Unable to handle kernel NULL pointer dereference at virtual address 0000004c
...
(__iommu_probe_device) from [<c055b334>] (iommu_probe_device+0x18/0x114)
(iommu_probe_device) from [<c055b4ac>] (iommu_bus_notifier+0x7c/0x94)
(iommu_bus_notifier) from [<c014e8ec>] (notifier_call_chain+0x44/0x84)
(notifier_call_chain) from [<c014e9ec>]
(__blocking_notifier_call_chain+0x48/0x60)
(__blocking_notifier_call_chain) from [<c014ea1c>]
(blocking_notifier_call_chain+0x18/0x20)
(blocking_notifier_call_chain) from [<c05c8d1c>] (device_add+0x3e8/0x704)
(device_add) from [<c07bafc4>] (of_platform_device_create_pdata+0x90/0xc4)
(of_platform_device_create_pdata) from [<c07bb138>]
(of_platform_bus_create+0x134/0x48c)
(of_platform_bus_create) from [<c07bb1a4>]
(of_platform_bus_create+0x1a0/0x48c)
(of_platform_bus_create) from [<c07bb63c>] (of_platform_populate+0x84/0x114)
(of_platform_populate) from [<c1125e9c>]
(of_platform_default_populate_init+0x90/0xac)
(of_platform_default_populate_init) from [<c010326c>]
(do_one_initcall+0x80/0x42c)
(do_one_initcall) from [<c1101074>] (kernel_init_freeable+0x15c/0x208)
(kernel_init_freeable) from [<c0afdde0>] (kernel_init+0x8/0x118)
(kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
I've tried returning ERR_PTR(-EPROBE_DEFER) from device_probe(), but I
doesn't work there. Some more changes in the framework are needed...
> ...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Marek,
On Wed, Apr 08, 2020 at 04:23:05PM +0200, Marek Szyprowski wrote:
> I need a place to initialize properly all the structures for the given
> master (IOMMU client device, the one which performs DMA operations).
That could be in the probe_device() call-back, no?
> I tried to move all the initialization from xlate() to device_probe(),
> but such approach doesn't work.
device_probe() is exynos_sysmmu_probe(), then yes, this is called before
any of the xlate() calls are made.
Would it work to keep the iommu_device structures in the sysmmus and
also create them for the owners? This isn't really a nice solution but
should work the the IOMMU driver until there is a better way to fix
this.
Regards,
Joerg
Hi Marek,
I had some more thoughts and discussions with Robin about how to make
this work with the Exynos driver. The result is the patch below, can you
please test it and report back? Even better if you can fix up any
breakage it might cause :)
From 60a288509baa34df6a0bf437c977925a0a617c72 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
Date: Thu, 9 Apr 2020 13:38:18 +0200
Subject: [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner'
Remove 'struct exynos_iommu_owner' and replace it with a single-linked
list of 'struct sysmmu_drvdata'. The first item in the list acts as a
replacement for the previous exynos_iommu_owner structure. The
iommu_device member of the first list item is reported to the IOMMU
core code for the master device.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/exynos-iommu.c | 155 ++++++++++++++++++++---------------
1 file changed, 88 insertions(+), 67 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 186ff5cc975c..e70eb360093f 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -225,18 +225,6 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
{ 20, REG_V5_FAULT_AW_VA, "AW SECURITY PROTECTION", IOMMU_FAULT_WRITE },
};
-/*
- * This structure is attached to dev.archdata.iommu of the master device
- * on device add, contains a list of SYSMMU controllers defined by device tree,
- * which are bound to given master device. It is usually referenced by 'owner'
- * pointer.
-*/
-struct exynos_iommu_owner {
- struct list_head controllers; /* list of sysmmu_drvdata.owner_node */
- struct iommu_domain *domain; /* domain this device is attached */
- struct mutex rpm_lock; /* for runtime pm of all sysmmus */
-};
-
/*
* This structure exynos specific generalization of struct iommu_domain.
* It contains list of SYSMMU controllers from all master devices, which has
@@ -271,13 +259,23 @@ struct sysmmu_drvdata {
bool active; /* current status */
struct exynos_iommu_domain *domain; /* domain we belong to */
struct list_head domain_node; /* node for domain clients list */
- struct list_head owner_node; /* node for owner controllers list */
+ struct sysmmu_drvdata *next; /* Single-linked list to group SMMUs for
+ one master. NULL means not in any
+ list, ERR_PTR(-ENODEV) means end of
+ list */
+ struct mutex rpm_lock; /* for runtime pm of all sysmmus */
phys_addr_t pgtable; /* assigned page table structure */
unsigned int version; /* our version */
struct iommu_device iommu; /* IOMMU core handle */
};
+/* Helper to iterate over all SYSMMUs for a given platform device */
+#define for_each_sysmmu(dev, drvdata) \
+ for (drvdata = (dev)->archdata.iommu; \
+ drvdata != ERR_PTR(-ENODEV); \
+ drvdata = drvdata->next)
+
static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
{
return container_of(dom, struct exynos_iommu_domain, domain);
@@ -624,6 +622,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
data->sysmmu = dev;
spin_lock_init(&data->lock);
+ data->next = NULL;
+ mutex_init(&data->rpm_lock);
ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
dev_name(data->sysmmu));
@@ -668,17 +668,20 @@ static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
{
struct sysmmu_drvdata *data = dev_get_drvdata(dev);
struct device *master = data->master;
+ struct sysmmu_drvdata *master_data;
- if (master) {
- struct exynos_iommu_owner *owner = master->archdata.iommu;
+ if (!master)
+ return 0;
- mutex_lock(&owner->rpm_lock);
- if (data->domain) {
- dev_dbg(data->sysmmu, "saving state\n");
- __sysmmu_disable(data);
- }
- mutex_unlock(&owner->rpm_lock);
+ master_data = master->archdata.iommu;
+
+ mutex_lock(&master_data->rpm_lock);
+ if (data->domain) {
+ dev_dbg(data->sysmmu, "saving state\n");
+ __sysmmu_disable(data);
}
+ mutex_unlock(&master_data->rpm_lock);
+
return 0;
}
@@ -686,17 +689,20 @@ static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
{
struct sysmmu_drvdata *data = dev_get_drvdata(dev);
struct device *master = data->master;
+ struct sysmmu_drvdata *master_data;
- if (master) {
- struct exynos_iommu_owner *owner = master->archdata.iommu;
+ if (!master)
+ return 0;
- mutex_lock(&owner->rpm_lock);
- if (data->domain) {
- dev_dbg(data->sysmmu, "restoring state\n");
- __sysmmu_enable(data);
- }
- mutex_unlock(&owner->rpm_lock);
+ master_data = master->archdata.iommu;
+
+ mutex_lock(&master_data->rpm_lock);
+ if (data->domain) {
+ dev_dbg(data->sysmmu, "restoring state\n");
+ __sysmmu_enable(data);
}
+ mutex_unlock(&master_data->rpm_lock);
+
return 0;
}
@@ -834,21 +840,21 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
kfree(domain);
}
-static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
- struct device *dev)
+static void __exynos_iommu_detach_device(struct exynos_iommu_domain *domain,
+ struct device *dev)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
- struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
phys_addr_t pagetable = virt_to_phys(domain->pgtable);
- struct sysmmu_drvdata *data, *next;
+ struct sysmmu_drvdata *dev_data, *data, *next;
unsigned long flags;
- if (!has_sysmmu(dev) || owner->domain != iommu_domain)
+ dev_data = dev->archdata.iommu;
+
+ if (!has_sysmmu(dev) || dev_data->domain != domain)
return;
- mutex_lock(&owner->rpm_lock);
+ mutex_lock(&dev_data->rpm_lock);
- list_for_each_entry(data, &owner->controllers, owner_node) {
+ for_each_sysmmu(dev, data) {
pm_runtime_get_noresume(data->sysmmu);
if (pm_runtime_active(data->sysmmu))
__sysmmu_disable(data);
@@ -863,51 +869,59 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
list_del_init(&data->domain_node);
spin_unlock(&data->lock);
}
- owner->domain = NULL;
spin_unlock_irqrestore(&domain->lock, flags);
- mutex_unlock(&owner->rpm_lock);
+ mutex_unlock(&dev_data->rpm_lock);
dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
&pagetable);
}
+static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
+ struct device *dev)
+{
+ struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
+
+ __exynos_iommu_detach_device(domain, dev);
+}
+
static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
struct device *dev)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
- struct sysmmu_drvdata *data;
+ struct sysmmu_drvdata *dev_data, *data;
phys_addr_t pagetable = virt_to_phys(domain->pgtable);
unsigned long flags;
if (!has_sysmmu(dev))
return -ENODEV;
- if (owner->domain)
- exynos_iommu_detach_device(owner->domain, dev);
+ dev_data = dev->archdata.iommu;
- mutex_lock(&owner->rpm_lock);
+ if (dev_data->domain)
+ __exynos_iommu_detach_device(dev_data->domain, dev);
+
+ mutex_lock(&dev_data->rpm_lock);
spin_lock_irqsave(&domain->lock, flags);
- list_for_each_entry(data, &owner->controllers, owner_node) {
+ for_each_sysmmu(dev, data) {
spin_lock(&data->lock);
data->pgtable = pagetable;
data->domain = domain;
list_add_tail(&data->domain_node, &domain->clients);
spin_unlock(&data->lock);
}
- owner->domain = iommu_domain;
spin_unlock_irqrestore(&domain->lock, flags);
- list_for_each_entry(data, &owner->controllers, owner_node) {
+
+ for_each_sysmmu(dev, data) {
pm_runtime_get_noresume(data->sysmmu);
if (pm_runtime_active(data->sysmmu))
__sysmmu_enable(data);
pm_runtime_put(data->sysmmu);
}
- mutex_unlock(&owner->rpm_lock);
+ mutex_unlock(&dev_data->rpm_lock);
dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
&pagetable);
@@ -1237,7 +1251,6 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain,
static int exynos_iommu_add_device(struct device *dev)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
struct sysmmu_drvdata *data;
struct iommu_group *group;
@@ -1249,7 +1262,7 @@ static int exynos_iommu_add_device(struct device *dev)
if (IS_ERR(group))
return PTR_ERR(group);
- list_for_each_entry(data, &owner->controllers, owner_node) {
+ for_each_sysmmu(dev, data) {
/*
* SYSMMU will be runtime activated via device link
* (dependency) to its master device, so there are no
@@ -1261,37 +1274,39 @@ static int exynos_iommu_add_device(struct device *dev)
}
iommu_group_put(group);
+ data = dev->archdata.iommu;
+ iommu_device_link(&data->iommu, dev);
+
return 0;
}
static void exynos_iommu_remove_device(struct device *dev)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
- struct sysmmu_drvdata *data;
+ struct sysmmu_drvdata *data = dev->archdata.iommu;
if (!has_sysmmu(dev))
return;
- if (owner->domain) {
+ if (data->domain) {
struct iommu_group *group = iommu_group_get(dev);
if (group) {
- WARN_ON(owner->domain !=
+ WARN_ON(&data->domain->domain !=
iommu_group_default_domain(group));
- exynos_iommu_detach_device(owner->domain, dev);
+ __exynos_iommu_detach_device(data->domain, dev);
iommu_group_put(group);
}
}
+ iommu_device_unlink(&data->iommu, dev);
iommu_group_remove_device(dev);
- list_for_each_entry(data, &owner->controllers, owner_node)
+ for_each_sysmmu(dev, data)
device_link_del(data->link);
}
static int exynos_iommu_of_xlate(struct device *dev,
struct of_phandle_args *spec)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
struct platform_device *sysmmu = of_find_device_by_node(spec->np);
struct sysmmu_drvdata *data, *entry;
@@ -1302,22 +1317,28 @@ static int exynos_iommu_of_xlate(struct device *dev,
if (!data)
return -ENODEV;
- if (!owner) {
- owner = kzalloc(sizeof(*owner), GFP_KERNEL);
- if (!owner)
- return -ENOMEM;
+ data->master = dev;
- INIT_LIST_HEAD(&owner->controllers);
- mutex_init(&owner->rpm_lock);
- dev->archdata.iommu = owner;
+ if (!dev->archdata.iommu) {
+ WARN_ON(data->next != NULL);
+
+ /* SYSMMU list is empty - add drvdata and return */
+ data->next = ERR_PTR(-ENODEV);
+ dev->archdata.iommu = data;
+
+ return 0;
}
- list_for_each_entry(entry, &owner->controllers, owner_node)
+ /* Check if SYSMMU is already in the list */
+ for_each_sysmmu(dev, entry)
if (entry == data)
return 0;
- list_add_tail(&data->owner_node, &owner->controllers);
- data->master = dev;
+ /* Not in the list yet */
+ WARN_ON(data->next != NULL);
+ entry = dev->archdata.iommu;
+ data->next = entry->next;
+ entry->next = data;
return 0;
}
--
2.25.1
Hi Marek,
On Thu, Apr 09, 2020 at 03:58:00PM +0200, Marek Szyprowski wrote:
> The main problem after your conversion is the fact that ->probe_device()
> is called very early, before any other platform device (thus IOMMU
> controller) is is probed. It doesn't handle EPROBE_DEFER too.
I don't quite understand why probe_device() is called too early, as it
is called at the same time add_device() was called before. But anyway,
I have seen a similar problem on OMAP. If the SYSMMU for a master is not
probed yet when probe_device() is called, it can just return -ENODEV and
in your driver you just call but_iommu_probe() when a new SYSMMU got
initialized to re-probe uninitialized masters on the bus. This patch-set
contains a change to export bus_iommu_probe() for exactly that reason.
What do you think?
Regards,
Joerg
Hi Joerg,
On 09.04.2020 13:46, Joerg Roedel wrote:
> Hi Marek,
>
> I had some more thoughts and discussions with Robin about how to make
> this work with the Exynos driver. The result is the patch below, can you
> please test it and report back? Even better if you can fix up any
> breakage it might cause :)
I've checked and it works fine on top of
ff68eb23308e6538ec7864c83d39540f423bbe90. However I'm not a fan of
removing this 'owner' structure. It gave a nice abstraction for the all
SYSMMU controllers for the given device (although most devices in the
system have only one SYSMMU). Why this structure is a problem for your
rework?
I've also spent some time trying to fix exynos-iommu on top of your
iommu-probe-device branch. I really wonder if it works on any ARM 32bit
or 64bit systems for other IOMMUs.
I got something working on ARM32bit, but I have to move all the
initialization from exynos_iommu_probe_device/exynos_iommu_of_xlate to
exynos_sysmmu_probe(). I don't like such approach, because I had to use
bus_find_device() and manually find the owner for the every SYSMMU
controller in the system. This approach also lack a proper symmetry and
release path.
The main problem after your conversion is the fact that ->probe_device()
is called very early, before any other platform device (thus IOMMU
controller) is is probed. It doesn't handle EPROBE_DEFER too.
The other issue I've noticed is that iommu_device_set_ops() is not
enough to assign ops properly. I had to add iommu_fwspec_init(dev,
&dev->of_node->fwnode, &exynos_iommu_ops) to ensure that the 'dev' gets
proper iommu ops.
I will send my patch in a few minutes to show you the changes.
> >From 60a288509baa34df6a0bf437c977925a0a617c72 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <[email protected]>
> Date: Thu, 9 Apr 2020 13:38:18 +0200
> Subject: [PATCH] iommu/exynos: Get rid of 'struct exynos_iommu_owner'
>
> Remove 'struct exynos_iommu_owner' and replace it with a single-linked
> list of 'struct sysmmu_drvdata'. The first item in the list acts as a
> replacement for the previous exynos_iommu_owner structure. The
> iommu_device member of the first list item is reported to the IOMMU
> core code for the master device.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/exynos-iommu.c | 155 ++++++++++++++++++++---------------
> 1 file changed, 88 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 186ff5cc975c..e70eb360093f 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -225,18 +225,6 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
> { 20, REG_V5_FAULT_AW_VA, "AW SECURITY PROTECTION", IOMMU_FAULT_WRITE },
> };
>
> -/*
> - * This structure is attached to dev.archdata.iommu of the master device
> - * on device add, contains a list of SYSMMU controllers defined by device tree,
> - * which are bound to given master device. It is usually referenced by 'owner'
> - * pointer.
> -*/
> -struct exynos_iommu_owner {
> - struct list_head controllers; /* list of sysmmu_drvdata.owner_node */
> - struct iommu_domain *domain; /* domain this device is attached */
> - struct mutex rpm_lock; /* for runtime pm of all sysmmus */
> -};
> -
> /*
> * This structure exynos specific generalization of struct iommu_domain.
> * It contains list of SYSMMU controllers from all master devices, which has
> @@ -271,13 +259,23 @@ struct sysmmu_drvdata {
> bool active; /* current status */
> struct exynos_iommu_domain *domain; /* domain we belong to */
> struct list_head domain_node; /* node for domain clients list */
> - struct list_head owner_node; /* node for owner controllers list */
> + struct sysmmu_drvdata *next; /* Single-linked list to group SMMUs for
> + one master. NULL means not in any
> + list, ERR_PTR(-ENODEV) means end of
> + list */
> + struct mutex rpm_lock; /* for runtime pm of all sysmmus */
> phys_addr_t pgtable; /* assigned page table structure */
> unsigned int version; /* our version */
>
> struct iommu_device iommu; /* IOMMU core handle */
> };
>
> +/* Helper to iterate over all SYSMMUs for a given platform device */
> +#define for_each_sysmmu(dev, drvdata) \
> + for (drvdata = (dev)->archdata.iommu; \
> + drvdata != ERR_PTR(-ENODEV); \
> + drvdata = drvdata->next)
> +
> static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> {
> return container_of(dom, struct exynos_iommu_domain, domain);
> @@ -624,6 +622,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>
> data->sysmmu = dev;
> spin_lock_init(&data->lock);
> + data->next = NULL;
> + mutex_init(&data->rpm_lock);
>
> ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
> dev_name(data->sysmmu));
> @@ -668,17 +668,20 @@ static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
> {
> struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> struct device *master = data->master;
> + struct sysmmu_drvdata *master_data;
>
> - if (master) {
> - struct exynos_iommu_owner *owner = master->archdata.iommu;
> + if (!master)
> + return 0;
>
> - mutex_lock(&owner->rpm_lock);
> - if (data->domain) {
> - dev_dbg(data->sysmmu, "saving state\n");
> - __sysmmu_disable(data);
> - }
> - mutex_unlock(&owner->rpm_lock);
> + master_data = master->archdata.iommu;
> +
> + mutex_lock(&master_data->rpm_lock);
> + if (data->domain) {
> + dev_dbg(data->sysmmu, "saving state\n");
> + __sysmmu_disable(data);
> }
> + mutex_unlock(&master_data->rpm_lock);
> +
> return 0;
> }
>
> @@ -686,17 +689,20 @@ static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
> {
> struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> struct device *master = data->master;
> + struct sysmmu_drvdata *master_data;
>
> - if (master) {
> - struct exynos_iommu_owner *owner = master->archdata.iommu;
> + if (!master)
> + return 0;
>
> - mutex_lock(&owner->rpm_lock);
> - if (data->domain) {
> - dev_dbg(data->sysmmu, "restoring state\n");
> - __sysmmu_enable(data);
> - }
> - mutex_unlock(&owner->rpm_lock);
> + master_data = master->archdata.iommu;
> +
> + mutex_lock(&master_data->rpm_lock);
> + if (data->domain) {
> + dev_dbg(data->sysmmu, "restoring state\n");
> + __sysmmu_enable(data);
> }
> + mutex_unlock(&master_data->rpm_lock);
> +
> return 0;
> }
>
> @@ -834,21 +840,21 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
> kfree(domain);
> }
>
> -static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> - struct device *dev)
> +static void __exynos_iommu_detach_device(struct exynos_iommu_domain *domain,
> + struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> - struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> - struct sysmmu_drvdata *data, *next;
> + struct sysmmu_drvdata *dev_data, *data, *next;
> unsigned long flags;
>
> - if (!has_sysmmu(dev) || owner->domain != iommu_domain)
> + dev_data = dev->archdata.iommu;
> +
> + if (!has_sysmmu(dev) || dev_data->domain != domain)
> return;
>
> - mutex_lock(&owner->rpm_lock);
> + mutex_lock(&dev_data->rpm_lock);
>
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> + for_each_sysmmu(dev, data) {
> pm_runtime_get_noresume(data->sysmmu);
> if (pm_runtime_active(data->sysmmu))
> __sysmmu_disable(data);
> @@ -863,51 +869,59 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> list_del_init(&data->domain_node);
> spin_unlock(&data->lock);
> }
> - owner->domain = NULL;
> spin_unlock_irqrestore(&domain->lock, flags);
>
> - mutex_unlock(&owner->rpm_lock);
> + mutex_unlock(&dev_data->rpm_lock);
>
> dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
> &pagetable);
> }
>
> +static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
> + struct device *dev)
> +{
> + struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> +
> + __exynos_iommu_detach_device(domain, dev);
> +}
> +
> static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
> struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct exynos_iommu_domain *domain = to_exynos_domain(iommu_domain);
> - struct sysmmu_drvdata *data;
> + struct sysmmu_drvdata *dev_data, *data;
> phys_addr_t pagetable = virt_to_phys(domain->pgtable);
> unsigned long flags;
>
> if (!has_sysmmu(dev))
> return -ENODEV;
>
> - if (owner->domain)
> - exynos_iommu_detach_device(owner->domain, dev);
> + dev_data = dev->archdata.iommu;
>
> - mutex_lock(&owner->rpm_lock);
> + if (dev_data->domain)
> + __exynos_iommu_detach_device(dev_data->domain, dev);
> +
> + mutex_lock(&dev_data->rpm_lock);
>
> spin_lock_irqsave(&domain->lock, flags);
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> + for_each_sysmmu(dev, data) {
> spin_lock(&data->lock);
> data->pgtable = pagetable;
> data->domain = domain;
> list_add_tail(&data->domain_node, &domain->clients);
> spin_unlock(&data->lock);
> }
> - owner->domain = iommu_domain;
> spin_unlock_irqrestore(&domain->lock, flags);
>
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> +
> + for_each_sysmmu(dev, data) {
> pm_runtime_get_noresume(data->sysmmu);
> if (pm_runtime_active(data->sysmmu))
> __sysmmu_enable(data);
> pm_runtime_put(data->sysmmu);
> }
>
> - mutex_unlock(&owner->rpm_lock);
> + mutex_unlock(&dev_data->rpm_lock);
>
> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
> &pagetable);
> @@ -1237,7 +1251,6 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain,
>
> static int exynos_iommu_add_device(struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct sysmmu_drvdata *data;
> struct iommu_group *group;
>
> @@ -1249,7 +1262,7 @@ static int exynos_iommu_add_device(struct device *dev)
> if (IS_ERR(group))
> return PTR_ERR(group);
>
> - list_for_each_entry(data, &owner->controllers, owner_node) {
> + for_each_sysmmu(dev, data) {
> /*
> * SYSMMU will be runtime activated via device link
> * (dependency) to its master device, so there are no
> @@ -1261,37 +1274,39 @@ static int exynos_iommu_add_device(struct device *dev)
> }
> iommu_group_put(group);
>
> + data = dev->archdata.iommu;
> + iommu_device_link(&data->iommu, dev);
> +
> return 0;
> }
>
> static void exynos_iommu_remove_device(struct device *dev)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> - struct sysmmu_drvdata *data;
> + struct sysmmu_drvdata *data = dev->archdata.iommu;
>
> if (!has_sysmmu(dev))
> return;
>
> - if (owner->domain) {
> + if (data->domain) {
> struct iommu_group *group = iommu_group_get(dev);
>
> if (group) {
> - WARN_ON(owner->domain !=
> + WARN_ON(&data->domain->domain !=
> iommu_group_default_domain(group));
> - exynos_iommu_detach_device(owner->domain, dev);
> + __exynos_iommu_detach_device(data->domain, dev);
> iommu_group_put(group);
> }
> }
> + iommu_device_unlink(&data->iommu, dev);
> iommu_group_remove_device(dev);
>
> - list_for_each_entry(data, &owner->controllers, owner_node)
> + for_each_sysmmu(dev, data)
> device_link_del(data->link);
> }
>
> static int exynos_iommu_of_xlate(struct device *dev,
> struct of_phandle_args *spec)
> {
> - struct exynos_iommu_owner *owner = dev->archdata.iommu;
> struct platform_device *sysmmu = of_find_device_by_node(spec->np);
> struct sysmmu_drvdata *data, *entry;
>
> @@ -1302,22 +1317,28 @@ static int exynos_iommu_of_xlate(struct device *dev,
> if (!data)
> return -ENODEV;
>
> - if (!owner) {
> - owner = kzalloc(sizeof(*owner), GFP_KERNEL);
> - if (!owner)
> - return -ENOMEM;
> + data->master = dev;
>
> - INIT_LIST_HEAD(&owner->controllers);
> - mutex_init(&owner->rpm_lock);
> - dev->archdata.iommu = owner;
> + if (!dev->archdata.iommu) {
> + WARN_ON(data->next != NULL);
> +
> + /* SYSMMU list is empty - add drvdata and return */
> + data->next = ERR_PTR(-ENODEV);
> + dev->archdata.iommu = data;
> +
> + return 0;
> }
>
> - list_for_each_entry(entry, &owner->controllers, owner_node)
> + /* Check if SYSMMU is already in the list */
> + for_each_sysmmu(dev, entry)
> if (entry == data)
> return 0;
>
> - list_add_tail(&data->owner_node, &owner->controllers);
> - data->master = dev;
> + /* Not in the list yet */
> + WARN_ON(data->next != NULL);
> + entry = dev->archdata.iommu;
> + data->next = entry->next;
> + entry->next = data;
>
> return 0;
> }
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Fix initialization after driver conversion to
probe_device()/release_device(). Prepared on top of:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/linux.git/log/?h=iommu-probe-device
Signed-off-by: Marek Szyprowski <[email protected]>
---
drivers/iommu/exynos-iommu.c | 80 +++++++++++++++++++++++++-------------------
1 file changed, 46 insertions(+), 34 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index f865c90..53c784f 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -565,6 +565,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
}
static const struct iommu_ops exynos_iommu_ops;
+static int exynos_iommu_initialize_owner(struct device *sysmmu);
static int exynos_sysmmu_probe(struct platform_device *pdev)
{
@@ -573,6 +574,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
struct sysmmu_drvdata *data;
struct resource *res;
+ dev_info(dev, "%s %d\n", __func__, __LINE__);
+
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
@@ -649,6 +652,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
+ exynos_iommu_initialize_owner(dev);
+
return 0;
}
@@ -1225,24 +1230,8 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain,
static struct iommu_device *exynos_iommu_probe_device(struct device *dev)
{
- struct exynos_iommu_owner *owner = dev->archdata.iommu;
- struct sysmmu_drvdata *data;
-
- if (!has_sysmmu(dev))
- return ERR_PTR(-ENODEV);
-
- list_for_each_entry(data, &owner->controllers, owner_node) {
- /*
- * SYSMMU will be runtime activated via device link
- * (dependency) to its master device, so there are no
- * direct calls to pm_runtime_get/put in this driver.
- */
- data->link = device_link_add(dev, data->sysmmu,
- DL_FLAG_STATELESS |
- DL_FLAG_PM_RUNTIME);
- }
-
- return &owner->iommu;
+ /* this is called too early on ARM 32bit to do anything usefull */
+ return ERR_PTR(-ENODEV);
}
static void exynos_iommu_release_device(struct device *dev)
@@ -1268,7 +1257,8 @@ static void exynos_iommu_release_device(struct device *dev)
device_link_del(data->link);
}
-static int exynos_iommu_device_init(struct exynos_iommu_owner *owner)
+static int exynos_iommu_device_init(struct device *dev,
+ struct exynos_iommu_owner *owner)
{
static u32 counter = 0;
int ret;
@@ -1287,6 +1277,12 @@ static int exynos_iommu_device_init(struct exynos_iommu_owner *owner)
iommu_device_set_ops(&owner->iommu, &exynos_iommu_ops);
+ /*
+ * the above iommu_device_set_ops is not enough, initializing fwspec
+ * is also required
+ */
+ iommu_fwspec_init(dev, &dev->of_node->fwnode, &exynos_iommu_ops);
+
return 0;
}
@@ -1308,7 +1304,7 @@ static int exynos_owner_init(struct device *dev)
if (!owner)
return -ENOMEM;
- ret = exynos_iommu_device_init(owner);
+ ret = exynos_iommu_device_init(dev, owner);
if (ret)
goto out_free_owner;
@@ -1330,34 +1326,51 @@ static int exynos_owner_init(struct device *dev)
return ret;
}
-static int exynos_iommu_of_xlate(struct device *dev,
- struct of_phandle_args *spec)
+static int exynos_iommu_dev_match_owner(struct device *dev, const void *data)
+{
+ const struct device *sysmmu = data;
+ struct device_node *np;
+ int idx = 0;
+
+ do {
+ np = of_parse_phandle(dev->of_node, "iommus", idx++);
+ if (np == sysmmu->of_node)
+ return true;
+ } while (np);
+
+ return false;
+}
+
+static int exynos_iommu_initialize_owner(struct device *sysmmu)
{
- struct platform_device *sysmmu = of_find_device_by_node(spec->np);
- struct sysmmu_drvdata *data, *entry;
+ struct sysmmu_drvdata *data = dev_get_drvdata(sysmmu);
struct exynos_iommu_owner *owner;
+ struct device *dev;
int ret;
- if (!sysmmu)
+ dev = bus_find_device(&platform_bus_type, NULL, sysmmu,
+ exynos_iommu_dev_match_owner);
+ if (!dev)
return -ENODEV;
- data = platform_get_drvdata(sysmmu);
- if (!data)
- return -ENODEV;
+ dev_info(sysmmu, "found master device %s\n", dev_name(dev));
ret = exynos_owner_init(dev);
if (ret)
return ret;
owner = dev->archdata.iommu;
-
- list_for_each_entry(entry, &owner->controllers, owner_node)
- if (entry == data)
- return 0;
-
list_add_tail(&data->owner_node, &owner->controllers);
data->master = dev;
+ /*
+ * SYSMMU will be runtime activated via device link
+ * (dependency) to its master device, so there are no
+ * direct calls to pm_runtime_get/put in this driver.
+ */
+ data->link = device_link_add(dev, data->sysmmu,
+ DL_FLAG_STATELESS |
+ DL_FLAG_PM_RUNTIME);
return 0;
}
@@ -1373,7 +1386,6 @@ static int exynos_iommu_of_xlate(struct device *dev,
.probe_device = exynos_iommu_probe_device,
.release_device = exynos_iommu_release_device,
.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
- .of_xlate = exynos_iommu_of_xlate,
};
static int __init exynos_iommu_init(void)
--
1.9.1
On Thu, Apr 09, 2020 at 03:58:00PM +0200, Marek Szyprowski wrote:
> I've checked and it works fine on top of
> ff68eb23308e6538ec7864c83d39540f423bbe90. However I'm not a fan of
> removing this 'owner' structure. It gave a nice abstraction for the all
> SYSMMU controllers for the given device (although most devices in the
> system have only one SYSMMU). Why this structure is a problem for your
> rework?
Okay, the structure itself is not a problem, I just thought it is not
really necessary. But to keep things simple I've taken another approach
for v2 of this series: Just use the first SYSMMU of the controllers list
to link the device and the IOMMU. When the owner structure exists there
is always one entry in this list, so that should work fine.
Regards,
Joerg