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@xxxxxxxxxx> 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 -- Len Brown, Intel Open Source Technology Center -- 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
