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

[andrzej zaborowski]

On 19/04/2008, Jan Kiszka <address@hidden> wrote:
> andrzej zaborowski wrote:
>  > On 18/04/2008, Jan Kiszka <address@hidden> wrote:
>  >>  Andrzej, as you have written the wm8750, do you already know where which
>  >>  volume level would have to be applied (open-coded or via some
>  >>  AUD_set_volume)? I'm currently only using LOUT2VOL, and I'm a bit lazy
>  >>  to study the datasheet /wrt all the mixer details.
>  >
>  > My idea was to open
>  > http://www.wolfsonmicro.com/uploads/documents/en/WM8750.pdf and on the
>  > first page every Wolfson datasheet has its diagram of all audio paths
>  > (of which there are always too many) and then trace with my finger the
>  > path between the source (the I2C or I2S interfaces) and the sink (the
>  > analog output), and then multiply all the volume values that are
>  > applied there (both analog and digital) and pass that to host mixer
>  > through some functions in audio/ for the given SWVoice - but we don't
>  > have any such functions and I'm ok with using the host mixer manually.
>  >  (VirtualBox has them implemented iirc)  So yes, maybe it makes sense
>  > to multiply the samples for the moment and use only LOUTnVOL /
>  > ROUTnVOL values as these are used by the guests we're interested in.
>
>
> Done, and it finally works. One of the two quirks I found in wm8750 made
>  the switch a bit hairy. Patches will follow.

Thanks.  I pushed the patch with fixes.  Regarding the wm8750_fini
patch, I'll #if 0 it because it's possible that a board will have this
chip on something hotpluggable and will need to create and destroy it
various times and it's easy to miss something in the clean-up.
Regarding the volume patch, I'll make a look-up table at one point,
and then merge.  Also, if we have 16-bit data and 7-bit volume scale
maybe we're fine with scalling only the most-significant-byte and
avoiding endianness headaches (or maybe not).  Nevertheless the
MusicPal emulator should be bootable without that.

>
>
>  >
>  >>
>  >>  >>>   - 128×64 display with brightness control
>  >>  >>>   - all input buttons
>  >>  >>>
>  >>  >>>  Using up to 32 MB flash, I hit a limit /wrt phys_ram_size. I worked
>  >>  >>>  around this for now by extending MAX_BIOS_SIZE to 32 MB, surely not 
> a
>  >>  >>>  nice solution.
>  >>  >> You can use -m 150 or similar.
>  >>  >>
>  >>  >> Please also format the code similarly to rest of Qemu.
>  >>  >
>  >>  > That would just increase ram_size, thus won't help as I need memory
>  >>  > beyond it (here for the pflash in R/W mode).
>  >
>  > Yes, I had not looked at how ram_size was used in the musicpal board
>  > initialisation, sorry.
>  >
>  >>
>  >> OK, I see what you mean after looking at your N800 patches: You apply a
>  >>  fixed RAM size, leaving the rest of what the user provided via -m to
>  >>  SRAM and flash. Not optimal IMHO, you may sometimes also want to play
>  >>  with the RAM size even if the real devices has a fixed amount. And it is
>  >>  far from being intuitive as well.
>  >
>  > Yes, although you allow the user to set also a smaller RAM than what
>  > the virtual machine expects.
>
>
> That's indeed something the machine should take of (if there are such
>  hard limits).
>
>
>  >
>  >>  The only true solution I see right now is moving qemu_vmalloc into the
>  >>  machine initialization code. Is there anything between current
>  >>  qemu_vmalloc and machine->init that relies on phys_ram_base being valid
>  >>  (and which can't be moved after the machine init) and thus prevents this?
>  >
>  > I had a different idea: add a field ram_constraint in struct
>  > QEMUMachine, which would hold the amount of RAM the machine always
>  > needs (e.g. bios and video RAM), and the low bit could hold a flag
>  > RAM_SIZE_FIXED for machines that have only such RAM (basically the
>  > criteria should be whether it's possible for the guest to detect the
>  > memory size there is on board - on machines like Spitz there's no way)
>
>
> IIRC, embedded boards let the boot loader "detect" this. I see valid
>  scenarios where one wants to play with different sizes and may therefore
>  patch U-Boot - or load the kernel directly which should make QEMU set
>  the related ATAG field appropriately, no?

Yes, in case of a standard firmware like Linux or U-boot - but we
probably don't need to provide options for everything one may want to
play with unless it's a valid hardware configuration (like in the PC
case where you can add and take away RAM sticks), at some point the
user needs to edit the source either way.

Anyway almost half of the boards in qemu ignored ram_size until now
and risked the provided size being too low and segfaulting, so with
the patch I sent in another mail at least there's a check, and the
check is only done once for all boards so it can be removed from the
few boards that did it.

>
>
>  > and for such machines the -m parameter would be invalid.  I'll try to
>  > come up with a patch.
>
>
> I originally had the same idea but I dropped it because it would still
>  overload -m with semantics that don't belong there. IMHO -m should only
>  describe the main RAM size, not any additionally by QEMU required memory
>  for establishing fixed SRAM or even for backing up flash devices. That's
>  at least what I would expect from this switch and what the documentation
>  suggests as well so far.

This property is not changed by the patch (I hope).

>
>  Thus, back to square #1: Can't we move qemu_vmalloc into the
>  machine-specific init handlers?

I think it would work (but I wouldn't do that).

Cheers
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.