Message ID | 20240819092850.1590758-3-ryan_chen@aspeedtech.com |
---|---|
State | New |
Headers | show |
Series | Add ASPEED AST2600 I2Cv2 controller driver | expand |
On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > Add i2c new register mode driver to support AST2600 i2c > new register mode. AST2600 i2c controller have legacy and > new register mode. The new register mode have global register > support 4 base clock for scl clock selection, and new clock > divider mode. The new register mode have separate register > set to control i2c master and slave. This patch is for i2c > master mode driver. ... > +struct ast2600_i2c_bus { Have you run `pahole` to be sure the layout is optimal? > + struct i2c_adapter adap; > + struct device *dev; > + void __iomem *reg_base; > + struct regmap *global_regs; > + struct reset_control *rst; > + int irq; > + enum xfer_mode mode; > + struct clk *clk; > + u32 apb_clk; > + struct i2c_timings timing_info; > + u32 timeout; > + /* smbus alert */ > + bool alert_enable; > + struct i2c_smbus_alert_setup alert_data; > + struct i2c_client *ara; > + /* Multi-master */ > + bool multi_master; > + /* master structure */ > + int cmd_err; > + struct completion cmd_complete; > + struct i2c_msg *msgs; > + size_t buf_index; > + /* cur xfer msgs index*/ > + int msgs_index; > + int msgs_count; > + u8 *master_safe_buf; > + dma_addr_t master_dma_addr; > + /*total xfer count */ > + int master_xfer_cnt; > + /* Buffer mode */ > + void __iomem *buf_base; > + size_t buf_size; > +}; ... > +static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus) > +{ > + unsigned long base_clk[16]; > + int baseclk_idx; > + u32 clk_div_reg; > + u32 scl_low; > + u32 scl_high; > + int divisor; > + u32 data; > + > + regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg); > + > + for (int i = 0; i < 16; i++) { unsigned int ARRAY_SIZE(base_clk) // Will need array_size.h > + if (i == 0) > + base_clk[i] = i2c_bus->apb_clk; > + else if ((i > 0) || (i < 5)) > + base_clk[i] = (i2c_bus->apb_clk * 2) / > + (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); > + else > + base_clk[i] = base_clk[4] / (1 << (i - 5)); This is the same as if (i == 0) base_clk[i] = i2c_bus->apb_clk; else if (i < 5) base_clk[i] = (i2c_bus->apb_clk * 2) / (((clk_div_reg / BIT((i - 1) * 8)) & GENMASK(7, 0)) + 2); else base_clk[i] = base_clk[4] / BIT(i - 5); Alternatively if (i == 0) base_clk[i] = i2c_bus->apb_clk; else if (i < 5) base_clk[i] = (i2c_bus->apb_clk * 2) / (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); else base_clk[i] = base_clk[4] >> (i - 5); > + > + if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32) { > + baseclk_idx = i; > + divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); > + break; > + } > + } > + baseclk_idx = min(baseclk_idx, 15); If the last conditional inside the loop is never true, you are going to use\ a garbage here. > + divisor = min(divisor, 32); Ditto. > + scl_low = min(divisor * 9 / 16 - 1, 15); Missing minmax.h in the inclusion block. > + scl_high = (divisor - scl_low - 2) & GENMASK(3, 0); > + data = (scl_high - 1) << 20 | scl_high << 16 | scl_low << 12 | baseclk_idx; > + if (i2c_bus->timeout) { > + data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK); > + data |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout); > + } > + > + return data; > +} ... > +static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) > +{ > + u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > + int ret = 0; > + u32 ctrl; > + int r; > + > + dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr, state); > + > + ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > + > + /* Disable master/slave mode */ > + writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN), > + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > + > + /* Enable master mode only */ > + writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | AST2600_I2CC_MASTER_EN, > + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > + > + reinit_completion(&i2c_bus->cmd_complete); > + i2c_bus->cmd_err = 0; > + > + /* Check 0x14's SDA and SCL status */ > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { > + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); > + if (r == 0) { > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > + ret = -ETIMEDOUT; > + } else { > + if (i2c_bus->cmd_err) { > + dev_dbg(i2c_bus->dev, "recovery error\n"); > + ret = -EPROTO; > + } > + } > + } ret is set but maybe overridden. > + /* Recovery done */ Even if it fails above? > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > + ret = -EPROTO; > + } > + > + /* restore original master/slave setting */ > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > + return ret; > +} ... > +static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) > +{ > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > + int xfer_len; > + > + cmd |= AST2600_I2CM_PKT_EN; > + xfer_len = msg->len - i2c_bus->master_xfer_cnt; > + if (xfer_len > AST2600_I2C_DMA_SIZE) { > + xfer_len = AST2600_I2C_DMA_SIZE; > + } else { > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) else if (...) > + cmd |= AST2600_I2CM_STOP_CMD; > + } > + > + if (cmd & AST2600_I2CM_START_CMD) { > + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); > + i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); > + if (!i2c_bus->master_safe_buf) > + return -ENOMEM; > + i2c_bus->master_dma_addr = > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, > + msg->len, DMA_TO_DEVICE); > + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) { > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); > + i2c_bus->master_safe_buf = NULL; > + return -ENOMEM; Why is the dma_mapping_error() returned error code shadowed? > + } > + } > + > + if (xfer_len) { > + cmd |= AST2600_I2CM_TX_DMA_EN | AST2600_I2CM_TX_CMD; > + writel(AST2600_I2CM_SET_TX_DMA_LEN(xfer_len - 1), > + i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); > + writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt, > + i2c_bus->reg_base + AST2600_I2CM_TX_DMA); > + } > + > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + > + return 0; > +} > + > +static int ast2600_i2c_setup_buff_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) > +{ > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > + u32 wbuf_dword; > + int xfer_len; > + u8 wbuf[4]; > + int i; Why signed? > + cmd |= AST2600_I2CM_PKT_EN; > + xfer_len = msg->len - i2c_bus->master_xfer_cnt; > + if (xfer_len > i2c_bus->buf_size) { > + xfer_len = i2c_bus->buf_size; > + } else { > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) else if (...) > + cmd |= AST2600_I2CM_STOP_CMD; > + } > + > + if (cmd & AST2600_I2CM_START_CMD) > + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); > + > + if (xfer_len) { > + cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD; > + /* > + * The controller's buffer register supports dword writes only. > + * Therefore, write dwords to the buffer register in a 4-byte aligned, > + * and write the remaining unaligned data at the end. > + */ > + for (i = 0; i < xfer_len; i++) { > + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > + if ((i % 4) == 3 || i == xfer_len - 1) { > + wbuf_dword = get_unaligned_le32(wbuf); > + writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > + } > + } This is overcomplicated and can be simplified. Why you can't perform get_unaligned_leXX(msg->buf[i2c_bus->master_xfer_cnt + i]); ? for (i = 0; i < xfer_len; i += 4) { switch (min(xfer_len - i, 4) % 4) { case 1: wbuf_dword = ...; writel(wbuf_dword, i2c_bus->buf_base + i); break; case 2: wbuf_dword = get_unaligned_le16(...); writel(wbuf_dword, i2c_bus->buf_base + i); break; case 3: wbuf_dword = get_unaligned_le24(...); writel(wbuf_dword, i2c_bus->buf_base + i); break; default: wbuf_dword = get_unaligned_le32(...); writel(wbuf_dword, i2c_bus->buf_base + i); break; } } Now, with this it's can be a helper, with which for (i = 0; i < xfer_len; i += 4) { switch (min(xfer_len - i, 4) % 4) { case 1: ast2600_write_data(i2c_bus, i, ...); break; case 2: ast2600_write_data(i2c_bus, i, get_unaligned_le16(...)); break; case 3: ast2600_write_data(i2c_bus, i, get_unaligned_le24(...)); break; default: ast2600_write_data(i2c_bus, i, get_unaligned_le32(...)); break; } } > + writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len), > + i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); > + } > + > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + > + return 0; > +} ... > +static int ast2600_i2c_setup_dma_rx(struct ast2600_i2c_bus *i2c_bus) > +{ > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > + int xfer_len; > + u32 cmd; > + > + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | > + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_DMA_EN; > + > + if (msg->flags & I2C_M_RECV_LEN) { > + xfer_len = 1; > + } else { > + if (msg->len > AST2600_I2C_DMA_SIZE) { } else if (...) { > + xfer_len = AST2600_I2C_DMA_SIZE; > + } else { > + xfer_len = msg->len; > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) > + cmd |= MASTER_TRIGGER_LAST_STOP; > + } > + } > + writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1), i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); > + i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); > + if (!i2c_bus->master_safe_buf) > + return -ENOMEM; > + i2c_bus->master_dma_addr = > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, msg->len, DMA_FROM_DEVICE); > + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) { > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); > + i2c_bus->master_safe_buf = NULL; > + return -ENOMEM; > + } > + writel(i2c_bus->master_dma_addr, i2c_bus->reg_base + AST2600_I2CM_RX_DMA); > + > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + > + return 0; > +} > + > +static int ast2600_i2c_setup_buff_rx(struct ast2600_i2c_bus *i2c_bus) > +{ > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > + int xfer_len; > + u32 cmd; > + > + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | > + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_BUFF_EN; > + > + if (msg->flags & I2C_M_RECV_LEN) { > + dev_dbg(i2c_bus->dev, "smbus read\n"); > + xfer_len = 1; > + } else { > + if (msg->len > i2c_bus->buf_size) { } else if (...) { > + xfer_len = i2c_bus->buf_size; > + } else { > + xfer_len = msg->len; > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) > + cmd |= MASTER_TRIGGER_LAST_STOP; > + } > + } > + writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); > + > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + > + return 0; > +} > + > +static int ast2600_i2c_setup_byte_rx(struct ast2600_i2c_bus *i2c_bus) > +{ > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > + u32 cmd; > + > + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | > + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_CMD; > + > + if (msg->flags & I2C_M_RECV_LEN) { > + dev_dbg(i2c_bus->dev, "smbus read\n"); > + } else { > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) { } else if (...) { > + if (msg->len == 1) > + cmd |= MASTER_TRIGGER_LAST_STOP; > + } > + } > + > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > + > + return 0; > +} ... > +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) > +{ > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > + > + /* send start */ > + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", Drop 'ing', no need to have this in the debug message. > + i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > + msg->len, msg->len > 1 ? "s" : "", str_plural() > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > + i2c_bus->master_xfer_cnt = 0; > + i2c_bus->buf_index = 0; > + > + if (msg->flags & I2C_M_RD) { > + if (i2c_bus->mode == DMA_MODE) > + return ast2600_i2c_setup_dma_rx(i2c_bus); > + else if (i2c_bus->mode == BUFF_MODE) > + return ast2600_i2c_setup_buff_rx(i2c_bus); > + else > + return ast2600_i2c_setup_byte_rx(i2c_bus); > + } else { > + if (i2c_bus->mode == DMA_MODE) > + return ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > + else if (i2c_bus->mode == BUFF_MODE) > + return ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); > + else > + return ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); > + } > +} ... > +master_out: > + if (i2c_bus->mode == DMA_MODE) { > + kfree(i2c_bus->master_safe_buf); > + i2c_bus->master_safe_buf = NULL; > + } Indentation issues. > + return ret; ... > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); Why do you need this table before _probe()? Isn't the only user is below? > +static int ast2600_i2c_probe(struct platform_device *pdev) ... > + i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,global-regs"); dev_of_node(dev) > + if (IS_ERR(i2c_bus->global_regs)) > + return PTR_ERR(i2c_bus->global_regs); ... > + if (device_property_read_bool(&pdev->dev, "aspeed,enable-dma")) You have 'dev' Why not use it? > + i2c_bus->mode = DMA_MODE; ... > + if (i2c_bus->mode == BUFF_MODE) { > + i2c_bus->buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res); > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > + i2c_bus->buf_size = resource_size(res) / 2; > + else > + i2c_bus->mode = BYTE_MODE; What's wrong with positive conditional? And is it even possible to have NULL here? > + } ... > + strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name)); Use 2-argument strscpy(). ... > + i2c_bus->alert_enable = device_property_read_bool(dev, "smbus-alert"); > + if (i2c_bus->alert_enable) { > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data); > + if (!i2c_bus->ara) > + dev_warn(dev, "Failed to register ARA client\n"); > + > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } else { > + i2c_bus->alert_enable = false; > + /* Set interrupt generation of I2C master controller */ > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, > + i2c_bus->reg_base + AST2600_I2CM_IER); > + } I2C core calls i2c_setup_smbus_alert() when registering the adapter. Why do you need to have something special here?
Hi Ryan, kernel test robot noticed the following build errors: [auto build test ERROR on andi-shyti/i2c/i2c-host] [also build test ERROR on linus/master v6.11-rc4 next-20240819] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20240819-173106 base: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240819092850.1590758-3-ryan_chen%40aspeedtech.com patch subject: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register mode driver config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240819/202408192327.nZeNynmO-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240819/202408192327.nZeNynmO-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408192327.nZeNynmO-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/i2c/busses/i2c-ast2600.c: In function 'ast2600_i2c_setup_buff_tx': >> drivers/i2c/busses/i2c-ast2600.c:442:46: error: implicit declaration of function 'get_unaligned_le32' [-Wimplicit-function-declaration] 442 | wbuf_dword = get_unaligned_le32(wbuf); | ^~~~~~~~~~~~~~~~~~ vim +/get_unaligned_le32 +442 drivers/i2c/busses/i2c-ast2600.c 411 412 static int ast2600_i2c_setup_buff_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) 413 { 414 struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; 415 u32 wbuf_dword; 416 int xfer_len; 417 u8 wbuf[4]; 418 int i; 419 420 cmd |= AST2600_I2CM_PKT_EN; 421 xfer_len = msg->len - i2c_bus->master_xfer_cnt; 422 if (xfer_len > i2c_bus->buf_size) { 423 xfer_len = i2c_bus->buf_size; 424 } else { 425 if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) 426 cmd |= AST2600_I2CM_STOP_CMD; 427 } 428 429 if (cmd & AST2600_I2CM_START_CMD) 430 cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); 431 432 if (xfer_len) { 433 cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD; 434 /* 435 * The controller's buffer register supports dword writes only. 436 * Therefore, write dwords to the buffer register in a 4-byte aligned, 437 * and write the remaining unaligned data at the end. 438 */ 439 for (i = 0; i < xfer_len; i++) { 440 wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; 441 if ((i % 4) == 3 || i == xfer_len - 1) { > 442 wbuf_dword = get_unaligned_le32(wbuf); 443 writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); 444 } 445 } 446 447 writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len), 448 i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); 449 } 450 451 writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); 452 453 return 0; 454 } 455
Hi Ryan, kernel test robot noticed the following build errors: [auto build test ERROR on andi-shyti/i2c/i2c-host] [also build test ERROR on linus/master v6.11-rc4 next-20240819] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20240819-173106 base: https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host patch link: https://lore.kernel.org/r/20240819092850.1590758-3-ryan_chen%40aspeedtech.com patch subject: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register mode driver config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240820/202408200236.8yWq6lLk-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 26670e7fa4f032a019d23d56c6a02926e854e8af) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240820/202408200236.8yWq6lLk-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408200236.8yWq6lLk-lkp@intel.com/ All error/warnings (new ones prefixed by >>): In file included from drivers/i2c/busses/i2c-ast2600.c:11: In file included from include/linux/dma-mapping.h:11: In file included from include/linux/scatterlist.h:8: In file included from include/linux/mm.h:2228: include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ In file included from drivers/i2c/busses/i2c-ast2600.c:11: In file included from include/linux/dma-mapping.h:11: In file included from include/linux/scatterlist.h:9: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 548 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from drivers/i2c/busses/i2c-ast2600.c:11: In file included from include/linux/dma-mapping.h:11: In file included from include/linux/scatterlist.h:9: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from drivers/i2c/busses/i2c-ast2600.c:11: In file included from include/linux/dma-mapping.h:11: In file included from include/linux/scatterlist.h:9: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 585 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ >> drivers/i2c/busses/i2c-ast2600.c:295:20: warning: overlapping comparisons always evaluate to true [-Wtautological-overlap-compare] 295 | else if ((i > 0) || (i < 5)) | ~~~~~~~~^~~~~~~~~~ drivers/i2c/busses/i2c-ast2600.c:292:18: warning: variable 'baseclk_idx' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized] 292 | for (int i = 0; i < 16; i++) { | ^~~~~~ drivers/i2c/busses/i2c-ast2600.c:307:20: note: uninitialized use occurs here 307 | baseclk_idx = min(baseclk_idx, 15); | ^~~~~~~~~~~ include/linux/minmax.h:129:38: note: expanded from macro 'min' 129 | #define min(x, y) __careful_cmp(min, x, y) | ^ include/linux/minmax.h:105:25: note: expanded from macro '__careful_cmp' 105 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^ include/linux/minmax.h:99:20: note: expanded from macro '__careful_cmp_once' 99 | __auto_type ux = (x); __auto_type uy = (y); \ | ^ drivers/i2c/busses/i2c-ast2600.c:292:18: note: remove the condition if it is always true 292 | for (int i = 0; i < 16; i++) { | ^~~~~~ drivers/i2c/busses/i2c-ast2600.c:283:17: note: initialize the variable 'baseclk_idx' to silence this warning 283 | int baseclk_idx; | ^ | = 0 >> drivers/i2c/busses/i2c-ast2600.c:442:18: error: call to undeclared function 'get_unaligned_le32'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 442 | wbuf_dword = get_unaligned_le32(wbuf); | ^ 9 warnings and 1 error generated. vim +/get_unaligned_le32 +442 drivers/i2c/busses/i2c-ast2600.c 279 280 static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus) 281 { 282 unsigned long base_clk[16]; 283 int baseclk_idx; 284 u32 clk_div_reg; 285 u32 scl_low; 286 u32 scl_high; 287 int divisor; 288 u32 data; 289 290 regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg); 291 292 for (int i = 0; i < 16; i++) { 293 if (i == 0) 294 base_clk[i] = i2c_bus->apb_clk; > 295 else if ((i > 0) || (i < 5)) 296 base_clk[i] = (i2c_bus->apb_clk * 2) / 297 (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); 298 else 299 base_clk[i] = base_clk[4] / (1 << (i - 5)); 300 301 if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32) { 302 baseclk_idx = i; 303 divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); 304 break; 305 } 306 } 307 baseclk_idx = min(baseclk_idx, 15); 308 divisor = min(divisor, 32); 309 scl_low = min(divisor * 9 / 16 - 1, 15); 310 scl_high = (divisor - scl_low - 2) & GENMASK(3, 0); 311 data = (scl_high - 1) << 20 | scl_high << 16 | scl_low << 12 | baseclk_idx; 312 if (i2c_bus->timeout) { 313 data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK); 314 data |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout); 315 } 316 317 return data; 318 } 319 320 static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) 321 { 322 u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); 323 int ret = 0; 324 u32 ctrl; 325 int r; 326 327 dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr, state); 328 329 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); 330 331 /* Disable master/slave mode */ 332 writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN), 333 i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); 334 335 /* Enable master mode only */ 336 writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | AST2600_I2CC_MASTER_EN, 337 i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); 338 339 reinit_completion(&i2c_bus->cmd_complete); 340 i2c_bus->cmd_err = 0; 341 342 /* Check 0x14's SDA and SCL status */ 343 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); 344 if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { 345 writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); 346 r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); 347 if (r == 0) { 348 dev_dbg(i2c_bus->dev, "recovery timed out\n"); 349 ret = -ETIMEDOUT; 350 } else { 351 if (i2c_bus->cmd_err) { 352 dev_dbg(i2c_bus->dev, "recovery error\n"); 353 ret = -EPROTO; 354 } 355 } 356 } 357 358 /* Recovery done */ 359 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); 360 if (state & AST2600_I2CC_BUS_BUSY_STS) { 361 dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); 362 ret = -EPROTO; 363 } 364 365 /* restore original master/slave setting */ 366 writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); 367 return ret; 368 } 369 370 static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) 371 { 372 struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; 373 int xfer_len; 374 375 cmd |= AST2600_I2CM_PKT_EN; 376 xfer_len = msg->len - i2c_bus->master_xfer_cnt; 377 if (xfer_len > AST2600_I2C_DMA_SIZE) { 378 xfer_len = AST2600_I2C_DMA_SIZE; 379 } else { 380 if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) 381 cmd |= AST2600_I2CM_STOP_CMD; 382 } 383 384 if (cmd & AST2600_I2CM_START_CMD) { 385 cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); 386 i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); 387 if (!i2c_bus->master_safe_buf) 388 return -ENOMEM; 389 i2c_bus->master_dma_addr = 390 dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, 391 msg->len, DMA_TO_DEVICE); 392 if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) { 393 i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); 394 i2c_bus->master_safe_buf = NULL; 395 return -ENOMEM; 396 } 397 } 398 399 if (xfer_len) { 400 cmd |= AST2600_I2CM_TX_DMA_EN | AST2600_I2CM_TX_CMD; 401 writel(AST2600_I2CM_SET_TX_DMA_LEN(xfer_len - 1), 402 i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); 403 writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt, 404 i2c_bus->reg_base + AST2600_I2CM_TX_DMA); 405 } 406 407 writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); 408 409 return 0; 410 } 411 412 static int ast2600_i2c_setup_buff_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) 413 { 414 struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; 415 u32 wbuf_dword; 416 int xfer_len; 417 u8 wbuf[4]; 418 int i; 419 420 cmd |= AST2600_I2CM_PKT_EN; 421 xfer_len = msg->len - i2c_bus->master_xfer_cnt; 422 if (xfer_len > i2c_bus->buf_size) { 423 xfer_len = i2c_bus->buf_size; 424 } else { 425 if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) 426 cmd |= AST2600_I2CM_STOP_CMD; 427 } 428 429 if (cmd & AST2600_I2CM_START_CMD) 430 cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); 431 432 if (xfer_len) { 433 cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD; 434 /* 435 * The controller's buffer register supports dword writes only. 436 * Therefore, write dwords to the buffer register in a 4-byte aligned, 437 * and write the remaining unaligned data at the end. 438 */ 439 for (i = 0; i < xfer_len; i++) { 440 wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; 441 if ((i % 4) == 3 || i == xfer_len - 1) { > 442 wbuf_dword = get_unaligned_le32(wbuf); 443 writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); 444 } 445 } 446 447 writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len), 448 i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); 449 } 450 451 writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); 452 453 return 0; 454 } 455
> Subject: Re: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register > mode driver > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > > Add i2c new register mode driver to support AST2600 i2c new register > > mode. AST2600 i2c controller have legacy and new register mode. The > > new register mode have global register support 4 base clock for scl > > clock selection, and new clock divider mode. The new register mode > > have separate register set to control i2c master and slave. This patch > > is for i2c master mode driver. > > ... > > > +struct ast2600_i2c_bus { > > Have you run `pahole` to be sure the layout is optimal? It will replace by following. struct ast2600_i2c_bus { struct i2c_adapter adap; struct device *dev; void __iomem *reg_base; struct regmap *global_regs; struct reset_control *rst; struct clk *clk; struct i2c_timings timing_info; struct completion cmd_complete; struct i2c_client *ara; struct i2c_msg *msgs; u8 *master_safe_buf; dma_addr_t master_dma_addr; u32 apb_clk; u32 timeout; int irq; int cmd_err; int msgs_index; int msgs_count; int master_xfer_cnt; size_t buf_index; size_t buf_size; enum xfer_mode mode; bool alert_enable; bool multi_master; /* Buffer mode */ void __iomem *buf_base; struct i2c_smbus_alert_setup alert_data; }; > > > + struct i2c_adapter adap; > > + struct device *dev; > > + void __iomem *reg_base; > > + struct regmap *global_regs; > > + struct reset_control *rst; > > + int irq; > > + enum xfer_mode mode; > > + struct clk *clk; > > + u32 apb_clk; > > + struct i2c_timings timing_info; > > + u32 timeout; > > + /* smbus alert */ > > + bool alert_enable; > > + struct i2c_smbus_alert_setup alert_data; > > + struct i2c_client *ara; > > + /* Multi-master */ > > + bool multi_master; > > + /* master structure */ > > + int cmd_err; > > + struct completion cmd_complete; > > + struct i2c_msg *msgs; > > + size_t buf_index; > > + /* cur xfer msgs index*/ > > + int msgs_index; > > + int msgs_count; > > + u8 *master_safe_buf; > > + dma_addr_t master_dma_addr; > > + /*total xfer count */ > > + int master_xfer_cnt; > > + /* Buffer mode */ > > + void __iomem *buf_base; > > + size_t buf_size; > > +}; > > ... > > > +static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus) > > +{ > > + unsigned long base_clk[16]; > > + int baseclk_idx; > > + u32 clk_div_reg; > > + u32 scl_low; > > + u32 scl_high; > > + int divisor; > > + u32 data; > > + > > + regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, > > +&clk_div_reg); > > + > > + for (int i = 0; i < 16; i++) { > > unsigned int > ARRAY_SIZE(base_clk) // Will need array_size.h Will update to for (int i = 0; i < ARRAY_SIZE(base_clk); i++) And add include array_size.h > > > > + if (i == 0) > > + base_clk[i] = i2c_bus->apb_clk; > > + else if ((i > 0) || (i < 5)) > > + base_clk[i] = (i2c_bus->apb_clk * 2) / > > + (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); > > + else > > + base_clk[i] = base_clk[4] / (1 << (i - 5)); > > This is the same as > > if (i == 0) > base_clk[i] = i2c_bus->apb_clk; > else if (i < 5) > base_clk[i] = (i2c_bus->apb_clk * 2) / > (((clk_div_reg / BIT((i - 1) * 8)) & GENMASK(7, 0)) + > 2); > else > base_clk[i] = base_clk[4] / BIT(i - 5); > > Alternatively > > if (i == 0) > base_clk[i] = i2c_bus->apb_clk; > else if (i < 5) > base_clk[i] = (i2c_bus->apb_clk * 2) / > (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); > else > base_clk[i] = base_clk[4] >> (i - 5); > Will take the for better understand. if (i == 0) base_clk[i] = i2c_bus->apb_clk; else if (i < 5) base_clk[i] = (i2c_bus->apb_clk * 2) / (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); else base_clk[i] = base_clk[4] >> (i - 5); > > + > > + if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32) { > > + baseclk_idx = i; > > + divisor = DIV_ROUND_UP(base_clk[i], > i2c_bus->timing_info.bus_freq_hz); > > + break; > > + } > > + } > > > + baseclk_idx = min(baseclk_idx, 15); > > If the last conditional inside the loop is never true, you are going to use\ a > garbage here. I will give initial int baseclk_idx = 0; in function begin. > > > + divisor = min(divisor, 32); > > Ditto. I will give u32 divisor = 32; in function begin. > > > + scl_low = min(divisor * 9 / 16 - 1, 15); > > Missing minmax.h in the inclusion block. Will add. > > > + scl_high = (divisor - scl_low - 2) & GENMASK(3, 0); > > + data = (scl_high - 1) << 20 | scl_high << 16 | scl_low << 12 | baseclk_idx; > > + if (i2c_bus->timeout) { > > + data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK); > > + data |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout); > > + } > > + > > + return data; > > +} > > ... > > > +static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) { > > + u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > + int ret = 0; > > + u32 ctrl; > > + int r; > > + > > + dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", > > +i2c_bus->adap.nr, state); > > + > > + ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > + > > + /* Disable master/slave mode */ > > + writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN), > > + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > + > > + /* Enable master mode only */ > > + writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | > AST2600_I2CC_MASTER_EN, > > + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > + > > + reinit_completion(&i2c_bus->cmd_complete); > > + i2c_bus->cmd_err = 0; > > + > > + /* Check 0x14's SDA and SCL status */ > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > AST2600_I2CC_SCL_LINE_STS)) { > > + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + > AST2600_I2CM_CMD_STS); > > + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, > i2c_bus->adap.timeout); > > + if (r == 0) { > > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > + ret = -ETIMEDOUT; > > + } else { > > + if (i2c_bus->cmd_err) { > > + dev_dbg(i2c_bus->dev, "recovery error\n"); > > + ret = -EPROTO; > > + } > > + } > > + } > > ret is set but maybe overridden. > If will modify by following. if (r == 0) { dev_dbg(i2c_bus->dev, "recovery timed out\n"); ret = -ETIMEDOUT; } else if (i2c_bus->cmd_err) { dev_dbg(i2c_bus->dev, "recovery error\n"); ret = -EPROTO; } If no error keep ret = 0; > > + /* Recovery done */ > > Even if it fails above? This will keep check the bus status, if bus busy, will give ret = -EPROTO; > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > + ret = -EPROTO; > > + } > > + > > + /* restore original master/slave setting */ > > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > + return ret; > > +} > > ... > > > +static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus > > +*i2c_bus) { > > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > > + int xfer_len; > > + > > + cmd |= AST2600_I2CM_PKT_EN; > > + xfer_len = msg->len - i2c_bus->master_xfer_cnt; > > + if (xfer_len > AST2600_I2C_DMA_SIZE) { > > + xfer_len = AST2600_I2C_DMA_SIZE; > > > + } else { > > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) > > else if (...) Will update to else if(...). > > > + cmd |= AST2600_I2CM_STOP_CMD; > > + } > > + > > + if (cmd & AST2600_I2CM_START_CMD) { > > + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); > > + i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); > > + if (!i2c_bus->master_safe_buf) > > + return -ENOMEM; > > + i2c_bus->master_dma_addr = > > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, > > + msg->len, DMA_TO_DEVICE); > > > + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) > { > > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, > false); > > + i2c_bus->master_safe_buf = NULL; > > > + return -ENOMEM; > > Why is the dma_mapping_error() returned error code shadowed? Sorry, please point me why you are think it is shadowed? As I know dma_mapping_error() will return 0 or -ENOMEM. So I check if it is !=0. Than return -ENOMEM. > > > + } > > + } > > + > > + if (xfer_len) { > > + cmd |= AST2600_I2CM_TX_DMA_EN | AST2600_I2CM_TX_CMD; > > + writel(AST2600_I2CM_SET_TX_DMA_LEN(xfer_len - 1), > > + i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); > > + writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt, > > + i2c_bus->reg_base + AST2600_I2CM_TX_DMA); > > + } > > + > > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > > + > > + return 0; > > +} > > + > > +static int ast2600_i2c_setup_buff_tx(u32 cmd, struct ast2600_i2c_bus > > +*i2c_bus) { > > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > > + u32 wbuf_dword; > > + int xfer_len; > > + u8 wbuf[4]; > > > + int i; > > Why signed? Because it is for xfer len plus, also align with xfer_len and master_xfer_cnt in struct i2c_bus struct. > > > + cmd |= AST2600_I2CM_PKT_EN; > > + xfer_len = msg->len - i2c_bus->master_xfer_cnt; > > + if (xfer_len > i2c_bus->buf_size) { > > + xfer_len = i2c_bus->buf_size; > > > + } else { > > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) > > else if (...) Will update else if (...). > > > + cmd |= AST2600_I2CM_STOP_CMD; > > + } > > + > > + if (cmd & AST2600_I2CM_START_CMD) > > + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); > > + > > + if (xfer_len) { > > + cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD; > > > + /* > > + * The controller's buffer register supports dword writes only. > > + * Therefore, write dwords to the buffer register in a 4-byte aligned, > > + * and write the remaining unaligned data at the end. > > + */ > > + for (i = 0; i < xfer_len; i++) { > > + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; > > + if ((i % 4) == 3 || i == xfer_len - 1) { > > + wbuf_dword = get_unaligned_le32(wbuf); > > + writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); > > + } > > + } > > This is overcomplicated and can be simplified. > Why you can't perform > > get_unaligned_leXX(msg->buf[i2c_bus->master_xfer_cnt + i]); > > ? > > for (i = 0; i < xfer_len; i += 4) { > switch (min(xfer_len - i, 4) % 4) { > case 1: > wbuf_dword = ...; > writel(wbuf_dword, i2c_bus->buf_base + i); > break; > case 2: > wbuf_dword = get_unaligned_le16(...); > writel(wbuf_dword, i2c_bus->buf_base + i); > break; > case 3: > wbuf_dword = get_unaligned_le24(...); > writel(wbuf_dword, i2c_bus->buf_base + i); > break; > default: > wbuf_dword = get_unaligned_le32(...); > writel(wbuf_dword, i2c_bus->buf_base + i); > break; > } > } > > > Now, with this it's can be a helper, with which > > for (i = 0; i < xfer_len; i += 4) { > switch (min(xfer_len - i, 4) % 4) { > case 1: > ast2600_write_data(i2c_bus, i, ...); > break; > case 2: > ast2600_write_data(i2c_bus, i, get_unaligned_le16(...)); > break; > case 3: > ast2600_write_data(i2c_bus, i, get_unaligned_le24(...)); > break; > default: > ast2600_write_data(i2c_bus, i, get_unaligned_le32(...)); > break; > } > } > OK, I will modify by this. for (i = 0; i < xfer_len; i += 4) { switch (min(xfer_len - i, 4) % 4) { case 1: ast2600_write_data(i2c_bus, i, ...); break; case 2: ast2600_write_data(i2c_bus, i, get_unaligned_le16(...)); break; case 3: ast2600_write_data(i2c_bus, i, get_unaligned_le24(...)); break; default: ast2600_write_data(i2c_bus, i, get_unaligned_le32(...)); break; } } > > + writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len), > > + i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); > > + } > > + > > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > > + > > + return 0; > > +} > > ... > > > +static int ast2600_i2c_setup_dma_rx(struct ast2600_i2c_bus *i2c_bus) > > +{ > > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > > + int xfer_len; > > + u32 cmd; > > + > > + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) > | > > + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_DMA_EN; > > + > > + if (msg->flags & I2C_M_RECV_LEN) { > > + xfer_len = 1; > > > + } else { > > + if (msg->len > AST2600_I2C_DMA_SIZE) { > > } else if (...) { > Will update to else if (...) { > > + xfer_len = AST2600_I2C_DMA_SIZE; > > + } else { > > + xfer_len = msg->len; > > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) > > + cmd |= MASTER_TRIGGER_LAST_STOP; > > + } > > + } > > + writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1), > i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); > > + i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); > > + if (!i2c_bus->master_safe_buf) > > + return -ENOMEM; > > + i2c_bus->master_dma_addr = > > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, msg->len, > DMA_FROM_DEVICE); > > + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) { > > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); > > + i2c_bus->master_safe_buf = NULL; > > + return -ENOMEM; > > + } > > + writel(i2c_bus->master_dma_addr, i2c_bus->reg_base + > > +AST2600_I2CM_RX_DMA); > > + > > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > > + > > + return 0; > > +} > > + > > +static int ast2600_i2c_setup_buff_rx(struct ast2600_i2c_bus *i2c_bus) > > +{ > > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > > + int xfer_len; > > + u32 cmd; > > + > > + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) > | > > + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_BUFF_EN; > > + > > + if (msg->flags & I2C_M_RECV_LEN) { > > + dev_dbg(i2c_bus->dev, "smbus read\n"); > > + xfer_len = 1; > > > + } else { > > + if (msg->len > i2c_bus->buf_size) { > > } else if (...) { Will update to else if (...) { > > > + xfer_len = i2c_bus->buf_size; > > + } else { > > + xfer_len = msg->len; > > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) > > + cmd |= MASTER_TRIGGER_LAST_STOP; > > + } > > + } > > + writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len), i2c_bus->reg_base + > > +AST2600_I2CC_BUFF_CTRL); > > + > > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > > + > > + return 0; > > +} > > + > > +static int ast2600_i2c_setup_byte_rx(struct ast2600_i2c_bus *i2c_bus) > > +{ > > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > > + u32 cmd; > > + > > + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) > | > > + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_CMD; > > + > > + if (msg->flags & I2C_M_RECV_LEN) { > > + dev_dbg(i2c_bus->dev, "smbus read\n"); > > > + } else { > > + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) { > > } else if (...) { > Will update to else if (...) { > > + if (msg->len == 1) > > + cmd |= MASTER_TRIGGER_LAST_STOP; > > + } > > + } > > + > > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > > + > > + return 0; > > +} > > ... > > > +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) { > > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; > > + > > + /* send start */ > > + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", > > Drop 'ing', no need to have this in the debug message. Will Drop 'ing' > > > + i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), > > + msg->len, msg->len > 1 ? "s" : "", > > str_plural() Will replace by following. dev_dbg(i2c_bus->dev, "[%d] %s %d byte%s %s 0x%02x\n", i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), msg->len, str_plural(msg->len), > > > + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); > > > + i2c_bus->master_xfer_cnt = 0; > > + i2c_bus->buf_index = 0; > > + > > + if (msg->flags & I2C_M_RD) { > > + if (i2c_bus->mode == DMA_MODE) > > + return ast2600_i2c_setup_dma_rx(i2c_bus); > > + else if (i2c_bus->mode == BUFF_MODE) > > + return ast2600_i2c_setup_buff_rx(i2c_bus); > > + else > > + return ast2600_i2c_setup_byte_rx(i2c_bus); > > + } else { > > + if (i2c_bus->mode == DMA_MODE) > > + return > ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); > > + else if (i2c_bus->mode == BUFF_MODE) > > + return ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, > i2c_bus); > > + else > > + return > ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); > > + } > > +} > > ... > > > +master_out: > > + if (i2c_bus->mode == DMA_MODE) { > > + kfree(i2c_bus->master_safe_buf); > > + i2c_bus->master_safe_buf = NULL; > > + } > > Indentation issues. Will update > > > + return ret; > > ... > > > > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); > > Why do you need this table before _probe()? Isn't the only user is below? It is for next generation table list. Do you suggest remove it? > > > +static int ast2600_i2c_probe(struct platform_device *pdev) > > ... > > > + i2c_bus->global_regs = > syscon_regmap_lookup_by_phandle(dev->of_node, > > +"aspeed,global-regs"); > > dev_of_node(dev) Will update i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(dev_of_node(dev), "aspeed,global-regs"); > > > + if (IS_ERR(i2c_bus->global_regs)) > > + return PTR_ERR(i2c_bus->global_regs); > > ... > > > + if (device_property_read_bool(&pdev->dev, "aspeed,enable-dma")) > > You have 'dev' Why not use it? Will update if (device_property_read_bool(dev, "aspeed,enable-dma")) > > > + i2c_bus->mode = DMA_MODE; > > ... > > > + if (i2c_bus->mode == BUFF_MODE) { > > + i2c_bus->buf_base = > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > + i2c_bus->buf_size = resource_size(res) / 2; > > + else > > + i2c_bus->mode = BYTE_MODE; > > What's wrong with positive conditional? And is it even possible to have NULL > here? > Yes, if dtsi fill not following yaml example have reg 1, that will failure at buffer mode. And I can swith to byte mode. reg = <0x80 0x80>, <0xc00 0x20>; > > + } > > ... > > > + strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name)); > > Use 2-argument strscpy(). Do you mean strscpy(i2c_bus->adap.name, pdev->name); is acceptable? > > ... > > > + i2c_bus->alert_enable = device_property_read_bool(dev, "smbus-alert"); > > + if (i2c_bus->alert_enable) { > > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, > &i2c_bus->alert_data); > > + if (!i2c_bus->ara) > > + dev_warn(dev, "Failed to register ARA client\n"); > > + > > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER > | AST2600_I2CM_SMBUS_ALT, > > + i2c_bus->reg_base + AST2600_I2CM_IER); > > + } else { > > + i2c_bus->alert_enable = false; > > + /* Set interrupt generation of I2C master controller */ > > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, > > + i2c_bus->reg_base + AST2600_I2CM_IER); > > + } > > I2C core calls i2c_setup_smbus_alert() when registering the adapter. Why do > you need to have something special here? The ast2600 i2c support smbus alert, and according my reference. If enable alert, that will need i2c_new_smbus_alert_device for alert handler. When interrupt coming driver can use this hander to up use i2c_handle_smbus_alert And update layer will handle alert. Does I mis-understand. If yes, I will remove this in next. > > -- > With Best Regards, > Andy Shevchenko >
On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: ... > > > + /* Check 0x14's SDA and SCL status */ > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > > AST2600_I2CC_SCL_LINE_STS)) { > > > + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + > > AST2600_I2CM_CMD_STS); > > > + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, > > i2c_bus->adap.timeout); > > > + if (r == 0) { > > > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > + ret = -ETIMEDOUT; > > > + } else { > > > + if (i2c_bus->cmd_err) { > > > + dev_dbg(i2c_bus->dev, "recovery error\n"); > > > + ret = -EPROTO; > > > + } > > > + } > > > + } > > > > ret is set but maybe overridden. > > If will modify by following. > if (r == 0) { > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > ret = -ETIMEDOUT; > } else if (i2c_bus->cmd_err) { > dev_dbg(i2c_bus->dev, "recovery error\n"); > ret = -EPROTO; > } > If no error keep ret = 0; It doesn't change the behaviour. Still ret can be overridden below... > > > + /* Recovery done */ > > > > Even if it fails above? > > This will keep check the bus status, if bus busy, will give ret = -EPROTO; > > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > > > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > > + ret = -EPROTO; ...here. > > > + } > > > + > > > + /* restore original master/slave setting */ > > > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > > + return ret; ... > > > + i2c_bus->master_dma_addr = > > > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, > > > + msg->len, DMA_TO_DEVICE); > > > > > + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) > > { > > > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, > > false); > > > + i2c_bus->master_safe_buf = NULL; > > > > > + return -ENOMEM; > > > > Why is the dma_mapping_error() returned error code shadowed? > > Sorry, please point me why you are think it is shadowed? > As I know dma_mapping_error() will return 0 or -ENOMEM. So I check if it is !=0. > Than return -ENOMEM. First of all, it is a bad style to rely on the implementation details where it's not crucial. Second, today it may return only ENOMEM, tomorrow it can return a different code or codes. And in general, one should not shadow an error code without justification. > > > + } ... > > > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); > > > > Why do you need this table before _probe()? Isn't the only user is below? > > It is for next generation table list. Do you suggest remove it? My question was regarding to the location of this table in the code, that's it, no other implications. ... > > > + if (i2c_bus->mode == BUFF_MODE) { > > > + i2c_bus->buf_base = > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > + else > > > + i2c_bus->mode = BYTE_MODE; > > > > What's wrong with positive conditional? And is it even possible to have NULL > > here? > > > Yes, if dtsi fill not following yaml example have reg 1, that will failure at buffer mode. > And I can swith to byte mode. > > reg = <0x80 0x80>, <0xc00 0x20>; I was asking about if (!IS_ERR_OR_NULL(...)) line: 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? 2) Why _NULL? > > > + } ... > > > + strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name)); > > > > Use 2-argument strscpy(). > Do you mean strscpy(i2c_bus->adap.name, pdev->name); is acceptable? Yes. And not only acceptable but robust for the copying to the [string] arrays. ... > > > + i2c_bus->alert_enable = device_property_read_bool(dev, "smbus-alert"); > > > + if (i2c_bus->alert_enable) { > > > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, > > &i2c_bus->alert_data); > > > + if (!i2c_bus->ara) > > > + dev_warn(dev, "Failed to register ARA client\n"); > > > + > > > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER > > | AST2600_I2CM_SMBUS_ALT, > > > + i2c_bus->reg_base + AST2600_I2CM_IER); > > > + } else { > > > + i2c_bus->alert_enable = false; > > > + /* Set interrupt generation of I2C master controller */ > > > + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, > > > + i2c_bus->reg_base + AST2600_I2CM_IER); > > > + } > > > > I2C core calls i2c_setup_smbus_alert() when registering the adapter. Why do > > you need to have something special here? > The ast2600 i2c support smbus alert, and according my reference. > If enable alert, that will need i2c_new_smbus_alert_device for alert handler. > When interrupt coming driver can use this hander to up use i2c_handle_smbus_alert > And update layer will handle alert. > Does I mis-understand. If yes, I will remove this in next. Have you seen i2c_new_smbus_alert_device() ?
> Subject: Re: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register > mode driver > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > > ... > > > > > + /* Check 0x14's SDA and SCL status */ > > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > > > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > > > AST2600_I2CC_SCL_LINE_STS)) { > > > > + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base > + > > > AST2600_I2CM_CMD_STS); > > > > + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, > > > i2c_bus->adap.timeout); > > > > + if (r == 0) { > > > > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > > + ret = -ETIMEDOUT; > > > > + } else { > > > > + if (i2c_bus->cmd_err) { > > > > + dev_dbg(i2c_bus->dev, "recovery error\n"); > > > > + ret = -EPROTO; > > > > + } > > > > + } > > > > + } > > > > > > ret is set but maybe overridden. > > > > If will modify by following. > > if (r == 0) { > > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > ret = -ETIMEDOUT; > > } else if (i2c_bus->cmd_err) { > > dev_dbg(i2c_bus->dev, "recovery error\n"); > > ret = -EPROTO; > > } > > If no error keep ret = 0; > > It doesn't change the behaviour. Still ret can be overridden below... Yes, it is expectable, previous is issue recovery command out then the following is double confirm the bus status. If bus still busy, the function still return recovery fail. Or should I modify by following? /* Check 0x14's SDA and SCL status */ state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); if (r == 0) { dev_dbg(i2c_bus->dev, "recovery timed out\n"); ret = -ETIMEDOUT; } else if (i2c_bus->cmd_err) { dev_dbg(i2c_bus->dev, "recovery error\n"); ret = -EPROTO; } /* check bus status */ state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); if (state & AST2600_I2CC_BUS_BUSY_STS) { dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); ret = -EPROTO; } } > > > > > + /* Recovery done */ > > > > > > Even if it fails above? > > > > This will keep check the bus status, if bus busy, will give ret = > > -EPROTO; > > > > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > > > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > > > > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > > > + ret = -EPROTO; > > ...here. > > > > > + } > > > > + > > > > + /* restore original master/slave setting */ > > > > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > > > + return ret; > > ... > > > > > > + i2c_bus->master_dma_addr = > > > > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, > > > > + msg->len, DMA_TO_DEVICE); > > > > > > > + if (dma_mapping_error(i2c_bus->dev, > i2c_bus->master_dma_addr)) > > > { > > > > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, > msg, > > > false); > > > > + i2c_bus->master_safe_buf = NULL; > > > > > > > + return -ENOMEM; > > > > > > Why is the dma_mapping_error() returned error code shadowed? > > > > Sorry, please point me why you are think it is shadowed? > > As I know dma_mapping_error() will return 0 or -ENOMEM. So I check if it > is !=0. > > Than return -ENOMEM. > > First of all, it is a bad style to rely on the implementation details where it's not > crucial. Second, today it may return only ENOMEM, tomorrow it can return a > different code or codes. And in general, one should not shadow an error code > without justification. > Understood, The following is better, am I right? (if yest, those will update in driver) Int ret; ..... ret = dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr) if (ret) { i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); i2c_bus->master_safe_buf = NULL; return ret; } > > > > + } > > ... > > > > > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); > > > > > > Why do you need this table before _probe()? Isn't the only user is below? > > > > It is for next generation table list. Do you suggest remove it? > > My question was regarding to the location of this table in the code, that's it, no > other implications. > I will move before platform_driver ast2600_i2c_bus_driver, like following. static const struct of_device_id ast2600_i2c_bus_of_table[] = { { .compatible = "aspeed,ast2600-i2cv2", }, {} }; MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); static struct platform_driver ast2600_i2c_bus_driver = { ....... } > ... > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > + i2c_bus->buf_base = > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > + else > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > What's wrong with positive conditional? And is it even possible to > > > have NULL here? > > > > > Yes, if dtsi fill not following yaml example have reg 1, that will failure at buffer > mode. > > And I can swith to byte mode. > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? I will update to following. if (IS_ERR(i2c_bus->buf_base)) i2c_bus->mode = BYTE_MODE; else i2c_bus->buf_size = resource_size(res) / 2; > 2) Why _NULL? If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that will goto byte mode. reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. due to 2nd is buffer register offset. > > > > > + } > > ... > > > > > + strscpy(i2c_bus->adap.name, pdev->name, > > > > +sizeof(i2c_bus->adap.name)); > > > > > > Use 2-argument strscpy(). > > Do you mean strscpy(i2c_bus->adap.name, pdev->name); is acceptable? > > Yes. And not only acceptable but robust for the copying to the [string] arrays. Got it. > > ... > > > > > + i2c_bus->alert_enable = device_property_read_bool(dev, > "smbus-alert"); > > > > + if (i2c_bus->alert_enable) { > > > > + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, > > > &i2c_bus->alert_data); > > > > + if (!i2c_bus->ara) > > > > + dev_warn(dev, "Failed to register ARA client\n"); > > > > + > > > > + writel(AST2600_I2CM_PKT_DONE | > AST2600_I2CM_BUS_RECOVER > > > | AST2600_I2CM_SMBUS_ALT, > > > > + i2c_bus->reg_base + AST2600_I2CM_IER); > > > > + } else { > > > > + i2c_bus->alert_enable = false; > > > > + /* Set interrupt generation of I2C master controller */ > > > > + writel(AST2600_I2CM_PKT_DONE | > AST2600_I2CM_BUS_RECOVER, > > > > + i2c_bus->reg_base + AST2600_I2CM_IER); > > > > + } > > > > > > I2C core calls i2c_setup_smbus_alert() when registering the adapter. > > > Why do you need to have something special here? > > The ast2600 i2c support smbus alert, and according my reference. > > If enable alert, that will need i2c_new_smbus_alert_device for alert handler. > > When interrupt coming driver can use this hander to up use > > i2c_handle_smbus_alert And update layer will handle alert. > > Does I mis-understand. If yes, I will remove this in next. > > Have you seen i2c_new_smbus_alert_device() ? No, I think I will remove it, when if it is more clear. Thanks a lot. > > -- > With Best Regards, > Andy Shevchenko >
On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: ... > > > > > + /* Check 0x14's SDA and SCL status */ > > > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > > > > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > > > > AST2600_I2CC_SCL_LINE_STS)) { > > > > > + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base > > + > > > > AST2600_I2CM_CMD_STS); > > > > > + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, > > > > i2c_bus->adap.timeout); > > > > > + if (r == 0) { > > > > > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > > > + ret = -ETIMEDOUT; > > > > > + } else { > > > > > + if (i2c_bus->cmd_err) { > > > > > + dev_dbg(i2c_bus->dev, "recovery error\n"); > > > > > + ret = -EPROTO; > > > > > + } > > > > > + } > > > > > + } > > > > > > > > ret is set but maybe overridden. > > > > > > If will modify by following. > > > if (r == 0) { > > > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > ret = -ETIMEDOUT; > > > } else if (i2c_bus->cmd_err) { > > > dev_dbg(i2c_bus->dev, "recovery error\n"); > > > ret = -EPROTO; > > > } > > > If no error keep ret = 0; > > > > It doesn't change the behaviour. Still ret can be overridden below... > > Yes, it is expectable, previous is issue recovery command out then the > following is double confirm the bus status. > If bus still busy, the function still return recovery fail. > > Or should I modify by following? > /* Check 0x14's SDA and SCL status */ > state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { > writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); > r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); > if (r == 0) { > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > ret = -ETIMEDOUT; This assignment doesn't make sense. > } else if (i2c_bus->cmd_err) { > dev_dbg(i2c_bus->dev, "recovery error\n"); > ret = -EPROTO; > } > /* check bus status */ > state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > if (state & AST2600_I2CC_BUS_BUSY_STS) { > dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > ret = -EPROTO; > } > } > > > > > + /* Recovery done */ > > > > > > > > Even if it fails above? > > > > > > This will keep check the bus status, if bus busy, will give ret = > > > -EPROTO; > > > > > > > > + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > > > > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > > > > > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > > > > + ret = -EPROTO; > > > > ...here. See above. > > > > > + } > > > > > + > > > > > + /* restore original master/slave setting */ > > > > > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > > > > + return ret; ... > > > > > + i2c_bus->master_dma_addr = > > > > > + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, > > > > > + msg->len, DMA_TO_DEVICE); > > > > > > > > > + if (dma_mapping_error(i2c_bus->dev, > > i2c_bus->master_dma_addr)) > > > > { > > > > > + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, > > msg, > > > > false); > > > > > + i2c_bus->master_safe_buf = NULL; > > > > > > > > > + return -ENOMEM; > > > > > > > > Why is the dma_mapping_error() returned error code shadowed? > > > > > > Sorry, please point me why you are think it is shadowed? > > > As I know dma_mapping_error() will return 0 or -ENOMEM. So I check if it > > is !=0. > > > Than return -ENOMEM. > > > > First of all, it is a bad style to rely on the implementation details where it's not > > crucial. Second, today it may return only ENOMEM, tomorrow it can return a > > different code or codes. And in general, one should not shadow an error code > > without justification. > > > Understood, The following is better, am I right? (if yest, those will update in driver) Yes. > Int ret; > ..... > ret = dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr) > if (ret) { > i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); > i2c_bus->master_safe_buf = NULL; > return ret; > } > > > > > > + } ... > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > + i2c_bus->buf_base = > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > + else > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > What's wrong with positive conditional? And is it even possible to > > > > have NULL here? > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, that will failure at buffer > > mode. > > > And I can swith to byte mode. > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > I will update to following. > if (IS_ERR(i2c_bus->buf_base)) > i2c_bus->mode = BYTE_MODE; > else > i2c_bus->buf_size = resource_size(res) / 2; > > > 2) Why _NULL? > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that will goto byte mode. > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > due to 2nd is buffer register offset. I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > > > > > + }
> Subject: Re: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register > mode driver > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > > ... > > > > > > > + /* Check 0x14's SDA and SCL status */ > > > > > > + state = readl(i2c_bus->reg_base + > AST2600_I2CC_STS_AND_BUFF); > > > > > > + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > > > > > AST2600_I2CC_SCL_LINE_STS)) { > > > > > > + writel(AST2600_I2CM_RECOVER_CMD_EN, > i2c_bus->reg_base > > > + > > > > > AST2600_I2CM_CMD_STS); > > > > > > + r = > wait_for_completion_timeout(&i2c_bus->cmd_complete, > > > > > i2c_bus->adap.timeout); > > > > > > + if (r == 0) { > > > > > > + dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > > > > + ret = -ETIMEDOUT; > > > > > > + } else { > > > > > > + if (i2c_bus->cmd_err) { > > > > > > + dev_dbg(i2c_bus->dev, "recovery error\n"); > > > > > > + ret = -EPROTO; > > > > > > + } > > > > > > + } > > > > > > + } > > > > > > > > > > ret is set but maybe overridden. > > > > > > > > If will modify by following. > > > > if (r == 0) { > > > > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > > ret = -ETIMEDOUT; > > > > } else if (i2c_bus->cmd_err) { > > > > dev_dbg(i2c_bus->dev, "recovery error\n"); > > > > ret = -EPROTO; > > > > } > > > > If no error keep ret = 0; > > > > > > It doesn't change the behaviour. Still ret can be overridden below... > > > > Yes, it is expectable, previous is issue recovery command out then the > > following is double confirm the bus status. > > If bus still busy, the function still return recovery fail. > > > > Or should I modify by following? > > /* Check 0x14's SDA and SCL status */ > > state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & > AST2600_I2CC_SCL_LINE_STS)) { > > writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + > AST2600_I2CM_CMD_STS); > > r = wait_for_completion_timeout(&i2c_bus->cmd_complete, > i2c_bus->adap.timeout); > > if (r == 0) { > > dev_dbg(i2c_bus->dev, "recovery timed out\n"); > > > ret = -ETIMEDOUT; > > This assignment doesn't make sense. > > > } else if (i2c_bus->cmd_err) { > > dev_dbg(i2c_bus->dev, "recovery error\n"); > > ret = -EPROTO; > > } > > /* check bus status */ > > state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); > > if (state & AST2600_I2CC_BUS_BUSY_STS) { > > dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > ret = -EPROTO; > > } > > } > > > > > > > + /* Recovery done */ > > > > > > > > > > Even if it fails above? > > > > > > > > This will keep check the bus status, if bus busy, will give ret = > > > > -EPROTO; > > > > > > > > > > + state = readl(i2c_bus->reg_base + > AST2600_I2CC_STS_AND_BUFF); > > > > > > + if (state & AST2600_I2CC_BUS_BUSY_STS) { > > > > > > + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); > > > > > > + ret = -EPROTO; > > > > > > ...here. > > See above. OH, I understand now. I will modify following if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); if (r == 0) { dev_dbg(i2c_bus->dev, "recovery timed out\n"); ++ writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); ++ return -ETIMEDOUT; } else { if (i2c_bus->cmd_err) { dev_dbg(i2c_bus->dev, "recovery error\n"); ret = -EPROTO; } } > > > > > > + } > > > > > > + > > > > > > + /* restore original master/slave setting */ > > > > > > + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); > > > > > > + return ret; > > ... > > > > > > > + i2c_bus->master_dma_addr = > > > > > > + dma_map_single(i2c_bus->dev, > i2c_bus->master_safe_buf, > > > > > > + msg->len, DMA_TO_DEVICE); > > > > > > > > > > > + if (dma_mapping_error(i2c_bus->dev, > > > i2c_bus->master_dma_addr)) > > > > > { > > > > > > + > i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, > > > msg, > > > > > false); > > > > > > + i2c_bus->master_safe_buf = NULL; > > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > Why is the dma_mapping_error() returned error code shadowed? > > > > > > > > Sorry, please point me why you are think it is shadowed? > > > > As I know dma_mapping_error() will return 0 or -ENOMEM. So I check > > > > if it > > > is !=0. > > > > Than return -ENOMEM. > > > > > > First of all, it is a bad style to rely on the implementation > > > details where it's not crucial. Second, today it may return only > > > ENOMEM, tomorrow it can return a different code or codes. And in > > > general, one should not shadow an error code without justification. > > > > > Understood, The following is better, am I right? (if yest, those will > > update in driver) > > Yes. Thanks. > > > Int ret; > > ..... > > ret = dma_mapping_error(i2c_bus->dev, > i2c_bus->master_dma_addr) > > if (ret) { > > i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, > false); > > i2c_bus->master_safe_buf = NULL; > > return ret; > > } > > > > > > > > + } > > ... > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > + i2c_bus->buf_base = > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > + else > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > What's wrong with positive conditional? And is it even possible > > > > > to have NULL here? > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, that will > > > > failure at buffer > > > mode. > > > > And I can swith to byte mode. > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > I will update to following. > > if (IS_ERR(i2c_bus->buf_base)) > > i2c_bus->mode = BYTE_MODE; > > else > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > 2) Why _NULL? > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that will goto byte > mode. > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > due to 2nd is buffer register offset. > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > OH, I will doing by this. if (IS_ERR_OR_NULL(i2c_bus->buf_base)) i2c_bus->mode = BYTE_MODE; else i2c_bus->buf_size = resource_size(res) / 2; > > -- > With Best Regards, > Andy Shevchenko >
On Fri, Aug 23, 2024 at 06:23:54AM +0000, Ryan Chen wrote: > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: ... > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > > + i2c_bus->buf_base = > > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > + else > > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > > > What's wrong with positive conditional? And is it even possible > > > > > > to have NULL here? > > > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, that will > > > > > failure at buffer > > > > mode. > > > > > And I can swith to byte mode. > > > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > > I will update to following. > > > if (IS_ERR(i2c_bus->buf_base)) > > > i2c_bus->mode = BYTE_MODE; > > > else > > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > 2) Why _NULL? > > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that will goto byte > > mode. > > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > > due to 2nd is buffer register offset. > > > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > > > OH, I will doing by this. > if (IS_ERR_OR_NULL(i2c_bus->buf_base)) The question about _NULL remains unanswered... > i2c_bus->mode = BYTE_MODE; > else > i2c_bus->buf_size = resource_size(res) / 2;
> Subject: Re: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register > mode driver > > On Fri, Aug 23, 2024 at 06:23:54AM +0000, Ryan Chen wrote: > > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > > ... > > > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > > > + i2c_bus->buf_base = > > > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > + else > > > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > > > > > What's wrong with positive conditional? And is it even > > > > > > > possible to have NULL here? > > > > > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, that > > > > > > will failure at buffer > > > > > mode. > > > > > > And I can swith to byte mode. > > > > > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > > > I will update to following. > > > > if (IS_ERR(i2c_bus->buf_base)) > > > > i2c_bus->mode = BYTE_MODE; > > > > else > > > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > 2) Why _NULL? > > > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that > > > > will goto byte > > > mode. > > > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > > > due to 2nd is buffer register offset. > > > > > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > > > > > OH, I will doing by this. > > if (IS_ERR_OR_NULL(i2c_bus->buf_base)) > > The question about _NULL remains unanswered... Sorry, I may not catch your point. So, Do you mean I should passive coding by following? If (i2c_bus->buf_base > 0) i2c_bus->buf_size = resource_size(res) / 2; else i2c_bus->mode = BYTE_MODE; > > > i2c_bus->mode = BYTE_MODE; > > else > > i2c_bus->buf_size = resource_size(res) / 2; > > -- > With Best Regards, > Andy Shevchenko >
On Mon, Aug 26, 2024 at 07:50:24AM +0000, Ryan Chen wrote: > > On Fri, Aug 23, 2024 at 06:23:54AM +0000, Ryan Chen wrote: > > > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: ... > > > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > > > > + i2c_bus->buf_base = > > > > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > + else > > > > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > > > > > > > What's wrong with positive conditional? And is it even > > > > > > > > possible to have NULL here? > > > > > > > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, that > > > > > > > will failure at buffer > > > > > > mode. > > > > > > > And I can swith to byte mode. > > > > > > > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > > > > I will update to following. > > > > > if (IS_ERR(i2c_bus->buf_base)) > > > > > i2c_bus->mode = BYTE_MODE; > > > > > else > > > > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > > > 2) Why _NULL? > > > > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; that > > > > > will goto byte > > > > mode. > > > > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > > > > due to 2nd is buffer register offset. > > > > > > > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > > > > > > > OH, I will doing by this. > > > if (IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > The question about _NULL remains unanswered... > Sorry, I may not catch your point. > So, Do you mean I should passive coding by following? No. I already mentioned that in one of the previous mails. Why do you use IS_ERR_OR_NULL() and not IS_ERR()? You should understand your code better than me :-) > If (i2c_bus->buf_base > 0) > i2c_bus->buf_size = resource_size(res) / 2; > else > i2c_bus->mode = BYTE_MODE; > > > > i2c_bus->mode = BYTE_MODE; > > > else > > > i2c_bus->buf_size = resource_size(res) / 2;
> Subject: Re: [PATCH v13 2/3] i2c: aspeed: support AST2600 i2c new register > mode driver > > On Mon, Aug 26, 2024 at 07:50:24AM +0000, Ryan Chen wrote: > > > On Fri, Aug 23, 2024 at 06:23:54AM +0000, Ryan Chen wrote: > > > > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > > > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: > > ... > > > > > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > > > > > + i2c_bus->buf_base = > > > > > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > > + else > > > > > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > > > > > > > > > What's wrong with positive conditional? And is it even > > > > > > > > > possible to have NULL here? > > > > > > > > > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, > > > > > > > > that will failure at buffer > > > > > > > mode. > > > > > > > > And I can swith to byte mode. > > > > > > > > > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > > > > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > > > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > > > > > I will update to following. > > > > > > if (IS_ERR(i2c_bus->buf_base)) > > > > > > i2c_bus->mode = BYTE_MODE; > > > > > > else > > > > > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > > > > > 2) Why _NULL? > > > > > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; > > > > > > that will goto byte > > > > > mode. > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > > > > > due to 2nd is buffer register offset. > > > > > > > > > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > > > > > > > > > OH, I will doing by this. > > > > if (IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > The question about _NULL remains unanswered... > > Sorry, I may not catch your point. > > So, Do you mean I should passive coding by following? > > No. I already mentioned that in one of the previous mails. > Why do you use IS_ERR_OR_NULL() and not IS_ERR()? > > You should understand your code better than me :-) Understood, I will change to if (IS_ERR(i2c_bus->buf_base)) i2c_bus->mode = BYTE_MODE; else i2c_bus->buf_size = resource_size(res) / 2; > > If (i2c_bus->buf_base > 0) > > i2c_bus->buf_size = resource_size(res) / 2; else > > i2c_bus->mode = BYTE_MODE; > > > > > > i2c_bus->mode = BYTE_MODE; > > > > else > > > > i2c_bus->buf_size = resource_size(res) / 2; > > -- > With Best Regards, > Andy Shevchenko >
On Wed, Aug 28, 2024 at 02:34:43AM +0000, Ryan Chen wrote: > > On Mon, Aug 26, 2024 at 07:50:24AM +0000, Ryan Chen wrote: > > > > On Fri, Aug 23, 2024 at 06:23:54AM +0000, Ryan Chen wrote: > > > > > > On Thu, Aug 22, 2024 at 02:24:26AM +0000, Ryan Chen wrote: > > > > > > > > On Wed, Aug 21, 2024 at 06:43:01AM +0000, Ryan Chen wrote: > > > > > > > > > > On Mon, Aug 19, 2024 at 05:28:49PM +0800, Ryan Chen wrote: ... > > > > > > > > > > > + if (i2c_bus->mode == BUFF_MODE) { > > > > > > > > > > > + i2c_bus->buf_base = > > > > > > > > > > devm_platform_get_and_ioremap_resource(pdev, 1, &res); > > > > > > > > > > > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > > > > > > + i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > > > + else > > > > > > > > > > > + i2c_bus->mode = BYTE_MODE; > > > > > > > > > > > > > > > > > > > > What's wrong with positive conditional? And is it even > > > > > > > > > > possible to have NULL here? > > > > > > > > > > > > > > > > > > > Yes, if dtsi fill not following yaml example have reg 1, > > > > > > > > > that will failure at buffer > > > > > > > > mode. > > > > > > > > > And I can swith to byte mode. > > > > > > > > > > > > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; > > > > > > > > > > > > > > > > I was asking about if (!IS_ERR_OR_NULL(...)) line: > > > > > > > > 1) Why 'if (!foo) {} else {}' instead of 'if (foo) {} else {}'? > > > > > > > I will update to following. > > > > > > > if (IS_ERR(i2c_bus->buf_base)) > > > > > > > i2c_bus->mode = BYTE_MODE; > > > > > > > else > > > > > > > i2c_bus->buf_size = resource_size(res) / 2; > > > > > > > > > > > > > > > 2) Why _NULL? > > > > > > > If dtsi file is claim only 1 reg offset. reg = <0x80 0x80>; > > > > > > > that will goto byte > > > > > > mode. > > > > > > > reg = <0x80 0x80>, <0xc00 0x20>; can support buffer mode. > > > > > > > due to 2nd is buffer register offset. > > > > > > > > > > > > I have asked why IS_ERR_OR_NULL() and not IS_ERR(). > > > > > > > > > > > OH, I will doing by this. > > > > > if (IS_ERR_OR_NULL(i2c_bus->buf_base)) > > > > > > > > The question about _NULL remains unanswered... > > > Sorry, I may not catch your point. > > > So, Do you mean I should passive coding by following? > > > > No. I already mentioned that in one of the previous mails. > > Why do you use IS_ERR_OR_NULL() and not IS_ERR()? > > > > You should understand your code better than me :-) > Understood, I will change to OK! > if (IS_ERR(i2c_bus->buf_base)) > i2c_bus->mode = BYTE_MODE; > else > i2c_bus->buf_size = resource_size(res) / 2; > > > > If (i2c_bus->buf_base > 0) > > > i2c_bus->buf_size = resource_size(res) / 2; else > > > i2c_bus->mode = BYTE_MODE; > > > > > > > > i2c_bus->mode = BYTE_MODE; > > > > > else > > > > > i2c_bus->buf_size = resource_size(res) / 2;
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index a22f9125322a..abfb027350d4 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -410,6 +410,17 @@ config I2C_ALTERA This driver can also be built as a module. If so, the module will be called i2c-altera. +config I2C_AST2600 + tristate "Aspeed I2C v2 Controller" + depends on ARCH_ASPEED || COMPILE_TEST + select I2C_SMBUS + help + If you say yes to this option, support will be included for the + Aspeed I2C controller with new register set. + + This driver can also be built as a module. If so, the module + will be called i2c-ast2600. + config I2C_ASPEED tristate "Aspeed I2C Controller" depends on ARCH_ASPEED || COMPILE_TEST diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 78d0561339e5..5665b60b1566 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o obj-$(CONFIG_I2C_AMD_MP2) += i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o +obj-$(CONFIG_I2C_AST2600) += i2c-ast2600.o obj-$(CONFIG_I2C_AT91) += i2c-at91.o i2c-at91-objs := i2c-at91-core.o i2c-at91-master.o ifeq ($(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL),y) diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c new file mode 100644 index 000000000000..c9b6013746a3 --- /dev/null +++ b/drivers/i2c/busses/i2c-ast2600.c @@ -0,0 +1,1057 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ASPEED AST2600 new register set I2C controller driver + * + * Copyright (C) ASPEED Technology Inc. + */ +#include <linux/bits.h> +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/delay.h> +#include <linux/dma-mapping.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/i2c-smbus.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/mfd/syscon.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/reset.h> +#include <linux/slab.h> +#include <linux/string_helpers.h> + +#define AST2600_I2CG_ISR 0x00 +#define AST2600_I2CG_SLAVE_ISR 0x04 +#define AST2600_I2CG_OWNER 0x08 +#define AST2600_I2CG_CTRL 0x0C +#define AST2600_I2CG_CLK_DIV_CTRL 0x10 + +#define AST2600_I2CG_SLAVE_PKT_NAK BIT(4) +#define AST2600_I2CG_M_S_SEPARATE_INTR BIT(3) +#define AST2600_I2CG_CTRL_NEW_REG BIT(2) +#define AST2600_I2CG_CTRL_NEW_CLK_DIV BIT(1) +#define AST2600_GLOBAL_INIT \ + (AST2600_I2CG_CTRL_NEW_REG | AST2600_I2CG_CTRL_NEW_CLK_DIV) +/* + * APB clk : 100Mhz + * div : scl : baseclk [APB/((div/2) + 1)] : tBuf [1/bclk * 16] + * I2CG10[31:24] base clk4 for i2c auto recovery timeout counter (0xC6) + * I2CG10[23:16] base clk3 for Standard-mode (100Khz) min tBuf 4.7us + * 0x3c : 100.8Khz : 3.225Mhz : 4.96us + * 0x3d : 99.2Khz : 3.174Mhz : 5.04us + * 0x3e : 97.65Khz : 3.125Mhz : 5.12us + * 0x40 : 97.75Khz : 3.03Mhz : 5.28us + * 0x41 : 99.5Khz : 2.98Mhz : 5.36us (default) + * I2CG10[15:8] base clk2 for Fast-mode (400Khz) min tBuf 1.3us + * 0x12 : 400Khz : 10Mhz : 1.6us + * I2CG10[7:0] base clk1 for Fast-mode Plus (1Mhz) min tBuf 0.5us + * 0x08 : 1Mhz : 20Mhz : 0.8us + */ +#define I2CCG_DIV_CTRL 0xC6411208 + +/* 0x00 : I2CC Master/Slave Function Control Register */ +#define AST2600_I2CC_FUN_CTRL 0x00 +#define AST2600_I2CC_SLAVE_ADDR_RX_EN BIT(20) +#define AST2600_I2CC_MASTER_RETRY_MASK GENMASK(19, 18) +#define AST2600_I2CC_MASTER_RETRY(x) (((x) & GENMASK(1, 0)) << 18) +#define AST2600_I2CC_BUS_AUTO_RELEASE BIT(17) +#define AST2600_I2CC_M_SDA_LOCK_EN BIT(16) +#define AST2600_I2CC_MULTI_MASTER_DIS BIT(15) +#define AST2600_I2CC_M_SCL_DRIVE_EN BIT(14) +#define AST2600_I2CC_MSB_STS BIT(9) +#define AST2600_I2CC_SDA_DRIVE_1T_EN BIT(8) +#define AST2600_I2CC_M_SDA_DRIVE_1T_EN BIT(7) +#define AST2600_I2CC_M_HIGH_SPEED_EN BIT(6) +/* reserver 5 : 2 */ +#define AST2600_I2CC_SLAVE_EN BIT(1) +#define AST2600_I2CC_MASTER_EN BIT(0) + +/* 0x04 : I2CC Master/Slave Clock and AC Timing Control Register #1 */ +#define AST2600_I2CC_AC_TIMING 0x04 +#define AST2600_I2CC_TTIMEOUT(x) (((x) & GENMASK(4, 0)) << 24) +#define AST2600_I2CC_TCKHIGHMIN(x) (((x) & GENMASK(3, 0)) << 20) +#define AST2600_I2CC_TCKHIGH(x) (((x) & GENMASK(3, 0)) << 16) +#define AST2600_I2CC_TCKLOW(x) (((x) & GENMASK(3, 0)) << 12) +#define AST2600_I2CC_THDDAT(x) (((x) & GENMASK(1, 0)) << 10) +#define AST2600_I2CC_TOUTBASECLK(x) (((x) & GENMASK(1, 0)) << 8) +#define AST2600_I2CC_TBASECLK(x) ((x) & GENMASK(3, 0)) + +/* 0x08 : I2CC Master/Slave Transmit/Receive Byte Buffer Register */ +#define AST2600_I2CC_STS_AND_BUFF 0x08 +#define AST2600_I2CC_TX_DIR_MASK GENMASK(31, 29) +#define AST2600_I2CC_SDA_OE BIT(28) +#define AST2600_I2CC_SDA_O BIT(27) +#define AST2600_I2CC_SCL_OE BIT(26) +#define AST2600_I2CC_SCL_O BIT(25) + +#define AST2600_I2CC_SCL_LINE_STS BIT(18) +#define AST2600_I2CC_SDA_LINE_STS BIT(17) +#define AST2600_I2CC_BUS_BUSY_STS BIT(16) + +#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) & GENMASK(7, 0)) + +/* 0x0C : I2CC Master/Slave Pool Buffer Control Register */ +#define AST2600_I2CC_BUFF_CTRL 0x0C +#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24) +#define AST2600_I2CC_SET_RX_BUF_LEN(x) (((((x) - 1) & GENMASK(4, 0)) << 16) | BIT(0)) +#define AST2600_I2CC_SET_TX_BUF_LEN(x) (((((x) - 1) & GENMASK(4, 0)) << 8) | BIT(0)) +#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1) + +/* 0x10 : I2CM Master Interrupt Control Register */ +#define AST2600_I2CM_IER 0x10 +/* 0x14 : I2CM Master Interrupt Status Register : WC */ +#define AST2600_I2CM_ISR 0x14 + +#define AST2600_I2CM_PKT_TIMEOUT BIT(18) +#define AST2600_I2CM_PKT_ERROR BIT(17) +#define AST2600_I2CM_PKT_DONE BIT(16) + +#define AST2600_I2CM_BUS_RECOVER_FAIL BIT(15) +#define AST2600_I2CM_SDA_DL_TO BIT(14) +#define AST2600_I2CM_BUS_RECOVER BIT(13) +#define AST2600_I2CM_SMBUS_ALT BIT(12) + +#define AST2600_I2CM_SCL_LOW_TO BIT(6) +#define AST2600_I2CM_ABNORMAL BIT(5) +#define AST2600_I2CM_NORMAL_STOP BIT(4) +#define AST2600_I2CM_ARBIT_LOSS BIT(3) +#define AST2600_I2CM_RX_DONE BIT(2) +#define AST2600_I2CM_TX_NAK BIT(1) +#define AST2600_I2CM_TX_ACK BIT(0) + +/* 0x18 : I2CM Master Command/Status Register */ +#define AST2600_I2CM_CMD_STS 0x18 +#define AST2600_I2CM_PKT_ADDR(x) (((x) & GENMASK(6, 0)) << 24) +#define AST2600_I2CM_PKT_EN BIT(16) +#define AST2600_I2CM_SDA_OE_OUT_DIR BIT(15) +#define AST2600_I2CM_SDA_O_OUT_DIR BIT(14) +#define AST2600_I2CM_SCL_OE_OUT_DIR BIT(13) +#define AST2600_I2CM_SCL_O_OUT_DIR BIT(12) +#define AST2600_I2CM_RECOVER_CMD_EN BIT(11) + +#define AST2600_I2CM_RX_DMA_EN BIT(9) +#define AST2600_I2CM_TX_DMA_EN BIT(8) +/* Command Bit */ +#define AST2600_I2CM_RX_BUFF_EN BIT(7) +#define AST2600_I2CM_TX_BUFF_EN BIT(6) +#define AST2600_I2CM_STOP_CMD BIT(5) +#define AST2600_I2CM_RX_CMD_LAST BIT(4) +#define AST2600_I2CM_RX_CMD BIT(3) + +#define AST2600_I2CM_TX_CMD BIT(1) +#define AST2600_I2CM_START_CMD BIT(0) + +/* 0x1C : I2CM Master DMA Transfer Length Register */ +#define AST2600_I2CM_DMA_LEN 0x1C +/* Tx Rx support length 1 ~ 4096 */ +#define AST2600_I2CM_SET_RX_DMA_LEN(x) ((((x) & GENMASK(11, 0)) << 16) | BIT(31)) +#define AST2600_I2CM_SET_TX_DMA_LEN(x) (((x) & GENMASK(11, 0)) | BIT(15)) + +/* 0x20 : I2CS Slave Interrupt Control Register */ +#define AST2600_I2CS_IER 0x20 +/* 0x24 : I2CS Slave Interrupt Status Register */ +#define AST2600_I2CS_ISR 0x24 + +#define AST2600_I2CS_ADDR_INDICATE_MASK GENMASK(31, 30) +#define AST2600_I2CS_SLAVE_PENDING BIT(29) + +#define AST2600_I2CS_WAIT_TX_DMA BIT(25) +#define AST2600_I2CS_WAIT_RX_DMA BIT(24) + +#define AST2600_I2CS_ADDR3_NAK BIT(22) +#define AST2600_I2CS_ADDR2_NAK BIT(21) +#define AST2600_I2CS_ADDR1_NAK BIT(20) + +#define AST2600_I2CS_ADDR_MASK GENMASK(19, 18) +#define AST2600_I2CS_PKT_ERROR BIT(17) +#define AST2600_I2CS_PKT_DONE BIT(16) +#define AST2600_I2CS_INACTIVE_TO BIT(15) + +#define AST2600_I2CS_SLAVE_MATCH BIT(7) +#define AST2600_I2CS_ABNOR_STOP BIT(5) +#define AST2600_I2CS_STOP BIT(4) +#define AST2600_I2CS_RX_DONE_NAK BIT(3) +#define AST2600_I2CS_RX_DONE BIT(2) +#define AST2600_I2CS_TX_NAK BIT(1) +#define AST2600_I2CS_TX_ACK BIT(0) + +/* 0x28 : I2CS Slave CMD/Status Register */ +#define AST2600_I2CS_CMD_STS 0x28 +#define AST2600_I2CS_ACTIVE_ALL GENMASK(18, 17) +#define AST2600_I2CS_PKT_MODE_EN BIT(16) +#define AST2600_I2CS_AUTO_NAK_NOADDR BIT(15) +#define AST2600_I2CS_AUTO_NAK_EN BIT(14) + +#define AST2600_I2CS_ALT_EN BIT(10) +#define AST2600_I2CS_RX_DMA_EN BIT(9) +#define AST2600_I2CS_TX_DMA_EN BIT(8) +#define AST2600_I2CS_RX_BUFF_EN BIT(7) +#define AST2600_I2CS_TX_BUFF_EN BIT(6) +#define AST2600_I2CS_RX_CMD_LAST BIT(4) + +#define AST2600_I2CS_TX_CMD BIT(2) + +#define AST2600_I2CS_DMA_LEN 0x2C +#define AST2600_I2CS_SET_RX_DMA_LEN(x) (((((x) - 1) & GENMASK(11, 0)) << 16) | BIT(31)) +#define AST2600_I2CS_SET_TX_DMA_LEN(x) ((((x) - 1) & GENMASK(11, 0)) | BIT(15)) + +/* I2CM Master DMA Tx Buffer Register */ +#define AST2600_I2CM_TX_DMA 0x30 +/* I2CM Master DMA Rx Buffer Register */ +#define AST2600_I2CM_RX_DMA 0x34 +/* I2CS Slave DMA Tx Buffer Register */ +#define AST2600_I2CS_TX_DMA 0x38 +/* I2CS Slave DMA Rx Buffer Register */ +#define AST2600_I2CS_RX_DMA 0x3C + +#define AST2600_I2CS_ADDR_CTRL 0x40 + +#define AST2600_I2CS_ADDR3_MASK GENMASK(22, 16) +#define AST2600_I2CS_ADDR2_MASK GENMASK(14, 8) +#define AST2600_I2CS_ADDR1_MASK GENMASK(6, 0) + +#define AST2600_I2CM_DMA_LEN_STS 0x48 +#define AST2600_I2CS_DMA_LEN_STS 0x4C + +#define AST2600_I2C_GET_TX_DMA_LEN(x) ((x) & GENMASK(12, 0)) +#define AST2600_I2C_GET_RX_DMA_LEN(x) (((x) & GENMASK(28, 16)) >> 16) + +/* 0x40 : Slave Device Address Register */ +#define AST2600_I2CS_ADDR3_ENABLE BIT(23) +#define AST2600_I2CS_ADDR3(x) ((x) << 16) +#define AST2600_I2CS_ADDR2_ENABLE BIT(15) +#define AST2600_I2CS_ADDR2(x) ((x) << 8) +#define AST2600_I2CS_ADDR1_ENABLE BIT(7) +#define AST2600_I2CS_ADDR1(x) (x) + +#define I2C_SLAVE_MSG_BUF_SIZE 256 + +#define AST2600_I2C_DMA_SIZE 4096 + +#define MASTER_TRIGGER_LAST_STOP (AST2600_I2CM_RX_CMD_LAST | AST2600_I2CM_STOP_CMD) +#define SLAVE_TRIGGER_CMD (AST2600_I2CS_ACTIVE_ALL | AST2600_I2CS_PKT_MODE_EN) + +#define AST_I2C_TIMEOUT_CLK 0x2 + +enum xfer_mode { + BYTE_MODE, + BUFF_MODE, + DMA_MODE, +}; + +struct ast2600_i2c_bus { + struct i2c_adapter adap; + struct device *dev; + void __iomem *reg_base; + struct regmap *global_regs; + struct reset_control *rst; + int irq; + enum xfer_mode mode; + struct clk *clk; + u32 apb_clk; + struct i2c_timings timing_info; + u32 timeout; + /* smbus alert */ + bool alert_enable; + struct i2c_smbus_alert_setup alert_data; + struct i2c_client *ara; + /* Multi-master */ + bool multi_master; + /* master structure */ + int cmd_err; + struct completion cmd_complete; + struct i2c_msg *msgs; + size_t buf_index; + /* cur xfer msgs index*/ + int msgs_index; + int msgs_count; + u8 *master_safe_buf; + dma_addr_t master_dma_addr; + /*total xfer count */ + int master_xfer_cnt; + /* Buffer mode */ + void __iomem *buf_base; + size_t buf_size; +}; + +static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus) +{ + unsigned long base_clk[16]; + int baseclk_idx; + u32 clk_div_reg; + u32 scl_low; + u32 scl_high; + int divisor; + u32 data; + + regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg); + + for (int i = 0; i < 16; i++) { + if (i == 0) + base_clk[i] = i2c_bus->apb_clk; + else if ((i > 0) || (i < 5)) + base_clk[i] = (i2c_bus->apb_clk * 2) / + (((clk_div_reg >> ((i - 1) * 8)) & GENMASK(7, 0)) + 2); + else + base_clk[i] = base_clk[4] / (1 << (i - 5)); + + if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32) { + baseclk_idx = i; + divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz); + break; + } + } + baseclk_idx = min(baseclk_idx, 15); + divisor = min(divisor, 32); + scl_low = min(divisor * 9 / 16 - 1, 15); + scl_high = (divisor - scl_low - 2) & GENMASK(3, 0); + data = (scl_high - 1) << 20 | scl_high << 16 | scl_low << 12 | baseclk_idx; + if (i2c_bus->timeout) { + data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK); + data |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout); + } + + return data; +} + +static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) +{ + u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); + int ret = 0; + u32 ctrl; + int r; + + dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr, state); + + ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + + /* Disable master/slave mode */ + writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN), + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + + /* Enable master mode only */ + writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | AST2600_I2CC_MASTER_EN, + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + + reinit_completion(&i2c_bus->cmd_complete); + i2c_bus->cmd_err = 0; + + /* Check 0x14's SDA and SCL status */ + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) { + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); + if (r == 0) { + dev_dbg(i2c_bus->dev, "recovery timed out\n"); + ret = -ETIMEDOUT; + } else { + if (i2c_bus->cmd_err) { + dev_dbg(i2c_bus->dev, "recovery error\n"); + ret = -EPROTO; + } + } + } + + /* Recovery done */ + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); + if (state & AST2600_I2CC_BUS_BUSY_STS) { + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state); + ret = -EPROTO; + } + + /* restore original master/slave setting */ + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + return ret; +} + +static int ast2600_i2c_setup_dma_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + int xfer_len; + + cmd |= AST2600_I2CM_PKT_EN; + xfer_len = msg->len - i2c_bus->master_xfer_cnt; + if (xfer_len > AST2600_I2C_DMA_SIZE) { + xfer_len = AST2600_I2C_DMA_SIZE; + } else { + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) + cmd |= AST2600_I2CM_STOP_CMD; + } + + if (cmd & AST2600_I2CM_START_CMD) { + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); + i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); + if (!i2c_bus->master_safe_buf) + return -ENOMEM; + i2c_bus->master_dma_addr = + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, + msg->len, DMA_TO_DEVICE); + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) { + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); + i2c_bus->master_safe_buf = NULL; + return -ENOMEM; + } + } + + if (xfer_len) { + cmd |= AST2600_I2CM_TX_DMA_EN | AST2600_I2CM_TX_CMD; + writel(AST2600_I2CM_SET_TX_DMA_LEN(xfer_len - 1), + i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); + writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt, + i2c_bus->reg_base + AST2600_I2CM_TX_DMA); + } + + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + + return 0; +} + +static int ast2600_i2c_setup_buff_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + u32 wbuf_dword; + int xfer_len; + u8 wbuf[4]; + int i; + + cmd |= AST2600_I2CM_PKT_EN; + xfer_len = msg->len - i2c_bus->master_xfer_cnt; + if (xfer_len > i2c_bus->buf_size) { + xfer_len = i2c_bus->buf_size; + } else { + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) + cmd |= AST2600_I2CM_STOP_CMD; + } + + if (cmd & AST2600_I2CM_START_CMD) + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); + + if (xfer_len) { + cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD; + /* + * The controller's buffer register supports dword writes only. + * Therefore, write dwords to the buffer register in a 4-byte aligned, + * and write the remaining unaligned data at the end. + */ + for (i = 0; i < xfer_len; i++) { + wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i]; + if ((i % 4) == 3 || i == xfer_len - 1) { + wbuf_dword = get_unaligned_le32(wbuf); + writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4)); + } + } + + writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len), + i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); + } + + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + + return 0; +} + +static int ast2600_i2c_setup_byte_tx(u32 cmd, struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + int xfer_len; + + xfer_len = msg->len - i2c_bus->master_xfer_cnt; + + cmd |= AST2600_I2CM_PKT_EN; + + if (cmd & AST2600_I2CM_START_CMD) + cmd |= AST2600_I2CM_PKT_ADDR(msg->addr); + + if ((i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) && + ((i2c_bus->master_xfer_cnt + 1) == msg->len)) + cmd |= AST2600_I2CM_STOP_CMD; + + if (xfer_len) { + cmd |= AST2600_I2CM_TX_CMD; + writel(msg->buf[i2c_bus->master_xfer_cnt], + i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF); + } + + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + + return 0; +} + +static int ast2600_i2c_setup_dma_rx(struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + int xfer_len; + u32 cmd; + + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_DMA_EN; + + if (msg->flags & I2C_M_RECV_LEN) { + xfer_len = 1; + } else { + if (msg->len > AST2600_I2C_DMA_SIZE) { + xfer_len = AST2600_I2C_DMA_SIZE; + } else { + xfer_len = msg->len; + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) + cmd |= MASTER_TRIGGER_LAST_STOP; + } + } + writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1), i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); + i2c_bus->master_safe_buf = i2c_get_dma_safe_msg_buf(msg, 1); + if (!i2c_bus->master_safe_buf) + return -ENOMEM; + i2c_bus->master_dma_addr = + dma_map_single(i2c_bus->dev, i2c_bus->master_safe_buf, msg->len, DMA_FROM_DEVICE); + if (dma_mapping_error(i2c_bus->dev, i2c_bus->master_dma_addr)) { + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, false); + i2c_bus->master_safe_buf = NULL; + return -ENOMEM; + } + writel(i2c_bus->master_dma_addr, i2c_bus->reg_base + AST2600_I2CM_RX_DMA); + + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + + return 0; +} + +static int ast2600_i2c_setup_buff_rx(struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + int xfer_len; + u32 cmd; + + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_BUFF_EN; + + if (msg->flags & I2C_M_RECV_LEN) { + dev_dbg(i2c_bus->dev, "smbus read\n"); + xfer_len = 1; + } else { + if (msg->len > i2c_bus->buf_size) { + xfer_len = i2c_bus->buf_size; + } else { + xfer_len = msg->len; + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) + cmd |= MASTER_TRIGGER_LAST_STOP; + } + } + writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); + + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + + return 0; +} + +static int ast2600_i2c_setup_byte_rx(struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + u32 cmd; + + cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | + AST2600_I2CM_START_CMD | AST2600_I2CM_RX_CMD; + + if (msg->flags & I2C_M_RECV_LEN) { + dev_dbg(i2c_bus->dev, "smbus read\n"); + } else { + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) { + if (msg->len == 1) + cmd |= MASTER_TRIGGER_LAST_STOP; + } + } + + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + + return 0; +} + +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + + /* send start */ + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n", + i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD), + msg->len, msg->len > 1 ? "s" : "", + msg->flags & I2C_M_RD ? "from" : "to", msg->addr); + + i2c_bus->master_xfer_cnt = 0; + i2c_bus->buf_index = 0; + + if (msg->flags & I2C_M_RD) { + if (i2c_bus->mode == DMA_MODE) + return ast2600_i2c_setup_dma_rx(i2c_bus); + else if (i2c_bus->mode == BUFF_MODE) + return ast2600_i2c_setup_buff_rx(i2c_bus); + else + return ast2600_i2c_setup_byte_rx(i2c_bus); + } else { + if (i2c_bus->mode == DMA_MODE) + return ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus); + else if (i2c_bus->mode == BUFF_MODE) + return ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus); + else + return ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus); + } +} + +static int ast2600_i2c_irq_err_to_errno(u32 irq_status) +{ + if (irq_status & AST2600_I2CM_ARBIT_LOSS) + return -EAGAIN; + if (irq_status & (AST2600_I2CM_SDA_DL_TO | AST2600_I2CM_SCL_LOW_TO)) + return -EBUSY; + if (irq_status & (AST2600_I2CM_ABNORMAL)) + return -EPROTO; + + return 0; +} + +static void ast2600_i2c_master_package_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts) +{ + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index]; + u32 cmd = AST2600_I2CM_PKT_EN; + int xfer_len; + int i; + + sts &= ~AST2600_I2CM_PKT_DONE; + writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR); + switch (sts) { + case AST2600_I2CM_PKT_ERROR: + i2c_bus->cmd_err = -EAGAIN; + complete(&i2c_bus->cmd_complete); + break; + case AST2600_I2CM_PKT_ERROR | AST2600_I2CM_TX_NAK: /* a0 fix for issue */ + fallthrough; + case AST2600_I2CM_PKT_ERROR | AST2600_I2CM_TX_NAK | AST2600_I2CM_NORMAL_STOP: + i2c_bus->cmd_err = -ENXIO; + complete(&i2c_bus->cmd_complete); + break; + case AST2600_I2CM_NORMAL_STOP: + /* write 0 byte only have stop isr */ + i2c_bus->msgs_index++; + if (i2c_bus->msgs_index < i2c_bus->msgs_count) { + if (ast2600_i2c_do_start(i2c_bus)) { + i2c_bus->cmd_err = -ENOMEM; + complete(&i2c_bus->cmd_complete); + } + } else { + i2c_bus->cmd_err = i2c_bus->msgs_index; + complete(&i2c_bus->cmd_complete); + } + break; + case AST2600_I2CM_TX_ACK: + case AST2600_I2CM_TX_ACK | AST2600_I2CM_NORMAL_STOP: + if (i2c_bus->mode == DMA_MODE) + xfer_len = AST2600_I2C_GET_TX_DMA_LEN(readl(i2c_bus->reg_base + + AST2600_I2CM_DMA_LEN_STS)); + else if (i2c_bus->mode == BUFF_MODE) + xfer_len = AST2600_I2CC_GET_TX_BUF_LEN(readl(i2c_bus->reg_base + + AST2600_I2CC_BUFF_CTRL)); + else + xfer_len = 1; + + i2c_bus->master_xfer_cnt += xfer_len; + + if (i2c_bus->master_xfer_cnt == msg->len) { + if (i2c_bus->mode == DMA_MODE) { + dma_unmap_single(i2c_bus->dev, i2c_bus->master_dma_addr, msg->len, + DMA_TO_DEVICE); + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, true); + i2c_bus->master_safe_buf = NULL; + } + i2c_bus->msgs_index++; + if (i2c_bus->msgs_index == i2c_bus->msgs_count) { + i2c_bus->cmd_err = i2c_bus->msgs_index; + complete(&i2c_bus->cmd_complete); + } else { + if (ast2600_i2c_do_start(i2c_bus)) { + i2c_bus->cmd_err = -ENOMEM; + complete(&i2c_bus->cmd_complete); + } + } + } else { + if (i2c_bus->mode == DMA_MODE) + ast2600_i2c_setup_dma_tx(0, i2c_bus); + else if (i2c_bus->mode == BUFF_MODE) + ast2600_i2c_setup_buff_tx(0, i2c_bus); + else + ast2600_i2c_setup_byte_tx(0, i2c_bus); + } + break; + case AST2600_I2CM_RX_DONE: + case AST2600_I2CM_RX_DONE | AST2600_I2CM_NORMAL_STOP: + /* do next rx */ + if (i2c_bus->mode == DMA_MODE) { + xfer_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base + + AST2600_I2CM_DMA_LEN_STS)); + } else if (i2c_bus->mode == BUFF_MODE) { + xfer_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base + + AST2600_I2CC_BUFF_CTRL)); + for (i = 0; i < xfer_len; i++) + msg->buf[i2c_bus->master_xfer_cnt + i] = + readb(i2c_bus->buf_base + 0x10 + i); + } else { + xfer_len = 1; + msg->buf[i2c_bus->master_xfer_cnt] = + AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base + + AST2600_I2CC_STS_AND_BUFF)); + } + + if (msg->flags & I2C_M_RECV_LEN) { + msg->len = min_t(unsigned int, msg->buf[0], I2C_SMBUS_BLOCK_MAX); + msg->len += ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1); + msg->flags &= ~I2C_M_RECV_LEN; + } + i2c_bus->master_xfer_cnt += xfer_len; + + if (i2c_bus->master_xfer_cnt == msg->len) { + if (i2c_bus->mode == DMA_MODE) { + dma_unmap_single(i2c_bus->dev, i2c_bus->master_dma_addr, msg->len, + DMA_FROM_DEVICE); + i2c_put_dma_safe_msg_buf(i2c_bus->master_safe_buf, msg, true); + i2c_bus->master_safe_buf = NULL; + } + + i2c_bus->msgs_index++; + if (i2c_bus->msgs_index == i2c_bus->msgs_count) { + i2c_bus->cmd_err = i2c_bus->msgs_index; + complete(&i2c_bus->cmd_complete); + } else { + if (ast2600_i2c_do_start(i2c_bus)) { + i2c_bus->cmd_err = -ENOMEM; + complete(&i2c_bus->cmd_complete); + } + } + } else { + /* next rx */ + cmd |= AST2600_I2CM_RX_CMD; + if (i2c_bus->mode == DMA_MODE) { + cmd |= AST2600_I2CM_RX_DMA_EN; + xfer_len = msg->len - i2c_bus->master_xfer_cnt; + if (xfer_len > AST2600_I2C_DMA_SIZE) { + xfer_len = AST2600_I2C_DMA_SIZE; + } else { + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) + cmd |= MASTER_TRIGGER_LAST_STOP; + } + writel(AST2600_I2CM_SET_RX_DMA_LEN(xfer_len - 1), + i2c_bus->reg_base + AST2600_I2CM_DMA_LEN); + writel(i2c_bus->master_dma_addr + i2c_bus->master_xfer_cnt, + i2c_bus->reg_base + AST2600_I2CM_RX_DMA); + } else if (i2c_bus->mode == BUFF_MODE) { + cmd |= AST2600_I2CM_RX_BUFF_EN; + xfer_len = msg->len - i2c_bus->master_xfer_cnt; + if (xfer_len > i2c_bus->buf_size) { + xfer_len = i2c_bus->buf_size; + } else { + if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) + cmd |= MASTER_TRIGGER_LAST_STOP; + } + writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len), + i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL); + } else { + if ((i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) && + ((i2c_bus->master_xfer_cnt + 1) == msg->len)) { + cmd |= MASTER_TRIGGER_LAST_STOP; + } + } + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); + } + break; + default: + dev_dbg(i2c_bus->dev, "unhandled sts %x\n", sts); + break; + } +} + +static int ast2600_i2c_master_irq(struct ast2600_i2c_bus *i2c_bus) +{ + u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR); + u32 ier = readl(i2c_bus->reg_base + AST2600_I2CM_IER); + u32 ctrl; + + if (!i2c_bus->alert_enable) + sts &= ~AST2600_I2CM_SMBUS_ALT; + + if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) { + writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base + AST2600_I2CM_ISR); + ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + i2c_bus->cmd_err = -EPROTO; + complete(&i2c_bus->cmd_complete); + return 1; + } + + if (AST2600_I2CM_BUS_RECOVER & sts) { + writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base + AST2600_I2CM_ISR); + i2c_bus->cmd_err = 0; + complete(&i2c_bus->cmd_complete); + return 1; + } + + if (AST2600_I2CM_SMBUS_ALT & sts) { + if (ier & AST2600_I2CM_SMBUS_ALT) { + /* Disable ALT INT */ + writel(ier & ~AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_IER); + i2c_handle_smbus_alert(i2c_bus->ara); + writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_ISR); + dev_err(i2c_bus->dev, + "ast2600_master_alert_recv bus id %d, Disable Alt, Please Imple\n", + i2c_bus->adap.nr); + return 1; + } + } + + i2c_bus->cmd_err = ast2600_i2c_irq_err_to_errno(sts); + if (i2c_bus->cmd_err) { + writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR); + complete(&i2c_bus->cmd_complete); + return 1; + } + + if (AST2600_I2CM_PKT_DONE & sts) { + ast2600_i2c_master_package_irq(i2c_bus, sts); + return 1; + } + + return 0; +} + +static irqreturn_t ast2600_i2c_bus_irq(int irq, void *dev_id) +{ + struct ast2600_i2c_bus *i2c_bus = dev_id; + + return IRQ_RETVAL(ast2600_i2c_master_irq(i2c_bus)); +} + +static int ast2600_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) +{ + struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(adap); + unsigned long timeout; + int ret; + + /* If bus is busy in a single master environment, attempt recovery. */ + if (!i2c_bus->multi_master && + (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & AST2600_I2CC_BUS_BUSY_STS)) { + ret = ast2600_i2c_recover_bus(i2c_bus); + if (ret) + return ret; + } + + i2c_bus->cmd_err = 0; + i2c_bus->msgs = msgs; + i2c_bus->msgs_index = 0; + i2c_bus->msgs_count = num; + reinit_completion(&i2c_bus->cmd_complete); + ret = ast2600_i2c_do_start(i2c_bus); + if (ret) + goto master_out; + timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout); + if (timeout == 0) { + u32 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + + dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n", + readl(i2c_bus->reg_base + AST2600_I2CM_ISR), + readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF)); + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + + if (i2c_bus->multi_master && + (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & + AST2600_I2CC_BUS_BUSY_STS)) + ast2600_i2c_recover_bus(i2c_bus); + + ret = -ETIMEDOUT; + } else { + ret = i2c_bus->cmd_err; + } + + dev_dbg(i2c_bus->dev, "bus%d-m: %d end\n", i2c_bus->adap.nr, i2c_bus->cmd_err); + +master_out: + if (i2c_bus->mode == DMA_MODE) { + kfree(i2c_bus->master_safe_buf); + i2c_bus->master_safe_buf = NULL; + } + + return ret; +} + +static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus) +{ + struct platform_device *pdev = to_platform_device(i2c_bus->dev); + u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN; + + /* I2C Reset */ + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + + i2c_bus->multi_master = device_property_read_bool(&pdev->dev, "multi-master"); + if (!i2c_bus->multi_master) + fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS; + + /* Enable Master Mode */ + writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + /* disable slave address */ + writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL); + + /* Set AC Timing */ + writel(ast2600_select_i2c_clock(i2c_bus), i2c_bus->reg_base + AST2600_I2CC_AC_TIMING); + + /* Clear Interrupt */ + writel(GENMASK(27, 0), i2c_bus->reg_base + AST2600_I2CM_ISR); +} + +static u32 ast2600_i2c_functionality(struct i2c_adapter *adap) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA; +} + +static const struct i2c_algorithm i2c_ast2600_algorithm = { + .master_xfer = ast2600_i2c_master_xfer, + .functionality = ast2600_i2c_functionality, +}; + +static const struct of_device_id ast2600_i2c_bus_of_table[] = { + { + .compatible = "aspeed,ast2600-i2cv2", + }, + {} +}; +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table); + +static int ast2600_i2c_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct ast2600_i2c_bus *i2c_bus; + struct resource *res; + u32 global_ctrl; + int ret; + + i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL); + if (!i2c_bus) + return -ENOMEM; + + i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(i2c_bus->reg_base)) + return PTR_ERR(i2c_bus->reg_base); + + i2c_bus->rst = devm_reset_control_get_shared(dev, NULL); + if (IS_ERR(i2c_bus->rst)) + return dev_err_probe(dev, PTR_ERR(i2c_bus->rst), "Missing reset ctrl\n"); + + reset_control_deassert(i2c_bus->rst); + + i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,global-regs"); + if (IS_ERR(i2c_bus->global_regs)) + return PTR_ERR(i2c_bus->global_regs); + + regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl); + if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) { + regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL, AST2600_GLOBAL_INIT); + regmap_write(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL); + } + + i2c_bus->dev = dev; + i2c_bus->mode = BUFF_MODE; + + if (device_property_read_bool(&pdev->dev, "aspeed,enable-dma")) + i2c_bus->mode = DMA_MODE; + + if (i2c_bus->mode == BUFF_MODE) { + i2c_bus->buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res); + if (!IS_ERR_OR_NULL(i2c_bus->buf_base)) + i2c_bus->buf_size = resource_size(res) / 2; + else + i2c_bus->mode = BYTE_MODE; + } + + /* + * i2c timeout counter: use base clk4 1Mhz, + * per unit: 1/(1000/4096) = 4096us + */ + ret = device_property_read_u32(dev, "i2c-scl-clk-low-timeout-us", &i2c_bus->timeout); + if (!ret) + i2c_bus->timeout /= 4096; + + init_completion(&i2c_bus->cmd_complete); + + i2c_bus->irq = platform_get_irq(pdev, 0); + if (i2c_bus->irq < 0) + return i2c_bus->irq; + + platform_set_drvdata(pdev, i2c_bus); + + i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL); + if (IS_ERR(i2c_bus->clk)) + return dev_err_probe(i2c_bus->dev, PTR_ERR(i2c_bus->clk), "Can't get clock\n"); + + i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk); + + i2c_parse_fw_timings(i2c_bus->dev, &i2c_bus->timing_info, true); + + /* Initialize the I2C adapter */ + i2c_bus->adap.owner = THIS_MODULE; + i2c_bus->adap.algo = &i2c_ast2600_algorithm; + i2c_bus->adap.retries = 0; + i2c_bus->adap.dev.parent = i2c_bus->dev; + device_set_node(&i2c_bus->adap.dev, dev_fwnode(dev)); + i2c_bus->adap.algo_data = i2c_bus; + strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name)); + i2c_set_adapdata(&i2c_bus->adap, i2c_bus); + + ast2600_i2c_init(i2c_bus); + + ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0, + dev_name(dev), i2c_bus); + if (ret < 0) + return dev_err_probe(dev, ret, "Unable to request irq %d\n", i2c_bus->irq); + + i2c_bus->alert_enable = device_property_read_bool(dev, "smbus-alert"); + if (i2c_bus->alert_enable) { + i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data); + if (!i2c_bus->ara) + dev_warn(dev, "Failed to register ARA client\n"); + + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT, + i2c_bus->reg_base + AST2600_I2CM_IER); + } else { + i2c_bus->alert_enable = false; + /* Set interrupt generation of I2C master controller */ + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER, + i2c_bus->reg_base + AST2600_I2CM_IER); + } + + ret = devm_i2c_add_adapter(dev, &i2c_bus->adap); + if (ret) + return ret; + + return 0; +} + +static void ast2600_i2c_remove(struct platform_device *pdev) +{ + struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev); + + /* Disable everything. */ + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL); + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER); +} + +static struct platform_driver ast2600_i2c_bus_driver = { + .probe = ast2600_i2c_probe, + .remove = ast2600_i2c_remove, + .driver = { + .name = KBUILD_MODNAME, + .of_match_table = ast2600_i2c_bus_of_table, + }, +}; +module_platform_driver(ast2600_i2c_bus_driver); + +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>"); +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver"); +MODULE_LICENSE("GPL");
Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The new register mode have separate register set to control i2c master and slave. This patch is for i2c master mode driver. Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> --- drivers/i2c/busses/Kconfig | 11 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-ast2600.c | 1057 ++++++++++++++++++++++++++++++ 3 files changed, 1069 insertions(+) create mode 100644 drivers/i2c/busses/i2c-ast2600.c