Hi Igor,
On 17 September 2014 02:37, Igor Grinberg <grinb...@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 <s...@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.
>
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
