| Message ID | 20180829130632.31111-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] i2c: designware: Re-init controllers with pm_disabled set on resume | expand |
Hi, On 29-08-18 15:06, 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. > > Since we now no longer keep the I2C controller alive during suspend, we > also no longer need to set IRQF_NO_SUSPEND. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Self-nack this breaks resume from suspend on devices where the PUNIT shares the i2c bus, we need to keep IRQF_NO_SUSPEND there. youling, thank you for catching the problem with v1 of this patch. I will send a v2 of this patch fixing this. Note I've tested patch 1 of this set extensively, this was a quick add-on to the set. Since patch 1 fixes a regression it would be good to get that merged soon. Regards, Hans > --- > drivers/i2c/busses/i2c-designware-baytrail.c | 2 +- > drivers/i2c/busses/i2c-designware-core.h | 4 ++-- > drivers/i2c/busses/i2c-designware-master.c | 10 ++------- > drivers/i2c/busses/i2c-designware-platdrv.c | 23 ++++---------------- > 4 files changed, 9 insertions(+), 30 deletions(-) > > 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..d563a2c2ec97 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -672,7 +672,6 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) > int i2c_dw_probe(struct dw_i2c_dev *dev) > { > struct i2c_adapter *adap = &dev->adapter; > - unsigned long irq_flags; > int ret; > > init_completion(&dev->cmd_complete); > @@ -692,14 +691,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) > adap->dev.parent = dev->dev; > i2c_set_adapdata(adap, dev); > > - if (dev->pm_disabled) { > - irq_flags = IRQF_NO_SUSPEND; > - } else { > - irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND; > - } > - > i2c_dw_disable_int(dev); > - ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags, > + ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, > + IRQF_SHARED | IRQF_COND_SUSPEND, > dev_name(dev->dev), dev); > if (ret) { > dev_err(dev->dev, "failure requesting irq %i: %d\n", > 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); >
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..d563a2c2ec97 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -672,7 +672,6 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) int i2c_dw_probe(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; - unsigned long irq_flags; int ret; init_completion(&dev->cmd_complete); @@ -692,14 +691,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); - if (dev->pm_disabled) { - irq_flags = IRQF_NO_SUSPEND; - } else { - irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND; - } - i2c_dw_disable_int(dev); - ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags, + ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, + IRQF_SHARED | IRQF_COND_SUSPEND, dev_name(dev->dev), dev); if (ret) { dev_err(dev->dev, "failure requesting irq %i: %d\n", 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. Since we now no longer keep the I2C controller alive during suspend, we also no longer need to set IRQF_NO_SUSPEND. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-designware-baytrail.c | 2 +- drivers/i2c/busses/i2c-designware-core.h | 4 ++-- drivers/i2c/busses/i2c-designware-master.c | 10 ++------- drivers/i2c/busses/i2c-designware-platdrv.c | 23 ++++---------------- 4 files changed, 9 insertions(+), 30 deletions(-)