* [PATCH v2 0/5] Add i2c support for MediaTek mt8512
@ 2020-09-30 8:21 mingming lee
2020-09-30 8:21 ` [PATCH v2 1/5] i2c: mediatek: add basic driver support mingming lee
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: mingming lee @ 2020-09-30 8:21 UTC (permalink / raw)
To: u-boot
From: Mingming Lee <Mingming.Lee@mediatek.com>
This patch series adds basic i2c support for MediaTek MT8512 EMMC boards.
---
Changes for v2:
- using error number defined in include/linux/errno.h.
- add Mediatek i2c controller driver to ARM MEDIATEK in MAINTAINERS.
- add bindings for mediatek i2c driver.
Mingming Lee (5):
i2c: mediatek: add basic driver support
ARM: dts: Mediatek: add i2c node support for mt8512
configs: mt8512: Enable I2C related configs
dt-binding: i2c: add bindings for mediatek i2c driver
MAINTAINERS: add i2c driver to ARM MEDIATEK
MAINTAINERS | 2 +
arch/arm/dts/mt8512-bm1-emmc.dts | 12 +
arch/arm/dts/mt8512.dtsi | 38 +-
configs/mt8512_bm1_emmc_defconfig | 3 +
doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++
drivers/i2c/Kconfig | 9 +
drivers/i2c/Makefile | 1 +
drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++
8 files changed, 720 insertions(+), 1 deletion(-)
create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt
create mode 100644 drivers/i2c/mt_i2c.c
--
2.18.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/5] i2c: mediatek: add basic driver support
2020-09-30 8:21 [PATCH v2 0/5] Add i2c support for MediaTek mt8512 mingming lee
@ 2020-09-30 8:21 ` mingming lee
2020-10-06 0:02 ` Simon Glass
2020-09-30 8:21 ` [PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512 mingming lee
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: mingming lee @ 2020-09-30 8:21 UTC (permalink / raw)
To: u-boot
From: Mingming Lee <Mingming.Lee@mediatek.com>
Add MediaTek I2C basic driver
Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
Changes for v2:
- using error number defined in include/linux/errno.h
---
drivers/i2c/Kconfig | 9 +
drivers/i2c/Makefile | 1 +
drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 627 insertions(+)
create mode 100644 drivers/i2c/mt_i2c.c
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index dec6dc9dfa..103688ed36 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -159,6 +159,15 @@ config SYS_I2C_MESON
internal buffer holding up to 8 bytes for transfers and supports
both 7-bit and 10-bit addresses.
+config SYS_I2C_MTK
+ bool "MediaTek I2C driver"
+ default n
+ help
+ This selects the MediaTek Integrated Inter Circuit bus driver.
+ The I2C bus adapter is the base for some other I2C client, eg: touch, sensors.
+ If you want to use MediaTek I2C interface, say Y here.
+ If unsure, say N.
+
config SYS_I2C_MXC
bool "NXP MXC I2C driver"
help
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index e851ec462e..7227742f8d 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SYS_I2C_LPC32XX) += lpc32xx_i2c.o
obj-$(CONFIG_SYS_I2C_MESON) += meson_i2c.o
obj-$(CONFIG_SYS_I2C_MVTWSI) += mvtwsi.o
obj-$(CONFIG_SYS_I2C_MXC) += mxc_i2c.o
+obj-$(CONFIG_SYS_I2C_MTK) += mt_i2c.o
obj-$(CONFIG_SYS_I2C_NEXELL) += nx_i2c.o
obj-$(CONFIG_SYS_I2C_OCTEON) += octeon_i2c.o
obj-$(CONFIG_SYS_I2C_OMAP24XX) += omap24xx_i2c.o
diff --git a/drivers/i2c/mt_i2c.c b/drivers/i2c/mt_i2c.c
new file mode 100644
index 0000000000..be6262d3d2
--- /dev/null
+++ b/drivers/i2c/mt_i2c.c
@@ -0,0 +1,617 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MediaTek I2C Interface driver
+ *
+ * Copyright (C) 2020 MediaTek Inc.
+ * Author: Mingming Lee <Mingming.Lee@mediatek.com>
+ */
+
+#include <asm/cache.h>
+#include <asm/io.h>
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <i2c.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+
+#define I2C_OK 0
+
+#define I2C_RS_TRANSFER BIT(4)
+#define I2C_HS_NACKERR BIT(2)
+#define I2C_ACKERR BIT(1)
+#define I2C_TRANSAC_COMP BIT(0)
+#define I2C_TRANSAC_START BIT(0)
+#define I2C_RS_MUL_CNFG BIT(15)
+#define I2C_RS_MUL_TRIG BIT(14)
+#define I2C_DCM_DISABLE 0x0000
+#define I2C_IO_CONFIG_OPEN_DRAIN 0x0003
+#define I2C_IO_CONFIG_PUSH_PULL 0x0000
+#define I2C_SOFT_RST 0x0001
+#define I2C_FIFO_ADDR_CLR 0x0001
+#define I2C_DELAY_LEN 0x0002
+#define I2C_ST_START_CON 0x8001
+#define I2C_FS_START_CON 0x1800
+#define I2C_TIME_CLR_VALUE 0x0000
+#define I2C_TIME_DEFAULT_VALUE 0x0003
+#define I2C_WRRD_TRANAC_VALUE 0x0002
+#define I2C_RD_TRANAC_VALUE 0x0001
+
+#define I2C_DMA_CON_TX 0x0000
+#define I2C_DMA_CON_RX 0x0001
+#define I2C_DMA_START_EN 0x0001
+#define I2C_DMA_INT_FLAG_NONE 0x0000
+#define I2C_DMA_CLR_FLAG 0x0000
+#define I2C_DMA_HARD_RST 0x0002
+
+#define MAX_ST_MODE_SPEED 100000
+#define MAX_FS_MODE_SPEED 400000
+#define MAX_HS_MODE_SPEED 3400000
+#define MAX_SAMPLE_CNT_DIV 8
+#define MAX_STEP_CNT_DIV 64
+#define MAX_HS_STEP_CNT_DIV 8
+#define I2C_DEFAULT_CLK_DIV 4
+
+#define MAX_I2C_ADDR 0x7F
+#define MAX_I2C_LEN 0xFF
+#define TRANS_ADDR_ONLY BIT(8)
+#define TRANSFER_TIMEOUT 50000 /* us */
+#define I2C_FIFO_STAT1_MASK 0x001F
+#define TIMING_SAMPLE_OFFSET 8
+#define HS_SAMPLE_OFFSET 12
+#define HS_STEP_OFFSET 8
+
+#define I2C_CONTROL_WRAPPER BIT(0)
+#define I2C_CONTROL_RS BIT(1)
+#define I2C_CONTROL_DMA_EN BIT(2)
+#define I2C_CONTROL_CLK_EXT_EN BIT(3)
+#define I2C_CONTROL_DIR_CHANGE BIT(4)
+#define I2C_CONTROL_ACKERR_DET_EN BIT(5)
+#define I2C_CONTROL_TRANSFER_LEN_CHANGE BIT(6)
+#define I2C_CONTROL_DMAACK BIT(8)
+#define I2C_CONTROL_ASYNC BIT(9)
+
+enum I2C_REGS_OFFSET {
+ OFFSET_PORT = 0x0,
+ OFFSET_SLAVE_ADDR = 0x04,
+ OFFSET_INTR_MASK = 0x08,
+ OFFSET_INTR_STAT = 0x0C,
+ OFFSET_CONTROL = 0x10,
+ OFFSET_TRANSFER_LEN = 0x14,
+ OFFSET_TRANSAC_LEN = 0x18,
+ OFFSET_DELAY_LEN = 0x1C,
+ OFFSET_TIMING = 0x20,
+ OFFSET_START = 0x24,
+ OFFSET_EXT_CONF = 0x28,
+ OFFSET_FIFO_STAT1 = 0x2C,
+ OFFSET_FIFO_STAT = 0x30,
+ OFFSET_FIFO_THRESH = 0x34,
+ OFFSET_FIFO_ADDR_CLR = 0x38,
+ OFFSET_IO_CONFIG = 0x40,
+ OFFSET_RSV_DEBUG = 0x44,
+ OFFSET_HS = 0x48,
+ OFFSET_SOFTRESET = 0x50,
+ OFFSET_DCM_EN = 0x54,
+ OFFSET_DEBUGSTAT = 0x64,
+ OFFSET_DEBUGCTRL = 0x68,
+ OFFSET_TRANSFER_LEN_AUX = 0x6C,
+ OFFSET_CLOCK_DIV = 0x70,
+ OFFSET_SCL_HL_RATIO = 0x74,
+ OFFSET_SCL_HS_HL_RATIO = 0x78,
+ OFFSET_SCL_MIS_COMP_POINT = 0x7C,
+ OFFSET_STA_STOP_AC_TIME = 0x80,
+ OFFSET_HS_STA_STOP_AC_TIME = 0x84,
+ OFFSET_DATA_TIME = 0x88,
+};
+
+enum DMA_REGS_OFFSET {
+ OFFSET_INT_FLAG = 0x0,
+ OFFSET_INT_EN = 0x04,
+ OFFSET_EN = 0x08,
+ OFFSET_RST = 0x0C,
+ OFFSET_CON = 0x18,
+ OFFSET_TX_MEM_ADDR = 0x1C,
+ OFFSET_RX_MEM_ADDR = 0x20,
+ OFFSET_TX_LEN = 0x24,
+ OFFSET_RX_LEN = 0x28,
+};
+
+enum mtk_trans_op {
+ I2C_MASTER_WR = 1,
+ I2C_MASTER_RD,
+ I2C_MASTER_WRRD,
+};
+
+struct mtk_i2c_soc_data {
+ u8 dma_sync: 1;
+};
+
+struct mtk_i2c_priv {
+ /* set in i2c probe */
+ void __iomem *base; /* i2c base addr */
+ void __iomem *pdmabase; /* dma base address*/
+ struct clk clk_main; /* main clock for i2c bus */
+ struct clk clk_dma; /* DMA clock for i2c via DMA */
+ const struct mtk_i2c_soc_data *soc_data;
+ enum mtk_trans_op op;
+ bool zero_len;
+ bool pushpull; /* open drain */
+ bool filter_msg; /* filter msg error log */
+ bool auto_restart;
+ bool ignore_restart_irq;
+ u32 speed; /* hz */
+ u8 *tx_buff;
+ u8 *rx_buff;
+};
+
+static inline void i2c_writel(struct mtk_i2c_priv *priv, u8 offset, u32 value)
+{
+ writel(value, (priv->base + offset));
+}
+
+static inline u32 i2c_readl(struct mtk_i2c_priv *priv, u8 offset)
+{
+ return readl(priv->base + offset);
+}
+
+static void mtk_i2c_clk_enable(struct mtk_i2c_priv *priv)
+{
+ clk_enable(&priv->clk_main);
+ clk_enable(&priv->clk_dma);
+}
+
+static void mtk_i2c_clk_disable(struct mtk_i2c_priv *priv)
+{
+ clk_disable(&priv->clk_dma);
+ clk_disable(&priv->clk_main);
+}
+
+static void mtk_i2c_init_hw(struct mtk_i2c_priv *priv)
+{
+ u16 control_reg;
+
+ writel(I2C_DMA_HARD_RST, priv->pdmabase + OFFSET_RST);
+ writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_RST);
+ i2c_writel(priv, OFFSET_SOFTRESET, I2C_SOFT_RST);
+ /* set ioconfig */
+ if (priv->pushpull)
+ i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_PUSH_PULL);
+ else
+ i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_OPEN_DRAIN);
+
+ i2c_writel(priv, OFFSET_DCM_EN, I2C_DCM_DISABLE);
+ control_reg = I2C_CONTROL_ACKERR_DET_EN | I2C_CONTROL_CLK_EXT_EN;
+ if (priv->soc_data->dma_sync)
+ control_reg |= I2C_CONTROL_DMAACK | I2C_CONTROL_ASYNC;
+ i2c_writel(priv, OFFSET_CONTROL, control_reg);
+ i2c_writel(priv, OFFSET_DELAY_LEN, I2C_DELAY_LEN);
+}
+
+static int mtk_i2c_calculate_speed(struct mtk_i2c_priv *priv,
+ u32 clk_src,
+ u32 target_speed,
+ u32 *timing_step_cnt,
+ u32 *timing_sample_cnt)
+{
+ u32 step_cnt;
+ u32 sample_cnt;
+ u32 max_step_cnt;
+ u32 base_sample_cnt = MAX_SAMPLE_CNT_DIV;
+ u32 base_step_cnt;
+ u32 opt_div;
+ u32 best_mul;
+ u32 cnt_mul;
+
+ if (target_speed > MAX_HS_MODE_SPEED)
+ target_speed = MAX_HS_MODE_SPEED;
+
+ if (target_speed > MAX_FS_MODE_SPEED)
+ max_step_cnt = MAX_HS_STEP_CNT_DIV;
+ else
+ max_step_cnt = MAX_STEP_CNT_DIV;
+
+ base_step_cnt = max_step_cnt;
+ /* Find the best combination */
+ opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
+ best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
+
+ /* Search for the best pair (sample_cnt, step_cnt) with
+ * 0 < sample_cnt < MAX_SAMPLE_CNT_DIV
+ * 0 < step_cnt < max_step_cnt
+ * sample_cnt * step_cnt >= opt_div
+ * optimizing for sample_cnt * step_cnt being minimal
+ */
+ for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
+ step_cnt = DIV_ROUND_UP(opt_div, sample_cnt);
+ cnt_mul = step_cnt * sample_cnt;
+ if (step_cnt > max_step_cnt)
+ continue;
+
+ if (cnt_mul < best_mul) {
+ best_mul = cnt_mul;
+ base_sample_cnt = sample_cnt;
+ base_step_cnt = step_cnt;
+ if (best_mul == opt_div)
+ break;
+ }
+ }
+
+ sample_cnt = base_sample_cnt;
+ step_cnt = base_step_cnt;
+
+ if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
+ /* In this case, hardware can't support such
+ * low i2c_bus_freq
+ */
+ debug("Unsupported speed(%uhz)\n", target_speed);
+ return -EINVAL;
+ }
+
+ *timing_step_cnt = step_cnt - 1;
+ *timing_sample_cnt = sample_cnt - 1;
+ return 0;
+}
+
+static int mtk_i2c_set_speed(struct udevice *dev, uint speed)
+{
+ u32 clk_src;
+ u32 step_cnt;
+ u32 sample_cnt;
+ u16 timing_reg;
+ u16 high_speed_reg;
+ int ret = 0;
+ struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+ priv->speed = speed;
+ mtk_i2c_clk_enable(priv);
+ clk_src = clk_get_rate(&priv->clk_main) /
+ I2C_DEFAULT_CLK_DIV;
+ i2c_writel(priv, OFFSET_CLOCK_DIV, (I2C_DEFAULT_CLK_DIV - 1));
+ if (priv->speed > MAX_FS_MODE_SPEED) {
+ /* Set master code speed register */
+ ret = mtk_i2c_calculate_speed(priv, clk_src, MAX_FS_MODE_SPEED,
+ &step_cnt, &sample_cnt);
+ if (ret < 0)
+ goto exit;
+
+ timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
+ writel(timing_reg, priv->base + OFFSET_TIMING);
+ /* Set the high speed mode register */
+ ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
+ &step_cnt, &sample_cnt);
+ if (ret < 0)
+ goto exit;
+
+ high_speed_reg = I2C_TIME_DEFAULT_VALUE |
+ (sample_cnt << HS_SAMPLE_OFFSET) |
+ (step_cnt << HS_STEP_OFFSET);
+ writel(high_speed_reg, priv->base + OFFSET_HS);
+ } else {
+ ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
+ &step_cnt, &sample_cnt);
+ if (ret < 0)
+ goto exit;
+
+ timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
+ /* Disable the high speed transaction */
+ high_speed_reg = I2C_TIME_CLR_VALUE;
+ writel(timing_reg, priv->base + OFFSET_TIMING);
+ writel(high_speed_reg, priv->base + OFFSET_HS);
+ }
+exit:
+ mtk_i2c_clk_disable(priv);
+ return ret;
+}
+
+static int mtk_i2c_do_transfer(struct mtk_i2c_priv *priv,
+ struct i2c_msg *msgs,
+ int num, int left_num)
+{
+ u16 addr_reg;
+ u16 start_reg;
+ u16 control_reg;
+ u16 restart_flag = 0;
+ u16 irq_stat = 0;
+ u8 trans_error = 0;
+ bool tmo = false;
+ u8 *ptr = msgs->buf;
+ u16 data_size = msgs->len;
+ u32 tmo_poll = 0;
+ int ret;
+
+ if (priv->auto_restart)
+ restart_flag = I2C_RS_TRANSFER;
+
+ control_reg = i2c_readl(priv, OFFSET_CONTROL) &
+ ~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
+
+ if (priv->speed > MAX_FS_MODE_SPEED || num > 1)
+ control_reg |= I2C_CONTROL_RS;
+
+ if (priv->op == I2C_MASTER_WRRD)
+ control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
+ control_reg |= I2C_CONTROL_DMA_EN;
+ i2c_writel(priv, OFFSET_CONTROL, control_reg);
+
+ /* set start condition */
+ if (priv->speed <= MAX_ST_MODE_SPEED)
+ i2c_writel(priv, OFFSET_EXT_CONF, I2C_ST_START_CON);
+ else
+ i2c_writel(priv, OFFSET_EXT_CONF, I2C_FS_START_CON);
+
+ addr_reg = msgs->addr << 1;
+ if (priv->op == I2C_MASTER_RD)
+ addr_reg |= I2C_M_RD;
+ if (priv->zero_len)
+ i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg | TRANS_ADDR_ONLY);
+ else
+ i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg);
+
+ /* clear interrupt status */
+ i2c_writel(priv, OFFSET_INTR_STAT, restart_flag | I2C_HS_NACKERR |
+ I2C_ACKERR | I2C_TRANSAC_COMP);
+ i2c_writel(priv, OFFSET_FIFO_ADDR_CLR, I2C_FIFO_ADDR_CLR);
+
+ /* enable interrupt */
+ i2c_writel(priv, OFFSET_INTR_MASK, restart_flag | I2C_HS_NACKERR |
+ I2C_ACKERR | I2C_TRANSAC_COMP);
+
+ /* set transfer and transaction len */
+ if (priv->op == I2C_MASTER_WRRD) {
+ i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
+ i2c_writel(priv, OFFSET_TRANSFER_LEN_AUX,
+ (msgs + 1)->len);
+ i2c_writel(priv, OFFSET_TRANSAC_LEN, I2C_WRRD_TRANAC_VALUE);
+ } else {
+ i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
+ i2c_writel(priv, OFFSET_TRANSAC_LEN, num);
+ }
+
+ /* prepare buffer data to start transfer */
+ if (priv->op == I2C_MASTER_RD) {
+ writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
+ OFFSET_INT_FLAG);
+ writel(I2C_DMA_CON_RX, priv->pdmabase + OFFSET_CON);
+ priv->rx_buff = (u8 *)noncached_alloc(msgs->len,
+ ARCH_DMA_MINALIGN);
+ writel((u32)priv->rx_buff, priv->pdmabase +
+ OFFSET_RX_MEM_ADDR);
+ writel(msgs->len, priv->pdmabase + OFFSET_RX_LEN);
+ } else if (priv->op == I2C_MASTER_WR) {
+ writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
+ OFFSET_INT_FLAG);
+ writel(I2C_DMA_CON_TX, priv->pdmabase + OFFSET_CON);
+ priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
+ ARCH_DMA_MINALIGN);
+ memcpy(priv->tx_buff, msgs->buf, msgs->len);
+ writel((u32)priv->tx_buff, priv->pdmabase +
+ OFFSET_TX_MEM_ADDR);
+ writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
+ } else {
+ writel(I2C_DMA_CLR_FLAG, priv->pdmabase +
+ OFFSET_INT_FLAG);
+ writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_CON);
+ priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
+ ARCH_DMA_MINALIGN);
+ priv->rx_buff = (u8 *)noncached_alloc((msgs + 1)->len,
+ ARCH_DMA_MINALIGN);
+ memcpy(priv->tx_buff, msgs->buf, msgs->len);
+ writel((u32)priv->tx_buff, priv->pdmabase +
+ OFFSET_TX_MEM_ADDR);
+ writel((u32)priv->rx_buff, priv->pdmabase +
+ OFFSET_RX_MEM_ADDR);
+ writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
+ writel((msgs + 1)->len, priv->pdmabase +
+ OFFSET_RX_LEN);
+ }
+ writel(I2C_DMA_START_EN, priv->pdmabase + OFFSET_EN);
+
+ if (!priv->auto_restart) {
+ start_reg = I2C_TRANSAC_START;
+ } else {
+ start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
+ if (left_num >= 1)
+ start_reg |= I2C_RS_MUL_CNFG;
+ }
+ i2c_writel(priv, OFFSET_START, start_reg);
+
+ for (;;) {
+ irq_stat = i2c_readl(priv, OFFSET_INTR_STAT);
+
+ /* ignore the first restart irq after the master code */
+ if (priv->ignore_restart_irq && (irq_stat | restart_flag)) {
+ priv->ignore_restart_irq = false;
+ irq_stat = 0;
+ writel(I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
+ I2C_TRANSAC_START, priv->base + OFFSET_START);
+ }
+
+ if (irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
+ tmo = false;
+ if (irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))
+ trans_error = 1;
+
+ break;
+ }
+ udelay(1);
+ if (tmo_poll++ >= TRANSFER_TIMEOUT) {
+ tmo = true;
+ break;
+ }
+ }
+ /* clear interrupt mask */
+ i2c_writel(priv, OFFSET_INTR_MASK, ~(restart_flag | I2C_HS_NACKERR |
+ I2C_ACKERR | I2C_TRANSAC_COMP));
+
+ if (!tmo && trans_error == 0) {
+ /* transfer success ,we need to get data from fifo */
+ if (priv->op == I2C_MASTER_RD || priv->op == I2C_MASTER_WRRD) {
+ ptr = priv->op == I2C_MASTER_RD ?
+ msgs->buf : (msgs + 1)->buf;
+ data_size = priv->op == I2C_MASTER_RD ?
+ msgs->len : (msgs + 1)->len;
+ memcpy(ptr, priv->rx_buff, data_size);
+ }
+ } else {
+ /* timeout or ACKERR */
+ if (tmo) {
+ ret = -ETIMEDOUT;
+ if (!priv->filter_msg)
+ debug("transfer timeout! addr: 0x%x,\n",
+ msgs->addr);
+ } else {
+ ret = -ENXIO;
+ if (!priv->filter_msg)
+ debug("ACKERR! addr: 0x%x,IRQ:0x%x\n",
+ msgs->addr, irq_stat);
+ }
+ mtk_i2c_init_hw(priv);
+ return ret;
+ }
+ return 0;
+}
+
+static int mtk_i2c_transfer(struct udevice *dev, struct i2c_msg *msg,
+ int nmsgs)
+{
+ int ret;
+ int left_num;
+ u8 num_cnt;
+ struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+ priv->auto_restart = true;
+ left_num = nmsgs;
+ mtk_i2c_clk_enable(priv);
+ for (num_cnt = 0; num_cnt < nmsgs; num_cnt++) {
+ if (((msg + num_cnt)->addr) > MAX_I2C_ADDR) {
+ ret = -EINVAL;
+ goto err_exit;
+ }
+ if ((msg + num_cnt)->len > MAX_I2C_LEN) {
+ ret = -EINVAL;
+ goto err_exit;
+ }
+ }
+
+ /* checking if we can skip restart and optimize using WRRD mode */
+ if (priv->auto_restart && nmsgs == 2) {
+ if (!(msg[0].flags & I2C_M_RD) && (msg[1].flags & I2C_M_RD) &&
+ msg[0].addr == msg[1].addr) {
+ priv->auto_restart = false;
+ }
+ }
+
+ if (priv->auto_restart && nmsgs >= 2 && priv->speed > MAX_FS_MODE_SPEED)
+ /* ignore the first restart irq after the master code,
+ * otherwise the first transfer will be discarded.
+ */
+ priv->ignore_restart_irq = true;
+ else
+ priv->ignore_restart_irq = false;
+
+ while (left_num--) {
+ /*priv->zero_len = true
+ *transfer slave address only to support devices detect
+ */
+ if (!msg->buf)
+ priv->zero_len = true;
+ else
+ priv->zero_len = false;
+
+ if (msg->flags & I2C_M_RD)
+ priv->op = I2C_MASTER_RD;
+ else
+ priv->op = I2C_MASTER_WR;
+
+ if (!priv->auto_restart) {
+ if (nmsgs > 1) {
+ /* combined two messages into one transaction */
+ priv->op = I2C_MASTER_WRRD;
+ left_num--;
+ }
+ }
+ ret = mtk_i2c_do_transfer(priv, msg, nmsgs, left_num);
+ if (ret < 0)
+ goto err_exit;
+ msg++;
+ }
+ ret = I2C_OK;
+
+err_exit:
+ mtk_i2c_clk_disable(priv);
+ return ret;
+}
+
+static int mtk_i2c_ofdata_to_platdata(struct udevice *dev)
+{
+ int ret;
+ struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+ priv->base = dev_remap_addr_index(dev, 0);
+ priv->pdmabase = dev_remap_addr_index(dev, 1);
+ ret = clk_get_by_index(dev, 0, &priv->clk_main);
+ if (ret)
+ return ret;
+
+ ret = clk_get_by_index(dev, 1, &priv->clk_dma);
+
+ return ret;
+}
+
+static int mtk_i2c_probe(struct udevice *dev)
+{
+ struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+ priv->soc_data = (struct mtk_i2c_soc_data *)dev_get_driver_data(dev);
+ mtk_i2c_clk_enable(priv);
+ mtk_i2c_init_hw(priv);
+ mtk_i2c_clk_disable(priv);
+ return 0;
+}
+
+static int mtk_i2c_deblock(struct udevice *dev)
+{
+ struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+ mtk_i2c_clk_enable(priv);
+ mtk_i2c_init_hw(priv);
+ mtk_i2c_clk_disable(priv);
+ return 0;
+}
+
+static const struct mtk_i2c_soc_data mt8518_soc_data = {
+ .dma_sync = 0,
+};
+
+static const struct mtk_i2c_soc_data mt8512_soc_data = {
+ .dma_sync = 1,
+};
+
+static const struct dm_i2c_ops mtk_i2c_ops = {
+ .xfer = mtk_i2c_transfer,
+ .set_bus_speed = mtk_i2c_set_speed,
+ .deblock = mtk_i2c_deblock,
+};
+
+static const struct udevice_id mtk_i2c_ids[] = {
+ {
+ .compatible = "mediatek,mt8518-i2c",
+ .data = (ulong)&mt8518_soc_data,
+ },
+ {
+ .compatible = "mediatek,mt8512-i2c",
+ .data = (ulong)&mt8512_soc_data,
+ },
+ {
+ }
+};
+
+U_BOOT_DRIVER(mtk_i2c) = {
+ .name = "mtk_i2c",
+ .id = UCLASS_I2C,
+ .of_match = mtk_i2c_ids,
+ .ofdata_to_platdata = mtk_i2c_ofdata_to_platdata,
+ .probe = mtk_i2c_probe,
+ .priv_auto_alloc_size = sizeof(struct mtk_i2c_priv),
+ .ops = &mtk_i2c_ops,
+};
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512
2020-09-30 8:21 [PATCH v2 0/5] Add i2c support for MediaTek mt8512 mingming lee
2020-09-30 8:21 ` [PATCH v2 1/5] i2c: mediatek: add basic driver support mingming lee
@ 2020-09-30 8:21 ` mingming lee
2020-10-12 3:34 ` Simon Glass
2020-09-30 8:21 ` [PATCH v2 3/5] configs: mt8512: Enable I2C related configs mingming lee
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: mingming lee @ 2020-09-30 8:21 UTC (permalink / raw)
To: u-boot
From: Mingming Lee <Mingming.Lee@mediatek.com>
add i2c dts node support for mt8512
Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
arch/arm/dts/mt8512-bm1-emmc.dts | 12 ++++++++++
arch/arm/dts/mt8512.dtsi | 38 +++++++++++++++++++++++++++++++-
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/arch/arm/dts/mt8512-bm1-emmc.dts b/arch/arm/dts/mt8512-bm1-emmc.dts
index 296ed93b9e..7c02539fca 100644
--- a/arch/arm/dts/mt8512-bm1-emmc.dts
+++ b/arch/arm/dts/mt8512-bm1-emmc.dts
@@ -45,6 +45,18 @@
};
};
+&i2c0 {
+ status = "okay";
+};
+
+&i2c1 {
+ status = "okay";
+};
+
+&i2c2 {
+ status = "okay";
+};
+
&mmc0 {
pinctrl-names = "default";
pinctrl-0 = <&mmc0_pins_default>;
diff --git a/arch/arm/dts/mt8512.dtsi b/arch/arm/dts/mt8512.dtsi
index 01a02a7ebf..39e94993c1 100644
--- a/arch/arm/dts/mt8512.dtsi
+++ b/arch/arm/dts/mt8512.dtsi
@@ -90,6 +90,42 @@
reg = <0x10200a80 0x50>;
};
+ i2c0: i2c at 11007000 {
+ compatible = "mediatek,mt8512-i2c";
+ reg = <0x11007000 0x1000>,
+ <0x11000080 0x80>;
+ clocks = <&infracfg CLK_INFRA_I2C0_AXI>,
+ <&infracfg CLK_INFRA_AP_DMA>;
+ clock-names = "main", "dma";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c at 10019000 {
+ compatible = "mediatek,mt8512-i2c";
+ reg = <0x10019000 0x1000>,
+ <0x11000100 0x80>;
+ clocks = <&infracfg CLK_INFRA_I2C1_AXI>,
+ <&infracfg CLK_INFRA_AP_DMA>;
+ clock-names = "main", "dma";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c2: i2c at 1001e000 {
+ compatible = "mediatek,mt8512-i2c";
+ reg = <0x1001e000 0x1000>,
+ <0x11000180 0x80>;
+ clocks = <&infracfg CLK_INFRA_I2C1_AXI>,
+ <&infracfg CLK_INFRA_AP_DMA>;
+ clock-names = "main", "dma";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
uart0: serial at 11002000 {
compatible = "mediatek,hsuart";
reg = <0x11002000 0x1000>;
@@ -112,4 +148,4 @@
status = "disabled";
};
-};
\ No newline at end of file
+};
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] configs: mt8512: Enable I2C related configs
2020-09-30 8:21 [PATCH v2 0/5] Add i2c support for MediaTek mt8512 mingming lee
2020-09-30 8:21 ` [PATCH v2 1/5] i2c: mediatek: add basic driver support mingming lee
2020-09-30 8:21 ` [PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512 mingming lee
@ 2020-09-30 8:21 ` mingming lee
2020-10-12 3:34 ` Simon Glass
2020-09-30 8:21 ` [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver mingming lee
2020-09-30 8:21 ` [PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK mingming lee
4 siblings, 1 reply; 14+ messages in thread
From: mingming lee @ 2020-09-30 8:21 UTC (permalink / raw)
To: u-boot
From: Mingming Lee <Mingming.Lee@mediatek.com>
Enable MTK I2C
Enable I2C basic command
Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
configs/mt8512_bm1_emmc_defconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/configs/mt8512_bm1_emmc_defconfig b/configs/mt8512_bm1_emmc_defconfig
index 10a2083134..6b3d9a4e9b 100644
--- a/configs/mt8512_bm1_emmc_defconfig
+++ b/configs/mt8512_bm1_emmc_defconfig
@@ -33,3 +33,6 @@ CONFIG_MTK_TIMER=y
CONFIG_WDT=y
CONFIG_WDT_MTK=y
CONFIG_LZO=y
+CONFIG_CMD_I2C=y
+CONFIG_DM_I2C=y
+CONFIG_SYS_I2C_MTK=y
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver
2020-09-30 8:21 [PATCH v2 0/5] Add i2c support for MediaTek mt8512 mingming lee
` (2 preceding siblings ...)
2020-09-30 8:21 ` [PATCH v2 3/5] configs: mt8512: Enable I2C related configs mingming lee
@ 2020-09-30 8:21 ` mingming lee
2020-10-12 3:34 ` Simon Glass
2020-09-30 8:21 ` [PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK mingming lee
4 siblings, 1 reply; 14+ messages in thread
From: mingming lee @ 2020-09-30 8:21 UTC (permalink / raw)
To: u-boot
From: Mingming Lee <Mingming.Lee@mediatek.com>
add bindings for mediatek i2c driver
Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++++++++++++++++++++++++
1 file changed, 39 insertions(+)
create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt
diff --git a/doc/device-tree-bindings/i2c/i2c-mtk.txt b/doc/device-tree-bindings/i2c/i2c-mtk.txt
new file mode 100644
index 0000000000..10a3f29a1e
--- /dev/null
+++ b/doc/device-tree-bindings/i2c/i2c-mtk.txt
@@ -0,0 +1,39 @@
+I2C for Mediatek platforms
+
+Required properties :
+- compatible : Must be "mediatek,mt8512-i2c"
+- reg: physical base address of the controller and length of memory mapped
+ region.
+- #address-cells = <1>;
+- #size-cells = <0>;
+- clocks: phandles to input clocks.
+- clock-names : Contains the names of the clocks:
+ "main", the clock used for normal mode I2C.
+ "dma", the clock used for apdma mode I2C.
+- status : enable in requried dts or not.
+
+Examples :
+
+ i2c0: i2c at 11007000 {
+ compatible = "mediatek,mt8512-i2c";
+ reg = <0x11007000 0x1000>,
+ <0x11000080 0x80>;
+ clocks = <&infracfg CLK_INFRA_I2C0_AXI>,
+ <&infracfg CLK_INFRA_AP_DMA>;
+ clock-names = "main", "dma";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
+ i2c1: i2c at 10019000 {
+ compatible = "mediatek,mt8512-i2c";
+ reg = <0x10019000 0x1000>,
+ <0x11000100 0x80>;
+ clocks = <&infracfg CLK_INFRA_I2C1_AXI>,
+ <&infracfg CLK_INFRA_AP_DMA>;
+ clock-names = "main", "dma";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
\ No newline at end of file
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK
2020-09-30 8:21 [PATCH v2 0/5] Add i2c support for MediaTek mt8512 mingming lee
` (3 preceding siblings ...)
2020-09-30 8:21 ` [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver mingming lee
@ 2020-09-30 8:21 ` mingming lee
2020-10-12 3:34 ` Simon Glass
4 siblings, 1 reply; 14+ messages in thread
From: mingming lee @ 2020-09-30 8:21 UTC (permalink / raw)
To: u-boot
From: Mingming Lee <Mingming.Lee@mediatek.com>
add Mediatek i2c controller driver to ARM MEDIATEK.
Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 889a73f15f..cc78561818 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -254,10 +254,12 @@ S: Maintained
F: arch/arm/mach-mediatek/
F: arch/arm/include/asm/arch-mediatek/
F: board/mediatek/
+F: doc/device-tree-bindings/i2c/i2c-mtk.txt
F: doc/device-tree-bindings/phy/phy-mtk-*
F: doc/device-tree-bindings/usb/mediatek,*
F: doc/README.mediatek
F: drivers/clk/mediatek/
+F: drivers/i2c/mt_i2c.c
F: drivers/mmc/mtk-sd.c
F: drivers/phy/phy-mtk-*
F: drivers/pinctrl/mediatek/
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 1/5] i2c: mediatek: add basic driver support
2020-09-30 8:21 ` [PATCH v2 1/5] i2c: mediatek: add basic driver support mingming lee
@ 2020-10-06 0:02 ` Simon Glass
2020-10-10 2:23 ` Mingming Lee
0 siblings, 1 reply; 14+ messages in thread
From: Simon Glass @ 2020-10-06 0:02 UTC (permalink / raw)
To: u-boot
Hi Mingming,
On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> Add MediaTek I2C basic driver
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
>
> ---
> Changes for v2:
> - using error number defined in include/linux/errno.h
> ---
> drivers/i2c/Kconfig | 9 +
> drivers/i2c/Makefile | 1 +
> drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 627 insertions(+)
> create mode 100644 drivers/i2c/mt_i2c.c
This looks good to me. Some nits below.
>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index dec6dc9dfa..103688ed36 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -159,6 +159,15 @@ config SYS_I2C_MESON
> internal buffer holding up to 8 bytes for transfers and supports
> both 7-bit and 10-bit addresses.
>
> +config SYS_I2C_MTK
> + bool "MediaTek I2C driver"
> + default n
Not needed
> + help
> + This selects the MediaTek Integrated Inter Circuit bus driver.
> + The I2C bus adapter is the base for some other I2C client, eg: touch, sensors.
> + If you want to use MediaTek I2C interface, say Y here.
> + If unsure, say N.
> +
> config SYS_I2C_MXC
> bool "NXP MXC I2C driver"
> help
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index e851ec462e..7227742f8d 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_SYS_I2C_LPC32XX) += lpc32xx_i2c.o
> obj-$(CONFIG_SYS_I2C_MESON) += meson_i2c.o
> obj-$(CONFIG_SYS_I2C_MVTWSI) += mvtwsi.o
> obj-$(CONFIG_SYS_I2C_MXC) += mxc_i2c.o
> +obj-$(CONFIG_SYS_I2C_MTK) += mt_i2c.o
> obj-$(CONFIG_SYS_I2C_NEXELL) += nx_i2c.o
> obj-$(CONFIG_SYS_I2C_OCTEON) += octeon_i2c.o
> obj-$(CONFIG_SYS_I2C_OMAP24XX) += omap24xx_i2c.o
> diff --git a/drivers/i2c/mt_i2c.c b/drivers/i2c/mt_i2c.c
> new file mode 100644
> index 0000000000..be6262d3d2
> --- /dev/null
> +++ b/drivers/i2c/mt_i2c.c
> @@ -0,0 +1,617 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek I2C Interface driver
> + *
> + * Copyright (C) 2020 MediaTek Inc.
> + * Author: Mingming Lee <Mingming.Lee@mediatek.com>
> + */
> +
> +#include <asm/cache.h>
> +#include <asm/io.h>
Check the include-file ordering here:
https://www.denx.de/wiki/U-Boot/CodingStyle
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +
> +#define I2C_OK 0
Drop this and just use 0
> +
> +#define I2C_RS_TRANSFER BIT(4)
> +#define I2C_HS_NACKERR BIT(2)
> +#define I2C_ACKERR BIT(1)
> +#define I2C_TRANSAC_COMP BIT(0)
> +#define I2C_TRANSAC_START BIT(0)
> +#define I2C_RS_MUL_CNFG BIT(15)
> +#define I2C_RS_MUL_TRIG BIT(14)
> +#define I2C_DCM_DISABLE 0x0000
> +#define I2C_IO_CONFIG_OPEN_DRAIN 0x0003
> +#define I2C_IO_CONFIG_PUSH_PULL 0x0000
> +#define I2C_SOFT_RST 0x0001
> +#define I2C_FIFO_ADDR_CLR 0x0001
> +#define I2C_DELAY_LEN 0x0002
> +#define I2C_ST_START_CON 0x8001
> +#define I2C_FS_START_CON 0x1800
> +#define I2C_TIME_CLR_VALUE 0x0000
> +#define I2C_TIME_DEFAULT_VALUE 0x0003
> +#define I2C_WRRD_TRANAC_VALUE 0x0002
> +#define I2C_RD_TRANAC_VALUE 0x0001
> +
> +#define I2C_DMA_CON_TX 0x0000
> +#define I2C_DMA_CON_RX 0x0001
> +#define I2C_DMA_START_EN 0x0001
> +#define I2C_DMA_INT_FLAG_NONE 0x0000
> +#define I2C_DMA_CLR_FLAG 0x0000
> +#define I2C_DMA_HARD_RST 0x0002
> +
> +#define MAX_ST_MODE_SPEED 100000
> +#define MAX_FS_MODE_SPEED 400000
> +#define MAX_HS_MODE_SPEED 3400000
> +#define MAX_SAMPLE_CNT_DIV 8
> +#define MAX_STEP_CNT_DIV 64
> +#define MAX_HS_STEP_CNT_DIV 8
> +#define I2C_DEFAULT_CLK_DIV 4
> +
> +#define MAX_I2C_ADDR 0x7F
> +#define MAX_I2C_LEN 0xFF
Can you use lower-case hex?
> +#define TRANS_ADDR_ONLY BIT(8)
> +#define TRANSFER_TIMEOUT 50000 /* us */
> +#define I2C_FIFO_STAT1_MASK 0x001F
> +#define TIMING_SAMPLE_OFFSET 8
> +#define HS_SAMPLE_OFFSET 12
> +#define HS_STEP_OFFSET 8
> +
> +#define I2C_CONTROL_WRAPPER BIT(0)
> +#define I2C_CONTROL_RS BIT(1)
> +#define I2C_CONTROL_DMA_EN BIT(2)
> +#define I2C_CONTROL_CLK_EXT_EN BIT(3)
> +#define I2C_CONTROL_DIR_CHANGE BIT(4)
> +#define I2C_CONTROL_ACKERR_DET_EN BIT(5)
> +#define I2C_CONTROL_TRANSFER_LEN_CHANGE BIT(6)
> +#define I2C_CONTROL_DMAACK BIT(8)
> +#define I2C_CONTROL_ASYNC BIT(9)
> +
> +enum I2C_REGS_OFFSET {
> + OFFSET_PORT = 0x0,
> + OFFSET_SLAVE_ADDR = 0x04,
> + OFFSET_INTR_MASK = 0x08,
> + OFFSET_INTR_STAT = 0x0C,
> + OFFSET_CONTROL = 0x10,
> + OFFSET_TRANSFER_LEN = 0x14,
> + OFFSET_TRANSAC_LEN = 0x18,
> + OFFSET_DELAY_LEN = 0x1C,
> + OFFSET_TIMING = 0x20,
Perhaps REG instead of OFFSET since it is shorter?
> + OFFSET_START = 0x24,
> + OFFSET_EXT_CONF = 0x28,
> + OFFSET_FIFO_STAT1 = 0x2C,
> + OFFSET_FIFO_STAT = 0x30,
> + OFFSET_FIFO_THRESH = 0x34,
> + OFFSET_FIFO_ADDR_CLR = 0x38,
> + OFFSET_IO_CONFIG = 0x40,
> + OFFSET_RSV_DEBUG = 0x44,
> + OFFSET_HS = 0x48,
> + OFFSET_SOFTRESET = 0x50,
> + OFFSET_DCM_EN = 0x54,
> + OFFSET_DEBUGSTAT = 0x64,
> + OFFSET_DEBUGCTRL = 0x68,
> + OFFSET_TRANSFER_LEN_AUX = 0x6C,
> + OFFSET_CLOCK_DIV = 0x70,
> + OFFSET_SCL_HL_RATIO = 0x74,
> + OFFSET_SCL_HS_HL_RATIO = 0x78,
> + OFFSET_SCL_MIS_COMP_POINT = 0x7C,
> + OFFSET_STA_STOP_AC_TIME = 0x80,
> + OFFSET_HS_STA_STOP_AC_TIME = 0x84,
> + OFFSET_DATA_TIME = 0x88,
> +};
> +
> +enum DMA_REGS_OFFSET {
> + OFFSET_INT_FLAG = 0x0,
> + OFFSET_INT_EN = 0x04,
> + OFFSET_EN = 0x08,
> + OFFSET_RST = 0x0C,
> + OFFSET_CON = 0x18,
> + OFFSET_TX_MEM_ADDR = 0x1C,
> + OFFSET_RX_MEM_ADDR = 0x20,
> + OFFSET_TX_LEN = 0x24,
> + OFFSET_RX_LEN = 0x28,
> +};
> +
> +enum mtk_trans_op {
> + I2C_MASTER_WR = 1,
> + I2C_MASTER_RD,
> + I2C_MASTER_WRRD,
> +};
> +
> +struct mtk_i2c_soc_data {
> + u8 dma_sync: 1;
> +};
> +
> +struct mtk_i2c_priv {
> + /* set in i2c probe */
> + void __iomem *base; /* i2c base addr */
> + void __iomem *pdmabase; /* dma base address*/
> + struct clk clk_main; /* main clock for i2c bus */
> + struct clk clk_dma; /* DMA clock for i2c via DMA */
> + const struct mtk_i2c_soc_data *soc_data;
> + enum mtk_trans_op op;
> + bool zero_len;
> + bool pushpull; /* open drain */
> + bool filter_msg; /* filter msg error log */
> + bool auto_restart;
> + bool ignore_restart_irq;
> + u32 speed; /* hz */
> + u8 *tx_buff;
> + u8 *rx_buff;
Please add comments where they are currently missing.
> +};
> +
> +static inline void i2c_writel(struct mtk_i2c_priv *priv, u8 offset, u32 value)
uint for args. There is no point in using u8, etc. and it just
potentially increases code size. Please fix globally
> +{
> + writel(value, (priv->base + offset));
> +}
> +
> +static inline u32 i2c_readl(struct mtk_i2c_priv *priv, u8 offset)
> +{
> + return readl(priv->base + offset);
> +}
> +
> +static void mtk_i2c_clk_enable(struct mtk_i2c_priv *priv)
> +{
> + clk_enable(&priv->clk_main);
Error check.
> + clk_enable(&priv->clk_dma);
> +}
> +
> +static void mtk_i2c_clk_disable(struct mtk_i2c_priv *priv)
> +{
> + clk_disable(&priv->clk_dma);
> + clk_disable(&priv->clk_main);
Error checks.
These two functions should return error codes.
> +}
> +
> +static void mtk_i2c_init_hw(struct mtk_i2c_priv *priv)
> +{
> + u16 control_reg;
> +
> + writel(I2C_DMA_HARD_RST, priv->pdmabase + OFFSET_RST);
> + writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_RST);
> + i2c_writel(priv, OFFSET_SOFTRESET, I2C_SOFT_RST);
> + /* set ioconfig */
> + if (priv->pushpull)
> + i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_PUSH_PULL);
> + else
> + i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_OPEN_DRAIN);
> +
> + i2c_writel(priv, OFFSET_DCM_EN, I2C_DCM_DISABLE);
> + control_reg = I2C_CONTROL_ACKERR_DET_EN | I2C_CONTROL_CLK_EXT_EN;
> + if (priv->soc_data->dma_sync)
> + control_reg |= I2C_CONTROL_DMAACK | I2C_CONTROL_ASYNC;
> + i2c_writel(priv, OFFSET_CONTROL, control_reg);
> + i2c_writel(priv, OFFSET_DELAY_LEN, I2C_DELAY_LEN);
> +}
> +
> +static int mtk_i2c_calculate_speed(struct mtk_i2c_priv *priv,
> + u32 clk_src,
> + u32 target_speed,
> + u32 *timing_step_cnt,
> + u32 *timing_sample_cnt)
function comment
> +{
> + u32 step_cnt;
> + u32 sample_cnt;
> + u32 max_step_cnt;
> + u32 base_sample_cnt = MAX_SAMPLE_CNT_DIV;
> + u32 base_step_cnt;
> + u32 opt_div;
> + u32 best_mul;
> + u32 cnt_mul;
uint?
I don't see why these need to be u32. Please fix globally
> +
> + if (target_speed > MAX_HS_MODE_SPEED)
> + target_speed = MAX_HS_MODE_SPEED;
> +
> + if (target_speed > MAX_FS_MODE_SPEED)
> + max_step_cnt = MAX_HS_STEP_CNT_DIV;
> + else
> + max_step_cnt = MAX_STEP_CNT_DIV;
> +
> + base_step_cnt = max_step_cnt;
> + /* Find the best combination */
> + opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
> + best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
> +
> + /* Search for the best pair (sample_cnt, step_cnt) with
> + * 0 < sample_cnt < MAX_SAMPLE_CNT_DIV
> + * 0 < step_cnt < max_step_cnt
> + * sample_cnt * step_cnt >= opt_div
> + * optimizing for sample_cnt * step_cnt being minimal
> + */
> + for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
> + step_cnt = DIV_ROUND_UP(opt_div, sample_cnt);
> + cnt_mul = step_cnt * sample_cnt;
> + if (step_cnt > max_step_cnt)
> + continue;
> +
> + if (cnt_mul < best_mul) {
> + best_mul = cnt_mul;
> + base_sample_cnt = sample_cnt;
> + base_step_cnt = step_cnt;
> + if (best_mul == opt_div)
> + break;
> + }
> + }
> +
> + sample_cnt = base_sample_cnt;
> + step_cnt = base_step_cnt;
> +
> + if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
> + /* In this case, hardware can't support such
> + * low i2c_bus_freq
> + */
> + debug("Unsupported speed(%uhz)\n", target_speed);
> + return -EINVAL;
> + }
> +
> + *timing_step_cnt = step_cnt - 1;
> + *timing_sample_cnt = sample_cnt - 1;
blank line before final return (please fix globally)
> + return 0;
> +}
> +
> +static int mtk_i2c_set_speed(struct udevice *dev, uint speed)
> +{
> + u32 clk_src;
> + u32 step_cnt;
> + u32 sample_cnt;
> + u16 timing_reg;
> + u16 high_speed_reg;
> + int ret = 0;
> + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> + priv->speed = speed;
> + mtk_i2c_clk_enable(priv);
> + clk_src = clk_get_rate(&priv->clk_main) /
> + I2C_DEFAULT_CLK_DIV;
> + i2c_writel(priv, OFFSET_CLOCK_DIV, (I2C_DEFAULT_CLK_DIV - 1));
> + if (priv->speed > MAX_FS_MODE_SPEED) {
> + /* Set master code speed register */
> + ret = mtk_i2c_calculate_speed(priv, clk_src, MAX_FS_MODE_SPEED,
> + &step_cnt, &sample_cnt);
> + if (ret < 0)
> + goto exit;
> +
> + timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> + writel(timing_reg, priv->base + OFFSET_TIMING);
> + /* Set the high speed mode register */
> + ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> + &step_cnt, &sample_cnt);
> + if (ret < 0)
> + goto exit;
> +
> + high_speed_reg = I2C_TIME_DEFAULT_VALUE |
> + (sample_cnt << HS_SAMPLE_OFFSET) |
> + (step_cnt << HS_STEP_OFFSET);
> + writel(high_speed_reg, priv->base + OFFSET_HS);
> + } else {
> + ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> + &step_cnt, &sample_cnt);
> + if (ret < 0)
> + goto exit;
> +
> + timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> + /* Disable the high speed transaction */
> + high_speed_reg = I2C_TIME_CLR_VALUE;
> + writel(timing_reg, priv->base + OFFSET_TIMING);
> + writel(high_speed_reg, priv->base + OFFSET_HS);
> + }
> +exit:
> + mtk_i2c_clk_disable(priv);
> + return ret;
> +}
> +
> +static int mtk_i2c_do_transfer(struct mtk_i2c_priv *priv,
> + struct i2c_msg *msgs,
> + int num, int left_num)
comment
> +{
> + u16 addr_reg;
> + u16 start_reg;
> + u16 control_reg;
> + u16 restart_flag = 0;
> + u16 irq_stat = 0;
> + u8 trans_error = 0;
> + bool tmo = false;
> + u8 *ptr = msgs->buf;
> + u16 data_size = msgs->len;
> + u32 tmo_poll = 0;
> + int ret;
> +
> + if (priv->auto_restart)
> + restart_flag = I2C_RS_TRANSFER;
> +
> + control_reg = i2c_readl(priv, OFFSET_CONTROL) &
> + ~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
> +
> + if (priv->speed > MAX_FS_MODE_SPEED || num > 1)
> + control_reg |= I2C_CONTROL_RS;
> +
> + if (priv->op == I2C_MASTER_WRRD)
> + control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> + control_reg |= I2C_CONTROL_DMA_EN;
> + i2c_writel(priv, OFFSET_CONTROL, control_reg);
> +
> + /* set start condition */
> + if (priv->speed <= MAX_ST_MODE_SPEED)
> + i2c_writel(priv, OFFSET_EXT_CONF, I2C_ST_START_CON);
> + else
> + i2c_writel(priv, OFFSET_EXT_CONF, I2C_FS_START_CON);
> +
> + addr_reg = msgs->addr << 1;
> + if (priv->op == I2C_MASTER_RD)
> + addr_reg |= I2C_M_RD;
> + if (priv->zero_len)
> + i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg | TRANS_ADDR_ONLY);
> + else
> + i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg);
> +
> + /* clear interrupt status */
> + i2c_writel(priv, OFFSET_INTR_STAT, restart_flag | I2C_HS_NACKERR |
> + I2C_ACKERR | I2C_TRANSAC_COMP);
> + i2c_writel(priv, OFFSET_FIFO_ADDR_CLR, I2C_FIFO_ADDR_CLR);
> +
> + /* enable interrupt */
> + i2c_writel(priv, OFFSET_INTR_MASK, restart_flag | I2C_HS_NACKERR |
> + I2C_ACKERR | I2C_TRANSAC_COMP);
> +
> + /* set transfer and transaction len */
> + if (priv->op == I2C_MASTER_WRRD) {
> + i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> + i2c_writel(priv, OFFSET_TRANSFER_LEN_AUX,
> + (msgs + 1)->len);
> + i2c_writel(priv, OFFSET_TRANSAC_LEN, I2C_WRRD_TRANAC_VALUE);
> + } else {
> + i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> + i2c_writel(priv, OFFSET_TRANSAC_LEN, num);
> + }
> +
> + /* prepare buffer data to start transfer */
> + if (priv->op == I2C_MASTER_RD) {
> + writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> + OFFSET_INT_FLAG);
> + writel(I2C_DMA_CON_RX, priv->pdmabase + OFFSET_CON);
> + priv->rx_buff = (u8 *)noncached_alloc(msgs->len,
> + ARCH_DMA_MINALIGN);
> + writel((u32)priv->rx_buff, priv->pdmabase +
> + OFFSET_RX_MEM_ADDR);
> + writel(msgs->len, priv->pdmabase + OFFSET_RX_LEN);
> + } else if (priv->op == I2C_MASTER_WR) {
> + writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> + OFFSET_INT_FLAG);
> + writel(I2C_DMA_CON_TX, priv->pdmabase + OFFSET_CON);
> + priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> + ARCH_DMA_MINALIGN);
> + memcpy(priv->tx_buff, msgs->buf, msgs->len);
> + writel((u32)priv->tx_buff, priv->pdmabase +
> + OFFSET_TX_MEM_ADDR);
> + writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> + } else {
> + writel(I2C_DMA_CLR_FLAG, priv->pdmabase +
> + OFFSET_INT_FLAG);
> + writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_CON);
> + priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> + ARCH_DMA_MINALIGN);
Do you need the cast?
> + priv->rx_buff = (u8 *)noncached_alloc((msgs + 1)->len,
> + ARCH_DMA_MINALIGN);
> + memcpy(priv->tx_buff, msgs->buf, msgs->len);
> + writel((u32)priv->tx_buff, priv->pdmabase +
> + OFFSET_TX_MEM_ADDR);
> + writel((u32)priv->rx_buff, priv->pdmabase +
> + OFFSET_RX_MEM_ADDR);
> + writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> + writel((msgs + 1)->len, priv->pdmabase +
> + OFFSET_RX_LEN);
> + }
> + writel(I2C_DMA_START_EN, priv->pdmabase + OFFSET_EN);
> +
> + if (!priv->auto_restart) {
> + start_reg = I2C_TRANSAC_START;
> + } else {
> + start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
> + if (left_num >= 1)
> + start_reg |= I2C_RS_MUL_CNFG;
> + }
> + i2c_writel(priv, OFFSET_START, start_reg);
> +
> + for (;;) {
> + irq_stat = i2c_readl(priv, OFFSET_INTR_STAT);
> +
> + /* ignore the first restart irq after the master code */
> + if (priv->ignore_restart_irq && (irq_stat | restart_flag)) {
> + priv->ignore_restart_irq = false;
> + irq_stat = 0;
> + writel(I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
> + I2C_TRANSAC_START, priv->base + OFFSET_START);
> + }
> +
> + if (irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
> + tmo = false;
> + if (irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))
> + trans_error = 1;
> +
> + break;
> + }
> + udelay(1);
> + if (tmo_poll++ >= TRANSFER_TIMEOUT) {
> + tmo = true;
> + break;
> + }
> + }
> + /* clear interrupt mask */
> + i2c_writel(priv, OFFSET_INTR_MASK, ~(restart_flag | I2C_HS_NACKERR |
> + I2C_ACKERR | I2C_TRANSAC_COMP));
> +
> + if (!tmo && trans_error == 0) {
> + /* transfer success ,we need to get data from fifo */
> + if (priv->op == I2C_MASTER_RD || priv->op == I2C_MASTER_WRRD) {
> + ptr = priv->op == I2C_MASTER_RD ?
> + msgs->buf : (msgs + 1)->buf;
> + data_size = priv->op == I2C_MASTER_RD ?
> + msgs->len : (msgs + 1)->len;
> + memcpy(ptr, priv->rx_buff, data_size);
> + }
> + } else {
> + /* timeout or ACKERR */
> + if (tmo) {
> + ret = -ETIMEDOUT;
> + if (!priv->filter_msg)
> + debug("transfer timeout! addr: 0x%x,\n",
> + msgs->addr);
> + } else {
> + ret = -ENXIO;
Should that be REMOTEIO? Up to you.
> + if (!priv->filter_msg)
> + debug("ACKERR! addr: 0x%x,IRQ:0x%x\n",
> + msgs->addr, irq_stat);
> + }
> + mtk_i2c_init_hw(priv);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int mtk_i2c_transfer(struct udevice *dev, struct i2c_msg *msg,
> + int nmsgs)
> +{
> + int ret;
> + int left_num;
> + u8 num_cnt;
uint (please fix globally)
> + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> + priv->auto_restart = true;
> + left_num = nmsgs;
> + mtk_i2c_clk_enable(priv);
error check
> + for (num_cnt = 0; num_cnt < nmsgs; num_cnt++) {
> + if (((msg + num_cnt)->addr) > MAX_I2C_ADDR) {
> + ret = -EINVAL;
> + goto err_exit;
> + }
> + if ((msg + num_cnt)->len > MAX_I2C_LEN) {
> + ret = -EINVAL;
> + goto err_exit;
> + }
> + }
> +
> + /* checking if we can skip restart and optimize using WRRD mode */
check
> + if (priv->auto_restart && nmsgs == 2) {
> + if (!(msg[0].flags & I2C_M_RD) && (msg[1].flags & I2C_M_RD) &&
> + msg[0].addr == msg[1].addr) {
> + priv->auto_restart = false;
> + }
> + }
> +
> + if (priv->auto_restart && nmsgs >= 2 && priv->speed > MAX_FS_MODE_SPEED)
> + /* ignore the first restart irq after the master code,
> + * otherwise the first transfer will be discarded.
> + */
> + priv->ignore_restart_irq = true;
> + else
> + priv->ignore_restart_irq = false;
> +
> + while (left_num--) {
> + /*priv->zero_len = true
What is that comment for?
> + *transfer slave address only to support devices detect
> + */
> + if (!msg->buf)
> + priv->zero_len = true;
> + else
> + priv->zero_len = false;
> +
> + if (msg->flags & I2C_M_RD)
> + priv->op = I2C_MASTER_RD;
> + else
> + priv->op = I2C_MASTER_WR;
> +
> + if (!priv->auto_restart) {
> + if (nmsgs > 1) {
> + /* combined two messages into one transaction */
> + priv->op = I2C_MASTER_WRRD;
> + left_num--;
> + }
> + }
> + ret = mtk_i2c_do_transfer(priv, msg, nmsgs, left_num);
> + if (ret < 0)
> + goto err_exit;
> + msg++;
> + }
> + ret = I2C_OK;
ret = 0
> +
> +err_exit:
> + mtk_i2c_clk_disable(priv);
error check
> + return ret;
> +}
> +
> +static int mtk_i2c_ofdata_to_platdata(struct udevice *dev)
> +{
> + int ret;
> + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> + priv->base = dev_remap_addr_index(dev, 0);
> + priv->pdmabase = dev_remap_addr_index(dev, 1);
> + ret = clk_get_by_index(dev, 0, &priv->clk_main);
> + if (ret)
> + return ret;
consider log_ret()ret or log_msg_ret(ret)
> +
> + ret = clk_get_by_index(dev, 1, &priv->clk_dma);
> +
> + return ret;
> +}
> +
> +static int mtk_i2c_probe(struct udevice *dev)
> +{
> + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> + priv->soc_data = (struct mtk_i2c_soc_data *)dev_get_driver_data(dev);
> + mtk_i2c_clk_enable(priv);
> + mtk_i2c_init_hw(priv);
> + mtk_i2c_clk_disable(priv);
> + return 0;
> +}
> +
> +static int mtk_i2c_deblock(struct udevice *dev)
> +{
> + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> + mtk_i2c_clk_enable(priv);
> + mtk_i2c_init_hw(priv);
> + mtk_i2c_clk_disable(priv);
> + return 0;
> +}
> +
> +static const struct mtk_i2c_soc_data mt8518_soc_data = {
> + .dma_sync = 0,
> +};
> +
> +static const struct mtk_i2c_soc_data mt8512_soc_data = {
> + .dma_sync = 1,
> +};
> +
> +static const struct dm_i2c_ops mtk_i2c_ops = {
> + .xfer = mtk_i2c_transfer,
> + .set_bus_speed = mtk_i2c_set_speed,
> + .deblock = mtk_i2c_deblock,
> +};
> +
> +static const struct udevice_id mtk_i2c_ids[] = {
> + {
> + .compatible = "mediatek,mt8518-i2c",
> + .data = (ulong)&mt8518_soc_data,
> + },
> + {
Put the above { on the line above
> + .compatible = "mediatek,mt8512-i2c",
> + .data = (ulong)&mt8512_soc_data,
> + },
> + {
> + }
same here
> +};
> +
> +U_BOOT_DRIVER(mtk_i2c) = {
> + .name = "mtk_i2c",
> + .id = UCLASS_I2C,
> + .of_match = mtk_i2c_ids,
> + .ofdata_to_platdata = mtk_i2c_ofdata_to_platdata,
> + .probe = mtk_i2c_probe,
> + .priv_auto_alloc_size = sizeof(struct mtk_i2c_priv),
> + .ops = &mtk_i2c_ops,
> +};
> --
> 2.18.0
Regards,
SImon
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/5] i2c: mediatek: add basic driver support
2020-10-06 0:02 ` Simon Glass
@ 2020-10-10 2:23 ` Mingming Lee
0 siblings, 0 replies; 14+ messages in thread
From: Mingming Lee @ 2020-10-10 2:23 UTC (permalink / raw)
To: u-boot
hi Simon,
Thank you for your carefully reveiw, and I think I would modify all of
them in next version.
On Mon, 2020-10-05 at 18:02 -0600, Simon Glass wrote:
> Hi Mingming,
>
> On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
> >
> > From: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > Add MediaTek I2C basic driver
> >
> > Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > ---
> > Changes for v2:
> > - using error number defined in include/linux/errno.h
> > ---
> > drivers/i2c/Kconfig | 9 +
> > drivers/i2c/Makefile | 1 +
> > drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 627 insertions(+)
> > create mode 100644 drivers/i2c/mt_i2c.c
>
> This looks good to me. Some nits below.
>
> >
> > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > index dec6dc9dfa..103688ed36 100644
> > --- a/drivers/i2c/Kconfig
> > +++ b/drivers/i2c/Kconfig
> > @@ -159,6 +159,15 @@ config SYS_I2C_MESON
> > internal buffer holding up to 8 bytes for transfers and supports
> > both 7-bit and 10-bit addresses.
> >
> > +config SYS_I2C_MTK
> > + bool "MediaTek I2C driver"
> > + default n
>
> Not needed
I would remove it.
>
> > + help
> > + This selects the MediaTek Integrated Inter Circuit bus driver.
> > + The I2C bus adapter is the base for some other I2C client, eg: touch, sensors.
> > + If you want to use MediaTek I2C interface, say Y here.
> > + If unsure, say N.
> > +
> > config SYS_I2C_MXC
> > bool "NXP MXC I2C driver"
> > help
> > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> > index e851ec462e..7227742f8d 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_SYS_I2C_LPC32XX) += lpc32xx_i2c.o
> > obj-$(CONFIG_SYS_I2C_MESON) += meson_i2c.o
> > obj-$(CONFIG_SYS_I2C_MVTWSI) += mvtwsi.o
> > obj-$(CONFIG_SYS_I2C_MXC) += mxc_i2c.o
> > +obj-$(CONFIG_SYS_I2C_MTK) += mt_i2c.o
> > obj-$(CONFIG_SYS_I2C_NEXELL) += nx_i2c.o
> > obj-$(CONFIG_SYS_I2C_OCTEON) += octeon_i2c.o
> > obj-$(CONFIG_SYS_I2C_OMAP24XX) += omap24xx_i2c.o
> > diff --git a/drivers/i2c/mt_i2c.c b/drivers/i2c/mt_i2c.c
> > new file mode 100644
> > index 0000000000..be6262d3d2
> > --- /dev/null
> > +++ b/drivers/i2c/mt_i2c.c
> > @@ -0,0 +1,617 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MediaTek I2C Interface driver
> > + *
> > + * Copyright (C) 2020 MediaTek Inc.
> > + * Author: Mingming Lee <Mingming.Lee@mediatek.com>
> > + */
> > +
> > +#include <asm/cache.h>
> > +#include <asm/io.h>
>
> Check the include-file ordering here:
>
> https://www.denx.de/wiki/U-Boot/CodingStyle
>
I would modify the order.
> > +#include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +
> > +#define I2C_OK 0
>
> Drop this and just use 0
>
I would drop this.
> > +
> > +#define I2C_RS_TRANSFER BIT(4)
> > +#define I2C_HS_NACKERR BIT(2)
> > +#define I2C_ACKERR BIT(1)
> > +#define I2C_TRANSAC_COMP BIT(0)
> > +#define I2C_TRANSAC_START BIT(0)
> > +#define I2C_RS_MUL_CNFG BIT(15)
> > +#define I2C_RS_MUL_TRIG BIT(14)
> > +#define I2C_DCM_DISABLE 0x0000
> > +#define I2C_IO_CONFIG_OPEN_DRAIN 0x0003
> > +#define I2C_IO_CONFIG_PUSH_PULL 0x0000
> > +#define I2C_SOFT_RST 0x0001
> > +#define I2C_FIFO_ADDR_CLR 0x0001
> > +#define I2C_DELAY_LEN 0x0002
> > +#define I2C_ST_START_CON 0x8001
> > +#define I2C_FS_START_CON 0x1800
> > +#define I2C_TIME_CLR_VALUE 0x0000
> > +#define I2C_TIME_DEFAULT_VALUE 0x0003
> > +#define I2C_WRRD_TRANAC_VALUE 0x0002
> > +#define I2C_RD_TRANAC_VALUE 0x0001
> > +
> > +#define I2C_DMA_CON_TX 0x0000
> > +#define I2C_DMA_CON_RX 0x0001
> > +#define I2C_DMA_START_EN 0x0001
> > +#define I2C_DMA_INT_FLAG_NONE 0x0000
> > +#define I2C_DMA_CLR_FLAG 0x0000
> > +#define I2C_DMA_HARD_RST 0x0002
> > +
> > +#define MAX_ST_MODE_SPEED 100000
> > +#define MAX_FS_MODE_SPEED 400000
> > +#define MAX_HS_MODE_SPEED 3400000
> > +#define MAX_SAMPLE_CNT_DIV 8
> > +#define MAX_STEP_CNT_DIV 64
> > +#define MAX_HS_STEP_CNT_DIV 8
> > +#define I2C_DEFAULT_CLK_DIV 4
> > +
> > +#define MAX_I2C_ADDR 0x7F
> > +#define MAX_I2C_LEN 0xFF
>
> Can you use lower-case hex?
>
I would modify it.
> > +#define TRANS_ADDR_ONLY BIT(8)
> > +#define TRANSFER_TIMEOUT 50000 /* us */
> > +#define I2C_FIFO_STAT1_MASK 0x001F
> > +#define TIMING_SAMPLE_OFFSET 8
> > +#define HS_SAMPLE_OFFSET 12
> > +#define HS_STEP_OFFSET 8
> > +
> > +#define I2C_CONTROL_WRAPPER BIT(0)
> > +#define I2C_CONTROL_RS BIT(1)
> > +#define I2C_CONTROL_DMA_EN BIT(2)
> > +#define I2C_CONTROL_CLK_EXT_EN BIT(3)
> > +#define I2C_CONTROL_DIR_CHANGE BIT(4)
> > +#define I2C_CONTROL_ACKERR_DET_EN BIT(5)
> > +#define I2C_CONTROL_TRANSFER_LEN_CHANGE BIT(6)
> > +#define I2C_CONTROL_DMAACK BIT(8)
> > +#define I2C_CONTROL_ASYNC BIT(9)
> > +
> > +enum I2C_REGS_OFFSET {
> > + OFFSET_PORT = 0x0,
> > + OFFSET_SLAVE_ADDR = 0x04,
> > + OFFSET_INTR_MASK = 0x08,
> > + OFFSET_INTR_STAT = 0x0C,
> > + OFFSET_CONTROL = 0x10,
> > + OFFSET_TRANSFER_LEN = 0x14,
> > + OFFSET_TRANSAC_LEN = 0x18,
> > + OFFSET_DELAY_LEN = 0x1C,
> > + OFFSET_TIMING = 0x20,
>
> Perhaps REG instead of OFFSET since it is shorter?
>
> > + OFFSET_START = 0x24,
> > + OFFSET_EXT_CONF = 0x28,
> > + OFFSET_FIFO_STAT1 = 0x2C,
> > + OFFSET_FIFO_STAT = 0x30,
> > + OFFSET_FIFO_THRESH = 0x34,
> > + OFFSET_FIFO_ADDR_CLR = 0x38,
> > + OFFSET_IO_CONFIG = 0x40,
> > + OFFSET_RSV_DEBUG = 0x44,
> > + OFFSET_HS = 0x48,
> > + OFFSET_SOFTRESET = 0x50,
> > + OFFSET_DCM_EN = 0x54,
> > + OFFSET_DEBUGSTAT = 0x64,
> > + OFFSET_DEBUGCTRL = 0x68,
> > + OFFSET_TRANSFER_LEN_AUX = 0x6C,
> > + OFFSET_CLOCK_DIV = 0x70,
> > + OFFSET_SCL_HL_RATIO = 0x74,
> > + OFFSET_SCL_HS_HL_RATIO = 0x78,
> > + OFFSET_SCL_MIS_COMP_POINT = 0x7C,
> > + OFFSET_STA_STOP_AC_TIME = 0x80,
> > + OFFSET_HS_STA_STOP_AC_TIME = 0x84,
> > + OFFSET_DATA_TIME = 0x88,
> > +};
> > +
> > +enum DMA_REGS_OFFSET {
> > + OFFSET_INT_FLAG = 0x0,
> > + OFFSET_INT_EN = 0x04,
> > + OFFSET_EN = 0x08,
> > + OFFSET_RST = 0x0C,
> > + OFFSET_CON = 0x18,
> > + OFFSET_TX_MEM_ADDR = 0x1C,
> > + OFFSET_RX_MEM_ADDR = 0x20,
> > + OFFSET_TX_LEN = 0x24,
> > + OFFSET_RX_LEN = 0x28,
> > +};
> > +
> > +enum mtk_trans_op {
> > + I2C_MASTER_WR = 1,
> > + I2C_MASTER_RD,
> > + I2C_MASTER_WRRD,
> > +};
> > +
> > +struct mtk_i2c_soc_data {
> > + u8 dma_sync: 1;
> > +};
> > +
> > +struct mtk_i2c_priv {
> > + /* set in i2c probe */
> > + void __iomem *base; /* i2c base addr */
> > + void __iomem *pdmabase; /* dma base address*/
> > + struct clk clk_main; /* main clock for i2c bus */
> > + struct clk clk_dma; /* DMA clock for i2c via DMA */
> > + const struct mtk_i2c_soc_data *soc_data;
> > + enum mtk_trans_op op;
> > + bool zero_len;
> > + bool pushpull; /* open drain */
> > + bool filter_msg; /* filter msg error log */
> > + bool auto_restart;
> > + bool ignore_restart_irq;
> > + u32 speed; /* hz */
> > + u8 *tx_buff;
> > + u8 *rx_buff;
>
> Please add comments where they are currently missing.
>
> > +};
> > +
> > +static inline void i2c_writel(struct mtk_i2c_priv *priv, u8 offset, u32 value)
>
> uint for args. There is no point in using u8, etc. and it just
> potentially increases code size. Please fix globally
>
It is better to use uint and I would modify them to uint.
> > +{
> > + writel(value, (priv->base + offset));
> > +}
> > +
> > +static inline u32 i2c_readl(struct mtk_i2c_priv *priv, u8 offset)
> > +{
> > + return readl(priv->base + offset);
> > +}
> > +
> > +static void mtk_i2c_clk_enable(struct mtk_i2c_priv *priv)
> > +{
> > + clk_enable(&priv->clk_main);
>
> Error check.
I would add error check.
>
> > + clk_enable(&priv->clk_dma);
> > +}
> > +
> > +static void mtk_i2c_clk_disable(struct mtk_i2c_priv *priv)
> > +{
> > + clk_disable(&priv->clk_dma);
> > + clk_disable(&priv->clk_main);
>
> Error checks.
>
> These two functions should return error codes.
>
> > +}
> > +
> > +static void mtk_i2c_init_hw(struct mtk_i2c_priv *priv)
> > +{
> > + u16 control_reg;
> > +
> > + writel(I2C_DMA_HARD_RST, priv->pdmabase + OFFSET_RST);
> > + writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_RST);
> > + i2c_writel(priv, OFFSET_SOFTRESET, I2C_SOFT_RST);
> > + /* set ioconfig */
> > + if (priv->pushpull)
> > + i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_PUSH_PULL);
> > + else
> > + i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_OPEN_DRAIN);
> > +
> > + i2c_writel(priv, OFFSET_DCM_EN, I2C_DCM_DISABLE);
> > + control_reg = I2C_CONTROL_ACKERR_DET_EN | I2C_CONTROL_CLK_EXT_EN;
> > + if (priv->soc_data->dma_sync)
> > + control_reg |= I2C_CONTROL_DMAACK | I2C_CONTROL_ASYNC;
> > + i2c_writel(priv, OFFSET_CONTROL, control_reg);
> > + i2c_writel(priv, OFFSET_DELAY_LEN, I2C_DELAY_LEN);
> > +}
> > +
> > +static int mtk_i2c_calculate_speed(struct mtk_i2c_priv *priv,
> > + u32 clk_src,
> > + u32 target_speed,
> > + u32 *timing_step_cnt,
> > + u32 *timing_sample_cnt)
>
> function comment
>
I would add function comment.
> > +{
> > + u32 step_cnt;
> > + u32 sample_cnt;
> > + u32 max_step_cnt;
> > + u32 base_sample_cnt = MAX_SAMPLE_CNT_DIV;
> > + u32 base_step_cnt;
> > + u32 opt_div;
> > + u32 best_mul;
> > + u32 cnt_mul;
>
> uint?
>
> I don't see why these need to be u32. Please fix globally
>
I would modify it to uint.
> > +
> > + if (target_speed > MAX_HS_MODE_SPEED)
> > + target_speed = MAX_HS_MODE_SPEED;
> > +
> > + if (target_speed > MAX_FS_MODE_SPEED)
> > + max_step_cnt = MAX_HS_STEP_CNT_DIV;
> > + else
> > + max_step_cnt = MAX_STEP_CNT_DIV;
> > +
> > + base_step_cnt = max_step_cnt;
> > + /* Find the best combination */
> > + opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
> > + best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
> > +
> > + /* Search for the best pair (sample_cnt, step_cnt) with
> > + * 0 < sample_cnt < MAX_SAMPLE_CNT_DIV
> > + * 0 < step_cnt < max_step_cnt
> > + * sample_cnt * step_cnt >= opt_div
> > + * optimizing for sample_cnt * step_cnt being minimal
> > + */
> > + for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
> > + step_cnt = DIV_ROUND_UP(opt_div, sample_cnt);
> > + cnt_mul = step_cnt * sample_cnt;
> > + if (step_cnt > max_step_cnt)
> > + continue;
> > +
> > + if (cnt_mul < best_mul) {
> > + best_mul = cnt_mul;
> > + base_sample_cnt = sample_cnt;
> > + base_step_cnt = step_cnt;
> > + if (best_mul == opt_div)
> > + break;
> > + }
> > + }
> > +
> > + sample_cnt = base_sample_cnt;
> > + step_cnt = base_step_cnt;
> > +
> > + if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
> > + /* In this case, hardware can't support such
> > + * low i2c_bus_freq
> > + */
> > + debug("Unsupported speed(%uhz)\n", target_speed);
> > + return -EINVAL;
> > + }
> > +
> > + *timing_step_cnt = step_cnt - 1;
> > + *timing_sample_cnt = sample_cnt - 1;
>
> blank line before final return (please fix globally)
>
I would modify it.
> > + return 0;
> > +}
> > +
> > +static int mtk_i2c_set_speed(struct udevice *dev, uint speed)
> > +{
> > + u32 clk_src;
> > + u32 step_cnt;
> > + u32 sample_cnt;
> > + u16 timing_reg;
> > + u16 high_speed_reg;
> > + int ret = 0;
> > + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > + priv->speed = speed;
> > + mtk_i2c_clk_enable(priv);
> > + clk_src = clk_get_rate(&priv->clk_main) /
> > + I2C_DEFAULT_CLK_DIV;
> > + i2c_writel(priv, OFFSET_CLOCK_DIV, (I2C_DEFAULT_CLK_DIV - 1));
> > + if (priv->speed > MAX_FS_MODE_SPEED) {
> > + /* Set master code speed register */
> > + ret = mtk_i2c_calculate_speed(priv, clk_src, MAX_FS_MODE_SPEED,
> > + &step_cnt, &sample_cnt);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> > + writel(timing_reg, priv->base + OFFSET_TIMING);
> > + /* Set the high speed mode register */
> > + ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> > + &step_cnt, &sample_cnt);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + high_speed_reg = I2C_TIME_DEFAULT_VALUE |
> > + (sample_cnt << HS_SAMPLE_OFFSET) |
> > + (step_cnt << HS_STEP_OFFSET);
> > + writel(high_speed_reg, priv->base + OFFSET_HS);
> > + } else {
> > + ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> > + &step_cnt, &sample_cnt);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> > + /* Disable the high speed transaction */
> > + high_speed_reg = I2C_TIME_CLR_VALUE;
> > + writel(timing_reg, priv->base + OFFSET_TIMING);
> > + writel(high_speed_reg, priv->base + OFFSET_HS);
> > + }
> > +exit:
> > + mtk_i2c_clk_disable(priv);
> > + return ret;
> > +}
> > +
> > +static int mtk_i2c_do_transfer(struct mtk_i2c_priv *priv,
> > + struct i2c_msg *msgs,
> > + int num, int left_num)
>
> comment
>
I would add function comment.
> > +{
> > + u16 addr_reg;
> > + u16 start_reg;
> > + u16 control_reg;
> > + u16 restart_flag = 0;
> > + u16 irq_stat = 0;
> > + u8 trans_error = 0;
> > + bool tmo = false;
> > + u8 *ptr = msgs->buf;
> > + u16 data_size = msgs->len;
> > + u32 tmo_poll = 0;
> > + int ret;
> > +
> > + if (priv->auto_restart)
> > + restart_flag = I2C_RS_TRANSFER;
> > +
> > + control_reg = i2c_readl(priv, OFFSET_CONTROL) &
> > + ~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
> > +
> > + if (priv->speed > MAX_FS_MODE_SPEED || num > 1)
> > + control_reg |= I2C_CONTROL_RS;
> > +
> > + if (priv->op == I2C_MASTER_WRRD)
> > + control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> > + control_reg |= I2C_CONTROL_DMA_EN;
> > + i2c_writel(priv, OFFSET_CONTROL, control_reg);
> > +
> > + /* set start condition */
> > + if (priv->speed <= MAX_ST_MODE_SPEED)
> > + i2c_writel(priv, OFFSET_EXT_CONF, I2C_ST_START_CON);
> > + else
> > + i2c_writel(priv, OFFSET_EXT_CONF, I2C_FS_START_CON);
> > +
> > + addr_reg = msgs->addr << 1;
> > + if (priv->op == I2C_MASTER_RD)
> > + addr_reg |= I2C_M_RD;
> > + if (priv->zero_len)
> > + i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg | TRANS_ADDR_ONLY);
> > + else
> > + i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg);
> > +
> > + /* clear interrupt status */
> > + i2c_writel(priv, OFFSET_INTR_STAT, restart_flag | I2C_HS_NACKERR |
> > + I2C_ACKERR | I2C_TRANSAC_COMP);
> > + i2c_writel(priv, OFFSET_FIFO_ADDR_CLR, I2C_FIFO_ADDR_CLR);
> > +
> > + /* enable interrupt */
> > + i2c_writel(priv, OFFSET_INTR_MASK, restart_flag | I2C_HS_NACKERR |
> > + I2C_ACKERR | I2C_TRANSAC_COMP);
> > +
> > + /* set transfer and transaction len */
> > + if (priv->op == I2C_MASTER_WRRD) {
> > + i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> > + i2c_writel(priv, OFFSET_TRANSFER_LEN_AUX,
> > + (msgs + 1)->len);
> > + i2c_writel(priv, OFFSET_TRANSAC_LEN, I2C_WRRD_TRANAC_VALUE);
> > + } else {
> > + i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> > + i2c_writel(priv, OFFSET_TRANSAC_LEN, num);
> > + }
> > +
> > + /* prepare buffer data to start transfer */
> > + if (priv->op == I2C_MASTER_RD) {
> > + writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> > + OFFSET_INT_FLAG);
> > + writel(I2C_DMA_CON_RX, priv->pdmabase + OFFSET_CON);
> > + priv->rx_buff = (u8 *)noncached_alloc(msgs->len,
> > + ARCH_DMA_MINALIGN);
> > + writel((u32)priv->rx_buff, priv->pdmabase +
> > + OFFSET_RX_MEM_ADDR);
> > + writel(msgs->len, priv->pdmabase + OFFSET_RX_LEN);
> > + } else if (priv->op == I2C_MASTER_WR) {
> > + writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> > + OFFSET_INT_FLAG);
> > + writel(I2C_DMA_CON_TX, priv->pdmabase + OFFSET_CON);
> > + priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> > + ARCH_DMA_MINALIGN);
> > + memcpy(priv->tx_buff, msgs->buf, msgs->len);
> > + writel((u32)priv->tx_buff, priv->pdmabase +
> > + OFFSET_TX_MEM_ADDR);
> > + writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> > + } else {
> > + writel(I2C_DMA_CLR_FLAG, priv->pdmabase +
> > + OFFSET_INT_FLAG);
> > + writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_CON);
> > + priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> > + ARCH_DMA_MINALIGN);
>
> Do you need the cast?
yes,noncached_alloc() returns uinit and priv->tx_buff is u8 *,
if do not using cast, it would have warning -Wint-conversion.
>
> > + priv->rx_buff = (u8 *)noncached_alloc((msgs + 1)->len,
> > + ARCH_DMA_MINALIGN);
> > + memcpy(priv->tx_buff, msgs->buf, msgs->len);
> > + writel((u32)priv->tx_buff, priv->pdmabase +
> > + OFFSET_TX_MEM_ADDR);
> > + writel((u32)priv->rx_buff, priv->pdmabase +
> > + OFFSET_RX_MEM_ADDR);
> > + writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> > + writel((msgs + 1)->len, priv->pdmabase +
> > + OFFSET_RX_LEN);
> > + }
> > + writel(I2C_DMA_START_EN, priv->pdmabase + OFFSET_EN);
> > +
> > + if (!priv->auto_restart) {
> > + start_reg = I2C_TRANSAC_START;
> > + } else {
> > + start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
> > + if (left_num >= 1)
> > + start_reg |= I2C_RS_MUL_CNFG;
> > + }
> > + i2c_writel(priv, OFFSET_START, start_reg);
> > +
> > + for (;;) {
> > + irq_stat = i2c_readl(priv, OFFSET_INTR_STAT);
> > +
> > + /* ignore the first restart irq after the master code */
> > + if (priv->ignore_restart_irq && (irq_stat | restart_flag)) {
> > + priv->ignore_restart_irq = false;
> > + irq_stat = 0;
> > + writel(I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
> > + I2C_TRANSAC_START, priv->base + OFFSET_START);
> > + }
> > +
> > + if (irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
> > + tmo = false;
> > + if (irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))
> > + trans_error = 1;
> > +
> > + break;
> > + }
> > + udelay(1);
> > + if (tmo_poll++ >= TRANSFER_TIMEOUT) {
> > + tmo = true;
> > + break;
> > + }
> > + }
> > + /* clear interrupt mask */
> > + i2c_writel(priv, OFFSET_INTR_MASK, ~(restart_flag | I2C_HS_NACKERR |
> > + I2C_ACKERR | I2C_TRANSAC_COMP));
> > +
> > + if (!tmo && trans_error == 0) {
> > + /* transfer success ,we need to get data from fifo */
> > + if (priv->op == I2C_MASTER_RD || priv->op == I2C_MASTER_WRRD) {
> > + ptr = priv->op == I2C_MASTER_RD ?
> > + msgs->buf : (msgs + 1)->buf;
> > + data_size = priv->op == I2C_MASTER_RD ?
> > + msgs->len : (msgs + 1)->len;
> > + memcpy(ptr, priv->rx_buff, data_size);
> > + }
> > + } else {
> > + /* timeout or ACKERR */
> > + if (tmo) {
> > + ret = -ETIMEDOUT;
> > + if (!priv->filter_msg)
> > + debug("transfer timeout! addr: 0x%x,\n",
> > + msgs->addr);
> > + } else {
> > + ret = -ENXIO;
>
> Should that be REMOTEIO? Up to you.
>
I would modify it to EREMOTEIO.
> > + if (!priv->filter_msg)
> > + debug("ACKERR! addr: 0x%x,IRQ:0x%x\n",
> > + msgs->addr, irq_stat);
> > + }
> > + mtk_i2c_init_hw(priv);
> > + return ret;
> > + }
> > + return 0;
> > +}
> > +
> > +static int mtk_i2c_transfer(struct udevice *dev, struct i2c_msg *msg,
> > + int nmsgs)
> > +{
> > + int ret;
> > + int left_num;
> > + u8 num_cnt;
>
> uint (please fix globally)
I would fixed this globally.
>
> > + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > + priv->auto_restart = true;
> > + left_num = nmsgs;
> > + mtk_i2c_clk_enable(priv);
>
> error check
I would do error check globally.
>
> > + for (num_cnt = 0; num_cnt < nmsgs; num_cnt++) {
> > + if (((msg + num_cnt)->addr) > MAX_I2C_ADDR) {
> > + ret = -EINVAL;
> > + goto err_exit;
> > + }
> > + if ((msg + num_cnt)->len > MAX_I2C_LEN) {
> > + ret = -EINVAL;
> > + goto err_exit;
> > + }
> > + }
> > +
> > + /* checking if we can skip restart and optimize using WRRD mode */
>
> check
>
I would modify this.
> > + if (priv->auto_restart && nmsgs == 2) {
> > + if (!(msg[0].flags & I2C_M_RD) && (msg[1].flags & I2C_M_RD) &&
> > + msg[0].addr == msg[1].addr) {
> > + priv->auto_restart = false;
> > + }
> > + }
> > +
> > + if (priv->auto_restart && nmsgs >= 2 && priv->speed > MAX_FS_MODE_SPEED)
> > + /* ignore the first restart irq after the master code,
> > + * otherwise the first transfer will be discarded.
> > + */
> > + priv->ignore_restart_irq = true;
> > + else
> > + priv->ignore_restart_irq = false;
> > +
> > + while (left_num--) {
> > + /*priv->zero_len = true
>
> What is that comment for?
This is for debug before and I would drop it.
>
> > + *transfer slave address only to support devices detect
> > + */
> > + if (!msg->buf)
> > + priv->zero_len = true;
> > + else
> > + priv->zero_len = false;
> > +
> > + if (msg->flags & I2C_M_RD)
> > + priv->op = I2C_MASTER_RD;
> > + else
> > + priv->op = I2C_MASTER_WR;
> > +
> > + if (!priv->auto_restart) {
> > + if (nmsgs > 1) {
> > + /* combined two messages into one transaction */
> > + priv->op = I2C_MASTER_WRRD;
> > + left_num--;
> > + }
> > + }
> > + ret = mtk_i2c_do_transfer(priv, msg, nmsgs, left_num);
> > + if (ret < 0)
> > + goto err_exit;
> > + msg++;
> > + }
> > + ret = I2C_OK;
>
> ret = 0
>
> > +
> > +err_exit:
> > + mtk_i2c_clk_disable(priv);
>
> error check
>
> > + return ret;
> > +}
> > +
> > +static int mtk_i2c_ofdata_to_platdata(struct udevice *dev)
> > +{
> > + int ret;
> > + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > + priv->base = dev_remap_addr_index(dev, 0);
> > + priv->pdmabase = dev_remap_addr_index(dev, 1);
> > + ret = clk_get_by_index(dev, 0, &priv->clk_main);
> > + if (ret)
> > + return ret;
>
> consider log_ret()ret or log_msg_ret(ret)
>
I would modify it.
> > +
> > + ret = clk_get_by_index(dev, 1, &priv->clk_dma);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_i2c_probe(struct udevice *dev)
> > +{
> > + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > + priv->soc_data = (struct mtk_i2c_soc_data *)dev_get_driver_data(dev);
> > + mtk_i2c_clk_enable(priv);
> > + mtk_i2c_init_hw(priv);
> > + mtk_i2c_clk_disable(priv);
> > + return 0;
> > +}
> > +
> > +static int mtk_i2c_deblock(struct udevice *dev)
> > +{
> > + struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > + mtk_i2c_clk_enable(priv);
> > + mtk_i2c_init_hw(priv);
> > + mtk_i2c_clk_disable(priv);
> > + return 0;
> > +}
> > +
> > +static const struct mtk_i2c_soc_data mt8518_soc_data = {
> > + .dma_sync = 0,
> > +};
> > +
> > +static const struct mtk_i2c_soc_data mt8512_soc_data = {
> > + .dma_sync = 1,
> > +};
> > +
> > +static const struct dm_i2c_ops mtk_i2c_ops = {
> > + .xfer = mtk_i2c_transfer,
> > + .set_bus_speed = mtk_i2c_set_speed,
> > + .deblock = mtk_i2c_deblock,
> > +};
> > +
> > +static const struct udevice_id mtk_i2c_ids[] = {
> > + {
> > + .compatible = "mediatek,mt8518-i2c",
> > + .data = (ulong)&mt8518_soc_data,
> > + },
> > + {
>
> Put the above { on the line above
>
I would modify it.
> > + .compatible = "mediatek,mt8512-i2c",
> > + .data = (ulong)&mt8512_soc_data,
> > + },
> > + {
> > + }
>
> same here
>
> > +};
> > +
> > +U_BOOT_DRIVER(mtk_i2c) = {
> > + .name = "mtk_i2c",
> > + .id = UCLASS_I2C,
> > + .of_match = mtk_i2c_ids,
> > + .ofdata_to_platdata = mtk_i2c_ofdata_to_platdata,
> > + .probe = mtk_i2c_probe,
> > + .priv_auto_alloc_size = sizeof(struct mtk_i2c_priv),
> > + .ops = &mtk_i2c_ops,
> > +};
> > --
> > 2.18.0
>
> Regards,
> SImon
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK
2020-09-30 8:21 ` [PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK mingming lee
@ 2020-10-12 3:34 ` Simon Glass
0 siblings, 0 replies; 14+ messages in thread
From: Simon Glass @ 2020-10-12 3:34 UTC (permalink / raw)
To: u-boot
On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> add Mediatek i2c controller driver to ARM MEDIATEK.
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
> MAINTAINERS | 2 ++
> 1 file changed, 2 insertions(+)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512
2020-09-30 8:21 ` [PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512 mingming lee
@ 2020-10-12 3:34 ` Simon Glass
0 siblings, 0 replies; 14+ messages in thread
From: Simon Glass @ 2020-10-12 3:34 UTC (permalink / raw)
To: u-boot
On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> add i2c dts node support for mt8512
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
> arch/arm/dts/mt8512-bm1-emmc.dts | 12 ++++++++++
> arch/arm/dts/mt8512.dtsi | 38 +++++++++++++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 1 deletion(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] configs: mt8512: Enable I2C related configs
2020-09-30 8:21 ` [PATCH v2 3/5] configs: mt8512: Enable I2C related configs mingming lee
@ 2020-10-12 3:34 ` Simon Glass
2020-10-12 8:33 ` Mingming Lee
0 siblings, 1 reply; 14+ messages in thread
From: Simon Glass @ 2020-10-12 3:34 UTC (permalink / raw)
To: u-boot
On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> Enable MTK I2C
> Enable I2C basic command
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
> configs/mt8512_bm1_emmc_defconfig | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
Please check that the sorting is correct.
Something like this bash function may help:
# $1 is config name
function fix_kconfig()
{
make O=/tmp/b/$1_defconfig cfg && \
make O=/tmp/b/$1 savedefconfig && \
meld /tmp/b/$1/defconfig configs/$1_defconfig
}
Regards,
Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver
2020-09-30 8:21 ` [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver mingming lee
@ 2020-10-12 3:34 ` Simon Glass
2020-10-12 8:37 ` Mingming Lee
0 siblings, 1 reply; 14+ messages in thread
From: Simon Glass @ 2020-10-12 3:34 UTC (permalink / raw)
To: u-boot
On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> add bindings for mediatek i2c driver
Should indicate which version of linux this file comes from.
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
> doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
> create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/5] configs: mt8512: Enable I2C related configs
2020-10-12 3:34 ` Simon Glass
@ 2020-10-12 8:33 ` Mingming Lee
0 siblings, 0 replies; 14+ messages in thread
From: Mingming Lee @ 2020-10-12 8:33 UTC (permalink / raw)
To: u-boot
hello Simon,
thank you for your review
I have re-order it and would send it in next version.
On Sun, 2020-10-11 at 21:34 -0600, Simon Glass wrote:
> On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
> >
> > From: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > Enable MTK I2C
> > Enable I2C basic command
> >
> > Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> > ---
> > configs/mt8512_bm1_emmc_defconfig | 3 +++
> > 1 file changed, 3 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Please check that the sorting is correct.
>
> Something like this bash function may help:
>
> # $1 is config name
> function fix_kconfig()
> {
> make O=/tmp/b/$1_defconfig cfg && \
> make O=/tmp/b/$1 savedefconfig && \
> meld /tmp/b/$1/defconfig configs/$1_defconfig
> }
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver
2020-10-12 3:34 ` Simon Glass
@ 2020-10-12 8:37 ` Mingming Lee
0 siblings, 0 replies; 14+ messages in thread
From: Mingming Lee @ 2020-10-12 8:37 UTC (permalink / raw)
To: u-boot
hello Simon,
On Sun, 2020-10-11 at 21:34 -0600, Simon Glass wrote:
> On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
> >
> > From: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > add bindings for mediatek i2c driver
>
> Should indicate which version of linux this file comes from.
>
we just send it in u-boot, do not sync with kernel yet.
> >
> > Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> > ---
> > doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> > create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-12 8:37 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 8:21 [PATCH v2 0/5] Add i2c support for MediaTek mt8512 mingming lee
2020-09-30 8:21 ` [PATCH v2 1/5] i2c: mediatek: add basic driver support mingming lee
2020-10-06 0:02 ` Simon Glass
2020-10-10 2:23 ` Mingming Lee
2020-09-30 8:21 ` [PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512 mingming lee
2020-10-12 3:34 ` Simon Glass
2020-09-30 8:21 ` [PATCH v2 3/5] configs: mt8512: Enable I2C related configs mingming lee
2020-10-12 3:34 ` Simon Glass
2020-10-12 8:33 ` Mingming Lee
2020-09-30 8:21 ` [PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver mingming lee
2020-10-12 3:34 ` Simon Glass
2020-10-12 8:37 ` Mingming Lee
2020-09-30 8:21 ` [PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK mingming lee
2020-10-12 3:34 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.