On Thursday 23 February 2023 07:17:26 Stefan Roese wrote:
> Hi Pali,
>
> On 2/22/23 19:06, Pali Rohár wrote:
> > On Wednesday 22 February 2023 10:55:54 Stefan Roese wrote:
> > > Hi Pali,
> > >
> > > On 2/21/23 21:18, Pali Rohár wrote:
> > > > Support for burning into the correct eMMC HW boot partition was broken
> > > > and
> > > > removed in commit 96be2f072768 ("mvebu: bubt: Drop dead code").
> > > > Reimplement
> > > > this functionality and bring it back again.
> > > >
> > > > Fixes: 96be2f072768 ("mvebu: bubt: Drop dead code")
> > > > Signed-off-by: Pali Rohár <p...@kernel.org>
> > > > ---
> > > > cmd/mvebu/bubt.c | 53
> > > > ++++++++++++++++++++++++++++++++++++++----
> > > > doc/mvebu/cmd/bubt.txt | 21 ++++++++++++-----
> > > > 2 files changed, 63 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> > > > index 2bcdf145f64a..4bad9a69527c 100644
> > > > --- a/cmd/mvebu/bubt.c
> > > > +++ b/cmd/mvebu/bubt.c
> > > > @@ -189,6 +189,11 @@ static int mmc_burn_image(size_t image_size)
> > > > #ifdef CONFIG_BLK
> > > > struct blk_desc *blk_desc;
> > > > #endif
> > > > +#ifdef CONFIG_SUPPORT_EMMC_BOOT
> > > > + u8 part;
> > > > + u8 orig_part;
> > > > +#endif
> > > > +
> > > > mmc = find_mmc_device(mmc_dev_num);
> > > > if (!mmc) {
> > > > printf("No SD/MMC/eMMC card found\n");
> > > > @@ -202,6 +207,38 @@ static int mmc_burn_image(size_t image_size)
> > > > return err;
> > > > }
> > > > +#ifdef CONFIG_BLK
> > > > + blk_desc = mmc_get_blk_desc(mmc);
> > > > + if (!blk_desc) {
> > > > + printf("Error - failed to obtain block descriptor\n");
> > > > + return -ENODEV;
> > > > + }
> > > > +#endif
> > >
> > > This #ifdef usage really gets out of hand IMHO. Yes I know, you did not
> > > introduce it here. Still, perhaps some of this can be moved to using
> > > IS_ENABLED or CONFIG_IS_ENABLED instead?
> > >
> > > And I just checked that "bubt" is not compiled for CONFIG_BLK not
> > > defined. So please depend this file on CONFIG_BLK in Kconfig and remove
> > > the non CONFIG_BLK code here.
> >
> > bubt is used for at least 4 different mvebu platforms (AXP, A38x, A3720,
> > A7K) and every one has slightly different code and configuration.
> >
> > I really do not know if all those configurations support CONFIG_BLK, so
> > I decided to let it as it was before to eliminate possible issues.
>
> As mentioned above, I've checked this myself before sending the
> suggestion yesterday. No platform compiling this cmd has CONFIG_BLK
> disabled.
OK! In this case I would rather send an additional cleanup patch which
removes usage of non-CONFIG_BLK code from the whole command. As it does
not make sense to not-have it in MMC and have it in SATA.
> My understanding is that CONFIG_BLK will be required at some time and
> all non CONFIG_BLK code will get dropped.
>
> Still, if you feel you don't want to make this change, as it's not
> really related to your current patchset, then this is no showstopper.
>
> Thanks,
> Stefan
>
> > > Thanks,
> > > Stefan
> > >
> > > > +
> > > > +#ifdef CONFIG_SUPPORT_EMMC_BOOT
> > > > +#ifdef CONFIG_BLK
> > > > + orig_part = blk_desc->hwpart;
> > > > +#else
> > > > + orig_part = mmc->block_dev.hwpart;
> > > > +#endif
> > > > +
> > > > + part = (mmc->part_config >> 3) & PART_ACCESS_MASK;
> > > > +
> > > > + if (part == 7)
> > > > + part = 0;
> > > > +
> > > > +#ifdef CONFIG_BLK
> > > > + err = blk_dselect_hwpart(blk_desc, part);
> > > > +#else
> > > > + err = mmc_switch_part(mmc, part);
> > > > +#endif
> > > > +
> > > > + if (err) {
> > > > + printf("Error - 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
> > > > */
> > > > @@ -211,11 +248,6 @@ static int mmc_burn_image(size_t image_size)
> > > > if (image_size % mmc->write_bl_len)
> > > > blk_count += 1;
> > > > - blk_desc = mmc_get_blk_desc(mmc);
> > > > - if (!blk_desc) {
> > > > - printf("Error - failed to obtain block descriptor\n");
> > > > - return -ENODEV;
> > > > - }
> > > > blk_written = blk_dwrite(blk_desc, start_lba, blk_count,
> > > > (void *)get_load_addr());
> > > > #else
> > > > @@ -227,6 +259,17 @@ static int mmc_burn_image(size_t image_size)
> > > > start_lba, blk_count,
> > > > (void
> > > > *)get_load_addr());
> > > > #endif /* CONFIG_BLK */
> > > > +
> > > > +#ifdef CONFIG_SUPPORT_EMMC_BOOT
> > > > +#ifdef CONFIG_BLK
> > > > + err = blk_dselect_hwpart(blk_desc, orig_part);
> > > > +#else
> > > > + err = mmc_switch_part(mmc, orig_part);
> > > > +#endif
> > > > + if (err)
> > > > + printf("Error - MMC failed to switch back to original
> > > > partition\n");
> > > > +#endif
> > > > +
> > > > if (blk_written != blk_count) {
> > > > printf("Error - written %#lx blocks\n", blk_written);
> > > > return -ENOSPC;
> > > > diff --git a/doc/mvebu/cmd/bubt.txt b/doc/mvebu/cmd/bubt.txt
> > > > index 6051243f1165..1fe1f07dd187 100644
> > > > --- a/doc/mvebu/cmd/bubt.txt
> > > > +++ b/doc/mvebu/cmd/bubt.txt
> > > > @@ -14,8 +14,7 @@ Examples:
> > > > Notes:
> > > > - For the TFTP interface set serverip and ipaddr.
> > > > -- To burn image to SD/eMMC device, the target is defined
> > > > - by parameters CONFIG_SYS_MMC_ENV_DEV and CONFIG_SYS_MMC_ENV_PART.
> > > > +- To burn image to SD/eMMC device, the target is defined by HW
> > > > partition.
> > > > Bubt command details (burn image step by-step)
> > > > ----------------------------------------------
> > > > @@ -40,10 +39,20 @@ Notes:
> > > > Number 0 is used for user data partition and should not be
> > > > utilized for storing
> > > > boot images and U-Boot environment in RAW mode since it will break
> > > > file system
> > > > structures usually located here.
> > > > - The default boot partition is BOOT0. It is selected by the following
> > > > parameter:
> > > > - CONFIG_SYS_MMC_ENV_PART=1
> > > > - Valid values for this parameter are 1 for BOOT0 and 2 for BOOT1.
> > > > - Please never use partition number 0 here!
> > > > +
> > > > + Currently configured boot partition can be printed by command:
> > > > + # mmc partconf 0
> > > > + (search for BOOT_PARTITION_ACCESS output, number 7 is user data)
> > > > +
> > > > + Change it to BOOT0:
> > > > + # mmc partconf 0 0 1 1
> > > > +
> > > > + Change it to BOOT1:
> > > > + # mmc partconf 0 0 2 2
> > > > +
> > > > + Change it to user data:
> > > > + # mmc partconf 0 0 7 0
> > > > +
> > > > - The partition number is ignored if the target device is SD card.
> > > > - The boot image offset starts at block 0 for eMMC and block 1 for
> > > > SD devices.
> > > > The block 0 on SD devices is left for MBR storage.
> > >
> > > Viele Grüße,
> > > Stefan Roese
> > >
> > > --
> > > DENX Software Engineering GmbH, Managing Director: Erika Unter
> > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de
>
> Viele Grüße,
> Stefan Roese
>
> --
> DENX Software Engineering GmbH, Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de
