Hi Igor,
On 17 September 2014 02:37, Igor Grinberg 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@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