And there's also the issue with maintaining cache coherence on systems that use system caches (MMIO-based), that CpuDxe wouldn't be aware of (that is, FdtBusDxe would expect such cache drivers to implement a particular protocol).

PciHostBridgeLibEcam.c assumes the "ranges" map from PCI memory/IO spaces to CPU space, but that doesn't necessarily have to be true. But the code has no way of detecting whether the parent value is a CPU address or not. Like the reg parsing code, we ought to try translating all the way to the root and let clients see the additional translatedbase value.

See #11

DtIoLookup first translates the ASCII DT path to an EFI_DEVICE_PATH_PROTOCOL, then uses DtPathToHandle to lookup/connect the handle. DtPathToHandle uses LocateDevicePath.

Glancing at LocateDevicePath, this seems rather suboptimal, as it can iterate over potentially quite a lot of handles (esp. if LocateDevicePath is successively called as controllers are connected).

Another issue is that LocateDevicePath has no notion of "fuzzy" matching, e.g. when we want /soc/pci to match /soc/pci@3000000 (dropping the unit address in unambiguous situations).

This should be fixed by directly traversing the ASCII DT path. DT_DEVICE has a Parent already, and could be made to keep a linked list of children as well.

The other place DtPathToHandle is used is inside DtDeviceCreate, to avoid creating duplicates. Again, if a DtDevice knows what children it has, this problem becomes much cheaper to solve.

gDeviceTreeBase is initialized based on the pointer in DT HOB. This is fine, since the bus driver only does read access. However, if a setprop-like interface is ever added (matching FdtClientDxe functionality), then this will need to change. Instead, fdt_open_into should be used with an appropriately-sized buffer.

Today FdtBusPkg provides a PciHostBridgeLib (PciHostBridgeLibEcam), for use with MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf, which is somewhat limited:

• Only one RC node is supported.
• Not driver-binding based.
• No support for non-cache coherent or strange DMA ranges.

See #11, #2, #3, #4, #5.

Currently the Supported () function goes against the spec. It doesn't validate RemainingDevicePath.

Also validate the Start () logic wrt RemainingPath.

Really for the tool apps, not anything else.

See #11

Provide example template code that can be quickly used as a base for a writing a driver.

This should bind to a test device.

Will be necessary to use this driver on non-ecam platforms. This basically should be an init/fini pair + function to read/write a config space register. Also add a VOID *Context to _PCI_ROOT_BRIDGE_INSTANCE so the lib can maintain extra state if needed.

All state manipulated by init/fini/cfg accessors should be pushed into a common struct, i.e.:

  UINT32                                              Segment;
  EFI_DT_REG                                          ConfigReg;
  UINT64                                              Attributes;
  UINT64                                              Supports;
  EFI_DT_RANGE                                        BusRange;
  EFI_DT_RANGE                                        IoRange;
  EFI_DT_RANGE                                        MemRange;
  EFI_DT_RANGE                                        PMemRange;
  EFI_DT_RANGE                                        MemAbove4GRange;
  EFI_DT_RANGE                                        PMemAbove4GRange;
  BOOLEAN                                             DmaAbove4G;
  BOOLEAN                                             NoExtendedConfigSpace;

Originally posted by @andreiw in #12 (comment)

Provide example template code that can be quickly used as a base for a writing a driver.

This should bind to a test device, and show child enumeration, I/O callbacks. The child should be supported by #62.

This is because EFI_DT_BUS_ADDRESS is defined as a __int128 (as FDT_MAX_NCELLS == 4).

Any cpu fences/barriers are implied by Map/Unmap and Reg access code. If code uses common buffer operations, it would need to explicitly use barriers/fences on architectures requiring these.

It will be interesting to figure out AllocateBuffer if there is a "MinAddress" that isn't 0.

This should be inherited by all devices below, the attributes ought to be applied automatically.

See #11

Need to figure this out. Basically could expect IOMMU device drivers to publish an IOMMU protocol that would be used by FdtBusDxe.

Describe the principles of operation, interface for device drivers to use that abstracts access to DT/device.

Use case is ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c.

E.g. a driver binding to the RTC device need to disable the RTC device so that the OS always goes through UEFI RT.

Depends on #8

Depends on new NonCoherent flag in EFI_DT_IO_PROTOCOL_DMA_EXTRA

Originally posted by @andreiw in #12 (comment)

There's also the the issue with ordering, e.g. ensuring correct barrier instructions are used on RV and AArch64.

See #19

The RISC-V Sec code processes all the "memory" devices in Memory.c, so HighMemDxe code does absolutely nothing.

To test HighMemDxe, this is what I do:

--- a/OvmfPkg/RiscVVirt/Sec/Memory.c
+++ b/OvmfPkg/RiscVVirt/Sec/Memory.c
@@ -253,6 +253,7 @@ MemoryPeimInitialization (
   INT32                       Node, Prev;
   INT32                       Len;
   VOID                        *FdtPointer;
+  BOOLEAN                     Once = TRUE;

   FirmwareContext = NULL;
   GetFirmwareContextPointer (&FirmwareContext);
@@ -285,18 +286,21 @@ MemoryPeimInitialization (
         CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
         CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));

-        DEBUG ((
-          DEBUG_INFO,
-          "%a: System RAM @ 0x%lx - 0x%lx\n",
-          __func__,
-          CurBase,
-          CurBase + CurSize - 1
-          ));
+        if (Once) {
+          DEBUG ((
+            DEBUG_INFO,
+            "%a: System RAM @ 0x%lx - 0x%lx\n",
+            __func__,
+            CurBase,
+            CurBase + CurSize - 1
+            ));

-        InitializeRamRegions (
-          CurBase,
-          CurSize
-          );
+          InitializeRamRegions (
+            CurBase,
+            CurSize
+            );
+          Once = FALSE;
+        }
       } else {
         DEBUG ((
           DEBUG_ERROR,

• Should enforce Width <= EfiPciWidthUint64
• Should avoid using a temporary buffer

See #11

Platform devices drivers in Tiano have common issues around DMA handling. On some platforms, the device may not be cache coherent, or have a reduced range of memory that can be addressed via DMA. This is handled by linking in the appropriate DmaLib at build time.

With an FdtBusPkg approach the bus driver itself, like the PCI bus driver, provides the DMA API and hides the complexity even from the firmware integrator/builder. That is, functionality wise, FdtBusDxe must implement both EmbeddedPkg/Library/CoherentDmaLib and EmbeddedPkg/Library/NonCoherentDmaLib internally, using the appropriate properties (cache coherence, dma-ranges) to guide choice of implementation.

There's also the the issue with ordering, e.g. ensuring correct barrier instructions are used on RV and AArch64. And there's also the issue with maintaining cache coherence on systems that use system caches (MMIO-based), that CpuDxe wouldn't be aware of (that is, FdtBusDxe would expect such cache drivers to implement a particular protocol).

E.g. flash driver.

Need to be able to pass additional DMA constraints, e.g. a 32-bit only PCI device may be used in a system that's otherwise capable of 64-bit DMA.

The simple way is to just to make this a bitfield attribute. It should only be necessary to support PCI devices on RCs described with DT. Everything else should be describe-able in DT.

This is a good example of Uefi DM vs legacy code, as well as use of uefi,critical. Document DtIo->Lookup in PlatformBm as an alternative to uefi,critical. Document «memory» devices being treated like uefi,critical

PI spec sez:

... the DXE driver that produces the EFI_CPU_ARCH_PROTOCOL must
seed the GCD memory space map with the initial state of the^M
attributes for all the memory regions visible to the processor.

Instead the GCD attrs remain 0, even for regions that get accessible page table mappings.

Originally posted by @andreiw in #12 (comment)

FS0:> dtreg /soc/rtc@101000 0
Dumping 1 bytes at offset 0x0 of reg via CPU 0x101000(1000):
!!!! RISCV64 Exception Type - 000000000000000D(EXCEPT_RISCV_LOAD_ACCESS_PAGE_FAULT) !!!!

E.g, for EFI_DT_REG that translate to a CPU address, the code should ensure
the range is registered with the gDS (e.g. AddMemorySpace + SetMemorySpaceAttributes)

Doing this in ParseProp/GetReg/GetRegByName is better then forcing callers to use yet another call (e.g. SetRegAttrs). And doing this for every (functional) device enumerated by FdtBusDxe is not a good idea because of overhead. GetReg seems like an okay compromise - it's rarely called, for one (once per driver per device, ideally).

...with a signature like the following. This has no use in a driver, but it would be super-useful for the DtProp tool, to be able to explore a Devicetree in a UEFI system.

typedef
EFI_STATUS
(EFIAPI *EFI_DT_IO_PROTOCOL_GET_PROP_NAME)(
  IN  EFI_DT_IO_PROTOCOL *This,
  IN  UINTN              Index,
  OUT CONST CHAR8        **String
  );

Implementation-wise, this could do something like. Of course it would be terribly inefficient... but this shouldn't matter for a debug aid.

do {
	tag = fdt_next_tag(fdt, offset, &nextoffset);
	if (tag == FDT_END) {
		return;
	}

	if (tag == FDT_PROP) {
		prop = _fdt_offset_ptr(fdt, offset);
		n = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));

		for  (i = 0; i < depth; i++) {
			printk(" ");
		}
		printk("%s: 0x%x@0x%x\n", n,
		       fdt32_to_cpu(prop->len),
		       prop->data);
	}

	offset = nextoffset;
} while ((tag != FDT_BEGIN_NODE) && (tag != FDT_END_NODE));

E.g. driver model based approaches, "library drivers"

A legacy device is one that ought to be ignored by a UEFI Driver Model compliant device driver. Legacy devices are all bound to by FdtBusPkg (so that their children can be enumerated), and the legacy status is inherited.

A device is considered legacy under 3 circumstances:

• uefi,legacy property
• ScanChildrenAsLegacy (or a new param to ScanChildren?)

Should enforce Width <= EfiPciWidthUint64

A driver may wish to map a particular reg range with specific memory attributes. This should take into account #44.

This would improve error handling in DriverStart

Need to add NonCoherent flag in EFI_DT_IO_PROTOCOL_DMA_EXTRA

Originally posted by @andreiw in #104 (comment)

Consider both a very deep hierarchy and repeated access to a particular node's properties. This is coming from a concern voiced from a 3rd party about overhead of parsing a giant DT tree, and may prompt optimizations to the FDT parsing code itself (which ultimately means taking in the libfdt code directly in instead of having this be a dependent on a Tiano library).

Support EFI_DT_VALUE_DEVICE and EFI_DT_VALUE_U128.

Follow the example in PciBusDxe.