[U-Boot] [PATCH v2 07/11] dm: imx: Add error checking to setup_i2c()

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Simon,

On 09/17/14 16:56, Simon Glass wrote:
> Hi Igor,
> 
> On 17 September 2014 02:37, Igor Grinberg <grinberg at compulab.co.il <mailto:grinberg@compulab.co.il>> wrote:
> 
>     Hi Simon,
> 
>     On 09/17/14 06:51, Simon Glass wrote:
>     > Since this function can fail, check its return value.
>     >
>     > Signed-off-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>     > ---
>     >
>     > Changes in v2:
>     > - Add new patch to add error checking to setup_i2c()
>     >
>     >  arch/arm/imx-common/i2c-mxv7.c            | 24 ++++++++++++++++-----
>     >  arch/arm/include/asm/imx-common/mxc_i2c.h |  4 ++--
>     >  board/compulab/cm_fx6/cm_fx6.c            | 35 +++++++++++++++++++++----------
>     >  3 files changed, 45 insertions(+), 18 deletions(-)
>     >
>     > diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c
>     > index a580873..70cff5c 100644
>     > --- a/arch/arm/imx-common/i2c-mxv7.c
>     > +++ b/arch/arm/imx-common/i2c-mxv7.c
>     > @@ -69,15 +69,29 @@ static void * const i2c_bases[] = {
>     >  };
>     >
>     >  /* i2c_index can be from 0 - 2 */
>     > -void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>     > -             struct i2c_pads_info *p)
>     > +int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>     > +           struct i2c_pads_info *p)
>     >  {
>     > +     int ret;
>     > +
>     >       if (i2c_index >= ARRAY_SIZE(i2c_bases))
>     > -             return;
>     > +             return -EINVAL;
>     >       /* Enable i2c clock */
>     > -     enable_i2c_clk(1, i2c_index);
>     > +     ret = enable_i2c_clk(1, i2c_index);
>     > +     if (ret)
>     > +             goto err_clk;
>     > +
>     >       /* Make sure bus is idle */
>     > -     force_idle_bus(p);
>     > +     ret = force_idle_bus(p);
>     > +     if (ret)
>     > +             goto err_idle;
>     > +
>     >       bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
>     >                       force_idle_bus, p);
>     > +
>     > +     return 0;
>     > +
>     > +err_idle:
>     > +err_clk:
>     > +     return ret;
>     >  }
>     > diff --git a/arch/arm/include/asm/imx-common/mxc_i2c.h b/arch/arm/include/asm/imx-common/mxc_i2c.h
>     > index 182c2f3..af86163 100644
>     > --- a/arch/arm/include/asm/imx-common/mxc_i2c.h
>     > +++ b/arch/arm/include/asm/imx-common/mxc_i2c.h
>     > @@ -52,8 +52,8 @@ struct i2c_pads_info {
>     >                                       &mx6q_##name : &mx6s_##name
>     >  #endif
>     >
>     > -void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>     > -             struct i2c_pads_info *p);
>     > +int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>     > +           struct i2c_pads_info *p);
>     >  void bus_i2c_init(void *base, int speed, int slave_addr,
>     >               int (*idle_bus_fn)(void *p), void *p);
>     >  int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,
>     > diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c
>     > index fdb8ebf..62c625a 100644
>     > --- a/board/compulab/cm_fx6/cm_fx6.c
>     > +++ b/board/compulab/cm_fx6/cm_fx6.c
>     > @@ -69,7 +69,7 @@ static iomux_v3_cfg_t const sata_pads[] = {
>     >       IOMUX_PADS(PAD_EIM_BCLK__GPIO6_IO31   | MUX_PAD_CTRL(NO_PAD_CTRL)),
>     >  };
>     >
>     > -static void cm_fx6_setup_issd(void)
>     > +static int cm_fx6_setup_issd(void)
>     >  {
>     >       SETUP_IOMUX_PADS(sata_pads);
>     >       /* Make sure this gpio has logical 0 value */
>     > @@ -79,14 +79,18 @@ static void cm_fx6_setup_issd(void)
>     >       cm_fx6_sata_power(0);
>     >       mdelay(250);
>     >       cm_fx6_sata_power(1);
>     > +
>     > +     return 0;
>     >  }
>     >
>     >  #define CM_FX6_SATA_INIT_RETRIES     10
>     >  int sata_initialize(void)
>     >  {
>     > -     int err, i;
>     > +     int err, i, ret;
>     >
>     > -     cm_fx6_setup_issd();
>     > +     ret = cm_fx6_setup_issd();
>     > +     if (ret)
>     > +             return ret;
> 
>     Hmm.. cm-fx6 may have iSSD not assembled and in this case
>     it has bypasses for a (m)SATA socket on the baseboard.
>     The socketed device of course is not controlled by those GPIOs.
>     Therefore, I think, it would be incorrect to fail the boot process
>     if there is a problem with an iSSD GPIO...
>     Instead a warning message will be enough. Something like:
> 
>     ret = cm_fx6_setup_issd();
>     if (ret)
>             printf("Warning: iSSD setup failed!\n");
> 
> 
> OK, my intent here was to add the missing gpio_request() calls so that things would work with DM. I think I am being drawn into board error policy! I'll give it another spin based on your comments.

I understand. Thanks for going for another round.
If you feel like it gets too much board specific, we can send a follow up.

>  
> 
> 
>     and then continue to the rest of SATA init.
> 
>     >       for (i = 0; i < CM_FX6_SATA_INIT_RETRIES; i++) {
>     >               err = setup_sata();
>     >               if (err) {
>     > @@ -141,14 +145,25 @@ I2C_PADS(i2c2_pads,
>     >        IMX_GPIO_NR(1, 6));
>     >
>     >
>     > -static void cm_fx6_setup_i2c(void)
>     > +static int cm_fx6_setup_i2c(void)
>     >  {
>     > -     setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c0_pads));
>     > -     setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c1_pads));
>     > -     setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c2_pads));
>     > +     int ret;
>     > +
>     > +     ret = setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f,
>     > +                     I2C_PADS_INFO(i2c0_pads));
>     > +     if (!ret) {
>     > +             ret = setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f,
>     > +                             I2C_PADS_INFO(i2c1_pads));
>     > +     }
>     > +     if (!ret) {
>     > +             ret = setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f,
>     > +                             I2C_PADS_INFO(i2c2_pads));
>     > +     }
> 
>     Almost same here, the fact that one of the (or even all) i2c buses
>     fails to initialize, should not lead to hang()...
>     The decision if this is a critical error or not should be decided
>     by the board code.
>     In this case, this is not a critical error and if one of the buses
>     fails to initialize, the others should still try.
>     So, here also, a warning message would be appropriate instead of
>     abort.
> 
>     > +
>     > +     return ret;
>     >  }
>     >  #else
>     > -static void cm_fx6_setup_i2c(void) { }
>     > +static int cm_fx6_setup_i2c(void) { return 0; }
>     >  #endif
>     >
>     >  #ifdef CONFIG_USB_EHCI_MX6
>     > @@ -411,9 +426,7 @@ int board_init(void)
>     >  {
>     >       gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
>     >       cm_fx6_setup_gpmi_nand();
>     > -     cm_fx6_setup_i2c();
>     > -
>     > -     return 0;
>     > +     return cm_fx6_setup_i2c();
> 
>     Same here, on cm-fx6, i2c is not that critical to U-Boot boot
>     process, and should not lead to hang().
> 
>     >  }
>     >
>     >  int checkboard(void)
>     >
> 
>     --
>     Regards,
>     Igor.
> 
> 
> 
> Regards,
> Simon
> 

-- 
Regards,
Igor.