[PATCH v24 3/4] i2c: ast2600: Add controller driver for new register layout
Jeremy Kerr
jk at codeconstruct.com.au
Tue Jan 27 20:41:28 AEDT 2026
Hi Ryan,
> Add i2c-ast2600 new register mode driver to support AST2600
> i2c new register mode. This i2c-ast2600 new driver and the
> legacy i2c-aspeed driver both match the same compatible string
> "aspeed,ast2600-i2c-bus" because they target the same I2C
> controller IP on AST2600.
You may want to describe the differing requirements of the old vs new
driver, in terms of DT node detection. More on that below, but probably
something like:
- the new driver is selected for DT nodes that comply with the current
aspeed,ast2600-i2c-bus binding spec (specifically: where the required
aspeed,global-regs property is present)
- for backwards compatibility with old DTs, the old driver is selected
for DT nodes that do not have the aspeed,global-regs property
> However, AST2600 SoCs may configure
> the controller instances to operate either in the legacy
> register layout or the new layout (via global register).
> The new register mode support following.
>
> - Add new clock divider option for more flexible and accurate
> clock rate generation
> - Add tCKHighMin timing to guarantee SCL high pulse width.
> - Add support dual pool buffer mode, split 32 bytes pool buffer
> of each device into 2 x 16 bytes for Tx and Rx individually.
> - Increase DMA buffer size to 4096 bytes and support byte alignment.
> - Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> - Re-define registers for separating controller and target
> mode control.
> - Support 4 individual DMA buffers for controller Tx and Rx,
> target Tx and Rx.
>
> And following is new register set for package transfer sequence.
s/package/packet/
> - New Master operation mode:
> S -> Aw -> P
> S -> Aw -> TxD -> P
> S -> Ar -> RxD -> P
> S -> Aw -> TxD -> Sr -> Ar -> RxD -> P
> - Bus SDA lock auto-release capability for new controller DMA
> command mode.
> - Bus auto timeout for new controller/target DMA mode.
>
> Since the register layout is selected via a global register at
> runtime and both drivers bind to the same compatible string,
> this patch defines the driver selection at build-time using
> Kconfig, ensuring that only one driver is compiled into the
> kernel.
This doesn't look to be correct, there are no Kconfig changes here.
Instead, you have introduced a top-level driver that selects one of the
new or old drivers based on the compatible and presence of the
aspeed,global-regs property.
You can simplify this into two independent drivers, with no need for the
multiplexing. The overlap on the aspeed,ast2600-i2c-bus compatible can
be resolved by ->probe() returning -ENODEV conditionally on the presence
of aspeed,global-regs (as your intended method of distinguishing between
new and backwards-compatible bindings).
With that approach, all you need to change in the old driver is the
(sufficiently-commented) -ENODEV return from ->probe().
> This approach avoids ambiguity and ensures consistent
> behavior for each platform configuration.
>
> The following is two versus register layout.
> Old register mode:
> {I2CD00}: Function Control Register
> {I2CD04}: Clock and AC Timing Control Register
> {I2CD08}: Clock and AC Timing Control Register
> {I2CD0C}: Interrupt Control Register
> {I2CD10}: Interrupt Status Register
> {I2CD14}: Command/Status Register
> {I2CD18}: Slave Device Address Register
> {I2CD1C}: Pool Buffer Control Register
> {I2CD20}: Transmit/Receive Byte Buffer Register
> {I2CD24}: DMA Mode Buffer Address Register
> {I2CD28}: DMA Transfer Length Register
> {I2CD2C}: Original DMA Mode Buffer Address Setting
> {I2CD30}: Original DMA Transfer Length Setting and Final Status
>
> New Register mode
> {I2CC00}: Master/Slave Function Control Register
> {I2CC04}: Master/Slave Clock and AC Timing Control Register
> {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
> {I2CC0C}: Master/Slave Pool Buffer Control Register
> {I2CM10}: Master Interrupt Control Register
> {I2CM14}: Master Interrupt Status Register
> {I2CM18}: Master Command/Status Register
> {I2CM1C}: Master DMA Buffer Length Register
> {I2CS20}: Slave~ Interrupt Control Register
> {I2CS24}: Slave~ Interrupt Status Register
> {I2CS28}: Slave~ Command/Status Register
> {I2CS2C}: Slave~ DMA Buffer Length Register
> {I2CM30}: Master DMA Mode Tx Buffer Base Address
> {I2CM34}: Master DMA Mode Rx Buffer Base Address
> {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
> {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
> {I2CS40}: Save Device Address Register
> {I2CM48}: Master DMA Length Status Register
> {I2CS4C}: Slave DMA Length Status Register
> {I2CC50}: Current DMA Operating Address Status
> {I2CC54}: Current DMA Operating Length Status
... I'm not sure we need a full register description in the changelog.
> Add a new core file(i2c-aspeed-core.c) to avoid device tree
> ABI break, allow both old and new device trees using the same
> compatible string "aspeed,ast2600-i2c-bus" to function correctly.
>
> Signed-off-by: Ryan Chen <ryan_chen at aspeedtech.com>
> ---
> drivers/i2c/busses/Makefile | 2 +-
> drivers/i2c/busses/i2c-aspeed-core.c | 89 +++
> drivers/i2c/busses/i2c-aspeed-core.h | 19 +
> drivers/i2c/busses/i2c-aspeed.c | 43 +-
> drivers/i2c/busses/i2c-ast2600.c | 1018 ++++++++++++++++++++++++++
> 5 files changed, 1136 insertions(+), 35 deletions(-)
> create mode 100644 drivers/i2c/busses/i2c-aspeed-core.c
> create mode 100644 drivers/i2c/busses/i2c-aspeed-core.h
> create mode 100644 drivers/i2c/busses/i2c-ast2600.c
>
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index fb985769f5ff..606b35196960 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -37,7 +37,7 @@ obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
> obj-$(CONFIG_I2C_ALTERA) += i2c-altera.o
> obj-$(CONFIG_I2C_AMD_MP2) += i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
> obj-$(CONFIG_I2C_AMD_ASF) += i2c-amd-asf-plat.o
> -obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
> +obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o i2c-ast2600.o i2c-aspeed-core.o
> obj-$(CONFIG_I2C_AT91) += i2c-at91.o
> i2c-at91-y := i2c-at91-core.o i2c-at91-master.o
> i2c-at91-$(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL) += i2c-at91-slave.o
> diff --git a/drivers/i2c/busses/i2c-aspeed-core.c b/drivers/i2c/busses/i2c-aspeed-core.c
> new file mode 100644
> index 000000000000..a2878e1273ed
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed-core.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ASPEED I2C core driver
> + *
> + * Copyright (C) ASPEED Technology Inc.
> + */
As above, this "multiplexing" core driver may not be necessary.
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +
> +#include "i2c-aspeed-core.h"
> +
> +struct aspeed_i2c_core_priv {
> + void (*remove)(struct platform_device *pdev);
> + void *bus_data;
> +};
> +
> +static const struct of_device_id aspeed_i2c_of_match[] = {
> + {
> + .compatible = "aspeed,ast2400-i2c-bus",
> + .data = (const void *)AST2400_I2C
> + },
> + {
> + .compatible = "aspeed,ast2500-i2c-bus",
> + .data = (const void *)AST2500_I2C
> + },
> + {
> + .compatible = "aspeed,ast2600-i2c-bus",
> + .data = (const void *)AST2600_I2C
> + },
> + { }
> +};
Just being enum members, the .data fields seem pointless; you could just
check against the actual compatible string to determine the device type?
> +
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_of_match);
> +
> +static int aspeed_i2c_core_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct aspeed_i2c_core_priv *priv;
> + const struct of_device_id *match;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + match = of_match_device(aspeed_i2c_of_match, dev);
> + if (!match)
> + return -ENODEV;
> +
> + if (device_is_compatible(dev, "aspeed,ast2600-i2c-bus") &&
> + device_property_present(dev, "aspeed,global-regs")) {
> + ret = ast2600_i2c_probe(match, pdev);
> + priv->remove = ast2600_i2c_remove;
> + } else {
> + ret = aspeed_i2c_probe_bus(match, pdev);
> + priv->remove = aspeed_i2c_remove_bus;
> + }
> +
> + priv->bus_data = platform_get_drvdata(pdev);
> + platform_set_drvdata(pdev, priv);
> + return ret;
> +}
> +
> +static void aspeed_i2c_core_remove(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_core_priv *priv = platform_get_drvdata(pdev);
> +
> + if (!priv || !priv->remove)
> + return;
> +
> + platform_set_drvdata(pdev, priv->bus_data);
> + return priv->remove(pdev);
> +}
> +
> +static struct platform_driver aspeed_i2c_driver = {
> + .probe = aspeed_i2c_core_probe,
> + .remove = aspeed_i2c_core_remove,
> + .driver = {
> + .name = "i2c-aspeed-core",
> + .of_match_table = aspeed_i2c_of_match,
> + },
> +};
> +module_platform_driver(aspeed_i2c_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen at aspeedtech.com>");
> +MODULE_DESCRIPTION("Unified ASPEED I2C driver (AST24xx/AST25xx/AST2600)");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/i2c/busses/i2c-aspeed-core.h b/drivers/i2c/busses/i2c-aspeed-core.h
> new file mode 100644
> index 000000000000..6e0091018985
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed-core.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _I2C_ASPEED_CORE_H
> +#define _I2C_ASPEED_CORE_H
> +
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +enum i2c_version {
> + AST2400_I2C,
> + AST2500_I2C,
> + AST2600_I2C,
> + AST2700_I2C,
> +};
> +
> +int aspeed_i2c_probe_bus(const struct of_device_id *match, struct platform_device *pdev);
> +void aspeed_i2c_remove_bus(struct platform_device *pdev);
> +int ast2600_i2c_probe(const struct of_device_id *match, struct platform_device *pdev);
> +void ast2600_i2c_remove(struct platform_device *pdev);
> +#endif
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a26b74c71206..8466c98b6c7b 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -25,6 +25,8 @@
> #include <linux/reset.h>
> #include <linux/slab.h>
>
> +#include "i2c-aspeed-core.h"
> +
> /* I2C Register */
> #define ASPEED_I2C_FUN_CTRL_REG 0x00
> #define ASPEED_I2C_AC_TIMING_REG1 0x04
> @@ -978,26 +980,9 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
> return ret;
> }
>
> -static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> - {
> - .compatible = "aspeed,ast2400-i2c-bus",
> - .data = aspeed_i2c_24xx_get_clk_reg_val,
> - },
> - {
> - .compatible = "aspeed,ast2500-i2c-bus",
> - .data = aspeed_i2c_25xx_get_clk_reg_val,
> - },
> - {
> - .compatible = "aspeed,ast2600-i2c-bus",
> - .data = aspeed_i2c_25xx_get_clk_reg_val,
> - },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
> -
> -static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +int aspeed_i2c_probe_bus(const struct of_device_id *match,
> + struct platform_device *pdev)
> {
> - const struct of_device_id *match;
> struct aspeed_i2c_bus *bus;
> struct clk *parent_clk;
> int irq, ret;
> @@ -1033,12 +1018,10 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
> }
>
> - match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
> - if (!match)
> + if ((enum i2c_version)(uintptr_t)match->data == AST2400_I2C)
> bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
> else
> - bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> - match->data;
> + bus->get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val;
>
> /* Initialize the I2C adapter */
> spin_lock_init(&bus->lock);
> @@ -1081,8 +1064,9 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(aspeed_i2c_probe_bus);
>
> -static void aspeed_i2c_remove_bus(struct platform_device *pdev)
> +void aspeed_i2c_remove_bus(struct platform_device *pdev)
> {
> struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> unsigned long flags;
> @@ -1099,16 +1083,7 @@ static void aspeed_i2c_remove_bus(struct platform_device *pdev)
>
> i2c_del_adapter(&bus->adap);
> }
> -
> -static struct platform_driver aspeed_i2c_bus_driver = {
> - .probe = aspeed_i2c_probe_bus,
> - .remove = aspeed_i2c_remove_bus,
> - .driver = {
> - .name = "aspeed-i2c-bus",
> - .of_match_table = aspeed_i2c_bus_of_table,
> - },
> -};
> -module_platform_driver(aspeed_i2c_bus_driver);
> +EXPORT_SYMBOL_GPL(aspeed_i2c_remove_bus);
>
> MODULE_AUTHOR("Brendan Higgins <brendanhiggins at google.com>");
> MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> new file mode 100644
> index 000000000000..04cb38e018a6
To prevent painting yourself into a corner (and the reference to 2700 in
this same file): is this named appropriately for future SoC revisions?
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ast2600.c
> @@ -0,0 +1,1018 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ASPEED AST2600 new register set I2C controller driver
> + *
> + * Copyright (C) ASPEED Technology Inc.
You'll need a year here.
> +/* 0x10 : I2CM Controller Interrupt Control Register */
> +#define AST2600_I2CM_IER 0x10
> +/* 0x14 : I2CM Controller Interrupt Status Register : WC */
> +#define AST2600_I2CM_ISR 0x14
> +
> +#define AST2600_I2CM_PKT_TIMEOUT BIT(18)
> +#define AST2600_I2CM_PKT_ERROR BIT(17)
> +#define AST2600_I2CM_PKT_DONE BIT(16)
> +
> +#define AST2600_I2CM_BUS_RECOVER_FAIL BIT(15)
> +#define AST2600_I2CM_SDA_DL_TO BIT(14)
> +#define AST2600_I2CM_BUS_RECOVER BIT(13)
> +#define AST2600_I2CM_SMBUS_ALT BIT(12)
_ALT implies "alternate", can you make this _ALERT?
> +enum xfer_mode {
> + BYTE_MODE,
> + BUFF_MODE,
> + DMA_MODE,
> +};
> +
> +struct ast2600_i2c_bus {
> + struct i2c_adapter adap;
> + struct device *dev;
> + void __iomem *reg_base;
> + struct regmap *global_regs;
> + struct clk *clk;
> + struct i2c_timings timing_info;
> + struct completion cmd_complete;
> + struct i2c_msg *msgs;
> + u8 *controller_dma_safe_buf;
> + dma_addr_t controller_dma_addr;
> + u32 apb_clk;
> + u32 timeout;
> + int irq;
> + int cmd_err;
> + int msgs_index;
> + int msgs_count;
> + int controller_xfer_cnt;
> + size_t buf_index;
> + size_t buf_size;
> + enum xfer_mode mode;
> + bool multi_master;
> + /* Buffer mode */
> + void __iomem *buf_base;
> + struct i2c_smbus_alert_setup alert_data;
You don't seem to use alert_data?
> +static int ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus)
> +{
> + u32 state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> + int ret = 0;
> + u32 ctrl;
> + int r;
> +
> + dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr, state);
> +
> + ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> + /* Disable controller */
> + writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN),
> + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> + writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) | AST2600_I2CC_MASTER_EN,
> + i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
Maybe split the readl out to make this simpler (and not wrapped).
> +
> + reinit_completion(&i2c_bus->cmd_complete);
> + i2c_bus->cmd_err = 0;
> +
> + /* Check 0x14's SDA and SCL status */
What is 0x14?
> + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> + if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state & AST2600_I2CC_SCL_LINE_STS)) {
> + writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> + r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> + if (r == 0) {
> + dev_dbg(i2c_bus->dev, "recovery timed out\n");
> + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> + return -ETIMEDOUT;
> + } else if (i2c_bus->cmd_err) {
> + dev_dbg(i2c_bus->dev, "recovery error\n");
> + ret = -EPROTO;
> + }
One of these paths restores controller state, the other does not. Is
that okay?
(if both should be restoring it, you may want to just set ret and use
the common exit path)
> + }
> +
> + /* Recovery done */
> + state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> + if (state & AST2600_I2CC_BUS_BUSY_STS) {
> + dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n", state);
> + ret = -EPROTO;
> + }
This (including the SCL/SDA state detection above) only seems sensible
if this controller is the only controller on the bus, as your conditions
depend on sampled bus state.
If the recovery command completed successfully, but the bus is
coincidentally busy due to some other controller, this will fail.
> +
> + /* restore original controller setting */
> + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> + return ret;
> +}
[...]
> +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;
> + i2c_bus->buf_index = 0;
> +
> + if (msg->flags & I2C_M_RD) {
> + if (i2c_bus->mode == DMA_MODE)
> + return ast2600_i2c_setup_dma_rx(AST2600_I2CM_START_CMD, i2c_bus);
> + else if (i2c_bus->mode == BUFF_MODE)
> + return ast2600_i2c_setup_buff_rx(AST2600_I2CM_START_CMD, i2c_bus);
> + else
> + return ast2600_i2c_setup_byte_rx(AST2600_I2CM_START_CMD, i2c_bus);
> + } else {
> + if (i2c_bus->mode == DMA_MODE)
> + return ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus);
> + else if (i2c_bus->mode == BUFF_MODE)
> + return ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus);
> + else
> + return ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus);
Would ->setup_rx() and ->setup_tx() ops be helpful? You have quite a few
instances of this same three-branch condition.
If so, you may also want symmetric cleanup ops too.
> + }
> +}
> +
> +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 -EBUSY;
> + if (irq_status & (AST2600_I2CM_ABNORMAL))
> + return -EPROTO;
> +
> + return 0;
> +}
> +
> +static void ast2600_i2c_controller_package_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
minor: package -> packet
> +{
> + 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 = -ENOMEM;
> + 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:
> + if (i2c_bus->mode == DMA_MODE)
> + xfer_len = AST2600_I2C_GET_TX_DMA_LEN(readl(i2c_bus->reg_base +
> + AST2600_I2CM_DMA_LEN_STS));
> + else if (i2c_bus->mode == BUFF_MODE)
> + xfer_len = AST2600_I2CC_GET_TX_BUF_LEN(readl(i2c_bus->reg_base +
> + AST2600_I2CC_BUFF_CTRL));
> + else
> + xfer_len = 1;
> +
> + i2c_bus->controller_xfer_cnt += xfer_len;
> +
> + if (i2c_bus->controller_xfer_cnt == msg->len) {
> + if (i2c_bus->mode == DMA_MODE) {
> + dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
> + msg->len, DMA_TO_DEVICE);
> + i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg,
> + true);
> + i2c_bus->controller_dma_safe_buf = NULL;
> + }
> + 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 = -ENOMEM;
> + complete(&i2c_bus->cmd_complete);
> + }
> + }
> + } else {
> + if (i2c_bus->mode == DMA_MODE)
> + ast2600_i2c_setup_dma_tx(0, i2c_bus);
> + else if (i2c_bus->mode == BUFF_MODE)
> + ast2600_i2c_setup_buff_tx(0, i2c_bus);
> + else
> + ast2600_i2c_setup_byte_tx(0, i2c_bus);
> + }
You have quite a bit of repetition across the start and the re-start
cases. Perhaps you can combine these into helpers?
> + break;
> + case AST2600_I2CM_RX_DONE:
> + case AST2600_I2CM_RX_DONE | AST2600_I2CM_NORMAL_STOP:
> + /* do next rx */
> + if (i2c_bus->mode == DMA_MODE) {
> + xfer_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
> + AST2600_I2CM_DMA_LEN_STS));
> + } else if (i2c_bus->mode == BUFF_MODE) {
> + 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 + 0x10 + i);
> + } else {
> + xfer_len = 1;
> + msg->buf[i2c_bus->controller_xfer_cnt] =
> + AST2600_I2CC_GET_RX_BUFF(readl(i2c_bus->reg_base +
> + AST2600_I2CC_STS_AND_BUFF));
> + }
> +
> + 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) {
> + if (i2c_bus->mode == DMA_MODE) {
> + dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
> + msg->len, DMA_FROM_DEVICE);
> + i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf,
> + msg, true);
> + i2c_bus->controller_dma_safe_buf = NULL;
> + }
> +
> + 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 = -ENOMEM;
> + complete(&i2c_bus->cmd_complete);
> + }
> + }
> + } else {
> + if (i2c_bus->mode == DMA_MODE)
> + ast2600_i2c_setup_dma_rx(0, i2c_bus);
> + else if (i2c_bus->mode == BUFF_MODE)
> + ast2600_i2c_setup_buff_rx(0, i2c_bus);
> + else
> + ast2600_i2c_setup_byte_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_ALT;
> +
> + if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
minor: these conditions seem inverted. 'sts & ...' would be more clear.
> + 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) {
> + 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 (AST2600_I2CM_PKT_DONE & sts) {
> + ast2600_i2c_controller_package_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_I2CC_FUN_CTRL);
> + writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> + 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);
Can you add a comment about how this is safe (mainly: what conditions
would have lead to this timeout)? Doing a bus recovery - by performing
additional SCL cycles - may not be considered good controller behaviour
on an entirely-local error condition.
> +
> + 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:
> + if (i2c_bus->mode == DMA_MODE) {
> + if (i2c_bus->controller_dma_safe_buf) {
Minor: you can combine these conditions. You also seem to only set
controller_dma_safe_buf in DMA_MODE anyway, so maybe you can just check
that.
> + struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> +
> + if (msg->flags & I2C_M_RD)
> + dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
> + msg->len, DMA_FROM_DEVICE);
> + else
> + dma_unmap_single(i2c_bus->dev, i2c_bus->controller_dma_addr,
> + msg->len, DMA_TO_DEVICE);
> + i2c_put_dma_safe_msg_buf(i2c_bus->controller_dma_safe_buf, msg, true);
> + i2c_bus->controller_dma_safe_buf = NULL;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus)
> +{
> + struct platform_device *pdev = to_platform_device(i2c_bus->dev);
> + u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN;
> +
> + /* I2C Reset */
> + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> + i2c_bus->multi_master = device_property_read_bool(&pdev->dev, "multi-master");
I would suggest doing this in the same place as the other DT parsing.
> + 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);
> +}
> +
> +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,
> +};
> +
> +int ast2600_i2c_probe(const struct of_device_id *match, struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct ast2600_i2c_bus *i2c_bus;
> + struct reset_control *rst;
> + const char *xfer_mode;
> + struct resource *res;
> + u32 global_ctrl;
> + int ret;
> +
> + 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->mode = BUFF_MODE;
> + if (!device_property_read_string(dev, "aspeed,transfer-mode", &xfer_mode)) {
> + if (!strcmp(xfer_mode, "dma"))
> + i2c_bus->mode = DMA_MODE;
> + else if (!strcmp(xfer_mode, "byte"))
> + i2c_bus->mode = BYTE_MODE;
> + else
> + i2c_bus->mode = BUFF_MODE;
> + }
> +
> + if (i2c_bus->mode == BUFF_MODE) {
> + i2c_bus->buf_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
> + if (IS_ERR(i2c_bus->buf_base))
> + i2c_bus->mode = BYTE_MODE;
> + else
> + 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 /= 1024;
> +
> + 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.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);
> +
> + ast2600_i2c_init(i2c_bus);
If anything past this point fails, we'll leave the controller enabled.
Is that a problem?
> +
> + ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
> + dev_name(dev), i2c_bus);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Unable to request irq %d\n", i2c_bus->irq);
> +
> + writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> + i2c_bus->reg_base + AST2600_I2CM_IER);
> +
> + ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ast2600_i2c_probe);
> +
> +void ast2600_i2c_remove(struct platform_device *pdev)
> +{
> + struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
> +
> + /* Disable everything. */
> + writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> + writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> +}
> +EXPORT_SYMBOL_GPL(ast2600_i2c_remove);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen at aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
> +MODULE_LICENSE("GPL");
Cheers,
Jeremy
More information about the Linux-aspeed
mailing list