diff mbox series

[Linaro-uefi,linaro-uefi,v2,05/11] Hisilicon/PciHostBridgeDxe: Assign BAR resource from PciRegionBase

Message ID 1505918938-52550-9-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: 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(-)

Comments

Leif Lindholm Sept. 20, 2017, 3:47 p.m. UTC | #1 
On Wed, Sep 20, 2017 at 10:48:52PM +0800, Heyi Guo wrote:
> From: huangming <huangming23@huawei.com>
> 
> Io BAR should be based IoBase and Mem BAR should be based PciRegionBase.

The change looks sensible, but I would like to see Ard's comments as
well.

Some style comments below.

> 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
> @@ -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) {

Use MAX_UINT32 instead?

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

Use MAX_UINT32 instead?

>          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) {

Use MAX_UINT32 instead?

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

Use MAX_UINT32 instead?

>          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);

Use MAX_UINT64 instead?

>          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);

Use MAX_UINT64 instead?

>          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);

Use MAX_UINT64 instead?

Indentation incorrect.

Regards,

Leif

> +      }
> +      Configuration.SpaceDesp[Index].AddrRangeMax = Configuration.SpaceDesp[Index].AddrRangeMin + PrivateData->ResAllocNode[Index].Length - 1;
>        Configuration.SpaceDesp[Index].AddrLen      = PrivateData->ResAllocNode[Index].Length;
>      }
>    }
> -- 
> 1.9.1
>
Ard Biesheuvel Sept. 21, 2017, 12:19 a.m. UTC | #2 
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
>
Ard Biesheuvel Sept. 21, 2017, 1:30 p.m. UTC | #3 
On 21 September 2017 at 02:03, zhangjinsong (A)
<zhangjinsong2@huawei.com> wrote:
> 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.
>

We have platforms like that (using Synopsys IP as well), and all it
needs is a PciHostBridgeLib imlementation that performs the low-level
ATU window programming in its constructor, and a PciSegmentLib
implementation that supports config space accesses for different
segments.

Please refer to
https://git.linaro.org/people/ard.biesheuvel/edk2-platforms.git/commit/?h=synquacer&id=8b31ff98ec73168d914ee52b922a5144357dcb25
https://git.linaro.org/people/ard.biesheuvel/edk2-platforms.git/commit/?h=synquacer&id=9f3f94b601d2e21692085d49dc573a194e970e6a

Again, it is too late to change this, but for the next respin of the
platform, please switch to the generic 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.
>
>

OK

>>          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
>>
diff mbox series

Patch

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
@@ -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);
         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);
         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;
     }
   }