From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Wed, 28 Oct 2015 15:48:32 +0100 Subject: [U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init() In-Reply-To: <5630DD7F.8050301@gmail.com> References: <1444045673-31770-1-git-send-email-tjakobi@math.uni-bielefeld.de> <1444045673-31770-4-git-send-email-tjakobi@math.uni-bielefeld.de> <56306C52.8030503@samsung.com> <5630B29E.3000901@samsung.com> <5630BFCB.7050301@samsung.com> <5630D1C3.6020804@samsung.com> <5630DD7F.8050301@gmail.com> Message-ID: <5630E040.3080701@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>> Signed-off-by: Tobias Jakobi >>>>>> --- >>>>>> 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 >>> --- >>> 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