Re: [edk2] [PATCH 1/4] Platforms: Add Sanchip Zx296718 basic library

2017-08-17 Thread Leif Lindholm 
On Thu, Aug 17, 2017 at 11:42:23PM +0800, Jun Nie wrote:
> On 2017年08月10日 21:04, Leif Lindholm wrote:
> > > diff --git a/Silicon/Sanchip/Zx296718/Include/Zx296718.h 
> > > b/Silicon/Sanchip/Zx296718/Include/Zx296718.h
> > > new file mode 100644
> > > index 000..3ace9ab
> > > --- /dev/null
> > > +++ b/Silicon/Sanchip/Zx296718/Include/Zx296718.h
> > > @@ -0,0 +1,30 @@
> > > +/** @file
> > > +*
> > > +*  Copyright (c) 2016, Linaro Limited. All rights reserved.
> > 
> > Guess 2016-2017 by now?
> > 
> > > +*
> > > +*  This program and the accompanying materials
> > > +*  are licensed and made available under the terms and conditions of the 
> > > BSD License
> > > +*  which accompanies this distribution.  The full text of the license 
> > > may be found at
> > > +*  http://opensource.org/licenses/bsd-license.php
> > > +*
> > > +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> > > IMPLIED.
> > > +*
> > > +**/
> > > +
> > > +#ifndef __ZX296718_H__
> > > +#define __ZX296718_H__
> > 
> > Coding style specifies:
> > "Every header file must have a ‘#ifndef FILE_NAME_H_’ and ‘#endif’ guard"
> > so you could drop 3 of those underscores.
> 
> Will change it. Is there any coding style readme file? Because I find most
> of header file use __FILE_NAME_H__ sytle macro.

Ah, yes - it helps knowing it exists :)

It is the C Coding Standards document on
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications
You will potentially find other documents there useful as well.

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] Platforms: Add Sanchip Zx296718 basic library

2017-08-17 Thread Jun Nie 

On 2017年08月10日 21:04, Leif Lindholm wrote:

You've reworked this to be targeted for edk2-platforms, which is
excellent! However:
- Please update subject line to match.
- Please also cc edk2-devel@lists.01.org (since you are with Linaro,
   it makes sense to still cc linaro-uefi)

To get the appropriate subject line for edk2-devel, as desribed by
https://github.com/tianocore/edk2-platforms/blob/about/Readme.md it
would also be helpful to add
--subject-prefix="edk2-platforms/master][PATCH" to git format-patch
command line (unless Laszlo can point out a better way of achieving
the same).

On Wed, Aug 09, 2017 at 10:12:36PM +0800, Jun Nie wrote:

Add Sanchip Zx296718 basic library files for Zx296718 SoC

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie 
---
  .../Library/Zx296718EvbLib/Zx296718Evb.c   | 132 +
  .../Library/Zx296718EvbLib/Zx296718EvbHelper.S |  50 
  .../Library/Zx296718EvbLib/Zx296718EvbLib.inf  |  53 +
  .../Library/Zx296718EvbLib/Zx296718EvbMem.c| 107 +
  Silicon/Sanchip/Zx296718/Include/Zx296718.h|  30 +
  Silicon/Sanchip/Zx296718/Zx296718.dec  |  32 +


It is more clear (especially when submitting new ports) to generate
the patches with --stat=1000, to make the full file paths visible.

See Laszlo's guide for helpful steps when submitting patches:
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers


  6 files changed, 404 insertions(+)
  create mode 100644 
Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
  create mode 100644 
Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbHelper.S
  create mode 100644 
Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbLib.inf
  create mode 100644 
Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbMem.c
  create mode 100644 Silicon/Sanchip/Zx296718/Include/Zx296718.h
  create mode 100644 Silicon/Sanchip/Zx296718/Zx296718.dec


Since this ends up with a .dec, please rename
Silicon/Sanchip/Zx296718/ -> Silicon/Sanchip/Zx296718Pkg/.

That said, I don't see this .dec file providing any information that
is subsequently used enywhere. Is this in preparation for further
patches coming after this initial platform support?


No plan yet. So this file will be deleted.



diff --git a/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c 
b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
new file mode 100644
index 000..4e4eb54
--- /dev/null
+++ b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
@@ -0,0 +1,132 @@
+/** @file
+*
+*  Copyright (C) 2017 Sanechips Technology Co., Ltd.


Just a clarification: Sanchip/Sanechips?
"The Internet" suggests Sanechips is the appropriate company name, so
should the directory name change >

+*  Copyright (c) 2017, Linaro Ltd.
+*
+*  This program and the accompanying materials
+*  are licensed and made available under the terms and conditions of the BSD 
License
+*  which accompanies this distribution.  The full text of the license may be 
found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+*
+**/
+
+#include 
+#include 
+#include 
+#include 


Please sort include files alphabetically.


+
+#include 
+
+#include 
+
+ARM_CORE_INFO mZx296718EvbInfoTable[] = {
+  {
+// Cluster 0, Core 0
+0x0, 0x0,
+
+// MP Core MailBox Set/Get/Clear Addresses and Clear Value
+(UINT64)0x
+  },
+  {
+// Cluster 0, Core 1
+0x0, 0x1,
+
+// MP Core MailBox Set/Get/Clear Addresses and Clear Value
+(UINT64)0x
+  },
+  {
+// Cluster 0, Core 2
+0x0, 0x2,
+
+// MP Core MailBox Set/Get/Clear Addresses and Clear Value
+(UINT64)0x
+  },
+  {
+// Cluster 0, Core 3
+0x0, 0x3,
+
+// MP Core MailBox Set/Get/Clear Addresses and Clear Value
+(UINT64)0x
+  },
+};
+
+/**
+  Return the current Boot Mode
+
+  This function returns the boot reason on the platform
+
+  @return   Return the current Boot Mode of the platform
+
+**/
+EFI_BOOT_MODE
+ArmPlatformGetBootMode (
+  VOID
+  )
+{
+  return BOOT_WITH_FULL_CONFIGURATION;
+}
+
+/**
+  Initialize controllers that must setup in the normal world
+
+  This function is called by the ArmPlatformPkg/Pei or 
ArmPlatformPkg/Pei/PlatformPeim
+  in the PEI phase.
+
+**/
+RETURN_STATUS
+ArmPlatformInitialize (
+  IN  UINTN MpId
+  )
+{
+  return RETURN_SUCCESS;
+}
+
+/**
+  Initialize the system (or sometimes called permanent) memory
+
+  This memory is generally represented by the DRAM.
+
+**/
+VOID
+ArmPlatformInitializeSystemMemory (
+  VOID
+  )
+{
+}
+
+EFI_STATUS
+PrePeiCoreGetMpCoreInfo (
+  OUT UINTN   *CoreCount,
+  OUT ARM_C

Re: [edk2] [PATCH 1/4] Platforms: Add Sanchip Zx296718 basic library

2017-08-10 Thread Laszlo Ersek 
Thanks for the CC

On 08/10/17 15:04, Leif Lindholm wrote:
> You've reworked this to be targeted for edk2-platforms, which is
> excellent! However:
> - Please update subject line to match.
> - Please also cc edk2-devel@lists.01.org (since you are with Linaro,
>   it makes sense to still cc linaro-uefi)
> 
> To get the appropriate subject line for edk2-devel, as desribed by
> https://github.com/tianocore/edk2-platforms/blob/about/Readme.md it
> would also be helpful to add
> --subject-prefix="edk2-platforms/master][PATCH" to git format-patch
> command line (unless Laszlo can point out a better way of achieving
> the same).

I seem to recall a discussion (not necessarily related to the
edk2-platforms tree, but to another edk2-related tree, still posted to
edk2-devel) that we're fine if you put the tree identified right after
the PATCH word, between the same brackets. So just use

  --subject-prefix="PATCH edk2-platforms/master"

or even (if "master" is quite obvious)

  --subject-prefix="PATCH edk2-platforms"

> 
> On Wed, Aug 09, 2017 at 10:12:36PM +0800, Jun Nie wrote:
>> Add Sanchip Zx296718 basic library files for Zx296718 SoC
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jun Nie 
>> ---
>>  .../Library/Zx296718EvbLib/Zx296718Evb.c   | 132 
>> +
>>  .../Library/Zx296718EvbLib/Zx296718EvbHelper.S |  50 
>>  .../Library/Zx296718EvbLib/Zx296718EvbLib.inf  |  53 +
>>  .../Library/Zx296718EvbLib/Zx296718EvbMem.c| 107 +
>>  Silicon/Sanchip/Zx296718/Include/Zx296718.h|  30 +
>>  Silicon/Sanchip/Zx296718/Zx296718.dec  |  32 +
> 
> It is more clear (especially when submitting new ports) to generate
> the patches with --stat=1000, to make the full file paths visible.

Yes, please -- I recommend:

  --stat=1000 --stat-graph-width=20

("--stat=1000" without "--stat-graph-width=20" won't be pleasant.)

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] Platforms: Add Sanchip Zx296718 basic library

2017-08-10 Thread Leif Lindholm 
You've reworked this to be targeted for edk2-platforms, which is
excellent! However:
- Please update subject line to match.
- Please also cc edk2-devel@lists.01.org (since you are with Linaro,
  it makes sense to still cc linaro-uefi)

To get the appropriate subject line for edk2-devel, as desribed by
https://github.com/tianocore/edk2-platforms/blob/about/Readme.md it
would also be helpful to add
--subject-prefix="edk2-platforms/master][PATCH" to git format-patch
command line (unless Laszlo can point out a better way of achieving
the same).

On Wed, Aug 09, 2017 at 10:12:36PM +0800, Jun Nie wrote:
> Add Sanchip Zx296718 basic library files for Zx296718 SoC
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie 
> ---
>  .../Library/Zx296718EvbLib/Zx296718Evb.c   | 132 
> +
>  .../Library/Zx296718EvbLib/Zx296718EvbHelper.S |  50 
>  .../Library/Zx296718EvbLib/Zx296718EvbLib.inf  |  53 +
>  .../Library/Zx296718EvbLib/Zx296718EvbMem.c| 107 +
>  Silicon/Sanchip/Zx296718/Include/Zx296718.h|  30 +
>  Silicon/Sanchip/Zx296718/Zx296718.dec  |  32 +

It is more clear (especially when submitting new ports) to generate
the patches with --stat=1000, to make the full file paths visible.

See Laszlo's guide for helpful steps when submitting patches:
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

>  6 files changed, 404 insertions(+)
>  create mode 100644 
> Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
>  create mode 100644 
> Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbHelper.S
>  create mode 100644 
> Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbLib.inf
>  create mode 100644 
> Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718EvbMem.c
>  create mode 100644 Silicon/Sanchip/Zx296718/Include/Zx296718.h
>  create mode 100644 Silicon/Sanchip/Zx296718/Zx296718.dec

Since this ends up with a .dec, please rename
Silicon/Sanchip/Zx296718/ -> Silicon/Sanchip/Zx296718Pkg/.

That said, I don't see this .dec file providing any information that
is subsequently used enywhere. Is this in preparation for further
patches coming after this initial platform support?

> diff --git 
> a/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c 
> b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
> new file mode 100644
> index 000..4e4eb54
> --- /dev/null
> +++ b/Platform/Sanchip/Zx296718Evb/Library/Zx296718EvbLib/Zx296718Evb.c
> @@ -0,0 +1,132 @@
> +/** @file
> +*
> +*  Copyright (C) 2017 Sanechips Technology Co., Ltd.

Just a clarification: Sanchip/Sanechips?
"The Internet" suggests Sanechips is the appropriate company name, so
should the directory name change?

> +*  Copyright (c) 2017, Linaro Ltd.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 

Please sort include files alphabetically.

> +
> +#include 
> +
> +#include 
> +
> +ARM_CORE_INFO mZx296718EvbInfoTable[] = {
> +  {
> +// Cluster 0, Core 0
> +0x0, 0x0,
> +
> +// MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +(UINT64)0x
> +  },
> +  {
> +// Cluster 0, Core 1
> +0x0, 0x1,
> +
> +// MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +(UINT64)0x
> +  },
> +  {
> +// Cluster 0, Core 2
> +0x0, 0x2,
> +
> +// MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +(UINT64)0x
> +  },
> +  {
> +// Cluster 0, Core 3
> +0x0, 0x3,
> +
> +// MP Core MailBox Set/Get/Clear Addresses and Clear Value
> +(UINT64)0x
> +  },
> +};
> +
> +/**
> +  Return the current Boot Mode
> +
> +  This function returns the boot reason on the platform
> +
> +  @return   Return the current Boot Mode of the platform
> +
> +**/
> +EFI_BOOT_MODE
> +ArmPlatformGetBootMode (
> +  VOID
> +  )
> +{
> +  return BOOT_WITH_FULL_CONFIGURATION;
> +}
> +
> +/**
> +  Initialize controllers that must setup in the normal world
> +
> +  This function is called by the ArmPlatformPkg/Pei or 
> ArmPlatformPkg/Pei/PlatformPeim
> +  in the PEI phase.
> +
> +**/
> +RETURN_STATUS
> +ArmPlatformInitialize (
> +  IN  UINTN MpId
> +  )
> +{
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Initialize the system (or sometimes called permanent) memory
> +
> +  This memory is generally represented by the DRAM.
> +
> +**/
> +VOID
> +ArmPl