| Message ID | 20180829152753.15041-1-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: designware: Remove Cherry Trail PMIC I2C bus pm_disabled workaround | expand |
On Wed, Aug 29, 2018 at 05:27:53PM +0200, Hans de Goede wrote: > Commit a3d411fb38c0 ("i2c: designware: Disable pm for PMIC i2c-bus even if > there is no _SEM method"), always set the pm_disabled flag on the I2C7 > controller, even if its bus was not shared with the PUNIT. > > This was a workaround for various suspend/resume issues, after the > following 2 commits this workaround is no longer necessary: > > Commit 541527728341 ("PM: i2c-designware-platdrv: Suspend/resume at the > late/early stages") > Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card > dependency on I2C") > > Therefor this commit removes this workaround. > > After this commit the pm_disabled flag is only used to indicate that the > bus is shared with the PUNIT and after other recent changes we no longer > call dev_pm_syscore_device(dev, true), so we are no longer actually > disabling (non-runtime) pm, so this commit also renames the flag to > shared_with_punit to better reflect what it is for. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Waiting for tags here... I assume it is OK to have applied patch 1/2 from the previous series independently of this patch?
On Thu, Aug 30, 2018 at 11:05:41PM +0200, Wolfram Sang wrote: > On Wed, Aug 29, 2018 at 05:27:53PM +0200, Hans de Goede wrote: > > Commit a3d411fb38c0 ("i2c: designware: Disable pm for PMIC i2c-bus even if > > there is no _SEM method"), always set the pm_disabled flag on the I2C7 > > controller, even if its bus was not shared with the PUNIT. > > > > This was a workaround for various suspend/resume issues, after the > > following 2 commits this workaround is no longer necessary: > > > > Commit 541527728341 ("PM: i2c-designware-platdrv: Suspend/resume at the > > late/early stages") > > Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card > > dependency on I2C") > > > > Therefor this commit removes this workaround. > > > > After this commit the pm_disabled flag is only used to indicate that the > > bus is shared with the PUNIT and after other recent changes we no longer > > call dev_pm_syscore_device(dev, true), so we are no longer actually > > disabling (non-runtime) pm, so this commit also renames the flag to > > shared_with_punit to better reflect what it is for. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Waiting for tags here... I assume it is OK to have applied patch 1/2 > from the previous series independently of this patch? I'm fine with it Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On 08/31/2018 10:17 AM, Andy Shevchenko wrote: > On Thu, Aug 30, 2018 at 11:05:41PM +0200, Wolfram Sang wrote: >> On Wed, Aug 29, 2018 at 05:27:53PM +0200, Hans de Goede wrote: >>> Commit a3d411fb38c0 ("i2c: designware: Disable pm for PMIC i2c-bus even if >>> there is no _SEM method"), always set the pm_disabled flag on the I2C7 >>> controller, even if its bus was not shared with the PUNIT. >>> >>> This was a workaround for various suspend/resume issues, after the >>> following 2 commits this workaround is no longer necessary: >>> >>> Commit 541527728341 ("PM: i2c-designware-platdrv: Suspend/resume at the >>> late/early stages") >>> Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card >>> dependency on I2C") >>> >>> Therefor this commit removes this workaround. >>> >>> After this commit the pm_disabled flag is only used to indicate that the >>> bus is shared with the PUNIT and after other recent changes we no longer >>> call dev_pm_syscore_device(dev, true), so we are no longer actually >>> disabling (non-runtime) pm, so this commit also renames the flag to >>> shared_with_punit to better reflect what it is for. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> Waiting for tags here... I assume it is OK to have applied patch 1/2 >> from the previous series independently of this patch? > > I'm fine with it > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Oh I didn't notice yesterday there was already v2 of this patch. I have here a Cherry Trail without _SEM method and after this patch 808622C1:06 is runtime power managed but didn't see any issue at quick test. Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Hi, On 30-08-18 23:05, Wolfram Sang wrote: > On Wed, Aug 29, 2018 at 05:27:53PM +0200, Hans de Goede wrote: >> Commit a3d411fb38c0 ("i2c: designware: Disable pm for PMIC i2c-bus even if >> there is no _SEM method"), always set the pm_disabled flag on the I2C7 >> controller, even if its bus was not shared with the PUNIT. >> >> This was a workaround for various suspend/resume issues, after the >> following 2 commits this workaround is no longer necessary: >> >> Commit 541527728341 ("PM: i2c-designware-platdrv: Suspend/resume at the >> late/early stages") >> Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card >> dependency on I2C") >> >> Therefor this commit removes this workaround. >> >> After this commit the pm_disabled flag is only used to indicate that the >> bus is shared with the PUNIT and after other recent changes we no longer >> call dev_pm_syscore_device(dev, true), so we are no longer actually >> disabling (non-runtime) pm, so this commit also renames the flag to >> shared_with_punit to better reflect what it is for. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Waiting for tags here... I assume it is OK to have applied patch 1/2 > from the previous series independently of this patch? Yes that is fine, given that that is a bugfix for a regression introduced in 4.18 it is actually a good thing that you've already applied patch 1/2. I assume you've added this to a fixes branch? Thank you for quickly merging this. Regards, Hans
Hi, On 31-08-18 11:57, Hans de Goede wrote: > Hi, > > On 30-08-18 23:05, Wolfram Sang wrote: >> On Wed, Aug 29, 2018 at 05:27:53PM +0200, Hans de Goede wrote: >>> Commit a3d411fb38c0 ("i2c: designware: Disable pm for PMIC i2c-bus even if >>> there is no _SEM method"), always set the pm_disabled flag on the I2C7 >>> controller, even if its bus was not shared with the PUNIT. >>> >>> This was a workaround for various suspend/resume issues, after the >>> following 2 commits this workaround is no longer necessary: >>> >>> Commit 541527728341 ("PM: i2c-designware-platdrv: Suspend/resume at the >>> late/early stages") >>> Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card >>> dependency on I2C") >>> >>> Therefor this commit removes this workaround. >>> >>> After this commit the pm_disabled flag is only used to indicate that the >>> bus is shared with the PUNIT and after other recent changes we no longer >>> call dev_pm_syscore_device(dev, true), so we are no longer actually >>> disabling (non-runtime) pm, so this commit also renames the flag to >>> shared_with_punit to better reflect what it is for. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> Waiting for tags here... I assume it is OK to have applied patch 1/2 >> from the previous series independently of this patch? > > Yes that is fine, given that that is a bugfix for a regression > introduced in 4.18 it is actually a good thing that you've already > applied patch 1/2. I assume you've added this to a fixes branch? Never mind I just saw you said you put it in for current. Regards, Hans
> Yes that is fine, given that that is a bugfix for a regression > introduced in 4.18 it is actually a good thing that you've already > applied patch 1/2. I assume you've added this to a fixes branch? Yes, for-current means fixes :) This patch here doesn't seem urgent, so it would be more for-next material? > Thank you for quickly merging this. You are welcome!
Hi, On 31-08-18 12:01, Wolfram Sang wrote: > >> Yes that is fine, given that that is a bugfix for a regression >> introduced in 4.18 it is actually a good thing that you've already >> applied patch 1/2. I assume you've added this to a fixes branch? > > Yes, for-current means fixes :) This patch here doesn't seem urgent, so > it would be more for-next material? Yes this patch is more for-next material. Regards, Hans
Hi Wolfram, On 31-08-18 12:03, Hans de Goede wrote: > Hi, > > On 31-08-18 12:01, Wolfram Sang wrote: >> >>> Yes that is fine, given that that is a bugfix for a regression >>> introduced in 4.18 it is actually a good thing that you've already >>> applied patch 1/2. I assume you've added this to a fixes branch? >> >> Yes, for-current means fixes :) This patch here doesn't seem urgent, so >> it would be more for-next material? > > Yes this patch is more for-next material. I noticed that you've merged a couple of i2c-designware patches into your next branch, but not this one. I know it is still very early in the next cycle, so no need to hurry, I just want to make sure this does not fall through the cracks. Regards, Hans
> I noticed that you've merged a couple of i2c-designware patches > into your next branch, but not this one. Yeah, sorry, I forgot to tell you. It doesn't apply on top of these patches. Do you have time to rebase it on top of i2c/for-next?
Hi, On 05-09-18 12:13, Wolfram Sang wrote: > >> I noticed that you've merged a couple of i2c-designware patches >> into your next branch, but not this one. > > Yeah, sorry, I forgot to tell you. It doesn't apply on top of these > patches. Do you have time to rebase it on top of i2c/for-next? Sure, I've just completed the rebase. I will send out v3 (which is just a rebase compared to v2) directly after this mail. Regards, Hans
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c index dbda8c9c8a1c..812438cce341 100644 --- a/drivers/i2c/busses/i2c-designware-baytrail.c +++ b/drivers/i2c/busses/i2c-designware-baytrail.c @@ -172,7 +172,7 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) dev_info(dev->dev, "I2C bus managed by PUNIT\n"); dev->acquire_lock = baytrail_i2c_acquire; dev->release_lock = baytrail_i2c_release; - dev->pm_disabled = true; + dev->shared_with_punit = true; pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index d690e648bc01..f90de032e8ec 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -224,7 +224,7 @@ * @pm_qos: pm_qos_request used while holding a hardware lock on the bus * @acquire_lock: function to acquire a hardware lock on the bus * @release_lock: function to release a hardware lock on the bus - * @pm_disabled: true if power-management should be disabled for this i2c-bus + * @shared_with_punit: true if this bus is shared with the SoCs PUNIT * @disable: function to disable the controller * @disable_int: function to disable all interrupts * @init: function to initialize the I2C hardware @@ -279,7 +279,7 @@ struct dw_i2c_dev { struct pm_qos_request pm_qos; int (*acquire_lock)(struct dw_i2c_dev *dev); void (*release_lock)(struct dw_i2c_dev *dev); - bool pm_disabled; + bool shared_with_punit; void (*disable)(struct dw_i2c_dev *dev); void (*disable_int)(struct dw_i2c_dev *dev); int (*init)(struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 54b2a3a86677..b9765828da94 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -692,7 +692,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); - if (dev->pm_disabled) { + if (dev->shared_with_punit) { 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 d281d21cdd8e..fcb07be8f684 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -97,10 +97,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev) { struct dw_i2c_dev *dev = platform_get_drvdata(pdev); u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0; - acpi_handle handle = ACPI_HANDLE(&pdev->dev); const struct acpi_device_id *id; - struct acpi_device *adev; - const char *uid; dev->adapter.nr = -1; dev->tx_fifo_depth = 32; @@ -135,18 +132,6 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev) if (id && id->driver_data) dev->flags |= (u32)id->driver_data; - if (acpi_bus_get_device(handle, &adev)) - return -ENODEV; - - /* - * Cherrytrail I2C7 gets used for the PMIC which gets accessed - * through ACPI opregions during late suspend / early resume - * disable pm for it. - */ - uid = adev->pnp.unique_id; - if ((dev->flags & MODEL_CHERRYTRAIL) && !strcmp(uid, "7")) - dev->pm_disabled = true; - return 0; } @@ -231,7 +216,7 @@ static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev) { pm_runtime_disable(dev->dev); - if (dev->pm_disabled) + if (dev->shared_with_punit) pm_runtime_put_noidle(dev->dev); } @@ -362,7 +347,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_active(&pdev->dev); - if (dev->pm_disabled) + if (dev->shared_with_punit) pm_runtime_get_noresume(&pdev->dev); pm_runtime_enable(&pdev->dev); @@ -448,7 +433,7 @@ static int dw_i2c_plat_suspend(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); - if (i_dev->pm_disabled) + if (i_dev->shared_with_punit) return 0; i_dev->disable(i_dev); @@ -461,7 +446,7 @@ static int dw_i2c_plat_resume(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); - if (!i_dev->pm_disabled) + if (!i_dev->shared_with_punit) i2c_dw_prepare_clk(i_dev, true); i_dev->init(i_dev);
Commit a3d411fb38c0 ("i2c: designware: Disable pm for PMIC i2c-bus even if there is no _SEM method"), always set the pm_disabled flag on the I2C7 controller, even if its bus was not shared with the PUNIT. This was a workaround for various suspend/resume issues, after the following 2 commits this workaround is no longer necessary: Commit 541527728341 ("PM: i2c-designware-platdrv: Suspend/resume at the late/early stages") Commit e6ce0ce34f65 ("ACPI / LPSS: Add device link for CHT SD card dependency on I2C") Therefor this commit removes this workaround. After this commit the pm_disabled flag is only used to indicate that the bus is shared with the PUNIT and after other recent changes we no longer call dev_pm_syscore_device(dev, true), so we are no longer actually disabling (non-runtime) pm, so this commit also renames the flag to shared_with_punit to better reflect what it is for. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Keep setting IRQF_NO_SUSPEND for devices which set shared_with_punit, otherwise they will not resume properly --- drivers/i2c/busses/i2c-designware-baytrail.c | 2 +- drivers/i2c/busses/i2c-designware-core.h | 4 ++-- drivers/i2c/busses/i2c-designware-master.c | 2 +- drivers/i2c/busses/i2c-designware-platdrv.c | 23 ++++---------------- 4 files changed, 8 insertions(+), 23 deletions(-)