Re: [PATCH edk2-platforms 3/7] Platform/NXP: LX2160aRdbPkg: Add PlatformDxe driver

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Jun 10, 2020 at 03:47:26 +0530, Wasim Khan wrote:
> From: Wasim Khan <wasim.khan@nxp.com>
> 
> Add PlatformDxe to do platform specific work.
> At present it perform platform specific Pci initialization.
 
> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> ---
>  Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf | 35 +++++++++
>  Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c   | 78 ++++++++++++++++++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
> new file mode 100644
> index 000000000000..2514adf1d69d
> --- /dev/null
> +++ b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -0,0 +1,35 @@
> +## @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION               = 0x00010019
> +  BASE_NAME                 = PlatformDxe
> +  FILE_GUID                 = C4063EBA-7729-11EA-BC55-0232AC130003
> +  MODULE_TYPE               = DXE_DRIVER
> +  VERSION_STRING            = 1.0
> +  ENTRY_POINT               = PlatformDxeEntryPoint
> +
> +[Sources]
> +  PlatformDxe.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  Silicon/NXP/Chassis3V2/Chassis3V2.dec
> +  Silicon/NXP/LX2160A/LX2160A.dec
> +  Silicon/NXP/NxpQoriqLs.dec
> +
> +[LibraryClasses]
> +  PcdLib
> +  UefiDriverEntryPoint
> +
> +[Pcd]
> +  gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable
> +  gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl
> +
> +[Depex]
> +  TRUE
> diff --git a/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c
> new file mode 100644
> index 000000000000..73599aaeb7bf
> --- /dev/null
> +++ b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -0,0 +1,78 @@
> +/** @file
> +*
> +*  Copyright 2020 NXP
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +#include <Library/PcdLib.h>
> +#include <Library/SocLib.h>
> +#include <Soc.h>
> +
> +/**
> +  Enable PciCfgShift feature for LX2160-Rev2

... which means what?

> +
> +**/
> +VOID
> +EnableCfgShift (

But even more importantly, that isn't what this function does:

> +  VOID
> +  )
> +{
> +  UINT32 Svr;
> +
> +  Svr = SocGetSvr ();
> +  if ((SVR_SOC_VER(Svr) == SVR_LX2160A) && (SVR_MAJOR(Svr) == 0x2)) {

... it performs variant/version detection and sets feature flags based
on it. The fact that it currently happens to be setting only a single
flag shouldn't mean that in order to understand what PlatformPciInit()
does even at a high level, I need to know what an "EnableCfgShift" is.

The function should be called something like DetectSocVersion,
EnableSocRevisionWorkarounds, or something other descriptive.

*Then* there should be a comment here describing what
PcdPciCfgShiftEnable is and what it does.

> +    PcdSetBoolS (PcdPciCfgShiftEnable, TRUE);
> +  }
> +}
> +
> +/**
> +  Enable Layerscape Gen4 controller for LX2160A-Rev1
> +
> +**/
> +VOID
> +EnablePciController (
> +  VOID
> +  )
> +{
> +  UINT32 Svr;
> +
> +  Svr = SocGetSvr ();
> +  if ((SVR_SOC_VER(Svr) == SVR_LX2160A) && (SVR_MAJOR(Svr) == 0x1)) {
> +    PcdSetBoolS (PcdPciLsGen4Ctrl, TRUE);

Again, this doesn't *enable* a controller, which is what the function
name says it does. It sets a Pcd, which is then used by subsequent
code. I don't know if this function should be smashed together with
the previous one under an even more generic name, but this too
motivates a comment on what a PcdPciLsGen4Ctrl is.

> +  }
> +}
> +
> +/**
> +  Platfrom Specific PCI Initialization

Platform

/
    Leif

> +
> +**/
> +VOID
> +PlatformPciInit (
> +  VOID
> +  )
> +{
> +  EnableCfgShift ();
> +  EnablePciController ();
> +}
> +
> +/**
> +  The entry point for PlatformDxe driver. This driver
> +  intends to perform platform specific initialization.
> +
> +  @param[in] ImageHandle     The image handle of the driver.
> +  @param[in] SystemTable     The system table.
> +
> +  @retval EFI_SUCCESS         Driver initialization success.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +PlatformDxeEntryPoint (
> +  IN EFI_HANDLE                   ImageHandle,
> +  IN EFI_SYSTEM_TABLE             *SystemTable
> +  )
> +{
> +  // Platfrom Specific PCI Initialization
> +  PlatformPciInit ();
> +  return EFI_SUCCESS;
> +}
> -- 
> 2.7.4
>