Hi Peter,
All comments addressed. V10 on list soon:
changed from v9:
s/dma/DMA in patch subject
s/PL330/PL330State/ (PMM review)
Fixed 80 char violation (PMM review)
Fixed pl330_queue_put_insn prototype indentation (PMM review)
s/DEVICE_LITTLE_ENDIAN/DEVICE_NATIVE_ENDIAN (PMM review)
Made more QOM savvy (PMM review):
Define and use TYPE_PL330
Added PL330() type cast macro
converted sysbus init fn to a device realize fn
On Tue, Feb 19, 2013 at 2:30 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> 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).
>
s/PL330/PL330State
>> +/* 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)
>
Fixed
>> +/* Put instruction to queue.
>
> "in queue".
>
Fixed
>> + * 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').
>
Fixed as suggested
>> +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 ?
>
Change to 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);
>
Fixed globally
> where PL330() is the standard QOM macro:
>
> #define TYPE_PL330 "pl330"
> #define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)
>
Added
> (and use TYPE_PL330 rather than "pl330" in the TypeInfo .name
> field.)
>
Done
>> +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.
>
I took the conservative approach and left everything as a realize.
There arent any task in there that strike me as appropriate for early
init (e.g. no child creations or link setting etc), its all the
standard sysbus-foo which is generally property dependent. I didn't
want to mix and match, with some sysbus-init-foo in init() and some in
realize() so opted for all in realize.
>> + 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).
Very doubtful. My linux test case is highly ad-hoc as it is. Clean
generic PL330 kernel support is on the drawing board and I think
Samsung/Exynos are leading the effort there kernel side.
I assume you've got some
> more serious test cases :-)
>
An ancient Zynq Kernel built from the XIlinx tree. Does the job however.
Regards,
Peter
> -- PMM
>
