diff mbox

[v3] OvmfPkg/PlatformPei: Initialise RCBA (B0:D31:F0 0xf0) register

Message ID 1433801233-15578-1-git-send-email-pcacjr@zytor.com
State New
Headers show

Commit Message

Paulo Alcantara June 8, 2015, 10:07 p.m. UTC 
This patch initialises root complex register block BAR in order to
support TCO watchdog emulation features (e.g. reboot upon NO_REBOOT bit
not set) on QEMU.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---
 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  5 +++++
 OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

Comments

Laszlo Ersek June 8, 2015, 10:29 p.m. UTC | #1 
On 06/09/15 00:07, Paulo Alcantara wrote:
> This patch initialises root complex register block BAR in order to
> support TCO watchdog emulation features (e.g. reboot upon NO_REBOOT bit
> not set) on QEMU.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> ---
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  5 +++++
>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 4f59a7c..18b34a3 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -77,6 +77,9 @@
>  #define ICH9_GEN_PMCON_1          0xA0
>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>  
> +#define ICH9_RCBA                 0xF0
> +#define ICH9_RCBA_EN                BIT0
> +
>  //
>  // IO ports
>  //
> @@ -90,4 +93,6 @@
>  #define ICH9_SMI_EN_APMC_EN      BIT5
>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>  
> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> +
>  #endif
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 1126c65..3811162 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -212,13 +212,16 @@ MemMapInitialization (
>      // 0xFEC00000    IO-APIC                        4 KB
>      // 0xFEC01000    gap                         1020 KB
>      // 0xFED00000    HPET                           1 KB
> -    // 0xFED00400    gap                         1023 KB
> +    // 0xFED00400    gap                          111 KB
> +    // 0xFED1C000    RCRB                          16 KB
> +    // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>    }
>  }
> @@ -292,6 +295,15 @@ MiscInitialization (
>      //
>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>    }
> +
> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> +    //
> +    // Set Root Complex Register Block BAR
> +    //
> +    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
> +                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
> +               );
> +  }
>  }

The right formatting for the last function call would be

PciWrite32 (
  POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
  ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
  );

or

PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
  ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN);

but I won't obsess about it.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Let's see what Jordan says.

Thanks
Laszlo
jordan.l.justen@intel.com June 8, 2015, 10:49 p.m. UTC | #2 
On 2015-06-08 15:07:13, Paulo Alcantara wrote:
> This patch initialises root complex register block BAR in order to
> support TCO watchdog emulation features (e.g. reboot upon NO_REBOOT bit
> not set) on QEMU.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> ---
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  5 +++++
>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 4f59a7c..18b34a3 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -77,6 +77,9 @@
>  #define ICH9_GEN_PMCON_1          0xA0
>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>  
> +#define ICH9_RCBA                 0xF0
> +#define ICH9_RCBA_EN                BIT0
> +
>  //
>  // IO ports
>  //
> @@ -90,4 +93,6 @@
>  #define ICH9_SMI_EN_APMC_EN      BIT5
>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>  
> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> +
>  #endif
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 1126c65..3811162 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -212,13 +212,16 @@ MemMapInitialization (
>      // 0xFEC00000    IO-APIC                        4 KB
>      // 0xFEC01000    gap                         1020 KB
>      // 0xFED00000    HPET                           1 KB
> -    // 0xFED00400    gap                         1023 KB
> +    // 0xFED00400    gap                          111 KB
> +    // 0xFED1C000    RCRB                          16 KB

Should we make this conditional?
    // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB

... and make mHostBridgeDevId a global var, and then conditionally add
the memory io HOB?

I don't think it is critical, but with that
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

-Jordan

> +    // 0xFED20000    gap                          896 KB
>      // 0xFEE00000    LAPIC                          1 MB
>      //
>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>    }
>  }
> @@ -292,6 +295,15 @@ MiscInitialization (
>      //
>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>    }
> +
> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> +    //
> +    // Set Root Complex Register Block BAR
> +    //
> +    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
> +                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
> +               );
> +  }
>  }
>  
>  
> -- 
> 2.1.0
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
Laszlo Ersek June 8, 2015, 11:09 p.m. UTC | #3 
On 06/09/15 00:49, Jordan Justen wrote:
> On 2015-06-08 15:07:13, Paulo Alcantara wrote:
>> This patch initialises root complex register block BAR in order to
>> support TCO watchdog emulation features (e.g. reboot upon NO_REBOOT bit
>> not set) on QEMU.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>> ---
>>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  5 +++++
>>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>> index 4f59a7c..18b34a3 100644
>> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>> @@ -77,6 +77,9 @@
>>  #define ICH9_GEN_PMCON_1          0xA0
>>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>>  
>> +#define ICH9_RCBA                 0xF0
>> +#define ICH9_RCBA_EN                BIT0
>> +
>>  //
>>  // IO ports
>>  //
>> @@ -90,4 +93,6 @@
>>  #define ICH9_SMI_EN_APMC_EN      BIT5
>>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>>  
>> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
>> +
>>  #endif
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 1126c65..3811162 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -212,13 +212,16 @@ MemMapInitialization (
>>      // 0xFEC00000    IO-APIC                        4 KB
>>      // 0xFEC01000    gap                         1020 KB
>>      // 0xFED00000    HPET                           1 KB
>> -    // 0xFED00400    gap                         1023 KB
>> +    // 0xFED00400    gap                          111 KB
>> +    // 0xFED1C000    RCRB                          16 KB
> 
> Should we make this conditional?
>     // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB
> 
> ... and make mHostBridgeDevId a global var, and then conditionally add
> the memory io HOB?

Good point.

Laszlo

> I don't think it is critical, but with that
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> 
> -Jordan
> 
>> +    // 0xFED20000    gap                          896 KB
>>      // 0xFEE00000    LAPIC                          1 MB
>>      //
>>      AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
>>                           BASE_2GB : TopOfLowRam, 0xFC000000);
>>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>> +    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
>>      AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
>>    }
>>  }
>> @@ -292,6 +295,15 @@ MiscInitialization (
>>      //
>>      PciOr8 (AcpiCtlReg, AcpiEnBit);
>>    }
>> +
>> +  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>> +    //
>> +    // Set Root Complex Register Block BAR
>> +    //
>> +    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
>> +                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
>> +               );
>> +  }
>>  }
>>  
>>  
>> -- 
>> 2.1.0
>>
>>
>> ------------------------------------------------------------------------------
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
Paulo Alcantara June 8, 2015, 11:30 p.m. UTC | #4 
On Tue, 09 Jun 2015 01:09:19 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 06/09/15 00:49, Jordan Justen wrote:
> > On 2015-06-08 15:07:13, Paulo Alcantara wrote:
> >> This patch initialises root complex register block BAR in order to
> >> support TCO watchdog emulation features (e.g. reboot upon
> >> NO_REBOOT bit not set) on QEMU.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
> >> ---
> >>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  5 +++++
> >>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
> >>  2 files changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> >> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h index
> >> 4f59a7c..18b34a3 100644 ---
> >> a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h +++
> >> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h @@ -77,6 +77,9 @@
> >>  #define ICH9_GEN_PMCON_1          0xA0
> >>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
> >>  
> >> +#define ICH9_RCBA                 0xF0
> >> +#define ICH9_RCBA_EN                BIT0
> >> +
> >>  //
> >>  // IO ports
> >>  //
> >> @@ -90,4 +93,6 @@
> >>  #define ICH9_SMI_EN_APMC_EN      BIT5
> >>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
> >>  
> >> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
> >> +
> >>  #endif
> >> diff --git a/OvmfPkg/PlatformPei/Platform.c
> >> b/OvmfPkg/PlatformPei/Platform.c index 1126c65..3811162 100644
> >> --- a/OvmfPkg/PlatformPei/Platform.c
> >> +++ b/OvmfPkg/PlatformPei/Platform.c
> >> @@ -212,13 +212,16 @@ MemMapInitialization (
> >>      // 0xFEC00000    IO-APIC                        4 KB
> >>      // 0xFEC01000    gap                         1020 KB
> >>      // 0xFED00000    HPET                           1 KB
> >> -    // 0xFED00400    gap                         1023 KB
> >> +    // 0xFED00400    gap                          111 KB
> >> +    // 0xFED1C000    RCRB                          16 KB
> > 
> > Should we make this conditional?
> >     // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB
> > 
> > ... and make mHostBridgeDevId a global var, and then conditionally
> > add the memory io HOB?
> 
> Good point.

Good point, indeed. Unlike HPET, I/O APIC and other addresses that will
be shared between PIIX4 and ICH9, the RCRB will be exclusive to ICH9. I
can make these changes on top this patch, or do you guys prefer it to
be placed in another?

Thanks,

Paulo
Laszlo Ersek June 8, 2015, 11:46 p.m. UTC | #5 
On 06/09/15 01:30, Paulo Alcantara wrote:
> On Tue, 09 Jun 2015 01:09:19 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 06/09/15 00:49, Jordan Justen wrote:
>>> On 2015-06-08 15:07:13, Paulo Alcantara wrote:
>>>> This patch initialises root complex register block BAR in order to
>>>> support TCO watchdog emulation features (e.g. reboot upon
>>>> NO_REBOOT bit not set) on QEMU.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>>> ---
>>>>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  5 +++++
>>>>  OvmfPkg/PlatformPei/Platform.c                | 14 +++++++++++++-
>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
>>>> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h index
>>>> 4f59a7c..18b34a3 100644 ---
>>>> a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h +++
>>>> b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h @@ -77,6 +77,9 @@
>>>>  #define ICH9_GEN_PMCON_1          0xA0
>>>>  #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
>>>>  
>>>> +#define ICH9_RCBA                 0xF0
>>>> +#define ICH9_RCBA_EN                BIT0
>>>> +
>>>>  //
>>>>  // IO ports
>>>>  //
>>>> @@ -90,4 +93,6 @@
>>>>  #define ICH9_SMI_EN_APMC_EN      BIT5
>>>>  #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
>>>>  
>>>> +#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
>>>> +
>>>>  #endif
>>>> diff --git a/OvmfPkg/PlatformPei/Platform.c
>>>> b/OvmfPkg/PlatformPei/Platform.c index 1126c65..3811162 100644
>>>> --- a/OvmfPkg/PlatformPei/Platform.c
>>>> +++ b/OvmfPkg/PlatformPei/Platform.c
>>>> @@ -212,13 +212,16 @@ MemMapInitialization (
>>>>      // 0xFEC00000    IO-APIC                        4 KB
>>>>      // 0xFEC01000    gap                         1020 KB
>>>>      // 0xFED00000    HPET                           1 KB
>>>> -    // 0xFED00400    gap                         1023 KB
>>>> +    // 0xFED00400    gap                          111 KB
>>>> +    // 0xFED1C000    RCRB                          16 KB
>>>
>>> Should we make this conditional?
>>>     // 0xFED1C000    gap (PIIX4) / RCRB (ICH9)     16 KB
>>>
>>> ... and make mHostBridgeDevId a global var, and then conditionally
>>> add the memory io HOB?
>>
>> Good point.
> 
> Good point, indeed. Unlike HPET, I/O APIC and other addresses that will
> be shared between PIIX4 and ICH9, the RCRB will be exclusive to ICH9. I
> can make these changes on top this patch, or do you guys prefer it to
> be placed in another?

I think mHostBridgeDevId should be made an extern in the first patch,
and then the second patch could be this one, also including the
conditional stuff in MemMapInitialization().

Thanks
Laszlo
diff mbox

Patch

diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index 4f59a7c..18b34a3 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -77,6 +77,9 @@ 
 #define ICH9_GEN_PMCON_1          0xA0
 #define ICH9_GEN_PMCON_1_SMI_LOCK   BIT4
 
+#define ICH9_RCBA                 0xF0
+#define ICH9_RCBA_EN                BIT0
+
 //
 // IO ports
 //
@@ -90,4 +93,6 @@ 
 #define ICH9_SMI_EN_APMC_EN      BIT5
 #define ICH9_SMI_EN_GBL_SMI_EN   BIT0
 
+#define ICH9_ROOT_COMPLEX_BASE 0xFED1C000
+
 #endif
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 1126c65..3811162 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -212,13 +212,16 @@  MemMapInitialization (
     // 0xFEC00000    IO-APIC                        4 KB
     // 0xFEC01000    gap                         1020 KB
     // 0xFED00000    HPET                           1 KB
-    // 0xFED00400    gap                         1023 KB
+    // 0xFED00400    gap                          111 KB
+    // 0xFED1C000    RCRB                          16 KB
+    // 0xFED20000    gap                          896 KB
     // 0xFEE00000    LAPIC                          1 MB
     //
     AddIoMemoryRangeHob (TopOfLowRam < BASE_2GB ?
                          BASE_2GB : TopOfLowRam, 0xFC000000);
     AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
     AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
+    AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
     AddIoMemoryBaseSizeHob (PcdGet32(PcdCpuLocalApicBaseAddress), SIZE_1MB);
   }
 }
@@ -292,6 +295,15 @@  MiscInitialization (
     //
     PciOr8 (AcpiCtlReg, AcpiEnBit);
   }
+
+  if (HostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+    //
+    // Set Root Complex Register Block BAR
+    //
+    PciWrite32 (POWER_MGMT_REGISTER_Q35 (ICH9_RCBA),
+                ICH9_ROOT_COMPLEX_BASE | ICH9_RCBA_EN
+               );
+  }
 }