Hi Aswath,

On Thu, Mar 25, 2021 at 4:19 AM Aswath Govindraju a-govindraju@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@ti.com Reviewed-by: Lokesh Vutla lokeshvutla@ti.com Reviewed-by: Jaehoon Chung jh80.chung@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.

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)

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.

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

Thanks,

Hi Aswath,

Thanks for the quick response.

On Wed, Jul 7, 2021 at 1:55 AM Aswath Govindraju a-govindraju@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@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@ti.com Reviewed-by: Lokesh Vutla lokeshvutla@ti.com Reviewed-by: Jaehoon Chung jh80.chung@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,

On 3/25/21 4:18 PM, Aswath Govindraju wrote:

Use dev_seq() to read aliases node's index and pass it as device number for creating bulk device.

Suggested-by: Grygorii Strashko grygorii.strashko@ti.com Signed-off-by: Aswath Govindraju a-govindraju@ti.com

Reviewed-by: Jaehoon Chung jh80.chung@samsung.com

Best Regards, Jaehoon Chung

---

drivers/mmc/mmc-uclass.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 53eabc9e612d..d36aae367e7c 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -383,18 +383,16 @@ int mmc_bind(struct udevice *dev, struct mmc *mmc, const struct mmc_config *cfg) { struct blk_desc *bdesc; struct udevice *bdev;

• int ret, devnum = -1;

• int ret;

  if (!mmc_get_ops(dev)) return -ENOSYS;

-#ifndef CONFIG_SPL_BUILD

• /* Use the fixed index with aliase node's index */
• ret = dev_read_alias_seq(dev, &devnum);
• debug("%s: alias ret=%d, devnum=%d\n", __func__, ret, devnum);

-#endif

• /* Use the fixed index with aliases node's index */

• debug("%s: alias devnum=%d\n", __func__, dev_seq(dev));

  ret = blk_create_devicef(dev, "mmc_blk", "blk", IF_TYPE_MMC,

• 	devnum, 512, 0, &bdev);
  

• 	dev_seq(dev), 512, 0, &bdev);
  

  if (ret) { debug("Cannot create block device\n"); return ret;

On 20.05.21 18:55, Andre Przywara wrote:

On Thu, 20 May 2021 18:32:38 +0200 Heinrich Schuchardt xypron.glpk@gmx.de wrote:

Hi Heinrich,

...

On 25.03.21 08:18, Aswath Govindraju wrote:

...

Use dev_seq() to read aliases node's index and pass it as device number for creating bulk device.

Suggested-by: Grygorii Strashko grygorii.strashko@ti.com Signed-off-by: Aswath Govindraju a-govindraju@ti.com Reviewed-by: Jaehoon Chung jh80.chung@samsung.com

Since this patch merged as commit 2243d19e5618122 SD cards on orangepi_pc_defconfig are not detected anymore.

=> mmc rescan MMC Device 0 not found no mmc device at slot 0 =>

Thanks for the report! This is a known issue, please join the discussion here: https://lists.denx.de/pipermail/u-boot/2021-April/447472.html

I am in the middle of some patch to make fastboot detect the eMMC, which would render the need for this "mmc1 = &mmc2;" alias in our sunxi-u-boot.dtsi obsolete. Removing this line should restore the old probe order. However this is probably a bigger change (and affects other platforms), so this is not 2021.07 material anymore.

I will push for this original patch of mine again (adding an mmc0 alias), once I have a proof of concept for the proper fastboot fix.

As you figured, this is a serious user-visible regression, so we need to fix this.

Cheers, Andre

I applied the patch in https://lists.denx.de/pipermail/u-boot/2021-April/447472.html and Debian boots like a charm.

Either using my pre-existing boot option

/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(2)/SD(0)/HD(1,MBR,0xb6b4facb,0x800,0x200000)/EFI\debian\grubarm.efi

Or using Debian's flash-kernel package.

As long as you don't have two mmc devices installed (SD-card + eMMC) I would not expect any hickup due to the additional alias when using distroboot.

Best regards

Heinrich

...

Best regards

Heinrich

...

---

drivers/mmc/mmc-uclass.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c index 53eabc9e612d..d36aae367e7c 100644 --- a/drivers/mmc/mmc-uclass.c +++ b/drivers/mmc/mmc-uclass.c @@ -383,18 +383,16 @@ int mmc_bind(struct udevice *dev, struct mmc *mmc, const struct mmc_config *cfg) { struct blk_desc *bdesc; struct udevice *bdev;

• int ret, devnum = -1;

• int ret;

  if (!mmc_get_ops(dev)) return -ENOSYS;

-#ifndef CONFIG_SPL_BUILD

• /* Use the fixed index with aliase node's index */
• ret = dev_read_alias_seq(dev, &devnum);
• debug("%s: alias ret=%d, devnum=%d\n", __func__, ret, devnum);

-#endif

• /* Use the fixed index with aliases node's index */

• debug("%s: alias devnum=%d\n", __func__, dev_seq(dev));

  ret = blk_create_devicef(dev, "mmc_blk", "blk", IF_TYPE_MMC,

• 	devnum, 512, 0, &bdev);
  

• 	dev_seq(dev), 512, 0, &bdev);
  

  if (ret) { debug("Cannot create block device\n"); return ret;