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.
@@ -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.
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.
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