From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50102 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PwI3n-00070I-7M for qemu-devel@nongnu.org; Sun, 06 Mar 2011 12:48:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PwI3k-0005bF-SD for qemu-devel@nongnu.org; Sun, 06 Mar 2011 12:48:03 -0500 Received: from mail-yi0-f45.google.com ([209.85.218.45]:55448) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PwI3k-0005ab-OO for qemu-devel@nongnu.org; Sun, 06 Mar 2011 12:48:00 -0500 Received: by yib19 with SMTP id 19so1696302yib.4 for ; Sun, 06 Mar 2011 09:47:59 -0800 (PST) Message-ID: <4D73C8CE.6080800@codemonkey.ws> Date: Sun, 06 Mar 2011 11:47:58 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 4/4] i8254: convert to qdev References: <4D73B6CA.8070000@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On 03/06/2011 10:47 AM, Blue Swirl wrote: >> The PIT is not an ISA device. Modelling it as such is worse than leaving it >> unmodelled. >> > No. These days, PIT is part of Super I/O chip, which is accessed via > LPC bus. LPC is from software point of view equal to ISA. Therefore, > in absence of LPC, ISA is correct. > In the system we model, the PIT is part of the PIIX3. The right way to model it is as a DeviceState that's no_user=1 and created as part of the initialized of PIIX3 (for the PC at least). LPC is still an expansion bus and it's primarily used for discrete components like a TPM. For components that are all part of a Super I/O chip, there really just isn't a bus in the middle. >>> -PITState *pit_init(int base, qemu_irq irq); >>> -void pit_set_gate(PITState *pit, int channel, int val); >>> -int pit_get_gate(PITState *pit, int channel); >>> -int pit_get_initial_count(PITState *pit, int channel); >>> -int pit_get_mode(PITState *pit, int channel); >>> -int pit_get_out(PITState *pit, int channel, int64_t current_time); >>> +void pit_set_gate(ISADevice *dev, int channel, int val); >>> +int pit_get_gate(ISADevice *dev, int channel); >>> +int pit_get_initial_count(ISADevice *dev, int channel); >>> +int pit_get_mode(ISADevice *dev, int channel); >>> +int pit_get_out(ISADevice *dev, int channel, int64_t current_time); >>> >>> >> Making these functions take an ISADevice hurts type safety. They should >> take a PITState. >> > These functions should be removed anyway. For example, pcspk probably > should be merged with i825, or a signal like interface could be used. > It's tough to do that without doing a real simulation. The PC speaker uses one of the PIT channel outputs to drive its tone output. But we don't want to do something like use a qemu_irq with a timer programmed to generate the right frequency wave to drive the tone. So having the PC speaker hold a reference to the PIT and providing a high level method interface makes sense to me. Regards, Anthony Liguori