| Message ID | 20190201133436.10500-9-ming.huang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix issues and improve D0x | expand |
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. > + > + > #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. / Leif > +} > + > -- > 2.9.5 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. Thanks. > > / > Leif > >> +} >> + >> -- >> 2.9.5 >> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 + + #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 ( + VOID + ) +{ + return HCCS_PLL_VALUE_2600; +} +
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(+) -- 2.9.5 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel