Hi,
On Mon, 2022-07-11 at 12:18 +0200, Quentin Schulz wrote:
> Hi Harald,
>
> On 7/6/22 12:58, Harald Seiler wrote:
> > When attempting to load images from multiple MMC devices in sequence,
> > spl_mmc_load() chooses the wrong device from the second attempt onwards.
> >
> > The reason is that MMC initialization is only done on its first call and
> > spl_mmc_load() will then continue using this same device for all future
> > calls.
> >
> > Fix this by checking the devnum of the "cached" device struct against
> > the one which is requested. If they match, use the cached one but if
> > they do not match, initialize the new device.
> >
> > This fixes specifying multiple MMC devices in the SPL's boot order to
> > fall back when U-Boot Proper is corrupted or missing on the first
> > attempted MMC device.
> >
> > Fixes: e1eb6ada4e38 ("spl: Make image loader infrastructure more universal")
> > Signed-off-by: Harald Seiler <h...@denx.de>
> > ---
> > common/spl/spl_mmc.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > index e1a7d25bd0..22c8a8097c 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -371,9 +371,11 @@ int spl_mmc_load(struct spl_image_info *spl_image,
> > u32 boot_mode;
> > int err = 0;
> > __maybe_unused int part = 0;
> > + int mmc_dev;
> >
> > - /* Perform peripheral init only once */
> > - if (!mmc) {
> > + /* Perform peripheral init only once for an mmc device */
> > + mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
> > + if (!mmc || mmc->block_dev.devnum != mmc_dev) {
>
> block_dev field only exists when CONFIG_BLK is not defined, c.f.
> https://elixir.bootlin.com/u-boot/latest/source/include/mmc.h#L705
>
> Considering the commit introducing this, c.f.
> https://source.denx.de/u-boot/u-boot/-/commit/33fb211dd2706e666db4008801dc0d5903fd82f6,
>
> I can suggest the following diff (which makes it compile with
> rk3399-puma_defconfig):
>
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index e0e1a80536..c40b436a11 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -409,10 +409,17 @@ int spl_mmc_load(struct spl_image_info *spl_image,
> int err = 0;
> __maybe_unused int part = 0;
> int mmc_dev;
> + struct blk_desc *block_dev;
>
> /* Perform peripheral init only once for an mmc device */
> mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
> - if (!mmc || mmc->block_dev.devnum != mmc_dev) {
> +#if !CONFIG_IS_ENABLED(BLK)
> + block_dev = &mmc->block_dev;
> +#else
> + block_dev = dev_get_uclass_plat(mmc->dev);
> +#endif
> +
> + if (!mmc || block_dev->devnum != mmc_dev) {
> err = spl_mmc_find_device(&mmc, bootdev->boot_device);
> if (err)
> return err;
>
> Note: Only build tested.
Thanks for reporting. It's not quite that simple because `mmc` is NULL
on first call to spl_mmc_load(). I'll send a v2 to fix this.
--
Harald
