From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f172.google.com (mail-yb1-f172.google.com [209.85.219.172]) by mx.groups.io with SMTP id smtpd.web11.34955.1631015966335441531 for ; Tue, 07 Sep 2021 04:59:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=fle3VviG; spf=pass (domain: nuviainc.com, ip: 209.85.219.172, mailfrom: leif@nuviainc.com) Received: by mail-yb1-f172.google.com with SMTP id c6so19246863ybm.10 for ; Tue, 07 Sep 2021 04:59:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7Xp7fKr67x7spEzaqXZPW4qCp2Q+wbXa8n3kxLo6tdw=; b=fle3VviGWo37+Lm28h1aqW4aLrSLg8MOhe1jtHZMqqCj6gdKQoXBlgMIit1mGb5XMf Veo8k0KCUPaUx3hgP31BAlqQvaW2A2TlFZqa9sTdHDT2RpnKYW7WRGjONNyBGTxmJ2JZ 2EfZ0l49W/BADi5fqk9ohu+Oe1IwXSwz2Ravk1TmryHGNqevP2VrBK2XT7xAkB/hDgsy ODsQ2ZX9tUU+Eb0sDsIhLTxukJUo2jwoJHZCicSZ6S1ybD6LjsWpKThaVlxiRUBTPwJ6 yjBPCsuW4K6PbBZ4IJ4KcMZC1v4Y11kuafVfUqp9yzH92+K677HIJDHcPeAZQ0SHzZjQ ivig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7Xp7fKr67x7spEzaqXZPW4qCp2Q+wbXa8n3kxLo6tdw=; b=i4JuMcVmzS53vJySa+fuhJf7mbYzIFUn6K5gGmRm9Uu2F//BNo35YWDtLYiCMsK8MX M/I3bd2/he/GHGkMa1HeeY+WGvt7IsUqWSy7o1uN55d944ECkvgxzF3PAAU0x5wrxYw2 QnARysat/YbWEAXJULFkSNMrgvFsqHsD40Rf4OVxOnNwUd0+puqNDIMGKwMZA1j6MrBW 8D8kFO+byWkhqKkxzq+15zdY5stwMf+dMakEWFWCojt+jPwIDfbpLQ43dijnInF4AYOS Rtnqv156YFCeWIw8tNxjl4MLxbHT2S9tz8J+MGt7lPIqUS9Dm/lHZPVOVk8TUibbq4Tq toRQ== X-Gm-Message-State: AOAM533aTBhL6eIahlq04oRr7aMSuacdDtCp+kVygE5N52PaSkbmpibE qrHfMB5pWLv0MxFuoHB78U02/QrHXdGE5ZWcynYecMFstHGQ00zn X-Google-Smtp-Source: ABdhPJz+8PIIG/a/Xppa7IMx2HP0o3xTo7aaha5Fu376Lweh0IUhJKfUf+le7PyzTrZVxlqeEpwilJobssRlOT0jg6w= X-Received: by 2002:a25:27c1:: with SMTP id n184mr22711679ybn.496.1631015965371; Tue, 07 Sep 2021 04:59:25 -0700 (PDT) MIME-Version: 1.0 References: <20210317072647.77340-1-jialing@phytium.com.cn> <20210527124308.nrt6u6eqpfhthor7@leviathan> In-Reply-To: <20210527124308.nrt6u6eqpfhthor7@leviathan> From: "Leif Lindholm" Date: Tue, 7 Sep 2021 12:59:14 +0100 Message-ID: Subject: Re: [PATCH v3 00/10] Added support for FT2000/4 chip To: Ling Jia Cc: edk2-devel-groups-io Content-Type: multipart/alternative; boundary="0000000000003d429805cb667f23" --0000000000003d429805cb667f23 Content-Type: text/plain; charset="UTF-8" Hi Ling, Apologies for the delay. To simplify things, I will respond to v4 in this thread, since there are still a few things remaining from what I asked for v3. Most of what I asked for has been addressed, but: On Thu, May 27, 2021 at 1:43 PM Leif Lindholm wrote: > First of all, some process comments: > I responded for v2 that you should add "Reviewed-by: Leif Lindholm < leif@nuviainc.com>" > to those patches where I had said so. But you appear to have added it to *all* > patches in v3. Done, thanks! > Also, some of the feedback/comments on v2 has not been acted on, and I had no > reply explaining why not. Some things remain. > As I commented on v3 - there should be no revision history in the commit messages. > Please drop these from v4. Done, thanks! > 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. Done, thanks! > 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 Doone, thanks! > 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] > >>> This has not happened, so the platform does not build against current edk2 master. Additionally, since then, further changes now mean the following change also requires folding in for a successful build: diff --git a/Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc b/Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc index 121fe0e7c549..0e488c56a819 100644 --- a/Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc +++ b/Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc @@ -116,6 +116,10 @@ [LibraryClasses.common] UdpIoLib|NetworkPkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf HttpLib|NetworkPkg/Library/DxeHttpLib/DxeHttpLib.inf + SecureBootVariableLib|SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf + SecureBootVariableProvisionLib|SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.inf + [LibraryClasses.common.SEC] ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf > 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. Not (fully) addressed. Let me reword it. The use of volatile in *all* locations in patch 4 are incorrect. It does not have the effect suggested by the adjacent comments. They should be dropped completely. If the presence of the volatile keyword in this location has any effect on program behaviour, and *if* that change in behaviour is required for successful execution, the most likely root cause is missing memory barriers. > 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. Question still not answered, from https://edk2.groups.io/g/devel/message/71582 : --- I am slightly unsure of this mechanism. These commands are only set once - is the intent to implement a single driver for several versions of chip? --- All other feedback has been acted on. But please respond to this question. > - 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. Gah, the above referred to patch 9, not patch 7, This has now been addressed, so please add Reviewed-by: Leif Lindholm for patch 9/10. Best Regards, Leif --0000000000003d429805cb667f23 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ling,

Apologies for t= he delay.

To simplify things, I will respond to v4 in thi= s thread, since there are still a few things remaining from what I asked fo= r v3.
Most of what I asked for has been addressed, but:

On Thu, M= ay 27, 2021 at 1:43 PM Leif Lindholm <leif@nuviainc.com> wrote:
> First of all, some process comm= ents:
> I responded for v2 that you should add "Reviewed-by: Lei= f Lindholm <leif@nuviainc.com&g= t;"
> to those patches where I had said so. But you appear to ha= ve added it to *all*
> patches in v3.

Done, th= anks!

> Also, some of the feedback/comments= on v2 has not been acted on, and I had no
> reply explaining why n= ot.

Some things remain.

> As I commented on v3 - there should be no revision history in the com= mit messages.
> Please drop these from v4.

D= one, thanks!

> 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: Lei= f Lindholm <leif@nuviainc.com&g= t;
> to 2, 5, 6, 8.

Done, thanks!

> While I did give a Reviewed-by for 1/10 in v2, I spot= ted a few minor issues
> when looking at v3 1/10 (commented 13 Apri= l):
> - A typo - CORE spelled as COURE.
> - Some of the structu= res defined in SystemServiceInterface.h have too common names
> =C2= =A0 and should be given PHYTIUM_ prefix:
> =C2=A0 MCU_DIMM, MCU_DIMMS= , MEMORY_BLOCK, MEMORY_INFO, PCI_BLOCK, PCI_HOST_BLOCK

Doone, thanks!

> In addition to this, ed= k2 changes means the following diff needs to be folded in:
>
>= ; <<<
> diff --git a/Platform/Phytium/DurianPkg/DurianPkg.ds= c b/Platform/Phytium/DurianPkg/DurianPkg.dsc
> index 9579f8e9b7d0..19= 009106a2bf 100644
> --- a/Platform/Phytium/DurianPkg/DurianPkg.dsc> +++ b/Platform/Phytium/DurianPkg/DurianPkg.dsc
> @@ -23,6 +23,7= @@ [Defines]
> =C2=A0 =C2=A0SKUID_IDENTIFIER =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D DEFAULT
> =C2=A0 =C2=A0FLASH_DEFINITI= ON =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D Platform/Phytium/Du= rianPkg/DurianPkg.fdf
>
> +!include MdePkg/MdeLibs.dsc.inc
&= gt; =C2=A0!include Silicon/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.in= c
>
> =C2=A0[LibraryClasses.common]
> >>>

This has not happened, so the platform does not build agai= nst current edk2 master.
Additionally, since then, further change= s now mean=C2=A0 the following change also requires folding in for a succes= sful build:

diff --git a/Silicon/Phytium/PhytiumCo= mmonPkg/PhytiumCommonPkg.dsc.inc b/Silicon/Phytium/PhytiumCommonPkg/Phytium= CommonPkg.dsc.inc
index 121fe0e7c549..0e488c56a819 100644
--- a/Silic= on/Phytium/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc
+++ b/Silicon/Phyti= um/PhytiumCommonPkg/PhytiumCommonPkg.dsc.inc
@@ -116,6 +116,10 @@ [Libra= ryClasses.common]
=C2=A0 =C2=A0UdpIoLib|NetworkPkg/Library/DxeUdpIoLib/D= xeUdpIoLib.inf
=C2=A0 =C2=A0HttpLib|NetworkPkg/Library/DxeHttpLib/DxeHtt= pLib.inf
=C2=A0
+ =C2=A0SecureBootVariableLib|SecurityPkg/Library/Sec= ureBootVariableLib/SecureBootVariableLib.inf
+ =C2=A0SecureBootVariableP= rovisionLib|SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVa= riableProvisionLib.inf
+
=C2=A0[LibraryClasses.common.SEC]
=C2=A0 = =C2=A0ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf=C2=A0 =C2=A0DebugAgentLib|ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAg= entSymbolsBaseLib.inf

> If you do all of these thing= s mentioned, you can keep Reviewed-by on 1/10 when sending out v4.
><= br>> I gave comments on patch 4 in v2 that were not acted on for v3.
=
> These were regarding suspicious use of the volatile keyword.

Not (fully) addressed.
Let me reword it.
The use of volatile in *all* locations in patch 4 are incorrect.
It does not have the effect suggested by the adjacent comments.
They should be dropped completely.

If th= e presence of the volatile keyword in this location has any effect on progr= am behaviour,
and *if* that change in behaviour is required for s= uccessful execution, the most likely root
cause is missing memory= barriers.

> I gave comments on patch 7 in v2 th= at were not acted on for v3.
> - Include files should only themselves= include files needed for its internal definitions.
> =C2=A0 .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 que= stions that were not answered.

Question still not = answered, from htt= ps://edk2.groups.io/g/devel/message/71582 :
---
=
I am slightly unsure of this mechanism.
These commands are only set once - is the intent to implement a single
driver for several versions of chip?
---

All other feedback has been acted on.
But please respond = to this question.

> - 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.

<= div>Gah, the above referred to patch 9, not patch 7,
This has now= been addressed, so please add
Reviewed-by: Leif Lindholm <= ;leif@nuviainc.com>
for patch 9/10.

Best Regards,

Leif

--0000000000003d429805cb667f23--