Re: [PATCH 0/2] arm: Add collie and sx functional tests

["Jan Lübbe" <jlu@pengutronix.de>]

On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:
> On 10/25/24 02:57, Jan Lübbe wrote:
> > On Fri, 2024-10-25 at 08:55 +0200, Cédric Le Goater wrote:
> > > On 10/24/24 19:59, Philippe Mathieu-Daudé wrote:
> > > > Cc'ing Jan.
> > > > 
> > > > On 22/10/24 12:04, Guenter Roeck wrote:
> > > > 
> > > > > I have no idea how this is supposed to work, and I don't think it works
> > > > > as implemented. I'll revert commit e32ac563b83 in my local copy of qemu.
> > > > > Jan, can you have a look at this bug report please? Otherwise I'll
> > > > > have a look during soft freeze.
> > 
> > Guenter, just to make sure: In your case [1], you had "boot-emmc" (see
> > aspeed_machine_ast2600_class_emmc_init) enabled, right? Otherwise the
> 
> I tried both enabled and disabled.
> 
> > boot partition size would be 0, meaning that the eMMC has no boot
> > partitions.
> 
> That is what I would have expected, but it does not reflect reality.
> 
> > In that configuration, your backing file needs to have space for the
> > boot partitions at the start (2*1MiB) and the rootfs starts at the 2
> > MiB offset.
> > 
> 
> boot-emmc doesn't make a difference, because
> 
>          if (emmc) {
>              qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>              qdev_prop_set_uint8(card, "boot-config",
>                                  boot_emmc ? 0x1 << 3 : 0x0);
>          }
> 
> in hw/arm/aspeed.c sets the boot partition size independently of the
> boot-emmc flag. 

Ah, yes. :/

So you can have SD, eMMC with boot from boot partition *disabled* or
eMMC with boot from boot partition *enabled*.

> I suspect that the expectation is that I always provide
> an emmc image with a 2 MB gap at the beginning, but in my opinion that is
> really hyper-clumsy, and I simply won't do it, sorry.

I was surprised that the boot partitions' contents are stored in the
same backing file as the main area (instead of being separate files).
But I've not researched why it was designed this way.

> I can work around
> the problem by changing the above code to only set boot-partition-size if
> boot_emmc is set, or I can revert commit e32ac563b83. For now I chose the
> latter.

With e32ac563b83 reverted, your backing file layout will change *at
runtime* depending on whether booting from the boot partition is
enabled via EXT CSD registers. You should be able to try that using
'mmc bootpart enable <boot_part> 0 /dev/mmcblk0' (with boot_part=0 for
disabled and boot_part=1/2 for enabled).


I'm not sure what the best way forward is... perhaps make the boot-
partition-size zero if boot_emmc is false? e.g.

@@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
         }
         card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
         if (emmc) {
-            qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
+            /*
+             * Avoid the requirement for a padded disk image if booting from
+             * eMMC boot partitions is not needed.
+             */
+            qdev_prop_set_uint64(card, "boot-partition-size",
+                                boot_emmc ? 1 * MiB : 0);
             qdev_prop_set_uint8(card, "boot-config",
                                 boot_emmc ? 0x1 << 3 : 0x0);
         }


Then you'd have the choice between:
- an eMMC, but without boot partitions (and simple backing file layout)
- an eMMC with boot partitions and need a backing file with 
  the boot partitions at the beginning

That might be the lesser evil. :)

Jan

-- 
Pengutronix e.K.                        |                             |
Steuerwalder Str. 21                    | https://www.pengutronix.de/ |
31137 Hildesheim, Germany               | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686        | Fax:   +49-5121-206917-5555 |