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

Ryan Chen ryan_chen at aspeedtech.com
Tue Jan 10 03:41:53 CET 2023


Hello Simon,
	Thank your feedback.

> -----Original Message-----
> From: Simon Glass <sjg at chromium.org>
> Sent: Tuesday, January 10, 2023 3:47 AM
> To: Ryan Chen <ryan_chen at aspeedtech.com>
> Cc: Heiko Schocher <hs at denx.de>; BMC-SW <BMC-SW at aspeedtech.com>;
> u-boot at 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 at 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 at 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 at aspeedtech.com>
> > +R:     Aspeed BMC SW team <BMC-SW at aspeedtech.com>
> > +S:     Maintained
> > +F:     drivers/i2c/ast2600_i2c.c
> > +
> >  ASPEED FMC SPI DRIVER
> >  M:     Chin-Ting Kuo <chin-ting_kuo at aspeedtech.com>
> >  M:     Cédric Le Goater <clg at 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


More information about the U-Boot mailing list