From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Mon, 20 Apr 2015 13:49:02 +0800 Subject: [U-Boot] [PATCH] dm: i2c: mxc support DM In-Reply-To: References: <1429090554-24295-1-git-send-email-Peng.Fan@freescale.com> Message-ID: <5534934E.4090400@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, Thanks for reviewing. I'll address most comments and try to merge DM and non-DM part into one. will send out v2 for review. The only unsure part is bus_i2c_init, I also reply you inline. I want to pass force_idle_bus and pinmux setting to i2c driver, so i use bus_i2c_init, same with non-DM way. On 4/19/2015 9:53 PM, Simon Glass wrote: > Hi Peng, > > On 15 April 2015 at 03:35, Peng Fan wrote: >> Add support when CONFIG_DM_I2C configured. >> >> Test results: >> => i2c dev 0 >> Setting bus to 0 >> => i2c probe >> Valid chip addresses: 08 50 >> => i2c md 8 38 >> 0038: 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 08 ................ >> => i2c mw 8 38 5 1 >> => i2c md 8 38 >> 0038: 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 05 ................ >> => dm tree >> Class Probed Name >> ---------------------------------------- >> root [ + ] root_driver >> thermal [ ] |-- imx_thermal >> simple_bus [ + ] |-- soc >> simple_bus [ ] | |-- aips-bus at 30000000 >> simple_bus [ ] | | |-- anatop at 30360000 >> simple_bus [ ] | | `-- snvs at 30370000 >> simple_bus [ ] | |-- aips-bus at 30400000 >> simple_bus [ + ] | `-- aips-bus at 30800000 >> i2c [ + ] | |-- i2c at 30a20000 >> i2c_generic [ + ] | | |-- generic_8 >> i2c_generic [ + ] | | `-- generic_50 >> i2c [ ] | |-- i2c at 30a40000 >> spi [ ] | `-- qspi at 30bb0000 >> simple_bus [ ] `-- regulators >> >> Signed-off-by: Peng Fan >> --- >> arch/arm/imx-common/i2c-mxv7.c | 4 + >> arch/arm/include/asm/imx-common/mxc_i2c.h | 5 + >> drivers/i2c/mxc_i2c.c | 354 ++++++++++++++++++++++++++++++ >> 3 files changed, 363 insertions(+) >> >> diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c >> index 1a632e7..99fe112 100644 >> --- a/arch/arm/imx-common/i2c-mxv7.c >> +++ b/arch/arm/imx-common/i2c-mxv7.c >> @@ -99,8 +99,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, >> if (ret) >> goto err_idle; >> >> +#ifndef CONFIG_DM_I2C >> bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr, >> force_idle_bus, p); >> +#else >> + bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p); > One of the goals of driver model is to remove code like this, and have > the devices init themselves when they are used. Here you are probing > each device and then changing its configuration outside the device's > probe() method. This should not be needed with driver model. See > below. Agree. But i want to pass force_idle_bus and pinmux settings(parameter p) to i2c driver. I did not find a good way how to pass these two to DM mxc_i2c driver. > >> +#endif >> >> return 0; >> >> diff --git a/arch/arm/include/asm/imx-common/mxc_i2c.h b/arch/arm/include/asm/imx-common/mxc_i2c.h >> index af86163..8f9c83e 100644 >> --- a/arch/arm/include/asm/imx-common/mxc_i2c.h >> +++ b/arch/arm/include/asm/imx-common/mxc_i2c.h >> @@ -54,8 +54,13 @@ struct i2c_pads_info { >> >> int setup_i2c(unsigned i2c_index, int speed, int slave_addr, >> struct i2c_pads_info *p); >> +#ifndef CONFIG_DM_I2C >> void bus_i2c_init(void *base, int speed, int slave_addr, >> int (*idle_bus_fn)(void *p), void *p); >> +#else >> +void bus_i2c_init(int index, int speed, int slave_addr, >> + int (*idle_bus_fn)(void *p), void *p); >> +#endif >> int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf, >> int len); >> int bus_i2c_write(void *base, uchar chip, uint addr, int alen, >> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c >> index fc5ee35..9488e24 100644 >> --- a/drivers/i2c/mxc_i2c.c >> +++ b/drivers/i2c/mxc_i2c.c >> @@ -21,6 +21,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> DECLARE_GLOBAL_DATA_PTR; >> >> @@ -224,6 +226,7 @@ static int tx_byte(struct mxc_i2c_regs *i2c_regs, u8 byte) >> return 0; >> } >> >> +#ifndef CONFIG_DM_I2C >> /* >> * Stop I2C transaction >> */ >> @@ -552,3 +555,354 @@ U_BOOT_I2C_ADAP_COMPLETE(mxc2, mxc_i2c_init, mxc_i2c_probe, >> CONFIG_SYS_MXC_I2C3_SPEED, >> CONFIG_SYS_MXC_I2C3_SLAVE, 2) >> #endif >> +#else >> +/* >> + * Information about one i2c bus >> + * struct i2c_bus - information about the i2c[x] bus >> + * >> + * @id: Index of i2c bus > What do you mean by 'index'? Is this actually used? I think you should > drop this. See dev->seq for a probed device. Yeah, it is not used. Will remove it. > >> + * @speed: Speed of i2c bus >> + * @driver_data: Flags for different platforms, not used now. > You could drop it, or change to ulong. Will change it to ulong, and use it cover the I2C_QUIRK_REG. > > \> + * @regs: Pointer, the address of i2c bus >> + * @idle_bus_fn: function to force bus idle > We should not call functions like this in driver model. I can not follow you about this. idle_bus_fn is used to force bus idle, when something err during data transfer. > >> + * @idle_bus_data: parameter for idle_bus_fun >> + */ >> +struct i2c_bus { >> + int id; >> + int speed; >> + int pinmux_config; >> + int driver_data; >> + struct mxc_i2c_regs *regs; >> + int (*idle_bus_fn)(void *p); >> + void *idle_bus_data; >> +}; >> + >> +/* >> + * Stop I2C transaction >> + */ >> +static void i2c_imx_stop(struct i2c_bus *i2c_bus) >> +{ >> + struct mxc_i2c_regs *i2c_regs = i2c_bus->regs; >> + int ret; >> + unsigned int temp = readb(&i2c_regs->i2cr); >> + >> + temp &= ~(I2CR_MSTA | I2CR_MTX); >> + writeb(temp, &i2c_regs->i2cr); >> + ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE); >> + if (ret < 0) >> + debug("%s:trigger stop failed\n", __func__); >> + return; >> +} >> + >> +static int i2c_idle_bus(struct i2c_bus *i2c_bus) >> +{ >> + if (i2c_bus && i2c_bus->idle_bus_fn) >> + return i2c_bus->idle_bus_fn(i2c_bus->idle_bus_data); > Can you explain what this does? Used to force bus idle when something err during data transfer. You can see arch/arm/imx-common/i2c-mxv7.c, there is a function "force_idle_bus" which is used to force i2c bus idle using gpio mode. > >> + >> + return 0; >> +} >> + >> +static void i2c_init_controller(struct i2c_bus *i2c_bus) > Drop this function? It seems to do nothing. ok. > >> +{ >> + if (!i2c_bus->speed) >> + return; >> + >> + debug("%s: speed=%d\n", __func__, i2c_bus->speed); >> + >> + return; >> +} >> + >> +static int mxc_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) >> +{ >> + struct i2c_bus *i2c_bus = dev_get_priv(bus); >> + >> + return bus_i2c_set_bus_speed(i2c_bus->regs, speed); >> +} >> + >> +static int mxc_i2c_probe(struct udevice *bus) >> +{ >> + struct i2c_bus *i2c_bus = dev_get_priv(bus); >> + fdt_addr_t addr; >> + >> + i2c_bus->id = bus->seq; >> + /* >> + * driver_dats is not used now, later we can use driver data >> + * to cover I2C_QUIRK_REG and etc. >> + * >> + * TODO >> + */ >> + i2c_bus->driver_data = dev_get_of_data(bus); > dev_get_driver_data() now. ok. > >> + >> + addr = dev_get_addr(bus); >> + if (addr == FDT_ADDR_T_NONE) >> + return -ENODEV; >> + >> + i2c_bus->regs = (struct mxc_i2c_regs *)addr; >> + >> + /* >> + * Pinmux settings are in board file now, until pinmux is supported, >> + * we can set pinmux here in probe function. >> + * >> + * TODO: Pinmux >> + */ >> + >> + i2c_init_controller(i2c_bus); >> + debug("i2c : controller bus %d at %p , speed %d: ", >> + bus->seq, i2c_bus->regs, >> + i2c_bus->speed); >> + >> + return 0; >> +} >> + >> +void bus_i2c_init(int busnum, int speed, int slave, int (*idle_bus_fn)(void *p), >> + void *idle_bus_data) >> +{ >> + struct udevice *bus; >> + struct i2c_bus *i2c_bus; >> + int ret; >> + >> + ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus); >> + if (ret) { >> + debug("Cannot find I2C bus %d\n", busnum); >> + return; >> + } >> + >> + i2c_bus = dev_get_priv(bus); >> + >> + i2c_bus->idle_bus_fn = idle_bus_fn; >> + i2c_bus->idle_bus_data = idle_bus_data; >> + >> + mxc_i2c_set_bus_speed(bus, speed); > This should move into your probe function. You should not need > bus_i2c_init(). See for example tegra_i2c_probe(). I want to use "force_bus_idle" and pinmux settings, but I did not find a good way to pass force_bus_idle to the i2c driver. bus_i2c_init is called from arch/arm/imx-common/i2c-mxv7.c > >> + >> + return; >> +} >> + >> +static int i2c_init_transfer_(struct i2c_bus *i2c_bus, u32 chip, >> + bool read) >> +{ >> + unsigned int temp; >> + int ret; >> + struct mxc_i2c_regs *i2c_regs = i2c_bus->regs; >> + >> + /* Enable I2C controller */ >> +#ifdef I2C_QUIRK_REG >> + if (readb(&i2c_regs->i2cr) & I2CR_IDIS) { >> +#else >> + if (!(readb(&i2c_regs->i2cr) & I2CR_IEN)) { >> +#endif > Can you work this out from the device tree or the driver data, and > avoid the #ifdef? will use driver data to work this out. > >> + writeb(I2CR_IEN, &i2c_regs->i2cr); >> + /* Wait for controller to be stable */ >> + udelay(50); >> + } >> + if (readb(&i2c_regs->iadr) == (chip << 1)) >> + writeb((chip << 1) ^ 2, &i2c_regs->iadr); >> + writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr); >> + ret = wait_for_sr_state(i2c_regs, ST_BUS_IDLE); >> + if (ret < 0) >> + return ret; >> + >> + /* Start I2C transaction */ >> + temp = readb(&i2c_regs->i2cr); >> + temp |= I2CR_MSTA; >> + writeb(temp, &i2c_regs->i2cr); >> + >> + ret = wait_for_sr_state(i2c_regs, ST_BUS_BUSY); >> + if (ret < 0) >> + return ret; >> + >> + temp |= I2CR_MTX | I2CR_TX_NO_AK; >> + writeb(temp, &i2c_regs->i2cr); >> + >> + /* write slave address */ >> + ret = tx_byte(i2c_regs, chip << 1 | read); >> + if (ret < 0) { >> + i2c_imx_stop(i2c_bus); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int i2c_init_transfer(struct i2c_bus *i2c_bus, u32 chip, bool read) >> +{ >> + int retry; >> + int ret; >> + struct mxc_i2c_regs *i2c_regs = i2c_bus->regs; >> + >> + for (retry = 0; retry < 3; retry++) { >> + ret = i2c_init_transfer_(i2c_bus, chip, read); >> + if (ret >= 0) >> + return 0; >> + i2c_imx_stop(i2c_bus); >> + if (ret == -ENODEV) >> + return ret; >> + >> + debug("%s: failed for chip 0x%x retry=%d\n", __func__, chip, >> + retry); >> + if (ret != -ERESTART) >> + /* Disable controller */ >> + writeb(I2CR_IDIS, &i2c_regs->i2cr); >> + udelay(100); >> + if (i2c_idle_bus(i2c_bus) < 0) >> + break; >> + } >> + >> + debug("%s: give up i2c_regs=%p\n", __func__, i2c_regs); >> + return ret; >> +} > This looks very similar to the old i2c_init_transfer(). Can you create > a common function and avoid copying the code? Just want to not mix with non-DM part. Since we are migrating to DM, mix DM and non-DM will introuce more #ifdefs. I'll look into whether can use a common function to cover DM and non-DM part. > Also in the old function > you dealt with 'alen' (now called offset length) but I don't see that > code here. Here I suppose only support 7 bit, so I just removed alen, seems better to keep it. Will add it back and use the flags from chip->flags to determine alen. > >> + >> + >> +static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr, >> + u32 chip_flags) >> +{ >> + int ret; >> + struct i2c_bus *i2c_bus = dev_get_priv(bus); >> + >> + ret = i2c_init_transfer(i2c_bus, chip_addr, false); >> + if (ret < 0) >> + return ret; >> + >> + i2c_imx_stop(i2c_bus); >> + >> + return 0; >> +} >> + >> +static int i2c_write_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buffer, >> + int len, bool end_with_repeated_start) >> +{ >> + int i, ret = 0; >> + >> + struct mxc_i2c_regs *i2c_regs = i2c_bus->regs; > blank line should move up one. ok. > >> + debug("i2c_write_data: chip=0x%x, len=0x%x\n", chip, len); >> + debug("write_data: "); >> + /* use rc for counter */ >> + for (i = 0; i < len; ++i) >> + debug(" 0x%02x", buffer[i]); >> + debug("\n"); >> + >> + for (i = 0; i < len; i++) { >> + ret = tx_byte(i2c_regs, buffer[i]); >> + if (ret < 0) { >> + debug("i2c_write_data(): rc=%d\n", ret); >> + break; >> + } >> + } >> + >> + if (end_with_repeated_start) { >> + /* Reuse ret */ >> + ret = readb(&i2c_regs->i2cr); >> + ret |= I2CR_RSTA; >> + writeb(ret, &i2c_regs->i2cr); >> + >> + ret = tx_byte(i2c_regs, (chip << 1) | 1); >> + if (ret < 0) { >> + i2c_imx_stop(i2c_bus); >> + return ret; >> + } >> + } >> + >> + return ret; >> +} >> + >> +static int i2c_read_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buf, >> + int len) >> +{ >> + int ret; >> + unsigned int temp; >> + int i; >> + struct mxc_i2c_regs *i2c_regs = i2c_bus->regs; >> + >> + debug("i2c_read_data: chip=0x%x, len=0x%x\n", chip, len); >> + >> + /* setup bus to read data */ >> + temp = readb(&i2c_regs->i2cr); >> + temp &= ~(I2CR_MTX | I2CR_TX_NO_AK); >> + if (len == 1) >> + temp |= I2CR_TX_NO_AK; >> + writeb(temp, &i2c_regs->i2cr); >> + writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr); >> + readb(&i2c_regs->i2dr); /* dummy read to clear ICF */ >> + >> + /* read data */ >> + for (i = 0; i < len; i++) { >> + ret = wait_for_sr_state(i2c_regs, ST_IIF); >> + if (ret < 0) { >> + debug("i2c_read_data(): ret=%d\n", ret); >> + i2c_imx_stop(i2c_bus); >> + return ret; >> + } >> + >> + /* >> + * It must generate STOP before read I2DR to prevent >> + * controller from generating another clock cycle >> + */ >> + if (i == (len - 1)) { >> + i2c_imx_stop(i2c_bus); >> + } else if (i == (len - 2)) { >> + temp = readb(&i2c_regs->i2cr); >> + temp |= I2CR_TX_NO_AK; >> + writeb(temp, &i2c_regs->i2cr); >> + } >> + writeb(I2SR_IIF_CLEAR, &i2c_regs->i2sr); >> + buf[i] = readb(&i2c_regs->i2dr); >> + } >> + >> + /* reuse ret for counter*/ >> + for (ret = 0; ret < len; ++ret) >> + debug(" 0x%02x", buf[ret]); >> + debug("\n"); >> + >> + i2c_imx_stop(i2c_bus); >> + return 0; >> +} > Again we have duplicated code. While we have both driver model and > non-drivel-model code co-existing, we should try to avoid this. ok. will try to merge them into a common one. > >> + >> +static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs) >> +{ >> + struct i2c_bus *i2c_bus = dev_get_priv(bus); >> + int ret; >> + >> + ret = i2c_init_transfer(i2c_bus, msg->addr, false); >> + if (ret < 0) >> + return ret; >> + >> + for (; nmsgs > 0; nmsgs--, msg++) { >> + bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD); >> + debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len); >> + if (msg->flags & I2C_M_RD) >> + ret = i2c_read_data(i2c_bus, msg->addr, msg->buf, >> + msg->len); >> + else >> + ret = i2c_write_data(i2c_bus, msg->addr, msg->buf, >> + msg->len, next_is_read); >> + if (ret) { >> + debug("i2c_write: error sending\n"); >> + return -EREMOTEIO; > return ret? Is the error the wrong value? should return ret. Thanks for correcting me. > >> + } >> + } >> + >> + i2c_imx_stop(i2c_bus); >> + >> + return 0; >> +} >> + >> +static const struct dm_i2c_ops mxc_i2c_ops = { >> + .xfer = mxc_i2c_xfer, >> + .probe_chip = mxc_i2c_probe_chip, >> + .set_bus_speed = mxc_i2c_set_bus_speed, >> +}; >> + >> +static const struct udevice_id mxc_i2c_ids[] = { >> + { .compatible = "fsl,imx21-i2c", }, >> + { .compatible = "fsl,vf610-i2c", }, >> + {} >> +}; >> + >> +U_BOOT_DRIVER(i2c_mxc) = { >> + .name = "i2c_mxc", >> + .id = UCLASS_I2C, >> + .of_match = mxc_i2c_ids, >> + .probe = mxc_i2c_probe, >> + .priv_auto_alloc_size = sizeof(struct i2c_bus), >> + .ops = &mxc_i2c_ops, >> +}; >> +#endif >> -- >> 1.8.4 >> >> > Regards, > Simon > . > Regards, Peng.