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, archived
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.
Philippe Mathieu-Daudé July 9, 2024, 9:32 p.m. UTC | #5 
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.
Cédric Le Goater July 10, 2024, 5:14 a.m. UTC | #6 
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.
Philippe Mathieu-Daudé July 10, 2024, 7:07 a.m. UTC | #7 
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.
> 
>
Cédric Le Goater July 10, 2024, 7:54 a.m. UTC | #8 
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.
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) {