On 11 April 2016 at 21:14, Laszlo Ersek <[email protected]> wrote: > 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. >
Actually, looking at this again, wouldn't it make sense to have a single plugin library that sets both PcdPciDisableBusEnumeration and PcdPciExpressBaseAddress, and use it for these two modules and for the BaseCachingPciExpressLib? _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
