[PATCH v2 1/5] i2c: designware: Rename accessor_flags to flags

[PATCH v2 1/5] i2c: designware: Rename accessor_flags to flags

[Hans de Goede]

Rename accessor_flags to flags, so that we can use the field for
other flags too. This is a preparation patch for adding cherrytrail
support to the punit semaphore code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-core.c    | 14 +++++++-------
 drivers/i2c/busses/i2c-designware-core.h    |  2 +-
 drivers/i2c/busses/i2c-designware-platdrv.c |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index b403fa5..b6a7989 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -177,13 +177,13 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 {
 	u32 value;
 
-	if (dev->accessor_flags & ACCESS_16BIT)
+	if (dev->flags & ACCESS_16BIT)
 		value = readw_relaxed(dev->base + offset) |
 			(readw_relaxed(dev->base + offset + 2) << 16);
 	else
 		value = readl_relaxed(dev->base + offset);
 
-	if (dev->accessor_flags & ACCESS_SWAP)
+	if (dev->flags & ACCESS_SWAP)
 		return swab32(value);
 	else
 		return value;
@@ -191,10 +191,10 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
 
 static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
 {
-	if (dev->accessor_flags & ACCESS_SWAP)
+	if (dev->flags & ACCESS_SWAP)
 		b = swab32(b);
 
-	if (dev->accessor_flags & ACCESS_16BIT) {
+	if (dev->flags & ACCESS_16BIT) {
 		writew_relaxed((u16)b, dev->base + offset);
 		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
 	} else {
@@ -339,10 +339,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	reg = dw_readl(dev, DW_IC_COMP_TYPE);
 	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
 		/* Configure register endianess access */
-		dev->accessor_flags |= ACCESS_SWAP;
+		dev->flags |= ACCESS_SWAP;
 	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
 		/* Configure register access mode 16bit */
-		dev->accessor_flags |= ACCESS_16BIT;
+		dev->flags |= ACCESS_16BIT;
 	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
 		dev_err(dev->dev, "Unknown Synopsys component type: "
 			"0x%08x\n", reg);
@@ -886,7 +886,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 tx_aborted:
 	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
 		complete(&dev->cmd_complete);
-	else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
+	else if (unlikely(dev->flags & ACCESS_INTR_MASK)) {
 		/* workaround to trigger pending interrupt */
 		stat = dw_readl(dev, DW_IC_INTR_MASK);
 		i2c_dw_disable_int(dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 0d44d2a..fb143f5 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -95,7 +95,7 @@ struct dw_i2c_dev {
 	unsigned int		status;
 	u32			abort_source;
 	int			irq;
-	u32			accessor_flags;
+	u32			flags;
 	struct i2c_adapter	adapter;
 	u32			functionality;
 	u32			master_cfg;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..97a2ca1 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -112,7 +112,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 
 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
 	if (id && id->driver_data)
-		dev->accessor_flags |= (u32)id->driver_data;
+		dev->flags |= (u32)id->driver_data;
 
 	return 0;
 }
-- 
2.9.3

[PATCH v2 2/5] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions

[Hans de Goede]

Pass dw_i2c_dev into the helper functions, this is a preparation patch
for the punit semaphore fixes done in the other patches in this set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1590ad0..a3f581c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -28,14 +28,14 @@
 
 static unsigned long acquired;
 
-static int get_sem(struct device *dev, u32 *sem)
+static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 {
 	u32 data;
 	int ret;
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
 	if (ret) {
-		dev_err(dev, "iosf failed to read punit semaphore\n");
+		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 		return ret;
 	}
 
@@ -44,18 +44,18 @@ static int get_sem(struct device *dev, u32 *sem)
 	return 0;
 }
 
-static void reset_semaphore(struct device *dev)
+static void reset_semaphore(struct dw_i2c_dev *dev)
 {
 	u32 data;
 
 	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
-		dev_err(dev, "iosf failed to reset punit semaphore during read\n");
+		dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
 		return;
 	}
 
 	data &= ~PUNIT_SEMAPHORE_BIT;
 	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
-		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
+		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -83,7 +83,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	start = jiffies;
 	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
 	do {
-		ret = get_sem(dev->dev, &sem);
+		ret = get_sem(dev, &sem);
 		if (!ret && sem) {
 			acquired = jiffies;
 			dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
@@ -95,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	} while (time_before(jiffies, end));
 
 	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
-	reset_semaphore(dev->dev);
+	reset_semaphore(dev);
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
 	if (ret)
@@ -116,7 +116,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev *dev)
 	if (!dev->acquire_lock)
 		return;
 
-	reset_semaphore(dev->dev);
+	reset_semaphore(dev);
 	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
 		jiffies_to_msecs(jiffies - acquired));
 }
-- 
2.9.3

[PATCH v2 3/5] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts

[Hans de Goede]

There is no need to delay the probe if iosf_mbi_available() returns
false when an i2c bus is not using the punit semaphore.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index a3f581c..a419777 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -139,14 +139,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 		return 0;
 
 	if (shared_host) {
+		if (!iosf_mbi_available())
+			return -EPROBE_DEFER;
+
 		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;
 	}
 
-	if (!iosf_mbi_available())
-		return -EPROBE_DEFER;
-
 	return 0;
 }
-- 
2.9.3

Re: [PATCH v2 3/5] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts

[Andy Shevchenko]

On Sat, 2016-12-10 at 15:19 +0100, Hans de Goede wrote:
> There is no need to delay the probe if iosf_mbi_available() returns
> false when an i2c bus is not using the punit semaphore.

> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -139,14 +139,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev
> *dev)
>  		return 0;
>  
>  	if (shared_host) {
> +		if (!iosf_mbi_available())
> +			return -EPROBE_DEFER;
> +

Looking to the implementation of i2c_dw_eval_lock_support() I would
rewrite this like

if (!shared_host)
  return 0;

>  		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;
>  	}
>  
> -	if (!iosf_mbi_available())
> -		return -EPROBE_DEFER;
> -
>  	return 0;
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 3/5] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts

[Hans de Goede]

Hi,

On 10-12-16 15:54, Andy Shevchenko wrote:
> On Sat, 2016-12-10 at 15:19 +0100, Hans de Goede wrote:
>> There is no need to delay the probe if iosf_mbi_available() returns
>> false when an i2c bus is not using the punit semaphore.
>
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>> @@ -139,14 +139,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev
>> *dev)
>>  		return 0;
>>
>>  	if (shared_host) {
>> +		if (!iosf_mbi_available())
>> +			return -EPROBE_DEFER;
>> +
>
> Looking to the implementation of i2c_dw_eval_lock_support() I would
> rewrite this like
>
> if (!shared_host)
>   return 0;

I had the same thought when writing this patch, so ack, will fix for v3.

Regards,

Hans



>
>>  		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;
>>  	}
>>
>> -	if (!iosf_mbi_available())
>> -		return -EPROBE_DEFER;
>> -
>>  	return 0;
>>  }
>

[PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Hans de Goede]

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 cpuidle / intel_idle driver trying to
change the C-state while we hold the punit bus semaphore, at which point
everything just hangs.

Avoid this by forcing the CPU to C1 before acquiring the punit bus
semaphore.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 12 ++++++++++++
 drivers/i2c/busses/i2c-designware-core.h     |  3 +++
 drivers/i2c/busses/i2c-designware-platdrv.c  |  3 +++
 3 files changed, 18 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index a419777..4f10ebb 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;
 
+	/*
+	 * Force CPU to C1 state, otherwise if the cpuidle / intel_idle
+	 * driver tries to change the C state 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)
@@ -143,6 +153,8 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 			return -EPROBE_DEFER;
 
 		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+		pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
+				   PM_QOS_DEFAULT_VALUE);
 		dev->acquire_lock = baytrail_i2c_acquire;
 		dev->release_lock = baytrail_i2c_release;
 		dev->pm_runtime_disabled = true;
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index fb143f5..47d284c 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;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 97a2ca1..6d72929 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -291,6 +291,9 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 	if (!dev->pm_runtime_disabled)
 		pm_runtime_disable(&pdev->dev);
 
+	if (dev->acquire_lock)
+		pm_qos_remove_request(&dev->pm_qos);
+
 	return 0;
 }
 
-- 
2.9.3

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Andy Shevchenko]

+Cc: Len
Len, I think you would be interested by this.

Hans, thanks for the change! Most probably we will anticipate Len's ACK
on this one.

On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to
> change the C-state while we hold the punit bus semaphore, at which
> point
> everything just hangs.
> 
> Avoid this by forcing the CPU to C1 before acquiring the punit bus
> semaphore.

Isn't it C0? C1 as far as I remember is halted state.

> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> *sem)
>  	u32 data;
>  	int ret;
>  
> +	/*
> +	 * Force CPU to C1 state, otherwise if the cpuidle /
> intel_idle
> +	 * driver tries to change the C state while we're holding the
> +	 * semaphore, the SoC hangs.

C0?

> +	 */
> +	pm_qos_update_request(&dev->pm_qos, 0);

C1 is when you set 1 here, right?

> platform_device *pdev)
>  	if (!dev->pm_runtime_disabled)
>  		pm_runtime_disable(&pdev->dev);

> +	if (dev->acquire_lock)
> +		pm_qos_remove_request(&dev->pm_qos);
> +

Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
(Even with PCI enumerated host with ACPI-enabled firmware you may get
_SEM object present)

>  	return 0;
>  }
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Hans de Goede]

Hi,

On 10-12-16 15:53, Andy Shevchenko wrote:
> +Cc: Len
> Len, I think you would be interested by this.
>
> Hans, thanks for the change!

You're welcome I ended up comparing the code in
i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from:

https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches

against the mainline code while I was trying to fix the maddening
problem of the entire SoC hanging more or less as soon
as I tried to use the pmic i2c bus and there I found
some fiddling with pm_qos which let to this patch.

> Most probably we will anticipate Len's ACK
> on this one.
>
> On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to
>> change the C-state while we hold the punit bus semaphore, at which
>> point
>> everything just hangs.
>>
>> Avoid this by forcing the CPU to C1 before acquiring the punit bus
>> semaphore.
>
> Isn't it C0? C1 as far as I remember is halted state.

You're right, I will fix it.

>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>> *sem)
>>  	u32 data;
>>  	int ret;
>>
>> +	/*
>> +	 * Force CPU to C1 state, otherwise if the cpuidle /
>> intel_idle
>> +	 * driver tries to change the C state while we're holding the
>> +	 * semaphore, the SoC hangs.
>
> C0?
>
>> +	 */
>> +	pm_qos_update_request(&dev->pm_qos, 0);
>
> C1 is when you set 1 here, right?

I believe so, yes.

>
>> platform_device *pdev)
>>  	if (!dev->pm_runtime_disabled)
>>  		pm_runtime_disable(&pdev->dev);
>
>> +	if (dev->acquire_lock)
>> +		pm_qos_remove_request(&dev->pm_qos);
>> +
>
> Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
> (Even with PCI enumerated host with ACPI-enabled firmware you may get
> _SEM object present)

Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support()
which does the pm_qos_add_request, so I put it here to keep things
balanced.

Regards,

Hans

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Hans de Goede]

Hi,

On 10-12-16 20:33, Hans de Goede wrote:
> Hi,
>
> On 10-12-16 15:53, Andy Shevchenko wrote:
>> +Cc: Len
>> Len, I think you would be interested by this.
>>
>> Hans, thanks for the change!
>
> You're welcome I ended up comparing the code in
> i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from:
>
> https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches
>
> against the mainline code while I was trying to fix the maddening
> problem of the entire SoC hanging more or less as soon
> as I tried to use the pmic i2c bus and there I found
> some fiddling with pm_qos which let to this patch.
>
>> Most probably we will anticipate Len's ACK
>> on this one.
>>
>> On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to
>>> change the C-state while we hold the punit bus semaphore, at which
>>> point
>>> everything just hangs.
>>>
>>> Avoid this by forcing the CPU to C1 before acquiring the punit bus
>>> semaphore.
>>
>> Isn't it C0? C1 as far as I remember is halted state.
>
> You're right, I will fix it.

Correction, upon closer reading of the docs, we cannot disallow
the CPU to enter C1 / force it to either C0 or C1, what we can
disallow is for it to enter C6/C7. Which also makes sense wrt
this bug, since entering C6/C7 involves turning of the
CPU-core power-plane, which requires the punit to access the pmic.

So I've changes the text in both the commit msg and the comment
to: "Disallow the CPU to enter C6 or C7"

I still need to re-test (just to make sure I did not cause
any regressions) and then I'll send a v3.

Regards,

Hans



>>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>>> *sem)
>>>      u32 data;
>>>      int ret;
>>>
>>> +    /*
>>> +     * Force CPU to C1 state, otherwise if the cpuidle /
>>> intel_idle
>>> +     * driver tries to change the C state while we're holding the
>>> +     * semaphore, the SoC hangs.
>>
>> C0?
>>
>>> +     */
>>> +    pm_qos_update_request(&dev->pm_qos, 0);
>>
>> C1 is when you set 1 here, right?
>
> I believe so, yes.
>
>>
>>> platform_device *pdev)
>>>      if (!dev->pm_runtime_disabled)
>>>          pm_runtime_disable(&pdev->dev);
>>
>>> +    if (dev->acquire_lock)
>>> +        pm_qos_remove_request(&dev->pm_qos);
>>> +
>>
>> Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
>> (Even with PCI enumerated host with ACPI-enabled firmware you may get
>> _SEM object present)
>
> Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support()
> which does the pm_qos_add_request, so I put it here to keep things
> balanced.
>
> Regards,
>
> Hans

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Len Brown]

Is there a simple way to run a test to keep deep C-states
and instead disable part or all of i2c on this platform,
to see how much stability separating the two will buy us?

A lot of people are struggling w/ the stability of this platform,
and it would be great to make some progress on that.

thanks,
-Len



On Sat, Dec 10, 2016 at 2:59 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 10-12-16 20:33, Hans de Goede wrote:
>>
>> Hi,
>>
>> On 10-12-16 15:53, Andy Shevchenko wrote:
>>>
>>> +Cc: Len
>>> Len, I think you would be interested by this.
>>>
>>> Hans, thanks for the change!
>>
>>
>> You're welcome I ended up comparing the code in
>> i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from:
>>
>>
>> https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches
>>
>> against the mainline code while I was trying to fix the maddening
>> problem of the entire SoC hanging more or less as soon
>> as I tried to use the pmic i2c bus and there I found
>> some fiddling with pm_qos which let to this patch.
>>
>>> Most probably we will anticipate Len's ACK
>>> on this one.
>>>
>>> On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to
>>>> change the C-state while we hold the punit bus semaphore, at which
>>>> point
>>>> everything just hangs.
>>>>
>>>> Avoid this by forcing the CPU to C1 before acquiring the punit bus
>>>> semaphore.
>>>
>>>
>>> Isn't it C0? C1 as far as I remember is halted state.
>>
>>
>> You're right, I will fix it.
>
>
> Correction, upon closer reading of the docs, we cannot disallow
> the CPU to enter C1 / force it to either C0 or C1, what we can
> disallow is for it to enter C6/C7. Which also makes sense wrt
> this bug, since entering C6/C7 involves turning of the
> CPU-core power-plane, which requires the punit to access the pmic.
>
> So I've changes the text in both the commit msg and the comment
> to: "Disallow the CPU to enter C6 or C7"
>
> I still need to re-test (just to make sure I did not cause
> any regressions) and then I'll send a v3.
>
> Regards,
>
> Hans
>
>
>
>
>>>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>>>> *sem)
>>>>      u32 data;
>>>>      int ret;
>>>>
>>>> +    /*
>>>> +     * Force CPU to C1 state, otherwise if the cpuidle /
>>>> intel_idle
>>>> +     * driver tries to change the C state while we're holding the
>>>> +     * semaphore, the SoC hangs.
>>>
>>>
>>> C0?
>>>
>>>> +     */
>>>> +    pm_qos_update_request(&dev->pm_qos, 0);
>>>
>>>
>>> C1 is when you set 1 here, right?
>>
>>
>> I believe so, yes.
>>
>>>
>>>> platform_device *pdev)
>>>>      if (!dev->pm_runtime_disabled)
>>>>          pm_runtime_disable(&pdev->dev);
>>>
>>>
>>>> +    if (dev->acquire_lock)
>>>> +        pm_qos_remove_request(&dev->pm_qos);
>>>> +
>>>
>>>
>>> Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
>>> (Even with PCI enumerated host with ACPI-enabled firmware you may get
>>> _SEM object present)
>>
>>
>> Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support()
>> which does the pm_qos_add_request, so I put it here to keep things
>> balanced.
>>
>> Regards,
>>
>> Hans



-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Hans de Goede]

Hi,

On 25-12-16 19:31, Len Brown wrote:
> Is there a simple way to run a test to keep deep C-states
> and instead disable part or all of i2c on this platform,
> to see how much stability separating the two will buy us?

This should do the trick to completely disable i2c from the kernel
to the pmic, leaving the bus fully free for the punit:

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1590ad0..fe73271 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -140,6 +140,7 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)

  	if (shared_host) {
  		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+		return -ENODEV;
  		dev->acquire_lock = baytrail_i2c_acquire;
  		dev->release_lock = baytrail_i2c_release;
  		dev->pm_runtime_disabled = true;

Note that my patch only disabled deep C-states while the kernel
is accessing the pmic i2c bus, not all the time as most other
workarounds are doing.

> A lot of people are struggling w/ the stability of this platform,
> and it would be great to make some progress on that.

I know that many people are seeing these instabilities related
to deep C-states on Baytrail, but the platform I wrote this patch
on is Cherrytrail, which is actually reasonably stable.

Currently on Cherrytrail we effectively do the above -ENODEV,
because the punit semaphore code is using a wrong register offset
on cherrytrail, causing any attempts by the kernel to acquire
the semaphore to always fail. Once the wrong register offset is
fixed I can very reliably freeze my cherrytrail tablet in
seconds by reading *and only reading* from the pmic, e.g. doing:

i2cdump -y -f 6 0x34

Will usually freeze the system on the second attempt, sometimes
on the first / third and that is repeating it manually, so with
long (100-s ms) pauses between runs.

Debugging that lead me to:

https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch

Which does the same pm_qos cpu latency tricks as my patch is
doing, any that makes the problem completely go away.

TL;DR: yes there still is a lot to investigate wrt stability
issues on Baytrail, but this patch is needed for Cherrytrail
too, fixes a 100% reproducable problem there and the same
workaround is used in Android x86 too, so I believe it would
be good to merge this regardless of the ongoing Baytrail
investigation.

Regardsm

Hans


> thanks,
> -Len
>
>
>
> On Sat, Dec 10, 2016 at 2:59 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 10-12-16 20:33, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 10-12-16 15:53, Andy Shevchenko wrote:
>>>>
>>>> +Cc: Len
>>>> Len, I think you would be interested by this.
>>>>
>>>> Hans, thanks for the change!
>>>
>>>
>>> You're welcome I ended up comparing the code in
>>> i2c-dw_i2c-Ported-punit-locking-patch-from-MCG-kerne.patch from:
>>>
>>>
>>> https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches
>>>
>>> against the mainline code while I was trying to fix the maddening
>>> problem of the entire SoC hanging more or less as soon
>>> as I tried to use the pmic i2c bus and there I found
>>> some fiddling with pm_qos which let to this patch.
>>>
>>>> Most probably we will anticipate Len's ACK
>>>> on this one.
>>>>
>>>> On Sat, 2016-12-10 at 15:19 +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 cpuidle / intel_idle driver trying to
>>>>> change the C-state while we hold the punit bus semaphore, at which
>>>>> point
>>>>> everything just hangs.
>>>>>
>>>>> Avoid this by forcing the CPU to C1 before acquiring the punit bus
>>>>> semaphore.
>>>>
>>>>
>>>> Isn't it C0? C1 as far as I remember is halted state.
>>>
>>>
>>> You're right, I will fix it.
>>
>>
>> Correction, upon closer reading of the docs, we cannot disallow
>> the CPU to enter C1 / force it to either C0 or C1, what we can
>> disallow is for it to enter C6/C7. Which also makes sense wrt
>> this bug, since entering C6/C7 involves turning of the
>> CPU-core power-plane, which requires the punit to access the pmic.
>>
>> So I've changes the text in both the commit msg and the comment
>> to: "Disallow the CPU to enter C6 or C7"
>>
>> I still need to re-test (just to make sure I did not cause
>> any regressions) and then I'll send a v3.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>>> @@ -33,6 +34,13 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>>>>> *sem)
>>>>>      u32 data;
>>>>>      int ret;
>>>>>
>>>>> +    /*
>>>>> +     * Force CPU to C1 state, otherwise if the cpuidle /
>>>>> intel_idle
>>>>> +     * driver tries to change the C state while we're holding the
>>>>> +     * semaphore, the SoC hangs.
>>>>
>>>>
>>>> C0?
>>>>
>>>>> +     */
>>>>> +    pm_qos_update_request(&dev->pm_qos, 0);
>>>>
>>>>
>>>> C1 is when you set 1 here, right?
>>>
>>>
>>> I believe so, yes.
>>>
>>>>
>>>>> platform_device *pdev)
>>>>>      if (!dev->pm_runtime_disabled)
>>>>>          pm_runtime_disable(&pdev->dev);
>>>>
>>>>
>>>>> +    if (dev->acquire_lock)
>>>>> +        pm_qos_remove_request(&dev->pm_qos);
>>>>> +
>>>>
>>>>
>>>> Perhaps you need to do this in -core.c. Otherwise you missed PCI case.
>>>> (Even with PCI enumerated host with ACPI-enabled firmware you may get
>>>> _SEM object present)
>>>
>>>
>>> Currently only i2c-designware-plardrv.c calls i2c_dw_eval_lock_support()
>>> which does the pm_qos_add_request, so I put it here to keep things
>>> balanced.
>>>
>>> Regards,
>>>
>>> Hans
>
>
>

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Hans de Goede]

Hi,

On 26-12-16 12:07, Hans de Goede wrote:
> Hi,
>
> On 25-12-16 19:31, Len Brown wrote:
>> Is there a simple way to run a test to keep deep C-states
>> and instead disable part or all of i2c on this platform,
>> to see how much stability separating the two will buy us?
>
> This should do the trick to completely disable i2c from the kernel
> to the pmic, leaving the bus fully free for the punit:
>
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 1590ad0..fe73271 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -140,6 +140,7 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
>
>      if (shared_host) {
>          dev_info(dev->dev, "I2C bus managed by PUNIT\n");
> +        return -ENODEV;
>          dev->acquire_lock = baytrail_i2c_acquire;
>          dev->release_lock = baytrail_i2c_release;
>          dev->pm_runtime_disabled = true;
>
> Note that my patch only disabled deep C-states while the kernel
> is accessing the pmic i2c bus, not all the time as most other
> workarounds are doing.

Ok, some more very interesting input on this, from the bug
about the PUNIT semaphore issues on cherrytrail:

https://bugzilla.kernel.org/show_bug.cgi?id=155241#c37

"My device is a laptop with no USB charging or OTG. So, I'd tried only SDIO _ADR patch, i2c and axp288_fuel_gauge patches. Everything works well for upto 3 mins after boot, then the device freezes. I hadn't tried any drm patches BTW. Here's the log:

[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
clocksource:                       'refined-jiffies' wd_now: 10002ee30 wd_last: 10002edb8 mask: ffffffff
clocksource:                       'tsc' cs_now: 16ac2c7744a cs_last: 16a8d9bd8f2 mask: ffffffffffffffff
clocksource: Switched to clocksource refined-jiffies
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: axp288 reg read err:-110
axp288_fuel_gauge axp288_fuel_gauge: PWR STAT read failed:-110
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 0
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: IIO channel read error: fffffffb, 0
power_supply axp288_fuel_gauge: driver failed to report `voltage_now' property: -5
***SYSTEM FREEZE***

If I blacklist axp288_fuel_gauge, then there were no errors."

So it seems that not only bad things happen when the punit tries to
change the cpu C-state while we're holding the pmic i2c bus
semaphore, but that similar bad things happen when the gpu code
tries to acquire / release a forcewake lock on the GPU while
we're accessing the pmic i2c bus.

This makes sense, the iosf_mbi code which is used by the
i2c bus semaphore code has this:

arch/x86/platform/intel/iosf_mbi.c:

int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
{
         u32 mcr, mcrx;
         unsigned long flags;
         int ret;

         /* Access to the GFX unit is handled by GPU code */
         if (port == BT_MBI_UNIT_GFX) {
                 WARN_ON(1);
                 return -EPERM;
         }

	...
}

So the i915 driver definitely is interacting with the punit
through the mailbox interface too...

I'll try to write a quick and dirty patch where the i915 code
simply calls intel_uncore_forcewake_get(dev_priv,  FORCEWAKE_ALL);
an extra time on probe, and ask the user who is seeing this to
test.

Regards,

Hans


code calls
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore

[Andy Shevchenko]

On Sat, 2016-12-31 at 22:29 +0100, Hans de Goede wrote:

> This makes sense, the iosf_mbi code which is used by the
> i2c bus semaphore code has this:
> 
> arch/x86/platform/intel/iosf_mbi.c:
> 
> int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
> {
>          u32 mcr, mcrx;
>          unsigned long flags;
>          int ret;
> 
>          /* Access to the GFX unit is handled by GPU code */
>          if (port == BT_MBI_UNIT_GFX) {
>                  WARN_ON(1);
>                  return -EPERM;
>          }
> 
> 	...
> }
> 
> So the i915 driver definitely is interacting with the punit
> through the mailbox interface too...

Yes, they have private mailbox support. Once I talked to Ville to make
at least definitions common between two: iosf_mbi.h and whatever i915 is
using, but have no time to implement that.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v2 5/5] i2c: designware-baytrail: Add support for cherrytrail

[Hans de Goede]

The cherrytrail punit has the pmic i2c bus access semaphore at a
different register address.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
Changes in v2:
-Adjust for accessor_flags -> flags rename
-Add flags field to struct dw_pci_controller
-Add get_sem_addr() helper replacing MODEL_CHERRYTRAIL flag checking in
 PUNIT_SEMAPHORE macro
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 22 +++++++++++++++++-----
 drivers/i2c/busses/i2c-designware-core.h     |  1 +
 drivers/i2c/busses/i2c-designware-pcidrv.c   | 26 +++++++++++++++++++-------
 drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 4f10ebb..16cfe16 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -24,13 +24,23 @@
 
 #define SEMAPHORE_TIMEOUT	100
 #define PUNIT_SEMAPHORE		0x7
+#define PUNIT_SEMAPHORE_CHV	0x10e
 #define PUNIT_SEMAPHORE_BIT	BIT(0)
 #define PUNIT_SEMAPHORE_ACQUIRE	BIT(1)
 
 static unsigned long acquired;
 
+static u32 get_sem_addr(struct dw_i2c_dev *dev)
+{
+	if (dev->flags & MODEL_CHERRYTRAIL)
+		return PUNIT_SEMAPHORE_CHV;
+	else
+		return PUNIT_SEMAPHORE;
+}
+
 static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 {
+	u32 addr = get_sem_addr(dev);
 	u32 data;
 	int ret;
 
@@ -41,7 +51,7 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 	 */
 	pm_qos_update_request(&dev->pm_qos, 0);
 
-	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &data);
 	if (ret) {
 		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 		return ret;
@@ -54,15 +64,16 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 
 static void reset_semaphore(struct dw_i2c_dev *dev)
 {
+	u32 addr = get_sem_addr(dev);
 	u32 data;
 
-	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
+	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &data)) {
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
 		return;
 	}
 
 	data &= ~PUNIT_SEMAPHORE_BIT;
-	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
+	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, 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);
@@ -70,6 +81,7 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 {
+	u32 addr = get_sem_addr(dev);
 	u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
 	int ret;
 	unsigned long start, end;
@@ -83,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 		return 0;
 
 	/* host driver writes to side band semaphore register */
-	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, sem);
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, sem);
 	if (ret) {
 		dev_err(dev->dev, "iosf punit semaphore request failed\n");
 		return ret;
@@ -107,7 +119,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
 	reset_semaphore(dev);
 
-	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &sem);
 	if (ret)
 		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 	else
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 47d284c..735fc63 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -126,6 +126,7 @@ struct dw_i2c_dev {
 #define ACCESS_SWAP		0x00000001
 #define ACCESS_16BIT		0x00000002
 #define ACCESS_INTR_MASK	0x00000004
+#define MODEL_CHERRYTRAIL	0x00000008
 
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 96f8230..4e53a9f 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
 	medfield,
 	merrifield,
 	baytrail,
+	cherrytrail,
 	haswell,
 };
 
@@ -63,6 +64,7 @@ struct dw_pci_controller {
 	u32 rx_fifo_depth;
 	u32 clk_khz;
 	u32 functionality;
+	u32 flags;
 	struct dw_scl_sda_cfg *scl_sda_cfg;
 	int (*setup)(struct pci_dev *pdev, struct dw_pci_controller *c);
 };
@@ -174,6 +176,15 @@ static struct dw_pci_controller dw_pci_controllers[] = {
 		.functionality = I2C_FUNC_10BIT_ADDR,
 		.scl_sda_cfg = &hsw_config,
 	},
+	[cherrytrail] = {
+		.bus_num = -1,
+		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
+		.tx_fifo_depth = 32,
+		.rx_fifo_depth = 32,
+		.functionality = I2C_FUNC_10BIT_ADDR,
+		.flags = MODEL_CHERRYTRAIL,
+		.scl_sda_cfg = &byt_config,
+	},
 };
 
 #ifdef CONFIG_PM
@@ -241,6 +252,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	dev->base = pcim_iomap_table(pdev)[0];
 	dev->dev = &pdev->dev;
 	dev->irq = pdev->irq;
+	dev->flags |= controller->flags;
 
 	if (controller->setup) {
 		r = controller->setup(pdev, controller);
@@ -321,13 +333,13 @@ static const struct pci_device_id i2_designware_pci_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
 	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
 	/* Braswell / Cherrytrail */
-	{ PCI_VDEVICE(INTEL, 0x22C1), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C2), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C3), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C4), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C5), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C6), baytrail },
-	{ PCI_VDEVICE(INTEL, 0x22C7), baytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
+	{ PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
 	{ 0,}
 };
 MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 6d72929..589f07e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -123,7 +123,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
 	{ "INT3432", 0 },
 	{ "INT3433", 0 },
 	{ "80860F41", 0 },
-	{ "808622C1", 0 },
+	{ "808622C1", MODEL_CHERRYTRAIL },
 	{ "AMD0010", ACCESS_INTR_MASK },
 	{ "AMDI0010", ACCESS_INTR_MASK },
 	{ "AMDI0510", 0 },
-- 
2.9.3

Re: [PATCH v2 5/5] i2c: designware-baytrail: Add support for cherrytrail

[Andy Shevchenko]

On Sat, 2016-12-10 at 15:19 +0100, Hans de Goede wrote:
> The cherrytrail punit has the pmic i2c bus access semaphore at a
> different register address.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> Tested-by: Takashi Iwai <tiwai@suse.de>
> ---
> Changes in v2:
> -Adjust for accessor_flags -> flags rename
> -Add flags field to struct dw_pci_controller
> -Add get_sem_addr() helper replacing MODEL_CHERRYTRAIL flag checking
> in
>  PUNIT_SEMAPHORE macro

Yes! That's exactly what I would like to see.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

P.S. One very minor comment, perhaps make a gap between ACCESS_* and
MODEL_* flags, like starting MODEL_XXX from 0x10?

> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 22 +++++++++++++++++
> -----
>  drivers/i2c/busses/i2c-designware-core.h     |  1 +
>  drivers/i2c/busses/i2c-designware-pcidrv.c   | 26
> +++++++++++++++++++-------
>  drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
>  4 files changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 4f10ebb..16cfe16 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -24,13 +24,23 @@
>  
>  #define SEMAPHORE_TIMEOUT	100
>  #define PUNIT_SEMAPHORE		0x7
> +#define PUNIT_SEMAPHORE_CHV	0x10e
>  #define PUNIT_SEMAPHORE_BIT	BIT(0)
>  #define PUNIT_SEMAPHORE_ACQUIRE	BIT(1)
>  
>  static unsigned long acquired;
>  
> +static u32 get_sem_addr(struct dw_i2c_dev *dev)
> +{
> +	if (dev->flags & MODEL_CHERRYTRAIL)
> +		return PUNIT_SEMAPHORE_CHV;
> +	else
> +		return PUNIT_SEMAPHORE;
> +}
> +
>  static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
>  {
> +	u32 addr = get_sem_addr(dev);
>  	u32 data;
>  	int ret;
>  
> @@ -41,7 +51,7 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
>  	 */
>  	pm_qos_update_request(&dev->pm_qos, 0);
>  
> -	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data);
> +	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr,
> &data);
>  	if (ret) {
>  		dev_err(dev->dev, "iosf failed to read punit
> semaphore\n");
>  		return ret;
> @@ -54,15 +64,16 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> *sem)
>  
>  static void reset_semaphore(struct dw_i2c_dev *dev)
>  {
> +	u32 addr = get_sem_addr(dev);
>  	u32 data;
>  
> -	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data)) {
> +	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr,
> &data)) {
>  		dev_err(dev->dev, "iosf failed to reset punit
> semaphore during read\n");
>  		return;
>  	}
>  
>  	data &= ~PUNIT_SEMAPHORE_BIT;
> -	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, data))
> +	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr,
> 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);
> @@ -70,6 +81,7 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>  
>  static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  {
> +	u32 addr = get_sem_addr(dev);
>  	u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
>  	int ret;
>  	unsigned long start, end;
> @@ -83,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> *dev)
>  		return 0;
>  
>  	/* host driver writes to side band semaphore register */
> -	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, sem);
> +	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr,
> sem);
>  	if (ret) {
>  		dev_err(dev->dev, "iosf punit semaphore request
> failed\n");
>  		return ret;
> @@ -107,7 +119,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> *dev)
>  	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
>  	reset_semaphore(dev);
>  
> -	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &sem);
> +	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr,
> &sem);
>  	if (ret)
>  		dev_err(dev->dev, "iosf failed to read punit
> semaphore\n");
>  	else
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 47d284c..735fc63 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -126,6 +126,7 @@ struct dw_i2c_dev {
>  #define ACCESS_SWAP		0x00000001
>  #define ACCESS_16BIT		0x00000002
>  #define ACCESS_INTR_MASK	0x00000004
> +#define MODEL_CHERRYTRAIL	0x00000008
>  
>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 96f8230..4e53a9f 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
>  	medfield,
>  	merrifield,
>  	baytrail,
> +	cherrytrail,
>  	haswell,
>  };
>  
> @@ -63,6 +64,7 @@ struct dw_pci_controller {
>  	u32 rx_fifo_depth;
>  	u32 clk_khz;
>  	u32 functionality;
> +	u32 flags;
>  	struct dw_scl_sda_cfg *scl_sda_cfg;
>  	int (*setup)(struct pci_dev *pdev, struct dw_pci_controller
> *c);
>  };
> @@ -174,6 +176,15 @@ static struct dw_pci_controller
> dw_pci_controllers[] = {
>  		.functionality = I2C_FUNC_10BIT_ADDR,
>  		.scl_sda_cfg = &hsw_config,
>  	},
> +	[cherrytrail] = {
> +		.bus_num = -1,
> +		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
> +		.tx_fifo_depth = 32,
> +		.rx_fifo_depth = 32,
> +		.functionality = I2C_FUNC_10BIT_ADDR,
> +		.flags = MODEL_CHERRYTRAIL,
> +		.scl_sda_cfg = &byt_config,
> +	},
>  };
>  
>  #ifdef CONFIG_PM
> @@ -241,6 +252,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>  	dev->base = pcim_iomap_table(pdev)[0];
>  	dev->dev = &pdev->dev;
>  	dev->irq = pdev->irq;
> +	dev->flags |= controller->flags;
>  
>  	if (controller->setup) {
>  		r = controller->setup(pdev, controller);
> @@ -321,13 +333,13 @@ static const struct pci_device_id
> i2_designware_pci_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
>  	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
>  	/* Braswell / Cherrytrail */
> -	{ PCI_VDEVICE(INTEL, 0x22C1), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C2), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C3), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C4), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C5), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C6), baytrail },
> -	{ PCI_VDEVICE(INTEL, 0x22C7), baytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
> +	{ PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
>  	{ 0,}
>  };
>  MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 6d72929..589f07e 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -123,7 +123,7 @@ static const struct acpi_device_id
> dw_i2c_acpi_match[] = {
>  	{ "INT3432", 0 },
>  	{ "INT3433", 0 },
>  	{ "80860F41", 0 },
> -	{ "808622C1", 0 },
> +	{ "808622C1", MODEL_CHERRYTRAIL },
>  	{ "AMD0010", ACCESS_INTR_MASK },
>  	{ "AMDI0010", ACCESS_INTR_MASK },
>  	{ "AMDI0510", 0 },

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 1/5] i2c: designware: Rename accessor_flags to flags

[Andy Shevchenko]

On Sat, 2016-12-10 at 15:19 +0100, Hans de Goede wrote:
> Rename accessor_flags to flags, so that we can use the field for
> other flags too. This is a preparation patch for adding cherrytrail
> support to the punit semaphore code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---
>  drivers/i2c/busses/i2c-designware-core.c    | 14 +++++++-------
>  drivers/i2c/busses/i2c-designware-core.h    |  2 +-
>  drivers/i2c/busses/i2c-designware-platdrv.c |  2 +-
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c
> index b403fa5..b6a7989 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -177,13 +177,13 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int
> offset)
>  {
>  	u32 value;
>  
> -	if (dev->accessor_flags & ACCESS_16BIT)
> +	if (dev->flags & ACCESS_16BIT)
>  		value = readw_relaxed(dev->base + offset) |
>  			(readw_relaxed(dev->base + offset + 2) <<
> 16);
>  	else
>  		value = readl_relaxed(dev->base + offset);
>  
> -	if (dev->accessor_flags & ACCESS_SWAP)
> +	if (dev->flags & ACCESS_SWAP)
>  		return swab32(value);
>  	else
>  		return value;
> @@ -191,10 +191,10 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int
> offset)
>  
>  static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
>  {
> -	if (dev->accessor_flags & ACCESS_SWAP)
> +	if (dev->flags & ACCESS_SWAP)
>  		b = swab32(b);
>  
> -	if (dev->accessor_flags & ACCESS_16BIT) {
> +	if (dev->flags & ACCESS_16BIT) {
>  		writew_relaxed((u16)b, dev->base + offset);
>  		writew_relaxed((u16)(b >> 16), dev->base + offset +
> 2);
>  	} else {
> @@ -339,10 +339,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	reg = dw_readl(dev, DW_IC_COMP_TYPE);
>  	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
>  		/* Configure register endianess access */
> -		dev->accessor_flags |= ACCESS_SWAP;
> +		dev->flags |= ACCESS_SWAP;
>  	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
>  		/* Configure register access mode 16bit */
> -		dev->accessor_flags |= ACCESS_16BIT;
> +		dev->flags |= ACCESS_16BIT;
>  	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
>  		dev_err(dev->dev, "Unknown Synopsys component type: "
>  			"0x%08x\n", reg);
> @@ -886,7 +886,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void
> *dev_id)
>  tx_aborted:
>  	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) ||
> dev->msg_err)
>  		complete(&dev->cmd_complete);
> -	else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
> +	else if (unlikely(dev->flags & ACCESS_INTR_MASK)) {
>  		/* workaround to trigger pending interrupt */
>  		stat = dw_readl(dev, DW_IC_INTR_MASK);
>  		i2c_dw_disable_int(dev);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 0d44d2a..fb143f5 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -95,7 +95,7 @@ struct dw_i2c_dev {
>  	unsigned int		status;
>  	u32			abort_source;
>  	int			irq;
> -	u32			accessor_flags;
> +	u32			flags;
>  	struct i2c_adapter	adapter;
>  	u32			functionality;
>  	u32			master_cfg;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..97a2ca1 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -112,7 +112,7 @@ static int dw_i2c_acpi_configure(struct
> platform_device *pdev)
>  
>  	id = acpi_match_device(pdev->dev.driver->acpi_match_table,
> &pdev->dev);
>  	if (id && id->driver_data)
> -		dev->accessor_flags |= (u32)id->driver_data;
> +		dev->flags |= (u32)id->driver_data;
>  
>  	return 0;
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy