From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48767) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RCz0a-00048j-FK for qemu-devel@nongnu.org; Sun, 09 Oct 2011 15:26:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RCz0Z-0007QP-2U for qemu-devel@nongnu.org; Sun, 09 Oct 2011 15:26:00 -0400 Received: from caiajhbdcbbj.dreamhost.com ([208.97.132.119]:44414 helo=homiemail-a12.g.dreamhost.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RCz0Y-0007QF-O0 for qemu-devel@nongnu.org; Sun, 09 Oct 2011 15:25:59 -0400 Message-ID: <4E91F53F.9080606@elasticsheep.com> Date: Sun, 09 Oct 2011 21:25:51 +0200 From: Mathieu Sonet MIME-Version: 1.0 References: <4E84AB63.3090709@elasticsheep.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] Add AACI audio playback support to the ARM Versatile/PB platform List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, Paul Brook Peter Maydell wrote: > On 29 September 2011 18:31, Mathieu Sonet 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