On Sat, 2016-12-10 at 23:43 +0100, Hans de Goede wrote: > On my cherrytrail tablet with axp288 pmic, just doing a bunch of > repeated > reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet > in > 1 - 3 runs guaranteed. > > This seems to be causes by the cpu trying to enter C6 or C7 while we > hold > the punit bus semaphore, at which point everything just hangs. > > Avoid this by the CPU to enter C6 or C7 before acquiring the punit bus > semaphore. Please, keep Len Brown in a loop for this patch. And perhaps someone from our Graphics team. Maybe Imre Deak? See my comments below. > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051 > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > Changes in v2: > -New patch in v2 of this set > Changes in v3: > -Change commit message and comment in the code from "force the CPU to > C1" > to "Disallow the CPU to enter C6 or C7", as the CPU may still be in > either > C0 or C1 with the request pm_qos > --- > drivers/i2c/busses/i2c-designware-baytrail.c | 12 ++++++++++++ > drivers/i2c/busses/i2c-designware-core.h | 3 +++ > drivers/i2c/busses/i2c-designware-platdrv.c | 3 +++ > 3 files changed, 18 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c > b/drivers/i2c/busses/i2c-designware-baytrail.c > index cf02222..997d048 100644 > --- a/drivers/i2c/busses/i2c-designware-baytrail.c > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c > @@ -16,6 +16,7 @@ > #include <linux/acpi.h> > #include <linux/i2c.h> > #include <linux/interrupt.h> > +#include <linux/pm_qos.h> > > #include <asm/iosf_mbi.h> > > @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32 > *sem) > u32 data; > int ret; > > + /* > + * Disallow the CPU to enter C6 or C7 state, entering these > states > + * requires the punit to talk to the pmic and if this happens > while > + * we're holding the semaphore, the SoC hangs. > + */ > + pm_qos_update_request(&dev->pm_qos, 0); > + > ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, > PUNIT_SEMAPHORE, &data); > if (ret) { > dev_err(dev->dev, "iosf failed to read punit > semaphore\n"); > @@ -56,6 +64,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev) > data &= ~PUNIT_SEMAPHORE_BIT; > if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, > PUNIT_SEMAPHORE, data)) > dev_err(dev->dev, "iosf failed to reset punit > semaphore during write\n"); > + > + pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE); > } > > static int baytrail_i2c_acquire(struct dw_i2c_dev *dev) > @@ -145,6 +155,8 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev > *dev) > return -EPROBE_DEFER; > > dev_info(dev->dev, "I2C bus managed by PUNIT\n"); > + pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY, > + PM_QOS_DEFAULT_VALUE); Perhaps move this to the end of the function? For sake of readability. > dev->acquire_lock = baytrail_i2c_acquire; > dev->release_lock = baytrail_i2c_release; > dev->pm_runtime_disabled = true; > diff --git a/drivers/i2c/busses/i2c-designware-core.h > b/drivers/i2c/busses/i2c-designware-core.h > index fb143f5..47d284c 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -22,6 +22,7 @@ > * > */ > > +#include <linux/pm_qos.h> > > #define DW_IC_CON_MASTER 0x1 > #define DW_IC_CON_SPEED_STD 0x2 > @@ -67,6 +68,7 @@ > * @fp_lcnt: fast plus LCNT value > * @hs_hcnt: high speed HCNT value > * @hs_lcnt: high speed LCNT value > + * @pm_qos: pm_qos_request used while holding a hardware lock on the > bus > * @acquire_lock: function to acquire a hardware lock on the bus > * @release_lock: function to release a hardware lock on the bus > * @pm_runtime_disabled: true if pm runtime is disabled > @@ -114,6 +116,7 @@ struct dw_i2c_dev { > u16 fp_lcnt; > u16 hs_hcnt; > u16 hs_lcnt; > + struct pm_qos_request pm_qos; > int (*acquire_lock)(struct dw_i2c_dev > *dev); > void (*release_lock)(struct dw_i2c_dev > *dev); > bool pm_runtime_disabled; > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c > b/drivers/i2c/busses/i2c-designware-platdrv.c > index 97a2ca1..6d72929 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -291,6 +291,9 @@ static int dw_i2c_plat_remove(struct > platform_device *pdev) > if (!dev->pm_runtime_disabled) > pm_runtime_disable(&pdev->dev); > > > + if (dev->acquire_lock) > + pm_qos_remove_request(&dev->pm_qos); > + I read your answer to v2 of this, so, taking into consideration your notice I would recommend to provide an additional helper like i2c_dw_down_lock_support() (I didn't find proper antonym for eval, feel free to choose a better name) and collect the above there. > return 0; > } > -- Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
