Re: [PATCH edk2-platforms v1 08/16] Hisilicon/D06: Change HCCS speed from 30G to 26G

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

On Tue, Feb 12, 2019 at 10:45:05PM +0800, Ming Huang wrote:
> 
> 
> On 2/12/2019 2:36 AM, Leif Lindholm wrote:
> > On Fri, Feb 01, 2019 at 09:34:28PM +0800, Ming Huang wrote:
> >> Follow chip team suggestion to change HCCS(Huawei Cache-Coherent
> >> System) speed from 30G to 26G, this modification can avoid some
> >> unstable stress issue.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> >> ---
> >>  Silicon/Hisilicon/Include/Library/OemMiscLib.h               | 10 ++++++++++
> >>  Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c |  8 ++++++++
> >>  2 files changed, 18 insertions(+)
> >>
> >> diff --git a/Silicon/Hisilicon/Include/Library/OemMiscLib.h b/Silicon/Hisilicon/Include/Library/OemMiscLib.h
> >> index dfac87d635d9..3c0cd0319122 100644
> >> --- a/Silicon/Hisilicon/Include/Library/OemMiscLib.h
> >> +++ b/Silicon/Hisilicon/Include/Library/OemMiscLib.h
> >> @@ -22,6 +22,11 @@
> >>  #include <PlatformArch.h>
> >>  #include <Library/I2CLib.h>
> >>  
> >> +#define HCCS_PLL_VALUE_3000  0x52240781
> >> +#define HCCS_PLL_VALUE_2600  0x52240681
> >> +#define HCCS_PLL_VALUE_2800  0x52240701
> > 
> > Could these be described by a proper macro instead of just values?
> > A cursory glance suggests that an increase of 0x80 in the lower half
> > means 200MHz.
> > 
> > If not, please sort them by frequency, ascending.
> 
> As the macros have use in other files, I prefer sort them by frequency.
> 
> > 
> >> +
> >> +
> >>  #define PCIEDEVICE_REPORT_MAX      8
> >>  #define MAX_PROCESSOR_SOCKETS      MAX_SOCKET
> >>  #define MAX_MEMORY_CHANNELS        MAX_CHANNEL
> >> @@ -55,4 +60,9 @@ extern EFI_STRING_ID gDimmToDevLocator[MAX_SOCKET][MAX_CHANNEL][MAX_DIMM];
> >>  EFI_HII_HANDLE EFIAPI OemGetPackages ();
> >>  UINTN OemGetCpuFreq (UINT8 Socket);
> >>  
> >> +UINTN
> >> +OemGetHccsFreq (
> >> +  VOID
> >> +  );
> >> +
> >>  #endif
> >> diff --git a/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c b/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
> >> index 8f2ac308c7b9..83e53cfeb5dd 100644
> >> --- a/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
> >> +++ b/Platform/Hisilicon/D06/Library/OemMiscLibD06/OemMiscLibD06.c
> >> @@ -223,3 +223,11 @@ UINTN OemGetCpuFreq (UINT8 Socket)
> >>    }
> >>  }
> >>  
> >> +UINTN
> >> +OemGetHccsFreq (
> > 
> > The commit message describes this patch as changing the frequency.
> > The actual code simply returns a value.
> > The name of the function returning this value suggests the value is a
> > frequency>
> >> +  VOID
> >> +  )
> >> +{
> >> +  return HCCS_PLL_VALUE_2600;
> > 
> > But the constant returned is named suggesting a PLL configuration
> > value. And the frequency suggested by the name is many orders of
> > magnitude below that described by the commit message.
> 
> Yes, the macros and function name are not very matched.
> I plan to modify the commit title and message:
> Hisilicon/D06: Use HCCS speed with 2.6G
> 
> Follow chip team suggestion, HCCS(Huawei Cache-Coherent System)
> may be unstable while speed is 3.0G, so use 2.6G to avoid some
> unstable stress issue.

This all sounds good, thanks.

/
    Leif