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