On 6/21/23 13:22, Bjorn Helgaas wrote:
In subject, IIUC this patch does not actually create device tree nodes
for selected devices. It looks like it:
- Adds an of_pci_make_dev_node() *interface* that can be used to
create this node
- Creates such a node for *every* bridge
- Does nothing at all for "selected devices" or the Xilinx Alveo
On Wed, Jun 21, 2023 at 10:34:06AM -0700, Lizhi Hou wrote:
The PCI endpoint device such as Xilinx Alveo PCI card maps the register
spaces from multiple hardware peripherals to its PCI BAR. Normally,
the PCI core discovers devices and BARs using the PCI enumeration process.
There is no infrastructure to discover the hardware peripherals that are
present in a PCI device, and which can be accessed through the PCI BARs.
For Alveo PCI card, the card firmware provides a flattened device tree to
describe the hardware peripherals on its BARs. The Alveo card driver can
load this flattened device tree and leverage device tree framework to
generate platform devices for the hardware peripherals eventually.
The Alveo details are relevant to the quirk patch but not to *this*
patch.
But the reason for creating a node for every bridge device *is*
relevant and should be included here, since that change affects
everybody that uses OF.
Apparently, the device tree framework requires a device tree node for the
PCI device. Thus, it can generate the device tree nodes for hardware
peripherals underneath. Because PCI is self discoverable bus, there might
not be a device tree node created for PCI devices. This patch is to add
support to generate device tree node for PCI devices.
s/This patch is to add/Add/
Added a kernel option. When the option is turned on, the kernel will
generate device tree nodes for PCI bridges unconditionally.
s/Added a kernel option/Add a PCI_DYNAMIC_OF_NODES config option/
(Be specific, and way what the patch does, not what you did.)
Initially, the basic properties are added for the dynamically generated
device tree nodes.
Make this specific, e.g., list the specific properties added.
I rewrote the description as below. Does it look better?
PCI: Create device tree node for bridge
The PCI endpoint device such as Xilinx Alveo PCI card maps the register
spaces from multiple hardware peripherals to its PCI BAR. Normally,
the PCI core discovers devices and BARs using the PCI enumeration
process.
There is no infrastructure to discover the hardware peripherals
that are
present in a PCI device, and which can be accessed through the PCI
BARs.
Apparently, the device tree framework requires a device tree node
for the
PCI device. Thus, it can generate the device tree nodes for hardware
peripherals underneath. Because PCI is self discoverable bus, there
might
not be a device tree node created for PCI devices. Furthermore, if
the PCI
device is hot pluggable, when it is plugged in, the device tree
nodes for
its parent bridges are required. Add support to generate device
tree node
for PCI bridges.
Added an of_pci_make_dev_node() interface that can be used to create
device tree node for PCI devices.
Added a PCI_DYNAMIC_OF_NODES config option. When the option is
turned on,
the kernel will generate device tree nodes for PCI bridges
unconditionally.
Initially, the basic properties are added for the dynamically generated
device tree nodes which include #address-cells, #size-cells,
device_type,
compatible, ranges, reg.
+config PCI_DYNAMIC_OF_NODES
+ bool "Create Devicetree nodes for PCI devices"
+ depends on OF
+ select OF_DYNAMIC
+ help
+ This option enables support for generating device tree nodes for some
+ PCI devices. Thus, the driver of this kind can load and overlay
+ flattened device tree for its downstream devices.
+
+ Once this option is selected, the device tree nodes will be generated
+ for all PCI bridges.
Is there a convention for using "devicetree" vs "device tree"? The
help message uses both and it would be nice to only use one or the
other.
Ok. Will use "device tree".
@@ -501,8 +501,10 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
* to rely on this function (you ship a firmware that doesn't
* create device nodes for all PCI devices).
*/
- if (ppnode)
+ if (ppnode && of_property_present(ppnode, "interrupt-map"))
Maybe this deserves a comment? The connection between "interrupt-map"
and the rest of this patch isn't obvious to me.
Also, it looks like this happens for *everybody*, regardless of
PCI_DYNAMIC_OF_NODES, which seems a little suspect. If it's an
unrelated bug fix it should be a different patch.
This is not a bug fix. The check will distinguish between device tree
nodes automatically created for pci bridges by this patch with those
created by a DT based system. With this patch, device tree nodes are
created for pci bridges, thus ppnode here will be non-zero and we will
break out of the loop. In order to still use
pci_swizzle_interrupt_pin(), checking “interrupt-map” for ppnode is
added here.
After thinking about this more, using “interrupt-map” property may not
be correct for the cases where ppnode is not dynamically generated and
it does not have “interrupt-map”. So, I would introduce a new property
“dynamic” for pci bridge nodes generated dynamically. And change the
code to: if (ppnode && of_property_present(ppnode, "dynamic")).
Does this make sense?
break;
+ else
+ ppnode = NULL;
+void of_pci_make_dev_node(struct pci_dev *pdev)
+{
+ struct device_node *ppnode, *np = NULL;
+ const char *pci_type = "dev";
+ struct of_changeset *cset;
+ const char *name;
+ int ret;
+
+ /*
+ * If there is already a device tree node linked to this device,
+ * return immediately.
+ */
+ if (pci_device_to_OF_node(pdev))
+ return;
+
+ /* Check if there is device tree node for parent device */
+ if (!pdev->bus->self)
+ ppnode = pdev->bus->dev.of_node;
+ else
+ ppnode = pdev->bus->self->dev.of_node;
+ if (!ppnode)
+ return;
+
+ if (pci_is_bridge(pdev))
+ pci_type = "pci";
Initialize pci_type = "dev" here instead of way up top:
if (pci_is_bridge(pdev))
pci_type = "pci";
else
pci_type = "dev";
sure.
+ name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
+ struct device_node *np)
+{
+ struct of_pci_range *rp;
+ struct resource *res;
+ int i = 0, j, ret;
+ u32 flags, num;
+ u64 val64;
+
+ if (pci_is_bridge(pdev)) {
+ num = PCI_BRIDGE_RESOURCE_NUM;
+ res = &pdev->resource[PCI_BRIDGE_RESOURCES];
+ } else {
+ num = PCI_STD_NUM_BARS;
+ res = &pdev->resource[PCI_STD_RESOURCES];
+ }
+
+ rp = kcalloc(num, sizeof(*rp), GFP_KERNEL);
+ if (!rp)
+ return -ENOMEM;
+
+ for (j = 0; j < num; j++) {
Initialize i = 0 here so it's connected with the use:
for (i = 0, j = 0; j < num; ...)
ok.
+ if (!resource_size(&res[j]))
+ continue;
+
+ if (of_pci_get_addr_flags(&res[j], &flags))
+ continue;
+
+ val64 = res[j].start;
+ of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
+ false);
+ if (pci_is_bridge(pdev)) {
+ memcpy(rp[i].child_addr, rp[i].parent_addr,
+ sizeof(rp[i].child_addr));
+ } else {
+ /*
+ * For endpoint device, the lower 64-bits of child
+ * address is always zero.
For the non-OF folks (like me), can you say what the semantics of
parent_addr vs child_addr are? I suppose maybe parent_addr is an
address on the primary side of a bridge and child_addr is the
corresponding address on the secondary side?
And PCI bridges don't perform address translation, so they are
identical?
I will add more comments here.
+ */
+ rp[i].child_addr[0] = j;
+ }
+int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
+ struct device_node *np)
+{
+ int ret = 0;
+
+ if (pci_is_bridge(pdev)) {
+ ret |= of_changeset_add_prop_string(ocs, np, "device_type",
+ "pci");
+ }
+
+ ret |= of_pci_prop_ranges(pdev, ocs, np);
+ ret |= of_changeset_add_prop_u32(ocs, np, "#address-cells",
+ OF_PCI_ADDRESS_CELLS);
+ ret |= of_changeset_add_prop_u32(ocs, np, "#size-cells",
+ OF_PCI_SIZE_CELLS);
+ ret |= of_pci_prop_reg(pdev, ocs, np);
+ ret |= of_pci_prop_compatible(pdev, ocs, np);
+
+ /*
+ * The added properties will be released when the
+ * changeset is destroyed.
+ */
I don't think it's meaningful to OR together the "negative error
values" returned by all these functions. Presumably those are things
like -EINVAL, -ENOMEM, etc. ORing them together is admittedly
non-zero, but yields nonsense.
ok. I will return for each failure.
+ return ret;
+static inline void
+of_pci_make_dev_node(struct pci_dev *pdev)
+{
+}
+
+static inline void
+of_pci_remove_node(struct pci_dev *pdev)
+{
+}
Pull these functions all onto one line, like other similar stubs in
this file.
Sure.
+#endif /* CONFIG_PCI_DYNAMIC_OF_NODES */
Unnecessary comment since this is all 10 lines.
Will remove it.
Thanks,
Lizhi
