[edk2-devel][edk2-platforms][PATCH v1 6/7] KabylakeOpenBoardPkg/AspireVn7Dash572G: Improve board detection

["Benjamin Doron" <benjamin.doron00@gmail.com>]

Improvements based on ENE KB9012Q datasheet. Values read from EC ADC are
much more consistent now. Some improvement still necessary before this
is reliable, as my Rayleigh-SL (PCH-LP) is now consistently detected as
a Newgate-SLS (PCH-H).

Cc: Sai Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 .../Include/Library/BoardEcLib.h              |  5 +-
 .../Library/BoardEcLib/BoardEcLib.inf         |  1 +
 .../Library/BoardEcLib/EcCommands.c           | 36 ++++++++++----
 .../BoardInitLib/PeiAspireVn7Dash572GDetect.c | 47 +++++++++++--------
 .../AspireVn7Dash572G/OpenBoardPkg.dsc        |  5 +-
 .../PeiBoardPolicyUpdate.c                    |  2 +-
 .../Include/PlatformBoardId.h                 |  5 +-
 7 files changed, 65 insertions(+), 36 deletions(-)

diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library/BoardEcLib.h b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library/BoardEcLib.h
index 8bb4cccb8f19..56fdd4ed756c 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library/BoardEcLib.h
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Include/Library/BoardEcLib.h
@@ -82,8 +82,9 @@ EcIdxWrite (
   );
 
 /**
-  Read EC analog-digital converter.
-  TODO: Check if ADC is valid.
+  Read an analog-digital converter from the EC.
+  TODO: There are actually 8 ADCs, but those can remain unused.
+  - Handling port enable bits and pin IE could get complicated.
 
   @param[in]  Adc
   @param[out] DataBuffer
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/BoardEcLib.inf b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/BoardEcLib.inf
index 56527c3b9a3c..7287301583e0 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/BoardEcLib.inf
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/BoardEcLib.inf
@@ -18,6 +18,7 @@
   DebugLib
   EcLib
   IoLib
+  TimerLib
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/EcCommands.c b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/EcCommands.c
index 54cfaba47b1b..182cda6f1933 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/EcCommands.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardEcLib/EcCommands.c
@@ -9,9 +9,11 @@
 
 #include <Base.h>
 #include <Uefi.h>
+#include <Library/BoardEcLib.h>
 #include <Library/DebugLib.h>
 #include <Library/EcLib.h>
 #include <Library/IoLib.h>
+#include <Library/TimerLib.h>
 
 /*
  * Notes:
@@ -193,8 +195,9 @@ EcIdxWrite (
 }
 
 /**
-  Read EC analog-digital converter.
-  TODO: Check if ADC is valid.
+  Read an analog-digital converter from the EC.
+  TODO: There are actually 8 ADCs, but those can remain unused.
+  - Handling port enable bits and pin IE could get complicated.
 
   @param[in]  Adc
   @param[out] DataBuffer
@@ -205,23 +208,36 @@ ReadEcAdcConverter (
   OUT UINT16       *DataBuffer
   )
 {
-  UINT8            AdcConvertersEnabled;  // Contains some ADCs and some DACs
+  UINT8            LowAdcsEnabled;  // Contains some ADCs and some DACs
   UINT8            IdxData;
 
   if (DataBuffer == NULL) {
     return;
   }
 
+  if (Adc >= 4) {
+    DEBUG ((DEBUG_ERROR, "Handling ADC%d is unsupported!\n", Adc));
+    return;
+  }
+
   // Backup enabled ADCs
-  EcIdxRead (0xff15, &AdcConvertersEnabled);  // ADDAEN
+  EcIdxRead (0xff15, &LowAdcsEnabled);   // ADDAEN
 
-  // Enable desired ADC in bitmask (not enabled by EC FW, not used by vendor FW)
-  EcIdxWrite (0xff15, AdcConvertersEnabled | ((1 << Adc) & 0xf));  // ADDAEN
+  /* 1. Clear IE of the related pin - ADC0: "GPIOIE38[0] (0xFC67[0]) = 0b" */
+  EcIdxRead (0xfc67, &IdxData);
+  IdxData &= ~(1 << Adc);
+  EcIdxWrite (0xfc67, IdxData);
 
-  // Sample the desired ADC in binary field; OR the start bit
-  EcIdxWrite (0xff18, ((Adc << 1) & 0xf) | 1);  // ADCTRL
+  /* 2. Enable desired ADC function in bitmask */
+  EcIdxWrite (0xff15, (1 << Adc) & 0xf);  // ADDAEN
 
-  // Read the desired ADC
+  /* 3. Enable control of desired ADC in bit field, OR the start bit */
+  EcIdxWrite (0xff18, ((Adc << 1) & 7) | 1);  // ADCTRL
+
+  /* TODO: Await ADC interrupt */
+  MicroSecondDelay (256);  // Wait "Voltage Conversion Time"
+
+  /* 4. Read the desired ADC */
   EcIdxRead (0xff19, &IdxData);  // ADCDAT
   *DataBuffer = (IdxData << 2);
   // Lower 2-bits of 10-bit ADC are in high bits of next register
@@ -229,5 +245,5 @@ ReadEcAdcConverter (
   *DataBuffer |= ((IdxData & 0xc0) >> 6);
 
   // Restore enabled ADCs
-  EcIdxWrite (0xff15, AdcConvertersEnabled);  // ADDAEN
+  EcIdxWrite (0xff15, LowAdcsEnabled);   // ADDAEN
 }
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardInitLib/PeiAspireVn7Dash572GDetect.c b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardInitLib/PeiAspireVn7Dash572GDetect.c
index 344e06859e9b..0ce747bb67c6 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardInitLib/PeiAspireVn7Dash572GDetect.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Library/BoardInitLib/PeiAspireVn7Dash572GDetect.c
@@ -1,6 +1,7 @@
 /** @file
 
 Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2021, Baruch Binyamin Doron<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -10,14 +11,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BoardEcLib.h>
 #include <Library/DebugLib.h>
 
-#define ADC_3V_10BIT_GRANULARITY_MAX  (3005/1023)
+#define ADC_3V_10BIT_GRANULARITY_MAX  (3005 / 1023)
 #define PCB_VER_AD                    1
 #define MODEL_ID_AD                   3
 
 /**
-  Get Aspire V Nitro (Skylake) board ID.
-  There are 2 different boards having different ID.
-  This function will return board ID to caller.
+  Get Aspire V Nitro (Skylake) board ID. There are 3 different boards
+  having different PCH (therefore, ID). This function will return board ID to caller.
+  - TODO/NB: Detection is still unreliable. Likely must await interrupt.
 
   @param[out] DataBuffer
 
@@ -32,34 +33,42 @@ GetAspireVn7Dash572GBoardId (
   UINT16        DataBuffer;
 
   ReadEcAdcConverter (MODEL_ID_AD, &DataBuffer);
-  DEBUG ((DEBUG_INFO, "BoardId (raw) = 0x%X\n", DataBuffer));
+  DEBUG ((DEBUG_INFO, "BoardId (raw) = %d\n", DataBuffer));
   // Board by max millivoltage range (of 10-bit, 3.005 V ADC)
-  if (DataBuffer <= (1374/ADC_3V_10BIT_GRANULARITY_MAX)) {
+  if (DataBuffer <= (1374 / ADC_3V_10BIT_GRANULARITY_MAX)) {
     // Consider returning an error
     DEBUG ((DEBUG_ERROR, "BoardId is reserved?\n"));
-  } else if (DataBuffer <= (2017/ADC_3V_10BIT_GRANULARITY_MAX)) {
-    *BoardId = BoardIdNewgateSLx_dGPU;
+    *BoardId = 0;
+  } else if (DataBuffer <= (2017 / ADC_3V_10BIT_GRANULARITY_MAX)) {
+    *BoardId = BoardIdNewgateSLS_dGPU;
+  } else if (DataBuffer <= (2259 / ADC_3V_10BIT_GRANULARITY_MAX)) {
+    *BoardId = BoardIdRayleighSLS_960M;
   } else {
-    *BoardId = BoardIdRayleighSLx_dGPU;
+    *BoardId = BoardIdRayleighSL_dGPU;
   }
   DEBUG ((DEBUG_INFO, "BoardId = 0x%X\n", *BoardId));
 
   ReadEcAdcConverter (PCB_VER_AD, &DataBuffer);
-  DEBUG ((DEBUG_INFO, "PCB version (raw) = 0x%X\n", DataBuffer));
+  DEBUG ((DEBUG_INFO, "PCB version (raw) = %d\n", DataBuffer));
   DEBUG ((DEBUG_INFO, "PCB version: "));
   // PCB by max millivoltage range (of 10-bit, 3.005 V ADC)
-  if (DataBuffer <= (2017/ADC_3V_10BIT_GRANULARITY_MAX)) {
+  if (DataBuffer <= (2017 / ADC_3V_10BIT_GRANULARITY_MAX)) {
     // Consider returning an error
     DEBUG ((DEBUG_ERROR, "Reserved?\n"));
-  } else if (DataBuffer <= (2259/ADC_3V_10BIT_GRANULARITY_MAX)) {
-    DEBUG ((DEBUG_ERROR, "-1\n"));
-  } else if (DataBuffer <= (2493/ADC_3V_10BIT_GRANULARITY_MAX)) {
-    DEBUG ((DEBUG_ERROR, "SC\n"));
-  } else if (DataBuffer <= (2759/ADC_3V_10BIT_GRANULARITY_MAX)) {
-    DEBUG ((DEBUG_ERROR, "SB\n"));
+  } else if (DataBuffer <= (2259 / ADC_3V_10BIT_GRANULARITY_MAX)) {
+    DEBUG ((DEBUG_INFO, "-1\n"));
+  } else if (DataBuffer <= (2493 / ADC_3V_10BIT_GRANULARITY_MAX)) {
+    DEBUG ((DEBUG_INFO, "SC\n"));
+  } else if (DataBuffer <= (2759 / ADC_3V_10BIT_GRANULARITY_MAX)) {
+    DEBUG ((DEBUG_INFO, "SB\n"));
   } else {
-    DEBUG ((DEBUG_ERROR, "SA\n"));
+    DEBUG ((DEBUG_INFO, "SA\n"));
   }
+
+  // FIXME
+  DEBUG ((DEBUG_WARN, "OVERRIDE: Detection is unreliable and other boards unsupported!\n"));
+  DEBUG ((DEBUG_INFO, "Setting board SKU to Rayleigh-SL\n"));
+  *BoardId = BoardIdRayleighSL_dGPU;
 }
 
 EFI_STATUS
@@ -76,7 +85,7 @@ AspireVn7Dash572GBoardDetect (
 
   DEBUG ((DEBUG_INFO, "AspireVn7Dash572GDetectionCallback\n"));
   GetAspireVn7Dash572GBoardId (&BoardId);
-  if (BoardId == BoardIdNewgateSLx_dGPU || BoardId == BoardIdRayleighSLx_dGPU) {
+  if (BoardId == BoardIdNewgateSLS_dGPU || BoardId == BoardIdRayleighSLS_960M || BoardId == BoardIdRayleighSL_dGPU) {
     LibPcdSetSku (BoardId);
     ASSERT (LibPcdGetSku() == BoardId);
   } else {
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
index 75c537f1253f..4458e7b75118 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/OpenBoardPkg.dsc
@@ -88,8 +88,9 @@
 [SkuIds]
   0x00|DEFAULT                # 0|DEFAULT is reserved and always required.
   # For further details on specific SKUs (which dGPU installed), see EC page of schematics
-  0x41|RayleighSLx_dGPU       # Detect the UMA board by GPIO
-  0x42|NewgateSLx_dGPU
+  0x41|NewgateSLS_dGPU
+  0x42|RayleighSLS_960M
+  0x43|RayleighSL_dGPU        # Detect the UMA board by GPIO
 
 ################################################################################
 #
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c
index 95b7c4ad5f77..54c742147b19 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c
+++ b/Platform/Intel/KabylakeOpenBoardPkg/AspireVn7Dash572G/Policy/Library/PeiSiliconPolicyUpdateLib/PeiBoardPolicyUpdate.c
@@ -175,7 +175,7 @@ PeiFspBoardPolicyUpdate (
   //       that it does - this appears to be static text?) and is UART0 merely supporting
   //       the UART2 devfn?
 
-  // Acer IDs (TODO: "Newgate" IDs)
+  // Acer IDs (TODO: "Newgate" and "RayleighSLS" IDs)
 //FIXME  FspsUpd->FspsConfig.DefaultSvid = 0x1025;
 //FIXME  FspsUpd->FspsConfig.DefaultSid = 0x1037;
   PchGeneralConfig->SubSystemVendorId = 0x1025;
diff --git a/Platform/Intel/KabylakeOpenBoardPkg/Include/PlatformBoardId.h b/Platform/Intel/KabylakeOpenBoardPkg/Include/PlatformBoardId.h
index 0db4fb23583e..78ea0dea88df 100644
--- a/Platform/Intel/KabylakeOpenBoardPkg/Include/PlatformBoardId.h
+++ b/Platform/Intel/KabylakeOpenBoardPkg/Include/PlatformBoardId.h
@@ -22,8 +22,9 @@ Kaby Lake Platform Board Identifiers
 
 #define BoardIdSkylakeRvp3                  0x4
 #define BoardIdGalagoPro3                   0x20
-#define BoardIdRayleighSLx_dGPU             0x41
-#define BoardIdNewgateSLx_dGPU              0x42
+#define BoardIdNewgateSLS_dGPU              0x41
+#define BoardIdRayleighSLS_960M             0x42
+#define BoardIdRayleighSL_dGPU              0x43
 #define BoardIdKabyLakeYLpddr3Rvp3          0x60
 
 #define BoardIdUnknown1                     0xffff
-- 
2.37.2