[PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY


Vikas Singh
 

This patch series basically aims to extend the Dynamic ACPI
framework towards NXP's LS1046AFRWY platform.

In continuation to https://edk2.groups.io/g/devel/message/71709

The change set in the series is in below order -

(1)Introducing a new platform specific macro "PLAT_SOC_NAME"
This macro will be consumed by Configuration Manager(CM).
Platforms who extends CM services for themselves must notify
their SoC details to CM using this macro only.
Additionally also update the lx2160ardb platform header with
PLAT_SOC_NAME, this will be consumed by CM.

(2)Introduced a function to get SoC's System Version Register(SVR)
This function will fetch SVR details for LS1046A SoC based platforms.
In current patch series, this function will be used by LS1046aFrwyPkg.

(3)Extending Configuration Manager (CM) and its services to leverage
the Dynamic ACPI support for NXP's LS1046aFrwy platform.

(4)Introduced an OEM specific firmware acpi table generator
Also add Dsdt.asl as a place holder having only platform's clock
related dsdt properties for now and will accommodate other IP specific
dsdt tables(acpi properties) for LS1046AFRWY in future patch series.

Vikas Singh (4):
Platform/NXP: Make SoC version log in ConfigurationManager generic
Silicon/NXP: Add support of SVR handling for LS1046A SoC
NXP/LS1046aFrwyPkg: Enable ConfigurationManager on LS1046AFRWY
Platform/NXP/LS1046aFrwyPkg: Add OEM specific DSDT generator

Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c | 11 +-
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl | 60 ++++++++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl | 15 ++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf | 39 +++++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c | 138 +++++++++++++++++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h | 23 +++
Platform/NXP/LS1046aFrwyPkg/Include/Platform.h | 156 ++++++++++++++++++++
Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc | 29 ++++
Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf | 13 ++
Platform/NXP/LX2160aRdbPkg/Include/Platform.h | 8 +-
Silicon/NXP/LS1046A/LS1046A.dsc.inc | 11 ++
Silicon/NXP/LS1046A/Library/SocLib/SocLib.c | 16 ++
Silicon/NXP/LX2160A/LX2160A.dsc.inc | 3 +-
13 files changed, 508 insertions(+), 14 deletions(-)
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h
create mode 100644 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h

--
2.25.1


Sunny Wang
 

Hi Vikas,

Thanks for working on this.

As our offline discussion with NXP, our goal is to make the Tiano edk2-platform and NXP LSDK opensource https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms in sync. Now the main problem is that some folders' names and locations have been changed to be different from NXP LSDK opensource in previous commits, which causes difficulty in doing synchronization between Tiano edk2-platform and NXP LSDK opensource and also causes LSDK user's confusion. I'm fine with keeping some changes that are needed for cleanup purposes or fixing build issues. However, I think we can still avoid some folder-renaming or folder-moving changes. For avoiding them, could you check my questions/comments below?

1. Why do we need to have ConfigurationManagerPkg.dec? Can we remove this? After removing it, we can rename the ConfigurationManagerPkg folder back to ConfigurationManager to be consistent with other platforms (JunoPkg).
2. Can we move \Platform\NXP\LX2160aRdbPkg\Include\Platform.h to the same location as LSDK (\Platform\NXP\LX2160aRdbPkg\AcpiTables\)?
3. Can we move \Platform\NXP\LS1046aFrwyPkg\Include\Platform.h t h to the same location as LSDK (\Platform\NXP\ LS1046aFrwyPkg\AcpiTables\)?
4. Can we add \Silicon\NXP\LS1046A\Library\SocFixupLib\ for patch 2/4 (adding SocGetSvr() function)? Furthermore, can we just add the whole Silicon\NXP\LS1046A\Library\SocFixupLib\ from NXP LSDK?

Add few more NXP guys.



Hi Leif and Meenakshi,

Can we just push the latest LSDK to the Tiano edk2-platform in one patch set? Then, If there is anything that needs to be cleaned up like the Coding style issue, we can create an issue in Bugzilla for it. What do you guys think?


Best Regards,
Sunny Wang

-----Original Message-----
From: Vikas Singh <vikas.singh@puresoftware.com>
Sent: Friday, June 18, 2021 11:28 PM
To: devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@arm.com>; leif@nuviainc.com; Meenakshi Aggarwal (meenakshi.aggarwal@nxp.com) <meenakshi.aggarwal@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; V Sethi (v.sethi@nxp.com) <v.sethi@nxp.com>; arokia.samy <arokia.samy@puresoftware.com>; kuldip.dwivedi@puresoftware.com; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; vikas.singh@nxp.com; Sunny Wang <Sunny.Wang@arm.com>
Subject: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

This patch series basically aims to extend the Dynamic ACPI
framework towards NXP's LS1046AFRWY platform.

In continuation to https://edk2.groups.io/g/devel/message/71709

The change set in the series is in below order -

(1)Introducing a new platform specific macro "PLAT_SOC_NAME"
This macro will be consumed by Configuration Manager(CM).
Platforms who extends CM services for themselves must notify
their SoC details to CM using this macro only.
Additionally also update the lx2160ardb platform header with
PLAT_SOC_NAME, this will be consumed by CM.

(2)Introduced a function to get SoC's System Version Register(SVR)
This function will fetch SVR details for LS1046A SoC based platforms.
In current patch series, this function will be used by LS1046aFrwyPkg.

(3)Extending Configuration Manager (CM) and its services to leverage
the Dynamic ACPI support for NXP's LS1046aFrwy platform.

(4)Introduced an OEM specific firmware acpi table generator
Also add Dsdt.asl as a place holder having only platform's clock
related dsdt properties for now and will accommodate other IP specific
dsdt tables(acpi properties) for LS1046AFRWY in future patch series.

Vikas Singh (4):
Platform/NXP: Make SoC version log in ConfigurationManager generic
Silicon/NXP: Add support of SVR handling for LS1046A SoC
NXP/LS1046aFrwyPkg: Enable ConfigurationManager on LS1046AFRWY
Platform/NXP/LS1046aFrwyPkg: Add OEM specific DSDT generator

Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c | 11 +-
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl | 60 ++++++++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl | 15 ++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf | 39 +++++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c | 138 +++++++++++++++++
Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h | 23 +++
Platform/NXP/LS1046aFrwyPkg/Include/Platform.h | 156 ++++++++++++++++++++
Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc | 29 ++++
Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf | 13 ++
Platform/NXP/LX2160aRdbPkg/Include/Platform.h | 8 +-
Silicon/NXP/LS1046A/LS1046A.dsc.inc | 11 ++
Silicon/NXP/LS1046A/Library/SocLib/SocLib.c | 16 ++
Silicon/NXP/LX2160A/LX2160A.dsc.inc | 3 +-
13 files changed, 508 insertions(+), 14 deletions(-)
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h
create mode 100644 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h

--
2.25.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Vikas Singh
 

Sunny,    thanks for your review and PSB my remarks.


From: Sunny Wang <Sunny.Wang@...>
Sent: Monday, July 12, 2021 4:03 PM
To: Vikas Singh <vikas.singh@...>; devel@edk2.groups.io <devel@edk2.groups.io>; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; leif@... <leif@...>
Cc: Sami Mujawar <Sami.Mujawar@...>; leif@... <leif@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; Arokia Samy <arokia.samy@...>; Kuldip Dwivedi <kuldip.dwivedi@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@... <vikas.singh@...>; White Weng <white.weng@...>; Ran Wang <ran.wang_1@...>; Sunny Wang <Sunny.Wang@...>; Joe Byrne <joseph.byrne@...>
Subject: RE: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY
 
Hi Vikas,

Thanks for working on this.

As our offline discussion with NXP, our goal is to make the Tiano edk2-platform and NXP LSDK opensource https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms in sync. Now the main problem is that some folders' names and locations have been changed to be different from NXP LSDK opensource in previous commits, which causes difficulty in doing synchronization between Tiano edk2-platform and NXP LSDK opensource and also causes LSDK user's confusion.  I'm fine with keeping some changes that are needed for cleanup purposes or fixing build issues. However, I think we can still avoid some folder-renaming or folder-moving changes. For avoiding them, could you check my questions/comments below?

1. Why do we need to have ConfigurationManagerPkg.dec? Can we remove this? After removing it, we can rename the ConfigurationManagerPkg folder back to ConfigurationManager to be consistent with other platforms (JunoPkg).
[[Vikas]] ConfigurationManagerPkg folder should not be renamed back to ConfigurationManager because of the hierarchy and placement of this folder as a common generic for all the platform pkgs.
This is already been discussed with leif and moreover in case of JunoPkg the CM is private to JunoPkg not visible to other ARM platform pkgs. Here in our case CM serves all the NXP platforms.

2. Can we move \Platform\NXP\LX2160aRdbPkg\Include\Platform.h to the same location as LSDK (\Platform\NXP\LX2160aRdbPkg\AcpiTables\)?
[[Vikas]] This involves extra effort/rework.

3. Can we move \Platform\NXP\LS1046aFrwyPkg\Include\Platform.h t h to the same location as LSDK (\Platform\NXP\ LS1046aFrwyPkg\AcpiTables\)?
[[Vikas]] This involves extra effort/rework.

4. Can we add \Silicon\NXP\LS1046A\Library\SocFixupLib\ for patch 2/4 (adding SocGetSvr() function)? Furthermore, can we just add the whole Silicon\NXP\LS1046A\Library\SocFixupLib\ from NXP LSDK?
[[Vikas]] This involves extra effort/rework.

Add few more NXP guys.



Hi Leif and Meenakshi,

Can we just push the latest LSDK to the Tiano edk2-platform in one patch set? Then, If there is anything that needs to be cleaned up like the Coding style issue, we can create an issue in Bugzilla for it. What do you guys think?


Best Regards,
Sunny Wang

-----Original Message-----
From: Vikas Singh <vikas.singh@...>
Sent: Friday, June 18, 2021 11:28 PM
To: devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@...>; leif@...; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; arokia.samy <arokia.samy@...>; kuldip.dwivedi@...; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@...; Sunny Wang <Sunny.Wang@...>
Subject: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

This patch series basically aims to extend the Dynamic ACPI
framework towards NXP's LS1046AFRWY platform.

In continuation to https://edk2.groups.io/g/devel/message/71709

The change set in the series is in below order -

(1)Introducing a new platform specific macro "PLAT_SOC_NAME"
This macro will be consumed by Configuration Manager(CM).
Platforms who extends CM services for themselves must notify
their SoC details to CM using this macro only.
Additionally also update the lx2160ardb platform header with
PLAT_SOC_NAME, this will be consumed by CM.

(2)Introduced a function to get SoC's System Version Register(SVR)
This function will fetch SVR details for LS1046A SoC based platforms.
In current patch series, this function will be used by LS1046aFrwyPkg.

(3)Extending Configuration Manager (CM) and its services to leverage
the Dynamic ACPI support for NXP's LS1046aFrwy platform.

(4)Introduced an OEM specific firmware acpi table generator
Also add Dsdt.asl as a place holder having only platform's clock
related dsdt properties for now and will accommodate other IP specific
dsdt tables(acpi properties) for LS1046AFRWY in future patch series.

Vikas Singh (4):
  Platform/NXP: Make SoC version log in ConfigurationManager generic
  Silicon/NXP: Add support of SVR handling for LS1046A SoC
  NXP/LS1046aFrwyPkg: Enable ConfigurationManager on LS1046AFRWY
  Platform/NXP/LS1046aFrwyPkg: Add OEM specific DSDT generator

 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c  |  11 +-
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl                           |  60 ++++++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl                          |  15 ++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf                |  39 +++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c | 138 +++++++++++++++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h                      |  23 +++
 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h                                       | 156 ++++++++++++++++++++
 Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc                                       |  29 ++++
 Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf                                       |  13 ++
 Platform/NXP/LX2160aRdbPkg/Include/Platform.h                                        |   8 +-
 Silicon/NXP/LS1046A/LS1046A.dsc.inc                                                  |  11 ++
 Silicon/NXP/LS1046A/Library/SocLib/SocLib.c                                          |  16 ++
 Silicon/NXP/LX2160A/LX2160A.dsc.inc                                                  |   3 +-
 13 files changed, 508 insertions(+), 14 deletions(-)
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h

--
2.25.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Sunny Wang
 

Hi Vikas,

 

Sorry for the delay and thanks for clarifying. Added my further rcomments below:

  1. For your previous discussion with Leif, do you mean this one https://edk2.groups.io/g/devel/message/70090? If my understanding of your previous discussion is correct, the reason why you needed to rename ConfigurationManager to ConfigurationManagerPkg is that you created ConfigurationManagerPkg.dec file. However, I don’t see a need to create ConfigurationManagerPkg.dec. At least, the current NXP LSDK can work without it. Did you have any plan to use ConfigurationManagerPkg.dec? Moreover, for putting common files for all platforms to consume, it doesn't have to be an XXXXPkg folder and doesn't need a .dec file. In your previous discussion with Leif (https://edk2.groups.io/g/devel/message/70090), It looks like Leif was also asking to remove ConfigurationManagerPkg.dec if the purpose of adding the .dec file is just for all platforms to include common header files in this folder. Actually, I thought we would only name the folder with the postfix string “Pkg” when the folder needs to be separately built. If my thought is correct, I think we don’t need to separately build ConfigurationManager.
  2. After thinking this over, I think \Platform\NXP\LX2160aRdbPkg\Include\ is a better place for putting LX2160aRdbPkg’s Platform.h, so this change looks good to me.
  3. After thinking this over, I think \Platform\NXP\ LS1046aFrwyPkg\Include\ is a better place for putting LS1046aFrwyPkg’s Platform.h, so this change looks good to me.
  4. Could you give me more information about why you don’t want to put extra effort at this moment? It looks worth doing this because we will need to add the whole Silicon\NXP\LS1046A\Library\SocFixupLib\ from NXP LSDK later and adding it now can avoid confusion caused by the mismatch between edk2 and NXP LSDK. Why don’t we add it now?

  5. Best Regards,

    Sunny Wang

From: Vikas Singh <vikas.singh@...>
Sent: Monday, July 26, 2021 9:04 PM
To: Sunny Wang <Sunny.Wang@...>; devel@edk2.groups.io; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; leif@...
Cc: Sami Mujawar <Sami.Mujawar@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; Arokia Samy <arokia.samy@...>; Kuldip Dwivedi <kuldip.dwivedi@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@...; White Weng <white.weng@...>; Ran Wang <ran.wang_1@...>; Joe Byrne <joseph.byrne@...>
Subject: Re: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

 

Sunny,    thanks for your review and PSB my remarks.

 


From: Sunny Wang <Sunny.Wang@...>
Sent: Monday, July 12, 2021 4:03 PM
To: Vikas Singh <vikas.singh@...>; devel@edk2.groups.io <devel@edk2.groups.io>; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; leif@... <leif@...>
Cc: Sami Mujawar <Sami.Mujawar@...>; leif@... <leif@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; Arokia Samy <arokia.samy@...>; Kuldip Dwivedi <kuldip.dwivedi@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@... <vikas.singh@...>; White Weng <white.weng@...>; Ran Wang <ran.wang_1@...>; Sunny Wang <Sunny.Wang@...>; Joe Byrne <joseph.byrne@...>
Subject: RE: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

 

Hi Vikas,

Thanks for working on this.

As our offline discussion with NXP, our goal is to make the Tiano edk2-platform and NXP LSDK opensource https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms in sync. Now the main problem is that some folders' names and locations have been changed to be different from NXP LSDK opensource in previous commits, which causes difficulty in doing synchronization between Tiano edk2-platform and NXP LSDK opensource and also causes LSDK user's confusion.  I'm fine with keeping some changes that are needed for cleanup purposes or fixing build issues. However, I think we can still avoid some folder-renaming or folder-moving changes. For avoiding them, could you check my questions/comments below?

1. Why do we need to have ConfigurationManagerPkg.dec? Can we remove this? After removing it, we can rename the ConfigurationManagerPkg folder back to ConfigurationManager to be consistent with other platforms (JunoPkg).

[[Vikas]] ConfigurationManagerPkg folder should not be renamed back to ConfigurationManager because of the hierarchy and placement of this folder as a common generic for all the platform pkgs.

This is already been discussed with leif and moreover in case of JunoPkg the CM is private to JunoPkg not visible to other ARM platform pkgs. Here in our case CM serves all the NXP platforms.


2. Can we move \Platform\NXP\LX2160aRdbPkg\Include\Platform.h to the same location as LSDK (\Platform\NXP\LX2160aRdbPkg\AcpiTables\)?

[[Vikas]] This involves extra effort/rework.


3. Can we move \Platform\NXP\LS1046aFrwyPkg\Include\Platform.h t h to the same location as LSDK (\Platform\NXP\ LS1046aFrwyPkg\AcpiTables\)?

[[Vikas]] This involves extra effort/rework.


4. Can we add \Silicon\NXP\LS1046A\Library\SocFixupLib\ for patch 2/4 (adding SocGetSvr() function)? Furthermore, can we just add the whole Silicon\NXP\LS1046A\Library\SocFixupLib\ from NXP LSDK?
[[Vikas]] This involves extra effort/rework.


Add few more NXP guys.



Hi Leif and Meenakshi,

Can we just push the latest LSDK to the Tiano edk2-platform in one patch set? Then, If there is anything that needs to be cleaned up like the Coding style issue, we can create an issue in Bugzilla for it. What do you guys think?


Best Regards,
Sunny Wang

-----Original Message-----
From: Vikas Singh <vikas.singh@...>
Sent: Friday, June 18, 2021 11:28 PM
To: devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@...>; leif@...; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; arokia.samy <arokia.samy@...>; kuldip.dwivedi@...; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@...; Sunny Wang <Sunny.Wang@...>
Subject: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

This patch series basically aims to extend the Dynamic ACPI
framework towards NXP's LS1046AFRWY platform.

In continuation to https://edk2.groups.io/g/devel/message/71709

The change set in the series is in below order -

(1)Introducing a new platform specific macro "PLAT_SOC_NAME"
This macro will be consumed by Configuration Manager(CM).
Platforms who extends CM services for themselves must notify
their SoC details to CM using this macro only.
Additionally also update the lx2160ardb platform header with
PLAT_SOC_NAME, this will be consumed by CM.

(2)Introduced a function to get SoC's System Version Register(SVR)
This function will fetch SVR details for LS1046A SoC based platforms.
In current patch series, this function will be used by LS1046aFrwyPkg.

(3)Extending Configuration Manager (CM) and its services to leverage
the Dynamic ACPI support for NXP's LS1046aFrwy platform.

(4)Introduced an OEM specific firmware acpi table generator
Also add Dsdt.asl as a place holder having only platform's clock
related dsdt properties for now and will accommodate other IP specific
dsdt tables(acpi properties) for LS1046AFRWY in future patch series.

Vikas Singh (4):
  Platform/NXP: Make SoC version log in ConfigurationManager generic
  Silicon/NXP: Add support of SVR handling for LS1046A SoC
  NXP/LS1046aFrwyPkg: Enable ConfigurationManager on LS1046AFRWY
  Platform/NXP/LS1046aFrwyPkg: Add OEM specific DSDT generator

 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c  |  11 +-
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl                           |  60 ++++++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl                          |  15 ++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf                |  39 +++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c | 138 +++++++++++++++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h                      |  23 +++
 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h                                       | 156 ++++++++++++++++++++
 Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc                                       |  29 ++++
 Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf                                       |  13 ++
 Platform/NXP/LX2160aRdbPkg/Include/Platform.h                                        |   8 +-
 Silicon/NXP/LS1046A/LS1046A.dsc.inc                                                  |  11 ++
 Silicon/NXP/LS1046A/Library/SocLib/SocLib.c                                          |  16 ++
 Silicon/NXP/LX2160A/LX2160A.dsc.inc                                                  |   3 +-
 13 files changed, 508 insertions(+), 14 deletions(-)
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h

--
2.25.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Vikas Singh
 

Sunny, Apologies for late response. (Not keeping well these days)

Ok, PSB my remarks.

 

#1. True, if an entity is independently getting build must be prefixed with Pkg. In my previous discussion with Leif (https://edk2.groups.io/g/devel/message/70090) Leif requested a reason of this kind of setup.

So basically Here the ConfigurationManager is postfixed with  “Pkg” because it has its own .dec and the reason being having this .dec is like CM exposes few headers which are OEM/platform specific (generic in nature consumed by platforms, not by CM itself). In case when platform try to expose its own OEM specific generators these headers cannot come from DynamicTablePkg(edk2) and even platforms are nearly clueless about these headers. So kept these headers as part of CM and exposed them using CM’s  .dec in platforms inf files as and when needed. Not sure if this type of setup is acceptable, but seems that Lief was OK, plz check with leif and let me know if at all any correction needed.

I will check and plan modifications accordingly.

 

#2. Good to hear that.

#3. Got to hear that.

 

#4. Sunny renaming to directory structure and changes as such are inevitable. In my opinion we must choose what represents our software best. For instance plz check these 2 repo instance, they are already out of sync from LSDK.

LSDK specific à https://github.com/ossdev07/edk2-platforms/tree/UEFI_ACPI_EAR1-PS-Devel/Silicon/NXP/LX2160A/Library

Tianocore à https://github.com/tianocore/edk2-platforms/tree/master/Silicon/NXP/LX2160A/Library

 

Now, referring the “SocFixupLib” as you mentioned, I would suggest pushing changes as and when needed makes more sense.

No point of pushing something which is not needed or is not being used by someone currently.

However I’m ok if you conclude it to be pushed in advance but still there is nothing called as “SocFixupLib” in current tianocore repos.

 

Let me know if I can help further.

 

Regards,

Vikas Singh

 

From: Sunny Wang <Sunny.Wang@...>
Sent: 04 August 2021 14:17
To: Vikas Singh <vikas.singh@...>; devel@edk2.groups.io; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; leif@...
Cc: Sami Mujawar <Sami.Mujawar@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; Arokia Samy <arokia.samy@...>; Kuldip Dwivedi <kuldip.dwivedi@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@...; White Weng <white.weng@...>; Ran Wang <ran.wang_1@...>; Joe Byrne <joseph.byrne@...>; Sunny Wang <Sunny.Wang@...>
Subject: RE: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

 

Hi Vikas,

 

Sorry for the delay and thanks for clarifying. Added my further rcomments below:

  1. For your previous discussion with Leif, do you mean this one https://edk2.groups.io/g/devel/message/70090? If my understanding of your previous discussion is correct, the reason why you needed to rename ConfigurationManager to ConfigurationManagerPkg is that you created ConfigurationManagerPkg.dec file. However, I don’t see a need to create ConfigurationManagerPkg.dec. At least, the current NXP LSDK can work without it. Did you have any plan to use ConfigurationManagerPkg.dec? Moreover, for putting common files for all platforms to consume, it doesn't have to be an XXXXPkg folder and doesn't need a .dec file. In your previous discussion with Leif (https://edk2.groups.io/g/devel/message/70090), It looks like Leif was also asking to remove ConfigurationManagerPkg.dec if the purpose of adding the .dec file is just for all platforms to include common header files in this folder. Actually, I thought we would only name the folder with the postfix string “Pkg” when the folder needs to be separately built. If my thought is correct, I think we don’t need to separately build ConfigurationManager.
  2. After thinking this over, I think \Platform\NXP\LX2160aRdbPkg\Include\ is a better place for putting LX2160aRdbPkg’s Platform.h, so this change looks good to me.
  3. After thinking this over, I think \Platform\NXP\ LS1046aFrwyPkg\Include\ is a better place for putting LS1046aFrwyPkg’s Platform.h, so this change looks good to me.
  4. Could you give me more information about why you don’t want to put extra effort at this moment? It looks worth doing this because we will need to add the whole Silicon\NXP\LS1046A\Library\SocFixupLib\ from NXP LSDK later and adding it now can avoid confusion caused by the mismatch between edk2 and NXP LSDK. Why don’t we add it now?


Best Regards,

Sunny Wang

From: Vikas Singh <vikas.singh@...>
Sent: Monday, July 26, 2021 9:04 PM
To: Sunny Wang <Sunny.Wang@...>; devel@edk2.groups.io; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; leif@...
Cc: Sami Mujawar <Sami.Mujawar@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; Arokia Samy <arokia.samy@...>; Kuldip Dwivedi <kuldip.dwivedi@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@...; White Weng <white.weng@...>; Ran Wang <ran.wang_1@...>; Joe Byrne <joseph.byrne@...>
Subject: Re: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

 

Sunny,    thanks for your review and PSB my remarks.

 


From: Sunny Wang <Sunny.Wang@...>
Sent: Monday, July 12, 2021 4:03 PM
To: Vikas Singh <vikas.singh@...>; devel@edk2.groups.io <devel@edk2.groups.io>; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; leif@... <leif@...>
Cc: Sami Mujawar <Sami.Mujawar@...>; leif@... <leif@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; Arokia Samy <arokia.samy@...>; Kuldip Dwivedi <kuldip.dwivedi@...>; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@... <vikas.singh@...>; White Weng <white.weng@...>; Ran Wang <ran.wang_1@...>; Sunny Wang <Sunny.Wang@...>; Joe Byrne <joseph.byrne@...>
Subject: RE: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

 

Hi Vikas,

Thanks for working on this.

As our offline discussion with NXP, our goal is to make the Tiano edk2-platform and NXP LSDK opensource https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms in sync. Now the main problem is that some folders' names and locations have been changed to be different from NXP LSDK opensource in previous commits, which causes difficulty in doing synchronization between Tiano edk2-platform and NXP LSDK opensource and also causes LSDK user's confusion.  I'm fine with keeping some changes that are needed for cleanup purposes or fixing build issues. However, I think we can still avoid some folder-renaming or folder-moving changes. For avoiding them, could you check my questions/comments below?

1. Why do we need to have ConfigurationManagerPkg.dec? Can we remove this? After removing it, we can rename the ConfigurationManagerPkg folder back to ConfigurationManager to be consistent with other platforms (JunoPkg).

[[Vikas]] ConfigurationManagerPkg folder should not be renamed back to ConfigurationManager because of the hierarchy and placement of this folder as a common generic for all the platform pkgs.

This is already been discussed with leif and moreover in case of JunoPkg the CM is private to JunoPkg not visible to other ARM platform pkgs. Here in our case CM serves all the NXP platforms.


2. Can we move \Platform\NXP\LX2160aRdbPkg\Include\Platform.h to the same location as LSDK (\Platform\NXP\LX2160aRdbPkg\AcpiTables\)?

[[Vikas]] This involves extra effort/rework.


3. Can we move \Platform\NXP\LS1046aFrwyPkg\Include\Platform.h t h to the same location as LSDK (\Platform\NXP\ LS1046aFrwyPkg\AcpiTables\)?

[[Vikas]] This involves extra effort/rework.


4. Can we add \Silicon\NXP\LS1046A\Library\SocFixupLib\ for patch 2/4 (adding SocGetSvr() function)? Furthermore, can we just add the whole Silicon\NXP\LS1046A\Library\SocFixupLib\ from NXP LSDK?
[[Vikas]] This involves extra effort/rework.


Add few more NXP guys.



Hi Leif and Meenakshi,

Can we just push the latest LSDK to the Tiano edk2-platform in one patch set? Then, If there is anything that needs to be cleaned up like the Coding style issue, we can create an issue in Bugzilla for it. What do you guys think?


Best Regards,
Sunny Wang

-----Original Message-----
From: Vikas Singh <vikas.singh@...>
Sent: Friday, June 18, 2021 11:28 PM
To: devel@edk2.groups.io
Cc: Sami Mujawar <Sami.Mujawar@...>; leif@...; Meenakshi Aggarwal (meenakshi.aggarwal@...) <meenakshi.aggarwal@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>; V Sethi (v.sethi@...) <v.sethi@...>; arokia.samy <arokia.samy@...>; kuldip.dwivedi@...; Ard Biesheuvel <Ard.Biesheuvel@...>; vikas.singh@...; Sunny Wang <Sunny.Wang@...>
Subject: [PATCH V2 0/4] Enable Dynamic ACPI for LS1046AFRWY

This patch series basically aims to extend the Dynamic ACPI
framework towards NXP's LS1046AFRWY platform.

In continuation to https://edk2.groups.io/g/devel/message/71709

The change set in the series is in below order -

(1)Introducing a new platform specific macro "PLAT_SOC_NAME"
This macro will be consumed by Configuration Manager(CM).
Platforms who extends CM services for themselves must notify
their SoC details to CM using this macro only.
Additionally also update the lx2160ardb platform header with
PLAT_SOC_NAME, this will be consumed by CM.

(2)Introduced a function to get SoC's System Version Register(SVR)
This function will fetch SVR details for LS1046A SoC based platforms.
In current patch series, this function will be used by LS1046aFrwyPkg.

(3)Extending Configuration Manager (CM) and its services to leverage
the Dynamic ACPI support for NXP's LS1046aFrwy platform.

(4)Introduced an OEM specific firmware acpi table generator
Also add Dsdt.asl as a place holder having only platform's clock
related dsdt properties for now and will accommodate other IP specific
dsdt tables(acpi properties) for LS1046AFRWY in future patch series.

Vikas Singh (4):
  Platform/NXP: Make SoC version log in ConfigurationManager generic
  Silicon/NXP: Add support of SVR handling for LS1046A SoC
  NXP/LS1046aFrwyPkg: Enable ConfigurationManager on LS1046AFRWY
  Platform/NXP/LS1046aFrwyPkg: Add OEM specific DSDT generator

 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c  |  11 +-
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl                           |  60 ++++++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl                          |  15 ++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf                |  39 +++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c | 138 +++++++++++++++++
 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h                      |  23 +++
 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h                                       | 156 ++++++++++++++++++++
 Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc                                       |  29 ++++
 Platform/NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf                                       |  13 ++
 Platform/NXP/LX2160aRdbPkg/Include/Platform.h                                        |   8 +-
 Silicon/NXP/LS1046A/LS1046A.dsc.inc                                                  |  11 ++
 Silicon/NXP/LS1046A/Library/SocLib/SocLib.c                                          |  16 ++
 Silicon/NXP/LX2160A/LX2160A.dsc.inc                                                  |   3 +-
 13 files changed, 508 insertions(+), 14 deletions(-)
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h
 create mode 100644 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h

--
2.25.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.