[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / Musi
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal |
Date: |
Mon, 14 Apr 2008 08:59:30 +0200 |
User-agent: |
Thunderbird 2.0.0.12 (X11/20080226) |
malc wrote:
> On Sun, 13 Apr 2008, Jan Kiszka wrote:
>
>> 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..]
>
>> +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);
>
> Zero is not correct "mute" value for signed formats.
Hmm, maybe it's still too early for me - but what else??
>
>> + return;
>> + }
>> +
>> + if (s->playback_mode & 1)
>> + for (pos = 0; pos < length; pos += 2) {
>> + val = *(int16_t *)(s->target_buffer + s->play_pos + pos);
>> + val = val * pow(10.0, s->volume/20.0);
>> + *(int16_t *)(s->mixer_buffer + pos) = val;
>> + }
>
> On the surface it's sounds like endianess problems are ahead.
Oh, yes.
>
>> + else
>> + for (pos = 0; pos < length; pos += 1) {
>> + val = *(int8_t *)(s->target_buffer + s->play_pos + pos);
>> + val = val * pow(10.0, s->volume/20.0);
>> + *(int8_t *)(s->mixer_buffer + pos) = val;
>> + }
>> +}
>
> 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.
>
> 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.
>
>> +static void sound_callback(void *opaque, int free)
>> +{
>> + mv88w8618_sound_state *s = opaque;
>> + uint32_t old_status = s->status;
>> + int64_t now = qemu_get_clock(rt_clock);
>> + unsigned int n;
>> +
>> + if (now - s->last_callback < (1000 * s->threshold/2) / (44100*2))
>> + return;
>> + s->last_callback = now;
>
> Here two clocks are being used for things - the audio one which influences
> `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...
>
>> + while (free > 0) {
>> + n = audio_MIN(s->threshold - s->play_pos, (unsigned int)free);
>> + sound_fill_mixer_buffer(s, n);
>> + n = AUD_write(s->voice, s->mixer_buffer, n);
>> + if (!n)
>> + break;
>> +
>> + s->play_pos += n;
>> + free -= n;
>> +
>> + if (s->play_pos >= s->threshold/2)
>> + s->status |= 1 << 6;
>> + if (s->play_pos == s->threshold) {
>> + s->status |= 1 << 7;
>> + s->play_pos = 0;
>> + break;
>> + }
>> + }
>> + if ((s->status ^ old_status) & s->irq_enable)
>> + qemu_irq_raise(s->irq);
>
> 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.
>
> [..snip..]
>
>> +static void mv88w8618_sound_write(void *opaque, target_phys_addr_t
>> offset,
>> + uint32_t value)
>> +{
>> + mv88w8618_sound_state *s = opaque;
>> +
>> + offset -= s->base;
>> + switch (offset) {
>> + case 0x00: /* Playback Mode */
>> + s->playback_mode = value;
>> + if (value & (1 << 7)) {
>> + audsettings_t as = {0, 2, 0, 0};
>
> Endianess set to 0 while the actual volume manipulations imply host byte
> order..
Yes, will "officially" switch to host order.
>
>> + if (value & (1 << 9))
>> + as.freq = (24576000 / 64) / (s->clock_div + 1);
>> + else
>> + as.freq = (11289600 / 64) / (s->clock_div + 1);
>> + if (value & 1)
>> + as.fmt = AUD_FMT_S16;
>> + else
>> + as.fmt = AUD_FMT_S8;
>> +
>> + s->voice = 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 = 0;
>> + s->status = 0;
>> + } else if (s->voice)
>> + AUD_set_active_out(s->voice, 0);
>> + break;
>> +
>> + case 0x18: /* Clock Divider */
>> + s->clock_div = (value >> 8) & 0xFF;
>> + break;
>> +
>> + case 0x20: /* Interrupt Status */
>> + s->status &= ~value;
>> + break;
>> +
>> + case 0x24: /* Interrupt Enable */
>> + s->irq_enable = value;
>> + if (s->status & s->irq_enable)
>> + qemu_irq_raise(s->irq);
>
>> + break;
>> +
>> + case 0x28: /* Tx Start Low */
>> + s->phys_buf = (s->phys_buf & 0xFFFF0000) | (value & 0xFFFF);
>
> [..snip..]
>
Thanks a lot for your review!
Jan
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, Jan Kiszka, 2008/04/13
- Re: [Qemu-devel] [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, malc, 2008/04/13
- [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal,
Jan Kiszka <=
- Re: [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, malc, 2008/04/14
- [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, Jan Kiszka, 2008/04/14
- Re: [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, malc, 2008/04/15
- [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, Jan Kiszka, 2008/04/15
- Re: [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, malc, 2008/04/16
- [Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, Jan Kiszka, 2008/04/17
[Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal, Jan Kiszka, 2008/04/14