[U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init()

[U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init()

[Tobias Jakobi]

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;
 
-- 
2.0.5

[U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init()

[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().
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

[U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes()

[Tobias Jakobi]

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

[U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Tobias Jakobi]

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

[U-Boot] [PATCH v2 1/4] exynos: Properly initialize host_caps in s5p_sdhci_core_init()

[Przemyslaw Marczak]

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

[U-Boot] [PATCH v2 2/4] exynos: Fix passing of errors in exynos_mmc_init()

[Przemyslaw Marczak]

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

[U-Boot] [PATCH v2 3/4] exynos: be more verbose in process_nodes()

[Przemyslaw Marczak]

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

[U-Boot] [PATCH v2 4/4] exynos: more debug and cleanup in do_sdhci_init()

[Przemyslaw Marczak]

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