From: Pankaj Bansal <pankaj.bansal@nxp.com>

A Chassis is a base framework used for building SoCs.
We can think of Chassis/Soc/Platform(a.k.a Borad) in Object model terms.
Chassis is base. Soc is based on some Chassis.
Platform is based on some Soc.

SOCs that are designed around same chassis, reuse most of the components.

Therefore, add the package for Chassis2. LS1043A and LS1046A SOCs belong
to Chassis2.

Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
---

Notes:
    - in patch description Oops -> Object model
    - Sorted includes alphabetically
    - removed direct calls to SwapMmio** APIs and used GetMmioOperations**

 Silicon/NXP/Chassis2/Chassis2.dec                      | 23 +++++
 Silicon/NXP/NxpQoriqLs.dec                             |  4 +
 Silicon/NXP/Chassis2/Chassis2.dsc.inc                  | 10 ++
 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf | 34 +++++++
 Silicon/NXP/Chassis2/Include/Chassis.h                 | 34 +++++++
 Silicon/NXP/Include/Library/ChassisLib.h               | 51 ++++++++++
 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c   | 97 ++++++++++++++++++++
 7 files changed, 253 insertions(+)

diff --git a/Silicon/NXP/Chassis2/Chassis2.dec b/Silicon/NXP/Chassis2/Chassis2.dec
new file mode 100644
index 000000000000..a0048bd784ea
--- /dev/null
+++ b/Silicon/NXP/Chassis2/Chassis2.dec
@@ -0,0 +1,23 @@
+# @file
+# NXP Layerscape processor package.
+#
+# Copyright 2020 NXP
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+
+[Defines]
+  DEC_SPECIFICATION              = 1.27
+  PACKAGE_VERSION                = 0.1
+
+################################################################################
+#
+# Include Section - list of Include Paths that are provided by this package.
+#                   Comments are used for Keywords and Module Types.
+#
+#
+################################################################################
+[Includes.common]
+  Include                        # Root include for the package
+
diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
index 2ac047a89274..3e79f502c127 100644
--- a/Silicon/NXP/NxpQoriqLs.dec
+++ b/Silicon/NXP/NxpQoriqLs.dec
@@ -14,6 +14,9 @@
   Include
 
 [LibraryClasses]
+  ##  @libraryclass  Provides Chassis specific functions to other modules
+  ChassisLib|Include/Library/ChassisLib.h
+
   ##  @libraryclass  Provides services to read/write to I2c devices
   I2cLib|Include/Library/I2cLib.h
 
@@ -29,4 +32,5 @@
 
 [PcdsFeatureFlag]
   gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315
+  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316
 
diff --git a/Silicon/NXP/Chassis2/Chassis2.dsc.inc b/Silicon/NXP/Chassis2/Chassis2.dsc.inc
new file mode 100644
index 000000000000..db8e5a92eacb
--- /dev/null
+++ b/Silicon/NXP/Chassis2/Chassis2.dsc.inc
@@ -0,0 +1,10 @@
+#  @file
+#
+#  Copyright 2020 NXP
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+
+[LibraryClasses.common]
+  ChassisLib|Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
new file mode 100644
index 000000000000..2bb16af53134
--- /dev/null
+++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
@@ -0,0 +1,34 @@
+#  @file
+#
+#  Copyright 2020 NXP
+#
+#  SPDX-License-Identifier: BSD-2-Clause
+#
+#
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = Chassis2Lib
+  FILE_GUID                      = fae0d077-5fc2-494f-b8e1-c51a3023ee3e
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ChassisLib
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+  Silicon/NXP/Chassis2/Chassis2.dec
+  Silicon/NXP/NxpQoriqLs.dec
+
+[LibraryClasses]
+  IoAccessLib
+  IoLib
+  PcdLib
+  SerialPortLib
+
+[Sources.common]
+  ChassisLib.c
+
+[FeaturePcd]
+  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
+
diff --git a/Silicon/NXP/Chassis2/Include/Chassis.h b/Silicon/NXP/Chassis2/Include/Chassis.h
new file mode 100644
index 000000000000..72bd97efd004
--- /dev/null
+++ b/Silicon/NXP/Chassis2/Include/Chassis.h
@@ -0,0 +1,34 @@
+/** @file
+
+  Copyright 2020 NXP
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#ifndef CHASSIS_H__
+#define CHASSIS_H__
+
+#define  NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS  0x1EE0000
+
+/* SMMU Defintions */
+#define SMMU_BASE_ADDR             0x09000000
+#define SMMU_REG_SCR0              (SMMU_BASE_ADDR + 0x0)
+#define SMMU_REG_SACR              (SMMU_BASE_ADDR + 0x10)
+#define SMMU_REG_NSCR0             (SMMU_BASE_ADDR + 0x400)
+
+#define SCR0_USFCFG_MASK           0x00000400
+#define SCR0_CLIENTPD_MASK         0x00000001
+#define SACR_PAGESIZE_MASK         0x00010000
+
+/**
+  The Device Configuration Unit provides general purpose configuration and
+  status for the device. These registers only support 32-bit accesses.
+**/
+#pragma pack(1)
+typedef struct {
+  UINT8   Reserved0[0x100 - 0x0];
+  UINT32  RcwSr[16]; // Reset Control Word Status Register
+} NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG;
+#pragma pack()
+
+#endif // CHASSIS_H__
diff --git a/Silicon/NXP/Include/Library/ChassisLib.h b/Silicon/NXP/Include/Library/ChassisLib.h
new file mode 100644
index 000000000000..89992a4b6fd5
--- /dev/null
+++ b/Silicon/NXP/Include/Library/ChassisLib.h
@@ -0,0 +1,51 @@
+/** @file
+  Chassis Lib to provide Chessis specific functionality to all SOCs in
+  a Chassis.
+
+  Copyright 2020 NXP
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef CHASSIS_LIB_H__
+#define CHASSIS_LIB_H__
+
+#include <Chassis.h>
+
+/**
+  Read Dcfg register
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+**/
+UINT32
+EFIAPI
+DcfgRead32 (
+  IN  UINTN     Address
+  );
+
+/**
+  Write Dcfg register
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+  @return Value.
+**/
+UINT32
+EFIAPI
+DcfgWrite32 (
+  IN      UINTN                     Address,
+  IN      UINT32                    Value
+  );
+
+/**
+  Function to initialize Chassis Specific functions
+ **/
+VOID
+ChassisInit (
+  VOID
+  );
+
+#endif // CHASSIS_LIB_H__
diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
new file mode 100644
index 000000000000..b3bb25029dd2
--- /dev/null
+++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
@@ -0,0 +1,97 @@
+/** @file
+  Chassis specific functions common to all SOCs based on a specific Chessis
+
+  Copyright 2020 NXP
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Chassis.h>
+#include <Uefi.h>
+#include <Library/IoAccessLib.h>
+#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/SerialPortLib.h>
+
+/**
+  Read Dcfg register
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+**/
+UINT32
+EFIAPI
+DcfgRead32 (
+  IN  UINTN     Address
+  )
+{
+  MMIO_OPERATIONS_32  *DcfgOps;
+
+  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
+
+  return DcfgOps->Read32 (Address);
+}
+
+/**
+  Write Dcfg register
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+  @return Value.
+**/
+UINT32
+EFIAPI
+DcfgWrite32 (
+  IN      UINTN                     Address,
+  IN      UINT32                    Value
+  )
+{
+  MMIO_OPERATIONS_32  *DcfgOps;
+
+  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
+
+  return DcfgOps->Write32 (Address, Value);
+}
+
+/*
+ * Setup SMMU in bypass mode
+ * and also set its pagesize
+ */
+STATIC
+VOID
+SmmuInit (
+  VOID
+  )
+{
+  UINT32 Value;
+
+  /* set pagesize as 64K and ssmu-500 in bypass mode */
+  Value = (MmioRead32 ((UINTN)SMMU_REG_SACR) | SACR_PAGESIZE_MASK);
+  MmioWrite32 ((UINTN)SMMU_REG_SACR, Value);
+
+  Value = (MmioRead32 ((UINTN)SMMU_REG_SCR0) | SCR0_CLIENTPD_MASK);
+  Value &= ~SCR0_USFCFG_MASK;
+  MmioWrite32 ((UINTN)SMMU_REG_SCR0, Value);
+
+  Value = (MmioRead32 ((UINTN)SMMU_REG_NSCR0) | SCR0_CLIENTPD_MASK);
+  Value &= ~SCR0_USFCFG_MASK;
+  MmioWrite32 ((UINTN)SMMU_REG_NSCR0, Value);
+}
+
+/**
+  Function to initialize Chassis Specific functions
+ **/
+VOID
+ChassisInit (
+  VOID
+  )
+{
+  //
+  // Early init serial Port to get board information.
+  //
+  SerialPortInitialize ();
+
+  SmmuInit ();
+}
-- 
2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57322): https://edk2.groups.io/g/devel/message/57322
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Apr 15, 2020 at 17:43:34 +0530, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> A Chassis is a base framework used for building SoCs.
> We can think of Chassis/Soc/Platform(a.k.a Borad) in Object model terms.
> Chassis is base. Soc is based on some Chassis.
> Platform is based on some Soc.
> 
> SOCs that are designed around same chassis, reuse most of the components.
> 
> Therefore, add the package for Chassis2. LS1043A and LS1046A SOCs belong
> to Chassis2.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
> 
> Notes:
>     - in patch description Oops -> Object model
>     - Sorted includes alphabetically
>     - removed direct calls to SwapMmio** APIs and used GetMmioOperations**
> 
>  Silicon/NXP/Chassis2/Chassis2.dec                      | 23 +++++
>  Silicon/NXP/NxpQoriqLs.dec                             |  4 +
>  Silicon/NXP/Chassis2/Chassis2.dsc.inc                  | 10 ++
>  Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf | 34 +++++++
>  Silicon/NXP/Chassis2/Include/Chassis.h                 | 34 +++++++
>  Silicon/NXP/Include/Library/ChassisLib.h               | 51 ++++++++++
>  Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c   | 97 ++++++++++++++++++++
>  7 files changed, 253 insertions(+)
> 
> diff --git a/Silicon/NXP/Chassis2/Chassis2.dec b/Silicon/NXP/Chassis2/Chassis2.dec
> new file mode 100644
> index 000000000000..a0048bd784ea
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Chassis2.dec
> @@ -0,0 +1,23 @@
> +# @file
> +# NXP Layerscape processor package.
> +#
> +# Copyright 2020 NXP
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +
> +[Defines]
> +  DEC_SPECIFICATION              = 1.27
> +  PACKAGE_VERSION                = 0.1
> +
> +################################################################################
> +#
> +# Include Section - list of Include Paths that are provided by this package.
> +#                   Comments are used for Keywords and Module Types.
> +#
> +#
> +################################################################################
> +[Includes.common]
> +  Include                        # Root include for the package
> +
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index 2ac047a89274..3e79f502c127 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -14,6 +14,9 @@
>    Include
>  
>  [LibraryClasses]
> +  ##  @libraryclass  Provides Chassis specific functions to other modules
> +  ChassisLib|Include/Library/ChassisLib.h
> +
>    ##  @libraryclass  Provides services to read/write to I2c devices
>    I2cLib|Include/Library/I2cLib.h
>  
> @@ -29,4 +32,5 @@
>  
>  [PcdsFeatureFlag]
>    gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315
> +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316
>  
> diff --git a/Silicon/NXP/Chassis2/Chassis2.dsc.inc b/Silicon/NXP/Chassis2/Chassis2.dsc.inc
> new file mode 100644
> index 000000000000..db8e5a92eacb
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Chassis2.dsc.inc
> @@ -0,0 +1,10 @@
> +#  @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +
> +[LibraryClasses.common]
> +  ChassisLib|Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> new file mode 100644
> index 000000000000..2bb16af53134
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> @@ -0,0 +1,34 @@
> +#  @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause
> +#
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 1.27
> +  BASE_NAME                      = Chassis2Lib
> +  FILE_GUID                      = fae0d077-5fc2-494f-b8e1-c51a3023ee3e
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ChassisLib
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/NXP/Chassis2/Chassis2.dec
> +  Silicon/NXP/NxpQoriqLs.dec
> +
> +[LibraryClasses]
> +  IoAccessLib
> +  IoLib
> +  PcdLib
> +  SerialPortLib
> +
> +[Sources.common]
> +  ChassisLib.c
> +
> +[FeaturePcd]
> +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> diff --git a/Silicon/NXP/Chassis2/Include/Chassis.h b/Silicon/NXP/Chassis2/Include/Chassis.h
> new file mode 100644
> index 000000000000..72bd97efd004
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Include/Chassis.h
> @@ -0,0 +1,34 @@
> +/** @file
> +
> +  Copyright 2020 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef CHASSIS_H__
> +#define CHASSIS_H__
> +
> +#define  NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS  0x1EE0000
> +
> +/* SMMU Defintions */
> +#define SMMU_BASE_ADDR             0x09000000
> +#define SMMU_REG_SCR0              (SMMU_BASE_ADDR + 0x0)
> +#define SMMU_REG_SACR              (SMMU_BASE_ADDR + 0x10)
> +#define SMMU_REG_NSCR0             (SMMU_BASE_ADDR + 0x400)
> +
> +#define SCR0_USFCFG_MASK           0x00000400
> +#define SCR0_CLIENTPD_MASK         0x00000001
> +#define SACR_PAGESIZE_MASK         0x00010000
> +
> +/**
> +  The Device Configuration Unit provides general purpose configuration and
> +  status for the device. These registers only support 32-bit accesses.
> +**/
> +#pragma pack(1)
> +typedef struct {
> +  UINT8   Reserved0[0x100 - 0x0];
> +  UINT32  RcwSr[16]; // Reset Control Word Status Register
> +} NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG;
> +#pragma pack()
> +
> +#endif // CHASSIS_H__
> diff --git a/Silicon/NXP/Include/Library/ChassisLib.h b/Silicon/NXP/Include/Library/ChassisLib.h
> new file mode 100644
> index 000000000000..89992a4b6fd5
> --- /dev/null
> +++ b/Silicon/NXP/Include/Library/ChassisLib.h
> @@ -0,0 +1,51 @@
> +/** @file
> +  Chassis Lib to provide Chessis specific functionality to all SOCs in
> +  a Chassis.
> +
> +  Copyright 2020 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef CHASSIS_LIB_H__
> +#define CHASSIS_LIB_H__
> +
> +#include <Chassis.h>
> +
> +/**
> +  Read Dcfg register
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +**/
> +UINT32
> +EFIAPI
> +DcfgRead32 (
> +  IN  UINTN     Address
> +  );
> +
> +/**
> +  Write Dcfg register
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +  @return Value.
> +**/
> +UINT32
> +EFIAPI
> +DcfgWrite32 (
> +  IN      UINTN                     Address,
> +  IN      UINT32                    Value
> +  );
> +
> +/**
> +  Function to initialize Chassis Specific functions
> + **/
> +VOID
> +ChassisInit (
> +  VOID
> +  );
> +
> +#endif // CHASSIS_LIB_H__
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> new file mode 100644
> index 000000000000..b3bb25029dd2
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> @@ -0,0 +1,97 @@
> +/** @file
> +  Chassis specific functions common to all SOCs based on a specific Chessis
> +
> +  Copyright 2020 NXP
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Chassis.h>
> +#include <Uefi.h>
> +#include <Library/IoAccessLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +/**
> +  Read Dcfg register
> +
> +  @param  Address The MMIO register to read.
> +
> +  @return The value read.
> +**/
> +UINT32
> +EFIAPI
> +DcfgRead32 (
> +  IN  UINTN     Address
> +  )
> +{
> +  MMIO_OPERATIONS_32  *DcfgOps;
> +
> +  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
> +
> +  return DcfgOps->Read32 (Address);
> +}

The intended usage model for IoAccessLib is to retrieve the function
pointer struct once and then always refer to it. Since this is a
library, we could have a CONSTRUCTOR function (specified in the .inf)
and do something like:

STATIC MMIO_OPERATIONS mDcfgOps;

/**
  Read Dcfg register

  @param  Address The MMIO register to read.

  @return The value read.
**/
UINT32
EFIAPI
DcfgRead32 (
  IN  UINTN     Address
  )
{
  return mDcfgOps->Read32 (Address);
}

/**
  Write Dcfg register

  @param  Address The MMIO register to write.
  @param  Value   The value to write to the MMIO register.

  @return Value.

**/
UINT32
EFIAPI
DcfgWrite32 (
  IN      UINTN                     Address,
  IN      UINT32                    Value
  )
{
  return mDcfgOps->Write32 (Address, Value);
}

...

/**
  The constructor function initializes the IoAccessLib
  function pointer structure.

  @retval RETURN_SUCCESS   The constructor always returns EFI_SUCCESS.

**/
EFI_STATUS
EFIAPI
ChassisLibConstructor (
  VOID
  )
{
  mDcfgOps = GetMmioOperations (FeaturePcdGet (PcdDcfgBigEndian));

  return EFI_SUCCESS;
}

/
    Leif

> +
> +/**
> +  Write Dcfg register
> +
> +  @param  Address The MMIO register to write.
> +  @param  Value   The value to write to the MMIO register.
> +
> +  @return Value.
> +**/
> +UINT32
> +EFIAPI
> +DcfgWrite32 (
> +  IN      UINTN                     Address,
> +  IN      UINT32                    Value
> +  )
> +{
> +  MMIO_OPERATIONS_32  *DcfgOps;
> +
> +  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
> +
> +  return DcfgOps->Write32 (Address, Value);
> +}
> +
> +/*
> + * Setup SMMU in bypass mode
> + * and also set its pagesize
> + */
> +STATIC
> +VOID
> +SmmuInit (
> +  VOID
> +  )
> +{
> +  UINT32 Value;
> +
> +  /* set pagesize as 64K and ssmu-500 in bypass mode */
> +  Value = (MmioRead32 ((UINTN)SMMU_REG_SACR) | SACR_PAGESIZE_MASK);
> +  MmioWrite32 ((UINTN)SMMU_REG_SACR, Value);
> +
> +  Value = (MmioRead32 ((UINTN)SMMU_REG_SCR0) | SCR0_CLIENTPD_MASK);
> +  Value &= ~SCR0_USFCFG_MASK;
> +  MmioWrite32 ((UINTN)SMMU_REG_SCR0, Value);
> +
> +  Value = (MmioRead32 ((UINTN)SMMU_REG_NSCR0) | SCR0_CLIENTPD_MASK);
> +  Value &= ~SCR0_USFCFG_MASK;
> +  MmioWrite32 ((UINTN)SMMU_REG_NSCR0, Value);
> +}
> +
> +/**
> +  Function to initialize Chassis Specific functions
> + **/
> +VOID
> +ChassisInit (
> +  VOID
> +  )
> +{
> +  //
> +  // Early init serial Port to get board information.
> +  //
> +  SerialPortInitialize ();
> +
> +  SmmuInit ();
> +}
> -- 
> 2.17.1
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57949): https://edk2.groups.io/g/devel/message/57949
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> > +
> > +  @return The value read.
> > +**/
> > +UINT32
> > +EFIAPI
> > +DcfgRead32 (
> > +  IN  UINTN     Address
> > +  )
> > +{
> > +  MMIO_OPERATIONS_32  *DcfgOps;
> > +
> > +  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
> > +
> > +  return DcfgOps->Read32 (Address);
> > +}
> 
> The intended usage model for IoAccessLib is to retrieve the function
> pointer struct once and then always refer to it. Since this is a
> library, we could have a CONSTRUCTOR function (specified in the .inf)

I had thought of this, but decided against it because of this reason:
    The order of Library constructor call for a module cannot be guaranteed.
    https://edk2.groups.io/g/devel/message/57254  is an good example of this.
    BaseDebugLibSerialPortConstructor would need to depend on ChassisLibConstructor,
    to retrieve the UART clock frequency. If the constructor calls are not guaranteed, BaseDebugLibSerialPortConstructor
    would fail and it would cause ASSERT due to ASSERT_RETURN_ERROR (Status)

> and do something like:
> 
> STATIC MMIO_OPERATIONS mDcfgOps;
> 
> /**
>   Read Dcfg register
> 
>   @param  Address The MMIO register to read.
> 
>   @return The value read.
> **/
> UINT32
> EFIAPI
> DcfgRead32 (
>   IN  UINTN     Address
>   )
> {
>   return mDcfgOps->Read32 (Address);
> }
> 
> /**
>   Write Dcfg register
> 
>   @param  Address The MMIO register to write.
>   @param  Value   The value to write to the MMIO register.
> 
>   @return Value.
> 
> **/
> UINT32
> EFIAPI
> DcfgWrite32 (
>   IN      UINTN                     Address,
>   IN      UINT32                    Value
>   )
> {
>   return mDcfgOps->Write32 (Address, Value);
> }
> 
> ...
> 
> /**
>   The constructor function initializes the IoAccessLib
>   function pointer structure.
> 
>   @retval RETURN_SUCCESS   The constructor always returns EFI_SUCCESS.
> 
> **/
> EFI_STATUS
> EFIAPI
> ChassisLibConstructor (
>   VOID
>   )
> {
>   mDcfgOps = GetMmioOperations (FeaturePcdGet (PcdDcfgBigEndian));
> 
>   return EFI_SUCCESS;
> }
> 
> /
>     Leif
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57960): https://edk2.groups.io/g/devel/message/57960
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Apr 23, 2020 at 11:38:12 +0000, Pankaj Bansal (OSS) wrote:
> > > +
> > > +  @return The value read.
> > > +**/
> > > +UINT32
> > > +EFIAPI
> > > +DcfgRead32 (
> > > +  IN  UINTN     Address
> > > +  )
> > > +{
> > > +  MMIO_OPERATIONS_32  *DcfgOps;
> > > +
> > > +  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
> > > +
> > > +  return DcfgOps->Read32 (Address);
> > > +}
> > 
> > The intended usage model for IoAccessLib is to retrieve the function
> > pointer struct once and then always refer to it. Since this is a
> > library, we could have a CONSTRUCTOR function (specified in the .inf)
> 
> I had thought of this, but decided against it because of this reason:
>     The order of Library constructor call for a module cannot be guaranteed.
>     https://edk2.groups.io/g/devel/message/57254  is an good example of this.
>     BaseDebugLibSerialPortConstructor would need to depend on ChassisLibConstructor,
>     to retrieve the UART clock frequency. If the constructor calls are not guaranteed, BaseDebugLibSerialPortConstructor
>     would fail and it would cause ASSERT due to ASSERT_RETURN_ERROR (Status)

If this is a problem (and I recall Ard pointing out some shortcomings
in the dependency handling in the past), we can solve this with an
explicit initialisation call in the SoC or platform init code.

/
    Leif

> 
> > and do something like:
> > 
> > STATIC MMIO_OPERATIONS mDcfgOps;
> > 
> > /**
> >   Read Dcfg register
> > 
> >   @param  Address The MMIO register to read.
> > 
> >   @return The value read.
> > **/
> > UINT32
> > EFIAPI
> > DcfgRead32 (
> >   IN  UINTN     Address
> >   )
> > {
> >   return mDcfgOps->Read32 (Address);
> > }
> > 
> > /**
> >   Write Dcfg register
> > 
> >   @param  Address The MMIO register to write.
> >   @param  Value   The value to write to the MMIO register.
> > 
> >   @return Value.
> > 
> > **/
> > UINT32
> > EFIAPI
> > DcfgWrite32 (
> >   IN      UINTN                     Address,
> >   IN      UINT32                    Value
> >   )
> > {
> >   return mDcfgOps->Write32 (Address, Value);
> > }
> > 
> > ...
> > 
> > /**
> >   The constructor function initializes the IoAccessLib
> >   function pointer structure.
> > 
> >   @retval RETURN_SUCCESS   The constructor always returns EFI_SUCCESS.
> > 
> > **/
> > EFI_STATUS
> > EFIAPI
> > ChassisLibConstructor (
> >   VOID
> >   )
> > {
> >   mDcfgOps = GetMmioOperations (FeaturePcdGet (PcdDcfgBigEndian));
> > 
> >   return EFI_SUCCESS;
> > }
> > 
> > /
> >     Leif
> > 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57962): https://edk2.groups.io/g/devel/message/57962
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Thursday, April 23, 2020 5:27 PM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH edk2-platforms v3 16/24] Silicon/NXP: Add Chassis2
> Package
> 
> On Thu, Apr 23, 2020 at 11:38:12 +0000, Pankaj Bansal (OSS) wrote:
> > > > +
> > > > +  @return The value read.
> > > > +**/
> > > > +UINT32
> > > > +EFIAPI
> > > > +DcfgRead32 (
> > > > +  IN  UINTN     Address
> > > > +  )
> > > > +{
> > > > +  MMIO_OPERATIONS_32  *DcfgOps;
> > > > +
> > > > +  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
> > > > +
> > > > +  return DcfgOps->Read32 (Address);
> > > > +}
> > >
> > > The intended usage model for IoAccessLib is to retrieve the function
> > > pointer struct once and then always refer to it. Since this is a
> > > library, we could have a CONSTRUCTOR function (specified in the .inf)
> >
> > I had thought of this, but decided against it because of this reason:
> >     The order of Library constructor call for a module cannot be guaranteed.
> >     https://edk2.groups.io/g/devel/message/57254  is an good example of this.
> >     BaseDebugLibSerialPortConstructor would need to depend on
> ChassisLibConstructor,
> >     to retrieve the UART clock frequency. If the constructor calls are not
> guaranteed, BaseDebugLibSerialPortConstructor
> >     would fail and it would cause ASSERT due to ASSERT_RETURN_ERROR
> (Status)
> 
> If this is a problem (and I recall Ard pointing out some shortcomings
> in the dependency handling in the past), we can solve this with an
> explicit initialisation call in the SoC or platform init code.

You mean put ChassisLibConstructor() call in SerialPortInitialize () ?

> 
> /
>     Leif
> 
> >
> > > and do something like:
> > >
> > > STATIC MMIO_OPERATIONS mDcfgOps;
> > >
> > > /**
> > >   Read Dcfg register
> > >
> > >   @param  Address The MMIO register to read.
> > >
> > >   @return The value read.
> > > **/
> > > UINT32
> > > EFIAPI
> > > DcfgRead32 (
> > >   IN  UINTN     Address
> > >   )
> > > {
> > >   return mDcfgOps->Read32 (Address);
> > > }
> > >
> > > /**
> > >   Write Dcfg register
> > >
> > >   @param  Address The MMIO register to write.
> > >   @param  Value   The value to write to the MMIO register.
> > >
> > >   @return Value.
> > >
> > > **/
> > > UINT32
> > > EFIAPI
> > > DcfgWrite32 (
> > >   IN      UINTN                     Address,
> > >   IN      UINT32                    Value
> > >   )
> > > {
> > >   return mDcfgOps->Write32 (Address, Value);
> > > }
> > >
> > > ...
> > >
> > > /**
> > >   The constructor function initializes the IoAccessLib
> > >   function pointer structure.
> > >
> > >   @retval RETURN_SUCCESS   The constructor always returns EFI_SUCCESS.
> > >
> > > **/
> > > EFI_STATUS
> > > EFIAPI
> > > ChassisLibConstructor (
> > >   VOID
> > >   )
> > > {
> > >   mDcfgOps = GetMmioOperations (FeaturePcdGet (PcdDcfgBigEndian));
> > >
> > >   return EFI_SUCCESS;
> > > }
> > >
> > > /
> > >     Leif
> > >
> >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57963): https://edk2.groups.io/g/devel/message/57963
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Apr 23, 2020 at 12:02:16 +0000, Pankaj Bansal (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Thursday, April 23, 2020 5:27 PM
> > To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> > <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> > <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Subject: Re: [PATCH edk2-platforms v3 16/24] Silicon/NXP: Add Chassis2
> > Package
> > 
> > On Thu, Apr 23, 2020 at 11:38:12 +0000, Pankaj Bansal (OSS) wrote:
> > > > > +
> > > > > +  @return The value read.
> > > > > +**/
> > > > > +UINT32
> > > > > +EFIAPI
> > > > > +DcfgRead32 (
> > > > > +  IN  UINTN     Address
> > > > > +  )
> > > > > +{
> > > > > +  MMIO_OPERATIONS_32  *DcfgOps;
> > > > > +
> > > > > +  DcfgOps = GetMmioOperations32 (FeaturePcdGet (PcdDcfgBigEndian));
> > > > > +
> > > > > +  return DcfgOps->Read32 (Address);
> > > > > +}
> > > >
> > > > The intended usage model for IoAccessLib is to retrieve the function
> > > > pointer struct once and then always refer to it. Since this is a
> > > > library, we could have a CONSTRUCTOR function (specified in the .inf)
> > >
> > > I had thought of this, but decided against it because of this reason:
> > >     The order of Library constructor call for a module cannot be guaranteed.
> > >     https://edk2.groups.io/g/devel/message/57254  is an good example of this.
> > >     BaseDebugLibSerialPortConstructor would need to depend on
> > ChassisLibConstructor,
> > >     to retrieve the UART clock frequency. If the constructor calls are not
> > guaranteed, BaseDebugLibSerialPortConstructor
> > >     would fail and it would cause ASSERT due to ASSERT_RETURN_ERROR
> > (Status)
> > 
> > If this is a problem (and I recall Ard pointing out some shortcomings
> > in the dependency handling in the past), we can solve this with an
> > explicit initialisation call in the SoC or platform init code.
> 
> You mean put ChassisLibConstructor() call in SerialPortInitialize () ?

Or before the call to SerialPortInitialize in ChassisInit.

/
    Leif

> 
> > 
> > /
> >     Leif
> > 
> > >
> > > > and do something like:
> > > >
> > > > STATIC MMIO_OPERATIONS mDcfgOps;
> > > >
> > > > /**
> > > >   Read Dcfg register
> > > >
> > > >   @param  Address The MMIO register to read.
> > > >
> > > >   @return The value read.
> > > > **/
> > > > UINT32
> > > > EFIAPI
> > > > DcfgRead32 (
> > > >   IN  UINTN     Address
> > > >   )
> > > > {
> > > >   return mDcfgOps->Read32 (Address);
> > > > }
> > > >
> > > > /**
> > > >   Write Dcfg register
> > > >
> > > >   @param  Address The MMIO register to write.
> > > >   @param  Value   The value to write to the MMIO register.
> > > >
> > > >   @return Value.
> > > >
> > > > **/
> > > > UINT32
> > > > EFIAPI
> > > > DcfgWrite32 (
> > > >   IN      UINTN                     Address,
> > > >   IN      UINT32                    Value
> > > >   )
> > > > {
> > > >   return mDcfgOps->Write32 (Address, Value);
> > > > }
> > > >
> > > > ...
> > > >
> > > > /**
> > > >   The constructor function initializes the IoAccessLib
> > > >   function pointer structure.
> > > >
> > > >   @retval RETURN_SUCCESS   The constructor always returns EFI_SUCCESS.
> > > >
> > > > **/
> > > > EFI_STATUS
> > > > EFIAPI
> > > > ChassisLibConstructor (
> > > >   VOID
> > > >   )
> > > > {
> > > >   mDcfgOps = GetMmioOperations (FeaturePcdGet (PcdDcfgBigEndian));
> > > >
> > > >   return EFI_SUCCESS;
> > > > }
> > > >
> > > > /
> > > >     Leif
> > > >
> > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57964): https://edk2.groups.io/g/devel/message/57964
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Thursday, April 23, 2020 5:36 PM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH edk2-platforms v3 16/24] Silicon/NXP: Add Chassis2
> Package
> 
> On Thu, Apr 23, 2020 at 12:02:16 +0000, Pankaj Bansal (OSS) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Leif Lindholm <leif@nuviainc.com>
> > > Sent: Thursday, April 23, 2020 5:27 PM
> > > To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D
> Kinney
> > > <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> > > <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > > Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>; Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>
> > > Subject: Re: [PATCH edk2-platforms v3 16/24] Silicon/NXP: Add Chassis2
> > > Package
> > >
> > > On Thu, Apr 23, 2020 at 11:38:12 +0000, Pankaj Bansal (OSS) wrote:
> > > > > > +
> > > > > > +  @return The value read.
> > > > > > +**/
> > > > > > +UINT32
> > > > > > +EFIAPI
> > > > > > +DcfgRead32 (
> > > > > > +  IN  UINTN     Address
> > > > > > +  )
> > > > > > +{
> > > > > > +  MMIO_OPERATIONS_32  *DcfgOps;
> > > > > > +
> > > > > > +  DcfgOps = GetMmioOperations32 (FeaturePcdGet
> (PcdDcfgBigEndian));
> > > > > > +
> > > > > > +  return DcfgOps->Read32 (Address);
> > > > > > +}
> > > > >
> > > > > The intended usage model for IoAccessLib is to retrieve the function
> > > > > pointer struct once and then always refer to it. Since this is a
> > > > > library, we could have a CONSTRUCTOR function (specified in the .inf)
> > > >
> > > > I had thought of this, but decided against it because of this reason:
> > > >     The order of Library constructor call for a module cannot be guaranteed.
> > > >     https://edk2.groups.io/g/devel/message/57254  is an good example of
> this.
> > > >     BaseDebugLibSerialPortConstructor would need to depend on
> > > ChassisLibConstructor,
> > > >     to retrieve the UART clock frequency. If the constructor calls are not
> > > guaranteed, BaseDebugLibSerialPortConstructor
> > > >     would fail and it would cause ASSERT due to ASSERT_RETURN_ERROR
> > > (Status)
> > >
> > > If this is a problem (and I recall Ard pointing out some shortcomings
> > > in the dependency handling in the past), we can solve this with an
> > > explicit initialisation call in the SoC or platform init code.
> >
> > You mean put ChassisLibConstructor() call in SerialPortInitialize () ?
> 
> Or before the call to SerialPortInitialize in ChassisInit.

But SerialPortInitialize would be called from each module.
Each module that has SerialPort and ChassiLib linked to it would have a
local copy of mDcfgOps, which needs to be initialized.

> 
> /
>     Leif
> 
> >
> > >
> > > /
> > >     Leif
> > >
> > > >
> > > > > and do something like:
> > > > >
> > > > > STATIC MMIO_OPERATIONS mDcfgOps;
> > > > >
> > > > > /**
> > > > >   Read Dcfg register
> > > > >
> > > > >   @param  Address The MMIO register to read.
> > > > >
> > > > >   @return The value read.
> > > > > **/
> > > > > UINT32
> > > > > EFIAPI
> > > > > DcfgRead32 (
> > > > >   IN  UINTN     Address
> > > > >   )
> > > > > {
> > > > >   return mDcfgOps->Read32 (Address);
> > > > > }
> > > > >
> > > > > /**
> > > > >   Write Dcfg register
> > > > >
> > > > >   @param  Address The MMIO register to write.
> > > > >   @param  Value   The value to write to the MMIO register.
> > > > >
> > > > >   @return Value.
> > > > >
> > > > > **/
> > > > > UINT32
> > > > > EFIAPI
> > > > > DcfgWrite32 (
> > > > >   IN      UINTN                     Address,
> > > > >   IN      UINT32                    Value
> > > > >   )
> > > > > {
> > > > >   return mDcfgOps->Write32 (Address, Value);
> > > > > }
> > > > >
> > > > > ...
> > > > >
> > > > > /**
> > > > >   The constructor function initializes the IoAccessLib
> > > > >   function pointer structure.
> > > > >
> > > > >   @retval RETURN_SUCCESS   The constructor always returns
> EFI_SUCCESS.
> > > > >
> > > > > **/
> > > > > EFI_STATUS
> > > > > EFIAPI
> > > > > ChassisLibConstructor (
> > > > >   VOID
> > > > >   )
> > > > > {
> > > > >   mDcfgOps = GetMmioOperations (FeaturePcdGet (PcdDcfgBigEndian));
> > > > >
> > > > >   return EFI_SUCCESS;
> > > > > }
> > > > >
> > > > > /
> > > > >     Leif
> > > > >
> > > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57967): https://edk2.groups.io/g/devel/message/57967
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Apr 23, 2020 at 13:41:14 +0000, Pankaj Bansal (OSS) wrote:
> > > > > > The intended usage model for IoAccessLib is to retrieve the function
> > > > > > pointer struct once and then always refer to it. Since this is a
> > > > > > library, we could have a CONSTRUCTOR function (specified in the .inf)
> > > > >
> > > > > I had thought of this, but decided against it because of this reason:
> > > > >     The order of Library constructor call for a module cannot be guaranteed.
> > > > >     https://edk2.groups.io/g/devel/message/57254  is an good example of
> > this.
> > > > >     BaseDebugLibSerialPortConstructor would need to depend on
> > > > ChassisLibConstructor,
> > > > >     to retrieve the UART clock frequency. If the constructor calls are not
> > > > guaranteed, BaseDebugLibSerialPortConstructor
> > > > >     would fail and it would cause ASSERT due to ASSERT_RETURN_ERROR
> > > > (Status)
> > > >
> > > > If this is a problem (and I recall Ard pointing out some shortcomings
> > > > in the dependency handling in the past), we can solve this with an
> > > > explicit initialisation call in the SoC or platform init code.
> > >
> > > You mean put ChassisLibConstructor() call in SerialPortInitialize () ?
> > 
> > Or before the call to SerialPortInitialize in ChassisInit.
> 
> But SerialPortInitialize would be called from each module.

Why would multiple modules need to initialize the serial port?

> Each module that has SerialPort and ChassiLib linked to it would have a
> local copy of mDcfgOps, which needs to be initialized.

On the surface it makes more sense for the function that initializes
mmio accessors for chassis to be in the chassis initialization
function.

In the current tree I can only see one user of SerialPortLib and one
of ChassisLib - but you are suggesting there will be several per
platform? If so, A better splution may ne to consider wrapping
DcfgRead32/DcfgWrite32 in a protocol instead of depending on the
ChassisLib.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57971): https://edk2.groups.io/g/devel/message/57971
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> > > > You mean put ChassisLibConstructor() call in SerialPortInitialize () ?
> > >
> > > Or before the call to SerialPortInitialize in ChassisInit.
> >
> > But SerialPortInitialize would be called from each module.
> 
> Why would multiple modules need to initialize the serial port?

That's how the DebugLib has been designed.
DebugLib is used by all modules to print info on console.
BaseDebugLibSerialPortConstructor calls SerialPortInitialize.
So SerialPortInitialize is called by all the modules.

Which is the reason when I forked the BaseSerialPortLib16550,
I removed SerialPortInitalize functionality.
https://edk2.groups.io/g/devel/message/54011

> 
> > Each module that has SerialPort and ChassiLib linked to it would have a
> > local copy of mDcfgOps, which needs to be initialized.
> 
> On the surface it makes more sense for the function that initializes
> mmio accessors for chassis to be in the chassis initialization
> function.
> 
> In the current tree I can only see one user of SerialPortLib and one
> of ChassisLib - but you are suggesting there will be several per
> platform? If so, A better splution may ne to consider wrapping
> DcfgRead32/DcfgWrite32 in a protocol instead of depending on the
> ChassisLib.

The protocol would not be available in SEC and PEI phase.

> 
> /
>     Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57977): https://edk2.groups.io/g/devel/message/57977
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Apr 23, 2020 at 14:45:03 +0000, Pankaj Bansal (OSS) wrote:
> > > > > You mean put ChassisLibConstructor() call in SerialPortInitialize () ?
> > > >
> > > > Or before the call to SerialPortInitialize in ChassisInit.
> > >
> > > But SerialPortInitialize would be called from each module.
> > 
> > Why would multiple modules need to initialize the serial port?
> 
> That's how the DebugLib has been designed.
> DebugLib is used by all modules to print info on console.
> BaseDebugLibSerialPortConstructor calls SerialPortInitialize.
> So SerialPortInitialize is called by all the modules.

Sure, but the bit where ChassisLib returns the active clock
configuration does not need to happen for each initialization.
That value can be cached.

> Which is the reason when I forked the BaseSerialPortLib16550,
> I removed SerialPortInitalize functionality.
> https://edk2.groups.io/g/devel/message/54011
> 
> > 
> > > Each module that has SerialPort and ChassiLib linked to it would have a
> > > local copy of mDcfgOps, which needs to be initialized.
> > 
> > On the surface it makes more sense for the function that initializes
> > mmio accessors for chassis to be in the chassis initialization
> > function.
> > 
> > In the current tree I can only see one user of SerialPortLib and one
> > of ChassisLib - but you are suggesting there will be several per
> > platform? If so, A better splution may ne to consider wrapping
> > DcfgRead32/DcfgWrite32 in a protocol instead of depending on the
> > ChassisLib.
> 
> The protocol would not be available in SEC and PEI phase.

Fair.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57980): https://edk2.groups.io/g/devel/message/57980
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> > >
> > > Why would multiple modules need to initialize the serial port?
> >
> > That's how the DebugLib has been designed.
> > DebugLib is used by all modules to print info on console.
> > BaseDebugLibSerialPortConstructor calls SerialPortInitialize.
> > So SerialPortInitialize is called by all the modules.
> 
> Sure, but the bit where ChassisLib returns the active clock
> configuration does not need to happen for each initialization.
> That value can be cached.

The only mechanism I know for passing a cached value between different modules
is either use PCDs or use HOBs.
We have already explored both in https://edk2.groups.io/g/devel/message/57254
and https://edk2.groups.io/g/devel/message/56530 

Moreover the compiler can optimize the PcdDcfgBigEndian evaluation.
So no overhead would be observed in evaluating PcdDcfgBigEndian in every call.

> 
> > Which is the reason when I forked the BaseSerialPortLib16550,
> > I removed SerialPortInitalize functionality.
> > https://edk2.groups.io/g/devel/message/54011
> >
> > >
> > > > Each module that has SerialPort and ChassiLib linked to it would have a
> > > > local copy of mDcfgOps, which needs to be initialized.
> > >
> > > On the surface it makes more sense for the function that initializes
> > > mmio accessors for chassis to be in the chassis initialization
> > > function.
> > >
> > > In the current tree I can only see one user of SerialPortLib and one
> > > of ChassisLib - but you are suggesting there will be several per
> > > platform? If so, A better splution may ne to consider wrapping
> > > DcfgRead32/DcfgWrite32 in a protocol instead of depending on the
> > > ChassisLib.
> >
> > The protocol would not be available in SEC and PEI phase.
> 
> Fair.
> 
> /
>     Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#58012): https://edk2.groups.io/g/devel/message/58012
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Fri, Apr 24, 2020 at 02:42:13 +0000, Pankaj Bansal (OSS) wrote:
> > > > Why would multiple modules need to initialize the serial port?
> > >
> > > That's how the DebugLib has been designed.
> > > DebugLib is used by all modules to print info on console.
> > > BaseDebugLibSerialPortConstructor calls SerialPortInitialize.
> > > So SerialPortInitialize is called by all the modules.
> > 
> > Sure, but the bit where ChassisLib returns the active clock
> > configuration does not need to happen for each initialization.
> > That value can be cached.
> 
> The only mechanism I know for passing a cached value between different modules
> is either use PCDs or use HOBs.
> We have already explored both in https://edk2.groups.io/g/devel/message/57254
> and https://edk2.groups.io/g/devel/message/56530 

That was discussing what to do with regards to the generic 16550
driver. If we go with Laszlo's suggestion[1] for a separate
SerialUartClockLib instead of adding a vendor GUID HOB *into the
generic driver*, that does not preclude your using a HOB to cache the
value in your platform code for later use in your own
SerialUartClockLib implementation.

[1] https://edk2.groups.io/g/devel/message/56767

> Moreover the compiler can optimize the PcdDcfgBigEndian evaluation.
> So no overhead would be observed in evaluating PcdDcfgBigEndian in every call.

I don't question that, but that was always going to be the case anyway
- especially with LTO. The point is reducing code duplication -
especially for weird and wacky hardware workarounds.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#58066): https://edk2.groups.io/g/devel/message/58066
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 4/24/20 5:51 PM, Leif Lindholm via groups.io wrote:
> On Fri, Apr 24, 2020 at 02:42:13 +0000, Pankaj Bansal (OSS) wrote:
>>>>> Why would multiple modules need to initialize the serial port?
>>>>
>>>> That's how the DebugLib has been designed.
>>>> DebugLib is used by all modules to print info on console.
>>>> BaseDebugLibSerialPortConstructor calls SerialPortInitialize.
>>>> So SerialPortInitialize is called by all the modules.
>>>
>>> Sure, but the bit where ChassisLib returns the active clock
>>> configuration does not need to happen for each initialization.
>>> That value can be cached.
>>
>> The only mechanism I know for passing a cached value between different modules
>> is either use PCDs or use HOBs.
>> We have already explored both in https://edk2.groups.io/g/devel/message/57254
>> and https://edk2.groups.io/g/devel/message/56530
> 
> That was discussing what to do with regards to the generic 16550
> driver. If we go with Laszlo's suggestion[1] for a separate
> SerialUartClockLib instead of adding a vendor GUID HOB *into the
> generic driver*, that does not preclude your using a HOB to cache the
> value in your platform code for later use in your own
> SerialUartClockLib implementation.
> 
> [1] https://edk2.groups.io/g/devel/message/56767
> 

Caching using a HOB is a bit problematic, given that 
SerialPortInitialize() does not honour constructor ordering (it may be 
called before any of the constructors), and so whether we implement 
Laszlo's suggestion or not, using a HOB to cache anything that is 
required to set the correct baud rate is not going to work (given that 
HobLib may rely on its constructor to be able to access the HOB list)

Unfortunately, that leaves us with little else, given that we cannot use 
global variables either, since BASE libraries may be incorporated into 
PEIMs that run execute-in-place from ROM.

So the bottom line is that we don't have that many options that are 
actually feasible, and so converging on one of the non-optimal ones is 
all that we can really hope for at this point.


>> Moreover the compiler can optimize the PcdDcfgBigEndian evaluation.
>> So no overhead would be observed in evaluating PcdDcfgBigEndian in every call.
> 
> I don't question that, but that was always going to be the case anyway
> - especially with LTO. The point is reducing code duplication -
> especially for weird and wacky hardware workarounds.
> 

Have to agree here :-)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#58260): https://edk2.groups.io/g/devel/message/58260
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Apr 28, 2020 at 19:46:36 +0200, Ard Biesheuvel wrote:
> On 4/24/20 5:51 PM, Leif Lindholm via groups.io wrote:
> > On Fri, Apr 24, 2020 at 02:42:13 +0000, Pankaj Bansal (OSS) wrote:
> > > > > > Why would multiple modules need to initialize the serial port?
> > > > > 
> > > > > That's how the DebugLib has been designed.
> > > > > DebugLib is used by all modules to print info on console.
> > > > > BaseDebugLibSerialPortConstructor calls SerialPortInitialize.
> > > > > So SerialPortInitialize is called by all the modules.
> > > > 
> > > > Sure, but the bit where ChassisLib returns the active clock
> > > > configuration does not need to happen for each initialization.
> > > > That value can be cached.
> > > 
> > > The only mechanism I know for passing a cached value between different modules
> > > is either use PCDs or use HOBs.
> > > We have already explored both in https://edk2.groups.io/g/devel/message/57254
> > > and https://edk2.groups.io/g/devel/message/56530
> > 
> > That was discussing what to do with regards to the generic 16550
> > driver. If we go with Laszlo's suggestion[1] for a separate
> > SerialUartClockLib instead of adding a vendor GUID HOB *into the
> > generic driver*, that does not preclude your using a HOB to cache the
> > value in your platform code for later use in your own
> > SerialUartClockLib implementation.
> > 
> > [1] https://edk2.groups.io/g/devel/message/56767
> > 
> 
> Caching using a HOB is a bit problematic, given that SerialPortInitialize()
> does not honour constructor ordering (it may be called before any of the
> constructors), and so whether we implement Laszlo's suggestion or not, using
> a HOB to cache anything that is required to set the correct baud rate is not
> going to work (given that HobLib may rely on its constructor to be able to
> access the HOB list)
> 
> Unfortunately, that leaves us with little else, given that we cannot use
> global variables either, since BASE libraries may be incorporated into PEIMs
> that run execute-in-place from ROM.
> 
> So the bottom line is that we don't have that many options that are actually
> feasible, and so converging on one of the non-optimal ones is all that we
> can really hope for at this point.

Argh, OK. I hadn't tweaked this - so thanks for pointing it out.

OK, given that:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
for this patch in its current state.

Just, please use a single call to set up the struct pointer for future
components where that is possible.

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#58261): https://edk2.groups.io/g/devel/message/58261
Mute This Topic: https://groups.io/mt/73008827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-