Re: [RFC PATCH 20/23] hw/ssi: add support for flexspi

[Philippe Mathieu-Daudé]

Hi Octavian,

On 5/8/24 22:17, Octavian Purdila wrote:
> This is mostly a stub which completes SPI transactions as noops
> by masking out the error interrupts and never clearing the IPCMDDONE
> interrupt.
>
> Although incomplete, this allows software that uses NXP's mcuxpresso
> SDK to run the SDK board initialization functions.
>
> It also supports AHB memory access, aka XIP, for now as simple RAM
> memory regions.
>
> Signed-off-by: Octavian Purdila <tavip@google.com>
> ---
>   hw/arm/svd/meson.build   |   4 +
>   hw/ssi/Kconfig           |   4 +
>   hw/ssi/flexspi.c         | 216 +++++++++++++++++++++++++++++++++++++++
>   hw/ssi/meson.build       |   1 +
>   hw/ssi/trace-events      |   4 +
>   include/hw/ssi/flexspi.h |  34 ++++++
>   6 files changed, 263 insertions(+)
>   create mode 100644 hw/ssi/flexspi.c
>   create mode 100644 include/hw/ssi/flexspi.h


> diff --git a/hw/ssi/flexspi.c b/hw/ssi/flexspi.c
> new file mode 100644
> index 0000000000..305d1a5bac
> --- /dev/null
> +++ b/hw/ssi/flexspi.c
> @@ -0,0 +1,216 @@
> +/*
> + * QEMU model for FLEXSPI
> + *
> + * Copyright (c) 2024 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/mmap-alloc.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "migration/vmstate.h"
> +#include "exec/address-spaces.h"
> +#include "hw/regs.h"
> +#include "hw/ssi/flexspi.h"
> +#include "hw/arm/svd/flexspi.h"
> +
> +#include "trace.h"
> +
> +#define reg(field) offsetof(FLEXSPI_Type, field)
> +#define regi(x) (reg(x) / sizeof(uint32_t))
> +#define REG_NO (sizeof(FLEXSPI_Type) / sizeof(uint32_t))
> +
> +static FLEXSPI_REGISTER_NAMES_ARRAY(reg_names);
> +
> +static void flexspi_reset(DeviceState *dev)
> +{
> +    FlexSpiState *s = FLEXSPI(dev);
> +
> +    memset(&s->regs, 0, sizeof(s->regs));
> +
> +    flexspi_reset_registers(&s->regs);
> +
> +    /* idle immediately after reset */
> +    s->regs.STS0_b.SEQIDLE = 1;
> +}
> +
> +static MemTxResult flexspi_read(void *opaque, hwaddr addr,
> +                                     uint64_t *data, unsigned size,
> +                                     MemTxAttrs attrs)
> +{
> +    FlexSpiState *s = opaque;
> +    MemTxResult ret = MEMTX_OK;
> +
> +    if (!reg32_aligned_access(addr, size)) {
> +        ret = MEMTX_ERROR;
> +        goto out;
> +    }
> +
> +    switch (addr) {
> +    default:
> +        *data = reg32_read(&s->regs, addr);
> +        break;
> +    }
> +
> +out:
> +    trace_flexspi_reg_read(DEVICE(s)->id, reg_names[addr], addr, *data);
> +    return ret;
> +}
> +
> +static uint32_t wr_mask[REG_NO] = {
> +    [regi(MCR0)] = BITS(31, 14) | BITS(12, 8) | BITS(5, 4) | BITS(1, 0),
> +    [regi(MCR1)] = BITS(31, 0),
> +    [regi(MCR2)] = BITS(31, 24) | BIT(11),
> +    [regi(AHBCR)] = BIT(10) | BITS(7, 2) | BIT(0),
> +    [regi(INTEN)] = BITS(13, 0),
> +    /*
> +     * TODO: once SPI transfers are implemented restore mask to:
> +     *
> +     * [regi(INTR)] = BIT(16) | BITS(12, 0).
> +     *
> +     * In the meantime this INTR mask allows for fake SPI transfers.
> +     */
> +    [regi(INTR)] = BIT(0),
> +    [regi(LUTKEY)] = BITS(31, 0),
> +    [regi(LUTCR)] = BITS(1, 0),
> +    [regi(AHBRXBUF0CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF1CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF2CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF3CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF4CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF5CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF6CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(AHBRXBUF7CR0)] = BIT(31) | BITS(26, 24) | BITS(19, 16) | BITS(7, 0),
> +    [regi(FLSHA1CR0)] = BITS(22, 0),
> +    [regi(FLSHA2CR0)] = BITS(22, 0),
> +    [regi(FLSHB1CR0)] = BITS(22, 0),
> +    [regi(FLSHB2CR0)] = BITS(22, 0),
> +    [regi(FLSHCR1A1)] = BITS(31, 0),
> +    [regi(FLSHCR1A2)] = BITS(31, 0),
> +    [regi(FLSHCR1B1)] = BITS(31, 0),
> +    [regi(FLSHCR1B2)] = BITS(31, 0),
> +    [regi(FLSHCR2A1)] = BITS(30, 13) | BITS(11, 5) | BITS(3, 0),
> +    [regi(FLSHCR2A2)] = BITS(30, 13) | BITS(11, 5) | BITS(3, 0),
> +    [regi(FLSHCR2B1)] = BITS(30, 13) | BITS(11, 5) | BITS(3, 0),
> +    [regi(FLSHCR2B2)] = BITS(30, 13) | BITS(11, 5) | BITS(3, 0),
> +    [regi(FLSHCR4)] = BITS(3, 2) | BIT(0),
> +    [regi(IPCR0)] = BITS(31, 0),
> +    [regi(IPCR1)] = BIT(31) | BITS(26, 24) | BITS(19, 0),
> +    [regi(IPCMD)] = BIT(1),
> +    [regi(DLPR)] = BITS(31, 0),
> +    [regi(IPRXFCR)] = BITS(8, 0),
> +    [regi(IPTXFCR)] = BITS(8, 0),
> +    [regi(DLLCRA)] = BITS(14, 8) | BITS(6, 3) | BITS(1, 0),
> +    [regi(DLLCRB)] = BITS(14, 8) | BITS(6, 3) | BITS(1, 0),
> +    [regi(HADDRSTART)] = BITS(31, 12) | BIT(0),
> +    [regi(HADDREND)] = BITS(31, 12),
> +    [regi(HADDROFFSET)] = BITS(31, 12),
> +};
> +
> +static MemTxResult flexspi_write(void *opaque, hwaddr addr,
> +                                      uint64_t value, unsigned size,
> +                                      MemTxAttrs attrs)
> +{
> +    FlexSpiState *s = opaque;
> +    MemTxResult ret = MEMTX_OK;
> +
> +    trace_flexspi_reg_write(DEVICE(s)->id, reg_names[addr], addr, value);
> +
> +    if (!reg32_aligned_access(addr, size)) {
> +        ret = MEMTX_ERROR;
> +        goto out;
> +    }
> +
> +    switch (addr) {
> +    case reg(MCR0):
> +    {
> +        reg32_write(&s->regs, addr, value, wr_mask);
> +
> +        if (s->regs.MCR0_b.SWRESET) {
> +            s->regs.MCR0_b.SWRESET = 0;
> +        }
> +        break;
> +    }
> +
> +    default:
> +        reg32_write(&s->regs, addr, value, wr_mask);
> +        break;
> +    }
> +
> +out:
> +    return ret;
> +}
> +
> +static const MemoryRegionOps flexspi_ops = {
> +    .read_with_attrs = flexspi_read,
> +    .write_with_attrs = flexspi_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,

I'm not a fan of your reg32_aligned_access() method, I'd rather
use the generic path with:

    .valid = {
        .max_access_size = 4,
        .min_access_size = 4,
        .unaligned = false
    },

> +};
> +
> +static Property flexspi_properties[] = {
> +    DEFINE_PROP_UINT32("mmap_base", FlexSpiState, mmap_base, 0),
> +    DEFINE_PROP_UINT32("mmap_size", FlexSpiState, mmap_size, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void flexspi_init(Object *obj)
> +{
> +    FlexSpiState *s = FLEXSPI(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &flexspi_ops, s, TYPE_FLEXSPI,
> +                          sizeof(FLEXSPI_Type));
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +
> +static void flexspi_realize(DeviceState *dev, Error **errp)
> +{
> +    FlexSpiState *s = FLEXSPI(dev);
> +
> +    if (s->mmap_size) {
> +        memory_region_init_ram(&s->mem, OBJECT(s), DEVICE(s)->id, s->mmap_size,
> +                               NULL);
> +        memory_region_add_subregion(get_system_memory(), s->mmap_base, 
> &s->mem);

Where is this region used?

> +    }
> +}
> +
> +static void flexspi_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = flexspi_reset;
> +    dc->realize = flexspi_realize;
> +    device_class_set_props(dc, flexspi_properties);
> +}
> +
> +static const TypeInfo flexspi_info = {
> +    .name          = TYPE_FLEXSPI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(FlexSpiState),
> +    .instance_init = flexspi_init,
> +    .class_init    = flexspi_class_init,
> +};
> +
> +static void flexspi_register_types(void)
> +{
> +    int i;
> +
> +    for (i = 0; i < 32; i++) {
> +        wr_mask[regi(TFDR[i])] = BITS(31, 0);
> +    }
> +    for (i = 0; i < 64; i++) {
> +        wr_mask[regi(LUT[i])] = BITS(31, 0);
> +    }
> +
> +    type_register_static(&flexspi_info);
> +}
> +
> +type_init(flexspi_register_types)
> diff --git a/hw/ssi/meson.build b/hw/ssi/meson.build
> index 57d3e14727..c5b7e0a6e2 100644
> --- a/hw/ssi/meson.build
> +++ b/hw/ssi/meson.build
> @@ -13,3 +13,4 @@ system_ss.add(when: 'CONFIG_OMAP', if_true: 
> files('omap_spi.c'))
>   system_ss.add(when: 'CONFIG_IBEX', if_true: files('ibex_spi_host.c'))
>   system_ss.add(when: 'CONFIG_BCM2835_SPI', if_true: files('bcm2835_spi.c'))
>   system_ss.add(when: 'CONFIG_FLEXCOMM', if_true: files('flexcomm_spi.c'))
> +system_ss.add(when: 'CONFIG_FLEXSPI', if_true: files('flexspi.c'))
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> index 5caa1c17ac..d623022a79 100644
> --- a/hw/ssi/trace-events
> +++ b/hw/ssi/trace-events
> @@ -40,3 +40,7 @@ flexcomm_spi_fifostat(const char *id, uint32_t fifostat, uint32_t 
> fifoinstat) "%
>   flexcomm_spi_irq(const char *id, bool irq, bool fifoirqs, bool perirqs, bool enabled) 
> "%s: %d %d %d %d"
>   flexcomm_spi_chr_rx_space(const char *id, uint32_t rx) "%s: %d"
>   flexcomm_spi_chr_rx(const char *id) "%s"
> +
> +# flexspi.c
> +flexspi_reg_read(const char *id, const char *reg_name, uint32_t addr, uint32_t val) " 
> %s: %s[0x%04x] -> 0x%08x"
> +flexspi_reg_write(const char *id, const char *reg_name, uint32_t addr, uint32_t val) 
> "%s: %s[0x%04x] <- 0x%08x"
> diff --git a/include/hw/ssi/flexspi.h b/include/hw/ssi/flexspi.h
> new file mode 100644
> index 0000000000..f5fea9dee9
> --- /dev/null
> +++ b/include/hw/ssi/flexspi.h
> @@ -0,0 +1,34 @@
> +/*
> + * QEMU model for FLEXSPI
> + *
> + * Copyright (c) 2024 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef HW_RT500_FLEXSPI_H
> +#define HW_RT500_FLEXSPI_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/ssi/ssi.h"
> +#include "hw/arm/svd/flexspi.h"
> +
> +#define TYPE_FLEXSPI "flexspi"
> +#define FLEXSPI(obj) OBJECT_CHECK(FlexSpiState, (obj), TYPE_FLEXSPI)
> +
> +typedef struct {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion mmio;
> +    FLEXSPI_Type regs;
> +    MemoryRegion mem;
> +    uint32_t mmap_base;
> +    uint32_t mmap_size;

We usually use uint64_t for MR base/size. I.e., althought your SoC
address space is 32-bit, this model could be use in another one,
mapped at a 64-bit base.

> +} FlexSpiState;
> +
> +#endif /* HW_RT500_FLEXSPI_H */