diff mbox series

[Linaro-uefi,linaro-uefi,v2,11/11] Hisilicon D03/D05: Enlarge iATU for RP with ARI capable device.

Message ID 1505918938-52550-15-git-send-email-heyi.guo@linaro.org
State New
Headers show
Series Update D03/D05 binary for edk2 update and bug fix. | expand

Commit Message

gary guo Sept. 20, 2017, 2:48 p.m. UTC 
From: Ming Huang <waip23@foxmail.com>

1. 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.
2. Just enlarge iatu for those root port with ARI capable device
   attached, Non-ARI capable device's RP, keep iatu limitation.
3. Remove previous temporary solution as below commit id:
   "7d157da88852cc91df2b11b10ade2edbbfbe77da"

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(+)

Comments

Leif Lindholm Sept. 20, 2017, 4:08 p.m. UTC | #1 
On Wed, Sep 20, 2017 at 10:48:58PM +0800, Heyi Guo wrote:
> From: Ming Huang <waip23@foxmail.com>
> 
> 1. 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.
> 2. Just enlarge iatu for those root port with ARI capable device
>    attached, Non-ARI capable device's RP, keep iatu limitation.
> 3. 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?

> +
> +    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?

> +    }
> +  } 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
>
Leif Lindholm Sept. 21, 2017, 9:20 a.m. UTC | #2 
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>
> > 
> > 1. 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.
> > 2. Just enlarge iatu for those root port with ARI capable device
> >    attached, Non-ARI capable device's RP, keep iatu limitation.
> > 3. 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
diff mbox series

Patch

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);
+
+    while ((CapabilityOffset != 0) && (CapabilityOffset != 0xff)) {
+      CapId = MmioRead8 (PciBaseAddr + CapabilityOffset);
+      if (CapId == EFI_PCI_CAPABILITY_ID_PCIEXP) {
+        break;
+      }
+      CapabilityOffset = MmioRead8 (PciBaseAddr + CapabilityOffset + 1);
+      CapabilityOffset &= ~(BIT0 | BIT1);
+    }
+  } else {
+    PCIE_DEBUG ("[%a:%d] - No PCIE Capability.\n", __FUNCTION__, __LINE__);
+    return FALSE;
+  }
+
+  if ((CapabilityOffset == 0xff) || (CapabilityOffset == 0x0)) {
+    PCIE_DEBUG ("[%a:%d] - No PCIE Capability.\n", __FUNCTION__, __LINE__);
+    return FALSE;
+  }
+
+  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)) {
+      MemLimit = GetPcieCfgAddress (RootBridgeInstance->Ecam, RootBridgeInstance->BusBase + 2, 0, 0, 0) - 1;
+      MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_VIEW_POINT, 1);
+      MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LIMIT, (UINT32) MemLimit);
+    }
+    List = List->ForwardLink;
+  }
+}