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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.