diff mbox series

[1/1] i2c:aspeed:support ast2600 i2c new register mode driver

Message ID 20230109082940.3400780-2-ryan_chen@aspeedtech.com
State Superseded
Delegated to: Heiko Schocher
Headers show
Series Add ASPEED AST2600 I2C new controller driver | expand

Commit Message

Ryan Chen Jan. 9, 2023, 8:29 a.m. UTC 
Add i2c new register mode driver to support AST2600 i2c
new register mode. AST2600 i2c controller have legacy and
new register mode. The new register mode have global register
support 4 base clock for scl clock selection, and new clock
divider mode.

Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com>
---
 MAINTAINERS               |   6 +
 drivers/i2c/Kconfig       |   7 +
 drivers/i2c/Makefile      |   1 +
 drivers/i2c/ast2600_i2c.c | 480 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 494 insertions(+)
 create mode 100644 drivers/i2c/ast2600_i2c.c

Comments

Simon Glass Jan. 9, 2023, 7:47 p.m. UTC | #1 
Hi Ryan_chen,

On Mon, 9 Jan 2023 at 01:30, ryan_chen <ryan_chen@aspeedtech.com> wrote:
>
> Add i2c new register mode driver to support AST2600 i2c
> new register mode. AST2600 i2c controller have legacy and
> new register mode. The new register mode have global register
> support 4 base clock for scl clock selection, and new clock
> divider mode.
>
> Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com>
> ---
>  MAINTAINERS               |   6 +
>  drivers/i2c/Kconfig       |   7 +
>  drivers/i2c/Makefile      |   1 +
>  drivers/i2c/ast2600_i2c.c | 480 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 494 insertions(+)
>  create mode 100644 drivers/i2c/ast2600_i2c.c

This generally looks OK, but I have quite a few minor comments, and
one major one: could/should this driver be an update to the existing
one, instead? That is the short of thing that really should be in your
commit message.

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3fc4cd0f12..1cf54f0b4e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -769,6 +769,12 @@ S: Maintained
>  F:     drivers/pci/pcie_phytium.c
>  F:     arch/arm/dts/phytium-durian.dts
>
> +ASPEED AST2600 I2C DRIVER
> +M:     Ryan Chen <ryan_chen@aspeedtech.com>
> +R:     Aspeed BMC SW team <BMC-SW@aspeedtech.com>
> +S:     Maintained
> +F:     drivers/i2c/ast2600_i2c.c
> +
>  ASPEED FMC SPI DRIVER
>  M:     Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
>  M:     Cédric Le Goater <clg@kaod.org>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index 08b6c7bdcc..34f507fb3b 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -221,6 +221,13 @@ config SYS_I2C_DW
>           controller is used in various SoCs, e.g. the ST SPEAr, Altera
>           SoCFPGA, Synopsys ARC700 and some Intel x86 SoCs.
>
> +config SYS_I2C_AST2600
> +    bool "AST2600 I2C Controller"
> +    depends on DM_I2C && ARCH_ASPEED
> +    help
> +      Say yes here to select AST2600 I2C Host Controller. The driver
> +      support AST2600 I2C new mode register.

What speeds does it support? Please add at least 3 lines of info.

A link to the datasheet would help too, either here or in doc/

> +
>  config SYS_I2C_ASPEED
>         bool "Aspeed I2C Controller"
>         depends on DM_I2C && ARCH_ASPEED
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index 920aafb91c..89db2d8e37 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_$(SPL_)I2C_CROS_EC_LDO) += cros_ec_ldo.o
>
>  obj-$(CONFIG_$(SPL_)SYS_I2C_LEGACY) += i2c_core.o
>  obj-$(CONFIG_SYS_I2C_ASPEED) += ast_i2c.o
> +obj-$(CONFIG_SYS_I2C_AST2600) += ast2600_i2c.o
>  obj-$(CONFIG_SYS_I2C_AT91) += at91_i2c.o
>  obj-$(CONFIG_SYS_I2C_CADENCE) += i2c-cdns.o
>  obj-$(CONFIG_SYS_I2C_CA) += i2c-cortina.o
> diff --git a/drivers/i2c/ast2600_i2c.c b/drivers/i2c/ast2600_i2c.c
> new file mode 100644
> index 0000000000..52aea460ac
> --- /dev/null
> +++ b/drivers/i2c/ast2600_i2c.c
> @@ -0,0 +1,480 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright ASPEED Technology Inc.
> + */
> +#include <linux/iopoll.h>

The ordering is off here. Please see

https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <i2c.h>
> +#include <asm/io.h>
> +#include <regmap.h>
> +#include <reset.h>
> +
> +struct ast2600_i2c_regs {
> +       u32 fun_ctrl;
> +       u32 ac_timing;
> +       u32 trx_buff;
> +       u32 icr;
> +       u32 ier;
> +       u32 isr;
> +       u32 cmd_sts;
> +};
> +
> +/* 0x00 : I2CC Master/Slave Function Control Register  */
> +#define AST2600_I2CC_SLAVE_ADDR_RX_EN  BIT(20)

Perhaps this should go in the .h file like the other drivers?

> +#define AST2600_I2CC_MASTER_RETRY_MASK GENMASK(19, 18)
> +#define AST2600_I2CC_MASTER_RETRY(x)   (((x) & GENMASK(1, 0)) << 18)

Can you drop unused things?

> +#define AST2600_I2CC_BUS_AUTO_RELEASE  BIT(17)
> +#define AST2600_I2CC_M_SDA_LOCK_EN             BIT(16)
> +#define AST2600_I2CC_MULTI_MASTER_DIS  BIT(15)
> +#define AST2600_I2CC_M_SCL_DRIVE_EN            BIT(14)
> +#define AST2600_I2CC_MSB_STS                   BIT(9)
> +#define AST2600_I2CC_SDA_DRIVE_1T_EN   BIT(8)
> +#define AST2600_I2CC_M_SDA_DRIVE_1T_EN BIT(7)
> +#define AST2600_I2CC_M_HIGH_SPEED_EN   BIT(6)
> +/* reserver 5 : 2 */

reserved?

> +#define AST2600_I2CC_SLAVE_EN                  BIT(1)
> +#define AST2600_I2CC_MASTER_EN                 BIT(0)
> +
> +/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +/* Base register value. These bits are always set by the driver. */
> +#define I2CD_CACTC_BASE                0xfff00300
> +#define I2CD_TCKHIGH_SHIFT     16
> +#define I2CD_TCKLOW_SHIFT      12
> +#define I2CD_THDDAT_SHIFT      10
> +#define I2CD_TO_DIV_SHIFT      8
> +#define I2CD_BASE_DIV_SHIFT    0
> +
> +/* 0x08 : I2CC Master/Slave Transmit/Receive Byte Buffer Register */
> +#define AST2600_I2CC_TX_DIR_MASK               GENMASK(31, 29)
> +#define AST2600_I2CC_SDA_OE                            BIT(28)
> +#define AST2600_I2CC_SDA_O                             BIT(27)
> +#define AST2600_I2CC_SCL_OE                            BIT(26)
> +#define AST2600_I2CC_SCL_O                             BIT(25)
> +
> +#define AST2600_I2CC_SCL_LINE_STS              BIT(18)
> +#define AST2600_I2CC_SDA_LINE_STS              BIT(17)
> +#define AST2600_I2CC_BUS_BUSY_STS              BIT(16)
> +#define AST2600_I2CC_GET_RX_BUFF(x)            (((x) >> 8) & GENMASK(7, 0))

Can you drop the AST2600 prefix here? It is too long and we know it is
this driver.

> +
> +/* 0x10 : I2CM Master Interrupt Control Register */
> +/* 0x14 : I2CM Master Interrupt Status Register  */
> +#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)
> +
> +#define AST2600_I2CM_SCL_LOW_TO                        BIT(6)
> +#define AST2600_I2CM_ABNORMAL                  BIT(5)
> +#define AST2600_I2CM_NORMAL_STOP               BIT(4)
> +#define AST2600_I2CM_ARBIT_LOSS                        BIT(3)
> +#define AST2600_I2CM_RX_DONE                   BIT(2)
> +#define AST2600_I2CM_TX_NAK                            BIT(1)
> +#define AST2600_I2CM_TX_ACK                            BIT(0)
> +
> +/* 0x18 : I2CM Master Command/Status Register   */
> +#define AST2600_I2CM_PKT_ADDR(x)               (((x) & GENMASK(6, 0)) << 24)
> +#define AST2600_I2CM_PKT_EN                            BIT(16)
> +#define AST2600_I2CM_SDA_OE_OUT_DIR            BIT(15)
> +#define AST2600_I2CM_SDA_O_OUT_DIR             BIT(14)
> +#define AST2600_I2CM_SCL_OE_OUT_DIR            BIT(13)
> +#define AST2600_I2CM_SCL_O_OUT_DIR             BIT(12)
> +#define AST2600_I2CM_RECOVER_CMD_EN            BIT(11)
> +
> +#define AST2600_I2CM_RX_DMA_EN                 BIT(9)
> +#define AST2600_I2CM_TX_DMA_EN                 BIT(8)
> +/* Command Bit */
> +#define AST2600_I2CM_RX_BUFF_EN                        BIT(7)
> +#define AST2600_I2CM_TX_BUFF_EN                        BIT(6)
> +#define AST2600_I2CM_STOP_CMD                  BIT(5)
> +#define AST2600_I2CM_RX_CMD_LAST               BIT(4)
> +#define AST2600_I2CM_RX_CMD                            BIT(3)
> +
> +#define AST2600_I2CM_TX_CMD                            BIT(1)
> +#define AST2600_I2CM_START_CMD                 BIT(0)
> +
> +#define I2C_TIMEOUT_US 100000
> +/*
> + * Device private data
> + */

single-line comment style is

/* Device-private data */

> +struct ast2600_i2c_priv {
> +       struct clk clk;
> +       struct ast2600_i2c_regs *regs;
> +       void __iomem *global;
> +};
> +
> +static int ast2600_i2c_read_data(struct ast2600_i2c_priv *priv, u8 chip_addr,
> +                                u8 *buffer, size_t len, bool send_stop)

Please add a full comment for this one

> +{
> +       int rx_cnt, ret = 0;
> +       u32 cmd, isr;
> +
> +       for (rx_cnt = 0; rx_cnt < len; rx_cnt++, buffer++) {
> +               cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(chip_addr) |
> +                     AST2600_I2CM_RX_CMD;
> +               if (!rx_cnt)
> +                       cmd |= AST2600_I2CM_START_CMD;
> +
> +               if ((len - 1) == rx_cnt)
> +                       cmd |= AST2600_I2CM_RX_CMD_LAST;
> +
> +               if (send_stop && ((len - 1) == rx_cnt))
> +                       cmd |= AST2600_I2CM_STOP_CMD;
> +
> +               writel(cmd, &priv->regs->cmd_sts);
> +
> +               ret = readl_poll_timeout(&priv->regs->isr, isr,
> +                                        isr & AST2600_I2CM_PKT_DONE,
> +                                        I2C_TIMEOUT_US);
> +               if (ret)
> +                       return -ETIMEDOUT;
> +
> +               *buffer =
> +                       AST2600_I2CC_GET_RX_BUFF(readl(&priv->regs->trx_buff));

^^ too long

> +
> +               writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
> +
> +               if (isr & AST2600_I2CM_TX_NAK)
> +                       return -EREMOTEIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ast2600_i2c_write_data(struct ast2600_i2c_priv *priv, u8 chip_addr,
> +                                 u8 *buffer, size_t len, bool send_stop)

and this

> +{
> +       int tx_cnt, ret = 0;
> +       u32 cmd, isr;
> +
> +       if (!len) {
> +               cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(chip_addr) |
> +                     AST2600_I2CM_START_CMD;
> +               writel(cmd, &priv->regs->cmd_sts);
> +               ret = readl_poll_timeout(&priv->regs->isr, isr,
> +                                        isr & AST2600_I2CM_PKT_DONE,
> +                                        I2C_TIMEOUT_US);
> +               if (ret)
> +                       return -ETIMEDOUT;
> +
> +               writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
> +
> +               if (isr & AST2600_I2CM_TX_NAK)
> +                       return -EREMOTEIO;
> +       }
> +
> +       for (tx_cnt = 0; tx_cnt < len; tx_cnt++, buffer++) {
> +               cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(chip_addr);
> +               cmd |= AST2600_I2CM_TX_CMD;
> +
> +               if (!tx_cnt)
> +                       cmd |= AST2600_I2CM_START_CMD;
> +
> +               if (send_stop && ((len - 1) == tx_cnt))
> +                       cmd |= AST2600_I2CM_STOP_CMD;
> +
> +               writel(*buffer, &priv->regs->trx_buff);
> +               writel(cmd, &priv->regs->cmd_sts);
> +               ret = readl_poll_timeout(&priv->regs->isr, isr,
> +                                        isr & AST2600_I2CM_PKT_DONE,
> +                                        I2C_TIMEOUT_US);
> +               if (ret)
> +                       return -ETIMEDOUT;
> +
> +               writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
> +
> +               if (isr & AST2600_I2CM_TX_NAK)
> +                       return -EREMOTEIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ast2600_i2c_deblock(struct udevice *dev)
> +{
> +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> +       u32 csr = readl(&priv->regs->cmd_sts);
> +       u32 isr;
> +       int ret;
> +
> +       //reinit

/* reinit */

> +       writel(0, &priv->regs->fun_ctrl);
> +       /* Enable Master Mode. Assuming single-master */
> +       writel(AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN |
> +                      AST2600_I2CC_MULTI_MASTER_DIS,
> +              &priv->regs->fun_ctrl);
> +

[..]

> +static int ast2600_i2c_set_speed(struct udevice *dev, unsigned int speed)
> +{
> +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> +       unsigned long base_clk1, base_clk2, base_clk3, base_clk4;
> +       int baseclk_idx;
> +       u32 clk_div_reg;
> +       u32 apb_clk;
> +       u32 scl_low;
> +       u32 scl_high;
> +       int divisor;
> +       int inc = 0;
> +       u32 data;
> +
> +       debug("Setting speed for I2C%d to <%u>\n", dev->seq_, speed);
> +       if (!speed) {
> +               debug("No valid speed specified\n");
> +               return -EINVAL;
> +       }
> +
> +       apb_clk = clk_get_rate(&priv->clk);
> +
> +       clk_div_reg = readl(priv->global + 0x10);
> +       base_clk1 = (apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2);

Can apb_clk * 10 go in a local var to reduce duplication?

> +       base_clk2 =
> +               (apb_clk * 10) / (((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2);
> +       base_clk3 = (apb_clk * 10) /
> +                   (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2);
> +       base_clk4 = (apb_clk * 10) /
> +                   (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2);

Normally we like to define these fields, like:

#define CLK1_DIV_SHIFT 0
#define CLK1_DIV_MASK (0xff >> CLK1_DIV_SHIFT)
#define CLK2_DIV_SHIFT 8
#define CLK2_DIV_MASK (0xff >> CLK2_DIV_SHIFT)

etc.

> +
> +       if ((apb_clk / speed) <= 32) {
> +               baseclk_idx = 0;
> +               divisor = DIV_ROUND_UP(apb_clk, speed);
> +       } else if ((base_clk1 / speed) <= 32) {
> +               baseclk_idx = 1;
> +               divisor = DIV_ROUND_UP(base_clk1, speed);
> +       } else if ((base_clk2 / speed) <= 32) {
> +               baseclk_idx = 2;
> +               divisor = DIV_ROUND_UP(base_clk2, speed);
> +       } else if ((base_clk3 / speed) <= 32) {
> +               baseclk_idx = 3;
> +               divisor = DIV_ROUND_UP(base_clk3, speed);
> +       } else {
> +               baseclk_idx = 4;
> +               divisor = DIV_ROUND_UP(base_clk4, speed);
> +               inc = 0;
> +               while ((divisor + inc) > 32) {
> +                       inc |= divisor & 0x1;
> +                       divisor >>= 1;
> +                       baseclk_idx++;
> +               }
> +               divisor += inc;
> +       }
> +       divisor = min_t(int, divisor, 32);
> +       baseclk_idx &= 0xf;

What happens if baseclk_idx is >= 16 ?

> +       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);

drop extra () around baseclk_idx

> +       /* Set AC Timing */
> +       writel(data, &priv->regs->ac_timing);
> +
> +       return 0;
> +}
> +
> +static int ast2600_i2c_probe(struct udevice *dev)
> +{
> +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> +       struct udevice *misc_dev;
> +       int ret;
> +
> +       /* find global base address */
> +       ret = uclass_get_device_by_driver(UCLASS_MISC,
> +                                         DM_DRIVER_GET(aspeed_i2c_global),
> +                                         &misc_dev);

But isn't this the parent device of this one? Just use device_get_parent(dev)

Also misc_dev is not a great name. How about glob_dev ?

> +       if (ret) {
> +               debug("i2c global not defined\n");
> +               return ret;
> +       }
> +
> +       priv->global = devfdt_get_addr_ptr(misc_dev);

dev_read_addr()

> +       if (IS_ERR(priv->global)) {
> +               debug("%s(): can't get global\n", __func__);
> +               return PTR_ERR(priv->global);
> +       }
> +
> +       /* Reset device */
> +       writel(0, &priv->regs->fun_ctrl);

When you have functions with a lot of priv->regs things it makes sense
to add a local 'regs' variable.

> +       /* Enable Master Mode. Assuming single-master */
> +       writel(AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN |
> +                      AST2600_I2CC_MULTI_MASTER_DIS,
> +              &priv->regs->fun_ctrl);
> +
> +       writel(0, &priv->regs->ier);
> +       /* Clear Interrupt */
> +       writel(~0, &priv->regs->isr);
> +
> +       return 0;
> +}
> +
> +static int ast2600_i2c_of_to_plat(struct udevice *dev)
> +{
> +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> +       int ret;
> +
> +       priv->regs = dev_read_addr_ptr(dev);
> +       if (!(priv->regs))

Drop extra ()

> +               return -EINVAL;
> +
> +       ret = clk_get_by_index(dev, 0, &priv->clk);
> +       if (ret < 0) {
> +               debug("%s: Can't get clock for %s: %d\n", __func__, dev->name,
> +                     ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct dm_i2c_ops ast2600_i2c_ops = {
> +       .xfer = ast2600_i2c_xfer,
> +       .deblock = ast2600_i2c_deblock,
> +       .set_bus_speed = ast2600_i2c_set_speed,
> +};
> +
> +static const struct udevice_id ast2600_i2c_ids[] = {
> +       { .compatible = "aspeed,ast2600-i2c" },
> +       {},
> +};
> +
> +U_BOOT_DRIVER(ast2600_i2c) = {
> +       .name = "ast2600_i2c",
> +       .id = UCLASS_I2C,
> +       .of_match = ast2600_i2c_ids,
> +       .probe = ast2600_i2c_probe,
> +       .of_to_plat = ast2600_i2c_of_to_plat,
> +       .priv_auto = sizeof(struct ast2600_i2c_priv),
> +       .ops = &ast2600_i2c_ops,
> +};
> +
> +#define AST2600_I2CG_ISR                       0x00
> +#define AST2600_I2CG_SLAVE_ISR         0x04
> +#define AST2600_I2CG_OWNER                 0x08
> +#define AST2600_I2CG_CTRL                  0x0C
> +#define AST2600_I2CG_CLK_DIV_CTRL      0x10
> +
> +#define AST2600_I2CG_SLAVE_PKT_NAK         BIT(4)
> +#define AST2600_I2CG_M_S_SEPARATE_INTR BIT(3)
> +#define AST2600_I2CG_CTRL_NEW_REG          BIT(2)
> +#define AST2600_I2CG_CTRL_NEW_CLK_DIV  BIT(1)
> +
> +#define AST2600_GLOBAL_INIT                                    \
> +                       (AST2600_I2CG_SLAVE_PKT_NAK |   \
> +                       AST2600_I2CG_CTRL_NEW_REG |             \
> +                       AST2600_I2CG_CTRL_NEW_CLK_DIV)
> +#define I2CCG_DIV_CTRL 0xC6411208

Please use lower-case hex consistently

Hmm these should really go in the header file too?

> +
> +struct ast2600_i2c_global_priv {
> +       void __iomem *regs;
> +       struct reset_ctl reset;
> +};
> +
> +/*
> + * APB clk : 100Mhz
> + * div  : scl       : baseclk [APB/((div/2) + 1)] : tBuf [1/bclk * 16]
> + * I2CG10[31:24] base clk4 for i2c auto recovery timeout counter (0xC6)
> + * I2CG10[23:16] base clk3 for Standard-mode (100Khz) min tBuf 4.7us
> + * 0x3c : 100.8Khz  : 3.225Mhz                    : 4.96us
> + * 0x3d : 99.2Khz   : 3.174Mhz                    : 5.04us
> + * 0x3e : 97.65Khz  : 3.125Mhz                    : 5.12us
> + * 0x40 : 97.75Khz  : 3.03Mhz                     : 5.28us
> + * 0x41 : 99.5Khz   : 2.98Mhz                     : 5.36us (default)
> + * I2CG10[15:8] base clk2 for Fast-mode (400Khz) min tBuf 1.3us
> + * 0x12 : 400Khz    : 10Mhz                       : 1.6us
> + * I2CG10[7:0] base clk1 for Fast-mode Plus (1Mhz) min tBuf 0.5us
> + * 0x08 : 1Mhz      : 20Mhz                       : 0.8us
> + */
> +
> +static int aspeed_i2c_global_probe(struct udevice *dev)
> +{
> +       struct ast2600_i2c_global_priv *i2c_global = dev_get_priv(dev);
> +       int ret = 0;
> +
> +       i2c_global->regs = dev_read_addr_ptr(dev);
> +       if (!(i2c_global->regs))

again too many (). Please fix globally

> +               return -EINVAL;
> +
> +       debug("%s(dev=%p)\n", __func__, dev);
> +
> +       ret = reset_get_by_index(dev, 0, &i2c_global->reset);
> +       if (ret) {
> +               printf("%s(): Failed to get reset signal\n", __func__);

log_err() if you want an error. Try to avoid using __func__ - use
log...() instead

> +               return ret;
> +       }
> +
> +       reset_deassert(&i2c_global->reset);

Can this fail?

> +
> +       writel(AST2600_GLOBAL_INIT, i2c_global->regs +
> +               AST2600_I2CG_CTRL);
> +       writel(I2CCG_DIV_CTRL, i2c_global->regs +
> +               AST2600_I2CG_CLK_DIV_CTRL);
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id aspeed_i2c_global_ids[] = {
> +       {       .compatible = "aspeed,ast2600-i2c-global",      },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(aspeed_i2c_global) = {
> +       .name           = "aspeed_i2c_global",
> +       .id                     = UCLASS_MISC,
> +       .of_match       = aspeed_i2c_global_ids,
> +       .probe          = aspeed_i2c_global_probe,
> +       .priv_auto  = sizeof(struct ast2600_i2c_global_priv),
> +};
> +
> --
> 2.34.1
>

Regards,
Simon
Ryan Chen Jan. 10, 2023, 2:41 a.m. UTC | #2 
Hello Simon,
	Thank your feedback.

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Tuesday, January 10, 2023 3:47 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>
> Cc: Heiko Schocher <hs@denx.de>; BMC-SW <BMC-SW@aspeedtech.com>;
> u-boot@lists.denx.de
> Subject: Re: [PATCH 1/1] i2c:aspeed:support ast2600 i2c new register mode
> driver
> 
>   Hi Ryan_chen,
> 
> On Mon, 9 Jan 2023 at 01:30, ryan_chen <ryan_chen@aspeedtech.com>
> wrote:
> >
> > Add i2c new register mode driver to support AST2600 i2c new register
> > mode. AST2600 i2c controller have legacy and new register mode. The
> > new register mode have global register support 4 base clock for scl
> > clock selection, and new clock divider mode.
> >
> > Signed-off-by: ryan_chen <ryan_chen@aspeedtech.com>
> > ---
> >  MAINTAINERS               |   6 +
> >  drivers/i2c/Kconfig       |   7 +
> >  drivers/i2c/Makefile      |   1 +
> >  drivers/i2c/ast2600_i2c.c | 480
> > ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 494 insertions(+)
> >  create mode 100644 drivers/i2c/ast2600_i2c.c
> 
> This generally looks OK, but I have quite a few minor comments, and one
> major one: could/should this driver be an update to the existing one, instead?
> That is the short of thing that really should be in your commit message.
> 

This driver is now driver not be an update to the exiting one.

> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 3fc4cd0f12..1cf54f0b4e
> > 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -769,6 +769,12 @@ S: Maintained
> >  F:     drivers/pci/pcie_phytium.c
> >  F:     arch/arm/dts/phytium-durian.dts
> >
> > +ASPEED AST2600 I2C DRIVER
> > +M:     Ryan Chen <ryan_chen@aspeedtech.com>
> > +R:     Aspeed BMC SW team <BMC-SW@aspeedtech.com>
> > +S:     Maintained
> > +F:     drivers/i2c/ast2600_i2c.c
> > +
> >  ASPEED FMC SPI DRIVER
> >  M:     Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> >  M:     Cédric Le Goater <clg@kaod.org>
> > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index
> > 08b6c7bdcc..34f507fb3b 100644
> > --- a/drivers/i2c/Kconfig
> > +++ b/drivers/i2c/Kconfig
> > @@ -221,6 +221,13 @@ config SYS_I2C_DW
> >           controller is used in various SoCs, e.g. the ST SPEAr, Altera
> >           SoCFPGA, Synopsys ARC700 and some Intel x86 SoCs.
> >
> > +config SYS_I2C_AST2600
> > +    bool "AST2600 I2C Controller"
> > +    depends on DM_I2C && ARCH_ASPEED
> > +    help
> > +      Say yes here to select AST2600 I2C Host Controller. The driver
> > +      support AST2600 I2C new mode register.
> 
> What speeds does it support? 

Will modify by following.
+config SYS_I2C_AST2600
+    bool "AST2600 I2C Controller"
+    depends on DM_I2C && ARCH_ASPEED
+    help
+      Say yes here to select AST2600 I2C Host Controller. The driver
+      support AST2600 I2C new mode register. This I2C controller supports:
+      -Standard-mode (up to 100 kHz)
+      -Fast-mode (up to 400 kHz)
+      -Fast-mode Plus (up to 1 MHz)

>Please add at least 3 lines of info.
Sorry, what do you mean about 3 lines of info?
The i2c have two lines, SDA/SCL only.

> 
> A link to the datasheet would help too, either here or in doc/
> 
> > +
> >  config SYS_I2C_ASPEED
> >         bool "Aspeed I2C Controller"
> >         depends on DM_I2C && ARCH_ASPEED diff --git
> > a/drivers/i2c/Makefile b/drivers/i2c/Makefile index
> > 920aafb91c..89db2d8e37 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_$(SPL_)I2C_CROS_EC_LDO) +=
> > cros_ec_ldo.o
> >
> >  obj-$(CONFIG_$(SPL_)SYS_I2C_LEGACY) += i2c_core.o
> >  obj-$(CONFIG_SYS_I2C_ASPEED) += ast_i2c.o
> > +obj-$(CONFIG_SYS_I2C_AST2600) += ast2600_i2c.o
> >  obj-$(CONFIG_SYS_I2C_AT91) += at91_i2c.o
> >  obj-$(CONFIG_SYS_I2C_CADENCE) += i2c-cdns.o
> >  obj-$(CONFIG_SYS_I2C_CA) += i2c-cortina.o diff --git
> > a/drivers/i2c/ast2600_i2c.c b/drivers/i2c/ast2600_i2c.c new file mode
> > 100644 index 0000000000..52aea460ac
> > --- /dev/null
> > +++ b/drivers/i2c/ast2600_i2c.c
> > @@ -0,0 +1,480 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright ASPEED Technology Inc.
> > + */
> > +#include <linux/iopoll.h>
> 
> The ordering is off here. Please see
> 
> https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

Will update

> 
> > +#include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <errno.h>
> > +#include <fdtdec.h>
> > +#include <i2c.h>
> > +#include <asm/io.h>
> > +#include <regmap.h>
> > +#include <reset.h>
> > +
> > +struct ast2600_i2c_regs {
> > +       u32 fun_ctrl;
> > +       u32 ac_timing;
> > +       u32 trx_buff;
> > +       u32 icr;
> > +       u32 ier;
> > +       u32 isr;
> > +       u32 cmd_sts;
> > +};
> > +
> > +/* 0x00 : I2CC Master/Slave Function Control Register  */ #define
> > +AST2600_I2CC_SLAVE_ADDR_RX_EN  BIT(20)
> 
> Perhaps this should go in the .h file like the other drivers?

Yes, will go for ast2600_i2c.h file
> 
> > +#define AST2600_I2CC_MASTER_RETRY_MASK GENMASK(19, 18)
> > +#define AST2600_I2CC_MASTER_RETRY(x)   (((x) & GENMASK(1, 0)) <<
> 18)
> 
> Can you drop unused things?

Those are register definition, could I keep this for avoid future used? 
> 
> > +#define AST2600_I2CC_BUS_AUTO_RELEASE  BIT(17)
> > +#define AST2600_I2CC_M_SDA_LOCK_EN             BIT(16)
> > +#define AST2600_I2CC_MULTI_MASTER_DIS  BIT(15)
> > +#define AST2600_I2CC_M_SCL_DRIVE_EN            BIT(14)
> > +#define AST2600_I2CC_MSB_STS                   BIT(9)
> > +#define AST2600_I2CC_SDA_DRIVE_1T_EN   BIT(8)
> > +#define AST2600_I2CC_M_SDA_DRIVE_1T_EN BIT(7)
> > +#define AST2600_I2CC_M_HIGH_SPEED_EN   BIT(6)
> > +/* reserver 5 : 2 */
> 
> reserved?
will update
/* reserved 5 : 2 */

> 
> > +#define AST2600_I2CC_SLAVE_EN                  BIT(1)
> > +#define AST2600_I2CC_MASTER_EN                 BIT(0)
> > +
> > +/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> > +/* Base register value. These bits are always set by the driver. */
> > +#define I2CD_CACTC_BASE                0xfff00300
> > +#define I2CD_TCKHIGH_SHIFT     16
> > +#define I2CD_TCKLOW_SHIFT      12
> > +#define I2CD_THDDAT_SHIFT      10
> > +#define I2CD_TO_DIV_SHIFT      8
> > +#define I2CD_BASE_DIV_SHIFT    0
> > +
> > +/* 0x08 : I2CC Master/Slave Transmit/Receive Byte Buffer Register */
> > +#define AST2600_I2CC_TX_DIR_MASK               GENMASK(31, 29)
> > +#define AST2600_I2CC_SDA_OE                            BIT(28)
> > +#define AST2600_I2CC_SDA_O                             BIT(27)
> > +#define AST2600_I2CC_SCL_OE                            BIT(26)
> > +#define AST2600_I2CC_SCL_O                             BIT(25)
> > +
> > +#define AST2600_I2CC_SCL_LINE_STS              BIT(18)
> > +#define AST2600_I2CC_SDA_LINE_STS              BIT(17)
> > +#define AST2600_I2CC_BUS_BUSY_STS              BIT(16)
> > +#define AST2600_I2CC_GET_RX_BUFF(x)            (((x) >> 8) &
> GENMASK(7, 0))
> 
> Can you drop the AST2600 prefix here? It is too long and we know it is this
> driver.
Sorry, Do you mean direct drop string "AST2600"?
Ex: AST2600_I2CC_SCL_LINE_STS => I2CC_SCL_LINE_STS
> 
> > +
> > +/* 0x10 : I2CM Master Interrupt Control Register */
> > +/* 0x14 : I2CM Master Interrupt Status Register  */
> > +#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)
> > +
> > +#define AST2600_I2CM_SCL_LOW_TO                        BIT(6)
> > +#define AST2600_I2CM_ABNORMAL                  BIT(5)
> > +#define AST2600_I2CM_NORMAL_STOP               BIT(4)
> > +#define AST2600_I2CM_ARBIT_LOSS                        BIT(3)
> > +#define AST2600_I2CM_RX_DONE                   BIT(2)
> > +#define AST2600_I2CM_TX_NAK                            BIT(1)
> > +#define AST2600_I2CM_TX_ACK                            BIT(0)
> > +
> > +/* 0x18 : I2CM Master Command/Status Register   */
> > +#define AST2600_I2CM_PKT_ADDR(x)               (((x) & GENMASK(6,
> 0)) << 24)
> > +#define AST2600_I2CM_PKT_EN                            BIT(16)
> > +#define AST2600_I2CM_SDA_OE_OUT_DIR            BIT(15)
> > +#define AST2600_I2CM_SDA_O_OUT_DIR             BIT(14)
> > +#define AST2600_I2CM_SCL_OE_OUT_DIR            BIT(13)
> > +#define AST2600_I2CM_SCL_O_OUT_DIR             BIT(12)
> > +#define AST2600_I2CM_RECOVER_CMD_EN            BIT(11)
> > +
> > +#define AST2600_I2CM_RX_DMA_EN                 BIT(9)
> > +#define AST2600_I2CM_TX_DMA_EN                 BIT(8)
> > +/* Command Bit */
> > +#define AST2600_I2CM_RX_BUFF_EN                        BIT(7)
> > +#define AST2600_I2CM_TX_BUFF_EN                        BIT(6)
> > +#define AST2600_I2CM_STOP_CMD                  BIT(5)
> > +#define AST2600_I2CM_RX_CMD_LAST               BIT(4)
> > +#define AST2600_I2CM_RX_CMD                            BIT(3)
> > +
> > +#define AST2600_I2CM_TX_CMD                            BIT(1)
> > +#define AST2600_I2CM_START_CMD                 BIT(0)
> > +
> > +#define I2C_TIMEOUT_US 100000
> > +/*
> > + * Device private data
> > + */
> 
> single-line comment style is
> 
> /* Device-private data */
> 
Will update

> > +struct ast2600_i2c_priv {
> > +       struct clk clk;
> > +       struct ast2600_i2c_regs *regs;
> > +       void __iomem *global;
> > +};
> > +
> > +static int ast2600_i2c_read_data(struct ast2600_i2c_priv *priv, u8
> chip_addr,
> > +                                u8 *buffer, size_t len, bool
> > +send_stop)
> 
> Please add a full comment for this one

Sorry, what comment for this function, it is i2c read/write function like others driver implement.
I don't see any comment in others driver implement.
Could you help point me what driver reference?

> 
> > +{
> > +       int rx_cnt, ret = 0;
> > +       u32 cmd, isr;
> > +
> > +       for (rx_cnt = 0; rx_cnt < len; rx_cnt++, buffer++) {
> > +               cmd = AST2600_I2CM_PKT_EN |
> AST2600_I2CM_PKT_ADDR(chip_addr) |
> > +                     AST2600_I2CM_RX_CMD;
> > +               if (!rx_cnt)
> > +                       cmd |= AST2600_I2CM_START_CMD;
> > +
> > +               if ((len - 1) == rx_cnt)
> > +                       cmd |= AST2600_I2CM_RX_CMD_LAST;
> > +
> > +               if (send_stop && ((len - 1) == rx_cnt))
> > +                       cmd |= AST2600_I2CM_STOP_CMD;
> > +
> > +               writel(cmd, &priv->regs->cmd_sts);
> > +
> > +               ret = readl_poll_timeout(&priv->regs->isr, isr,
> > +                                        isr &
> AST2600_I2CM_PKT_DONE,
> > +                                        I2C_TIMEOUT_US);
> > +               if (ret)
> > +                       return -ETIMEDOUT;
> > +
> > +               *buffer =
> > +
> > + AST2600_I2CC_GET_RX_BUFF(readl(&priv->regs->trx_buff));
> 
> ^^ too long
> 
tmp = readl(&priv->regs->trx_buff)
AST2600_I2CC_GET_RX_BUFF(tmp);
Is better ?
> > +
> > +               writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
> > +
> > +               if (isr & AST2600_I2CM_TX_NAK)
> > +                       return -EREMOTEIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int ast2600_i2c_write_data(struct ast2600_i2c_priv *priv, u8
> chip_addr,
> > +                                 u8 *buffer, size_t len, bool
> > +send_stop)
> 
> and this
> 
> > +{
> > +       int tx_cnt, ret = 0;
> > +       u32 cmd, isr;
> > +
> > +       if (!len) {
> > +               cmd = AST2600_I2CM_PKT_EN |
> AST2600_I2CM_PKT_ADDR(chip_addr) |
> > +                     AST2600_I2CM_START_CMD;
> > +               writel(cmd, &priv->regs->cmd_sts);
> > +               ret = readl_poll_timeout(&priv->regs->isr, isr,
> > +                                        isr &
> AST2600_I2CM_PKT_DONE,
> > +                                        I2C_TIMEOUT_US);
> > +               if (ret)
> > +                       return -ETIMEDOUT;
> > +
> > +               writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
> > +
> > +               if (isr & AST2600_I2CM_TX_NAK)
> > +                       return -EREMOTEIO;
> > +       }
> > +
> > +       for (tx_cnt = 0; tx_cnt < len; tx_cnt++, buffer++) {
> > +               cmd = AST2600_I2CM_PKT_EN |
> AST2600_I2CM_PKT_ADDR(chip_addr);
> > +               cmd |= AST2600_I2CM_TX_CMD;
> > +
> > +               if (!tx_cnt)
> > +                       cmd |= AST2600_I2CM_START_CMD;
> > +
> > +               if (send_stop && ((len - 1) == tx_cnt))
> > +                       cmd |= AST2600_I2CM_STOP_CMD;
> > +
> > +               writel(*buffer, &priv->regs->trx_buff);
> > +               writel(cmd, &priv->regs->cmd_sts);
> > +               ret = readl_poll_timeout(&priv->regs->isr, isr,
> > +                                        isr &
> AST2600_I2CM_PKT_DONE,
> > +                                        I2C_TIMEOUT_US);
> > +               if (ret)
> > +                       return -ETIMEDOUT;
> > +
> > +               writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
> > +
> > +               if (isr & AST2600_I2CM_TX_NAK)
> > +                       return -EREMOTEIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int ast2600_i2c_deblock(struct udevice *dev) {
> > +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> > +       u32 csr = readl(&priv->regs->cmd_sts);
> > +       u32 isr;
> > +       int ret;
> > +
> > +       //reinit
> 
> /* reinit */
Will update

> 
> > +       writel(0, &priv->regs->fun_ctrl);
> > +       /* Enable Master Mode. Assuming single-master */
> > +       writel(AST2600_I2CC_BUS_AUTO_RELEASE |
> AST2600_I2CC_MASTER_EN |
> > +                      AST2600_I2CC_MULTI_MASTER_DIS,
> > +              &priv->regs->fun_ctrl);
> > +
> 
> [..]
> 
> > +static int ast2600_i2c_set_speed(struct udevice *dev, unsigned int
> > +speed) {
> > +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> > +       unsigned long base_clk1, base_clk2, base_clk3, base_clk4;
> > +       int baseclk_idx;
> > +       u32 clk_div_reg;
> > +       u32 apb_clk;
> > +       u32 scl_low;
> > +       u32 scl_high;
> > +       int divisor;
> > +       int inc = 0;
> > +       u32 data;
> > +
> > +       debug("Setting speed for I2C%d to <%u>\n", dev->seq_, speed);
> > +       if (!speed) {
> > +               debug("No valid speed specified\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       apb_clk = clk_get_rate(&priv->clk);
> > +
> > +       clk_div_reg = readl(priv->global + 0x10);
> > +       base_clk1 = (apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) *
> > + 10) / 2);
> 
> Can apb_clk * 10 go in a local var to reduce duplication?

Yes, will update
> 
> > +       base_clk2 =
> > +               (apb_clk * 10) / (((((clk_div_reg >> 8) & 0xff) + 2) * 10) /
> 2);
> > +       base_clk3 = (apb_clk * 10) /
> > +                   (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2);
> > +       base_clk4 = (apb_clk * 10) /
> > +                   (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2);
> 
> Normally we like to define these fields, like:
> 
> #define CLK1_DIV_SHIFT 0
> #define CLK1_DIV_MASK (0xff >> CLK1_DIV_SHIFT) #define CLK2_DIV_SHIFT 8
> #define CLK2_DIV_MASK (0xff >> CLK2_DIV_SHIFT)
> 
Thanks, will MASK define for usage. 

> etc.
> 
> > +
> > +       if ((apb_clk / speed) <= 32) {
> > +               baseclk_idx = 0;
> > +               divisor = DIV_ROUND_UP(apb_clk, speed);
> > +       } else if ((base_clk1 / speed) <= 32) {
> > +               baseclk_idx = 1;
> > +               divisor = DIV_ROUND_UP(base_clk1, speed);
> > +       } else if ((base_clk2 / speed) <= 32) {
> > +               baseclk_idx = 2;
> > +               divisor = DIV_ROUND_UP(base_clk2, speed);
> > +       } else if ((base_clk3 / speed) <= 32) {
> > +               baseclk_idx = 3;
> > +               divisor = DIV_ROUND_UP(base_clk3, speed);
> > +       } else {
> > +               baseclk_idx = 4;
> > +               divisor = DIV_ROUND_UP(base_clk4, speed);
> > +               inc = 0;
> > +               while ((divisor + inc) > 32) {
> > +                       inc |= divisor & 0x1;
> > +                       divisor >>= 1;
> > +                       baseclk_idx++;
> > +               }
> > +               divisor += inc;
> > +       }
> > +       divisor = min_t(int, divisor, 32);
> > +       baseclk_idx &= 0xf;
> 
> What happens if baseclk_idx is >= 16 ?
In previous if else case. 
baseclk_idx only have five cases.
baseclk_idx =0, 1, 2, 3, 4, 5, no others cases. Even >= 16
> 
> > +       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);
> 
> drop extra () around baseclk_idx


Will remove ()

> 
> > +       /* Set AC Timing */
> > +       writel(data, &priv->regs->ac_timing);
> > +
> > +       return 0;
> > +}
> > +
> > +static int ast2600_i2c_probe(struct udevice *dev) {
> > +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> > +       struct udevice *misc_dev;
> > +       int ret;
> > +
> > +       /* find global base address */
> > +       ret = uclass_get_device_by_driver(UCLASS_MISC,
> > +
> DM_DRIVER_GET(aspeed_i2c_global),
> > +                                         &misc_dev);
> 
> But isn't this the parent device of this one? Just use device_get_parent(dev)
> 
Yes, use parent device will be better.

> Also misc_dev is not a great name. How about glob_dev ?
Great, this naming is better. global_dev
> 
> > +       if (ret) {
> > +               debug("i2c global not defined\n");
> > +               return ret;
> > +       }
> > +
> > +       priv->global = devfdt_get_addr_ptr(misc_dev);
> 
> dev_read_addr()
Will update
> 
> > +       if (IS_ERR(priv->global)) {
> > +               debug("%s(): can't get global\n", __func__);
> > +               return PTR_ERR(priv->global);
> > +       }
> > +
> > +       /* Reset device */
> > +       writel(0, &priv->regs->fun_ctrl);
> 
> When you have functions with a lot of priv->regs things it makes sense to add a
> local 'regs' variable.
Yes, will update with local regs = &priv->regs 
> 
> > +       /* Enable Master Mode. Assuming single-master */
> > +       writel(AST2600_I2CC_BUS_AUTO_RELEASE |
> AST2600_I2CC_MASTER_EN |
> > +                      AST2600_I2CC_MULTI_MASTER_DIS,
> > +              &priv->regs->fun_ctrl);
> > +
> > +       writel(0, &priv->regs->ier);
> > +       /* Clear Interrupt */
> > +       writel(~0, &priv->regs->isr);
> > +
> > +       return 0;
> > +}
> > +
> > +static int ast2600_i2c_of_to_plat(struct udevice *dev) {
> > +       struct ast2600_i2c_priv *priv = dev_get_priv(dev);
> > +       int ret;
> > +
> > +       priv->regs = dev_read_addr_ptr(dev);
> > +       if (!(priv->regs))
> 
> Drop extra ()
Will update

> 
> > +               return -EINVAL;
> > +
> > +       ret = clk_get_by_index(dev, 0, &priv->clk);
> > +       if (ret < 0) {
> > +               debug("%s: Can't get clock for %s: %d\n", __func__,
> dev->name,
> > +                     ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_i2c_ops ast2600_i2c_ops = {
> > +       .xfer = ast2600_i2c_xfer,
> > +       .deblock = ast2600_i2c_deblock,
> > +       .set_bus_speed = ast2600_i2c_set_speed, };
> > +
> > +static const struct udevice_id ast2600_i2c_ids[] = {
> > +       { .compatible = "aspeed,ast2600-i2c" },
> > +       {},
> > +};
> > +
> > +U_BOOT_DRIVER(ast2600_i2c) = {
> > +       .name = "ast2600_i2c",
> > +       .id = UCLASS_I2C,
> > +       .of_match = ast2600_i2c_ids,
> > +       .probe = ast2600_i2c_probe,
> > +       .of_to_plat = ast2600_i2c_of_to_plat,
> > +       .priv_auto = sizeof(struct ast2600_i2c_priv),
> > +       .ops = &ast2600_i2c_ops,
> > +};
> > +
> > +#define AST2600_I2CG_ISR                       0x00
> > +#define AST2600_I2CG_SLAVE_ISR         0x04
> > +#define AST2600_I2CG_OWNER                 0x08
> > +#define AST2600_I2CG_CTRL                  0x0C
> > +#define AST2600_I2CG_CLK_DIV_CTRL      0x10
> > +
> > +#define AST2600_I2CG_SLAVE_PKT_NAK         BIT(4)
> > +#define AST2600_I2CG_M_S_SEPARATE_INTR BIT(3)
> > +#define AST2600_I2CG_CTRL_NEW_REG          BIT(2)
> > +#define AST2600_I2CG_CTRL_NEW_CLK_DIV  BIT(1)
> > +
> > +#define AST2600_GLOBAL_INIT
> \
> > +                       (AST2600_I2CG_SLAVE_PKT_NAK |   \
> > +                       AST2600_I2CG_CTRL_NEW_REG |
> \
> > +                       AST2600_I2CG_CTRL_NEW_CLK_DIV) #define
> > +I2CCG_DIV_CTRL 0xC6411208
> 
> Please use lower-case hex consistently
> 
> Hmm these should really go in the header file too?
Will update and add "ast2600-i2c.h"
> 
> > +
> > +struct ast2600_i2c_global_priv {
> > +       void __iomem *regs;
> > +       struct reset_ctl reset;
> > +};
> > +
> > +/*
> > + * APB clk : 100Mhz
> > + * div  : scl       : baseclk [APB/((div/2) + 1)] : tBuf [1/bclk * 16]
> > + * I2CG10[31:24] base clk4 for i2c auto recovery timeout counter
> > +(0xC6)
> > + * I2CG10[23:16] base clk3 for Standard-mode (100Khz) min tBuf 4.7us
> > + * 0x3c : 100.8Khz  : 3.225Mhz                    : 4.96us
> > + * 0x3d : 99.2Khz   : 3.174Mhz                    : 5.04us
> > + * 0x3e : 97.65Khz  : 3.125Mhz                    : 5.12us
> > + * 0x40 : 97.75Khz  : 3.03Mhz                     : 5.28us
> > + * 0x41 : 99.5Khz   : 2.98Mhz                     : 5.36us (default)
> > + * I2CG10[15:8] base clk2 for Fast-mode (400Khz) min tBuf 1.3us
> > + * 0x12 : 400Khz    : 10Mhz                       : 1.6us
> > + * I2CG10[7:0] base clk1 for Fast-mode Plus (1Mhz) min tBuf 0.5us
> > + * 0x08 : 1Mhz      : 20Mhz                       : 0.8us
> > + */
> > +
> > +static int aspeed_i2c_global_probe(struct udevice *dev) {
> > +       struct ast2600_i2c_global_priv *i2c_global = dev_get_priv(dev);
> > +       int ret = 0;
> > +
> > +       i2c_global->regs = dev_read_addr_ptr(dev);
> > +       if (!(i2c_global->regs))
> 
> again too many (). Please fix globally

Will update if (!i2c_global->regs)
> 
> > +               return -EINVAL;
> > +
> > +       debug("%s(dev=%p)\n", __func__, dev);
> > +
> > +       ret = reset_get_by_index(dev, 0, &i2c_global->reset);
> > +       if (ret) {
> > +               printf("%s(): Failed to get reset signal\n",
> > + __func__);
> 
> log_err() if you want an error. Try to avoid using __func__ - use
> log...() instead
> 
Will update to log_err

> > +               return ret;
> > +       }
> > +
> > +       reset_deassert(&i2c_global->reset);
> 
> Can this fail?
Will add fail check
> 
> > +
> > +       writel(AST2600_GLOBAL_INIT, i2c_global->regs +
> > +               AST2600_I2CG_CTRL);
> > +       writel(I2CCG_DIV_CTRL, i2c_global->regs +
> > +               AST2600_I2CG_CLK_DIV_CTRL);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct udevice_id aspeed_i2c_global_ids[] = {
> > +       {       .compatible = "aspeed,ast2600-i2c-global",      },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(aspeed_i2c_global) = {
> > +       .name           = "aspeed_i2c_global",
> > +       .id                     = UCLASS_MISC,
> > +       .of_match       = aspeed_i2c_global_ids,
> > +       .probe          = aspeed_i2c_global_probe,
> > +       .priv_auto  = sizeof(struct ast2600_i2c_global_priv), };
> > +
> > --
> > 2.34.1
> >
> 
> Regards,
> Simon
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 3fc4cd0f12..1cf54f0b4e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -769,6 +769,12 @@  S:	Maintained
 F:	drivers/pci/pcie_phytium.c
 F:	arch/arm/dts/phytium-durian.dts
 
+ASPEED AST2600 I2C DRIVER
+M:	Ryan Chen <ryan_chen@aspeedtech.com>
+R:	Aspeed BMC SW team <BMC-SW@aspeedtech.com>
+S:	Maintained
+F:	drivers/i2c/ast2600_i2c.c
+
 ASPEED FMC SPI DRIVER
 M:	Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
 M:	Cédric Le Goater <clg@kaod.org>
diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index 08b6c7bdcc..34f507fb3b 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -221,6 +221,13 @@  config SYS_I2C_DW
 	  controller is used in various SoCs, e.g. the ST SPEAr, Altera
 	  SoCFPGA, Synopsys ARC700 and some Intel x86 SoCs.
 
+config SYS_I2C_AST2600
+    bool "AST2600 I2C Controller"
+    depends on DM_I2C && ARCH_ASPEED
+    help
+      Say yes here to select AST2600 I2C Host Controller. The driver
+      support AST2600 I2C new mode register.
+
 config SYS_I2C_ASPEED
 	bool "Aspeed I2C Controller"
 	depends on DM_I2C && ARCH_ASPEED
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index 920aafb91c..89db2d8e37 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -12,6 +12,7 @@  obj-$(CONFIG_$(SPL_)I2C_CROS_EC_LDO) += cros_ec_ldo.o
 
 obj-$(CONFIG_$(SPL_)SYS_I2C_LEGACY) += i2c_core.o
 obj-$(CONFIG_SYS_I2C_ASPEED) += ast_i2c.o
+obj-$(CONFIG_SYS_I2C_AST2600) += ast2600_i2c.o
 obj-$(CONFIG_SYS_I2C_AT91) += at91_i2c.o
 obj-$(CONFIG_SYS_I2C_CADENCE) += i2c-cdns.o
 obj-$(CONFIG_SYS_I2C_CA) += i2c-cortina.o
diff --git a/drivers/i2c/ast2600_i2c.c b/drivers/i2c/ast2600_i2c.c
new file mode 100644
index 0000000000..52aea460ac
--- /dev/null
+++ b/drivers/i2c/ast2600_i2c.c
@@ -0,0 +1,480 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright ASPEED Technology Inc.
+ */
+#include <linux/iopoll.h>
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <i2c.h>
+#include <asm/io.h>
+#include <regmap.h>
+#include <reset.h>
+
+struct ast2600_i2c_regs {
+	u32 fun_ctrl;
+	u32 ac_timing;
+	u32 trx_buff;
+	u32 icr;
+	u32 ier;
+	u32 isr;
+	u32 cmd_sts;
+};
+
+/* 0x00 : I2CC Master/Slave Function Control Register  */
+#define AST2600_I2CC_SLAVE_ADDR_RX_EN	BIT(20)
+#define AST2600_I2CC_MASTER_RETRY_MASK	GENMASK(19, 18)
+#define AST2600_I2CC_MASTER_RETRY(x)	(((x) & GENMASK(1, 0)) << 18)
+#define AST2600_I2CC_BUS_AUTO_RELEASE	BIT(17)
+#define AST2600_I2CC_M_SDA_LOCK_EN		BIT(16)
+#define AST2600_I2CC_MULTI_MASTER_DIS	BIT(15)
+#define AST2600_I2CC_M_SCL_DRIVE_EN		BIT(14)
+#define AST2600_I2CC_MSB_STS			BIT(9)
+#define AST2600_I2CC_SDA_DRIVE_1T_EN	BIT(8)
+#define AST2600_I2CC_M_SDA_DRIVE_1T_EN	BIT(7)
+#define AST2600_I2CC_M_HIGH_SPEED_EN	BIT(6)
+/* reserver 5 : 2 */
+#define AST2600_I2CC_SLAVE_EN			BIT(1)
+#define AST2600_I2CC_MASTER_EN			BIT(0)
+
+/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
+/* Base register value. These bits are always set by the driver. */
+#define I2CD_CACTC_BASE		0xfff00300
+#define I2CD_TCKHIGH_SHIFT	16
+#define I2CD_TCKLOW_SHIFT	12
+#define I2CD_THDDAT_SHIFT	10
+#define I2CD_TO_DIV_SHIFT	8
+#define I2CD_BASE_DIV_SHIFT	0
+
+/* 0x08 : I2CC Master/Slave Transmit/Receive Byte Buffer Register */
+#define AST2600_I2CC_TX_DIR_MASK		GENMASK(31, 29)
+#define AST2600_I2CC_SDA_OE				BIT(28)
+#define AST2600_I2CC_SDA_O				BIT(27)
+#define AST2600_I2CC_SCL_OE				BIT(26)
+#define AST2600_I2CC_SCL_O				BIT(25)
+
+#define AST2600_I2CC_SCL_LINE_STS		BIT(18)
+#define AST2600_I2CC_SDA_LINE_STS		BIT(17)
+#define AST2600_I2CC_BUS_BUSY_STS		BIT(16)
+#define AST2600_I2CC_GET_RX_BUFF(x)		(((x) >> 8) & GENMASK(7, 0))
+
+/* 0x10 : I2CM Master Interrupt Control Register */
+/* 0x14 : I2CM Master Interrupt Status Register  */
+#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)
+
+#define AST2600_I2CM_SCL_LOW_TO			BIT(6)
+#define AST2600_I2CM_ABNORMAL			BIT(5)
+#define AST2600_I2CM_NORMAL_STOP		BIT(4)
+#define AST2600_I2CM_ARBIT_LOSS			BIT(3)
+#define AST2600_I2CM_RX_DONE			BIT(2)
+#define AST2600_I2CM_TX_NAK				BIT(1)
+#define AST2600_I2CM_TX_ACK				BIT(0)
+
+/* 0x18 : I2CM Master Command/Status Register   */
+#define AST2600_I2CM_PKT_ADDR(x)		(((x) & GENMASK(6, 0)) << 24)
+#define AST2600_I2CM_PKT_EN				BIT(16)
+#define AST2600_I2CM_SDA_OE_OUT_DIR		BIT(15)
+#define AST2600_I2CM_SDA_O_OUT_DIR		BIT(14)
+#define AST2600_I2CM_SCL_OE_OUT_DIR		BIT(13)
+#define AST2600_I2CM_SCL_O_OUT_DIR		BIT(12)
+#define AST2600_I2CM_RECOVER_CMD_EN		BIT(11)
+
+#define AST2600_I2CM_RX_DMA_EN			BIT(9)
+#define AST2600_I2CM_TX_DMA_EN			BIT(8)
+/* Command Bit */
+#define AST2600_I2CM_RX_BUFF_EN			BIT(7)
+#define AST2600_I2CM_TX_BUFF_EN			BIT(6)
+#define AST2600_I2CM_STOP_CMD			BIT(5)
+#define AST2600_I2CM_RX_CMD_LAST		BIT(4)
+#define AST2600_I2CM_RX_CMD				BIT(3)
+
+#define AST2600_I2CM_TX_CMD				BIT(1)
+#define AST2600_I2CM_START_CMD			BIT(0)
+
+#define I2C_TIMEOUT_US 100000
+/*
+ * Device private data
+ */
+struct ast2600_i2c_priv {
+	struct clk clk;
+	struct ast2600_i2c_regs *regs;
+	void __iomem *global;
+};
+
+static int ast2600_i2c_read_data(struct ast2600_i2c_priv *priv, u8 chip_addr,
+				 u8 *buffer, size_t len, bool send_stop)
+{
+	int rx_cnt, ret = 0;
+	u32 cmd, isr;
+
+	for (rx_cnt = 0; rx_cnt < len; rx_cnt++, buffer++) {
+		cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(chip_addr) |
+		      AST2600_I2CM_RX_CMD;
+		if (!rx_cnt)
+			cmd |= AST2600_I2CM_START_CMD;
+
+		if ((len - 1) == rx_cnt)
+			cmd |= AST2600_I2CM_RX_CMD_LAST;
+
+		if (send_stop && ((len - 1) == rx_cnt))
+			cmd |= AST2600_I2CM_STOP_CMD;
+
+		writel(cmd, &priv->regs->cmd_sts);
+
+		ret = readl_poll_timeout(&priv->regs->isr, isr,
+					 isr & AST2600_I2CM_PKT_DONE,
+					 I2C_TIMEOUT_US);
+		if (ret)
+			return -ETIMEDOUT;
+
+		*buffer =
+			AST2600_I2CC_GET_RX_BUFF(readl(&priv->regs->trx_buff));
+
+		writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
+
+		if (isr & AST2600_I2CM_TX_NAK)
+			return -EREMOTEIO;
+	}
+
+	return 0;
+}
+
+static int ast2600_i2c_write_data(struct ast2600_i2c_priv *priv, u8 chip_addr,
+				  u8 *buffer, size_t len, bool send_stop)
+{
+	int tx_cnt, ret = 0;
+	u32 cmd, isr;
+
+	if (!len) {
+		cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(chip_addr) |
+		      AST2600_I2CM_START_CMD;
+		writel(cmd, &priv->regs->cmd_sts);
+		ret = readl_poll_timeout(&priv->regs->isr, isr,
+					 isr & AST2600_I2CM_PKT_DONE,
+					 I2C_TIMEOUT_US);
+		if (ret)
+			return -ETIMEDOUT;
+
+		writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
+
+		if (isr & AST2600_I2CM_TX_NAK)
+			return -EREMOTEIO;
+	}
+
+	for (tx_cnt = 0; tx_cnt < len; tx_cnt++, buffer++) {
+		cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(chip_addr);
+		cmd |= AST2600_I2CM_TX_CMD;
+
+		if (!tx_cnt)
+			cmd |= AST2600_I2CM_START_CMD;
+
+		if (send_stop && ((len - 1) == tx_cnt))
+			cmd |= AST2600_I2CM_STOP_CMD;
+
+		writel(*buffer, &priv->regs->trx_buff);
+		writel(cmd, &priv->regs->cmd_sts);
+		ret = readl_poll_timeout(&priv->regs->isr, isr,
+					 isr & AST2600_I2CM_PKT_DONE,
+					 I2C_TIMEOUT_US);
+		if (ret)
+			return -ETIMEDOUT;
+
+		writel(AST2600_I2CM_PKT_DONE, &priv->regs->isr);
+
+		if (isr & AST2600_I2CM_TX_NAK)
+			return -EREMOTEIO;
+	}
+
+	return 0;
+}
+
+static int ast2600_i2c_deblock(struct udevice *dev)
+{
+	struct ast2600_i2c_priv *priv = dev_get_priv(dev);
+	u32 csr = readl(&priv->regs->cmd_sts);
+	u32 isr;
+	int ret;
+
+	//reinit
+	writel(0, &priv->regs->fun_ctrl);
+	/* Enable Master Mode. Assuming single-master */
+	writel(AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN |
+		       AST2600_I2CC_MULTI_MASTER_DIS,
+	       &priv->regs->fun_ctrl);
+
+	csr = readl(&priv->regs->cmd_sts);
+
+	if (!(csr & AST2600_I2CC_SDA_LINE_STS) &&
+	    (csr & AST2600_I2CC_SCL_LINE_STS)) {
+		debug("Bus stuck (%x), attempting recovery\n", csr);
+		writel(AST2600_I2CM_RECOVER_CMD_EN, &priv->regs->cmd_sts);
+		ret = readl_poll_timeout(&priv->regs->isr, isr,
+					 isr & (AST2600_I2CM_BUS_RECOVER_FAIL |
+						AST2600_I2CM_BUS_RECOVER),
+					 I2C_TIMEOUT_US);
+		writel(~0, &priv->regs->isr);
+		if (ret)
+			return -EREMOTEIO;
+	}
+
+	return 0;
+}
+
+static int ast2600_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
+{
+	struct ast2600_i2c_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	if (readl(&priv->regs->trx_buff) & AST2600_I2CC_BUS_BUSY_STS)
+		return -EREMOTEIO;
+
+	for (; nmsgs > 0; nmsgs--, msg++) {
+		if (msg->flags & I2C_M_RD) {
+			debug("i2c_read: chip=0x%x, len=0x%x, flags=0x%x\n",
+			      msg->addr, msg->len, msg->flags);
+			ret = ast2600_i2c_read_data(priv, msg->addr, msg->buf,
+						    msg->len, (nmsgs == 1));
+		} else {
+			debug("i2c_write: chip=0x%x, len=0x%x, flags=0x%x\n",
+			      msg->addr, msg->len, msg->flags);
+			ret = ast2600_i2c_write_data(priv, msg->addr, msg->buf,
+						     msg->len, (nmsgs == 1));
+		}
+		if (ret) {
+			debug("%s: error (%d)\n", __func__, ret);
+			return -EREMOTEIO;
+		}
+	}
+
+	return 0;
+}
+
+static int ast2600_i2c_set_speed(struct udevice *dev, unsigned int speed)
+{
+	struct ast2600_i2c_priv *priv = dev_get_priv(dev);
+	unsigned long base_clk1, base_clk2, base_clk3, base_clk4;
+	int baseclk_idx;
+	u32 clk_div_reg;
+	u32 apb_clk;
+	u32 scl_low;
+	u32 scl_high;
+	int divisor;
+	int inc = 0;
+	u32 data;
+
+	debug("Setting speed for I2C%d to <%u>\n", dev->seq_, speed);
+	if (!speed) {
+		debug("No valid speed specified\n");
+		return -EINVAL;
+	}
+
+	apb_clk = clk_get_rate(&priv->clk);
+
+	clk_div_reg = readl(priv->global + 0x10);
+	base_clk1 = (apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2);
+	base_clk2 =
+		(apb_clk * 10) / (((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2);
+	base_clk3 = (apb_clk * 10) /
+		    (((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2);
+	base_clk4 = (apb_clk * 10) /
+		    (((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2);
+
+	if ((apb_clk / speed) <= 32) {
+		baseclk_idx = 0;
+		divisor = DIV_ROUND_UP(apb_clk, speed);
+	} else if ((base_clk1 / speed) <= 32) {
+		baseclk_idx = 1;
+		divisor = DIV_ROUND_UP(base_clk1, speed);
+	} else if ((base_clk2 / speed) <= 32) {
+		baseclk_idx = 2;
+		divisor = DIV_ROUND_UP(base_clk2, speed);
+	} else if ((base_clk3 / speed) <= 32) {
+		baseclk_idx = 3;
+		divisor = DIV_ROUND_UP(base_clk3, speed);
+	} else {
+		baseclk_idx = 4;
+		divisor = DIV_ROUND_UP(base_clk4, speed);
+		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);
+	/* Set AC Timing */
+	writel(data, &priv->regs->ac_timing);
+
+	return 0;
+}
+
+static int ast2600_i2c_probe(struct udevice *dev)
+{
+	struct ast2600_i2c_priv *priv = dev_get_priv(dev);
+	struct udevice *misc_dev;
+	int ret;
+
+	/* find global base address */
+	ret = uclass_get_device_by_driver(UCLASS_MISC,
+					  DM_DRIVER_GET(aspeed_i2c_global),
+					  &misc_dev);
+	if (ret) {
+		debug("i2c global not defined\n");
+		return ret;
+	}
+
+	priv->global = devfdt_get_addr_ptr(misc_dev);
+	if (IS_ERR(priv->global)) {
+		debug("%s(): can't get global\n", __func__);
+		return PTR_ERR(priv->global);
+	}
+
+	/* Reset device */
+	writel(0, &priv->regs->fun_ctrl);
+	/* Enable Master Mode. Assuming single-master */
+	writel(AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN |
+		       AST2600_I2CC_MULTI_MASTER_DIS,
+	       &priv->regs->fun_ctrl);
+
+	writel(0, &priv->regs->ier);
+	/* Clear Interrupt */
+	writel(~0, &priv->regs->isr);
+
+	return 0;
+}
+
+static int ast2600_i2c_of_to_plat(struct udevice *dev)
+{
+	struct ast2600_i2c_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	priv->regs = dev_read_addr_ptr(dev);
+	if (!(priv->regs))
+		return -EINVAL;
+
+	ret = clk_get_by_index(dev, 0, &priv->clk);
+	if (ret < 0) {
+		debug("%s: Can't get clock for %s: %d\n", __func__, dev->name,
+		      ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dm_i2c_ops ast2600_i2c_ops = {
+	.xfer = ast2600_i2c_xfer,
+	.deblock = ast2600_i2c_deblock,
+	.set_bus_speed = ast2600_i2c_set_speed,
+};
+
+static const struct udevice_id ast2600_i2c_ids[] = {
+	{ .compatible = "aspeed,ast2600-i2c" },
+	{},
+};
+
+U_BOOT_DRIVER(ast2600_i2c) = {
+	.name = "ast2600_i2c",
+	.id = UCLASS_I2C,
+	.of_match = ast2600_i2c_ids,
+	.probe = ast2600_i2c_probe,
+	.of_to_plat = ast2600_i2c_of_to_plat,
+	.priv_auto = sizeof(struct ast2600_i2c_priv),
+	.ops = &ast2600_i2c_ops,
+};
+
+#define AST2600_I2CG_ISR			0x00
+#define AST2600_I2CG_SLAVE_ISR		0x04
+#define AST2600_I2CG_OWNER		    0x08
+#define AST2600_I2CG_CTRL		    0x0C
+#define AST2600_I2CG_CLK_DIV_CTRL	0x10
+
+#define AST2600_I2CG_SLAVE_PKT_NAK	    BIT(4)
+#define AST2600_I2CG_M_S_SEPARATE_INTR	BIT(3)
+#define AST2600_I2CG_CTRL_NEW_REG	    BIT(2)
+#define AST2600_I2CG_CTRL_NEW_CLK_DIV	BIT(1)
+
+#define AST2600_GLOBAL_INIT					\
+			(AST2600_I2CG_SLAVE_PKT_NAK |	\
+			AST2600_I2CG_CTRL_NEW_REG |		\
+			AST2600_I2CG_CTRL_NEW_CLK_DIV)
+#define I2CCG_DIV_CTRL 0xC6411208
+
+struct ast2600_i2c_global_priv {
+	void __iomem *regs;
+	struct reset_ctl reset;
+};
+
+/*
+ * APB clk : 100Mhz
+ * div  : scl       : baseclk [APB/((div/2) + 1)] : tBuf [1/bclk * 16]
+ * I2CG10[31:24] base clk4 for i2c auto recovery timeout counter (0xC6)
+ * I2CG10[23:16] base clk3 for Standard-mode (100Khz) min tBuf 4.7us
+ * 0x3c : 100.8Khz  : 3.225Mhz                    : 4.96us
+ * 0x3d : 99.2Khz   : 3.174Mhz                    : 5.04us
+ * 0x3e : 97.65Khz  : 3.125Mhz                    : 5.12us
+ * 0x40 : 97.75Khz  : 3.03Mhz                     : 5.28us
+ * 0x41 : 99.5Khz   : 2.98Mhz                     : 5.36us (default)
+ * I2CG10[15:8] base clk2 for Fast-mode (400Khz) min tBuf 1.3us
+ * 0x12 : 400Khz    : 10Mhz                       : 1.6us
+ * I2CG10[7:0] base clk1 for Fast-mode Plus (1Mhz) min tBuf 0.5us
+ * 0x08 : 1Mhz      : 20Mhz                       : 0.8us
+ */
+
+static int aspeed_i2c_global_probe(struct udevice *dev)
+{
+	struct ast2600_i2c_global_priv *i2c_global = dev_get_priv(dev);
+	int ret = 0;
+
+	i2c_global->regs = dev_read_addr_ptr(dev);
+	if (!(i2c_global->regs))
+		return -EINVAL;
+
+	debug("%s(dev=%p)\n", __func__, dev);
+
+	ret = reset_get_by_index(dev, 0, &i2c_global->reset);
+	if (ret) {
+		printf("%s(): Failed to get reset signal\n", __func__);
+		return ret;
+	}
+
+	reset_deassert(&i2c_global->reset);
+
+	writel(AST2600_GLOBAL_INIT, i2c_global->regs +
+		AST2600_I2CG_CTRL);
+	writel(I2CCG_DIV_CTRL, i2c_global->regs +
+		AST2600_I2CG_CLK_DIV_CTRL);
+
+	return 0;
+}
+
+static const struct udevice_id aspeed_i2c_global_ids[] = {
+	{	.compatible = "aspeed,ast2600-i2c-global",	},
+	{ }
+};
+
+U_BOOT_DRIVER(aspeed_i2c_global) = {
+	.name		= "aspeed_i2c_global",
+	.id			= UCLASS_MISC,
+	.of_match	= aspeed_i2c_global_ids,
+	.probe		= aspeed_i2c_global_probe,
+	.priv_auto  = sizeof(struct ast2600_i2c_global_priv),
+};
+