* [U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init()
2015-09-25 17:18 [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init() Tobias Jakobi
@ 2015-09-25 17:18 ` Tobias Jakobi
2015-09-28 17:22 ` Przemyslaw Marczak
2015-09-25 17:18 ` [U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes() Tobias Jakobi
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Tobias Jakobi @ 2015-09-25 17:18 UTC (permalink / raw)
To: u-boot
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.
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
drivers/mmc/s5p_sdhci.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index e9c43a9..b461fde 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -172,6 +172,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);
@@ -185,11 +186,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)
@@ -201,8 +209,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
--
2.0.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* [U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init()
2015-09-25 17:18 ` [U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init() Tobias Jakobi
@ 2015-09-28 17:22 ` Przemyslaw Marczak
0 siblings, 0 replies; 8+ messages in thread
From: Przemyslaw Marczak @ 2015-09-28 17:22 UTC (permalink / raw)
To: u-boot
Hello Tobias,
On 09/25/2015 07:18 PM, Tobias Jakobi wrote:
> 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.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
> drivers/mmc/s5p_sdhci.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index e9c43a9..b461fde 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -172,6 +172,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);
>
> @@ -185,11 +186,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)
> @@ -201,8 +209,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
>
Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes()
2015-09-25 17:18 [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init() Tobias Jakobi
2015-09-25 17:18 ` [U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init() Tobias Jakobi
@ 2015-09-25 17:18 ` Tobias Jakobi
2015-09-28 17:22 ` Przemyslaw Marczak
2015-09-25 17:18 ` [U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init() Tobias Jakobi
2015-09-28 17:21 ` [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init() Przemyslaw Marczak
3 siblings, 1 reply; 8+ messages in thread
From: Tobias Jakobi @ 2015-09-25 17:18 UTC (permalink / raw)
To: u-boot
In case sdhci_get_config() or do_sdhci_init() fail, show
the error code that was returned.
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
drivers/mmc/s5p_sdhci.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
index b461fde..b60bc23 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -171,7 +171,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);
@@ -184,14 +184,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++;
}
}
--
2.0.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* [U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes()
2015-09-25 17:18 ` [U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes() Tobias Jakobi
@ 2015-09-28 17:22 ` Przemyslaw Marczak
0 siblings, 0 replies; 8+ messages in thread
From: Przemyslaw Marczak @ 2015-09-28 17:22 UTC (permalink / raw)
To: u-boot
Hello Tobias,
On 09/25/2015 07:18 PM, Tobias Jakobi wrote:
> In case sdhci_get_config() or do_sdhci_init() fail, show
> the error code that was returned.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
> drivers/mmc/s5p_sdhci.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index b461fde..b60bc23 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -171,7 +171,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);
> @@ -184,14 +184,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++;
> }
> }
>
Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init()
2015-09-25 17:18 [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init() Tobias Jakobi
2015-09-25 17:18 ` [U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init() Tobias Jakobi
2015-09-25 17:18 ` [U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes() Tobias Jakobi
@ 2015-09-25 17:18 ` Tobias Jakobi
2015-09-28 17:22 ` Przemyslaw Marczak
2015-09-28 17:21 ` [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init() Przemyslaw Marczak
3 siblings, 1 reply; 8+ messages in thread
From: Tobias Jakobi @ 2015-09-25 17:18 UTC (permalink / raw)
To: u-boot
Add more debug printfs in do_sdhci_init() for calls
that can potentially fail.
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 b60bc23..f88d76b 100644
--- a/drivers/mmc/s5p_sdhci.c
+++ b/drivers/mmc/s5p_sdhci.c
@@ -102,29 +102,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;
}
}
--
2.0.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* [U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init()
2015-09-25 17:18 ` [U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init() Tobias Jakobi
@ 2015-09-28 17:22 ` Przemyslaw Marczak
0 siblings, 0 replies; 8+ messages in thread
From: Przemyslaw Marczak @ 2015-09-28 17:22 UTC (permalink / raw)
To: u-boot
Hello,
On 09/25/2015 07:18 PM, Tobias Jakobi wrote:
> Add more debug printfs in do_sdhci_init() for calls
> that can potentially fail.
>
> 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 b60bc23..f88d76b 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -102,29 +102,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;
> }
> }
>
>
Acked-by: Przemyslaw Marczak <p.marczak@samsung.com>
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init()
2015-09-25 17:18 [U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init() Tobias Jakobi
` (2 preceding siblings ...)
2015-09-25 17:18 ` [U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init() Tobias Jakobi
@ 2015-09-28 17:21 ` Przemyslaw Marczak
3 siblings, 0 replies; 8+ messages in thread
From: Przemyslaw Marczak @ 2015-09-28 17:21 UTC (permalink / raw)
To: u-boot
Hello Tobias,
On 09/25/2015 07:18 PM, Tobias Jakobi wrote:
> The sdhci_host struct is allocated in s5p_sdhci_init() but the
> fields are not initialized.
>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
> drivers/mmc/s5p_sdhci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/s5p_sdhci.c b/drivers/mmc/s5p_sdhci.c
> index 4db51d6..e9c43a9 100644
> --- a/drivers/mmc/s5p_sdhci.c
> +++ b/drivers/mmc/s5p_sdhci.c
> @@ -76,6 +76,7 @@ static int s5p_sdhci_core_init(struct sdhci_host *host)
> host->set_control_reg = &s5p_sdhci_set_control_reg;
> host->set_clock = set_mmc_clk;
>
> + host->host_caps = 0;
> if (host->bus_width == 8)
> host->host_caps |= MMC_MODE_8BIT;
>
>
Good point, but we usually initialize only required fields of structures.
What about use of calloc() instead of malloc() for this structure
allocation, in s5p_sdhci_init() function?
Some time ago, the the memory reserved for malloc was initialized with
zeroes at early init, so potentially "bugs" like this one, were hidden
very well.
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
^ permalink raw reply [flat|nested] 8+ messages in thread