[Intel-gfx] [PATCH v4 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
Hans de Goede
hdegoede at redhat.com
Tue Dec 13 12:21:45 UTC 2016
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 <andriy.shevchenko at linux.intel.com>
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 <hdegoede at redhat.com>
>> Tested-by: Takashi Iwai <tiwai at suse.de>
>> ---
>> 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 <linux/acpi.h>
>> #include <linux/i2c.h>
>> #include <linux/interrupt.h>
>> +#include <linux/pm_qos.h>
>>
>> #include <asm/iosf_mbi.h>
>>
>> @@ -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 <linux/pm_qos.h>
>>
>> #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;
>> }
>>
>
More information about the Intel-gfx
mailing list