RE: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver

Hello Andy,

On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

quoted

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 i2c new register mode
have separate register set to control i2c master and slave.

...

+ bits.h

quoted

+#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>

quoted

+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>

You missed property.h
and these of*.h probably not needed at all, see below.

quoted

+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>

...

quoted

+#define AST2600_I2CC_GET_RX_BUFF(x)			(((x) >> 8) &

GENMASK(7, 0))

quoted

+#define AST2600_I2CC_GET_RX_BUF_LEN(x)		(((x) >> 24) &

GENMASK(5, 0))

quoted

+#define AST2600_I2CC_GET_TX_BUF_LEN(x)		((((x) >> 8) &

GENMASK(4, 0)) + 1)

With right shifts it's better to have GENMASK to be applied first, it will show
exact MSB of the bitfield.

(Ditto for other cases similar to these)

...

quoted

+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+	unsigned long base_clk[4];
+	int baseclk_idx;
+	u32 clk_div_reg;
+	u32 scl_low;
+	u32 scl_high;
+	int divisor;
+	int inc = 0;
+	u32 data;
+
+	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL,

&clk_div_reg);

quoted

+	for (int i = 0; i < 4; i++) {

See below.

quoted

+		base_clk[i] = (i2c_bus->apb_clk * 10) /
+		(((((clk_div_reg >> (i * 8)) & GENMASK(7, 0)) + 2) * 10) / 2);

Second line is wrongly indented.

quoted

+	}

quoted

+	if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 0;
+		divisor = DIV_ROUND_UP(i2c_bus->apb_clk,

i2c_bus->bus_frequency);

quoted

+	} else {

quoted

+		int i;
+

Just add to the definition block:

	unsigned int i;

quoted

+		for (i = 0; i < 4; i++) {
+			if ((base_clk[i] / i2c_bus->bus_frequency) <= 32) {

quoted

+				baseclk_idx = i + 1;
+				divisor = DIV_ROUND_UP(base_clk[i],

i2c_bus->bus_frequency);

These two can be moved outside of the loop

quoted

+				break;
+			}

			if ((base_clk[i] / i2c_bus->bus_frequency) <= 32)
				break;

quoted

+		}
+		if (i == 4) {
+			baseclk_idx = 4;
+			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->bus_frequency);

quoted

+			while ((divisor + inc) > 32) {
+				inc |= divisor & 0x1;
+				divisor >>= 1;

	unsigned long divisor;

			for_each_set_bit(divisor, ...)

I.o.w. think about this, maybe you can refactor with the above.

quoted

+				baseclk_idx++;
+			}
+			divisor += inc;

		} else {
			...those two lines...

quoted

+		}

quoted

+	}
+
+	divisor = min_t(int, divisor, 32);
+	baseclk_idx &= GENMASK(3, 0);

quoted

+	scl_low = ((divisor * 9) / 16) - 1;
+	scl_low = min_t(u32, scl_low, GENMASK(3, 0));

(with the divisor being unsigned long) this can be rewritten as

	scl_low = min(divisor * 9 / 16 - 1, GENMASK(3, 0));

which improves type checking and readability.

quoted

+	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;
+}

...

quoted

+static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) {
+	int ret = 0;
+	u32 ctrl;
+	u32 state;
+	int r;

quoted

+	dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr,
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

Why you can't reuse "state" (assigned below)?
If not, then something like

	/* ...comment that state can be changed... */
	state = ...
	dev_dbg(state);

quoted

+	ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN),
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) |
+AST2600_I2CC_MASTER_EN,

will it be different from ctrl value?

quoted

+	       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)) {

quoted

+		writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base +

AST2600_I2CM_CMD_STS);

quoted

+		r = wait_for_completion_timeout(&i2c_bus->cmd_complete,

i2c_bus->adap.timeout);

quoted

+		if (r == 0) {
+			dev_dbg(i2c_bus->dev, "recovery timed out\n");
+			ret = -ETIMEDOUT;

quoted

+		} else {
+			if (i2c_bus->cmd_err) {

		} else if (...) {

quoted

+				dev_dbg(i2c_bus->dev, "recovery error\n");
+				ret = -EPROTO;
+			}
+		}
+	}
+
+	dev_dbg(i2c_bus->dev, "Recovery done [%x]\n",
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

As above.

quoted

+	if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
+AST2600_I2CC_BUS_BUSY_STS) {

Two sequential reads may give you different values?

quoted

+		dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n",
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

Again? With this inconsistency it will be "nice" to debug.

quoted

+		ret = -EPROTO;
+	}
+
+	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	return ret;
+}

...

quoted

+#ifdef CONFIG_I2C_SLAVE

For (at least) review purposes I recommend to split slave out to the separate
change. This driver is 16 hundreds LoCs long...

quoted

+#endif

...

quoted

+		} else if (i2c_bus->mode == BUFF_MODE) {
+			/* buff mode */
+			cmd |= AST2600_I2CM_RX_BUFF_EN;

quoted

+			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;
+				}
+			}

This...

quoted

+			writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len),
+			       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		} else {

quoted

+			/* byte mode */
+			xfer_len = 1;
+			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;
+				}
+			}

...and this have a lot in common, can it be deduplicated?

quoted

+		}

...

quoted

+			if (msg->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;
+				xfer_len = msg->len;
+			}

See above.

...

quoted

+			u8 wbuf[4];
+			/* buff mode */
+			if (msg->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;
+				xfer_len = msg->len;
+			}
+			if (xfer_len) {
+				cmd |= AST2600_I2CM_TX_BUFF_EN |

AST2600_I2CM_TX_CMD;

quoted

+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i];
+					if (i % 4 == 3)

quoted

+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);

This is incorrect memory accessor.

quoted

+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));

Ditto.

quoted

+			}

...

quoted

+static int ast2600_i2c_is_irq_error(u32 irq_status)

This function is not boolean, so "_is_" seems misleading.

This is basically error code conversion, something like

ast2600_i2c_irq_err_to_errno(u32 irq_status)

quoted

+{
+	if (irq_status & AST2600_I2CM_ARBIT_LOSS)
+		return -EAGAIN;
+	if (irq_status & (AST2600_I2CM_SDA_DL_TO |

AST2600_I2CM_SCL_LOW_TO))

quoted

+		return -EBUSY;
+	if (irq_status & (AST2600_I2CM_ABNORMAL))
+		return -EPROTO;
+
+	return 0;
+}

...

quoted

+				u8 wbuf[4];
+
+				cmd |= AST2600_I2CM_TX_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 |= AST2600_I2CM_STOP_CMD;
+				}
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
+					if (i % 4 == 3)
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

Wrong memory accessors. You should use something from asm/byteorder.h
which includes linux/byteorder/generic.h.

Are you preferring add comment to explain more by following?
				/*
				 * 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];
					/* accumulating 4 bytes of data, write as a Dword to the buffer register */
					if (i % 4 == 3)
						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
				}
				/* less than 4 bytes of remaining data, write the remaining part as a Dword */
				if (--i % 4 != 3)
					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

Or more columns (use get_unaligned_le32(wbuf); ) by following.

	for (i = 0; i < xfer_len; i++) {
		wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
		if (i % 4 == 3) {
			wbuf_dword = get_unaligned_le32(wbuf);
			writel(wbuf_dword, i2c_bus->buf_base + i - 3);
		}
	}

	if (--i % 4 != 3) {
		wbuf_dword = get_unaligned_le32(wbuf);
		writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4));
	}

...

quoted

+#ifdef CONFIG_I2C_SLAVE
+		/* Workaround for master/slave package mode enable rx done stuck

issue

quoted

+		 * When master go for first read (RX_DONE), slave mode will also

effect

quoted

+		 * Then controller will send nack, not operate anymore.
+		 */

/*
 * Wrong style of multi-line
 * comments. You need to fix
 * them in the entire driver.
 */

quoted

+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) &

AST2600_I2CS_PKT_MODE_EN) {

quoted

+			u32 slave_cmd = readl(i2c_bus->reg_base +

AST2600_I2CS_CMD_STS);

quoted

+
+			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			writel(slave_cmd, i2c_bus->reg_base +

AST2600_I2CS_CMD_STS);

quoted

+		}
+		fallthrough;
+#endif

...

quoted

+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 = 0;

Redundant assignment.

quoted

+	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);

quoted

+		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);

quoted

+		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);

quoted

+			i2c_handle_smbus_alert(i2c_bus->ara);
+			writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base +

AST2600_I2CM_ISR);

quoted

+			dev_err(i2c_bus->dev,
+				"ast2600_master_alert_recv bus id %d, Disable Alt, Please

Imple\n",

quoted

+				i2c_bus->adap.nr);
+			return 1;
+		}
+	}
+
+	i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+	if (i2c_bus->cmd_err) {
+		writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base +

AST2600_I2CM_ISR);

quoted

+		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;
+}

...

quoted

+	if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
+		i2c_bus->multi_master = true;
+	else
+		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;

	i2c_bus->multi_master = device_property_read_bool(&pdev->dev,
"multi-master");
	if (!i2c_bus->multi_master)
		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;

...

quoted

+	struct device_node *np = pdev->dev.of_node;

It should use dev, but see below.

quoted

+	struct device *dev = &pdev->dev;
+	struct ast2600_i2c_bus *i2c_bus;
+	struct resource *res;
+	u32 global_ctrl;

quoted

+	int ret = 0;

Do you need this assignment?

...

quoted

+		i2c_bus->buf_base = devm_platform_ioremap_resource(pdev, 1);

quoted

+		if (!IS_ERR_OR_NULL(i2c_bus->buf_base))

Why not positive check?

quoted

+			i2c_bus->buf_size = resource_size(res) / 2;
+		else
+			i2c_bus->mode = BYTE_MODE;
+	}

...

quoted

+	ret = of_property_read_u32(dev->of_node,
+				   "i2c-scl-clk-low-timeout-us",
+				   &i2c_bus->timeout);

	device_property_read_u32()

quoted

+	if (!ret)
+		i2c_bus->timeout /= 4096;

...

quoted

+	ret = device_property_read_u32(&pdev->dev, "clock-frequency",

&i2c_bus->bus_frequency);

quoted

+	if (ret < 0) {
+		dev_warn(dev, "Could not read clock-frequency property\n");
+		i2c_bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
+	}

Use standard API from I2C core for this.

...

quoted

+	if (of_property_read_bool(dev->of_node, "smbus-alert")) {

device_property_read_bool()

Doesn't I2C core handle this property?

quoted

+		i2c_bus->alert_enable = true;
+		i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap,

&i2c_bus->alert_data);

quoted

+		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,

quoted

+		       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);
+	}

...

quoted

+	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);

Why explicit call?

...

quoted

+		dmam_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
+				   i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);

Ditto.

...

It looks to me like you ignored part of my comments. If so, I would like to know
why.

--
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel