Re: [PATCH] hw/sd/sdcard: Allow user creation of eMMCs

["Cédric Le Goater" <clg@kaod.org>]

On 11/8/24 10:29, Jan Lübbe wrote:
> On Tue, 2024-10-29 at 15:06 +0000, Peter Maydell wrote:
>> On Fri, 18 Oct 2024 at 16:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On Tue, 15 Oct 2024 at 14:57, Jan Luebbe <jlu@pengutronix.de> wrote:
>>>> For testing eMMC-specific functionality (such as handling boot
>>>> partitions), it would be very useful to attach them to generic VMs such
>>>> as x86_64 via the sdhci-pci device:
>>>>   ...
>>>>   -drive if=none,id=emmc-drive,file=emmc.img,format=raw \
>>>>   -device sdhci-pci \
>>>>   -device emmc,id=emmc0,drive=emmc-drive,boot-partition-size=1048576 \
>>>>   ...
>>>>
>>>> While most eMMCs are soldered to boards, they can also be connected to
>>>> SD controllers with just a passive adapter, such as:
>>>>   https://docs.radxa.com/en/accessories/emmc-to-usd
>>>>   https://github.com/voltlog/emmc-wfbga153-microsd
>>>>
>>>> The only change necessary to make the options above work is to avoid
>>>> disabling user_creatable, so do that. The SDHCI-PCI driver in the Linux
>>>> kernel already supports this just fine.
>>>>
>>>> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
>>>
>>> Applied to target-arm.next, thanks (unless anybody would
>>> prefer it to go via some other route).
>>
>> I'm dropping this from target-arm.next since it seems like
>> we have a problem with the handling of boot partitions
>> and how the user should provide an image for an emmc card
>> that has boot partitions). Since that's an emmc specific
>> thing, sorting that out with a minimum of breaking
>> compatibility with previously working setups is going to
>> be easier if we stay temporarily in the state of "emmc
>> only happens for the specific board that creates them
>> and the user can't arbitrarily create them on the
>> command line".
>>
>> I expect this to just be a temporary delay while we sort
>> out in the other thread how emmc boot partitions should work.
> 
> With Cédric's e8f3acdbb8 ("aspeed: Don't set always boot properties of the emmc
> device") and my c078298301 ("hw/sd/sdcard: Fix calculation of size when using
> eMMC boot partitions") in master, compatibility for existing setups should be
> taken care of.
> 
> As mentioned in Cédric's patch, allowing user creatable eMMC devices is still
> desirable. With my patch, that would work for machines where the SD controller
> is user-created as well (e.g. x86_64 with sdhci-pci).
> 
> For machines where the SD controller and SD/eMMC are pre-created, additional
> changes seem to be needed.

Yes. That's a problem to fix for the emmc/sd devices created on the
Aspeed machines. They should only be created when defaults_enabled(),
just like the flash devices.

Also, to be able to assign a device to a sd bus from the command line,
more changes are required. A first series would be to avoid referencing
"sd-bus" where possible :

-        qdev_realize_and_unref(card,
-                               qdev_get_child_bus(DEVICE(sdhci), "sd-bus"),
-                               &error_fatal);
+        qdev_realize_and_unref(card, BUS(&sdhci->sdbus), &error_fatal);

the final touch would be :

  void sdhci_initfn(SDHCIState *s)
  {
-    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
+    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SDHCI_BUS, DEVICE(s), NULL);

I haven't looked at all the machines though. Aspeed works fine with that
and emmc devices can be defined using :

   -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw
   -device emmc,bus=sdhci-bus.2,drive=emmc0,id=foo

> Would you consider taking this patch to solve the
> simple case first?

I think we need to address the above first for sd and emmc devices.

Thanks,

C.