On Wed, Aug 7, 2024 at 10:11 PM Philippe Mathieu-Daudé
<phi...@linaro.org> wrote:
>
> Hi Octavian,
>
Hi Philippe,
Thanks for the review!
> 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 <ta...@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
> },
>
Noted. I will switch to use this approach where possible.
Note that flexcom_spi allows 8/16/32 bit access on some registers, so
I would still like to keep the reg32_aligned_access() method for that
case, unless there is a better option.
> > +};
> > +
> > +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?
>
These regions are enabled in rt500.c when instantiating the flexspi
peripherals. As implemented now they are backed by RAM, the full
implementation should translate accesses to spi commands to FLASH or
PSRAM devices.
We need the memory regions because even the simplest NXP SDK examples
are using the memory mapped flexspi0 region.
> > + }
> > +}
> > +
> > +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.
>
I will fix it, thanks.
> > +} FlexSpiState;
> > +
> > +#endif /* HW_RT500_FLEXSPI_H */
>
