| Message ID | 20170227093830.3416-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 27 Feb 2017 10:38:29 +0100, Hans de Goede wrote: > > index a8e74ca..a4ac473 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -79,7 +79,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_runtime_disabled: true if pm runtime is disabled > + * @pm_disabled: true if power-management should be disabled for this i2c-bus > * > * HCNT and LCNT parameters can be used if the platform knows more accurate > * values than the one computed based only on the input clock frequency. > @@ -127,7 +127,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_runtime_disabled; > + bool pm_disabled; > bool dynamic_tar_update_enabled; I couldn't find this dynamic_tar_update_enabled field in your previous patchset. What am I missing? thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 27, 2017 at 11:25:01AM +0100, Takashi Iwai wrote: > On Mon, 27 Feb 2017 10:38:29 +0100, > Hans de Goede wrote: > > > > index a8e74ca..a4ac473 100644 > > --- a/drivers/i2c/busses/i2c-designware-core.h > > +++ b/drivers/i2c/busses/i2c-designware-core.h > > @@ -79,7 +79,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_runtime_disabled: true if pm runtime is disabled > > + * @pm_disabled: true if power-management should be disabled for this i2c-bus > > * > > * HCNT and LCNT parameters can be used if the platform knows more accurate > > * values than the one computed based only on the input clock frequency. > > @@ -127,7 +127,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_runtime_disabled; > > + bool pm_disabled; > > bool dynamic_tar_update_enabled; > > I couldn't find this dynamic_tar_update_enabled field in your previous > patchset. What am I missing? It got reverted with 12688dc21f71f4 ("Revert "i2c: designware: detect when dynamic tar update is possible"") around 4.10-rc7 time. I also wondered that Hans didn't get a merge conflict somewhere.
Hi, On 27-02-17 11:25, Takashi Iwai wrote: > On Mon, 27 Feb 2017 10:38:29 +0100, > Hans de Goede wrote: >> >> index a8e74ca..a4ac473 100644 >> --- a/drivers/i2c/busses/i2c-designware-core.h >> +++ b/drivers/i2c/busses/i2c-designware-core.h >> @@ -79,7 +79,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_runtime_disabled: true if pm runtime is disabled >> + * @pm_disabled: true if power-management should be disabled for this i2c-bus >> * >> * HCNT and LCNT parameters can be used if the platform knows more accurate >> * values than the one computed based only on the input clock frequency. >> @@ -127,7 +127,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_runtime_disabled; >> + bool pm_disabled; >> bool dynamic_tar_update_enabled; > > I couldn't find this dynamic_tar_update_enabled field in your previous > patchset. What am I missing? My testing branch for all this stuff is based on intel-drm-next-queued, which is still based on 4.10-rc$, and it seems that 4.11-rc1 will have this: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=12688dc21f71f4dcc9e2b8b5556b0c6cc8df1491 Removing the dynamic_tar_update_enabled member from that struct. I will send out a new rebased version when 4.11-rc1 gets merged in intel-drm-next-queued. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 27 Feb 2017 11:32:45 +0100, Hans de Goede wrote: > > Hi, > > On 27-02-17 11:25, Takashi Iwai wrote: > > On Mon, 27 Feb 2017 10:38:29 +0100, > > Hans de Goede wrote: > >> > >> index a8e74ca..a4ac473 100644 > >> --- a/drivers/i2c/busses/i2c-designware-core.h > >> +++ b/drivers/i2c/busses/i2c-designware-core.h > >> @@ -79,7 +79,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_runtime_disabled: true if pm runtime is disabled > >> + * @pm_disabled: true if power-management should be disabled for this i2c-bus > >> * > >> * HCNT and LCNT parameters can be used if the platform knows more accurate > >> * values than the one computed based only on the input clock frequency. > >> @@ -127,7 +127,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_runtime_disabled; > >> + bool pm_disabled; > >> bool dynamic_tar_update_enabled; > > > > I couldn't find this dynamic_tar_update_enabled field in your previous > > patchset. What am I missing? > > My testing branch for all this stuff is based on intel-drm-next-queued, which > is still based on 4.10-rc$, and it seems that 4.11-rc1 will have this: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=12688dc21f71f4dcc9e2b8b5556b0c6cc8df1491 > > Removing the dynamic_tar_update_enabled member from that struct. > > I will send out a new rebased version when 4.11-rc1 gets merged in > intel-drm-next-queued. Alright, thanks! I'm testing the stuff right now. Since I didn't have the issue on my machine, I can't say whether it fixes or not. But at least it doesn't give any regression, so far ;) I've put it to topic/i2c-dw-cherrytrail branch for anyone who wants to try. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c index 1749a0f..b8cc226 100644 --- a/drivers/i2c/busses/i2c-designware-baytrail.c +++ b/drivers/i2c/busses/i2c-designware-baytrail.c @@ -170,7 +170,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_runtime_disabled = true; + dev->pm_disabled = 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.c b/drivers/i2c/busses/i2c-designware-core.c index 8c3ba42..3d86059 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -962,6 +962,7 @@ EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param); int i2c_dw_probe(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; + unsigned long irq_flags; int r; u32 reg; @@ -998,9 +999,14 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); + if (dev->pm_disabled) { + dev_pm_syscore_device(dev->dev, true); + irq_flags = IRQF_NO_SUSPEND; + } else + irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND; + i2c_dw_disable_int(dev); - r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, - IRQF_SHARED | IRQF_COND_SUSPEND, + r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags, dev_name(dev->dev), dev); if (r) { dev_err(dev->dev, "failure requesting irq %i: %d\n", diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index a8e74ca..a4ac473 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -79,7 +79,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_runtime_disabled: true if pm runtime is disabled + * @pm_disabled: true if power-management should be disabled for this i2c-bus * * HCNT and LCNT parameters can be used if the platform knows more accurate * values than the one computed based only on the input clock frequency. @@ -127,7 +127,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_runtime_disabled; + bool pm_disabled; bool dynamic_tar_update_enabled; }; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index df0ff7d..8ed96dd 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -276,7 +276,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->dev.of_node = pdev->dev.of_node; - if (dev->pm_runtime_disabled) { + if (dev->pm_disabled) { pm_runtime_forbid(&pdev->dev); } else { pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); @@ -286,7 +286,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) } r = i2c_dw_probe(dev); - if (r && !dev->pm_runtime_disabled) + if (r && !dev->pm_disabled) pm_runtime_disable(&pdev->dev); return r; @@ -304,7 +304,7 @@ static int dw_i2c_plat_remove(struct platform_device *pdev) pm_runtime_dont_use_autosuspend(&pdev->dev); pm_runtime_put_sync(&pdev->dev); - if (!dev->pm_runtime_disabled) + if (!dev->pm_disabled) pm_runtime_disable(&pdev->dev); i2c_dw_remove_lock_support(dev); @@ -354,9 +354,7 @@ static int dw_i2c_plat_resume(struct device *dev) struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); i2c_dw_plat_prepare_clk(i_dev, true); - - if (!i_dev->pm_runtime_disabled) - i2c_dw_init(i_dev); + i2c_dw_init(i_dev); return 0; }
Currently we are already setting a pm_runtime_disabled flag and disabling runtime-pm for i2c-busses used for accessing the system PMIC on x86. But this is not enough, there are ACPI opregions which may want to access the PMIC during late-suspend and early-resume, so we need to completely disable pm to be safe. This commit renames the flag from pm_runtime_disabled to pm_disabled and adds the following new behavior if the flag is set: 1) Call dev_pm_syscore_device(dev, true) which disables normal suspend / resume and remove the pm_runtime_disabled check from dw_i2c_plat_resume since that will now never get called. This fixes suspend_late handlers which use ACPI PMIC opregions causing errors like these: PM: Suspending system (freeze) PM: suspend of devices complete after 1127.751 msecs i2c_designware 808622C1:06: timeout waiting for bus ready ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion] acpi 80860F14:02: Failed to change power state to D3hot PM: late suspend of devices failed 2) Set IRQF_NO_SUSPEND irq flag. This fixes resume_early handlers which handlers which use ACPI PMIC opregions causing errors like these: PM: resume from suspend-to-idle i2c_designware 808622C1:06: controller timed out ACPI Exception: AE_ERROR, Returned by Handler for [UserDefinedRegion] Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-designware-baytrail.c | 2 +- drivers/i2c/busses/i2c-designware-core.c | 10 ++++++++-- drivers/i2c/busses/i2c-designware-core.h | 4 ++-- drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++------ 4 files changed, 15 insertions(+), 11 deletions(-)