qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling


From: BALATON Zoltan
Subject: Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
Date: Sun, 24 Dec 2023 01:25:35 +0100 (CET)

On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
On Mon, 18 Dec 2023, Bernhard Beschow wrote:
The VIA south bridges are able to relocate and toggle (enable or disable) their
SuperI/O functions. So far this is hardcoded such that all functions are always
enabled and are located at fixed addresses.

Some PC BIOSes seem to probe for I/O occupancy before activating such a function
and issue an error in case of a conflict. Since the functions are enabled on
reset, conflicts are always detected. Prevent that by implementing relocation
and toggling of the SuperI/O functions.

Note that all SuperI/O functions are now deactivated upon reset (except for
VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
enabled by default). Rely on firmware -- or in case of pegasos2 on board code if
no -bios is given -- to configure the functions accordingly.

Pegasos2 emulates firmware when no -bios is given, this was explained in 
previos commit so maybe not needed to be explained it here again so you could 
drop the comment between -- -- but I don't mind.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
1 file changed, 90 insertions(+), 31 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9c2333a277..be202d23cf 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -15,6 +15,9 @@

#include "qemu/osdep.h"
#include "hw/isa/vt82c686.h"
+#include "hw/block/fdc.h"
+#include "hw/char/parallel-isa.h"
+#include "hw/char/serial.h"
#include "hw/pci/pci.h"
#include "hw/qdev-properties.h"
#include "hw/ide/pci.h"
@@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {

#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"

+static void vt82c686b_superio_update(ViaSuperIOState *s)
+{
+    isa_parallel_set_enabled(s->superio.parallel[0],
+                             (s->regs[0xe2] & 0x3) != 3);
+    isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
+    isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
+    isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
+
+    isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
+    isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
+    isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
+    isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
+}

I wonder if some code duplication could be saved by adding a shared 
via_superio_update() for this further up in the abstract via-superio class 
instead of this method and vt8231_superio_update() below. This common method in 
abstract class would need to handle the differences which seem to be reg 
addresses offset by 0x10 and VT8231 having only 1 serial port. These could 
either be handled by adding function parameters or fields to ViaSuperIOState 
for this that the subclasses can set and the method check. (Such as reg 
base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how 
many ports are there then can have a for loop for those that would only run 
once for vt8231).

Only the enable bits and the parallel port base address line up, the serial 
port(s) and the floppy would need special treatment. Not worth it IMO.


+static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
+{
+    ViaSuperIOState *s = opaque;
+
+    vt82c686b_superio_update(s);
+
+    return 0;

You could lose some blank lines here. You seem to love them, half of your cover 
letter is just blank lines :-)

Yes, I want people to see the light rather than a wall (of text) ;-)

Well, information is in the text and anything else just distracts from it. I guess you can configure your editor to present text the way you like but it's hard for me or others to get rid of hard formatting so it's better to only add the needed text.

but I'm the opposite and like more code to fit in one screen even on todays 
displays that are wider than tall. So this funciton would take less space 
without blank lines. (Even the local variable may not be necessary as you don't 
access any fields within and void * should just cast without a warning but for 
spelling out the desired type as a reminder I'm ok with leaving that but no 
excessive blank lines please if you don't mind that much.)

In this case I'll remove the s variable and the blank line above the return if 
that's so important to you.

I think it's simper and more readable that way that's why it's important to me.

Regards,
BALATON Zoltan



reply via email to

[Prev in Thread] Current Thread [Next in Thread]