[U-Boot] [v3] armv8: ls1012a: enable/disable eSDHC1 through hwconfig for new RDB

[York Sun]

On 12/06/2017 02:19 AM, Yangbo Lu wrote:
> For LS1012ARDB RevD and later versions, the I2C reading for DIP
> switch setting had been no longer reliable since the board was
> reworked. This patch is to add hwconfig support to enable/disable

I think this message is not accurate. How about saying "I2C reading for
DIP switch setting is not reliable for LS1012ARDB rev D and later
revisions."?

> eSDHC1 manually for these boards. Also let kernel decide status
> of esdhc0.

I see you dropped fixing up esdhc0. What do you mean "let kernel
decide"? How?

> 
> Signed-off-by: Yangbo Lu <yangbo.lu at nxp.com>
> ---
> Changes for v2:
> 	- Just used hwconfig() instead of getenv() and hwconfig_f().
> Changes for v3:
> 	- only applied hwconfig support for RevD and later versions.
> ---
>  board/freescale/ls1012ardb/ls1012ardb.c | 51 +++++++++++++++++++++------------
>  include/configs/ls1012ardb.h            |  4 +++
>  2 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/board/freescale/ls1012ardb/ls1012ardb.c b/board/freescale/ls1012ardb/ls1012ardb.c
> index c6c1c71202..b73c7d6639 100644
> --- a/board/freescale/ls1012ardb/ls1012ardb.c
> +++ b/board/freescale/ls1012ardb/ls1012ardb.c
> @@ -132,39 +132,52 @@ int board_init(void)
>  
>  int esdhc_status_fixup(void *blob, const char *compat)
>  {
> -	char esdhc0_path[] = "/soc/esdhc at 1560000";
>  	char esdhc1_path[] = "/soc/esdhc at 1580000";
> +	u8 in1;

I think you can reuse "io". Not a big deal though.

>  	u8 io = 0;
>  	u8 mux_sdhc2;
> -
> -	do_fixup_by_path(blob, esdhc0_path, "status", "okay",
> -			 sizeof("okay"), 1);
> +	bool sdhc2_en = false;
>  
>  	i2c_set_bus_num(0);
>  
> -	/*
> -	 * The I2C IO-expander for mux select is used to control the muxing
> -	 * of various onboard interfaces.
> -	 *
> -	 * IO1[3:2] indicates SDHC2 interface demultiplexer select lines.
> -	 *	00 - SDIO wifi
> -	 *	01 - GPIO (to Arduino)
> -	 *	10 - eMMC Memory
> -	 *	11 - SPI
> -	 */
> -	if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) {
> +	if (i2c_read(I2C_MUX_IO1_ADDR, 1, 1, &in1, 1) < 0) {

It would be helpful to use a named macro instead of "1" here as the
register address. It is not obvious that you are reading a revision
number. You can also put a comment.

You said I2C reading the DIP is not reliable. Is this reading reliable?

>  		printf("Error reading i2c boot information!\n");
> -		return 0; /* Don't want to hang() on this error */
> +		return 0;

What does I2C failure mean here? Returning 0 will cause the code keep
going in fdt_fixup_esdhc().

> +	}
> +
> +	if ((in1 & (~__SW_REV_MASK)) <= __SW_REV_D) {
> +		if (hwconfig("esdhc1"))

Please double check. I remember in the past if hwconfig is not enabled,
any check returns true.

> +			sdhc2_en = true;
> +	} else {
> +		/*
> +		 * The I2C IO-expander for mux select is used to control
> +		 * the muxing of various onboard interfaces.
> +		 *
> +		 * IO1[3:2] indicates SDHC2 interface demultiplexer
> +		 * select lines.
> +		 *	00 - SDIO wifi
> +		 *	01 - GPIO (to Arduino)
> +		 *	10 - eMMC Memory
> +		 *	11 - SPI
> +		 */
> +		if (i2c_read(I2C_MUX_IO1_ADDR, 0, 1, &io, 1) < 0) {
> +			printf("Error reading i2c boot information!\n");
> +			return 0; /* Don't want to hang() on this error */
> +		}
> +
> +		mux_sdhc2 = (io & 0x0c) >> 2;
> +		/* Enable SDHC2 only when use SDIO wifi and eMMC */
> +		if (mux_sdhc2 == 2 || mux_sdhc2 == 0)
> +			sdhc2_en = true;
>  	}
>  
> -	mux_sdhc2 = (io & 0x0c) >> 2;
> -	/* Enable SDHC2 only when use SDIO wifi and eMMC */
> -	if (mux_sdhc2 == 2 || mux_sdhc2 == 0)
> +	if (sdhc2_en)
>  		do_fixup_by_path(blob, esdhc1_path, "status", "okay",
>  				 sizeof("okay"), 1);
>  	else
>  		do_fixup_by_path(blob, esdhc1_path, "status", "disabled",
>  				 sizeof("disabled"), 1);
> +
>  	return 0;
>  }
>  
> diff --git a/include/configs/ls1012ardb.h b/include/configs/ls1012ardb.h
> index 794117062f..d5384e6027 100644
> --- a/include/configs/ls1012ardb.h
> +++ b/include/configs/ls1012ardb.h
> @@ -32,6 +32,10 @@
>  #define __SW_REV_MASK		0x07
>  #define __SW_REV_A		0xF8
>  #define __SW_REV_B		0xF0
> +#define __SW_REV_C		0xE8
> +#define __SW_REV_C1		0xE0
> +#define __SW_REV_C2		0xD8
> +#define __SW_REV_D		0xD0

I don't have strong opinion about these macros. I would not use
underscore myself. I guess I missed them at the first place.

York