Re: [PATCH v2] aspeed: Don't set always boot properties of the emmc device

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

On Sun, 2024-11-03 at 22:08 +0100, Cédric Le Goater wrote:
> Commit e554e45b4478 ("aspeed: Tune eMMC device properties to reflect
> HW strapping") added support to boot from an eMMC device by setting
> the boot properties of the eMMC device. This change made the
> assumption that the device always has boot areas.
> 
> However, if the machine boots from the flash device (or -kernel) and
> uses an eMMC device without boot areas, support would be broken. This
> impacts the ast2600-evb machine which can choose to boot from flash or
> eMMC using the "boot-emmc" machine option.
> 
> To provide some flexibility for Aspeed machine users to use different
> flavors of eMMC devices (with or without boot areas), do not set the
> eMMC device boot properties when the machine is not configured to boot
> from eMMC. However, this approach makes another assumption about eMMC
> devices, namely that eMMC devices from which the machine does not boot
> do not have boot areas.
> 
> A preferable alternative would be to add support for user creatable
> eMMC devices and define the device boot properties on the QEMU command
> line :
> 
>   -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
>   -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
> 
> This is a global change requiring more thinking. Nevertheless, in the
> case of the ast2600-evb machine booting from an eMMC device and when
> default devices are created, the proposed change still makes sense
> since the device is required to have boot areas.
> 
> Cc: Jan Luebbe <jlu@pengutronix.de>
> Fixes: e554e45b4478 ("aspeed: Tune eMMC device properties to reflect
> HW strapping")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 
>  Changes in v2:
> 
>  - Simplified "boot-config setting
> 
>  hw/arm/aspeed.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index e447923536b4..6ca145362cbd 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -338,10 +338,20 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>              return;
>          }
>          card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
> -        if (emmc) {
> +
> +        /*
> +         * Force the boot properties of the eMMC device only when the
> +         * machine is strapped to boot from eMMC. Without these
> +         * settings, the machine would not boot.
> +         *
> +         * This also allows the machine to use an eMMC device without
> +         * boot areas when booting from the flash device (or -kernel)
> +         * Ideally, the device and its properties should be defined on
> +         * the command line.
> +         */
> +        if (emmc && boot_emmc) {
>              qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
> -            qdev_prop_set_uint8(card, "boot-config",
> -                                boot_emmc ? 0x1 << 3 : 0x0);
> +            qdev_prop_set_uint8(card, "boot-config", 0x1 << 3);
>          }
>          qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
>                                  &error_fatal);

Reviewed-by: Jan Luebbe <jlu@pengutronix.de>

Thanks,
Jan