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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
