| Message ID | 20161210141908.16470-4-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
+Cc: Len Len, I think you would be interested by this. Hans, thanks for the change! Most probably we will anticipate Len's ACK on this one. On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to > change the C-state while we hold the punit bus semaphore, at which > point > everything just hangs. > > Avoid this by forcing the CPU to C1 before acquiring the punit bus > semaphore. Isn't it C0? C1 as far as I remember is halted state. > @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32 > *sem) > u32 data; > int ret; > > + /* > + * Force CPU to C1 state, otherwise if the cpuidle / > intel_idle > + * driver tries to change the C state while we're holding the > + * semaphore, the SoC hangs. C0? > + */ > + pm_qos_update_request(&dev->pm_qos, 0); C1 is when you set 1 here, right? > platform_device *pdev) > if (!dev->pm_runtime_disabled) > pm_runtime_disable(&pdev->dev); > + if (dev->acquire_lock) > + pm_qos_remove_request(&dev->pm_qos); > + Perhaps you need to do this in -core.c. Otherwise you missed PCI case. (Even with PCI enumerated host with ACPI-enabled firmware you may get _SEM object present) > return 0; > } >
Hi, On 10-12-16 15:53, Andy Shevchenko wrote: > +Cc: Len > Len, I think you would be interested by this. > > Hans, thanks for the change! You're welcome I ended up comparing the code in i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from: https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches against the mainline code while I was trying to fix the maddening problem of the entire SoC hanging more or less as soon as I tried to use the pmic i2c bus and there I found some fiddling with pm_qos which let to this patch. > Most probably we will anticipate Len's ACK > on this one. > > On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to >> change the C-state while we hold the punit bus semaphore, at which >> point >> everything just hangs. >> >> Avoid this by forcing the CPU to C1 before acquiring the punit bus >> semaphore. > > Isn't it C0? C1 as far as I remember is halted state. You're right, I will fix it. >> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32 >> *sem) >> u32 data; >> int ret; >> >> + /* >> + * Force CPU to C1 state, otherwise if the cpuidle / >> intel_idle >> + * driver tries to change the C state while we're holding the >> + * semaphore, the SoC hangs. > > C0? > >> + */ >> + pm_qos_update_request(&dev->pm_qos, 0); > > C1 is when you set 1 here, right? I believe so, yes. > >> platform_device *pdev) >> if (!dev->pm_runtime_disabled) >> pm_runtime_disable(&pdev->dev); > >> + if (dev->acquire_lock) >> + pm_qos_remove_request(&dev->pm_qos); >> + > > Perhaps you need to do this in -core.c. Otherwise you missed PCI case. > (Even with PCI enumerated host with ACPI-enabled firmware you may get > _SEM object present) Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support() which does the pm_qos_add_request, so I put it here to keep things balanced. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10-12-16 20:33, Hans de Goede wrote: > Hi, > > On 10-12-16 15:53, Andy Shevchenko wrote: >> +Cc: Len >> Len, I think you would be interested by this. >> >> Hans, thanks for the change! > > You're welcome I ended up comparing the code in > i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from: > > https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches > > against the mainline code while I was trying to fix the maddening > problem of the entire SoC hanging more or less as soon > as I tried to use the pmic i2c bus and there I found > some fiddling with pm_qos which let to this patch. > >> Most probably we will anticipate Len's ACK >> on this one. >> >> On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to >>> change the C-state while we hold the punit bus semaphore, at which >>> point >>> everything just hangs. >>> >>> Avoid this by forcing the CPU to C1 before acquiring the punit bus >>> semaphore. >> >> Isn't it C0? C1 as far as I remember is halted state. > > You're right, I will fix it. Correction, upon closer reading of the docs, we cannot disallow the CPU to enter C1 / force it to either C0 or C1, what we can disallow is for it to enter C6/C7. Which also makes sense wrt this bug, since entering C6/C7 involves turning of the CPU-core power-plane, which requires the punit to access the pmic. So I've changes the text in both the commit msg and the comment to: "Disallow the CPU to enter C6 or C7" I still need to re-test (just to make sure I did not cause any regressions) and then I'll send a v3. Regards, Hans >>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32 >>> *sem) >>> u32 data; >>> int ret; >>> >>> + /* >>> + * Force CPU to C1 state, otherwise if the cpuidle / >>> intel_idle >>> + * driver tries to change the C state while we're holding the >>> + * semaphore, the SoC hangs. >> >> C0? >> >>> + */ >>> + pm_qos_update_request(&dev->pm_qos, 0); >> >> C1 is when you set 1 here, right? > > I believe so, yes. > >> >>> platform_device *pdev) >>> if (!dev->pm_runtime_disabled) >>> pm_runtime_disable(&pdev->dev); >> >>> + if (dev->acquire_lock) >>> + pm_qos_remove_request(&dev->pm_qos); >>> + >> >> Perhaps you need to do this in -core.c. Otherwise you missed PCI case. >> (Even with PCI enumerated host with ACPI-enabled firmware you may get >> _SEM object present) > > Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support() > which does the pm_qos_add_request, so I put it here to keep things > balanced. > > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Is there a simple way to run a test to keep deep C-states and instead disable part or all of i2c on this platform, to see how much stability separating the two will buy us? A lot of people are struggling w/ the stability of this platform, and it would be great to make some progress on that. thanks, -Len On Sat, Dec 10, 2016 at 2:59 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 10-12-16 20:33, Hans de Goede wrote: >> >> Hi, >> >> On 10-12-16 15:53, Andy Shevchenko wrote: >>> >>> +Cc: Len >>> Len, I think you would be interested by this. >>> >>> Hans, thanks for the change! >> >> >> You're welcome I ended up comparing the code in >> i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from: >> >> >> https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches >> >> against the mainline code while I was trying to fix the maddening >> problem of the entire SoC hanging more or less as soon >> as I tried to use the pmic i2c bus and there I found >> some fiddling with pm_qos which let to this patch. >> >>> Most probably we will anticipate Len's ACK >>> on this one. >>> >>> On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to >>>> change the C-state while we hold the punit bus semaphore, at which >>>> point >>>> everything just hangs. >>>> >>>> Avoid this by forcing the CPU to C1 before acquiring the punit bus >>>> semaphore. >>> >>> >>> Isn't it C0? C1 as far as I remember is halted state. >> >> >> You're right, I will fix it. > > > Correction, upon closer reading of the docs, we cannot disallow > the CPU to enter C1 / force it to either C0 or C1, what we can > disallow is for it to enter C6/C7. Which also makes sense wrt > this bug, since entering C6/C7 involves turning of the > CPU-core power-plane, which requires the punit to access the pmic. > > So I've changes the text in both the commit msg and the comment > to: "Disallow the CPU to enter C6 or C7" > > I still need to re-test (just to make sure I did not cause > any regressions) and then I'll send a v3. > > Regards, > > Hans > > > > >>>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32 >>>> *sem) >>>> u32 data; >>>> int ret; >>>> >>>> + /* >>>> + * Force CPU to C1 state, otherwise if the cpuidle / >>>> intel_idle >>>> + * driver tries to change the C state while we're holding the >>>> + * semaphore, the SoC hangs. >>> >>> >>> C0? >>> >>>> + */ >>>> + pm_qos_update_request(&dev->pm_qos, 0); >>> >>> >>> C1 is when you set 1 here, right? >> >> >> I believe so, yes. >> >>> >>>> platform_device *pdev) >>>> if (!dev->pm_runtime_disabled) >>>> pm_runtime_disable(&pdev->dev); >>> >>> >>>> + if (dev->acquire_lock) >>>> + pm_qos_remove_request(&dev->pm_qos); >>>> + >>> >>> >>> Perhaps you need to do this in -core.c. Otherwise you missed PCI case. >>> (Even with PCI enumerated host with ACPI-enabled firmware you may get >>> _SEM object present) >> >> >> Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support() >> which does the pm_qos_add_request, so I put it here to keep things >> balanced. >> >> Regards, >> >> Hans
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c index a419777..4f10ebb 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; + /* + * Force CPU to C1 state, otherwise if the cpuidle / intel_idle + * driver tries to change the C state 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) @@ -143,6 +153,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); 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); + return 0; }
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 cpuidle / intel_idle driver trying to change the C-state while we hold the punit bus semaphore, at which point everything just hangs. Avoid this by forcing the CPU to C1 before acquiring the punit bus semaphore. BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- 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(+)