Re: [Qemu-devel] [PATCH v2] Add AACI audio playback support to the ARM Versatile/PB platform

[Peter Maydell]

On 13 May 2011 23:08, Mathieu Sonet <address@hidden> wrote:
[PL041/AACI patches]

I apologise for the exceedingly delayed review here.

So to start with, I tested this patch with the vexpress-a9
board (by adding the one line to add a pl041 to it) and it
works, which is really cool.

A minor note on patch formatting: since the bit of the email up
to the '---' goes into the git commit history, it should be a
description of this version of the patch rather than a list
of the differences to the previous version. (I usually put the
"v1->v2" summary after the '---' before the diffstat.)

Moving on to the code review...

> +/*** Functions ***/

I'm not a fan of comments which state the obvious.

> +
> +static void lm4549_store_init(lm4549_state *s, uint32_t offset, uint32_t 
> value,
> +                              uint32_t is_read_only)

Making value a uint16_t would be consistent with lm4549_store and _load.

> +{
> +    lm4549_registers *r = &s->codec_regs;
> +
> +    if (offset > 128) {
> +        DPRINTF("store_init: Out of bound offset 0x%x\n", (int)offset);
> +    }

You need a return here, otherwise we blunder on and access the
array out of bounds. (Or you could just assert(), since we only
use this function internally.)

> +    r->data[offset].value = value & 0xFFFF;
> +    r->data[offset].default_value = value & 0xFFFF;

These "& 0xFFFF" aren't necessary.

> +static void lm4549_store(lm4549_state *s, uint32_t offset, uint16_t value)

To be honest I'm not entirely convinced of the necessity of
abstracting out the register file into these _load/_store/_reset
functions rather than just having lm4549_read and _write access
the array directly. (eg since there are only 3 read-only registers
it would be simpler just to have lm4549_write's switch statement have:
 case LM4549_Extended_Audio_ID:
 case LM4549_Vendor_ID1:
 case LM4549_Vendor_ID2:
     /* Read-only registers */
     break;

rather than having all the machinery for the read_only field in
the lm4549_registers struct. Similarly if you just handle the
reset values in a reset function then your register array is
just a simple uint16_t registers[128].)

> +{
> +    lm4549_registers *r = &s->codec_regs;
> +
> +    if (offset > 128) {
> +        DPRINTF("store: Out of bound offset 0x%x\n", (int)offset);
> +    }

Needs a return again. (Also the case in lm4549_load.)

> +    r->data[offset].value = value & 0xFFFF;

Unnecessary mask.

> +    if (written_samples == s->buffer_level) {
> +        s->buffer_level = 0;
> +    } else {
> +        s->buffer_level -= written_samples;
> +
> +        if (s->buffer_level > 0) {
> +            /* Move the data back to the start of the buffer */
> +            for (i = 0; i < s->buffer_level; i++) {
> +                s->buffer[i] = s->buffer[i + written_samples];
> +            }
> +        }
> +    }

You can just delete the if () { ... } part here, because
if written_samples == s->buffer_level then the else {}
clause is equivalent in effect to s->buffer_level = 0.
[In fact "(s->buffer_level > 0)" can only be true if
written_samples != s->buffer_level, because this is all
unsigned arithmetic.]

> diff --git a/hw/pl041.c b/hw/pl041.c
> new file mode 100644
> index 0000000..123612c
> --- /dev/null
> +++ b/hw/pl041.c
> @@ -0,0 +1,406 @@
> +/*
> + * Arm PrimeCell PL041 Advanced Audio Codec Interface
> + *
> + * Copyright (c) 2011
> + * Written by Mathieu Sonet - www.elasticsheep.com
> + *
> + * This code is licenced under the GPL.
> + *
> + * *****************************************************************
> + *
> + * This driver emulates the ARM AACI interface
> + * connected to a LM4549 codec.
> + *
> + */
> +
> +#include "sysbus.h"
> +
> +#include "pl041.h"
> +#include "lm4549.h"
> +
> +/*** Debug macros ***/
> +
> +/* #define PL041_DEBUG_LEVEL 1 */
> +
> +#if defined(PL041_DEBUG_LEVEL) && (PL041_DEBUG_LEVEL >= 1)
> +#define DBG_L1(fmt, ...) \
> +do { printf("pl041: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DBG_L1(fmt, ...) \
> +do { } while (0)
> +#endif
> +
> +#if defined(PL041_DEBUG_LEVEL) && (PL041_DEBUG_LEVEL >= 2)
> +#define DBG_L2(fmt, ...) \
> +do { printf("pl041: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DBG_L2(fmt, ...) \
> +do { } while (0)
> +#endif
> +
> +/*** Constants ***/
> +
> +#define FIFO_DEPTH  16

The FIFOs in the pl041 in the various ARM dev boards
are actually larger than the stock pl041 FIFO. Possibly
FIFO size should be a qdev property (does changing it
have a (good or bad) effect on the model pl041's latency
or tendency to dropouts?).

> +#define SLOT1_RW    (1 << 19)
> +
> +/*** Types ***/
> +
> +typedef struct {
> +    uint32_t size;
> +    uint32_t half;
> +    uint32_t level;
> +    uint32_t data[FIFO_DEPTH];
> +} pl041_fifo;

FIFO size is a property of the pl041, not per-FIFO, so
it doesn't belong in this struct. Also, there's not much
point in just caching size / 2 as 'half'.

> +
> +typedef struct {
> +    SysBusDevice busdev;
> +    qemu_irq irq;
> +    pl041_regfile regs;
> +    pl041_fifo fifo1;
> +    lm4549_state codec;
> +} pl041_state;

...this is implicitly modelling the versatile/vexpress
modified PL041 which only has one channel -- stock PL041
has four channels with a fifo each.

> +/*** Globals ***/
> +
> +static const unsigned char pl041_id[8] = {
> +    0x41, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
> +};

The modified PL041s in the versatilepb, vexpress etc
have slightly different ID values (the FIFO depth and
number of channels are encoded in one of the bytes).

> +#if defined(PL041_DEBUG_LEVEL)
> +#define REGISTER(name, offset) #name,
> +static const char *pl041_regs_name[] = {
> +    #include "pl041.hx"
> +};
> +#undef REGISTER
> +#endif
> +
> +/*** Functions ***/
> +
> +#if defined(PL041_DEBUG_LEVEL)
> +static const char *get_reg_name(target_phys_addr_t offset)

Minor point but "pl041_reg_name()" would be a better name I think.

> +{
> +    if (offset <= 0xAC) {

Use sizeof() rather than a magic constant. Returning
"unknown" if the pl041_regs_name[] array entry is NULL
would also be more robust since it means we don't rely
on having a REGISTER() entry for each offset.

> +        return pl041_regs_name[offset >> 2];
> +    }
> +
> +    return "unknown";
> +}
> +#endif
> +
> +static void pl041_fifo1_push(pl041_state *s, uint32_t value)
> +{
> +    pl041_fifo *f = &s->fifo1;
> +
> +    /* Check the FIFO level */
> +    if (f->level >= f->size) {
> +        hw_error("fifo1 push: overrun\n");
> +    }

Calling hw_error() on a FIFO overrun looks wrong. hw_error() will
make qemu call abort() so as a general rule a device model should
not use it for any condition the guest can provoke.

> +static void pl041_isr1_update(pl041_state *s)
> +{
> +    uint32_t mask = 0;
> +
> +    /* Update ISR1 */
> +    if (s->regs.sr1 & TXUNDERRUN) {
> +        s->regs.isr1 |= URINTR;
> +    } else {
> +        s->regs.isr1 &= ~URINTR;
> +    }
> +
> +    if (s->regs.sr1 & TXHE) {
> +        s->regs.isr1 |= TXINTR;
> +    } else {
> +        s->regs.isr1 &= ~TXINTR;
> +    }
> +
> +    if (!(s->regs.sr1 & TXBUSY) && (s->regs.sr1 & TXFE)) {
> +        s->regs.isr1 |= TXCINTR;
> +    } else {
> +        s->regs.isr1 &= ~TXCINTR;
> +    }
> +
> +    /* Set the irq mask */
> +    if (s->regs.ie1 & TXUIE) {
> +        mask |= URINTR;
> +    }
> +
> +    if (s->regs.ie1 & TXIE) {
> +        mask |= TXINTR;
> +    }
> +
> +    if (s->regs.ie1 & TXCIE) {
> +        mask |= TXCINTR;
> +    }

The IEx mask bits are deliberately arranged to match the
ISRx bits, so you don't need to construct the mask like
this, you can just directly use s->regs.ie1 as your mask.

> +    /* Update the irq state */
> +    qemu_set_irq(s->irq, ((s->regs.isr1 & mask) > 0) ? 1 : 0);
> +    DBG_L2("Set interrupt sr1 = 0x%08x isr1 = 0x%08x masked = 0x%08x\n",
> +           s->regs.sr1, s->regs.isr1, s->regs.isr1 & mask);
> +}

> +static uint32_t pl041_read(void *opaque, target_phys_addr_t offset)
> +{
> +    pl041_state *s = (pl041_state *)opaque;
> +    int value;
> +
> +    if (offset >= 0xfe0 && offset < 0x1000) {
> +        DBG_L1("pl041_read [0x%08x]\n", offset);
> +        return pl041_id[(offset - 0xfe0) >> 2];
> +    }
> +
> +    if (offset < 0x110) {

Again, use sizeof().

> +        value = *((uint32_t *)&s->regs + (offset >> 2));
> +    } else {
> +        hw_error("pl041_read: Bad offset %x\n", (int)offset);

hw_error() here is definitely wrong. I'd suggest printing a
message if debugging is enabled and otherwise just read as
zero/writes ignored.

> +    }
> +
> +    switch (offset) {
> +    case PL041_allints:
> +        value = s->regs.isr1 & 0x7F;
> +        break;
> +    }

There's quite a lot of PL041 functionality unimplemented here;
I'm not going to ask you to implement it all, but a comment
at the top of the file somewhere giving a brief summary of
what isn't implemented would be good.

> +static int pl041_init(SysBusDevice *dev)
> +{
> +    pl041_state *s = FROM_SYSBUS(pl041_state, dev);
> +    int iomemtype;
> +
> +    DBG_L1("pl041_init\n");
> +
> +    /* Connect the device to the sysbus */
> +    iomemtype = cpu_register_io_memory(pl041_readfn,
> +                                       pl041_writefn,
> +                                       s,
> +                                       DEVICE_NATIVE_ENDIAN);
> +    sysbus_init_mmio(dev, 0x1000, iomemtype);
> +    sysbus_init_irq(dev, &s->irq);
> +
> +    /* Reset the device */
> +    pl041_reset(s);
> +
> +    /* Init the codec */
> +    lm4549_init(&s->codec, &pl041_request_data, (void *)s);
> +
> +    return 0;
> +}
> +
> +static void pl041_register_devices(void)
> +{
> +    sysbus_register_dev("pl041", sizeof(pl041_state), pl041_init);
> +}

This is missing save/load support, for which you need:
 (1) use sysbus_register_withprop() and pass it a SysBusDeviceInfo
 (2) write a VMStateDescription and pass it as the .qdev.vmsd
 field of your SysBusDeviceInfo

(When you do this you can then pass your reset routine as .qdev.reset
and you don't need to call it in your pl041_init() function. Watch
out that the .reset routine takes a DeviceState*.)

You can see an example of this in hw/pl190.c.

> +
> +device_init(pl041_register_devices)
> diff --git a/hw/pl041.h b/hw/pl041.h
> new file mode 100644
> index 0000000..ddf452d
> --- /dev/null
> +++ b/hw/pl041.h
> @@ -0,0 +1,125 @@
> +/*
> + * Arm PrimeCell PL041 Advanced Audio Codec Interface

This header is only used by hw/pl041.c so you might as well
just put all its contents in the .c file. (If you do leave
it as a separate header it needs #ifndef HW_PL041_H guards.)

-- PMM