[U-Boot] [PATCH v3 1/4] exynos: Properly zero initialize host in s5p_sdhci_init()

[U-Boot] [PATCH v3 1/4] exynos: Properly zero initialize host in s5p_sdhci_init()

[Tobias Jakobi]

This makes sure that setting the host_caps in s5p_sdhci_core_init()
doesn't operate on potentially uninitialized memory.

Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/mmc/s5p_sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index 4db51d6..911e7a8 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -84,9 +84,9 @@ static int s5p_sdhci_core_init(struct sdhci_host *host)
 
 int s5p_sdhci_init(u32 regbase, int index, int bus_width)
 {
-	struct sdhci_host *host = malloc(sizeof(struct sdhci_host));
+	struct sdhci_host *host = calloc(1, sizeof(struct sdhci_host));
 	if (!host) {
-		printf("sdhci__host malloc fail!\n");
+		printf("sdhci__host allocation fail!\n");
 		return 1;
 	}
 	host->ioaddr = (void *)regbase;
-- 
2.0.5

[U-Boot] [PATCH v3 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.

v2: Only fail in process_nodes() if we fail on all
    available nodes.

Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/mmc/s5p_sdhci.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index 911e7a8..bd9e014 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -171,6 +171,7 @@ static int process_nodes(const void *blob, int node_list[], int count)
 {
 	struct sdhci_host *host;
 	int i, node;
+	int failed = 0;
 
 	debug("%s: count = %d\n", __func__, count);
 
@@ -184,11 +185,18 @@ static int process_nodes(const void *blob, int node_list[], int count)
 
 		if (sdhci_get_config(blob, node, host)) {
 			printf("%s: failed to decode dev %d\n",	__func__, i);
-			return -1;
+			failed++;
+			continue;
+		}
+
+		if (do_sdhci_init(host)) {
+			printf("%s: failed to initialize dev %d\n", __func__, i);
+			failed++;
 		}
-		do_sdhci_init(host);
 	}
-	return 0;
+
+	/* we only consider it an error when all nodes fail */
+	return (failed == count ? -1 : 0);
 }
 
 int exynos_mmc_init(const void *blob)
@@ -200,8 +208,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 v3 3/4] exynos: be more verbose in process_nodes()

[Tobias Jakobi]

In case sdhci_get_config() or do_sdhci_init() fail, show
the error code that was returned.

Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/mmc/s5p_sdhci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index bd9e014..b203bee 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -170,7 +170,7 @@ static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host)
 static int process_nodes(const void *blob, int node_list[], int count)
 {
 	struct sdhci_host *host;
-	int i, node;
+	int i, node, ret;
 	int failed = 0;
 
 	debug("%s: count = %d\n", __func__, count);
@@ -183,14 +183,16 @@ static int process_nodes(const void *blob, int node_list[], int count)
 
 		host = &sdhci_host[i];
 
-		if (sdhci_get_config(blob, node, host)) {
-			printf("%s: failed to decode dev %d\n",	__func__, i);
+		ret = sdhci_get_config(blob, node, host);
+		if (ret) {
+			printf("%s: failed to decode dev %d (%d)\n",	__func__, i, ret);
 			failed++;
 			continue;
 		}
 
-		if (do_sdhci_init(host)) {
-			printf("%s: failed to initialize dev %d\n", __func__, i);
+		ret = do_sdhci_init(host);
+		if (ret) {
+			printf("%s: failed to initialize dev %d (%d)\n", __func__, i, ret);
 			failed++;
 		}
 	}
-- 
2.0.5

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Tobias Jakobi]

Add more debug printfs in do_sdhci_init() for calls
that can potentially fail.

Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index b203bee..15ecfee 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
 
 static int do_sdhci_init(struct sdhci_host *host)
 {
-	int dev_id, flag;
-	int err = 0;
+	int dev_id, flag, ret;
 
 	flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
 	dev_id = host->index + PERIPH_ID_SDMMC0;
 
 	if (dm_gpio_is_valid(&host->pwr_gpio)) {
 		dm_gpio_set_value(&host->pwr_gpio, 1);
-		err = exynos_pinmux_config(dev_id, flag);
-		if (err) {
+		ret = exynos_pinmux_config(dev_id, flag);
+		if (ret) {
 			debug("MMC not configured\n");
-			return err;
+			return ret;
 		}
 	}
 
 	if (dm_gpio_is_valid(&host->cd_gpio)) {
-		if (dm_gpio_get_value(&host->cd_gpio))
+		ret = dm_gpio_get_value(&host->cd_gpio);
+		if (ret) {
+			debug("no SD card detected (%d)\n", ret);
 			return -ENODEV;
+		}
 
-		err = exynos_pinmux_config(dev_id, flag);
-		if (err) {
+		ret = exynos_pinmux_config(dev_id, flag);
+		if (ret) {
 			printf("external SD not configured\n");
-			return err;
+			return ret;
 		}
 	}
 
-- 
2.0.5

[U-Boot] [PATCH v3 1/4] exynos: Properly zero initialize host in s5p_sdhci_init()

[Minkyu Kang]

On 05/10/15 20:47, Tobias Jakobi wrote:
> This makes sure that setting the host_caps in s5p_sdhci_core_init()
> doesn't operate on potentially uninitialized memory.
> 
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

applied to u-boot-samsung.

Thanks,
Minkyu Kang.

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

[Minkyu Kang]

On 05/10/15 20:47, Tobias Jakobi wrote:
> 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.
> 
> v2: Only fail in process_nodes() if we fail on all
>     available nodes.
> 
> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

applied to u-boot-samsung.

Thanks,
Minkyu Kang.

[U-Boot] [PATCH v3 3/4] exynos: be more verbose in process_nodes()

[Minkyu Kang]

On 05/10/15 20:47, Tobias Jakobi wrote:
> In case sdhci_get_config() or do_sdhci_init() fail, show
> the error code that was returned.
> 
> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 

applied to u-boot-samsung.

Thanks,
Minkyu Kang.

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Minkyu Kang]

On 05/10/15 20:47, Tobias Jakobi wrote:
> Add more debug printfs in do_sdhci_init() for calls
> that can potentially fail.
> 
> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 

applied to u-boot-samsung.

Thanks,
Minkyu Kang.

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Jaehoon Chung]

Hi, All.

On 10/05/2015 08:47 PM, Tobias Jakobi wrote:
> Add more debug printfs in do_sdhci_init() for calls
> that can potentially fail.
> 
> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index b203bee..15ecfee 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
>  
>  static int do_sdhci_init(struct sdhci_host *host)
>  {
> -	int dev_id, flag;
> -	int err = 0;
> +	int dev_id, flag, ret;
>  
>  	flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
>  	dev_id = host->index + PERIPH_ID_SDMMC0;
>  
>  	if (dm_gpio_is_valid(&host->pwr_gpio)) {
>  		dm_gpio_set_value(&host->pwr_gpio, 1);
> -		err = exynos_pinmux_config(dev_id, flag);
> -		if (err) {
> +		ret = exynos_pinmux_config(dev_id, flag);
> +		if (ret) {
>  			debug("MMC not configured\n");
> -			return err;
> +			return ret;
>  		}
>  	}
>  
>  	if (dm_gpio_is_valid(&host->cd_gpio)) {
> -		if (dm_gpio_get_value(&host->cd_gpio))
> +		ret = dm_gpio_get_value(&host->cd_gpio);
> +		if (ret) {
> +			debug("no SD card detected (%d)\n", ret);
>  			return -ENODEV;
> +		}

This patch was already applied. But i didn't know why used "ret" at here.
If cd-gpio is active-high, this should be always returned "no SD card detected".
Even if commonly cd-gpio is active-low, we don't know whether cd-gpio is active-low or not.

And dm_gpio_get_value() should be returned error for only one case.

Best Regards,
Jaehoon Chung

>  
> -		err = exynos_pinmux_config(dev_id, flag);
> -		if (err) {
> +		ret = exynos_pinmux_config(dev_id, flag);
> +		if (ret) {
>  			printf("external SD not configured\n");
> -			return err;
> +			return ret;
>  		}
>  	}
>  
> 

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Przemyslaw Marczak]

Hello Jaehoon,

On 10/28/2015 07:33 AM, Jaehoon Chung wrote:
> Hi, All.
>
> On 10/05/2015 08:47 PM, Tobias Jakobi wrote:
>> Add more debug printfs in do_sdhci_init() for calls
>> that can potentially fail.
>>
>> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>   drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>> index b203bee..15ecfee 100644
>> --- a/drivers/mmc/s5p_sdhci.c
>> +++ b/drivers/mmc/s5p_sdhci.c
>> @@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
>>
>>   static int do_sdhci_init(struct sdhci_host *host)
>>   {
>> -	int dev_id, flag;
>> -	int err = 0;
>> +	int dev_id, flag, ret;
>>
>>   	flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
>>   	dev_id = host->index + PERIPH_ID_SDMMC0;
>>
>>   	if (dm_gpio_is_valid(&host->pwr_gpio)) {
>>   		dm_gpio_set_value(&host->pwr_gpio, 1);
>> -		err = exynos_pinmux_config(dev_id, flag);
>> -		if (err) {
>> +		ret = exynos_pinmux_config(dev_id, flag);
>> +		if (ret) {
>>   			debug("MMC not configured\n");
>> -			return err;
>> +			return ret;
>>   		}
>>   	}
>>
>>   	if (dm_gpio_is_valid(&host->cd_gpio)) {
>> -		if (dm_gpio_get_value(&host->cd_gpio))
>> +		ret = dm_gpio_get_value(&host->cd_gpio);
>> +		if (ret) {
>> +			debug("no SD card detected (%d)\n", ret);
>>   			return -ENODEV;
>> +		}
>
> This patch was already applied. But i didn't know why used "ret" at here.
> If cd-gpio is active-high, this should be always returned "no SD card detected".
> Even if commonly cd-gpio is active-low, we don't know whether cd-gpio is active-low or not.
>
> And dm_gpio_get_value() should be returned error for only one case.
>
> Best Regards,
> Jaehoon Chung
>

Could you precise, where is the problem exactly? The active low or high 
can be set in device tree, so the code can be still common.

And the ret value can inform, if card is not detected or the error is in 
GPIO subsystem(ret < 0). So where is the problem?

>>
>> -		err = exynos_pinmux_config(dev_id, flag);
>> -		if (err) {
>> +		ret = exynos_pinmux_config(dev_id, flag);
>> +		if (ret) {
>>   			printf("external SD not configured\n");
>> -			return err;
>> +			return ret;
>>   		}
>>   	}
>>
>>
>
>
Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Jaehoon Chung]

Hi, Przemyslaw.

On 10/28/2015 08:33 PM, Przemyslaw Marczak wrote:
> Hello Jaehoon,
> 
> On 10/28/2015 07:33 AM, Jaehoon Chung wrote:
>> Hi, All.
>>
>> On 10/05/2015 08:47 PM, Tobias Jakobi wrote:
>>> Add more debug printfs in do_sdhci_init() for calls
>>> that can potentially fail.
>>>
>>> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>   drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>>> index b203bee..15ecfee 100644
>>> --- a/drivers/mmc/s5p_sdhci.c
>>> +++ b/drivers/mmc/s5p_sdhci.c
>>> @@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
>>>
>>>   static int do_sdhci_init(struct sdhci_host *host)
>>>   {
>>> -    int dev_id, flag;
>>> -    int err = 0;
>>> +    int dev_id, flag, ret;
>>>
>>>       flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
>>>       dev_id = host->index + PERIPH_ID_SDMMC0;
>>>
>>>       if (dm_gpio_is_valid(&host->pwr_gpio)) {
>>>           dm_gpio_set_value(&host->pwr_gpio, 1);
>>> -        err = exynos_pinmux_config(dev_id, flag);
>>> -        if (err) {
>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>> +        if (ret) {
>>>               debug("MMC not configured\n");
>>> -            return err;
>>> +            return ret;
>>>           }
>>>       }
>>>
>>>       if (dm_gpio_is_valid(&host->cd_gpio)) {
>>> -        if (dm_gpio_get_value(&host->cd_gpio))
>>> +        ret = dm_gpio_get_value(&host->cd_gpio);
>>> +        if (ret) {
>>> +            debug("no SD card detected (%d)\n", ret);
>>>               return -ENODEV;
>>> +        }
>>
>> This patch was already applied. But i didn't know why used "ret" at here.
>> If cd-gpio is active-high, this should be always returned "no SD card detected".
>> Even if commonly cd-gpio is active-low, we don't know whether cd-gpio is active-low or not.
>>
>> And dm_gpio_get_value() should be returned error for only one case.
>>
>> Best Regards,
>> Jaehoon Chung
>>
> 
> Could you precise, where is the problem exactly? The active low or high can be set in device tree, so the code can be still common.

How can active low or high be set in device tree? I can't find any information.

Anyway, I have tested on Odroid-x2 board. And i have tried to boot with SD-card.
In that case, host can't detect SD-card.

MMC:   process_nodes: failed to initialize dev 0 (-19)

After Enable debug message..

no SD card detected (1)
process_nodes: failed to initialize dev 0 (-19)

no SD card detected (1) -> 1 is just gpio value. it should be to 0.
Do you distinguish between 0 and 1? We don't know whether gpio is active-low or high.
I don't think that included information. 

It's just my thinking...other people should be helpful.

And I'm making some patches...and below is one of them for detecting SD-card.

Subject: [PATCH] mmc: s5p_sdhci: change the flags of cd-gpio to ACTIVE_LOW

In case of exynos, "cd-gpio" is commonly used the active-low.
So it changed the flags of gpio from GPIOD_IS_IN to GPIOD_ACTIVE_LOW.

Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
 drivers/mmc/s5p_sdhci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index 15ecfee..26a8fe8 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -164,7 +164,7 @@ static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host)
        gpio_request_by_name_nodev(blob, node, "pwr-gpios", 0, &host->pwr_gpio,
                                   GPIOD_IS_OUT);
        gpio_request_by_name_nodev(blob, node, "cd-gpios", 0, &host->cd_gpio,
-                                  GPIOD_IS_IN);
+                                  GPIOD_ACTIVE_LOW);

        return 0;
 }
@@ -187,7 +187,8 @@ static int process_nodes(const void *blob, int node_list[], int count)

                ret = sdhci_get_config(blob, node, host);
                if (ret) {
-                       printf("%s: failed to decode dev %d (%d)\n",    __func__, i, ret);
+                       printf("%s: failed to decode dev %d (%d)\n",
+                                       __func__, i, ret);
                        failed++;
                        continue;
                }
--

Best Regards,
Jaehoon Chung


> 
> And the ret value can inform, if card is not detected or the error is in GPIO subsystem(ret < 0). So where is the problem?
> 
>>>
>>> -        err = exynos_pinmux_config(dev_id, flag);
>>> -        if (err) {
>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>> +        if (ret) {
>>>               printf("external SD not configured\n");
>>> -            return err;
>>> +            return ret;
>>>           }
>>>       }
>>>
>>>
>>
>>
> Best regards,

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Przemyslaw Marczak]

Hello Jaehoon,

On 10/28/2015 01:30 PM, Jaehoon Chung wrote:
> Hi, Przemyslaw.
>
> On 10/28/2015 08:33 PM, Przemyslaw Marczak wrote:
>> Hello Jaehoon,
>>
>> On 10/28/2015 07:33 AM, Jaehoon Chung wrote:
>>> Hi, All.
>>>
>>> On 10/05/2015 08:47 PM, Tobias Jakobi wrote:
>>>> Add more debug printfs in do_sdhci_init() for calls
>>>> that can potentially fail.
>>>>
>>>> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> ---
>>>>    drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>>>>    1 file changed, 11 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>>>> index b203bee..15ecfee 100644
>>>> --- a/drivers/mmc/s5p_sdhci.c
>>>> +++ b/drivers/mmc/s5p_sdhci.c
>>>> @@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
>>>>
>>>>    static int do_sdhci_init(struct sdhci_host *host)
>>>>    {
>>>> -    int dev_id, flag;
>>>> -    int err = 0;
>>>> +    int dev_id, flag, ret;
>>>>
>>>>        flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
>>>>        dev_id = host->index + PERIPH_ID_SDMMC0;
>>>>
>>>>        if (dm_gpio_is_valid(&host->pwr_gpio)) {
>>>>            dm_gpio_set_value(&host->pwr_gpio, 1);
>>>> -        err = exynos_pinmux_config(dev_id, flag);
>>>> -        if (err) {
>>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>>> +        if (ret) {
>>>>                debug("MMC not configured\n");
>>>> -            return err;
>>>> +            return ret;
>>>>            }
>>>>        }
>>>>
>>>>        if (dm_gpio_is_valid(&host->cd_gpio)) {
>>>> -        if (dm_gpio_get_value(&host->cd_gpio))
>>>> +        ret = dm_gpio_get_value(&host->cd_gpio);
>>>> +        if (ret) {
>>>> +            debug("no SD card detected (%d)\n", ret);
>>>>                return -ENODEV;
>>>> +        }
>>>
>>> This patch was already applied. But i didn't know why used "ret" at here.
>>> If cd-gpio is active-high, this should be always returned "no SD card detected".
>>> Even if commonly cd-gpio is active-low, we don't know whether cd-gpio is active-low or not.
>>>
>>> And dm_gpio_get_value() should be returned error for only one case.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>
>> Could you precise, where is the problem exactly? The active low or high can be set in device tree, so the code can be still common.
>
> How can active low or high be set in device tree? I can't find any information.
>

It is defined here:
arch/arm/dts/include/dt-bindings/gpio/gpio.h
and is used by gpio's phandle, e.g.:
sdhci at 12530000 {
	samsung,bus-width = <4>;
	samsung,timing = <1 2 3>;
	cd-gpios = <&gpk2 2 0>;
}

> Anyway, I have tested on Odroid-x2 board. And i have tried to boot with SD-card.
> In that case, host can't detect SD-card.

Can you also see this error:
-----------------------------------------------------------
DRAM:  2 GiB
__of_translate_address: Bad cell count for gpf0
__of_translate_address: Bad cell count for gpj0
__of_translate_address: Bad cell count for gpk0
__of_translate_address: Bad cell count for gpm0
__of_translate_address: Bad cell count for gpx0
LDO20 at VDDQ_EMMC_1.8V: set 1800000 uV; enabling
LDO22 at VDDQ_EMMC_2.8V: set 2800000 uV; enabling
LDO21 at TFLASH_2.8V: set 2800000 uV; enabling
MMC:   process_nodes: failed to initialize dev 0 (-19)
-----------------------------------------------------------

This is caused by this commit:
commit ef5cd33064f83db6f6cfe774ecdb36e32ac1d232
dm: core: Enable optional use of fdt_translate_address()

And it's not related to GPIO configuration.

Right now, I'm looking on how to fix it.

>
> MMC:   process_nodes: failed to initialize dev 0 (-19)
>
> After Enable debug message..
>
> no SD card detected (1)
> process_nodes: failed to initialize dev 0 (-19)
>
> no SD card detected (1) -> 1 is just gpio value. it should be to 0.
> Do you distinguish between 0 and 1? We don't know whether gpio is active-low or high.
> I don't think that included information.
>
> It's just my thinking...other people should be helpful.
>
> And I'm making some patches...and below is one of them for detecting SD-card.
>
> Subject: [PATCH] mmc: s5p_sdhci: change the flags of cd-gpio to ACTIVE_LOW
>
> In case of exynos, "cd-gpio" is commonly used the active-low.
> So it changed the flags of gpio from GPIOD_IS_IN to GPIOD_ACTIVE_LOW.
>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> ---
>   drivers/mmc/s5p_sdhci.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index 15ecfee..26a8fe8 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -164,7 +164,7 @@ static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host)
>          gpio_request_by_name_nodev(blob, node, "pwr-gpios", 0, &host->pwr_gpio,
>                                     GPIOD_IS_OUT);
>          gpio_request_by_name_nodev(blob, node, "cd-gpios", 0, &host->cd_gpio,
> -                                  GPIOD_IS_IN);
> +                                  GPIOD_ACTIVE_LOW);

This change, is a bad idea.

We uses this flag to set the direction (input/output), and also the flag 
GPIOD_ACTIVE_LOW is checked for output only in this function.

Your commit fixes the SD detection issue by a mistake.

I will send the fix in a moment, then you will see what is wrong.

>
>          return 0;
>   }
> @@ -187,7 +187,8 @@ static int process_nodes(const void *blob, int node_list[], int count)
>
>                  ret = sdhci_get_config(blob, node, host);
>                  if (ret) {
> -                       printf("%s: failed to decode dev %d (%d)\n",    __func__, i, ret);
> +                       printf("%s: failed to decode dev %d (%d)\n",
> +                                       __func__, i, ret);
>                          failed++;
>                          continue;
>                  }
> --
>
> Best Regards,
> Jaehoon Chung
>
>
>>
>> And the ret value can inform, if card is not detected or the error is in GPIO subsystem(ret < 0). So where is the problem?
>>
>>>>
>>>> -        err = exynos_pinmux_config(dev_id, flag);
>>>> -        if (err) {
>>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>>> +        if (ret) {
>>>>                printf("external SD not configured\n");
>>>> -            return err;
>>>> +            return ret;
>>>>            }
>>>>        }
>>>>
>>>>
>>>
>>>
>> Best regards,
>
>

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

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Jaehoon Chung]

Hi, Przemyslaw.

On 10/28/2015 10:46 PM, Przemyslaw Marczak wrote:
> Hello Jaehoon,
> 
> On 10/28/2015 01:30 PM, Jaehoon Chung wrote:
>> Hi, Przemyslaw.
>>
>> On 10/28/2015 08:33 PM, Przemyslaw Marczak wrote:
>>> Hello Jaehoon,
>>>
>>> On 10/28/2015 07:33 AM, Jaehoon Chung wrote:
>>>> Hi, All.
>>>>
>>>> On 10/05/2015 08:47 PM, Tobias Jakobi wrote:
>>>>> Add more debug printfs in do_sdhci_init() for calls
>>>>> that can potentially fail.
>>>>>
>>>>> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>>> ---
>>>>>    drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>>>>>    1 file changed, 11 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>>>>> index b203bee..15ecfee 100644
>>>>> --- a/drivers/mmc/s5p_sdhci.c
>>>>> +++ b/drivers/mmc/s5p_sdhci.c
>>>>> @@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
>>>>>
>>>>>    static int do_sdhci_init(struct sdhci_host *host)
>>>>>    {
>>>>> -    int dev_id, flag;
>>>>> -    int err = 0;
>>>>> +    int dev_id, flag, ret;
>>>>>
>>>>>        flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
>>>>>        dev_id = host->index + PERIPH_ID_SDMMC0;
>>>>>
>>>>>        if (dm_gpio_is_valid(&host->pwr_gpio)) {
>>>>>            dm_gpio_set_value(&host->pwr_gpio, 1);
>>>>> -        err = exynos_pinmux_config(dev_id, flag);
>>>>> -        if (err) {
>>>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>>>> +        if (ret) {
>>>>>                debug("MMC not configured\n");
>>>>> -            return err;
>>>>> +            return ret;
>>>>>            }
>>>>>        }
>>>>>
>>>>>        if (dm_gpio_is_valid(&host->cd_gpio)) {
>>>>> -        if (dm_gpio_get_value(&host->cd_gpio))
>>>>> +        ret = dm_gpio_get_value(&host->cd_gpio);
>>>>> +        if (ret) {
>>>>> +            debug("no SD card detected (%d)\n", ret);
>>>>>                return -ENODEV;
>>>>> +        }
>>>>
>>>> This patch was already applied. But i didn't know why used "ret" at here.
>>>> If cd-gpio is active-high, this should be always returned "no SD card detected".
>>>> Even if commonly cd-gpio is active-low, we don't know whether cd-gpio is active-low or not.
>>>>
>>>> And dm_gpio_get_value() should be returned error for only one case.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>
>>> Could you precise, where is the problem exactly? The active low or high can be set in device tree, so the code can be still common.
>>
>> How can active low or high be set in device tree? I can't find any information.
>>
> 
> It is defined here:
> arch/arm/dts/include/dt-bindings/gpio/gpio.h
> and is used by gpio's phandle, e.g.:
> sdhci at 12530000 {
>     samsung,bus-width = <4>;
>     samsung,timing = <1 2 3>;
>     cd-gpios = <&gpk2 2 0>;
> }

Yes, I have checked.

> 
>> Anyway, I have tested on Odroid-x2 board. And i have tried to boot with SD-card.
>> In that case, host can't detect SD-card.
> 
> Can you also see this error:
> -----------------------------------------------------------
> DRAM:  2 GiB
> __of_translate_address: Bad cell count for gpf0
> __of_translate_address: Bad cell count for gpj0
> __of_translate_address: Bad cell count for gpk0
> __of_translate_address: Bad cell count for gpm0
> __of_translate_address: Bad cell count for gpx0
> LDO20 at VDDQ_EMMC_1.8V: set 1800000 uV; enabling
> LDO22 at VDDQ_EMMC_2.8V: set 2800000 uV; enabling
> LDO21 at TFLASH_2.8V: set 2800000 uV; enabling
> MMC:   process_nodes: failed to initialize dev 0 (-19)
> -----------------------------------------------------------
> 
> This is caused by this commit:
> commit ef5cd33064f83db6f6cfe774ecdb36e32ac1d232
> dm: core: Enable optional use of fdt_translate_address()
> 
> And it's not related to GPIO configuration.
> 
> Right now, I'm looking on how to fix it.

Yes, i also found those message. I'm also finding solution. 
> 
>>
>> MMC:   process_nodes: failed to initialize dev 0 (-19)
>>
>> After Enable debug message..
>>
>> no SD card detected (1)
>> process_nodes: failed to initialize dev 0 (-19)
>>
>> no SD card detected (1) -> 1 is just gpio value. it should be to 0.
>> Do you distinguish between 0 and 1? We don't know whether gpio is active-low or high.
>> I don't think that included information.
>>
>> It's just my thinking...other people should be helpful.
>>
>> And I'm making some patches...and below is one of them for detecting SD-card.
>>
>> Subject: [PATCH] mmc: s5p_sdhci: change the flags of cd-gpio to ACTIVE_LOW
>>
>> In case of exynos, "cd-gpio" is commonly used the active-low.
>> So it changed the flags of gpio from GPIOD_IS_IN to GPIOD_ACTIVE_LOW.
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> ---
>>   drivers/mmc/s5p_sdhci.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>> index 15ecfee..26a8fe8 100644
>> --- a/drivers/mmc/s5p_sdhci.c
>> +++ b/drivers/mmc/s5p_sdhci.c
>> @@ -164,7 +164,7 @@ static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host)
>>          gpio_request_by_name_nodev(blob, node, "pwr-gpios", 0, &host->pwr_gpio,
>>                                     GPIOD_IS_OUT);
>>          gpio_request_by_name_nodev(blob, node, "cd-gpios", 0, &host->cd_gpio,
>> -                                  GPIOD_IS_IN);
>> +                                  GPIOD_ACTIVE_LOW);
> 
> This change, is a bad idea.

Yep, it's not good idea. 
I'm not sure, cd-gpio pin has to set GPIOD_IS_IN. I can't find the any information about gpio direction.
Tomorrow, when i go to the office, i will check. 

> 
> We uses this flag to set the direction (input/output), and also the flag GPIOD_ACTIVE_LOW is checked for output only in this function.
> 
> Your commit fixes the SD detection issue by a mistake.
> 
> I will send the fix in a moment, then you will see what is wrong.

Well, i didn't minutely see u-boot code during long time.
So i may miss something..I will check more for u-boot code.

Thanks for pointing out. :)

Best Regards,
Jaehoon Chung

> 
>>
>>          return 0;
>>   }
>> @@ -187,7 +187,8 @@ static int process_nodes(const void *blob, int node_list[], int count)
>>
>>                  ret = sdhci_get_config(blob, node, host);
>>                  if (ret) {
>> -                       printf("%s: failed to decode dev %d (%d)\n",    __func__, i, ret);
>> +                       printf("%s: failed to decode dev %d (%d)\n",
>> +                                       __func__, i, ret);
>>                          failed++;
>>                          continue;
>>                  }
>> -- 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>
>>>
>>> And the ret value can inform, if card is not detected or the error is in GPIO subsystem(ret < 0). So where is the problem?
>>>
>>>>>
>>>>> -        err = exynos_pinmux_config(dev_id, flag);
>>>>> -        if (err) {
>>>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>>>> +        if (ret) {
>>>>>                printf("external SD not configured\n");
>>>>> -            return err;
>>>>> +            return ret;
>>>>>            }
>>>>>        }
>>>>>
>>>>>
>>>>
>>>>
>>> Best regards,
>>
>>
> 
> Best regards,

[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Przemyslaw Marczak]

Hello Jaehoon,

On 10/28/2015 03:36 PM, Jaehoon Chung wrote:
> Hi, Przemyslaw.
>
> On 10/28/2015 10:46 PM, Przemyslaw Marczak wrote:
>> Hello Jaehoon,
>>
>> On 10/28/2015 01:30 PM, Jaehoon Chung wrote:
>>> Hi, Przemyslaw.
>>>
>>> On 10/28/2015 08:33 PM, Przemyslaw Marczak wrote:
>>>> Hello Jaehoon,
>>>>
>>>> On 10/28/2015 07:33 AM, Jaehoon Chung wrote:
>>>>> Hi, All.
>>>>>
>>>>> On 10/05/2015 08:47 PM, Tobias Jakobi wrote:
>>>>>> Add more debug printfs in do_sdhci_init() for calls
>>>>>> that can potentially fail.
>>>>>>
>>>>>> Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
>>>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>>>> ---
>>>>>>     drivers/mmc/s5p_sdhci.c | 20 +++++++++++---------
>>>>>>     1 file changed, 11 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>>>>>> index b203bee..15ecfee 100644
>>>>>> --- a/drivers/mmc/s5p_sdhci.c
>>>>>> +++ b/drivers/mmc/s5p_sdhci.c
>>>>>> @@ -101,29 +101,31 @@ struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
>>>>>>
>>>>>>     static int do_sdhci_init(struct sdhci_host *host)
>>>>>>     {
>>>>>> -    int dev_id, flag;
>>>>>> -    int err = 0;
>>>>>> +    int dev_id, flag, ret;
>>>>>>
>>>>>>         flag = host->bus_width == 8 ? PINMUX_FLAG_8BIT_MODE : PINMUX_FLAG_NONE;
>>>>>>         dev_id = host->index + PERIPH_ID_SDMMC0;
>>>>>>
>>>>>>         if (dm_gpio_is_valid(&host->pwr_gpio)) {
>>>>>>             dm_gpio_set_value(&host->pwr_gpio, 1);
>>>>>> -        err = exynos_pinmux_config(dev_id, flag);
>>>>>> -        if (err) {
>>>>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>>>>> +        if (ret) {
>>>>>>                 debug("MMC not configured\n");
>>>>>> -            return err;
>>>>>> +            return ret;
>>>>>>             }
>>>>>>         }
>>>>>>
>>>>>>         if (dm_gpio_is_valid(&host->cd_gpio)) {
>>>>>> -        if (dm_gpio_get_value(&host->cd_gpio))
>>>>>> +        ret = dm_gpio_get_value(&host->cd_gpio);
>>>>>> +        if (ret) {
>>>>>> +            debug("no SD card detected (%d)\n", ret);
>>>>>>                 return -ENODEV;
>>>>>> +        }
>>>>>
>>>>> This patch was already applied. But i didn't know why used "ret" at here.
>>>>> If cd-gpio is active-high, this should be always returned "no SD card detected".
>>>>> Even if commonly cd-gpio is active-low, we don't know whether cd-gpio is active-low or not.
>>>>>
>>>>> And dm_gpio_get_value() should be returned error for only one case.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>
>>>> Could you precise, where is the problem exactly? The active low or high can be set in device tree, so the code can be still common.
>>>
>>> How can active low or high be set in device tree? I can't find any information.
>>>
>>
>> It is defined here:
>> arch/arm/dts/include/dt-bindings/gpio/gpio.h
>> and is used by gpio's phandle, e.g.:
>> sdhci at 12530000 {
>>      samsung,bus-width = <4>;
>>      samsung,timing = <1 2 3>;
>>      cd-gpios = <&gpk2 2 0>;
>> }
>
> Yes, I have checked.
>
>>
>>> Anyway, I have tested on Odroid-x2 board. And i have tried to boot with SD-card.
>>> In that case, host can't detect SD-card.
>>
>> Can you also see this error:
>> -----------------------------------------------------------
>> DRAM:  2 GiB
>> __of_translate_address: Bad cell count for gpf0
>> __of_translate_address: Bad cell count for gpj0
>> __of_translate_address: Bad cell count for gpk0
>> __of_translate_address: Bad cell count for gpm0
>> __of_translate_address: Bad cell count for gpx0
>> LDO20 at VDDQ_EMMC_1.8V: set 1800000 uV; enabling
>> LDO22 at VDDQ_EMMC_2.8V: set 2800000 uV; enabling
>> LDO21 at TFLASH_2.8V: set 2800000 uV; enabling
>> MMC:   process_nodes: failed to initialize dev 0 (-19)
>> -----------------------------------------------------------
>>
>> This is caused by this commit:
>> commit ef5cd33064f83db6f6cfe774ecdb36e32ac1d232
>> dm: core: Enable optional use of fdt_translate_address()
>>
>> And it's not related to GPIO configuration.
>>
>> Right now, I'm looking on how to fix it.
>
> Yes, i also found those message. I'm also finding solution.

Please check the patches for which you're Cc'ed :)

1 fdt fix:
https://patchwork.ozlabs.org/patch/537372/

2 patches for sd detection fix:
https://patchwork.ozlabs.org/patch/537381/
https://patchwork.ozlabs.org/patch/537379/

Tested for X2 and U3.

>>
>>>
>>> MMC:   process_nodes: failed to initialize dev 0 (-19)
>>>
>>> After Enable debug message..
>>>
>>> no SD card detected (1)
>>> process_nodes: failed to initialize dev 0 (-19)
>>>
>>> no SD card detected (1) -> 1 is just gpio value. it should be to 0.
>>> Do you distinguish between 0 and 1? We don't know whether gpio is active-low or high.
>>> I don't think that included information.
>>>
>>> It's just my thinking...other people should be helpful.
>>>
>>> And I'm making some patches...and below is one of them for detecting SD-card.
>>>
>>> Subject: [PATCH] mmc: s5p_sdhci: change the flags of cd-gpio to ACTIVE_LOW
>>>
>>> In case of exynos, "cd-gpio" is commonly used the active-low.
>>> So it changed the flags of gpio from GPIOD_IS_IN to GPIOD_ACTIVE_LOW.
>>>
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> ---
>>>    drivers/mmc/s5p_sdhci.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
>>> index 15ecfee..26a8fe8 100644
>>> --- a/drivers/mmc/s5p_sdhci.c
>>> +++ b/drivers/mmc/s5p_sdhci.c
>>> @@ -164,7 +164,7 @@ static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host)
>>>           gpio_request_by_name_nodev(blob, node, "pwr-gpios", 0, &host->pwr_gpio,
>>>                                      GPIOD_IS_OUT);
>>>           gpio_request_by_name_nodev(blob, node, "cd-gpios", 0, &host->cd_gpio,
>>> -                                  GPIOD_IS_IN);
>>> +                                  GPIOD_ACTIVE_LOW);
>>
>> This change, is a bad idea.
>
> Yep, it's not good idea.
> I'm not sure, cd-gpio pin has to set GPIOD_IS_IN. I can't find the any information about gpio direction.
> Tomorrow, when i go to the office, i will check.
>

Please see the schematic of U3 and also XU3, the CD-det pin is pulled 
up. When you insert the card, it's mechanically pulled down - so card is 
detected for low state.

>>
>> We uses this flag to set the direction (input/output), and also the flag GPIOD_ACTIVE_LOW is checked for output only in this function.
>>
>> Your commit fixes the SD detection issue by a mistake.
>>
>> I will send the fix in a moment, then you will see what is wrong.
>
> Well, i didn't minutely see u-boot code during long time.
> So i may miss something..I will check more for u-boot code.
>
> Thanks for pointing out. :)
>
> Best Regards,
> Jaehoon Chung
>

No problem, thanks for mention about this issue:)

>>
>>>
>>>           return 0;
>>>    }
>>> @@ -187,7 +187,8 @@ static int process_nodes(const void *blob, int node_list[], int count)
>>>
>>>                   ret = sdhci_get_config(blob, node, host);
>>>                   if (ret) {
>>> -                       printf("%s: failed to decode dev %d (%d)\n",    __func__, i, ret);
>>> +                       printf("%s: failed to decode dev %d (%d)\n",
>>> +                                       __func__, i, ret);
>>>                           failed++;
>>>                           continue;
>>>                   }
>>> --
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>
>>>>
>>>> And the ret value can inform, if card is not detected or the error is in GPIO subsystem(ret < 0). So where is the problem?
>>>>
>>>>>>
>>>>>> -        err = exynos_pinmux_config(dev_id, flag);
>>>>>> -        if (err) {
>>>>>> +        ret = exynos_pinmux_config(dev_id, flag);
>>>>>> +        if (ret) {
>>>>>>                 printf("external SD not configured\n");
>>>>>> -            return err;
>>>>>> +            return ret;
>>>>>>             }
>>>>>>         }
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>> Best regards,
>>>
>>>
>>
>> Best regards,
>

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