Re: [PATCH v5 6/6] hw/ppc/e500: Add Freescale eSDHC to e500plat

[Bernhard Beschow]

On Mon, Oct 31, 2022 at 4:19 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

On 31/10/22 16:12, Philippe Mathieu-Daudé wrote:
> On 31/10/22 12:54, Philippe Mathieu-Daudé wrote:
>> From: Bernhard Beschow <shentey@gmail.com>
>>
>> Adds missing functionality to e500plat machine which increases the
>> chance of given "real" firmware images to access SD cards.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Message-Id: <20221018210146.193159-8-shentey@gmail.com>
>> [PMD: Simplify using create_unimplemented_device("esdhc")]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   docs/system/ppc/ppce500.rst | 12 ++++++++++
>>   hw/ppc/Kconfig              |  2 ++
>>   hw/ppc/e500.c               | 48 ++++++++++++++++++++++++++++++++++++-
>>   hw/ppc/e500.h               |  1 +
>>   hw/ppc/e500plat.c           |  1 +
>>   5 files changed, 63 insertions(+), 1 deletion(-)
>
>> @@ -992,6 +1018,26 @@ void ppce500_init(MachineState *machine)
>>       i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
>>       i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
>> +    /* eSDHC */
>> +    if (pmc->has_esdhc) {
>> +        uint64_t sdhci_regsize;
>> +
>> +        dev = qdev_new(TYPE_SYSBUS_SDHCI);
>> +        /*
>> +         * Compatible with:
>> +         * - SD Host Controller Specification Version 2.0 Part A2
>> +         */
>> +        qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>> +        s = SYS_BUS_DEVICE(dev);
>> +        sysbus_realize_and_unref(s, &error_fatal);
>> +        sysbus_mmio_map(s, 0, pmc->ccsrbar_base +
>> MPC85XX_ESDHC_REGS_OFFSET);
>> +        sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev,
>> MPC85XX_ESDHC_IRQ));
>> +        sdhci_regsize = memory_region_size(sysbus_mmio_get_region(s,
>> 0));
>> +        create_unimplemented_device("esdhc",
>> +                                    pmc->ccsrbar_base
>> +                                    + MPC85XX_ESDHC_REGS_OFFSET +
>> sdhci_regsize,
>> +                                    MPC85XX_ESDHC_REGS_SIZE -
>> sdhci_regsize);
>> +    }
>
> Since the UNIMP device has lower priority, we can simplify as:
>
> if (pmc->has_esdhc) {
>      create_unimplemented_device("esdhc",
>                                  pmc->ccsrbar_base
>                                  + MPC85XX_ESDHC_REGS_OFFSET,
>                                  MPC85XX_ESDHC_REGS_SIZE);
>
>      dev = qdev_new(TYPE_SYSBUS_SDHCI);
>      /*
>       * Compatible with:
>       * - SD Host Controller Specification Version 2.0 Part A2
>       */
>      qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>      s = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(s, &error_fatal);
>      sysbus_mmio_map(s, 0, pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET);

  memory_region_add_subregion(ccsr_addr_space, MPC85XX_ESDHC_REGS_OFFSET,
                              sysbus_mmio_get_region(s, 0));

seems to be equivalent, works as well and mimics other devices, e.g. i2c. So perhaps use that?

So the SDHCI is mapped inside the CCSR block. Better would be to map it
into ccsr_addr_space.

Doesn't the above code map it into ccsr_addr_space?

I presume the CCSR is the device responsible of endian swapping, but TBH
I have no clue about this board.