Re: [PATCH edk2-platforms 5/8] Silicon/SynQuacer: disable PCI RC #0 DT node if disabled

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Dec 12, 2017 at 05:35:15PM +0000, Ard Biesheuvel wrote:
> On 12 December 2017 at 17:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > Suggested subject tweak:
> > Silicon/SynQuacer: disable PCI RC #0 DT node if disabled ->
> > Silicon/SynQuacer: disable PCI RC DT node if RC disabled.
> >
> 
> Ack. It's out of date, as you probably suspected
> 
> > The code below will disable either #0 or #1 node if not enabled by
> > Pcd.
> >
> > On Tue, Dec 12, 2017 at 10:38:04AM +0000, Ard Biesheuvel wrote:
> >> If PCIe RC #0 is not enabled (due to the fact that the slot is not
> >> populated), set its DT node 'status' property to 'disabled' so that
> >> the OS will not attempt to attach to it.
> >>
> >> This means we will need to switch from the default DtPlatformDtbLoaderLib
> >> to a special one for our platform.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                    |  8 +-
> >>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                        |  3 +-
> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c   | 94 ++++++++++++++++++++
> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf | 42 +++++++++
> >>  4 files changed, 141 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> >> index 5ec26f9cdd34..80728fedbc20 100644
> >> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> >> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> >> @@ -160,7 +160,8 @@ [LibraryClasses.common.DXE_CORE]
> >>    PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
> >>
> >>  [LibraryClasses.common.DXE_DRIVER]
> >> -  DtPlatformDtbLoaderLib|EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
> >> +  DtPlatformDtbLoaderLib|Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> >> +  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
> >>    SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
> >>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
> >>
> >> @@ -611,10 +612,7 @@ [Components.common]
> >>    #
> >>    # Console preference selection
> >>    #
> >> -  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf {
> >> -    <LibraryClasses>
> >> -      FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
> >> -  }
> >> +  EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
> >>
> >>    #
> >>    # DT support
> >> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> index bc8ddd452d4b..c71425664bdc 100644
> >> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> >> @@ -159,7 +159,8 @@ [LibraryClasses.common.DXE_CORE]
> >>    PerformanceLib|MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.inf
> >>
> >>  [LibraryClasses.common.DXE_DRIVER]
> >> -  DtPlatformDtbLoaderLib|EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
> >> +  DtPlatformDtbLoaderLib|Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> >> +  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
> >>    SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
> >>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
> >>
> >> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> >> new file mode 100644
> >> index 000000000000..a93a6027e64d
> >> --- /dev/null
> >> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.c
> >> @@ -0,0 +1,94 @@
> >> +/** @file
> >> +*
> >> +*  Copyright (c) 2017, 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.
> >> +*
> >> +**/
> >> +
> >> +#include <PiDxe.h>
> >> +
> >> +#include <libfdt.h>
> >> +#include <Library/BaseLib.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/DxeServicesLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +
> >> +#define DTB_PADDING               64
> >
> > If there isn't a simplish way of determining this value
> > arithmetically, can you add a comment explaining why 64 is sufficient?
> >
> 
> Something like
> 
> // add enough space for two instances of 'status = "disabled"'
> 
> ?

Good enough for me.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> 
> >> +STATIC
> >> +VOID
> >> +DisableDtNode (
> >> +  IN  VOID                        *Dtb,
> >> +  IN  CONST CHAR8                 *NodePath
> >> +  )
> >> +{
> >> +  INT32                           Node;
> >> +  INT32                           Rc;
> >> +
> >> +  Node = fdt_path_offset (Dtb, NodePath);
> >> +  if (Node < 0) {
> >> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate DT path '%a': %a\n",
> >> +      __FUNCTION__, NodePath, fdt_strerror (Node)));
> >> +    return;
> >> +  }
> >> +  Rc = fdt_setprop_string (Dtb, Node, "status", "disabled");
> >> +  if (Rc < 0) {
> >> +    DEBUG ((DEBUG_ERROR, "%a: failed to set status to 'disabled' on '%a': %a\n",
> >> +      __FUNCTION__, NodePath, fdt_strerror (Rc)));
> >> +  }
> >> +}
> >> +
> >> +/**
> >> +  Return a pool allocated copy of the DTB image that is appropriate for
> >> +  booting the current platform via DT.
> >> +
> >> +  @param[out]   Dtb                   Pointer to the DTB copy
> >> +  @param[out]   DtbSize               Size of the DTB copy
> >> +
> >> +  @retval       EFI_SUCCESS           Operation completed successfully
> >> +  @retval       EFI_NOT_FOUND         No suitable DTB image could be located
> >> +  @retval       EFI_OUT_OF_RESOURCES  No pool memory available
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +DtPlatformLoadDtb (
> >> +  OUT   VOID        **Dtb,
> >> +  OUT   UINTN       *DtbSize
> >> +  )
> >> +{
> >> +  EFI_STATUS      Status;
> >> +  VOID            *OrigDtb;
> >> +  VOID            *CopyDtb;
> >> +  UINTN           OrigDtbSize;
> >> +
> >> +  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
> >> +             EFI_SECTION_RAW, 0, &OrigDtb, &OrigDtbSize);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return EFI_NOT_FOUND;
> >> +  }
> >> +
> >> +  CopyDtb = AllocateCopyPool (OrigDtbSize + DTB_PADDING, OrigDtb);
> >> +  if (CopyDtb == NULL) {
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  if (!(PcdGet8 (PcdPcieEnableMask) & BIT0)) {
> >> +    DisableDtNode (CopyDtb, "/pcie@60000000");
> >> +  }
> >> +  if (!(PcdGet8 (PcdPcieEnableMask) & BIT1)) {
> >> +    DisableDtNode (CopyDtb, "/pcie@70000000");
> >> +  }
> >> +
> >> +  *Dtb = CopyDtb;
> >> +  *DtbSize = OrigDtbSize + DTB_PADDING;
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> >> new file mode 100644
> >> index 000000000000..e1f564f73078
> >> --- /dev/null
> >> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerDtbLoaderLib/SynQuacerDtbLoaderLib.inf
> >> @@ -0,0 +1,42 @@
> >> +/** @file
> >> +*
> >> +*  Copyright (c) 2017, 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                    = 0x0001001A
> >> +  BASE_NAME                      = SynQuacerDtbLoaderLib
> >> +  FILE_GUID                      = 59df69c4-8724-4e49-8974-d0691364338c
> >> +  MODULE_TYPE                    = DXE_DRIVER
> >> +  VERSION_STRING                 = 1.0
> >> +  LIBRARY_CLASS                  = DtPlatformDtbLoaderLib|DXE_DRIVER
> >> +
> >> +[Sources]
> >> +  SynQuacerDtbLoaderLib.c
> >> +
> >> +[Packages]
> >> +  MdePkg/MdePkg.dec
> >> +  EmbeddedPkg/EmbeddedPkg.dec
> >> +  Silicon/Socionext/SynQuacer/SynQuacer.dec
> >> +
> >> +[LibraryClasses]
> >> +  BaseLib
> >> +  DebugLib
> >> +  DxeServicesLib
> >> +  FdtLib
> >> +  MemoryAllocationLib
> >> +
> >> +[Pcd]
> >> +  gSynQuacerTokenSpaceGuid.PcdPcieEnableMask
> >> +
> >> +[Guids]
> >> +  gDtPlatformDefaultDtbFileGuid
> >> --
> >> 2.11.0
> >>