From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v4 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore Date: Tue, 13 Dec 2016 13:21:45 +0100 Message-ID: <63a1993a-7df8-db09-01e7-7b82ddcc85b0@redhat.com> References: <20161212215612.9262-1-hdegoede@redhat.com> <20161212215612.9262-4-hdegoede@redhat.com> <1481622976.7188.60.camel@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43156 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753242AbcLMMVu (ORCPT ); Tue, 13 Dec 2016 07:21:50 -0500 In-Reply-To: <1481622976.7188.60.camel@linux.intel.com> Sender: linux-i2c-owner@vger.kernel.org List-Id: linux-i2c@vger.kernel.org To: Andy Shevchenko , Jarkko Nikula , Wolfram Sang , Len Brown Cc: Mika Westerberg , Jani Nikula , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Takashi Iwai , "russianneuromancer @ ya . ru" , linux-i2c@vger.kernel.org, intel-gfx Hi, On 13-12-16 10:56, Andy Shevchenko wrote: > On Mon, 2016-12-12 at 22:56 +0100, Hans de Goede wrote: >> On my cherrytrail tablet with axp288 pmic, just doing a bunch of >> repeated >> reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet >> in >> 1 - 3 runs guaranteed. >> >> This seems to be causes by the cpu trying to enter C6 or C7 while we >> hold >> the punit bus semaphore, at which point everything just hangs. >> >> Avoid this by disallowing the CPU to enter C6 or C7 before acquiring >> the >> punit bus semaphore. >> > > Just a nitpick for abbreviations: pmic -> PMIC, punit -> P-Unit, but I'm > okay with the contents which is more important. Erm, the rest of the code, including dev_info and dev_err messages also uses punit without the - in there, anyways not planning to send a v5 for now. > Reviewed-by: Andy Shevchenko Thank you. >> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051 > > What would be good is to have comments / tags from Len and Ville. About this patch vs bug bko109051, yesterday I've spend time reading that entire bug. It seems it is a combination of at least 3 bugs combined, 2 i915 related with commits which seem to trigger the problem (2 different groups of users with a different problem it seems) which causes a hang every few hours. And one other bug where the system freezes in minutes, that one sounds like what I was seeing without this patch (but may well be yet another issue). As for the 2 i915 bugs, there have been git bisects for both of them, it would be good if someone could take a look at these, just search for bisect in that huge bug. Regards, Hans > >> Signed-off-by: Hans de Goede >> Tested-by: Takashi Iwai >> --- >> Changes in v2: >> -New patch in v2 of this set >> Changes in v3: >> -Change commit message and comment in the code from "force the CPU to >> C1" >> to "Disallow the CPU to enter C6 or C7", as the CPU may still be in >> either >> C0 or C1 with the request pm_qos >> Changes in v4: >> -Rename i2c_dw_eval_lock_support to i2c_dw_probe_lock_support so that >> we can >> add a matching i2c_dw_remove_lock_support cleanup function >> -Move qm_pos removal to new i2c_dw_remove_lock_support function >> -Move pm_qos_add_request to the end of i2c_dw_probe_lock_support >> --- >> drivers/i2c/busses/i2c-designware-baytrail.c | 21 >> ++++++++++++++++++++- >> drivers/i2c/busses/i2c-designware-core.h | 9 +++++++-- >> drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++- >> 3 files changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c >> b/drivers/i2c/busses/i2c-designware-baytrail.c >> index cf02222..95d7013 100644 >> --- a/drivers/i2c/busses/i2c-designware-baytrail.c >> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c >> @@ -16,6 +16,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> >> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32 >> *sem) >> u32 data; >> int ret; >> >> + /* >> + * Disallow the CPU to enter C6 or C7 state, entering these >> states >> + * requires the punit to talk to the pmic and if this happens >> while >> + * we're holding the semaphore, the SoC hangs. >> + */ >> + pm_qos_update_request(&dev->pm_qos, 0); >> + >> ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, >> PUNIT_SEMAPHORE, &data); >> if (ret) { >> dev_err(dev->dev, "iosf failed to read punit >> semaphore\n"); >> @@ -56,6 +64,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev) >> data &= ~PUNIT_SEMAPHORE_BIT; >> if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, >> PUNIT_SEMAPHORE, data)) >> dev_err(dev->dev, "iosf failed to reset punit >> semaphore during write\n"); >> + >> + pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE); >> } >> >> static int baytrail_i2c_acquire(struct dw_i2c_dev *dev) >> @@ -121,7 +131,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev >> *dev) >> jiffies_to_msecs(jiffies - acquired)); >> } >> >> -int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) >> +int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) >> { >> acpi_status status; >> unsigned long long shared_host = 0; >> @@ -149,5 +159,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev >> *dev) >> dev->release_lock = baytrail_i2c_release; >> dev->pm_runtime_disabled = true; >> >> + pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY, >> + PM_QOS_DEFAULT_VALUE); >> + >> return 0; >> } >> + >> +void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev) >> +{ >> + if (dev->acquire_lock) >> + pm_qos_remove_request(&dev->pm_qos); >> +} >> diff --git a/drivers/i2c/busses/i2c-designware-core.h >> b/drivers/i2c/busses/i2c-designware-core.h >> index fb143f5..871970e 100644 >> --- a/drivers/i2c/busses/i2c-designware-core.h >> +++ b/drivers/i2c/busses/i2c-designware-core.h >> @@ -22,6 +22,7 @@ >> * >> */ >> >> +#include >> >> #define DW_IC_CON_MASTER 0x1 >> #define DW_IC_CON_SPEED_STD 0x2 >> @@ -67,6 +68,7 @@ >> * @fp_lcnt: fast plus LCNT value >> * @hs_hcnt: high speed HCNT value >> * @hs_lcnt: high speed LCNT value >> + * @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 >> @@ -114,6 +116,7 @@ struct dw_i2c_dev { >> u16 fp_lcnt; >> u16 hs_hcnt; >> u16 hs_lcnt; >> + 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; >> @@ -131,7 +134,9 @@ extern u32 i2c_dw_read_comp_param(struct >> dw_i2c_dev *dev); >> extern int i2c_dw_probe(struct dw_i2c_dev *dev); >> >> #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL) >> -extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev); >> +extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev); >> +extern void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev); >> #else >> -static inline int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) { >> return 0; } >> +static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) { >> return 0; } >> +static inline void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev) >> {} >> #endif >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c >> b/drivers/i2c/busses/i2c-designware-platdrv.c >> index 97a2ca1..b0a64a2 100644 >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >> @@ -210,7 +210,7 @@ static int dw_i2c_plat_probe(struct >> platform_device *pdev) >> return -EINVAL; >> } >> >> - r = i2c_dw_eval_lock_support(dev); >> + r = i2c_dw_probe_lock_support(dev); >> if (r) >> return r; >> >> @@ -291,6 +291,8 @@ static int dw_i2c_plat_remove(struct >> platform_device *pdev) >> if (!dev->pm_runtime_disabled) >> pm_runtime_disable(&pdev->dev); >> >> + i2c_dw_remove_lock_support(dev); >> + >> return 0; >> } >> >