On Fri, Apr 29, 2016 at 05:41:19PM +0200, Ard Biesheuvel wrote:
> The PCI related PCDs are not platform specific, and architectural
> protocols such as CpuIo2 are based on PCI provided MMIO to IO
> translation, so these PCDs belong in ArmPkg not ArmPlatformPkg.

Very happy with this - but, could you before pushing add a notice to
message saying something like:

"This *WILL* break some out-of-tree platforms, the fix is changing all
consumers of gArmPlatformTokenSpaceGuid.PcdPci* to
gArmTokenSpaceGuid.PcdPci."?

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <[email protected]>

Reviewed-by: Leif Lindholm <[email protected]>

> ---
>  ArmPkg/ArmPkg.dec                                                       | 62 
> ++++++++++++++++++++
>  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf             |  4 
> +-
>  ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf | 16 
> ++---
>  ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoLib.inf             |  8 
> +--
>  ArmPlatformPkg/ArmPlatformPkg.dec                                       | 62 
> --------------------
>  5 files changed, 76 insertions(+), 76 deletions(-)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index e90306653864..75c238aa1e3d 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -255,3 +255,65 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
>    gArmTokenSpaceGuid.PcdGicRedistributorsBase|0|UINT32|0x0000000E
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0|UINT32|0x0000000D
>    gArmTokenSpaceGuid.PcdGicSgiIntId|0|UINT32|0x00000025
> +
> +  #
> +  # Bases, sizes and translation offsets of IO and MMIO spaces, respectively.
> +  # Note that "IO" is just another MMIO range that simulates IO space; there
> +  # are no special instructions to access it.
> +  #
> +  # The base addresses PcdPciIoBase, PcdPciMmio32Base and PcdPciMmio64Base 
> are
> +  # specific to their containing address spaces. In order to get the physical
> +  # address for the CPU, for a given access, the respective translation value
> +  # has to be added.
> +  #
> +  # The translations always have to be initialized like this, using UINT64:
> +  #
> +  #   UINT64 IoCpuBase;     // mapping target in 64-bit cpu-physical space
> +  #   UINT64 Mmio32CpuBase; // mapping target in 64-bit cpu-physical space
> +  #   UINT64 Mmio64CpuBase; // mapping target in 64-bit cpu-physical space
> +  #
> +  #   PcdPciIoTranslation     = IoCpuBase     - PcdPciIoBase;
> +  #   PcdPciMmio32Translation = Mmio32CpuBase - (UINT64)PcdPciMmio32Base;
> +  #   PcdPciMmio64Translation = Mmio64CpuBase - PcdPciMmio64Base;
> +  #
> +  # because (a) the target address space (ie. the cpu-physical space) is
> +  # 64-bit, and (b) the translation values are meant as offsets for *modular*
> +  # arithmetic.
> +  #
> +  # Accordingly, the translation itself needs to be implemented as:
> +  #
> +  #   UINT64 UntranslatedIoAddress;     // input parameter
> +  #   UINT32 UntranslatedMmio32Address; // input parameter
> +  #   UINT64 UntranslatedMmio64Address; // input parameter
> +  #
> +  #   UINT64 TranslatedIoAddress;       // output parameter
> +  #   UINT64 TranslatedMmio32Address;   // output parameter
> +  #   UINT64 TranslatedMmio64Address;   // output parameter
> +  #
> +  #   TranslatedIoAddress     = UntranslatedIoAddress +
> +  #                             PcdPciIoTranslation;
> +  #   TranslatedMmio32Address = (UINT64)UntranslatedMmio32Address +
> +  #                             PcdPciMmio32Translation;
> +  #   TranslatedMmio64Address = UntranslatedMmio64Address +
> +  #                             PcdPciMmio64Translation;
> +  #
> +  #  The modular arithmetic performed in UINT64 ensures that the translation
> +  #  works correctly regardless of the relation between IoCpuBase and
> +  #  PcdPciIoBase, Mmio32CpuBase and PcdPciMmio32Base, and Mmio64CpuBase and
> +  #  PcdPciMmio64Base.
> +  #
> +  gArmTokenSpaceGuid.PcdPciIoBase|0x0|UINT64|0x00000050
> +  gArmTokenSpaceGuid.PcdPciIoSize|0x0|UINT64|0x00000051
> +  gArmTokenSpaceGuid.PcdPciIoTranslation|0x0|UINT64|0x00000052
> +  gArmTokenSpaceGuid.PcdPciMmio32Base|0x0|UINT32|0x00000053
> +  gArmTokenSpaceGuid.PcdPciMmio32Size|0x0|UINT32|0x00000054
> +  gArmTokenSpaceGuid.PcdPciMmio32Translation|0x0|UINT64|0x00000055
> +  gArmTokenSpaceGuid.PcdPciMmio64Base|0x0|UINT64|0x00000056
> +  gArmTokenSpaceGuid.PcdPciMmio64Size|0x0|UINT64|0x00000057
> +  gArmTokenSpaceGuid.PcdPciMmio64Translation|0x0|UINT64|0x00000058
> +
> +  #
> +  # Inclusive range of allowed PCI buses.
> +  #
> +  gArmTokenSpaceGuid.PcdPciBusMin|0x0|UINT32|0x00000059
> +  gArmTokenSpaceGuid.PcdPciBusMax|0x0|UINT32|0x0000005A
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf 
> b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> index 6ab81e8dd60a..a2617982b259 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.inf
> @@ -80,8 +80,8 @@ [FixedPcd]
>  
>    # PCI Root complex specific PCDs
>    gArmJunoTokenSpaceGuid.PcdPciConfigurationSpaceBaseAddress
> -  gArmPlatformTokenSpaceGuid.PcdPciBusMin
> -  gArmPlatformTokenSpaceGuid.PcdPciBusMax
> +  gArmTokenSpaceGuid.PcdPciBusMin
> +  gArmTokenSpaceGuid.PcdPciBusMax
>  
>  [Pcd]
>    gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths
> diff --git 
> a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf 
> b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
> index 9f526910c48a..de28c805ae6e 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
> @@ -58,14 +58,14 @@ [Pcd.common]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
>    gArmTokenSpaceGuid.PcdSystemMemorySize
>  
> -  gArmPlatformTokenSpaceGuid.PcdPciBusMin
> -  gArmPlatformTokenSpaceGuid.PcdPciBusMax
> -  gArmPlatformTokenSpaceGuid.PcdPciIoBase
> -  gArmPlatformTokenSpaceGuid.PcdPciIoSize
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Base
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Size
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Base
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Size
> +  gArmTokenSpaceGuid.PcdPciBusMin
> +  gArmTokenSpaceGuid.PcdPciBusMax
> +  gArmTokenSpaceGuid.PcdPciIoBase
> +  gArmTokenSpaceGuid.PcdPciIoSize
> +  gArmTokenSpaceGuid.PcdPciMmio32Base
> +  gArmTokenSpaceGuid.PcdPciMmio32Size
> +  gArmTokenSpaceGuid.PcdPciMmio64Base
> +  gArmTokenSpaceGuid.PcdPciMmio64Size
>  
>    gArmJunoTokenSpaceGuid.PcdPcieControlBaseAddress
>    gArmJunoTokenSpaceGuid.PcdPcieRootPortBaseAddress
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoLib.inf 
> b/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
> index 530bb1cb9dc0..8c8b141c35a2 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
> +++ b/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
> @@ -58,10 +58,10 @@ [FixedPcd]
>    gArmJunoTokenSpaceGuid.PcdPciConfigurationSpaceSize
>  
>  [Pcd]
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Base
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Size
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Base
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Size
> +  gArmTokenSpaceGuid.PcdPciMmio32Base
> +  gArmTokenSpaceGuid.PcdPciMmio32Size
> +  gArmTokenSpaceGuid.PcdPciMmio64Base
> +  gArmTokenSpaceGuid.PcdPciMmio64Size
>  
>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec 
> b/ArmPlatformPkg/ArmPlatformPkg.dec
> index 0a7e78443eff..1c05132cd625 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -125,68 +125,6 @@ [PcdsFixedAtBuild.common,PcdsDynamic.common]
>    gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0|UINT32|0x00000024
>    
> gArmPlatformTokenSpaceGuid.PcdPL031RtcPpmAccuracy|300000000|UINT32|0x00000022
>  
> -  #
> -  # Inclusive range of allowed PCI buses.
> -  #
> -  gArmPlatformTokenSpaceGuid.PcdPciBusMin|0x0|UINT32|0x0000003E
> -  gArmPlatformTokenSpaceGuid.PcdPciBusMax|0x0|UINT32|0x0000003F
> -
> -  #
> -  # Bases, sizes and translation offsets of IO and MMIO spaces, respectively.
> -  # Note that "IO" is just another MMIO range that simulates IO space; there
> -  # are no special instructions to access it.
> -  #
> -  # The base addresses PcdPciIoBase, PcdPciMmio32Base and PcdPciMmio64Base 
> are
> -  # specific to their containing address spaces. In order to get the physical
> -  # address for the CPU, for a given access, the respective translation value
> -  # has to be added.
> -  #
> -  # The translations always have to be initialized like this, using UINT64:
> -  #
> -  #   UINT64 IoCpuBase;     // mapping target in 64-bit cpu-physical space
> -  #   UINT64 Mmio32CpuBase; // mapping target in 64-bit cpu-physical space
> -  #   UINT64 Mmio64CpuBase; // mapping target in 64-bit cpu-physical space
> -  #
> -  #   PcdPciIoTranslation     = IoCpuBase     - PcdPciIoBase;
> -  #   PcdPciMmio32Translation = Mmio32CpuBase - (UINT64)PcdPciMmio32Base;
> -  #   PcdPciMmio64Translation = Mmio64CpuBase - PcdPciMmio64Base;
> -  #
> -  # because (a) the target address space (ie. the cpu-physical space) is
> -  # 64-bit, and (b) the translation values are meant as offsets for *modular*
> -  # arithmetic.
> -  #
> -  # Accordingly, the translation itself needs to be implemented as:
> -  #
> -  #   UINT64 UntranslatedIoAddress;     // input parameter
> -  #   UINT32 UntranslatedMmio32Address; // input parameter
> -  #   UINT64 UntranslatedMmio64Address; // input parameter
> -  #
> -  #   UINT64 TranslatedIoAddress;       // output parameter
> -  #   UINT64 TranslatedMmio32Address;   // output parameter
> -  #   UINT64 TranslatedMmio64Address;   // output parameter
> -  #
> -  #   TranslatedIoAddress     = UntranslatedIoAddress +
> -  #                             PcdPciIoTranslation;
> -  #   TranslatedMmio32Address = (UINT64)UntranslatedMmio32Address +
> -  #                             PcdPciMmio32Translation;
> -  #   TranslatedMmio64Address = UntranslatedMmio64Address +
> -  #                             PcdPciMmio64Translation;
> -  #
> -  #  The modular arithmetic performed in UINT64 ensures that the translation
> -  #  works correctly regardless of the relation between IoCpuBase and
> -  #  PcdPciIoBase, Mmio32CpuBase and PcdPciMmio32Base, and Mmio64CpuBase and
> -  #  PcdPciMmio64Base.
> -  #
> -  gArmPlatformTokenSpaceGuid.PcdPciIoBase|0x0|UINT64|0x00000040
> -  gArmPlatformTokenSpaceGuid.PcdPciIoSize|0x0|UINT64|0x00000041
> -  gArmPlatformTokenSpaceGuid.PcdPciIoTranslation|0x0|UINT64|0x00000042
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Base|0x0|UINT32|0x00000043
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Size|0x0|UINT32|0x00000044
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio32Translation|0x0|UINT64|0x00000045
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Base|0x0|UINT64|0x00000046
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Size|0x0|UINT64|0x00000047
> -  gArmPlatformTokenSpaceGuid.PcdPciMmio64Translation|0x0|UINT64|0x00000048
> -
>  [PcdsFixedAtBuild.ARM]
>    # Stack for CPU Cores in Secure Monitor Mode
>    gArmPlatformTokenSpaceGuid.PcdCPUCoresSecMonStackBase|0|UINT64|0x00000007
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to