On Thu, Sep 21, 2017 at 09:04:11AM +0000, zhangjinsong (A) wrote:
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/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".
Understood. However, please move this bit of knowledge off into a #define.
Maybe PCI_CAPABILITY_POINTER_MASK?
- 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.
Same as above.
Regards,
Leif