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

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

[Hans de Goede]

Hi,

On 26-12-16 12:07, Hans de Goede wrote:
> Hi,
>
> On 25-12-16 19:31, Len Brown wrote:
>> 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?
>
> This should do the trick to completely disable i2c from the kernel
> to the pmic, leaving the bus fully free for the punit:
>
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 1590ad0..fe73271 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -140,6 +140,7 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
>
>      if (shared_host) {
>          dev_info(dev->dev, "I2C bus managed by PUNIT\n");
> +        return -ENODEV;
>          dev->acquire_lock = baytrail_i2c_acquire;
>          dev->release_lock = baytrail_i2c_release;
>          dev->pm_runtime_disabled = true;
>
> Note that my patch only disabled deep C-states while the kernel
> is accessing the pmic i2c bus, not all the time as most other
> workarounds are doing.

Ok, some more very interesting input on this, from the bug
about the PUNIT semaphore issues on cherrytrail:

https://bugzilla.kernel.org/show_bug.cgi?id=155241#c37

"My device is a laptop with no USB charging or OTG. So, I'd tried only SDIO _ADR patch, i2c and axp288_fuel_gauge patches. Everything works well for upto 3 mins after boot, then the device freezes. I hadn't tried any drm patches BTW. Here's the log:

[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
clocksource:                       'refined-jiffies' wd_now: 10002ee30 wd_last: 10002edb8 mask: ffffffff
clocksource:                       'tsc' cs_now: 16ac2c7744a cs_last: 16a8d9bd8f2 mask: ffffffffffffffff
clocksource: Switched to clocksource refined-jiffies
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: axp288 reg read err:-110
axp288_fuel_gauge axp288_fuel_gauge: PWR STAT read failed:-110
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 0
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: IIO channel read error: fffffffb, 0
power_supply axp288_fuel_gauge: driver failed to report `voltage_now' property: -5
***SYSTEM FREEZE***

If I blacklist axp288_fuel_gauge, then there were no errors."

So it seems that not only bad things happen when the punit tries to
change the cpu C-state while we're holding the pmic i2c bus
semaphore, but that similar bad things happen when the gpu code
tries to acquire / release a forcewake lock on the GPU while
we're accessing the pmic i2c bus.

This makes sense, the iosf_mbi code which is used by the
i2c bus semaphore code has this:

arch/x86/platform/intel/iosf_mbi.c:

int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
{
         u32 mcr, mcrx;
         unsigned long flags;
         int ret;

         /* Access to the GFX unit is handled by GPU code */
         if (port == BT_MBI_UNIT_GFX) {
                 WARN_ON(1);
                 return -EPERM;
         }

	...
}

So the i915 driver definitely is interacting with the punit
through the mailbox interface too...

I'll try to write a quick and dirty patch where the i915 code
simply calls intel_uncore_forcewake_get(dev_priv,  FORCEWAKE_ALL);
an extra time on probe, and ask the user who is seeing this to
test.

Regards,

Hans


code calls
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Andy Shevchenko]

On Sat, 2016-12-31 at 22:29 +0100, Hans de Goede wrote:

> This makes sense, the iosf_mbi code which is used by the
> i2c bus semaphore code has this:
> 
> arch/x86/platform/intel/iosf_mbi.c:
> 
> int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
> {
>          u32 mcr, mcrx;
>          unsigned long flags;
>          int ret;
> 
>          /* Access to the GFX unit is handled by GPU code */
>          if (port == BT_MBI_UNIT_GFX) {
>                  WARN_ON(1);
>                  return -EPERM;
>          }
> 
> 	...
> }
> 
> So the i915 driver definitely is interacting with the punit
> through the mailbox interface too...

Yes, they have private mailbox support. Once I talked to Ville to make
at least definitions common between two: iosf_mbi.h and whatever i915 is
using, but have no time to implement that.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy