diff mbox series

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

Message ID 20240704053651.1100732-6-clg@redhat.com (mailing list archive)
State New
Headers show
Series aspeed: Add boot from eMMC support (AST2600) | expand

Commit Message

Cédric Le Goater July 4, 2024, 5:36 a.m. UTC 
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(-)

Comments

Andrew Jeffery July 5, 2024, 3:41 a.m. UTC | #1 
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>
Cédric Le Goater July 5, 2024, 5:38 a.m. UTC | #2 
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.
Philippe Mathieu-Daudé July 5, 2024, 1:28 p.m. UTC | #3 
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.
Philippe Mathieu-Daudé July 5, 2024, 3:52 p.m. UTC | #4 
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.
diff mbox series

Patch

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) {