* [PATCH] i2c: add bcm283x i2c host controller support
@ 2022-06-11 21:28 Daniel Brát
2022-06-14 9:41 ` Sascha Hauer
2022-06-14 9:52 ` Ahmad Fatoum
0 siblings, 2 replies; 3+ messages in thread
From: Daniel Brát @ 2022-06-11 21:28 UTC (permalink / raw)
To: barebox; +Cc: Daniel Brát
Add a driver to support the i2c host controller (BSC)
found on bcm283x family os SoCs.
Signed-off-by: Daniel Brát <danek.brat@gmail.com>
---
arch/arm/configs/rpi_v8a_defconfig | 3 +-
drivers/i2c/busses/Kconfig | 4 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-bcm283x.c | 372 +++++++++++++++++++++++++++++
4 files changed, 379 insertions(+), 1 deletion(-)
create mode 100644 drivers/i2c/busses/i2c-bcm283x.c
diff --git a/arch/arm/configs/rpi_v8a_defconfig b/arch/arm/configs/rpi_v8a_defconfig
index 68cd2438b..e218311a7 100644
--- a/arch/arm/configs/rpi_v8a_defconfig
+++ b/arch/arm/configs/rpi_v8a_defconfig
@@ -81,6 +81,8 @@ CONFIG_SERIAL_AMBA_PL011=y
CONFIG_DRIVER_SERIAL_NS16550=y
CONFIG_NET_USB=y
CONFIG_NET_USB_SMSC95XX=y
+CONFIG_I2C=y
+CONFIG_I2C_BCM283X=y
CONFIG_USB_HOST=y
CONFIG_USB_DWC2_HOST=y
CONFIG_USB_DWC2_GADGET=y
@@ -108,4 +110,3 @@ CONFIG_FS_FAT=y
CONFIG_FS_FAT_WRITE=y
CONFIG_FS_FAT_LFN=y
CONFIG_ZLIB=y
-CONFIG_LZO_DECOMPRESS=y
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 58f865606..429f5ba42 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -17,6 +17,10 @@ config I2C_AT91
bool "AT91 I2C Master driver"
depends on ARCH_AT91
+config I2C_BCM283X
+ bool "BCM283X I2C Master driver"
+ depends on ARCH_BCM283X
+
config I2C_IMX
bool "MPC85xx/MPC5200/i.MX I2C Master driver"
depends on ARCH_IMX || ARCH_MPC85XX || ARCH_MPC5200 || ARCH_LAYERSCAPE
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index a8661f605..a1ab46fb2 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_I2C_AT91) += i2c-at91.o
+obj-$(CONFIG_I2C_BCM283X) += i2c-bcm283x.o
obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o
obj-$(CONFIG_I2C_IMX) += i2c-imx.o
lwl-$(CONFIG_I2C_IMX_EARLY) += i2c-imx-early.o
diff --git a/drivers/i2c/busses/i2c-bcm283x.c b/drivers/i2c/busses/i2c-bcm283x.c
new file mode 100644
index 000000000..dc9a2ea11
--- /dev/null
+++ b/drivers/i2c/busses/i2c-bcm283x.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * I2C bus driver for the BSC peripheral on Broadcom's bcm283x family of SoCs
+ *
+ * Based on documentation published by Raspberry Pi foundation and the kernel
+ * driver written by Stephen Warren.
+ *
+ * Copyright (C) Stephen Warren
+ * Copyright (C) 2022 Daniel Brát
+ */
+
+#include <common.h>
+#include <driver.h>
+#include <malloc.h>
+#include <i2c/i2c.h>
+#include <i2c/i2c-algo-bit.h>
+#include <linux/iopoll.h>
+#include <linux/clk.h>
+#include <init.h>
+#include <of_address.h>
+
+//#define DEBUG
+//#define LOGLEVEL 7
+
+// BSC C (Control) register
+#define BSC_C_READ BIT(0)
+#define BSC_C_CLEAR1 BIT(4)
+#define BSC_C_CLEAR2 BIT(5)
+#define BSC_C_ST BIT(7)
+#define BSC_C_INTD BIT(8)
+#define BSC_C_INTT BIT(9)
+#define BSC_C_INTR BIT(10)
+#define BSC_C_I2CEN BIT(15)
+
+// BSC S (Status) register
+#define BSC_S_TA BIT(0)
+#define BSC_S_DONE BIT(1)
+#define BSC_S_TXW BIT(2)
+#define BSC_S_RXR BIT(3)
+#define BSC_S_TXD BIT(4)
+#define BSC_S_RXD BIT(5)
+#define BSC_S_TXE BIT(6)
+#define BSC_S_RXF BIT(7)
+#define BSC_S_ERR BIT(8)
+#define BSC_S_CLKT BIT(9)
+
+// BSC A (Address) register
+#define BSC_A_MASK 0x7f
+
+// Constants
+#define BSC_CDIV_MIN 0x0002
+#define BSC_CDIV_MAX 0xfffe
+#define BSC_FIFO_SIZE 16U
+
+struct __packed bcm283x_i2c_regs {
+ u32 c;
+ u32 s;
+ u32 dlen;
+ u32 a;
+ u32 fifo;
+ u32 div;
+ u32 del;
+ u32 clkt;
+};
+
+struct bcm283x_i2c {
+ struct i2c_adapter adapter;
+ struct clk *mclk;
+ struct bcm283x_i2c_regs __iomem *regs;
+ u32 bitrate;
+};
+
+#define to_bcm283x_i2c(a) container_of(a, struct bcm283x_i2c, adapter)
+
+static inline int bcm283x_i2c_init(struct bcm283x_i2c *bcm_i2c)
+{
+ struct device_d *dev = &bcm_i2c->adapter.dev;
+ u32 mclk_rate, cdiv, redl, fedl;
+
+ /*
+ * Reset control reg, flush FIFO, clear all flags and disable
+ * clock stretching
+ */
+ writel(0UL, &bcm_i2c->regs->c);
+ writel(BSC_C_CLEAR1, &bcm_i2c->regs->c);
+ writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
+ writel(0UL, &bcm_i2c->regs->clkt);
+
+ /*
+ * Set the divider based on the master clock frequency and the
+ * requested i2c bitrate
+ */
+ mclk_rate = clk_get_rate(bcm_i2c->mclk);
+ cdiv = DIV_ROUND_UP(mclk_rate, bcm_i2c->bitrate);
+ dev_dbg(dev, "bcm283x_i2c_init: mclk_rate=%u, cdiv=%08x\n",
+ mclk_rate, cdiv);
+ /* Note from kernel driver:
+ * Per the datasheet, the register is always interpreted as an even
+ * number, by rounding down. In other words, the LSB is ignored. So,
+ * if the LSB is set, increment the divider to avoid any issue.
+ */
+ if (cdiv & 1)
+ cdiv++;
+ if ((cdiv < BSC_CDIV_MIN) || (cdiv > BSC_CDIV_MAX)) {
+ dev_err(dev, "failed to calculate valid clock divider value\n");
+ return -EINVAL;
+ }
+ dev_dbg(dev, "bcm283x_i2c_init: cdiv adjusted to %04x\n", cdiv);
+ fedl = max(cdiv / 16, 1U);
+ redl = max(cdiv / 4, 1U);
+ dev_dbg(dev, "bcm283x_i2c_init: fedl=%04x, redl=%04x\n", fedl, redl);
+ writel(cdiv & 0xffff, &bcm_i2c->regs->div);
+ writel((fedl << 16) | redl, &bcm_i2c->regs->del);
+ dev_dbg(dev, "bcm283x_i2c_init: regs->div=%08x, regs->del=%08x\n",
+ readl(&bcm_i2c->regs->div), readl(&bcm_i2c->regs->del));
+
+ return 0;
+}
+
+/*
+ * Macro to calculate generous timeout for given bitrate and number of bytes
+ */
+#define calc_byte_timeout_us(bitrate) \
+ (3 * 9 * DIV_ROUND_UP(1000000, bitrate))
+#define calc_msg_timeout_us(bitrate, bytes) \
+ ((bytes + 1) * calc_byte_timeout_us(bitrate))
+
+static int bcm283x_i2c_xfer(struct i2c_adapter *adapter,
+ struct i2c_msg *msgs, int count)
+{
+ int ret, i;
+ bool msg_read, msg_10bit;
+ struct i2c_msg *msg;
+ u16 buf_pos;
+ u32 reg_c, reg_s, reg_dlen, bytes_left, timeout, bulk_written;
+ struct device_d *dev = &adapter->dev;
+ struct bcm283x_i2c *bcm_i2c = to_bcm283x_i2c(adapter);
+ dev_dbg(dev, "bcm283x_i2c_xfer: count=%d\n", count);
+
+ /*
+ * Reset control reg, flush FIFO, clear flags and enable the BSC
+ */
+ writel(0UL, &bcm_i2c->regs->c);
+ writel(BSC_C_CLEAR1, &bcm_i2c->regs->c);
+ writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
+ writel(BSC_C_I2CEN, &bcm_i2c->regs->c);
+
+ for (i = 0; i < count; i++) {
+ msg = &msgs[i];
+ msg_read = (msg->flags & I2C_M_RD) > 0;
+ msg_10bit = (msg->flags & I2C_M_TEN) > 0;
+ reg_dlen = bytes_left = msg->len;
+ buf_pos = 0;
+ dev_dbg(dev, "bcm283x_i2c_xfer: msg #%d: "
+ "msg_read=%d, msg_10bit=%d, "
+ "reg_dlen=%u, buf_pos=%u\n",
+ i, msg_read, msg_10bit, reg_dlen, buf_pos);
+
+ if (msg_10bit && msg_read) {
+ timeout = calc_byte_timeout_us(bcm_i2c->bitrate);
+ writel(1UL, &bcm_i2c->regs->dlen);
+ writel(msg->addr & 0xff, &bcm_i2c->regs->fifo);
+ writel(((msg->addr >> 8) | 0x78) & BSC_A_MASK,
+ &bcm_i2c->regs->a);
+ writel(BSC_C_ST | BSC_C_I2CEN, &bcm_i2c->regs->c);
+ ret = readl_poll_timeout(&bcm_i2c->regs->s, reg_s,
+ reg_s & (BSC_S_TA | BSC_S_ERR),
+ timeout);
+ if (ret) {
+ dev_err(dev, "timeout: 10bit addr "
+ "read initilization\n");
+ goto out;
+ }
+ if (reg_s & BSC_S_ERR) {
+ dev_dbg(dev, "device with addr %x didnt ACK\n",
+ msg->addr);
+ ret = -EREMOTEIO;
+ goto out;
+ }
+ } else if (msg_10bit) {
+ reg_dlen++;
+ writel(msg->addr & 0xff, &bcm_i2c->regs->fifo);
+ writel(((msg->addr >> 8) | 0x78) & BSC_A_MASK,
+ &bcm_i2c->regs->a);
+ } else {
+ writel(msg->addr & BSC_A_MASK, &bcm_i2c->regs->a);
+ }
+
+ writel(reg_dlen, &bcm_i2c->regs->dlen);
+ reg_c = BSC_C_ST | BSC_C_I2CEN;
+ if (msg_read)
+ reg_c |= BSC_C_READ;
+ writel(reg_c, &bcm_i2c->regs->c);
+ dev_dbg(dev, "bcm283x_i2c_xfer: msg #%d: reg_c=%08x\n",
+ i, reg_c);
+
+ if (msg_read) {
+ /*
+ * Read out data from FIFO as soon as it is available
+ */
+ timeout = calc_byte_timeout_us(bcm_i2c->bitrate);
+ dev_dbg(dev, "bcm283x_i2c_xfer: msg #%d: "
+ "reading data from FIFO, timeout=%u, "
+ "bytes_left=%u\n",
+ i, timeout, bytes_left);
+ for (; bytes_left; bytes_left--) {
+ ret = readl_poll_timeout(&bcm_i2c->regs->s,
+ reg_s, reg_s &
+ (BSC_S_RXD | BSC_S_ERR),
+ timeout);
+ if (ret) {
+ dev_err(dev, "timeout: "
+ "waiting for data in FIFO\n");
+ goto out;
+ }
+ if (reg_s & BSC_S_ERR) {
+ ret = -EREMOTEIO;
+ goto out;
+ }
+ msg->buf[buf_pos++] =
+ (u8) readl(&bcm_i2c->regs->fifo);
+ dev_dbg(dev, "read %02x from FIFO",
+ msg->buf[buf_pos-1]);
+ }
+ } else {
+ /*
+ * Fit as much data to the FIFO as we can in one go
+ */
+ for (bulk_written = min(bytes_left, (msg_10bit ?
+ (BSC_FIFO_SIZE - 1U) : BSC_FIFO_SIZE));
+ bulk_written; bulk_written--, bytes_left--)
+ {
+ dev_dbg(dev, "writing %02x to FIFO\n",
+ msg->buf[buf_pos]);
+ writel(msg->buf[buf_pos++],
+ &bcm_i2c->regs->fifo);
+ }
+ timeout = calc_byte_timeout_us(bcm_i2c->bitrate);
+ /*
+ * Feed any remaining data to FIFO as soon as there
+ * is space for them
+ */
+ dev_dbg(dev, "remaining bytes after bulk write: %u\n",
+ bytes_left);
+ for (; bytes_left; bytes_left--) {
+ ret = readl_poll_timeout(&bcm_i2c->regs->s,
+ reg_s, reg_s &
+ (BSC_S_TXD | BSC_S_ERR),
+ timeout);
+ if (ret) {
+ dev_err(dev, "timeout: "
+ "waiting for space in FIFO\n");
+ goto out;
+ }
+ if (reg_s & BSC_S_ERR) {
+ dev_dbg(dev, "device with addr %x "
+ "didnt ACK\n", msg->addr);
+ ret = -EREMOTEIO;
+ goto out;
+ }
+ dev_dbg(dev, "writing %02x to FIFO\n",
+ msg->buf[buf_pos]);
+ writel(msg->buf[buf_pos++],
+ &bcm_i2c->regs->fifo);
+ }
+ }
+ /*
+ * Wait for the current transfer to finish and then flush FIFO
+ * and clear any flags so that we are ready for next msg
+ */
+ timeout = calc_msg_timeout_us(bcm_i2c->bitrate, reg_dlen);
+ ret = readl_poll_timeout(&bcm_i2c->regs->s, reg_s,
+ reg_s & (BSC_S_DONE | BSC_S_ERR), timeout);
+ if (ret) {
+ dev_err(dev, "timeout: waiting for transfer to end\n");
+ goto out;
+ }
+ if (reg_s & BSC_S_ERR) {
+ dev_dbg(dev, "device with addr %x didnt ACK\n",
+ msg->addr);
+ ret = -EREMOTEIO;
+ goto out;
+ }
+ writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
+ writel(BSC_C_CLEAR1 | BSC_C_I2CEN, &bcm_i2c->regs->c);
+ }
+ writel(0UL, &bcm_i2c->regs->c);
+ dev_dbg(dev, "bcm283x_i2c_xfer: returning successfully\n");
+ return count;
+out:
+ writel(0UL, &bcm_i2c->regs->c);
+ writel(BSC_C_CLEAR1, &bcm_i2c->regs->c);
+ writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
+ dev_dbg(dev, "bcm283x_i2c_xfer: returning via err, ret: %d\n", ret);
+ return ret;
+}
+
+static int bcm283x_i2c_probe(struct device_d *dev)
+{
+ int ret;
+ struct resource *iores;
+ struct bcm283x_i2c *bcm_i2c;
+ struct device_node *np = dev->device_node;
+
+ bcm_i2c = xzalloc(sizeof(*bcm_i2c));
+
+ if (!np) {
+ ret = -ENXIO;
+ goto err;
+ }
+
+ if (!IS_ENABLED(CONFIG_OFDEVICE)) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ iores = dev_request_mem_resource(dev, 0);
+ if (IS_ERR(iores)) {
+ dev_err(dev, "could not get iomem region\n");
+ ret = PTR_ERR(iores);
+ goto err;
+ }
+ bcm_i2c->regs = IOMEM(iores->start);
+ dev_dbg(dev, "bcm283x_i2c_probe: regs at %p\n", bcm_i2c->regs);
+
+ bcm_i2c->mclk = clk_get(dev, NULL);
+ if (IS_ERR(bcm_i2c->mclk)) {
+ dev_err(dev, "could not aquire clock\n");
+ ret = PTR_ERR(bcm_i2c->mclk);
+ goto err;
+ }
+ clk_enable(bcm_i2c->mclk);
+ dev_dbg(dev, "bcm283x_i2c_probe: enabled mclk\n");
+
+ bcm_i2c->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
+ of_property_read_u32(np, "clock-frequency", &bcm_i2c->bitrate);
+ if (bcm_i2c->bitrate > I2C_MAX_FAST_MODE_FREQ) {
+ dev_err(dev, "clock frequency of %u is not supported\n",
+ bcm_i2c->bitrate);
+ ret = -EINVAL;
+ goto err;
+ }
+ dev_dbg(dev, "bcm283x_i2c_probe: i2c_bitrate: %u\n", bcm_i2c->bitrate);
+
+ bcm_i2c->adapter.master_xfer = bcm283x_i2c_xfer;
+ bcm_i2c->adapter.nr = dev->id;
+ bcm_i2c->adapter.dev.parent = dev;
+ bcm_i2c->adapter.dev.device_node = np;
+
+ ret = bcm283x_i2c_init(bcm_i2c);
+ if (ret)
+ goto err;
+
+ return i2c_add_numbered_adapter(&bcm_i2c->adapter);
+err:
+ free(bcm_i2c);
+ dev_dbg(dev, "bcm283x_i2c_probe: exiting via err, ret: %d\n", ret);
+ return ret;
+}
+
+static struct of_device_id bcm283x_i2c_dt_ids[] = {
+ { .compatible = "brcm,bcm2835-i2c", },
+ { .compatible = "brcm,bcm2711-i2c", },
+ { /* sentinel */ }
+};
+
+static struct driver_d bcm283x_i2c_driver = {
+ .name = "i2c-bcm283x",
+ .probe = bcm283x_i2c_probe,
+ .of_compatible = DRV_OF_COMPAT(bcm283x_i2c_dt_ids),
+};
+device_platform_driver(bcm283x_i2c_driver);
--
2.17.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] i2c: add bcm283x i2c host controller support
2022-06-11 21:28 [PATCH] i2c: add bcm283x i2c host controller support Daniel Brát
@ 2022-06-14 9:41 ` Sascha Hauer
2022-06-14 9:52 ` Ahmad Fatoum
1 sibling, 0 replies; 3+ messages in thread
From: Sascha Hauer @ 2022-06-14 9:41 UTC (permalink / raw)
To: Daniel Brát; +Cc: barebox
On Sat, Jun 11, 2022 at 11:28:42PM +0200, Daniel Brát wrote:
> Add a driver to support the i2c host controller (BSC)
> found on bcm283x family os SoCs.
>
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
> arch/arm/configs/rpi_v8a_defconfig | 3 +-
> drivers/i2c/busses/Kconfig | 4 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-bcm283x.c | 372 +++++++++++++++++++++++++++++
> 4 files changed, 379 insertions(+), 1 deletion(-)
> create mode 100644 drivers/i2c/busses/i2c-bcm283x.c
>
> diff --git a/arch/arm/configs/rpi_v8a_defconfig b/arch/arm/configs/rpi_v8a_defconfig
> index 68cd2438b..e218311a7 100644
> --- a/arch/arm/configs/rpi_v8a_defconfig
> +++ b/arch/arm/configs/rpi_v8a_defconfig
> @@ -81,6 +81,8 @@ CONFIG_SERIAL_AMBA_PL011=y
> CONFIG_DRIVER_SERIAL_NS16550=y
> CONFIG_NET_USB=y
> CONFIG_NET_USB_SMSC95XX=y
> +CONFIG_I2C=y
> +CONFIG_I2C_BCM283X=y
> CONFIG_USB_HOST=y
> CONFIG_USB_DWC2_HOST=y
> CONFIG_USB_DWC2_GADGET=y
> @@ -108,4 +110,3 @@ CONFIG_FS_FAT=y
> CONFIG_FS_FAT_WRITE=y
> CONFIG_FS_FAT_LFN=y
> CONFIG_ZLIB=y
> -CONFIG_LZO_DECOMPRESS=y
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 58f865606..429f5ba42 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -17,6 +17,10 @@ config I2C_AT91
> bool "AT91 I2C Master driver"
> depends on ARCH_AT91
>
> +config I2C_BCM283X
> + bool "BCM283X I2C Master driver"
> + depends on ARCH_BCM283X
> +
> config I2C_IMX
> bool "MPC85xx/MPC5200/i.MX I2C Master driver"
> depends on ARCH_IMX || ARCH_MPC85XX || ARCH_MPC5200 || ARCH_LAYERSCAPE
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index a8661f605..a1ab46fb2 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> obj-$(CONFIG_I2C_AT91) += i2c-at91.o
> +obj-$(CONFIG_I2C_BCM283X) += i2c-bcm283x.o
> obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o
> obj-$(CONFIG_I2C_IMX) += i2c-imx.o
> lwl-$(CONFIG_I2C_IMX_EARLY) += i2c-imx-early.o
> diff --git a/drivers/i2c/busses/i2c-bcm283x.c b/drivers/i2c/busses/i2c-bcm283x.c
> new file mode 100644
> index 000000000..dc9a2ea11
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-bcm283x.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * I2C bus driver for the BSC peripheral on Broadcom's bcm283x family of SoCs
> + *
> + * Based on documentation published by Raspberry Pi foundation and the kernel
> + * driver written by Stephen Warren.
> + *
> + * Copyright (C) Stephen Warren
> + * Copyright (C) 2022 Daniel Brát
> + */
> +
> +#include <common.h>
> +#include <driver.h>
> +#include <malloc.h>
> +#include <i2c/i2c.h>
> +#include <i2c/i2c-algo-bit.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <init.h>
> +#include <of_address.h>
> +
> +//#define DEBUG
> +//#define LOGLEVEL 7
Please drop these lines. They are wrong after all. DEBUG has to be
defined before the includes and LOGLEVEL is unused.
> +
> +// BSC C (Control) register
> +#define BSC_C_READ BIT(0)
> +#define BSC_C_CLEAR1 BIT(4)
> +#define BSC_C_CLEAR2 BIT(5)
> +#define BSC_C_ST BIT(7)
> +#define BSC_C_INTD BIT(8)
> +#define BSC_C_INTT BIT(9)
> +#define BSC_C_INTR BIT(10)
> +#define BSC_C_I2CEN BIT(15)
> +
> +// BSC S (Status) register
> +#define BSC_S_TA BIT(0)
> +#define BSC_S_DONE BIT(1)
> +#define BSC_S_TXW BIT(2)
> +#define BSC_S_RXR BIT(3)
> +#define BSC_S_TXD BIT(4)
> +#define BSC_S_RXD BIT(5)
> +#define BSC_S_TXE BIT(6)
> +#define BSC_S_RXF BIT(7)
> +#define BSC_S_ERR BIT(8)
> +#define BSC_S_CLKT BIT(9)
> +
> +// BSC A (Address) register
> +#define BSC_A_MASK 0x7f
> +
> +// Constants
> +#define BSC_CDIV_MIN 0x0002
> +#define BSC_CDIV_MAX 0xfffe
> +#define BSC_FIFO_SIZE 16U
> +
> +struct __packed bcm283x_i2c_regs {
> + u32 c;
> + u32 s;
> + u32 dlen;
> + u32 a;
> + u32 fifo;
> + u32 div;
> + u32 del;
> + u32 clkt;
> +};
> +
> +struct bcm283x_i2c {
> + struct i2c_adapter adapter;
> + struct clk *mclk;
> + struct bcm283x_i2c_regs __iomem *regs;
> + u32 bitrate;
> +};
> +
> +#define to_bcm283x_i2c(a) container_of(a, struct bcm283x_i2c, adapter)
Please make this a static inline function for better type safety. Also
it makes it obvious which type 'a' must have.
> +
> +static inline int bcm283x_i2c_init(struct bcm283x_i2c *bcm_i2c)
> +{
> + struct device_d *dev = &bcm_i2c->adapter.dev;
> + u32 mclk_rate, cdiv, redl, fedl;
> +
> + /*
> + * Reset control reg, flush FIFO, clear all flags and disable
> + * clock stretching
> + */
> + writel(0UL, &bcm_i2c->regs->c);
> + writel(BSC_C_CLEAR1, &bcm_i2c->regs->c);
> + writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
> + writel(0UL, &bcm_i2c->regs->clkt);
> +
> + /*
> + * Set the divider based on the master clock frequency and the
> + * requested i2c bitrate
> + */
> + mclk_rate = clk_get_rate(bcm_i2c->mclk);
> + cdiv = DIV_ROUND_UP(mclk_rate, bcm_i2c->bitrate);
> + dev_dbg(dev, "bcm283x_i2c_init: mclk_rate=%u, cdiv=%08x\n",
> + mclk_rate, cdiv);
> + /* Note from kernel driver:
> + * Per the datasheet, the register is always interpreted as an even
> + * number, by rounding down. In other words, the LSB is ignored. So,
> + * if the LSB is set, increment the divider to avoid any issue.
> + */
> + if (cdiv & 1)
> + cdiv++;
> + if ((cdiv < BSC_CDIV_MIN) || (cdiv > BSC_CDIV_MAX)) {
> + dev_err(dev, "failed to calculate valid clock divider value\n");
> + return -EINVAL;
> + }
> + dev_dbg(dev, "bcm283x_i2c_init: cdiv adjusted to %04x\n", cdiv);
> + fedl = max(cdiv / 16, 1U);
> + redl = max(cdiv / 4, 1U);
> + dev_dbg(dev, "bcm283x_i2c_init: fedl=%04x, redl=%04x\n", fedl, redl);
> + writel(cdiv & 0xffff, &bcm_i2c->regs->div);
> + writel((fedl << 16) | redl, &bcm_i2c->regs->del);
> + dev_dbg(dev, "bcm283x_i2c_init: regs->div=%08x, regs->del=%08x\n",
> + readl(&bcm_i2c->regs->div), readl(&bcm_i2c->regs->del));
> +
> + return 0;
> +}
> +
> +/*
> + * Macro to calculate generous timeout for given bitrate and number of bytes
> + */
> +#define calc_byte_timeout_us(bitrate) \
> + (3 * 9 * DIV_ROUND_UP(1000000, bitrate))
> +#define calc_msg_timeout_us(bitrate, bytes) \
> + ((bytes + 1) * calc_byte_timeout_us(bitrate))
> +
> +static int bcm283x_i2c_xfer(struct i2c_adapter *adapter,
> + struct i2c_msg *msgs, int count)
> +{
> + int ret, i;
> + bool msg_read, msg_10bit;
> + struct i2c_msg *msg;
> + u16 buf_pos;
> + u32 reg_c, reg_s, reg_dlen, bytes_left, timeout, bulk_written;
> + struct device_d *dev = &adapter->dev;
> + struct bcm283x_i2c *bcm_i2c = to_bcm283x_i2c(adapter);
> + dev_dbg(dev, "bcm283x_i2c_xfer: count=%d\n", count);
> +
> + /*
> + * Reset control reg, flush FIFO, clear flags and enable the BSC
> + */
> + writel(0UL, &bcm_i2c->regs->c);
> + writel(BSC_C_CLEAR1, &bcm_i2c->regs->c);
> + writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
> + writel(BSC_C_I2CEN, &bcm_i2c->regs->c);
> +
> + for (i = 0; i < count; i++) {
You could move the body of this loop into an extra function. It saves
you one indentation level and thus gives you less linebreaks and better
readability.
> + msg = &msgs[i];
> + msg_read = (msg->flags & I2C_M_RD) > 0;
> + msg_10bit = (msg->flags & I2C_M_TEN) > 0;
> + reg_dlen = bytes_left = msg->len;
> + buf_pos = 0;
> + dev_dbg(dev, "bcm283x_i2c_xfer: msg #%d: "
> + "msg_read=%d, msg_10bit=%d, "
> + "reg_dlen=%u, buf_pos=%u\n",
> + i, msg_read, msg_10bit, reg_dlen, buf_pos);
> +
> + if (msg_10bit && msg_read) {
> + timeout = calc_byte_timeout_us(bcm_i2c->bitrate);
> + writel(1UL, &bcm_i2c->regs->dlen);
> + writel(msg->addr & 0xff, &bcm_i2c->regs->fifo);
> + writel(((msg->addr >> 8) | 0x78) & BSC_A_MASK,
> + &bcm_i2c->regs->a);
> + writel(BSC_C_ST | BSC_C_I2CEN, &bcm_i2c->regs->c);
> + ret = readl_poll_timeout(&bcm_i2c->regs->s, reg_s,
> + reg_s & (BSC_S_TA | BSC_S_ERR),
> + timeout);
> + if (ret) {
> + dev_err(dev, "timeout: 10bit addr "
> + "read initilization\n");
s/initilization/initialization/
> + goto out;
> + }
> + if (reg_s & BSC_S_ERR) {
> + dev_dbg(dev, "device with addr %x didnt ACK\n",
> + msg->addr);
> + ret = -EREMOTEIO;
> + goto out;
> + }
> + } else if (msg_10bit) {
> + reg_dlen++;
> + writel(msg->addr & 0xff, &bcm_i2c->regs->fifo);
> + writel(((msg->addr >> 8) | 0x78) & BSC_A_MASK,
> + &bcm_i2c->regs->a);
> + } else {
> + writel(msg->addr & BSC_A_MASK, &bcm_i2c->regs->a);
> + }
> +
> + writel(reg_dlen, &bcm_i2c->regs->dlen);
> + reg_c = BSC_C_ST | BSC_C_I2CEN;
> + if (msg_read)
> + reg_c |= BSC_C_READ;
> + writel(reg_c, &bcm_i2c->regs->c);
> + dev_dbg(dev, "bcm283x_i2c_xfer: msg #%d: reg_c=%08x\n",
> + i, reg_c);
This driver is far too chatty in debug mode for my taste. Could you
remove some of the messages? This particular one doesn't give any
additional information for example.
> +
> + if (msg_read) {
> + /*
> + * Read out data from FIFO as soon as it is available
> + */
> + timeout = calc_byte_timeout_us(bcm_i2c->bitrate);
> + dev_dbg(dev, "bcm283x_i2c_xfer: msg #%d: "
> + "reading data from FIFO, timeout=%u, "
> + "bytes_left=%u\n",
> + i, timeout, bytes_left);
Ditto. All information has been printed directly or indirectly before.
> + for (; bytes_left; bytes_left--) {
> + ret = readl_poll_timeout(&bcm_i2c->regs->s,
> + reg_s, reg_s &
> + (BSC_S_RXD | BSC_S_ERR),
> + timeout);
> + if (ret) {
> + dev_err(dev, "timeout: "
> + "waiting for data in FIFO\n");
> + goto out;
> + }
> + if (reg_s & BSC_S_ERR) {
> + ret = -EREMOTEIO;
> + goto out;
> + }
> + msg->buf[buf_pos++] =
> + (u8) readl(&bcm_i2c->regs->fifo);
> + dev_dbg(dev, "read %02x from FIFO",
> + msg->buf[buf_pos-1]);
> + }
> + } else {
> + /*
> + * Fit as much data to the FIFO as we can in one go
> + */
> + for (bulk_written = min(bytes_left, (msg_10bit ?
> + (BSC_FIFO_SIZE - 1U) : BSC_FIFO_SIZE));
> + bulk_written; bulk_written--, bytes_left--)
> + {
> + dev_dbg(dev, "writing %02x to FIFO\n",
> + msg->buf[buf_pos]);
> + writel(msg->buf[buf_pos++],
> + &bcm_i2c->regs->fifo);
> + }
Filling up the FIFO upfront looks like adding code for no gain. I think
you can drop this and only use the loop below.
> + timeout = calc_byte_timeout_us(bcm_i2c->bitrate);
> + /*
> + * Feed any remaining data to FIFO as soon as there
> + * is space for them
> + */
> + dev_dbg(dev, "remaining bytes after bulk write: %u\n",
> + bytes_left);
> + for (; bytes_left; bytes_left--) {
> + ret = readl_poll_timeout(&bcm_i2c->regs->s,
> + reg_s, reg_s &
> + (BSC_S_TXD | BSC_S_ERR),
> + timeout);
> + if (ret) {
> + dev_err(dev, "timeout: "
> + "waiting for space in FIFO\n");
> + goto out;
> + }
> + if (reg_s & BSC_S_ERR) {
> + dev_dbg(dev, "device with addr %x "
> + "didnt ACK\n", msg->addr);
> + ret = -EREMOTEIO;
> + goto out;
> + }
> + dev_dbg(dev, "writing %02x to FIFO\n",
> + msg->buf[buf_pos]);
> + writel(msg->buf[buf_pos++],
> + &bcm_i2c->regs->fifo);
> + }
> + }
> + /*
> + * Wait for the current transfer to finish and then flush FIFO
> + * and clear any flags so that we are ready for next msg
> + */
> + timeout = calc_msg_timeout_us(bcm_i2c->bitrate, reg_dlen);
> + ret = readl_poll_timeout(&bcm_i2c->regs->s, reg_s,
> + reg_s & (BSC_S_DONE | BSC_S_ERR), timeout);
> + if (ret) {
> + dev_err(dev, "timeout: waiting for transfer to end\n");
> + goto out;
> + }
> + if (reg_s & BSC_S_ERR) {
> + dev_dbg(dev, "device with addr %x didnt ACK\n",
> + msg->addr);
> + ret = -EREMOTEIO;
> + goto out;
> + }
> + writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
> + writel(BSC_C_CLEAR1 | BSC_C_I2CEN, &bcm_i2c->regs->c);
> + }
> + writel(0UL, &bcm_i2c->regs->c);
> + dev_dbg(dev, "bcm283x_i2c_xfer: returning successfully\n");
> + return count;
> +out:
> + writel(0UL, &bcm_i2c->regs->c);
> + writel(BSC_C_CLEAR1, &bcm_i2c->regs->c);
> + writel(BSC_S_DONE | BSC_S_ERR | BSC_S_CLKT, &bcm_i2c->regs->s);
> + dev_dbg(dev, "bcm283x_i2c_xfer: returning via err, ret: %d\n", ret);
> + return ret;
> +}
> +
> +static int bcm283x_i2c_probe(struct device_d *dev)
> +{
> + int ret;
> + struct resource *iores;
> + struct bcm283x_i2c *bcm_i2c;
> + struct device_node *np = dev->device_node;
> +
> + bcm_i2c = xzalloc(sizeof(*bcm_i2c));
> +
> + if (!np) {
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!IS_ENABLED(CONFIG_OFDEVICE)) {
> + ret = -ENODEV;
> + goto err;
> + }
That shouldn't be necessary. You already tested that you have a
device_node pointer which you will only get when CONFIG_OFDEVICE
is enabled.
> +
> + iores = dev_request_mem_resource(dev, 0);
> + if (IS_ERR(iores)) {
> + dev_err(dev, "could not get iomem region\n");
> + ret = PTR_ERR(iores);
> + goto err;
> + }
> + bcm_i2c->regs = IOMEM(iores->start);
> + dev_dbg(dev, "bcm283x_i2c_probe: regs at %p\n", bcm_i2c->regs);
Unncecessary, you can view the register in the output of the 'iomem'
command.
> +
> + bcm_i2c->mclk = clk_get(dev, NULL);
> + if (IS_ERR(bcm_i2c->mclk)) {
> + dev_err(dev, "could not aquire clock\n");
s/aquire/acquire/
> + ret = PTR_ERR(bcm_i2c->mclk);
> + goto err;
> + }
> + clk_enable(bcm_i2c->mclk);
> + dev_dbg(dev, "bcm283x_i2c_probe: enabled mclk\n");
Also unnecessary. The next message will tell you that you've been here
as well.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] i2c: add bcm283x i2c host controller support
2022-06-11 21:28 [PATCH] i2c: add bcm283x i2c host controller support Daniel Brát
2022-06-14 9:41 ` Sascha Hauer
@ 2022-06-14 9:52 ` Ahmad Fatoum
1 sibling, 0 replies; 3+ messages in thread
From: Ahmad Fatoum @ 2022-06-14 9:52 UTC (permalink / raw)
To: Daniel Brát, barebox
Hello Daniel,
Thanks for your patch.
On 11.06.22 23:28, Daniel Brát wrote:
> Add a driver to support the i2c host controller (BSC)
> found on bcm283x family os SoCs.
s/os/of/
>
> Signed-off-by: Daniel Brát <danek.brat@gmail.com>
> ---
> arch/arm/configs/rpi_v8a_defconfig | 3 +-
You can enable it for rpi_defconfig too
> drivers/i2c/busses/Kconfig | 4 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-bcm283x.c | 372 +++++++++++++++++++++++++++++
> 4 files changed, 379 insertions(+), 1 deletion(-)
> create mode 100644 drivers/i2c/busses/i2c-bcm283x.c
>
> diff --git a/arch/arm/configs/rpi_v8a_defconfig b/arch/arm/configs/rpi_v8a_defconfig
> index 68cd2438b..e218311a7 100644
> --- a/arch/arm/configs/rpi_v8a_defconfig
> +++ b/arch/arm/configs/rpi_v8a_defconfig
> @@ -81,6 +81,8 @@ CONFIG_SERIAL_AMBA_PL011=y
> CONFIG_DRIVER_SERIAL_NS16550=y
> CONFIG_NET_USB=y
> CONFIG_NET_USB_SMSC95XX=y
> +CONFIG_I2C=y
> +CONFIG_I2C_BCM283X=y
> CONFIG_USB_HOST=y
> CONFIG_USB_DWC2_HOST=y
> CONFIG_USB_DWC2_GADGET=y
> @@ -108,4 +110,3 @@ CONFIG_FS_FAT=y
> CONFIG_FS_FAT_WRITE=y
> CONFIG_FS_FAT_LFN=y
> CONFIG_ZLIB=y
> -CONFIG_LZO_DECOMPRESS=y
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 58f865606..429f5ba42 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -17,6 +17,10 @@ config I2C_AT91
> bool "AT91 I2C Master driver"
> depends on ARCH_AT91
>
> +config I2C_BCM283X
> + bool "BCM283X I2C Master driver"
> + depends on ARCH_BCM283X
|| COMPILE_TEST
> + if (ret) {
> + dev_err(dev, "timeout: 10bit addr "
> + "read initilization\n");
Don't line-break strings. It needlessly complicates searching for messages
in the code. Also you don't need to adhere religiously to 80 characters.
Up to 100 characters is usually ok. If you go beyond that, reevaluate
if it wouldn't be more readable to reduce indentation.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-14 9:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-11 21:28 [PATCH] i2c: add bcm283x i2c host controller support Daniel Brát
2022-06-14 9:41 ` Sascha Hauer
2022-06-14 9:52 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox