qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 10/18] hw/sd: Add Cadence SDHCI emulation


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH 10/18] hw/sd: Add Cadence SDHCI emulation
Date: Sat, 15 Aug 2020 10:51:48 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 8/14/20 6:40 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Cadence SD/SDIO/eMMC Host Controller (SD4HC) is an SDHCI compatible
> controller. The SDHCI compatible registers start from offset 0x200,
> which are called Slot Register Set (SRS) in its datasheet.
> 
> This creates a Cadence SDHCI model built on top of the existing
> generic SDHCI model. Cadence specific Host Register Set (HRS) is
> implemented to make guest software happy.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>  hw/sd/Kconfig                 |   4 ++
>  hw/sd/Makefile.objs           |   1 +
>  hw/sd/cadence_sdhci.c         | 162 
> ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/sd/cadence_sdhci.h |  65 +++++++++++++++++
>  4 files changed, 232 insertions(+)
>  create mode 100644 hw/sd/cadence_sdhci.c
>  create mode 100644 include/hw/sd/cadence_sdhci.h

Please consider using scripts/git.orderfile.

> 
> diff --git a/hw/sd/Kconfig b/hw/sd/Kconfig
> index c5e1e55..633b9af 100644
> --- a/hw/sd/Kconfig
> +++ b/hw/sd/Kconfig
> @@ -19,3 +19,7 @@ config SDHCI_PCI
>      default y if PCI_DEVICES
>      depends on PCI
>      select SDHCI
> +
> +config CADENCE_SDHCI
> +    bool
> +    select SDHCI
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index 0d1df17..4d500a6 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -10,3 +10,4 @@ common-obj-$(CONFIG_OMAP) += omap_mmc.o
>  common-obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
>  common-obj-$(CONFIG_RASPI) += bcm2835_sdhost.o
>  common-obj-$(CONFIG_ASPEED_SOC) += aspeed_sdhci.o
> +common-obj-$(CONFIG_CADENCE_SDHCI) += cadence_sdhci.o
> diff --git a/hw/sd/cadence_sdhci.c b/hw/sd/cadence_sdhci.c
> new file mode 100644
> index 0000000..67ffd14
> --- /dev/null
> +++ b/hw/sd/cadence_sdhci.c
> @@ -0,0 +1,162 @@
> +/*
> + * Cadence SDHCI emulation
> + *
> + * Copyright (c) 2020 Wind River Systems, Inc.
> + *
> + * Author:
> + *   Bin Meng <bin.meng@windriver.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 or
> + * (at your option) version 3 of the License.
> + *
> + * 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/error-report.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "hw/irq.h"
> +#include "hw/sd/cadence_sdhci.h"
> +#include "sdhci-internal.h"
> +
> +#define TO_REG(addr)    ((addr) / sizeof(uint32_t))
> +
> +static void cadence_sdhci_reset(DeviceState *dev)
> +{
> +    CadenceSDHCIState *sdhci = CADENCE_SDHCI(dev);

We usually use 's' for the model state.

> +
> +    memset(sdhci->regs, 0, CADENCE_SDHCI_REG_SIZE);
> +    sdhci->regs[TO_REG(SDHCI_CDNS_HRS00)] = SDHCI_CDNS_HRS00_POR_VAL;

As commented in previous patch, here goes:

       device_legacy_reset(DEVICE(&sdhci->slot));

> +}
> +
> +static uint64_t cadence_sdhci_read(void *opaque, hwaddr addr, unsigned int 
> size)
> +{
> +    uint32_t val = 0;
> +    CadenceSDHCIState *sdhci = opaque;
> +
> +    if (addr < CADENCE_SDHCI_REG_SIZE) {
> +        val = sdhci->regs[TO_REG(addr)];
> +    } else {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Out-of-bounds read at 0x%" HWADDR_PRIx "\n",
> +                      __func__, addr);
> +    }
> +
> +    return (uint64_t)val;
> +}
> +
> +static void cadence_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
> +                                unsigned int size)
> +{
> +    CadenceSDHCIState *sdhci = opaque;
> +    uint32_t val32 = (uint32_t)val;
> +
> +    switch (addr) {
> +    case SDHCI_CDNS_HRS00:
> +        /*
> +         * The only writable bit is SWR (software reset) and it automatically
> +         * clears to zero, so essentially this register remains unchanged.
> +         */
> +        if (val32 & SDHCI_CDNS_HRS00_SWR) {
> +            cadence_sdhci_reset(DEVICE(sdhci));
> +            sdhci_poweron_reset(DEVICE(&sdhci->slot));

The 'slot' reset has to go in cadence_sdhci_reset().

> +        }
> +
> +        break;
> +    case SDHCI_CDNS_HRS04:
> +        /*
> +         * Only emulate the ACK bit behavior when read or write transaction
> +         * are requested.
> +         */
> +        if (val32 & (SDHCI_CDNS_HRS04_WR | SDHCI_CDNS_HRS04_RD)) {
> +            val32 |= SDHCI_CDNS_HRS04_ACK;
> +        } else {
> +            val32 &= ~SDHCI_CDNS_HRS04_ACK;
> +        }
> +
> +        sdhci->regs[TO_REG(addr)] = val32;
> +        break;
> +    case SDHCI_CDNS_HRS06:
> +        if (val32 & SDHCI_CDNS_HRS06_TUNE_UP) {
> +            val32 &= ~SDHCI_CDNS_HRS06_TUNE_UP;
> +        }
> +
> +        sdhci->regs[TO_REG(addr)] = val32;
> +        break;
> +    default:
> +        if (addr < CADENCE_SDHCI_REG_SIZE) {
> +            sdhci->regs[TO_REG(addr)] = val32;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Out-of-bounds write at 0x%" HWADDR_PRIx "\n",
> +                          __func__, addr);
> +        }
> +    }
> +}
> +
> +static const MemoryRegionOps cadence_sdhci_ops = {
> +    .read = cadence_sdhci_read,
> +    .write = cadence_sdhci_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,

To make it obvious, please also add:

       .impl = {
           .min_access_size = 4,
           .max_access_size = 4,
       },

> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    }
> +};
> +
> +static void cadence_sdhci_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    CadenceSDHCIState *sdhci = CADENCE_SDHCI(dev);
> +    SysBusDevice *sbd_slot = SYS_BUS_DEVICE(&sdhci->slot);

Where is sdhci->slot initialized??

> +
> +    memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &cadence_sdhci_ops,
> +                          sdhci, TYPE_CADENCE_SDHCI, 0x1000);
> +    sysbus_init_mmio(sbd, &sdhci->iomem);
> +
> +    sysbus_realize(sbd_slot, errp);
> +    memory_region_add_subregion(&sdhci->iomem, SDHCI_CDNS_SRS_BASE,
> +                                &sdhci->slot.iomem);

You should not access private field that way, you have to use
the public interface:

  memory_region_add_subregion(&sdhci->iomem, SDHCI_CDNS_SRS_BASE,
           sysbus_mmio_get_region(SYS_BUS_DEVICE(&sdhci->slot), 0));

To have this model useful, you need to propagate at least the 'sd-bus'
and 'dma' properties from the 'slot', else how the TYPE_CADENCE_SDHCI
user is supposed to set them?

> +}
> +
> +static const VMStateDescription vmstate_cadence_sdhci = {
> +    .name = TYPE_CADENCE_SDHCI,
> +    .version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, CadenceSDHCIState, 
> CADENCE_SDHCI_NUM_REGS),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static void cadence_sdhci_class_init(ObjectClass *classp, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(classp);
> +
> +    dc->realize = cadence_sdhci_realize;
> +    dc->reset = cadence_sdhci_reset;
> +    dc->vmsd = &vmstate_cadence_sdhci;
> +}
> +
> +static TypeInfo cadence_sdhci_info = {
> +    .name          = TYPE_CADENCE_SDHCI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(CadenceSDHCIState),
> +    .class_init    = cadence_sdhci_class_init,
> +};
> +
> +static void cadence_sdhci_register_types(void)
> +{
> +    type_register_static(&cadence_sdhci_info);
> +}
> +
> +type_init(cadence_sdhci_register_types)
> diff --git a/include/hw/sd/cadence_sdhci.h b/include/hw/sd/cadence_sdhci.h
> new file mode 100644
> index 0000000..9ff78a9
> --- /dev/null
> +++ b/include/hw/sd/cadence_sdhci.h
> @@ -0,0 +1,65 @@
> +/*
> + * Cadence SDHCI emulation
> + *
> + * Copyright (c) 2020 Wind River Systems, Inc.
> + *
> + * Author:
> + *   Bin Meng <bin.meng@windriver.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 or
> + * (at your option) version 3 of the License.
> + *
> + * 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 CADENCE_SDHCI_H
> +#define CADENCE_SDHCI_H
> +
> +#include "qemu/bitops.h"
> +#include "hw/sd/sdhci.h"
> +
> +/* HRS - Host Register Set (specific to Cadence) */
> +
> +#define SDHCI_CDNS_HRS00                0x00    /* general information */
> +#define SDHCI_CDNS_HRS00_SWR                BIT(0)
> +#define SDHCI_CDNS_HRS00_POR_VAL            0x00010000
> +
> +#define SDHCI_CDNS_HRS04                0x10    /* PHY access port */
> +#define SDHCI_CDNS_HRS04_WR                 BIT(24)
> +#define SDHCI_CDNS_HRS04_RD                 BIT(25)
> +#define SDHCI_CDNS_HRS04_ACK                BIT(26)
> +
> +#define SDHCI_CDNS_HRS06                0x18    /* eMMC control */
> +#define SDHCI_CDNS_HRS06_TUNE_UP            BIT(15)
> +
> +/* SRS - Slot Register Set (SDHCI-compatible) */
> +#define SDHCI_CDNS_SRS_BASE             0x200

Please keep the previous definitions local to cadence_sdhci.c.

> +
> +#define CADENCE_SDHCI_CAPABILITIES 0x01E80080
> +#define CADENCE_SDHCI_REG_SIZE     0x100
> +#define CADENCE_SDHCI_NUM_REGS     (CADENCE_SDHCI_REG_SIZE / 
> sizeof(uint32_t))
> +
> +typedef struct CadenceSDHCIState {
> +    SysBusDevice parent;
> +
> +    MemoryRegion iomem;
> +    qemu_irq irq;
> +
> +    uint32_t regs[CADENCE_SDHCI_NUM_REGS];
> +
> +    SDHCIState slot;

Why name this 'slot' and not 'sdhci' or 'generic_sdhci'?

Thanks,

Phil.

> +} CadenceSDHCIState;
> +
> +#define TYPE_CADENCE_SDHCI  "cadence.sdhci"
> +#define CADENCE_SDHCI(obj)  OBJECT_CHECK(CadenceSDHCIState, (obj), \
> +                                         TYPE_CADENCE_SDHCI)
> +
> +#endif /* CADENCE_SDHCI_H */
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]