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:0.312 / U:0.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site