Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
Am 3. Januar 2024 12:26:07 UTC schrieb BALATON Zoltan :
>On Tue, 2 Jan 2024, Bernhard Beschow wrote:
>> Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan
>> :
>>> On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan
:
> 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
>> ---
>> 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.
>>>
>>> Missed this part in previous reply. The serial ports would be taken care of
>>> by a loop for number of ports so only the floppy needs an if which could
>>> also use the number of serial ports for lack of better way to distinguish
>>> these cips easily. Number of ports are in the superio class which you could
>>> get with ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count
>>> would be 2 for 686b and 1 for 8231.
>>
>> I'm not very convinced about telling the device models apart by their number
>> of sub devices. So let's omit this part for now.
>>
>>>
>>> But now I think another way may be better that is to drop the
>>> superio_update function as this updates all functions on writing any
>>> register unnecessarily and put the lines from it in the
>>> vt*_superio_cfg_write() functions under the separate cases. This was the
>>> original intent, that's why the
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
On Tue, 2 Jan 2024, Bernhard Beschow wrote:
Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan :
On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan :
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
---
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.
Missed this part in previous reply. The serial ports would be taken care of by
a loop for number of ports so only the floppy needs an if which could also use
the number of serial ports for lack of better way to distinguish these cips
easily. Number of ports are in the superio class which you could get with
ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2
for 686b and 1 for 8231.
I'm not very convinced about telling the device models apart by their number of
sub devices. So let's omit this part for now.
But now I think another way may be better that is to drop the
superio_update function as this updates all functions on writing any
register unnecessarily and put the lines from it in the
vt*_superio_cfg_write() functions under the separate cases. This was
the original intent, that's why the reset function goes through that
write function so it can enable/disable functions. That way you could
apply mask on write so via_superio_cfg_read() would return 0 bits as 0
(although the data sheet is not clear about what real chip does, just
says these must be 0 not that it's enforced but if we enforce that it's
probably better to return the effective value on read as well). Then
when state saving is added in separate patch you can have a similar
function as vt82c686b_superio_reset() (or rename that to update and
make it use regs[xx] instead of constant values and call that from
reset after setting regs values like you did here. But that needs more
thought as the vmstate added
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan :
>On Tue, 19 Dec 2023, Bernhard Beschow wrote:
>> Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan
>> :
>>> 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
---
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.
>
>Missed this part in previous reply. The serial ports would be taken care of by
>a loop for number of ports so only the floppy needs an if which could also use
>the number of serial ports for lack of better way to distinguish these cips
>easily. Number of ports are in the superio class which you could get with
>ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2
>for 686b and 1 for 8231.
I'm not very convinced about telling the device models apart by their number of
sub devices. So let's omit this part for now.
>
>But now I think another way may be better that is to drop the superio_update
>function as this updates all functions on writing any register unnecessarily
>and put the lines from it in the vt*_superio_cfg_write() functions under the
>separate cases. This was the original intent, that's why the reset function
>goes through that write function so it can enable/disable functions. That way
>you could apply mask on write so via_superio_cfg_read() would return 0 bits as
>0 (although the data sheet is not clear about what real chip does, just says
>these must be 0 not that it's enforced but if we enforce that it's probably
>better to return the
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan :
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
---
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.
Missed this part in previous reply. The serial ports would be taken care
of by a loop for number of ports so only the floppy needs an if which
could also use the number of serial ports for lack of better way to
distinguish these cips easily. Number of ports are in the superio class
which you could get with ISA_SUPERIO_GET_CLASS (see via_superio_realize)
then serial.count would be 2 for 686b and 1 for 8231.
But now I think another way may be better that is to drop the
superio_update function as this updates all functions on writing any
register unnecessarily and put the lines from it in the
vt*_superio_cfg_write() functions under the separate cases. This was the
original intent, that's why the reset function goes through that write
function so it can enable/disable functions. That way you could apply mask
on write so via_superio_cfg_read() would return 0 bits as 0 (although the
data sheet is not clear about what real chip does, just says these must be
0 not that it's enforced but if we enforce that it's probably better to
return the effective value on read as well). Then when state saving is
added in separate patch you can have a similar function as
vt82c686b_superio_reset() (or rename that to update and make it use
regs[xx] instead of constant values and call that from reset after setting
regs values like you did here. But that needs more thought as the vmstate
added by this patch is incomplete and would not work so you could just
drop it for now and add it later with also adding other necessary state as
well. The idea was to implement the chip first then add state saving so we
don't need to
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan :
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
---
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
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan :
>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
>> ---
>> 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) ;-)
> 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.
Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> +}
>> +
>> +static const VMStateDescription vmstate_vt82c686b_superio = {
>> +.name = "vt82c686b_superio",
>> +.version_id = 1,
>> +.post_load = vmstate_vt82c686b_superio_post_load,
>> +};
>> +
>> static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
>> uint64_t data, unsigned size)
>> {
>> @@ -368,7 +400,11 @@ static void vt82c686b_superio_cfg_write(void *opaque,
>> hwaddr addr,
>> case 0xfd ... 0xff:
>> /* ignore
Re: [PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
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
---
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).
+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 :-) 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.)
Regards,
BALATON Zoltan
+}
+
+static const VMStateDescription vmstate_vt82c686b_superio = {
+.name = "vt82c686b_superio",
+.version_id = 1,
+.post_load = vmstate_vt82c686b_superio_post_load,
+};
+
static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
@@ -368,7 +400,11 @@ static void vt82c686b_superio_cfg_write(void *opaque,
hwaddr addr,
case 0xfd ... 0xff:
/* ignore write to read only registers */
return;
-/* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
+case 0xe2 ... 0xe3:
+case 0xe6 ... 0xe8:
+sc->regs[idx] = data;
+vt82c686b_superio_update(sc);
+return;
default:
qemu_log_mask(LOG_UNIMP,
"via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -393,25 +429,24 @@ static void vt82c686b_superio_reset(DeviceState *dev)
memset(s->regs, 0, sizeof(s->regs));
/* Device ID */
-vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
-vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
-/* Function select - all disabled */
-
[PATCH v2 12/12] hw/isa/vt82c686: Implement relocation and toggling of SuperI/O functions
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.
Signed-off-by: Bernhard Beschow
---
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);
+}
+
+static int vmstate_vt82c686b_superio_post_load(void *opaque, int version_id)
+{
+ViaSuperIOState *s = opaque;
+
+vt82c686b_superio_update(s);
+
+return 0;
+}
+
+static const VMStateDescription vmstate_vt82c686b_superio = {
+.name = "vt82c686b_superio",
+.version_id = 1,
+.post_load = vmstate_vt82c686b_superio_post_load,
+};
+
static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size)
{
@@ -368,7 +400,11 @@ static void vt82c686b_superio_cfg_write(void *opaque,
hwaddr addr,
case 0xfd ... 0xff:
/* ignore write to read only registers */
return;
-/* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
+case 0xe2 ... 0xe3:
+case 0xe6 ... 0xe8:
+sc->regs[idx] = data;
+vt82c686b_superio_update(sc);
+return;
default:
qemu_log_mask(LOG_UNIMP,
"via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -393,25 +429,24 @@ static void vt82c686b_superio_reset(DeviceState *dev)
memset(s->regs, 0, sizeof(s->regs));
/* Device ID */
-vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
-vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
-/* Function select - all disabled */
-vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
-vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
+s->regs[0xe0] = 0x3c;
+/*
+ * Function select - only serial enabled
+ * Fuloong 2e's rescue-yl prints to the serial console w/o enabling it.
This
+ * suggests that the serial ports are enabled by default, so override the
+ * datasheet.
+ */
+s->regs[0xe2] = 0x0f;
/* Floppy ctrl base addr 0x3f0-7 */
-vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
-vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
+s->regs[0xe3] = 0xfc;
/* Parallel port base addr 0x378-f */
-vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
-vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
+s->regs[0xe6] = 0xde;
/* Serial port 1 base addr 0x3f8-f */
-vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
-vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
+s->regs[0xe7] = 0xfe;
/* Serial port 2 base addr 0x2f8-f */
-vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
-vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
+s->regs[0xe8] = 0xbe;
-vt82c686b_superio_cfg_write(s, 0, 0, 1);
+vt82c686b_superio_update(s);
}
static void vt82c686b_superio_init(Object *obj)
@@ -429,6 +464,7 @@ static void vt82c686b_superio_class_init(ObjectClass
*klass, void *data)
sc->parallel.count = 1;
sc->ide.count = 0; /* emulated by via-ide */
sc->floppy.count = 1;
+dc->vmsd = _vt82c686b_superio;
}
static const TypeInfo vt82c686b_superio_info = {
@@ -443,6 +479,33 @@ static const TypeInfo
