RE: [PATCH v30 4/5] i2c: ast2600: Add controller driver for AST2600 new register set

Hello William,

> Subject: Re: [PATCH v30 4/5] i2c: ast2600: Add controller driver for AST2600
> new register set
> 
.......
> > +
> > +static int ast2600_i2c_setup_buff_rx(u32 cmd, struct ast2600_i2c_bus
> > +*i2c_bus) {
> > +	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> > +	int xfer_len = msg->len - i2c_bus->controller_xfer_cnt;
> > +
> > +	cmd |= AST2600_I2CM_PKT_EN | AST2600_I2CM_RX_BUFF_EN |
> > +AST2600_I2CM_RX_CMD;
> > +
> > +	if (cmd & AST2600_I2CM_START_CMD)
> > +		cmd |= AST2600_I2CM_PKT_ADDR(msg->addr);
> > +
> > +	if (msg->flags & I2C_M_RECV_LEN) {
> > +		dev_dbg(i2c_bus->dev, "smbus read\n");
> > +		xfer_len = 1;
> > +	} else 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 |= CONTROLLER_TRIGGER_LAST_STOP;
> > +	}
> 
> You need to check `xfer_len > 0` somewhere before
> `AST2600_I2CC_SET_RX_BUF_LEN(xfer_len)`. Otherwise, a value of 0 will
> become -1 inside `AST2600_I2CC_SET_RX_BUF_LEN(x)` and cause a kernel
> crash when the device reads back extra memory.
> 
> Consider userspace doing the following
> ```
> union i2c_smbus_data data = {};
> // data.block[0] (length) is 0 here
> struct i2c_smbus_ioctl_data args = {};
> args.read_write = I2C_SMBUS_READ;
> args.command = mycmd;
> args.size = I2C_SMBUS_I2C_BLOCK_DATA;
> args.data = &data;
> ioctl(fd, I2C_SMBUS, &args);
> ```
OH, thanks your example.
I will update with following.
	if (xfer_len <= 0)
		return -EINVAL;

> > +	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_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] %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->controller_xfer_cnt = 0;
> > +
> > +	if (msg->flags & I2C_M_RD)
> > +		return ast2600_i2c_setup_buff_rx(AST2600_I2CM_START_CMD,
> i2c_bus);
> > +
> > +	return ast2600_i2c_setup_buff_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 -ETIMEDOUT;
> > +	if (irq_status & (AST2600_I2CM_ABNORMAL))
> > +		return -EPROTO;
> > +
> > +	return 0;
> > +}
> > +
> > +static void ast2600_i2c_controller_packet_irq(struct ast2600_i2c_bus
> > +*i2c_bus, u32 sts) {
> > +	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> > +	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 = -EBUSY;
> > +				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:
> > +		xfer_len =
> AST2600_I2CC_GET_TX_BUF_LEN(readl(i2c_bus->reg_base +
> > +						       AST2600_I2CC_BUFF_CTRL));
> > +		i2c_bus->controller_xfer_cnt += xfer_len;
> > +
> > +		if (i2c_bus->controller_xfer_cnt == msg->len) {
> > +			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 = -EBUSY;
> > +					complete(&i2c_bus->cmd_complete);
> > +				}
> > +			}
> > +		} else {
> > +			ast2600_i2c_setup_buff_tx(0, i2c_bus);
> > +		}
> > +		break;
> > +	case AST2600_I2CM_RX_DONE:
> > +	case AST2600_I2CM_RX_DONE | AST2600_I2CM_NORMAL_STOP:
> > +		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->controller_xfer_cnt + i] =
> > +				readb(i2c_bus->buf_base + i2c_bus->buf_size + i);
> > +
> > +		if (msg->flags & I2C_M_RECV_LEN) {
> > +			u8 recv_len =
> AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base
> > +						       + AST2600_I2CC_STS_AND_BUFF));
> > +			msg->len = min_t(unsigned int, recv_len,
> I2C_SMBUS_BLOCK_MAX);
> > +			msg->len += ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
> > +			msg->flags &= ~I2C_M_RECV_LEN;
> > +			if (!recv_len)
> > +				i2c_bus->controller_xfer_cnt = 0;
> > +			else
> > +				i2c_bus->controller_xfer_cnt = 1;
> > +		} else {
> > +			i2c_bus->controller_xfer_cnt += xfer_len;
> > +		}
> > +
> > +		if (i2c_bus->controller_xfer_cnt == msg->len) {
> > +			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 = -EBUSY;
> > +					complete(&i2c_bus->cmd_complete);
> > +				}
> > +			}
> > +		} else {
> > +			ast2600_i2c_setup_buff_rx(0, i2c_bus);
> > +		}
> > +		break;
> > +	default:
> > +		dev_dbg(i2c_bus->dev, "unhandled sts %x\n", sts);
> > +		break;
> > +	}
> > +}
> > +
> > +static int ast2600_i2c_controller_irq(struct ast2600_i2c_bus
> > +*i2c_bus) {
> > +	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
> > +	u32 ctrl;
> > +
> > +	sts &= ~AST2600_I2CM_SMBUS_ALERT;
> > +
> > +	if (sts & AST2600_I2CM_BUS_RECOVER_FAIL) {
> > +		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 (sts & AST2600_I2CM_BUS_RECOVER) {
> > +		writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base +
> AST2600_I2CM_ISR);
> > +		i2c_bus->cmd_err = 0;
> > +		complete(&i2c_bus->cmd_complete);
> > +		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 (sts & AST2600_I2CM_PKT_DONE) {
> > +		ast2600_i2c_controller_packet_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_controller_irq(i2c_bus));
> > +}
> > +
> > +static int ast2600_i2c_controller_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 (!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 controller_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_I2CM_IER);
> > +		synchronize_irq(i2c_bus->irq);
> > +		writel(readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
> > +		       i2c_bus->reg_base + AST2600_I2CM_ISR);
> > +
> > +		writel(ctrl & ~AST2600_I2CC_MASTER_EN, i2c_bus->reg_base +
> AST2600_I2CC_FUN_CTRL);
> > +		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +
> > +		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> > +		       i2c_bus->reg_base + AST2600_I2CM_IER);
> > +
> > +		/*
> > +		 * A slave holding SCL low can stall the transfer and trigger
> > +		 * a master timeout. In multi-master mode, attempt bus recovery
> > +		 * if the bus is still busy.
> > +		 */
> > +		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);
> > +
> > +controller_out:
> > +	return ret;
> > +}
> > +
> > +static int ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus) {
> > +	u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE |
> > +AST2600_I2CC_MASTER_EN;
> > +
> > +	/* I2C Reset */
> > +	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +
> > +	if (!i2c_bus->multi_master)
> > +		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
> > +
> > +	/* Enable Controller Mode */
> > +	writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +	/* disable target address */
> > +	writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> > +
> > +	/* Set AC Timing */
> > +	ast2600_i2c_ac_timing_config(i2c_bus);
> > +
> > +	/* Clear Interrupt */
> > +	writel(GENMASK(27, 0), i2c_bus->reg_base + AST2600_I2CM_ISR);
> > +
> > +	return 0;
> > +}
> > +
> > +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 = {
> > +	.xfer = ast2600_i2c_controller_xfer,
> > +	.functionality = ast2600_i2c_functionality, };
> > +
> > +static const struct i2c_adapter_quirks ast2600_i2c_quirks = {
> > +	.flags = I2C_AQ_NO_ZERO_LEN_READ,
> > +};
> > +
> > +static int ast2600_i2c_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct ast2600_i2c_bus *i2c_bus;
> > +	void __iomem *buf_base;
> > +	struct reset_control *rst;
> > +	struct resource *res;
> > +	u32 global_ctrl;
> > +	int ret;
> > +
> > +	if (!device_property_present(dev, "aspeed,global-regs"))
> > +		return -ENODEV;
> > +
> > +	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);
> > +
> > +	rst = devm_reset_control_get_shared_deasserted(dev, NULL);
> > +	if (IS_ERR(rst))
> > +		return dev_err_probe(dev, PTR_ERR(rst), "Missing reset ctrl\n");
> > +
> > +	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);
> > +
> > +	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->multi_master = device_property_read_bool(dev,
> > +"multi-master");
> > +
> > +	buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
> > +	if (IS_ERR(buf_base))
> > +		return dev_err_probe(dev, PTR_ERR(buf_base), "Missing buffer
> resource\n");
> > +	i2c_bus->buf_base = buf_base;
> > +	i2c_bus->buf_size = resource_size(res) / 2;
> > +
> > +	/*
> > +	 * i2c timeout counter: use base clk4 1Mhz,
> > +	 * per unit: 1/(1000/1024) = 1024us
> > +	 */
> > +	ret = device_property_read_u32(dev, "i2c-scl-clk-low-timeout-us",
> &i2c_bus->timeout);
> > +	if (!ret) {
> > +		i2c_bus->timeout = DIV_ROUND_UP(i2c_bus->timeout, 1024);
> > +		if (i2c_bus->timeout > GENMASK(4, 0)) {
> > +			dev_warn(dev,
> > +				 "i2c-scl-clk-low-timeout-us exceeds HW max (31 *
> 1024us), clamped\n");
> > +			i2c_bus->timeout = GENMASK(4, 0);
> > +		}
> > +	}
> > +
> > +	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.quirks = &ast2600_i2c_quirks;
> > +	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);
> > +	i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
> > +
> > +	ret = ast2600_i2c_init(i2c_bus);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Unable to initialize i2c %d\n",
> > +ret);
> > +
> > +	ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
> > +			       dev_name(dev), i2c_bus);
> > +	if (ret < 0) {
> > +		ret = dev_err_probe(dev, ret, "Unable to request irq %d\n",
> > +				    i2c_bus->irq);
> > +		goto err;
> > +	}
> > +
> > +	writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> > +	       i2c_bus->reg_base + AST2600_I2CM_IER);
> > +
> > +	ret = i2c_add_adapter(&i2c_bus->adap);
> > +	if (ret)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> > +	return ret;
> > +}
> > +
> > +static void ast2600_i2c_remove(struct platform_device *pdev) {
> > +	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
> > +
> > +	i2c_del_adapter(&i2c_bus->adap);
> > +
> > +	/* Disable everything. */
> > +	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> > +	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER); }
> > +
> > +static const struct of_device_id ast2600_i2c_of_match[] = {
> > +	{ .compatible = "aspeed,ast2600-i2c-bus" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ast2600_i2c_of_match);
> > +
> > +static struct platform_driver ast2600_i2c_driver = {
> > +	.probe		= ast2600_i2c_probe,
> > +	.remove		= ast2600_i2c_remove,
> > +	.driver		= {
> > +		.name		= "ast2600-i2c",
> > +		.of_match_table	= ast2600_i2c_of_match,
> > +	},
> > +};
> > +module_platform_driver(ast2600_i2c_driver);
> > +
> > +MODULE_AUTHOR("Ryan Chen <[email protected]>");
> > +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
> > +MODULE_LICENSE("GPL");
> >