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

[Peter Crosthwaite]

On Tue, Oct 23, 2012 at 2:12 AM, Peter Maydell <address@hidden> wrote:
> 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).
>

Annoyingly X is the most descriptive name for what it is if you go by
the TRM. We could call it "variant" or some such?

>> +} 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.)
>

Im implementing register features as I discover my guest software
needs them. This was first cab off the rank. I think you are right in
that it should come from the other side - as nand io happens this is
captured in the device state at the time. Reads then just read the
state out with no side effects.

> The dynamic cast seems a bit ugly.
>

Will go away with above soln.

>> +        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" ?
>

Hangover from an out of date implementation. Will fix.

>> +     * 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?
>

Yes

> 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?

Prefer not, rather say its not supported at all. The board just
attaches devices via QOM and qdev_init for this device figures it out.
Im trying to abstract away to need for the board model to manage
memory mappings that are device specific. But until this model gets
some more attention that approach could be made to work.

(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).
>

I dont think so. The register interface does not radically change with
x, so I think the discrepancies can be handled by ifery on x rather
than having seperate implementation for the control region for
different x.

>> +    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 :-)
>

Yeh links have to exist before qdev_init(). this gets called at
qdev_create(), machine models get a chance to set links then when
qdev_init() happens the links are there.

>> +
>> +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.
>

Dont think so. too much annoying to maintain copy-pasted code. (Four
VMSDs etc). I think parametrisation of x or "variant" can be made to
work with some polish and QOMs RTTI can be used to cast and talk to
devices.

Ill action the other review comments that ive no-replied too.

Regards,
Peter
> -- PMM
>