On 8 February 2013 03:42, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> Device model for Primecell PL330 dma controller.
> +typedef struct PL330 PL330;
This struct and typedef should be named "PL330State" -- "PL330" is
needed for the name of the standard QOM cast macro (see below).
> +/* Initialize queue */
> +static void pl330_queue_init(PL330Queue *s, int size, int channum, PL330
> *parent)
WARNING: line over 80 characters
#518: FILE: hw/pl330.c:476:
+static void pl330_queue_init(PL330Queue *s, int size, int channum,
PL330 *parent)
> +/* Put instruction to queue.
"in queue".
> + * Return value:
> + * - zero - OK
> + * - non-zero - queue is full
> + */
> +
> +static int pl330_queue_put_insn(PL330Queue *s, uint32_t addr,
> + int len, int n, bool inc, bool z, uint8_t tag,
> + PL330Chan *c)
I'm not sure what your indenting rule for multi-line function
prototype/definitions is, but I would suggest that the subsequent
lines should all line up with the first (so 'int' and 'PL330Chan'
here line up with 'PL330Queue').
> +static const MemoryRegionOps pl330_ops = {
> + .read = pl330_iomem_read,
> + .write = pl330_iomem_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
Are you sure this shouldn't be DEVICE_NATIVE_ENDIAN ?
> + .impl = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + }
> +};
> +
> +/* Controller logic and initialization */
> +
> +static void pl330_chan_reset(PL330Chan *ch)
> +{
> + ch->src = 0;
> + ch->dst = 0;
> + ch->pc = 0;
> + ch->state = pl330_chan_stopped;
> + ch->watchdog_timer = 0;
> + ch->stall = 0;
> + ch->control = 0;
> + ch->status = 0;
> + ch->fault_type = 0;
> +}
> +
> +static void pl330_reset(DeviceState *d)
> +{
> + int i;
> + PL330 *s = FROM_SYSBUS(PL330, SYS_BUS_DEVICE(d));
FROM_SYSBUS is deprecated. This should be
PL330 *s = PL330(d);
where PL330() is the standard QOM macro:
#define TYPE_PL330 "pl330"
#define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)
(and use TYPE_PL330 rather than "pl330" in the TypeInfo .name
field.)
> +static void pl330_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = pl330_init;
Instead of a SysBusDeviceClass::init function, current QOM
best practice is to have an Object::instance_init function
and a Device::realize function.
Instance init has this prototype:
static void pl330_init(Object *obj)
and you set it with
.instance_init = pl330_init,
in the TypeInfo struct.
Realize has this prototype:
static void pl330_realize(DeviceState *dev, Error **errp)
and you set it with
dc->realize = pl330_realize;
in the class init fn.
instance_init runs when the object is first created, and
must not fail. realize runs after all object properties
have been set [ie at qdev_init() time]; it may fail and
should do so via the Error** parameter.
The instance init function should do as much as possible,
but it cannot do anything which (a) relies on the values
of user set properties or (b) might fail. Those things
must be postponed to the realize function.
> + dc->reset = pl330_reset;
> + dc->props = pl330_properties;
> + dc->vmsd = &vmstate_pl330;
> +}
> +
> +static const TypeInfo pl330_type_info = {
> + .name = "pl330",
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(PL330),
> + .class_init = pl330_class_init,
> +};
Looks OK otherwise. I stuck a pl330 in the vexpress-a15
and Linux didn't blow up (but I'm not sure if Linux
will try to prod it either). I assume you've got some
more serious test cases :-)
-- PMM
