Re: [PATCH V9 2/5] PCI: Create device tree node for selected devices

[Bjorn Helgaas <helgaas@kernel.org>]

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