Re: [PATCH v2 13/13] hw/ppc/e500: Add Freescale eSDHC to e500 boards

[Bernhard Beschow]

Am 3. Oktober 2022 21:06:57 UTC schrieb "Philippe Mathieu-Daudé" 
<f4bug@amsat.org>:
>On 3/10/22 22:31, Bernhard Beschow wrote:
>> Adds missing functionality to emulated e500 SOCs which increases the
>> chance of given "real" firmware images to access SD cards.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   docs/system/ppc/ppce500.rst | 13 +++++++++++++
>>   hw/ppc/Kconfig              |  1 +
>>   hw/ppc/e500.c               | 31 ++++++++++++++++++++++++++++++-
>>   3 files changed, 44 insertions(+), 1 deletion(-)
>
>> +static void dt_sdhc_create(void *fdt, const char *parent, const char *mpic)
>> +{
>> +    hwaddr mmio = MPC85XX_ESDHC_REGS_OFFSET;
>> +    hwaddr size = MPC85XX_ESDHC_REGS_SIZE;
>> +    int irq = MPC85XX_ESDHC_IRQ;
>
>Why not pass these 3 variable as argument?

In anticipation of data-driven board creation, I'd ideally infer those from the 
device's QOM properties. This seems similar to what Mark suggested in the BoF 
at KVM Forum [1], where -- IIUC -- he stated that QOM properties could be the 
foundation of all wiring representations. And device tree seems just like one 
specialized representation to me. (Note that I'm slightly hijacking the review 
here because I don't know where and how to express these thoughts elsewhere).

Does it make sense to add the missing properties here?

Best regards,
Bernhard

[1] https://etherpad.opendev.org/p/qemu-emulation-bof%40kvmforum2022

>
>> +    g_autofree char *name = NULL;
>> +
>> +    name = g_strdup_printf("%s/sdhc@%" PRIx64, parent, mmio);
>> +    qemu_fdt_add_subnode(fdt, name);
>> +    qemu_fdt_setprop(fdt, name, "sdhci,auto-cmd12", NULL, 0);
>> +    qemu_fdt_setprop_phandle(fdt, name, "interrupt-parent", mpic);
>> +    qemu_fdt_setprop_cells(fdt, name, "bus-width", 4);
>> +    qemu_fdt_setprop_cells(fdt, name, "interrupts", irq, 0x2);
>> +    qemu_fdt_setprop_cells(fdt, name, "reg", mmio, size);
>> +    qemu_fdt_setprop_string(fdt, name, "compatible", "fsl,esdhc");
>> +}
>>     typedef struct PlatformDevtreeData {
>>       void *fdt;
>> @@ -553,6 +573,8 @@ static int ppce500_load_device_tree(PPCE500MachineState 
>> *pms,
>>         dt_rtc_create(fdt, "i2c", "rtc");
>>   +    /* sdhc */
>> +    dt_sdhc_create(fdt, soc, mpic);
>>