i2c-designware: Intel BayTrail PMIC I2C bus support

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-VuQAYsv1563Yd54FQh9/[email protected]>

---
  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

drivers/i2c/busses/Kconfig +0 -0

index 2ac87fa..672ef23 100644

drivers/i2c/busses/Kconfig +9 -0

+	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

drivers/i2c/busses/Makefile +0 -0

index 49bf07e..33d62d1 100644

drivers/i2c/busses/Makefile +0 -0

  i2c-designware-pci-objs := i2c-designware-pcidrv.o
  obj-$(CONFIG_I2C_EFM32)		+= i2c-efm32.o
  obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o

drivers/i2c/busses/i2c-designware-core.h +14 -0

index d66b6cb..a2b72f4 100644
@@ -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

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

index f9b1dec..f86c285 100644
@@ -52,6 +52,32 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
  	return clk_get_rate(dev->clk)/1000;

drivers/i2c/busses/i2c-designware-platdrv.c +41 -2

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

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

  	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;
  }
 

drivers/i2c/busses/i2c-shared-controller.c new file +5 -0

+ * 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.

drivers/i2c/busses/i2c-shared-controller.c new file +41 -0

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

drivers/i2c/busses/i2c-shared-controller.c new file +20 -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;
+}

drivers/i2c/busses/i2c-designware-platdrv.c static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev) · line 63

Quoted 1 line

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

+		err = dev->acquire_ownership(dev->dev);

Have you considered using hwspinlocks instead?

Quoted 16 lines

drivers/i2c/busses/i2c-designware-platdrv.c +12 -0

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

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 280

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c +7 -0

+	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?

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_resume(struct device *dev) · line 340

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c +4 -1

@@ -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?

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-VuQAYsv1563Yd54FQh9/[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 2ac87fa..672ef23 100644
@@ -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.

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.

Quoted 3 lines · On Tue, 16 Sep 2014 12:44:49 +0300 Mika Westerberg…

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

Quoted 7 lines · 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.

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.

Quoted 19 lines

Signed-off-by David E. Box <[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 2ac87fa..672ef23 100644
@@ -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.

drivers/i2c/busses/Kconfig config I2C_DESIGNWARE_PCI · line 452

Quoted 10 lines

drivers/i2c/busses/Kconfig +9 -0

 
+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.

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

drivers/i2c/busses/i2c-designware-platdrv.c +15 -0

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

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-VuQAYsv1563Yd54FQh9/[email protected]>

Hi David,

Quoted 8 lines · 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.

Quoted 1 line

Signed-off-by David E. Box <[email protected]>

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?

Hi Maxime,

Quoted 13 lines · Maxime Ripard

Hi David,

Quoted 9 lines · 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-VuQAYsv1563Yd54FQh9/[email protected]>

Quoted 3 lines

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.

Hi Maxime,

Quoted 37 lines · Maxime Ripard

Hi David,

Quoted 16 lines · David E. Box

Hi Maxime,

Quoted 13 lines · Maxime Ripard

Hi David,

Quoted 9 lines · 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-VuQAYsv1563Yd54FQh9/[email protected]>

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

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.

+config I2C_BAYTRAIL_SEM

Quoted 3 lines · 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.

Quoted 4 lines

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?

Quoted 1 line

+config I2C_BAYTRAIL_SEM

I2C_DESIGNWARE_BAYTRAIL_SEM

drivers/i2c/busses/Kconfig config I2C_DESIGNWARE_PCI · line 447

Quoted 3 lines

drivers/i2c/busses/Kconfig +3 -0

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

This select is already covered by I2C_DESIGNWARE_PLATFORM.

drivers/i2c/busses/Kconfig config I2C_DESIGNWARE_PCI · line 458

Quoted 11 lines

drivers/i2c/busses/Kconfig +11 -0

+	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.

drivers/i2c/busses/i2c-baytrail-sem.c line 3

Quoted 6 lines

drivers/i2c/busses/i2c-baytrail-sem.c new file +3 -0

@@ -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?

drivers/i2c/busses/i2c-designware-core.h line 69

Quoted 10 lines

drivers/i2c/busses/i2c-designware-core.h +2 -0

index d66b6cb..13e0809 100644
@@ -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.

drivers/i2c/busses/i2c-designware-core.h extern void i2c_dw_disable(struct dw_i2c_dev *dev); · line 137

Quoted 12 lines

drivers/i2c/busses/i2c-designware-core.h +8 -0

@@ -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

drivers/i2c/busses/i2c-baytrail-sem.c new file +8 -0

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

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 <[email protected]>

---
 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(-)

drivers/i2c/busses/i2c-designware-core.c +0 -0

index 3c20e4b..377deeb 100644

drivers/i2c/busses/i2c-designware-core.c +1 -0

+
 	pm_runtime_mark_last_busy(dev->dev);
 	pm_runtime_put_autosuspend(dev->dev);
 	mutex_unlock(&dev->lock);

drivers/i2c/busses/i2c-designware-core.h +6 -0

index d66b6cb..a472c91 100644
@@ -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

drivers/i2c/busses/i2c-designware-platdrv.c +13 -5

index a743115..afdff3b 100644
@@ -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;
 }

Signature · 1 line

1.9.1

Quoted 16 lines · 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-VuQAYsv1563Yd54FQh9/[email protected]>

---
 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(-)

drivers/i2c/busses/i2c-designware-core.c +0 -0

index 3c20e4b..377deeb 100644

drivers/i2c/busses/i2c-designware-core.c i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) · line 638

Quoted 8 lines

drivers/i2c/busses/i2c-designware-core.c +5 -0

@@ -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...

Quoted 9 lines

drivers/i2c/busses/i2c-designware-core.c +3 -0

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

drivers/i2c/busses/i2c-designware-core.c i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) · line 688

Quoted 3 lines

drivers/i2c/busses/i2c-designware-core.c +2 -0

 done:
+	if (dev->release_lock)
+		dev->release_lock(dev);

... but here you unconditionally release it?

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-VuQAYsv1563Yd54FQh9/[email protected]>

---
  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(-)

drivers/i2c/busses/i2c-designware-platdrv.c +9 -0

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

Hi

Quoted 11 lines · 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 <[email protected]>

---
 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(-)

...

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 263

Quoted 12 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -4

index a743115..afdff3b 100644
@@ -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.

Quoted 12 lines

drivers/i2c/busses/i2c-designware-platdrv.c +9 -0

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

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_resume(struct device *dev) · line 325

Quoted 6 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -1

 	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.

Hi Dave,

Quoted 4 lines

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

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 <[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 917c358..d2bfd88 100644

Quoted 25 lines · 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-VuQAYsv1563Yd54FQh9/[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 917c358..d2bfd88 100644
@@ -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.
 

drivers/i2c/busses/Kconfig config I2C_DESIGNWARE_PCI · line 469

Quoted 3 lines

drivers/i2c/busses/Kconfig +3 -0

+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?

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.

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-VuQAYsv1563Yd54FQh9/[email protected]>

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-VuQAYsv1563Yd54FQh9/[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 917c358..9a83c46 100644

drivers/i2c/busses/Kconfig +9 -0

+	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.
+

Quoted 9 lines · 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 <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

One comment, though:

Quoted 17 lines

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 917c358..9a83c46 100644
@@ -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.
 

drivers/i2c/busses/Kconfig config I2C_DESIGNWARE_PCI · line 468

Quoted 2 lines

drivers/i2c/busses/Kconfig +2 -0

+config I2C_DESIGNWARE_BAYTRAIL
+	bool "Intel Baytrail I2C semaphore support"

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

Quoted 1 line

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.

V4: - Added bus lock to i2c_dw_init() as suggested by Jarrko Nikula
<jarkko.nikula-VuQAYsv1563Yd54FQh9/[email protected]>. 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.