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

[Jan Kiszka]

andrzej zaborowski wrote:
> 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,

Don't understand yet why (are you afraid of pow, libm, or float in
general?), but if it helps to get things merged... ;)

> 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

Hmm, wasn't endianness about finding out which byte is most-significant
and which not? :->

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

Yes, it restores the original semantic, at least as long as
RAMSIZE_FIXED is not set. That case is still a bit suboptimal as you
have to provide pessimistic values, e.g. the maximum flash size that can
be used. But I can live with it I guess.

Jan

signature.asc
Description: OpenPGP digital signature