On 22-02-2024 22:36, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments
On Thu, Feb 22, 2024 at 06:11:10PM +0530, Vidya Sagar wrote:
Add support for preserving the boot configuration done by the
platform firmware per host bridge basis, based on the presence of
'linux,pci-probe-only' property in the respective PCIe host bridge
device-tree node. It also unifies the ACPI and DT based boot flows
in this regard.
+/**
+ * of_pci_bridge_check_probe_only - Return true if the boot configuration
+ * needs to be preserved
I don't like the "check_probe_only" name because it's a boolean
function but the name doesn't tell me what a true/false return value
means. Something like "preserve_resources" would be better. If you
want "probe_only", even removing the "check" would help.
I'll change it in the next patch.
+ * @node: Device tree node with the domain information.
+ *
+ * This function looks for "linux,pci-probe-only" property for a given
+ * PCIe controller's node and returns true if found. Having this property
+ * for a PCIe controller ensures that the kernel doesn't re-enumerate and
+ * reconfigure the BAR resources that are already done by the platform firmware.
This is generic PCI, not PCIe-specific (also in commit log and comment
below).
I think "enumeration" specifically refers to discovering what devices
are present, and the kernel always does that, so drop that part.
Reconfiguring BARs and bridge windows is what we want to prevent.
Agree and I'll address it in the next patch.
+ * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe bridge device
+ * is limited to the hierarchy under that particular bridge device. whereas
+ * the scope of "linux,pci-probe-only" defined within chosen node is
+ * system wide.
+ *
+ * Return: true if the property exists false otherwise.
+ */
+bool of_pci_bridge_check_probe_only(struct device_node *node)
+{
+ return of_property_read_bool(node, "linux,pci-probe-only");
+}
+EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only);
Why does this need to be exported for modules and exposed via
include/linux/pci.h?
On a second through, I think it is not required to be exported.
I'll address this also in the next patch.
+static void pci_check_config_preserve(struct pci_host_bridge *host_bridge)
+{
+ if (&host_bridge->dev) {
Checking &host_bridge->dev doesn't seem like the right way to
determine whether this is an ACPI host bridge.
Honestly, I couldn't find a clear way to differentiate between an ACPI based
host bridge and a DT based host bridge. Hence, the current code tries to get
the information using both ways and since a system can only be either
ACPI or
DT based, but one at a time, preserve_config will be set only once (assuming
the system wants it to be set). Let me know if there is a better approach
for this?
I was looking at the way 'external_facing' gets set in both the boot
flows and
I see that there is no common place for it and the respective flows have
their
functions separately i.e. pci_acpi_set_external_facing() for ACPI and
pci_set_bus_of_node() for DT.
Thanks,
Vidya Sagar
+ union acpi_object *obj;
+
+ /*
+ * Evaluate the "PCI Boot Configuration" _DSM Function. If it
+ * exists and returns 0, we must preserve any PCI resource
+ * assignments made by firmware for this host bridge.
+ */
+ obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1,
+ DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
+ if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
+ host_bridge->preserve_config = 1;
+ ACPI_FREE(obj);
+ }
