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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
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
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 <[email protected]>
---
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
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
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