From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JlIfI-00087s-39 for qemu-devel@nongnu.org; Mon, 14 Apr 2008 02:59:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JlIfG-00085d-U0 for qemu-devel@nongnu.org; Mon, 14 Apr 2008 02:59:43 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JlIfG-00085N-Nu for qemu-devel@nongnu.org; Mon, 14 Apr 2008 02:59:42 -0400 Received: from mx20.gnu.org ([199.232.41.8]) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1JlIfG-0004yc-7X for qemu-devel@nongnu.org; Mon, 14 Apr 2008 02:59:42 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JlIfE-0002sj-RR for qemu-devel@nongnu.org; Mon, 14 Apr 2008 02:59:41 -0400 Received: from smtp05.web.de (fmsmtp05.dlan.cinetic.de [172.20.4.166]) by fmmailgate03.web.de (Postfix) with ESMTP id A1E96D700024 for ; Mon, 14 Apr 2008 08:59:38 +0200 (CEST) Received: from [88.64.24.193] (helo=[192.168.1.198]) by smtp05.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.109 #226) id 1JlIfC-0004KU-00 for qemu-devel@nongnu.org; Mon, 14 Apr 2008 08:59:38 +0200 Message-ID: <480300D2.2060400@web.de> Date: Mon, 14 Apr 2008 08:59:30 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <4801DC59.1010403@web.de> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig2059632180557193D3C40125" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig2059632180557193D3C40125 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable malc wrote: > On Sun, 13 Apr 2008, Jan Kiszka wrote: >=20 >> This is the board emulation for Freecom's MusicPal, featuring >> - rudimentary PIT and PIC >> - up to 2 UARTs >> - 88W8xx8 Ethernet controller >> - 88W8618 audio controller >> - Wolfson WM8750 mixer chip (volume control and mute only) >> - 128?64 display with brightness control >> - all input buttons >> > [..snip..] >=20 >> +static void sound_fill_mixer_buffer(mv88w8618_sound_state *s, >> unsigned int length) >> +{ >> + unsigned int pos; >> + double val; >> + >> + if (s->mute) { >> + memset(s->mixer_buffer, 0, length); >=20 > Zero is not correct "mute" value for signed formats. Hmm, maybe it's still too early for me - but what else?? >=20 >> + return; >> + } >> + >> + if (s->playback_mode & 1) >> + for (pos =3D 0; pos < length; pos +=3D 2) { >> + val =3D *(int16_t *)(s->target_buffer + s->play_pos + pos= ); >> + val =3D val * pow(10.0, s->volume/20.0); >> + *(int16_t *)(s->mixer_buffer + pos) =3D val; >> + } >=20 > On the surface it's sounds like endianess problems are ahead. Oh, yes. >=20 >> + else >> + for (pos =3D 0; pos < length; pos +=3D 1) { >> + val =3D *(int8_t *)(s->target_buffer + s->play_pos + pos)= ; >> + val =3D val * pow(10.0, s->volume/20.0); >> + *(int8_t *)(s->mixer_buffer + pos) =3D val; >> + } >> +} >=20 > There's actually a generic framework for volume manipulation in QEMUs > audio, unfortunatelly it's unconditionally disabled (define NOVOL in > audio/mixeng.c) for the reasons i can't recall now. Yeah, looked around a bit before hacking those loops but indeed found nothing ready to use. >=20 > Also adlib emulation is only conditionally available due to > usage of floating point arithmetics, maybe rules have changed now > though, but "fixing" this particular issue in this particular case > seems easy enough. Sorry, didn't get what you mean here. >=20 >> +static void sound_callback(void *opaque, int free) >> +{ >> + mv88w8618_sound_state *s =3D opaque; >> + uint32_t old_status =3D s->status; >> + int64_t now =3D qemu_get_clock(rt_clock); >> + unsigned int n; >> + >> + if (now - s->last_callback < (1000 * s->threshold/2) / (44100*2))= >> + return; >> + s->last_callback =3D now; >=20 > Here two clocks are being used for things - the audio one which influen= ces > `free' paramter and rt_clock, even if there are reasons why free could = not > be used choice of rt_clock over vm_clock seems wrong. Yes, using the monotonic vm_clock is better. Will change. Starring at the condition above, I realized that it is also bogus, in several ways. It should still cause far more interrupts than on the real device... >=20 >> + while (free > 0) { >> + n =3D audio_MIN(s->threshold - s->play_pos, (unsigned int)fre= e); >> + sound_fill_mixer_buffer(s, n); >> + n =3D AUD_write(s->voice, s->mixer_buffer, n); >> + if (!n) >> + break; >> + >> + s->play_pos +=3D n; >> + free -=3D n; >> + >> + if (s->play_pos >=3D s->threshold/2) >> + s->status |=3D 1 << 6; >> + if (s->play_pos =3D=3D s->threshold) { >> + s->status |=3D 1 << 7; >> + s->play_pos =3D 0; >> + break; >> + } >> + } >> + if ((s->status ^ old_status) & s->irq_enable) >> + qemu_irq_raise(s->irq); >=20 > While might be perfectly fine the above checking when to fire IRQ > looks suspicious. Both status and irq_enable use the same bit semantics, so playing with bit operations is indeed OK here. >=20 > [..snip..] >=20 >> +static void mv88w8618_sound_write(void *opaque, target_phys_addr_t >> offset, >> + uint32_t value) >> +{ >> + mv88w8618_sound_state *s =3D opaque; >> + >> + offset -=3D s->base; >> + switch (offset) { >> + case 0x00: /* Playback Mode */ >> + s->playback_mode =3D value; >> + if (value & (1 << 7)) { >> + audsettings_t as =3D {0, 2, 0, 0}; >=20 > Endianess set to 0 while the actual volume manipulations imply host byt= e > order.. Yes, will "officially" switch to host order. >=20 >> + if (value & (1 << 9)) >> + as.freq =3D (24576000 / 64) / (s->clock_div + 1); >> + else >> + as.freq =3D (11289600 / 64) / (s->clock_div + 1); >> + if (value & 1) >> + as.fmt =3D AUD_FMT_S16; >> + else >> + as.fmt =3D AUD_FMT_S8; >> + >> + s->voice =3D AUD_open_out(&s->card, s->voice, sound_name,= >> + s, sound_callback, &as); >> + if (s->voice) >> + AUD_set_active_out(s->voice, 1); >> + else >> + AUD_log(sound_name, "Could not open voice\n"); >> + s->last_callback =3D 0; >> + s->status =3D 0; >> + } else if (s->voice) >> + AUD_set_active_out(s->voice, 0); >> + break; >> + >> + case 0x18: /* Clock Divider */ >> + s->clock_div =3D (value >> 8) & 0xFF; >> + break; >> + >> + case 0x20: /* Interrupt Status */ >> + s->status &=3D ~value; >> + break; >> + >> + case 0x24: /* Interrupt Enable */ >> + s->irq_enable =3D value; >> + if (s->status & s->irq_enable) >> + qemu_irq_raise(s->irq); >=20 >> + break; >> + >> + case 0x28: /* Tx Start Low */ >> + s->phys_buf =3D (s->phys_buf & 0xFFFF0000) | (value & 0xFFFF)= ; >=20 > [..snip..] >=20 Thanks a lot for your review! Jan --------------enig2059632180557193D3C40125 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4-svn0 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iD8DBQFIAwDZniDOoMHTA+kRAofxAJwLd1n7Uq+w3/IAJs4SWruXOYwHUgCeIg+7 Bq74MdW0hmriJPyte0eNYb4= =FY7l -----END PGP SIGNATURE----- --------------enig2059632180557193D3C40125--