Message ID | 1411497626-7984-1-git-send-email-david.e.box@linux.intel.com |
---|---|
State | Changes Requested |
Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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
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); -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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!
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, + ®_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; }
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