Date:   Tue, 3 Jan 2023 11:45:58 -0800
From:   Lizhi Hou <lizhi.hou@....com>
To:     Clément Léger <clement.leger@...tlin.com>
CC:     <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <robh@...nel.org>,
        <frowand.list@...il.com>, <helgaas@...nel.org>, <max.zhen@....com>,
        <sonal.santan@....com>, <larry.liu@....com>, <brian.xu@....com>,
        <stefano.stabellini@...inx.com>, <trix@...hat.com>,
        "Allan.Nielsen@...rochip.com" <Allan.Nielsen@...rochip.com>,
        "Horatiu.Vultur@...rochip.com" <Horatiu.Vultur@...rochip.com>,
        "Steen.Hegelund@...rochip.com" <Steen.Hegelund@...rochip.com>
Subject: Re: [PATCH V5 2/3] PCI: Create device tree node for selected devices


On 1/2/23 09:16, Clément Léger wrote:
> Le Thu, 15 Dec 2022 09:30:45 -0800,
> Lizhi Hou <lizhi.hou@....com> a écrit :
>
>> 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.
>>
>> 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.
>>
>> Added a kernel 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.
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@....com>
>> Signed-off-by: Sonal Santan <sonal.santan@....com>
>> Signed-off-by: Max Zhen <max.zhen@....com>
>> Reviewed-by: Brian Xu <brian.xu@....com>
>> ---
>>   drivers/pci/Kconfig         |  12 ++
>>   drivers/pci/Makefile        |   1 +
>>   drivers/pci/bus.c           |   2 +
>>   drivers/pci/msi/irqdomain.c |   6 +-
>>   drivers/pci/of.c            |  71 ++++++++++++
>>   drivers/pci/of_property.c   | 222 ++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci-driver.c    |   3 +-
>>   drivers/pci/pci.h           |  19 +++
>>   drivers/pci/remove.c        |   1 +
>>   9 files changed, 334 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/pci/of_property.c
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 55c028af4bd9..1b2347aa1e5b 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -198,6 +198,18 @@ config PCI_HYPERV
>>   	  The PCI device frontend driver allows the kernel to import arbitrary
>>   	  PCI devices from a PCI backend to support PCI driver domains.
>>   
>> +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.
>> +
>>   choice
>>   	prompt "PCI Express hierarchy optimization setting"
>>   	default PCIE_BUS_DEFAULT
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 2680e4c92f0a..cc8b4e01e29d 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_PCI_P2PDMA)	+= p2pdma.o
>>   obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>>   obj-$(CONFIG_VGA_ARB)		+= vgaarb.o
>>   obj-$(CONFIG_PCI_DOE)		+= doe.o
>> +obj-$(CONFIG_PCI_DYNAMIC_OF_NODES) += of_property.o
>>   
>>   # Endpoint library must be initialized before its users
>>   obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 3cef835b375f..8507cc32b61d 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -316,6 +316,8 @@ void pci_bus_add_device(struct pci_dev *dev)
>>   	 */
>>   	pcibios_bus_add_device(dev);
>>   	pci_fixup_device(pci_fixup_final, dev);
>> +	if (pci_is_bridge(dev))
>> +		of_pci_make_dev_node(dev);
>>   	pci_create_sysfs_dev_files(dev);
>>   	pci_proc_attach_device(dev);
>>   	pci_bridge_d3_update(dev);
>> diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
>> index e9cf318e6670..eeaf44169bfd 100644
>> --- a/drivers/pci/msi/irqdomain.c
>> +++ b/drivers/pci/msi/irqdomain.c
>> @@ -230,8 +230,10 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
>>   	pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
>>   
>>   	of_node = irq_domain_get_of_node(domain);
>> -	rid = of_node ? of_msi_map_id(&pdev->dev, of_node, rid) :
>> -			iort_msi_map_id(&pdev->dev, rid);
>> +	if (of_node && !of_node_check_flag(of_node, OF_DYNAMIC))
>> +		rid = of_msi_map_id(&pdev->dev, of_node, rid);
>> +	else
>> +		rid = iort_msi_map_id(&pdev->dev, rid);
>>   
>>   	return rid;
>>   }
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index 196834ed44fe..cb34a73ac8a3 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -469,6 +469,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
>>   		} else {
>>   			/* We found a P2P bridge, check if it has a node */
>>   			ppnode = pci_device_to_OF_node(ppdev);
>> +			if (ppnode && of_node_check_flag(ppnode, OF_DYNAMIC))
>> +				ppnode = NULL;
>>   		}
>>   
>>   		/*
>> @@ -599,6 +601,75 @@ int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge)
>>   	return pci_parse_request_of_pci_ranges(dev, bridge);
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_PCI_DYNAMIC_OF_NODES)
>> +
>> +void of_pci_remove_node(struct pci_dev *pdev)
>> +{
>> +	struct device_node *np;
>> +
>> +	np = pci_device_to_OF_node(pdev);
>> +	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
>> +		return;
>> +	pdev->dev.of_node = NULL;
>> +
>> +	of_destroy_node(np);
>> +}
>> +
>> +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";
>> +
>> +	name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
>> +			 PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +	if (!name)
>> +		goto failed;
>> +
>> +	np = of_create_node(ppnode, name, &cset);
>> +	if (!np)
>> +		goto failed;
>> +
>> +	ret = of_pci_add_properties(pdev, cset, np);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	ret = of_changeset_apply(cset);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	pdev->dev.of_node = np;
>> +	kfree(name);
>> +
>> +	return;
>> +
>> +failed:
>> +	if (np)
>> +		of_destroy_node(np);
>> +	kfree(name);
>> +}
>> +#endif
>> +
>>   #endif /* CONFIG_PCI */
>>   
>>   /**
>> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
>> new file mode 100644
>> index 000000000000..05c8ca05a71b
>> --- /dev/null
>> +++ b/drivers/pci/of_property.c
>> @@ -0,0 +1,222 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <linux/of.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <asm/unaligned.h>
>> +#include "pci.h"
>> +
>> +#define OF_PCI_ADDRESS_CELLS		3
>> +#define OF_PCI_SIZE_CELLS		2
>> +
>> +struct of_pci_addr_pair {
>> +	u32		phys_addr[OF_PCI_ADDRESS_CELLS];
>> +	u32		size[OF_PCI_SIZE_CELLS];
>> +};
>> +
>> +struct of_pci_range {
>> +	u32		child_addr[OF_PCI_ADDRESS_CELLS];
>> +	u32		parent_addr[OF_PCI_ADDRESS_CELLS];
>> +	u32		size[OF_PCI_SIZE_CELLS];
>> +};
>> +
>> +#define OF_PCI_ADDR_SPACE_CONFIG	0x0
>> +#define OF_PCI_ADDR_SPACE_IO		0x1
>> +#define OF_PCI_ADDR_SPACE_MEM32		0x2
>> +#define OF_PCI_ADDR_SPACE_MEM64		0x3
>> +
>> +#define OF_PCI_ADDR_FIELD_NONRELOC	BIT(31)
>> +#define OF_PCI_ADDR_FIELD_SS		GENMASK(25, 24)
>> +#define OF_PCI_ADDR_FIELD_PREFETCH	BIT(30)
>> +#define OF_PCI_ADDR_FIELD_BUS		GENMASK(23, 16)
>> +#define OF_PCI_ADDR_FIELD_DEV		GENMASK(15, 11)
>> +#define OF_PCI_ADDR_FIELD_FUNC		GENMASK(10, 8)
>> +#define OF_PCI_ADDR_FIELD_REG		GENMASK(7, 0)
>> +
>> +#define OF_PCI_ADDR_HI			GENMASK_ULL(63, 32)
>> +#define OF_PCI_ADDR_LO			GENMASK_ULL(31, 0)
>> +#define OF_PCI_SIZE_HI			GENMASK_ULL(63, 32)
>> +#define OF_PCI_SIZE_LO			GENMASK_ULL(31, 0)
>> +
>> +enum of_pci_prop_compatible {
>> +	PROP_COMPAT_PCI_VVVV_DDDD,
>> +	PROP_COMPAT_PCICLASS_CCSSPP,
>> +	PROP_COMPAT_PCICLASS_CCSS,
>> +	PROP_COMPAT_NUM,
>> +};
>> +
>> +static int of_pci_prop_device_type(struct pci_dev *pdev,
>> +				   struct of_changeset *ocs,
>> +				   struct device_node *np)
>> +{
>> +	return of_changeset_add_prop_string(ocs, np, "device_type", "pci");
>> +}
>> +
>> +static int of_pci_prop_address_cells(struct pci_dev *pdev,
>> +				     struct of_changeset *ocs,
>> +				     struct device_node *np)
>> +{
>> +	return of_changeset_add_prop_u32(ocs, np, "#address_cells",
>> +					 OF_PCI_ADDRESS_CELLS);
>> +}
>> +
>> +static int of_pci_prop_size_cells(struct pci_dev *pdev,
>> +				  struct of_changeset *ocs,
>> +				  struct device_node *np)
>> +{
>> +	return of_changeset_add_prop_u32(ocs, np, "#size_cells",
>> +					 OF_PCI_SIZE_CELLS);
>> +}
>> +
>> +static void of_pci_set_address(u32 *prop, u64 addr, u32 flags)
>> +{
>> +	prop[0] = flags;
>> +	put_unaligned(addr, &prop[1]);
>> +}
> Here, the put_unaligned() call will assume you want to store a 32bits
> value since prop is pointing to a u32 array. This can't work.
>
> Moreover, when storing a 64bits address in device-tree properties, they
> are stored starting with their 32 MSB in the first cell and the
> 32 LSB in the next cells.
>
> This should probably be something like this:
>
> static void of_pci_set_address(u32 *prop, u64 addr, u32 flags)
> {
> 	prop[0] = flags;
> 	prop[1] = addr >> 32;
> 	prop[2] = addr;
> }
>
> [snip]

Thanks for pointing this out. And I should add (__be64 *) as below.

  put_unaligned(addr, (__be64 *)&prop[1]);

>
>> +
>> +static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
>> +			      struct device_node *np)
>> +{
>> +	struct of_pci_range rp[PCI_BRIDGE_RESOURCE_NUM];
>> +	struct resource *res;
>> +	int i = 0, j, ret;
>> +	u64 val64;
>> +	u32 flags;
>> +
>> +	res = &pdev->resource[PCI_BRIDGE_RESOURCES];
>> +	for (j = 0; j < PCI_BRIDGE_RESOURCE_NUM; j++) {
>> +		if (!resource_size(&res[j]))
>> +			continue;
>> +
>> +		flags = OF_PCI_ADDR_FIELD_NONRELOC;
>> +		if (of_pci_get_addr_flags(&res[j], &flags))
>> +			continue;
>> +
>> +		val64 = res[j].start;
>> +		of_pci_set_address(rp[i].parent_addr, val64, flags);
>> +		of_pci_set_address(rp[i].child_addr, val64, flags);
>> +
>> +		val64 = resource_size(&res[j]);
>> +		put_unaligned(val64, rp[i].size);
> Same problem here, the size is meant to be a 64 bits values but will
> assume you want to store it using a 32bits pointer.
Will change to: put_unaligned(val64, (__be64 *)rp[i].size);
>
>> +
>> +		i++;
>> +	}
>> +
>> +	ret = of_changeset_add_prop_u32_array(ocs, np, "ranges", (u32 *)rp,
>> +					      i * sizeof(*rp) / sizeof(u32));
>> +
>> +	return ret;
>> +}
>> +
>> +static int of_pci_prop_reg(struct pci_dev *pdev, struct of_changeset *ocs,
>> +			   struct device_node *np)
>> +{
>> +	struct of_pci_addr_pair *reg;
>> +	int i = 1, resno, ret = 0;
>> +	u32 reg_val, base_addr;
>> +	resource_size_t sz;
>> +
>> +	reg = kzalloc(sizeof(*reg) * (PCI_STD_NUM_BARS + 1), GFP_KERNEL);
>> +	if (!reg)
>> +		return -ENOMEM;
>> +
>> +	reg_val = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, OF_PCI_ADDR_SPACE_CONFIG) |
>> +		FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) |
>> +		FIELD_PREP(OF_PCI_ADDR_FIELD_DEV, PCI_SLOT(pdev->devfn)) |
>> +		FIELD_PREP(OF_PCI_ADDR_FIELD_FUNC, PCI_FUNC(pdev->devfn));
>> +	of_pci_set_address(reg[0].phys_addr, 0, reg_val);
>> +
>> +	base_addr = PCI_BASE_ADDRESS_0;
>> +	for (resno = PCI_STD_RESOURCES; resno <= PCI_STD_RESOURCE_END;
>> +	     resno++, base_addr += 4) {
>> +		sz = pci_resource_len(pdev, resno);
>> +		if (!sz)
>> +			continue;
>> +
>> +		ret = of_pci_get_addr_flags(&pdev->resource[resno], &reg_val);
>> +		if (!ret)
>> +			continue;
>> +
>> +		reg_val &= ~OF_PCI_ADDR_FIELD_REG;
>> +		reg_val |= FIELD_PREP(OF_PCI_ADDR_FIELD_REG, base_addr);
>> +		of_pci_set_address(reg[i].phys_addr, 0, reg_val);
>> +		put_unaligned((u64)sz, reg[i].size);
> And same here. Something like this should probably be used (untested):

will change to: put_unaligned((u64)sz, (__be64 *)reg[i].size);


Thanks,

Lizhi

>
> static void of_pci_set_size(u32 *prop, u64 size)
> {
> 	prop[0] = size >> 32;
> 	prop[1] = size;
> }

>

Powered by blists - more mailing lists