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

[Aswath Govindraju]

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.

> 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.

Thanks,
Aswath

> Can you verify on your target hardware if this is indeed working as intended?
> 
> Thanks,
>