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

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

[Cédric Le Goater]

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>
---
 hw/arm/aspeed.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index e161e6b1c582..ac6d8dde71b3 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -338,7 +338,18 @@ 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);
-- 
2.47.0

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

[Guenter Roeck]

On 11/2/24 07:39, 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>
> ---
>   hw/arm/aspeed.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index e161e6b1c582..ac6d8dde71b3 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -338,7 +338,18 @@ 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);

That second boot_emmc check is no longer necessary, so this could be
simplified to
		qdev_prop_set_uint8(card, "boot-config", 0x1 << 3);

Thanks,
Guenter

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

[Jan Lübbe]

On Sat, 2024-11-02 at 15:39 +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>
> ---
>  hw/arm/aspeed.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index e161e6b1c582..ac6d8dde71b3 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -338,7 +338,18 @@ 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);

With the change proposed by Guenter, this looks good to me.

Thanks,
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 |

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

[Cédric Le Goater]

On 11/4/24 09:50, Jan Lübbe wrote:
> On Sat, 2024-11-02 at 15:39 +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>
>> ---
>>   hw/arm/aspeed.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index e161e6b1c582..ac6d8dde71b3 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -338,7 +338,18 @@ 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);
> 
> With the change proposed by Guenter, this looks good to me.

yes. Could you please review v2 them ?

Thanks,

C.