[edk2-devel] [edk2-platforms][PATCH v2 0/5] KabylakeOpenBoardPkg: Add AspireVn7Dash572G

Mon Aug 16 05:22:18 UTC 2021

Hi Benjamin,

First of all this is fantastic work, it is very clear that you have put a lot of effort into this. Thank you for doing so! I do have some minor code review comments for you. Here is a summary of my feedback:

1. KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PcieDeviceTable.c: Line 16:

   I think it would be a good idea to file a Bugzilla for this. Could you please do so?

2. KabylakeOpenBoardPkg/AspireVn7Dash572G/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiBoardPolicyUpdate.c

   A lot of this code in this file demonstrates that is probably makes sense to create a top level PolicyUpdateLib in KabylakeOpenBoardPkg that the various board ports import because most of these settings look either identical to RVP3/GalagoPro3 or they come from PCDs and hence the code will always be generic.

Definitely not something you need to do for this patch series, but something to think about for a future enhancement.

2(a). Line 49:

   I find the whole situation with PchHpetBusNumber, PchHpetBusDeviceNumber, PchHpetFunctionNumber, PchHpetBdfValid to be very odd.

   I looked though the FSP source code to see what these policy options do and it boils down to if(PchHpetBdfValid) then the FSP will program the following register:

   https://edc.intel.com/content/www/us/en/design/products-and-solutions/processors-and-chipsets/comet-lake-u/intel-400-series-chipset-on-package-platform-controller-hub-register-database/hpet-bus-device-function-hbdf-offset-70/

   I can confirm that in the FSP source code that the default FSP UPD values differ from the default config block values. Moreover, the reference BIOS takes the config blocks and converts them into FSP UPDs just like the open source code, so most of the testing for KBL FSP was actually done without the default UPD values. We consider cases like this where the UPD and the config block defaults don't match to be a bug, but given the age of the KBL FSP getting that bug fixed is difficult at this point.

   Have you tried booting these settings?
   PchHpetBdfValid = 0
   PchHpetBusNumber = 0
   PchHpetBusDeviceNumber = 0
   PchHpetFunctionNumber = 0

   That is what the reference BIOS uses.

   The equivalent UPD and Config Block defaults for I/O APIC do match.

2(b). Line 50:
   The default for PeciC10Reset should totally be 1. Good catch!

   I have created a patch to fix the properly. Please merge https://edk2.groups.io/g/devel/message/79328

   Then we should be able to remove this line.

2(c). Line 97:
   This is OK for your initial patch series but please make sure work on a follow up patch series that moves this to a setup option since it is insecure to have this be the default.

2(d). Line 107:
   From what I remember the VN-572G only supports S3 and not S0ix. In which case setting PchPmSlpS0VmEnable = 0 would be mandatory because that feature only works in S0ix mode. Historically most systems with switchable graphics have opted for S3 instead of S0ix because power state transitions are more difficult to implement correctly in S0ix.

   Please add a comment that describes this.

2(e). Line 198:
   Please create a #define for "3" that describes why these settings are significant for only the first 3 USB2 ports.