serial: 8250_dw: Add DMA support for non-ACPI platforms

Quoted 186 lines · Ray Jui

Add initial support to the Broadcom iProc I2C controller found in the iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has a size of 64 bytes. The iProc I2C controller supports two bus speeds including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
  drivers/i2c/busses/Kconfig         |    9 +
  drivers/i2c/busses/Makefile        |    1 +
  drivers/i2c/busses/i2c-bcm-iproc.c |  503
++++++++++++++++++++++++++++++++++++
  3 files changed, 513 insertions(+)

create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

drivers/i2c/busses/Kconfig +9 -0

index c1351d9..8a2eb7e 100644
@@ -372,6 +372,15 @@ config I2C_BCM2835
        This support is also available as a module.  If so, the module
        will be called i2c-bcm2835.
+config I2C_BCM_IPROC
+    tristate "Broadcom iProc I2C controller"
+    depends on ARCH_BCM_IPROC
+    help
+      If you say yes to this option, support will be included for the
+      Broadcom iProc I2C controller.
+
+      If you don't know what to do here, say N.
+
  config I2C_BCM_KONA
      tristate "BCM Kona I2C adapter"
      depends on ARCH_BCM_MOBILE

drivers/i2c/busses/Makefile +1 -0

index 5e6c822..216e7be 100644
@@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91)        += i2c-at91.o
  obj-$(CONFIG_I2C_AU1550)    += i2c-au1550.o
  obj-$(CONFIG_I2C_AXXIA)        += i2c-axxia.o
  obj-$(CONFIG_I2C_BCM2835)    += i2c-bcm2835.o
+obj-$(CONFIG_I2C_BCM_IPROC)    += i2c-bcm-iproc.o
  obj-$(CONFIG_I2C_BLACKFIN_TWI)    += i2c-bfin-twi.o
  obj-$(CONFIG_I2C_CADENCE)    += i2c-cadence.o
  obj-$(CONFIG_I2C_CBUS_GPIO)    += i2c-cbus-gpio.o

drivers/i2c/busses/i2c-bcm-iproc.c new file +102 -0

new file mode 100644
index 0000000..0e6e603
@@ -0,0 +1,503 @@
+/*
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+
+#define CFG_OFFSET                   0x00
+#define CFG_RESET_SHIFT              31
+#define CFG_EN_SHIFT                 30
+#define CFG_M_RETRY_CNT_SHIFT        16
+#define CFG_M_RETRY_CNT_MASK         0x0f
+
+#define TIM_CFG_OFFSET               0x04
+#define TIME_CFG_MODE_400_SHIFT      31
+
+#define M_FIFO_CTRL_OFFSET           0x0c
+#define M_FIFO_RX_FLUSH_SHIFT        31
+#define M_FIFO_TX_FLUSH_SHIFT        30
+#define M_FIFO_RX_CNT_SHIFT          16
+#define M_FIFO_RX_CNT_MASK           0x7f
+#define M_FIFO_RX_THLD_SHIFT         8
+#define M_FIFO_RX_THLD_MASK          0x3f
+
+#define M_CMD_OFFSET                 0x30
+#define M_CMD_START_BUSY_SHIFT       31
+#define M_CMD_STATUS_SHIFT           25
+#define M_CMD_STATUS_MASK            0x07
+#define M_CMD_STATUS_SUCCESS         0x0
+#define M_CMD_STATUS_LOST_ARB        0x1
+#define M_CMD_STATUS_NACK_ADDR       0x2
+#define M_CMD_STATUS_NACK_DATA       0x3
+#define M_CMD_STATUS_TIMEOUT         0x4
+#define M_CMD_PROTOCOL_SHIFT         9
+#define M_CMD_PROTOCOL_MASK          0xf
+#define M_CMD_PROTOCOL_BLK_WR        0x7
+#define M_CMD_PROTOCOL_BLK_RD        0x8
+#define M_CMD_PEC_SHIFT              8
+#define M_CMD_RD_CNT_SHIFT           0
+#define M_CMD_RD_CNT_MASK            0xff
+
+#define IE_OFFSET                    0x38
+#define IE_M_RX_FIFO_FULL_SHIFT      31
+#define IE_M_RX_THLD_SHIFT           30
+#define IE_M_START_BUSY_SHIFT        28
+
+#define IS_OFFSET                    0x3c
+#define IS_M_RX_FIFO_FULL_SHIFT      31
+#define IS_M_RX_THLD_SHIFT           30
+#define IS_M_START_BUSY_SHIFT        28
+
+#define M_TX_OFFSET                  0x40
+#define M_TX_WR_STATUS_SHIFT         31
+#define M_TX_DATA_SHIFT              0
+#define M_TX_DATA_MASK               0xff
+
+#define M_RX_OFFSET                  0x44
+#define M_RX_STATUS_SHIFT            30
+#define M_RX_STATUS_MASK             0x03
+#define M_RX_PEC_ERR_SHIFT           29
+#define M_RX_DATA_SHIFT              0
+#define M_RX_DATA_MASK               0xff
+
+#define I2C_TIMEOUT_MESC             100
+#define M_TX_RX_FIFO_SIZE            64
+
+enum bus_speed_index {
+    I2C_SPD_100K = 0,
+    I2C_SPD_400K,
+};
+
+struct bcm_iproc_i2c_dev {
+    struct device *device;
+
+    void __iomem *base;
+    struct i2c_msg *msg;
+
+    struct i2c_adapter adapter;
+
+    struct completion done;
+};
+
+/*
+ * Can be expanded in the future if more interrupt status bits are

utilized
+ */

drivers/i2c/busses/i2c-bcm-iproc.c new file +21 -0

+#define ISR_MASK (1 << IS_M_START_BUSY_SHIFT)
+
+static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
+{
+    struct bcm_iproc_i2c_dev *dev = data;
+    u32 status = readl(dev->base + IS_OFFSET);
+
+    status &= ISR_MASK;
+
+    if (!status)
+        return IRQ_NONE;
+
+    writel(status, dev->base + IS_OFFSET);
+    complete_all(&dev->done);
+
+    return IRQ_HANDLED;
+}
+
+static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *dev)
+{
+    unsigned long timeout = jiffies +

msecs_to_jiffies(I2C_TIMEOUT_MESC);

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+
+    while (readl(dev->base + M_CMD_OFFSET) &
+            (1 << M_CMD_START_BUSY_SHIFT)) {
+        if (time_after(jiffies, timeout)) {
+            dev_err(dev->device, "wait for bus idle timeout\n");

drivers/i2c/busses/i2c-bcm-iproc.c line 139

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+            return -ETIMEDOUT;
+        }
+    }
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *dev,
+        struct i2c_msg *msg, u8 *addr)
+{

Match open parenthesis..

static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *dev,
struct i2c_msg *msg, u8 *addr)

Quoted 46 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +46 -0

+
+    if (msg->flags & I2C_M_TEN) {
+        dev_err(dev->device, "no support for 10-bit address\n");
+        return -EINVAL;
+    }
+
+    *addr = (msg->addr << 1) & 0xfe;
+
+    if (msg->flags & I2C_M_RD)
+        *addr |= 1;
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + M_CMD_OFFSET);
+    val = (val >> M_CMD_STATUS_SHIFT) & M_CMD_STATUS_MASK;
+
+    switch (val) {
+    case M_CMD_STATUS_SUCCESS:
+        return 0;
+
+    case M_CMD_STATUS_LOST_ARB:
+        dev_err(dev->device, "lost bus arbitration\n");
+        return -EREMOTEIO;
+
+    case M_CMD_STATUS_NACK_ADDR:
+        dev_err(dev->device, "NAK addr:0x%02x\n", dev->msg->addr);
+        return -EREMOTEIO;
+
+    case M_CMD_STATUS_NACK_DATA:
+        dev_err(dev->device, "NAK data\n");
+        return -EREMOTEIO;
+
+    case M_CMD_STATUS_TIMEOUT:
+        dev_err(dev->device, "bus timeout\n");
+        return -ETIMEDOUT;
+
+    default:
+        dev_err(dev->device, "unknown error code=%d\n", val);
+        return -EREMOTEIO;
+    }
+

drivers/i2c/busses/i2c-bcm-iproc.c line 191

Quoted 6 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +6 -0

+    return -EREMOTEIO;
+}
+
+static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *dev,
+        struct i2c_msg *msg)
+{

dto...

Quoted 234 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +48 -0

+    int ret, i;
+    u8 addr;
+    u32 val;
+    unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC);
+
+    if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) {
+        dev_err(dev->device,
+            "supported data length is 1 - %u bytes\n",
+                M_TX_RX_FIFO_SIZE - 1);
+        return -EINVAL;
+    }
+
+    dev->msg = msg;
+    ret = __wait_for_bus_idle(dev);
+    if (ret)
+        return ret;
+
+    ret = bcm_iproc_i2c_format_addr(dev, msg, &addr);
+    if (ret)
+        return ret;
+
+    /* load slave address into the TX FIFO */
+    writel(addr, dev->base + M_TX_OFFSET);
+
+    /* for a write transaction, load data into the TX FIFO */
+    if (!(msg->flags & I2C_M_RD)) {
+        for (i = 0; i < msg->len; i++) {
+            val = msg->buf[i];
+
+            /* mark the last byte */
+            if (i == msg->len - 1)
+                val |= 1 << M_TX_WR_STATUS_SHIFT;
+
+            writel(val, dev->base + M_TX_OFFSET);
+        }
+    }
+
+    /* mark as incomplete before starting the transaction */
+    reinit_completion(&dev->done);
+
+    /*
+     * Enable the "start busy" interrupt, which will be triggered after
+     * the transaction is done
+     */
+    writel(1 << IE_M_START_BUSY_SHIFT, dev->base + IE_OFFSET);
+
+    /*
+     * Now we can activate the transfer. For a read operation,

specify the

drivers/i2c/busses/i2c-bcm-iproc.c new file +90 -0

+     * number of bytes to read
+     */
+    val = 1 << M_CMD_START_BUSY_SHIFT;
+    if (msg->flags & I2C_M_RD) {
+        val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
+            (msg->len << M_CMD_RD_CNT_SHIFT);
+    } else {
+        val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
+    }
+    writel(val, dev->base + M_CMD_OFFSET);
+
+    time_left = wait_for_completion_timeout(&dev->done, time_left);
+
+    /* disable all interrupts */
+    writel(0, dev->base + IE_OFFSET);
+
+    if (!time_left) {
+        dev_err(dev->device, "transaction times out\n");
+
+        /* flush FIFOs */
+        val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+            (1 << M_FIFO_TX_FLUSH_SHIFT);
+        writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+        return -EREMOTEIO;
+    }
+
+    ret = bcm_iproc_i2c_check_status(dev);
+    if (ret) {
+        /* flush both TX/RX FIFOs */
+        val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+            (1 << M_FIFO_TX_FLUSH_SHIFT);
+        writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+        return ret;
+    }
+
+    /*
+     * For a read operation, we now need to load the data from FIFO
+     * into the memory buffer
+     */
+    if (msg->flags & I2C_M_RD) {
+        for (i = 0; i < msg->len; i++) {
+            msg->buf[i] = (readl(dev->base + M_RX_OFFSET) >>
+                    M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
+        }
+    }
+
+    dev_dbg(dev->device, "xfer %c, addr=0x%02x, len=%d\n",
+            (msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
+            msg->len);
+    dev_dbg(dev->device, "**** data start ****\n");
+    for (i = 0; i < msg->len; i++)
+        dev_dbg(dev->device, "0x%02x ",  msg->buf[i]);
+    dev_dbg(dev->device, "**** data end ****\n");
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
+                 struct i2c_msg msgs[], int num)
+{
+    struct bcm_iproc_i2c_dev *dev = i2c_get_adapdata(adapter);
+    int ret, i;
+
+    /* go through all messages */
+    for (i = 0; i < num; i++) {
+        ret = bcm_iproc_i2c_xfer_single_msg(dev, &msgs[i]);
+        if (ret) {
+            dev_err(dev->device, "xfer failed\n");
+            return ret;
+        }
+    }
+
+    return num;
+}
+
+static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
+{
+    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm bcm_iproc_algo = {
+    .master_xfer = bcm_iproc_i2c_xfer,
+    .functionality = bcm_iproc_i2c_functionality,
+};
+
+static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *dev)
+{
+    unsigned int bus_speed, speed_bit;
+    u32 val;
+    int ret = of_property_read_u32(dev->device->of_node,

"clock-frequency",

drivers/i2c/busses/i2c-bcm-iproc.c new file +94 -0

+                       &bus_speed);
+    if (ret < 0) {
+        dev_err(dev->device, "missing clock-frequency property\n");
+        return -ENODEV;
+    }
+
+    switch (bus_speed) {
+    case 100000:
+        speed_bit = 0;
+        break;
+    case 400000:
+        speed_bit = 1;
+        break;
+    default:
+        dev_err(dev->device, "%d Hz bus speed not supported\n",
+                bus_speed);
+        dev_err(dev->device, "valid speeds are 100khz and 400khz\n");
+        return -EINVAL;
+    }
+
+    val = readl(dev->base + TIM_CFG_OFFSET);
+    val &= ~(1 << TIME_CFG_MODE_400_SHIFT);
+    val |= speed_bit << TIME_CFG_MODE_400_SHIFT;
+    writel(val, dev->base + TIM_CFG_OFFSET);
+
+    dev_info(dev->device, "bus set to %u Hz\n", bus_speed);
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    /* put controller in reset */
+    val = readl(dev->base + CFG_OFFSET);
+    val |= 1 << CFG_RESET_SHIFT;
+    val &= ~(1 << CFG_EN_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+
+    /* wait 100 usec per spec */
+    udelay(100);
+
+    /* bring controller out of reset */
+    val = readl(dev->base + CFG_OFFSET);
+    val &= ~(1 << CFG_RESET_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+
+    /* flush TX/RX FIFOs and set RX FIFO threshold to zero */
+    val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
+    writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+
+    /* disable all interrupts */
+    val = 0;
+    writel(val, dev->base + IE_OFFSET);
+
+    /* clear all pending interrupts */
+    val = readl(dev->base + IS_OFFSET);
+    writel(val, dev->base + IS_OFFSET);
+
+    return 0;
+}
+
+static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + CFG_OFFSET);
+    val |= 1 << CFG_EN_SHIFT;
+    writel(val, dev->base + CFG_OFFSET);
+}
+
+static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + CFG_OFFSET);
+    val &= ~(1 << CFG_EN_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+}
+
+static int bcm_iproc_i2c_probe(struct platform_device *pdev)
+{
+    int irq, ret = 0;
+    struct bcm_iproc_i2c_dev *dev;
+    struct i2c_adapter *adap;
+    struct resource *res;
+
+    dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+    if (!dev)
+        return -ENOMEM;
+
+    platform_set_drvdata(pdev, dev);
+    dev->device = &pdev->dev;

drivers/i2c/busses/i2c-bcm-iproc.c line 428

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+    init_completion(&dev->done);
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res)
+        return -ENODEV;

We can remove this resource check. This checking will happen with devm_ioremap_resource()

Quoted 60 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +30 -0

+    dev->base = devm_ioremap_resource(dev->device, res);
+    if (IS_ERR(dev->base))
+        return -ENOMEM;
+
+    ret = bcm_iproc_i2c_init(dev);
+    if (ret)
+        return ret;
+
+    ret = bcm_iproc_i2c_cfg_speed(dev);
+    if (ret)
+        return ret;
+
+    irq = platform_get_irq(pdev, 0);
+    if (irq < 0) {
+        dev_err(dev->device, "no irq resource\n");
+        return irq;
+    }
+
+    ret = devm_request_irq(&pdev->dev, irq, bcm_iproc_i2c_isr,
+            IRQF_SHARED, pdev->name, dev);
+    if (ret) {
+        dev_err(dev->device, "unable to request irq %i\n", irq);
+        return ret;
+    }
+
+    bcm_iproc_i2c_enable(dev);
+
+    adap = &dev->adapter;
+    i2c_set_adapdata(adap, dev);
+    strlcpy(adap->name, "Broadcom iProc I2C adapter",

sizeof(adap->name));

drivers/i2c/busses/i2c-bcm-iproc.c new file +29 -0

+    adap->algo = &bcm_iproc_algo;
+    adap->dev.parent = &pdev->dev;
+    adap->dev.of_node = pdev->dev.of_node;
+
+    ret = i2c_add_adapter(adap);
+    if (ret) {
+        dev_err(dev->device, "failed to add adapter\n");
+        return ret;
+    }
+
+    dev_info(dev->device, "device registered successfully\n");
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_remove(struct platform_device *pdev)
+{
+    struct bcm_iproc_i2c_dev *dev = platform_get_drvdata(pdev);
+
+    i2c_del_adapter(&dev->adapter);
+    bcm_iproc_i2c_disable(dev);
+
+    return 0;
+}
+
+static const struct of_device_id bcm_iproc_i2c_of_match[] = {
+    {.compatible = "brcm,iproc-i2c",},
+    {},
+};

drivers/i2c/busses/i2c-bcm-iproc.c line 493

Quoted 6 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +6 -0

+MODULE_DEVICE_TABLE(of, bcm_iproc_i2c_of_match);
+
+static struct platform_driver bcm_iproc_i2c_driver = {
+    .driver = {
+           .name = "bcm-iproc-i2c",
+           .owner = THIS_MODULE,

No need to update this field. Its updated by module_platform_driver().

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+           .of_match_table = bcm_iproc_i2c_of_match,
+           },
+    .probe = bcm_iproc_i2c_probe,
+    .remove = bcm_iproc_i2c_remove,
+};
+module_platform_driver(bcm_iproc_i2c_driver);
+
+MODULE_AUTHOR("Ray Jui <[email protected]>");
+MODULE_DESCRIPTION("Broadcom iProc I2C Driver");
+MODULE_LICENSE("GPL v2");

Quoted 20 lines · Varka Bhadram

Quoted 17 lines · Ray Jui

Add initial support to the Broadcom iProc I2C controller found in the iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has a size of 64 bytes. The iProc I2C controller supports two bus speeds including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
  drivers/i2c/busses/Kconfig         |    9 +
  drivers/i2c/busses/Makefile        |    1 +
  drivers/i2c/busses/i2c-bcm-iproc.c |  503
++++++++++++++++++++++++++++++++++++
  3 files changed, 513 insertions(+)

create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

Quoted 1 line

Quoted 1 line

(...)

+static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *dev,

Quoted 4 lines

Quoted 3 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +2 -0

+        struct i2c_msg *msg)
+{

Quoted 3 lines

dto...

One more indent? Sure.

Yes...

+ int ret, i;

Quoted 243 lines

Quoted 229 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +47 -0

+    u8 addr;
+    u32 val;
+    unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC);
+
+    if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) {
+        dev_err(dev->device,
+            "supported data length is 1 - %u bytes\n",
+                M_TX_RX_FIFO_SIZE - 1);
+        return -EINVAL;
+    }
+
+    dev->msg = msg;
+    ret = __wait_for_bus_idle(dev);
+    if (ret)
+        return ret;
+
+    ret = bcm_iproc_i2c_format_addr(dev, msg, &addr);
+    if (ret)
+        return ret;
+
+    /* load slave address into the TX FIFO */
+    writel(addr, dev->base + M_TX_OFFSET);
+
+    /* for a write transaction, load data into the TX FIFO */
+    if (!(msg->flags & I2C_M_RD)) {
+        for (i = 0; i < msg->len; i++) {
+            val = msg->buf[i];
+
+            /* mark the last byte */
+            if (i == msg->len - 1)
+                val |= 1 << M_TX_WR_STATUS_SHIFT;
+
+            writel(val, dev->base + M_TX_OFFSET);
+        }
+    }
+
+    /* mark as incomplete before starting the transaction */
+    reinit_completion(&dev->done);
+
+    /*
+     * Enable the "start busy" interrupt, which will be triggered after
+     * the transaction is done
+     */
+    writel(1 << IE_M_START_BUSY_SHIFT, dev->base + IE_OFFSET);
+
+    /*
+     * Now we can activate the transfer. For a read operation,

specify the

drivers/i2c/busses/i2c-bcm-iproc.c new file +90 -0

+     * number of bytes to read
+     */
+    val = 1 << M_CMD_START_BUSY_SHIFT;
+    if (msg->flags & I2C_M_RD) {
+        val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
+            (msg->len << M_CMD_RD_CNT_SHIFT);
+    } else {
+        val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
+    }
+    writel(val, dev->base + M_CMD_OFFSET);
+
+    time_left = wait_for_completion_timeout(&dev->done, time_left);
+
+    /* disable all interrupts */
+    writel(0, dev->base + IE_OFFSET);
+
+    if (!time_left) {
+        dev_err(dev->device, "transaction times out\n");
+
+        /* flush FIFOs */
+        val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+            (1 << M_FIFO_TX_FLUSH_SHIFT);
+        writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+        return -EREMOTEIO;
+    }
+
+    ret = bcm_iproc_i2c_check_status(dev);
+    if (ret) {
+        /* flush both TX/RX FIFOs */
+        val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+            (1 << M_FIFO_TX_FLUSH_SHIFT);
+        writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+        return ret;
+    }
+
+    /*
+     * For a read operation, we now need to load the data from FIFO
+     * into the memory buffer
+     */
+    if (msg->flags & I2C_M_RD) {
+        for (i = 0; i < msg->len; i++) {
+            msg->buf[i] = (readl(dev->base + M_RX_OFFSET) >>
+                    M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
+        }
+    }
+
+    dev_dbg(dev->device, "xfer %c, addr=0x%02x, len=%d\n",
+            (msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
+            msg->len);
+    dev_dbg(dev->device, "**** data start ****\n");
+    for (i = 0; i < msg->len; i++)
+        dev_dbg(dev->device, "0x%02x ",  msg->buf[i]);
+    dev_dbg(dev->device, "**** data end ****\n");
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
+                 struct i2c_msg msgs[], int num)
+{
+    struct bcm_iproc_i2c_dev *dev = i2c_get_adapdata(adapter);
+    int ret, i;
+
+    /* go through all messages */
+    for (i = 0; i < num; i++) {
+        ret = bcm_iproc_i2c_xfer_single_msg(dev, &msgs[i]);
+        if (ret) {
+            dev_err(dev->device, "xfer failed\n");
+            return ret;
+        }
+    }
+
+    return num;
+}
+
+static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
+{
+    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm bcm_iproc_algo = {
+    .master_xfer = bcm_iproc_i2c_xfer,
+    .functionality = bcm_iproc_i2c_functionality,
+};
+
+static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *dev)
+{
+    unsigned int bus_speed, speed_bit;
+    u32 val;
+    int ret = of_property_read_u32(dev->device->of_node,

"clock-frequency",

drivers/i2c/busses/i2c-bcm-iproc.c new file +89 -0

+                       &bus_speed);
+    if (ret < 0) {
+        dev_err(dev->device, "missing clock-frequency property\n");
+        return -ENODEV;
+    }
+
+    switch (bus_speed) {
+    case 100000:
+        speed_bit = 0;
+        break;
+    case 400000:
+        speed_bit = 1;
+        break;
+    default:
+        dev_err(dev->device, "%d Hz bus speed not supported\n",
+                bus_speed);
+        dev_err(dev->device, "valid speeds are 100khz and 400khz\n");
+        return -EINVAL;
+    }
+
+    val = readl(dev->base + TIM_CFG_OFFSET);
+    val &= ~(1 << TIME_CFG_MODE_400_SHIFT);
+    val |= speed_bit << TIME_CFG_MODE_400_SHIFT;
+    writel(val, dev->base + TIM_CFG_OFFSET);
+
+    dev_info(dev->device, "bus set to %u Hz\n", bus_speed);
+
+    return 0;
+}
+
+static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    /* put controller in reset */
+    val = readl(dev->base + CFG_OFFSET);
+    val |= 1 << CFG_RESET_SHIFT;
+    val &= ~(1 << CFG_EN_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+
+    /* wait 100 usec per spec */
+    udelay(100);
+
+    /* bring controller out of reset */
+    val = readl(dev->base + CFG_OFFSET);
+    val &= ~(1 << CFG_RESET_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+
+    /* flush TX/RX FIFOs and set RX FIFO threshold to zero */
+    val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
+    writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+
+    /* disable all interrupts */
+    val = 0;
+    writel(val, dev->base + IE_OFFSET);
+
+    /* clear all pending interrupts */
+    val = readl(dev->base + IS_OFFSET);
+    writel(val, dev->base + IS_OFFSET);
+
+    return 0;
+}
+
+static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + CFG_OFFSET);
+    val |= 1 << CFG_EN_SHIFT;
+    writel(val, dev->base + CFG_OFFSET);
+}
+
+static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *dev)
+{
+    u32 val;
+
+    val = readl(dev->base + CFG_OFFSET);
+    val &= ~(1 << CFG_EN_SHIFT);
+    writel(val, dev->base + CFG_OFFSET);
+}
+
+static int bcm_iproc_i2c_probe(struct platform_device *pdev)
+{
+    int irq, ret = 0;
+    struct bcm_iproc_i2c_dev *dev;
+    struct i2c_adapter *adap;
+    struct resource *res;
+
+    dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);

drivers/i2c/busses/i2c-bcm-iproc.c line 428

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+    if (!dev)
+        return -ENOMEM;
+
+    platform_set_drvdata(pdev, dev);
+    dev->device = &pdev->dev;
+    init_completion(&dev->done);
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res)
+        return -ENODEV;

We can remove this resource check. This checking will happen with devm_ioremap_resource()

Quoted 1 line

Don't you need to obtain a valid resource and pass it into

devm_ioremap_resource? Without 'res' being assigned a valid resource, devm_ioremap_resource will reject with "invalid resource".

platform_get_resource() will return a resource, checking on this

Quoted 24 lines · Ray Jui

Quoted 20 lines · Varka Bhadram

Quoted 17 lines · Ray Jui

Add initial support to the Broadcom iProc I2C controller found in the iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has a size of 64 bytes. The iProc I2C controller supports two bus speeds including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
  drivers/i2c/busses/Kconfig         |    9 +
  drivers/i2c/busses/Makefile        |    1 +
  drivers/i2c/busses/i2c-bcm-iproc.c |  503
++++++++++++++++++++++++++++++++++++
  3 files changed, 513 insertions(+)

create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

Quoted 1 line

Quoted 1 line

Quoted 1 line

(...)

+static int bcm_iproc_i2c_probe(struct platform_device *pdev)

Quoted 22 lines

Quoted 22 lines

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+{
+    int irq, ret = 0;
+    struct bcm_iproc_i2c_dev *dev;
+    struct i2c_adapter *adap;
+    struct resource *res;
+
+    dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);

drivers/i2c/busses/i2c-bcm-iproc.c line 428

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+    if (!dev)
+        return -ENOMEM;
+
+    platform_set_drvdata(pdev, dev);
+    dev->device = &pdev->dev;
+    init_completion(&dev->done);
+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res)
+        return -ENODEV;

We can remove this resource check. This checking will happen with devm_ioremap_resource()

Quoted 5 lines

Quoted 1 line

Don't you need to obtain a valid resource and pass it into

devm_ioremap_resource? Without 'res' being assigned a valid resource, devm_ioremap_resource will reject with "invalid resource".

platform_get_resource() will return a resource, checking on this

resource is happening at
http://lxr.free-electrons.com/source/lib/devres.c#L115. So no need to check it explicitly.

If you check here it will be duplication of check with resource. Two times we are checking on
the resource. No point of doing like that.

Thanks.

On Wed, Dec 10, 2014 at 8:51 AM, Varka Bhadram <[email protected] <mailto:[email protected]>> wrote:

On Wed, Dec 10, 2014 at 7:11 AM, Ray Jui <[email protected] <mailto:[email protected]>> wrote:

On 12/9/2014 5:33 PM, Varka Bhadram wrote:

On Wednesday 10 December 2014 06:24 AM, Ray Jui wrote:

Add initial support to the Broadcom iProc I2C controller found in the
iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has
a size of 64 bytes. The iProc I2C controller supports two bus speeds
including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by Ray Jui <[email protected]

<mailto:[email protected]>>

Reviewed-by Scott Branden <[email protected]

<mailto:[email protected]>>
---
drivers/i2c/busses/Kconfig | 9 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-bcm-iproc.c | 503
++++++++++++++++++++++++++++++++++++
3 files changed, 513 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

(...)

+static int bcm_iproc_i2c_probe(struct platform_device *pdev)
+{
+ int irq, ret = 0;
+ struct bcm_iproc_i2c_dev *dev;
+ struct i2c_adapter *adap;
+ struct resource *res;
+
+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev),
GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, dev);
+ dev->device = &pdev->dev;
+ init_completion(&dev->done);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;

We can remove this resource check. This checking will happen with
devm_ioremap_resource()

Don't you need to obtain a valid resource and pass it into devm_ioremap_resource? Without 'res' being assigned a valid resource, devm_ioremap_resource will reject with "invalid resource".

platform_get_resource() will return a resource, checking on this resource is happening at
http://lxr.free-electrons.com/source/lib/devres.c#L115. So no need to check it explicitly.

If you check here it will be duplication of check with resource. Two times we are checking on
the resource. No point of doing like that.

Thanks.

See this: http://lxr.free-electrons.com/source/lib/devres.c#L102

--
Thanks and Regards,
Varka Bhadram.

Enable I2C driver support for Broadcom iProc family of SoCs by selecting I2C_BCM_IPROC

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
 arch/arm/mach-bcm/Kconfig |    1 +
 1 file changed, 1 insertion(+)

arch/arm/mach-bcm/Kconfig +0 -0

index aaeec78..86ee90b 100644

arch/arm/mach-bcm/Kconfig +0 -0

 	help
 	  This enables support for systems based on Broadcom IPROC architected SoCs.
 	  The IPROC complex contains one or more ARM CPUs along with common

Quoted 14 lines · Ray Jui

Enable I2C driver support for Broadcom iProc family of SoCs by selecting I2C_BCM_IPROC

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
  arch/arm/mach-bcm/Kconfig |    1 +
  1 file changed, 1 insertion(+)

arch/arm/mach-bcm/Kconfig +0 -0

index aaeec78..86ee90b 100644

arch/arm/mach-bcm/Kconfig config ARCH_BCM_IPROC · line 22

Quoted 4 lines

arch/arm/mach-bcm/Kconfig +1 -0

@@ -19,6 +19,7 @@ config ARCH_BCM_IPROC
  	select ARCH_REQUIRE_GPIOLIB
  	select ARM_AMBA
  	select PINCTRL
+	select I2C_BCM_IPROC

One way to avoid having to modify mach-bcm/Kconfig would be to have your i2c driver Kconfig do this:

default ARCH_BCM_IPROC

would that work?

Quoted 4 lines

arch/arm/mach-bcm/Kconfig +0 -0

  	help
  	  This enables support for systems based on Broadcom IPROC architected SoCs.
  	  The IPROC complex contains one or more ARM CPUs along with common

Quoted 23 lines · Florian Fainelli

Quoted 14 lines · Ray Jui

Enable I2C driver support for Broadcom iProc family of SoCs by selecting I2C_BCM_IPROC

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
  arch/arm/mach-bcm/Kconfig |    1 +
  1 file changed, 1 insertion(+)

arch/arm/mach-bcm/Kconfig +0 -0

index aaeec78..86ee90b 100644

arch/arm/mach-bcm/Kconfig config ARCH_BCM_IPROC · line 22

Quoted 4 lines

arch/arm/mach-bcm/Kconfig +1 -0

@@ -19,6 +19,7 @@ config ARCH_BCM_IPROC
      select ARCH_REQUIRE_GPIOLIB
      select ARM_AMBA
      select PINCTRL
+    select I2C_BCM_IPROC

One way to avoid having to modify mach-bcm/Kconfig would be to have your i2c driver Kconfig do this:

Quoted 4 lines

default ARCH_BCM_IPROC

would that work?

Yes. So in which case it is better to select a driver from the architecture specific Kconfig?

Quoted 8 lines

Quoted 7 lines

arch/arm/mach-bcm/Kconfig +0 -0

      help
        This enables support for systems based on Broadcom IPROC

architected SoCs.
The IPROC complex contains one or more ARM CPUs along with
common

Quoted 26 lines · Ray Jui

Quoted 23 lines · Florian Fainelli

Quoted 14 lines · Ray Jui

Enable I2C driver support for Broadcom iProc family of SoCs by selecting I2C_BCM_IPROC

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
   arch/arm/mach-bcm/Kconfig |    1 +
   1 file changed, 1 insertion(+)

arch/arm/mach-bcm/Kconfig +0 -0

index aaeec78..86ee90b 100644

arch/arm/mach-bcm/Kconfig config ARCH_BCM_IPROC · line 22

Quoted 4 lines

arch/arm/mach-bcm/Kconfig +1 -0

@@ -19,6 +19,7 @@ config ARCH_BCM_IPROC
       select ARCH_REQUIRE_GPIOLIB
       select ARM_AMBA
       select PINCTRL
+    select I2C_BCM_IPROC

One way to avoid having to modify mach-bcm/Kconfig would be to have your i2c driver Kconfig do this:

Quoted 6 lines

Quoted 4 lines

default ARCH_BCM_IPROC

would that work?

Yes. So in which case it is better to select a driver from the architecture specific Kconfig?

I suppose if your driver/subsystem is critical for system boot, like powering a regulator or something that has a critical purpose, a select is probably more appropriate here. If this is just exposing non-critical devices, I would go with a depends on/default at the driver Kconfig level.

This is just how I see things, others would definitively have a different view.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

arch/arm/mach-bcm/Kconfig +0 -0

       help
         This enables support for systems based on Broadcom IPROC

architected SoCs.
The IPROC complex contains one or more ARM CPUs along with
common

Add initial support to the Broadcom iProc I2C controller found in the iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has a size of 64 bytes. The iProc I2C controller supports two bus speeds including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
 drivers/i2c/busses/Kconfig         |   10 +
 drivers/i2c/busses/Makefile        |    1 +
 drivers/i2c/busses/i2c-bcm-iproc.c |  500 ++++++++++++++++++++++++++++++++++++
 3 files changed, 511 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

drivers/i2c/busses/Kconfig +0 -0

index c1351d9..df21366 100644

drivers/i2c/busses/Kconfig +6 -0

+	help
+	  If you say yes to this option, support will be included for the
+	  Broadcom iProc I2C controller.
+
+	  If you don't know what to do here, say N.
+
 config I2C_BCM_KONA
 	tristate "BCM Kona I2C adapter"
 	depends on ARCH_BCM_MOBILE

drivers/i2c/busses/Makefile +1 -0

index 5e6c822..216e7be 100644
@@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_AXXIA)		+= i2c-axxia.o
 obj-$(CONFIG_I2C_BCM2835)	+= i2c-bcm2835.o
+obj-$(CONFIG_I2C_BCM_IPROC)	+= i2c-bcm-iproc.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CADENCE)	+= i2c-cadence.o
 obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-gpio.o

drivers/i2c/busses/i2c-bcm-iproc.c new file +121 -0

new file mode 100644
index 0000000..35ac497
@@ -0,0 +1,500 @@
+/*
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+
+#define CFG_OFFSET                   0x00
+#define CFG_RESET_SHIFT              31
+#define CFG_EN_SHIFT                 30
+#define CFG_M_RETRY_CNT_SHIFT        16
+#define CFG_M_RETRY_CNT_MASK         0x0f
+
+#define TIM_CFG_OFFSET               0x04
+#define TIME_CFG_MODE_400_SHIFT      31
+
+#define M_FIFO_CTRL_OFFSET           0x0c
+#define M_FIFO_RX_FLUSH_SHIFT        31
+#define M_FIFO_TX_FLUSH_SHIFT        30
+#define M_FIFO_RX_CNT_SHIFT          16
+#define M_FIFO_RX_CNT_MASK           0x7f
+#define M_FIFO_RX_THLD_SHIFT         8
+#define M_FIFO_RX_THLD_MASK          0x3f
+
+#define M_CMD_OFFSET                 0x30
+#define M_CMD_START_BUSY_SHIFT       31
+#define M_CMD_STATUS_SHIFT           25
+#define M_CMD_STATUS_MASK            0x07
+#define M_CMD_STATUS_SUCCESS         0x0
+#define M_CMD_STATUS_LOST_ARB        0x1
+#define M_CMD_STATUS_NACK_ADDR       0x2
+#define M_CMD_STATUS_NACK_DATA       0x3
+#define M_CMD_STATUS_TIMEOUT         0x4
+#define M_CMD_PROTOCOL_SHIFT         9
+#define M_CMD_PROTOCOL_MASK          0xf
+#define M_CMD_PROTOCOL_BLK_WR        0x7
+#define M_CMD_PROTOCOL_BLK_RD        0x8
+#define M_CMD_PEC_SHIFT              8
+#define M_CMD_RD_CNT_SHIFT           0
+#define M_CMD_RD_CNT_MASK            0xff
+
+#define IE_OFFSET                    0x38
+#define IE_M_RX_FIFO_FULL_SHIFT      31
+#define IE_M_RX_THLD_SHIFT           30
+#define IE_M_START_BUSY_SHIFT        28
+
+#define IS_OFFSET                    0x3c
+#define IS_M_RX_FIFO_FULL_SHIFT      31
+#define IS_M_RX_THLD_SHIFT           30
+#define IS_M_START_BUSY_SHIFT        28
+
+#define M_TX_OFFSET                  0x40
+#define M_TX_WR_STATUS_SHIFT         31
+#define M_TX_DATA_SHIFT              0
+#define M_TX_DATA_MASK               0xff
+
+#define M_RX_OFFSET                  0x44
+#define M_RX_STATUS_SHIFT            30
+#define M_RX_STATUS_MASK             0x03
+#define M_RX_PEC_ERR_SHIFT           29
+#define M_RX_DATA_SHIFT              0
+#define M_RX_DATA_MASK               0xff
+
+#define I2C_TIMEOUT_MESC             100
+#define M_TX_RX_FIFO_SIZE            64
+
+enum bus_speed_index {
+	I2C_SPD_100K = 0,
+	I2C_SPD_400K,
+};
+
+struct bcm_iproc_i2c_dev {
+	struct device *device;
+
+	void __iomem *base;
+	struct i2c_msg *msg;
+
+	struct i2c_adapter adapter;
+
+	struct completion done;
+};
+
+/*
+ * Can be expanded in the future if more interrupt status bits are utilized
+ */
+#define ISR_MASK (1 << IS_M_START_BUSY_SHIFT)
+
+static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
+{
+	struct bcm_iproc_i2c_dev *dev = data;
+	u32 status = readl(dev->base + IS_OFFSET);
+
+	status &= ISR_MASK;
+
+	if (!status)
+		return IRQ_NONE;
+
+	writel(status, dev->base + IS_OFFSET);
+	complete_all(&dev->done);
+
+	return IRQ_HANDLED;
+}
+

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+	}
+
+	return 0;
+}
+

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+
+	if (msg->flags & I2C_M_RD)
+		*addr |= 1;
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;

drivers/i2c/busses/i2c-bcm-iproc.c new file +58 -0

+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_ADDR:
+		dev_err(dev->device, "NAK addr:0x%02x\n", dev->msg->addr);
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_DATA:
+		dev_err(dev->device, "NAK data\n");
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_TIMEOUT:
+		dev_err(dev->device, "bus timeout\n");
+		return -ETIMEDOUT;
+
+	default:
+		dev_err(dev->device, "unknown error code=%d\n", val);
+		return -EREMOTEIO;
+	}
+
+	return -EREMOTEIO;
+}
+
+static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *dev,
+					 struct i2c_msg *msg)
+{
+	int ret, i;
+	u8 addr;
+	u32 val;
+	unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC);
+
+	if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) {
+		dev_err(dev->device,
+			"supported data length is 1 - %u bytes\n",
+				M_TX_RX_FIFO_SIZE - 1);
+		return -EINVAL;
+	}
+
+	dev->msg = msg;
+	ret = __wait_for_bus_idle(dev);
+	if (ret)
+		return ret;
+
+	ret = bcm_iproc_i2c_format_addr(dev, msg, &addr);
+	if (ret)
+		return ret;
+
+	/* load slave address into the TX FIFO */
+	writel(addr, dev->base + M_TX_OFFSET);
+
+	/* for a write transaction, load data into the TX FIFO */
+	if (!(msg->flags & I2C_M_RD)) {
+		for (i = 0; i < msg->len; i++) {
+			val = msg->buf[i];
+
+			/* mark the last byte */
+			if (i == msg->len - 1)
+				val |= 1 << M_TX_WR_STATUS_SHIFT;
+

drivers/i2c/busses/i2c-bcm-iproc.c new file +86 -0

+	 */
+	writel(1 << IE_M_START_BUSY_SHIFT, dev->base + IE_OFFSET);
+
+	/*
+	 * Now we can activate the transfer. For a read operation, specify the
+	 * number of bytes to read
+	 */
+	val = 1 << M_CMD_START_BUSY_SHIFT;
+	if (msg->flags & I2C_M_RD) {
+		val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
+			(msg->len << M_CMD_RD_CNT_SHIFT);
+	} else {
+		val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
+	}
+	writel(val, dev->base + M_CMD_OFFSET);
+
+	time_left = wait_for_completion_timeout(&dev->done, time_left);
+
+	/* disable all interrupts */
+	writel(0, dev->base + IE_OFFSET);
+
+	if (!time_left) {
+		dev_err(dev->device, "transaction times out\n");
+
+		/* flush FIFOs */
+		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+			(1 << M_FIFO_TX_FLUSH_SHIFT);
+		writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+		return -EREMOTEIO;
+	}
+
+	ret = bcm_iproc_i2c_check_status(dev);
+	if (ret) {
+		/* flush both TX/RX FIFOs */
+		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+			(1 << M_FIFO_TX_FLUSH_SHIFT);
+		writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+		return ret;
+	}
+
+	/*
+	 * For a read operation, we now need to load the data from FIFO
+	 * into the memory buffer
+	 */
+	if (msg->flags & I2C_M_RD) {
+		for (i = 0; i < msg->len; i++) {
+			msg->buf[i] = (readl(dev->base + M_RX_OFFSET) >>
+					M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
+		}
+	}
+
+	dev_dbg(dev->device, "xfer %c, addr=0x%02x, len=%d\n",
+			(msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
+			msg->len);
+	dev_dbg(dev->device, "**** data start ****\n");
+	for (i = 0; i < msg->len; i++)
+		dev_dbg(dev->device, "0x%02x ",  msg->buf[i]);
+	dev_dbg(dev->device, "**** data end ****\n");
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
+			      struct i2c_msg msgs[], int num)
+{
+	struct bcm_iproc_i2c_dev *dev = i2c_get_adapdata(adapter);
+	int ret, i;
+
+	/* go through all messages */
+	for (i = 0; i < num; i++) {
+		ret = bcm_iproc_i2c_xfer_single_msg(dev, &msgs[i]);
+		if (ret) {
+			dev_err(dev->device, "xfer failed\n");
+			return ret;
+		}
+	}
+
+	return num;
+}
+
+static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm bcm_iproc_algo = {

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+	if (ret < 0) {
+		dev_err(dev->device, "missing clock-frequency property\n");
+		return -ENODEV;
+	}
+
+	switch (bus_speed) {
+	case 100000:
+		speed_bit = 0;

drivers/i2c/busses/i2c-bcm-iproc.c new file +16 -0

+
+	val = readl(dev->base + TIM_CFG_OFFSET);
+	val &= ~(1 << TIME_CFG_MODE_400_SHIFT);
+	val |= speed_bit << TIME_CFG_MODE_400_SHIFT;
+	writel(val, dev->base + TIM_CFG_OFFSET);
+
+	dev_info(dev->device, "bus set to %u Hz\n", bus_speed);
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;
+
+	/* put controller in reset */

drivers/i2c/busses/i2c-bcm-iproc.c new file +16 -0

+
+	return 0;
+}
+
+static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;
+
+	val = readl(dev->base + CFG_OFFSET);
+	val |= 1 << CFG_EN_SHIFT;
+	writel(val, dev->base + CFG_OFFSET);
+}
+
+static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+	struct i2c_adapter *adap;
+	struct resource *res;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)

drivers/i2c/busses/i2c-bcm-iproc.c new file +17 -0

+		dev_err(dev->device, "no irq resource\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, bcm_iproc_i2c_isr,
+			IRQF_SHARED, pdev->name, dev);
+	if (ret) {
+		dev_err(dev->device, "unable to request irq %i\n", irq);
+		return ret;
+	}
+
+	bcm_iproc_i2c_enable(dev);
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	strlcpy(adap->name, "Broadcom iProc I2C adapter", sizeof(adap->name));
+	adap->algo = &bcm_iproc_algo;

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+
+	return 0;
+}
+
+static const struct of_device_id bcm_iproc_i2c_of_match[] = {

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+	.probe = bcm_iproc_i2c_probe,
+	.remove = bcm_iproc_i2c_remove,
+};
+module_platform_driver(bcm_iproc_i2c_driver);
+
+MODULE_AUTHOR("Ray Jui <[email protected]>");
+MODULE_DESCRIPTION("Broadcom iProc I2C Driver");
+MODULE_LICENSE("GPL v2");

Hello,

Quoted 21 lines · Ray Jui

Add initial support to the Broadcom iProc I2C controller found in the iProc family of SoCs.

The iProc I2C controller has separate internal TX and RX FIFOs, each has a size of 64 bytes. The iProc I2C controller supports two bus speeds including standard mode (100kHz) and fast mode (400kHz)

Signed-off-by Ray Jui <[email protected]>

Reviewed-by Scott Branden <[email protected]>

---
 drivers/i2c/busses/Kconfig         |   10 +
 drivers/i2c/busses/Makefile        |    1 +
 drivers/i2c/busses/i2c-bcm-iproc.c |  500 ++++++++++++++++++++++++++++++++++++
 3 files changed, 511 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c

drivers/i2c/busses/Kconfig +0 -0

index c1351d9..df21366 100644

drivers/i2c/busses/Kconfig config I2C_BCM2835 · line 378

Quoted 7 lines

drivers/i2c/busses/Kconfig +4 -0

@@ -372,6 +372,16 @@ config I2C_BCM2835
 	  This support is also available as a module.  If so, the module
 	  will be called i2c-bcm2835.
 
+config I2C_BCM_IPROC
+	tristate "Broadcom iProc I2C controller"
+	depends on ARCH_BCM_IPROC
+	default y

It would be nice to have the following here to improve compile coverage testing:

	depends on ARCH_BCM_IPROC || COMPILE_TEST
	default ARCH_BCM_IPROC

Quoted 151 lines

drivers/i2c/busses/Kconfig +6 -0

+	help
+	  If you say yes to this option, support will be included for the
+	  Broadcom iProc I2C controller.
+
+	  If you don't know what to do here, say N.
+
 config I2C_BCM_KONA
 	tristate "BCM Kona I2C adapter"
 	depends on ARCH_BCM_MOBILE

drivers/i2c/busses/Makefile +1 -0

index 5e6c822..216e7be 100644
@@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_AXXIA)		+= i2c-axxia.o
 obj-$(CONFIG_I2C_BCM2835)	+= i2c-bcm2835.o
+obj-$(CONFIG_I2C_BCM_IPROC)	+= i2c-bcm-iproc.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CADENCE)	+= i2c-cadence.o
 obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-gpio.o

drivers/i2c/busses/i2c-bcm-iproc.c new file +124 -0

new file mode 100644
index 0000000..35ac497
@@ -0,0 +1,500 @@
+/*
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+
+#define CFG_OFFSET                   0x00
+#define CFG_RESET_SHIFT              31
+#define CFG_EN_SHIFT                 30
+#define CFG_M_RETRY_CNT_SHIFT        16
+#define CFG_M_RETRY_CNT_MASK         0x0f
+
+#define TIM_CFG_OFFSET               0x04
+#define TIME_CFG_MODE_400_SHIFT      31
+
+#define M_FIFO_CTRL_OFFSET           0x0c
+#define M_FIFO_RX_FLUSH_SHIFT        31
+#define M_FIFO_TX_FLUSH_SHIFT        30
+#define M_FIFO_RX_CNT_SHIFT          16
+#define M_FIFO_RX_CNT_MASK           0x7f
+#define M_FIFO_RX_THLD_SHIFT         8
+#define M_FIFO_RX_THLD_MASK          0x3f
+
+#define M_CMD_OFFSET                 0x30
+#define M_CMD_START_BUSY_SHIFT       31
+#define M_CMD_STATUS_SHIFT           25
+#define M_CMD_STATUS_MASK            0x07
+#define M_CMD_STATUS_SUCCESS         0x0
+#define M_CMD_STATUS_LOST_ARB        0x1
+#define M_CMD_STATUS_NACK_ADDR       0x2
+#define M_CMD_STATUS_NACK_DATA       0x3
+#define M_CMD_STATUS_TIMEOUT         0x4
+#define M_CMD_PROTOCOL_SHIFT         9
+#define M_CMD_PROTOCOL_MASK          0xf
+#define M_CMD_PROTOCOL_BLK_WR        0x7
+#define M_CMD_PROTOCOL_BLK_RD        0x8
+#define M_CMD_PEC_SHIFT              8
+#define M_CMD_RD_CNT_SHIFT           0
+#define M_CMD_RD_CNT_MASK            0xff
+
+#define IE_OFFSET                    0x38
+#define IE_M_RX_FIFO_FULL_SHIFT      31
+#define IE_M_RX_THLD_SHIFT           30
+#define IE_M_START_BUSY_SHIFT        28
+
+#define IS_OFFSET                    0x3c
+#define IS_M_RX_FIFO_FULL_SHIFT      31
+#define IS_M_RX_THLD_SHIFT           30
+#define IS_M_START_BUSY_SHIFT        28
+
+#define M_TX_OFFSET                  0x40
+#define M_TX_WR_STATUS_SHIFT         31
+#define M_TX_DATA_SHIFT              0
+#define M_TX_DATA_MASK               0xff
+
+#define M_RX_OFFSET                  0x44
+#define M_RX_STATUS_SHIFT            30
+#define M_RX_STATUS_MASK             0x03
+#define M_RX_PEC_ERR_SHIFT           29
+#define M_RX_DATA_SHIFT              0
+#define M_RX_DATA_MASK               0xff
+
+#define I2C_TIMEOUT_MESC             100
+#define M_TX_RX_FIFO_SIZE            64
+
+enum bus_speed_index {
+	I2C_SPD_100K = 0,
+	I2C_SPD_400K,
+};
+
+struct bcm_iproc_i2c_dev {
+	struct device *device;
+
+	void __iomem *base;
+	struct i2c_msg *msg;
+
+	struct i2c_adapter adapter;
+
+	struct completion done;
+};
+
+/*
+ * Can be expanded in the future if more interrupt status bits are utilized
+ */
+#define ISR_MASK (1 << IS_M_START_BUSY_SHIFT)
+
+static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
+{
+	struct bcm_iproc_i2c_dev *dev = data;
+	u32 status = readl(dev->base + IS_OFFSET);
+
+	status &= ISR_MASK;
+
+	if (!status)
+		return IRQ_NONE;
+
+	writel(status, dev->base + IS_OFFSET);
+	complete_all(&dev->done);
+
+	return IRQ_HANDLED;
+}
+
+static int __wait_for_bus_idle(struct bcm_iproc_i2c_dev *dev)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(I2C_TIMEOUT_MESC);

drivers/i2c/busses/i2c-bcm-iproc.c line 131

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+
+	while (readl(dev->base + M_CMD_OFFSET) &
+			(1 << M_CMD_START_BUSY_SHIFT)) {
+		if (time_after(jiffies, timeout)) {
+			dev_err(dev->device, "wait for bus idle timeout\n");
+			return -ETIMEDOUT;
+		}

Add a call to cpu_relax here. Do you really need a tight loop here?

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+	}
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *dev,
+				     struct i2c_msg *msg, u8 *addr)
+{

drivers/i2c/busses/i2c-bcm-iproc.c line 146

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+
+	if (msg->flags & I2C_M_TEN) {
+		dev_err(dev->device, "no support for 10-bit address\n");
+		return -EINVAL;
+	}
+
+	*addr = (msg->addr << 1) & 0xfe;

I don't see what difference the & 0xfe makes. I think you can drop that.

Quoted 14 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +14 -0

+
+	if (msg->flags & I2C_M_RD)
+		*addr |= 1;
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;
+
+	val = readl(dev->base + M_CMD_OFFSET);
+	val = (val >> M_CMD_STATUS_SHIFT) & M_CMD_STATUS_MASK;
+

drivers/i2c/busses/i2c-bcm-iproc.c line 166

Quoted 6 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +6 -0

+	switch (val) {
+	case M_CMD_STATUS_SUCCESS:
+		return 0;
+
+	case M_CMD_STATUS_LOST_ARB:
+		dev_err(dev->device, "lost bus arbitration\n");

I wouldn't dev_err that, only dev_dbg. I'm not sure how usual the errors for the next two cases is, maybe degrade them to dev_dbg, too?

Quoted 62 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +62 -0

+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_ADDR:
+		dev_err(dev->device, "NAK addr:0x%02x\n", dev->msg->addr);
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_DATA:
+		dev_err(dev->device, "NAK data\n");
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_TIMEOUT:
+		dev_err(dev->device, "bus timeout\n");
+		return -ETIMEDOUT;
+
+	default:
+		dev_err(dev->device, "unknown error code=%d\n", val);
+		return -EREMOTEIO;
+	}
+
+	return -EREMOTEIO;
+}
+
+static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *dev,
+					 struct i2c_msg *msg)
+{
+	int ret, i;
+	u8 addr;
+	u32 val;
+	unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MESC);
+
+	if (msg->len < 1 || msg->len > M_TX_RX_FIFO_SIZE - 1) {
+		dev_err(dev->device,
+			"supported data length is 1 - %u bytes\n",
+				M_TX_RX_FIFO_SIZE - 1);
+		return -EINVAL;
+	}
+
+	dev->msg = msg;
+	ret = __wait_for_bus_idle(dev);
+	if (ret)
+		return ret;
+
+	ret = bcm_iproc_i2c_format_addr(dev, msg, &addr);
+	if (ret)
+		return ret;
+
+	/* load slave address into the TX FIFO */
+	writel(addr, dev->base + M_TX_OFFSET);
+
+	/* for a write transaction, load data into the TX FIFO */
+	if (!(msg->flags & I2C_M_RD)) {
+		for (i = 0; i < msg->len; i++) {
+			val = msg->buf[i];
+
+			/* mark the last byte */
+			if (i == msg->len - 1)
+				val |= 1 << M_TX_WR_STATUS_SHIFT;
+
+			writel(val, dev->base + M_TX_OFFSET);
+		}
+	}
+

drivers/i2c/busses/i2c-bcm-iproc.c line 234

Quoted 6 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +6 -0

+	/* mark as incomplete before starting the transaction */
+	reinit_completion(&dev->done);
+
+	/*
+	 * Enable the "start busy" interrupt, which will be triggered after
+	 * the transaction is done

This sound's wrong. I'd expect "start busy" to trigger as soon as the controller gets hold of the bus.

Quoted 91 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +91 -0

+	 */
+	writel(1 << IE_M_START_BUSY_SHIFT, dev->base + IE_OFFSET);
+
+	/*
+	 * Now we can activate the transfer. For a read operation, specify the
+	 * number of bytes to read
+	 */
+	val = 1 << M_CMD_START_BUSY_SHIFT;
+	if (msg->flags & I2C_M_RD) {
+		val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
+			(msg->len << M_CMD_RD_CNT_SHIFT);
+	} else {
+		val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
+	}
+	writel(val, dev->base + M_CMD_OFFSET);
+
+	time_left = wait_for_completion_timeout(&dev->done, time_left);
+
+	/* disable all interrupts */
+	writel(0, dev->base + IE_OFFSET);
+
+	if (!time_left) {
+		dev_err(dev->device, "transaction times out\n");
+
+		/* flush FIFOs */
+		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+			(1 << M_FIFO_TX_FLUSH_SHIFT);
+		writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+		return -EREMOTEIO;
+	}
+
+	ret = bcm_iproc_i2c_check_status(dev);
+	if (ret) {
+		/* flush both TX/RX FIFOs */
+		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
+			(1 << M_FIFO_TX_FLUSH_SHIFT);
+		writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+		return ret;
+	}
+
+	/*
+	 * For a read operation, we now need to load the data from FIFO
+	 * into the memory buffer
+	 */
+	if (msg->flags & I2C_M_RD) {
+		for (i = 0; i < msg->len; i++) {
+			msg->buf[i] = (readl(dev->base + M_RX_OFFSET) >>
+					M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
+		}
+	}
+
+	dev_dbg(dev->device, "xfer %c, addr=0x%02x, len=%d\n",
+			(msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
+			msg->len);
+	dev_dbg(dev->device, "**** data start ****\n");
+	for (i = 0; i < msg->len; i++)
+		dev_dbg(dev->device, "0x%02x ",  msg->buf[i]);
+	dev_dbg(dev->device, "**** data end ****\n");
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
+			      struct i2c_msg msgs[], int num)
+{
+	struct bcm_iproc_i2c_dev *dev = i2c_get_adapdata(adapter);
+	int ret, i;
+
+	/* go through all messages */
+	for (i = 0; i < num; i++) {
+		ret = bcm_iproc_i2c_xfer_single_msg(dev, &msgs[i]);
+		if (ret) {
+			dev_err(dev->device, "xfer failed\n");
+			return ret;
+		}
+	}
+
+	return num;
+}
+
+static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm bcm_iproc_algo = {
+	.master_xfer = bcm_iproc_i2c_xfer,
+	.functionality = bcm_iproc_i2c_functionality,
+};
+
+static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *dev)

drivers/i2c/busses/i2c-bcm-iproc.c line 330

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+{
+	unsigned int bus_speed, speed_bit;
+	u32 val;
+	int ret = of_property_read_u32(dev->device->of_node, "clock-frequency",
+				       &bus_speed);

Inconsistent coding style. At most other places you use 2 tabs to indent a continued line. (Well you use this style for breaks in function declarations. Using the same style for both cases would be nice.)

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+	if (ret < 0) {
+		dev_err(dev->device, "missing clock-frequency property\n");
+		return -ENODEV;
+	}
+
+	switch (bus_speed) {
+	case 100000:
+		speed_bit = 0;

drivers/i2c/busses/i2c-bcm-iproc.c line 348

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+		break;
+	case 400000:
+		speed_bit = 1;
+		break;
+	default:
+		dev_err(dev->device, "%d Hz bus speed not supported\n",
+				bus_speed);
+		dev_err(dev->device, "valid speeds are 100khz and 400khz\n");
+		return -EINVAL;
+	}

I'd be more graceful here:

	if (bus_speed < 100000)
		error_out;
	else if (bus_speed < 400000)
		speed_bit = 0;
	else
		/* >= 400000 */
		speed_bit = 1;

Quoted 19 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +19 -0

+
+	val = readl(dev->base + TIM_CFG_OFFSET);
+	val &= ~(1 << TIME_CFG_MODE_400_SHIFT);
+	val |= speed_bit << TIME_CFG_MODE_400_SHIFT;
+	writel(val, dev->base + TIM_CFG_OFFSET);
+
+	dev_info(dev->device, "bus set to %u Hz\n", bus_speed);
+
+	return 0;
+}
+
+static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;
+
+	/* put controller in reset */
+	val = readl(dev->base + CFG_OFFSET);
+	val |= 1 << CFG_RESET_SHIFT;
+	val &= ~(1 << CFG_EN_SHIFT);

drivers/i2c/busses/i2c-bcm-iproc.c line 374

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+	writel(val, dev->base + CFG_OFFSET);
+
+	/* wait 100 usec per spec */
+	udelay(100);
+
+	/* bring controller out of reset */
+	val = readl(dev->base + CFG_OFFSET);

Is it necessary to reread the register value here?

drivers/i2c/busses/i2c-bcm-iproc.c line 384

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+	val &= ~(1 << CFG_RESET_SHIFT);
+	writel(val, dev->base + CFG_OFFSET);
+
+	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
+	val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
+	writel(val, dev->base + M_FIFO_CTRL_OFFSET);
+
+	/* disable all interrupts */
+	val = 0;
+	writel(val, dev->base + IE_OFFSET);

writel(0, dev->base + IE_OFFSET);

drivers/i2c/busses/i2c-bcm-iproc.c line 388

Quoted 3 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +3 -0

+	/* clear all pending interrupts */
+	val = readl(dev->base + IS_OFFSET);
+	writel(val, dev->base + IS_OFFSET);

writel(0xffffffff, dev->base + IS_OFFSET); ??

Quoted 16 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +16 -0

+
+	return 0;
+}
+
+static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;
+
+	val = readl(dev->base + CFG_OFFSET);
+	val |= 1 << CFG_EN_SHIFT;
+	writel(val, dev->base + CFG_OFFSET);
+}
+
+static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *dev)
+{
+	u32 val;

drivers/i2c/busses/i2c-bcm-iproc.c line 414

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +10 -0

+
+	val = readl(dev->base + CFG_OFFSET);
+	val &= ~(1 << CFG_EN_SHIFT);
+	writel(val, dev->base + CFG_OFFSET);
+}
+
+static int bcm_iproc_i2c_probe(struct platform_device *pdev)
+{
+	int irq, ret = 0;
+	struct bcm_iproc_i2c_dev *dev;

"dev" is a misleading name here. I'd call this

	bcm_iproc_i2c_ddata *ddata;

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+	struct i2c_adapter *adap;
+	struct resource *res;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, dev);

drivers/i2c/busses/i2c-bcm-iproc.c line 429

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+	dev->device = &pdev->dev;
+	init_completion(&dev->done);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dev->base = devm_ioremap_resource(dev->device, res);
+	if (IS_ERR(dev->base))
+		return -ENOMEM;

		return PTR_ERR(dev->base);

Quoted 4 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +4 -0

+
+	ret = bcm_iproc_i2c_init(dev);
+	if (ret)
+		return ret;

drivers/i2c/busses/i2c-bcm-iproc.c line 440

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+
+	ret = bcm_iproc_i2c_cfg_speed(dev);
+	if (ret)
+		return ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {

irq == 0 should be handled as error, too.

Quoted 20 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +20 -0

+		dev_err(dev->device, "no irq resource\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, bcm_iproc_i2c_isr,
+			IRQF_SHARED, pdev->name, dev);
+	if (ret) {
+		dev_err(dev->device, "unable to request irq %i\n", irq);
+		return ret;
+	}
+
+	bcm_iproc_i2c_enable(dev);
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	strlcpy(adap->name, "Broadcom iProc I2C adapter", sizeof(adap->name));
+	adap->algo = &bcm_iproc_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+

drivers/i2c/busses/i2c-bcm-iproc.c line 467

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+	ret = i2c_add_adapter(adap);
+	if (ret) {
+		dev_err(dev->device, "failed to add adapter\n");
+		return ret;
+	}
+
+	dev_info(dev->device, "device registered successfully\n");

This just clutters the boot log. Please remove.

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+
+	return 0;
+}
+
+static int bcm_iproc_i2c_remove(struct platform_device *pdev)

drivers/i2c/busses/i2c-bcm-iproc.c line 477

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +5 -0

+{
+	struct bcm_iproc_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+	bcm_iproc_i2c_disable(dev);

I think you have a problem here if bcm_iproc_i2c_remove is called while an irq is still being serviced. I'm not sure how to prevent this properly for a shared interrupt.

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+
+	return 0;
+}
+
+static const struct of_device_id bcm_iproc_i2c_of_match[] = {
+	{.compatible = "brcm,iproc-i2c",},
+	{},
+};

drivers/i2c/busses/i2c-bcm-iproc.c line 492

Quoted 7 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +7 -0

+MODULE_DEVICE_TABLE(of, bcm_iproc_i2c_of_match);
+
+static struct platform_driver bcm_iproc_i2c_driver = {
+	.driver = {
+		   .name = "bcm-iproc-i2c",
+		   .of_match_table = bcm_iproc_i2c_of_match,
+		   },

Inconsistent indention.

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+	.probe = bcm_iproc_i2c_probe,
+	.remove = bcm_iproc_i2c_remove,
+};
+module_platform_driver(bcm_iproc_i2c_driver);
+
+MODULE_AUTHOR("Ray Jui <[email protected]>");
+MODULE_DESCRIPTION("Broadcom iProc I2C Driver");
+MODULE_LICENSE("GPL v2");

Quoted 4 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 440

Quoted 2 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +2 -0

+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {

irq == 0 should be handled as error, too.

Ah. I thought zero is a valid global interrupt number, and I see other drivers checking against < 0 as well. Is my understanding incorrect?

Quoted 3 lines

Quoted 3 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +3 -0

+		dev_err(dev->device, "no irq resource\n");
+		return irq;
+	}

[...]

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 477

Quoted 6 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +6 -0

+static int bcm_iproc_i2c_remove(struct platform_device *pdev)
+{
+	struct bcm_iproc_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+	bcm_iproc_i2c_disable(dev);

I think you have a problem here if bcm_iproc_i2c_remove is called while an irq is still being serviced. I'm not sure how to prevent this properly for a shared interrupt.

Can I grab i2c_lock_adapter to ensure the bus is locked (so there's no outstanding transactions or IRQs by the time we remove the adapter)? But I see no I2C bus driver does this in their remove function...

Hello,

Quoted 4 lines · Ray Jui

Quoted 4 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 440

Quoted 2 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +2 -0

+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {

irq == 0 should be handled as error, too.

Quoted 2 lines

Ah. I thought zero is a valid global interrupt number, and I see other drivers checking against < 0 as well. Is my understanding incorrect?

These are wrong, too. 0 should never be a valid interrupt number. There are some exceptions but mostly for historic reasons. The right handling is used for example in drivers/i2c/busses/i2c-efm32.c.

Quoted 14 lines

Quoted 3 lines

Quoted 3 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +3 -0

+		dev_err(dev->device, "no irq resource\n");
+		return irq;
+	}

[...]

Quoted 10 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 477

Quoted 6 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +6 -0

+static int bcm_iproc_i2c_remove(struct platform_device *pdev)
+{
+	struct bcm_iproc_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&dev->adapter);
+	bcm_iproc_i2c_disable(dev);

I think you have a problem here if bcm_iproc_i2c_remove is called while an irq is still being serviced. I'm not sure how to prevent this properly for a shared interrupt.

Quoted 3 lines

Can I grab i2c_lock_adapter to ensure the bus is locked (so there's no outstanding transactions or IRQs by the time we remove the adapter)? But I see no I2C bus driver does this in their remove function...

The problem I pointed out is the reason for some driver authors not to use devm_request_irq. If you use plain request_irq and the matching free_irq in the .remove callback you can be sure that the irq isn't running any more as soon as free_irq returns.

BTW, if you use vim, you can add

set cinoptions=(,:
if has("autocmd")
filetype plugin indent on
endif

to your .vimrc. Then while typing vim does the indention right and consistent, and with the = command you can reindent.

Best regards
Uwe

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 166

Quoted 2 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +2 -0

+	case M_CMD_STATUS_LOST_ARB:
+		dev_err(dev->device, "lost bus arbitration\n");

I wouldn't dev_err that, only dev_dbg. I'm not sure how usual the errors for the next two cases is, maybe degrade them to dev_dbg, too?

These errors are rare, and it's nice to keep them at the dev_err level so the user will be more aware.

Quoted 14 lines

Quoted 14 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +13 -0

+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_ADDR:
+		dev_err(dev->device, "NAK addr:0x%02x\n", dev->msg->addr);
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_DATA:
+		dev_err(dev->device, "NAK data\n");
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_TIMEOUT:
+		dev_err(dev->device, "bus timeout\n");
+		return -ETIMEDOUT;

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 187

Quoted 8 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +8 -0

+
+	default:
+		dev_err(dev->device, "unknown error code=%d\n", val);
+		return -EREMOTEIO;
+	}
+
+	return -EREMOTEIO;
+}

Quoted 7 lines

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c line 166

Quoted 2 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +2 -0

+	case M_CMD_STATUS_LOST_ARB:
+		dev_err(dev->device, "lost bus arbitration\n");

I wouldn't dev_err that, only dev_dbg. I'm not sure how usual the errors for the next two cases is, maybe degrade them to dev_dbg, too?

These errors are rare, and it's nice to keep them at the dev_err level so the user will be more aware.

This is wrong. Arbitration lost and NACK is pretty standard stuff on an I2C bus. User doesn't need to know about it, it is just noise in the logs. Timeout is different, you can report that (although I should probably move such a message into the core). Please also use the proper errno codes defined in Documentation/i2c/fault-codes. They should be distinct enough to drop the messages.

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

drivers/i2c/busses/i2c-bcm-iproc.c new file +21 -0

+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_ADDR:
+		dev_err(dev->device, "NAK addr:0x%02x\n", dev->msg->addr);
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_NACK_DATA:
+		dev_err(dev->device, "NAK data\n");
+		return -EREMOTEIO;
+
+	case M_CMD_STATUS_TIMEOUT:
+		dev_err(dev->device, "bus timeout\n");
+		return -ETIMEDOUT;
+
+	default:
+		dev_err(dev->device, "unknown error code=%d\n", val);
+		return -EREMOTEIO;
+	}
+
+	return -EREMOTEIO;
+}

I guess you are probably extremely busy and might not have had a chance to look into the iProc I2C driver v3 patch that was submitted on December 9. I'm not trying to rush it, and am perfectly fine with waiting longer. I understand subsystem maintainers like you are usually very busy.

As I'm quite new to the upstreaming process, here I'd just like to find out in a bit more details on what to expect and how to proceed forward.

Hi,

please don't drop the i2c-list.

Quoted 4 lines

I guess you are probably extremely busy and might not have had a chance to look into the iProc I2C driver v3 patch that was submitted on December 9. I'm not trying to rush it, and am perfectly fine with waiting longer. I understand subsystem maintainers like you are usually very busy.

True, true.

Quoted 2 lines

As I'm quite new to the upstreaming process, here I'd just like to find out in a bit more details on what to expect and how to proceed forward.

We are in the merge window currently, and I will start looking at new patches probably around rc2 or so.

If you want to speed up things, check the reviews of other new drivers I did in the recent past and check if some of the remarks also apply to your driver.

Thanks,

Wolfram

Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt line 17

Quoted 5 lines · On Thursday 11 December 2014 16:05:04 Ray Jui wrote:

Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt new file +4 -0

+
+- linux,gpio-base:
+    Base GPIO number of this controller
+

We've NAK'ed properties like this multiple times before, and it doesn't get any better this time. What are you trying to achieve here?

Quoted 10 lines · Arnd Bergmann

Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt line 17

Quoted 5 lines · On Thursday 11 December 2014 16:05:04 Ray Jui wrote:

Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt new file +4 -0

+
+- linux,gpio-base:
+    Base GPIO number of this controller
+

We've NAK'ed properties like this multiple times before, and it doesn't get any better this time. What are you trying to achieve here?

I am to blame for suggesting using this property to Ray, and I am fully aware that this has been rejected before, but look at what people came with recently to palliate the lack of control over the GPIO number space for DT platforms:

http://www.spinics.net/lists/arm-kernel/msg384847.html
https://lkml.org/lkml/2014/12/10/133

Right now GPIO numbering for platforms using DT is a very inconsistent process, subject to change by the simple action of adjusting the value of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO controller, or changing the probe order of devices. For users of the integer or sysfs interfaces, this results in GPIO numbers that change, and drivers and/or user-space programs that behave incorrectly. Ironically, the only way to have consistent numbers is to use the old platform files, where you can specify the base number of a gpio_chip.

DT is actually probably not such a bad place to provide consistency in GPIO numbering. It has a global vision of the system layout, including all GPIO controllers and the number of GPIOs they include, and thus can make informed decisions. It provides a consistent result regardless of probe order. And allowing it to assign GPIO bases to controllers will free us from the nonsensical dependency of some arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it since we don't need it anymore after the removal of the global gpio_descs array. This will again interfere with the numbering of GPIO chips that do not have a base number provided.

Note that I don't really like this, either - but the problem is the GPIO integer interface. Until everyone has upgraded to gpiod and we have a replacement for the current sysfs interface (this will take a while) we have to cope with this. This issue has been bothering users for years, so this time I'd like to try and solve it the less ugly way. If there is a better solution, of course I'm all for it.

Quoted 41 lines · On Friday 12 December 2014 22:05:37 Alexandre Courbot wrote:

Quoted 10 lines · Arnd Bergmann

Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt line 17

Quoted 5 lines · On Thursday 11 December 2014 16:05:04 Ray Jui wrote:

Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt new file +4 -0

+
+- linux,gpio-base:
+    Base GPIO number of this controller
+

We've NAK'ed properties like this multiple times before, and it doesn't get any better this time. What are you trying to achieve here?

I am to blame for suggesting using this property to Ray, and I am fully aware that this has been rejected before, but look at what people came with recently to palliate the lack of control over the GPIO number space for DT platforms:

http://www.spinics.net/lists/arm-kernel/msg384847.html
https://lkml.org/lkml/2014/12/10/133

Right now GPIO numbering for platforms using DT is a very inconsistent process, subject to change by the simple action of adjusting the value of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO controller, or changing the probe order of devices. For users of the integer or sysfs interfaces, this results in GPIO numbers that change, and drivers and/or user-space programs that behave incorrectly. Ironically, the only way to have consistent numbers is to use the old platform files, where you can specify the base number of a gpio_chip.

DT is actually probably not such a bad place to provide consistency in GPIO numbering. It has a global vision of the system layout, including all GPIO controllers and the number of GPIOs they include, and thus can make informed decisions. It provides a consistent result regardless of probe order. And allowing it to assign GPIO bases to controllers will free us from the nonsensical dependency of some arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it since we don't need it anymore after the removal of the global gpio_descs array. This will again interfere with the numbering of GPIO chips that do not have a base number provided.

Quoted 6 lines

Note that I don't really like this, either - but the problem is the GPIO integer interface. Until everyone has upgraded to gpiod and we have a replacement for the current sysfs interface (this will take a while) we have to cope with this. This issue has been bothering users for years, so this time I'd like to try and solve it the less ugly way. If there is a better solution, of course I'm all for it.

I think the scheme will fail if you ever get gpio controllers that are not part of the DT: We have hotpluggable devices (PCI, USB, ...) that are not represented in DT and that may also provide GPIOs for internal uses.

The current state of affairs is definitely problematic, but defining the GPIO numbers in DT properties would only be a relative improvement, not a solution, and I fear it would make it harder to change the kernel to remove the gpio numbers eventually.

I wonder if we could instead come up with an approach that completely randomizes the gpio numbers (as a compile-time option) to find any places that still rely on specific numbers.

Arnd

Like I said previously, dynamic GPIO allocation works fine in the kernel, as long as all of our GPIO clients in the kernel use gpiod based API, which is what we will enforce going forward. The only problem is with some of our customers who use GPIO through sysfs and expect fixed global GPIO numbers. Thinking about this more, it's probably not that difficult to add a script for those customers to convert/map the GPIO numbers based on readings parsed from sysfs, so I guess that's fine.

Quoted 9 lines · On Monday 15 December 2014 13:35:47 Ray Jui wrote:

Like I said previously, dynamic GPIO allocation works fine in the kernel, as long as all of our GPIO clients in the kernel use gpiod based API, which is what we will enforce going forward. The only problem is with some of our customers who use GPIO through sysfs and expect fixed global GPIO numbers. Thinking about this more, it's probably not that difficult to add a script for those customers to convert/map the GPIO numbers based on readings parsed from sysfs, so I guess that's fine.