Re: [PATCH] hw/sd/allwinner-sdhost: Use AddressSpace for DMA transfers

[Niek Linnenbank <nieklinnenbank@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6993 bytes --]

Hi Philippe,

Nice improvement, I didnt know about this API. Makes sense to use it indeed.
The patch works fine. I tested your patches by applying the previous two
sets first, and then this one.
It ran well with the avocado tests and also with the official image
OrangePi_pc_debian_stretch_server_linux5.3.5_v1.0.img.

Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Regards,
Niek



On Fri, Aug 14, 2020 at 1:01 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Allow the device to execute the DMA transfers in a different
> AddressSpace.
>
> The A10 and H3 SoC keep using the system_memory address space,
> but via the proper dma_memory_access() API.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Based-on: <20200814092346.21825-1-f4bug@amsat.org>
> "hw/sd: Use sdbus_read_data/sdbus_write_data for multiple bytes access"
>
> Tested with:
>   AVOCADO_ALLOW_LARGE_STORAGE=1 avocado run -t machine:orangepi-pc -t
> machine:cubieboard tests/acceptance/
> ---
>  include/hw/sd/allwinner-sdhost.h |  6 ++++++
>  hw/arm/allwinner-a10.c           |  2 ++
>  hw/arm/allwinner-h3.c            |  2 ++
>  hw/sd/allwinner-sdhost.c         | 37 ++++++++++++++++++++++++++------
>  4 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/sd/allwinner-sdhost.h
> b/include/hw/sd/allwinner-sdhost.h
> index d94606a853..839732ebf3 100644
> --- a/include/hw/sd/allwinner-sdhost.h
> +++ b/include/hw/sd/allwinner-sdhost.h
> @@ -71,6 +71,12 @@ typedef struct AwSdHostState {
>      /** Interrupt output signal to notify CPU */
>      qemu_irq irq;
>
> +    /** Memory region where DMA transfers are done */
> +    MemoryRegion *dma_mr;
> +
> +    /** Address space used internally for DMA transfers */
> +    AddressSpace dma_as;
> +
>      /** Number of bytes left in current DMA transfer */
>      uint32_t transfer_cnt;
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index e258463747..d404f31e02 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -155,6 +155,8 @@ static void aw_a10_realize(DeviceState *dev, Error
> **errp)
>      }
>
>      /* SD/MMC */
> +    object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
> +                             OBJECT(get_system_memory()), &error_fatal);
>      sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, AW_A10_MMC0_BASE);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0, qdev_get_gpio_in(dev,
> 32));
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index ff92ded82c..43a8d3dc48 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -349,6 +349,8 @@ static void allwinner_h3_realize(DeviceState *dev,
> Error **errp)
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, s->memmap[AW_H3_SID]);
>
>      /* SD/MMC */
> +    object_property_set_link(OBJECT(&s->mmc0), "dma-memory",
> +                             OBJECT(get_system_memory()), &error_fatal);
>      sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, s->memmap[AW_H3_MMC0]);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0,
> diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c
> index f9eb92c09e..e82afb75eb 100644
> --- a/hw/sd/allwinner-sdhost.c
> +++ b/hw/sd/allwinner-sdhost.c
> @@ -21,7 +21,10 @@
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "qemu/units.h"
> +#include "qapi/error.h"
>  #include "sysemu/blockdev.h"
> +#include "sysemu/dma.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/irq.h"
>  #include "hw/sd/allwinner-sdhost.h"
>  #include "migration/vmstate.h"
> @@ -306,7 +309,7 @@ static uint32_t
> allwinner_sdhost_process_desc(AwSdHostState *s,
>      uint8_t buf[1024];
>
>      /* Read descriptor */
> -    cpu_physical_memory_read(desc_addr, desc, sizeof(*desc));
> +    dma_memory_read(&s->dma_as, desc_addr, desc, sizeof(*desc));
>      if (desc->size == 0) {
>          desc->size = klass->max_desc_size;
>      } else if (desc->size > klass->max_desc_size) {
> @@ -331,22 +334,24 @@ static uint32_t
> allwinner_sdhost_process_desc(AwSdHostState *s,
>
>          /* Write to SD bus */
>          if (is_write) {
> -            cpu_physical_memory_read((desc->addr & DESC_SIZE_MASK) +
> num_done,
> -                                      buf, buf_bytes);
> +            dma_memory_read(&s->dma_as,
> +                            (desc->addr & DESC_SIZE_MASK) + num_done,
> +                            buf, buf_bytes);
>              sdbus_write_data(&s->sdbus, buf, buf_bytes);
>
>          /* Read from SD bus */
>          } else {
>              sdbus_read_data(&s->sdbus, buf, buf_bytes);
> -            cpu_physical_memory_write((desc->addr & DESC_SIZE_MASK) +
> num_done,
> -                                       buf, buf_bytes);
> +            dma_memory_write(&s->dma_as,
> +                             (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));
> +    dma_memory_write(&s->dma_as, desc_addr, desc, sizeof(*desc));
>
>      return num_done;
>  }
> @@ -721,6 +726,12 @@ static const VMStateDescription
> vmstate_allwinner_sdhost = {
>      }
>  };
>
> +static Property allwinner_sdhost_properties[] = {
> +    DEFINE_PROP_LINK("dma-memory", AwSdHostState, dma_mr,
> +                     TYPE_MEMORY_REGION, MemoryRegion *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void allwinner_sdhost_init(Object *obj)
>  {
>      AwSdHostState *s = AW_SDHOST(obj);
> @@ -734,6 +745,18 @@ static void allwinner_sdhost_init(Object *obj)
>      sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
>  }
>
> +static void allwinner_sdhost_realize(DeviceState *dev, Error **errp)
> +{
> +    AwSdHostState *s = AW_SDHOST(dev);
> +
> +    if (!s->dma_mr) {
> +        error_setg(errp, TYPE_AW_SDHOST " 'dma-memory' link not set");
> +        return;
> +    }
> +
> +    address_space_init(&s->dma_as, s->dma_mr, "sdhost-dma");
> +}
> +
>  static void allwinner_sdhost_reset(DeviceState *dev)
>  {
>      AwSdHostState *s = AW_SDHOST(dev);
> @@ -792,6 +815,8 @@ static void allwinner_sdhost_class_init(ObjectClass
> *klass, void *data)
>
>      dc->reset = allwinner_sdhost_reset;
>      dc->vmsd = &vmstate_allwinner_sdhost;
> +    dc->realize = allwinner_sdhost_realize;
> +    device_class_set_props(dc, allwinner_sdhost_properties);
>  }
>
>  static void allwinner_sdhost_sun4i_class_init(ObjectClass *klass, void
> *data)
> --
> 2.21.3
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 8816 bytes --]