Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Jun 2023 13:11:55 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Lizhi Hou <lizhi.hou@amd.com>
Cc:     linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, robh@kernel.org, max.zhen@amd.com,
        sonal.santan@amd.com, stefano.stabellini@xilinx.com
Subject: Re: [PATCH V9 2/5] PCI: Create device tree node for selected devices
Message-ID: <20230626181155.GA250405@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <af9b6bb3-a98d-4fb6-b51e-b48bca61dada@amd.com>
Precedence: bulk

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