On Mon, Jun 26, 2023 at 10:34:05AM -0700, Lizhi Hou wrote: > On 6/21/23 13:22, Bjorn Helgaas wrote: > 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. s/Added/Add/ (twice, mentioned before). The commit log should say what the *patch* does, not what *you* did. > > > @@ -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? Makes a lot more sense to me than relying on some unrelated and undocumented property. Probably still would benefit from an #ifdef. Rob might have an opinion on whether "dynamic" makes sense from a DT perspective. Bjorn
