[PATCH v2 0/4] iommu/rockchip: Runtime PM fixes

[PATCH v2 0/4] iommu/rockchip: Runtime PM fixes

[Marc Zyngier]

This small series addresses a couple of runtime PM issues I've spotted
while running 4.18 on a Chromebook Plus (kevin, rk3399) platform, and
specifically doing kexec.

In order to avoid making a complete mess of the IOMMU code, Heiko has
requested that all RK platforms would select CONFIG_PM, which
simplifies a lot of things. I've kept 32 and 64bit patches separate,
but feel free to squash them into on if that's more convenient.

Note that even with these patches, kexec is still fairly broken on
rk3399, as the VOP is never turned off (see [1] for a fix).

[1] https://www.spinics.net/lists/arm-kernel/msg670229.html

* From v1:
  - Collected RBs from Heiko
  - Added two patches forcing CONFIG_PM on all Rockchip platforms at
    Heiko's request, following the example set by Tegra platforms.

Marc Zyngier (4):
  ARM: rockchip: Force CONFIG_PM on Rockchip systems
  arm64: rockchip: Force CONFIG_PM on Rockchip systems
  iommu/rockchip: Handle errors returned from PM framework
  iommu/rockchip: Move irq request past pm_runtime_enable

 arch/arm/mach-rockchip/Kconfig |  1 +
 arch/arm64/Kconfig.platforms   |  1 +
 drivers/iommu/rockchip-iommu.c | 45 +++++++++++++++++++++-------------
 3 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.18.0

[PATCH v2 1/4] ARM: rockchip: Force CONFIG_PM on Rockchip systems

[Marc Zyngier]

A number of the Rockchip-specific drivers (IOMMU, display controllers)
are now assuming that CONFIG_PM is set, and may completely misbehave
if that's not the case.

Since there is hardly any reason for this configuration option not
to be selected anyway, let's require it (in the same way Tegra already
does).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/mach-rockchip/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index fafd3d7f9f8c..8ca926522026 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -17,6 +17,7 @@ config ARCH_ROCKCHIP
 	select ARM_GLOBAL_TIMER
 	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
 	select ZONE_DMA if ARM_LPAE
+	select PM
 	help
 	  Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
 	  containing the RK2928, RK30xx and RK31xx series.
-- 
2.18.0

[PATCH v2 2/4] arm64: rockchip: Force CONFIG_PM on Rockchip systems

[Marc Zyngier]

A number of the Rockchip-specific drivers (IOMMU, display controllers)
are now assuming that CONFIG_PM is set, and may completely misbehave
if that's not the case.

Since there is hardly any reason for this configuration option not
to be selected anyway, let's require it (in the same way Tegra already
does).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index d5aeac351fc3..21a715ad8222 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -151,6 +151,7 @@ config ARCH_ROCKCHIP
 	select GPIOLIB
 	select PINCTRL
 	select PINCTRL_ROCKCHIP
+	select PM
 	select ROCKCHIP_TIMER
 	help
 	  This enables support for the ARMv8 based Rockchip chipsets,
-- 
2.18.0

[PATCH v2 3/4] iommu/rockchip: Handle errors returned from PM framework

[Marc Zyngier]

pm_runtime_get_if_in_use can fail: either PM has been disabled
altogether (-EINVAL), or the device hasn't been enabled yet (0).
Sadly, the Rockchip IOMMU driver tends to conflate the two things
by considering a non-zero return value as successful.

This has the consequence of hiding other bugs, so let's handle this
case throughout the driver, with a WARN_ON_ONCE so that we can try
and work out what happened.

Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 054cd2c8e9c8..4e0f9b61cd7f 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -521,10 +521,11 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
 	u32 int_status;
 	dma_addr_t iova;
 	irqreturn_t ret = IRQ_NONE;
-	int i;
+	int i, err;
 
-	if (WARN_ON(!pm_runtime_get_if_in_use(iommu->dev)))
-		return 0;
+	err = pm_runtime_get_if_in_use(iommu->dev);
+	if (WARN_ON_ONCE(err <= 0))
+		return ret;
 
 	if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
 		goto out;
@@ -620,11 +621,15 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain,
 	spin_lock_irqsave(&rk_domain->iommus_lock, flags);
 	list_for_each(pos, &rk_domain->iommus) {
 		struct rk_iommu *iommu;
+		int ret;
 
 		iommu = list_entry(pos, struct rk_iommu, node);
 
 		/* Only zap TLBs of IOMMUs that are powered on. */
-		if (pm_runtime_get_if_in_use(iommu->dev)) {
+		ret = pm_runtime_get_if_in_use(iommu->dev);
+		if (WARN_ON_ONCE(ret < 0))
+			continue;
+		if (ret) {
 			WARN_ON(clk_bulk_enable(iommu->num_clocks,
 						iommu->clocks));
 			rk_iommu_zap_lines(iommu, iova, size);
@@ -891,6 +896,7 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
 	struct rk_iommu *iommu;
 	struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
 	unsigned long flags;
+	int ret;
 
 	/* Allow 'virtual devices' (eg drm) to detach from domain */
 	iommu = rk_iommu_from_dev(dev);
@@ -909,7 +915,9 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
 	list_del_init(&iommu->node);
 	spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
 
-	if (pm_runtime_get_if_in_use(iommu->dev)) {
+	ret = pm_runtime_get_if_in_use(iommu->dev);
+	WARN_ON_ONCE(ret < 0);
+	if (ret > 0) {
 		rk_iommu_disable(iommu);
 		pm_runtime_put(iommu->dev);
 	}
@@ -946,7 +954,8 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
 	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))
+	ret = pm_runtime_get_if_in_use(iommu->dev);
+	if (!ret || WARN_ON_ONCE(ret < 0))
 		return 0;
 
 	ret = rk_iommu_enable(iommu);
-- 
2.18.0

[PATCH v2 4/4] iommu/rockchip: Move irq request past pm_runtime_enable

[Marc Zyngier]

Enabling the interrupt early, before power has been applied to the
device, can result in an interrupt being delivered too early if:

- the IOMMU shares an interrupt with a VOP
- the VOP has a pending interrupt (after a kexec, for example)

In these conditions, we end-up taking the interrupt without
the IOMMU being ready to handle the interrupt (not powered on).

Moving the interrupt request past the pm_runtime_enable() call
makes sure we can at least access the IOMMU registers. Note that
this is only a partial fix, and that the VOP interrupt will still
be screaming until the VOP driver kicks in, which advocates for
a more synchronized interrupt enabling/disabling approach.

Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/iommu/rockchip-iommu.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 4e0f9b61cd7f..2b1724e8d307 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1161,17 +1161,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (iommu->num_mmu == 0)
 		return PTR_ERR(iommu->bases[0]);
 
-	i = 0;
-	while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
-		if (irq < 0)
-			return irq;
-
-		err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
-				       IRQF_SHARED, dev_name(dev), iommu);
-		if (err)
-			return err;
-	}
-
 	iommu->reset_disabled = device_property_read_bool(dev,
 					"rockchip,disable-mmu-reset");
 
@@ -1228,6 +1217,19 @@ static int rk_iommu_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	i = 0;
+	while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
+		if (irq < 0)
+			return irq;
+
+		err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+				       IRQF_SHARED, dev_name(dev), iommu);
+		if (err) {
+			pm_runtime_disable(dev);
+			goto err_remove_sysfs;
+		}
+	}
+
 	return 0;
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
-- 
2.18.0

Re: [PATCH v2 0/4] iommu/rockchip: Runtime PM fixes

[Olof Johansson]

On Fri, Aug 24, 2018 at 04:06:33PM +0100, Marc Zyngier wrote:
> This small series addresses a couple of runtime PM issues I've spotted
> while running 4.18 on a Chromebook Plus (kevin, rk3399) platform, and
> specifically doing kexec.
> 
> In order to avoid making a complete mess of the IOMMU code, Heiko has
> requested that all RK platforms would select CONFIG_PM, which
> simplifies a lot of things. I've kept 32 and 64bit patches separate,
> but feel free to squash them into on if that's more convenient.
> 
> Note that even with these patches, kexec is still fairly broken on
> rk3399, as the VOP is never turned off (see [1] for a fix).
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg670229.html
> 
> * From v1:
>   - Collected RBs from Heiko
>   - Added two patches forcing CONFIG_PM on all Rockchip platforms at
>     Heiko's request, following the example set by Tegra platforms.

Thanks, applied to our next/late branch which I plan to send in tomorrow.


-Olof

Re: [PATCH v2 0/4] iommu/rockchip: Runtime PM fixes

[Heiko Stuebner]

Am Freitag, 24. August 2018, 17:50:59 CEST schrieb Olof Johansson:
> On Fri, Aug 24, 2018 at 04:06:33PM +0100, Marc Zyngier wrote:
> > This small series addresses a couple of runtime PM issues I've spotted
> > while running 4.18 on a Chromebook Plus (kevin, rk3399) platform, and
> > specifically doing kexec.
> > 
> > In order to avoid making a complete mess of the IOMMU code, Heiko has
> > requested that all RK platforms would select CONFIG_PM, which
> > simplifies a lot of things. I've kept 32 and 64bit patches separate,
> > but feel free to squash them into on if that's more convenient.
> > 
> > Note that even with these patches, kexec is still fairly broken on
> > rk3399, as the VOP is never turned off (see [1] for a fix).
> > 
> > [1] https://www.spinics.net/lists/arm-kernel/msg670229.html
> > 
> > * From v1:
> >   - Collected RBs from Heiko
> >   - Added two patches forcing CONFIG_PM on all Rockchip platforms at
> >     Heiko's request, following the example set by Tegra platforms.
> 
> Thanks, applied to our next/late branch which I plan to send in tomorrow.

that was a quick turn around :-) .
Thanks for picking the series.

Heiko