[PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[David E. Box]

This patch implements an I2C bus sharing mechanism between the host and platform
hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC.

On these platforms access to the PMIC must be shared with platform hardware. The
hardware unit assumes full control of the I2C bus and the host must request
access through a special semaphore. Hardware control of the bus also makes it
necessary to disable runtime pm to avoid interfering with hardware transactions.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/i2c/busses/Kconfig                  |  10 +++
 drivers/i2c/busses/Makefile                 |   1 +
 drivers/i2c/busses/i2c-designware-core.h    |  14 ++++
 drivers/i2c/busses/i2c-designware-platdrv.c |  78 +++++++++++++++++++--
 drivers/i2c/busses/i2c-shared-controller.c  | 101 ++++++++++++++++++++++++++++
 5 files changed, 200 insertions(+), 4 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-shared-controller.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..672ef23 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_SHARED_CONTROLLER
+	tristate "Intel Baytrail PMIC shared I2C bus support"
+	depends on ACPI
+	select IOSF_MBI
+	select I2C_DESIGNWARE_CORE
+	help
+	  This driver enables shared access to the PMIC I2C bus on select Intel
+	  BayTrail platforms using the XPower AXP288 PMIC. This driver is
+	  required for host access to the PMIC on these platforms.
+
 config I2C_EFM32
 	tristate "EFM32 I2C controller"
 	depends on ARCH_EFM32 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 49bf07e..33d62d1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
 obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
+obj-$(CONFIG_I2C_SHARED_CONTROLLER)	+= i2c-shared-controller.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
 obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d66b6cb..a2b72f4 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -65,6 +65,10 @@
  * @ss_lcnt: standard speed LCNT value
  * @fs_hcnt: fast speed HCNT value
  * @fs_lcnt: fast speed LCNT value
+ * @shared_host: if host must share access to adapter with other
+ * firmware/hardware units
+ * @acquire_ownership: function to acquire exclusive use of the controller
+ * @release_ownership: function to release exclusive use of the controller
  *
  * HCNT and LCNT parameters can be used if the platform knows more accurate
  * values than the one computed based only on the input clock frequency.
@@ -105,6 +109,11 @@ struct dw_i2c_dev {
 	u16			ss_lcnt;
 	u16			fs_hcnt;
 	u16			fs_lcnt;
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+	int			shared_host;
+	int			(*acquire_ownership)(struct device *dev);
+	int			(*release_ownership)(struct device *dev);
+#endif
 };
 
 #define ACCESS_SWAP		0x00000001
@@ -123,3 +132,8 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+extern int i2c_acquire_ownership(struct device *dev);
+extern int i2c_release_ownership(struct device *dev);
+#endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index f9b1dec..f86c285 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -52,6 +52,32 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 	return clk_get_rate(dev->clk)/1000;
 }
 
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+int i2c_shared_controller_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+	int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	int err;
+
+	if (dev->shared_host) {
+		err = dev->acquire_ownership(dev->dev);
+		if (!err) {
+			err = i2c_dw_xfer(adap, msgs, num);
+			dev->release_ownership(dev->dev);
+		} else
+			dev_WARN(dev->dev, "couldnt acquire ownership\n");
+
+		return err;
+	} else
+		return i2c_dw_xfer(adap, msgs, num);
+}
+
+static struct i2c_algorithm i2c_sc_algo = {
+	.master_xfer	= i2c_shared_controller_xfer,
+	.functionality	= i2c_dw_func,
+};
+#endif
+
 #ifdef CONFIG_ACPI
 static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],
 			       u16 *hcnt, u16 *lcnt, u32 *sda_hold)
@@ -80,8 +106,11 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	bool fs_mode = dev->master_cfg & DW_IC_CON_SPEED_FAST;
+	acpi_handle handle = ACPI_HANDLE(&pdev->dev);
+	acpi_status status;
+	unsigned long long shared_host = 0;
 
-	if (!ACPI_HANDLE(&pdev->dev))
+	if (!handle)
 		return -ENODEV;
 
 	dev->adapter.nr = -1;
@@ -97,6 +126,24 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt,
 			   fs_mode ? &dev->sda_hold_time : NULL);
 
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+	/*
+	 * For Intel SOC's determine if i2c bus must be shared with PUNIT
+	 * hardware
+	 */
+	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
+
+	if (ACPI_SUCCESS(status))
+		dev_info(&pdev->dev, "_SEM=%ld\n", (long)shared_host);
+
+	if (shared_host != 0) {
+		dev_info(&pdev->dev,
+			"Host shares controller with other hardware\n");
+		dev->shared_host = 1;
+		dev->acquire_ownership = i2c_acquire_ownership;
+		dev->release_ownership = i2c_release_ownership;
+	}
+#endif /* CONFIG_I2C_SHARED_CONTROLLER */
 	return 0;
 }
 
@@ -115,7 +162,7 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
 	return -ENODEV;
 }
-#endif
+#endif /* CONFIG_ACPI */
 
 static int dw_i2c_probe(struct platform_device *pdev)
 {
@@ -212,6 +259,25 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
 
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+	if (dev->shared_host)
+		adap->algo = &i2c_sc_algo;
+
+	r = i2c_add_numbered_adapter(adap);
+	if (r) {
+		dev_err(&pdev->dev, "failure adding adapter\n");
+		return r;
+	}
+
+	if (dev->shared_host)
+		pm_runtime_forbid(&pdev->dev);
+	else {
+		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+		pm_runtime_use_autosuspend(&pdev->dev);
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
+#else
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
@@ -222,7 +288,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
-
+#endif
 	return 0;
 }
 
@@ -268,7 +334,11 @@ static int dw_i2c_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	clk_prepare_enable(i_dev->clk);
-	i2c_dw_init(i_dev);
+
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+	if (!i_dev->shared_host)
+#endif
+		i2c_dw_init(i_dev);
 
 	return 0;
 }
diff --git a/drivers/i2c/busses/i2c-shared-controller.c b/drivers/i2c/busses/i2c-shared-controller.c
new file mode 100644
index 0000000..b9119a0
--- /dev/null
+++ b/drivers/i2c/busses/i2c-shared-controller.c
@@ -0,0 +1,101 @@
+/*
+ * Intel SOC I2C bus sharing semphore implementation
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <asm/iosf_mbi.h>
+
+#define PUNIT_SEMAPHORE 0x7
+static unsigned long start_time, end_time;
+
+static int get_sem(struct device *dev, u32 *sem)
+{
+	u32 reg_val;
+	int ret;
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ, PUNIT_SEMAPHORE,
+			    &reg_val);
+	if (ret) {
+		dev_WARN(dev, "iosf failed to read punit semaphore\n");
+		return ret;
+	}
+
+	*sem = reg_val & 0x1;
+
+	return 0;
+}
+
+static void reset_semaphore(struct device *dev)
+{
+	u32 data;
+
+	if (iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+				PUNIT_SEMAPHORE, &data)) {
+		dev_err(dev, "iosf failed to reset punit semaphore\n");
+		return;
+	}
+
+	data = data & 0xfffffffe;
+	if (iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, data))
+		dev_err(dev, "iosf failed to reset punit semaphore\n");
+}
+
+int i2c_acquire_ownership(struct device *dev)
+{
+	u32 sem = 0;
+	int ret;
+	int timeout = 100;
+
+	/* host driver writes 0x2 to side band semaphore register */
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, 0x2);
+	if (ret) {
+		dev_WARN(dev, "iosf failed to request punit semaphore\n");
+		return ret;
+	}
+
+	/* host driver waits for bit 0 to be set in semaphore register */
+	while (1) {
+		ret = get_sem(dev, &sem);
+		if (!ret && sem) {
+			start_time = jiffies;
+			dev_dbg(dev, "punit semaphore acquired after %d attempts\n",
+				101 - timeout);
+			return 0;
+		}
+
+		usleep_range(1000, 2000);
+		timeout--;
+		if (timeout <= 0) {
+			dev_err(dev, "punit semaphore timed out, resetting\n");
+			reset_semaphore(dev);
+			iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+				PUNIT_SEMAPHORE, &sem);
+			dev_err(dev, "PUNIT SEM: %d\n", sem);
+			WARN_ON(1);
+			return -ETIMEDOUT;
+		}
+
+	}
+}
+
+int i2c_release_ownership(struct device *dev)
+{
+	reset_semaphore(dev);
+	end_time = jiffies;
+	dev_dbg(dev, "punit semaphore release call finish, held for %ldms\n",
+		(end_time - start_time) * 1000 / HZ);
+	return 0;
+}
-- 
1.9.1

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[Maxime Coquelin]

Hi David,

On 09/12/2014 07:36 PM, David E. Box wrote:
> This patch implements an I2C bus sharing mechanism between the host and platform
> hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC.
>
> On these platforms access to the PMIC must be shared with platform hardware. The
> hardware unit assumes full control of the I2C bus and the host must request
> access through a special semaphore. Hardware control of the bus also makes it
> necessary to disable runtime pm to avoid interfering with hardware transactions.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>   drivers/i2c/busses/Kconfig                  |  10 +++
>   drivers/i2c/busses/Makefile                 |   1 +
>   drivers/i2c/busses/i2c-designware-core.h    |  14 ++++
>   drivers/i2c/busses/i2c-designware-platdrv.c |  78 +++++++++++++++++++--
>   drivers/i2c/busses/i2c-shared-controller.c  | 101 ++++++++++++++++++++++++++++
>   5 files changed, 200 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/i2c/busses/i2c-shared-controller.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..672ef23 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called i2c-designware-pci.
>
> +config I2C_SHARED_CONTROLLER
This config name is too generic.
 From what I understand, it is very Intel dependant.

> +	tristate "Intel Baytrail PMIC shared I2C bus support"
> +	depends on ACPI
> +	select IOSF_MBI
> +	select I2C_DESIGNWARE_CORE
> +	help
> +	  This driver enables shared access to the PMIC I2C bus on select Intel
> +	  BayTrail platforms using the XPower AXP288 PMIC. This driver is
> +	  required for host access to the PMIC on these platforms.
> +
>   config I2C_EFM32
>   	tristate "EFM32 I2C controller"
>   	depends on ARCH_EFM32 || COMPILE_TEST
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 49bf07e..33d62d1 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
>   obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
>   i2c-designware-platform-objs := i2c-designware-platdrv.o
>   obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
> +obj-$(CONFIG_I2C_SHARED_CONTROLLER)	+= i2c-shared-controller.o
Ditto, the file name is too generic.

>   i2c-designware-pci-objs := i2c-designware-pcidrv.o
>   obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
>   obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index d66b6cb..a2b72f4 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -65,6 +65,10 @@
>    * @ss_lcnt: standard speed LCNT value
>    * @fs_hcnt: fast speed HCNT value
>    * @fs_lcnt: fast speed LCNT value
> + * @shared_host: if host must share access to adapter with other
> + * firmware/hardware units
> + * @acquire_ownership: function to acquire exclusive use of the controller
> + * @release_ownership: function to release exclusive use of the controller
>    *
>    * HCNT and LCNT parameters can be used if the platform knows more accurate
>    * values than the one computed based only on the input clock frequency.
> @@ -105,6 +109,11 @@ struct dw_i2c_dev {
>   	u16			ss_lcnt;
>   	u16			fs_hcnt;
>   	u16			fs_lcnt;
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +	int			shared_host;
> +	int			(*acquire_ownership)(struct device *dev);
> +	int			(*release_ownership)(struct device *dev);
> +#endif
>   };
>
>   #define ACCESS_SWAP		0x00000001
> @@ -123,3 +132,8 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
>   extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
>   extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
>   extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
> +
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +extern int i2c_acquire_ownership(struct device *dev);
> +extern int i2c_release_ownership(struct device *dev);
> +#endif
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index f9b1dec..f86c285 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -52,6 +52,32 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
>   	return clk_get_rate(dev->clk)/1000;
>   }
>
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +int i2c_shared_controller_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> +	int num)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int err;
> +
> +	if (dev->shared_host) {
this share_host variable is not needed, you could just check whether 
acquire callback is set.
> +		err = dev->acquire_ownership(dev->dev);
Have you considered using hwspinlocks instead?

> +		if (!err) {
Please prefer checking for error, no succes.


> +			err = i2c_dw_xfer(adap, msgs, num);
> +			dev->release_ownership(dev->dev);
> +		} else
You need braces here too.
> +			dev_WARN(dev->dev, "couldnt acquire ownership\n");
why not dev_warn? or better, dev_err?
> +
> +		return err;
> +	} else
Braces around this too.
> +		return i2c_dw_xfer(adap, msgs, num);
> +}
> +
> +static struct i2c_algorithm i2c_sc_algo = {
> +	.master_xfer	= i2c_shared_controller_xfer,
> +	.functionality	= i2c_dw_func,
> +};
> +#endif
> +
>   #ifdef CONFIG_ACPI
>   static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],
>   			       u16 *hcnt, u16 *lcnt, u32 *sda_hold)
> @@ -80,8 +106,11 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
>   {
>   	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>   	bool fs_mode = dev->master_cfg & DW_IC_CON_SPEED_FAST;
> +	acpi_handle handle = ACPI_HANDLE(&pdev->dev);
> +	acpi_status status;
> +	unsigned long long shared_host = 0;
>
> -	if (!ACPI_HANDLE(&pdev->dev))
> +	if (!handle)
>   		return -ENODEV;
>
>   	dev->adapter.nr = -1;
> @@ -97,6 +126,24 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
>   	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt,
>   			   fs_mode ? &dev->sda_hold_time : NULL);
>
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +	/*
> +	 * For Intel SOC's determine if i2c bus must be shared with PUNIT
> +	 * hardware
> +	 */
> +	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
> +
> +	if (ACPI_SUCCESS(status))
> +		dev_info(&pdev->dev, "_SEM=%ld\n", (long)shared_host);
> +
> +	if (shared_host != 0) {
> +		dev_info(&pdev->dev,
> +			"Host shares controller with other hardware\n");
> +		dev->shared_host = 1;
> +		dev->acquire_ownership = i2c_acquire_ownership;
> +		dev->release_ownership = i2c_release_ownership;
> +	}
> +#endif /* CONFIG_I2C_SHARED_CONTROLLER */
>   	return 0;
>   }
>
> @@ -115,7 +162,7 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
>   {
>   	return -ENODEV;
>   }
> -#endif
> +#endif /* CONFIG_ACPI */
>
>   static int dw_i2c_probe(struct platform_device *pdev)
>   {
> @@ -212,6 +259,25 @@ static int dw_i2c_probe(struct platform_device *pdev)
>   	adap->dev.parent = &pdev->dev;
>   	adap->dev.of_node = pdev->dev.of_node;
>
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +	if (dev->shared_host)
> +		adap->algo = &i2c_sc_algo;
> +
> +	r = i2c_add_numbered_adapter(adap);
> +	if (r) {
> +		dev_err(&pdev->dev, "failure adding adapter\n");
> +		return r;
> +	}
> +
> +	if (dev->shared_host)
> +		pm_runtime_forbid(&pdev->dev);
> +	else {
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +		pm_runtime_use_autosuspend(&pdev->dev);
> +		pm_runtime_set_active(&pdev->dev);
> +		pm_runtime_enable(&pdev->dev);
> +	}
> +#else
Why do you put all this under config flags?

>   	r = i2c_add_numbered_adapter(adap);
>   	if (r) {
>   		dev_err(&pdev->dev, "failure adding adapter\n");
> @@ -222,7 +288,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
>   	pm_runtime_use_autosuspend(&pdev->dev);
>   	pm_runtime_set_active(&pdev->dev);
>   	pm_runtime_enable(&pdev->dev);
> -
> +#endif
>   	return 0;
>   }
>
> @@ -268,7 +334,11 @@ static int dw_i2c_resume(struct device *dev)
>   	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>
>   	clk_prepare_enable(i_dev->clk);
> -	i2c_dw_init(i_dev);
> +
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +	if (!i_dev->shared_host)
> +#endif
Putting this under config flag should not be needed.

And even not under config flags, why don't you re-initialize your device 
in case of resume?
> +		i2c_dw_init(i_dev);
>
>   	return 0;
>   }
> diff --git a/drivers/i2c/busses/i2c-shared-controller.c b/drivers/i2c/busses/i2c-shared-controller.c
> new file mode 100644
> index 0000000..b9119a0
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-shared-controller.c
> @@ -0,0 +1,101 @@
> +/*
> + * Intel SOC I2C bus sharing semphore implementation
s/semphore/semaphore/
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <asm/iosf_mbi.h>
This driver should depend on x86, as this include file is only present 
in this architecture.

> +
> +#define PUNIT_SEMAPHORE 0x7
> +static unsigned long start_time, end_time;
> +
> +static int get_sem(struct device *dev, u32 *sem)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ, PUNIT_SEMAPHORE,
> +			    &reg_val);
> +	if (ret) {
> +		dev_WARN(dev, "iosf failed to read punit semaphore\n");
dev_err?
> +		return ret;
> +	}
> +
> +	*sem = reg_val & 0x1;
> +
> +	return 0;
> +}
> +
> +static void reset_semaphore(struct device *dev)
> +{
> +	u32 data;
> +
> +	if (iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
> +				PUNIT_SEMAPHORE, &data)) {
> +		dev_err(dev, "iosf failed to reset punit semaphore\n");
> +		return;
> +	}
> +
> +	data = data & 0xfffffffe;
> +	if (iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
> +				 PUNIT_SEMAPHORE, data))
> +		dev_err(dev, "iosf failed to reset punit semaphore\n");
> +}
> +
> +int i2c_acquire_ownership(struct device *dev)
> +{
> +	u32 sem = 0;
> +	int ret;
> +	int timeout = 100;
> +
> +	/* host driver writes 0x2 to side band semaphore register */
> +	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
> +				 PUNIT_SEMAPHORE, 0x2);
> +	if (ret) {
> +		dev_WARN(dev, "iosf failed to request punit semaphore\n");
> +		return ret;
> +	}
> +
> +	/* host driver waits for bit 0 to be set in semaphore register */
> +	while (1) {
> +		ret = get_sem(dev, &sem);
> +		if (!ret && sem) {
> +			start_time = jiffies;
> +			dev_dbg(dev, "punit semaphore acquired after %d attempts\n",
> +				101 - timeout);
> +			return 0;
> +		}
> +
> +		usleep_range(1000, 2000);
> +		timeout--;
> +		if (timeout <= 0) {
Couldn't you use time_after(jiffies, jiffies + timeout)...?

> +			dev_err(dev, "punit semaphore timed out, resetting\n");
> +			reset_semaphore(dev);
> +			iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
> +				PUNIT_SEMAPHORE, &sem);
> +			dev_err(dev, "PUNIT SEM: %d\n", sem);
> +			WARN_ON(1);
> +			return -ETIMEDOUT;
> +		}
> +
> +	}
> +}
> +
> +int i2c_release_ownership(struct device *dev)
> +{
> +	reset_semaphore(dev);
> +	end_time = jiffies;
> +	dev_dbg(dev, "punit semaphore release call finish, held for %ldms\n",
> +		(end_time - start_time) * 1000 / HZ);
> +	return 0;
> +}
>

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[David E. Box]

Hi Maxime,

On Mon, Sep 15, 2014 at 08:57:38AM +0200, Maxime Coquelin wrote:

> >+		err = dev->acquire_ownership(dev->dev);
> Have you considered using hwspinlocks instead?

No, I've not used it before but it looks applicable here. I'll take a look.

> >@@ -212,6 +259,25 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  	adap->dev.parent = &pdev->dev;
> >  	adap->dev.of_node = pdev->dev.of_node;
> >
> >+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> >+	if (dev->shared_host)
> >+		adap->algo = &i2c_sc_algo;
> >+
> >+	r = i2c_add_numbered_adapter(adap);
> >+	if (r) {
> >+		dev_err(&pdev->dev, "failure adding adapter\n");
> >+		return r;
> >+	}
> >+
> >+	if (dev->shared_host)
> >+		pm_runtime_forbid(&pdev->dev);
> >+	else {
> >+		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >+		pm_runtime_use_autosuspend(&pdev->dev);
> >+		pm_runtime_set_active(&pdev->dev);
> >+		pm_runtime_enable(&pdev->dev);
> >+	}
> >+#else
> Why do you put all this under config flags?

So that this additional code only compiles for this very specific
implementation.

> >@@ -268,7 +334,11 @@ static int dw_i2c_resume(struct device *dev)
> >  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> >
> >  	clk_prepare_enable(i_dev->clk);
> >-	i2c_dw_init(i_dev);
> >+
> >+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> >+	if (!i_dev->shared_host)
> >+#endif
> Putting this under config flag should not be needed.
> 
> And even not under config flags, why don't you re-initialize your
> device in case of resume?

Because the device is already being managed by hardware, not the OS.

David Box

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[Mika Westerberg]

On Fri, Sep 12, 2014 at 10:36:07AM -0700, David E. Box wrote:
> This patch implements an I2C bus sharing mechanism between the host and platform
> hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC.
> 
> On these platforms access to the PMIC must be shared with platform hardware. The
> hardware unit assumes full control of the I2C bus and the host must request
> access through a special semaphore. Hardware control of the bus also makes it
> necessary to disable runtime pm to avoid interfering with hardware transactions.

Is this because we need to access the PMIC from host as well? I mean
from some PMIC driver (which driver btw)?

Otherwise it would be best to just detect _SEM and return -ENODEV.

> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/i2c/busses/Kconfig                  |  10 +++
>  drivers/i2c/busses/Makefile                 |   1 +
>  drivers/i2c/busses/i2c-designware-core.h    |  14 ++++
>  drivers/i2c/busses/i2c-designware-platdrv.c |  78 +++++++++++++++++++--
>  drivers/i2c/busses/i2c-shared-controller.c  | 101 ++++++++++++++++++++++++++++
>  5 files changed, 200 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-shared-controller.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 2ac87fa..672ef23 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-designware-pci.
>  
> +config I2C_SHARED_CONTROLLER
> +	tristate "Intel Baytrail PMIC shared I2C bus support"
> +	depends on ACPI
> +	select IOSF_MBI
> +	select I2C_DESIGNWARE_CORE
> +	help
> +	  This driver enables shared access to the PMIC I2C bus on select Intel
> +	  BayTrail platforms using the XPower AXP288 PMIC. This driver is
> +	  required for host access to the PMIC on these platforms.

Can't we detect this runtime in the i2c-designware-platdrv.c code so
that you look (in the ACPI part of the driver) for _SEM and in that case
change the xfer function behaviour a bit to return -EBUSY or whatever?

Without this horrible #ifdeffery.

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[Jacob Pan]

On Tue, 16 Sep 2014 12:44:49 +0300
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> Is this because we need to access the PMIC from host as well? I mean
> from some PMIC driver (which driver btw)?
> 
This is used by the X-Powers PMIC.
https://lkml.org/lkml/2014/9/11/1016

> Otherwise it would be best to just detect _SEM and return -ENODEV.

Thanks,

Jacob

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[Mika Westerberg]

On Tue, Sep 16, 2014 at 03:53:27AM -0700, Jacob Pan wrote:
> On Tue, 16 Sep 2014 12:44:49 +0300
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > Is this because we need to access the PMIC from host as well? I mean
> > from some PMIC driver (which driver btw)?
> > 
> This is used by the X-Powers PMIC.
> https://lkml.org/lkml/2014/9/11/1016

Ok, thanks Jacob. So just returning -ENODEV is out of the question :-(

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[Li, Aubrey]

On 2014/9/16 17:44, Mika Westerberg wrote:
> On Fri, Sep 12, 2014 at 10:36:07AM -0700, David E. Box wrote:
>> This patch implements an I2C bus sharing mechanism between the host and platform
>> hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC.
>>
>> On these platforms access to the PMIC must be shared with platform hardware. The
>> hardware unit assumes full control of the I2C bus and the host must request
>> access through a special semaphore. Hardware control of the bus also makes it
>> necessary to disable runtime pm to avoid interfering with hardware transactions.
> 
> Is this because we need to access the PMIC from host as well? I mean
> from some PMIC driver (which driver btw)?
> 
> Otherwise it would be best to just detect _SEM and return -ENODEV.

On some Baytrail platform, one I2C port is shared between CPU and Punit,
and Punit access is out the control of CPU, so

1. need to keep this shared I2C port always enabled, that is, no runtime
pm, in case Punit wants to access it.

2. need the semaphore mechanism to access i2c.

> 
>>
>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>> ---
>>  drivers/i2c/busses/Kconfig                  |  10 +++
>>  drivers/i2c/busses/Makefile                 |   1 +
>>  drivers/i2c/busses/i2c-designware-core.h    |  14 ++++
>>  drivers/i2c/busses/i2c-designware-platdrv.c |  78 +++++++++++++++++++--
>>  drivers/i2c/busses/i2c-shared-controller.c  | 101 ++++++++++++++++++++++++++++
>>  5 files changed, 200 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/i2c/busses/i2c-shared-controller.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 2ac87fa..672ef23 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called i2c-designware-pci.
>>  
>> +config I2C_SHARED_CONTROLLER
>> +	tristate "Intel Baytrail PMIC shared I2C bus support"
>> +	depends on ACPI
>> +	select IOSF_MBI
>> +	select I2C_DESIGNWARE_CORE
>> +	help
>> +	  This driver enables shared access to the PMIC I2C bus on select Intel
>> +	  BayTrail platforms using the XPower AXP288 PMIC. This driver is
>> +	  required for host access to the PMIC on these platforms.
> 
> Can't we detect this runtime in the i2c-designware-platdrv.c code so
> that you look (in the ACPI part of the driver) for _SEM and in that case
> change the xfer function behaviour a bit to return -EBUSY or whatever?
> 
> Without this horrible #ifdeffery.

not necessary to have #ifdef, really.

Thanks,
-Aubrey


> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support

[One Thousand Gnomes]

On Fri, 12 Sep 2014 10:36:07 -0700
"David E. Box" <david.e.box@linux.intel.com> wrote:

> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +extern int i2c_acquire_ownership(struct device *dev);
> +extern int i2c_release_ownership(struct device *dev);
> +#endif

You can just have the prototypes anyway - no need for more ifdefs than
required


> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +int i2c_shared_controller_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> +	int num)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int err;
> +
> +	if (dev->shared_host) {
> +		err = dev->acquire_ownership(dev->dev);
> +		if (!err) {
> +			err = i2c_dw_xfer(adap, msgs, num);
> +			dev->release_ownership(dev->dev);
> +		} else
> +			dev_WARN(dev->dev, "couldnt acquire ownership\n");
> +
> +		return err;
> +	} else
> +		return i2c_dw_xfer(adap, msgs, num);
> +}
> +
> +static struct i2c_algorithm i2c_sc_algo = {
> +	.master_xfer	= i2c_shared_controller_xfer,
> +	.functionality	= i2c_dw_func,
> +};
> +#endif

I think this might be a lot cleaner if you put these pieces as functions
into i2c-designware-sem.c or a similar file and made the methods NULL
functions in the header in the case it's not supported ? Most of the
ifdeffery would then vanish into the extra file and keep the core code
cleaner ?


Alan

[PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

This patch implements an I2C bus sharing mechanism between the host and platform
hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.

On these platforms access to the PMIC must be shared with platform hardware. The
hardware unit assumes full control of the I2C bus and the host must request
access through a special semaphore. Hardware control of the bus also makes it
necessary to disable runtime pm to avoid interfering with hardware transactions.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---

V2:	Moved semaphore detection out of dw platform driver
	Replaced function pointers with defined acquire/release functions in dw
		core. This helps elliminate the ifdefery in the dw platform driver.
	Use new has_hw_lock flag to check if the lock exists on a given bus.
	Use new pm_runtime_disabled flag to conditionally turnoff runtime pm
		in the dw platform driver.

 drivers/i2c/busses/Kconfig                  |  16 +++
 drivers/i2c/busses/Makefile                 |   1 +
 drivers/i2c/busses/i2c-baytrail-sem.c       | 157 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c    |   7 ++
 drivers/i2c/busses/i2c-designware-core.h    |  19 ++++
 drivers/i2c/busses/i2c-designware-platdrv.c |  18 +++-
 6 files changed, 213 insertions(+), 5 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-baytrail-sem.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..036f16f 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -441,6 +441,22 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_BAYTRAIL_SEM
+	tristate "Intel Baytrail I2C semaphore support"
+	depends on I2C_DESIGNWARE_PLATFORM
+	select I2C_DESIGNWARE_CORE
+	select IOSF_MBI
+	help
+	  This driver enables host access to the PMIC I2C bus on select Intel
+	  BayTrail platforms using the X-Powers AXP288 PMIC. This driver is
+	  required for host access to the PMIC on these platforms. You should
+	  probably say Y if you have a BayTrail system, unless you know it uses
+	  a different PMIC. Otherwises critical PMIC functions, like charging,
+	  may not operate.
+
+	  This driver should be built as a m if I2C_DESIGNWARE_PLATFORM=m,
+	  and as y if I2C_DESIGNWARE_PLATFORM=y.
+
 config I2C_EFM32
 	tristate "EFM32 I2C controller"
 	depends on ARCH_EFM32 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 49bf07e..6f143b4 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
+obj-$(CONFIG_I2C_BAYTRAIL_SEM)	+= i2c-baytrail-sem.o
 obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
 obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
diff --git a/drivers/i2c/busses/i2c-baytrail-sem.c b/drivers/i2c/busses/i2c-baytrail-sem.c
new file mode 100644
index 0000000..389aa23
--- /dev/null
+++ b/drivers/i2c/busses/i2c-baytrail-sem.c
@@ -0,0 +1,157 @@
+/*
+ * Intel BayTrail PMIC I2C bus semaphore implementaion
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <asm/iosf_mbi.h>
+#include "i2c-designware-core.h"
+
+#define SEMAPHORE_TIMEOUT	100
+#define PUNIT_SEMAPHORE		0x7
+
+static unsigned long acquired;
+
+void baytrail_evaluate_sem(struct dw_i2c_dev *dev)
+{
+	acpi_status status;
+	unsigned long long shared_host = 0;
+	acpi_handle handle;
+
+	if (!dev || !dev->dev) {
+		pr_err("%s:%d: device is NULL\n", __func__, __LINE__);
+		return;
+	}
+
+	handle = ACPI_HANDLE(dev->dev);
+	if (!handle)
+		return;
+
+	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
+
+	if (shared_host) {
+		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+		dev->has_hw_lock = true;
+		dev->pm_runtime_disabled = true;
+	}
+}
+EXPORT_SYMBOL(baytrail_evaluate_sem);
+
+static int get_sem(struct device *dev, u32 *sem)
+{
+	u32 reg_val;
+	int ret;
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ, PUNIT_SEMAPHORE,
+			    &reg_val);
+	if (ret) {
+		dev_err(dev, "iosf failed to read punit semaphore\n");
+		return ret;
+	}
+
+	*sem = reg_val & 0x1;
+
+	return 0;
+}
+
+static void reset_semaphore(struct device *dev)
+{
+	u32 data;
+
+	if (iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+				PUNIT_SEMAPHORE, &data)) {
+		dev_err(dev, "iosf failed to reset punit semaphore\n");
+		return;
+	}
+
+	data = data & 0xfffffffe;
+	if (iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, data))
+		dev_err(dev, "iosf failed to reset punit semaphore\n");
+}
+
+int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
+{
+	u32 sem = 0;
+	int ret;
+	unsigned long start, end;
+
+	if (!dev || !dev->dev) {
+		pr_err("%s:%d: device is NULL\n", __func__, __LINE__);
+		return -ENOMEM;
+	}
+
+	if (!dev->has_hw_lock)
+		return 0;
+
+	/* host driver writes 0x2 to side band semaphore register */
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, 0x2);
+	if (ret) {
+		dev_err(dev->dev, "iosf failed to request punit semaphore\n");
+		return ret;
+	}
+
+	/* host driver waits for bit 0 to be set in semaphore register */
+	start = jiffies;
+	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
+	while (!time_after(jiffies, end)) {
+		ret = get_sem(dev->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);
+	}
+
+	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
+	reset_semaphore(dev->dev);
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+		PUNIT_SEMAPHORE, &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;
+}
+EXPORT_SYMBOL(baytrail_i2c_acquire);
+
+void baytrail_i2c_release(struct dw_i2c_dev *dev)
+{
+	if (!dev || !dev->dev) {
+		pr_err("%s:%d: device is NULL\n", __func__, __LINE__);
+		return;
+	}
+
+	if (!dev->has_hw_lock)
+		return;
+
+	reset_semaphore(dev->dev);
+	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
+		jiffies_to_msecs(jiffies - acquired));
+}
+EXPORT_SYMBOL(baytrail_i2c_release);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Baytrail I2C Semaphore driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 3c20e4b..185401f 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -631,6 +631,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
 
+	ret = i2c_dw_acquire_ownership(dev);
+	if (ret) {
+		dev_err(dev->dev, "couldn't acquire ownership of bus\n");
+		goto done;
+	}
+
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
 		goto done;
@@ -676,6 +682,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = -EIO;
 
 done:
+	i2c_dw_release_ownership(dev);
 	pm_runtime_mark_last_busy(dev->dev);
 	pm_runtime_put_autosuspend(dev->dev);
 	mutex_unlock(&dev->lock);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d66b6cb..13e0809 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -65,6 +65,8 @@
  * @ss_lcnt: standard speed LCNT value
  * @fs_hcnt: fast speed HCNT value
  * @fs_lcnt: fast speed LCNT value
+ * has_hw_lock: true if bus access requires hardware lock
+ * pm_runtime_disabled: true if pm runtime is disabled
  *
  * HCNT and LCNT parameters can be used if the platform knows more accurate
  * values than the one computed based only on the input clock frequency.
@@ -105,6 +107,8 @@ struct dw_i2c_dev {
 	u16			ss_lcnt;
 	u16			fs_hcnt;
 	u16			fs_lcnt;
+	bool			has_hw_lock;
+	bool			pm_runtime_disabled;
 };
 
 #define ACCESS_SWAP		0x00000001
@@ -123,3 +127,18 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+
+#if IS_ENABLED(CONFIG_I2C_BAYTRAIL_SEM)
+extern int baytrail_i2c_acquire(struct dw_i2c_dev *dev);
+extern void baytrail_i2c_release(struct dw_i2c_dev *dev);
+extern void baytrail_evaluate_sem(struct dw_i2c_dev *dev);
+#define i2c_dw_acquire_ownership(dev) baytrail_i2c_acquire(dev)
+#define i2c_dw_release_ownership(dev) baytrail_i2c_release(dev)
+#define i2c_dw_eval_lock(dev) baytrail_evaluate_sem(dev)
+
+#else
+static inline int i2c_dw_acquire_ownership(struct dw_i2c_dev *dev) { return 0; }
+static inline void i2c_dw_release_ownership(struct dw_i2c_dev *dev) { }
+static inline void i2c_dw_eval_lock(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 f9b1dec..036c6ef 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -218,10 +218,16 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	i2c_dw_eval_lock(dev);
+
+	if (dev->pm_runtime_disabled) {
+		pm_runtime_forbid(&pdev->dev);
+	} else {
+		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+		pm_runtime_use_autosuspend(&pdev->dev);
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
 
 	return 0;
 }
@@ -268,7 +274,9 @@ static int dw_i2c_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	clk_prepare_enable(i_dev->clk);
-	i2c_dw_init(i_dev);
+
+	if (!i_dev->pm_runtime_disabled)
+		i2c_dw_init(i_dev);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Maxime Ripard]

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

Hi David,

On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> This patch implements an I2C bus sharing mechanism between the host and platform
> hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> 
> On these platforms access to the PMIC must be shared with platform hardware. The
> hardware unit assumes full control of the I2C bus and the host must request
> access through a special semaphore. Hardware control of the bus also makes it
> necessary to disable runtime pm to avoid interfering with hardware transactions.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Sorry for stepping in like this without really knowing your platform,
but wouldn't using the hwspinlock framework make more sense than
hardcoding your own internal functions here?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

Hi Maxime,

On Tue, Sep 23, 2014 at 09:00:57PM +0200, Maxime Ripard wrote:
> Hi David,
> 
> On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> > This patch implements an I2C bus sharing mechanism between the host and platform
> > hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> > 
> > On these platforms access to the PMIC must be shared with platform hardware. The
> > hardware unit assumes full control of the I2C bus and the host must request
> > access through a special semaphore. Hardware control of the bus also makes it
> > necessary to disable runtime pm to avoid interfering with hardware transactions.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> 
> Sorry for stepping in like this without really knowing your platform,
> but wouldn't using the hwspinlock framework make more sense than
> hardcoding your own internal functions here?

I looked into this but didn't see a clear way on our platform to identify the
semaphore seperately from doing it in the designware platform driver. The way
we can find it now is through evaluating an ACPI _SEM object on every i2c device
that gets probed by the dw driver since at probe time we can get the acpi handle.
Without this handle however there isn't a clear way of evaluating the _SEM
object which would be needed to register a hwspinlock in separate code.

Plus it would still require changes to the designware i2c core, though admittedly
having a generic hwspinlock pointer added to the struct is cleaner.

Dave

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Maxime Ripard]

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

Hi David,

On Tue, Sep 23, 2014 at 12:58:54PM -0700, David E. Box wrote:
> Hi Maxime,
> 
> On Tue, Sep 23, 2014 at 09:00:57PM +0200, Maxime Ripard wrote:
> > Hi David,
> > 
> > On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> > > This patch implements an I2C bus sharing mechanism between the host and platform
> > > hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> > > 
> > > On these platforms access to the PMIC must be shared with platform hardware. The
> > > hardware unit assumes full control of the I2C bus and the host must request
> > > access through a special semaphore. Hardware control of the bus also makes it
> > > necessary to disable runtime pm to avoid interfering with hardware transactions.
> > > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > 
> > Sorry for stepping in like this without really knowing your platform,
> > but wouldn't using the hwspinlock framework make more sense than
> > hardcoding your own internal functions here?
> 
> I looked into this but didn't see a clear way on our platform to identify the
> semaphore seperately from doing it in the designware platform driver. The way
> we can find it now is through evaluating an ACPI _SEM object on every i2c device
> that gets probed by the dw driver since at probe time we can get the acpi handle.

And you have no way to turn it around and identify which semaphore is
associated to which i2c bus?

If so, there is probably some way to associate a given instance of the
i2c driver to one semaphore.

> Without this handle however there isn't a clear way of evaluating the _SEM
> object which would be needed to register a hwspinlock in separate code.
> 
> Plus it would still require changes to the designware i2c core, though admittedly
> having a generic hwspinlock pointer added to the struct is cleaner.

Not only cleaner, but that could also be used by other platforms that
are using this I2C driver (and since it's a designware IP, there must
be quite a lot) together with hardware locking.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Maxime Ripard]

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

Hi David,

On Tue, Oct 07, 2014 at 12:14:20PM -0700, David E. Box wrote:
> Hi Maxime,
> 
> On Thu, Sep 25, 2014 at 11:47:52AM +0200, Maxime Ripard wrote:
> > Hi David,
> > 
> > On Tue, Sep 23, 2014 at 12:58:54PM -0700, David E. Box wrote:
> > > Hi Maxime,
> > > 
> > > On Tue, Sep 23, 2014 at 09:00:57PM +0200, Maxime Ripard wrote:
> > > > Hi David,
> > > > 
> > > > On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> > > > > This patch implements an I2C bus sharing mechanism between the host and platform
> > > > > hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> > > > > 
> > > > > On these platforms access to the PMIC must be shared with platform hardware. The
> > > > > hardware unit assumes full control of the I2C bus and the host must request
> > > > > access through a special semaphore. Hardware control of the bus also makes it
> > > > > necessary to disable runtime pm to avoid interfering with hardware transactions.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > 
> > > > Sorry for stepping in like this without really knowing your platform,
> > > > but wouldn't using the hwspinlock framework make more sense than
> > > > hardcoding your own internal functions here?
> > > 
> > > I looked into this but didn't see a clear way on our platform to identify the
> > > semaphore seperately from doing it in the designware platform driver. The way
> > > we can find it now is through evaluating an ACPI _SEM object on every i2c device
> > > that gets probed by the dw driver since at probe time we can get the acpi handle.
> > 
> > And you have no way to turn it around and identify which semaphore is
> > associated to which i2c bus?
> > 
> > If so, there is probably some way to associate a given instance of the
> > i2c driver to one semaphore.
> > 
> > > Without this handle however there isn't a clear way of evaluating the _SEM
> > > object which would be needed to register a hwspinlock in separate code.
> > > 
> > > Plus it would still require changes to the designware i2c core, though admittedly
> > > having a generic hwspinlock pointer added to the struct is cleaner.
> > 
> > Not only cleaner, but that could also be used by other platforms that
> > are using this I2C driver (and since it's a designware IP, there must
> > be quite a lot) together with hardware locking.
> > 
> 
> After again considering a way to make this work I don't think this api can fit
> well with our platform. Acquisition of this semaphore is through a mailbox
> sequence where we set one register and then poll another for a value that
> confirms we have the lock. For best performance we need to be able to
> periodically sleep while waiting for that confirmation. This time can vary
> widely as it's dependent on the component we are requesting the semaphore from
> which is itself a user of that bus.
> 
> While we could simply fail after a short time, reattempts would still need
> to happen in the i2c-designware driver and the timing would be completely
> dependent on our hardware, making it less clean for reuse. In addition,
> if we timed out, we would have to immediately call unlock to cancel the
> mailbox transaction. This may not fit well with reuse either.

Ok, if Wolfram is ok with it, and if it makes your life much easier,
I'm ok :)

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Wolfram Sang]

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

On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> This patch implements an I2C bus sharing mechanism between the host and platform
> hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> 
> On these platforms access to the PMIC must be shared with platform hardware. The
> hardware unit assumes full control of the I2C bus and the host must request
> access through a special semaphore. Hardware control of the bus also makes it
> necessary to disable runtime pm to avoid interfering with hardware transactions.

Can we foresee that other platforms will have similar mechanisms in the
future?

> +config I2C_BAYTRAIL_SEM

I2C_DESIGNWARE_BAYTRAIL_SEM

> +	tristate "Intel Baytrail I2C semaphore support"
> +	depends on I2C_DESIGNWARE_PLATFORM
> +	select I2C_DESIGNWARE_CORE

This select is already covered by I2C_DESIGNWARE_PLATFORM.

> +	select IOSF_MBI
> +	help
> +	  This driver enables host access to the PMIC I2C bus on select Intel
> +	  BayTrail platforms using the X-Powers AXP288 PMIC. This driver is
> +	  required for host access to the PMIC on these platforms. You should
> +	  probably say Y if you have a BayTrail system, unless you know it uses
> +	  a different PMIC. Otherwises critical PMIC functions, like charging,
> +	  may not operate.
> +
> +	  This driver should be built as a m if I2C_DESIGNWARE_PLATFORM=m,
> +	  and as y if I2C_DESIGNWARE_PLATFORM=y.

That shouldn't be the user's task to ensure. Please enforce this in the
makefile. Check Documentation/kbuid/makefiles.txt, Section 3.3.

> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-baytrail-sem.c
> @@ -0,0 +1,157 @@
> +/*
> + * Intel BayTrail PMIC I2C bus semaphore implementaion
> + * Copyright (c) 2014, Intel Corporation.

Mika, can you have a look at the ACPI part here?

> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index d66b6cb..13e0809 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -65,6 +65,8 @@
>   * @ss_lcnt: standard speed LCNT value
>   * @fs_hcnt: fast speed HCNT value
>   * @fs_lcnt: fast speed LCNT value
> + * has_hw_lock: true if bus access requires hardware lock
> + * pm_runtime_disabled: true if pm runtime is disabled

Look closely. There is a difference to the entries above.

> @@ -123,3 +127,18 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
>  extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
>  extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
> +
> +#if IS_ENABLED(CONFIG_I2C_BAYTRAIL_SEM)
> +extern int baytrail_i2c_acquire(struct dw_i2c_dev *dev);
> +extern void baytrail_i2c_release(struct dw_i2c_dev *dev);
> +extern void baytrail_evaluate_sem(struct dw_i2c_dev *dev);
> +#define i2c_dw_acquire_ownership(dev) baytrail_i2c_acquire(dev)
> +#define i2c_dw_release_ownership(dev) baytrail_i2c_release(dev)
> +#define i2c_dw_eval_lock(dev) baytrail_evaluate_sem(dev)
i2c_dw_test_ownership_support()?

That doesn't scale in case other platformts will need this. I could
imagine a struct i2c_dw_ownership_ops() (or whatever name) which gets
populated according to the matched device.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> +void baytrail_evaluate_sem(struct dw_i2c_dev *dev)
> +{
> +	acpi_status status;
> +	unsigned long long shared_host = 0;
> +	acpi_handle handle;
> +
> +	if (!dev || !dev->dev) {
> +		pr_err("%s:%d: device is NULL\n", __func__, __LINE__);

Not sure if it is useful to print things like above.

> +		return;
> +	}
> +
> +	handle = ACPI_HANDLE(dev->dev);
> +	if (!handle)
> +		return;
> +
> +	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);

Maybe it is better to check first if the operation succeeded before
touching shared_host?

	if (ACPI_SUCCESS(status) && shared_host) {
	}

Otherwise ACPI parts look good to me.

> +
> +	if (shared_host) {
> +		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
> +		dev->has_hw_lock = true;
> +		dev->pm_runtime_disabled = true;
> +	}
> +}
> +EXPORT_SYMBOL(baytrail_evaluate_sem);

Re: [PATCH V2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

On Tue, Nov 11, 2014 at 12:32:56PM +0100, Wolfram Sang wrote:
> On Tue, Sep 23, 2014 at 11:40:26AM -0700, David E. Box wrote:
> > This patch implements an I2C bus sharing mechanism between the host and platform
> > hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> > 
> > On these platforms access to the PMIC must be shared with platform hardware. The
> > hardware unit assumes full control of the I2C bus and the host must request
> > access through a special semaphore. Hardware control of the bus also makes it
> > necessary to disable runtime pm to avoid interfering with hardware transactions.
> 
> Can we foresee that other platforms will have similar mechanisms in the
> future?
>

Maybe one other platform. Unlikely there'll be any others. Okay on your
comments below.

Dave

> > +config I2C_BAYTRAIL_SEM
> 
> I2C_DESIGNWARE_BAYTRAIL_SEM
> 
> > +	tristate "Intel Baytrail I2C semaphore support"
> > +	depends on I2C_DESIGNWARE_PLATFORM
> > +	select I2C_DESIGNWARE_CORE
> 
> This select is already covered by I2C_DESIGNWARE_PLATFORM.
> 
> > +	select IOSF_MBI
> > +	help
> > +	  This driver enables host access to the PMIC I2C bus on select Intel
> > +	  BayTrail platforms using the X-Powers AXP288 PMIC. This driver is
> > +	  required for host access to the PMIC on these platforms. You should
> > +	  probably say Y if you have a BayTrail system, unless you know it uses
> > +	  a different PMIC. Otherwises critical PMIC functions, like charging,
> > +	  may not operate.
> > +
> > +	  This driver should be built as a m if I2C_DESIGNWARE_PLATFORM=m,
> > +	  and as y if I2C_DESIGNWARE_PLATFORM=y.
> 
> That shouldn't be the user's task to ensure. Please enforce this in the
> makefile. Check Documentation/kbuid/makefiles.txt, Section 3.3.
> 
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-baytrail-sem.c
> > @@ -0,0 +1,157 @@
> > +/*
> > + * Intel BayTrail PMIC I2C bus semaphore implementaion
> > + * Copyright (c) 2014, Intel Corporation.
> 
> Mika, can you have a look at the ACPI part here?
> 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> > index d66b6cb..13e0809 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.h
> > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > @@ -65,6 +65,8 @@
> >   * @ss_lcnt: standard speed LCNT value
> >   * @fs_hcnt: fast speed HCNT value
> >   * @fs_lcnt: fast speed LCNT value
> > + * has_hw_lock: true if bus access requires hardware lock
> > + * pm_runtime_disabled: true if pm runtime is disabled
> 
> Look closely. There is a difference to the entries above.
> 
> > @@ -123,3 +127,18 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
> >  extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
> >  extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
> >  extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
> > +
> > +#if IS_ENABLED(CONFIG_I2C_BAYTRAIL_SEM)
> > +extern int baytrail_i2c_acquire(struct dw_i2c_dev *dev);
> > +extern void baytrail_i2c_release(struct dw_i2c_dev *dev);
> > +extern void baytrail_evaluate_sem(struct dw_i2c_dev *dev);
> > +#define i2c_dw_acquire_ownership(dev) baytrail_i2c_acquire(dev)
> > +#define i2c_dw_release_ownership(dev) baytrail_i2c_release(dev)
> > +#define i2c_dw_eval_lock(dev) baytrail_evaluate_sem(dev)
> i2c_dw_test_ownership_support()?
> 
> That doesn't scale in case other platformts will need this. I could
> imagine a struct i2c_dw_ownership_ops() (or whatever name) which gets
> populated according to the matched device.
> 
> Thanks,
> 
>    Wolfram
> 

[PATCH V3 0/2] i2c-designware: Baytrail bus locking driver

[David E. Box]

Select Intel Baytrail platforms support PMIC's whose i2c bus may be controlled
exclusively by platform hardware. This patch set adds support for i2c bus
locking to the designware core and provides a driver module for managing
the lock on these platforms. Since the lock on these systems isn't enumerable
outside of the i2c platform driver, the locking functions are assigned at
compile time.

V2:     Moved semaphore detection out of dw platform driver
        Replaced function pointers with defined acquire/release functions in dw
                core. This helps elliminate the ifdefery in the dw platform driver.
        Use new has_hw_lock flag to check if the lock exists on a given bus.
        Use new pm_runtime_disabled flag to conditionally turnoff runtime pm
                in the dw platform driver.

V3:	Split lock support and driver into separate patches
	Change module build to bool. Platforms running without this driver cannot
		perform critical functions such as charging. Futhermore attempts by
		other drivers to access the i2c bus without a lock will hang the
		platform.
	Replaced has_hw_lock flag with acquire/release function pointers.
	Replaced acquire/release ifdef code with single i2c_dw_eval_lock_support()
		test for cleaner (if still undesireable) compile time scalability.
		Future Intel platforms will however continue to use the Baytrail
		driver.

David E. Box (2):
  i2c-designware: Add i2c bus locking support
  i2c-designware: Add Intel Baytrail PMIC I2C bus support

 drivers/i2c/busses/Kconfig                   |  12 +++
 drivers/i2c/busses/Makefile                  |   1 +
 drivers/i2c/busses/i2c-designware-baytrail.c | 155 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c     |  11 ++
 drivers/i2c/busses/i2c-designware-core.h     |  12 +++
 drivers/i2c/busses/i2c-designware-platdrv.c  |  18 +++-
 6 files changed, 204 insertions(+), 5 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c

-- 
1.9.1

[PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[David E. Box]

Adds support for acquiring and releasing a hardware bus lock in the i2c
designware core transfer function. This is needed for i2c bus controllers
that are shared with but not controlled by the kernel.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c    | 11 +++++++++++
 drivers/i2c/busses/i2c-designware-core.h    |  6 ++++++
 drivers/i2c/busses/i2c-designware-platdrv.c | 18 +++++++++++++-----
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 3c20e4b..377deeb 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -631,6 +631,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
 
+	if (dev->acquire_lock) {
+		ret = dev->acquire_lock(dev);
+		if (ret) {
+			dev_err(dev->dev, "couldn't acquire bus ownership\n");
+			goto done;
+		}
+	}
+
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
 		goto done;
@@ -676,6 +684,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = -EIO;
 
 done:
+	if (dev->release_lock)
+		dev->release_lock(dev);
+
 	pm_runtime_mark_last_busy(dev->dev);
 	pm_runtime_put_autosuspend(dev->dev);
 	mutex_unlock(&dev->lock);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d66b6cb..a472c91 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -65,6 +65,9 @@
  * @ss_lcnt: standard speed LCNT value
  * @fs_hcnt: fast speed HCNT value
  * @fs_lcnt: fast speed LCNT value
+ * @acquire_lock: function to acquire a hardware lock on the bus
+ * @release_lock: function to release a hardware lock on the bus
+ * @pm_runtime_disabled: true if pm runtime is disabled
  *
  * HCNT and LCNT parameters can be used if the platform knows more accurate
  * values than the one computed based only on the input clock frequency.
@@ -105,6 +108,9 @@ struct dw_i2c_dev {
 	u16			ss_lcnt;
 	u16			fs_hcnt;
 	u16			fs_lcnt;
+	int			(*acquire_lock)(struct dw_i2c_dev *dev);
+	void			(*release_lock)(struct dw_i2c_dev *dev);
+	bool			pm_runtime_disabled;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index a743115..afdff3b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -261,10 +261,16 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	i2c_dw_eval_lock_support(dev);
+
+	if (dev->pm_runtime_disabled) {
+		pm_runtime_forbid(&pdev->dev);
+	} else {
+		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+		pm_runtime_use_autosuspend(&pdev->dev);
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
 
 	return 0;
 }
@@ -314,7 +320,9 @@ static int dw_i2c_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	clk_prepare_enable(i_dev->clk);
-	i2c_dw_init(i_dev);
+
+	if (!i_dev->pm_runtime_disabled)
+		i2c_dw_init(i_dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH V3 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

This patch implements an I2C bus sharing mechanism between the host and platform
hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.

On these platforms access to the PMIC must be shared with platform hardware. The
hardware unit assumes full control of the I2C bus and the host must request
access through a special semaphore. Hardware control of the bus also makes it
necessary to disable runtime pm to avoid interfering with hardware transactions.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/i2c/busses/Kconfig                   |  12 +++
 drivers/i2c/busses/Makefile                  |   1 +
 drivers/i2c/busses/i2c-designware-baytrail.c | 155 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h     |   6 ++
 4 files changed, 174 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 917c358..d2bfd88 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -464,6 +464,18 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_DESIGNWARE_BAYTRAIL
+	bool "Intel Baytrail I2C semaphore support"
+	depends on I2C_DESIGNWARE_PLATFORM=y
+	select IOSF_MBI
+	help
+	  This driver enables host access to the PMIC I2C bus on select Intel
+	  BayTrail platforms using the X-Powers AXP288 PMIC. This driver is
+	  required for host access to the PMIC on these platforms. You should
+	  probably say Y if you have a BayTrail system, unless you know it uses
+	  a different PMIC. Otherwise critical PMIC functions, like charging,
+	  may not operate.
+
 config I2C_EFM32
 	tristate "EFM32 I2C controller"
 	depends on ARCH_EFM32 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d56c5..48fc23b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
+obj-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL)	+= i2c-designware-baytrail.o
 obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
 obj-$(CONFIG_I2C_EXYNOS5)	+= i2c-exynos5.o
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
new file mode 100644
index 0000000..ce80241
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -0,0 +1,155 @@
+/*
+ * Intel BayTrail PMIC I2C bus semaphore implementaion
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <asm/iosf_mbi.h>
+#include "i2c-designware-core.h"
+
+#define SEMAPHORE_TIMEOUT	100
+#define PUNIT_SEMAPHORE		0x7
+
+static unsigned long acquired;
+
+static int get_sem(struct device *dev, u32 *sem)
+{
+	u32 reg_val;
+	int ret;
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ, PUNIT_SEMAPHORE,
+			    &reg_val);
+	if (ret) {
+		dev_err(dev, "iosf failed to read punit semaphore\n");
+		return ret;
+	}
+
+	*sem = reg_val & 0x1;
+
+	return 0;
+}
+
+static void reset_semaphore(struct device *dev)
+{
+	u32 data;
+
+	if (iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+				PUNIT_SEMAPHORE, &data)) {
+		dev_err(dev, "iosf failed to reset punit semaphore during read\n");
+		return;
+	}
+
+	data = data & 0xfffffffe;
+	if (iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, data))
+		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
+}
+
+int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
+{
+	u32 sem = 0;
+	int ret;
+	unsigned long start, end;
+
+	if (!dev || !dev->dev)
+		return -ENODEV;
+
+	if (!dev->acquire_lock)
+		return 0;
+
+	/* host driver writes 0x2 to side band semaphore register */
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, 0x2);
+	if (ret) {
+		dev_err(dev->dev, "iosf punit semaphore request failed\n");
+		return ret;
+	}
+
+	/* host driver waits for bit 0 to be set in semaphore register */
+	start = jiffies;
+	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
+	while (!time_after(jiffies, end)) {
+		ret = get_sem(dev->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);
+	}
+
+	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
+	reset_semaphore(dev->dev);
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+		PUNIT_SEMAPHORE, &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;
+}
+EXPORT_SYMBOL(baytrail_i2c_acquire);
+
+void baytrail_i2c_release(struct dw_i2c_dev *dev)
+{
+	if (!dev || !dev->dev)
+		return;
+
+	if (!dev->acquire_lock)
+		return;
+
+	reset_semaphore(dev->dev);
+	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
+		jiffies_to_msecs(jiffies - acquired));
+}
+EXPORT_SYMBOL(baytrail_i2c_release);
+
+void i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
+{
+	acpi_status status;
+	unsigned long long shared_host = 0;
+	acpi_handle handle;
+
+	if (!dev || !dev->dev)
+		return;
+
+	handle = ACPI_HANDLE(dev->dev);
+	if (!handle)
+		return;
+
+	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
+
+	if (ACPI_FAILURE(status))
+		return;
+
+	if (shared_host) {
+		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+		dev->acquire_lock = baytrail_i2c_acquire;
+		dev->release_lock = baytrail_i2c_release;
+		dev->pm_runtime_disabled = true;
+	}
+}
+EXPORT_SYMBOL(i2c_dw_eval_lock_support);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Baytrail I2C Semaphore driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index a472c91..72ddfa8 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -129,3 +129,9 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+
+#if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
+extern void i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
+#else
+static inline void i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) { }
+#endif
-- 
1.9.1

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[Mika Westerberg]

On Mon, Dec 01, 2014 at 04:09:32PM -0800, David E. Box wrote:
> Adds support for acquiring and releasing a hardware bus lock in the i2c
> designware core transfer function. This is needed for i2c bus controllers
> that are shared with but not controlled by the kernel.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c    | 11 +++++++++++
>  drivers/i2c/busses/i2c-designware-core.h    |  6 ++++++
>  drivers/i2c/busses/i2c-designware-platdrv.c | 18 +++++++++++++-----
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 3c20e4b..377deeb 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -631,6 +631,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
>  
> +	if (dev->acquire_lock) {
> +		ret = dev->acquire_lock(dev);
> +		if (ret) {
> +			dev_err(dev->dev, "couldn't acquire bus ownership\n");
> +			goto done;

I wonder what happens now since you failed to acquire the lock...

> +		}
> +	}
> +
>  	ret = i2c_dw_wait_bus_not_busy(dev);
>  	if (ret < 0)
>  		goto done;
> @@ -676,6 +684,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	ret = -EIO;
>  
>  done:
> +	if (dev->release_lock)
> +		dev->release_lock(dev);

... but here you unconditionally release it?

Otherwise the patch looks good to me.

> +
>  	pm_runtime_mark_last_busy(dev->dev);
>  	pm_runtime_put_autosuspend(dev->dev);
>  	mutex_unlock(&dev->lock);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index d66b6cb..a472c91 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -65,6 +65,9 @@
>   * @ss_lcnt: standard speed LCNT value
>   * @fs_hcnt: fast speed HCNT value
>   * @fs_lcnt: fast speed LCNT value
> + * @acquire_lock: function to acquire a hardware lock on the bus
> + * @release_lock: function to release a hardware lock on the bus
> + * @pm_runtime_disabled: true if pm runtime is disabled
>   *
>   * HCNT and LCNT parameters can be used if the platform knows more accurate
>   * values than the one computed based only on the input clock frequency.
> @@ -105,6 +108,9 @@ struct dw_i2c_dev {
>  	u16			ss_lcnt;
>  	u16			fs_hcnt;
>  	u16			fs_lcnt;
> +	int			(*acquire_lock)(struct dw_i2c_dev *dev);
> +	void			(*release_lock)(struct dw_i2c_dev *dev);
> +	bool			pm_runtime_disabled;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index a743115..afdff3b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -261,10 +261,16 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  		return r;
>  	}
>  
> -	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	i2c_dw_eval_lock_support(dev);
> +
> +	if (dev->pm_runtime_disabled) {
> +		pm_runtime_forbid(&pdev->dev);
> +	} else {
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +		pm_runtime_use_autosuspend(&pdev->dev);
> +		pm_runtime_set_active(&pdev->dev);
> +		pm_runtime_enable(&pdev->dev);
> +	}
>  
>  	return 0;
>  }
> @@ -314,7 +320,9 @@ static int dw_i2c_resume(struct device *dev)
>  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>  
>  	clk_prepare_enable(i_dev->clk);
> -	i2c_dw_init(i_dev);
> +
> +	if (!i_dev->pm_runtime_disabled)
> +		i2c_dw_init(i_dev);
>  
>  	return 0;
>  }
> -- 
> 1.9.1

Re: [PATCH V3 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Mon, Dec 01, 2014 at 04:09:33PM -0800, David E. Box wrote:
> This patch implements an I2C bus sharing mechanism between the host and platform
> hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> 
> On these platforms access to the PMIC must be shared with platform hardware. The
> hardware unit assumes full control of the I2C bus and the host must request
> access through a special semaphore. Hardware control of the bus also makes it
> necessary to disable runtime pm to avoid interfering with hardware transactions.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/i2c/busses/Kconfig                   |  12 +++
>  drivers/i2c/busses/Makefile                  |   1 +
>  drivers/i2c/busses/i2c-designware-baytrail.c | 155 +++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h     |   6 ++
>  4 files changed, 174 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 917c358..d2bfd88 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -464,6 +464,18 @@ config I2C_DESIGNWARE_PCI
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-designware-pci.
>  
> +config I2C_DESIGNWARE_BAYTRAIL
> +	bool "Intel Baytrail I2C semaphore support"
> +	depends on I2C_DESIGNWARE_PLATFORM=y

Hmm, is there something preventing to compile this as module?

What comes to the driver itself, no objections from me.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[Jarkko Nikula]

Hi

On 12/02/2014 02:09 AM, David E. Box wrote:
> Adds support for acquiring and releasing a hardware bus lock in the i2c
> designware core transfer function. This is needed for i2c bus controllers
> that are shared with but not controlled by the kernel.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-core.c    | 11 +++++++++++
>   drivers/i2c/busses/i2c-designware-core.h    |  6 ++++++
>   drivers/i2c/busses/i2c-designware-platdrv.c | 18 +++++++++++++-----
>   3 files changed, 30 insertions(+), 5 deletions(-)
>
...
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index a743115..afdff3b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -261,10 +261,16 @@ static int dw_i2c_probe(struct platform_device *pdev)
>   		return r;
>   	}
>   
> -	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	i2c_dw_eval_lock_support(dev);
i2c_dw_eval_lock_support() is added in the next patch.
> +
> +	if (dev->pm_runtime_disabled) {
> +		pm_runtime_forbid(&pdev->dev);
> +	} else {
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +		pm_runtime_use_autosuspend(&pdev->dev);
> +		pm_runtime_set_active(&pdev->dev);
> +		pm_runtime_enable(&pdev->dev);
> +	}
>   
>   	return 0;
>   }
> @@ -314,7 +320,9 @@ static int dw_i2c_resume(struct device *dev)
>   	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>   
>   	clk_prepare_enable(i_dev->clk);
> -	i2c_dw_init(i_dev);
> +
> +	if (!i_dev->pm_runtime_disabled)
> +		i2c_dw_init(i_dev);
>   
Should there be similar conditional call or locking around i2c_dw_init() 
and i2c_dw_disable_int() also in dw_i2c_probe()?

-- 
Jarkko

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[David E. Box]

On Thu, Dec 04, 2014 at 09:59:11AM +0200, Jarkko Nikula wrote:
> Hi
> 
> On 12/02/2014 02:09 AM, David E. Box wrote:
> >Adds support for acquiring and releasing a hardware bus lock in the i2c
> >designware core transfer function. This is needed for i2c bus controllers
> >that are shared with but not controlled by the kernel.
> >
> >Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> >---
> >  drivers/i2c/busses/i2c-designware-core.c    | 11 +++++++++++
> >  drivers/i2c/busses/i2c-designware-core.h    |  6 ++++++
> >  drivers/i2c/busses/i2c-designware-platdrv.c | 18 +++++++++++++-----
> >  3 files changed, 30 insertions(+), 5 deletions(-)
> >
> ...
> >diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >index a743115..afdff3b 100644
> >--- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >@@ -261,10 +261,16 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  		return r;
> >  	}
> >-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >-	pm_runtime_use_autosuspend(&pdev->dev);
> >-	pm_runtime_set_active(&pdev->dev);
> >-	pm_runtime_enable(&pdev->dev);
> >+	i2c_dw_eval_lock_support(dev);
> i2c_dw_eval_lock_support() is added in the next patch.
> >+
> >+	if (dev->pm_runtime_disabled) {
> >+		pm_runtime_forbid(&pdev->dev);
> >+	} else {
> >+		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >+		pm_runtime_use_autosuspend(&pdev);
> >+		pm_runtime_set_active(&pdev->dev);
> >+		pm_runtime_enable(&pdev->dev);
> >+	}
> >  	return 0;
> >  }
> >@@ -314,7 +320,9 @@ static int dw_i2c_resume(struct device *dev)
> >  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> >  	clk_prepare_enable(i_dev->clk);
> >-	i2c_dw_init(i_dev);
> >+
> >+	if (!i_dev->pm_runtime_disabled)
> >+		i2c_dw_init(i_dev);
> Should there be similar conditional call or locking around
> i2c_dw_init() and i2c_dw_disable_int() also in dw_i2c_probe()?
> 

Timely reply. Around i2c_dw_init(), yes. I just discovered this as the source
of a recent hang that's occuring in the loop in __i2c_dw_enable().
The hange occurs very infrequently and only, so far, when not built in. A
block around i2c_dw_disable_int() would make sense as well as a precaution.

Dave

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[David E. Box]

On Wed, Dec 03, 2014 at 06:01:25PM +0200, Mika Westerberg wrote:
> On Mon, Dec 01, 2014 at 04:09:32PM -0800, David E. Box wrote:
> > Adds support for acquiring and releasing a hardware bus lock in the i2c
> > designware core transfer function. This is needed for i2c bus controllers
> > that are shared with but not controlled by the kernel.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c    | 11 +++++++++++
> >  drivers/i2c/busses/i2c-designware-core.h    |  6 ++++++
> >  drivers/i2c/busses/i2c-designware-platdrv.c | 18 +++++++++++++-----
> >  3 files changed, 30 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > index 3c20e4b..377deeb 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -631,6 +631,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	dev->abort_source = 0;
> >  	dev->rx_outstanding = 0;
> >  
> > +	if (dev->acquire_lock) {
> > +		ret = dev->acquire_lock(dev);
> > +		if (ret) {
> > +			dev_err(dev->dev, "couldn't acquire bus ownership\n");
> > +			goto done;
> 
> I wonder what happens now since you failed to acquire the lock...
> 
> > +		}
> > +	}
> > +
> >  	ret = i2c_dw_wait_bus_not_busy(dev);
> >  	if (ret < 0)
> >  		goto done;
> > @@ -676,6 +684,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	ret = -EIO;
> >  
> >  done:
> > +	if (dev->release_lock)
> > +		dev->release_lock(dev);
> 
> ... but here you unconditionally release it?
> 

Releasing the lock unconditionally isn't a problem, but it is an unnecessary
path should we fail to acquire the lock. I'll add another label to skip it.

Dave

Re: [PATCH V3 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

On Wed, Dec 03, 2014 at 06:10:46PM +0200, Mika Westerberg wrote:
> On Mon, Dec 01, 2014 at 04:09:33PM -0800, David E. Box wrote:
> > This patch implements an I2C bus sharing mechanism between the host and platform
> > hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> > 
> > On these platforms access to the PMIC must be shared with platform hardware. The
> > hardware unit assumes full control of the I2C bus and the host must request
> > access through a special semaphore. Hardware control of the bus also makes it
> > necessary to disable runtime pm to avoid interfering with hardware transactions.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >  drivers/i2c/busses/Kconfig                   |  12 +++
> >  drivers/i2c/busses/Makefile                  |   1 +
> >  drivers/i2c/busses/i2c-designware-baytrail.c | 155 +++++++++++++++++++++++++++
> >  drivers/i2c/busses/i2c-designware-core.h     |   6 ++
> >  4 files changed, 174 insertions(+)
> >  create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 917c358..d2bfd88 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -464,6 +464,18 @@ config I2C_DESIGNWARE_PCI
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-designware-pci.
> >  
> > +config I2C_DESIGNWARE_BAYTRAIL
> > +	bool "Intel Baytrail I2C semaphore support"
> > +	depends on I2C_DESIGNWARE_PLATFORM=y
> 
> Hmm, is there something preventing to compile this as module?
> 

There were load order issues. This driver is really a support module for the
platform driver. I think Wolfram suggested this earlier but I didn't realize it
until now. The proper solution is to link it conditionally with
i2c-designware-platform.

Dave

Re: [PATCH V3 0/2] i2c-designware: Baytrail bus locking driver

[Shinya Kuribayashi]

On 14/12/02 9:09, David E. Box wrote:
> Select Intel Baytrail platforms support PMIC's whose i2c bus may be controlled
> exclusively by platform hardware. This patch set adds support for i2c bus
> locking to the designware core and provides a driver module for managing
> the lock on these platforms. Since the lock on these systems isn't enumerable
> outside of the i2c platform driver, the locking functions are assigned at
> compile time.

Have you ever look into the hwspinlock framework?  It seems to me that
such an exclusive operation between CPUs and external hardware blocks
is exactly what hwspinlock is for.  Further more hwspinlock takes care
of exclusiveness between SMP cores.

Ideally I would expect i2c-designware to have hwspinlock lock/unlock
API calls on one I2C transaction, but it's not necessarily the case.
Introducing such platform hooks (acquire_lock and release_lock) and
keeping actual exclusive operataion outside the driver might be good
for various usecases/platforms.

   Shinya

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[Wolfram Sang]

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

Hi Dave,

> Timely reply. Around i2c_dw_init(), yes. I just discovered this as the source
> of a recent hang that's occuring in the loop in __i2c_dw_enable().
> The hange occurs very infrequently and only, so far, when not built in. A
> block around i2c_dw_disable_int() would make sense as well as a precaution.

Any news on this or on a V4 of this series?

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V3 1/2] i2c-designware: Add i2c bus locking support

[David E. Box]

Hi,

Expect something tonight should the latest tests run okay. I needed to include
an EPROBE_DEFER to address the unavailibity of the pci driver needed in order to
request the lock during probe of the i2c device. This due to the lock now being
requested during probe because of the hang.

Dave

On Tue, Jan 13, 2015 at 10:48:33AM +0100, Wolfram Sang wrote:
> Hi Dave,
> 
> > Timely reply. Around i2c_dw_init(), yes. I just discovered this as the source
> > of a recent hang that's occuring in the loop in __i2c_dw_enable().
> > The hange occurs very infrequently and only, so far, when not built in. A
> > block around i2c_dw_disable_int() would make sense as well as a precaution.
> 
> Any news on this or on a V4 of this series?
> 
> Thanks,
> 
>    Wolfram
> 

[PATCH V4 0/2] i2c-designware: Add Intel Baytrail pmic i2c bus support

[David E. Box]

V4:	- Added bus lock to i2c_dw_init() as suggested by Jarrko Nikula
	  <jarkko.nikula@linux.intel.com>. Addresses infrequent hang that was
	  seen occuring during probe.
	- Added label in i2c_dw_xfer() to bypass unnecessary lock release when
	  acquire fails. Suggested by Mika.
	- Changed make to build driver as composite addition to
	  i2c-designware-platform. Driver can be module or built-in following
	  the platform driver selection. Tested as both.
	- Added EPROBE_DEFER return to baytrail definition of i2c_dw_eval_lock()
	  to defer probe if iosf_mbi driver (required for acquring bus lock) is
	  not available.

V3:	- Split lock support and driver into separate patches
	- Change module build to bool. Platforms running without this driver
	  cannot perform critical functions such as charging. Furthermore attempts
	  by other drivers to access the i2c bus without a lock will hang the
	  platform.
	- Replaced has_hw_lock flag with acquire/release function pointers.
	- Replaced acquire/release ifdef code with single
	  i2c_dw_eval_lock_support() test for cleaner (if still undesirable)
	  compile time scalability. Future Intel platforms will however continue
	  to use the Baytrail driver.

V2:	- Moved semaphore detection out of dw platform driver
	- Replaced function pointers with defined acquire/release functions in
	  dw core. This helps eliminate the ifdefery in the dw platform driver.
	- Use new has_hw_lock flag to check if the lock exists on a given bus.
	- Use new pm_runtime_disabled flag to conditionally turnoff runtime pm
	  in the dw platform driver.

David E. Box (2):
  i2c-designware: Add i2c bus locking support
  i2c-designware: Add Intel Baytrail PMIC I2C bus support

 drivers/i2c/busses/Kconfig                   |  11 ++
 drivers/i2c/busses/Makefile                  |   1 +
 drivers/i2c/busses/i2c-designware-baytrail.c | 160 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c     |  26 +++++
 drivers/i2c/busses/i2c-designware-core.h     |  12 ++
 drivers/i2c/busses/i2c-designware-platdrv.c  |  20 +++-
 6 files changed, 225 insertions(+), 5 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c

-- 
1.9.1

[PATCH V4 1/2] i2c-designware: Add i2c bus locking support

[David E. Box]

Adds support for acquiring and releasing a hardware bus lock in the i2c
designware core transfer function. This is needed for i2c bus controllers
that are shared with but not controlled by the kernel.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 26 ++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 3c20e4b..3d3ac4d 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -289,6 +289,15 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	u32 hcnt, lcnt;
 	u32 reg;
 	u32 sda_falling_time, scl_falling_time;
+	int ret;
+
+	if (dev->acquire_lock) {
+		ret = dev->acquire_lock(dev);
+		if (ret) {
+			dev_err(dev->dev, "couldn't acquire bus ownership\n");
+			return ret;
+		}
+	}
 
 	input_clock_khz = dev->get_clk_rate_khz(dev);
 
@@ -302,6 +311,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
 		dev_err(dev->dev, "Unknown Synopsys component type: "
 			"0x%08x\n", reg);
+		if (dev->release_lock)
+			dev->release_lock(dev);
 		return -ENODEV;
 	}
 
@@ -368,6 +379,9 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 
 	/* configure the i2c master */
 	dw_writel(dev, dev->master_cfg , DW_IC_CON);
+
+	if (dev->release_lock)
+		dev->release_lock(dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(i2c_dw_init);
@@ -631,6 +645,14 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
 
+	if (dev->acquire_lock) {
+		ret = dev->acquire_lock(dev);
+		if (ret) {
+			dev_err(dev->dev, "couldn't acquire bus ownership\n");
+			goto done_nolock;
+		}
+	}
+
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
 		goto done;
@@ -676,6 +698,10 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = -EIO;
 
 done:
+	if (dev->release_lock)
+		dev->release_lock(dev);
+
+done_nolock:
 	pm_runtime_mark_last_busy(dev->dev);
 	pm_runtime_put_autosuspend(dev->dev);
 	mutex_unlock(&dev->lock);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d66b6cb..a472c91 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -65,6 +65,9 @@
  * @ss_lcnt: standard speed LCNT value
  * @fs_hcnt: fast speed HCNT value
  * @fs_lcnt: fast speed LCNT value
+ * @acquire_lock: function to acquire a hardware lock on the bus
+ * @release_lock: function to release a hardware lock on the bus
+ * @pm_runtime_disabled: true if pm runtime is disabled
  *
  * HCNT and LCNT parameters can be used if the platform knows more accurate
  * values than the one computed based only on the input clock frequency.
@@ -105,6 +108,9 @@ struct dw_i2c_dev {
 	u16			ss_lcnt;
 	u16			fs_hcnt;
 	u16			fs_lcnt;
+	int			(*acquire_lock)(struct dw_i2c_dev *dev);
+	void			(*release_lock)(struct dw_i2c_dev *dev);
+	bool			pm_runtime_disabled;
 };
 
 #define ACCESS_SWAP		0x00000001
-- 
1.9.1

[PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

This patch implements an I2C bus sharing mechanism between the host and platform
hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.

On these platforms access to the PMIC must be shared with platform hardware. The
hardware unit assumes full control of the I2C bus and the host must request
access through a special semaphore. Hardware control of the bus also makes it
necessary to disable runtime pm to avoid interfering with hardware transactions.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/i2c/busses/Kconfig                   |  11 ++
 drivers/i2c/busses/Makefile                  |   1 +
 drivers/i2c/busses/i2c-designware-baytrail.c | 160 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h     |   6 +
 drivers/i2c/busses/i2c-designware-platdrv.c  |  20 +++-
 5 files changed, 193 insertions(+), 5 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 917c358..9a83c46 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -464,6 +464,17 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_DESIGNWARE_BAYTRAIL
+	bool "Intel Baytrail I2C semaphore support"
+	depends on I2C_DESIGNWARE_PLATFORM
+	select IOSF_MBI
+	help
+	  This driver enables managed host access to the PMIC I2C bus on select
+	  Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
+	  the host to request uninterrupted access to the PMIC's I2C bus from
+	  the platform firmware controlling it. You should say Y if running on
+	  a BayTrail system using the AXP288.
+
 config I2C_EFM32
 	tristate "EFM32 I2C controller"
 	depends on ARCH_EFM32 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d56c5..15b2965 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
 obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
+i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
 obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
new file mode 100644
index 0000000..5f1ff4c
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -0,0 +1,160 @@
+/*
+ * Intel BayTrail PMIC I2C bus semaphore implementaion
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <asm/iosf_mbi.h>
+#include "i2c-designware-core.h"
+
+#define SEMAPHORE_TIMEOUT	100
+#define PUNIT_SEMAPHORE		0x7
+
+static unsigned long acquired;
+
+static int get_sem(struct device *dev, u32 *sem)
+{
+	u32 reg_val;
+	int ret;
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ, PUNIT_SEMAPHORE,
+			    &reg_val);
+	if (ret) {
+		dev_err(dev, "iosf failed to read punit semaphore\n");
+		return ret;
+	}
+
+	*sem = reg_val & 0x1;
+
+	return 0;
+}
+
+static void reset_semaphore(struct device *dev)
+{
+	u32 data;
+
+	if (iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+				PUNIT_SEMAPHORE, &data)) {
+		dev_err(dev, "iosf failed to reset punit semaphore during read\n");
+		return;
+	}
+
+	data = data & 0xfffffffe;
+	if (iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, data))
+		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
+}
+
+int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
+{
+	u32 sem = 0;
+	int ret;
+	unsigned long start, end;
+
+	if (!dev || !dev->dev)
+		return -ENODEV;
+
+	if (!dev->acquire_lock)
+		return 0;
+
+	/* host driver writes 0x2 to side band semaphore register */
+	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
+				 PUNIT_SEMAPHORE, 0x2);
+	if (ret) {
+		dev_err(dev->dev, "iosf punit semaphore request failed\n");
+		return ret;
+	}
+
+	/* host driver waits for bit 0 to be set in semaphore register */
+	start = jiffies;
+	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
+	while (!time_after(jiffies, end)) {
+		ret = get_sem(dev->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);
+	}
+
+	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
+	reset_semaphore(dev->dev);
+
+	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
+		PUNIT_SEMAPHORE, &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;
+}
+EXPORT_SYMBOL(baytrail_i2c_acquire);
+
+void baytrail_i2c_release(struct dw_i2c_dev *dev)
+{
+	if (!dev || !dev->dev)
+		return;
+
+	if (!dev->acquire_lock)
+		return;
+
+	reset_semaphore(dev->dev);
+	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
+		jiffies_to_msecs(jiffies - acquired));
+}
+EXPORT_SYMBOL(baytrail_i2c_release);
+
+int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
+{
+	acpi_status status;
+	unsigned long long shared_host = 0;
+	acpi_handle handle;
+
+	if (!dev || !dev->dev)
+		return 0;
+
+	handle = ACPI_HANDLE(dev->dev);
+	if (!handle)
+		return 0;
+
+	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
+
+	if (ACPI_FAILURE(status))
+		return 0;
+
+	if (shared_host) {
+		dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+		dev->acquire_lock = baytrail_i2c_acquire;
+		dev->release_lock = baytrail_i2c_release;
+		dev->pm_runtime_disabled = true;
+	}
+
+	if (!iosf_mbi_available())
+		return -EPROBE_DEFER;
+
+	return 0;
+}
+EXPORT_SYMBOL(i2c_dw_eval_lock_support);
+
+MODULE_AUTHOR("David E. Box <david.e.box@linux.intel.com>");
+MODULE_DESCRIPTION("Baytrail I2C Semaphore driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index a472c91..92ceb65 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -129,3 +129,9 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+
+#if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
+extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
+#else
+static inline int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) { return 0; }
+#endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index a743115..ac32414 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -199,6 +199,10 @@ static int dw_i2c_probe(struct platform_device *pdev)
 			clk_freq = pdata->i2c_scl_freq;
 	}
 
+	r = i2c_dw_eval_lock_support(dev);
+	if (r)
+		return r;
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
@@ -261,10 +265,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return r;
 	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	if (dev->pm_runtime_disabled) {
+		pm_runtime_forbid(&pdev->dev);
+	} else {
+		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+		pm_runtime_use_autosuspend(&pdev->dev);
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
 
 	return 0;
 }
@@ -314,7 +322,9 @@ static int dw_i2c_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	clk_prepare_enable(i_dev->clk);
-	i2c_dw_init(i_dev);
+
+	if (!i_dev->pm_runtime_disabled)
+		i2c_dw_init(i_dev);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH V4 1/2] i2c-designware: Add i2c bus locking support

[Mika Westerberg]

On Thu, Jan 15, 2015 at 01:12:16AM -0800, David E. Box wrote:
> Adds support for acquiring and releasing a hardware bus lock in the i2c
> designware core transfer function. This is needed for i2c bus controllers
> that are shared with but not controlled by the kernel.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Looks good.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Thu, Jan 15, 2015 at 01:12:17AM -0800, David E. Box wrote:
> This patch implements an I2C bus sharing mechanism between the host and platform
> hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> 
> On these platforms access to the PMIC must be shared with platform hardware. The
> hardware unit assumes full control of the I2C bus and the host must request
> access through a special semaphore. Hardware control of the bus also makes it
> necessary to disable runtime pm to avoid interfering with hardware transactions.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

One comment, though:

> ---
>  drivers/i2c/busses/Kconfig                   |  11 ++
>  drivers/i2c/busses/Makefile                  |   1 +
>  drivers/i2c/busses/i2c-designware-baytrail.c | 160 +++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h     |   6 +
>  drivers/i2c/busses/i2c-designware-platdrv.c  |  20 +++-
>  5 files changed, 193 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 917c358..9a83c46 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -464,6 +464,17 @@ config I2C_DESIGNWARE_PCI
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-designware-pci.
>  
> +config I2C_DESIGNWARE_BAYTRAIL
> +	bool "Intel Baytrail I2C semaphore support"

It would be nice if it was possible to compile this as a module.

> +	depends on I2C_DESIGNWARE_PLATFORM
> +	select IOSF_MBI
> +	help
> +	  This driver enables managed host access to the PMIC I2C bus on select
> +	  Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
> +	  the host to request uninterrupted access to the PMIC's I2C bus from
> +	  the platform firmware controlling it. You should say Y if running on
> +	  a BayTrail system using the AXP288.
> +

Re: [PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[David E. Box]

Hi Mika,

On Thu, Jan 22, 2015 at 04:28:19PM +0200, Mika Westerberg wrote:
> On Thu, Jan 15, 2015 at 01:12:17AM -0800, David E. Box wrote:
> > This patch implements an I2C bus sharing mechanism between the host and platform
> > hardware on select Intel BayTrail SoC platforms using the X-Powers AXP288 PMIC.
> > 
> > On these platforms access to the PMIC must be shared with platform hardware. The
> > hardware unit assumes full control of the I2C bus and the host must request
> > access through a special semaphore. Hardware control of the bus also makes it
> > necessary to disable runtime pm to avoid interfering with hardware transactions.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> One comment, though:
> 
> > ---
> >  drivers/i2c/busses/Kconfig                   |  11 ++
> >  drivers/i2c/busses/Makefile                  |   1 +
> >  drivers/i2c/busses/i2c-designware-baytrail.c | 160 +++++++++++++++++++++++++++
> >  drivers/i2c/busses/i2c-designware-core.h     |   6 +
> >  drivers/i2c/busses/i2c-designware-platdrv.c  |  20 +++-
> >  5 files changed, 193 insertions(+), 5 deletions(-)
> >  create mode 100644 drivers/i2c/busses/i2c-designware-baytrail.c
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 917c358..9a83c46 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -464,6 +464,17 @@ config I2C_DESIGNWARE_PCI
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-designware-pci.
> >  
> > +config I2C_DESIGNWARE_BAYTRAIL
> > +	bool "Intel Baytrail I2C semaphore support"
> 
> It would be nice if it was possible to compile this as a module.

In the makefile the driver is now part of the composite object for
I2C_DESIGNWARE_PLATFORM. So if selected it will compile as the platform driver
is compiled. This makes the most sense since the linking doesn't work right if
both drivers aren't built the same. This is really an extension to the platform
driver anyway and not standalone code.

Dave

Re: [PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Mika Westerberg]

On Thu, Jan 22, 2015 at 12:48:51PM -0800, David E. Box wrote:
> > It would be nice if it was possible to compile this as a module.
> 
> In the makefile the driver is now part of the composite object for
> I2C_DESIGNWARE_PLATFORM. So if selected it will compile as the platform driver
> is compiled. This makes the most sense since the linking doesn't work right if
> both drivers aren't built the same. This is really an extension to the platform
> driver anyway and not standalone code.

Makes sense. Thanks for the explanation.

Re: [PATCH V4 2/2] i2c-designware: Add Intel Baytrail PMIC I2C bus support

[Wolfram Sang]

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


Mostly good, except one thing:

> +config I2C_DESIGNWARE_BAYTRAIL
> +	bool "Intel Baytrail I2C semaphore support"
> +	depends on I2C_DESIGNWARE_PLATFORM
> +	select IOSF_MBI

This needs some dependency on something Baytrail specific. Otherwise, it
causes lots of build errors:

With ARM allnoconfig, I can select this option but there are no headers
for iosf.

With x86_64_defconfig and disabled ACPI, I can select this option but it
fails over "implicit declaration of function ‘acpi_evaluate_integer’".

An incremental patch will do for me, the rest looks good.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V4 0/2] i2c-designware: Add Intel Baytrail pmic i2c bus support

[Wolfram Sang]

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

On Thu, Jan 15, 2015 at 01:12:15AM -0800, David E. Box wrote:
> V4:	- Added bus lock to i2c_dw_init() as suggested by Jarrko Nikula
> 	  <jarkko.nikula@linux.intel.com>. Addresses infrequent hang that was
> 	  seen occuring during probe.
> 	- Added label in i2c_dw_xfer() to bypass unnecessary lock release when
> 	  acquire fails. Suggested by Mika.
> 	- Changed make to build driver as composite addition to
> 	  i2c-designware-platform. Driver can be module or built-in following
> 	  the platform driver selection. Tested as both.
> 	- Added EPROBE_DEFER return to baytrail definition of i2c_dw_eval_lock()
> 	  to defer probe if iosf_mbi driver (required for acquring bus lock) is
> 	  not available.

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]