[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