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:   Wed, 21 Jun 2023 15:27:22 -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: <20230621202722.GA116477@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230621202233.GA115496@bhelgaas>
Precedence: bulk

On Wed, Jun 21, 2023 at 03:22:33PM -0500, 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

I forgot: with these comments addressed:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> 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.
> 
> > +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.
> 
> > @@ -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.
> 
> >  			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";
> 
> > +	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; ...)
> 
> > +		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?
> 
> > +			 */
> > +			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.
> 
> > +	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.
> 
> > +#endif /* CONFIG_PCI_DYNAMIC_OF_NODES */
> 
> Unnecessary comment since this is all 10 lines.