[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Tobias Jakobi]

Hello,

currently operation on Exynos4412-based Odroid devices is broken.

The bootloader stops with this message:
Card did not respond to voltage select!
*** Warning - MMC init failed, using default environment

This series fixes error handling in the s5p sdhci driver
and the cause of the issue, which is an inverted card
detection check.

Thanks goes to Marek Vasut and Sjoerd Simons who helped
me on IRC to get this triaged.

With best wishes,
Tobias

Tobias Jakobi (4):
  exynos: Properly initialize host_caps in s5p_sdhci_core_init()
  exynos: Fix passing of errors in exynos_mmc_init()
  exynos: be more verbose in process_nodes()
  exynos: fix and cleanup do_sdhci_init()

 drivers/mmc/s5p_sdhci.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

-- 
2.0.5

[U-Boot] [PATCH 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init()

[Tobias Jakobi]

The sdhci_host struct is allocated in s5p_sdhci_init() but the
fields are not initialized.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/mmc/s5p_sdhci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index 4db51d6..e9c43a9 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -76,6 +76,7 @@ static int s5p_sdhci_core_init(struct sdhci_host *host)
 	host->set_control_reg = &s5p_sdhci_set_control_reg;
 	host->set_clock = set_mmc_clk;
 
+	host->host_caps = 0;
 	if (host->bus_width == 8)
 		host->host_caps |= MMC_MODE_8BIT;
 
-- 
2.0.5

[U-Boot] [PATCH 2/4] exynos: Fix passing of errors in exynos_mmc_init()

[Tobias Jakobi]

exynos_mmc_init() always returns zero, so for the caller
it looks like it never fails.

Correct this by returning the error code of process_nodes().
For process_nodes() do something similar and return early
when do_sdhci_init() fails.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/mmc/s5p_sdhci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index e9c43a9..bc2102a 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -187,7 +187,11 @@ static int process_nodes(const void *blob, int node_list[], int count)
 			printf("%s: failed to decode dev %d\n",	__func__, i);
 			return -1;
 		}
-		do_sdhci_init(host);
+
+		if (do_sdhci_init(host)) {
+			printf("%s: failed to initialize dev %d\n", __func__, i);
+			return -2;
+		}
 	}
 	return 0;
 }
@@ -201,8 +205,6 @@ int exynos_mmc_init(const void *blob)
 			COMPAT_SAMSUNG_EXYNOS_MMC, node_list,
 			SDHCI_MAX_HOSTS);
 
-	process_nodes(blob, node_list, count);
-
-	return 0;
+	return process_nodes(blob, node_list, count);
 }
 #endif
-- 
2.0.5

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Jaehoon Chung]

Hi,

On 09/21/2015 08:18 AM, Tobias Jakobi wrote:
> Hello,
> 
> currently operation on Exynos4412-based Odroid devices is broken.
> 
> The bootloader stops with this message:
> Card did not respond to voltage select!
> *** Warning - MMC init failed, using default environment

Which board did you use? and Which cad is failed? SD or eMMC?
Your [PATCH 4/4] is strange..so i want to get more information.

If you can share more information, i can explain more exactly about your problem.

Best Regards,
Jaehoon Chung

> 
> This series fixes error handling in the s5p sdhci driver
> and the cause of the issue, which is an inverted card
> detection check.
> 
> Thanks goes to Marek Vasut and Sjoerd Simons who helped
> me on IRC to get this triaged.
> 
> With best wishes,
> Tobias
> 
> Tobias Jakobi (4):
>   exynos: Properly initialize host_caps in s5p_sdhci_core_init()
>   exynos: Fix passing of errors in exynos_mmc_init()
>   exynos: be more verbose in process_nodes()
>   exynos: fix and cleanup do_sdhci_init()
> 
>  drivers/mmc/s5p_sdhci.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Tobias Jakobi]

Hello,

Jaehoon Chung wrote:
> Hi,
> 
> On 09/21/2015 08:18 AM, Tobias Jakobi wrote:
>> Hello,
>>
>> currently operation on Exynos4412-based Odroid devices is broken.
>>
>> The bootloader stops with this message:
>> Card did not respond to voltage select!
>> *** Warning - MMC init failed, using default environment
> 
> Which board did you use? and Which cad is failed? SD or eMMC?
This is an Odroid-X2. I'm booting from SD with a Sandisk Extreme. I
don't have any eMMC here.

Actually it was some other person who encountered this issue with his
Odroid-X2 and booting from SD. After he made me aware of it, I also
updated my u-boot (I was then using a version from May 2015, which works
perfectly btw) and encountered the very same issue as him.



> Your [PATCH 4/4] is strange..so i want to get more information.
For me it restores proper operation of the bootloader to the board.


With best wishes,
Tobias


> 
> If you can share more information, i can explain more exactly about your problem.
> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> This series fixes error handling in the s5p sdhci driver
>> and the cause of the issue, which is an inverted card
>> detection check.
>>
>> Thanks goes to Marek Vasut and Sjoerd Simons who helped
>> me on IRC to get this triaged.
>>
>> With best wishes,
>> Tobias
>>
>> Tobias Jakobi (4):
>>   exynos: Properly initialize host_caps in s5p_sdhci_core_init()
>>   exynos: Fix passing of errors in exynos_mmc_init()
>>   exynos: be more verbose in process_nodes()
>>   exynos: fix and cleanup do_sdhci_init()
>>
>>  drivers/mmc/s5p_sdhci.c | 39 +++++++++++++++++++++++----------------
>>  1 file changed, 23 insertions(+), 16 deletions(-)
>>
> 

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Jaehoon Chung]

Dear, Tobias.

On 09/21/2015 06:34 PM, Tobias Jakobi wrote:
> Hello,
> 
> Jaehoon Chung wrote:
>> Hi,
>>
>> On 09/21/2015 08:18 AM, Tobias Jakobi wrote:
>>> Hello,
>>>
>>> currently operation on Exynos4412-based Odroid devices is broken.
>>>
>>> The bootloader stops with this message:
>>> Card did not respond to voltage select!
>>> *** Warning - MMC init failed, using default environment
>>
>> Which board did you use? and Which cad is failed? SD or eMMC?
> This is an Odroid-X2. I'm booting from SD with a Sandisk Extreme. I
> don't have any eMMC here.

But if your patch is applied, other exynos4412 and exynos using s5p_sdhci don't work fine for SD-card.
This case is the specific X2 board. I think schematic is wrong.
(I have checked the Odrodd_X-base_Rev_06.pdf)

Commonly, SD-detect pin is supplied to always-on power.
But odroid-x/u board used the same supplier with I/O line. It's wrong.

This is H/W mis-designed problem, not code problem.
Well, I will reproduce your problem with X2 board.
And if reproduced your problem, I will find other solution, not this. how about?

> 
> Actually it was some other person who encountered this issue with his
> Odroid-X2 and booting from SD. After he made me aware of it, I also
> updated my u-boot (I was then using a version from May 2015, which works
> perfectly btw) and encountered the very same issue as him.
> 

Need to consider other SoC. You seems to consider only Odroid based-on exyno4412.
Did you test other exynos4412 board?

Best Regards,
Jaehoon Chung

> 
> 
>> Your [PATCH 4/4] is strange..so i want to get more information.
> For me it restores proper operation of the bootloader to the board.
> 
> 
> With best wishes,
> Tobias
> 
> 
>>
>> If you can share more information, i can explain more exactly about your problem.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>> This series fixes error handling in the s5p sdhci driver
>>> and the cause of the issue, which is an inverted card
>>> detection check.
>>>
>>> Thanks goes to Marek Vasut and Sjoerd Simons who helped
>>> me on IRC to get this triaged.
>>>
>>> With best wishes,
>>> Tobias
>>>
>>> Tobias Jakobi (4):
>>>   exynos: Properly initialize host_caps in s5p_sdhci_core_init()
>>>   exynos: Fix passing of errors in exynos_mmc_init()
>>>   exynos: be more verbose in process_nodes()
>>>   exynos: fix and cleanup do_sdhci_init()
>>>
>>>  drivers/mmc/s5p_sdhci.c | 39 +++++++++++++++++++++++----------------
>>>  1 file changed, 23 insertions(+), 16 deletions(-)
>>>
>>
> 
> 

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Tobias Jakobi]

Hello Jaehoon,


Jaehoon Chung wrote:
> Dear, Tobias.
> 
> On 09/21/2015 06:34 PM, Tobias Jakobi wrote:
>> Hello,
>>
>> Jaehoon Chung wrote:
>>> Hi,
>>>
>>> On 09/21/2015 08:18 AM, Tobias Jakobi wrote:
>>>> Hello,
>>>>
>>>> currently operation on Exynos4412-based Odroid devices is broken.
>>>>
>>>> The bootloader stops with this message:
>>>> Card did not respond to voltage select!
>>>> *** Warning - MMC init failed, using default environment
>>>
>>> Which board did you use? and Which cad is failed? SD or eMMC?
>> This is an Odroid-X2. I'm booting from SD with a Sandisk Extreme. I
>> don't have any eMMC here.
> 
> But if your patch is applied, other exynos4412 and exynos using s5p_sdhci don't work fine for SD-card.
> This case is the specific X2 board. I think schematic is wrong.
> (I have checked the Odrodd_X-base_Rev_06.pdf)
> 
> Commonly, SD-detect pin is supplied to always-on power.
> But odroid-x/u board used the same supplier with I/O line. It's wrong.
I wouldn't know, I don't even know how to properly read these schematics.



> This is H/W mis-designed problem, not code problem.
> Well, I will reproduce your problem with X2 board.
> And if reproduced your problem, I will find other solution, not this. how about?
Sure, as long as this fixes the issue. Let me know if I can test anything.



>> Actually it was some other person who encountered this issue with his
>> Odroid-X2 and booting from SD. After he made me aware of it, I also
>> updated my u-boot (I was then using a version from May 2015, which works
>> perfectly btw) and encountered the very same issue as him.
>>
> 
> Need to consider other SoC. You seems to consider only Odroid based-on exyno4412.
> Did you test other exynos4412 board?
I have only one ARM board here, which is the Odroid-X2. Nothing else.


With best wishes,
Tobias


> 
> Best Regards,
> Jaehoon Chung
> 
>>
>>
>>> Your [PATCH 4/4] is strange..so i want to get more information.
>> For me it restores proper operation of the bootloader to the board.
>>
>>
>> With best wishes,
>> Tobias
>>
>>
>>>
>>> If you can share more information, i can explain more exactly about your problem.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>
>>>> This series fixes error handling in the s5p sdhci driver
>>>> and the cause of the issue, which is an inverted card
>>>> detection check.
>>>>
>>>> Thanks goes to Marek Vasut and Sjoerd Simons who helped
>>>> me on IRC to get this triaged.
>>>>
>>>> With best wishes,
>>>> Tobias
>>>>
>>>> Tobias Jakobi (4):
>>>>   exynos: Properly initialize host_caps in s5p_sdhci_core_init()
>>>>   exynos: Fix passing of errors in exynos_mmc_init()
>>>>   exynos: be more verbose in process_nodes()
>>>>   exynos: fix and cleanup do_sdhci_init()
>>>>
>>>>  drivers/mmc/s5p_sdhci.c | 39 +++++++++++++++++++++++----------------
>>>>  1 file changed, 23 insertions(+), 16 deletions(-)
>>>>
>>>
>>
>>
> 

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Tobias Jakobi]

Forgot to ask this: What about the first three patches?

- Tobias

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Jaehoon Chung]

Hi, Tobias,

On 09/21/2015 08:00 PM, Tobias Jakobi wrote:
> Forgot to ask this: What about the first three patches?

Will check and review them. 

Best Regards,
Jaehoon Chung
> 
> - Tobias
> 
> 

[U-Boot] [PATCH 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init()

[Lukasz Majewski]

Hi Tobias,

> The sdhci_host struct is allocated in s5p_sdhci_init() but the
> fields are not initialized.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index 4db51d6..e9c43a9 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -76,6 +76,7 @@ static int s5p_sdhci_core_init(struct sdhci_host
> *host) host->set_control_reg = &s5p_sdhci_set_control_reg;
>  	host->set_clock = set_mmc_clk;
>  
> +	host->host_caps = 0;
>  	if (host->bus_width == 8)
>  		host->host_caps |= MMC_MODE_8BIT;
>  

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/4] exynos: Fix passing of errors in exynos_mmc_init()

[Lukasz Majewski]

Hi Tobias,

> exynos_mmc_init() always returns zero, so for the caller
> it looks like it never fails.
> 
> Correct this by returning the error code of process_nodes().
> For process_nodes() do something similar and return early
> when do_sdhci_init() fails.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index e9c43a9..bc2102a 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -187,7 +187,11 @@ static int process_nodes(const void *blob, int
> node_list[], int count) printf("%s: failed to decode dev
> %d\n",	__func__, i); return -1;
>  		}
> -		do_sdhci_init(host);
> +
> +		if (do_sdhci_init(host)) {
> +			printf("%s: failed to initialize dev %d\n",
> __func__, i);
> +			return -2;

IMHO, it would be better to write this code as follows:

ret = do_sdhci_init(host);
if (ret) {

	printf();
	return ret;
}
	
In the above code you read the exact return code from do_sdhci_init()
and then you pass it to upper layer.

Returning only -2 is far less informational.

> +		}
>  	}
>  	return 0;
>  }
> @@ -201,8 +205,6 @@ int exynos_mmc_init(const void *blob)
>  			COMPAT_SAMSUNG_EXYNOS_MMC, node_list,
>  			SDHCI_MAX_HOSTS);
>  
> -	process_nodes(blob, node_list, count);
> -
> -	return 0;
> +	return process_nodes(blob, node_list, count);
>  }
>  #endif



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/4] exynos: Fix passing of errors in exynos_mmc_init()

[Jaehoon Chung]

Hi.

On 09/23/2015 06:39 PM, Lukasz Majewski wrote:
> Hi Tobias,
> 
>> exynos_mmc_init() always returns zero, so for the caller
>> it looks like it never fails.
>>
>> Correct this by returning the error code of process_nodes().
>> For process_nodes() do something similar and return early
>> when do_sdhci_init() fails.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/mmc/s5p_sdhci.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>> index e9c43a9..bc2102a 100644
>> --- a/drivers/mmc/s5p_sdhci.c
>> +++ b/drivers/mmc/s5p_sdhci.c
>> @@ -187,7 +187,11 @@ static int process_nodes(const void *blob, int
>> node_list[], int count) printf("%s: failed to decode dev
>> %d\n",	__func__, i); return -1;
>>  		}
>> -		do_sdhci_init(host);
>> +
>> +		if (do_sdhci_init(host)) {
>> +			printf("%s: failed to initialize dev %d\n",
>> __func__, i);
>> +			return -2;
> 
> IMHO, it would be better to write this code as follows:
> 
> ret = do_sdhci_init(host);
> if (ret) {
> 
> 	printf();
> 	return ret;
> }

I think it should be replaced to "continue;", not "return ret;"
If returned the fail, then next host can't initialize.(if there is next host..)
So maybe, it didn't use "return ret".

Best Regards,
Jaehoon Chung

> 	
> In the above code you read the exact return code from do_sdhci_init()
> and then you pass it to upper layer.
> 
> Returning only -2 is far less informational.
> 
>> +		}
>>  	}
>>  	return 0;
>>  }
>> @@ -201,8 +205,6 @@ int exynos_mmc_init(const void *blob)
>>  			COMPAT_SAMSUNG_EXYNOS_MMC, node_list,
>>  			SDHCI_MAX_HOSTS);
>>  
>> -	process_nodes(blob, node_list, count);
>> -
>> -	return 0;
>> +	return process_nodes(blob, node_list, count);
>>  }
>>  #endif
> 
> 
> 

[U-Boot] [PATCH 2/4] exynos: Fix passing of errors in exynos_mmc_init()

[Minkyu Kang]

On 23/09/15 18:54, Jaehoon Chung wrote:
> Hi.
> 
> On 09/23/2015 06:39 PM, Lukasz Majewski wrote:
>> Hi Tobias,
>>
>>> exynos_mmc_init() always returns zero, so for the caller
>>> it looks like it never fails.
>>>
>>> Correct this by returning the error code of process_nodes().
>>> For process_nodes() do something similar and return early
>>> when do_sdhci_init() fails.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  drivers/mmc/s5p_sdhci.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>>> index e9c43a9..bc2102a 100644
>>> --- a/drivers/mmc/s5p_sdhci.c
>>> +++ b/drivers/mmc/s5p_sdhci.c
>>> @@ -187,7 +187,11 @@ static int process_nodes(const void *blob, int
>>> node_list[], int count) printf("%s: failed to decode dev
>>> %d\n",	__func__, i); return -1;
>>>  		}
>>> -		do_sdhci_init(host);
>>> +
>>> +		if (do_sdhci_init(host)) {
>>> +			printf("%s: failed to initialize dev %d\n",
>>> __func__, i);
>>> +			return -2;
>>
>> IMHO, it would be better to write this code as follows:
>>
>> ret = do_sdhci_init(host);
>> if (ret) {
>>
>> 	printf();
>> 	return ret;
>> }
> 
> I think it should be replaced to "continue;", not "return ret;"
> If returned the fail, then next host can't initialize.(if there is next host..)
> So maybe, it didn't use "return ret".
> 

Right.
If succeeded to initialize at least one host, should not be returned an error.

Thanks,
Minkyu Kang.

[U-Boot] [PATCH 0/4] Fix operation on Odroid devices

[Przemyslaw Marczak]

Hello Tobias,

On 09/21/2015 01:18 AM, Tobias Jakobi wrote:
> Hello,
>
> currently operation on Exynos4412-based Odroid devices is broken.
>
> The bootloader stops with this message:
> Card did not respond to voltage select!
> *** Warning - MMC init failed, using default environment
>
> This series fixes error handling in the s5p sdhci driver
> and the cause of the issue, which is an inverted card
> detection check.
>
> Thanks goes to Marek Vasut and Sjoerd Simons who helped
> me on IRC to get this triaged.
>
> With best wishes,
> Tobias
>
> Tobias Jakobi (4):
>    exynos: Properly initialize host_caps in s5p_sdhci_core_init()
>    exynos: Fix passing of errors in exynos_mmc_init()
>    exynos: be more verbose in process_nodes()
>    exynos: fix and cleanup do_sdhci_init()
>
>   drivers/mmc/s5p_sdhci.c | 39 +++++++++++++++++++++++----------------
>   1 file changed, 23 insertions(+), 16 deletions(-)
>

NAK for the series, since the real problem is in device tree parsing, 
not in s5p_sdhci.c. I will send patches in a moment.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com