| Message ID | 20180919191518.18764-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Resume BYT/CHT LPSS I2C controller before the iGPU | expand |
On 09/19/2018 10:15 PM, Hans de Goede wrote: > On some Cherry Trail systems the GPU ACPI fwnode has power-resources which > point to the PMIC, which is connected over a LPSS I2C controller. The GPU > is a PCI device and PCI devices are powered-on at the resume_noirq resume > phase. > > Since the GPU power-resources need the I2C controller, recent acpi_lpss.c > changes now also power-up the LPSS I2C controllers on BYT and CHT devices > in the resume_noirq resume phase. But during this phase the IRQ of the > controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND > flag when requesting the interrupt on BYT and CHT devices, so that the IRQ > is left enabled during the noirq phase, fixing this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-master.c | 2 +- > drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- > 3 files changed, 4 insertions(+), 3 deletions(-) > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Thursday, September 20, 2018 3:08:28 PM CEST Jarkko Nikula wrote: > On 09/19/2018 10:15 PM, Hans de Goede wrote: > > On some Cherry Trail systems the GPU ACPI fwnode has power-resources which > > point to the PMIC, which is connected over a LPSS I2C controller. The GPU > > is a PCI device and PCI devices are powered-on at the resume_noirq resume > > phase. > > > > Since the GPU power-resources need the I2C controller, recent acpi_lpss.c > > changes now also power-up the LPSS I2C controllers on BYT and CHT devices > > in the resume_noirq resume phase. But during this phase the IRQ of the > > controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND > > flag when requesting the interrupt on BYT and CHT devices, so that the IRQ > > is left enabled during the noirq phase, fixing this. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/i2c/busses/i2c-designware-core.h | 1 + > > drivers/i2c/busses/i2c-designware-master.c | 2 +- > > drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- > > 3 files changed, 4 insertions(+), 3 deletions(-) > > > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Thanks! Wolfram, any objections here?
Hi Rafael, On 09/21/2018 01:11 PM, Rafael J. Wysocki wrote: > On Thursday, September 20, 2018 3:08:28 PM CEST Jarkko Nikula wrote: >> On 09/19/2018 10:15 PM, Hans de Goede wrote: >>> On some Cherry Trail systems the GPU ACPI fwnode has power-resources which >>> point to the PMIC, which is connected over a LPSS I2C controller. The GPU >>> is a PCI device and PCI devices are powered-on at the resume_noirq resume >>> phase. >>> >>> Since the GPU power-resources need the I2C controller, recent acpi_lpss.c >>> changes now also power-up the LPSS I2C controllers on BYT and CHT devices >>> in the resume_noirq resume phase. But during this phase the IRQ of the >>> controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND >>> flag when requesting the interrupt on BYT and CHT devices, so that the IRQ >>> is left enabled during the noirq phase, fixing this. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/i2c/busses/i2c-designware-core.h | 1 + >>> drivers/i2c/busses/i2c-designware-master.c | 2 +- >>> drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- >>> 3 files changed, 4 insertions(+), 3 deletions(-) >>> >> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> > > Thanks! > > Wolfram, any objections here? Although it would be good to get Wolfram's feedback here please note that the intend is for patches 1-6 to be merged through your tree and this patch through Wolfram's tree since both the acpi_lpss code and the i2c-designware code have seen some changes recently. As mentioned in the coverletter the order of merging these is not important, the bug will not be fixed until both are present but otherwise they can be merged in any order, so dividing them over 2 trees is not a problem. Regards, Hans
On Friday, September 21, 2018 4:05:57 PM CEST Hans de Goede wrote: > Hi Rafael, > > On 09/21/2018 01:11 PM, Rafael J. Wysocki wrote: > > On Thursday, September 20, 2018 3:08:28 PM CEST Jarkko Nikula wrote: > >> On 09/19/2018 10:15 PM, Hans de Goede wrote: > >>> On some Cherry Trail systems the GPU ACPI fwnode has power-resources which > >>> point to the PMIC, which is connected over a LPSS I2C controller. The GPU > >>> is a PCI device and PCI devices are powered-on at the resume_noirq resume > >>> phase. > >>> > >>> Since the GPU power-resources need the I2C controller, recent acpi_lpss.c > >>> changes now also power-up the LPSS I2C controllers on BYT and CHT devices > >>> in the resume_noirq resume phase. But during this phase the IRQ of the > >>> controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND > >>> flag when requesting the interrupt on BYT and CHT devices, so that the IRQ > >>> is left enabled during the noirq phase, fixing this. > >>> > >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>> --- > >>> drivers/i2c/busses/i2c-designware-core.h | 1 + > >>> drivers/i2c/busses/i2c-designware-master.c | 2 +- > >>> drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- > >>> 3 files changed, 4 insertions(+), 3 deletions(-) > >>> > >> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > >> > > > > Thanks! > > > > Wolfram, any objections here? > > Although it would be good to get Wolfram's feedback here please note > that the intend is for patches 1-6 to be merged through your tree > and this patch through Wolfram's tree since both the acpi_lpss code > and the i2c-designware code have seen some changes recently. OK > As mentioned in the coverletter the order of merging these is not > important, the bug will not be fixed until both are present but > otherwise they can be merged in any order, so dividing them > over 2 trees is not a problem. Not really a problem technichally, but it may make it sort of hard to backport the fixes, as that would require things to be tracked down to the submission of the patches. Thanks, Rafael
Hi, On 27-09-18 11:31, Rafael J. Wysocki wrote: > On Friday, September 21, 2018 4:05:57 PM CEST Hans de Goede wrote: >> Hi Rafael, >> >> On 09/21/2018 01:11 PM, Rafael J. Wysocki wrote: >>> On Thursday, September 20, 2018 3:08:28 PM CEST Jarkko Nikula wrote: >>>> On 09/19/2018 10:15 PM, Hans de Goede wrote: >>>>> On some Cherry Trail systems the GPU ACPI fwnode has power-resources which >>>>> point to the PMIC, which is connected over a LPSS I2C controller. The GPU >>>>> is a PCI device and PCI devices are powered-on at the resume_noirq resume >>>>> phase. >>>>> >>>>> Since the GPU power-resources need the I2C controller, recent acpi_lpss.c >>>>> changes now also power-up the LPSS I2C controllers on BYT and CHT devices >>>>> in the resume_noirq resume phase. But during this phase the IRQ of the >>>>> controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND >>>>> flag when requesting the interrupt on BYT and CHT devices, so that the IRQ >>>>> is left enabled during the noirq phase, fixing this. >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> drivers/i2c/busses/i2c-designware-core.h | 1 + >>>>> drivers/i2c/busses/i2c-designware-master.c | 2 +- >>>>> drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- >>>>> 3 files changed, 4 insertions(+), 3 deletions(-) >>>>> >>>> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >>>> >>> >>> Thanks! >>> >>> Wolfram, any objections here? >> >> Although it would be good to get Wolfram's feedback here please note >> that the intend is for patches 1-6 to be merged through your tree >> and this patch through Wolfram's tree since both the acpi_lpss code >> and the i2c-designware code have seen some changes recently. > > OK > >> As mentioned in the coverletter the order of merging these is not >> important, the bug will not be fixed until both are present but >> otherwise they can be merged in any order, so dividing them >> over 2 trees is not a problem. > > Not really a problem technichally, but it may make it sort of hard to backport > the fixes, as that would require things to be tracked down to the submission > of the patches. True. The problem is that Wolfram's for-next branch has this commit: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?id=9cbeeca05049b1109e7e445369898b8a88d5ea7b Which patch 7 depends on, so if you want to merge all commits through your tree then either you need to cherry-pick that one first, or Wolfram needs to create an immutable branch with that commit for you to merge. And then you need to create an immutable branch for Wolfram to merge further i2c-designware commits, since patchwork has a few more i2c-designware patches pending. Not sure this is worth the trouble to make backporting easier, but I will let you and Wolfram figure this out. Regards, Hans
On Thu, Sep 27, 2018 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 27-09-18 11:31, Rafael J. Wysocki wrote: > > On Friday, September 21, 2018 4:05:57 PM CEST Hans de Goede wrote: > >> Hi Rafael, > >> > >> On 09/21/2018 01:11 PM, Rafael J. Wysocki wrote: > >>> On Thursday, September 20, 2018 3:08:28 PM CEST Jarkko Nikula wrote: > >>>> On 09/19/2018 10:15 PM, Hans de Goede wrote: > >>>>> On some Cherry Trail systems the GPU ACPI fwnode has power-resources which > >>>>> point to the PMIC, which is connected over a LPSS I2C controller. The GPU > >>>>> is a PCI device and PCI devices are powered-on at the resume_noirq resume > >>>>> phase. > >>>>> > >>>>> Since the GPU power-resources need the I2C controller, recent acpi_lpss.c > >>>>> changes now also power-up the LPSS I2C controllers on BYT and CHT devices > >>>>> in the resume_noirq resume phase. But during this phase the IRQ of the > >>>>> controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND > >>>>> flag when requesting the interrupt on BYT and CHT devices, so that the IRQ > >>>>> is left enabled during the noirq phase, fixing this. > >>>>> > >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>>> --- > >>>>> drivers/i2c/busses/i2c-designware-core.h | 1 + > >>>>> drivers/i2c/busses/i2c-designware-master.c | 2 +- > >>>>> drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- > >>>>> 3 files changed, 4 insertions(+), 3 deletions(-) > >>>>> > >>>> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > >>>> > >>> > >>> Thanks! > >>> > >>> Wolfram, any objections here? > >> > >> Although it would be good to get Wolfram's feedback here please note > >> that the intend is for patches 1-6 to be merged through your tree > >> and this patch through Wolfram's tree since both the acpi_lpss code > >> and the i2c-designware code have seen some changes recently. > > > > OK > > > >> As mentioned in the coverletter the order of merging these is not > >> important, the bug will not be fixed until both are present but > >> otherwise they can be merged in any order, so dividing them > >> over 2 trees is not a problem. > > > > Not really a problem technichally, but it may make it sort of hard to backport > > the fixes, as that would require things to be tracked down to the submission > > of the patches. > > True. > > The problem is that Wolfram's for-next branch has this commit: > https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?id=9cbeeca05049b1109e7e445369898b8a88d5ea7b > > Which patch 7 depends on, so if you want to merge all commits through your > tree then either you need to cherry-pick that one first, or Wolfram needs > to create an immutable branch with that commit for you to merge. > > And then you need to create an immutable branch for Wolfram to merge > further i2c-designware commits, since patchwork has a few more > i2c-designware patches pending. > > Not sure this is worth the trouble to make backporting easier, > but I will let you and Wolfram figure this out. I'll take patches [1-6/7] to start with (when I get back home from here, so likely tomorrow) and then we'll see what to do with the last one. Thanks, Rafael
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 69b8a5df3866..450a4135af63 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -276,6 +276,7 @@ struct dw_i2c_dev { #define ACCESS_SWAP 0x00000001 #define ACCESS_16BIT 0x00000002 #define ACCESS_INTR_MASK 0x00000004 +#define ACCESS_NO_IRQ_SUSPEND 0x00000008 #define MODEL_CHERRYTRAIL 0x00000100 diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 2ccb527735f9..2a630ac35ba2 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -707,7 +707,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); - if (dev->shared_with_punit) { + if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { irq_flags = IRQF_NO_SUSPEND; } else { irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index e92fdb46b5a0..3f0289a8233d 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -123,8 +123,8 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = { { "INT33C3", 0 }, { "INT3432", 0 }, { "INT3433", 0 }, - { "80860F41", 0 }, - { "808622C1", MODEL_CHERRYTRAIL }, + { "80860F41", ACCESS_NO_IRQ_SUSPEND }, + { "808622C1", ACCESS_NO_IRQ_SUSPEND | MODEL_CHERRYTRAIL }, { "AMD0010", ACCESS_INTR_MASK }, { "AMDI0010", ACCESS_INTR_MASK }, { "AMDI0510", 0 },
On some Cherry Trail systems the GPU ACPI fwnode has power-resources which point to the PMIC, which is connected over a LPSS I2C controller. The GPU is a PCI device and PCI devices are powered-on at the resume_noirq resume phase. Since the GPU power-resources need the I2C controller, recent acpi_lpss.c changes now also power-up the LPSS I2C controllers on BYT and CHT devices in the resume_noirq resume phase. But during this phase the IRQ of the controller is disabled leading to these 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 commit makes the i2c-designware controller set the IRQF_NO_SUSPEND flag when requesting the interrupt on BYT and CHT devices, so that the IRQ is left enabled during the noirq phase, fixing this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-master.c | 2 +- drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-)