On 04/11/16 18:22, Ard Biesheuvel wrote: > On 11 April 2016 at 16:50, Laszlo Ersek <[email protected]> wrote: >> On 04/11/16 16:34, Ard Biesheuvel wrote: >>> On 11 April 2016 at 16:12, Laszlo Ersek <[email protected]> wrote: >>>> On 04/11/16 15:43, Ard Biesheuvel wrote: >> >> [snip] >> >>> I simply want to check that the 'pci-ecam-generic' DT node we end up >>> consuming is the only one that exists in the device tree. In fact, I >>> think it makes sense to assert that PcdPciExpressBaseAddress equals >>> the DT node reg property when we encounter it. >> >> Yes, that definitely makes sense (at least if I understand it >> correctly). That is, the library constructor would do the "reg" parsing >> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance, >> really), and *specifically* PciHostBridgeDxe, when it parses the same >> node (for those other two properties), could also look at "reg", and >> assert that PcdPciExpressBaseAddress is *already set* the way it expects >> it to be. I think that's a good idea. >> >>>> Do you have a fresh v3-wip branch somewhere? I'd like to try expressing >>>> my proposal for patches #13 and #14 in code. (If you are okay with that.) >>>> >>> >>> Sure, give me 30 mins. >> >> Let's do the following instead (as we discussed off-list): >> >> Please commit patches v2 1-10 (with the fixes I requested) to the master >> branch, and then we can collaborate, on the list, with patches for >> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master >> branch. >> > > OK, wip branch is here > git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3
Great, thank you. * Patch f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol is exactly what I had in mind. Please modify the commit message (as I pointed out previously, in the v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list of client drivers, plus update the count "three" to "four". With that, you can add my Reviewed-by: Laszlo Ersek <[email protected]> to the patch. I think it's also fine to push the first two patches to master afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol" and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node" (currently a889387fee53)). After pushing these two patches above (with my requested updates in the first one), can you please submit a smaller, separate series that focuses on the conversion of the PCI host node exclusively? Please see my comments below: * Patch 60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT client protocol is also precisely what I had in mind. - There is one typo in the commit message (with two instances): s/PciExpressBaseAddress/PcdPciExpressBaseAddress/ - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF default values in the two DSC files, saying that the MAX_UINT64 value means "undiscovered". - Furthermore, please modify the comment on PcdPciExpressBaseAddress in the INF file like this: ## CONSUMES ## SOMETIMES_PRODUCES With those tweaked: Reviewed-by: Laszlo Ersek <[email protected]> (However, this shouldn't be pushed without the rest below.) * Can you please submit a separate patch that removes PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"? * Patch 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol also looks mostly good. Some comments: - Please fix a typo in the commit message: s/host provided/host-provided/ - Also, the paragraph "This means some PCI related PCDs...", in the commit message, belongs to the next patch ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please move it over. - The ProcessPciHost() function's leading comment has become stale. Please drop it. - Please do not introduce those global variables just for returning data from ProcessPciHost() to InitializePciHostBridge(). Use locals and an appropriate signature for ProcessPciHost(). (You can collect those values in a new structure type too.) This might require you to bring back those explicit zero-assignments, before the "ranges" loop. - Since the MmioBase and MmioSize values are no longer kept in UINT32 PCDs (but UINT64 variables), the explicit (UINT64) cast is unnecessary, in InitializePciHostBridge(). - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are redundant, I think, after all of the GetNodeProperty() calls, in ProcessPciHost(). - Can you factor out the setting of the /chosen node to a separate function? (Together with the Tmp variable.) - In this (v3-wip) iteration, you also removed the PcdSetBool (PcdPciDisableBusEnumeration, FALSE); call. I almost suggested to reinstate it, but doing so wouldn't be safe actually. Namely, according to the build report, two drivers consume PcdPciDisableBusEnumeration: PciBusDxe and QemuFwCfgAcpiPlatformDxe. The PciBusDxe case is safe, because it is a UEFI driver model driver that depends on protocols produced by PciHostBridgeDxe. However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it consumes PcdPciDisableBusEnumeration first thing in its entry point. This means that you will have to introduce another plugin library (in a separate patch) just for setting PcdPciDisableBusEnumeration, and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe. Also, in this plugin library, since the PCD is just a boolean, we cannot use the caching trick (through a special value of the PCD), hence minimally the presence of "pci-host-ecam-generic" will have to be verified every time. Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
