Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script

["Yao, Jiewen" <jiewen.yao@intel.com>]

Yes, I also agree to add a new MACRO.

My only concern is the name - POWER_MGMT_REGISTER_Q35_EFI is too weird.
I have no idea on the meaning to use _EFI as suffix.

Since we already defined below in PciRootBridgeIo.h,
#define EFI_PCI_ADDRESS(bus, dev, func, reg) \
  (UINT64) ( \
  (((UINTN) bus) << 24) | \
  (((UINTN) dev) << 16) | \
  (((UINTN) func) << 8) | \
  (((UINTN) (reg)) < 256 ? ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) (reg), 32))))


How about we use POWER_MGMT_REGISTER_Q35_EFI_PCI_ADDRESS, or EFI_PCI_ADDRESS_POWER_MGMT_REGISTER_Q35 ?

Thank you
Yao Jiewen



From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, January 5, 2017 7:48 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel-01 <edk2-devel@ml01.01.org>
Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script

On 01/05/17 02:45, Yao, Jiewen wrote:
> Sorry, fix typo: I agree we do **not** use EFI_ prefix here.
>
> But using _EFI as suffix is also odd. :)
>
> Do we have better name?
>
> Such as POWER_MGMT_REGISTER_Q35_ADDRESS ?

POWER_MGMT_REGISTER_Q35() already returns a flat address, just in PciLib
encoding. So an _ADDRESS suffix for the variant with the UEFI spec
encoding is not particularly telling.

The new macro name should reflect the PciLib encoding <-> UEFI spec
encoding difference.

Thanks!
Laszlo


>
>
>
> Thank you
>
> Yao Jiewen
>
>
>
> *From:*edk2-devel [mailto:edk2-devel-bounces@lists.01.org] *On Behalf Of
> *Yao, Jiewen
> *Sent:* Thursday, January 5, 2017 9:03 AM
> *To:* Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Laszlo Ersek
> <lersek@redhat.com<mailto:lersek@redhat.com>>; edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> *Subject:* Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct
> PCI_CONFIG_READ_WRITE in S3 boot script
>
>
>
> Hi
> I agree we do use EFI_ prefix here.
>
> But using _EFI as suffix is also odd. :)
>
> Do we have better name?
>
> Thank you
> Yao Jiewen
>
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jordan Justen
> Sent: Thursday, January 5, 2017 6:02 AM
> To: Laszlo Ersek <lersek@redhat.com
<mailto:lersek@redhat.com%0b>> <mailto:lersek@redhat.com>>; edk2-devel-01 <edk2-devel@lists.01.org
<mailto:edk2-devel@lists.01.org%0b>> <mailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
>
> On 2017-01-04 03:19:20, Laszlo Ersek wrote:
>> On 01/04/17 02:30, Jordan Justen wrote:
>> > On 2016-12-02 02:48:44, Laszlo Ersek wrote:
>> >> @@ -362,7 +390,9 @@ OnS3SaveStateInstalled (
>> >>                            S3SaveState,
>> >>                            EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE,
>> >>                            EfiBootScriptWidthUint16,
>> >> -                          (UINT64)POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1),
>> >> +                          ConvertPciLibToEfiPciAddress (
>> >> +                            POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1)
>> >
>> > I think we should just add a EFI_POWER_MGMT_REGISTER_Q35 macro.
>>
>> I thought of that, but I didn't want to use the EFI_ prefix for a macro
>> that has nothing to do with the UEFI / PI specs. Can you suggest an
>> alternative name? Perhaps POWER_MGMT_REGISTER_Q35_EFI?
>
> Good point. Yeah, that name seems fine to me.
>
> -Jordan
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>
> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>