mbox series

[0/7] Resume BYT/CHT LPSS I2C controller before the iGPU

Message ID 20180919191518.18764-1-hdegoede@redhat.com
Headers show
Series Resume BYT/CHT LPSS I2C controller before the iGPU | expand

Message

Hans de Goede Sept. 19, 2018, 7:15 p.m. UTC 
Hi All,

On some Cherry Trail systems the GPU ACPI fwnode has power-resources which
point to the PMIC, which is connected over the LPSS I2C7 controller.

Currently the I2C controller resumes after the GPU is resumed leading to
the following errors:

 i2c_designware 808622C1:06: controller timed out
 ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
 ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR
 video LNXVIDEO:00: Failed to change power state to D0

This series fixes this. This actually requires 3 separate fixes each of
which is necessary and the error only goes away once all 3 are in place:

1) Patches 1-4 rework the acpi_lpss.c device-link code added by commit
   e6ce0ce34f65 ("CPI / LPSS: Add device link for CHT SD card dependency on I2")
   so that the code can be used to add a device link between an ACPI LPSS
   device and a PCI device (the GPU).
   Patch 5 then actually adds the device-link. Note that the other 2 fixes
   may give the impression that this fix is not necessary, that is not the
   case we most both make sure that the resume happens during the right
   resume phase *and* that within that phase it is done before the GPU resume

2) The GPU is a PCI device and PCI devices are brought up (moved from D3 to D0)
   in the noirq phase already. Normally ACPI LPSS device gets resumed in the
   resume_early phase. The ordering from 1) only orders things within the same
   phase. To make sure the I2C controller is ready before the GPU resumes
   we must resume LPSS I2C controllers (on BYT and CHT) from the noirq phase.
   This is done in patch 6.

3) Now we get a timeout waiting for the IRQ in the i2c-designware driver
   since IRQs are disabled in the noirq resume phase. Patch 7 marks the IRQ
   for BYT/CHT ACPI-LPSS i2c-designware controllers with IRQF_NO_SUSPEND.

These patches are posted a single series since together they fix a single
problem. But both the acpi_lpss and i2c-designware code have seen some
churn in -next and not having patch 7 in place results in the error staying
the same, so patches 1-6 can be merged indepedently without causing any
regresssions (and patch 7 without 1-6 is not an issue either).

Therefor I suggest that the acpi_lpss.c and the i2c-designware changes get
merged through separate trees.

Regards,

Hans

Comments

Hans de Goede Sept. 19, 2018, 7:43 p.m. UTC | #1 
Hi again,

On 19-09-18 21:15, Hans de Goede wrote:
> Hi All,
> 
> On some Cherry Trail systems the GPU ACPI fwnode has power-resources which
> point to the PMIC, which is connected over the LPSS I2C7 controller.
> 
> Currently the I2C controller resumes after the GPU is resumed leading to
> the following errors:
> 
>   i2c_designware 808622C1:06: controller timed out
>   ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
>   ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR
>   video LNXVIDEO:00: Failed to change power state to D0
> 
> This series fixes this. This actually requires 3 separate fixes each of
> which is necessary and the error only goes away once all 3 are in place:
> 
> 1) Patches 1-4 rework the acpi_lpss.c device-link code added by commit
>     e6ce0ce34f65 ("CPI / LPSS: Add device link for CHT SD card dependency on I2")
>     so that the code can be used to add a device link between an ACPI LPSS
>     device and a PCI device (the GPU).
>     Patch 5 then actually adds the device-link. Note that the other 2 fixes
>     may give the impression that this fix is not necessary, that is not the
>     case we most both make sure that the resume happens during the right
>     resume phase *and* that within that phase it is done before the GPU resume
> 
> 2) The GPU is a PCI device and PCI devices are brought up (moved from D3 to D0)
>     in the noirq phase already. Normally ACPI LPSS device gets resumed in the
>     resume_early phase. The ordering from 1) only orders things within the same
>     phase. To make sure the I2C controller is ready before the GPU resumes
>     we must resume LPSS I2C controllers (on BYT and CHT) from the noirq phase.
>     This is done in patch 6.
> 
> 3) Now we get a timeout waiting for the IRQ in the i2c-designware driver
>     since IRQs are disabled in the noirq resume phase. Patch 7 marks the IRQ
>     for BYT/CHT ACPI-LPSS i2c-designware controllers with IRQF_NO_SUSPEND.
> 
> These patches are posted a single series since together they fix a single
> problem. But both the acpi_lpss and i2c-designware code have seen some
> churn in -next and not having patch 7 in place results in the error staying
> the same, so patches 1-6 can be merged indepedently without causing any
> regresssions (and patch 7 without 1-6 is not an issue either).
> 
> Therefor I suggest that the acpi_lpss.c and the i2c-designware changes get
> merged through separate trees.

I forgot to mention that I've tested this series on the following device
which had the mentioned errors:

GPD win:
CHT device with a Whiskey Cove PMIC, PMIC I2C bus free for kernel use

And on the following other 3 devices to make sure that I covered all
combinations of BYT/CHT and PMIC with/without its I2C bus shared with
the PUNIT:

Chuwi Hi8 Pro:
CHT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel

Asus T100TA:
BYT device with a Crystal Cove PMIC, PMIC I2C bus free for kernel use

Peaq C1010 2-in-1:
BYT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel

And all 4 work fine with this series applied.

Regards,

Hans
Rafael J. Wysocki Sept. 20, 2018, 8:07 a.m. UTC | #2 
On Wednesday, September 19, 2018 9:43:08 PM CEST Hans de Goede wrote:
> Hi again,
> 
> On 19-09-18 21:15, Hans de Goede wrote:
> > Hi All,
> > 
> > On some Cherry Trail systems the GPU ACPI fwnode has power-resources which
> > point to the PMIC, which is connected over the LPSS I2C7 controller.
> > 
> > Currently the I2C controller resumes after the GPU is resumed leading to
> > the following errors:
> > 
> >   i2c_designware 808622C1:06: controller timed out
> >   ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
> >   ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR
> >   video LNXVIDEO:00: Failed to change power state to D0
> > 
> > This series fixes this. This actually requires 3 separate fixes each of
> > which is necessary and the error only goes away once all 3 are in place:
> > 
> > 1) Patches 1-4 rework the acpi_lpss.c device-link code added by commit
> >     e6ce0ce34f65 ("CPI / LPSS: Add device link for CHT SD card dependency on I2")
> >     so that the code can be used to add a device link between an ACPI LPSS
> >     device and a PCI device (the GPU).
> >     Patch 5 then actually adds the device-link. Note that the other 2 fixes
> >     may give the impression that this fix is not necessary, that is not the
> >     case we most both make sure that the resume happens during the right
> >     resume phase *and* that within that phase it is done before the GPU resume
> > 
> > 2) The GPU is a PCI device and PCI devices are brought up (moved from D3 to D0)
> >     in the noirq phase already. Normally ACPI LPSS device gets resumed in the
> >     resume_early phase. The ordering from 1) only orders things within the same
> >     phase. To make sure the I2C controller is ready before the GPU resumes
> >     we must resume LPSS I2C controllers (on BYT and CHT) from the noirq phase.
> >     This is done in patch 6.
> > 
> > 3) Now we get a timeout waiting for the IRQ in the i2c-designware driver
> >     since IRQs are disabled in the noirq resume phase. Patch 7 marks the IRQ
> >     for BYT/CHT ACPI-LPSS i2c-designware controllers with IRQF_NO_SUSPEND.
> > 
> > These patches are posted a single series since together they fix a single
> > problem. But both the acpi_lpss and i2c-designware code have seen some
> > churn in -next and not having patch 7 in place results in the error staying
> > the same, so patches 1-6 can be merged indepedently without causing any
> > regresssions (and patch 7 without 1-6 is not an issue either).
> > 
> > Therefor I suggest that the acpi_lpss.c and the i2c-designware changes get
> > merged through separate trees.
> 
> I forgot to mention that I've tested this series on the following device
> which had the mentioned errors:
> 
> GPD win:
> CHT device with a Whiskey Cove PMIC, PMIC I2C bus free for kernel use
> 
> And on the following other 3 devices to make sure that I covered all
> combinations of BYT/CHT and PMIC with/without its I2C bus shared with
> the PUNIT:
> 
> Chuwi Hi8 Pro:
> CHT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel
> 
> Asus T100TA:
> BYT device with a Crystal Cove PMIC, PMIC I2C bus free for kernel use
> 
> Peaq C1010 2-in-1:
> BYT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel
> 
> And all 4 work fine with this series applied.

OK

It looks reasonable to me overall, but I'll give Mika/Andy and others some
time to leave comments if any.

Thanks,
Rafael
Mika Westerberg Sept. 20, 2018, 8:48 a.m. UTC | #3 
On Thu, Sep 20, 2018 at 10:07:01AM +0200, Rafael J. Wysocki wrote:
> > I forgot to mention that I've tested this series on the following device
> > which had the mentioned errors:
> > 
> > GPD win:
> > CHT device with a Whiskey Cove PMIC, PMIC I2C bus free for kernel use
> > 
> > And on the following other 3 devices to make sure that I covered all
> > combinations of BYT/CHT and PMIC with/without its I2C bus shared with
> > the PUNIT:
> > 
> > Chuwi Hi8 Pro:
> > CHT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel
> > 
> > Asus T100TA:
> > BYT device with a Crystal Cove PMIC, PMIC I2C bus free for kernel use
> > 
> > Peaq C1010 2-in-1:
> > BYT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel
> > 
> > And all 4 work fine with this series applied.
> 
> OK
> 
> It looks reasonable to me overall, but I'll give Mika/Andy and others some
> time to leave comments if any.

Looks good to me as well. Nice work Hans! :)
Jarkko Nikula Sept. 20, 2018, 1:08 p.m. UTC | #4 
On 09/20/2018 11:48 AM, Mika Westerberg wrote:
> On Thu, Sep 20, 2018 at 10:07:01AM +0200, Rafael J. Wysocki wrote:
>>> I forgot to mention that I've tested this series on the following device
>>> which had the mentioned errors:
>>>
>>> GPD win:
>>> CHT device with a Whiskey Cove PMIC, PMIC I2C bus free for kernel use
>>>
>>> And on the following other 3 devices to make sure that I covered all
>>> combinations of BYT/CHT and PMIC with/without its I2C bus shared with
>>> the PUNIT:
>>>
>>> Chuwi Hi8 Pro:
>>> CHT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel
>>>
>>> Asus T100TA:
>>> BYT device with a Crystal Cove PMIC, PMIC I2C bus free for kernel use
>>>
>>> Peaq C1010 2-in-1:
>>> BYT device with an AXP288 PMIC, with its I2C bus shared between the PUNIT and the kernel
>>>
>>> And all 4 work fine with this series applied.
>>
>> OK
>>
>> It looks reasonable to me overall, but I'll give Mika/Andy and others some
>> time to leave comments if any.
> 
> Looks good to me as well. Nice work Hans! :)
> 
I tested the set on a BYT based MRD7 with shared I2C bus and on a CHT 
which I don't know. Both devices suspended and resumed fine before the 
set. Maybe my test setup with only VGA console doesn't stress enough? 
Anyway I didn't notice any regression either.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>