[PATCH v4 3/3] i2c: aspeed: support ast2600 i2c new register mode driver

Jeremy Kerr jk at ozlabs.org
Thu Feb 2 17:04:37 AEDT 2023


Hi Ryan,

> 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.

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

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

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

> +       /* 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.

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

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

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

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

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

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

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

Cheers,


Jeremy


More information about the openbmc mailing list