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

["Huangming (Mark)" <huangming23@huawei.com>]

On 2018/1/30 21:21, Leif Lindholm wrote:
> 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)];
> ?
> 

In my mind, simply replacing can not be compiled, but actually it works.
I will replace it as your comments.

Thanks,
Ming

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

-- 
Best Regards,

Ming