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

Thu Dec 7 08:10:06 UTC 2017

Hi York,

> -----Original Message-----
> From: York Sun
> Sent: 2017年12月7日 3:08
> To: Y.b. Lu <yangbo.lu at nxp.com>; u-boot at lists.denx.de
> Subject: Re: [v3] armv8: ls1012a: enable/disable eSDHC1 through hwconfig for
> new RDB
> 
> 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."?
> 

[Y.b. Lu] Thanks. This is better.

> > 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?

[Y.b. Lu] I mean unless to avoid some issue, we don’t need to do any fix-up for 'status'.
Let me rephrase it as "Also drop 'status' fix-up for esdhc0 and leave it as it is. It shouldn’t be always fixed up with 'okay'. ".

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

[Y.b. Lu] Ok. Will do that.

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

[Y.b. Lu] Let me put a comment here for this patch. I think it's proper to replace all the numbers with macro in this file in another patch.

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

[Y.b. Lu] This is a question... Although I had verified this patch on RevE(the value read is correct), let me confirm with Kinjalk before sending new version patch.

> 
> >  		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().

[Y.b. Lu] I don’t know the possibility of I2C failure. If the failure happens, just print error message, leave 'status' as it is, and return 0 because the other fix-ups in fdt_fixup_esdhc() are still needed like clock-frequency.

> 
> > +	}
> > +
> > +	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.

[Y.b. Lu] You're right. It's -ENOSYS. But what should I do here ?

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

[Y.b. Lu] It's confusing why define macro like this...
And the __SW_BOOT_EMU is wrong, it should be 0x02, not 0x10.
> 
> York