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

[Peter Maydell]

On 29 September 2011 18:31, Mathieu Sonet <address@hidden> wrote:
> This driver emulates the ARM AACI interface (PL041) connected to a LM4549
> codec.
> It enables audio playback for the Versatile/PB platform.
>
> Limitations:
> - Supports only a playback on one channel (Versatile/Vexpress)
> - Supports only a TX FIFO in compact-mode.

Actually you seem to have implemented a weird hybrid of compact
and non-compact modes.

Non-compact mode: FIFOs are 20 bits wide; a word write from the
CPU writes to bits [19:0] of the FIFO slot; each 'slot' of data
to the LM4549 reads 20 bits from the FIFO.
Compact mode: FIFOs are 40 bits wide (and half as deep); a word
write from the CPU writes to bits [31:0] of the FIFO slot; each
'slot' of data to the LM4549 reads 16 bits from the FIFO (bits
[15:0] then [31:16], and you have to read two slots (ie the
2 channels of stereo).

You've implemented a single 32 bit depth FIFO which one CPU
write writes to and which always passes one 32 bit word to the
LM4549. There are two issues here:
 (1) the LM4549 can take up to 18 bits of data per slot,
which your current lm4549_write_sample() API doesn't allow.
Passing two 32 bit words here would fix that. [My point here
is that we shouldn't hardwire the missing non-compact mode
support into the API between the two components.]
 (2) when the Linux driver dynamically identifies the size
of the FIFO it does it in non-compact mode, so it will return
a value half as big as is actually right for the compact-mode
behaviour you've implemented.

(Also your FIFO is twice as deep as it should be if we're
only implementing compact mode.)

> Playback tested successfully with speaker-test/aplay/mpg123.

I find that on vexpress mpg123 playback works but can be quite stuttery.
madplay (integer only) is somewhat better, so I suspect that qemu is
spending ages emulating Neon/VFP in mpg123...

Further (minor) comments below.

> +    regfile[LM4549_PCM_Front_DAC_Rate]  = 0xBB80;
> +    regfile[LM4549_PCM_ADC_Rate]        = 0xBB80;
> +    regfile[LM4549_Vendor_ID1]          = 0x4e53;
> +    regfile[LM4549_Vendor_ID2]          = 0x4331;

Can we be consistent about upper or lower case for the hex?

> +#define MAX_FIFO_DEPTH                 (1024)
> +#define VERSATILEPB_DEFAULT_FIFO_DEPTH (256)  /* AN115B - Table 1.1 */

pl041.c shouldn't know anything about VersatilePB. The default
fifo depth should be 8 (same as the hardware PL041).

> +static uint8_t pl041_compute_periphid3(pl041_state *s)
> +{
> +    uint8_t id3 = (1 | ((s->fifo_depth >> 4) << 3));
> +    return id3;
> +}

This isn't right.

[5:3] FIFO depth (non-compact mode)
b000  8
b001  16
b010  32
b011  64
b100  128
b101  256
b110  512
b111  1024

...which isn't what your function calculates.
(Linux determines FIFO depth programmatically by stuffing words
into the FIFO until the status register says it's full, which is
why it doesn't complain.)

NB that some of the board TRMs have what seem to be incorrect
labelling on the tables of depth vs ID register bits.

> +    /* Update the irq state */
> +    qemu_set_irq(s->irq, ((s->regs.isr1 & s->regs.ie1) > 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);
> +}

This debug printf won't compile if enabled -- you forgot
to update it when you changed the main code to remove the
'mask' variable.

> +static int pl041_post_load(void *opaque, int version_id)
> +{
> +    pl041_state *s = opaque;
> +    lm4549_post_load(&s->codec);
> +    return 0;
> +}

Is it not possible to just register lm4549_post_load()
as the post_load function for lm4549_state, rather than
having a pl041 post_load hook which only passes it through?

(Something in your mailsending path is wrapping long lines, by the way.)

-- PMM