From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by mx.groups.io with SMTP id smtpd.web08.6191.1622119392871847689 for ; Thu, 27 May 2021 05:43:13 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=cwFMc0kd; spf=pass (domain: nuviainc.com, ip: 209.85.221.42, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f42.google.com with SMTP id p7so4591052wru.10 for ; Thu, 27 May 2021 05:43:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=Zh90QG/zFhBq1W4e6/AE0/hfxbzy7zZtJkBRyz3tv1I=; b=cwFMc0kdp4rrlhXSeGkeyCui5mFQI9f3BUEs//8LX/g5DOe4FZiwlSQRAMn1+jx0XL LA2eYTeBN/7bBs+UZiTTwNFd8ltYf44A6vXF/guai5NrPiuA5zWciKLPUX5qfYPtYxxA xsjVEYM4m1vSsfWbscvevnVYIlGMW2LEkPFQrZZR/335g5t8Yr7MvbS4aYvDHxKJDN2b +FAU01mW6D+XxRSkFwmec7zMNfhvB5TotqZBsPOsvWcZh53PIcaVOV7I73dxRXqp3Huj BQ9seOXiePll6sntieYaGbljv0z/ZQke4t957RDNmI/Se6W9wuNCVXI8T5cVpaw9cgkr rQ6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=Zh90QG/zFhBq1W4e6/AE0/hfxbzy7zZtJkBRyz3tv1I=; b=bhnR5im9uiHLwQ35LVM8uCGLEN1dChY2ZnVI9FtKVvZzBqaiPppgu3PsfeKExCXTGi bhkz3k+L9WyZACexhyRpsDbTvldq/m/Ba7ertPv8lWMm6WzNdc6ZPbmUwvJaG6SHzXzH rwQx6U3eEvO77Ws/dyJrErvNiZrVkpTeSiKZMnox6laslgEP8pwieQH/2/7szAYV+ntn QPNQgBM7aO7eCIuOQxg4vsRuUNwRkzA6ylNj0giCTxEPDJ8zN0myMVw6hZpBcyvkCZdB 49aJkVNpze/vGDessvu8WpUKlGslShrAsL1tt0t5pg8lHosLpIm9bEuz3OaEoOAPX41a /oBQ== X-Gm-Message-State: AOAM530iS1uOMkRW2Y51UEUHFlxBRyzIo2X46PxTXy4HbFn5D0jBzAIF YCbYH81DRGlsywA1wN93M9c13X02Pl9GtkeA X-Google-Smtp-Source: ABdhPJx5RXX/96+vQ86+hP8LXvLfiMa1a7t2EZkjk79ppxOR3mX47kY/WHHayNb7CWik/cbakcYJgQ== X-Received: by 2002:a05:6000:1081:: with SMTP id y1mr3095649wrw.34.1622119391072; Thu, 27 May 2021 05:43:11 -0700 (PDT) Return-Path: Received: from leviathan (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id u17sm2965826wrt.61.2021.05.27.05.43.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 May 2021 05:43:09 -0700 (PDT) Date: Thu, 27 May 2021 13:43:08 +0100 From: "Leif Lindholm" To: Ling Jia Cc: devel@edk2.groups.io Subject: Re: [PATCH v3 00/10] Added support for FT2000/4 chip Message-ID: <20210527124308.nrt6u6eqpfhthor7@leviathan> References: <20210317072647.77340-1-jialing@phytium.com.cn> MIME-Version: 1.0 In-Reply-To: <20210317072647.77340-1-jialing@phytium.com.cn> Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 " 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 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 >