Re: [Qemu-devel] [PATCH v2 4/6] hw: Model of Primecell pl35x mem controller

[Peter Maydell]

On 22 October 2012 08:19, Peter Crosthwaite
<address@hidden> wrote:
> Initial device model for the pl35x series of memory controllers. The SRAM
> interface is just implemented as a passthrough using memory regions. NAND
> interfaces are modelled.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> changed since v1:
> use sysbus_mmio_get_region() for SRAM mappings (PMM Review)
> fixed header comment s/pl353/pl35x
> fixed complie warnings in debug mode (-DPL35X_DEBUG)
>
>  default-configs/arm-softmmu.mak |    1 +
>  hw/Makefile.objs                |    1 +
>  hw/pl35x.c                      |  299 
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+), 0 deletions(-)
>  create mode 100644 hw/pl35x.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 2f1a5c9..b24bf68 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -41,6 +41,7 @@ CONFIG_PL110=y
>  CONFIG_PL181=y
>  CONFIG_PL190=y
>  CONFIG_PL310=y
> +CONFIG_PL35X=y
>  CONFIG_CADENCE=y
>  CONFIG_XGMAC=y
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 854faa9..502f139 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -88,6 +88,7 @@ common-obj-$(CONFIG_PL110) += pl110.o
>  common-obj-$(CONFIG_PL181) += pl181.o
>  common-obj-$(CONFIG_PL190) += pl190.o
>  common-obj-$(CONFIG_PL310) += arm_l2x0.o
> +common-obj-$(CONFIG_PL35X) += pl35x.o
>  common-obj-$(CONFIG_VERSATILE_PCI) += versatile_pci.o
>  common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
>  common-obj-$(CONFIG_CADENCE) += cadence_uart.o
> diff --git a/hw/pl35x.c b/hw/pl35x.c
> new file mode 100644
> index 0000000..0f8c5ed
> --- /dev/null
> +++ b/hw/pl35x.c
> @@ -0,0 +1,299 @@
> +/*
> + * QEMU model of Primcell PL35X family of memory controllers

"Primecell"

> + *
> + * Copyright (c) 2012 Xilinx Inc.
> + * Copyright (c) 2012 Peter Crosthwaite <address@hidden>.
> + * Copyright (c) 2011 Edgar E. Iglesias.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw.h"
> +#include "qemu-timer.h"
> +#include "sysbus.h"
> +#include "sysemu.h"
> +#include "flash.h"
> +
> +#ifdef PL35X_ERR_DEBUG
> +#define DB_PRINT(...) do { \
> +    fprintf(stderr,  ": %s: ", __func__); \
> +    fprintf(stderr, ## __VA_ARGS__); \
> +    } while (0);
> +#else
> +    #define DB_PRINT(...)
> +#endif
> +
> +typedef struct PL35xItf {
> +    MemoryRegion mm;
> +    DeviceState *dev;
> +    uint8_t nand_pending_addr_cycles;
> +} PL35xItf;
> +
> +typedef struct PL35xState {
> +    SysBusDevice busdev;
> +    MemoryRegion mmio;
> +
> +    /* FIXME: add support for multiple chip selects/interface */
> +
> +    PL35xItf itf[2];
> +
> +    /* FIXME: add Interrupt support */
> +
> +    /* FIXME: add ECC support */
> +
> +    uint8_t x; /* the "x" in pl35x */

This field needs a better name (and as a device property it
desperately needs a better name).

> +} PL35xState;
> +
> +static uint64_t pl35x_read(void *opaque, target_phys_addr_t addr,
> +                         unsigned int size)
> +{
> +    PL35xState *s = opaque;
> +    uint32_t r = 0;
> +    int rdy;
> +
> +    addr >>= 2;
> +    switch (addr) {
> +    case 0x0:
> +        if (s->itf[0].dev && object_dynamic_cast(OBJECT(s->itf[0].dev),
> +                                                      "nand")) {
> +            nand_getpins(s->itf[0].dev, &rdy);
> +            r |= (!!rdy) << 5;
> +        }
> +        if (s->itf[1].dev && object_dynamic_cast(OBJECT(s->itf[1].dev),
> +                                                      "nand")) {
> +            nand_getpins(s->itf[1].dev, &rdy);
> +            r |= (!!rdy) << 6;
> +        }

These are the raw interrupt status bits; this code will probably
need to be rewritten when interrupt support is added. (You aren't
implementing the non-raw status bits or the enables for example,
and we might not want to be doing a nand_getpins query only on the
register read.)

The dynamic cast seems a bit ugly.

> +        break;
> +    default:
> +        DB_PRINT("Unimplemented SMC read access reg=" TARGET_FMT_plx "\n",
> +                 addr * 4);

You can use qemu_log_mask's LOG_UNIMP and LOG_GUEST_ERROR for this now.

> +        break;
> +    }
> +    return r;
> +}
> +
> +static void pl35x_write(void *opaque, target_phys_addr_t addr, uint64_t 
> value64,
> +                      unsigned int size)
> +{
> +    DB_PRINT("addr=%x v=%x\n", (unsigned)addr, (unsigned)value64);
> +    addr >>= 2;
> +    /* FIXME: implement */
> +    DB_PRINT("Unimplemented SMC write access reg=" TARGET_FMT_plx "\n",
> +                 addr * 4);
> +}
> +
> +static const MemoryRegionOps pl35x_ops = {
> +    .read = pl35x_read,
> +    .write = pl35x_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static uint64_t nand_read(void *opaque, target_phys_addr_t addr,
> +                           unsigned int size)
> +{
> +    PL35xItf *s = opaque;
> +    unsigned int len = size;
> +    int shift = 0;
> +    uint32_t r = 0;
> +
> +    while (len--) {
> +        uint8_t r8;
> +
> +        r8 = nand_getio(s->dev) & 0xff;
> +        r |= r8 << shift;
> +        shift += 8;
> +    }
> +    DB_PRINT("addr=%x r=%x size=%d\n", (unsigned)addr, r, size);
> +    return r;
> +}
> +
> +static void nand_write(void *opaque, target_phys_addr_t addr, uint64_t 
> value64,
> +                       unsigned int size)
> +{
> +    struct PL35xItf *s = opaque;
> +    bool data_phase, ecmd_valid;
> +    unsigned int addr_cycles = 0;
> +    uint16_t start_cmd, end_cmd;
> +    uint32_t value = value64;
> +    uint32_t nandaddr = value;
> +
> +    DB_PRINT("addr=%x v=%x size=%d\n", (unsigned)addr, (unsigned)value, 
> size);
> +
> +    /* Decode the various signals.  */
> +    data_phase = (addr >> 19) & 1;
> +    ecmd_valid = (addr >> 20) & 1;
> +    start_cmd = (addr >> 3) & 0xff;
> +    end_cmd = (addr >> 11) & 0xff;
> +    if (!data_phase) {
> +        addr_cycles = (addr >> 21) & 7;
> +    }
> +
> +    if (!data_phase) {
> +        DB_PRINT("start_cmd=%x end_cmd=%x (valid=%d) acycl=%d\n",
> +                start_cmd, end_cmd, ecmd_valid, addr_cycles);
> +    }
> +
> +    /* Writing data to the NAND.  */
> +    if (data_phase) {
> +        nand_setpins(s->dev, 0, 0, 0, 1, 0);
> +        while (size--) {
> +            nand_setio(s->dev, value & 0xff);
> +            value >>= 8;
> +        }
> +    }
> +
> +    /* Writing Start cmd.  */
> +    if (!data_phase && !s->nand_pending_addr_cycles) {
> +        nand_setpins(s->dev, 1, 0, 0, 1, 0);
> +        nand_setio(s->dev, start_cmd);
> +    }
> +
> +    if (!addr_cycles) {
> +        s->nand_pending_addr_cycles = 0;
> +    }
> +    if (s->nand_pending_addr_cycles) {
> +        addr_cycles = s->nand_pending_addr_cycles;
> +        s->nand_pending_addr_cycles = 0;
> +    }
> +    if (addr_cycles > 4) {
> +        s->nand_pending_addr_cycles = addr_cycles - 4;
> +        addr_cycles = 4;
> +    }
> +    while (addr_cycles) {
> +        nand_setpins(s->dev, 0, 1, 0, 1, 0);
> +        DB_PRINT("nand cycl=%d addr=%x\n", addr_cycles, nandaddr & 0xff);
> +        nand_setio(s->dev, nandaddr & 0xff);
> +        nandaddr >>= 8;
> +        addr_cycles--;
> +    }
> +
> +    /* Writing commands. One or two (Start and End).  */
> +    if (ecmd_valid && !s->nand_pending_addr_cycles) {
> +        nand_setpins(s->dev, 1, 0, 0, 1, 0);
> +        nand_setio(s->dev, end_cmd);
> +    }
> +}
> +
> +static const MemoryRegionOps nand_ops = {
> +    .read = nand_read,
> +    .write = nand_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static void pl35x_init_sram(SysBusDevice *dev, PL35xItf *itf)
> +{
> +    /* d Just needs to be a valid sysbus device with at least one memory

"d" ?

> +     * region
> +     */
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(itf->dev);
> +
> +    memory_region_init(&itf->mm, "pl35x.sram", 1 << 24);
> +    memory_region_add_subregion(&itf->mm, 0, sysbus_mmio_get_region(sbd, 0));
> +    sysbus_init_mmio(dev, &itf->mm);
> +}
> +
> +static void pl35x_init_nand(SysBusDevice *dev, PL35xItf *itf)
> +{
> +    /* d Must be a NAND flash */
> +    object_dynamic_cast_assert(OBJECT(itf->dev), "nand");
> +
> +    memory_region_init_io(&itf->mm, &nand_ops, itf, "pl35x.nand", 1 << 24);
> +    sysbus_init_mmio(dev, &itf->mm);
> +}

So we're only supporting a single chipselect on each interface at the
moment, is that right?

And we do not try to model the addr_mask/addr_match tieoffs but instead
let the board do the equivalent with memory region aliases/containers
to put things in the right place? (That makes sense to me -- it's not
guest visible so there's no need to implement the complication when we
can just use the qemu standard functions for the same purpose.)

> +static int pl35x_init(SysBusDevice *dev)
> +{
> +    PL35xState *s = FROM_SYSBUS(typeof(*s), dev);
> +    int itfn = 0;
> +
> +    memory_region_init_io(&s->mmio, &pl35x_ops, s, "pl35x_io", 0x1000);

Are we going to want different implementations of any of these registers
depending on whether NAND or SRAM or neither? (I think we can get away
without, the chip config registers are just "data, reads back as written"
as far as we're concerned, I think).

> +    sysbus_init_mmio(dev, &s->mmio);
> +    if (s->x != 1) { /* everything cept PL351 has at least one SRAM */
> +        pl35x_init_sram(dev, &s->itf[itfn]);
> +        itfn++;
> +    }
> +    if (s->x & 0x1) { /* PL351 and PL353 have NAND */
> +        pl35x_init_nand(dev, &s->itf[itfn]);
> +    } else if (s->x == 4) { /* PL354 has a second SRAM */
> +        pl35x_init_sram(dev, &s->itf[itfn]);
> +    }
> +    return 0;
> +}

I think it would be cute to make this table driven, something like
(untested uncompiled code):

typedef void interface_initfn(SysBusDevice *dev, PL35xItf *itf);

static void pl35x_init_no_if(SysBusDevice *dev, PL35xItf *itf)
{
    /* Dummy init function for variants with no second memory interface */
}

/* Different config variants of the PL35x support different combinations
 * of SRAM and NAND on their memory interfaces 0 and 1.
 */
static interface_initfn** variant_if_table[] = {
   { pl35x_init_nand, pl35x_init_no_if }, /* PL351 */
   { pl35x_init_sram, pl35x_init_no_if }, /* PL352 */
   { pl35x_init_sram, pl35x_init_nand },  /* PL353 */
   { pl35x_init_sram, pl35x_init_sram },  /* PL354 */
};

then in pl35x_init you can do:
    variant_if_table[var][0](dev, &s->itf[0]);
    variant_if_table[var][1](dev, &s->itf[1]);

> +static void pl35x_initfn(Object *obj)
> +{
> +    PL35xState *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
> +    Error *errp = NULL;
> +
> +    object_property_add_link(obj, "dev0", TYPE_DEVICE,
> +                             (Object **)&s->itf[0].dev, &errp);
> +    assert_no_error(errp);
> +    object_property_add_link(obj, "dev1", TYPE_DEVICE,
> +                             (Object **)&s->itf[1].dev, &errp);
> +    assert_no_error(errp);
> +}

We have a sysbus init function and an instance init function?
that's confusing :-)

> +
> +static Property pl35x_properties[] = {
> +    DEFINE_PROP_UINT8("x", PL35xState, x, 3),

Apart from the name, I'm not totally convinced we just want
to make this property the last digit of the model name.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static const VMStateDescription vmstate_pl35x = {
> +    .name = "pl35x",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(itf[0].nand_pending_addr_cycles, PL35xState),
> +        VMSTATE_UINT8(itf[1].nand_pending_addr_cycles, PL35xState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pl35x_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pl35x_init;
> +    dc->props = pl35x_properties;
> +    dc->vmsd = &vmstate_pl35x;
> +}
> +
> +static TypeInfo pl35x_info = {
> +    .name           = "arm.pl35x",

All the other plxxx devices just name themselves
"pl110" or whatever, so "pl35x" here.

> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(PL35xState),
> +    .class_init     = pl35x_class_init,
> +    .instance_init  = pl35x_initfn,
> +};
> +
> +static void pl35x_register_types(void)
> +{
> +    type_register_static(&pl35x_info);
> +}
> +
> +type_init(pl35x_register_types)

The other approach to the various part variants of course would
be to just define four different QOM classes. I guess that would
let us avoid having dynamic properties. Would it be better? Dunno.

-- PMM