Re: [PATCH v8 08/18] hw/arm/allwinner: add SD/MMC host controller

[Philippe Mathieu-Daudé]

Hi Niek,

On 3/11/20 11:18 PM, Niek Linnenbank wrote:
> The Allwinner System on Chip families sun4i and above contain
> an integrated storage controller for Secure Digital (SD) and
> Multi Media Card (MMC) interfaces. This commit adds support
> for the Allwinner SD/MMC storage controller with the following
> emulated features:
> 
>  * DMA transfers
>  * Direct FIFO I/O
>  * Short/Long format command responses
>  * Auto-Stop command (CMD12)
>  * Insert & remove card detection
> 
> The following boards are extended with the SD host controller:
> 
>  * Cubieboard (hw/arm/cubieboard.c)
>  * Orange Pi PC (hw/arm/orangepi.c)
> 
> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/arm/allwinner-a10.h   |   2 +
>  include/hw/arm/allwinner-h3.h    |   3 +
>  include/hw/sd/allwinner-sdhost.h | 135 +++++
>  hw/arm/allwinner-a10.c           |  11 +
>  hw/arm/allwinner-h3.c            |  15 +-
>  hw/arm/cubieboard.c              |  15 +
>  hw/arm/orangepi.c                |  16 +
>  hw/sd/allwinner-sdhost.c         | 854 +++++++++++++++++++++++++++++++
>  hw/arm/Kconfig                   |   1 +
>  hw/sd/Makefile.objs              |   1 +
>  hw/sd/trace-events               |   7 +
>  11 files changed, 1059 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/sd/allwinner-sdhost.h
>  create mode 100644 hw/sd/allwinner-sdhost.c
...

> +static uint32_t allwinner_sdhost_process_desc(AwSdHostState *s,
> +                                              hwaddr desc_addr,
> +                                              TransferDescriptor *desc,
> +                                              bool is_write, uint32_t 
> max_bytes)
> +{
> +    AwSdHostClass *klass = AW_SDHOST_GET_CLASS(s);
> +    uint32_t num_done = 0;
> +    uint32_t num_bytes = max_bytes;
> +    uint8_t buf[1024];

Is 1024 a constant specific for this device?

> +
> +    /* Read descriptor */
> +    cpu_physical_memory_read(desc_addr, desc, sizeof(*desc));

I missed that while reviewing, but IIUC from [*] below, this code
is emulating the DMA context right? So we should be using the DMA
accessors here, dma_memory_read() and dma_memory_write().

> +    if (desc->size == 0) {
> +        desc->size = klass->max_desc_size;
> +    } else if (desc->size > klass->max_desc_size) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA descriptor buffer size "
> +                      " is out-of-bounds: %" PRIu32 " > %zu",
> +                      __func__, desc->size, klass->max_desc_size);
> +        desc->size = klass->max_desc_size;
> +    }
> +    if (desc->size < num_bytes) {
> +        num_bytes = desc->size;
> +    }
> +
> +    trace_allwinner_sdhost_process_desc(desc_addr, desc->size,
> +                                        is_write, max_bytes);
> +
> +    while (num_done < num_bytes) {
> +        /* Try to completely fill the local buffer */
> +        uint32_t buf_bytes = num_bytes - num_done;
> +        if (buf_bytes > sizeof(buf)) {
> +            buf_bytes = sizeof(buf);
> +        }
> +
> +        /* Write to SD bus */
> +        if (is_write) {
> +            cpu_physical_memory_read((desc->addr & DESC_SIZE_MASK) + 
> num_done,
> +                                      buf, buf_bytes);
> +
> +            for (uint32_t i = 0; i < buf_bytes; i++) {
> +                sdbus_write_data(&s->sdbus, buf[i]);
> +            }
> +
> +        /* Read from SD bus */
> +        } else {
> +            for (uint32_t i = 0; i < buf_bytes; i++) {
> +                buf[i] = sdbus_read_data(&s->sdbus);
> +            }
> +            cpu_physical_memory_write((desc->addr & DESC_SIZE_MASK) + 
> num_done,
> +                                       buf, buf_bytes);
> +        }
> +        num_done += buf_bytes;
> +    }
> +
> +    /* Clear hold flag and flush descriptor */
> +    desc->status &= ~DESC_STATUS_HOLD;
> +    cpu_physical_memory_write(desc_addr, desc, sizeof(*desc));
> +
> +    return num_done;
> +}
> +
> +static void allwinner_sdhost_dma(AwSdHostState *s)
> +{
> +    TransferDescriptor desc;
> +    hwaddr desc_addr = s->desc_base;
> +    bool is_write = (s->command & SD_CMDR_WRITE);
> +    uint32_t bytes_done = 0;
> +
> +    /* Check if DMA can be performed */
> +    if (s->byte_count == 0 || s->block_size == 0 ||
> +      !(s->global_ctl & SD_GCTL_DMA_ENB)) {
> +        return;
> +    }
> +
> +    /*
> +     * For read operations, data must be available on the SD bus
> +     * If not, it is an error and we should not act at all
> +     */
> +    if (!is_write && !sdbus_data_ready(&s->sdbus)) {
> +        return;
> +    }
> +
> +    /* Process the DMA descriptors until all data is copied */
> +    while (s->byte_count > 0) {
> +        bytes_done = allwinner_sdhost_process_desc(s, desc_addr, &desc,
> +                                                   is_write, s->byte_count);

[*]

> +        allwinner_sdhost_update_transfer_cnt(s, bytes_done);
> +
> +        if (bytes_done <= s->byte_count) {
> +            s->byte_count -= bytes_done;
> +        } else {
> +            s->byte_count = 0;
> +        }
> +
> +        if (desc.status & DESC_STATUS_LAST) {
> +            break;
> +        } else {
> +            desc_addr = desc.next;
> +        }
> +    }
> +
> +    /* Raise IRQ to signal DMA is completed */
> +    s->irq_status |= SD_RISR_DATA_COMPLETE | SD_RISR_SDIO_INTR;
> +
> +    /* Update DMAC bits */
> +    s->dmac_status |= SD_IDST_INT_SUMMARY;
> +
> +    if (is_write) {
> +        s->dmac_status |= SD_IDST_TRANSMIT_IRQ;
> +    } else {
> +        s->dmac_status |= SD_IDST_RECEIVE_IRQ;
> +    }
> +}
...