diff mbox series

[v2,19/43] hw/isa/piix3: Allow board to provide PCI interrupt routes

Message ID 20221022150508.26830-20-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series Consolidate PIIX south bridges | expand

Commit Message

Bernhard Beschow Oct. 22, 2022, 3:04 p.m. UTC
PIIX3 initializes the PIRQx route control registers to the default
values as described in the 82371AB PCI-TO-ISA/IDE XCELERATOR (PIIX4)
April 1997 manual. PIIX4, however, initializes the routes according to
the Malta™ User’s Manual, ch 6.6, which are IRQs 10 and 11. In order to
allow the reset methods to be consolidated, allow board code to specify
the routes.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix3.c                | 12 ++++++++----
 include/hw/southbridge/piix.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 25, 2022, 11:34 p.m. UTC | #1
On 22/10/22 17:04, Bernhard Beschow wrote:
> PIIX3 initializes the PIRQx route control registers to the default
> values as described in the 82371AB PCI-TO-ISA/IDE XCELERATOR (PIIX4)
> April 1997 manual. PIIX4, however, initializes the routes according to
> the Malta™ User’s Manual, ch 6.6, which are IRQs 10 and 11. In order to
> allow the reset methods to be consolidated, allow board code to specify
> the routes.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix3.c                | 12 ++++++++----
>   include/hw/southbridge/piix.h |  1 +
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index aa32f43e4a..c6a8f1f27d 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -168,10 +168,10 @@ static void piix3_reset(DeviceState *dev)
>       pci_conf[0x4c] = 0x4d;
>       pci_conf[0x4e] = 0x03;
>       pci_conf[0x4f] = 0x00;
> -    pci_conf[0x60] = 0x80;
> -    pci_conf[0x61] = 0x80;
> -    pci_conf[0x62] = 0x80;
> -    pci_conf[0x63] = 0x80;

These values are the correct reset ones however (also for PIIX4).

The problem is the Malta machine abuse of the PIIX4 model. IOW
this doesn't seem the correct approach, we should fix how Malta
use the PIIX4 (in the emulated tiny boot loader). I'll try to
write smth before reviewing the rest of this series, because
it might simplify your refactor.

> +    pci_conf[PIIX_PIRQCA] = d->pci_irq_reset_mappings[0];
> +    pci_conf[PIIX_PIRQCB] = d->pci_irq_reset_mappings[1];
> +    pci_conf[PIIX_PIRQCC] = d->pci_irq_reset_mappings[2];
> +    pci_conf[PIIX_PIRQCD] = d->pci_irq_reset_mappings[3];
>       pci_conf[0x69] = 0x02;
>       pci_conf[0x70] = 0x80;
>       pci_conf[0x76] = 0x0c;
> @@ -383,6 +383,10 @@ static void pci_piix3_init(Object *obj)
>   
>   static Property pci_piix3_props[] = {
>       DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
> +    DEFINE_PROP_UINT8("pirqa", PIIX3State, pci_irq_reset_mappings[0], 0x80),
> +    DEFINE_PROP_UINT8("pirqb", PIIX3State, pci_irq_reset_mappings[1], 0x80),
> +    DEFINE_PROP_UINT8("pirqc", PIIX3State, pci_irq_reset_mappings[2], 0x80),
> +    DEFINE_PROP_UINT8("pirqd", PIIX3State, pci_irq_reset_mappings[3], 0x80),
>       DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
>       DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
>       DEFINE_PROP_BOOL("smm-enabled", PIIX3State, smm_enabled, false),
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 1f22eb1444..df3e0084c5 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -54,6 +54,7 @@ struct PIIXState {
>   
>       /* This member isn't used. Just for save/load compatibility */
>       int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
> +    uint8_t pci_irq_reset_mappings[PIIX_NUM_PIRQS];
>   
>       ISAPICState pic;
>       RTCState rtc;
Bernhard Beschow Oct. 26, 2022, 9:45 a.m. UTC | #2
Hi Phil,

Am 25. Oktober 2022 23:34:15 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 22/10/22 17:04, Bernhard Beschow wrote:
>> PIIX3 initializes the PIRQx route control registers to the default
>> values as described in the 82371AB PCI-TO-ISA/IDE XCELERATOR (PIIX4)
>> April 1997 manual. PIIX4, however, initializes the routes according to
>> the Malta™ User’s Manual, ch 6.6, which are IRQs 10 and 11. In order to
>> allow the reset methods to be consolidated, allow board code to specify
>> the routes.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/isa/piix3.c                | 12 ++++++++----
>>   include/hw/southbridge/piix.h |  1 +
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>> index aa32f43e4a..c6a8f1f27d 100644
>> --- a/hw/isa/piix3.c
>> +++ b/hw/isa/piix3.c
>> @@ -168,10 +168,10 @@ static void piix3_reset(DeviceState *dev)
>>       pci_conf[0x4c] = 0x4d;
>>       pci_conf[0x4e] = 0x03;
>>       pci_conf[0x4f] = 0x00;
>> -    pci_conf[0x60] = 0x80;
>> -    pci_conf[0x61] = 0x80;
>> -    pci_conf[0x62] = 0x80;
>> -    pci_conf[0x63] = 0x80;
>
>These values are the correct reset ones however (also for PIIX4).
>
>The problem is the Malta machine abuse of the PIIX4 model. IOW
>this doesn't seem the correct approach, we should fix how Malta
>use the PIIX4 (in the emulated tiny boot loader). I'll try to
>write smth before reviewing the rest of this series, because
>it might simplify your refactor.

Indeed my first approach for the refactor was to implement MachineClass::reset for Malta where the Malta-specific values would be set. I could redo that if you want. Just let me know.

Best regards,
Bernhard
>
>> +    pci_conf[PIIX_PIRQCA] = d->pci_irq_reset_mappings[0];
>> +    pci_conf[PIIX_PIRQCB] = d->pci_irq_reset_mappings[1];
>> +    pci_conf[PIIX_PIRQCC] = d->pci_irq_reset_mappings[2];
>> +    pci_conf[PIIX_PIRQCD] = d->pci_irq_reset_mappings[3];
>>       pci_conf[0x69] = 0x02;
>>       pci_conf[0x70] = 0x80;
>>       pci_conf[0x76] = 0x0c;
>> @@ -383,6 +383,10 @@ static void pci_piix3_init(Object *obj)
>>     static Property pci_piix3_props[] = {
>>       DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
>> +    DEFINE_PROP_UINT8("pirqa", PIIX3State, pci_irq_reset_mappings[0], 0x80),
>> +    DEFINE_PROP_UINT8("pirqb", PIIX3State, pci_irq_reset_mappings[1], 0x80),
>> +    DEFINE_PROP_UINT8("pirqc", PIIX3State, pci_irq_reset_mappings[2], 0x80),
>> +    DEFINE_PROP_UINT8("pirqd", PIIX3State, pci_irq_reset_mappings[3], 0x80),
>>       DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
>>       DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
>>       DEFINE_PROP_BOOL("smm-enabled", PIIX3State, smm_enabled, false),
>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>> index 1f22eb1444..df3e0084c5 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -54,6 +54,7 @@ struct PIIXState {
>>         /* This member isn't used. Just for save/load compatibility */
>>       int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>> +    uint8_t pci_irq_reset_mappings[PIIX_NUM_PIRQS];
>>         ISAPICState pic;
>>       RTCState rtc;
>
diff mbox series

Patch

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index aa32f43e4a..c6a8f1f27d 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -168,10 +168,10 @@  static void piix3_reset(DeviceState *dev)
     pci_conf[0x4c] = 0x4d;
     pci_conf[0x4e] = 0x03;
     pci_conf[0x4f] = 0x00;
-    pci_conf[0x60] = 0x80;
-    pci_conf[0x61] = 0x80;
-    pci_conf[0x62] = 0x80;
-    pci_conf[0x63] = 0x80;
+    pci_conf[PIIX_PIRQCA] = d->pci_irq_reset_mappings[0];
+    pci_conf[PIIX_PIRQCB] = d->pci_irq_reset_mappings[1];
+    pci_conf[PIIX_PIRQCC] = d->pci_irq_reset_mappings[2];
+    pci_conf[PIIX_PIRQCD] = d->pci_irq_reset_mappings[3];
     pci_conf[0x69] = 0x02;
     pci_conf[0x70] = 0x80;
     pci_conf[0x76] = 0x0c;
@@ -383,6 +383,10 @@  static void pci_piix3_init(Object *obj)
 
 static Property pci_piix3_props[] = {
     DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
+    DEFINE_PROP_UINT8("pirqa", PIIX3State, pci_irq_reset_mappings[0], 0x80),
+    DEFINE_PROP_UINT8("pirqb", PIIX3State, pci_irq_reset_mappings[1], 0x80),
+    DEFINE_PROP_UINT8("pirqc", PIIX3State, pci_irq_reset_mappings[2], 0x80),
+    DEFINE_PROP_UINT8("pirqd", PIIX3State, pci_irq_reset_mappings[3], 0x80),
     DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
     DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
     DEFINE_PROP_BOOL("smm-enabled", PIIX3State, smm_enabled, false),
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 1f22eb1444..df3e0084c5 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -54,6 +54,7 @@  struct PIIXState {
 
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
+    uint8_t pci_irq_reset_mappings[PIIX_NUM_PIRQS];
 
     ISAPICState pic;
     RTCState rtc;