| Message ID | 20200724211317.2678-1-trini@konsulko.com |
|---|---|
| State | Accepted |
| Commit | 96be2f072768a91eb0df3767b9c8c880dd1e4b5b |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | mvebu: bubt: Drop dead code | expand |
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
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!
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
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
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
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; }
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(-)