[U-Boot] [PATCH] dm: i2c: mxc support DM
Peng Fan
Peng.Fan at freescale.com
Mon Apr 20 07:49:02 CEST 2015
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 <Peng.Fan at freescale.com> 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 <Peng.Fan at freescale.com>
>> ---
>> 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 <asm/io.h>
>> #include <i2c.h>
>> #include <watchdog.h>
>> +#include <dm.h>
>> +#include <fdtdec.h>
>>
>> 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.
More information about the U-Boot
mailing list