Re: [Qemu-devel] [PATCH v9 2/3] pl330: Initial version

[Peter Crosthwaite]

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 <address@hidden> wrote:
> On 8 February 2013 03:42, Peter Crosthwaite
> <address@hidden> 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
>