Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Hans de Goede <hdegoede@redhat.com>]

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