Re: [PATCH edk2-platforms v2 05/15] Hisilicon D03/D05: Add capsule upgrade support

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Jan 30, 2018 at 08:48:27PM +0800, Huangming (Mark) wrote:
> 
> 
> On 2018/1/30 3:58, Leif Lindholm wrote:
> > A few style comments below.
> > 
> > On Fri, Jan 26, 2018 at 04:00:40PM +0800, Ming Huang wrote:
> >> From: Jason Zhang <zhangjinsong2@huawei.com>
> >>
> >> This module support updating the boot CPU firmware only.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Jason Zhang <zhangjinsong2@huawei.com>
> >> Signed-off-by: Ming Huang <huangming23@huawei.com>
> >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >> ---
> >>  Platform/Hisilicon/D03/Capsule/SystemFirmwareUpdateConfig/SystemFirmwareUpdateConfig.ini |  45 +++++++
> >>  Platform/Hisilicon/D03/D03.dsc                                                           |  17 ++-
> >>  Platform/Hisilicon/D03/D03.fdf                                                           |  70 +++++++++++
> >>  Platform/Hisilicon/D03/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.aslc    |  81 +++++++++++++
> >>  Platform/Hisilicon/D03/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.inf     |  50 ++++++++
> >>  Platform/Hisilicon/D03/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptorPei.c    |  70 +++++++++++
> >>  Platform/Hisilicon/D05/Capsule/SystemFirmwareUpdateConfig/SystemFirmwareUpdateConfig.ini |  45 +++++++
> >>  Platform/Hisilicon/D05/D05.dsc                                                           |  19 ++-
> >>  Platform/Hisilicon/D05/D05.fdf                                                           |  70 +++++++++++
> >>  Platform/Hisilicon/D05/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.aslc    |  81 +++++++++++++
> >>  Platform/Hisilicon/D05/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.inf     |  50 ++++++++
> >>  Platform/Hisilicon/D05/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptorPei.c    |  70 +++++++++++
> >>  Silicon/Hisilicon/Hisilicon.dsc.inc                                                      |  11 +-
> >>  Silicon/Hisilicon/Hisilicon.fdf.inc                                                      |   9 ++
> >>  Silicon/Hisilicon/Library/PlatformFlashAccessLib/PlatformFlashAccessLibDxe.c             | 123 ++++++++++++++++++++
> >>  Silicon/Hisilicon/Library/PlatformFlashAccessLib/PlatformFlashAccessLibDxe.inf           |  51 ++++++++
> >>  16 files changed, 859 insertions(+), 3 deletions(-)
> >>
> > 
> >> diff --git a/Platform/Hisilicon/D03/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.aslc b/Platform/Hisilicon/D03/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.aslc
> >> new file mode 100644
> >> index 0000000..d9f4a00
> >> --- /dev/null
> >> +++ b/Platform/Hisilicon/D03/Drivers/SystemFirmwareDescriptor/SystemFirmwareDescriptor.aslc
> >> @@ -0,0 +1,81 @@
> >> +/** @file
> >> +  System Firmware descriptor.
> >> +
> >> +  Copyright (c) 2018, Hisilicon Limited. All rights reserved.
> >> +  Copyright (c) 2018, Linaro Limited. All rights reserved.
> >> +  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
> >> +
> >> +  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 <PiPei.h>
> >> +#include <Guid/EdkiiSystemFmpCapsule.h>
> >> +#include <Protocol/FirmwareManagement.h>
> >> +
> >> +#define PACKAGE_VERSION                     0xFFFFFFFF
> >> +#define PACKAGE_VERSION_STRING              L"Unknown"
> >> +
> >> +#define CURRENT_FIRMWARE_VERSION            0x00000002
> >> +#define CURRENT_FIRMWARE_VERSION_STRING     L"0x00000002"
> >> +#define LOWEST_SUPPORTED_FIRMWARE_VERSION   0x00000001
> >> +
> >> +#define IMAGE_ID                            SIGNATURE_64('H','W','A', 'R', 'M', '_', 'F', 'd')
> >> +#define IMAGE_ID_STRING                     L"ARMPlatformFd"
> >> +
> >> +// PcdSystemFmpCapsuleImageTypeIdGuid
> >> +#define IMAGE_TYPE_ID_GUID                  { 0x44c850f2, 0x85ff, 0x4be5, { 0xbf, 0x34, 0xa5, 0x95, 0x28, 0xdf, 0x22, 0xd3 } }
> >> +
> >> +typedef struct {
> >> +  EDKII_SYSTEM_FIRMWARE_IMAGE_DESCRIPTOR  Descriptor;
> >> +  // real string data
> >> +  CHAR16                                  ImageIdNameStr[sizeof(IMAGE_ID_STRING) / sizeof(CHAR16)];
> >> +  CHAR16                                  VersionNameStr[sizeof(CURRENT_FIRMWARE_VERSION_STRING) / sizeof(CHAR16)];
> >> +  CHAR16                                  PackageVersionNameStr[sizeof(PACKAGE_VERSION_STRING) / sizeof(CHAR16)];
> > 
> > Use ARRAY_SIZE for the 3 above?
> 
> If use ARRAY_SIZE, the three macro must change to array of CHAR16.

What problem are you encountering when simply replacing 
  CHAR16                                  ImageIdNameStr[sizeof(IMAGE_ID_STRING) / sizeof(CHAR16)];
with
  CHAR16                                  ImageIdNameStr[ARRAY_SIZE (IMAGE_ID_STRING)];
?

> It is not necessary maybe.The same style is found in
> SystemFirmwareDescriptor.aslc from other platform,like
> AMD,Socionext.

Patches welcome.

ARRAY_SIZE was only added to MdePkg in October 2016.
Ideally, all code should be using this macro, but at least we can try
to make sure all _new_ code uses it, when we spot missed opportunities.

/
    Leif