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? -- With Best Regards, Andy Shevchenko