| Message ID | 20180923135812.29574-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Resume BYT/CHT LPSS I2C controller before the iGPU | expand |
On Sun, Sep 23, 2018 at 03:58:05PM +0200, Hans de Goede wrote: > Hi All, > > Here is v2 of my patch-series to make sure that on BYT/CHT systems the I2C > controller connected to the PMIC gets resumed before the GPU. > > New in v2: > -Dropped "ACPI / LPSS: Only add device links from consumer to supplier" patch > this was based on wrongly reading the code and the code it removed is fine > -Added "ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller" > it turns out we need to order things correctly on BYT too, otherwise things > also fail, but in a much nastier way then on CHT, see the commit msg of > the new patch > > Below is the original coverletter of v1 of the patch-set: > > 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-3 rework the acpi_lpss.c device-link code added by commit > e6ce0ce34f65 ("ACPI / 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 4 an 5 then actually add the device-links. 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. > Thanks for this work! Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Regards, > > Hans >
On Sunday, September 23, 2018 3:58:05 PM CEST Hans de Goede wrote: > Hi All, > > Here is v2 of my patch-series to make sure that on BYT/CHT systems the I2C > controller connected to the PMIC gets resumed before the GPU. > > New in v2: > -Dropped "ACPI / LPSS: Only add device links from consumer to supplier" patch > this was based on wrongly reading the code and the code it removed is fine > -Added "ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller" > it turns out we need to order things correctly on BYT too, otherwise things > also fail, but in a much nastier way then on CHT, see the commit msg of > the new patch > > Below is the original coverletter of v1 of the patch-set: > > 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-3 rework the acpi_lpss.c device-link code added by commit > e6ce0ce34f65 ("ACPI / 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 4 an 5 then actually add the device-links. 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. Patches [0-6/7] have been applied. I guess the [7/7] can go in via the I2C tree if everyone involved agrees. Thanks, Rafael
> Patches [0-6/7] have been applied. > > I guess the [7/7] can go in via the I2C tree if everyone involved agrees. I agree, but it doesn't apply, neither on i2c/for-4.20 nor i2c/for-next. Hans, do you have time to check?
Hi, On 05-10-18 18:02, Wolfram Sang wrote: > >> Patches [0-6/7] have been applied. >> >> I guess the [7/7] can go in via the I2C tree if everyone involved agrees. > > I agree, but it doesn't apply, neither on i2c/for-4.20 nor i2c/for-next. > > Hans, do you have time to check? Yes, rebasing now I will send out a rebased version soon (after running a few tests). Regards, Hans
Hi All, Here is v2 of my patch-series to make sure that on BYT/CHT systems the I2C controller connected to the PMIC gets resumed before the GPU. New in v2: -Dropped "ACPI / LPSS: Only add device links from consumer to supplier" patch this was based on wrongly reading the code and the code it removed is fine -Added "ACPI / LPSS: Add a device link from the GPU to the BYT I2C5 controller" it turns out we need to order things correctly on BYT too, otherwise things also fail, but in a much nastier way then on CHT, see the commit msg of the new patch Below is the original coverletter of v1 of the patch-set: 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-3 rework the acpi_lpss.c device-link code added by commit e6ce0ce34f65 ("ACPI / 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 4 an 5 then actually add the device-links. 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