[U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init()
Przemyslaw Marczak
p.marczak at samsung.com
Wed Oct 28 15:48:32 CET 2015
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 at samsung.com>
>>>>>> Signed-off-by: Tobias Jakobi <tjakobi at 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 at 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
More information about the U-Boot
mailing list