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

[Przemyslaw Marczak <p.marczak@samsung.com>]

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