From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Wed, 28 Oct 2015 14:46:43 +0100 Subject: [U-Boot] [PATCH v3 4/4] exynos: more debug and cleanup in do_sdhci_init() In-Reply-To: <5630BFCB.7050301@samsung.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> Message-ID: <5630D1C3.6020804@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 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>; } > 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 > --- > 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