See below reply please.
Jason 18929341291
-----邮件原件----- 发件人: Leif Lindholm [mailto:leif.lindholm@linaro.org] 发送时间: 2017年9月21日 0:08 收件人: Heyi Guo heyi.guo@linaro.org 抄送: linaro-uefi@lists.linaro.org; graeme.gregory@linaro.org; ard.biesheuvel@linaro.org; Guoheyi guoheyi@huawei.com; wanghuiqiang wanghuiqiang@huawei.com; Huangming (Mark) huangming23@huawei.com; zhangjinsong (A) zhangjinsong2@huawei.com; Ming Huang waip23@foxmail.com 主题: Re: [linaro-uefi v2 11/11] Hisilicon D03/D05: Enlarge iATU for RP with ARI capable device.
On Wed, Sep 20, 2017 at 10:48:58PM +0800, Heyi Guo wrote:
From: Ming Huang waip23@foxmail.com
- Because Hi161x chip doesn't support "ARI Forwarding Enable" function, BIOS will enumerate 32 same devices (Device Number 0~31) when attach a Non-ARI capable device in the RP. Hi161x chip will not fix it, need BIOS patch.
- Just enlarge iatu for those root port with ARI capable device attached, Non-ARI capable device's RP, keep iatu limitation.
- Remove previous temporary solution as below commit id: "7d157da88852cc91df2b11b10ade2edbbfbe77da"
I will leave the technical aspects to Ard for comment. A few style comments below.
Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jason zhang zhangjinsong2@huawei.com
.../Drivers/PciHostBridgeDxe/PciHostBridge.c | 1 + .../Drivers/PciHostBridgeDxe/PciHostBridge.h | 4 ++ .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c | 76 ++++++++++++++++++++++ 3 files changed, 81 insertions(+)
diff --git a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c index 6ecc1e5..5bc04a2 100644 --- a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c +++ b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c @@ -839,6 +839,7 @@ NotifyPhase( case EfiPciHostBridgeEndEnumeration: PCIE_DEBUG("Case EfiPciHostBridgeEndEnumeration\n");
- EnlargeAtuConfig0 (This); break;
case EfiPciHostBridgeBeginBusAllocation: diff --git a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h index cddda6b..925ed40 100644 --- a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h +++ b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h @@ -518,4 +518,8 @@ RootBridgeConstructor ( IN UINT32 Seg ); +VOID +EnlargeAtuConfig0 (
- IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL *This
- );
#endif diff --git a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c index 8dfb4b9..b41dbe2 100644 --- a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c +++ b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c @@ -14,6 +14,7 @@ **/ #include "PciHostBridge.h" +#include <IndustryStandard/PciExpress30.h> #include <Library/DevicePathLib.h> #include <Library/DmaLib.h> #include <Library/PciExpressLib.h> @@ -2322,3 +2323,78 @@ RootBridgeIoConfiguration ( return EFI_SUCCESS; } +BOOLEAN +PcieCheckAriFwdEn (
- UINTN PciBaseAddr
- )
+{
- UINT8 PciPrimaryStatus;
- UINT8 CapabilityOffset;
- UINT8 CapId;
- UINT8 TempData;
- PciPrimaryStatus = MmioRead16 (PciBaseAddr +
- PCI_PRIMARY_STATUS_OFFSET);
- if (PciPrimaryStatus & EFI_PCI_STATUS_CAPABILITY) {
- CapabilityOffset = MmioRead8 (PciBaseAddr + PCI_CAPBILITY_POINTER_OFFSET);
- CapabilityOffset &= ~(BIT0 | BIT1);
Why are the bottom two bits being stripped off? 【Reply】Just follow PCIe Spec 4.0 section 7.5.1.11 as below: " The bottom two bits are Reserved and must be set to 00b. Software must mask these bits off before using this register as a pointer in Configuration Space to the first entry of a linked list of new capabilities".
- while ((CapabilityOffset != 0) && (CapabilityOffset != 0xff)) {
At least the 0xff could do with a descriptively named #define. Maybe 0 could too.
CapId = MmioRead8 (PciBaseAddr + CapabilityOffset);if (CapId == EFI_PCI_CAPABILITY_ID_PCIEXP) {break;}CapabilityOffset = MmioRead8 (PciBaseAddr + CapabilityOffset + 1);CapabilityOffset &= ~(BIT0 | BIT1);
Why strip bottom bits? 【Reply】 The same as above.
- }
- } else {
- PCIE_DEBUG ("[%a:%d] - No PCIE Capability.\n", __FUNCTION__, __LINE__);
- return FALSE;
- }
- if ((CapabilityOffset == 0xff) || (CapabilityOffset == 0x0)) {
#define ?
- PCIE_DEBUG ("[%a:%d] - No PCIE Capability.\n", __FUNCTION__, __LINE__);
- return FALSE;
- }
- TempData = MmioRead16 (PciBaseAddr + CapabilityOffset +
- EFI_PCIE_CAPABILITY_DEVICE_CONTROL_2_OFFSET);
Line too long - format as: TempData = MmioRead16 (PciBaseAddr + CapabilityOffset + EFI_PCIE_CAPABILITY_DEVICE_CONTROL_2_OFFSET);
- TempData &=
- EFI_PCIE_CAPABILITY_DEVICE_CAPABILITIES_2_ARI_FORWARDING;
- if (TempData == EFI_PCIE_CAPABILITY_DEVICE_CAPABILITIES_2_ARI_FORWARDING) {
- return TRUE;
- } else {
- return FALSE;
- }
+}
+VOID +EnlargeAtuConfig0 (
- IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL *This
- )
+{
- UINTN RbPciBase;
- UINT64 MemLimit;
- LIST_ENTRY *List;
- PCI_HOST_BRIDGE_INSTANCE *HostBridgeInstance;
- PCI_ROOT_BRIDGE_INSTANCE *RootBridgeInstance;
- PCIE_DEBUG ("In Enlarge RP iatu Config 0.\n");
- HostBridgeInstance = INSTANCE_FROM_RESOURCE_ALLOCATION_THIS (This);
- List = HostBridgeInstance->Head.ForwardLink;
- while (List != &HostBridgeInstance->Head) {
- PCIE_DEBUG ("HostBridge has data.\n");
- RootBridgeInstance = DRIVER_INSTANCE_FROM_LIST_ENTRY (List);
- RbPciBase = RootBridgeInstance->RbPciBar;
- // Those ARI FWD Enable Root Bridge, need enlarge iatu window.
- if (PcieCheckAriFwdEn (RbPciBase)) {
Line too long, format as: MemLimit = GetPcieCfgAddress (RootBridgeInstance->Ecam, RootBridgeInstance->BusBase + 2, 0, 0, 0) - 1;
Regards,
Leif
MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_VIEW_POINT, 1);MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LIMIT, (UINT32) MemLimit);- }
- List = List->ForwardLink;
- }
+}
1.9.1