lkml.org 
[lkml]   [2014]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <david.e.box@linux.intel.com>
    > ---
    > drivers/i2c/busses/Kconfig | 10 +++
    > drivers/i2c/busses/Makefile | 1 +
    > drivers/i2c/busses/i2c-designware-core.h | 14 ++++
    > drivers/i2c/busses/i2c-designware-platdrv.c | 78 +++++++++++++++++++--
    > drivers/i2c/busses/i2c-shared-controller.c | 101 ++++++++++++++++++++++++++++
    > 5 files changed, 200 insertions(+), 4 deletions(-)
    > create mode 100644 drivers/i2c/busses/i2c-shared-controller.c
    >
    > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
    > index 2ac87fa..672ef23 100644
    > --- a/drivers/i2c/busses/Kconfig
    > +++ b/drivers/i2c/busses/Kconfig
    > @@ -441,6 +441,16 @@ config I2C_DESIGNWARE_PCI
    > This driver can also be built as a module. If so, the module
    > will be called i2c-designware-pci.
    >
    > +config I2C_SHARED_CONTROLLER
    This config name is too generic.
    From what I understand, it is very Intel dependant.

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

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

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


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

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

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

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

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


    \
     
     \ /
      Last update: 2014-09-15 09:41    [W:4.683 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site