[PATCH 0/8] aspeed: Add boot from eMMC support (AST2600)

[PATCH 0/8] aspeed: Add boot from eMMC support (AST2600)

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

Hello,

This series enables boot from eMMC on the rainier-bmc machine, which
is the default behavior and also on the AST2600 EVB using a machine
option to change the default.

It depends solely on the availability of an eMMC device model which is
currently being discussed upstream. Numerous patches have been merged
already and remaining ones [1] should be in 9.1 development cycle.
Since the changes of this series are small and localized, it would be
an interesting extension of the AST2600 SoC model for QEMU 9.1.

First 6 patches adjust the machine setup and HW strapping to boot from
eMMC, the last 2 are for the AST2600 EVB and are optional.

Thanks,

C.

[1] https://github.com/legoater/qemu/commits/aspeed-9.1

Cédric Le Goater (8):
  aspeed: Change type of eMMC device
  aspeed: Load eMMC first boot area as a boot rom
  aspeed/scu: Add boot-from-eMMC HW strapping bit for AST2600 SoC
  aspeed: Introduce a AspeedSoCClass 'boot_from_emmc' handler
  aspeed: Set eMMC 'boot-config' property to reflect HW strapping
  aspeed: Add boot-from-eMMC HW strapping bit to rainier-bmc machine
  aspeed: Introduce a 'hw_strap1' machine attribute
  aspeed: Introduce a 'boot-emmc' machine option

 docs/system/arm/aspeed.rst   |  2 ++
 include/hw/arm/aspeed_soc.h  |  1 +
 include/hw/misc/aspeed_scu.h |  4 +++
 hw/arm/aspeed.c              | 60 +++++++++++++++++++++++++++++++-----
 hw/arm/aspeed_ast2600.c      |  8 +++++
 hw/arm/aspeed_soc_common.c   |  7 +++++
 6 files changed, 74 insertions(+), 8 deletions(-)

-- 
2.45.2

[PATCH 1/8] aspeed: Change type of eMMC device

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

The QEMU device representing the eMMC device of machine is currently
created with type SD_CARD. Change the type to EMMC now that it is
available.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 53a4f665d0d0..105b990233fa 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -327,14 +327,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
     }
 }
 
-static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
+static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
 {
         DeviceState *card;
 
         if (!dinfo) {
             return;
         }
-        card = qdev_new(TYPE_SD_CARD);
+        card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
         qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
                                 &error_fatal);
         qdev_realize_and_unref(card,
@@ -436,12 +436,13 @@ static void aspeed_machine_init(MachineState *machine)
 
     for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
         sdhci_attach_drive(&bmc->soc->sdhci.slots[i],
-                           drive_get(IF_SD, 0, i));
+                           drive_get(IF_SD, 0, i), false);
     }
 
     if (bmc->soc->emmc.num_slots) {
         sdhci_attach_drive(&bmc->soc->emmc.slots[0],
-                           drive_get(IF_SD, 0, bmc->soc->sdhci.num_slots));
+                           drive_get(IF_SD, 0, bmc->soc->sdhci.num_slots),
+                           true);
     }
 
     if (!bmc->mmio_exec) {
-- 
2.45.2

Re: [PATCH 1/8] aspeed: Change type of eMMC device

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> The QEMU device representing the eMMC device of machine is currently
> created with type SD_CARD. Change the type to EMMC now that it is
> available.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Re: [PATCH 1/8] aspeed: Change type of eMMC device

[Philippe Mathieu-Daudé]

On 4/7/24 07:36, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> The QEMU device representing the eMMC device of machine is currently
> created with type SD_CARD. Change the type to EMMC now that it is
> available.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/8] aspeed: Load eMMC first boot area as a boot rom

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

The first boot area partition (64K) of the eMMC device should contain
an initial boot loader (u-boot SPL). Load it as a ROM only if an eMMC
device is available to boot from but no flash device is.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 105b990233fa..756deb91efd1 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -364,6 +364,7 @@ static void aspeed_machine_init(MachineState *machine)
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
     AspeedSoCClass *sc;
     int i;
+    DriveInfo *emmc0 = NULL;
 
     bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
@@ -440,9 +441,8 @@ static void aspeed_machine_init(MachineState *machine)
     }
 
     if (bmc->soc->emmc.num_slots) {
-        sdhci_attach_drive(&bmc->soc->emmc.slots[0],
-                           drive_get(IF_SD, 0, bmc->soc->sdhci.num_slots),
-                           true);
+        emmc0 = drive_get(IF_SD, 0, bmc->soc->sdhci.num_slots);
+        sdhci_attach_drive(&bmc->soc->emmc.slots[0], emmc0, true);
     }
 
     if (!bmc->mmio_exec) {
@@ -452,6 +452,8 @@ static void aspeed_machine_init(MachineState *machine)
         if (fmc0) {
             uint64_t rom_size = memory_region_size(&bmc->soc->spi_boot);
             aspeed_install_boot_rom(bmc, fmc0, rom_size);
+        } else if (emmc0) {
+            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(emmc0), 64 * KiB);
         }
     }
 
-- 
2.45.2

Re: [PATCH 2/8] aspeed: Load eMMC first boot area as a boot rom

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> The first boot area partition (64K) of the eMMC device should contain
> an initial boot loader (u-boot SPL). Load it as a ROM only if an eMMC
> device is available to boot from but no flash device is.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH 3/8] aspeed/scu: Add boot-from-eMMC HW strapping bit for AST2600 SoC

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

Bit SCU500[2] of the AST2600 controls the boot device of the SoC.

Future changes will configure this bit to boot from eMMC disk images
specially built for this purpose.

Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/misc/aspeed_scu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
index 58db28db45aa..c9f98c20ffd9 100644
--- a/include/hw/misc/aspeed_scu.h
+++ b/include/hw/misc/aspeed_scu.h
@@ -349,6 +349,10 @@ uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s);
 #define SCU_AST2600_H_PLL_BYPASS_EN                        (0x1 << 24)
 #define SCU_AST2600_H_PLL_OFF                              (0x1 << 23)
 
+/* STRAP1 SCU500 */
+#define AST2600_HW_STRAP_BOOT_SRC_EMMC            (0x1 << 2)
+#define AST2600_HW_STRAP_BOOT_SRC_SPI             (0x0 << 2)
+
 /*
  * SCU310   Clock Selection Register Set 4 (for Aspeed AST1030 SOC)
  *
-- 
2.45.2

Re: [PATCH 3/8] aspeed/scu: Add boot-from-eMMC HW strapping bit for AST2600 SoC

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> Bit SCU500[2] of the AST2600 controls the boot device of the SoC.
> 
> Future changes will configure this bit to boot from eMMC disk images
> specially built for this purpose.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/misc/aspeed_scu.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
> index 58db28db45aa..c9f98c20ffd9 100644
> --- a/include/hw/misc/aspeed_scu.h
> +++ b/include/hw/misc/aspeed_scu.h
> @@ -349,6 +349,10 @@ uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s);
>  #define SCU_AST2600_H_PLL_BYPASS_EN                        (0x1 << 24)
>  #define SCU_AST2600_H_PLL_OFF                              (0x1 << 23)
>  
> +/* STRAP1 SCU500 */
> +#define AST2600_HW_STRAP_BOOT_SRC_EMMC            (0x1 << 2)
> +#define AST2600_HW_STRAP_BOOT_SRC_SPI             (0x0 << 2)

Maybe these should have a `SCU_` prefix for consistency?

Anyway:

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Re: [PATCH 3/8] aspeed/scu: Add boot-from-eMMC HW strapping bit for AST2600 SoC

[Cédric Le Goater]

On 7/5/24 5:36 AM, Andrew Jeffery wrote:
> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> Bit SCU500[2] of the AST2600 controls the boot device of the SoC.
>>
>> Future changes will configure this bit to boot from eMMC disk images
>> specially built for this purpose.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/misc/aspeed_scu.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
>> index 58db28db45aa..c9f98c20ffd9 100644
>> --- a/include/hw/misc/aspeed_scu.h
>> +++ b/include/hw/misc/aspeed_scu.h
>> @@ -349,6 +349,10 @@ uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s);
>>   #define SCU_AST2600_H_PLL_BYPASS_EN                        (0x1 << 24)
>>   #define SCU_AST2600_H_PLL_OFF                              (0x1 << 23)
>>   
>> +/* STRAP1 SCU500 */
>> +#define AST2600_HW_STRAP_BOOT_SRC_EMMC            (0x1 << 2)
>> +#define AST2600_HW_STRAP_BOOT_SRC_SPI             (0x0 << 2)
> 
> Maybe these should have a `SCU_` prefix for consistency?

Yep. I agree.  
> Anyway:
> 
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 


Thanks,

C.

[PATCH 4/8] aspeed: Introduce a AspeedSoCClass 'boot_from_emmc' handler

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

Report support on the AST2600 SoC if the boot-from-eMMC HW strapping
bit is set at the board level. AST2700 also has support but it is not
yet ready in QEMU and others SoCs do not have support, so return false
always for these.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed_soc.h | 1 +
 hw/arm/aspeed_ast2600.c     | 8 ++++++++
 hw/arm/aspeed_soc_common.c  | 7 +++++++
 3 files changed, 16 insertions(+)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 849ba37f9524..624d489e0d63 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -164,6 +164,7 @@ struct AspeedSoCClass {
     const hwaddr *memmap;
     uint32_t num_cpus;
     qemu_irq (*get_irq)(AspeedSoCState *s, int dev);
+    bool (*boot_from_emmc)(AspeedSoCState *s);
 };
 
 const char *aspeed_soc_cpu_type(AspeedSoCClass *sc);
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 31713de74a5f..8114e7f2759f 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -646,6 +646,13 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static bool aspeed_soc_ast2600_boot_from_emmc(AspeedSoCState *s)
+{
+    uint32_t hw_strap1 = object_property_get_uint(OBJECT(&s->scu),
+                                                  "hw-strap1", &error_abort);
+    return !!(hw_strap1 & AST2600_HW_STRAP_BOOT_SRC_EMMC);
+}
+
 static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
 {
     static const char * const valid_cpu_types[] = {
@@ -673,6 +680,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
     sc->memmap       = aspeed_soc_ast2600_memmap;
     sc->num_cpus     = 2;
     sc->get_irq      = aspeed_soc_ast2600_get_irq;
+    sc->boot_from_emmc = aspeed_soc_ast2600_boot_from_emmc;
 }
 
 static const TypeInfo aspeed_soc_ast2600_types[] = {
diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c
index 1e8f2558fdc2..05551461aea0 100644
--- a/hw/arm/aspeed_soc_common.c
+++ b/hw/arm/aspeed_soc_common.c
@@ -134,6 +134,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static bool aspeed_soc_boot_from_emmc(AspeedSoCState *s)
+{
+    return false;
+}
+
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
@@ -145,9 +150,11 @@ static Property aspeed_soc_properties[] = {
 static void aspeed_soc_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    AspeedSoCClass *sc = ASPEED_SOC_CLASS(oc);
 
     dc->realize = aspeed_soc_realize;
     device_class_set_props(dc, aspeed_soc_properties);
+    sc->boot_from_emmc = aspeed_soc_boot_from_emmc;
 }
 
 static const TypeInfo aspeed_soc_types[] = {
-- 
2.45.2

Re: [PATCH 4/8] aspeed: Introduce a AspeedSoCClass 'boot_from_emmc' handler

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> Report support on the AST2600 SoC if the boot-from-eMMC HW strapping
> bit is set at the board level. AST2700 also has support but it is not
> yet ready in QEMU and others SoCs do not have support, so return false
> always for these.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
property to set the boot config register to boot from the first boot
area partition of the eMMC device.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 756deb91efd1..135f4eb72215 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
     }
 }
 
-static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
+static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
+                               bool boot_emmc)
 {
         DeviceState *card;
 
@@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
             return;
         }
         card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
+        if (emmc) {
+            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
+        }
         qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
                                 &error_fatal);
         qdev_realize_and_unref(card,
@@ -365,6 +369,7 @@ static void aspeed_machine_init(MachineState *machine)
     AspeedSoCClass *sc;
     int i;
     DriveInfo *emmc0 = NULL;
+    bool boot_emmc;
 
     bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
@@ -437,19 +442,21 @@ static void aspeed_machine_init(MachineState *machine)
 
     for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
         sdhci_attach_drive(&bmc->soc->sdhci.slots[i],
-                           drive_get(IF_SD, 0, i), false);
+                           drive_get(IF_SD, 0, i), false, false);
     }
 
+    boot_emmc = sc->boot_from_emmc(bmc->soc);
+
     if (bmc->soc->emmc.num_slots) {
         emmc0 = drive_get(IF_SD, 0, bmc->soc->sdhci.num_slots);
-        sdhci_attach_drive(&bmc->soc->emmc.slots[0], emmc0, true);
+        sdhci_attach_drive(&bmc->soc->emmc.slots[0], emmc0, true, boot_emmc);
     }
 
     if (!bmc->mmio_exec) {
         DeviceState *dev = ssi_get_cs(bmc->soc->fmc.spi, 0);
         BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
 
-        if (fmc0) {
+        if (fmc0 && !boot_emmc) {
             uint64_t rom_size = memory_region_size(&bmc->soc->spi_boot);
             aspeed_install_boot_rom(bmc, fmc0, rom_size);
         } else if (emmc0) {
-- 
2.45.2

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
> property to set the boot config register to boot from the first boot
> area partition of the eMMC device.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/aspeed.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 756deb91efd1..135f4eb72215 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>      }
>  }
>  
> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
> +                               bool boot_emmc)
>  {
>          DeviceState *card;
>  
> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>              return;
>          }
>          card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
> +        if (emmc) {
> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);

0x48 feels a little bit magic. I poked around a bit and there are some
boot-config macros, but not the ones you need and they're all in an
"internal" header anyway. I guess this is fine for now?

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Cédric Le Goater]

On 7/5/24 5:41 AM, Andrew Jeffery wrote:
> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>> From: Cédric Le Goater <clg@kaod.org>
>>
>> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
>> property to set the boot config register to boot from the first boot
>> area partition of the eMMC device.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/arm/aspeed.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 756deb91efd1..135f4eb72215 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>       }
>>   }
>>   
>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>> +                               bool boot_emmc)
>>   {
>>           DeviceState *card;
>>   
>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>>               return;
>>           }
>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>> +        if (emmc) {
>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
> 
> 0x48 feels a little bit magic. I poked around a bit and there are some
> boot-config macros, but not the ones you need and they're all in an
> "internal" header anyway. I guess this is fine for now?

You are right and we should be using these :

hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179     /* R/W */

hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG_ACC_MASK            (0x7)
hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG_ACC_DEFAULT         (0x0)
hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG_ACC_BOOT0           (0x1)
hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG_EN_MASK             (0x7 << 3)
hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG_EN_BOOT0            (0x1 << 3)
hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG_EN_USER             (0x7 << 3)

So I wonder where the 0x48 is coming from. Will change.

> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 


Thanks,

C.

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Philippe Mathieu-Daudé]

On 5/7/24 07:38, Cédric Le Goater wrote:
> On 7/5/24 5:41 AM, Andrew Jeffery wrote:
>> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>>> From: Cédric Le Goater <clg@kaod.org>
>>>
>>> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
>>> property to set the boot config register to boot from the first boot
>>> area partition of the eMMC device.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   hw/arm/aspeed.c | 15 +++++++++++----
>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index 756deb91efd1..135f4eb72215 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState *s, 
>>> const char *flashtype,
>>>       }
>>>   }
>>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, 
>>> bool emmc)
>>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, 
>>> bool emmc,
>>> +                               bool boot_emmc)
>>>   {
>>>           DeviceState *card;
>>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState *sdhci, 
>>> DriveInfo *dinfo, bool emmc)
>>>               return;
>>>           }
>>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>> +        if (emmc) {
>>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 
>>> 0x48 : 0x0);
>>
>> 0x48 feels a little bit magic. I poked around a bit and there are some
>> boot-config macros, but not the ones you need and they're all in an
>> "internal" header anyway. I guess this is fine for now?
> 
> You are right and we should be using these :
> 
> hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179     
> /* R/W */

This field is R/W and expected to be configured by the guest.

Why the guest (u-boot) doesn't detect partitioning support?

See eMMC v4.5 section 7.4.60 PARTITIONING_SUPPORT [160] and earlier

   For e•MMC 4.5 Devices, Bit 2-0 in PARTITIONING_SUPPORT
   shall be set to 1.

We don't set it so far.

I see in u-boot mmc_startup_v4():

     /* store the partition info of emmc */
     mmc->part_support = ext_csd[EXT_CSD_PARTITIONING_SUPPORT];
     if ((ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & PART_SUPPORT) ||
         ext_csd[EXT_CSD_BOOT_MULT])
             mmc->part_config = ext_csd[EXT_CSD_PART_CONF];
     if (part_completed &&
         (ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & ENHNCD_SUPPORT))
             mmc->part_attr = ext_csd[EXT_CSD_PARTITIONS_ATTRIBUTE];

I'm still waiting for the eMMC prerequisite to be reviewed
before looking at the eMMC patches in detail :/

Regards,

Phil.

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Philippe Mathieu-Daudé]

On 5/7/24 15:28, Philippe Mathieu-Daudé wrote:
> On 5/7/24 07:38, Cédric Le Goater wrote:
>> On 7/5/24 5:41 AM, Andrew Jeffery wrote:
>>> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>>>> From: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
>>>> property to set the boot config register to boot from the first boot
>>>> area partition of the eMMC device.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>   hw/arm/aspeed.c | 15 +++++++++++----
>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>>> index 756deb91efd1..135f4eb72215 100644
>>>> --- a/hw/arm/aspeed.c
>>>> +++ b/hw/arm/aspeed.c
>>>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState 
>>>> *s, const char *flashtype,
>>>>       }
>>>>   }
>>>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, 
>>>> bool emmc)
>>>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, 
>>>> bool emmc,
>>>> +                               bool boot_emmc)
>>>>   {
>>>>           DeviceState *card;
>>>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState 
>>>> *sdhci, DriveInfo *dinfo, bool emmc)
>>>>               return;
>>>>           }
>>>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>> +        if (emmc) {
>>>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 
>>>> 0x48 : 0x0);
>>>
>>> 0x48 feels a little bit magic. I poked around a bit and there are some
>>> boot-config macros, but not the ones you need and they're all in an
>>> "internal" header anyway. I guess this is fine for now?
>>
>> You are right and we should be using these :
>>
>> hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179 /* 
>> R/W */
> 
> This field is R/W and expected to be configured by the guest.
> 
> Why the guest (u-boot) doesn't detect partitioning support?
> 
> See eMMC v4.5 section 7.4.60 PARTITIONING_SUPPORT [160] and earlier
> 
>    For e•MMC 4.5 Devices, Bit 2-0 in PARTITIONING_SUPPORT
>    shall be set to 1.
> 
> We don't set it so far.

Sorry, I wasn't grepping in the correct branch, we do set it:

      sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;

I'll investigate.

> I see in u-boot mmc_startup_v4():
> 
>      /* store the partition info of emmc */
>      mmc->part_support = ext_csd[EXT_CSD_PARTITIONING_SUPPORT];
>      if ((ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & PART_SUPPORT) ||
>          ext_csd[EXT_CSD_BOOT_MULT])
>              mmc->part_config = ext_csd[EXT_CSD_PART_CONF];
>      if (part_completed &&
>          (ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & ENHNCD_SUPPORT))
>              mmc->part_attr = ext_csd[EXT_CSD_PARTITIONS_ATTRIBUTE];
> 
> I'm still waiting for the eMMC prerequisite to be reviewed
> before looking at the eMMC patches in detail :/
> 
> Regards,
> 
> Phil.

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Philippe Mathieu-Daudé]

On 5/7/24 17:52, Philippe Mathieu-Daudé wrote:
> On 5/7/24 15:28, Philippe Mathieu-Daudé wrote:
>> On 5/7/24 07:38, Cédric Le Goater wrote:
>>> On 7/5/24 5:41 AM, Andrew Jeffery wrote:
>>>> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>>>>> From: Cédric Le Goater <clg@kaod.org>
>>>>>
>>>>> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
>>>>> property to set the boot config register to boot from the first boot
>>>>> area partition of the eMMC device.
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>   hw/arm/aspeed.c | 15 +++++++++++----
>>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>>>> index 756deb91efd1..135f4eb72215 100644
>>>>> --- a/hw/arm/aspeed.c
>>>>> +++ b/hw/arm/aspeed.c
>>>>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState 
>>>>> *s, const char *flashtype,
>>>>>       }
>>>>>   }
>>>>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo 
>>>>> *dinfo, bool emmc)
>>>>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo 
>>>>> *dinfo, bool emmc,
>>>>> +                               bool boot_emmc)
>>>>>   {
>>>>>           DeviceState *card;
>>>>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState 
>>>>> *sdhci, DriveInfo *dinfo, bool emmc)
>>>>>               return;
>>>>>           }
>>>>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>>> +        if (emmc) {
>>>>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 
>>>>> 0x48 : 0x0);
>>>>
>>>> 0x48 feels a little bit magic. I poked around a bit and there are some
>>>> boot-config macros, but not the ones you need and they're all in an
>>>> "internal" header anyway. I guess this is fine for now?
>>>
>>> You are right and we should be using these :
>>>
>>> hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179 /* 
>>> R/W */
>>
>> This field is R/W and expected to be configured by the guest.
>>
>> Why the guest (u-boot) doesn't detect partitioning support?
>>
>> See eMMC v4.5 section 7.4.60 PARTITIONING_SUPPORT [160] and earlier
>>
>>    For e•MMC 4.5 Devices, Bit 2-0 in PARTITIONING_SUPPORT
>>    shall be set to 1.
>>
>> We don't set it so far.
> 
> Sorry, I wasn't grepping in the correct branch, we do set it:
> 
>       sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
> 
> I'll investigate.

Correct behavior implemented (I hope) here:
https://lore.kernel.org/qemu-devel/20240709152556.52896-16-philmd@linaro.org/

Using it also simplifies this patch, we can squash:
-- >8 --
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 5a22e66ff5..27fea3003a 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -331,4 +331,3 @@ void aspeed_board_init_flashes(AspeedSMCState *s, 
const char *flashtype,

-static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, 
bool emmc,
-                               bool boot_emmc)
+static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, 
bool emmc)
  {
@@ -341,4 +340,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, 
DriveInfo *dinfo, bool emmc,
          if (emmc) {
-            qdev_prop_set_uint8(card, "boot-config",
-                                boot_emmc ? 
EXT_CSD_PART_CONFIG_EN_BOOT0 : 0x0);
+            qdev_prop_set_uint64(card, "boot-size", 1 * MiB);
          }
@@ -374,3 +372,2 @@ static void aspeed_machine_init(MachineState *machine)
      DriveInfo *emmc0 = NULL;
-    bool boot_emmc;

@@ -447,10 +444,8 @@ static void aspeed_machine_init(MachineState *machine)
          sdhci_attach_drive(&bmc->soc->sdhci.slots[i],
-                           drive_get(IF_SD, 0, i), false, false);
+                           drive_get(IF_SD, 0, i), false);
      }

-    boot_emmc = sc->boot_from_emmc(bmc->soc);
-
      if (bmc->soc->emmc.num_slots) {
          emmc0 = drive_get(IF_SD, 0, bmc->soc->sdhci.num_slots);
-        sdhci_attach_drive(&bmc->soc->emmc.slots[0], emmc0, true, 
boot_emmc);
+        sdhci_attach_drive(&bmc->soc->emmc.slots[0], emmc0, true);
      }
@@ -461,3 +456,3 @@ static void aspeed_machine_init(MachineState *machine)

-        if (fmc0 && !boot_emmc) {
+        if (fmc0 && !sc->boot_from_emmc(bmc->soc)) {
              uint64_t rom_size = memory_region_size(&bmc->soc->spi_boot);
---

IOW where we boot only affects the FMC, not (directly) the eMMC.

Regards,

Phil.

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Cédric Le Goater]

On 7/9/24 11:32 PM, Philippe Mathieu-Daudé wrote:
> On 5/7/24 17:52, Philippe Mathieu-Daudé wrote:
>> On 5/7/24 15:28, Philippe Mathieu-Daudé wrote:
>>> On 5/7/24 07:38, Cédric Le Goater wrote:
>>>> On 7/5/24 5:41 AM, Andrew Jeffery wrote:
>>>>> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>>>>>> From: Cédric Le Goater <clg@kaod.org>
>>>>>>
>>>>>> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
>>>>>> property to set the boot config register to boot from the first boot
>>>>>> area partition of the eMMC device.
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>   hw/arm/aspeed.c | 15 +++++++++++----
>>>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>>>>> index 756deb91efd1..135f4eb72215 100644
>>>>>> --- a/hw/arm/aspeed.c
>>>>>> +++ b/hw/arm/aspeed.c
>>>>>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>>>>>       }
>>>>>>   }
>>>>>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>>>>>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>>>>>> +                               bool boot_emmc)
>>>>>>   {
>>>>>>           DeviceState *card;
>>>>>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>>>>>>               return;
>>>>>>           }
>>>>>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>>>> +        if (emmc) {
>>>>>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
>>>>>
>>>>> 0x48 feels a little bit magic. I poked around a bit and there are some
>>>>> boot-config macros, but not the ones you need and they're all in an
>>>>> "internal" header anyway. I guess this is fine for now?
>>>>
>>>> You are right and we should be using these :
>>>>
>>>> hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179 /* R/W */
>>>
>>> This field is R/W and expected to be configured by the guest.
>>>
>>> Why the guest (u-boot) doesn't detect partitioning support?
>>>
>>> See eMMC v4.5 section 7.4.60 PARTITIONING_SUPPORT [160] and earlier
>>>
>>>    For e•MMC 4.5 Devices, Bit 2-0 in PARTITIONING_SUPPORT
>>>    shall be set to 1.
>>>
>>> We don't set it so far.
>>
>> Sorry, I wasn't grepping in the correct branch, we do set it:
>>
>>       sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
>>
>> I'll investigate.
> 
> Correct behavior implemented (I hope) here:
> https://lore.kernel.org/qemu-devel/20240709152556.52896-16-philmd@linaro.org/
> 
> Using it also simplifies this patch, we can squash:

I think we need both "boot-size" and "boot-config" properties to set
the associated registers, BOOT_CONFIG and BOOT_SIZE_MULT. BOOT_CONFIG
defines which devices are enabled for boot (partition 1, 2 or user area)
and BOOT_SIZE_MULT defines the size.
   
There is also a BOOT_INFO reg to define support for the alternate boot
method but I don't think we care much today.

Thanks,

C.

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Philippe Mathieu-Daudé]

On 10/7/24 07:14, Cédric Le Goater wrote:
> On 7/9/24 11:32 PM, Philippe Mathieu-Daudé wrote:
>> On 5/7/24 17:52, Philippe Mathieu-Daudé wrote:
>>> On 5/7/24 15:28, Philippe Mathieu-Daudé wrote:
>>>> On 5/7/24 07:38, Cédric Le Goater wrote:
>>>>> On 7/5/24 5:41 AM, Andrew Jeffery wrote:
>>>>>> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>>>>>>> From: Cédric Le Goater <clg@kaod.org>
>>>>>>>
>>>>>>> When the boot-from-eMMC HW strapping bit is set, use the 
>>>>>>> 'boot-config'
>>>>>>> property to set the boot config register to boot from the first boot
>>>>>>> area partition of the eMMC device.
>>>>>>>
>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>>> ---
>>>>>>>   hw/arm/aspeed.c | 15 +++++++++++----
>>>>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>>>>>> index 756deb91efd1..135f4eb72215 100644
>>>>>>> --- a/hw/arm/aspeed.c
>>>>>>> +++ b/hw/arm/aspeed.c
>>>>>>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState 
>>>>>>> *s, const char *flashtype,
>>>>>>>       }
>>>>>>>   }
>>>>>>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo 
>>>>>>> *dinfo, bool emmc)
>>>>>>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo 
>>>>>>> *dinfo, bool emmc,
>>>>>>> +                               bool boot_emmc)
>>>>>>>   {
>>>>>>>           DeviceState *card;
>>>>>>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState 
>>>>>>> *sdhci, DriveInfo *dinfo, bool emmc)
>>>>>>>               return;
>>>>>>>           }
>>>>>>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>>>>> +        if (emmc) {
>>>>>>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 
>>>>>>> 0x48 : 0x0);
>>>>>>
>>>>>> 0x48 feels a little bit magic. I poked around a bit and there are 
>>>>>> some
>>>>>> boot-config macros, but not the ones you need and they're all in an
>>>>>> "internal" header anyway. I guess this is fine for now?
>>>>>
>>>>> You are right and we should be using these :
>>>>>
>>>>> hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179 
>>>>> /* R/W */
>>>>
>>>> This field is R/W and expected to be configured by the guest.
>>>>
>>>> Why the guest (u-boot) doesn't detect partitioning support?
>>>>
>>>> See eMMC v4.5 section 7.4.60 PARTITIONING_SUPPORT [160] and earlier
>>>>
>>>>    For e•MMC 4.5 Devices, Bit 2-0 in PARTITIONING_SUPPORT
>>>>    shall be set to 1.
>>>>
>>>> We don't set it so far.
>>>
>>> Sorry, I wasn't grepping in the correct branch, we do set it:
>>>
>>>       sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
>>>
>>> I'll investigate.
>>
>> Correct behavior implemented (I hope) here:
>> https://lore.kernel.org/qemu-devel/20240709152556.52896-16-philmd@linaro.org/
>>
>> Using it also simplifies this patch, we can squash:
> 
> I think we need both "boot-size" and "boot-config" properties to set
> the associated registers, BOOT_CONFIG and BOOT_SIZE_MULT. BOOT_CONFIG
> defines which devices are enabled for boot (partition 1, 2 or user area)
> and BOOT_SIZE_MULT defines the size.

I disagree: it is the guest responsibility to set the BOOT_CONFIG
register (using the SWITCH command). Unlike the BOOT_CONFIG register
which is in the (read-only) Properties Segment, the BOOT_SIZE_MULT
is in the (read-write) Modes segment and its default value is 0x00.

See the Spec v4.3, chapter 8.4 "Extended CSD register":

       The Extended CSD register defines the card properties
       and selected modes. It is 512 bytes long. The most
       significant 320 bytes are the Properties segment, which
       defines the card capabilities and cannot be modified by
       the host. The lower 192 bytes are the Modes segment,
       which defines the configuration the card is working in.
       These modes can be changed by the host by means of the
       SWITCH command.

For example in u-boot BOOT_CONFIG is set by mmc_set_part_conf():

     /*
      * Modify EXT_CSD[179] which is PARTITION_CONFIG (formerly
      * BOOT_CONFIG) based on the passed in values for BOOT_ACK,
      * BOOT_PARTITION_ENABLE and PARTITION_ACCESS.
      *
      * Returns 0 on success.
      */
     int mmc_set_part_conf(struct mmc *mmc, u8 ack,
                           u8 part_num, u8 access)
     {
         int ret;
         u8 part_conf;

         part_conf = EXT_CSD_BOOT_ACK(ack) |
                     EXT_CSD_BOOT_PART_NUM(part_num) |
                     EXT_CSD_PARTITION_ACCESS(access);

         ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
                          part_conf);
         if (!ret)
                 mmc->part_config = part_conf;

         return ret;
     }

> There is also a BOOT_INFO reg to define support for the alternate boot
> method but I don't think we care much today.

BOOT_INFO is in the RO Properties segment. We can add a QOM property
once we model the alternate boot mode (we don't so far).

> 
> Thanks,
> 
> C.
> 
> 

Re: [PATCH 5/8] aspeed: Set eMMC 'boot-config' property to reflect HW strapping

[Cédric Le Goater]

On 7/10/24 9:07 AM, Philippe Mathieu-Daudé wrote:
> On 10/7/24 07:14, Cédric Le Goater wrote:
>> On 7/9/24 11:32 PM, Philippe Mathieu-Daudé wrote:
>>> On 5/7/24 17:52, Philippe Mathieu-Daudé wrote:
>>>> On 5/7/24 15:28, Philippe Mathieu-Daudé wrote:
>>>>> On 5/7/24 07:38, Cédric Le Goater wrote:
>>>>>> On 7/5/24 5:41 AM, Andrew Jeffery wrote:
>>>>>>> On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
>>>>>>>> From: Cédric Le Goater <clg@kaod.org>
>>>>>>>>
>>>>>>>> When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
>>>>>>>> property to set the boot config register to boot from the first boot
>>>>>>>> area partition of the eMMC device.
>>>>>>>>
>>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>>>> ---
>>>>>>>>   hw/arm/aspeed.c | 15 +++++++++++----
>>>>>>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>>>>>>> index 756deb91efd1..135f4eb72215 100644
>>>>>>>> --- a/hw/arm/aspeed.c
>>>>>>>> +++ b/hw/arm/aspeed.c
>>>>>>>> @@ -327,7 +327,8 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>>>>>>>       }
>>>>>>>>   }
>>>>>>>> -static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>>>>>>>> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>>>>>>>> +                               bool boot_emmc)
>>>>>>>>   {
>>>>>>>>           DeviceState *card;
>>>>>>>> @@ -335,6 +336,9 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc)
>>>>>>>>               return;
>>>>>>>>           }
>>>>>>>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>>>>>> +        if (emmc) {
>>>>>>>> +            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
>>>>>>>
>>>>>>> 0x48 feels a little bit magic. I poked around a bit and there are some
>>>>>>> boot-config macros, but not the ones you need and they're all in an
>>>>>>> "internal" header anyway. I guess this is fine for now?
>>>>>>
>>>>>> You are right and we should be using these :
>>>>>>
>>>>>> hw/sd/sdmmc-internal.h:#define EXT_CSD_PART_CONFIG             179 /* R/W */
>>>>>
>>>>> This field is R/W and expected to be configured by the guest.
>>>>>
>>>>> Why the guest (u-boot) doesn't detect partitioning support?
>>>>>
>>>>> See eMMC v4.5 section 7.4.60 PARTITIONING_SUPPORT [160] and earlier
>>>>>
>>>>>    For e•MMC 4.5 Devices, Bit 2-0 in PARTITIONING_SUPPORT
>>>>>    shall be set to 1.
>>>>>
>>>>> We don't set it so far.
>>>>
>>>> Sorry, I wasn't grepping in the correct branch, we do set it:
>>>>
>>>>       sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
>>>>
>>>> I'll investigate.
>>>
>>> Correct behavior implemented (I hope) here:
>>> https://lore.kernel.org/qemu-devel/20240709152556.52896-16-philmd@linaro.org/
>>>
>>> Using it also simplifies this patch, we can squash:
>>
>> I think we need both "boot-size" and "boot-config" properties to set
>> the associated registers, BOOT_CONFIG and BOOT_SIZE_MULT. BOOT_CONFIG
>> defines which devices are enabled for boot (partition 1, 2 or user area)
>> and BOOT_SIZE_MULT defines the size.
> 
> I disagree: it is the guest responsibility to set the BOOT_CONFIG
> register (using the SWITCH command). Unlike the BOOT_CONFIG register
> which is in the (read-only) Properties Segment,

hmm, in section 8.4 Extended CSD register, table 49 says
BOOT_CONFIG is R/W.

> the BOOT_SIZE_MULT
> is in the (read-write) Modes segment and its default value is 0x00.

and BOOT_SIZE_MULTI is RO

> See the Spec v4.3, chapter 8.4 "Extended CSD register":
> 
>        The Extended CSD register defines the card properties
>        and selected modes. It is 512 bytes long. The most
>        significant 320 bytes are the Properties segment, which
>        defines the card capabilities and cannot be modified by
>        the host. The lower 192 bytes are the Modes segment,
>        which defines the configuration the card is working in.
>        These modes can be changed by the host by means of the
>        SWITCH command.

yes and how is initial boot done ? You have start from eMMC at some
point.

> 
> For example in u-boot BOOT_CONFIG is set by mmc_set_part_conf():
> 
>      /*
>       * Modify EXT_CSD[179] which is PARTITION_CONFIG (formerly
>       * BOOT_CONFIG) based on the passed in values for BOOT_ACK,
>       * BOOT_PARTITION_ENABLE and PARTITION_ACCESS.
>       *
>       * Returns 0 on success.
>       */
>      int mmc_set_part_conf(struct mmc *mmc, u8 ack,
>                            u8 part_num, u8 access)
>      {
>          int ret;
>          u8 part_conf;
> 
>          part_conf = EXT_CSD_BOOT_ACK(ack) |
>                      EXT_CSD_BOOT_PART_NUM(part_num) |
>                      EXT_CSD_PARTITION_ACCESS(access);
> 
>          ret = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_PART_CONF,
>                           part_conf);
>          if (!ret)
>                  mmc->part_config = part_conf;
> 
>          return ret;
>      }

OK. It has been 5y since I last looked ! The part above is a u-boot
command which means the board has already booted on some device (flash)
and this command lets you boot from another device (eMMC)

I don't remember how exactly how this works on AST2600. I think the
SoC secure boot controller sets the EXT_CSD config on the eMMC device
and loads boot data in RAM. This is what the model is doing today when
setting the boot config property and loading a ROM.

Anyhow, having two properties is not a big issue. It gives more
flexibility to model the HW implementation of the boot part.


Thanks,

C.

[PATCH 6/8] aspeed: Add boot-from-eMMC HW strapping bit to rainier-bmc machine

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

This value is taken from a running Rainier machine.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 135f4eb72215..abd2412ad8b9 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -189,7 +189,7 @@ struct AspeedMachineState {
 #define TACOMA_BMC_HW_STRAP2  0x00000040
 
 /* Rainier hardware value: (QEMU prototype) */
-#define RAINIER_BMC_HW_STRAP1 0x00422016
+#define RAINIER_BMC_HW_STRAP1 (0x00422016 | AST2600_HW_STRAP_BOOT_SRC_EMMC)
 #define RAINIER_BMC_HW_STRAP2 0x80000848
 
 /* Fuji hardware value */
-- 
2.45.2

Re: [PATCH 6/8] aspeed: Add boot-from-eMMC HW strapping bit to rainier-bmc machine

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> This value is taken from a running Rainier machine.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH 7/8] aspeed: Introduce a 'hw_strap1' machine attribute

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

To change default behavior of a machine and boot from eMMC, future
changes will add a machine option to let the user configure the
boot-from-eMMC HW strapping bit. Add a new machine attribute first.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index abd2412ad8b9..4ce995e875da 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -46,6 +46,7 @@ struct AspeedMachineState {
     uint32_t uart_chosen;
     char *fmc_model;
     char *spi_model;
+    uint32_t hw_strap1;
 };
 
 /* On 32-bit hosts, lower RAM to 1G because of the 2047 MB limit */
@@ -391,7 +392,7 @@ static void aspeed_machine_init(MachineState *machine)
         }
     }
 
-    object_property_set_int(OBJECT(bmc->soc), "hw-strap1", amc->hw_strap1,
+    object_property_set_int(OBJECT(bmc->soc), "hw-strap1", bmc->hw_strap1,
                             &error_abort);
     object_property_set_int(OBJECT(bmc->soc), "hw-strap2", amc->hw_strap2,
                             &error_abort);
@@ -1075,7 +1076,10 @@ static void aspeed_set_mmio_exec(Object *obj, bool value, Error **errp)
 
 static void aspeed_machine_instance_init(Object *obj)
 {
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(obj);
+
     ASPEED_MACHINE(obj)->mmio_exec = false;
+    ASPEED_MACHINE(obj)->hw_strap1 = amc->hw_strap1;
 }
 
 static char *aspeed_get_fmc_model(Object *obj, Error **errp)
-- 
2.45.2

Re: [PATCH 7/8] aspeed: Introduce a 'hw_strap1' machine attribute

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> To change default behavior of a machine and boot from eMMC, future
> changes will add a machine option to let the user configure the
> boot-from-eMMC HW strapping bit. Add a new machine attribute first.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH 8/8] aspeed: Introduce a 'boot-emmc' machine option

[Cédric Le Goater]

From: Cédric Le Goater <clg@kaod.org>

The default behavior of some Aspeed machines is to boot from the eMMC
device, like the rainier-bmc. Others like ast2600-evb could also boot
from eMMC if the HW strapping boot-from-eMMC bit was set. Add a
property to set or unset this bit. This is useful to test boot images.

For now, only activate this property on the ast2600-evb and rainier-bmc
machines for which eMMC images are available or can be built.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 docs/system/arm/aspeed.rst |  2 ++
 hw/arm/aspeed.c            | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index cd9559e3e291..6733ffd2b941 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -123,6 +123,8 @@ To boot the machine from the flash image, use an MTD drive :
 
 Options specific to Aspeed machines are :
 
+ * ``boot-emmc`` to set or unset boot from eMMC (AST2600).
+
  * ``execute-in-place`` which emulates the boot from the CE0 flash
    device by using the FMC controller to load the instructions, and
    not simply from RAM. This takes a little longer.
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 4ce995e875da..e7cfd0c5fbcd 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1176,6 +1176,34 @@ static void aspeed_machine_class_init_cpus_defaults(MachineClass *mc)
     mc->valid_cpu_types = sc->valid_cpu_types;
 }
 
+static bool aspeed_machine_ast2600_get_boot_from_emmc(Object *obj, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+
+    return !!(bmc->hw_strap1 & AST2600_HW_STRAP_BOOT_SRC_EMMC);
+}
+
+static void aspeed_machine_ast2600_set_boot_from_emmc(Object *obj, bool value,
+                                                      Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+
+    if (value) {
+        bmc->hw_strap1 |= AST2600_HW_STRAP_BOOT_SRC_EMMC;
+    } else {
+        bmc->hw_strap1 &= ~AST2600_HW_STRAP_BOOT_SRC_EMMC;
+    }
+}
+
+static void aspeed_machine_ast2600_class_emmc_init(ObjectClass *oc)
+{
+    object_class_property_add_bool(oc, "boot-emmc",
+                                   aspeed_machine_ast2600_get_boot_from_emmc,
+                                   aspeed_machine_ast2600_set_boot_from_emmc);
+    object_class_property_set_description(oc, "boot-emmc",
+                                          "Set or unset boot from EMMC");
+}
+
 static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1375,6 +1403,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
     amc->i2c_init  = ast2600_evb_i2c_init;
     mc->default_ram_size = 1 * GiB;
     aspeed_machine_class_init_cpus_defaults(mc);
+    aspeed_machine_ast2600_class_emmc_init(oc);
 };
 
 static void aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
@@ -1447,6 +1476,7 @@ static void aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
     amc->i2c_init  = rainier_bmc_i2c_init;
     mc->default_ram_size = 1 * GiB;
     aspeed_machine_class_init_cpus_defaults(mc);
+    aspeed_machine_ast2600_class_emmc_init(oc);
 };
 
 #define FUJI_BMC_RAM_SIZE ASPEED_RAM_SIZE(2 * GiB)
-- 
2.45.2

Re: [PATCH 8/8] aspeed: Introduce a 'boot-emmc' machine option

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> The default behavior of some Aspeed machines is to boot from the eMMC
> device, like the rainier-bmc. Others like ast2600-evb could also boot
> from eMMC if the HW strapping boot-from-eMMC bit was set. Add a
> property to set or unset this bit. This is useful to test boot images.
> 
> For now, only activate this property on the ast2600-evb and rainier-bmc
> machines for which eMMC images are available or can be built.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Re: [PATCH 0/8] aspeed: Add boot from eMMC support (AST2600)

[Andrew Jeffery]

On Thu, 2024-07-04 at 07:36 +0200, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> Hello,
> 
> This series enables boot from eMMC on the rainier-bmc machine, which
> is the default behavior and also on the AST2600 EVB using a machine
> option to change the default.
> 
> It depends solely on the availability of an eMMC device model which is
> currently being discussed upstream. Numerous patches have been merged
> already and remaining ones [1] should be in 9.1 development cycle.
> Since the changes of this series are small and localized, it would be
> an interesting extension of the AST2600 SoC model for QEMU 9.1.
> 
> First 6 patches adjust the machine setup and HW strapping to boot from
> eMMC, the last 2 are for the AST2600 EVB and are optional.
> 
> Thanks,
> 
> C.
> 
> [1] https://github.com/legoater/qemu/commits/aspeed-9.1

I built the series using the tree you linked above. It works well!

In that context:

Tested-by: Andrew Jeffery <andrew@codeconstruct.com.au>