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

[Mathieu Sonet <contact@elasticsheep.com>]

Peter Maydell wrote:
> On 29 September 2011 18:31, Mathieu Sonet <contact@elasticsheep.com> 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?

The lm4549 model has no knowledge of its pl041 client and uses a separate context structure.
The opaque pointer passed to the post_load hook can not be used directly to setup the lm4549.

I would prefer to keep the two models separated.

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

  Mathieu