Re: [PATCH edk2-non-osi v1 2/7] Hisilicon/D0x: Rename StartupAp() function

[Ming Huang <ming.huang@linaro.org>]

On 2/12/2019 6:44 PM, Leif Lindholm wrote:
> On Tue, Feb 12, 2019 at 04:05:50PM +0800, Ming Huang wrote:
>> On 2/12/2019 5:36 AM, Leif Lindholm wrote:
>>> On Fri, Feb 01, 2019 at 10:25:02PM +0800, Ming Huang wrote:
>>>> As suggestion of community, 'AP' is a bit unfortunate to use in EDK2
>>>> context. PI specifies 'BSP' for Boot-strap Processor, as the one
>>>> executing all of the EDK2 code. It then uses 'AP' to refer to
>>>> Additional Processors, which can be assigned tasks using the
>>>> EFI_MP_SERVICES_PROTOCOL. In a TianoCore context, this should be
>>>> 'BSP'. So, Rename StartupAp() to StartUpBSP.
>>>
>>> Please add a comment somewhere that this applies to D0x
>>> PlatformSysCtrlLib.
>>
>> ok
>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ming Huang <ming.huang@linaro.org>
>>>> ---
>>>>  Silicon/Hisilicon/Hi1610/Library/PlatformSysCtrlLibHi1610/PlatformSysCtrlLibHi1610.lib | Bin 297590 -> 229128 bytes
>>>>  Silicon/Hisilicon/Hi1616/Library/PlatformSysCtrlLibHi1616/PlatformSysCtrlLibHi1616.lib | Bin 344310 -> 275312 bytes
>>>>  Silicon/Hisilicon/Hi1620/Library/PlatformSysCtrlLibHi1620/PlatformSysCtrlLibHi1620.lib | Bin 356032 -> 375916 bytes
>>>>  3 files changed, 0 insertions(+), 0 deletions(-)
>>>
>>> These are substantial changes in image size from only changing the
>>> name of a function. So I'll have a little look around.
>>>
>>> 1610 version appears to have switched from building with GCC49_RELEASE
>>> to GCC48_RELEASE.
>>> 1616 and 1620 versions seem to have used GCC48_RELEASE all along.
>>>
>>> I definitely see additional renamed functions in these libraries too.
>>>
>>> Please have an inventory and determine what may be affecting image sizes.
>>>
>>> Also, I *beg* you - please upgrade from "GNU C 4.8.3 20131202 (prerelease)".
>>
>> We have plan to upgrage gcc to 7.3, but our build server is share for all ARM
>> project, so need discuss with other project groups, it may be not enough time
>> for 19.02.
> 
> Oh, we're too late in the game to change for this release.
> But you are using ancient toolchains with poor code generation and
> quite likely known bugs.
> And this has been the state for quite some time.
> If that can change for 19.06, that's good enough.

Ok, I will upgrade gcc from 4.8 for 19.06.

> 
>> For D05/D03 libraries, just remove 2 functions from OemMiscLib which used
>> by PlatformSysCtrlLib. Does edk2 version effect the libraries size?
>> old edk2 base on: 2017-0904
>> now edk2 base on: 2018-0801
> 
> Well, changing edk2 version will mean that command line options to
> compiler and linker may change. So certainly some change can be seen.
> But when the changes are this big, I suspect something else has been
> going on.

I also think it is strange for big size change.

> 
>> For D06 library, we use the same source code to support all Hi1620 projects,
>> include product projects,so there are some modify for this, like support
>> 3 sockets, 4 sockets and remove some useless functions.
> 
> So please reword the subject line of this commit to explain it is an
> overall update of PlatformSysCtrlLib - including which bits are dropped.
> 
> And I think this makes a good argument for moving the header files for
> binary-only libraries from edk2-platforms to edk2-non-osi.
> If you do that in a separate patch before this one, you won't need to
> include as much detail in the commit message as you will otherwise.

Do youe mean move PlatformSysCtrlLib.h, OemAddressMapLib.h and LpcLib.h to edk2-non-osi?

> 
> Regards,
> 
> Leif
> 
>> Thanks.
>>
>>>
>>> /
>>>     Leif
>>>
>>>>
>>>> diff --git a/Silicon/Hisilicon/Hi1610/Library/PlatformSysCtrlLibHi1610/PlatformSysCtrlLibHi1610.lib b/Silicon/Hisilicon/Hi1610/Library/PlatformSysCtrlLibHi1610/PlatformSysCtrlLibHi1610.lib
>>>> index 68be770..4c63a26 100644
>>>> Binary files a/Silicon/Hisilicon/Hi1610/Library/PlatformSysCtrlLibHi1610/PlatformSysCtrlLibHi1610.lib and b/Silicon/Hisilicon/Hi1610/Library/PlatformSysCtrlLibHi1610/PlatformSysCtrlLibHi1610.lib differ
>>>> diff --git a/Silicon/Hisilicon/Hi1616/Library/PlatformSysCtrlLibHi1616/PlatformSysCtrlLibHi1616.lib b/Silicon/Hisilicon/Hi1616/Library/PlatformSysCtrlLibHi1616/PlatformSysCtrlLibHi1616.lib
>>>> index b3cc88e..cb2c652 100644
>>>> Binary files a/Silicon/Hisilicon/Hi1616/Library/PlatformSysCtrlLibHi1616/PlatformSysCtrlLibHi1616.lib and b/Silicon/Hisilicon/Hi1616/Library/PlatformSysCtrlLibHi1616/PlatformSysCtrlLibHi1616.lib differ
>>>> diff --git a/Silicon/Hisilicon/Hi1620/Library/PlatformSysCtrlLibHi1620/PlatformSysCtrlLibHi1620.lib b/Silicon/Hisilicon/Hi1620/Library/PlatformSysCtrlLibHi1620/PlatformSysCtrlLibHi1620.lib
>>>> index 50d453a..d643f7b 100644
>>>> Binary files a/Silicon/Hisilicon/Hi1620/Library/PlatformSysCtrlLibHi1620/PlatformSysCtrlLibHi1620.lib and b/Silicon/Hisilicon/Hi1620/Library/PlatformSysCtrlLibHi1620/PlatformSysCtrlLibHi1620.lib differ
>>>> -- 
>>>> 2.9.5
>>>>