[Qemu-devel] Re: [RFC][PATCH 4/4] Add support for Marvell 88w8618 / MusicPal

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5690 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]