exynos: Properly zero initialize host in s5p_sdhci_init()
PATCH v3 1/4 exynos: Properly zero initialize host in s5p_sdhci_init()
This makes sure that setting the host_caps in s5p_sdhci_core_init() doesn't operate on potentially uninitialized memory.
--- drivers/mmc/s5p_sdhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/mmc/s5p_sdhci.c +2 -2
@@ -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;
Signature · 1 line
2.0.5
PATCH v3 2/4 exynos: Fix passing of errors in exynos_mmc_init()
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.
--- drivers/mmc/s5p_sdhci.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/mmc/s5p_sdhci.c +12 -6
@@ -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
Signature · 1 line
2.0.5
PATCH v3 2/4 exynos: Fix passing of errors in exynos_mmc_init()
Quoted 7 lines · 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(). Forprocess_nodes()do something similar and return early whendo_sdhci_init()fails.
Quoted 9 lines
v2: Only fail in
process_nodes()if we fail on all
available nodes.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
applied to u-boot-samsung.
Thanks,
Minkyu Kang.
PATCH v3 3/4 exynos: be more verbose in process_nodes()
In reply to Tobias JakobiIn case sdhci_get_config() or do_sdhci_init() fail, show the error code that was returned.
--- drivers/mmc/s5p_sdhci.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/mmc/s5p_sdhci.c +7 -5
@@ -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++; } }
Signature · 1 line
2.0.5
PATCH v3 3/4 exynos: be more verbose in process_nodes()
Quoted 9 lines · Tobias Jakobi
In case
sdhci_get_config()ordo_sdhci_init()fail, show the error code that was returned.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
applied to u-boot-samsung.
Thanks,
Minkyu Kang.
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
In reply to Tobias JakobiAdd more debug printfs in do_sdhci_init() for calls that can potentially fail.
--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/mmc/s5p_sdhci.c +11 -9
@@ -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; } }
Signature · 1 line
2.0.5
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
Quoted 9 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
applied to u-boot-samsung.
Thanks,
Minkyu Kang.
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
In reply to Tobias JakobiHi, All.
Quoted 33 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/mmc/s5p_sdhci.c +4
-5@@ -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;
drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 123
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +4
-1} }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
Quoted 12 lines
drivers/mmc/s5p_sdhci.c +3
-3- 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; } }
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
Hello Jaehoon,
Quoted 51 lines · Jaehoon Chung
Hi, All.
Quoted 33 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/mmc/s5p_sdhci.c +4
-5@@ -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;drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 123
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +4
-1} }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.
Quoted 5 lines
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?
Quoted 14 lines
drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS];
Quoted 12 lines
drivers/mmc/s5p_sdhci.c +3
-3- 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,
Signature · 4 lines
Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
Hi, Przemyslaw.
Quoted 54 lines · Przemyslaw Marczak
Hello Jaehoon,
Quoted 51 lines · Jaehoon Chung
Hi, All.
Quoted 33 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/mmc/s5p_sdhci.c +4
-5@@ -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;drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 123
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +4
-1} }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.
Quoted 7 lines
Quoted 5 lines
And
dm_gpio_get_value()should be returned error for only one case.Best Regards,
Jaehoon ChungCould 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.
--- drivers/mmc/s5p_sdhci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/s5p_sdhci.c +3 -2
@@ -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
Quoted 18 lines
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?
Quoted 14 lines
drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 131
Quoted 12 lines
drivers/mmc/s5p_sdhci.c +3
-3- 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,
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
Hello Jaehoon,
Quoted 59 lines · Jaehoon Chung
Hi, Przemyslaw.
Quoted 56 lines · Przemyslaw Marczak
Hello Jaehoon,
Quoted 53 lines · Jaehoon Chung
Hi, All.
Quoted 33 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/mmc/s5p_sdhci.c +4
-5@@ -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;drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 123
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +4
-1} }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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Best Regards,
Jaehoon ChungCould 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>;
}Quoted 2 lines
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.
Quoted 31 lines
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 toACTIVE_LOWIn case of exynos, "cd-gpio" is commonly used the active-low. So it changed the flags of gpio from
GPIOD_IS_INtoGPIOD_ACTIVE_LOW.Signed-off-by Jaehoon Chung <[email protected]>--- drivers/mmc/s5p_sdhci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/mmc/s5p_sdhci.c +0
-0
drivers/mmc/s5p_sdhci.c static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host) · line 167
Quoted 5 lines
drivers/mmc/s5p_sdhci.c +1
-1@@ -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.
Quoted 15 lines
drivers/mmc/s5p_sdhci.c +2
-2return 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 ; } --
Quoted 24 lines
Best Regards,
Jaehoon ChungQuoted 18 lines
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?
Quoted 14 lines
drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 131
Quoted 12 lines
drivers/mmc/s5p_sdhci.c +3
-3- 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,
Signature · 4 lines
Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
Hi, Przemyslaw.
Quoted 71 lines · Przemyslaw Marczak
Hello Jaehoon,
Quoted 59 lines · Jaehoon Chung
Hi, Przemyslaw.
Quoted 56 lines · Przemyslaw Marczak
Hello Jaehoon,
Quoted 53 lines · Jaehoon Chung
Hi, All.
Quoted 33 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/mmc/s5p_sdhci.c +4
-5@@ -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;drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 123
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +4
-1} }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.Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Best Regards,
Jaehoon ChungCould 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.
Quoted 8 lines
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.
Quoted 18 lines
Quoted 3 lines
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)
-----------------------------------------------------------
Quoted 7 lines
This is caused by this commit:
commitef5cd33064f83db6f6cfe774ecdb36e32ac1d232
dm: core: Enable optional use offdt_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.
Quoted 27 lines
Quoted 27 lines
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 toACTIVE_LOWIn case of exynos, "cd-gpio" is commonly used the active-low. So it changed the flags of gpio from
GPIOD_IS_INtoGPIOD_ACTIVE_LOW.Signed-off-by Jaehoon Chung <[email protected]>--- drivers/mmc/s5p_sdhci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Quoted 12 lines
drivers/mmc/s5p_sdhci.c static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host) · line 167
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +1
-1@@ -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.
Quoted 6 lines
We uses this flag to set the direction (input/output), and also the flag
GPIOD_ACTIVE_LOWis 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
Quoted 42 lines
Quoted 40 lines
drivers/mmc/s5p_sdhci.c +2
-1return 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 ; }Signature · 2 lines
Best Regards, Jaehoon ChungQuoted 17 lines
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?
Quoted 14 lines
drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 131
Quoted 12 lines
drivers/mmc/s5p_sdhci.c +3
-3- 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; } }Quoted 1 line
Best regards,
Best regards,
PATCH v3 4/4 exynos: more debug and cleanup in do_sdhci_init()
Hello Jaehoon,
Quoted 107 lines · Jaehoon Chung
Hi, Przemyslaw.
Quoted 71 lines · Przemyslaw Marczak
Hello Jaehoon,
Quoted 59 lines · Jaehoon Chung
Hi, Przemyslaw.
Quoted 56 lines · Przemyslaw Marczak
Hello Jaehoon,
Quoted 53 lines · Jaehoon Chung
Hi, All.
Quoted 33 lines · Tobias Jakobi
Add more debug printfs in
do_sdhci_init()for calls that can potentially fail.Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)drivers/mmc/s5p_sdhci.c +4
-5@@ -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;drivers/mmc/s5p_sdhci.c struct sdhci_host sdhci_host[SDHCI_MAX_HOSTS]; · line 123
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +4
-1} }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.Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Best Regards,
Jaehoon ChungCould 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.
Quoted 8 lines
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.
Quoted 22 lines
Quoted 3 lines
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:
commitef5cd33064f83db6f6cfe774ecdb36e32ac1d232
dm: core: Enable optional use offdt_translate_address()
Quoted 5 lines
Quoted 3 lines
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.
Quoted 40 lines
Quoted 27 lines
Quoted 27 lines
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 toACTIVE_LOWIn case of exynos, "cd-gpio" is commonly used the active-low. So it changed the flags of gpio from
GPIOD_IS_INtoGPIOD_ACTIVE_LOW.Signed-off-by Jaehoon Chung <[email protected]>--- drivers/mmc/s5p_sdhci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)Quoted 12 lines
drivers/mmc/s5p_sdhci.c static int sdhci_get_config(const void *blob, int node, struct sdhci_host *host) · line 167
Quoted 10 lines
drivers/mmc/s5p_sdhci.c +1
-1@@ -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.
Quoted 4 lines
Yep, it's not good idea.
I'm not sure, cd-gpio pin has to setGPIOD_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.
Quoted 7 lines
Quoted 6 lines
We uses this flag to set the direction (input/output), and also the flag
GPIOD_ACTIVE_LOWis 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.
Quoted 8 lines
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:)
Quoted 37 lines
Quoted 37 lines
Quoted 37 lines
drivers/mmc/s5p_sdhci.c +2
-2return 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 ChungQuoted 17 lines
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?
Quoted 14 lines
Quoted 12 lines
drivers/mmc/s5p_sdhci.c +3
-3- 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; } }
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Best regards,
Best regards,
Best regards,
Signature · 4 lines
Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com
PATCH v3 1/4 exynos: Properly zero initialize host in s5p_sdhci_init()
In reply to Tobias JakobiQuoted 9 lines · Tobias Jakobi
This makes sure that setting the
host_capsins5p_sdhci_core_init()doesn't operate on potentially uninitialized memory.Acked-by Lukasz Majewski <[email protected]>Signed-off-by Tobias Jakobi <[email protected]>--- drivers/mmc/s5p_sdhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
applied to u-boot-samsung.
Thanks,
Minkyu Kang.