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
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
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,
+ ®_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 <[email protected]>");
+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
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(-)
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
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 <[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(-)
>
> 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
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 <[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
>
> 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 <[email protected]>
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 <[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(-)
>
...
> 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
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 <[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(-)
> >
> ...
> >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
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 <[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(-)
> >
> > 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
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 <[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
> >
> > 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
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