Hi Gua,

I have some general feedback on this PR. I think once this feedback is integrated you can break the commit up into a patch series for edk2 email review.

1. Adding submodule for https://github.com/MIPI-Alliance/public-mipi-sys-t.git
1. .gitmodules change looks correct
2. .pytool change looks correct
3. Please add this external dependency to this section of edk2/ReadMe.rst

i. https://github.com/tianocore/edk2#license-details

ii. Please include edk2 stewards in review of ReadMe.rst to accept the new submodule and its license which is BSD 3 clause.

2. I am confused in the scope of the TraceHubDebugLibSysT library instance. By name it is not tied to Universal Payload, but the implementation of this lib references to Universal Payload structures.
1. Could TraceHubDebugLibSysT be used in non Universal Payload platforms?
2. Can Universal Payload concepts be removed from TraceHubDebugLibSysT and Universal Payload elements handled by a component in UefiPayloadPkg?
3. If they can not be separate, shouldn’t all this content be moved to the UefiPayloadPkg?
3. MdeModulePkg\Include\UniversalPayload\TraceHubDebugInfo.h
1. Should not include <Uefi.h> in these support includes. The module that includes this file will include top level include file like <Uefi.h> first.
2. Why is pragma pack(1) used? Most UEFI/PI spec includes do not use pragma pack(1).
4. MdeModulePkg\Include\Library\TraceHubDebugLib.h
1. Should not include <Uefi/UefiBaseTypes.h>. Module that uses this lib will include Uefi.h or PiDxe.h or PiPei.h that will get this include.
2. Why is #include <Guid/StatusCodeDataTypeDebug.h> included in this lib class. I do not see any definitions from that .h file used in the lib class iself. Recommend remove.
3. Why do TraceHubWriteCataLog64() and TraceHubWriteCataLog64StatusCode() have a return type of VOID. Can’t these fail due to bad input params or HW issues? Should they have a return type of EFI_STATUS or BOOLEAN?
4. TraceHubDebugWrite(). Should it define EFI_INVALID_PARAMETER if Buffer is NULL or NumberOfBytes is 0? I see ASSERT() for NumberOfBytes 0, and I see return EFI_SUCCESS for NumberOfBytes 0. Which is the correct behavior?
5. Split mipi and mipi support files out from TraceHubDebugLibSysT library instance
1. The mipi sub module provides the mipi-sys-t APIs that may be used by other lib instances in the future. By breaking it out into its own lib class/instance, it allows these mipi services to be used by more than once consumer. This is the same pattern that has been followed for other submodules such as OpenSSL, Cmocka, GoogleTest, etc.
2. Add MipiSysTLib class to MdeModulePkg.dec
3. MipiSysTLib class .h file should provide all the mipi related include files to support use of any of the mipi-sys-t APIs.
4. TraceHubDebugLibSysT will list the MipiSysTLib in its [LibraryClasses] section to use the mipi-sys-t APIs.
6. Please explain difference in features between BaseTraceHubLib and DxeSmmTraceHubLib and PeiTraceHubLib
1. If BaseTraceHubLib can be used by any module type, why do we need DxeSmmTraceHubLib or PeiTraceHubLib?
2. These features differences should be summarized in the INF file headers.
7. Looks like it would be a good idea to have an internal worker function to convert GUID value from little endan to big endian that can be shared.
8. I see use of AllocateRuntimePool(). Is RtData really required and the data need to be shared with OS after ExitBootServices()?
9. PcdTraceHubDebugVerbosity
1. I see use of BIT0, BIT1, BIT2 when looking at the PCD settings. I recommend you add defines/enums for these values to make it easier to understand the sources. Or perhaps add a structure with bitfields to access enable and debug level values.
2. This PCD is a mix of a bit mask for the enable and an enum for the debug level. Could this be split into 2 PCDs. One for enable and one for verbosity?
10. TraceHubDebugLibSystPlatform.c is using *(UINT32 *) to write MMIO register. These must be changed to use IoLib Mmio services
11. All internal worker functions of TraceHubDebugLibSysT should remove EFIAPI and be static.

Thanks,

Mike

From: Guo, Gua <gua.guo@intel.com>
Sent: Monday, January 2, 2023 6:32 PM
To: Hsu, VictorX <victorx.hsu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Chan, Laura <laura.chan@intel.com>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan@intel.com>; K N, Karthik <karthik.k.n@intel.com>
Cc: Lu, James <james.lu@intel.com>; Chen, Arthur G <arthur.g.chen@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Zimmer, Vincent <vincent.zimmer@intel.com>
Subject: RE: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]

@Chan, Laura, @Prakashan, Krishnadas Veliyathuparambil and @K N, Karthik

In parallel, please also help to review PR: https://github.com/tianocore/edk2/pull/3793 from TraceHubDebugLib owner viewpoint.

TraceHubDebugLib.h API Change

Origin

firmware.boot.uefi.iafw.intel/TraceHubDebugLib.h at main · intel-restricted/firmware.boot.uefi.iafw.intel · GitHub

• TraceHubWriteCataLog64_0 (SeverityType, Id, )
• TraceHubWriteCataLog64_1 (SeverityType, Id, Param1, Param2)
• TraceHubWriteCataLog64_2 (SeverityType, Id, Param1, Param2)
• TraceHubWriteCataLog64_3 (SeverityType, Id, Param1, Param2, Param3)
• TraceHubWriteCataLog64_4 (SeverityType, Id, Param1, Param2, Param3, Param4)
• TraceHubWriteCataLog64_5 (SeverityType, Id, Param1, Param2, Param3, Param4, Param5)
• TraceHubWriteCataLog64_6 (SeverityType, Id, Param1, Param2, Param3, Param4, Param5, Param6)

Modify

edk2_EXP/TraceHubDebugLib.h at th_leverageMIPI_v2 · hsuc1x/edk2_EXP · GitHub

• TraceHubWriteCataLog64 (SeverityType, Id, 0)
• TraceHubWriteCataLog64 (SeverityType, Id, 1, Param1)
• TraceHubWriteCataLog64 (SeverityType, Id, 2, Param1, Param2)
• TraceHubWriteCataLog64 (SeverityType, Id, 3, Param1, Param2, Param3)
• TraceHubWriteCataLog64 (SeverityType, Id, 4, Param1, Param2, Param3, Param4)
• TraceHubWriteCataLog64 (SeverityType, Id, 5, Param1, Param2, Param3, Param4, Param5)
• TraceHubWriteCataLog64 (SeverityType, Id, 6, Param1, Param2, Param3, Param4, Param5, Param6)

Thanks,

Gua

From: Hsu, VictorX <victorx.hsu@intel.com>
Sent: Wednesday, December 28, 2022 5:51 PM
To: Guo, Gua <gua.guo@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
Cc: Chan, Laura <laura.chan@intel.com>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan@intel.com>; K N, Karthik <karthik.k.n@intel.com>; Lu, James <james.lu@intel.com>; Chen, Arthur G <arthur.g.chen@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Zimmer, Vincent <vincent.zimmer@intel.com>
Subject: RE: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]

Hi @Kinney, Michael D

Please review https://github.com/tianocore/edk2/pull/3793 for below questions.

1. Need to add TraceHub PCD into edk2/MdeModulePkg.uni at master · tianocore/edk2 · GitHub

Answer: Done

2. What about the license ?

Answer: It’s BSD license, based on public-mipi-sys-t/LICENSE at master · MIPI-Alliance/public-mipi-sys-t (github.com)

3. Is PcdTraceHubDebugAddress MMIO address ? If yes, why we use UINT32

Answer: Done

4. For TraceHubWriteCataLog64_[0-6] try to use below macro to implement it.

#define VA_START va_start

#define VA_ARG   va_arg

#define VA_END   va_end

#define VA_LIST  va_list

Answer: Done

5. Is have SMM or MM version ?

Answer: Done, renamed to DxeSmmTraceHubDebugLibSyst.inf.

6. Whether have any chance to use .gitsubmodule to leverage https://github.com/MIPI-Alliance/public-mipi-sys-t.

Answer:. https://github.com/tianocore/edk2/pull/3793

thanks

From: Guo, Gua <gua.guo@intel.com>
Sent: Thursday, December 1, 2022 11:29 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
Cc: Chan, Laura <laura.chan@intel.com>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan@intel.com>; K N, Karthik <karthik.k.n@intel.com>; Lu, James <james.lu@intel.com>; Chen, Arthur G <arthur.g.chen@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Zimmer, Vincent <vincent.zimmer@intel.com>; Hsu, VictorX <victorx.hsu@intel.com>
Subject: RE: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]

@Kinney, Michael D

As we offline sync, there are below opens like below. Have any open, please also share for us.

1. Need to add TraceHub PCD into edk2/MdeModulePkg.uni at master · tianocore/edk2 · GitHub

Answer: Will update it in next patch

2. What about the license ?

Answer: It’s BSD license, based on public-mipi-sys-t/LICENSE at master · MIPI-Alliance/public-mipi-sys-t (github.com)

3. Is PcdTraceHubDebugAddress MMIO address ? If yes, why we use UINT32

Answer: It’s MMIO address, we can change it to UINT64, it make sense to let the PCH more generic. Will update it in next patch.

4. For TraceHubWriteCataLog64_[0-6] try to use below macro to implement it.

#define VA_START va_start

#define VA_ARG   va_arg

#define VA_END   va_end

#define VA_LIST  va_list

Answer: Will update it in next patch

5. Is have SMM or MM version ?

Answer: MdeModulePkg/Library/TraceHubDebugLibSysT/DxeTraceHubDebugLibSyst.inf can take care for SMM case. We can rename it like below to make it more clearly.

MdeModulePkg/Library/TraceHubDebugLibSysT/DxeSmmTraceHubDebugLibSyst.inf

6. Whether have any chance to use .gitsubmodule to leverage https://github.com/MIPI-Alliance/public-mipi-sys-t.

Answer: Will draft another PR and try to use gitsubmodule to check whether exist any predicament.

Thanks

Gua

From: Guo, Gua
Sent: Wednesday, November 23, 2022 3:29 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
Cc: Chan, Laura <laura.chan@intel.com>; Prakashan, Krishnadas Veliyathuparambil <krishnadas.veliyathuparambil.prakashan@intel.com>; K N, Karthik <karthik.k.n@intel.com>; Lu, James <james.lu@intel.com>; Chen, Arthur G <arthur.g.chen@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Zimmer, Vincent <vincent.zimmer@intel.com>; Hsu, VictorX <victorx.hsu@intel.com>
Subject: Edk2 support MIPI SyS-T TraceHub Debug Library [TraceHubDebugLibSysT]

@Kinney, Michael D and @gaoliming

We want to add TraceHubDebug library support on Edk2. It’s new added library on Edk2, so it doesn’t have any backward and forward issue.

May I know have any process we may need to follow up. Have any comment or concern please also share for me.

Reviewers/Maintainers: @Prakashan, Krishnadas Veliyathuparambil, @Chan, Laura, @K N, Karthik.

USF UPL spec update: https://github.com/UniversalScalableFirmware/documentation/pull/52 (Done)

Edk2 Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4144 (On-Going)

Edk2 PR: https://github.com/tianocore/edk2/pull/3613 (On-Going)

Background:

• Problem: In the UPL (Universal Payload) binary which contains only the Edk2 drivers, Tracehub debug message are missing during the execution of early DXE Core modules (before ReportStatusCode is ready).
• Solution: Add TraceHub lib on Edk2, to be consumed by the UPL.

About TraceHubDebugLibSysT:

• Library is consumed by the DebugLib and StatusCodeHandler to send the PostCodes, ProgressCodes and DEBUG messages to the TraceHub/NPK in MIPI SyS-T format.
• The MIPI SyS-T APIs in this library are taken from the OpenSource implementation: https://github.com/MIPI-Alliance/public-mipi-sys-t.
• Library changes done for upstreaming:  MipiSystLib is rewritten to comply EDK2 coding standards and changed ways to pass trace address/verbosity with HOB directly instead of calling into TraceHubHookLib to get these info.
• Proposal With UPL: consume HOB gUniversalPayloadTraceHubDebugInfoGuid to print TraceHub Debug message in DXE Core

Thanks,

Gua