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

["Cédric Le Goater" <clg@redhat.com>]

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.