[U-Boot] [PATCH 0/3] spl: mmc: Fix warning and unify spl_mmc_find_device()

[U-Boot] [PATCH 0/3] spl: mmc: Fix warning and unify spl_mmc_find_device()

[Simon Glass]

This little series fixes a compiler warning in SPL MMC which affects
Rockchip. It also joins up the spl_mmc_find_device() function again. I feel
that splitting the function was the wrong approach as discussed here:

https://patchwork.ozlabs.org/patch/537276/

The first patch fixes the warning. The other two are suggested improvements
but are separate from that problem.


Simon Glass (3):
  spl: mmc: Fix compiler warning with CONFIG_DM_MMC
  spl: mmc: Rename 'mmc' variable to 'mmcp'
  spl: mmc: Unify non/driver model spl_mmc_find_device()

 common/spl/spl_mmc.c | 44 +++++++++++---------------------------------
 1 file changed, 11 insertions(+), 33 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Simon Glass]

Since commit 4188ba3 we get the following warning on rockchip boards:

common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)

Correct this by move the variable init earlier.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index b3c2c64..9df4786 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -84,6 +84,7 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
 	struct udevice *dev;
 	int err, mmc_dev;
 
+	*mmc = NULL;
 	mmc_dev = spl_mmc_get_device_index(boot_device);
 	if (mmc_dev < 0)
 		return mmc_dev;
@@ -104,7 +105,6 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
 		return err;
 	}
 
-	*mmc = NULL;
 	*mmc = mmc_get_mmc_dev(dev);
 	return *mmc != NULL ? 0 : -ENODEV;
 }
-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Nikita Kiryanov]

Hi Simon,

On Mon, Nov 30, 2015 at 10:19:06AM -0700, Simon Glass wrote:
> Since commit 4188ba3 we get the following warning on rockchip boards:
> 
> common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)
> 
> Correct this by move the variable init earlier.

This looks suspicious. If the problem is that the variable is
undeclared, the only way to fix it is to declare the variable, which is
not what this patch does. I would expect this error to persist with
the patch applied. Also, mmc is clearly declared in the function
parameter list. It sounds to me like the source of the compile error is
somewhere earlier in the code.

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  common/spl/spl_mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index b3c2c64..9df4786 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -84,6 +84,7 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
>  	struct udevice *dev;
>  	int err, mmc_dev;
>  
> +	*mmc = NULL;
>  	mmc_dev = spl_mmc_get_device_index(boot_device);
>  	if (mmc_dev < 0)
>  		return mmc_dev;
> @@ -104,7 +105,6 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
>  		return err;
>  	}
>  
> -	*mmc = NULL;
>  	*mmc = mmc_get_mmc_dev(dev);
>  	return *mmc != NULL ? 0 : -ENODEV;
>  }
> -- 
> 2.6.0.rc2.230.g3dd15c0
> 

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Tom Rini]

On Tue, Dec 01, 2015 at 02:02:50PM +0200, Nikita Kiryanov wrote:
> Hi Simon,
> 
> On Mon, Nov 30, 2015 at 10:19:06AM -0700, Simon Glass wrote:
> > Since commit 4188ba3 we get the following warning on rockchip boards:
> > 
> > common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)
> > 
> > Correct this by move the variable init earlier.
> 
> This looks suspicious. If the problem is that the variable is
> undeclared, the only way to fix it is to declare the variable, which is
> not what this patch does. I would expect this error to persist with
> the patch applied. Also, mmc is clearly declared in the function
> parameter list. It sounds to me like the source of the compile error is
> somewhere earlier in the code.

It's strange, yes.  With gcc-5.x we get an uninitalized warning which I
posted a patch for, on rockchip.  I'm not sure how on earth it would
show up as undeclared, but at least the uninitalized is just
over-zealous checking.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151201/c2bcd6a2/attachment.sig>

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Simon Glass]

Hi Nikita,

On 1 December 2015 at 05:02, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> Hi Simon,
>
> On Mon, Nov 30, 2015 at 10:19:06AM -0700, Simon Glass wrote:
>> Since commit 4188ba3 we get the following warning on rockchip boards:
>>
>> common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)
>>
>> Correct this by move the variable init earlier.
>
> This looks suspicious. If the problem is that the variable is
> undeclared, the only way to fix it is to declare the variable, which is
> not what this patch does. I would expect this error to persist with
> the patch applied. Also, mmc is clearly declared in the function
> parameter list. It sounds to me like the source of the compile error is
> somewhere earlier in the code.
>

Well I just tried again and the warning is:

common/spl/spl_mmc.c: In function ?spl_mmc_load_image?:
common/spl/spl_mmc.c:31:24: warning: ?mmc? may be used uninitialized
in this function [-Wmaybe-uninitialized]
  count = mmc->block_dev.block_read(0, sector, 1, header);
                        ^
common/spl/spl_mmc.c:251:14: note: ?mmc? was declared here
  struct mmc *mmc;
              ^

I'll fix the commit message. I'm not sure what I was looking at there...

>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  common/spl/spl_mmc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>> index b3c2c64..9df4786 100644
>> --- a/common/spl/spl_mmc.c
>> +++ b/common/spl/spl_mmc.c
>> @@ -84,6 +84,7 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
>>       struct udevice *dev;
>>       int err, mmc_dev;
>>
>> +     *mmc = NULL;
>>       mmc_dev = spl_mmc_get_device_index(boot_device);
>>       if (mmc_dev < 0)
>>               return mmc_dev;
>> @@ -104,7 +105,6 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
>>               return err;
>>       }
>>
>> -     *mmc = NULL;
>>       *mmc = mmc_get_mmc_dev(dev);
>>       return *mmc != NULL ? 0 : -ENODEV;
>>  }
>> --
>> 2.6.0.rc2.230.g3dd15c0
>>

Regards,
Simon

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Tom Rini]

On Tue, Dec 01, 2015 at 01:01:51PM -0700, Simon Glass wrote:
> Hi Nikita,
> 
> On 1 December 2015 at 05:02, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> > Hi Simon,
> >
> > On Mon, Nov 30, 2015 at 10:19:06AM -0700, Simon Glass wrote:
> >> Since commit 4188ba3 we get the following warning on rockchip boards:
> >>
> >> common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)
> >>
> >> Correct this by move the variable init earlier.
> >
> > This looks suspicious. If the problem is that the variable is
> > undeclared, the only way to fix it is to declare the variable, which is
> > not what this patch does. I would expect this error to persist with
> > the patch applied. Also, mmc is clearly declared in the function
> > parameter list. It sounds to me like the source of the compile error is
> > somewhere earlier in the code.
> >
> 
> Well I just tried again and the warning is:
> 
> common/spl/spl_mmc.c: In function ?spl_mmc_load_image?:
> common/spl/spl_mmc.c:31:24: warning: ?mmc? may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>   count = mmc->block_dev.block_read(0, sector, 1, header);
>                         ^
> common/spl/spl_mmc.c:251:14: note: ?mmc? was declared here
>   struct mmc *mmc;
>               ^
> 
> I'll fix the commit message. I'm not sure what I was looking at there...

While you're in there, please just change to setting this to NULL in the
declaration instead of right above the call.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151201/b9a59b9b/attachment.sig>

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Simon Glass]

Hi Tom,

On 1 December 2015 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> On Tue, Dec 01, 2015 at 01:01:51PM -0700, Simon Glass wrote:
>> Hi Nikita,
>>
>> On 1 December 2015 at 05:02, Nikita Kiryanov <nikita@compulab.co.il> wrote:
>> > Hi Simon,
>> >
>> > On Mon, Nov 30, 2015 at 10:19:06AM -0700, Simon Glass wrote:
>> >> Since commit 4188ba3 we get the following warning on rockchip boards:
>> >>
>> >> common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)
>> >>
>> >> Correct this by move the variable init earlier.
>> >
>> > This looks suspicious. If the problem is that the variable is
>> > undeclared, the only way to fix it is to declare the variable, which is
>> > not what this patch does. I would expect this error to persist with
>> > the patch applied. Also, mmc is clearly declared in the function
>> > parameter list. It sounds to me like the source of the compile error is
>> > somewhere earlier in the code.
>> >
>>
>> Well I just tried again and the warning is:
>>
>> common/spl/spl_mmc.c: In function ?spl_mmc_load_image?:
>> common/spl/spl_mmc.c:31:24: warning: ?mmc? may be used uninitialized
>> in this function [-Wmaybe-uninitialized]
>>   count = mmc->block_dev.block_read(0, sector, 1, header);
>>                         ^
>> common/spl/spl_mmc.c:251:14: note: ?mmc? was declared here
>>   struct mmc *mmc;
>>               ^
>>
>> I'll fix the commit message. I'm not sure what I was looking at there...
>
> While you're in there, please just change to setting this to NULL in the
> declaration instead of right above the call.

I don't think I can do this, as it is a parameter to a function call.

Regards,
Simon

[U-Boot] [PATCH 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Tom Rini]

On Tue, Dec 01, 2015 at 05:33:32PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On 1 December 2015 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> > On Tue, Dec 01, 2015 at 01:01:51PM -0700, Simon Glass wrote:
> >> Hi Nikita,
> >>
> >> On 1 December 2015 at 05:02, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> >> > Hi Simon,
> >> >
> >> > On Mon, Nov 30, 2015 at 10:19:06AM -0700, Simon Glass wrote:
> >> >> Since commit 4188ba3 we get the following warning on rockchip boards:
> >> >>
> >> >> common/spl/spl_mmc.c:111:10: error: 'mmc' undeclared (first use in this function)
> >> >>
> >> >> Correct this by move the variable init earlier.
> >> >
> >> > This looks suspicious. If the problem is that the variable is
> >> > undeclared, the only way to fix it is to declare the variable, which is
> >> > not what this patch does. I would expect this error to persist with
> >> > the patch applied. Also, mmc is clearly declared in the function
> >> > parameter list. It sounds to me like the source of the compile error is
> >> > somewhere earlier in the code.
> >> >
> >>
> >> Well I just tried again and the warning is:
> >>
> >> common/spl/spl_mmc.c: In function ?spl_mmc_load_image?:
> >> common/spl/spl_mmc.c:31:24: warning: ?mmc? may be used uninitialized
> >> in this function [-Wmaybe-uninitialized]
> >>   count = mmc->block_dev.block_read(0, sector, 1, header);
> >>                         ^
> >> common/spl/spl_mmc.c:251:14: note: ?mmc? was declared here
> >>   struct mmc *mmc;
> >>               ^
> >>
> >> I'll fix the commit message. I'm not sure what I was looking at there...
> >
> > While you're in there, please just change to setting this to NULL in the
> > declaration instead of right above the call.
> 
> I don't think I can do this, as it is a parameter to a function call.

Yes you can.  I fixed (and lost, blarg, got side-tracked and blew away
my changes) by making the declartion on 251 be initalized to NULL.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151201/22da393e/attachment.sig>

[U-Boot] [PATCH 2/3] spl: mmc: Rename 'mmc' variable to 'mmcp'

[Simon Glass]

The 'p' suffix makes it more obvious that we are dealing with a pointer
to a (pointer) value that will be returned to its caller.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/spl/spl_mmc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 9df4786..8d47059 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -79,12 +79,12 @@ int spl_mmc_get_device_index(u32 boot_device)
 }
 
 #ifdef CONFIG_DM_MMC
-static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
+static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 {
 	struct udevice *dev;
 	int err, mmc_dev;
 
-	*mmc = NULL;
+	*mmcp = NULL;
 	mmc_dev = spl_mmc_get_device_index(boot_device);
 	if (mmc_dev < 0)
 		return mmc_dev;
@@ -105,11 +105,11 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
 		return err;
 	}
 
-	*mmc = mmc_get_mmc_dev(dev);
-	return *mmc != NULL ? 0 : -ENODEV;
+	*mmcp = mmc_get_mmc_dev(dev);
+	return *mmcp != NULL ? 0 : -ENODEV;
 }
 #else
-static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
+static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 {
 	int err, mmc_dev;
 
@@ -126,8 +126,8 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device)
 	}
 
 	/* We register only one device. So, the dev id is always 0 */
-	*mmc = find_mmc_device(mmc_dev);
-	if (!*mmc) {
+	*mmcp = find_mmc_device(mmc_dev);
+	if (!*mmcp) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 		puts("spl: mmc device not found\n");
 #endif
-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH 2/3] spl: mmc: Rename 'mmc' variable to 'mmcp'

[Nikita Kiryanov]

On Mon, Nov 30, 2015 at 10:19:07AM -0700, Simon Glass wrote:
> The 'p' suffix makes it more obvious that we are dealing with a pointer
> to a (pointer) value that will be returned to its caller.

Acked-by: Nikita Kiryanov <nikita@compulab.co.il>

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  common/spl/spl_mmc.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 

[U-Boot] [PATCH 3/3] spl: mmc: Unify non/driver model spl_mmc_find_device()

[Simon Glass]

It is risky to have two different functions with much the same code. Future
authors may update one but not the other. It is hard to see which parts are
the same and which are different.

Unify the functions and drop the differences that are not really needed.

Note that one puts() becomes printf() as Tom mentioned that this does not
affect image size:

https://patchwork.ozlabs.org/patch/537276/

Note: It would be better to have an empty printf() and avoid the #ifdef for
CONFIG_SPL_LIBCOMMON_SUPPORT.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/spl/spl_mmc.c | 40 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 8d47059..d9416a8 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -78,10 +78,11 @@ int spl_mmc_get_device_index(u32 boot_device)
 	return -ENODEV;
 }
 
-#ifdef CONFIG_DM_MMC
 static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 {
+#ifdef CONFIG_DM_MMC
 	struct udevice *dev;
+#endif
 	int err, mmc_dev;
 
 	*mmcp = NULL;
@@ -97,46 +98,23 @@ static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 		return err;
 	}
 
+#ifdef CONFIG_DM_MMC
 	err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev);
-	if (err) {
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("spl: could not find mmc device. error: %d\n", err);
-#endif
-		return err;
-	}
-
-	*mmcp = mmc_get_mmc_dev(dev);
-	return *mmcp != NULL ? 0 : -ENODEV;
-}
+	if (!err)
+		*mmcp = mmc_get_mmc_dev(dev);
 #else
-static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
-{
-	int err, mmc_dev;
-
-	mmc_dev = spl_mmc_get_device_index(boot_device);
-	if (mmc_dev < 0)
-		return mmc_dev;
-
-	err = mmc_initialize(gd->bd);
+	*mmcp = find_mmc_device(mmc_dev);
+	err = *mmcp ? 0 : -ENODEV;
+#endif
 	if (err) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("spl: could not initialize mmc. error: %d\n", err);
+		printf("spl: could not find mmc device. error: %d\n", err);
 #endif
 		return err;
 	}
 
-	/* We register only one device. So, the dev id is always 0 */
-	*mmcp = find_mmc_device(mmc_dev);
-	if (!*mmcp) {
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		puts("spl: mmc device not found\n");
-#endif
-		return -ENODEV;
-	}
-
 	return 0;
 }
-#endif
 
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
 static int mmc_load_image_raw_partition(struct mmc *mmc, int partition)
-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH 0/3] spl: mmc: Fix warning and unify spl_mmc_find_device()

[Michal Simek]

On 30.11.2015 18:19, Simon Glass wrote:
> This little series fixes a compiler warning in SPL MMC which affects
> Rockchip. It also joins up the spl_mmc_find_device() function again. I feel
> that splitting the function was the wrong approach as discussed here:
> 
> https://patchwork.ozlabs.org/patch/537276/
> 
> The first patch fixes the warning. The other two are suggested improvements
> but are separate from that problem.
> 
> 
> Simon Glass (3):
>   spl: mmc: Fix compiler warning with CONFIG_DM_MMC
>   spl: mmc: Rename 'mmc' variable to 'mmcp'
>   spl: mmc: Unify non/driver model spl_mmc_find_device()
> 
>  common/spl/spl_mmc.c | 44 +++++++++++---------------------------------
>  1 file changed, 11 insertions(+), 33 deletions(-)
> 

For whole series:
Tested-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151202/83517739/attachment.sig>