From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 0/5] KabylakeOpenBoardPkg: Add AspireVn7Dash572G To: Nate DeSimone ,devel@edk2.groups.io From: "Benjamin Doron" X-Originating-Location: Thornhill, Ontario, CA (24.52.200.135) X-Originating-Platform: Linux Firefox 90 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Wed, 18 Aug 2021 13:05:59 -0700 References: In-Reply-To: Message-ID: <5012.1629317159376896674@groups.io> Content-Type: multipart/alternative; boundary="5JcdxEhcK7buYAll6dJr" --5JcdxEhcK7buYAll6dJr Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Nate, Firstly, thanks! I'll respond to these comments here, and the rest on the main patch. 1. I've added it to my to-do list 2. We could also consider calling https://github.com/tianocore/edk2-platfor= ms/blob/master/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchPolicyLib= /PchSamplePreMemPolicyLib.c and https://github.com/tianocore/edk2-platforms= /blob/master/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchPolicyLib/P= chSamplePolicyLib.c ( https://github.com/tianocore/edk2-platforms/blob/mast= er/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchPolicyLib/PchSamplePo= licyLib.c, ) , the same way that a sample policy is called for SA at https:= //github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/KabylakeSil= iconPkg/SystemAgent/Library/PeiSaPolicyLib/PeiSaPolicyLib.c#L372. I'm also = aware that some of this file duplicates a *PolicyLib, but I never got aroun= d to following the specific policy entries being created or removing lines = from this file and testing. 2(a). I've dropped the HPET and IO APIC BDF settings, and the board still b= oots. I think I added these when it wasn't booting, so I tried to align as = closely with what I thought was correct. They never got dropped, I guess. 2(b). Okay, I can drop this line when that patch is merged. I don't think i= t's crucial for boot, though. 2(c). I'll try to do that, but I can also drop these lines until I get to i= t. 2(d). Where do you see that? Yes, the deepest the system has gone is packag= e C10. However, the FSP default for PchPmSlpS0Enable=3D1 is preserved. I do= n't know what support this feature depends on, so I don't know if the board= is missing it. If it's only S0ix, perhaps it should be enabled and it may = work once I figure out what needs to be power-gated. Of course, there could= be board-specific blockers: the WLAN doesn't support ASPM L0s. 2(e)-(i). Done. 3. This definition is used by BaseEcLib at https://github.com/tianocore/edk= 2-platforms/blob/master/Platform/Intel/KabylakeOpenBoardPkg/Library/BaseEcL= ib/BaseEcLib.c#L35. While this command is never issued, a definition is nee= ded to be able to use the library. 4(a)-(b). Done. I chose EcSendTime() instead. 4(c). I've implemented this function, which is now moved to DxeBoardInitLib= . Currently, I've only reverse engineered this function's HW interaction, n= ot the context in which the vendor firmware calls it's protocol. 4(d). Done. 5. I know. Are you responding to a previous comment I had here about enabli= ng modules, writing libraries and enabling the CSM? Currently, my change he= re is to PcdSiCatalogDebugEnable, which is conditionally set for https://gi= thub.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/KabylakeSilicon= Pkg/SiPkgBuildOption.dsc#L43-L45 to allow me to have release-optimised imag= es with debug printing enabled. I may be appropriating this PCD, as I think= we discussed, but it has no other purpose in the edk2-platforms code. I've also addressed your comment on VBT location. Regards, Benjamin --5JcdxEhcK7buYAll6dJr Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Nate,
Firstly, thanks!

I'll respond to these comments he= re, and the rest on the main patch.

1. I've added it to my to-do= list

2. We could also consider calling https://github.com/tianocore/edk2-platforms/blob/master/Si= licon/Intel/KabylakeSiliconPkg/Pch/Library/PeiPchPolicyLib/PchSamplePreMemP= olicyLib.c and https://github.com/t= ianocore/edk2-platforms/blob/master/Silicon/Intel/KabylakeSiliconPkg/Pch/Li= brary/PeiPchPolicyLib/PchSamplePolicyLib.c, the same way that a sample = policy is called for SA at https= ://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/KabylakeSi= liconPkg/SystemAgent/Library/PeiSaPolicyLib/PeiSaPolicyLib.c#L372. I'm = also aware that some of this file duplicates a *PolicyLib, but I never got = around to following the specific policy entries being created or removing l= ines from this file and testing.

2(a). I've dropped the HPET and= IO APIC BDF settings, and the board still boots. I think I added these whe= n it wasn't booting, so I tried to align as closely with what I thought was= correct. They never got dropped, I guess.

2(b). Okay, I can dro= p this line when that patch is merged. I don't think it's crucial for boot,= though.

2(c). I'll try to do that, but I can also drop these li= nes until I get to it.

2(d). Where do you see that? Yes, the dee= pest the system has gone is package C10. However, the FSP default for PchPm= SlpS0Enable=3D1 is preserved. I don't know what support this feature depend= s on, so I don't know if the board is missing it. If it's only S0ix, perhap= s it should be enabled and it may work once I figure out what needs to be p= ower-gated. Of course, there could be board-specific blockers: the WLAN doe= sn't support ASPM L0s.

2(e)-(i). Done.

3. This defini= tion is used by BaseEcLib at https://github.com/tian= ocore/edk2-platforms/blob/master/Platform/Intel/KabylakeOpenBoardPkg/Librar= y/BaseEcLib/BaseEcLib.c#L35. While this command is never issued, a defi= nition is needed to be able to use the library.

4(a)-(b). Done. = I chose EcSendTime() instead.

4(c). I've implemented this functi= on, which is now moved to DxeBoardInitLib. Currently, I've only reverse eng= ineered this function's HW interaction, not the context in which the vendor= firmware calls it's protocol.

4(d). Done.

5. I know.= Are you responding to a previous comment I had here about enabling modules= , writing libraries and enabling the CSM? Currently, my change here is to P= cdSiCatalogDebugEnable, which is conditionally set for https= ://github.com/tianocore/edk2-platforms/blob/master/Silicon/Intel/KabylakeSi= liconPkg/SiPkgBuildOption.dsc#L43-L45 to allow me to have release-optim= ised images with debug printing enabled. I may be appropriating this PCD, a= s I think we discussed, but it has no other purpose in the edk2-platforms c= ode.

I've also addressed your comment on VBT location.

Regards,
Benjamin --5JcdxEhcK7buYAll6dJr--