[PATCH 0/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC semaphore handling

[PATCH 0/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC semaphore handling

[Hans de Goede]

Hi All,

See the commit message of the first patch for the why and what of this
series.

Also the whole purpose of posting this series for now is just to get the
first patch reviewed and merged.

Once the first patch is merged we can look at using the new
iosf_mbi_[un]block_punit_i2c_access() functions in various drivers for the
AXP288 PMIC (and the TI dollarcove PMIC) to only take the P-Unit semaphore
once around a group of i2c accesses to the PMIC. The second patch is an
example of this.

The third patch is some trivial cleanup to the i2c-designware driver which
becomes possible after the first patch is merged. As mentioned in the commit
message of the first patch, that patch deliberately saves that cleanup for
later, so that it only touches i2c-designware-baytrail.c and not the main
i2c-designware*.c files, so that it can be merged through the x86 tree
without conflicts.

Regards,

Hans

[PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Hans de Goede]

On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
block it from accessing the shared bus while the kernel wants to access it.

Currently we have the I2C-controller driver acquiring and releasing the
semaphore around each I2C transfer. There are 2 problems with this:

1) PMIC accesses often come in the form of a read-modify-write on one of
the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
between the read and the write. If the P-Unit modifies the register during
this window?, then we end up overwriting the P-Unit's changes.
I believe that this is mostly an academic problem, but I'm not sure.

2) To safely access the shared I2C bus, we need to do 3 things:
a) Notify the GPU driver that we are starting a window in which it may not
access the P-Unit, since the P-Unit seems to ignore the semaphore for
explicit power-level requests made by the GPU driver
b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
C6/C7 while we hold the semaphore hangs the SoC
c) Finally take the P-Unit's PMIC bus semaphore
All 3 these steps together are somewhat expensive, so ideally if we have
a bunch of i2c transfers grouped together we only do this once for the
entire group.

Taking the read-modify-write on a PMIC register as example then ideally we
would only do all 3 steps once at the beginning and undo all 3 steps once
at the end.

For this we need to be able to take the semaphore from within e.g. the PMIC
opregion driver, yet we do not want to remove the taking of the semaphore
from the I2C-controller driver, as that is still necessary to protect many
other code-paths leading to accessing the shared I2C bus.

This means that we first have the PMIC driver acquire the semaphore and
then have the I2C controller driver trying to acquire it again.

To make this possible this commit does the following:

1) Move the semaphore code from being private to the I2C controller driver
into the generic iosf_mbi code, which already has other code to deal with
the shared bus so that it can be accessed outside of the I2C bus driver.

2) Rework the code so that it can be called multiple times nested, while
still blocking I2C accesses while e.g. the GPU driver has indicated the
P-Unit needs the bus through a iosf_mbi_punit_acquire() call.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note this commit deliberately limits the i2c-designware changes to
only touch i2c-designware-baytrail.c, deliberately not doing some cleanups
which become possible after removing the semaphore code from the
i2c-designmware code. This is done so that this commit can be merged
through the x86 tree without causing conflicts in the i2c tree.

The cleanups to the i2c-designware tree will be done in a follow up
patch which can be merged once this commit is in place.
---
 arch/x86/include/asm/iosf_mbi.h              |  39 +++--
 arch/x86/platform/intel/iosf_mbi.c           | 161 +++++++++++++++++--
 drivers/i2c/busses/i2c-designware-baytrail.c | 125 +-------------
 3 files changed, 180 insertions(+), 145 deletions(-)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index 3de0489deade..5270ff39b9af 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -105,8 +105,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
  * the PMIC bus while another driver is also accessing the PMIC bus various bad
  * things happen.
  *
- * To avoid these problems this function must be called before accessing the
- * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means.
+ * Call this function before sending requests to the P-Unit which may make it
+ * access the PMIC, be it through iosf_mbi* functions or through other means.
+ * This function will block all kernel access to the PMIC I2C bus, so that the
+ * P-Unit can safely access the PMIC over the shared I2C bus.
  *
  * Note on these systems the i2c-bus driver will request a sempahore from the
  * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing
@@ -122,6 +124,31 @@ void iosf_mbi_punit_acquire(void);
  */
 void iosf_mbi_punit_release(void);
 
+/**
+ * iosf_mbi_block_punit_i2c_access() - Block P-Unit accesses to the PMIC bus
+ *
+ * Call this function to block P-Unit access to the PMIC I2C bus, so that the
+ * kernel can safely access the PMIC over the shared I2C bus.
+ *
+ * This function acquires the P-Unit bus semaphore and notifies
+ * pmic_bus_access_notifier listeners that they may no longer access the
+ * P-Unit in a way which may cause it to access the shared I2C bus.
+ *
+ * Note this function may be called multiple times and the bus will not
+ * be released until iosf_mbi_unblock_punit_i2c_access() has been called the
+ * same amount of times.
+ *
+ * Return: Nonzero on error
+ */
+int iosf_mbi_block_punit_i2c_access(void);
+
+/*
+ * iosf_mbi_unblock_punit_i2c_access() - Release PMIC I2C bus block
+ *
+ * Release i2c access block gotten through iosf_mbi_block_punit_i2c_access().
+ */
+void iosf_mbi_unblock_punit_i2c_access(void);
+
 /**
  * iosf_mbi_register_pmic_bus_access_notifier - Register PMIC bus notifier
  *
@@ -158,14 +185,6 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
 int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
 	struct notifier_block *nb);
 
-/**
- * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain
- *
- * @val: action to pass into listener's notifier_call function
- * @v: data pointer to pass into listener's notifier_call function
- */
-int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
-
 /**
  * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired.
  */
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index 6f37a2137a79..5623a315f004 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -18,12 +18,14 @@
  * enumerate the device using PCI.
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <linux/pci.h>
 #include <linux/debugfs.h>
 #include <linux/capability.h>
+#include <linux/pm_qos.h>
 
 #include <asm/iosf_mbi.h>
 
@@ -34,8 +36,8 @@
 
 static struct pci_dev *mbi_pdev;
 static DEFINE_SPINLOCK(iosf_mbi_lock);
-static DEFINE_MUTEX(iosf_mbi_punit_mutex);
-static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
+
+/**************** Generic iosf_mbi access helpers ****************/
 
 static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
 {
@@ -192,6 +194,30 @@ bool iosf_mbi_available(void)
 }
 EXPORT_SYMBOL(iosf_mbi_available);
 
+/*
+ **************** PUNIT/kernel shared i2c bus arbritration ****************
+ *
+ * Some Bay Trail and Cherry Trail devices have the PUNIT and us (the kernel)
+ * share a single I2C bus to the PMIC. Below are helpers to arbitrate the
+ * accesses between the kernel and the PUNIT.
+ *
+ * See arch/x86/include/asm/iosf_mbi.h for kernel-doc text for each function.
+ */
+
+#define SEMAPHORE_TIMEOUT		500
+#define PUNIT_SEMAPHORE_BYT		0x7
+#define PUNIT_SEMAPHORE_CHT		0x10e
+#define PUNIT_SEMAPHORE_BIT		BIT(0)
+#define PUNIT_SEMAPHORE_ACQUIRE		BIT(1)
+
+static DEFINE_MUTEX(iosf_mbi_punit_mutex);
+static DEFINE_MUTEX(iosf_mbi_block_punit_i2c_access_count_mutex);
+static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
+static u32 iosf_mbi_block_punit_i2c_access_count;
+static u32 iosf_mbi_sem_address;
+static unsigned long iosf_mbi_sem_acquired;
+static struct pm_qos_request iosf_mbi_pm_qos;
+
 void iosf_mbi_punit_acquire(void)
 {
 	mutex_lock(&iosf_mbi_punit_mutex);
@@ -204,6 +230,113 @@ void iosf_mbi_punit_release(void)
 }
 EXPORT_SYMBOL(iosf_mbi_punit_release);
 
+static int iosf_mbi_get_sem(u32 *sem)
+{
+	int ret;
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
+			    iosf_mbi_sem_address, sem);
+	if (ret) {
+		dev_err(&mbi_pdev->dev, "Error punit semaphore read failed\n");
+		return ret;
+	}
+
+	*sem &= PUNIT_SEMAPHORE_BIT;
+	return 0;
+}
+
+static void iosf_mbi_reset_semaphore(void)
+{
+	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
+			    iosf_mbi_sem_address, 0, PUNIT_SEMAPHORE_BIT))
+		dev_err(&mbi_pdev->dev, "Error punit semaphore reset failed\n");
+
+	pm_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
+
+	blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
+				     MBI_PMIC_BUS_ACCESS_END, NULL);
+	mutex_unlock(&iosf_mbi_punit_mutex);
+}
+
+int iosf_mbi_block_punit_i2c_access(void)
+{
+	unsigned long start, end;
+	int ret = 0;
+	u32 sem;
+
+	if (WARN_ON(!mbi_pdev || !iosf_mbi_sem_address))
+		return -ENXIO;
+
+	mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
+
+	if (iosf_mbi_block_punit_i2c_access_count > 0)
+		goto out;
+
+	mutex_lock(&iosf_mbi_punit_mutex);
+	blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
+				     MBI_PMIC_BUS_ACCESS_BEGIN, NULL);
+
+	/*
+	 * 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(&iosf_mbi_pm_qos, 0);
+
+	/* host driver writes to side band semaphore register */
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
+			     iosf_mbi_sem_address, PUNIT_SEMAPHORE_ACQUIRE);
+	if (ret) {
+		dev_err(&mbi_pdev->dev, "Error punit semaphore request failed\n");
+		goto out;
+	}
+
+	/* host driver waits for bit 0 to be set in semaphore register */
+	start = jiffies;
+	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
+	do {
+		ret = iosf_mbi_get_sem(&sem);
+		if (!ret && sem) {
+			iosf_mbi_sem_acquired = jiffies;
+			dev_dbg(&mbi_pdev->dev, "punit semaphore acquired after %ums\n",
+				jiffies_to_msecs(jiffies - start));
+			goto out; /* Success, done. */
+		}
+
+		usleep_range(1000, 2000);
+	} while (time_before(jiffies, end));
+
+	ret = -ETIMEDOUT;
+	dev_err(&mbi_pdev->dev, "Error punit semaphore timed out, resetting\n");
+	iosf_mbi_reset_semaphore();
+
+	if (!iosf_mbi_get_sem(&sem))
+		dev_err(&mbi_pdev->dev, "PUNIT SEM: %d\n", sem);
+out:
+	if (!WARN_ON(ret))
+		iosf_mbi_block_punit_i2c_access_count++;
+
+	mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_block_punit_i2c_access);
+
+void iosf_mbi_unblock_punit_i2c_access(void)
+{
+	mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
+
+	iosf_mbi_block_punit_i2c_access_count--;
+	if (iosf_mbi_block_punit_i2c_access_count == 0) {
+		iosf_mbi_reset_semaphore();
+		dev_dbg(&mbi_pdev->dev, "punit semaphore held for %ums\n",
+			jiffies_to_msecs(jiffies - iosf_mbi_sem_acquired));
+	}
+
+	mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_unblock_punit_i2c_access);
+
 int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
 {
 	int ret;
@@ -241,19 +374,14 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
 
-int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
-{
-	return blocking_notifier_call_chain(
-				&iosf_mbi_pmic_bus_access_notifier, val, v);
-}
-EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
-
 void iosf_mbi_assert_punit_acquired(void)
 {
 	WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex));
 }
 EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired);
 
+/**************** iosf_mbi debug code ****************/
+
 #ifdef CONFIG_IOSF_MBI_DEBUG
 static u32	dbg_mdr;
 static u32	dbg_mcr;
@@ -338,7 +466,7 @@ static inline void iosf_debugfs_remove(void) { }
 #endif /* CONFIG_IOSF_MBI_DEBUG */
 
 static int iosf_mbi_probe(struct pci_dev *pdev,
-			  const struct pci_device_id *unused)
+			  const struct pci_device_id *dev_id)
 {
 	int ret;
 
@@ -349,12 +477,16 @@ static int iosf_mbi_probe(struct pci_dev *pdev,
 	}
 
 	mbi_pdev = pci_dev_get(pdev);
+	iosf_mbi_sem_address = dev_id->driver_data;
+
 	return 0;
 }
 
 static const struct pci_device_id iosf_mbi_pci_ids[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL),
+	  .driver_data = PUNIT_SEMAPHORE_BYT },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL),
+	  .driver_data = PUNIT_SEMAPHORE_CHT },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
 	{ 0, },
@@ -371,6 +503,9 @@ static int __init iosf_mbi_init(void)
 {
 	iosf_debugfs_init();
 
+	pm_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_CPU_DMA_LATENCY,
+			   PM_QOS_DEFAULT_VALUE);
+
 	return pci_register_driver(&iosf_mbi_pci_driver);
 }
 
@@ -381,6 +516,8 @@ static void __exit iosf_mbi_exit(void)
 	pci_unregister_driver(&iosf_mbi_pci_driver);
 	pci_dev_put(mbi_pdev);
 	mbi_pdev = NULL;
+
+	pm_qos_remove_request(&iosf_mbi_pm_qos);
 }
 
 module_init(iosf_mbi_init);
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 9ca1feaba98f..8c84fa0b0384 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -3,139 +3,23 @@
  * Intel BayTrail PMIC I2C bus semaphore implementaion
  * Copyright (c) 2014, Intel Corporation.
  */
-#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/pm_qos.h>
 
 #include <asm/iosf_mbi.h>
 
 #include "i2c-designware-core.h"
 
-#define SEMAPHORE_TIMEOUT	500
-#define PUNIT_SEMAPHORE		0x7
-#define PUNIT_SEMAPHORE_CHT	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_CHT;
-	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;
-
-	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;
-	}
-
-	*sem = data & PUNIT_SEMAPHORE_BIT;
-
-	return 0;
-}
-
-static void reset_semaphore(struct dw_i2c_dev *dev)
-{
-	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
-			    0, PUNIT_SEMAPHORE_BIT))
-		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
-
-	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
-
-	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
-						     NULL);
-	iosf_mbi_punit_release();
-}
-
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 {
-	u32 addr;
-	u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
-	int ret;
-	unsigned long start, end;
-
-	might_sleep();
-
-	if (!dev || !dev->dev)
-		return -ENODEV;
-
-	if (!dev->release_lock)
-		return 0;
-
-	iosf_mbi_punit_acquire();
-	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
-						     NULL);
-
-	/*
-	 * 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);
-
-	addr = get_sem_addr(dev);
-
-	/* host driver writes to side band semaphore register */
-	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");
-		goto out;
-	}
-
-	/* host driver waits for bit 0 to be set in semaphore register */
-	start = jiffies;
-	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
-	do {
-		ret = get_sem(dev, &sem);
-		if (!ret && sem) {
-			acquired = jiffies;
-			dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
-				jiffies_to_msecs(jiffies - start));
-			return 0;
-		}
-
-		usleep_range(1000, 2000);
-	} while (time_before(jiffies, end));
-
-	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
-out:
-	reset_semaphore(dev);
-
-	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
-		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
-
-	WARN_ON(1);
-
-	return -ETIMEDOUT;
+	return iosf_mbi_block_punit_i2c_access();
 }
 
 static void baytrail_i2c_release(struct dw_i2c_dev *dev)
 {
-	if (!dev || !dev->dev)
-		return;
-
-	if (!dev->acquire_lock)
-		return;
-
-	reset_semaphore(dev);
-	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
-		jiffies_to_msecs(jiffies - acquired));
+	iosf_mbi_unblock_punit_i2c_access();
 }
 
 int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
@@ -166,14 +50,9 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 	dev->release_lock = baytrail_i2c_release;
 	dev->shared_with_punit = true;
 
-	pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
-			   PM_QOS_DEFAULT_VALUE);
-
 	return 0;
 }
 
 void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
 {
-	if (dev->acquire_lock)
-		pm_qos_remove_request(&dev->pm_qos);
 }
-- 
2.19.0.rc1

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Andy Shevchenko]

On Sun, Sep 23, 2018 at 04:45:08PM +0200, Hans de Goede wrote:
> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
> block it from accessing the shared bus while the kernel wants to access it.
> 
> Currently we have the I2C-controller driver acquiring and releasing the
> semaphore around each I2C transfer. There are 2 problems with this:
> 
> 1) PMIC accesses often come in the form of a read-modify-write on one of
> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
> between the read and the write. If the P-Unit modifies the register during
> this window?, then we end up overwriting the P-Unit's changes.
> I believe that this is mostly an academic problem, but I'm not sure.
> 
> 2) To safely access the shared I2C bus, we need to do 3 things:
> a) Notify the GPU driver that we are starting a window in which it may not
> access the P-Unit, since the P-Unit seems to ignore the semaphore for
> explicit power-level requests made by the GPU driver
> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
> C6/C7 while we hold the semaphore hangs the SoC
> c) Finally take the P-Unit's PMIC bus semaphore
> All 3 these steps together are somewhat expensive, so ideally if we have
> a bunch of i2c transfers grouped together we only do this once for the
> entire group.
> 
> Taking the read-modify-write on a PMIC register as example then ideally we
> would only do all 3 steps once at the beginning and undo all 3 steps once
> at the end.
> 
> For this we need to be able to take the semaphore from within e.g. the PMIC
> opregion driver, yet we do not want to remove the taking of the semaphore
> from the I2C-controller driver, as that is still necessary to protect many
> other code-paths leading to accessing the shared I2C bus.
> 
> This means that we first have the PMIC driver acquire the semaphore and
> then have the I2C controller driver trying to acquire it again.
> 
> To make this possible this commit does the following:
> 
> 1) Move the semaphore code from being private to the I2C controller driver
> into the generic iosf_mbi code, which already has other code to deal with
> the shared bus so that it can be accessed outside of the I2C bus driver.
> 
> 2) Rework the code so that it can be called multiple times nested, while
> still blocking I2C accesses while e.g. the GPU driver has indicated the
> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note this commit deliberately limits the i2c-designware changes to
> only touch i2c-designware-baytrail.c, deliberately not doing some cleanups
> which become possible after removing the semaphore code from the
> i2c-designmware code. This is done so that this commit can be merged
> through the x86 tree without causing conflicts in the i2c tree.
> 
> The cleanups to the i2c-designware tree will be done in a follow up
> patch which can be merged once this commit is in place.

> +static void iosf_mbi_reset_semaphore(void)
> +{
> +	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> +			    iosf_mbi_sem_address, 0, PUNIT_SEMAPHORE_BIT))
> +		dev_err(&mbi_pdev->dev, "Error punit semaphore reset failed\n");
> +
> +	pm_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
> +
> +	blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
> +				     MBI_PMIC_BUS_ACCESS_END, NULL);

> +	mutex_unlock(&iosf_mbi_punit_mutex);

Can we actually move this to the callers?
To me sounds slightly more logical to see lock in *block*() call and unlock in
*unblock*() respectively.

> +}

> +int iosf_mbi_block_punit_i2c_access(void)
> +{
> +	unsigned long start, end;
> +	int ret = 0;
> +	u32 sem;
> +
> +	if (WARN_ON(!mbi_pdev || !iosf_mbi_sem_address))
> +		return -ENXIO;
> +
> +	mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +
> +	if (iosf_mbi_block_punit_i2c_access_count > 0)
> +		goto out;
> +
> +	mutex_lock(&iosf_mbi_punit_mutex);
> +	blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
> +				     MBI_PMIC_BUS_ACCESS_BEGIN, NULL);
> +
> +	/*
> +	 * 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(&iosf_mbi_pm_qos, 0);
> +
> +	/* host driver writes to side band semaphore register */
> +	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> +			     iosf_mbi_sem_address, PUNIT_SEMAPHORE_ACQUIRE);
> +	if (ret) {
> +		dev_err(&mbi_pdev->dev, "Error punit semaphore request failed\n");
> +		goto out;
> +	}
> +
> +	/* host driver waits for bit 0 to be set in semaphore register */
> +	start = jiffies;
> +	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
> +	do {
> +		ret = iosf_mbi_get_sem(&sem);
> +		if (!ret && sem) {
> +			iosf_mbi_sem_acquired = jiffies;
> +			dev_dbg(&mbi_pdev->dev, "punit semaphore acquired after %ums\n",
> +				jiffies_to_msecs(jiffies - start));
> +			goto out; /* Success, done. */
> +		}
> +
> +		usleep_range(1000, 2000);
> +	} while (time_before(jiffies, end));
> +
> +	ret = -ETIMEDOUT;
> +	dev_err(&mbi_pdev->dev, "Error punit semaphore timed out, resetting\n");
> +	iosf_mbi_reset_semaphore();
> +
> +	if (!iosf_mbi_get_sem(&sem))
> +		dev_err(&mbi_pdev->dev, "PUNIT SEM: %d\n", sem);
> +out:
> +	if (!WARN_ON(ret))
> +		iosf_mbi_block_punit_i2c_access_count++;
> +
> +	mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iosf_mbi_block_punit_i2c_access);
> +
> +void iosf_mbi_unblock_punit_i2c_access(void)
> +{
> +	mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +
> +	iosf_mbi_block_punit_i2c_access_count--;
> +	if (iosf_mbi_block_punit_i2c_access_count == 0) {
> +		iosf_mbi_reset_semaphore();
> +		dev_dbg(&mbi_pdev->dev, "punit semaphore held for %ums\n",
> +			jiffies_to_msecs(jiffies - iosf_mbi_sem_acquired));
> +	}
> +
> +	mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_unblock_punit_i2c_access);

> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL),
> +	  .driver_data = PUNIT_SEMAPHORE_BYT },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL),
> +	  .driver_data = PUNIT_SEMAPHORE_CHT },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
>  	{ 0, },

Perhaps it can be converted to use PCI_DEVICE_DATA() macro.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Hans de Goede]

Hi,

On 24-09-18 11:48, Andy Shevchenko wrote:
> On Sun, Sep 23, 2018 at 04:45:08PM +0200, Hans de Goede wrote:
>> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
>> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
>> block it from accessing the shared bus while the kernel wants to access it.
>>
>> Currently we have the I2C-controller driver acquiring and releasing the
>> semaphore around each I2C transfer. There are 2 problems with this:
>>
>> 1) PMIC accesses often come in the form of a read-modify-write on one of
>> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
>> between the read and the write. If the P-Unit modifies the register during
>> this window?, then we end up overwriting the P-Unit's changes.
>> I believe that this is mostly an academic problem, but I'm not sure.
>>
>> 2) To safely access the shared I2C bus, we need to do 3 things:
>> a) Notify the GPU driver that we are starting a window in which it may not
>> access the P-Unit, since the P-Unit seems to ignore the semaphore for
>> explicit power-level requests made by the GPU driver
>> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
>> C6/C7 while we hold the semaphore hangs the SoC
>> c) Finally take the P-Unit's PMIC bus semaphore
>> All 3 these steps together are somewhat expensive, so ideally if we have
>> a bunch of i2c transfers grouped together we only do this once for the
>> entire group.
>>
>> Taking the read-modify-write on a PMIC register as example then ideally we
>> would only do all 3 steps once at the beginning and undo all 3 steps once
>> at the end.
>>
>> For this we need to be able to take the semaphore from within e.g. the PMIC
>> opregion driver, yet we do not want to remove the taking of the semaphore
>> from the I2C-controller driver, as that is still necessary to protect many
>> other code-paths leading to accessing the shared I2C bus.
>>
>> This means that we first have the PMIC driver acquire the semaphore and
>> then have the I2C controller driver trying to acquire it again.
>>
>> To make this possible this commit does the following:
>>
>> 1) Move the semaphore code from being private to the I2C controller driver
>> into the generic iosf_mbi code, which already has other code to deal with
>> the shared bus so that it can be accessed outside of the I2C bus driver.
>>
>> 2) Rework the code so that it can be called multiple times nested, while
>> still blocking I2C accesses while e.g. the GPU driver has indicated the
>> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note this commit deliberately limits the i2c-designware changes to
>> only touch i2c-designware-baytrail.c, deliberately not doing some cleanups
>> which become possible after removing the semaphore code from the
>> i2c-designmware code. This is done so that this commit can be merged
>> through the x86 tree without causing conflicts in the i2c tree.
>>
>> The cleanups to the i2c-designware tree will be done in a follow up
>> patch which can be merged once this commit is in place.
> 
>> +static void iosf_mbi_reset_semaphore(void)
>> +{
>> +	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> +			    iosf_mbi_sem_address, 0, PUNIT_SEMAPHORE_BIT))
>> +		dev_err(&mbi_pdev->dev, "Error punit semaphore reset failed\n");
>> +
>> +	pm_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
>> +
>> +	blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
>> +				     MBI_PMIC_BUS_ACCESS_END, NULL);
> 
>> +	mutex_unlock(&iosf_mbi_punit_mutex);
> 
> Can we actually move this to the callers?
> To me sounds slightly more logical to see lock in *block*() call and unlock in
> *unblock*() respectively.

Done for v2, which I will send out as soon as I've ran some tests with it.

>> +}
> 
>> +int iosf_mbi_block_punit_i2c_access(void)
>> +{
>> +	unsigned long start, end;
>> +	int ret = 0;
>> +	u32 sem;
>> +
>> +	if (WARN_ON(!mbi_pdev || !iosf_mbi_sem_address))
>> +		return -ENXIO;
>> +
>> +	mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +
>> +	if (iosf_mbi_block_punit_i2c_access_count > 0)
>> +		goto out;
>> +
>> +	mutex_lock(&iosf_mbi_punit_mutex);
>> +	blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
>> +				     MBI_PMIC_BUS_ACCESS_BEGIN, NULL);
>> +
>> +	/*
>> +	 * 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(&iosf_mbi_pm_qos, 0);
>> +
>> +	/* host driver writes to side band semaphore register */
>> +	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>> +			     iosf_mbi_sem_address, PUNIT_SEMAPHORE_ACQUIRE);
>> +	if (ret) {
>> +		dev_err(&mbi_pdev->dev, "Error punit semaphore request failed\n");
>> +		goto out;
>> +	}
>> +
>> +	/* host driver waits for bit 0 to be set in semaphore register */
>> +	start = jiffies;
>> +	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
>> +	do {
>> +		ret = iosf_mbi_get_sem(&sem);
>> +		if (!ret && sem) {
>> +			iosf_mbi_sem_acquired = jiffies;
>> +			dev_dbg(&mbi_pdev->dev, "punit semaphore acquired after %ums\n",
>> +				jiffies_to_msecs(jiffies - start));
>> +			goto out; /* Success, done. */
>> +		}
>> +
>> +		usleep_range(1000, 2000);
>> +	} while (time_before(jiffies, end));
>> +
>> +	ret = -ETIMEDOUT;
>> +	dev_err(&mbi_pdev->dev, "Error punit semaphore timed out, resetting\n");
>> +	iosf_mbi_reset_semaphore();
>> +
>> +	if (!iosf_mbi_get_sem(&sem))
>> +		dev_err(&mbi_pdev->dev, "PUNIT SEM: %d\n", sem);
>> +out:
>> +	if (!WARN_ON(ret))
>> +		iosf_mbi_block_punit_i2c_access_count++;
>> +
>> +	mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_block_punit_i2c_access);
>> +
>> +void iosf_mbi_unblock_punit_i2c_access(void)
>> +{
>> +	mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +
>> +	iosf_mbi_block_punit_i2c_access_count--;
>> +	if (iosf_mbi_block_punit_i2c_access_count == 0) {
>> +		iosf_mbi_reset_semaphore();
>> +		dev_dbg(&mbi_pdev->dev, "punit semaphore held for %ums\n",
>> +			jiffies_to_msecs(jiffies - iosf_mbi_sem_acquired));
>> +	}
>> +
>> +	mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_unblock_punit_i2c_access);
> 
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL),
>> +	  .driver_data = PUNIT_SEMAPHORE_BYT },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL),
>> +	  .driver_data = PUNIT_SEMAPHORE_CHT },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
>>   	{ 0, },
> 
> Perhaps it can be converted to use PCI_DEVICE_DATA() macro.

Also done for v2.

Regards,

Hans

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Rafael J. Wysocki]

On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
> block it from accessing the shared bus while the kernel wants to access it.
>
> Currently we have the I2C-controller driver acquiring and releasing the
> semaphore around each I2C transfer. There are 2 problems with this:
>
> 1) PMIC accesses often come in the form of a read-modify-write on one of
> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
> between the read and the write. If the P-Unit modifies the register during
> this window?, then we end up overwriting the P-Unit's changes.
> I believe that this is mostly an academic problem, but I'm not sure.
>
> 2) To safely access the shared I2C bus, we need to do 3 things:
> a) Notify the GPU driver that we are starting a window in which it may not
> access the P-Unit, since the P-Unit seems to ignore the semaphore for
> explicit power-level requests made by the GPU driver
> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
> C6/C7 while we hold the semaphore hangs the SoC
> c) Finally take the P-Unit's PMIC bus semaphore
> All 3 these steps together are somewhat expensive, so ideally if we have
> a bunch of i2c transfers grouped together we only do this once for the
> entire group.
>
> Taking the read-modify-write on a PMIC register as example then ideally we
> would only do all 3 steps once at the beginning and undo all 3 steps once
> at the end.
>
> For this we need to be able to take the semaphore from within e.g. the PMIC
> opregion driver, yet we do not want to remove the taking of the semaphore
> from the I2C-controller driver, as that is still necessary to protect many
> other code-paths leading to accessing the shared I2C bus.
>
> This means that we first have the PMIC driver acquire the semaphore and
> then have the I2C controller driver trying to acquire it again.
>
> To make this possible this commit does the following:
>
> 1) Move the semaphore code from being private to the I2C controller driver
> into the generic iosf_mbi code, which already has other code to deal with
> the shared bus so that it can be accessed outside of the I2C bus driver.
>
> 2) Rework the code so that it can be called multiple times nested, while
> still blocking I2C accesses while e.g. the GPU driver has indicated the
> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

If there are no objections or concerns regarding this patch, I'm
inclined to take the entire series including it.

> ---
> Note this commit deliberately limits the i2c-designware changes to
> only touch i2c-designware-baytrail.c, deliberately not doing some cleanups
> which become possible after removing the semaphore code from the
> i2c-designmware code. This is done so that this commit can be merged
> through the x86 tree without causing conflicts in the i2c tree.
>
> The cleanups to the i2c-designware tree will be done in a follow up
> patch which can be merged once this commit is in place.
> ---
>  arch/x86/include/asm/iosf_mbi.h              |  39 +++--
>  arch/x86/platform/intel/iosf_mbi.c           | 161 +++++++++++++++++--
>  drivers/i2c/busses/i2c-designware-baytrail.c | 125 +-------------
>  3 files changed, 180 insertions(+), 145 deletions(-)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
> index 3de0489deade..5270ff39b9af 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -105,8 +105,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
>   * the PMIC bus while another driver is also accessing the PMIC bus various bad
>   * things happen.
>   *
> - * To avoid these problems this function must be called before accessing the
> - * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means.
> + * Call this function before sending requests to the P-Unit which may make it
> + * access the PMIC, be it through iosf_mbi* functions or through other means.
> + * This function will block all kernel access to the PMIC I2C bus, so that the
> + * P-Unit can safely access the PMIC over the shared I2C bus.
>   *
>   * Note on these systems the i2c-bus driver will request a sempahore from the
>   * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing
> @@ -122,6 +124,31 @@ void iosf_mbi_punit_acquire(void);
>   */
>  void iosf_mbi_punit_release(void);
>
> +/**
> + * iosf_mbi_block_punit_i2c_access() - Block P-Unit accesses to the PMIC bus
> + *
> + * Call this function to block P-Unit access to the PMIC I2C bus, so that the
> + * kernel can safely access the PMIC over the shared I2C bus.
> + *
> + * This function acquires the P-Unit bus semaphore and notifies
> + * pmic_bus_access_notifier listeners that they may no longer access the
> + * P-Unit in a way which may cause it to access the shared I2C bus.
> + *
> + * Note this function may be called multiple times and the bus will not
> + * be released until iosf_mbi_unblock_punit_i2c_access() has been called the
> + * same amount of times.
> + *
> + * Return: Nonzero on error
> + */
> +int iosf_mbi_block_punit_i2c_access(void);
> +
> +/*
> + * iosf_mbi_unblock_punit_i2c_access() - Release PMIC I2C bus block
> + *
> + * Release i2c access block gotten through iosf_mbi_block_punit_i2c_access().
> + */
> +void iosf_mbi_unblock_punit_i2c_access(void);
> +
>  /**
>   * iosf_mbi_register_pmic_bus_access_notifier - Register PMIC bus notifier
>   *
> @@ -158,14 +185,6 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
>  int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>         struct notifier_block *nb);
>
> -/**
> - * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain
> - *
> - * @val: action to pass into listener's notifier_call function
> - * @v: data pointer to pass into listener's notifier_call function
> - */
> -int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
> -
>  /**
>   * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired.
>   */
> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
> index 6f37a2137a79..5623a315f004 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -18,12 +18,14 @@
>   * enumerate the device using PCI.
>   */
>
> +#include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/spinlock.h>
>  #include <linux/pci.h>
>  #include <linux/debugfs.h>
>  #include <linux/capability.h>
> +#include <linux/pm_qos.h>
>
>  #include <asm/iosf_mbi.h>
>
> @@ -34,8 +36,8 @@
>
>  static struct pci_dev *mbi_pdev;
>  static DEFINE_SPINLOCK(iosf_mbi_lock);
> -static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> -static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
> +
> +/**************** Generic iosf_mbi access helpers ****************/
>
>  static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
>  {
> @@ -192,6 +194,30 @@ bool iosf_mbi_available(void)
>  }
>  EXPORT_SYMBOL(iosf_mbi_available);
>
> +/*
> + **************** PUNIT/kernel shared i2c bus arbritration ****************
> + *
> + * Some Bay Trail and Cherry Trail devices have the PUNIT and us (the kernel)
> + * share a single I2C bus to the PMIC. Below are helpers to arbitrate the
> + * accesses between the kernel and the PUNIT.
> + *
> + * See arch/x86/include/asm/iosf_mbi.h for kernel-doc text for each function.
> + */
> +
> +#define SEMAPHORE_TIMEOUT              500
> +#define PUNIT_SEMAPHORE_BYT            0x7
> +#define PUNIT_SEMAPHORE_CHT            0x10e
> +#define PUNIT_SEMAPHORE_BIT            BIT(0)
> +#define PUNIT_SEMAPHORE_ACQUIRE                BIT(1)
> +
> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> +static DEFINE_MUTEX(iosf_mbi_block_punit_i2c_access_count_mutex);
> +static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
> +static u32 iosf_mbi_block_punit_i2c_access_count;
> +static u32 iosf_mbi_sem_address;
> +static unsigned long iosf_mbi_sem_acquired;
> +static struct pm_qos_request iosf_mbi_pm_qos;
> +
>  void iosf_mbi_punit_acquire(void)
>  {
>         mutex_lock(&iosf_mbi_punit_mutex);
> @@ -204,6 +230,113 @@ void iosf_mbi_punit_release(void)
>  }
>  EXPORT_SYMBOL(iosf_mbi_punit_release);
>
> +static int iosf_mbi_get_sem(u32 *sem)
> +{
> +       int ret;
> +
> +       ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> +                           iosf_mbi_sem_address, sem);
> +       if (ret) {
> +               dev_err(&mbi_pdev->dev, "Error punit semaphore read failed\n");
> +               return ret;
> +       }
> +
> +       *sem &= PUNIT_SEMAPHORE_BIT;
> +       return 0;
> +}
> +
> +static void iosf_mbi_reset_semaphore(void)
> +{
> +       if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> +                           iosf_mbi_sem_address, 0, PUNIT_SEMAPHORE_BIT))
> +               dev_err(&mbi_pdev->dev, "Error punit semaphore reset failed\n");
> +
> +       pm_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
> +
> +       blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
> +                                    MBI_PMIC_BUS_ACCESS_END, NULL);
> +       mutex_unlock(&iosf_mbi_punit_mutex);
> +}
> +
> +int iosf_mbi_block_punit_i2c_access(void)
> +{
> +       unsigned long start, end;
> +       int ret = 0;
> +       u32 sem;
> +
> +       if (WARN_ON(!mbi_pdev || !iosf_mbi_sem_address))
> +               return -ENXIO;
> +
> +       mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +
> +       if (iosf_mbi_block_punit_i2c_access_count > 0)
> +               goto out;
> +
> +       mutex_lock(&iosf_mbi_punit_mutex);
> +       blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
> +                                    MBI_PMIC_BUS_ACCESS_BEGIN, NULL);
> +
> +       /*
> +        * 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(&iosf_mbi_pm_qos, 0);
> +
> +       /* host driver writes to side band semaphore register */
> +       ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> +                            iosf_mbi_sem_address, PUNIT_SEMAPHORE_ACQUIRE);
> +       if (ret) {
> +               dev_err(&mbi_pdev->dev, "Error punit semaphore request failed\n");
> +               goto out;
> +       }
> +
> +       /* host driver waits for bit 0 to be set in semaphore register */
> +       start = jiffies;
> +       end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
> +       do {
> +               ret = iosf_mbi_get_sem(&sem);
> +               if (!ret && sem) {
> +                       iosf_mbi_sem_acquired = jiffies;
> +                       dev_dbg(&mbi_pdev->dev, "punit semaphore acquired after %ums\n",
> +                               jiffies_to_msecs(jiffies - start));
> +                       goto out; /* Success, done. */
> +               }
> +
> +               usleep_range(1000, 2000);
> +       } while (time_before(jiffies, end));
> +
> +       ret = -ETIMEDOUT;
> +       dev_err(&mbi_pdev->dev, "Error punit semaphore timed out, resetting\n");
> +       iosf_mbi_reset_semaphore();
> +
> +       if (!iosf_mbi_get_sem(&sem))
> +               dev_err(&mbi_pdev->dev, "PUNIT SEM: %d\n", sem);
> +out:
> +       if (!WARN_ON(ret))
> +               iosf_mbi_block_punit_i2c_access_count++;
> +
> +       mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(iosf_mbi_block_punit_i2c_access);
> +
> +void iosf_mbi_unblock_punit_i2c_access(void)
> +{
> +       mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +
> +       iosf_mbi_block_punit_i2c_access_count--;
> +       if (iosf_mbi_block_punit_i2c_access_count == 0) {
> +               iosf_mbi_reset_semaphore();
> +               dev_dbg(&mbi_pdev->dev, "punit semaphore held for %ums\n",
> +                       jiffies_to_msecs(jiffies - iosf_mbi_sem_acquired));
> +       }
> +
> +       mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_unblock_punit_i2c_access);
> +
>  int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
>  {
>         int ret;
> @@ -241,19 +374,14 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
>
> -int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
> -{
> -       return blocking_notifier_call_chain(
> -                               &iosf_mbi_pmic_bus_access_notifier, val, v);
> -}
> -EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
> -
>  void iosf_mbi_assert_punit_acquired(void)
>  {
>         WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex));
>  }
>  EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired);
>
> +/**************** iosf_mbi debug code ****************/
> +
>  #ifdef CONFIG_IOSF_MBI_DEBUG
>  static u32     dbg_mdr;
>  static u32     dbg_mcr;
> @@ -338,7 +466,7 @@ static inline void iosf_debugfs_remove(void) { }
>  #endif /* CONFIG_IOSF_MBI_DEBUG */
>
>  static int iosf_mbi_probe(struct pci_dev *pdev,
> -                         const struct pci_device_id *unused)
> +                         const struct pci_device_id *dev_id)
>  {
>         int ret;
>
> @@ -349,12 +477,16 @@ static int iosf_mbi_probe(struct pci_dev *pdev,
>         }
>
>         mbi_pdev = pci_dev_get(pdev);
> +       iosf_mbi_sem_address = dev_id->driver_data;
> +
>         return 0;
>  }
>
>  static const struct pci_device_id iosf_mbi_pci_ids[] = {
> -       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL) },
> -       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL) },
> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL),
> +         .driver_data = PUNIT_SEMAPHORE_BYT },
> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL),
> +         .driver_data = PUNIT_SEMAPHORE_CHT },
>         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
>         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
>         { 0, },
> @@ -371,6 +503,9 @@ static int __init iosf_mbi_init(void)
>  {
>         iosf_debugfs_init();
>
> +       pm_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_CPU_DMA_LATENCY,
> +                          PM_QOS_DEFAULT_VALUE);
> +
>         return pci_register_driver(&iosf_mbi_pci_driver);
>  }
>
> @@ -381,6 +516,8 @@ static void __exit iosf_mbi_exit(void)
>         pci_unregister_driver(&iosf_mbi_pci_driver);
>         pci_dev_put(mbi_pdev);
>         mbi_pdev = NULL;
> +
> +       pm_qos_remove_request(&iosf_mbi_pm_qos);
>  }
>
>  module_init(iosf_mbi_init);
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 9ca1feaba98f..8c84fa0b0384 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -3,139 +3,23 @@
>   * Intel BayTrail PMIC I2C bus semaphore implementaion
>   * Copyright (c) 2014, Intel Corporation.
>   */
> -#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/acpi.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> -#include <linux/pm_qos.h>
>
>  #include <asm/iosf_mbi.h>
>
>  #include "i2c-designware-core.h"
>
> -#define SEMAPHORE_TIMEOUT      500
> -#define PUNIT_SEMAPHORE                0x7
> -#define PUNIT_SEMAPHORE_CHT    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_CHT;
> -       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;
> -
> -       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;
> -       }
> -
> -       *sem = data & PUNIT_SEMAPHORE_BIT;
> -
> -       return 0;
> -}
> -
> -static void reset_semaphore(struct dw_i2c_dev *dev)
> -{
> -       if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
> -                           0, PUNIT_SEMAPHORE_BIT))
> -               dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
> -
> -       pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
> -
> -       iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
> -                                                    NULL);
> -       iosf_mbi_punit_release();
> -}
> -
>  static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  {
> -       u32 addr;
> -       u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
> -       int ret;
> -       unsigned long start, end;
> -
> -       might_sleep();
> -
> -       if (!dev || !dev->dev)
> -               return -ENODEV;
> -
> -       if (!dev->release_lock)
> -               return 0;
> -
> -       iosf_mbi_punit_acquire();
> -       iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
> -                                                    NULL);
> -
> -       /*
> -        * 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);
> -
> -       addr = get_sem_addr(dev);
> -
> -       /* host driver writes to side band semaphore register */
> -       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");
> -               goto out;
> -       }
> -
> -       /* host driver waits for bit 0 to be set in semaphore register */
> -       start = jiffies;
> -       end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
> -       do {
> -               ret = get_sem(dev, &sem);
> -               if (!ret && sem) {
> -                       acquired = jiffies;
> -                       dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
> -                               jiffies_to_msecs(jiffies - start));
> -                       return 0;
> -               }
> -
> -               usleep_range(1000, 2000);
> -       } while (time_before(jiffies, end));
> -
> -       dev_err(dev->dev, "punit semaphore timed out, resetting\n");
> -out:
> -       reset_semaphore(dev);
> -
> -       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
> -               dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
> -
> -       WARN_ON(1);
> -
> -       return -ETIMEDOUT;
> +       return iosf_mbi_block_punit_i2c_access();
>  }
>
>  static void baytrail_i2c_release(struct dw_i2c_dev *dev)
>  {
> -       if (!dev || !dev->dev)
> -               return;
> -
> -       if (!dev->acquire_lock)
> -               return;
> -
> -       reset_semaphore(dev);
> -       dev_dbg(dev->dev, "punit semaphore held for %ums\n",
> -               jiffies_to_msecs(jiffies - acquired));
> +       iosf_mbi_unblock_punit_i2c_access();
>  }
>
>  int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> @@ -166,14 +50,9 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>         dev->release_lock = baytrail_i2c_release;
>         dev->shared_with_punit = true;
>
> -       pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
> -                          PM_QOS_DEFAULT_VALUE);
> -
>         return 0;
>  }
>
>  void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>  {
> -       if (dev->acquire_lock)
> -               pm_qos_remove_request(&dev->pm_qos);
>  }
> --
> 2.19.0.rc1
>

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Andy Shevchenko]

On Thu, Oct 18, 2018 at 10:33 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
> > kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
> > block it from accessing the shared bus while the kernel wants to access it.
> >
> > Currently we have the I2C-controller driver acquiring and releasing the
> > semaphore around each I2C transfer. There are 2 problems with this:
> >
> > 1) PMIC accesses often come in the form of a read-modify-write on one of
> > the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
> > between the read and the write. If the P-Unit modifies the register during
> > this window?, then we end up overwriting the P-Unit's changes.
> > I believe that this is mostly an academic problem, but I'm not sure.
> >
> > 2) To safely access the shared I2C bus, we need to do 3 things:
> > a) Notify the GPU driver that we are starting a window in which it may not
> > access the P-Unit, since the P-Unit seems to ignore the semaphore for
> > explicit power-level requests made by the GPU driver
> > b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
> > C6/C7 while we hold the semaphore hangs the SoC
> > c) Finally take the P-Unit's PMIC bus semaphore
> > All 3 these steps together are somewhat expensive, so ideally if we have
> > a bunch of i2c transfers grouped together we only do this once for the
> > entire group.
> >
> > Taking the read-modify-write on a PMIC register as example then ideally we
> > would only do all 3 steps once at the beginning and undo all 3 steps once
> > at the end.
> >
> > For this we need to be able to take the semaphore from within e.g. the PMIC
> > opregion driver, yet we do not want to remove the taking of the semaphore
> > from the I2C-controller driver, as that is still necessary to protect many
> > other code-paths leading to accessing the shared I2C bus.
> >
> > This means that we first have the PMIC driver acquire the semaphore and
> > then have the I2C controller driver trying to acquire it again.
> >
> > To make this possible this commit does the following:
> >
> > 1) Move the semaphore code from being private to the I2C controller driver
> > into the generic iosf_mbi code, which already has other code to deal with
> > the shared bus so that it can be accessed outside of the I2C bus driver.
> >
> > 2) Rework the code so that it can be called multiple times nested, while
> > still blocking I2C accesses while e.g. the GPU driver has indicated the
> > P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> If there are no objections or concerns regarding this patch, I'm
> inclined to take the entire series including it.

Please, go ahead, it looks good to me.
Thanks!

>
> > ---
> > Note this commit deliberately limits the i2c-designware changes to
> > only touch i2c-designware-baytrail.c, deliberately not doing some cleanups
> > which become possible after removing the semaphore code from the
> > i2c-designmware code. This is done so that this commit can be merged
> > through the x86 tree without causing conflicts in the i2c tree.
> >
> > The cleanups to the i2c-designware tree will be done in a follow up
> > patch which can be merged once this commit is in place.
> > ---
> >  arch/x86/include/asm/iosf_mbi.h              |  39 +++--
> >  arch/x86/platform/intel/iosf_mbi.c           | 161 +++++++++++++++++--
> >  drivers/i2c/busses/i2c-designware-baytrail.c | 125 +-------------
> >  3 files changed, 180 insertions(+), 145 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
> > index 3de0489deade..5270ff39b9af 100644
> > --- a/arch/x86/include/asm/iosf_mbi.h
> > +++ b/arch/x86/include/asm/iosf_mbi.h
> > @@ -105,8 +105,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
> >   * the PMIC bus while another driver is also accessing the PMIC bus various bad
> >   * things happen.
> >   *
> > - * To avoid these problems this function must be called before accessing the
> > - * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means.
> > + * Call this function before sending requests to the P-Unit which may make it
> > + * access the PMIC, be it through iosf_mbi* functions or through other means.
> > + * This function will block all kernel access to the PMIC I2C bus, so that the
> > + * P-Unit can safely access the PMIC over the shared I2C bus.
> >   *
> >   * Note on these systems the i2c-bus driver will request a sempahore from the
> >   * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing
> > @@ -122,6 +124,31 @@ void iosf_mbi_punit_acquire(void);
> >   */
> >  void iosf_mbi_punit_release(void);
> >
> > +/**
> > + * iosf_mbi_block_punit_i2c_access() - Block P-Unit accesses to the PMIC bus
> > + *
> > + * Call this function to block P-Unit access to the PMIC I2C bus, so that the
> > + * kernel can safely access the PMIC over the shared I2C bus.
> > + *
> > + * This function acquires the P-Unit bus semaphore and notifies
> > + * pmic_bus_access_notifier listeners that they may no longer access the
> > + * P-Unit in a way which may cause it to access the shared I2C bus.
> > + *
> > + * Note this function may be called multiple times and the bus will not
> > + * be released until iosf_mbi_unblock_punit_i2c_access() has been called the
> > + * same amount of times.
> > + *
> > + * Return: Nonzero on error
> > + */
> > +int iosf_mbi_block_punit_i2c_access(void);
> > +
> > +/*
> > + * iosf_mbi_unblock_punit_i2c_access() - Release PMIC I2C bus block
> > + *
> > + * Release i2c access block gotten through iosf_mbi_block_punit_i2c_access().
> > + */
> > +void iosf_mbi_unblock_punit_i2c_access(void);
> > +
> >  /**
> >   * iosf_mbi_register_pmic_bus_access_notifier - Register PMIC bus notifier
> >   *
> > @@ -158,14 +185,6 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
> >  int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> >         struct notifier_block *nb);
> >
> > -/**
> > - * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain
> > - *
> > - * @val: action to pass into listener's notifier_call function
> > - * @v: data pointer to pass into listener's notifier_call function
> > - */
> > -int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
> > -
> >  /**
> >   * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired.
> >   */
> > diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
> > index 6f37a2137a79..5623a315f004 100644
> > --- a/arch/x86/platform/intel/iosf_mbi.c
> > +++ b/arch/x86/platform/intel/iosf_mbi.c
> > @@ -18,12 +18,14 @@
> >   * enumerate the device using PCI.
> >   */
> >
> > +#include <linux/delay.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/pci.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/capability.h>
> > +#include <linux/pm_qos.h>
> >
> >  #include <asm/iosf_mbi.h>
> >
> > @@ -34,8 +36,8 @@
> >
> >  static struct pci_dev *mbi_pdev;
> >  static DEFINE_SPINLOCK(iosf_mbi_lock);
> > -static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> > -static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
> > +
> > +/**************** Generic iosf_mbi access helpers ****************/
> >
> >  static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
> >  {
> > @@ -192,6 +194,30 @@ bool iosf_mbi_available(void)
> >  }
> >  EXPORT_SYMBOL(iosf_mbi_available);
> >
> > +/*
> > + **************** PUNIT/kernel shared i2c bus arbritration ****************
> > + *
> > + * Some Bay Trail and Cherry Trail devices have the PUNIT and us (the kernel)
> > + * share a single I2C bus to the PMIC. Below are helpers to arbitrate the
> > + * accesses between the kernel and the PUNIT.
> > + *
> > + * See arch/x86/include/asm/iosf_mbi.h for kernel-doc text for each function.
> > + */
> > +
> > +#define SEMAPHORE_TIMEOUT              500
> > +#define PUNIT_SEMAPHORE_BYT            0x7
> > +#define PUNIT_SEMAPHORE_CHT            0x10e
> > +#define PUNIT_SEMAPHORE_BIT            BIT(0)
> > +#define PUNIT_SEMAPHORE_ACQUIRE                BIT(1)
> > +
> > +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> > +static DEFINE_MUTEX(iosf_mbi_block_punit_i2c_access_count_mutex);
> > +static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
> > +static u32 iosf_mbi_block_punit_i2c_access_count;
> > +static u32 iosf_mbi_sem_address;
> > +static unsigned long iosf_mbi_sem_acquired;
> > +static struct pm_qos_request iosf_mbi_pm_qos;
> > +
> >  void iosf_mbi_punit_acquire(void)
> >  {
> >         mutex_lock(&iosf_mbi_punit_mutex);
> > @@ -204,6 +230,113 @@ void iosf_mbi_punit_release(void)
> >  }
> >  EXPORT_SYMBOL(iosf_mbi_punit_release);
> >
> > +static int iosf_mbi_get_sem(u32 *sem)
> > +{
> > +       int ret;
> > +
> > +       ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > +                           iosf_mbi_sem_address, sem);
> > +       if (ret) {
> > +               dev_err(&mbi_pdev->dev, "Error punit semaphore read failed\n");
> > +               return ret;
> > +       }
> > +
> > +       *sem &= PUNIT_SEMAPHORE_BIT;
> > +       return 0;
> > +}
> > +
> > +static void iosf_mbi_reset_semaphore(void)
> > +{
> > +       if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > +                           iosf_mbi_sem_address, 0, PUNIT_SEMAPHORE_BIT))
> > +               dev_err(&mbi_pdev->dev, "Error punit semaphore reset failed\n");
> > +
> > +       pm_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
> > +
> > +       blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
> > +                                    MBI_PMIC_BUS_ACCESS_END, NULL);
> > +       mutex_unlock(&iosf_mbi_punit_mutex);
> > +}
> > +
> > +int iosf_mbi_block_punit_i2c_access(void)
> > +{
> > +       unsigned long start, end;
> > +       int ret = 0;
> > +       u32 sem;
> > +
> > +       if (WARN_ON(!mbi_pdev || !iosf_mbi_sem_address))
> > +               return -ENXIO;
> > +
> > +       mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> > +
> > +       if (iosf_mbi_block_punit_i2c_access_count > 0)
> > +               goto out;
> > +
> > +       mutex_lock(&iosf_mbi_punit_mutex);
> > +       blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
> > +                                    MBI_PMIC_BUS_ACCESS_BEGIN, NULL);
> > +
> > +       /*
> > +        * 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(&iosf_mbi_pm_qos, 0);
> > +
> > +       /* host driver writes to side band semaphore register */
> > +       ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> > +                            iosf_mbi_sem_address, PUNIT_SEMAPHORE_ACQUIRE);
> > +       if (ret) {
> > +               dev_err(&mbi_pdev->dev, "Error punit semaphore request failed\n");
> > +               goto out;
> > +       }
> > +
> > +       /* host driver waits for bit 0 to be set in semaphore register */
> > +       start = jiffies;
> > +       end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
> > +       do {
> > +               ret = iosf_mbi_get_sem(&sem);
> > +               if (!ret && sem) {
> > +                       iosf_mbi_sem_acquired = jiffies;
> > +                       dev_dbg(&mbi_pdev->dev, "punit semaphore acquired after %ums\n",
> > +                               jiffies_to_msecs(jiffies - start));
> > +                       goto out; /* Success, done. */
> > +               }
> > +
> > +               usleep_range(1000, 2000);
> > +       } while (time_before(jiffies, end));
> > +
> > +       ret = -ETIMEDOUT;
> > +       dev_err(&mbi_pdev->dev, "Error punit semaphore timed out, resetting\n");
> > +       iosf_mbi_reset_semaphore();
> > +
> > +       if (!iosf_mbi_get_sem(&sem))
> > +               dev_err(&mbi_pdev->dev, "PUNIT SEM: %d\n", sem);
> > +out:
> > +       if (!WARN_ON(ret))
> > +               iosf_mbi_block_punit_i2c_access_count++;
> > +
> > +       mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL(iosf_mbi_block_punit_i2c_access);
> > +
> > +void iosf_mbi_unblock_punit_i2c_access(void)
> > +{
> > +       mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> > +
> > +       iosf_mbi_block_punit_i2c_access_count--;
> > +       if (iosf_mbi_block_punit_i2c_access_count == 0) {
> > +               iosf_mbi_reset_semaphore();
> > +               dev_dbg(&mbi_pdev->dev, "punit semaphore held for %ums\n",
> > +                       jiffies_to_msecs(jiffies - iosf_mbi_sem_acquired));
> > +       }
> > +
> > +       mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
> > +}
> > +EXPORT_SYMBOL(iosf_mbi_unblock_punit_i2c_access);
> > +
> >  int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
> >  {
> >         int ret;
> > @@ -241,19 +374,14 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
> >  }
> >  EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
> >
> > -int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
> > -{
> > -       return blocking_notifier_call_chain(
> > -                               &iosf_mbi_pmic_bus_access_notifier, val, v);
> > -}
> > -EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
> > -
> >  void iosf_mbi_assert_punit_acquired(void)
> >  {
> >         WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex));
> >  }
> >  EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired);
> >
> > +/**************** iosf_mbi debug code ****************/
> > +
> >  #ifdef CONFIG_IOSF_MBI_DEBUG
> >  static u32     dbg_mdr;
> >  static u32     dbg_mcr;
> > @@ -338,7 +466,7 @@ static inline void iosf_debugfs_remove(void) { }
> >  #endif /* CONFIG_IOSF_MBI_DEBUG */
> >
> >  static int iosf_mbi_probe(struct pci_dev *pdev,
> > -                         const struct pci_device_id *unused)
> > +                         const struct pci_device_id *dev_id)
> >  {
> >         int ret;
> >
> > @@ -349,12 +477,16 @@ static int iosf_mbi_probe(struct pci_dev *pdev,
> >         }
> >
> >         mbi_pdev = pci_dev_get(pdev);
> > +       iosf_mbi_sem_address = dev_id->driver_data;
> > +
> >         return 0;
> >  }
> >
> >  static const struct pci_device_id iosf_mbi_pci_ids[] = {
> > -       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL) },
> > -       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL) },
> > +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL),
> > +         .driver_data = PUNIT_SEMAPHORE_BYT },
> > +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL),
> > +         .driver_data = PUNIT_SEMAPHORE_CHT },
> >         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
> >         { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
> >         { 0, },
> > @@ -371,6 +503,9 @@ static int __init iosf_mbi_init(void)
> >  {
> >         iosf_debugfs_init();
> >
> > +       pm_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_CPU_DMA_LATENCY,
> > +                          PM_QOS_DEFAULT_VALUE);
> > +
> >         return pci_register_driver(&iosf_mbi_pci_driver);
> >  }
> >
> > @@ -381,6 +516,8 @@ static void __exit iosf_mbi_exit(void)
> >         pci_unregister_driver(&iosf_mbi_pci_driver);
> >         pci_dev_put(mbi_pdev);
> >         mbi_pdev = NULL;
> > +
> > +       pm_qos_remove_request(&iosf_mbi_pm_qos);
> >  }
> >
> >  module_init(iosf_mbi_init);
> > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> > index 9ca1feaba98f..8c84fa0b0384 100644
> > --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> > @@ -3,139 +3,23 @@
> >   * Intel BayTrail PMIC I2C bus semaphore implementaion
> >   * Copyright (c) 2014, Intel Corporation.
> >   */
> > -#include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/acpi.h>
> >  #include <linux/i2c.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/pm_qos.h>
> >
> >  #include <asm/iosf_mbi.h>
> >
> >  #include "i2c-designware-core.h"
> >
> > -#define SEMAPHORE_TIMEOUT      500
> > -#define PUNIT_SEMAPHORE                0x7
> > -#define PUNIT_SEMAPHORE_CHT    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_CHT;
> > -       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;
> > -
> > -       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;
> > -       }
> > -
> > -       *sem = data & PUNIT_SEMAPHORE_BIT;
> > -
> > -       return 0;
> > -}
> > -
> > -static void reset_semaphore(struct dw_i2c_dev *dev)
> > -{
> > -       if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
> > -                           0, PUNIT_SEMAPHORE_BIT))
> > -               dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
> > -
> > -       pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
> > -
> > -       iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
> > -                                                    NULL);
> > -       iosf_mbi_punit_release();
> > -}
> > -
> >  static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
> >  {
> > -       u32 addr;
> > -       u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
> > -       int ret;
> > -       unsigned long start, end;
> > -
> > -       might_sleep();
> > -
> > -       if (!dev || !dev->dev)
> > -               return -ENODEV;
> > -
> > -       if (!dev->release_lock)
> > -               return 0;
> > -
> > -       iosf_mbi_punit_acquire();
> > -       iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
> > -                                                    NULL);
> > -
> > -       /*
> > -        * 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);
> > -
> > -       addr = get_sem_addr(dev);
> > -
> > -       /* host driver writes to side band semaphore register */
> > -       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");
> > -               goto out;
> > -       }
> > -
> > -       /* host driver waits for bit 0 to be set in semaphore register */
> > -       start = jiffies;
> > -       end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
> > -       do {
> > -               ret = get_sem(dev, &sem);
> > -               if (!ret && sem) {
> > -                       acquired = jiffies;
> > -                       dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
> > -                               jiffies_to_msecs(jiffies - start));
> > -                       return 0;
> > -               }
> > -
> > -               usleep_range(1000, 2000);
> > -       } while (time_before(jiffies, end));
> > -
> > -       dev_err(dev->dev, "punit semaphore timed out, resetting\n");
> > -out:
> > -       reset_semaphore(dev);
> > -
> > -       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
> > -               dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
> > -
> > -       WARN_ON(1);
> > -
> > -       return -ETIMEDOUT;
> > +       return iosf_mbi_block_punit_i2c_access();
> >  }
> >
> >  static void baytrail_i2c_release(struct dw_i2c_dev *dev)
> >  {
> > -       if (!dev || !dev->dev)
> > -               return;
> > -
> > -       if (!dev->acquire_lock)
> > -               return;
> > -
> > -       reset_semaphore(dev);
> > -       dev_dbg(dev->dev, "punit semaphore held for %ums\n",
> > -               jiffies_to_msecs(jiffies - acquired));
> > +       iosf_mbi_unblock_punit_i2c_access();
> >  }
> >
> >  int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> > @@ -166,14 +50,9 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
> >         dev->release_lock = baytrail_i2c_release;
> >         dev->shared_with_punit = true;
> >
> > -       pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
> > -                          PM_QOS_DEFAULT_VALUE);
> > -
> >         return 0;
> >  }
> >
> >  void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
> >  {
> > -       if (dev->acquire_lock)
> > -               pm_qos_remove_request(&dev->pm_qos);
> >  }
> > --
> > 2.19.0.rc1
> >



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Jarkko Nikula]

On 10/18/2018 10:36 AM, Andy Shevchenko wrote:
> On Thu, Oct 18, 2018 at 10:33 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
>>> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
>>> block it from accessing the shared bus while the kernel wants to access it.
>>>
>>> Currently we have the I2C-controller driver acquiring and releasing the
>>> semaphore around each I2C transfer. There are 2 problems with this:
>>>
>>> 1) PMIC accesses often come in the form of a read-modify-write on one of
>>> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
>>> between the read and the write. If the P-Unit modifies the register during
>>> this window?, then we end up overwriting the P-Unit's changes.
>>> I believe that this is mostly an academic problem, but I'm not sure.
>>>
>>> 2) To safely access the shared I2C bus, we need to do 3 things:
>>> a) Notify the GPU driver that we are starting a window in which it may not
>>> access the P-Unit, since the P-Unit seems to ignore the semaphore for
>>> explicit power-level requests made by the GPU driver
>>> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
>>> C6/C7 while we hold the semaphore hangs the SoC
>>> c) Finally take the P-Unit's PMIC bus semaphore
>>> All 3 these steps together are somewhat expensive, so ideally if we have
>>> a bunch of i2c transfers grouped together we only do this once for the
>>> entire group.
>>>
>>> Taking the read-modify-write on a PMIC register as example then ideally we
>>> would only do all 3 steps once at the beginning and undo all 3 steps once
>>> at the end.
>>>
>>> For this we need to be able to take the semaphore from within e.g. the PMIC
>>> opregion driver, yet we do not want to remove the taking of the semaphore
>>> from the I2C-controller driver, as that is still necessary to protect many
>>> other code-paths leading to accessing the shared I2C bus.
>>>
>>> This means that we first have the PMIC driver acquire the semaphore and
>>> then have the I2C controller driver trying to acquire it again.
>>>
>>> To make this possible this commit does the following:
>>>
>>> 1) Move the semaphore code from being private to the I2C controller driver
>>> into the generic iosf_mbi code, which already has other code to deal with
>>> the shared bus so that it can be accessed outside of the I2C bus driver.
>>>
>>> 2) Rework the code so that it can be called multiple times nested, while
>>> still blocking I2C accesses while e.g. the GPU driver has indicated the
>>> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> If there are no objections or concerns regarding this patch, I'm
>> inclined to take the entire series including it.
> 
> Please, go ahead, it looks good to me.
> Thanks!
> 
Just in case note: please remember to take patches and tags from v3 of 
the series, not from this v1 thread.

-- 
Jarkko

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Hans de Goede]

Hi,

On 18-10-18 09:29, Rafael J. Wysocki wrote:
> On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
>> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
>> block it from accessing the shared bus while the kernel wants to access it.
>>
>> Currently we have the I2C-controller driver acquiring and releasing the
>> semaphore around each I2C transfer. There are 2 problems with this:
>>
>> 1) PMIC accesses often come in the form of a read-modify-write on one of
>> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
>> between the read and the write. If the P-Unit modifies the register during
>> this window?, then we end up overwriting the P-Unit's changes.
>> I believe that this is mostly an academic problem, but I'm not sure.
>>
>> 2) To safely access the shared I2C bus, we need to do 3 things:
>> a) Notify the GPU driver that we are starting a window in which it may not
>> access the P-Unit, since the P-Unit seems to ignore the semaphore for
>> explicit power-level requests made by the GPU driver
>> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
>> C6/C7 while we hold the semaphore hangs the SoC
>> c) Finally take the P-Unit's PMIC bus semaphore
>> All 3 these steps together are somewhat expensive, so ideally if we have
>> a bunch of i2c transfers grouped together we only do this once for the
>> entire group.
>>
>> Taking the read-modify-write on a PMIC register as example then ideally we
>> would only do all 3 steps once at the beginning and undo all 3 steps once
>> at the end.
>>
>> For this we need to be able to take the semaphore from within e.g. the PMIC
>> opregion driver, yet we do not want to remove the taking of the semaphore
>> from the I2C-controller driver, as that is still necessary to protect many
>> other code-paths leading to accessing the shared I2C bus.
>>
>> This means that we first have the PMIC driver acquire the semaphore and
>> then have the I2C controller driver trying to acquire it again.
>>
>> To make this possible this commit does the following:
>>
>> 1) Move the semaphore code from being private to the I2C controller driver
>> into the generic iosf_mbi code, which already has other code to deal with
>> the shared bus so that it can be accessed outside of the I2C bus driver.
>>
>> 2) Rework the code so that it can be called multiple times nested, while
>> still blocking I2C accesses while e.g. the GPU driver has indicated the
>> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> If there are no objections or concerns regarding this patch, I'm
> inclined to take the entire series including it.

In that case let me send out a v4, with the following chunk added to the
2nd patch:

--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -515,7 +515,7 @@ config CRC_PMIC_OPREGION

  config XPOWER_PMIC_OPREGION
         bool "ACPI operation region support for XPower AXP288 PMIC"
-       depends on MFD_AXP20X_I2C
+       depends on MFD_AXP20X_I2C && IOSF_MBI
         help
           This config adds ACPI operation region support for XPower AXP288 PMIC.

This is necessary to avoid compilation issues on non x86 systems (where the
asm/iosf_mbi.h header is not available) and on x86 systems in case
IOSF_MBI support is not enabled there.  Note that the AXP288 PMIC is
connected through the LPSS i2c controller, so either we have IOSF_MBI support
selected through the X86_INTEL_LPSS option, or we have a kernel where the
opregion will never work anyways.

Regards,

Hans






>> ---
>> Note this commit deliberately limits the i2c-designware changes to
>> only touch i2c-designware-baytrail.c, deliberately not doing some cleanups
>> which become possible after removing the semaphore code from the
>> i2c-designmware code. This is done so that this commit can be merged
>> through the x86 tree without causing conflicts in the i2c tree.
>>
>> The cleanups to the i2c-designware tree will be done in a follow up
>> patch which can be merged once this commit is in place.
>> ---
>>   arch/x86/include/asm/iosf_mbi.h              |  39 +++--
>>   arch/x86/platform/intel/iosf_mbi.c           | 161 +++++++++++++++++--
>>   drivers/i2c/busses/i2c-designware-baytrail.c | 125 +-------------
>>   3 files changed, 180 insertions(+), 145 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
>> index 3de0489deade..5270ff39b9af 100644
>> --- a/arch/x86/include/asm/iosf_mbi.h
>> +++ b/arch/x86/include/asm/iosf_mbi.h
>> @@ -105,8 +105,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
>>    * the PMIC bus while another driver is also accessing the PMIC bus various bad
>>    * things happen.
>>    *
>> - * To avoid these problems this function must be called before accessing the
>> - * P-Unit or the PMIC, be it through iosf_mbi* functions or through other means.
>> + * Call this function before sending requests to the P-Unit which may make it
>> + * access the PMIC, be it through iosf_mbi* functions or through other means.
>> + * This function will block all kernel access to the PMIC I2C bus, so that the
>> + * P-Unit can safely access the PMIC over the shared I2C bus.
>>    *
>>    * Note on these systems the i2c-bus driver will request a sempahore from the
>>    * P-Unit for exclusive access to the PMIC bus when i2c drivers are accessing
>> @@ -122,6 +124,31 @@ void iosf_mbi_punit_acquire(void);
>>    */
>>   void iosf_mbi_punit_release(void);
>>
>> +/**
>> + * iosf_mbi_block_punit_i2c_access() - Block P-Unit accesses to the PMIC bus
>> + *
>> + * Call this function to block P-Unit access to the PMIC I2C bus, so that the
>> + * kernel can safely access the PMIC over the shared I2C bus.
>> + *
>> + * This function acquires the P-Unit bus semaphore and notifies
>> + * pmic_bus_access_notifier listeners that they may no longer access the
>> + * P-Unit in a way which may cause it to access the shared I2C bus.
>> + *
>> + * Note this function may be called multiple times and the bus will not
>> + * be released until iosf_mbi_unblock_punit_i2c_access() has been called the
>> + * same amount of times.
>> + *
>> + * Return: Nonzero on error
>> + */
>> +int iosf_mbi_block_punit_i2c_access(void);
>> +
>> +/*
>> + * iosf_mbi_unblock_punit_i2c_access() - Release PMIC I2C bus block
>> + *
>> + * Release i2c access block gotten through iosf_mbi_block_punit_i2c_access().
>> + */
>> +void iosf_mbi_unblock_punit_i2c_access(void);
>> +
>>   /**
>>    * iosf_mbi_register_pmic_bus_access_notifier - Register PMIC bus notifier
>>    *
>> @@ -158,14 +185,6 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
>>   int iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
>>          struct notifier_block *nb);
>>
>> -/**
>> - * iosf_mbi_call_pmic_bus_access_notifier_chain - Call PMIC bus notifier chain
>> - *
>> - * @val: action to pass into listener's notifier_call function
>> - * @v: data pointer to pass into listener's notifier_call function
>> - */
>> -int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
>> -
>>   /**
>>    * iosf_mbi_assert_punit_acquired - Assert that the P-Unit has been acquired.
>>    */
>> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
>> index 6f37a2137a79..5623a315f004 100644
>> --- a/arch/x86/platform/intel/iosf_mbi.c
>> +++ b/arch/x86/platform/intel/iosf_mbi.c
>> @@ -18,12 +18,14 @@
>>    * enumerate the device using PCI.
>>    */
>>
>> +#include <linux/delay.h>
>>   #include <linux/module.h>
>>   #include <linux/init.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/pci.h>
>>   #include <linux/debugfs.h>
>>   #include <linux/capability.h>
>> +#include <linux/pm_qos.h>
>>
>>   #include <asm/iosf_mbi.h>
>>
>> @@ -34,8 +36,8 @@
>>
>>   static struct pci_dev *mbi_pdev;
>>   static DEFINE_SPINLOCK(iosf_mbi_lock);
>> -static DEFINE_MUTEX(iosf_mbi_punit_mutex);
>> -static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
>> +
>> +/**************** Generic iosf_mbi access helpers ****************/
>>
>>   static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
>>   {
>> @@ -192,6 +194,30 @@ bool iosf_mbi_available(void)
>>   }
>>   EXPORT_SYMBOL(iosf_mbi_available);
>>
>> +/*
>> + **************** PUNIT/kernel shared i2c bus arbritration ****************
>> + *
>> + * Some Bay Trail and Cherry Trail devices have the PUNIT and us (the kernel)
>> + * share a single I2C bus to the PMIC. Below are helpers to arbitrate the
>> + * accesses between the kernel and the PUNIT.
>> + *
>> + * See arch/x86/include/asm/iosf_mbi.h for kernel-doc text for each function.
>> + */
>> +
>> +#define SEMAPHORE_TIMEOUT              500
>> +#define PUNIT_SEMAPHORE_BYT            0x7
>> +#define PUNIT_SEMAPHORE_CHT            0x10e
>> +#define PUNIT_SEMAPHORE_BIT            BIT(0)
>> +#define PUNIT_SEMAPHORE_ACQUIRE                BIT(1)
>> +
>> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
>> +static DEFINE_MUTEX(iosf_mbi_block_punit_i2c_access_count_mutex);
>> +static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
>> +static u32 iosf_mbi_block_punit_i2c_access_count;
>> +static u32 iosf_mbi_sem_address;
>> +static unsigned long iosf_mbi_sem_acquired;
>> +static struct pm_qos_request iosf_mbi_pm_qos;
>> +
>>   void iosf_mbi_punit_acquire(void)
>>   {
>>          mutex_lock(&iosf_mbi_punit_mutex);
>> @@ -204,6 +230,113 @@ void iosf_mbi_punit_release(void)
>>   }
>>   EXPORT_SYMBOL(iosf_mbi_punit_release);
>>
>> +static int iosf_mbi_get_sem(u32 *sem)
>> +{
>> +       int ret;
>> +
>> +       ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> +                           iosf_mbi_sem_address, sem);
>> +       if (ret) {
>> +               dev_err(&mbi_pdev->dev, "Error punit semaphore read failed\n");
>> +               return ret;
>> +       }
>> +
>> +       *sem &= PUNIT_SEMAPHORE_BIT;
>> +       return 0;
>> +}
>> +
>> +static void iosf_mbi_reset_semaphore(void)
>> +{
>> +       if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> +                           iosf_mbi_sem_address, 0, PUNIT_SEMAPHORE_BIT))
>> +               dev_err(&mbi_pdev->dev, "Error punit semaphore reset failed\n");
>> +
>> +       pm_qos_update_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
>> +
>> +       blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
>> +                                    MBI_PMIC_BUS_ACCESS_END, NULL);
>> +       mutex_unlock(&iosf_mbi_punit_mutex);
>> +}
>> +
>> +int iosf_mbi_block_punit_i2c_access(void)
>> +{
>> +       unsigned long start, end;
>> +       int ret = 0;
>> +       u32 sem;
>> +
>> +       if (WARN_ON(!mbi_pdev || !iosf_mbi_sem_address))
>> +               return -ENXIO;
>> +
>> +       mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +
>> +       if (iosf_mbi_block_punit_i2c_access_count > 0)
>> +               goto out;
>> +
>> +       mutex_lock(&iosf_mbi_punit_mutex);
>> +       blocking_notifier_call_chain(&iosf_mbi_pmic_bus_access_notifier,
>> +                                    MBI_PMIC_BUS_ACCESS_BEGIN, NULL);
>> +
>> +       /*
>> +        * 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(&iosf_mbi_pm_qos, 0);
>> +
>> +       /* host driver writes to side band semaphore register */
>> +       ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>> +                            iosf_mbi_sem_address, PUNIT_SEMAPHORE_ACQUIRE);
>> +       if (ret) {
>> +               dev_err(&mbi_pdev->dev, "Error punit semaphore request failed\n");
>> +               goto out;
>> +       }
>> +
>> +       /* host driver waits for bit 0 to be set in semaphore register */
>> +       start = jiffies;
>> +       end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
>> +       do {
>> +               ret = iosf_mbi_get_sem(&sem);
>> +               if (!ret && sem) {
>> +                       iosf_mbi_sem_acquired = jiffies;
>> +                       dev_dbg(&mbi_pdev->dev, "punit semaphore acquired after %ums\n",
>> +                               jiffies_to_msecs(jiffies - start));
>> +                       goto out; /* Success, done. */
>> +               }
>> +
>> +               usleep_range(1000, 2000);
>> +       } while (time_before(jiffies, end));
>> +
>> +       ret = -ETIMEDOUT;
>> +       dev_err(&mbi_pdev->dev, "Error punit semaphore timed out, resetting\n");
>> +       iosf_mbi_reset_semaphore();
>> +
>> +       if (!iosf_mbi_get_sem(&sem))
>> +               dev_err(&mbi_pdev->dev, "PUNIT SEM: %d\n", sem);
>> +out:
>> +       if (!WARN_ON(ret))
>> +               iosf_mbi_block_punit_i2c_access_count++;
>> +
>> +       mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_block_punit_i2c_access);
>> +
>> +void iosf_mbi_unblock_punit_i2c_access(void)
>> +{
>> +       mutex_lock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +
>> +       iosf_mbi_block_punit_i2c_access_count--;
>> +       if (iosf_mbi_block_punit_i2c_access_count == 0) {
>> +               iosf_mbi_reset_semaphore();
>> +               dev_dbg(&mbi_pdev->dev, "punit semaphore held for %ums\n",
>> +                       jiffies_to_msecs(jiffies - iosf_mbi_sem_acquired));
>> +       }
>> +
>> +       mutex_unlock(&iosf_mbi_block_punit_i2c_access_count_mutex);
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_unblock_punit_i2c_access);
>> +
>>   int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
>>   {
>>          int ret;
>> @@ -241,19 +374,14 @@ int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
>>   }
>>   EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
>>
>> -int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
>> -{
>> -       return blocking_notifier_call_chain(
>> -                               &iosf_mbi_pmic_bus_access_notifier, val, v);
>> -}
>> -EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
>> -
>>   void iosf_mbi_assert_punit_acquired(void)
>>   {
>>          WARN_ON(!mutex_is_locked(&iosf_mbi_punit_mutex));
>>   }
>>   EXPORT_SYMBOL(iosf_mbi_assert_punit_acquired);
>>
>> +/**************** iosf_mbi debug code ****************/
>> +
>>   #ifdef CONFIG_IOSF_MBI_DEBUG
>>   static u32     dbg_mdr;
>>   static u32     dbg_mcr;
>> @@ -338,7 +466,7 @@ static inline void iosf_debugfs_remove(void) { }
>>   #endif /* CONFIG_IOSF_MBI_DEBUG */
>>
>>   static int iosf_mbi_probe(struct pci_dev *pdev,
>> -                         const struct pci_device_id *unused)
>> +                         const struct pci_device_id *dev_id)
>>   {
>>          int ret;
>>
>> @@ -349,12 +477,16 @@ static int iosf_mbi_probe(struct pci_dev *pdev,
>>          }
>>
>>          mbi_pdev = pci_dev_get(pdev);
>> +       iosf_mbi_sem_address = dev_id->driver_data;
>> +
>>          return 0;
>>   }
>>
>>   static const struct pci_device_id iosf_mbi_pci_ids[] = {
>> -       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL) },
>> -       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL) },
>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BAYTRAIL),
>> +         .driver_data = PUNIT_SEMAPHORE_BYT },
>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_BRASWELL),
>> +         .driver_data = PUNIT_SEMAPHORE_CHT },
>>          { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_QUARK_X1000) },
>>          { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_TANGIER) },
>>          { 0, },
>> @@ -371,6 +503,9 @@ static int __init iosf_mbi_init(void)
>>   {
>>          iosf_debugfs_init();
>>
>> +       pm_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_CPU_DMA_LATENCY,
>> +                          PM_QOS_DEFAULT_VALUE);
>> +
>>          return pci_register_driver(&iosf_mbi_pci_driver);
>>   }
>>
>> @@ -381,6 +516,8 @@ static void __exit iosf_mbi_exit(void)
>>          pci_unregister_driver(&iosf_mbi_pci_driver);
>>          pci_dev_put(mbi_pdev);
>>          mbi_pdev = NULL;
>> +
>> +       pm_qos_remove_request(&iosf_mbi_pm_qos);
>>   }
>>
>>   module_init(iosf_mbi_init);
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index 9ca1feaba98f..8c84fa0b0384 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>> @@ -3,139 +3,23 @@
>>    * Intel BayTrail PMIC I2C bus semaphore implementaion
>>    * Copyright (c) 2014, Intel Corporation.
>>    */
>> -#include <linux/delay.h>
>>   #include <linux/device.h>
>>   #include <linux/acpi.h>
>>   #include <linux/i2c.h>
>>   #include <linux/interrupt.h>
>> -#include <linux/pm_qos.h>
>>
>>   #include <asm/iosf_mbi.h>
>>
>>   #include "i2c-designware-core.h"
>>
>> -#define SEMAPHORE_TIMEOUT      500
>> -#define PUNIT_SEMAPHORE                0x7
>> -#define PUNIT_SEMAPHORE_CHT    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_CHT;
>> -       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;
>> -
>> -       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;
>> -       }
>> -
>> -       *sem = data & PUNIT_SEMAPHORE_BIT;
>> -
>> -       return 0;
>> -}
>> -
>> -static void reset_semaphore(struct dw_i2c_dev *dev)
>> -{
>> -       if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
>> -                           0, PUNIT_SEMAPHORE_BIT))
>> -               dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
>> -
>> -       pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>> -
>> -       iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
>> -                                                    NULL);
>> -       iosf_mbi_punit_release();
>> -}
>> -
>>   static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>>   {
>> -       u32 addr;
>> -       u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
>> -       int ret;
>> -       unsigned long start, end;
>> -
>> -       might_sleep();
>> -
>> -       if (!dev || !dev->dev)
>> -               return -ENODEV;
>> -
>> -       if (!dev->release_lock)
>> -               return 0;
>> -
>> -       iosf_mbi_punit_acquire();
>> -       iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
>> -                                                    NULL);
>> -
>> -       /*
>> -        * 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);
>> -
>> -       addr = get_sem_addr(dev);
>> -
>> -       /* host driver writes to side band semaphore register */
>> -       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");
>> -               goto out;
>> -       }
>> -
>> -       /* host driver waits for bit 0 to be set in semaphore register */
>> -       start = jiffies;
>> -       end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
>> -       do {
>> -               ret = get_sem(dev, &sem);
>> -               if (!ret && sem) {
>> -                       acquired = jiffies;
>> -                       dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
>> -                               jiffies_to_msecs(jiffies - start));
>> -                       return 0;
>> -               }
>> -
>> -               usleep_range(1000, 2000);
>> -       } while (time_before(jiffies, end));
>> -
>> -       dev_err(dev->dev, "punit semaphore timed out, resetting\n");
>> -out:
>> -       reset_semaphore(dev);
>> -
>> -       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
>> -               dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
>> -
>> -       WARN_ON(1);
>> -
>> -       return -ETIMEDOUT;
>> +       return iosf_mbi_block_punit_i2c_access();
>>   }
>>
>>   static void baytrail_i2c_release(struct dw_i2c_dev *dev)
>>   {
>> -       if (!dev || !dev->dev)
>> -               return;
>> -
>> -       if (!dev->acquire_lock)
>> -               return;
>> -
>> -       reset_semaphore(dev);
>> -       dev_dbg(dev->dev, "punit semaphore held for %ums\n",
>> -               jiffies_to_msecs(jiffies - acquired));
>> +       iosf_mbi_unblock_punit_i2c_access();
>>   }
>>
>>   int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>> @@ -166,14 +50,9 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>>          dev->release_lock = baytrail_i2c_release;
>>          dev->shared_with_punit = true;
>>
>> -       pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
>> -                          PM_QOS_DEFAULT_VALUE);
>> -
>>          return 0;
>>   }
>>
>>   void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>>   {
>> -       if (dev->acquire_lock)
>> -               pm_qos_remove_request(&dev->pm_qos);
>>   }
>> --
>> 2.19.0.rc1
>>

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Rafael J. Wysocki]

On Thursday, October 18, 2018 10:34:57 AM CEST Hans de Goede wrote:
> Hi,
> 
> On 18-10-18 09:29, Rafael J. Wysocki wrote:
> > On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
> >> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
> >> block it from accessing the shared bus while the kernel wants to access it.
> >>
> >> Currently we have the I2C-controller driver acquiring and releasing the
> >> semaphore around each I2C transfer. There are 2 problems with this:
> >>
> >> 1) PMIC accesses often come in the form of a read-modify-write on one of
> >> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
> >> between the read and the write. If the P-Unit modifies the register during
> >> this window?, then we end up overwriting the P-Unit's changes.
> >> I believe that this is mostly an academic problem, but I'm not sure.
> >>
> >> 2) To safely access the shared I2C bus, we need to do 3 things:
> >> a) Notify the GPU driver that we are starting a window in which it may not
> >> access the P-Unit, since the P-Unit seems to ignore the semaphore for
> >> explicit power-level requests made by the GPU driver
> >> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
> >> C6/C7 while we hold the semaphore hangs the SoC
> >> c) Finally take the P-Unit's PMIC bus semaphore
> >> All 3 these steps together are somewhat expensive, so ideally if we have
> >> a bunch of i2c transfers grouped together we only do this once for the
> >> entire group.
> >>
> >> Taking the read-modify-write on a PMIC register as example then ideally we
> >> would only do all 3 steps once at the beginning and undo all 3 steps once
> >> at the end.
> >>
> >> For this we need to be able to take the semaphore from within e.g. the PMIC
> >> opregion driver, yet we do not want to remove the taking of the semaphore
> >> from the I2C-controller driver, as that is still necessary to protect many
> >> other code-paths leading to accessing the shared I2C bus.
> >>
> >> This means that we first have the PMIC driver acquire the semaphore and
> >> then have the I2C controller driver trying to acquire it again.
> >>
> >> To make this possible this commit does the following:
> >>
> >> 1) Move the semaphore code from being private to the I2C controller driver
> >> into the generic iosf_mbi code, which already has other code to deal with
> >> the shared bus so that it can be accessed outside of the I2C bus driver.
> >>
> >> 2) Rework the code so that it can be called multiple times nested, while
> >> still blocking I2C accesses while e.g. the GPU driver has indicated the
> >> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > If there are no objections or concerns regarding this patch, I'm
> > inclined to take the entire series including it.
> 
> In that case let me send out a v4, with the following chunk added to the
> 2nd patch:
> 
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -515,7 +515,7 @@ config CRC_PMIC_OPREGION
> 
>   config XPOWER_PMIC_OPREGION
>          bool "ACPI operation region support for XPower AXP288 PMIC"
> -       depends on MFD_AXP20X_I2C
> +       depends on MFD_AXP20X_I2C && IOSF_MBI
>          help
>            This config adds ACPI operation region support for XPower AXP288 PMIC.
> 
> This is necessary to avoid compilation issues on non x86 systems (where the
> asm/iosf_mbi.h header is not available) and on x86 systems in case
> IOSF_MBI support is not enabled there.  Note that the AXP288 PMIC is
> connected through the LPSS i2c controller, so either we have IOSF_MBI support
> selected through the X86_INTEL_LPSS option, or we have a kernel where the
> opregion will never work anyways.

I'd prefer to get an incremental patch for that at this point.

Thanks,
Rafael

Re: [PATCH 1/3] x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code

[Hans de Goede]

HI,

On 18-10-18 10:38, Rafael J. Wysocki wrote:
> On Thursday, October 18, 2018 10:34:57 AM CEST Hans de Goede wrote:
>> Hi,
>>
>> On 18-10-18 09:29, Rafael J. Wysocki wrote:
>>> On Sun, Sep 23, 2018 at 4:45 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> On some BYT/CHT systems the SoC's P-Unit shares the I2C bus with the
>>>> kernel. The P-Unit has a semaphore for the PMIC bus which we can take to
>>>> block it from accessing the shared bus while the kernel wants to access it.
>>>>
>>>> Currently we have the I2C-controller driver acquiring and releasing the
>>>> semaphore around each I2C transfer. There are 2 problems with this:
>>>>
>>>> 1) PMIC accesses often come in the form of a read-modify-write on one of
>>>> the PMIC registers, we currently release the P-Unit's PMIC bus semaphore
>>>> between the read and the write. If the P-Unit modifies the register during
>>>> this window?, then we end up overwriting the P-Unit's changes.
>>>> I believe that this is mostly an academic problem, but I'm not sure.
>>>>
>>>> 2) To safely access the shared I2C bus, we need to do 3 things:
>>>> a) Notify the GPU driver that we are starting a window in which it may not
>>>> access the P-Unit, since the P-Unit seems to ignore the semaphore for
>>>> explicit power-level requests made by the GPU driver
>>>> b) Make a pm_qos request to force all CPU cores out of C6/C7 since entering
>>>> C6/C7 while we hold the semaphore hangs the SoC
>>>> c) Finally take the P-Unit's PMIC bus semaphore
>>>> All 3 these steps together are somewhat expensive, so ideally if we have
>>>> a bunch of i2c transfers grouped together we only do this once for the
>>>> entire group.
>>>>
>>>> Taking the read-modify-write on a PMIC register as example then ideally we
>>>> would only do all 3 steps once at the beginning and undo all 3 steps once
>>>> at the end.
>>>>
>>>> For this we need to be able to take the semaphore from within e.g. the PMIC
>>>> opregion driver, yet we do not want to remove the taking of the semaphore
>>>> from the I2C-controller driver, as that is still necessary to protect many
>>>> other code-paths leading to accessing the shared I2C bus.
>>>>
>>>> This means that we first have the PMIC driver acquire the semaphore and
>>>> then have the I2C controller driver trying to acquire it again.
>>>>
>>>> To make this possible this commit does the following:
>>>>
>>>> 1) Move the semaphore code from being private to the I2C controller driver
>>>> into the generic iosf_mbi code, which already has other code to deal with
>>>> the shared bus so that it can be accessed outside of the I2C bus driver.
>>>>
>>>> 2) Rework the code so that it can be called multiple times nested, while
>>>> still blocking I2C accesses while e.g. the GPU driver has indicated the
>>>> P-Unit needs the bus through a iosf_mbi_punit_acquire() call.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> If there are no objections or concerns regarding this patch, I'm
>>> inclined to take the entire series including it.
>>
>> In that case let me send out a v4, with the following chunk added to the
>> 2nd patch:
>>
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -515,7 +515,7 @@ config CRC_PMIC_OPREGION
>>
>>    config XPOWER_PMIC_OPREGION
>>           bool "ACPI operation region support for XPower AXP288 PMIC"
>> -       depends on MFD_AXP20X_I2C
>> +       depends on MFD_AXP20X_I2C && IOSF_MBI
>>           help
>>             This config adds ACPI operation region support for XPower AXP288 PMIC.
>>
>> This is necessary to avoid compilation issues on non x86 systems (where the
>> asm/iosf_mbi.h header is not available) and on x86 systems in case
>> IOSF_MBI support is not enabled there.  Note that the AXP288 PMIC is
>> connected through the LPSS i2c controller, so either we have IOSF_MBI support
>> selected through the X86_INTEL_LPSS option, or we have a kernel where the
>> opregion will never work anyways.
> 
> I'd prefer to get an incremental patch for that at this point.

Ok, then I will prepare and send out an incremental patch for that.

Regards,

Hans

[PATCH 2/3] ACPI / PMIC: xpower: Block P-Unit I2C access during read-modify-write

[Hans de Goede]

intel_xpower_pmic_update_power() does a read-modify-write of the output
control register. The i2c-designware code blocks the P-Unit I2C access
during the read and write by taking the P-Unit's PMIC bus semaphore.
But between the read and the write that semaphore is released and the
P-Unit could make changes to the register which we then end up overwriting.

This commit makes intel_xpower_pmic_update_power() take the semaphore
itself so that it is held over the entire read-modify-write, avoiding this
race.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic_xpower.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 316e55174aa9..9e6ddf2a2d4e 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -18,6 +18,7 @@
 #include <linux/mfd/axp20x.h>
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
+#include <asm/iosf_mbi.h>
 #include "intel_pmic.h"
 
 #define XPOWER_GPADC_LOW	0x5b
@@ -180,15 +181,21 @@ static int intel_xpower_pmic_get_power(struct regmap *regmap, int reg,
 static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
 					  int bit, bool on)
 {
-	int data;
+	int data, ret;
 
 	/* GPIO1 LDO regulator needs special handling */
 	if (reg == XPOWER_GPI1_CTRL)
 		return regmap_update_bits(regmap, reg, GPI1_LDO_MASK,
 					  on ? GPI1_LDO_ON : GPI1_LDO_OFF);
 
-	if (regmap_read(regmap, reg, &data))
-		return -EIO;
+	ret = iosf_mbi_block_punit_i2c_access();
+	if (ret)
+		return ret;
+
+	if (regmap_read(regmap, reg, &data)) {
+		ret = -EIO;
+		goto out;
+	}
 
 	if (on)
 		data |= BIT(bit);
@@ -196,9 +203,11 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
 		data &= ~BIT(bit);
 
 	if (regmap_write(regmap, reg, data))
-		return -EIO;
+		ret = -EIO;
+out:
+	iosf_mbi_unblock_punit_i2c_access();
 
-	return 0;
+	return ret;
 }
 
 /**
-- 
2.19.0.rc1

[PATCH 3/3] i2c: designware: Cleanup bus lock handling

[Hans de Goede]

Now that most of the special Bay- / Cherry-Trail bus lock handling has
been moved to the iosf_mbi code we can simplify the remaining code a bit.

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

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 8c84fa0b0384..33da07d64494 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -12,16 +12,6 @@
 
 #include "i2c-designware-core.h"
 
-static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
-{
-	return iosf_mbi_block_punit_i2c_access();
-}
-
-static void baytrail_i2c_release(struct dw_i2c_dev *dev)
-{
-	iosf_mbi_unblock_punit_i2c_access();
-}
-
 int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 {
 	acpi_status status;
@@ -46,13 +36,9 @@ int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
 		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->acquire_lock = iosf_mbi_block_punit_i2c_access;
+	dev->release_lock = iosf_mbi_unblock_punit_i2c_access;
 	dev->shared_with_punit = true;
 
 	return 0;
 }
-
-void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
-{
-}
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 69ec4a791f23..7d50f230cd37 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -267,7 +267,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 	if (!dev->acquire_lock)
 		return 0;
 
-	ret = dev->acquire_lock(dev);
+	ret = dev->acquire_lock();
 	if (!ret)
 		return 0;
 
@@ -279,7 +279,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 void i2c_dw_release_lock(struct dw_i2c_dev *dev)
 {
 	if (dev->release_lock)
-		dev->release_lock(dev);
+		dev->release_lock();
 }
 
 /*
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 75362e5dd4cd..31e0c84ab4e7 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -10,7 +10,6 @@
  */
 
 #include <linux/i2c.h>
-#include <linux/pm_qos.h>
 
 #define DW_IC_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |			\
 					I2C_FUNC_SMBUS_BYTE |		\
@@ -209,7 +208,6 @@
  * @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
  * @shared_with_punit: true if this bus is shared with the SoCs PUNIT
@@ -263,9 +261,8 @@ 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);
+	int			(*acquire_lock)(void);
+	void			(*release_lock)(void);
 	bool			shared_with_punit;
 	void			(*disable)(struct dw_i2c_dev *dev);
 	void			(*disable_int)(struct dw_i2c_dev *dev);
@@ -320,8 +317,6 @@ static inline int i2c_dw_probe_slave(struct dw_i2c_dev *dev) { return -EINVAL; }
 
 #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
 extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev);
-extern void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev);
 #else
 static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) { return 0; }
-static inline void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev) {}
 #endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 99c22d764a6f..d4604bea78ad 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -370,8 +370,6 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
 
-	i2c_dw_remove_lock_support(dev);
-
 	return 0;
 }
 
-- 
2.19.0.rc1

Re: [PATCH 3/3] i2c: designware: Cleanup bus lock handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

On Sun, Sep 23, 2018 at 04:45:10PM +0200, Hans de Goede wrote:
> Now that most of the special Bay- / Cherry-Trail bus lock handling has
> been moved to the iosf_mbi code we can simplify the remaining code a bit.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]