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

Message ID 20190201133436.10500-9-ming.huang@linaro.org
State New
Headers show
Series
  • Fix issues and improve D0x
Related show

Commit Message

Ming Huang Feb. 1, 2019, 1:34 p.m.
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

Comments

Leif Lindholm Feb. 11, 2019, 6:36 p.m. | #1
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
Ming Huang Feb. 12, 2019, 2:45 p.m. | #2
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
Leif Lindholm Feb. 12, 2019, 2:59 p.m. | #3
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

Patch

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;
+}
+