[U-Boot] [PATCH] dm: i2c: mxc support DM

[Peng Fan <Peng.Fan@freescale.com>]

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@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@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.