See below reply please.
Jason 18929341291
-----邮件原件----- 发件人: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 发送时间: 2017年9月21日 8:20 收件人: Heyi Guo heyi.guo@linaro.org 抄送: Leif Lindholm leif.lindholm@linaro.org; linaro-uefi linaro-uefi@lists.linaro.org; Graeme Gregory graeme.gregory@linaro.org; Guoheyi guoheyi@huawei.com; wanghuiqiang wanghuiqiang@huawei.com; Huangming (Mark) huangming23@huawei.com; zhangjinsong (A) zhangjinsong2@huawei.com 主题: Re: [linaro-uefi v2 05/11] Hisilicon/PciHostBridgeDxe: Assign BAR resource from PciRegionBase
On 20 September 2017 at 07:48, Heyi Guo heyi.guo@linaro.org wrote:
From: huangming huangming23@huawei.com
Io BAR should be based IoBase and Mem BAR should be based PciRegionBase.
Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ming Huang huangming23@huawei.com
.../Drivers/PciHostBridgeDxe/PciHostBridge.c | 29 ++++++++++++++-------- .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c | 15 +++++++++-- 2 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c index a970da6..6ecc1e5 100644 --- a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c +++ b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c
I know I should have asked this years ago, but could you please clarify why you need a separate PciHostBridgeDxe in the first place? It doesn't matter for this patch, but I'm just curious.
【Reply】Because D03/D05 design is not the same as X86, each root port has own IO, BAR and ECAM resource, for each single root port, we need configure different ATU windows, and the non-continuous ECAM Base and Bar Configuration. So we have a new driver.
@@ -1410,9 +1410,8 @@ SetResource( Ptr->ResType = 1; Ptr->GenFlag = 0; Ptr->SpecificFlag = 0;
/* This is PCIE Device Bus which start address is the low 32bit of mem base*/Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +(RootBridgeInstance->MemBase & 0xFFFFFFFF);
/* PCIE Device Iobar address should be based on IoBase */Ptr->AddrRangeMin = RootBridgeInstance->IoBase; Ptr->AddrRangeMax = 0; Ptr->AddrTranslationOffset = \ (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED :EFI_RESOURCE_LESS; @@ -1429,9 +1428,13 @@ SetResource( Ptr->GenFlag = 0; Ptr->SpecificFlag = 0; Ptr->AddrSpaceGranularity = 32;
/* This is PCIE Device Bus which start address is the low 32bit of mem base*/
/* PCIE device Bar should be based on PciRegionBase */if (RootBridgeInstance->PciRegionBase > 0xFFFFFFFF) {DEBUG((DEBUG_ERROR, "PCIE Res(TypeMem32) unsupported.\n"));return EFI_UNSUPPORTED;} Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
(RootBridgeInstance->MemBase & 0xFFFFFFFF);
(RootBridgeInstance->PciRegionBase &- 0xFFFFFFFF);
I can't really comment on this, unless I go through the entire driver. Having a separate PCI host bridge driver for this IP seems slightly overkill, but I can understand that this is for historical reasons (the generic PciHostBridgeDxe is actually a fairly recent addition)
Are the ATU window configurations fixed? Or does the driver reprogram them at runtime for config space accesses? How many windows does the IP support in the first place? Is there only a single MMIO window, from which all BAR allocations are made?
Again, this is not going to block acceptance of this series, but down the road, I would like more consolidation between different users of the Synopsys IP.
【Reply】 a. ATU windows config is fixed? -- Yes, Fixed. b. Does the driver reprogram them at runtime for config space accesses? -- No. c. How many windows does the IP support in the first place? -- Depends on RP numbers, each RP have 4 types ATU windows. d. Is there only a single MMIO window, from which all BAR allocations are made? -- No, Independent by RP.
Ptr->AddrRangeMax = 0; Ptr->AddrTranslationOffset = \ (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED :EFI_RESOURCE_LESS; @@ -1448,9 +1451,13 @@ SetResource( Ptr->GenFlag = 0; Ptr->SpecificFlag = 6; Ptr->AddrSpaceGranularity = 32;
/* This is PCIE Device Bus which start address is the low 32bit of mem base*/
/* PCIE device Bar should be based on PciRegionBase */if (RootBridgeInstance->PciRegionBase > 0xFFFFFFFF) {DEBUG((DEBUG_ERROR, "PCIE Res(TypePMem32) unsupported.\n"));return EFI_UNSUPPORTED;} Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
(RootBridgeInstance->MemBase & 0xFFFFFFFF);
(RootBridgeInstance->PciRegionBase &- 0xFFFFFFFF); Ptr->AddrRangeMax = 0; Ptr->AddrTranslationOffset = \ (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED :
EFI_RESOURCE_LESS; @@ -1467,9 +1474,9 @@ SetResource( Ptr->GenFlag = 0; Ptr->SpecificFlag = 0; Ptr->AddrSpaceGranularity = 64;
/* This is PCIE Device Bus which start address is the low 32bit of mem base*/
/* PCIE device Bar should be based on PciRegionBase */ Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
(RootBridgeInstance->MemBase & 0xFFFFFFFFFFFFFFFF);
(RootBridgeInstance->PciRegionBase &- 0xFFFFFFFFFFFFFFFF);
Just remove the & operation please, unless you can explain when and'ing with MAX_UINT64 does anything useful. 【Reply】 Follow suggestion to remove.
Ptr->AddrRangeMax = 0; Ptr->AddrTranslationOffset = \ (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED :EFI_RESOURCE_LESS; @@ -1486,9 +1493,9 @@ SetResource( Ptr->GenFlag = 0; Ptr->SpecificFlag = 6; Ptr->AddrSpaceGranularity = 64;
/* This is PCIE Device Bus which start address is the low 32bit of mem base*/
/* PCIE device Bar should be based on PciRegionBase */ Ptr->AddrRangeMin = (RootBridgeInstance->ResAllocNode[Index].Base - RootBridgeInstance->MemBase) +
(RootBridgeInstance->MemBase & 0xFFFFFFFFFFFFFFFF);
(RootBridgeInstance->PciRegionBase &- 0xFFFFFFFFFFFFFFFF); Ptr->AddrRangeMax = 0; Ptr->AddrTranslationOffset = \ (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED :
EFI_RESOURCE_LESS; diff --git a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c index 03edcf1..8dfb4b9 100644 --- a/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c +++ b/Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c @@ -2301,8 +2301,19 @@ RootBridgeIoConfiguration ( PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS (This); for (Index = 0; Index < TypeMax; Index++) { if (PrivateData->ResAllocNode[Index].Status == ResAllocated) {
Configuration.SpaceDesp[Index].AddrRangeMin = PrivateData->ResAllocNode[Index].Base;Configuration.SpaceDesp[Index].AddrRangeMax = PrivateData->ResAllocNode[Index].Base + PrivateData->ResAllocNode[Index].Length - 1;
switch (Index) {case TypeIo:Configuration.SpaceDesp[Index].AddrRangeMin = PrivateData->IoBase;break;case TypeBus:Configuration.SpaceDesp[Index].AddrRangeMin = PrivateData->ResAllocNode[Index].Base;break;default:/* PCIE Device bar address should be base on PciRegionBase */Configuration.SpaceDesp[Index].AddrRangeMin = (PrivateData->ResAllocNode[Index].Base - PrivateData->MemBase) +(PrivateData->PciRegionBase & 0xFFFFFFFFFFFFFFFF);}Configuration.SpaceDesp[Index].AddrRangeMax =- Configuration.SpaceDesp[Index].AddrRangeMin +
- PrivateData->ResAllocNode[Index].Length - 1; Configuration.SpaceDesp[Index].AddrLen = PrivateData->ResAllocNode[Index].Length; } }
-- 1.9.1