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

[York Sun]

On 12/07/2017 12:10 AM, Y.b. Lu wrote:

<snip>

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

You can move on if it makes sense to do so.

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

It depends on which one is safe. If you want to presume esdhc1 is
available, you can keep current code. If you'd rather not to enable it,
add a check of #ifdef.

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

Add another patch before this one to clean them up, please.

York