Re: [PATCH v47 00/19] hw/sd/sdcard: Add eMMC support

[Cédric Le Goater]

On 7/9/24 11:39 PM, Philippe Mathieu-Daudé wrote:
> On 9/7/24 17:58, Cédric Le Goater wrote:
> > On 7/9/24 5:25 PM, Philippe Mathieu-Daudé wrote:
> > > Since v42:
> > > - Stick to spec v4.3 (re-simplified EXT_CSD register & migrate)
> > > - Fill CID register
> > > - Few changes to CSD register
> > > - Implement 'boot-mode' reset timing
> > > - Add 'boot-size' property
> > >
> > > Change required for aspeed branch:
> > > -- >8 --
> > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > > index 8c0e36badd..563816b710 100644
> > > --- a/hw/arm/aspeed.c
> > > +++ b/hw/arm/aspeed.c
> > > @@ -344,3 +344,3 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo 
> > > *dinfo, bool emmc,
> > >           if (emmc) {
> > > -            qdev_prop_set_uint8(card, "boot-config", boot_emmc ? 0x48 : 0x0);
> > > +            qdev_prop_set_uint64(card, "boot-size", 1 * MiB);
> > >           }
> > > (I'm still reluctant to merge patches 16-18)...
> >
> > Then, please drop all changes related to the boot partitions. I will keep
> > the original patches in my tree and address the feature when I have time.
> > TYPE_EMMC is already great to have.
>
> As you mentioned on today's community call, eMMC is a chipset soldered
> on a board, so our boards exactly knows what size to expect on the card,
> and whether boot partitions are present or not. 

My remark was on the use of 3 blockdevs to represent the eMMC contents.
1 should be enough.

Having a "boot-size" property to set the BOOT_SIZE_MULT register is fine.
The Aspeed model does some assumption today when installing the first
boot area as a boot ROM :

    aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(emmc0), 64 * KiB);

The "boot-size" property will clarify the settings. Please keep it.

Other topic :

     ROMs need to be installed early and IIRC, user creatable devices
     are not available at that time. So, when the flash devices are be
     defined on the command line with :

        -blockdev node-name=fmc0,driver=file,filename=./flash.img \
        -device mx66u51235f,cs=0x0,bus=ssi.0,drive=fmc0 \

     the ROM can not be installed and the initial boot will execute
     in place, using SPI transactions to fetch instructions which is
     slower. To install a ROM, the -drive API is required.

> I find the way of building the drive image described in patch #16 cumbersome, 

I agree but that's the layout on real HW. The flash layouts are even more
complex.

> but I'm OK
> to make some concession on it, since "it works" as it. If necessary
> we'll improve and deprecate the current properties. 

I don't think we will need to. The properties make sense.

> I'll repost and plan to merge as-is (modulo review comments).

OK. I will rebase then.

Thanks,

C.