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

[PATCH v3 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>
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;
 }
-- 
2.9.3

[PATCH v3 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 v3 3/5] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts

[Hans de Goede]

If (!shared_host) simply return 0, this avoids delaying the probe if
iosf_mbi_available() returns false when an i2c bus is not using the
punit semaphore.

Also move the if (!iosf_mbi_available()) check to above the
dev_info, so that we do not repeat the dev_info on every probe
until iosf_mbi_available() returns true.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-New patch in v2 of this set
Changes in v3:
-Use if (!shared_host) return 0, to simplify the non-shared_host path
 and to avoid nested ifs
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index a3f581c..cf02222 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -138,15 +138,16 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 	if (ACPI_FAILURE(status))
 		return 0;
 
-	if (shared_host) {
-		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 (!shared_host)
+		return 0;
 
 	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;
+
 	return 0;
 }
-- 
2.9.3

[PATCH v3 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 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 cpu trying to enter C6 or C7 while we hold
the punit bus semaphore, at which point everything just hangs.

Avoid this by the CPU to enter C6 or C7 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>
---
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
---
 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 cf02222..997d048 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)
@@ -145,6 +155,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

[PATCH v3 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
Changes in v3:
-Add a gap between ACCESS_* and MODEL_* flags as reserved space for
 future ACCESS_* flags
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 22 +++++++++++++++++-----
 drivers/i2c/busses/i2c-designware-core.h     |  2 ++
 drivers/i2c/busses/i2c-designware-pcidrv.c   | 26 +++++++++++++++++++-------
 drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 997d048..456a236 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..2db3177 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -127,6 +127,8 @@ struct dw_i2c_dev {
 #define ACCESS_16BIT		0x00000002
 #define ACCESS_INTR_MASK	0x00000004
 
+#define MODEL_CHERRYTRAIL	0x00000100
+
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(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 v3 3/5] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts

[Andy Shevchenko]

On Sat, 2016-12-10 at 23:43 +0100, Hans de Goede wrote:
> If (!shared_host) simply return 0, this avoids delaying the probe if
> iosf_mbi_available() returns false when an i2c bus is not using the
> punit semaphore.
> 
> Also move the if (!iosf_mbi_available()) check to above the
> dev_info, so that we do not repeat the dev_info on every probe
> until iosf_mbi_available() returns true.
> 

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

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -New patch in v2 of this set
> Changes in v3:
> -Use if (!shared_host) return 0, to simplify the non-shared_host path
>  and to avoid nested ifs
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index a3f581c..cf02222 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -138,15 +138,16 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev
> *dev)
>  	if (ACPI_FAILURE(status))
>  		return 0;
>  
> -	if (shared_host) {
> -		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 (!shared_host)
> +		return 0;
>  
>  	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;
> +
>  	return 0;
>  }

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

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

[Andy Shevchenko]

On Sat, 2016-12-10 at 23:43 +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>

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

One small nit: you are using CHV abbreviation for CherryTrail, we are
consider CHT is a right one, CHV for CherryView.

> ---
> 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
> Changes in v3:
> -Add a gap between ACCESS_* and MODEL_* flags as reserved space for
>  future ACCESS_* flags
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 22 +++++++++++++++++
> -----
>  drivers/i2c/busses/i2c-designware-core.h     |  2 ++
>  drivers/i2c/busses/i2c-designware-pcidrv.c   | 26
> +++++++++++++++++++-------
>  drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
>  4 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 997d048..456a236 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..2db3177 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -127,6 +127,8 @@ struct dw_i2c_dev {
>  #define ACCESS_16BIT		0x00000002
>  #define ACCESS_INTR_MASK	0x00000004
>  
> +#define MODEL_CHERRYTRAIL	0x00000100
> +
>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable_int(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 v3 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore

[Andy Shevchenko]

On Sat, 2016-12-10 at 23:43 +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 the CPU to enter C6 or C7 before acquiring the punit bus
> semaphore.

Please, keep Len Brown in a loop for this patch. And perhaps someone
from our Graphics team. Maybe Imre Deak?

See my comments below.

> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 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
> ---
>  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 cf02222..997d048 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)
> @@ -145,6 +155,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);

Perhaps move this to the end of the function? For sake of readability.

>  	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);
> +

I read your answer to v2 of this, so, taking into consideration your
notice I would recommend to provide an additional helper like
i2c_dw_down_lock_support() (I didn't find proper antonym for eval, feel
free to choose a better name) and collect the above there.

>  	return 0;
>  }
>  

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

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

[Hans de Goede]

Hi,

On 12-12-16 11:31, Andy Shevchenko wrote:
> On Sat, 2016-12-10 at 23:43 +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>
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> One small nit: you are using CHV abbreviation for CherryTrail, we are
> consider CHT is a right one, CHV for CherryView.

I thought that cherryview and cherrytrail where 2 names for
the same chip ?

Regards,

Hans


>
>> ---
>> 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
>> Changes in v3:
>> -Add a gap between ACCESS_* and MODEL_* flags as reserved space for
>>  future ACCESS_* flags
>> ---
>>  drivers/i2c/busses/i2c-designware-baytrail.c | 22 +++++++++++++++++
>> -----
>>  drivers/i2c/busses/i2c-designware-core.h     |  2 ++
>>  drivers/i2c/busses/i2c-designware-pcidrv.c   | 26
>> +++++++++++++++++++-------
>>  drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
>>  4 files changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
>> b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index 997d048..456a236 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..2db3177 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -127,6 +127,8 @@ struct dw_i2c_dev {
>>  #define ACCESS_16BIT		0x00000002
>>  #define ACCESS_INTR_MASK	0x00000004
>>
>> +#define MODEL_CHERRYTRAIL	0x00000100
>> +
>>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>>  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
>>  extern void i2c_dw_disable_int(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 },
>

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

[Andy Shevchenko]

On Mon, 2016-12-12 at 11:48 +0100, Hans de Goede wrote:
> Hi,
> 
> On 12-12-16 11:31, Andy Shevchenko wrote:
> > On Sat, 2016-12-10 at 23:43 +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>
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > One small nit: you are using CHV abbreviation for CherryTrail, we
> > are
> > consider CHT is a right one, CHV for CherryView.
> 
> I thought that cherryview and cherrytrail where 2 names for
> the same chip ?

TBH, I don't know the difference but it is there, Like SoC -> platform.

In any case, just to be consistent with your code below. Everywhere else
you used cherrytrail.

> 
> Regards,
> 
> Hans
> 
> 
> > 
> > > ---
> > > 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
> > > Changes in v3:
> > > -Add a gap between ACCESS_* and MODEL_* flags as reserved space
> > > for
> > >  future ACCESS_* flags
> > > ---
> > >  drivers/i2c/busses/i2c-designware-baytrail.c | 22
> > > +++++++++++++++++
> > > -----
> > >  drivers/i2c/busses/i2c-designware-core.h     |  2 ++
> > >  drivers/i2c/busses/i2c-designware-pcidrv.c   | 26
> > > +++++++++++++++++++-------
> > >  drivers/i2c/busses/i2c-designware-platdrv.c  |  2 +-
> > >  4 files changed, 39 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> > > b/drivers/i2c/busses/i2c-designware-baytrail.c
> > > index 997d048..456a236 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..2db3177 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > @@ -127,6 +127,8 @@ struct dw_i2c_dev {
> > >  #define ACCESS_16BIT		0x00000002
> > >  #define ACCESS_INTR_MASK	0x00000004
> > > 
> > > +#define MODEL_CHERRYTRAIL	0x00000100
> > > +
> > >  extern int i2c_dw_init(struct dw_i2c_dev *dev);
> > >  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
> > >  extern void i2c_dw_disable_int(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 v3 1/5] i2c: designware: Rename accessor_flags to flags

[Jarkko Nikula]

On 11.12.2016 00:43, 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(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

[Jarkko Nikula]

On 11.12.2016 00:43, Hans de Goede wrote:
> 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(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

[Jarkko Nikula]

On 12.12.2016 12:19, Andy Shevchenko wrote:
> On Sat, 2016-12-10 at 23:43 +0100, Hans de Goede wrote:
>> If (!shared_host) simply return 0, this avoids delaying the probe if
>> iosf_mbi_available() returns false when an i2c bus is not using the
>> punit semaphore.
>>
>> Also move the if (!iosf_mbi_available()) check to above the
>> dev_info, so that we do not repeat the dev_info on every probe
>> until iosf_mbi_available() returns true.
>>
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v3 4/5] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore

[Hans de Goede]

Hi,

On 12-12-16 11:37, Andy Shevchenko wrote:
> On Sat, 2016-12-10 at 23:43 +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 the CPU to enter C6 or C7 before acquiring the punit bus
>> semaphore.
>
> Please, keep Len Brown in a loop for this patch. And perhaps someone
> from our Graphics team. Maybe Imre Deak?
>
> See my comments below.
>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> 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
>> ---
>>  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 cf02222..997d048 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)
>> @@ -145,6 +155,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);
>
> Perhaps move this to the end of the function? For sake of readability.

Done.

>
>>  	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);
>> +
>
> I read your answer to v2 of this, so, taking into consideration your
> notice I would recommend to provide an additional helper like
> i2c_dw_down_lock_support() (I didn't find proper antonym for eval, feel
> free to choose a better name) and collect the above there.

Done for v4, will send v4 soon.

Regards,

Hans

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

[Takashi Iwai]

On Sat, 10 Dec 2016 23:43:46 +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>

All v3 patches:
  Tested-by: Takashi Iwai <tiwai@suse.de>


Takashi