diff mbox

[v2,12/13] iommu/rockchip: Add runtime PM support

Message ID 20180116132540.18939-13-jeffy.chen@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeffy Chen Jan. 16, 2018, 1:25 p.m. UTC
When the power domain is powered off, the IOMMU cannot be accessed and
register programming must be deferred until the power domain becomes
enabled.

Add runtime PM support, and use runtime PM device link from IOMMU to
master to startup and shutdown IOMMU.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v2: None

 drivers/iommu/rockchip-iommu.c | 191 ++++++++++++++++++++++++++++++-----------
 1 file changed, 143 insertions(+), 48 deletions(-)

Comments

Tomasz Figa Jan. 17, 2018, 6:20 a.m. UTC | #1
On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> When the power domain is powered off, the IOMMU cannot be accessed and
> register programming must be deferred until the power domain becomes
> enabled.
>
> Add runtime PM support, and use runtime PM device link from IOMMU to
> master to startup and shutdown IOMMU.
[snip]
> @@ -875,28 +889,19 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
>
>  static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
>  {
> -       return dev->archdata.iommu;
> +       struct rk_iommudata *data = dev->archdata.iommu;
> +
> +       return data ? data->iommu : NULL;
>  }

Is this change intentionally added to this patch? I see this
potentially relevant for the previous patch in this series.

[snip]
> +static int rk_iommu_startup(struct rk_iommu *iommu)
>  {
> -       struct rk_iommu *iommu;
> +       struct iommu_domain *domain = iommu->domain;
>         struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> -       unsigned long flags;
>         int ret, i;
>
> -       /*
> -        * Allow 'virtual devices' (e.g., drm) to attach to domain.
> -        * Such a device does not belong to an iommu group.
> -        */
> -       iommu = rk_iommu_from_dev(dev);
> -       if (!iommu)
> -               return 0;
> -
> -       if (iommu->domain)
> -               rk_iommu_detach_device(iommu->domain, dev);
> -
>         ret = rk_iommu_enable_clocks(iommu);
>         if (ret)
>                 return ret;
>

Don't we need to check here (and in _shutdown() too) if we have a
domain attached?

> +       mutex_lock(&iommu->pm_mutex);
>         ret = rk_iommu_enable_stall(iommu);
>         if (ret)
> -               goto err_disable_clocks;
> +               goto err_unlock_mutex;
[snip]
> +       iommu->domain = NULL;
> +
> +       spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> +       list_del_init(&iommu->node);
> +       spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
> +
> +       if (pm_runtime_get_if_in_use(iommu->dev) > 0) {

Actually, if the above call returns -EINVAL, don't we still need to
call rk_iommu_shutdown(), because it just means runtime PM is disabled
and the IOMMU is always powered on?

> +               rk_iommu_shutdown(iommu);
> +               pm_runtime_put(iommu->dev);
> +       }
> +}
[snip]
> +       spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> +       list_add_tail(&iommu->node, &rk_domain->iommus);
> +       spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
> +
> +       if (pm_runtime_get_if_in_use(iommu->dev) <= 0)

Ditto.

> +               return 0;
> +
> +       ret = rk_iommu_startup(iommu);
> +       if (ret)
> +               rk_iommu_detach_device(data->domain, dev);
[snip]
> @@ -1108,7 +1175,9 @@ static int rk_iommu_of_xlate(struct device *dev,
>                 return -ENODEV;
>         }
>
> -       dev->archdata.iommu = platform_get_drvdata(iommu_dev);
> +       data->iommu = platform_get_drvdata(iommu_dev);
> +       dev->archdata.iommu = data;
> +

I think this change might be mistakenly squashed to this patch instead
of previous.

Best regards,
Tomasz
Tomasz Figa Jan. 17, 2018, 7:38 a.m. UTC | #2
On Wed, Jan 17, 2018 at 4:26 PM, JeffyChen <jeffy.chen@rock-chips.com> wrote:
> Hi Tomasz,
>
> Thanks for your reply.
>
> On 01/17/2018 02:20 PM, Tomasz Figa wrote:
>>
>> On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen <jeffy.chen@rock-chips.com>
>> [snip]
>>>
>>> +static int rk_iommu_startup(struct rk_iommu *iommu)
>>>   {
>>> -       struct rk_iommu *iommu;
>>> +       struct iommu_domain *domain = iommu->domain;
>>>          struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
>>> -       unsigned long flags;
>>>          int ret, i;
>>>
>>> -       /*
>>> -        * Allow 'virtual devices' (e.g., drm) to attach to domain.
>>> -        * Such a device does not belong to an iommu group.
>>> -        */
>>> -       iommu = rk_iommu_from_dev(dev);
>>> -       if (!iommu)
>>> -               return 0;
>>> -
>>> -       if (iommu->domain)
>>> -               rk_iommu_detach_device(iommu->domain, dev);
>>> -
>>>          ret = rk_iommu_enable_clocks(iommu);
>>>          if (ret)
>>>                  return ret;
>>>
>>
>> Don't we need to check here (and in _shutdown() too) if we have a
>> domain attached?
>
> hmmm, right, the startup might been called by resume, so should check
> iommu->domain here.
>
> but the shutdown would be called at the end of detach or suspend, it could
> be not attached or attached.

If startup might be called by resume, without domain attached, what
prevents shutdown from being called by suspend after that resume,
still without domain attached? Is it guaranteed that if resume is
called, someone will attach a domain before suspend is called?

Best regards,
Tomasz
Jeffy Chen Jan. 17, 2018, 7:52 a.m. UTC | #3
Hi Tomasz,

On 01/17/2018 03:38 PM, Tomasz Figa wrote:
>>> >>Don't we need to check here (and in _shutdown() too) if we have a
>>> >>domain attached?
>> >
>> >hmmm, right, the startup might been called by resume, so should check
>> >iommu->domain here.
>> >
>> >but the shutdown would be called at the end of detach or suspend, it could
>> >be not attached or attached.
> If startup might be called by resume, without domain attached, what
> prevents shutdown from being called by suspend after that resume,
> still without domain attached? Is it guaranteed that if resume is
> called, someone will attach a domain before suspend is called?
>
no, the shutdown would be called by:
1/ end of detach_dev
so it would be not attached at that time

2/ suspend
so it could be attached, and also could be not attached


anyway, i think the shutdown would work without domain attached(just 
disable paging and clear the iommu bases) ;)

> Best regards,
> Tomasz
>
>
>
Jeffy Chen Jan. 17, 2018, 8:48 a.m. UTC | #4
Hi Tomasz,

On 01/17/2018 03:52 PM, JeffyChen wrote:
> Hi Tomasz,
>
> On 01/17/2018 03:38 PM, Tomasz Figa wrote:
>>>> >>Don't we need to check here (and in _shutdown() too) if we have a
>>>> >>domain attached?
>>> >
>>> >hmmm, right, the startup might been called by resume, so should check
>>> >iommu->domain here.
>>> >
>>> >but the shutdown would be called at the end of detach or suspend, it
>>> could
>>> >be not attached or attached.
>> If startup might be called by resume, without domain attached, what
>> prevents shutdown from being called by suspend after that resume,
>> still without domain attached? Is it guaranteed that if resume is
>> called, someone will attach a domain before suspend is called?
>>
> no, the shutdown would be called by:
> 1/ end of detach_dev
> so it would be not attached at that time
>
> 2/ suspend
> so it could be attached, and also could be not attached
>
>
> anyway, i think the shutdown would work without domain attached(just
> disable paging and clear the iommu bases) ;)
>
hmmm, i see the problem.

so we need to:
1/ move shutdown a little earlier in detach_dev, so it could still see 
the iommu->domain

2/ check iommu->domain in shutdown, to prevent unnecessary shutdown


or maybe just add iommu->domain check in suspend and resume.
>> Best regards,
>> Tomasz
>>
>>
>>
>
diff mbox

Patch

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index c2d3ac82184e..c8de1456a016 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -18,11 +18,13 @@ 
 #include <linux/list.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -98,6 +100,12 @@  struct rk_iommu {
 	struct iommu_device iommu;
 	struct list_head node; /* entry in rk_iommu_domain.iommus */
 	struct iommu_domain *domain; /* domain to which iommu is attached */
+	struct mutex pm_mutex; /* serializes power transitions */
+};
+
+struct rk_iommudata {
+	struct device_link *link; /* runtime PM link from IOMMU to master */
+	struct rk_iommu *iommu;
 };
 
 static struct device *dma_dev;
@@ -675,9 +683,15 @@  static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain,
 	list_for_each(pos, &rk_domain->iommus) {
 		struct rk_iommu *iommu;
 		iommu = list_entry(pos, struct rk_iommu, node);
-		rk_iommu_enable_clocks(iommu);
-		rk_iommu_zap_lines(iommu, iova, size);
-		rk_iommu_disable_clocks(iommu);
+
+		/* Only zap TLBs of IOMMUs that are powered on. */
+		if (pm_runtime_get_if_in_use(iommu->dev) > 0) {
+			rk_iommu_enable_clocks(iommu);
+			rk_iommu_zap_lines(iommu, iova, size);
+			rk_iommu_disable_clocks(iommu);
+
+			pm_runtime_put(iommu->dev);
+		}
 	}
 	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
 }
@@ -875,28 +889,19 @@  static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
 
 static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
 {
-	return dev->archdata.iommu;
+	struct rk_iommudata *data = dev->archdata.iommu;
+
+	return data ? data->iommu : NULL;
 }
 
-static void rk_iommu_detach_device(struct iommu_domain *domain,
-				   struct device *dev)
+static void rk_iommu_shutdown(struct rk_iommu *iommu)
 {
-	struct rk_iommu *iommu;
-	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
-	unsigned long flags;
 	int i;
 
-	/* Allow 'virtual devices' (eg drm) to detach from domain */
-	iommu = rk_iommu_from_dev(dev);
-	if (!iommu)
-		return;
-
-	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
-	list_del_init(&iommu->node);
-	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
-
 	/* Ignore error while disabling, just keep going */
 	WARN_ON(rk_iommu_enable_clocks(iommu));
+
+	mutex_lock(&iommu->pm_mutex);
 	rk_iommu_enable_stall(iommu);
 	rk_iommu_disable_paging(iommu);
 	for (i = 0; i < iommu->num_mmu; i++) {
@@ -904,46 +909,30 @@  static void rk_iommu_detach_device(struct iommu_domain *domain,
 		rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, 0);
 	}
 	rk_iommu_disable_stall(iommu);
-	rk_iommu_disable_clocks(iommu);
+	mutex_unlock(&iommu->pm_mutex);
 
-	iommu->domain = NULL;
-
-	dev_dbg(dev, "Detached from iommu domain\n");
+	rk_iommu_disable_clocks(iommu);
 }
 
-static int rk_iommu_attach_device(struct iommu_domain *domain,
-				  struct device *dev)
+static int rk_iommu_startup(struct rk_iommu *iommu)
 {
-	struct rk_iommu *iommu;
+	struct iommu_domain *domain = iommu->domain;
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
-	unsigned long flags;
 	int ret, i;
 
-	/*
-	 * Allow 'virtual devices' (e.g., drm) to attach to domain.
-	 * Such a device does not belong to an iommu group.
-	 */
-	iommu = rk_iommu_from_dev(dev);
-	if (!iommu)
-		return 0;
-
-	if (iommu->domain)
-		rk_iommu_detach_device(iommu->domain, dev);
-
 	ret = rk_iommu_enable_clocks(iommu);
 	if (ret)
 		return ret;
 
+	mutex_lock(&iommu->pm_mutex);
 	ret = rk_iommu_enable_stall(iommu);
 	if (ret)
-		goto err_disable_clocks;
+		goto err_unlock_mutex;
 
 	ret = rk_iommu_force_reset(iommu);
 	if (ret)
 		goto err_disable_stall;
 
-	iommu->domain = domain;
-
 	for (i = 0; i < iommu->num_mmu; i++) {
 		rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
 			       rk_domain->dt_dma);
@@ -955,21 +944,90 @@  static int rk_iommu_attach_device(struct iommu_domain *domain,
 	if (ret)
 		goto err_disable_stall;
 
-	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
-	list_add_tail(&iommu->node, &rk_domain->iommus);
-	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
-
-	dev_dbg(dev, "Attached to iommu domain\n");
-
 	rk_iommu_disable_stall(iommu);
+	mutex_unlock(&iommu->pm_mutex);
+
 	rk_iommu_disable_clocks(iommu);
 
 	return 0;
 
 err_disable_stall:
 	rk_iommu_disable_stall(iommu);
-err_disable_clocks:
+err_unlock_mutex:
+	mutex_unlock(&iommu->pm_mutex);
 	rk_iommu_disable_clocks(iommu);
+	return ret;
+}
+
+static void rk_iommu_detach_device(struct iommu_domain *domain,
+				   struct device *dev)
+{
+	struct rk_iommu *iommu;
+	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
+	unsigned long flags;
+
+	/* Allow 'virtual devices' (eg drm) to detach from domain */
+	iommu = rk_iommu_from_dev(dev);
+	if (!iommu)
+		return;
+
+	dev_dbg(dev, "Detaching from iommu domain\n");
+
+	/* iommu already detached */
+	if (iommu->domain != domain)
+		return;
+
+	iommu->domain = NULL;
+
+	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
+	list_del_init(&iommu->node);
+	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
+
+	if (pm_runtime_get_if_in_use(iommu->dev) > 0) {
+		rk_iommu_shutdown(iommu);
+		pm_runtime_put(iommu->dev);
+	}
+}
+
+static int rk_iommu_attach_device(struct iommu_domain *domain,
+		struct device *dev)
+{
+	struct rk_iommu *iommu;
+	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * Allow 'virtual devices' (e.g., drm) to attach to domain.
+	 * Such a device does not belong to an iommu group.
+	 */
+	iommu = rk_iommu_from_dev(dev);
+	if (!iommu)
+		return 0;
+
+	dev_dbg(dev, "Attaching to iommu domain\n");
+
+	/* iommu already attached */
+	if (iommu->domain == domain)
+		return 0;
+
+	if (iommu->domain)
+		rk_iommu_detach_device(iommu->domain, dev);
+
+	iommu->domain = domain;
+
+	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
+	list_add_tail(&iommu->node, &rk_domain->iommus);
+	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
+
+	if (pm_runtime_get_if_in_use(iommu->dev) <= 0)
+		return 0;
+
+	ret = rk_iommu_startup(iommu);
+	if (ret)
+		rk_iommu_detach_device(data->domain, dev);
+
+	pm_runtime_put(iommu->dev);
 
 	return ret;
 }
@@ -1059,6 +1117,7 @@  static int rk_iommu_add_device(struct device *dev)
 {
 	struct iommu_group *group;
 	struct rk_iommu *iommu;
+	struct rk_iommudata *data = dev->archdata.iommu;
 
 	iommu = rk_iommu_from_dev(dev);
 	if (!iommu)
@@ -1070,6 +1129,7 @@  static int rk_iommu_add_device(struct device *dev)
 	iommu_group_put(group);
 
 	iommu_device_link(&iommu->iommu, dev);
+	data->link = device_link_add(dev, iommu->dev, DL_FLAG_PM_RUNTIME);
 
 	return 0;
 }
@@ -1077,11 +1137,13 @@  static int rk_iommu_add_device(struct device *dev)
 static void rk_iommu_remove_device(struct device *dev)
 {
 	struct rk_iommu *iommu;
+	struct rk_iommudata *data = dev->archdata.iommu;
 
 	iommu = rk_iommu_from_dev(dev);
 	if (!iommu)
 		return;
 
+	device_link_del(data->link);
 	iommu_device_unlink(&iommu->iommu, dev);
 	iommu_group_remove_device(dev);
 }
@@ -1101,6 +1163,11 @@  static int rk_iommu_of_xlate(struct device *dev,
 			     struct of_phandle_args *args)
 {
 	struct platform_device *iommu_dev;
+	struct rk_iommudata *data;
+
+	data = devm_kzalloc(dma_dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
 	iommu_dev = of_find_device_by_node(args->np);
 	if (!iommu_dev) {
@@ -1108,7 +1175,9 @@  static int rk_iommu_of_xlate(struct device *dev,
 		return -ENODEV;
 	}
 
-	dev->archdata.iommu = platform_get_drvdata(iommu_dev);
+	data->iommu = platform_get_drvdata(iommu_dev);
+	dev->archdata.iommu = data;
+
 	of_dev_put(iommu_dev);
 
 	return 0;
@@ -1204,6 +1273,8 @@  static int rk_iommu_probe(struct platform_device *pdev)
 
 	bus_set_iommu(&platform_bus_type, &rk_iommu_ops);
 
+	pm_runtime_enable(dev);
+
 	return 0;
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
@@ -1216,6 +1287,8 @@  static int rk_iommu_remove(struct platform_device *pdev)
 {
 	struct rk_iommu *iommu = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&pdev->dev);
+
 	iommu_device_unregister(&iommu->iommu);
 	iommu_device_sysfs_remove(&iommu->iommu);
 	rk_iommu_put_clocks(iommu);
@@ -1223,6 +1296,27 @@  static int rk_iommu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused rk_iommu_suspend(struct device *dev)
+{
+	struct rk_iommu *iommu = dev_get_drvdata(dev);
+
+	rk_iommu_shutdown(iommu);
+	return 0;
+}
+
+static int __maybe_unused rk_iommu_resume(struct device *dev)
+{
+	struct rk_iommu *iommu = dev_get_drvdata(dev);
+
+	return rk_iommu_startup(iommu);
+}
+
+static const struct dev_pm_ops rk_iommu_pm_ops = {
+	SET_RUNTIME_PM_OPS(rk_iommu_suspend, rk_iommu_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
 static const struct of_device_id rk_iommu_dt_ids[] = {
 	{ .compatible = "rockchip,iommu" },
 	{ /* sentinel */ }
@@ -1235,6 +1329,7 @@  static struct platform_driver rk_iommu_driver = {
 	.driver = {
 		   .name = "rk_iommu",
 		   .of_match_table = rk_iommu_dt_ids,
+		   .pm = &rk_iommu_pm_ops,
 		   .suppress_bind_attrs = true,
 	},
 };