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

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

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.
> 
>