All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mvebu: bubt: Drop dead code
@ 2020-07-24 21:13 Tom Rini
  2020-07-25 11:38 ` Stefan Roese
  2020-08-06 12:08 ` Stefan Roese
  0 siblings, 2 replies; 6+ messages in thread
From: Tom Rini @ 2020-07-24 21:13 UTC (permalink / raw)
  To: u-boot

The code around CONFIG_SYS_MMC_ENV_PART has been untested since merge.
This can be seen by it referencing 'mmc->part_num' which was migrated
elsewhere prior to this code being merged.

Cc: Joel Johnson <mrjoel@lixil.net>
Cc: Stefan Roese <sr@denx.de>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 cmd/mvebu/bubt.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index a27b0df8ae78..85ae588676fe 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -176,16 +176,6 @@ static int mmc_burn_image(size_t image_size)
 		return err;
 	}
 
-#ifdef CONFIG_SYS_MMC_ENV_PART
-	if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART) {
-		err = mmc_switch_part(mmc_dev_num, CONFIG_SYS_MMC_ENV_PART);
-		if (err) {
-			printf("MMC partition switch failed\n");
-			return err;
-		}
-	}
-#endif
-
 	/* SD reserves LBA-0 for MBR and boots from LBA-1,
 	 * MMC/eMMC boots from LBA-0
 	 */
@@ -217,11 +207,6 @@ static int mmc_burn_image(size_t image_size)
 	}
 	printf("Done!\n");
 
-#ifdef CONFIG_SYS_MMC_ENV_PART
-	if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART)
-		mmc_switch_part(mmc_dev_num, mmc->part_num);
-#endif
-
 	return 0;
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] mvebu: bubt: Drop dead code
  2020-07-24 21:13 [PATCH] mvebu: bubt: Drop dead code Tom Rini
@ 2020-07-25 11:38 ` Stefan Roese
  2020-07-25 12:59   ` Tom Rini
  2020-08-06 12:08 ` Stefan Roese
  1 sibling, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2020-07-25 11:38 UTC (permalink / raw)
  To: u-boot

Hi Tom,

On 24.07.20 23:13, Tom Rini wrote:
> The code around CONFIG_SYS_MMC_ENV_PART has been untested since merge.
> This can be seen by it referencing 'mmc->part_num' which was migrated
> elsewhere prior to this code being merged.

I'm seeing that CONFIG_SYS_MMC_ENV_PART is also mentioned in the
documentation for this MVEBU cmd:

doc/mvebu/cmd/bubt.txt

So I hesitate a bit to remove it completely from this command (even
though I personally have never used it). Could you perhaps send me a
link a patch / commit, where 'mmc->part_num' has been migrated?

Thanks,
Stefan

> Cc: Joel Johnson <mrjoel@lixil.net>
> Cc: Stefan Roese <sr@denx.de>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>   cmd/mvebu/bubt.c | 15 ---------------
>   1 file changed, 15 deletions(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index a27b0df8ae78..85ae588676fe 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -176,16 +176,6 @@ static int mmc_burn_image(size_t image_size)
>   		return err;
>   	}
>   
> -#ifdef CONFIG_SYS_MMC_ENV_PART
> -	if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART) {
> -		err = mmc_switch_part(mmc_dev_num, CONFIG_SYS_MMC_ENV_PART);
> -		if (err) {
> -			printf("MMC partition switch failed\n");
> -			return err;
> -		}
> -	}
> -#endif
> -
>   	/* SD reserves LBA-0 for MBR and boots from LBA-1,
>   	 * MMC/eMMC boots from LBA-0
>   	 */
> @@ -217,11 +207,6 @@ static int mmc_burn_image(size_t image_size)
>   	}
>   	printf("Done!\n");
>   
> -#ifdef CONFIG_SYS_MMC_ENV_PART
> -	if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART)
> -		mmc_switch_part(mmc_dev_num, mmc->part_num);
> -#endif
> -
>   	return 0;
>   }
>   
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] mvebu: bubt: Drop dead code
  2020-07-25 11:38 ` Stefan Roese
@ 2020-07-25 12:59   ` Tom Rini
  2020-07-30  6:50     ` Stefan Roese
  0 siblings, 1 reply; 6+ messages in thread
From: Tom Rini @ 2020-07-25 12:59 UTC (permalink / raw)
  To: u-boot

On Sat, Jul 25, 2020 at 01:38:12PM +0200, Stefan Roese wrote:
> Hi Tom,
> 
> On 24.07.20 23:13, Tom Rini wrote:
> > The code around CONFIG_SYS_MMC_ENV_PART has been untested since merge.
> > This can be seen by it referencing 'mmc->part_num' which was migrated
> > elsewhere prior to this code being merged.
> 
> I'm seeing that CONFIG_SYS_MMC_ENV_PART is also mentioned in the
> documentation for this MVEBU cmd:
> 
> doc/mvebu/cmd/bubt.txt
> 
> So I hesitate a bit to remove it completely from this command (even
> though I personally have never used it). Could you perhaps send me a
> link a patch / commit, where 'mmc->part_num' has been migrated?

It was changed in:
commit 873cc1d7775ed5de07e6722c7ff423080c2e8f71
Author:     Stephen Warren <swarren@nvidia.com>
AuthorDate: Mon Dec 7 11:38:49 2015 -0700
Commit:     Tom Rini <trini@konsulko.com>
CommitDate: Wed Jan 13 21:05:19 2016 -0500

    mmc: store hwpart in the block device
    
    This will allow us to have multiple block device structs each referring
    to the same eMMC device, yet different HW partitions.
    
    For now, there is still a single block device per eMMC device. As before,
    this block device always accesses whichever HW partition was most recently
    selected. Clients wishing to make use of multiple block devices referring
    to different HW partitions can simply take a copy of this block device
    once it points at the correct HW partition, and use each one as they wish.
    This feature will be used by the next patch.
    
    In the future, perhaps get_device() could be enhanced to return a
    dynamically allocated block device struct, to avoid the client needing to
    copy it in order to maintain multiple block devices. However, this would
    require all users to be updated to free those block device structs at some
    point, which is rather a large change.
    
    Most callers of mmc_switch_part() wish to permanently switch the default
    MMC block device's HW partition. Enhance mmc_switch_part() so that it does
    this. This removes the need for callers to do this. However,
    common/env_mmc.c needs to save and restore the current HW partition. Make
    it do this more explicitly.
    
    Replace use of mmc_switch_part() with mmc_select_hwpart() in order to
    remove duplicate code that skips the call if that HW partition is already
    selected.
    
    Signed-off-by: Stephen Warren <swarren@nvidia.com>
    Reviewed-by: Tom Rini <trini@konsulko.com>

Which is before cmd/mvebu/bubt.c was added to mainline.  So while I'm
sure the command worked in the downstream tree, it wasn't ever tested in
mainline.  It also means that changing it to mmc->block_dev.hwpart
instead of mmc->part_num would also work, but the code would remain
untested.  Note that I ran in to this in moving
CONFIG_SYS_MMC_ENV_PART/DEV to Kconfig and that leading to us always
having these variables set to at least 0, so if you're going to fix the
code to build please keep that in mind as well and that would should
probably drop the #if check and always change to the correct dev/part.
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200725/b235d20a/attachment.sig>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] mvebu: bubt: Drop dead code
  2020-07-25 12:59   ` Tom Rini
@ 2020-07-30  6:50     ` Stefan Roese
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Roese @ 2020-07-30  6:50 UTC (permalink / raw)
  To: u-boot

Hi Tom,

On 25.07.20 14:59, Tom Rini wrote:
> On Sat, Jul 25, 2020 at 01:38:12PM +0200, Stefan Roese wrote:
>> Hi Tom,
>>
>> On 24.07.20 23:13, Tom Rini wrote:
>>> The code around CONFIG_SYS_MMC_ENV_PART has been untested since merge.
>>> This can be seen by it referencing 'mmc->part_num' which was migrated
>>> elsewhere prior to this code being merged.
>>
>> I'm seeing that CONFIG_SYS_MMC_ENV_PART is also mentioned in the
>> documentation for this MVEBU cmd:
>>
>> doc/mvebu/cmd/bubt.txt
>>
>> So I hesitate a bit to remove it completely from this command (even
>> though I personally have never used it). Could you perhaps send me a
>> link a patch / commit, where 'mmc->part_num' has been migrated?
> 
> It was changed in:
> commit 873cc1d7775ed5de07e6722c7ff423080c2e8f71
> Author:     Stephen Warren <swarren@nvidia.com>
> AuthorDate: Mon Dec 7 11:38:49 2015 -0700
> Commit:     Tom Rini <trini@konsulko.com>
> CommitDate: Wed Jan 13 21:05:19 2016 -0500
> 
>      mmc: store hwpart in the block device
>      
>      This will allow us to have multiple block device structs each referring
>      to the same eMMC device, yet different HW partitions.
>      
>      For now, there is still a single block device per eMMC device. As before,
>      this block device always accesses whichever HW partition was most recently
>      selected. Clients wishing to make use of multiple block devices referring
>      to different HW partitions can simply take a copy of this block device
>      once it points at the correct HW partition, and use each one as they wish.
>      This feature will be used by the next patch.
>      
>      In the future, perhaps get_device() could be enhanced to return a
>      dynamically allocated block device struct, to avoid the client needing to
>      copy it in order to maintain multiple block devices. However, this would
>      require all users to be updated to free those block device structs at some
>      point, which is rather a large change.
>      
>      Most callers of mmc_switch_part() wish to permanently switch the default
>      MMC block device's HW partition. Enhance mmc_switch_part() so that it does
>      this. This removes the need for callers to do this. However,
>      common/env_mmc.c needs to save and restore the current HW partition. Make
>      it do this more explicitly.
>      
>      Replace use of mmc_switch_part() with mmc_select_hwpart() in order to
>      remove duplicate code that skips the call if that HW partition is already
>      selected.
>      
>      Signed-off-by: Stephen Warren <swarren@nvidia.com>
>      Reviewed-by: Tom Rini <trini@konsulko.com>
> 
> Which is before cmd/mvebu/bubt.c was added to mainline.  So while I'm
> sure the command worked in the downstream tree, it wasn't ever tested in
> mainline.  It also means that changing it to mmc->block_dev.hwpart
> instead of mmc->part_num would also work, but the code would remain
> untested.


I see. Thanks for the update here. In this case:

Acked-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] mvebu: bubt: Drop dead code
  2020-07-24 21:13 [PATCH] mvebu: bubt: Drop dead code Tom Rini
  2020-07-25 11:38 ` Stefan Roese
@ 2020-08-06 12:08 ` Stefan Roese
  2020-08-06 14:27   ` Stefan Roese
  1 sibling, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2020-08-06 12:08 UTC (permalink / raw)
  To: u-boot

On 24.07.20 23:13, Tom Rini wrote:
> The code around CONFIG_SYS_MMC_ENV_PART has been untested since merge.
> This can be seen by it referencing 'mmc->part_num' which was migrated
> elsewhere prior to this code being merged.
> 
> Cc: Joel Johnson <mrjoel@lixil.net>
> Cc: Stefan Roese <sr@denx.de>
> Signed-off-by: Tom Rini <trini@konsulko.com>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   cmd/mvebu/bubt.c | 15 ---------------
>   1 file changed, 15 deletions(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index a27b0df8ae78..85ae588676fe 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -176,16 +176,6 @@ static int mmc_burn_image(size_t image_size)
>   		return err;
>   	}
>   
> -#ifdef CONFIG_SYS_MMC_ENV_PART
> -	if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART) {
> -		err = mmc_switch_part(mmc_dev_num, CONFIG_SYS_MMC_ENV_PART);
> -		if (err) {
> -			printf("MMC partition switch failed\n");
> -			return err;
> -		}
> -	}
> -#endif
> -
>   	/* SD reserves LBA-0 for MBR and boots from LBA-1,
>   	 * MMC/eMMC boots from LBA-0
>   	 */
> @@ -217,11 +207,6 @@ static int mmc_burn_image(size_t image_size)
>   	}
>   	printf("Done!\n");
>   
> -#ifdef CONFIG_SYS_MMC_ENV_PART
> -	if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART)
> -		mmc_switch_part(mmc_dev_num, mmc->part_num);
> -#endif
> -
>   	return 0;
>   }
>   
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] mvebu: bubt: Drop dead code
  2020-08-06 12:08 ` Stefan Roese
@ 2020-08-06 14:27   ` Stefan Roese
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Roese @ 2020-08-06 14:27 UTC (permalink / raw)
  To: u-boot

On 06.08.20 14:08, Stefan Roese wrote:
> On 24.07.20 23:13, Tom Rini wrote:
>> The code around CONFIG_SYS_MMC_ENV_PART has been untested since merge.
>> This can be seen by it referencing 'mmc->part_num' which was migrated
>> elsewhere prior to this code being merged.
>>
>> Cc: Joel Johnson <mrjoel@lixil.net>
>> Cc: Stefan Roese <sr@denx.de>
>> Signed-off-by: Tom Rini <trini@konsulko.com>
> 
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot-marvell/master

Thanks,
Stefan

> Thanks,
> Stefan
> 
>> ---
>> ? cmd/mvebu/bubt.c | 15 ---------------
>> ? 1 file changed, 15 deletions(-)
>>
>> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
>> index a27b0df8ae78..85ae588676fe 100644
>> --- a/cmd/mvebu/bubt.c
>> +++ b/cmd/mvebu/bubt.c
>> @@ -176,16 +176,6 @@ static int mmc_burn_image(size_t image_size)
>> ????????? return err;
>> ????? }
>> -#ifdef CONFIG_SYS_MMC_ENV_PART
>> -??? if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART) {
>> -??????? err = mmc_switch_part(mmc_dev_num, CONFIG_SYS_MMC_ENV_PART);
>> -??????? if (err) {
>> -??????????? printf("MMC partition switch failed\n");
>> -??????????? return err;
>> -??????? }
>> -??? }
>> -#endif
>> -
>> ????? /* SD reserves LBA-0 for MBR and boots from LBA-1,
>> ?????? * MMC/eMMC boots from LBA-0
>> ?????? */
>> @@ -217,11 +207,6 @@ static int mmc_burn_image(size_t image_size)
>> ????? }
>> ????? printf("Done!\n");
>> -#ifdef CONFIG_SYS_MMC_ENV_PART
>> -??? if (mmc->part_num != CONFIG_SYS_MMC_ENV_PART)
>> -??????? mmc_switch_part(mmc_dev_num, mmc->part_num);
>> -#endif
>> -
>> ????? return 0;
>> ? }
>>
> 
> 
> Viele Gr??e,
> Stefan
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-06 14:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 21:13 [PATCH] mvebu: bubt: Drop dead code Tom Rini
2020-07-25 11:38 ` Stefan Roese
2020-07-25 12:59   ` Tom Rini
2020-07-30  6:50     ` Stefan Roese
2020-08-06 12:08 ` Stefan Roese
2020-08-06 14:27   ` Stefan Roese

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.