Re: [PATCH v3 00/10] Added support for FT2000/4 chip

["Leif Lindholm" <leif@nuviainc.com>]

[-- Attachment #1: Type: text/plain, Size: 4928 bytes --]

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 <leif@nuviainc.com> 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 <leif@nuviainc.com>
> 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 <leif@nuviainc.com>
for patch 9/10.

Best Regards,

Leif

[-- Attachment #2: Type: text/html, Size: 6475 bytes --]