From a23318feeff662c8d25d21623daebdd2e55ec221 Mon Sep 17 00:00:00 2001 From: Ulf Hansson Date: Wed, 9 Aug 2017 15:28:22 +0200 Subject: i2c: designware: Fix system suspend The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend"), may suggest to the PM core to try out the so called direct_complete path for system sleep. In this path, the PM core treats a runtime suspended device as it's already in a proper low power state for system sleep, which makes it skip calling the system sleep callbacks for the device, except for the ->prepare() and the ->complete() callbacks. However, the PM core may unset the direct_complete flag for a parent device, in case its child device are being system suspended before. In this scenario, the PM core invokes the system sleep callbacks, no matter if the device is runtime suspended or not. Particularly in cases of an existing i2c slave device, the above path is triggered, which breaks the assumption that the i2c device is always runtime resumed whenever the dw_i2c_plat_suspend() is being called. More precisely, dw_i2c_plat_suspend() calls clk_core_disable() and clk_core_unprepare(), for an already disabled/unprepared clock, leading to a splat in the log about clocks calls being wrongly balanced and breaking system sleep. To still allow the direct_complete path in cases when it's possible, but also to keep the fix simple, let's runtime resume the i2c device in the ->suspend() callback, before continuing to put the device into low power state. Note, in cases when the i2c device is attached to the ACPI PM domain, this problem doesn't occur, because ACPI's ->suspend() callback, assigned to acpi_subsys_suspend(), already calls pm_runtime_resume() for the device. It should also be noted that this change does not fix commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during system suspend"). Because for the non-ACPI case, the system sleep support was already broken prior that point. Cc: # v4.4+ Signed-off-by: Ulf Hansson Acked-by: Rafael J. Wysocki Tested-by: John Stultz Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula Reviewed-by: Mika Westerberg Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 143a8fd582b4..441afc715a90 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -430,7 +430,7 @@ static void dw_i2c_plat_complete(struct device *dev) #endif #ifdef CONFIG_PM -static int dw_i2c_plat_suspend(struct device *dev) +static int dw_i2c_plat_runtime_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); @@ -452,11 +452,21 @@ static int dw_i2c_plat_resume(struct device *dev) return 0; } +#ifdef CONFIG_PM_SLEEP +static int dw_i2c_plat_suspend(struct device *dev) +{ + pm_runtime_resume(dev); + return dw_i2c_plat_runtime_suspend(dev); +} +#endif + static const struct dev_pm_ops dw_i2c_dev_pm_ops = { .prepare = dw_i2c_plat_prepare, .complete = dw_i2c_plat_complete, SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume) - SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL) + SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, + dw_i2c_plat_resume, + NULL) }; #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops) -- cgit v1.2.3 From 984277a041d5ee4a65aaadf0307d67a7c401e11c Mon Sep 17 00:00:00 2001 From: Jarkko Nikula Date: Fri, 11 Aug 2017 14:44:55 +0300 Subject: i2c: designware: Fix oops from i2c_dw_irq_handler_slave When i2c-designware is initialized in slave mode the i2c-designware-slave.c: i2c_dw_irq_handler_slave() can hit a NULL pointer dereference when I2C slave backend is not registered but code is accessing the struct dw_i2c_dev.slave without testing is it NULL. We might get spurious interrupts from other devices or from IRQ core during unloading the driver when CONFIG_DEBUG_SHIRQ is set. Existing check for enable and IRQ status is not enough since device can be power gated and those bits may read 1. Fix this by handling the interrupt only when also struct dw_i2c_dev.slave is set. Signed-off-by: Jarkko Nikula Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-designware-slave.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index 0548c7ea578c..4b62a3872763 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -272,7 +272,7 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev) slave_activity = ((dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_SLAVE_ACTIVITY) >> 6); - if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY)) + if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY) || !dev->slave) return 0; dev_dbg(dev->dev, -- cgit v1.2.3 From 4e2d93de070ceaca5097f7ee5c311731b83208a0 Mon Sep 17 00:00:00 2001 From: Jarkko Nikula Date: Wed, 9 Aug 2017 15:24:44 +0300 Subject: i2c: designware: Fix standard mode speed when configuring the slave mode Code sets bit DW_IC_CON_SPEED_FAST (0x4) always when configuring the slave mode. This results incorrect register value DW_IC_CON_SPEED_HIGH (0x6) when OR'ed together with DW_IC_CON_SPEED_STD (0x2). Remove this and let the code set the speed mode bits according to clock frequency or default to fast mode. Signed-off-by: Jarkko Nikula Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-designware-platdrv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 441afc715a90..57248bccadbc 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -198,8 +198,7 @@ static void i2c_dw_configure_slave(struct dw_i2c_dev *dev) dev->functionality = I2C_FUNC_SLAVE | DW_IC_DEFAULT_FUNCTIONALITY; dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL | - DW_IC_CON_RESTART_EN | DW_IC_CON_STOP_DET_IFADDRESSED | - DW_IC_CON_SPEED_FAST; + DW_IC_CON_RESTART_EN | DW_IC_CON_STOP_DET_IFADDRESSED; dev->mode = DW_IC_SLAVE; -- cgit v1.2.3 From f4b17a14faeec4160f97ad75ea7534f571f12404 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas Date: Wed, 9 Aug 2017 11:21:28 +0200 Subject: i2c: core: Make comment about I2C table requirement to reflect the code I2C drivers were required to have an I2C device ID table even if were for devices that would only be registered using a specific firmware interface (e.g: OF or ACPI). But commit da10c06a044b ("i2c: Make I2C ID tables non-mandatory for DT'ed devices") changed the I2C core to relax the requirement and allow drivers to avoid defining this table. Unfortunately it only took into account drivers for OF-only devices and forgot about ACPI-only ones, and this was fixed by commit c64ffff7a9d1 ("i2c: core: Allow empty id_table in ACPI case as well"). But the latter didn't update the original comment, so it doesn't reflect what the code does now. Signed-off-by: Javier Martinez Canillas Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 12822a4b8f8f..56e46581b84b 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -353,8 +353,8 @@ static int i2c_device_probe(struct device *dev) } /* - * An I2C ID table is not mandatory, if and only if, a suitable Device - * Tree match table entry is supplied for the probing device. + * An I2C ID table is not mandatory, if and only if, a suitable OF + * or ACPI ID table is supplied for the probing device. */ if (!driver->id_table && !i2c_acpi_match_device(dev->driver->acpi_match_table, client) && -- cgit v1.2.3 From 42543aeb48e3701b49e0a83654248afc38feb88f Mon Sep 17 00:00:00 2001 From: Anton Vasilyev Date: Thu, 10 Aug 2017 18:15:45 +0300 Subject: i2c: simtec: use release_mem_region instead of release_resource Use api pair of request_mem_region and release_mem_region instead of release_resource. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Anton Vasilyev Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-simtec.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-simtec.c b/drivers/i2c/busses/i2c-simtec.c index b4685bb9b5d7..adca51a99487 100644 --- a/drivers/i2c/busses/i2c-simtec.c +++ b/drivers/i2c/busses/i2c-simtec.c @@ -127,8 +127,7 @@ static int simtec_i2c_probe(struct platform_device *dev) iounmap(pd->reg); err_res: - release_resource(pd->ioarea); - kfree(pd->ioarea); + release_mem_region(pd->ioarea->start, size); err: kfree(pd); @@ -142,8 +141,7 @@ static int simtec_i2c_remove(struct platform_device *dev) i2c_del_adapter(&pd->adap); iounmap(pd->reg); - release_resource(pd->ioarea); - kfree(pd->ioarea); + release_mem_region(pd->ioarea->start, resource_size(pd->ioarea)); kfree(pd); return 0; -- cgit v1.2.3 From f1c0b7e448b9e66dd9a7343bab58a3d3a477e104 Mon Sep 17 00:00:00 2001 From: Brendan Higgins Date: Fri, 28 Jul 2017 18:00:12 -0700 Subject: i2c: aspeed: fixed potential null pointer dereference Before I skipped null checks when the master is in the STOP state; this fixes that. Signed-off-by: Brendan Higgins Acked-by: Joel Stanley Signed-off-by: Wolfram Sang Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C") --- drivers/i2c/busses/i2c-aspeed.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index f19348328a71..6fdf9231c23c 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -410,10 +410,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus) } /* We are in an invalid state; reset bus to a known state. */ - if (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP) { + if (!bus->msgs) { dev_err(bus->dev, "bus in unknown state"); bus->cmd_err = -EIO; - aspeed_i2c_do_stop(bus); + if (bus->master_state != ASPEED_I2C_MASTER_STOP) + aspeed_i2c_do_stop(bus); goto out_no_complete; } msg = &bus->msgs[bus->msgs_index]; -- cgit v1.2.3 From 733f6563979d96dec180c350abb8ac67cc0367ba Mon Sep 17 00:00:00 2001 From: Jarkko Nikula Date: Tue, 15 Aug 2017 17:34:44 +0300 Subject: i2c: designware: Remove needless pm_runtime_put_noidle() call I guess pm_runtime_put_noidle() call in i2c_dw_probe_slave() was copied by accident from similar master mode adapter registration code. It is unbalanced due missing pm_runtime_get_noresume() but harmless since it doesn't decrease dev->power.usage_count below zero. In theory we can hit similar needless runtime suspend/resume cycle during slave mode adapter registration that was happening when registering the master mode adapter. See commit cd998ded5c12 ("i2c: designware: Prevent runtime suspend during adapter registration"). However, since we are slave, we can consider it as a wrong configuration if we have other slaves attached under this adapter and can omit the pm_runtime_get_noresume()/pm_runtime_put_noidle() calls for simplicity. Signed-off-by: Jarkko Nikula Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-designware-slave.c | 1 - 1 file changed, 1 deletion(-) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index 4b62a3872763..25fa33927124 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -382,7 +382,6 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev) ret = i2c_add_numbered_adapter(adap); if (ret) dev_err(dev->dev, "failure adding adapter: %d\n", ret); - pm_runtime_put_noidle(dev->dev); return ret; } -- cgit v1.2.3 From 2a86cdd2e7d3c75580f41f89f9b9211e225573cc Mon Sep 17 00:00:00 2001 From: Jarkko Nikula Date: Tue, 15 Aug 2017 17:34:45 +0300 Subject: i2c: designware: Fix runtime PM for I2C slave mode I2C slave controller must be powered and active all the time when I2C slave backend is registered in order to let master address and communicate with us. Now if the controller is runtime PM capable it will be suspended after probe and cannot ever respond to the master or generate interrupts. Fix this by resuming the controller when I2C slave backend is registered and let it suspend after unregistering. Signed-off-by: Jarkko Nikula Signed-off-by: Wolfram Sang --- drivers/i2c/busses/i2c-designware-slave.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers') diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index 25fa33927124..78d8fb73927d 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -177,6 +177,8 @@ static int i2c_dw_reg_slave(struct i2c_client *slave) return -EBUSY; if (slave->flags & I2C_CLIENT_TEN) return -EAFNOSUPPORT; + pm_runtime_get_sync(dev->dev); + /* * Set slave address in the IC_SAR register, * the address to which the DW_apb_i2c responds. @@ -205,6 +207,7 @@ static int i2c_dw_unreg_slave(struct i2c_client *slave) dev->disable_int(dev); dev->disable(dev); dev->slave = NULL; + pm_runtime_put(dev->dev); return 0; } -- cgit v1.2.3