[PATCH v4 3/3] i2c: aspeed: support ast2600 i2c new register mode driver
Ryan Chen
ryan_chen at aspeedtech.com
Thu Feb 2 20:16:25 AEDT 2023
Hello Jeremy
>
> > Add i2c new register mode driver to support AST2600 i2c new register
> > mode.
>
> Nice!
>
> A bit of review below - I assume your state machine & event handling is
> correct, but this is more on the driver API side.
>
> > +static int ast2600_i2c_global_probe(struct platform_device *pdev) {
> > + struct ast2600_i2c_global *i2c_global;
> > + struct resource *res;
> > +
> > + i2c_global = devm_kzalloc(&pdev->dev, sizeof(*i2c_global),
> > +GFP_KERNEL);
> > + if (IS_ERR(i2c_global))
> > + return PTR_ERR(i2c_global);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + i2c_global->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(i2c_global->base))
> > + return PTR_ERR(i2c_global->base);
> > +
> > + i2c_global->rst = devm_reset_control_get_exclusive(&pdev->dev,
> > +NULL);
> > + if (IS_ERR(i2c_global->rst))
> > + return IS_ERR(i2c_global->rst);
> > +
> > + reset_control_assert(i2c_global->rst);
> > + udelay(3);
> > + reset_control_deassert(i2c_global->rst);
> > +
> > + writel(AST2600_GLOBAL_INIT, i2c_global->base +
> > +AST2600_I2CG_CTRL);
> > + writel(I2CCG_DIV_CTRL, i2c_global->base +
> > +AST2600_I2CG_CLK_DIV_CTRL);
> > +
> > + pr_info("i2c global registered\n");
>
> I'm not the printk police, but I don't think this message is very useful.
Due to i2c global is top of all i2c bus, should be early than i2c-ast2600.c driver probe.
Original though is show to check. I can remove it.
>
> > +
> > + return 0;
> > +}
>
> ... so this driver is doing some common reset bits then just the two register
> writes - is it possible to just use a common syscon/mfd device instead? The
> core i2c driver could handle the reg writes through the syscon regmap instead.
>
Sure. I can modify it by syscon/mfd device driver.
> > +
> > +static struct platform_driver ast2600_i2c_global_driver = {
> > + .probe = ast2600_i2c_global_probe,
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = ast2600_i2c_global_of_match,
> > + },
> > +};
> > +
> > +static int __init ast2600_i2c_global_init(void) {
> > + return platform_driver_register(&ast2600_i2c_global_driver);
> > +}
> > +device_initcall(ast2600_i2c_global_init);
>
> Maybe module_platform_driver() instead?
Due to i2c global is top of all i2c bus like the scu, it make sure the driver is before the i2c bus driver probe.
It is needed use device_initcal function.
>
> > +enum xfer_mode {
> > + BYTE_MODE = 0,
> > + BUFF_MODE,
> > + DMA_MODE,
> > +};
> > +
> > +struct ast2600_i2c_bus {
> > + struct i2c_adapter adap;
> > + struct device *dev;
> > + void __iomem *reg_base;
> > + struct regmap *global_reg;
> > + int irq;
> > + /* 0: dma, 1: pool, 2:byte */
> > + enum xfer_mode mode;
>
> The comment doesn't match the enum.
Yes, it is typo will remove it.
>
> > + /* 0: old mode, 1: new mode */
> >
> + int clk_div
> _mode;
>
> ... and if you're using an enum for xfer_mode, may as well do the same for
> clk_div_mode, which will make some of the conditionals below more obvious.
>
Yes, will add enum
enum div_mode {
OLD_DIV = 0,
NEW_DIV,
};
> > +static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
> > +{
> > + unsigned long base_clk1;
> > + unsigned long base_clk2;
> > + unsigned long base_clk3;
> > + unsigned long base_clk4;
> > + int baseclk_idx;
> > + u32 clk_div_reg;
> > + u32 scl_low;
> > + u32 scl_high;
> > + int divisor;
> > + int inc = 0;
> > + u32 data;
> > + int i;
> > +
> > + if (i2c_bus->clk_div_mode) {
> > + regmap_read(i2c_bus->global_reg,
> > +AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);
> > + base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg
> > +& 0xff) + 2) * 10) / 2);
> > + base_clk2 = (i2c_bus->apb_clk * 10) /
> > + (((((clk_div_reg >> 8) & 0xff)
> + 2) *
> > +10) / 2);
> > + base_clk3 = (i2c_bus->apb_clk * 10) /
> > + (((((clk_div_reg >> 16) &
> 0xff) + 2) *
> > +10) / 2);
> > + base_clk4 = (i2c_bus->apb_clk * 10) /
> > + (((((clk_div_reg >> 24) &
> 0xff) + 2) *
> > +10) / 2);
> > +
> > + 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);
> > + } else if ((base_clk1 / i2c_bus->bus_frequency) <=
> 32)
> > +{
> > + baseclk_idx = 1;
> > + divisor = DIV_ROUND_UP(base_clk1,
> > +i2c_bus->bus_frequency);
> > + } else if ((base_clk2 / i2c_bus->bus_frequency) <=
> 32)
> > +{
> > + baseclk_idx = 2;
> > + divisor = DIV_ROUND_UP(base_clk2,
> > +i2c_bus->bus_frequency);
> > + } else if ((base_clk3 / i2c_bus->bus_frequency) <=
> 32)
> > +{
> > + baseclk_idx = 3;
> > + divisor = DIV_ROUND_UP(base_clk3,
> > +i2c_bus->bus_frequency);
> > + } else {
> > + baseclk_idx = 4;
> > + divisor = DIV_ROUND_UP(base_clk4,
> > +i2c_bus->bus_frequency);
> > + inc = 0;
> > + while ((divisor + inc) > 32) {
> > + inc |= divisor & 0x1;
> > + divisor >>= 1;
> > + baseclk_idx++;
> > + }
> > + divisor += inc;
> > + }
> > + divisor = min_t(int, divisor, 32);
> > + baseclk_idx &= 0xf;
> > + scl_low = ((divisor * 9) / 16) - 1;
> > + scl_low = min_t(u32, scl_low, 0xf);
> > + scl_high = (divisor - scl_low - 2) & 0xf;
> > + /* Divisor : Base Clock : tCKHighMin : tCK High : tCK
> > +Low */
> > + data = ((scl_high - 1) << 20) | (scl_high << 16) |
> > +(scl_low << 12) | (baseclk_idx);
> > + if (i2c_bus->timeout_enable) {
> > + data |=
> > +AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK);
> > + data |=
> > +AST2600_I2CC_TTIMEOUT(AST_I2C_TIMEOUT_COUNT);
> > + }
> > + } else {
> > + for (i = 0; i < ARRAY_SIZE(i2c_legacy_timing_table);
> > +i++) {
> > + if ((i2c_bus->apb_clk /
> > +i2c_legacy_timing_table[i].divisor) <
> > + i2c_bus->bus_frequency) {
> > + break;
> > + }
> > + }
> > + data = i2c_legacy_timing_table[i].timing;
>
> If you don't match a timing, this will overflow i2c_legacy_timing_table.
Sorry, I don't have good idea to modify this.
If (i = ARRAY_SIZE(i2c_legacy_timing_table)) then i = default to check ??
Or you have any good suggestion?
>
> > +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) {
> > + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> > + int xfer_len = 0;
> > + int i = 0;
> > + u32 cmd;
> > +
> > + cmd = AST2600_I2CM_PKT_EN |
> AST2600_I2CM_PKT_ADDR(msg->addr) |
> > +AST2600_I2CM_START_CMD;
> > +
> > + dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
> > + i2c_bus->msgs_index, msg->flags & I2C_M_RD ?
> "read" :
> > +"write",
> > + 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) {
> > + cmd |= AST2600_I2CM_RX_CMD;
> > + if (i2c_bus->mode == DMA_MODE) {
> [...]
> > + } else if (i2c_bus->mode == BUFF_MODE) {
> [...]
> > + } else {
> [...]
> > + }
> > + } else {
> > + if (i2c_bus->mode == DMA_MODE) {
> [...]
> > + } else if (i2c_bus->mode == BUFF_MODE) {
> [...]
> > + } else {
> [...]
> > + }
> > + }
> > + dev_dbg(i2c_bus->dev, "len %d , cmd %x\n", xfer_len, cmd);
> > + writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> > + return 0;
> > +}
>
> The core parts of this function are essentially split into three, depending on the
> bus mode. Maybe it would be better to refactor those into separate functions
> for buffer handling?
Sorry, I will keep this, due to each condition check will decide what command to trigger.
writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
That can keep the driver readable logic.
>
> > +
> > +static int ast2600_i2c_is_irq_error(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)
>
> What does 'package' refer to here?
>
> Same as above with this one - there's a lot of inner conditionals on the bus
> modes.
It is sync with datasheet mode setting. The default will enable it.
So I use the package function naming.
>
> > +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;
> > +
> > + dev_dbg(i2c_bus->dev, "M sts %x\n", sts);
> > + if (!i2c_bus->alert_enable)
> > + sts &= ~AST2600_I2CM_SMBUS_ALT;
> > +
> > + if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
> > + dev_dbg(i2c_bus->dev, "M clear isr:
> > +AST2600_I2CM_BUS_RECOVER_FAIL= %x\n", 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) {
> > + dev_dbg(i2c_bus->dev, "M clear isr:
> > +AST2600_I2CM_BUS_RECOVER= %x\n", 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) {
> > + dev_dbg(i2c_bus->dev, "M clear isr:
> > +AST2600_I2CM_SMBUS_ALT= %x\n", sts);
> > + /* 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_is_irq_error(sts);
> > + if (i2c_bus->cmd_err) {
> > + dev_dbg(i2c_bus->dev, "received error interrupt:
> > +0x%02x\n", sts);
> > + 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;
> > + }
> > +
> > + if (readl(i2c_bus->reg_base + AST2600_I2CM_ISR)) {
> > + dev_dbg(i2c_bus->dev, "master TODO care sts
> %x\n",
> > + readl(i2c_bus->reg_base +
> AST2600_I2CM_ISR));
>
> You're reading the same register twice here, is that intentional?
Will modify it to sts.
>
> > + writel(readl(i2c_bus->reg_base +
> AST2600_I2CM_ISR),
> > + i2c_bus->reg_base +
> AST2600_I2CM_ISR);
> >
>
> make that three times :)
>
> > +static int ast2600_i2c_probe(struct platform_device *pdev) {
> > + struct ast2600_i2c_bus *i2c_bus;
> > + const struct of_device_id *match;
> > + struct resource *res;
> > + u32 global_ctrl;
> > + int ret = 0;
> > +
> > + i2c_bus = devm_kzalloc(&pdev->dev, sizeof(*i2c_bus),
> > +GFP_KERNEL);
> > + if (!i2c_bus)
> > + return -ENOMEM;
> > +
> > + i2c_bus->global_reg =
> > +syscon_regmap_lookup_by_compatible("aspeed,ast2600-i2c-global");
> > + if (IS_ERR(i2c_bus->global_reg)) {
> > + dev_err(&pdev->dev, "failed to find ast2600 i2c
> global
> > +regmap\n");
> > + ret = -ENOMEM;
> > + goto free_mem;
> > + }
> > +
> > + regmap_read(i2c_bus->global_reg, AST2600_I2CG_CTRL,
> > +&global_ctrl);
> > +
> > + if (global_ctrl & AST2600_I2CG_CTRL_NEW_CLK_DIV)
> > + i2c_bus->clk_div_mode = 1;
> > +
> > + if (!(global_ctrl & AST2600_I2CG_CTRL_NEW_REG)) {
> > + ret = -ENOENT;
> > + dev_err(&pdev->dev, "Expect I2CG0C[2] = 1 (new
> reg
> > +mode)\n");
> > + goto free_mem;
> > + }
> > +
> > + i2c_bus->mode = DMA_MODE;
> > + i2c_bus->slave_operate = 0;
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "byte-mode"))
> > + i2c_bus->mode = BYTE_MODE;
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "buff-mode")) {
> > + res = platform_get_resource(pdev,
> IORESOURCE_MEM, 1);
> > + if (res && resource_size(res) >= 2)
> > + i2c_bus->buf_base =
> > +devm_ioremap_resource(&pdev->dev, res);
> > +
> > + if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
> > + i2c_bus->buf_size = resource_size(res) /
> 2;
> > +
> > + i2c_bus->mode = BUFF_MODE;
> > + }
>
> The buff-mode and byte-mode properties do not appear in your OF binding
> document. Using booleans to indicate mutually-exclusive modes seems a bit
> odd.
>
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "timeout"))
> > + i2c_bus->timeout_enable = 1;
>
> 'timeout' is also not described in the binding.
Will add into binding.
>
> > +
> > + i2c_bus->dev = &pdev->dev;
> > + init_completion(&i2c_bus->cmd_complete);
> > +
> > + i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(i2c_bus->reg_base)) {
> > + ret = PTR_ERR(i2c_bus->reg_base);
> > + goto free_mem;
> > + }
> > +
> > + i2c_bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > + if (i2c_bus->irq < 0) {
> > + dev_err(&pdev->dev, "no irq specified\n");
> > + ret = -i2c_bus->irq;
> > + goto free_irq;
> > + }
> > +
> > + match = of_match_node(ast2600_i2c_bus_of_table,
> > +pdev->dev.of_node);
> > + if (!match) {
> > + ret = -ENOENT;
> > + goto free_irq;
> > + }
>
> You don't use 'match' after this.
Yes, will remove this.
>
> > +
> > + platform_set_drvdata(pdev, i2c_bus);
> > +
> > + i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
> > + if (IS_ERR(i2c_bus->clk)) {
> > + dev_err(i2c_bus->dev, "no clock defined\n");
> > + ret = -ENODEV;
> > + goto free_irq;
> > + }
> > + i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
> > + dev_dbg(i2c_bus->dev, "i2c_bus->apb_clk %d\n",
> > +i2c_bus->apb_clk);
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
> > +&i2c_bus->bus_frequency);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Could not read bus-frequency
> > +property\n");
> > + i2c_bus->bus_frequency = 100000;
> > + }
> > +
> > + /* 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;
> > + i2c_bus->adap.dev.of_node = pdev->dev.of_node;
> > + 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(&pdev->dev, i2c_bus->irq,
> > +ast2600_i2c_bus_irq, 0,
> > + dev_name(&pdev->dev),
> i2c_bus);
> > + if (ret < 0)
> > + goto unmap;
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "smbus-alert")) {
> > + i2c_bus->alert_enable = 1;
> > + i2c_bus->ara =
> > +i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data);
> > + if (!i2c_bus->ara)
> > + dev_warn(i2c_bus->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 = 0;
> > + /* Set interrupt generation of I2C master controller
> > +*/
> > + 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 < 0)
> > + goto unmap;
> > +
> > + dev_info(i2c_bus->dev, "%s [%d]: adapter [%d khz] mode
> > +[%d]\n",
> > + pdev->dev.of_node->name, i2c_bus->adap.nr,
> > +i2c_bus->bus_frequency / 1000,
> > + i2c_bus->mode);
> > +
> > + return 0;
> > +
> > +unmap:
> > + free_irq(i2c_bus->irq, i2c_bus);
> > +free_irq:
> > + devm_iounmap(&pdev->dev, i2c_bus->reg_base);
>
> Something looks off in these goto labels.
>
> > +free_mem:
> > + kfree(i2c_bus);
>
> you devm_kzalloc()ed above, but are kfree()ing here (as well as in the _remove
> function). Same with devm_request_irq(). You could just drop these?
Yes, will drop this.
Thanks your review
Ryan
More information about the Linux-aspeed
mailing list