[PATCH v2 1/2] mmc: Check for device with a seq number equal to num before checking against index

[Ricardo Salveti]

Hi Aswath,

Thanks for the quick response.

On Wed, Jul 7, 2021 at 1:55 AM Aswath Govindraju <a-govindraju at ti.com> wrote:
>
> Hi Ricardo,
>
> On 07/07/21 4:39 am, Ricardo Salveti wrote:
> > Hi Aswath,
> >
> > On Thu, Mar 25, 2021 at 4:19 AM Aswath Govindraju <a-govindraju at ti.com> wrote:
> >>
> >> First check if there is an alias for the device tree node defined with the
> >> given num before checking against device index.
> >>
> >> Signed-off-by: Aswath Govindraju <a-govindraju at ti.com>
> >> Reviewed-by: Lokesh Vutla <lokeshvutla at ti.com>
> >> Reviewed-by: Jaehoon Chung <jh80.chung at samsung.com>
> >> ---
> >>  drivers/mmc/mmc.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> >> index b4c8e7f293bd..1e83007286b2 100644
> >> --- a/drivers/mmc/mmc.c
> >> +++ b/drivers/mmc/mmc.c
> >> @@ -3052,9 +3052,11 @@ int mmc_init_device(int num)
> >>         struct mmc *m;
> >>         int ret;
> >>
> >> -       ret = uclass_get_device(UCLASS_MMC, num, &dev);
> >> -       if (ret)
> >> -               return ret;
> >> +       if (uclass_get_device_by_seq(UCLASS_MMC, num, &dev)) {
> >> +               ret = uclass_get_device(UCLASS_MMC, num, &dev);
> >> +               if (ret)
> >> +                       return ret;
> >> +       }
> >
> > uclass_get_device_by_seq returns 0 if OK and -ve on error, so I
> > believe this check is incorrect and we never end up calling
> > uclass_get_device on the working path.
>
> Yes, as you mentioned uclass_get_device_by_seq() returns 0 if OK and -ve
> on error is correct. However, the intention of this check here, is to
> only call uclass_get_device() when uclass_get_device_by_seq() returns an
> error.
>
> uclass_get_device_by_seq() returns the device only if it is able find a
> active device with matching sequence number "num" or will try to find a
> device that is requesting this number. As this function also returns the
> device we need not call uclass_get_device() again. Only on failure for
> find a device with matching sequence number do we call uclass_get_device().
>
> So, I believe the above mentioned check is correct.
>
> > While bisecting to try to debug why my zynqmp-based device is unable
> > to boot u-boot, I stopped at this patch (merged as part of
> > v2021.07-rc1).
> >
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > index 1e83007286b..56556353f8e 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -3052,11 +3052,13 @@ int mmc_init_device(int num)
> >         struct mmc *m;
> >         int ret;
> >
> > -       if (uclass_get_device_by_seq(UCLASS_MMC, num, &dev)) {
> > -               ret = uclass_get_device(UCLASS_MMC, num, &dev);
> > -               if (ret)
> > -                       return ret;
> > -       }
> > +       ret = uclass_get_device_by_seq(UCLASS_MMC, num, &dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = uclass_get_device(UCLASS_MMC, num, &dev);
> > +       if (ret)
> > +               return ret;
> >
> >         m = mmc_get_mmc_dev(dev);
> >         if (!m)
> >
>
> In the above method, the device is being fetched twice and I believe
> that this method is incorrect.

You're right, I had to read more about how seq is related with aliases
to understand the difference here.

> > This is enough for it to work correctly again, but I just wanted to
> > reply back here first before submitting as I'm surprised nobody
> > reported this issue before.
>
> If making this change got it work correctly again. I believe there might
> have been a error in assigning aliases for mmc devices in the device
> tree. As only after this patch are aliases being read for mmc class of
> devices from the device tree.

Was finally able to understand what happened, and it is indeed related
to how the aliases were used by this board.

At spl_mmc_find_device the function spl_mmc_get_device_index was
called with BOOT_DEVICE_MMC2 (sdcard on my board), following with
mmc_init_device (mmc_dev 1). This caused uclass_get_device_by_seq to
be called with num 1 (alias 1), which ended up loading the eMMC device
instead of sdcard, causing the boot failure.

Looking at my dts, this now makes sense as mmc0 was an alias to sdcard
while mmc1 to emmc. Inverting the alias node was enough to get my
board to boot again.

diff --git a/uz3eg-iocc/system-som.dtsi b/system-som.dtsi
index 192e343..ec2b8a2 100644
--- a/system-som.dtsi
+++ b/system-som.dtsi
@@ -21,8 +21,8 @@

        aliases {
                gpio0 = &gpio;
-               mmc0 = &sdhci1;
-               mmc1 = &sdhci0;
+               mmc1 = &sdhci1;
+               mmc0 = &sdhci0;
                rtc0 = &rtc;
                serial0 = &uart0;
                serial1 = &uart1;

Thanks,
-- 
Ricardo Salveti