Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
On Fri, 12 Sep 2014 10:36:07 -0700
"David E. Box" wrote:
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +extern int i2c_acquire_ownership(struct device *dev);
> +extern int i2c_release_ownership(struct device *dev);
> +#endif
You can just have the prototypes anyway - no need for more ifdefs than
required
> +#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> +int i2c_shared_controller_xfer(struct i2c_adapter *adap, struct i2c_msg
> msgs[],
> + int num)
> +{
> + struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> + int err;
> +
> + if (dev->shared_host) {
> + err = dev->acquire_ownership(dev->dev);
> + if (!err) {
> + err = i2c_dw_xfer(adap, msgs, num);
> + dev->release_ownership(dev->dev);
> + } else
> + dev_WARN(dev->dev, "couldnt acquire ownership\n");
> +
> + return err;
> + } else
> + return i2c_dw_xfer(adap, msgs, num);
> +}
> +
> +static struct i2c_algorithm i2c_sc_algo = {
> + .master_xfer= i2c_shared_controller_xfer,
> + .functionality = i2c_dw_func,
> +};
> +#endif
I think this might be a lot cleaner if you put these pieces as functions
into i2c-designware-sem.c or a similar file and made the methods NULL
functions in the header in the case it's not supported ? Most of the
ifdeffery would then vanish into the extra file and keep the core code
cleaner ?
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
On 2014/9/16 17:44, Mika Westerberg wrote: > On Fri, Sep 12, 2014 at 10:36:07AM -0700, David E. Box wrote: >> This patch implements an I2C bus sharing mechanism between the host and >> platform >> hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC. >> >> On these platforms access to the PMIC must be shared with platform hardware. >> The >> hardware unit assumes full control of the I2C bus and the host must request >> access through a special semaphore. Hardware control of the bus also makes it >> necessary to disable runtime pm to avoid interfering with hardware >> transactions. > > Is this because we need to access the PMIC from host as well? I mean > from some PMIC driver (which driver btw)? > > Otherwise it would be best to just detect _SEM and return -ENODEV. On some Baytrail platform, one I2C port is shared between CPU and Punit, and Punit access is out the control of CPU, so 1. need to keep this shared I2C port always enabled, that is, no runtime pm, in case Punit wants to access it. 2. need the semaphore mechanism to access i2c. > >> >> Signed-off-by: David E. Box >> --- >> drivers/i2c/busses/Kconfig | 10 +++ >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-designware-core.h| 14 >> drivers/i2c/busses/i2c-designware-platdrv.c | 78 +++-- >> drivers/i2c/busses/i2c-shared-controller.c | 101 >> >> 5 files changed, 200 insertions(+), 4 deletions(-) >> create mode 100644 drivers/i2c/busses/i2c-shared-controller.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 2ac87fa..672ef23 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI >>This driver can also be built as a module. If so, the module >>will be called i2c-designware-pci. >> >> +config I2C_SHARED_CONTROLLER >> +tristate "Intel Baytrail PMIC shared I2C bus support" >> +depends on ACPI >> +select IOSF_MBI >> +select I2C_DESIGNWARE_CORE >> +help >> + This driver enables shared access to the PMIC I2C bus on select Intel >> + BayTrail platforms using the XPower AXP288 PMIC. This driver is >> + required for host access to the PMIC on these platforms. > > Can't we detect this runtime in the i2c-designware-platdrv.c code so > that you look (in the ACPI part of the driver) for _SEM and in that case > change the xfer function behaviour a bit to return -EBUSY or whatever? > > Without this horrible #ifdeffery. not necessary to have #ifdef, really. Thanks, -Aubrey > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
On Tue, Sep 16, 2014 at 03:53:27AM -0700, Jacob Pan wrote: > On Tue, 16 Sep 2014 12:44:49 +0300 > Mika Westerberg wrote: > > > Is this because we need to access the PMIC from host as well? I mean > > from some PMIC driver (which driver btw)? > > > This is used by the X-Powers PMIC. > https://lkml.org/lkml/2014/9/11/1016 Ok, thanks Jacob. So just returning -ENODEV is out of the question :-( -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
On Tue, 16 Sep 2014 12:44:49 +0300 Mika Westerberg wrote: > Is this because we need to access the PMIC from host as well? I mean > from some PMIC driver (which driver btw)? > This is used by the X-Powers PMIC. https://lkml.org/lkml/2014/9/11/1016 > Otherwise it would be best to just detect _SEM and return -ENODEV. Thanks, Jacob -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
On Fri, Sep 12, 2014 at 10:36:07AM -0700, David E. Box wrote: > This patch implements an I2C bus sharing mechanism between the host and > platform > hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC. > > On these platforms access to the PMIC must be shared with platform hardware. > The > hardware unit assumes full control of the I2C bus and the host must request > access through a special semaphore. Hardware control of the bus also makes it > necessary to disable runtime pm to avoid interfering with hardware > transactions. Is this because we need to access the PMIC from host as well? I mean from some PMIC driver (which driver btw)? Otherwise it would be best to just detect _SEM and return -ENODEV. > > Signed-off-by: David E. Box > --- > drivers/i2c/busses/Kconfig | 10 +++ > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-designware-core.h| 14 > drivers/i2c/busses/i2c-designware-platdrv.c | 78 +++-- > drivers/i2c/busses/i2c-shared-controller.c | 101 > > 5 files changed, 200 insertions(+), 4 deletions(-) > create mode 100644 drivers/i2c/busses/i2c-shared-controller.c > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 2ac87fa..672ef23 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI > This driver can also be built as a module. If so, the module > will be called i2c-designware-pci. > > +config I2C_SHARED_CONTROLLER > + tristate "Intel Baytrail PMIC shared I2C bus support" > + depends on ACPI > + select IOSF_MBI > + select I2C_DESIGNWARE_CORE > + help > + This driver enables shared access to the PMIC I2C bus on select Intel > + BayTrail platforms using the XPower AXP288 PMIC. This driver is > + required for host access to the PMIC on these platforms. Can't we detect this runtime in the i2c-designware-platdrv.c code so that you look (in the ACPI part of the driver) for _SEM and in that case change the xfer function behaviour a bit to return -EBUSY or whatever? Without this horrible #ifdeffery. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
Hi Maxime,
On Mon, Sep 15, 2014 at 08:57:38AM +0200, Maxime Coquelin wrote:
> >+err = dev->acquire_ownership(dev->dev);
> Have you considered using hwspinlocks instead?
No, I've not used it before but it looks applicable here. I'll take a look.
> >@@ -212,6 +259,25 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > adap->dev.parent = &pdev->dev;
> > adap->dev.of_node = pdev->dev.of_node;
> >
> >+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> >+if (dev->shared_host)
> >+adap->algo = &i2c_sc_algo;
> >+
> >+r = i2c_add_numbered_adapter(adap);
> >+if (r) {
> >+dev_err(&pdev->dev, "failure adding adapter\n");
> >+return r;
> >+}
> >+
> >+if (dev->shared_host)
> >+pm_runtime_forbid(&pdev->dev);
> >+else {
> >+pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >+pm_runtime_use_autosuspend(&pdev->dev);
> >+pm_runtime_set_active(&pdev->dev);
> >+pm_runtime_enable(&pdev->dev);
> >+}
> >+#else
> Why do you put all this under config flags?
So that this additional code only compiles for this very specific
implementation.
> >@@ -268,7 +334,11 @@ static int dw_i2c_resume(struct device *dev)
> > struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> >
> > clk_prepare_enable(i_dev->clk);
> >-i2c_dw_init(i_dev);
> >+
> >+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
> >+if (!i_dev->shared_host)
> >+#endif
> Putting this under config flag should not be needed.
>
> And even not under config flags, why don't you re-initialize your
> device in case of resume?
Because the device is already being managed by hardware, not the OS.
David Box
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c-designware: Intel BayTrail PMIC I2C bus support
Hi David,
On 09/12/2014 07:36 PM, David E. Box wrote:
This patch implements an I2C bus sharing mechanism between the host and platform
hardware on select Intel BayTrail SoC platforms using the XPower AXP288 PMIC.
On these platforms access to the PMIC must be shared with platform hardware. The
hardware unit assumes full control of the I2C bus and the host must request
access through a special semaphore. Hardware control of the bus also makes it
necessary to disable runtime pm to avoid interfering with hardware transactions.
Signed-off-by: David E. Box
---
drivers/i2c/busses/Kconfig | 10 +++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-designware-core.h| 14
drivers/i2c/busses/i2c-designware-platdrv.c | 78 +++--
drivers/i2c/busses/i2c-shared-controller.c | 101
5 files changed, 200 insertions(+), 4 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-shared-controller.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..672ef23 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
This driver can also be built as a module. If so, the module
will be called i2c-designware-pci.
+config I2C_SHARED_CONTROLLER
This config name is too generic.
From what I understand, it is very Intel dependant.
+ tristate "Intel Baytrail PMIC shared I2C bus support"
+ depends on ACPI
+ select IOSF_MBI
+ select I2C_DESIGNWARE_CORE
+ help
+ This driver enables shared access to the PMIC I2C bus on select Intel
+ BayTrail platforms using the XPower AXP288 PMIC. This driver is
+ required for host access to the PMIC on these platforms.
+
config I2C_EFM32
tristate "EFM32 I2C controller"
depends on ARCH_EFM32 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 49bf07e..33d62d1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-objs := i2c-designware-platdrv.o
obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o
+obj-$(CONFIG_I2C_SHARED_CONTROLLER)+= i2c-shared-controller.o
Ditto, the file name is too generic.
i2c-designware-pci-objs := i2c-designware-pcidrv.o
obj-$(CONFIG_I2C_EFM32) += i2c-efm32.o
obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index d66b6cb..a2b72f4 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -65,6 +65,10 @@
* @ss_lcnt: standard speed LCNT value
* @fs_hcnt: fast speed HCNT value
* @fs_lcnt: fast speed LCNT value
+ * @shared_host: if host must share access to adapter with other
+ * firmware/hardware units
+ * @acquire_ownership: function to acquire exclusive use of the controller
+ * @release_ownership: function to release exclusive use of the controller
*
* HCNT and LCNT parameters can be used if the platform knows more accurate
* values than the one computed based only on the input clock frequency.
@@ -105,6 +109,11 @@ struct dw_i2c_dev {
u16 ss_lcnt;
u16 fs_hcnt;
u16 fs_lcnt;
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+ int shared_host;
+ int (*acquire_ownership)(struct device *dev);
+ int (*release_ownership)(struct device *dev);
+#endif
};
#define ACCESS_SWAP 0x0001
@@ -123,3 +132,8 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+extern int i2c_acquire_ownership(struct device *dev);
+extern int i2c_release_ownership(struct device *dev);
+#endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index f9b1dec..f86c285 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -52,6 +52,32 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
return clk_get_rate(dev->clk)/1000;
}
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+int i2c_shared_controller_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+ int num)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+ int err;
+
+ if (dev->shared_host) {
this share_host variable is not needed, you could just check whether
acquire callback is set.
+
[PATCH] 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
---
drivers/i2c/busses/Kconfig | 10 +++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-designware-core.h| 14
drivers/i2c/busses/i2c-designware-platdrv.c | 78 +++--
drivers/i2c/busses/i2c-shared-controller.c | 101
5 files changed, 200 insertions(+), 4 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-shared-controller.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ac87fa..672ef23 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
This driver can also be built as a module. If so, the module
will be called i2c-designware-pci.
+config I2C_SHARED_CONTROLLER
+ tristate "Intel Baytrail PMIC shared I2C bus support"
+ depends on ACPI
+ select IOSF_MBI
+ select I2C_DESIGNWARE_CORE
+ help
+ This driver enables shared access to the PMIC I2C bus on select Intel
+ BayTrail platforms using the XPower AXP288 PMIC. This driver is
+ required for host access to the PMIC on these platforms.
+
config I2C_EFM32
tristate "EFM32 I2C controller"
depends on ARCH_EFM32 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 49bf07e..33d62d1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-objs := i2c-designware-platdrv.o
obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o
+obj-$(CONFIG_I2C_SHARED_CONTROLLER)+= i2c-shared-controller.o
i2c-designware-pci-objs := i2c-designware-pcidrv.o
obj-$(CONFIG_I2C_EFM32)+= i2c-efm32.o
obj-$(CONFIG_I2C_EG20T)+= i2c-eg20t.o
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index d66b6cb..a2b72f4 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -65,6 +65,10 @@
* @ss_lcnt: standard speed LCNT value
* @fs_hcnt: fast speed HCNT value
* @fs_lcnt: fast speed LCNT value
+ * @shared_host: if host must share access to adapter with other
+ * firmware/hardware units
+ * @acquire_ownership: function to acquire exclusive use of the controller
+ * @release_ownership: function to release exclusive use of the controller
*
* HCNT and LCNT parameters can be used if the platform knows more accurate
* values than the one computed based only on the input clock frequency.
@@ -105,6 +109,11 @@ struct dw_i2c_dev {
u16 ss_lcnt;
u16 fs_hcnt;
u16 fs_lcnt;
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+ int shared_host;
+ int (*acquire_ownership)(struct device *dev);
+ int (*release_ownership)(struct device *dev);
+#endif
};
#define ACCESS_SWAP0x0001
@@ -123,3 +132,8 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
extern void i2c_dw_clear_int(struct dw_i2c_dev *dev);
extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
+
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+extern int i2c_acquire_ownership(struct device *dev);
+extern int i2c_release_ownership(struct device *dev);
+#endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index f9b1dec..f86c285 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -52,6 +52,32 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
return clk_get_rate(dev->clk)/1000;
}
+#if IS_ENABLED(CONFIG_I2C_SHARED_CONTROLLER)
+int i2c_shared_controller_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+ int num)
+{
+ struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+ int err;
+
+ if (dev->shared_host) {
+ err = dev->acquire_ownership(dev->dev);
+ if (!err) {
+ err = i2c_dw_xfer(adap, msgs, num);
+ dev->release_ownership(dev->dev);
+ } else
+ dev_WARN(dev->dev, "couldnt acquire ownership\n");
