Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe Mathieu-Daudé"
<phi...@linaro.org>:
>Hi Bernhard,
>
>On 18/10/22 23:01, Bernhard Beschow wrote:
>> Will allow e500 boards to access SD cards using just their own devices.
>>
>> Signed-off-by: Bernhard Beschow <shen...@gmail.com>
>> ---
>> hw/sd/sdhci.c | 120 +++++++++++++++++++++++++++++++++++++++++-
>> include/hw/sd/sdhci.h | 3 ++
>> 2 files changed, 122 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 306070c872..8d8ad9ff24 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1369,6 +1369,7 @@ void sdhci_initfn(SDHCIState *s)
>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> sdhci_data_transfer, s);
>> s->io_ops = &sdhci_mmio_ops;
>> + s->io_registers_map_size = SDHC_REGISTERS_MAP_SIZE;
>> }
>> void sdhci_uninitfn(SDHCIState *s)
>> @@ -1392,7 +1393,7 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
>> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>> memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci",
>> - SDHC_REGISTERS_MAP_SIZE);
>> + s->io_registers_map_size);
>
>I don't think we want to change this region size. [see below]
>
>> void sdhci_common_unrealize(SDHCIState *s)
>> @@ -1575,6 +1576,122 @@ static const TypeInfo sdhci_bus_info = {
>> .class_init = sdhci_bus_class_init,
>> };
>> +/* --- qdev Freescale eSDHC --- */
>> +
>> +/* Watermark Level Register */
>> +#define ESDHC_WML 0x44
>> +
>> +/* Control Register for DMA transfer */
>> +#define ESDHC_DMA_SYSCTL 0x40c
>> +
>> +#define ESDHC_REGISTERS_MAP_SIZE 0x410
>
>My preferred approach would be to create a container region with a
>size of ESDHC_REGISTERS_MAP_SIZE. Map the SDHC_REGISTERS_MAP region
>in the container at offset 0, priority -1. Add 2 register regions
>for ESDHC_WML and ESDHC_DMA_SYSCTL, and map them with priority 1 in
>the container. ...
>
>> +static uint64_t esdhci_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> + uint64_t ret;
>> +
>> + switch (offset) {
>> + case SDHC_SYSAD:
>> + case SDHC_BLKSIZE:
>> + case SDHC_ARGUMENT:
>> + case SDHC_TRNMOD:
>> + case SDHC_RSPREG0:
>> + case SDHC_RSPREG1:
>> + case SDHC_RSPREG2:
>> + case SDHC_RSPREG3:
>> + case SDHC_BDATA:
>> + case SDHC_PRNSTS:
>> + case SDHC_HOSTCTL:
>> + case SDHC_CLKCON:
>> + case SDHC_NORINTSTS:
>> + case SDHC_NORINTSTSEN:
>> + case SDHC_NORINTSIGEN:
>> + case SDHC_ACMD12ERRSTS:
>> + case SDHC_CAPAB:
>> + case SDHC_SLOT_INT_STATUS:
>> + ret = sdhci_read(opaque, offset, size);
>> + break;
>
>... Then you don't need these cases.
>
>> + case ESDHC_WML:
>> + case ESDHC_DMA_SYSCTL:
>> + ret = 0;
>> + qemu_log_mask(LOG_UNIMP, "ESDHC rd @0x%02" HWADDR_PRIx
>> + " not implemented\n", offset);
>
>But then I realize you only treat these 2 registers as UNIMP.
>
>So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>another UNIMP region of ESDHC_REGISTERS_MAP_SIZE - SDHC_REGISTERS_MAP_SIZE (=
>0x310) and map it normally at offset
>0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>hw/arm/bcm2835_peripherals.c.
>
>But the ESDHC_WML register has address 0x44 and fits inside the
>SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>upper part of the SDHC_CAPAB register. These bits are undefined
>on the spec v2, which I see you are setting in esdhci_init().
>So this register should already return 0, otherwise we have
>a bug. Thus we don't need to handle this ESDHC_WML particularly.
>
>And your model is reduced to handling create_unimp() in esdhci_realize().
>
>Am I missing something?
The mmio ops are big endian and need to be aligned to a 4-byte boundary. It
took me quite a while to debug this. So shall I just create an additional
memory region for the region above SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
Best regards,
Bernhard
>
>Regards,
>
>Phil.
