QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Niek Linnenbank <nieklinnenbank@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Beniamino Galvani <b.galvani@gmail.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller
Date: Mon, 16 Dec 2019 00:07:01 +0100
Message-ID: <CAPan3WpiWahU85-ZgV4BMPU8mUTxsq2TPn-szOHkHc5+PHUqpw@mail.gmail.com> (raw)
In-Reply-To: <6bee15d7-7d80-0709-ac90-ef2052b39329@redhat.com>


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

On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Hi Niek,
>
> On 12/11/19 11:34 PM, Niek Linnenbank wrote:
> > Ping!
> >
> > Anyone would like to comment on this driver?
> >
> > I finished the rework on all previous comments in this series.
> >
> > Currently debugging the hflags error reported by Philippe.
> > After that, I'm ready to send out v2 of these patches.
> >
> > Regards,
> > Niek
> >
> > On Mon, Dec 2, 2019 at 10:10 PM Niek Linnenbank
> > <nieklinnenbank@gmail.com <mailto:nieklinnenbank@gmail.com>> wrote:
> >
> >     The Allwinner H3 System on Chip contains an integrated storage
> >     controller for Secure Digital (SD) and Multi Media Card (MMC)
> >     interfaces. This commit adds support for the Allwinner H3
> >     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
> >
> >     Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com
> >     <mailto:nieklinnenbank@gmail.com>>
> >     ---
> >       hw/arm/allwinner-h3.c               |  20 +
> >       hw/arm/orangepi.c                   |  17 +
> >       hw/sd/Makefile.objs                 |   1 +
> >       hw/sd/allwinner-h3-sdhost.c         | 791
> ++++++++++++++++++++++++++++
> >       hw/sd/trace-events                  |   7 +
> >       include/hw/arm/allwinner-h3.h       |   2 +
> >       include/hw/sd/allwinner-h3-sdhost.h |  73 +++
> >       7 files changed, 911 insertions(+)
> >       create mode 100644 hw/sd/allwinner-h3-sdhost.c
> >       create mode 100644 include/hw/sd/allwinner-h3-sdhost.h
> >
> >     diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> >     index 4fc4c8c725..c2972caf88 100644
> >     --- a/hw/arm/allwinner-h3.c
> >     +++ b/hw/arm/allwinner-h3.c
> >     @@ -50,6 +50,9 @@ static void aw_h3_init(Object *obj)
> >
> >           sysbus_init_child_obj(obj, "sid", &s->sid, sizeof(s->sid),
> >                                 TYPE_AW_H3_SID);
> >     +
> >     +    sysbus_init_child_obj(obj, "mmc0", &s->mmc0, sizeof(s->mmc0),
> >     +                          TYPE_AW_H3_SDHOST);
> >       }
> >
> >       static void aw_h3_realize(DeviceState *dev, Error **errp)
> >     @@ -217,6 +220,23 @@ static void aw_h3_realize(DeviceState *dev,
> >     Error **errp)
> >           }
> >           sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, AW_H3_SID_BASE);
> >
> >     +    /* SD/MMC */
> >     +    object_property_set_bool(OBJECT(&s->mmc0), true, "realized",
> &err);
> >     +    if (err != NULL) {
> >     +        error_propagate(errp, err);
> >     +        return;
> >     +    }
> >     +    sysbusdev = SYS_BUS_DEVICE(&s->mmc0);
> >     +    sysbus_mmio_map(sysbusdev, 0, AW_H3_MMC0_BASE);
> >     +    sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_MMC0]);
> >     +
> >     +    object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(&s->mmc0),
> >     +                              "sd-bus", &err);
> >     +    if (err) {
> >     +        error_propagate(errp, err);
> >     +        return;
> >     +    }
> >     +
> >           /* Universal Serial Bus */
> >           sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
> >                                s->irq[AW_H3_GIC_SPI_EHCI0]);
> >     diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> >     index 5ef2735f81..dee3efaf08 100644
> >     --- a/hw/arm/orangepi.c
> >     +++ b/hw/arm/orangepi.c
> >     @@ -39,6 +39,10 @@ typedef struct OrangePiState {
> >       static void orangepi_init(MachineState *machine)
> >       {
> >           OrangePiState *s = g_new(OrangePiState, 1);
> >     +    DriveInfo *di;
> >     +    BlockBackend *blk;
> >     +    BusState *bus;
> >     +    DeviceState *carddev;
> >           Error *err = NULL;
> >
> >           s->h3 = AW_H3(object_new(TYPE_AW_H3));
> >     @@ -64,6 +68,18 @@ static void orangepi_init(MachineState *machine)
> >               exit(1);
> >           }
> >
> >     +    /* Create and plug in the SD card */
> >     +    di = drive_get_next(IF_SD);
> >     +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> >     +    bus = qdev_get_child_bus(DEVICE(s->h3), "sd-bus");
> >     +    if (bus == NULL) {
> >     +        error_report("No SD/MMC found in H3 object");
> >     +        exit(1);
> >     +    }
>
> Your device always creates a bus, so I don't think the if(bus) check is
> worthwhile. Eventually use an assert(bus)?
>
> >     +    carddev = qdev_create(bus, TYPE_SD_CARD);
> >     +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> >     +    object_property_set_bool(OBJECT(carddev), true, "realized",
> >     &error_fatal);
> >     +
> >           /* RAM */
> >           memory_region_allocate_system_memory(&s->sdram, NULL,
> >     "orangepi.ram",
> >                                                machine->ram_size);
> >     @@ -80,6 +96,7 @@ static void orangepi_machine_init(MachineClass *mc)
> >       {
> >           mc->desc = "Orange Pi PC";
> >           mc->init = orangepi_init;
> >     +    mc->block_default_type = IF_SD;
> >           mc->units_per_default_bus = 1;
> >           mc->min_cpus = AW_H3_NUM_CPUS;
> >           mc->max_cpus = AW_H3_NUM_CPUS;
> >     diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> >     index a884c238df..e7cc5ab739 100644
> >     --- a/hw/sd/Makefile.objs
> >     +++ b/hw/sd/Makefile.objs
> >     @@ -4,6 +4,7 @@ common-obj-$(CONFIG_SD) += sd.o core.o
> sdmmc-internal.o
> >       common-obj-$(CONFIG_SDHCI) += sdhci.o
> >       common-obj-$(CONFIG_SDHCI_PCI) += sdhci-pci.o
> >
> >     +obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-sdhost.o
> >       obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
> >       obj-$(CONFIG_OMAP) += omap_mmc.o
> >       obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
> >     diff --git a/hw/sd/allwinner-h3-sdhost.c
> b/hw/sd/allwinner-h3-sdhost.c
> >     new file mode 100644
> >     index 0000000000..26e113a144
> >     --- /dev/null
> >     +++ b/hw/sd/allwinner-h3-sdhost.c
> >     @@ -0,0 +1,791 @@
> >     +/*
> >     + * Allwinner H3 SD Host Controller emulation
> >     + *
> >     + * Copyright (C) 2019 Niek Linnenbank <nieklinnenbank@gmail.com
> >     <mailto:nieklinnenbank@gmail.com>>
> >     + *
> >     + * This program is free software: you can redistribute it and/or
> modify
> >     + * it under the terms of the GNU General Public License as
> published by
> >     + * the Free Software Foundation, either version 2 of the License, or
> >     + * (at your option) any later version.
> >     + *
> >     + * This program is distributed in the hope that it will be useful,
> >     + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >     + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >     + * GNU General Public License for more details.
> >     + *
> >     + * You should have received a copy of the GNU General Public License
> >     + * along with this program.  If not, see
> >     <http://www.gnu.org/licenses/>.
> >     + */
> >     +
> >     +#include "qemu/osdep.h"
> >     +#include "qemu/log.h"
> >     +#include "qemu/module.h"
> >     +#include "sysemu/blockdev.h"
> >     +#include "hw/irq.h"
> >     +#include "hw/sd/allwinner-h3-sdhost.h"
> >     +#include "migration/vmstate.h"
> >     +#include "trace.h"
> >     +
> >     +#define TYPE_AW_H3_SDHOST_BUS "allwinner-h3-sdhost-bus"
> >     +#define AW_H3_SDHOST_BUS(obj) \
> >     +    OBJECT_CHECK(SDBus, (obj), TYPE_AW_H3_SDHOST_BUS)
> >     +
> >     +/* SD Host register offsets */
> >     +#define REG_SD_GCTL        (0x00)  /* Global Control */
> >     +#define REG_SD_CKCR        (0x04)  /* Clock Control */
> >     +#define REG_SD_TMOR        (0x08)  /* Timeout */
> >     +#define REG_SD_BWDR        (0x0C)  /* Bus Width */
> >     +#define REG_SD_BKSR        (0x10)  /* Block Size */
> >     +#define REG_SD_BYCR        (0x14)  /* Byte Count */
> >     +#define REG_SD_CMDR        (0x18)  /* Command */
> >     +#define REG_SD_CAGR        (0x1C)  /* Command Argument */
> >     +#define REG_SD_RESP0       (0x20)  /* Response Zero */
> >     +#define REG_SD_RESP1       (0x24)  /* Response One */
> >     +#define REG_SD_RESP2       (0x28)  /* Response Two */
> >     +#define REG_SD_RESP3       (0x2C)  /* Response Three */
> >     +#define REG_SD_IMKR        (0x30)  /* Interrupt Mask */
> >     +#define REG_SD_MISR        (0x34)  /* Masked Interrupt Status */
> >     +#define REG_SD_RISR        (0x38)  /* Raw Interrupt Status */
> >     +#define REG_SD_STAR        (0x3C)  /* Status */
> >     +#define REG_SD_FWLR        (0x40)  /* FIFO Water Level */
> >     +#define REG_SD_FUNS        (0x44)  /* FIFO Function Select */
> >     +#define REG_SD_DBGC        (0x50)  /* Debug Enable */
> >     +#define REG_SD_A12A        (0x58)  /* Auto command 12 argument */
> >     +#define REG_SD_NTSR        (0x5C)  /* SD NewTiming Set */
> >     +#define REG_SD_SDBG        (0x60)  /* SD newTiming Set Debug */
> >     +#define REG_SD_HWRST       (0x78)  /* Hardware Reset Register */
> >     +#define REG_SD_DMAC        (0x80)  /* Internal DMA Controller
> >     Control */
> >     +#define REG_SD_DLBA        (0x84)  /* Descriptor List Base Address
> */
> >     +#define REG_SD_IDST        (0x88)  /* Internal DMA Controller
> Status */
> >     +#define REG_SD_IDIE        (0x8C)  /* Internal DMA Controller IRQ
> >     Enable */
> >     +#define REG_SD_THLDC       (0x100) /* Card Threshold Control */
> >     +#define REG_SD_DSBD        (0x10C) /* eMMC DDR Start Bit Detection
> >     Control */
> >     +#define REG_SD_RES_CRC     (0x110) /* Response CRC from card/eMMC */
> >     +#define REG_SD_DATA7_CRC   (0x114) /* CRC Data 7 from card/eMMC */
> >     +#define REG_SD_DATA6_CRC   (0x118) /* CRC Data 6 from card/eMMC */
> >     +#define REG_SD_DATA5_CRC   (0x11C) /* CRC Data 5 from card/eMMC */
> >     +#define REG_SD_DATA4_CRC   (0x120) /* CRC Data 4 from card/eMMC */
> >     +#define REG_SD_DATA3_CRC   (0x124) /* CRC Data 3 from card/eMMC */
> >     +#define REG_SD_DATA2_CRC   (0x128) /* CRC Data 2 from card/eMMC */
> >     +#define REG_SD_DATA1_CRC   (0x12C) /* CRC Data 1 from card/eMMC */
> >     +#define REG_SD_DATA0_CRC   (0x130) /* CRC Data 0 from card/eMMC */
> >     +#define REG_SD_CRC_STA     (0x134) /* CRC status from card/eMMC
> >     during write */
> >     +#define REG_SD_FIFO        (0x200) /* Read/Write FIFO */
> >     +
> >     +/* SD Host register flags */
> >     +#define SD_GCTL_FIFO_AC_MOD     (1 << 31)
> >     +#define SD_GCTL_DDR_MOD_SEL     (1 << 10)
> >     +#define SD_GCTL_CD_DBC_ENB      (1 << 8)
> >     +#define SD_GCTL_DMA_ENB         (1 << 5)
> >     +#define SD_GCTL_INT_ENB         (1 << 4)
> >     +#define SD_GCTL_DMA_RST         (1 << 2)
> >     +#define SD_GCTL_FIFO_RST        (1 << 1)
> >     +#define SD_GCTL_SOFT_RST        (1 << 0)
> >     +
> >     +#define SD_CMDR_LOAD            (1 << 31)
> >     +#define SD_CMDR_CLKCHANGE       (1 << 21)
> >     +#define SD_CMDR_WRITE           (1 << 10)
> >     +#define SD_CMDR_AUTOSTOP        (1 << 12)
> >     +#define SD_CMDR_DATA            (1 << 9)
> >     +#define SD_CMDR_RESPONSE_LONG   (1 << 7)
> >     +#define SD_CMDR_RESPONSE        (1 << 6)
> >     +#define SD_CMDR_CMDID_MASK      (0x3f)
> >     +
> >     +#define SD_RISR_CARD_REMOVE     (1 << 31)
> >     +#define SD_RISR_CARD_INSERT     (1 << 30)
> >     +#define SD_RISR_AUTOCMD_DONE    (1 << 14)
> >     +#define SD_RISR_DATA_COMPLETE   (1 << 3)
> >     +#define SD_RISR_CMD_COMPLETE    (1 << 2)
> >     +#define SD_RISR_NO_RESPONSE     (1 << 1)
> >     +
> >     +#define SD_STAR_CARD_PRESENT    (1 << 8)
> >     +
> >     +#define SD_IDST_SUM_RECEIVE_IRQ (1 << 8)
> >     +#define SD_IDST_RECEIVE_IRQ     (1 << 1)
> >     +#define SD_IDST_TRANSMIT_IRQ    (1 << 0)
> >     +#define SD_IDST_IRQ_MASK        (SD_IDST_RECEIVE_IRQ |
> >     SD_IDST_TRANSMIT_IRQ | \
> >     +                                 SD_IDST_SUM_RECEIVE_IRQ)
> >     +#define SD_IDST_WR_MASK         (0x3ff)
> >     +
> >     +/* SD Host register reset values */
> >     +#define REG_SD_GCTL_RST         (0x00000300)
> >     +#define REG_SD_CKCR_RST         (0x0)
> >     +#define REG_SD_TMOR_RST         (0xFFFFFF40)
> >     +#define REG_SD_BWDR_RST         (0x0)
> >     +#define REG_SD_BKSR_RST         (0x00000200)
> >     +#define REG_SD_BYCR_RST         (0x00000200)
> >     +#define REG_SD_CMDR_RST         (0x0)
> >     +#define REG_SD_CAGR_RST         (0x0)
> >     +#define REG_SD_RESP_RST         (0x0)
> >     +#define REG_SD_IMKR_RST         (0x0)
> >     +#define REG_SD_MISR_RST         (0x0)
> >     +#define REG_SD_RISR_RST         (0x0)
> >     +#define REG_SD_STAR_RST         (0x00000100)
> >     +#define REG_SD_FWLR_RST         (0x000F0000)
> >     +#define REG_SD_FUNS_RST         (0x0)
> >     +#define REG_SD_DBGC_RST         (0x0)
> >     +#define REG_SD_A12A_RST         (0x0000FFFF)
> >     +#define REG_SD_NTSR_RST         (0x00000001)
> >     +#define REG_SD_SDBG_RST         (0x0)
> >     +#define REG_SD_HWRST_RST        (0x00000001)
> >     +#define REG_SD_DMAC_RST         (0x0)
> >     +#define REG_SD_DLBA_RST         (0x0)
> >     +#define REG_SD_IDST_RST         (0x0)
> >     +#define REG_SD_IDIE_RST         (0x0)
> >     +#define REG_SD_THLDC_RST        (0x0)
> >     +#define REG_SD_DSBD_RST         (0x0)
> >     +#define REG_SD_RES_CRC_RST      (0x0)
> >     +#define REG_SD_DATA_CRC_RST     (0x0)
> >     +#define REG_SD_CRC_STA_RST      (0x0)
> >     +#define REG_SD_FIFO_RST         (0x0)
> >     +
> >     +/* Data transfer descriptor for DMA */
> >     +typedef struct TransferDescriptor {
> >     +    uint32_t status; /* Status flags */
> >     +    uint32_t size;   /* Data buffer size */
> >     +    uint32_t addr;   /* Data buffer address */
> >     +    uint32_t next;   /* Physical address of next descriptor */
> >     +} TransferDescriptor;
> >     +
> >     +/* Data transfer descriptor flags */
> >     +#define DESC_STATUS_HOLD   (1 << 31) /* Set when descriptor is in
> >     use by DMA */
> >     +#define DESC_STATUS_ERROR  (1 << 30) /* Set when DMA transfer error
> >     occurred */
> >     +#define DESC_STATUS_CHAIN  (1 << 4)  /* Indicates chained
> >     descriptor. */
> >     +#define DESC_STATUS_FIRST  (1 << 3)  /* Set on the first descriptor
> */
> >     +#define DESC_STATUS_LAST   (1 << 2)  /* Set on the last descriptor
> */
> >     +#define DESC_STATUS_NOIRQ  (1 << 1)  /* Skip raising interrupt
> >     after transfer */
> >     +
> >     +#define DESC_SIZE_MASK     (0xfffffffc)
> >     +
> >     +static void aw_h3_sdhost_update_irq(AwH3SDHostState *s)
> >     +{
> >     +    uint32_t irq_en = s->global_ctl & SD_GCTL_INT_ENB;
> >     +    uint32_t irq = irq_en ? s->irq_status & s->irq_mask : 0;
>
> The previous line is confuse, either use parenthesis or a if statement.
>
>      uint32_t irq = irq_en ? (s->irq_status & s->irq_mask) : 0;
>
> >     +
> >     +    trace_aw_h3_sdhost_update_irq(irq);
> >     +    qemu_set_irq(s->irq, irq);
> >     +}
> >     +
> >     +static void aw_h3_sdhost_update_transfer_cnt(AwH3SDHostState *s,
> >     uint32_t bytes)
> >     +{
> >     +    if (s->transfer_cnt > bytes) {
> >     +        s->transfer_cnt -= bytes;
> >     +    } else {
> >     +        s->transfer_cnt = 0;
> >     +    }
> >     +
> >     +    if (!s->transfer_cnt) {
> >     +        s->irq_status |= SD_RISR_DATA_COMPLETE |
> SD_RISR_AUTOCMD_DONE;
> >     +    }
> >     +}
> >     +
> >     +static void aw_h3_sdhost_set_inserted(DeviceState *dev, bool
> inserted)
> >     +{
> >     +    AwH3SDHostState *s = AW_H3_SDHOST(dev);
> >     +
> >     +    trace_aw_h3_sdhost_set_inserted(inserted);
> >     +
> >     +    if (inserted) {
> >     +        s->irq_status |= SD_RISR_CARD_INSERT;
> >     +        s->irq_status &= ~SD_RISR_CARD_REMOVE;
> >     +        s->status |= SD_STAR_CARD_PRESENT;
> >     +    } else {
> >     +        s->irq_status &= ~SD_RISR_CARD_INSERT;
> >     +        s->irq_status |= SD_RISR_CARD_REMOVE;
> >     +        s->status &= ~SD_STAR_CARD_PRESENT;
> >     +    }
> >     +
> >     +    aw_h3_sdhost_update_irq(s);
> >     +}
> >     +
> >     +static void aw_h3_sdhost_send_command(AwH3SDHostState *s)
> >     +{
> >     +    SDRequest request;
> >     +    uint8_t resp[16];
> >     +    int rlen;
> >     +
> >     +    /* Auto clear load flag */
> >     +    s->command &= ~SD_CMDR_LOAD;
> >     +
> >     +    /* Clock change does not actually interact with the SD bus */
> >     +    if (!(s->command & SD_CMDR_CLKCHANGE)) {
> >     +
> >     +        /* Prepare request */
> >     +        request.cmd = s->command & SD_CMDR_CMDID_MASK;
> >     +        request.arg = s->command_arg;
> >     +
> >     +        /* Send request to SD bus */
> >     +        rlen = sdbus_do_command(&s->sdbus, &request, resp);
> >     +        if (rlen < 0) {
> >     +            goto error;
> >     +        }
> >     +
> >     +        /* If the command has a response, store it in the response
> >     registers */
> >     +        if ((s->command & SD_CMDR_RESPONSE)) {
> >     +            if (rlen == 0 ||
> >     +               (rlen == 4 && (s->command & SD_CMDR_RESPONSE_LONG)))
> {
> >     +                goto error;
> >     +            }
> >     +            if (rlen != 4 && rlen != 16) {
> >     +                goto error;
> >     +            }
>
> Maybe remove previous if...
>
> >     +            if (rlen == 4) {
> >     +                s->response[0] = ldl_be_p(&resp[0]);
> >     +                s->response[1] = s->response[2] = s->response[3] =
> 0;
> >     +            } else {
>
> ...
>
>                     } else if (rlen == 16) { ...
>
> >     +                s->response[0] = ldl_be_p(&resp[12]);
> >     +                s->response[1] = ldl_be_p(&resp[8]);
> >     +                s->response[2] = ldl_be_p(&resp[4]);
> >     +                s->response[3] = ldl_be_p(&resp[0]);
>
> ...
>
>                     } else {
>                         goto error;
>
> >     +            }
> >     +        }
> >     +    }
> >     +
> >     +    /* Set interrupt status bits */
> >     +    s->irq_status |= SD_RISR_CMD_COMPLETE;
> >     +    return;
> >     +
> >     +error:
> >     +    s->irq_status |= SD_RISR_NO_RESPONSE;
> >     +}
> >     +
> >     +static void aw_h3_sdhost_auto_stop(AwH3SDHostState *s)
> >     +{
> >     +    /*
> >     +     * The stop command (CMD12) ensures the SD bus
> >     +     * returns to the transfer state.
> >     +     */
> >     +    if ((s->command & SD_CMDR_AUTOSTOP) && (s->transfer_cnt == 0)) {
> >     +        /* First save current command registers */
> >     +        uint32_t saved_cmd = s->command;
> >     +        uint32_t saved_arg = s->command_arg;
> >     +
> >     +        /* Prepare stop command (CMD12) */
> >     +        s->command &= ~SD_CMDR_CMDID_MASK;
> >     +        s->command |= 12; /* CMD12 */
> >     +        s->command_arg = 0;
> >     +
> >     +        /* Put the command on SD bus */
> >     +        aw_h3_sdhost_send_command(s);
> >     +
> >     +        /* Restore command values */
> >     +        s->command = saved_cmd;
> >     +        s->command_arg = saved_arg;
> >     +    }
> >     +}
> >     +
> >     +static uint32_t aw_h3_sdhost_process_desc(AwH3SDHostState *s,
> >     +                                          hwaddr desc_addr,
> >     +                                          TransferDescriptor *desc,
> >     +                                          bool is_write, uint32_t
> >     max_bytes)
> >     +{
> >     +    uint32_t num_done = 0;
> >     +    uint32_t num_bytes = max_bytes;
> >     +    uint8_t buf[1024];
> >     +
> >     +    /* Read descriptor */
> >     +    cpu_physical_memory_read(desc_addr, desc, sizeof(*desc));
>
> Should we worry about endianess here?
>

I tried to figure out what is expected, but the
Allwinner_H3_Datasheet_V1.2.pdf does not
explicitly mention endianness for any of its I/O devices. Currently it
seems all devices are
happy with using the same endianness as the CPUs. In the MemoryRegionOps
has DEVICE_NATIVE_ENDIAN
set to match the behavior seen.


>
> >     +    if (desc->size == 0) {
> >     +        desc->size = 0xffff + 1;
>
> Why not write '64 * KiB'?
>
> >     +    }
> >     +    if (desc->size < num_bytes) {
> >     +        num_bytes = desc->size;
> >     +    }
> >     +
> >     +    trace_aw_h3_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));
>
> (Related to previous endianess question).
>
> >     +
> >     +    return num_done;
> >     +}
> >     +
> >     +static void aw_h3_sdhost_dma(AwH3SDHostState *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 = aw_h3_sdhost_process_desc(s, desc_addr, &desc,
> >     +                                               is_write,
> >     s->byte_count);
> >     +        aw_h3_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_AUTOCMD_DONE;
> >     +
> >     +    /* Update DMAC bits */
> >     +    if (is_write) {
> >     +        s->dmac_status |= SD_IDST_TRANSMIT_IRQ;
> >     +    } else {
> >     +        s->dmac_status |= (SD_IDST_SUM_RECEIVE_IRQ |
> >     SD_IDST_RECEIVE_IRQ);
> >     +    }
> >     +}
> >     +
> >     +static uint64_t aw_h3_sdhost_read(void *opaque, hwaddr offset,
> >     +                                  unsigned size)
> >     +{
> >     +    AwH3SDHostState *s = (AwH3SDHostState *)opaque;
> >     +    uint32_t res = 0;
> >     +
> >     +    switch (offset) {
> >     +    case REG_SD_GCTL:      /* Global Control */
> >     +        res = s->global_ctl;
> >     +        break;
> >     +    case REG_SD_CKCR:      /* Clock Control */
> >     +        res = s->clock_ctl;
> >     +        break;
> >     +    case REG_SD_TMOR:      /* Timeout */
> >     +        res = s->timeout;
> >     +        break;
> >     +    case REG_SD_BWDR:      /* Bus Width */
> >     +        res = s->bus_width;
> >     +        break;
> >     +    case REG_SD_BKSR:      /* Block Size */
> >     +        res = s->block_size;
> >     +        break;
> >     +    case REG_SD_BYCR:      /* Byte Count */
> >     +        res = s->byte_count;
> >     +        break;
> >     +    case REG_SD_CMDR:      /* Command */
> >     +        res = s->command;
> >     +        break;
> >     +    case REG_SD_CAGR:      /* Command Argument */
> >     +        res = s->command_arg;
> >     +        break;
> >     +    case REG_SD_RESP0:     /* Response Zero */
> >     +        res = s->response[0];
> >     +        break;
> >     +    case REG_SD_RESP1:     /* Response One */
> >     +        res = s->response[1];
> >     +        break;
> >     +    case REG_SD_RESP2:     /* Response Two */
> >     +        res = s->response[2];
> >     +        break;
> >     +    case REG_SD_RESP3:     /* Response Three */
> >     +        res = s->response[3];
> >     +        break;
> >     +    case REG_SD_IMKR:      /* Interrupt Mask */
> >     +        res = s->irq_mask;
> >     +        break;
> >     +    case REG_SD_MISR:      /* Masked Interrupt Status */
> >     +        res = s->irq_status & s->irq_mask;
> >     +        break;
> >     +    case REG_SD_RISR:      /* Raw Interrupt Status */
> >     +        res = s->irq_status;
> >     +        break;
> >     +    case REG_SD_STAR:      /* Status */
> >     +        res = s->status;
> >     +        break;
> >     +    case REG_SD_FWLR:      /* FIFO Water Level */
> >     +        res = s->fifo_wlevel;
> >     +        break;
> >     +    case REG_SD_FUNS:      /* FIFO Function Select */
> >     +        res = s->fifo_func_sel;
> >     +        break;
> >     +    case REG_SD_DBGC:      /* Debug Enable */
> >     +        res = s->debug_enable;
> >     +        break;
> >     +    case REG_SD_A12A:      /* Auto command 12 argument */
> >     +        res = s->auto12_arg;
> >     +        break;
> >     +    case REG_SD_NTSR:      /* SD NewTiming Set */
> >     +        res = s->newtiming_set;
> >     +        break;
> >     +    case REG_SD_SDBG:      /* SD newTiming Set Debug */
> >     +        res = s->newtiming_debug;
> >     +        break;
> >     +    case REG_SD_HWRST:     /* Hardware Reset Register */
> >     +        res = s->hardware_rst;
> >     +        break;
> >     +    case REG_SD_DMAC:      /* Internal DMA Controller Control */
> >     +        res = s->dmac;
> >     +        break;
> >     +    case REG_SD_DLBA:      /* Descriptor List Base Address */
> >     +        res = s->desc_base;
> >     +        break;
> >     +    case REG_SD_IDST:      /* Internal DMA Controller Status */
> >     +        res = s->dmac_status;
> >     +        break;
> >     +    case REG_SD_IDIE:      /* Internal DMA Controller Interrupt
> >     Enable */
> >     +        res = s->dmac_irq;
> >     +        break;
> >     +    case REG_SD_THLDC:     /* Card Threshold Control */
> >     +        res = s->card_threshold;
> >     +        break;
> >     +    case REG_SD_DSBD:      /* eMMC DDR Start Bit Detection Control
> */
> >     +        res = s->startbit_detect;
> >     +        break;
> >     +    case REG_SD_RES_CRC:   /* Response CRC from card/eMMC */
> >     +        res = s->response_crc;
> >     +        break;
> >     +    case REG_SD_DATA7_CRC: /* CRC Data 7 from card/eMMC */
> >     +    case REG_SD_DATA6_CRC: /* CRC Data 6 from card/eMMC */
> >     +    case REG_SD_DATA5_CRC: /* CRC Data 5 from card/eMMC */
> >     +    case REG_SD_DATA4_CRC: /* CRC Data 4 from card/eMMC */
> >     +    case REG_SD_DATA3_CRC: /* CRC Data 3 from card/eMMC */
> >     +    case REG_SD_DATA2_CRC: /* CRC Data 2 from card/eMMC */
> >     +    case REG_SD_DATA1_CRC: /* CRC Data 1 from card/eMMC */
> >     +    case REG_SD_DATA0_CRC: /* CRC Data 0 from card/eMMC */
> >     +        res = s->data_crc[((offset - REG_SD_DATA7_CRC) /
> >     sizeof(uint32_t))];
> >     +        break;
> >     +    case REG_SD_CRC_STA:   /* CRC status from card/eMMC in write
> >     operation */
> >     +        res = s->status_crc;
> >     +        break;
> >     +    case REG_SD_FIFO:      /* Read/Write FIFO */
> >     +        if (sdbus_data_ready(&s->sdbus)) {
> >     +            res = sdbus_read_data(&s->sdbus);
> >     +            res |= sdbus_read_data(&s->sdbus) << 8;
> >     +            res |= sdbus_read_data(&s->sdbus) << 16;
> >     +            res |= sdbus_read_data(&s->sdbus) << 24;
> >     +            aw_h3_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> >     +            aw_h3_sdhost_auto_stop(s);
> >     +            aw_h3_sdhost_update_irq(s);
> >     +        } else {
> >     +            qemu_log_mask(LOG_GUEST_ERROR, "%s: no data ready on SD
> >     bus\n",
> >     +                          __func__);
> >     +        }
> >     +        break;
> >     +    default:
> >     +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset
> >     %"HWADDR_PRIx"\n",
> >     +                      __func__, offset);
> >     +        res = 0;
> >     +        break;
> >     +    }
> >     +
> >     +    trace_aw_h3_sdhost_read(offset, res, size);
> >     +    return res;
> >     +}
> >     +
> >     +static void aw_h3_sdhost_write(void *opaque, hwaddr offset,
> >     +                               uint64_t value, unsigned size)
> >     +{
> >     +    AwH3SDHostState *s = (AwH3SDHostState *)opaque;
> >     +
> >     +    trace_aw_h3_sdhost_write(offset, value, size);
> >     +
> >     +    switch (offset) {
> >     +    case REG_SD_GCTL:      /* Global Control */
> >     +        s->global_ctl = value;
> >     +        s->global_ctl &= ~(SD_GCTL_DMA_RST | SD_GCTL_FIFO_RST |
> >     +                           SD_GCTL_SOFT_RST);
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_CKCR:      /* Clock Control */
> >     +        s->clock_ctl = value;
> >     +        break;
> >     +    case REG_SD_TMOR:      /* Timeout */
> >     +        s->timeout = value;
> >     +        break;
> >     +    case REG_SD_BWDR:      /* Bus Width */
> >     +        s->bus_width = value;
> >     +        break;
> >     +    case REG_SD_BKSR:      /* Block Size */
> >     +        s->block_size = value;
> >     +        break;
> >     +    case REG_SD_BYCR:      /* Byte Count */
> >     +        s->byte_count = value;
> >     +        s->transfer_cnt = value;
> >     +        break;
> >     +    case REG_SD_CMDR:      /* Command */
> >     +        s->command = value;
> >     +        if (value & SD_CMDR_LOAD) {
> >     +            aw_h3_sdhost_send_command(s);
> >     +            aw_h3_sdhost_dma(s);
> >     +            aw_h3_sdhost_auto_stop(s);
> >     +        }
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_CAGR:      /* Command Argument */
> >     +        s->command_arg = value;
> >     +        break;
> >     +    case REG_SD_RESP0:     /* Response Zero */
> >     +        s->response[0] = value;
> >     +        break;
> >     +    case REG_SD_RESP1:     /* Response One */
> >     +        s->response[1] = value;
> >     +        break;
> >     +    case REG_SD_RESP2:     /* Response Two */
> >     +        s->response[2] = value;
> >     +        break;
> >     +    case REG_SD_RESP3:     /* Response Three */
> >     +        s->response[3] = value;
> >     +        break;
> >     +    case REG_SD_IMKR:      /* Interrupt Mask */
> >     +        s->irq_mask = value;
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_MISR:      /* Masked Interrupt Status */
> >     +    case REG_SD_RISR:      /* Raw Interrupt Status */
> >     +        s->irq_status &= ~value;
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_STAR:      /* Status */
> >     +        s->status &= ~value;
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_FWLR:      /* FIFO Water Level */
> >     +        s->fifo_wlevel = value;
> >     +        break;
> >     +    case REG_SD_FUNS:      /* FIFO Function Select */
> >     +        s->fifo_func_sel = value;
> >     +        break;
> >     +    case REG_SD_DBGC:      /* Debug Enable */
> >     +        s->debug_enable = value;
> >     +        break;
> >     +    case REG_SD_A12A:      /* Auto command 12 argument */
> >     +        s->auto12_arg = value;
> >     +        break;
> >     +    case REG_SD_NTSR:      /* SD NewTiming Set */
> >     +        s->newtiming_set = value;
> >     +        break;
> >     +    case REG_SD_SDBG:      /* SD newTiming Set Debug */
> >     +        s->newtiming_debug = value;
> >     +        break;
> >     +    case REG_SD_HWRST:     /* Hardware Reset Register */
> >     +        s->hardware_rst = value;
> >     +        break;
> >     +    case REG_SD_DMAC:      /* Internal DMA Controller Control */
> >     +        s->dmac = value;
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_DLBA:      /* Descriptor List Base Address */
> >     +        s->desc_base = value;
> >     +        break;
> >     +    case REG_SD_IDST:      /* Internal DMA Controller Status */
> >     +        s->dmac_status &= (~SD_IDST_WR_MASK) | (~value &
> >     SD_IDST_WR_MASK);
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_IDIE:      /* Internal DMA Controller Interrupt
> >     Enable */
> >     +        s->dmac_irq = value;
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_THLDC:     /* Card Threshold Control */
> >     +        s->card_threshold = value;
> >     +        break;
> >     +    case REG_SD_DSBD:      /* eMMC DDR Start Bit Detection Control
> */
> >     +        s->startbit_detect = value;
> >     +        break;
> >     +    case REG_SD_FIFO:      /* Read/Write FIFO */
> >     +        sdbus_write_data(&s->sdbus, value & 0xff);
> >     +        sdbus_write_data(&s->sdbus, (value >> 8) & 0xff);
> >     +        sdbus_write_data(&s->sdbus, (value >> 16) & 0xff);
> >     +        sdbus_write_data(&s->sdbus, (value >> 24) & 0xff);
> >     +        aw_h3_sdhost_update_transfer_cnt(s, sizeof(uint32_t));
> >     +        aw_h3_sdhost_auto_stop(s);
> >     +        aw_h3_sdhost_update_irq(s);
> >     +        break;
> >     +    case REG_SD_RES_CRC:   /* Response CRC from card/eMMC */
> >     +    case REG_SD_DATA7_CRC: /* CRC Data 7 from card/eMMC */
> >     +    case REG_SD_DATA6_CRC: /* CRC Data 6 from card/eMMC */
> >     +    case REG_SD_DATA5_CRC: /* CRC Data 5 from card/eMMC */
> >     +    case REG_SD_DATA4_CRC: /* CRC Data 4 from card/eMMC */
> >     +    case REG_SD_DATA3_CRC: /* CRC Data 3 from card/eMMC */
> >     +    case REG_SD_DATA2_CRC: /* CRC Data 2 from card/eMMC */
> >     +    case REG_SD_DATA1_CRC: /* CRC Data 1 from card/eMMC */
> >     +    case REG_SD_DATA0_CRC: /* CRC Data 0 from card/eMMC */
> >     +    case REG_SD_CRC_STA:   /* CRC status from card/eMMC in write
> >     operation */
> >     +        break;
> >     +    default:
> >     +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset
> >     %"HWADDR_PRIx"\n",
> >     +                      __func__, offset);
> >     +        break;
> >     +    }
> >     +}
> >     +
> >     +static const MemoryRegionOps aw_h3_sdhost_ops = {
> >     +    .read = aw_h3_sdhost_read,
> >     +    .write = aw_h3_sdhost_write,
> >     +    .endianness = DEVICE_NATIVE_ENDIAN,
>
> I haven't checked .valid accesses from the datasheet.
>
> However due to:
>
>    res = s->data_crc[((offset - REG_SD_DATA7_CRC) / sizeof(uint32_t))];
>
> You seem to expect:
>
>             .impl.min_access_size = 4,
>
> .impl.max_access_size unset is 8, which should works.
>
> It seems that all registers are aligned on at least 32-bit boundaries. And
the section 5.3.5.1 mentions
that the DMA descriptors must be stored in memory 32-bit aligned. So based
on that information,
I think 32-bit is a safe choice. I've verified this with Linux mainline and
U-Boot drivers and both are still working.


> >     +};
> >     +
> >     +static const VMStateDescription vmstate_aw_h3_sdhost = {
> >     +    .name = TYPE_AW_H3_SDHOST,
>
> Do not use TYPE name in VMStateDescription.name, because we might change
> the value of TYPE, but the migration state has to keep the same name.
>
> OK thanks, I will make that changes in the other commits as well.


> >     +    .version_id = 1,
> >     +    .minimum_version_id = 1,
> >     +    .fields = (VMStateField[]) {
> >     +        VMSTATE_UINT32(global_ctl, AwH3SDHostState),
> >     +        VMSTATE_UINT32(clock_ctl, AwH3SDHostState),
> >     +        VMSTATE_UINT32(timeout, AwH3SDHostState),
> >     +        VMSTATE_UINT32(bus_width, AwH3SDHostState),
> >     +        VMSTATE_UINT32(block_size, AwH3SDHostState),
> >     +        VMSTATE_UINT32(byte_count, AwH3SDHostState),
> >     +        VMSTATE_UINT32(transfer_cnt, AwH3SDHostState),
> >     +        VMSTATE_UINT32(command, AwH3SDHostState),
> >     +        VMSTATE_UINT32(command_arg, AwH3SDHostState),
> >     +        VMSTATE_UINT32_ARRAY(response, AwH3SDHostState, 4),
> >     +        VMSTATE_UINT32(irq_mask, AwH3SDHostState),
> >     +        VMSTATE_UINT32(irq_status, AwH3SDHostState),
> >     +        VMSTATE_UINT32(status, AwH3SDHostState),
> >     +        VMSTATE_UINT32(fifo_wlevel, AwH3SDHostState),
> >     +        VMSTATE_UINT32(fifo_func_sel, AwH3SDHostState),
> >     +        VMSTATE_UINT32(debug_enable, AwH3SDHostState),
> >     +        VMSTATE_UINT32(auto12_arg, AwH3SDHostState),
> >     +        VMSTATE_UINT32(newtiming_set, AwH3SDHostState),
> >     +        VMSTATE_UINT32(newtiming_debug, AwH3SDHostState),
> >     +        VMSTATE_UINT32(hardware_rst, AwH3SDHostState),
> >     +        VMSTATE_UINT32(dmac, AwH3SDHostState),
> >     +        VMSTATE_UINT32(desc_base, AwH3SDHostState),
> >     +        VMSTATE_UINT32(dmac_status, AwH3SDHostState),
> >     +        VMSTATE_UINT32(dmac_irq, AwH3SDHostState),
> >     +        VMSTATE_UINT32(card_threshold, AwH3SDHostState),
> >     +        VMSTATE_UINT32(startbit_detect, AwH3SDHostState),
> >     +        VMSTATE_UINT32(response_crc, AwH3SDHostState),
> >     +        VMSTATE_UINT32_ARRAY(data_crc, AwH3SDHostState, 8),
> >     +        VMSTATE_UINT32(status_crc, AwH3SDHostState),
> >     +        VMSTATE_END_OF_LIST()
> >     +    }
> >     +};
> >     +
> >     +static void aw_h3_sdhost_init(Object *obj)
> >     +{
> >     +    AwH3SDHostState *s = AW_H3_SDHOST(obj);
> >     +
> >     +    qbus_create_inplace(&s->sdbus, sizeof(s->sdbus),
> >     +                        TYPE_AW_H3_SDHOST_BUS, DEVICE(s), "sd-bus");
> >     +
> >     +    memory_region_init_io(&s->iomem, obj, &aw_h3_sdhost_ops, s,
> >     +                          TYPE_AW_H3_SDHOST,
> >     AW_H3_SDHOST_REGS_MEM_SIZE);
> >     +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> >     +    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
> >     +}
> >     +
> >     +static void aw_h3_sdhost_reset(DeviceState *dev)
> >     +{
> >     +    AwH3SDHostState *s = AW_H3_SDHOST(dev);
> >     +
> >     +    s->global_ctl = REG_SD_GCTL_RST;
> >     +    s->clock_ctl = REG_SD_CKCR_RST;
> >     +    s->timeout = REG_SD_TMOR_RST;
> >     +    s->bus_width = REG_SD_BWDR_RST;
> >     +    s->block_size = REG_SD_BKSR_RST;
> >     +    s->byte_count = REG_SD_BYCR_RST;
> >     +    s->transfer_cnt = 0;
> >     +
> >     +    s->command = REG_SD_CMDR_RST;
> >     +    s->command_arg = REG_SD_CAGR_RST;
> >     +
> >     +    for (int i = 0; i < sizeof(s->response) /
> >     sizeof(s->response[0]); i++) {
>
> Please use ARRAY_SIZE(s->response).
>
> >     +        s->response[i] = REG_SD_RESP_RST;
> >     +    }
> >     +
> >     +    s->irq_mask = REG_SD_IMKR_RST;
> >     +    s->irq_status = REG_SD_RISR_RST;
> >     +    s->status = REG_SD_STAR_RST;
> >     +
> >     +    s->fifo_wlevel = REG_SD_FWLR_RST;
> >     +    s->fifo_func_sel = REG_SD_FUNS_RST;
> >     +    s->debug_enable = REG_SD_DBGC_RST;
> >     +    s->auto12_arg = REG_SD_A12A_RST;
> >     +    s->newtiming_set = REG_SD_NTSR_RST;
> >     +    s->newtiming_debug = REG_SD_SDBG_RST;
> >     +    s->hardware_rst = REG_SD_HWRST_RST;
> >     +    s->dmac = REG_SD_DMAC_RST;
> >     +    s->desc_base = REG_SD_DLBA_RST;
> >     +    s->dmac_status = REG_SD_IDST_RST;
> >     +    s->dmac_irq = REG_SD_IDIE_RST;
> >     +    s->card_threshold = REG_SD_THLDC_RST;
> >     +    s->startbit_detect = REG_SD_DSBD_RST;
> >     +    s->response_crc = REG_SD_RES_CRC_RST;
> >     +
> >     +    for (int i = 0; i < sizeof(s->data_crc) /
> >     sizeof(s->data_crc[0]); i++) {
>
> ARRAY_SIZE
>
> >     +        s->data_crc[i] = REG_SD_DATA_CRC_RST;
> >     +    }
> >     +
> >     +    s->status_crc = REG_SD_CRC_STA_RST;
> >     +}
> >     +
> >     +static void aw_h3_sdhost_bus_class_init(ObjectClass *klass, void
> *data)
> >     +{
> >     +    SDBusClass *sbc = SD_BUS_CLASS(klass);
> >     +
> >     +    sbc->set_inserted = aw_h3_sdhost_set_inserted;
> >     +}
> >     +
> >     +static void aw_h3_sdhost_class_init(ObjectClass *klass, void *data)
> >     +{
> >     +    DeviceClass *dc = DEVICE_CLASS(klass);
> >     +
> >     +    dc->reset = aw_h3_sdhost_reset;
> >     +    dc->vmsd = &vmstate_aw_h3_sdhost;
> >     +}
> >     +
> >     +static TypeInfo aw_h3_sdhost_info = {
> >     +    .name          = TYPE_AW_H3_SDHOST,
> >     +    .parent        = TYPE_SYS_BUS_DEVICE,
> >     +    .instance_size = sizeof(AwH3SDHostState),
> >     +    .class_init    = aw_h3_sdhost_class_init,
> >     +    .instance_init = aw_h3_sdhost_init,
> >     +};
> >     +
> >     +static const TypeInfo aw_h3_sdhost_bus_info = {
> >     +    .name = TYPE_AW_H3_SDHOST_BUS,
> >     +    .parent = TYPE_SD_BUS,
> >     +    .instance_size = sizeof(SDBus),
> >     +    .class_init = aw_h3_sdhost_bus_class_init,
> >     +};
> >     +
> >     +static void aw_h3_sdhost_register_types(void)
> >     +{
> >     +    type_register_static(&aw_h3_sdhost_info);
> >     +    type_register_static(&aw_h3_sdhost_bus_info);
> >     +}
> >     +
> >     +type_init(aw_h3_sdhost_register_types)
> >     diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> >     index efcff666a2..c672a201b5 100644
> >     --- a/hw/sd/trace-events
> >     +++ b/hw/sd/trace-events
> >     @@ -1,5 +1,12 @@
> >       # See docs/devel/tracing.txt for syntax documentation.
> >
> >     +# allwinner-h3-sdhost.c
> >     +aw_h3_sdhost_set_inserted(bool inserted) "inserted %u"
> >     +aw_h3_sdhost_process_desc(uint64_t desc_addr, uint32_t desc_size,
> >     bool is_write, uint32_t max_bytes) "desc_addr 0x%" PRIx64 "
> >     desc_size %u is_write %u max_bytes %u"
>
> Please also use PRIu32 for desc_size/max_bytes.
>
> Done. I'll also use PRIu32 / PRIx32 in the other commits that have
trace-events changes.


> >     +aw_h3_sdhost_read(uint64_t offset, uint64_t data, unsigned size)
> >     "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> >     +aw_h3_sdhost_write(uint64_t offset, uint64_t data, unsigned size)
> >     "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> >     +aw_h3_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x"
>
> PRIx32
>

> >     +
> >       # bcm2835_sdhost.c
> >       bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size)
> >     "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> >       bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned
> >     size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> >     diff --git a/include/hw/arm/allwinner-h3.h
> >     b/include/hw/arm/allwinner-h3.h
> >     index 33602599eb..7aff4ebbd2 100644
> >     --- a/include/hw/arm/allwinner-h3.h
> >     +++ b/include/hw/arm/allwinner-h3.h
> >     @@ -30,6 +30,7 @@
> >       #include "hw/misc/allwinner-h3-cpucfg.h"
> >       #include "hw/misc/allwinner-h3-syscon.h"
> >       #include "hw/misc/allwinner-h3-sid.h"
> >     +#include "hw/sd/allwinner-h3-sdhost.h"
> >       #include "target/arm/cpu.h"
> >
> >       #define AW_H3_SRAM_A1_BASE     (0x00000000)
> >     @@ -117,6 +118,7 @@ typedef struct AwH3State {
> >           AwH3CpuCfgState cpucfg;
> >           AwH3SysconState syscon;
> >           AwH3SidState sid;
> >     +    AwH3SDHostState mmc0;
> >           GICState gic;
> >           MemoryRegion sram_a1;
> >           MemoryRegion sram_a2;
> >     diff --git a/include/hw/sd/allwinner-h3-sdhost.h
> >     b/include/hw/sd/allwinner-h3-sdhost.h
> >     new file mode 100644
> >     index 0000000000..6c898a3c84
> >     --- /dev/null
> >     +++ b/include/hw/sd/allwinner-h3-sdhost.h
> >     @@ -0,0 +1,73 @@
> >     +/*
> >     + * Allwinner H3 SD Host Controller emulation
> >     + *
> >     + * Copyright (C) 2019 Niek Linnenbank <nieklinnenbank@gmail.com
> >     <mailto:nieklinnenbank@gmail.com>>
> >     + *
> >     + * This program is free software: you can redistribute it and/or
> modify
> >     + * it under the terms of the GNU General Public License as
> published by
> >     + * the Free Software Foundation, either version 2 of the License, or
> >     + * (at your option) any later version.
> >     + *
> >     + * This program is distributed in the hope that it will be useful,
> >     + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >     + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >     + * GNU General Public License for more details.
> >     + *
> >     + * You should have received a copy of the GNU General Public License
> >     + * along with this program.  If not, see
> >     <http://www.gnu.org/licenses/>.
> >     + */
> >     +
> >     +#ifndef ALLWINNER_H3_SDHOST_H
> >     +#define ALLWINNER_H3_SDHOST_H
> >     +
> >     +#include "hw/sysbus.h"
> >     +#include "hw/sd/sd.h"
> >     +
> >     +#define AW_H3_SDHOST_REGS_MEM_SIZE  (1024)
>
> Move this definition to the source file.
>
> >     +
> >     +#define TYPE_AW_H3_SDHOST "allwinner-h3-sdhost"
> >     +#define AW_H3_SDHOST(obj) \
> >     +        OBJECT_CHECK(AwH3SDHostState, (obj), TYPE_AW_H3_SDHOST)
> >     +
> >     +typedef struct {
> >     +    SysBusDevice busdev;
> >     +    SDBus sdbus;
> >     +    MemoryRegion iomem;
> >     +
> >     +    uint32_t global_ctl;
> >     +    uint32_t clock_ctl;
> >     +    uint32_t timeout;
> >     +    uint32_t bus_width;
> >     +    uint32_t block_size;
> >     +    uint32_t byte_count;
> >     +    uint32_t transfer_cnt;
> >     +
> >     +    uint32_t command;
> >     +    uint32_t command_arg;
> >     +    uint32_t response[4];
> >     +
> >     +    uint32_t irq_mask;
> >     +    uint32_t irq_status;
> >     +    uint32_t status;
> >     +
> >     +    uint32_t fifo_wlevel;
> >     +    uint32_t fifo_func_sel;
> >     +    uint32_t debug_enable;
> >     +    uint32_t auto12_arg;
> >     +    uint32_t newtiming_set;
> >     +    uint32_t newtiming_debug;
> >     +    uint32_t hardware_rst;
> >     +    uint32_t dmac;
> >     +    uint32_t desc_base;
> >     +    uint32_t dmac_status;
> >     +    uint32_t dmac_irq;
> >     +    uint32_t card_threshold;
> >     +    uint32_t startbit_detect;
> >     +    uint32_t response_crc;
> >     +    uint32_t data_crc[8];
> >     +    uint32_t status_crc;
> >     +
> >     +    qemu_irq irq;
> >     +} AwH3SDHostState;
> >     +
> >     +#endif
> >     --
> >     2.17.1
>
> I haven't checked the datasheet for all the registers/bits.
>
> Patch very clean, chapeau!
>
> Regards,
>
> Phil.
>
>

-- 
Niek Linnenbank

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

  parent reply index

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 21:09 [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Niek Linnenbank
2019-12-02 21:09 ` [PATCH 01/10] hw: arm: add Allwinner H3 System-on-Chip Niek Linnenbank
2019-12-04 16:53   ` Philippe Mathieu-Daudé
2019-12-04 20:44     ` Niek Linnenbank
2019-12-10  9:02   ` Philippe Mathieu-Daudé
2019-12-10 19:17     ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine Niek Linnenbank
2019-12-03  9:17   ` Philippe Mathieu-Daudé
2019-12-03 19:33     ` Niek Linnenbank
2019-12-04  9:03       ` Philippe Mathieu-Daudé
2019-12-04 19:50         ` Niek Linnenbank
2019-12-05 22:15     ` Niek Linnenbank
2019-12-06  5:41       ` Philippe Mathieu-Daudé
2019-12-06 22:15         ` Niek Linnenbank
2019-12-10  8:59           ` Philippe Mathieu-Daudé
2019-12-10 19:14             ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 03/10] arm: allwinner-h3: add Clock Control Unit Niek Linnenbank
2019-12-13  0:03   ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 04/10] arm: allwinner-h3: add USB host controller Niek Linnenbank
2019-12-04 16:11   ` Aleksandar Markovic
2019-12-04 20:20     ` Niek Linnenbank
2019-12-10  7:56   ` Philippe Mathieu-Daudé
2019-12-10  8:29     ` Gerd Hoffmann
2019-12-10 19:11       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 05/10] arm: allwinner-h3: add System Control module Niek Linnenbank
2019-12-13  0:09   ` Philippe Mathieu-Daudé
2019-12-15 23:27     ` Niek Linnenbank
2019-12-16  0:17       ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 06/10] arm/arm-powerctl: set NSACR.{CP11, CP10} bits in arm_set_cpu_on() Niek Linnenbank
2019-12-06 14:24   ` Peter Maydell
2019-12-06 20:01     ` Niek Linnenbank
2019-12-13 20:52       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 07/10] arm: allwinner-h3: add CPU Configuration module Niek Linnenbank
2019-12-02 21:09 ` [PATCH 08/10] arm: allwinner-h3: add Security Identifier device Niek Linnenbank
2019-12-06 14:27   ` Peter Maydell
2019-12-06 16:35     ` Philippe Mathieu-Daudé
2019-12-06 20:20       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller Niek Linnenbank
2019-12-11 22:34   ` Niek Linnenbank
2019-12-12 23:56     ` Philippe Mathieu-Daudé
2019-12-13 21:00       ` Niek Linnenbank
2019-12-14 13:59         ` Philippe Mathieu-Daudé
2019-12-14 20:32           ` Niek Linnenbank
2019-12-15 23:07       ` Niek Linnenbank [this message]
2019-12-16  0:14         ` Philippe Mathieu-Daudé
2019-12-16 19:46           ` Niek Linnenbank
2019-12-16 21:28             ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device Niek Linnenbank
2019-12-03  9:33   ` KONRAD Frederic
2019-12-03 19:41     ` Niek Linnenbank
2019-12-04 15:14     ` Philippe Mathieu-Daudé
2019-12-04 15:22       ` KONRAD Frederic
2019-12-03  8:47 ` [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Philippe Mathieu-Daudé
2019-12-03 19:25   ` Niek Linnenbank
2019-12-10  8:40     ` Philippe Mathieu-Daudé
2019-12-09 21:37   ` Niek Linnenbank
2019-12-10  8:26     ` Philippe Mathieu-Daudé
2019-12-10 20:12       ` Niek Linnenbank
2019-12-12 23:07         ` Niek Linnenbank
2019-12-12 23:25           ` Philippe Mathieu-Daudé
2019-12-13 20:45             ` Niek Linnenbank
2019-12-03  9:02 ` Philippe Mathieu-Daudé
2019-12-03 19:32   ` Niek Linnenbank
2019-12-06 14:16     ` Peter Maydell
2019-12-09 22:24       ` Aleksandar Markovic
2019-12-10 10:34 ` KONRAD Frederic
2019-12-10 19:55   ` Niek Linnenbank

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPan3WpiWahU85-ZgV4BMPU8mUTxsq2TPn-szOHkHc5+PHUqpw@mail.gmail.com \
    --to=nieklinnenbank@gmail.com \
    --cc=b.galvani@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git