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

[malc]

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.

> +               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.

> +       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.

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.

> +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.

> +       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.

[..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..

> +                       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..]

--
mailto:address@hidden