[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 openbmc mailing list