[edk2,1/4] ArmVirtPkg: implement FdtPciPcdProducerLib

Message ID 1460468829-13982-2-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 33ac45661ada95134cc2fcd266f89da391c86b93
Headers show

Commit Message

Ard Biesheuvel April 12, 2016, 1:47 p.m.
This implements a library FdtPciPcdProducerLib which is intended to
be incorporated into modules that consume the PCI related dynamic PCDs
PcdPciExpressBaseAddress and PcdPciDisableBusEnumeration, either via NULL
library class resolution or via a direct dependency (for other libraries
or modules in ArmVirtPkg). This allows us to make them depend on the FDT
client protocol, and populate these PCDs based on the presence and the
contents of a 'pci-ecam-generic' DT node.

This also overloads the meaning of PcdPciExpressBaseAddress, which we will
set to MAX_UINT64 to signify that the actual values of these two PCDs have
not been assigned yet.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c   | 61 ++++++++++++++++++++
 ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 47 +++++++++++++++
 2 files changed, 108 insertions(+)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek April 12, 2016, 2:16 p.m. | #1
On 04/12/16 15:47, Ard Biesheuvel wrote:
> This implements a library FdtPciPcdProducerLib which is intended to

> be incorporated into modules that consume the PCI related dynamic PCDs

> PcdPciExpressBaseAddress and PcdPciDisableBusEnumeration, either via NULL

> library class resolution or via a direct dependency (for other libraries

> or modules in ArmVirtPkg). This allows us to make them depend on the FDT

> client protocol, and populate these PCDs based on the presence and the

> contents of a 'pci-ecam-generic' DT node.


Typo: should be 'pci-host-ecam-generic'.

> This also overloads the meaning of PcdPciExpressBaseAddress, which we will

> set to MAX_UINT64 to signify that the actual values of these two PCDs have

> not been assigned yet.

>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c   | 61

++++++++++++++++++++
>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 47

+++++++++++++++
>  2 files changed, 108 insertions(+)


INF file first:

> diff --git

a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> new file mode 100644

> index 000000000000..546125997ba6

> --- /dev/null

> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf

> @@ -0,0 +1,47 @@

> +#/** @file

> +#  FDT client library for consumers of PCI related dynamic PCDs

> +#

> +#  Copyright (c) 2016, Linaro Ltd. All rights reserved.

> +#

> +#  This program and the accompanying materials

> +#  are licensed and made available under the terms and conditions of

the BSD License
> +#  which accompanies this distribution. The full text of the license

may be found at
> +#  http://opensource.org/licenses/bsd-license.php

> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS

OR IMPLIED.
> +#

> +#

> +#**/


I believe one of these trailing empty lines should be between the URL of
the 2-BSDL and the no-warranty disclaimer.

> +

> +[Defines]

> +  INF_VERSION                    = 0x00010005

> +  BASE_NAME                      = FdtPciPcdProducerLib

> +  FILE_GUID                      = D584275B-BF1E-4DF8-A53D-980F5645C5E7

> +  MODULE_TYPE                    = BASE

> +  VERSION_STRING                 = 1.0

> +  LIBRARY_CLASS                  = PciPcdProducerLib|DXE_DRIVER

UEFI_DRIVER

I think it would make sense to match the library class name to the
(single) library instance we have. I don't insist of course, but I
believe this may have been an oversight. If it was intentional, then I'm
fine with it. (The subject also says FdtPciPcdProducerLib.)

> +  CONSTRUCTOR                    = FdtPciPcdProducerLibConstructor

> +

> +[Sources]

> +  FdtPciPcdProducerLib.c

> +

> +[Packages]

> +  ArmVirtPkg/ArmVirtPkg.dec

> +  MdeModulePkg/MdeModulePkg.dec

> +  MdePkg/MdePkg.dec

> +

> +[LibraryClasses]

> +  BaseLib

> +  DebugLib

> +  PcdLib

> +  UefiBootServicesTableLib

> +

> +[Protocols]

> +  gFdtClientProtocolGuid                                      ## CONSUMES

> +

> +[Pcd]

> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress           ## PRODUCES


For this, I think I proposed (based on existing usage in edk2):

  ## CONSUMES ## SOMETIMES_PRODUCES

I agree we do not "consume" the PCD any longer in the strict sense, in
this module; we just look if it was initialized. Hm... I guess it is
fine as-is.

> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration  ## PRODUCES

> +

> +[Depex]

> +  gFdtClientProtocolGuid


source code:

> diff --git

a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> new file mode 100644

> index 000000000000..64a3aa969680

> --- /dev/null

> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c

> @@ -0,0 +1,61 @@

> +/** @file

> +  FDT client library for consumers of PCI related dynamic PCDs

> +

> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>

> +

> +  This program and the accompanying materials

> +  are licensed and made available under the terms and conditions of

the BSD License
> +  which accompanies this distribution.  The full text of the license

may be found at
> +  http://opensource.org/licenses/bsd-license.php

> +

> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS

OR IMPLIED.
> +

> +**/

> +

> +#include <Uefi.h>

> +

> +#include <Library/BaseLib.h>

> +#include <Library/DebugLib.h>

> +#include <Library/PcdLib.h>

> +#include <Library/UefiBootServicesTableLib.h>

> +

> +#include <Protocol/FdtClient.h>

> +

> +RETURN_STATUS

> +EFIAPI

> +FdtPciPcdProducerLibConstructor (

> +  VOID

> +  )

> +{

> +  UINT64              PciExpressBaseAddress;

> +  FDT_CLIENT_PROTOCOL *FdtClient;

> +  CONST UINT64        *Reg;

> +  UINT32              RegElemSize, RegSize;

> +  EFI_STATUS          Status;

> +

> +  PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

> +  if (PciExpressBaseAddress != MAX_UINT64) {

> +    return EFI_SUCCESS;

> +  }

> +

> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

> +                  (VOID **)&FdtClient);

> +  ASSERT_EFI_ERROR (Status);

> +

> +  Status = FdtClient->FindCompatibleNodeReg (FdtClient,

> +                        "pci-host-ecam-generic", (CONST VOID **)&Reg,

> +                        &RegElemSize, &RegSize);

> +

> +  if (EFI_ERROR (Status)) {

> +    PciExpressBaseAddress = 0;

> +  } else {

> +    ASSERT (RegElemSize == sizeof (UINT64));

> +    PciExpressBaseAddress = SwapBytes64 (*Reg);

> +  }

> +

> +  PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);

> +  PcdSetBool (PcdPciDisableBusEnumeration, FALSE);


I think setting PcdPciDisableBusEnumeration unconditionally to FALSE is
not correct. The second argument should depend on whether
FindCompatibleNodeReg() succeeds. Probably an oversight?

With the above fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks!
Laszlo

> +

> +  return RETURN_SUCCESS;

> +}

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 12, 2016, 2:17 p.m. | #2
On 04/12/16 16:16, Laszlo Ersek wrote:
> [...]


Aargh, ignore this. I forgot to disable word wrapping in tbird, before
pasting my reply. I'll resend my reply sanely wrapped.

Sorry
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 12, 2016, 2:18 p.m. | #3
On 04/12/16 15:47, Ard Biesheuvel wrote:
> This implements a library FdtPciPcdProducerLib which is intended to

> be incorporated into modules that consume the PCI related dynamic PCDs

> PcdPciExpressBaseAddress and PcdPciDisableBusEnumeration, either via NULL

> library class resolution or via a direct dependency (for other libraries

> or modules in ArmVirtPkg). This allows us to make them depend on the FDT

> client protocol, and populate these PCDs based on the presence and the

> contents of a 'pci-ecam-generic' DT node.


Typo: should be 'pci-host-ecam-generic'.

> This also overloads the meaning of PcdPciExpressBaseAddress, which we will

> set to MAX_UINT64 to signify that the actual values of these two PCDs have

> not been assigned yet.

>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c   | 61 ++++++++++++++++++++

>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 47 +++++++++++++++

>  2 files changed, 108 insertions(+)


INF file first:

> diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf

> new file mode 100644

> index 000000000000..546125997ba6

> --- /dev/null

> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf

> @@ -0,0 +1,47 @@

> +#/** @file

> +#  FDT client library for consumers of PCI related dynamic PCDs

> +#

> +#  Copyright (c) 2016, Linaro Ltd. All rights reserved.

> +#

> +#  This program and the accompanying materials

> +#  are licensed and made available under the terms and conditions of the BSD License

> +#  which accompanies this distribution. The full text of the license may be found at

> +#  http://opensource.org/licenses/bsd-license.php

> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

> +#

> +#

> +#**/


I believe one of these trailing empty lines should be between the URL of
the 2-BSDL and the no-warranty disclaimer.

> +

> +[Defines]

> +  INF_VERSION                    = 0x00010005

> +  BASE_NAME                      = FdtPciPcdProducerLib

> +  FILE_GUID                      = D584275B-BF1E-4DF8-A53D-980F5645C5E7

> +  MODULE_TYPE                    = BASE

> +  VERSION_STRING                 = 1.0

> +  LIBRARY_CLASS                  = PciPcdProducerLib|DXE_DRIVER UEFI_DRIVER


I think it would make sense to match the library class name to the
(single) library instance we have. I don't insist of course, but I
believe this may have been an oversight. If it was intentional, then I'm
fine with it. (The subject also says FdtPciPcdProducerLib.)

> +  CONSTRUCTOR                    = FdtPciPcdProducerLibConstructor

> +

> +[Sources]

> +  FdtPciPcdProducerLib.c

> +

> +[Packages]

> +  ArmVirtPkg/ArmVirtPkg.dec

> +  MdeModulePkg/MdeModulePkg.dec

> +  MdePkg/MdePkg.dec

> +

> +[LibraryClasses]

> +  BaseLib

> +  DebugLib

> +  PcdLib

> +  UefiBootServicesTableLib

> +

> +[Protocols]

> +  gFdtClientProtocolGuid                                      ## CONSUMES

> +

> +[Pcd]

> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress           ## PRODUCES


For this, I think I proposed (based on existing usage in edk2):

  ## CONSUMES ## SOMETIMES_PRODUCES

I agree we do not "consume" the PCD any longer in the strict sense, in
this module; we just look if it was initialized. Hm... I guess it is
fine as-is.

> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration  ## PRODUCES

> +

> +[Depex]

> +  gFdtClientProtocolGuid


source code:

> diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c

> new file mode 100644

> index 000000000000..64a3aa969680

> --- /dev/null

> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c

> @@ -0,0 +1,61 @@

> +/** @file

> +  FDT client library for consumers of PCI related dynamic PCDs

> +

> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>

> +

> +  This program and the accompanying materials

> +  are licensed and made available under the terms and conditions of the BSD License

> +  which accompanies this distribution.  The full text of the license may be found at

> +  http://opensource.org/licenses/bsd-license.php

> +

> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

> +

> +**/

> +

> +#include <Uefi.h>

> +

> +#include <Library/BaseLib.h>

> +#include <Library/DebugLib.h>

> +#include <Library/PcdLib.h>

> +#include <Library/UefiBootServicesTableLib.h>

> +

> +#include <Protocol/FdtClient.h>

> +

> +RETURN_STATUS

> +EFIAPI

> +FdtPciPcdProducerLibConstructor (

> +  VOID

> +  )

> +{

> +  UINT64              PciExpressBaseAddress;

> +  FDT_CLIENT_PROTOCOL *FdtClient;

> +  CONST UINT64        *Reg;

> +  UINT32              RegElemSize, RegSize;

> +  EFI_STATUS          Status;

> +

> +  PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

> +  if (PciExpressBaseAddress != MAX_UINT64) {

> +    return EFI_SUCCESS;

> +  }

> +

> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

> +                  (VOID **)&FdtClient);

> +  ASSERT_EFI_ERROR (Status);

> +

> +  Status = FdtClient->FindCompatibleNodeReg (FdtClient,

> +                        "pci-host-ecam-generic", (CONST VOID **)&Reg,

> +                        &RegElemSize, &RegSize);

> +

> +  if (EFI_ERROR (Status)) {

> +    PciExpressBaseAddress = 0;

> +  } else {

> +    ASSERT (RegElemSize == sizeof (UINT64));

> +    PciExpressBaseAddress = SwapBytes64 (*Reg);

> +  }

> +

> +  PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);

> +  PcdSetBool (PcdPciDisableBusEnumeration, FALSE);


I think setting PcdPciDisableBusEnumeration unconditionally to FALSE is
not correct. The second argument should depend on whether
FindCompatibleNodeReg() succeeds. Probably an oversight?

With the above fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks!
Laszlo

> +

> +  return RETURN_SUCCESS;

> +}

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 12, 2016, 2:20 p.m. | #4
On 12 April 2016 at 16:18, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/12/16 15:47, Ard Biesheuvel wrote:

>> This implements a library FdtPciPcdProducerLib which is intended to

>> be incorporated into modules that consume the PCI related dynamic PCDs

>> PcdPciExpressBaseAddress and PcdPciDisableBusEnumeration, either via NULL

>> library class resolution or via a direct dependency (for other libraries

>> or modules in ArmVirtPkg). This allows us to make them depend on the FDT

>> client protocol, and populate these PCDs based on the presence and the

>> contents of a 'pci-ecam-generic' DT node.

>

> Typo: should be 'pci-host-ecam-generic'.

>


OK

>> This also overloads the meaning of PcdPciExpressBaseAddress, which we will

>> set to MAX_UINT64 to signify that the actual values of these two PCDs have

>> not been assigned yet.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c   | 61 ++++++++++++++++++++

>>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 47 +++++++++++++++

>>  2 files changed, 108 insertions(+)

>

> INF file first:

>

>> diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf

>> new file mode 100644

>> index 000000000000..546125997ba6

>> --- /dev/null

>> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf

>> @@ -0,0 +1,47 @@

>> +#/** @file

>> +#  FDT client library for consumers of PCI related dynamic PCDs

>> +#

>> +#  Copyright (c) 2016, Linaro Ltd. All rights reserved.

>> +#

>> +#  This program and the accompanying materials

>> +#  are licensed and made available under the terms and conditions of the BSD License

>> +#  which accompanies this distribution. The full text of the license may be found at

>> +#  http://opensource.org/licenses/bsd-license.php

>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>> +#

>> +#

>> +#**/

>

> I believe one of these trailing empty lines should be between the URL of

> the 2-BSDL and the no-warranty disclaimer.

>


OK

>> +

>> +[Defines]

>> +  INF_VERSION                    = 0x00010005

>> +  BASE_NAME                      = FdtPciPcdProducerLib

>> +  FILE_GUID                      = D584275B-BF1E-4DF8-A53D-980F5645C5E7

>> +  MODULE_TYPE                    = BASE

>> +  VERSION_STRING                 = 1.0

>> +  LIBRARY_CLASS                  = PciPcdProducerLib|DXE_DRIVER UEFI_DRIVER

>

> I think it would make sense to match the library class name to the

> (single) library instance we have. I don't insist of course, but I

> believe this may have been an oversight. If it was intentional, then I'm

> fine with it. (The subject also says FdtPciPcdProducerLib.)

>


Actually, it was intentional, since this way, at least you can reuse
the dependent modules by supplying a Null implementation of the
library class if you are not using DT.

>> +  CONSTRUCTOR                    = FdtPciPcdProducerLibConstructor

>> +

>> +[Sources]

>> +  FdtPciPcdProducerLib.c

>> +

>> +[Packages]

>> +  ArmVirtPkg/ArmVirtPkg.dec

>> +  MdeModulePkg/MdeModulePkg.dec

>> +  MdePkg/MdePkg.dec

>> +

>> +[LibraryClasses]

>> +  BaseLib

>> +  DebugLib

>> +  PcdLib

>> +  UefiBootServicesTableLib

>> +

>> +[Protocols]

>> +  gFdtClientProtocolGuid                                      ## CONSUMES

>> +

>> +[Pcd]

>> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress           ## PRODUCES

>

> For this, I think I proposed (based on existing usage in edk2):

>

>   ## CONSUMES ## SOMETIMES_PRODUCES

>

> I agree we do not "consume" the PCD any longer in the strict sense, in

> this module; we just look if it was initialized. Hm... I guess it is

> fine as-is.

>

>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration  ## PRODUCES

>> +

>> +[Depex]

>> +  gFdtClientProtocolGuid

>

> source code:

>

>> diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c

>> new file mode 100644

>> index 000000000000..64a3aa969680

>> --- /dev/null

>> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c

>> @@ -0,0 +1,61 @@

>> +/** @file

>> +  FDT client library for consumers of PCI related dynamic PCDs

>> +

>> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>

>> +

>> +  This program and the accompanying materials

>> +  are licensed and made available under the terms and conditions of the BSD License

>> +  which accompanies this distribution.  The full text of the license may be found at

>> +  http://opensource.org/licenses/bsd-license.php

>> +

>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>> +

>> +**/

>> +

>> +#include <Uefi.h>

>> +

>> +#include <Library/BaseLib.h>

>> +#include <Library/DebugLib.h>

>> +#include <Library/PcdLib.h>

>> +#include <Library/UefiBootServicesTableLib.h>

>> +

>> +#include <Protocol/FdtClient.h>

>> +

>> +RETURN_STATUS

>> +EFIAPI

>> +FdtPciPcdProducerLibConstructor (

>> +  VOID

>> +  )

>> +{

>> +  UINT64              PciExpressBaseAddress;

>> +  FDT_CLIENT_PROTOCOL *FdtClient;

>> +  CONST UINT64        *Reg;

>> +  UINT32              RegElemSize, RegSize;

>> +  EFI_STATUS          Status;

>> +

>> +  PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);

>> +  if (PciExpressBaseAddress != MAX_UINT64) {

>> +    return EFI_SUCCESS;

>> +  }

>> +

>> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

>> +                  (VOID **)&FdtClient);

>> +  ASSERT_EFI_ERROR (Status);

>> +

>> +  Status = FdtClient->FindCompatibleNodeReg (FdtClient,

>> +                        "pci-host-ecam-generic", (CONST VOID **)&Reg,

>> +                        &RegElemSize, &RegSize);

>> +

>> +  if (EFI_ERROR (Status)) {

>> +    PciExpressBaseAddress = 0;

>> +  } else {

>> +    ASSERT (RegElemSize == sizeof (UINT64));

>> +    PciExpressBaseAddress = SwapBytes64 (*Reg);

>> +  }

>> +

>> +  PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);

>> +  PcdSetBool (PcdPciDisableBusEnumeration, FALSE);

>

> I think setting PcdPciDisableBusEnumeration unconditionally to FALSE is

> not correct. The second argument should depend on whether

> FindCompatibleNodeReg() succeeds. Probably an oversight?

>


Yes, that was an oversight.

> With the above fixed:

>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>


Thanks!
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
new file mode 100644
index 000000000000..64a3aa969680
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
@@ -0,0 +1,61 @@ 
+/** @file
+  FDT client library for consumers of PCI related dynamic PCDs
+
+  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Uefi.h>
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/FdtClient.h>
+
+RETURN_STATUS
+EFIAPI
+FdtPciPcdProducerLibConstructor (
+  VOID
+  )
+{
+  UINT64              PciExpressBaseAddress;
+  FDT_CLIENT_PROTOCOL *FdtClient;
+  CONST UINT64        *Reg;
+  UINT32              RegElemSize, RegSize;
+  EFI_STATUS          Status;
+
+  PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
+  if (PciExpressBaseAddress != MAX_UINT64) {
+    return EFI_SUCCESS;
+  }
+
+  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
+                  (VOID **)&FdtClient);
+  ASSERT_EFI_ERROR (Status);
+
+  Status = FdtClient->FindCompatibleNodeReg (FdtClient,
+                        "pci-host-ecam-generic", (CONST VOID **)&Reg,
+                        &RegElemSize, &RegSize);
+
+  if (EFI_ERROR (Status)) {
+    PciExpressBaseAddress = 0;
+  } else {
+    ASSERT (RegElemSize == sizeof (UINT64));
+    PciExpressBaseAddress = SwapBytes64 (*Reg);
+  }
+
+  PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);
+  PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
+
+  return RETURN_SUCCESS;
+}
diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
new file mode 100644
index 000000000000..546125997ba6
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
@@ -0,0 +1,47 @@ 
+#/** @file
+#  FDT client library for consumers of PCI related dynamic PCDs
+#
+#  Copyright (c) 2016, Linaro Ltd. All rights reserved.
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD License
+#  which accompanies this distribution. The full text of the license may be found at
+#  http://opensource.org/licenses/bsd-license.php
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = FdtPciPcdProducerLib
+  FILE_GUID                      = D584275B-BF1E-4DF8-A53D-980F5645C5E7
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = PciPcdProducerLib|DXE_DRIVER UEFI_DRIVER
+  CONSTRUCTOR                    = FdtPciPcdProducerLibConstructor
+
+[Sources]
+  FdtPciPcdProducerLib.c
+
+[Packages]
+  ArmVirtPkg/ArmVirtPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  PcdLib
+  UefiBootServicesTableLib
+
+[Protocols]
+  gFdtClientProtocolGuid                                      ## CONSUMES
+
+[Pcd]
+  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress           ## PRODUCES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration  ## PRODUCES
+
+[Depex]
+  gFdtClientProtocolGuid