Hi Ling, Many apologies for delay. I have been juggling too many things and making little progress on any of them. I owe you some of the good 白酒 when we meet in person.
I have now looked over all of v3, and have some feedback, but most of it is minor. First of all, some process comments: I responded for v2 that you should add "Reviewed-by: Leif Lindholm <l...@nuviainc.com>" to those patches where I had said so. But you appear to have added it to *all* patches in v3. Also, some of the feedback/comments on v2 has not been acted on, and I had no reply explaining why not. As I commented on v3 - there should be no revision history in the commit messages. Please drop these from v4. The following patches were given Reviewed-by for v2: 1/10 3/10 10/10 2, 4, 5, 6, 7, 8, 9 should not have been sent out with my Revied-by added. However, after looking at v3, you can add Reviewed-by: Leif Lindholm <l...@nuviainc.com> to 2, 5, 6, 8. While I did give a Reviewed-by for 1/10 in v2, I spotted a few minor issues when looking at v3 1/10 (commented 13 April): - A typo - CORE spelled as COURE. - Some of the structures defined in SystemServiceInterface.h have too common names and should be given PHYTIUM_ prefix: MCU_DIMM, MCU_DIMMS, MEMORY_BLOCK, MEMORY_INFO, PCI_BLOCK, PCI_HOST_BLOCK In addition to this, edk2 changes means the following diff needs to be folded in: <<< diff --git a/Platform/Phytium/DurianPkg/DurianPkg.dsc b/Platform/Phytium/DurianPkg/DurianPkg.dsc index 9579f8e9b7d0..19009106a2bf 100644 --- a/Platform/Phytium/DurianPkg/DurianPkg.dsc +++ b/Platform/Phytium/DurianPkg/DurianPkg.dsc @@ -23,6 +23,7 @@ [Defines] SKUID_IDENTIFIER = DEFAULT FLASH_DEFINITION = Platform/Phytium/DurianPkg/DurianPkg.fdf +!include MdePkg/MdeLibs.dsc.inc !include Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc [LibraryClasses.common] >>> If you do all of these things mentioned, you can keep Reviewed-by on 1/10 when sending out v4. I gave comments on patch 4 in v2 that were not acted on for v3. These were regarding suspicious use of the volatile keyword. I gave comments on patch 7 in v2 that were not acted on for v3. - Include files should only themselves include files needed for its internal definitions. .c files should include all .h files they depend on themselves. - Typos of "norfalsh" (for "norflash") and "eares" (for "erase") were not corrected. - I also asked some questions that were not answered. - Other feedback I gave was addressed in v3. I gave comments on patch 7 in v2 that were not acted on for v3. - Use IsValidTimeZone from EmbeddedPkg TimeBaseLib instead of implementing the test yourself. - Other feedback I gave was addressed in v3. If you can address the above comments and send out a v4, I promise I will be *much* more responsive from this point onwards. Best Regards, Leif On Wed, Mar 17, 2021 at 15:26:37 +0800, Ling Jia wrote: > This series added packages to support FT2000/4 chip. > Platform/Phytium: Added DurianPkg, include DurianPkg.dsc and DurianPkg.fdf. > Silicon/Phytium: Added FT2000-4Pkg and PhytiumCommonPkg. > > The modules could be runed at the silicon of FT2000/4. > They supported Acpi parameter configuration, Pci bus scaning, > flash read-write and erase abd operating system boot function. > Maintainers.txt: Added maintainers and reviewers for the DurianPkg. > > The public git repository is : > https://github.com/jialing2020/edk2-platforms/tree/Phytium_Opensource_For_FT2000-4_v3 > > v3: > Optimized the codes to meet the edk2 coding specification. > > Ling Jia (10): > Silicon/Phytium: Added PlatformLib to FT2000/4 > Silicon/Phytium: Added Acpi support to FT2000/4 > Silicon/Phytium: Added SMBIOS support to FT2000/4 > Silicon/Phytium: Added PciSegmentLib to FT2000/4 > Silicon/Phytium: Added PciHostBridgeLib to FT2000/4 > Silicon/Phytium: Added Spi driver support to FT2000/4 > Silicon/Phytium: Added flash driver support to Phytium Silicon > Silicon/Phytium: Added fvb driver for norflash > Silicon/Phytium: Added Rtc driver to FT2000/4 > Maintainers.txt: Added maintainers and reviewers for the DurianPkg > > Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dec > | 52 + > Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc > | 345 +++++ > Platform/Phytium/DurianPkg/DurianPkg.dsc > | 331 +++++ > Platform/Phytium/DurianPkg/DurianPkg.fdf > | 235 ++++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/AcpiTables.inf > | 56 + > Silicon/Phytium/FT2000-4Pkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf > | 47 + > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiDxe/SpiDxe.inf > | 44 + > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiNorFlashDxe/SpiNorFlashDxe.inf > | 48 + > Silicon/Phytium/FT2000-4Pkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > | 47 + > Silicon/Phytium/FT2000-4Pkg/Library/PciSegmentLib/PciSegmentLib.inf > | 28 + > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/PlatformLib.inf > | 55 + > Silicon/Phytium/FT2000-4Pkg/Library/RealTimeClockLib/RealTimeClockLib.inf > | 39 + > Silicon/Phytium/PhytiumCommonPkg/Drivers/AcpiPlatformDxe/AcpiPlatformDxe.inf > | 53 + > Silicon/Phytium/PhytiumCommonPkg/Drivers/FlashFvbDxe/FlashFvbDxe.inf > | 61 + > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiDxe/SpiDxe.h > | 64 + > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiNorFlashDxe/SpiNorFlashDxe.h > | 99 ++ > Silicon/Phytium/FT2000-4Pkg/Library/RealTimeClockLib/RealTimeClockLib.h > | 24 + > Silicon/Phytium/PhytiumCommonPkg/Drivers/FlashFvbDxe/FlashFvbDxe.h > | 104 ++ > Silicon/Phytium/PhytiumCommonPkg/Include/Platform.h > | 80 ++ > Silicon/Phytium/PhytiumCommonPkg/Include/Protocol/SpiNorFlashProtocol.h > | 74 + > Silicon/Phytium/PhytiumCommonPkg/Include/Protocol/SpiProtocol.h > | 51 + > Silicon/Phytium/PhytiumCommonPkg/Include/SystemServiceInterface.h > | 112 ++ > Silicon/Phytium/FT2000-4Pkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c > | 943 +++++++++++++ > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiDxe/SpiDxe.c > | 198 +++ > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiNorFlashDxe/SpiNorFlashDxe.c > | 424 ++++++ > Silicon/Phytium/FT2000-4Pkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > | 181 +++ > Silicon/Phytium/FT2000-4Pkg/Library/PciSegmentLib/PciSegmentLib.c > | 1434 ++++++++++++++++++++ > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/PlatformLib.c > | 137 ++ > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/PlatformLibMem.c > | 156 +++ > Silicon/Phytium/FT2000-4Pkg/Library/RealTimeClockLib/RealTimeClockLib.c > | 462 +++++++ > Silicon/Phytium/PhytiumCommonPkg/Drivers/AcpiPlatformDxe/AcpiPlatform.c > | 250 ++++ > Silicon/Phytium/PhytiumCommonPkg/Drivers/FlashFvbDxe/FlashFvbDxe.c > | 1304 ++++++++++++++++++ > Maintainers.txt > | 8 + > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/AcpiSsdtRootPci.asl > | 209 +++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dbg2.aslc > | 80 ++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dsdt/Cpu.asl > | 85 ++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dsdt/Dsdt.asl > | 15 + > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dsdt/Uart.asl > | 65 + > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Fadt.aslc > | 77 ++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Gtdt.aslc > | 83 ++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Iort.aslc > | 89 ++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Madt.aslc > | 67 + > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Mcfg.aslc > | 65 + > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Pptt.aslc > | 219 +++ > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Spcr.aslc > | 73 + > > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/AArch64/PhytiumPlatformHelper.S > | 76 ++ > Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.fdf.inc > | 119 ++ > 47 files changed, 8868 insertions(+) > create mode 100644 Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dec > create mode 100644 Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc > create mode 100644 Platform/Phytium/DurianPkg/DurianPkg.dsc > create mode 100644 Platform/Phytium/DurianPkg/DurianPkg.fdf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/AcpiTables.inf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.inf > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/SpiDxe/SpiDxe.inf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiNorFlashDxe/SpiNorFlashDxe.inf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PciSegmentLib/PciSegmentLib.inf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/PlatformLib.inf > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/RealTimeClockLib/RealTimeClockLib.inf > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Drivers/AcpiPlatformDxe/AcpiPlatformDxe.inf > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Drivers/FlashFvbDxe/FlashFvbDxe.inf > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/SpiDxe/SpiDxe.h > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiNorFlashDxe/SpiNorFlashDxe.h > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/RealTimeClockLib/RealTimeClockLib.h > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Drivers/FlashFvbDxe/FlashFvbDxe.h > create mode 100644 Silicon/Phytium/PhytiumCommonPkg/Include/Platform.h > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Include/Protocol/SpiNorFlashProtocol.h > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Include/Protocol/SpiProtocol.h > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Include/SystemServiceInterface.h > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/SmbiosPlatformDxe/SmbiosPlatformDxe.c > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/SpiDxe/SpiDxe.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/SpiNorFlashDxe/SpiNorFlashDxe.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PciSegmentLib/PciSegmentLib.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/PlatformLib.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/PlatformLibMem.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/RealTimeClockLib/RealTimeClockLib.c > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Drivers/AcpiPlatformDxe/AcpiPlatform.c > create mode 100644 > Silicon/Phytium/PhytiumCommonPkg/Drivers/FlashFvbDxe/FlashFvbDxe.c > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/AcpiSsdtRootPci.asl > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dbg2.aslc > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dsdt/Cpu.asl > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dsdt/Dsdt.asl > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Dsdt/Uart.asl > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Fadt.aslc > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Gtdt.aslc > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Iort.aslc > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Madt.aslc > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Mcfg.aslc > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Pptt.aslc > create mode 100644 Silicon/Phytium/FT2000-4Pkg/Drivers/AcpiTables/Spcr.aslc > create mode 100644 > Silicon/Phytium/FT2000-4Pkg/Library/PlatformLib/AArch64/PhytiumPlatformHelper.S > create mode 100644 Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.fdf.inc > > -- > 2.25.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75809): https://edk2.groups.io/g/devel/message/75809 Mute This Topic: https://groups.io/mt/81410910/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
