Re: [PATCH 08/21] hw/arm/fsl-imx8mp: Add USDHC storage controllers

[Bernhard Beschow <shentey@gmail.com>]

Am 21. Januar 2025 02:52:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 20 Jan 2025, Bernhard Beschow wrote:
>> The USDHC emulation allows for running real-world images such as those generated
>> by Buildroot. Convert the board documentation accordingly instead of running a
>> Linux kernel with ephemeral storage.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> docs/system/arm/imx8mp-evk.rst | 39 +++++++++++++++++++++++-----------
>> include/hw/arm/fsl-imx8mp.h    |  7 ++++++
>> hw/arm/fsl-imx8mp.c            | 28 ++++++++++++++++++++++++
>> hw/arm/imx8mp-evk.c            | 18 ++++++++++++++++
>> hw/arm/Kconfig                 |  1 +
>> 5 files changed, 81 insertions(+), 12 deletions(-)
>> 
>> diff --git a/docs/system/arm/imx8mp-evk.rst b/docs/system/arm/imx8mp-evk.rst
>> index d7d08cc198..1514bc5864 100644
>> --- a/docs/system/arm/imx8mp-evk.rst
>> +++ b/docs/system/arm/imx8mp-evk.rst
>> @@ -13,6 +13,7 @@ The ``imx8mp-evk`` machine implements the following devices:
>>  * Up to 4 Cortex-A53 Cores
>>  * Generic Interrupt Controller (GICv3)
>>  * 4 UARTs
>> + * 3 USDHC Storage Controllers
>>  * Secure Non-Volatile Storage (SNVS) including an RTC
>>  * Clock Tree
>> 
>> @@ -25,25 +26,39 @@ for loading a Linux kernel.
>> Direct Linux Kernel Boot
>> ''''''''''''''''''''''''
>> 
>> -Linux mainline v6.12 release is tested at the time of writing. To build a Linux
>> -mainline kernel that can be booted by the ``imx8mp-evk`` machine, simply
>> -configure the kernel using the defconfig configuration:
>> +Probably the easiest way to get started with a whole Linux system on the machine
>> +is to generate an image with Buildroot. Version 2024.11.1 is tested at the time
>> +of writing and involves three steps. First run the following command in the
>> +toplevel directory of the Buildroot source tree:
>> 
>> .. code-block:: bash
>> 
>> -  $ export ARCH=arm64
>> -  $ export CROSS_COMPILE=aarch64-linux-gnu-
>> -  $ make defconfig
>> +  $ make freescale_imx8mpevk_defconfig
>>   $ make
>
>Adding documentation that is removed a few patches later seems unnecessary. Maybe you could keep the bare kernel docs as that could also be useful or not add it in the first place? But if this is already written keeping it may make more sense than dropping it.

I'd like the documentation to be complete and easy to follow at the same time. Buildroot achieves both, and allows for generating a cpio initrd instead. So I'd start with that which just requires minor adjustments here.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 
>> -To boot the newly built Linux kernel in QEMU with the ``imx8mp-evk`` machine,
>> -run:
>> +Once finished successfully there is an ``output/image`` subfolder. Navigate into
>> +it and resize the SD card image to a power of two:
>> +
>> +.. code-block:: bash
>> +
>> +  $ qemu-img resize sdcard.img 256M
>> +
>> +Finally, the device tree needs to be patched with the following commands which
>> +will remove the ``cpu-idle-states`` properties from CPU nodes:
>> +
>> +.. code-block:: bash
>> +
>> +  $ dtc imx8mp-evk.dtb | sed '/cpu-idle-states/d' > imx8mp-evk-patched.dts
>> +  $ dtc imx8mp-evk-patched.dts -o imx8mp-evk-patched.dtb
>> +
>> +Now that everything is prepared the newly built image can be run in the QEMU
>> +``imx8mp-evk`` machine:
>> 
>> .. code-block:: bash
>> 
>>   $ qemu-system-aarch64 -M imx8mp-evk -smp 4 -m 3G \
>>       -display none -serial null -serial stdio \
>> -      -kernel arch/arm64/boot/Image \
>> -      -dtb arch/arm64/boot/dts/freescale/imx8mp-evk.dtb \
>> -      -initrd /path/to/rootfs.ext4 \
>> -      -append "root=/dev/ram"
>> +      -kernel Image \
>> +      -dtb imx8mp-evk-patched.dtb \
>> +      -append "root=/dev/mmcblk2p2" \
>> +      -drive file=sdcard.img,if=sd,bus=2,format=raw,id=mmcblk2
>> diff --git a/include/hw/arm/fsl-imx8mp.h b/include/hw/arm/fsl-imx8mp.h
>> index 9d2a757c2a..9832c05e8c 100644
>> --- a/include/hw/arm/fsl-imx8mp.h
>> +++ b/include/hw/arm/fsl-imx8mp.h
>> @@ -14,6 +14,7 @@
>> #include "hw/intc/arm_gicv3_common.h"
>> #include "hw/misc/imx7_snvs.h"
>> #include "hw/misc/imx8mp_ccm.h"
>> +#include "hw/sd/sdhci.h"
>> #include "qom/object.h"
>> #include "qemu/units.h"
>> 
>> @@ -27,6 +28,7 @@ enum FslImx8mpConfiguration {
>>     FSL_IMX8MP_NUM_CPUS         = 4,
>>     FSL_IMX8MP_NUM_IRQS         = 160,
>>     FSL_IMX8MP_NUM_UARTS        = 4,
>> +    FSL_IMX8MP_NUM_USDHCS       = 3,
>> };
>> 
>> struct FslImx8mpState {
>> @@ -38,6 +40,7 @@ struct FslImx8mpState {
>>     IMX8MPAnalogState  analog;
>>     IMX7SNVSState      snvs;
>>     IMXSerialState     uart[FSL_IMX8MP_NUM_UARTS];
>> +    SDHCIState         usdhc[FSL_IMX8MP_NUM_USDHCS];
>> };
>> 
>> enum FslImx8mpMemoryRegions {
>> @@ -183,6 +186,10 @@ enum FslImx8mpMemoryRegions {
>> };
>> 
>> enum FslImx8mpIrqs {
>> +    FSL_IMX8MP_USDHC1_IRQ   = 22,
>> +    FSL_IMX8MP_USDHC2_IRQ   = 23,
>> +    FSL_IMX8MP_USDHC3_IRQ   = 24,
>> +
>>     FSL_IMX8MP_UART1_IRQ    = 26,
>>     FSL_IMX8MP_UART2_IRQ    = 27,
>>     FSL_IMX8MP_UART3_IRQ    = 28,
>> diff --git a/hw/arm/fsl-imx8mp.c b/hw/arm/fsl-imx8mp.c
>> index 0abde2b1fc..612ea7bf93 100644
>> --- a/hw/arm/fsl-imx8mp.c
>> +++ b/hw/arm/fsl-imx8mp.c
>> @@ -210,6 +210,11 @@ static void fsl_imx8mp_init(Object *obj)
>>         snprintf(name, NAME_SIZE, "uart%d", i + 1);
>>         object_initialize_child(obj, name, &s->uart[i], TYPE_IMX_SERIAL);
>>     }
>> +
>> +    for (i = 0; i < FSL_IMX8MP_NUM_USDHCS; i++) {
>> +        snprintf(name, NAME_SIZE, "usdhc%d", i + 1);
>> +        object_initialize_child(obj, name, &s->usdhc[i], TYPE_IMX_USDHC);
>> +    }
>> }
>> 
>> static void fsl_imx8mp_realize(DeviceState *dev, Error **errp)
>> @@ -350,6 +355,28 @@ static void fsl_imx8mp_realize(DeviceState *dev, Error **errp)
>>                            qdev_get_gpio_in(gicdev, serial_table[i].irq));
>>     }
>> 
>> +    /* USDHCs */
>> +    for (i = 0; i < FSL_IMX8MP_NUM_USDHCS; i++) {
>> +        static const struct {
>> +            hwaddr addr;
>> +            unsigned int irq;
>> +        } usdhc_table[FSL_IMX8MP_NUM_USDHCS] = {
>> +            { fsl_imx8mp_memmap[FSL_IMX8MP_USDHC1].addr, FSL_IMX8MP_USDHC1_IRQ },
>> +            { fsl_imx8mp_memmap[FSL_IMX8MP_USDHC2].addr, FSL_IMX8MP_USDHC2_IRQ },
>> +            { fsl_imx8mp_memmap[FSL_IMX8MP_USDHC3].addr, FSL_IMX8MP_USDHC3_IRQ },
>> +        };
>> +
>> +        object_property_set_uint(OBJECT(&s->usdhc[i]), "vendor",
>> +                                 SDHCI_VENDOR_IMX, &error_abort);
>> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->usdhc[i]), errp)) {
>> +            return;
>> +        }
>> +
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0, usdhc_table[i].addr);
>> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
>> +                           qdev_get_gpio_in(gicdev, usdhc_table[i].irq));
>> +    }
>> +
>>     /* SNVS */
>>     if (!sysbus_realize(SYS_BUS_DEVICE(&s->snvs), errp)) {
>>         return;
>> @@ -367,6 +394,7 @@ static void fsl_imx8mp_realize(DeviceState *dev, Error **errp)
>>         case FSL_IMX8MP_RAM:
>>         case FSL_IMX8MP_SNVS_HP:
>>         case FSL_IMX8MP_UART1 ... FSL_IMX8MP_UART4:
>> +        case FSL_IMX8MP_USDHC1 ... FSL_IMX8MP_USDHC3:
>>             /* device implemented and treated above */
>>             break;
>> 
>> diff --git a/hw/arm/imx8mp-evk.c b/hw/arm/imx8mp-evk.c
>> index 2756d4c21c..27d9e9e8ee 100644
>> --- a/hw/arm/imx8mp-evk.c
>> +++ b/hw/arm/imx8mp-evk.c
>> @@ -11,6 +11,7 @@
>> #include "hw/arm/boot.h"
>> #include "hw/arm/fsl-imx8mp.h"
>> #include "hw/boards.h"
>> +#include "hw/qdev-properties.h"
>> #include "system/qtest.h"
>> #include "qemu/error-report.h"
>> #include "qapi/error.h"
>> @@ -40,6 +41,23 @@ static void imx8mp_evk_init(MachineState *machine)
>>     memory_region_add_subregion(get_system_memory(), FSL_IMX8MP_RAM_START,
>>                                 machine->ram);
>> 
>> +    for (int i = 0; i < FSL_IMX8MP_NUM_USDHCS; i++) {
>> +        BusState *bus;
>> +        DeviceState *carddev;
>> +        BlockBackend *blk;
>> +        DriveInfo *di = drive_get(IF_SD, i, 0);
>> +
>> +        if (!di) {
>> +            continue;
>> +        }
>> +
>> +        blk = blk_by_legacy_dinfo(di);
>> +        bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
>> +        carddev = qdev_new(TYPE_SD_CARD);
>> +        qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
>> +        qdev_realize_and_unref(carddev, bus, &error_fatal);
>> +    }
>> +
>>     if (!qtest_enabled()) {
>>         arm_load_kernel(&s->cpu[0], machine, &boot_info);
>>     }
>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>> index adb4ed8076..f880c03d35 100644
>> --- a/hw/arm/Kconfig
>> +++ b/hw/arm/Kconfig
>> @@ -582,6 +582,7 @@ config FSL_IMX8MP
>>     imply TEST_DEVICES
>>     select ARM_GIC
>>     select IMX
>> +    select SDHCI
>>     select UNIMP
>> 
>> config FSL_IMX8MP_EVK
>>