[PATCH RFC 0/2] Generate device tree node for pci devices

[PATCH RFC 0/2] Generate device tree node for pci devices

[Lizhi Hou]

This patch series introduces OF overlay support for PCI devices which
primarily addresses two use cases. First, it provides a data driven method
to describe hardware peripherals that are present in a PCI endpoint and
hence can be accessed by the PCI host. An example device is Xilinx/AMD
Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
driver -- often used in SoC platforms -- in a PCI host based system. An
example device is Microchip LAN9662 Ethernet Controller.

This patch series consolidates previous efforts to define such an
infrastructure:
https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/

Normally, the PCI core discovers PCI devices and their BARs using the
PCI enumeration process. However, the process does not provide a way to
discover the hardware peripherals that are present in a PCI device, and
which can be accessed through the PCI BARs. Also, the enumeration process
does not provide a way to associate MSI-X vectors of a PCI device with the
hardware peripherals that are present in the device. PCI device drivers
often use header files to describe the hardware peripherals and their
resources as there is no standard data driven way to do so. This patch
series proposes to use flattened device tree blob to describe the
peripherals in a data driven way. Based on previous discussion, using
device tree overlay is the best way to unflatten the blob and populate
platform devices. To use device tree overlay, there are three obvious
problems that need to be resolved.

First, we need to create a base tree for non-DT system such as x86_64. A
patch series has been submitted for this:
https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/

Second, a device tree node corresponding to the PCI endpoint is required
for overlaying the flattened device tree blob for that PCI endpoint.
Because PCI is a self-discoverable bus, a device tree node is usually not
created for PCI devices. This series adds support to generate a device
tree node for a PCI device which advertises itself using PCI quirks
infrastructure.

Third, we need to generate device tree nodes for PCI bridges since a child
PCI endpoint may choose to have a device tree node created.

This patch series is made up of two patches.

The first patch is adding OF interface to allocate an OF node. It is copied
from:
https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/

The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
is turned on, the kernel will generate device tree nodes for all PCI
bridges unconditionally. The patch also shows how to use the PCI quirks
infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
a device. Specifically, the patch generates a device tree node for Xilinx
Alveo U50 PCIe accelerator device. The generated device tree nodes do not
have any property. Future patches will add the necessary properties.

Clément Léger (1):
  of: dynamic: add of_node_alloc()

Lizhi Hou (1):
  pci: create device tree node for selected devices

 drivers/of/dynamic.c        |  50 +++++++++++++----
 drivers/pci/Kconfig         |  11 ++++
 drivers/pci/bus.c           |   2 +
 drivers/pci/msi/irqdomain.c |   6 +-
 drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-driver.c    |   3 +-
 drivers/pci/pci.h           |  16 ++++++
 drivers/pci/quirks.c        |  11 ++++
 drivers/pci/remove.c        |   1 +
 include/linux/of.h          |   7 +++
 10 files changed, 200 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH RFC 1/2] of: dynamic: add of_node_alloc()

[Lizhi Hou]

From: Clément Léger <clement.leger@bootlin.com>

Add of_node_alloc() which allows to create nodes. The node full_name
field is allocated as part of the node allocation and the kfree() for
this field is checked at release time to allow users using their own
allocated name.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/dynamic.c | 50 +++++++++++++++++++++++++++++++++++---------
 include/linux/of.h   |  7 +++++++
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cd3821a6444f..0e08283fd4fd 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -362,10 +362,49 @@ void of_node_release(struct kobject *kobj)
 	fwnode_links_purge(of_fwnode_handle(node));
 
 	kfree(node->full_name);
+	if (node->full_name != (const char *)(node + 1))
+		kfree(node->full_name);
+
 	kfree(node->data);
 	kfree(node);
 }
 
+/**
+ * of_node_alloc - Allocate a node dynamically.
+ * @name:	Node name
+ *
+ * Create a node by dynamically allocating the memory of both the
+ * node structure and the node name & contents. The node's
+ * flags have the OF_DYNAMIC & OF_DETACHED bit set so that we can
+ * differentiate between dynamically allocated nodes and not.
+ *
+ * Return: The newly allocated node or NULL on out of memory error.
+ */
+struct device_node *of_node_alloc(const char *name)
+{
+	struct device_node *node;
+	int name_len = 0;
+
+	if (name)
+		name_len = strlen(name) + 1;
+
+	node = kzalloc(sizeof(*node) + name_len, GFP_KERNEL);
+	if (!node)
+		return NULL;
+
+	if (name) {
+		node->full_name = (const char *)(node + 1);
+		strcpy((char *)node->full_name, name);
+	}
+
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+	of_node_init(node);
+
+	return node;
+}
+EXPORT_SYMBOL(of_node_alloc);
+
 /**
  * __of_prop_dup - Copy a property dynamically.
  * @prop:	Property to copy
@@ -426,18 +465,9 @@ struct device_node *__of_node_dup(const struct device_node *np,
 {
 	struct device_node *node;
 
-	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	node = of_node_alloc(full_name);
 	if (!node)
 		return NULL;
-	node->full_name = kstrdup(full_name, GFP_KERNEL);
-	if (!node->full_name) {
-		kfree(node);
-		return NULL;
-	}
-
-	of_node_set_flag(node, OF_DYNAMIC);
-	of_node_set_flag(node, OF_DETACHED);
-	of_node_init(node);
 
 	/* Iterate over and duplicate all properties */
 	if (np) {
diff --git a/include/linux/of.h b/include/linux/of.h
index 766d002bddb9..fc71e0355f05 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1462,6 +1462,8 @@ enum of_reconfig_change {
 };
 
 #ifdef CONFIG_OF_DYNAMIC
+struct device_node *of_node_alloc(const char *name);
+
 extern int of_reconfig_notifier_register(struct notifier_block *);
 extern int of_reconfig_notifier_unregister(struct notifier_block *);
 extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
@@ -1506,6 +1508,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
 #else /* CONFIG_OF_DYNAMIC */
+static inline struct device_node *of_node_alloc(const char *name)
+{
+	return NULL;
+}
+
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
 	return -EINVAL;
-- 
2.27.0

Re: [PATCH RFC 1/2] of: dynamic: add of_node_alloc()

[Frank Rowand]

On 8/29/22 16:43, Lizhi Hou wrote:
> From: Clément Léger <clement.leger@bootlin.com>
> 
> Add of_node_alloc() which allows to create nodes. The node full_name
> field is allocated as part of the node allocation and the kfree() for
> this field is checked at release time to allow users using their own
> allocated name.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/dynamic.c | 50 +++++++++++++++++++++++++++++++++++---------
>  include/linux/of.h   |  7 +++++++
>  2 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..0e08283fd4fd 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -362,10 +362,49 @@ void of_node_release(struct kobject *kobj)
>  	fwnode_links_purge(of_fwnode_handle(node));
>  
>  	kfree(node->full_name);
> +	if (node->full_name != (const char *)(node + 1))
> +		kfree(node->full_name);
> +

Why free node->full_name a second time?

>  	kfree(node->data);k>  	kfree(node);
>  }
>  
> +/**
> + * of_node_alloc - Allocate a node dynamically.
> + * @name:	Node name
> + *
> + * Create a node by dynamically allocating the memory of both the
> + * node structure and the node name & contents. The node's
> + * flags have the OF_DYNAMIC & OF_DETACHED bit set so that we can
> + * differentiate between dynamically allocated nodes and not.
> + *
> + * Return: The newly allocated node or NULL on out of memory error.
> + */
> +struct device_node *of_node_alloc(const char *name)
> +{

The body of this function is already implemented by __of_node_dup(NULL, ...)

> +	struct device_node *node;
> +	int name_len = 0;
> +
> +	if (name)
> +		name_len = strlen(name) + 1;
> +
> +	node = kzalloc(sizeof(*node) + name_len, GFP_KERNEL);
> +	if (!node)
> +		return NULL;
> +
> +	if (name) {
> +		node->full_name = (const char *)(node + 1);
> +		strcpy((char *)node->full_name, name);
> +	}
> +
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +	of_node_init(node);
> +
> +	return node;
> +}
> +EXPORT_SYMBOL(of_node_alloc);
> +
>  /**
>   * __of_prop_dup - Copy a property dynamically.
>   * @prop:	Property to copy
> @@ -426,18 +465,9 @@ struct device_node *__of_node_dup(const struct device_node *np,
>  {
>  	struct device_node *node;
>  
> -	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	node = of_node_alloc(full_name);
>  	if (!node)
>  		return NULL;
> -	node->full_name = kstrdup(full_name, GFP_KERNEL);
> -	if (!node->full_name) {
> -		kfree(node);
> -		return NULL;
> -	}
> -
> -	of_node_set_flag(node, OF_DYNAMIC);
> -	of_node_set_flag(node, OF_DETACHED);
> -	of_node_init(node);
>  
>  	/* Iterate over and duplicate all properties */
>  	if (np) {
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 766d002bddb9..fc71e0355f05 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1462,6 +1462,8 @@ enum of_reconfig_change {
>  };
>  
>  #ifdef CONFIG_OF_DYNAMIC
> +struct device_node *of_node_alloc(const char *name);
> +
>  extern int of_reconfig_notifier_register(struct notifier_block *);
>  extern int of_reconfig_notifier_unregister(struct notifier_block *);
>  extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
> @@ -1506,6 +1508,11 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>  }
>  #else /* CONFIG_OF_DYNAMIC */
> +static inline struct device_node *of_node_alloc(const char *name)
> +{
> +	return NULL;
> +}
> +
>  static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>  {
>  	return -EINVAL;

[PATCH RFC 2/2] pci: create device tree node for selected devices

[Lizhi Hou]

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.
And the process does not provide a way to discover the hardware peripherals
been mapped to PCI BARs. For Alveo PCI card, the card firmware provides a
flattened device tree to describe the hardware peripherals on its BARs.
And 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. It introduces a
kernel option. When the option is turned on, the kernel will generate
device tree nodes for PCI bridges unconditionally. It will also generate
a device tree node for Xilinx Alveo U50 by using PCI quirks. The generated
device tree nodes do not have any property. The future patches will add
necessary properties.

Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Signed-off-by: Sonal Santan <sonal.santan@amd.com>
Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Brian Xu <brian.xu@amd.com>
---
 drivers/pci/Kconfig         |  11 ++++
 drivers/pci/bus.c           |   2 +
 drivers/pci/msi/irqdomain.c |   6 +-
 drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-driver.c    |   3 +-
 drivers/pci/pci.h           |  16 ++++++
 drivers/pci/quirks.c        |  11 ++++
 drivers/pci/remove.c        |   1 +
 8 files changed, 153 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 55c028af4bd9..9eca5420330b 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -198,6 +198,17 @@ 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_OF
+	bool "Device tree node for PCI devices"
+	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/PCIE bridges.
+
 choice
 	prompt "PCI Express hierarchy optimization setting"
 	default PCIE_BUS_DEFAULT
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375f..f752b804ad1f 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 (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
+		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..19856d42e147 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 (of_node_check_flag(ppnode, OF_DYNAMIC))
+				ppnode = NULL;
 		}
 
 		/*
@@ -599,6 +601,110 @@ 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_OF)
+struct of_pci_node {
+	struct list_head node;
+	struct device_node *dt_node;
+	struct of_changeset cset;
+};
+
+static LIST_HEAD(of_pci_node_list);
+static DEFINE_MUTEX(of_pci_node_lock);
+
+static void of_pci_free_node(struct of_pci_node *node)
+{
+	of_changeset_destroy(&node->cset);
+	kfree(node->dt_node->full_name);
+	if (node->dt_node)
+		of_node_put(node->dt_node);
+	kfree(node);
+}
+
+void of_pci_remove_node(struct pci_dev *pdev)
+{
+	struct list_head *ele, *next;
+	struct of_pci_node *node;
+
+	mutex_lock(&of_pci_node_lock);
+
+	list_for_each_safe(ele, next, &of_pci_node_list) {
+		node = list_entry(ele, struct of_pci_node, node);
+		if (node->dt_node == pdev->dev.of_node) {
+			list_del(&node->node);
+			mutex_unlock(&of_pci_node_lock);
+			of_pci_free_node(node);
+			break;
+		}
+	}
+	mutex_unlock(&of_pci_node_lock);
+}
+
+void of_pci_make_dev_node(struct pci_dev *pdev)
+{
+	const char *pci_type = "dev";
+	struct device_node *parent;
+	struct of_pci_node *node;
+	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)
+		parent = pdev->bus->dev.of_node;
+	else
+		parent = pdev->bus->self->dev.of_node;
+	if (!parent)
+		return;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return;
+	of_changeset_init(&node->cset);
+
+	node->dt_node = of_node_alloc(NULL);
+	if (!node->dt_node) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+	node->dt_node->parent = parent;
+
+	if (pci_is_bridge(pdev))
+		pci_type = "pci";
+
+	node->dt_node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
+					     PCI_SLOT(pdev->devfn),
+					     PCI_FUNC(pdev->devfn));
+	if (!node->dt_node->full_name) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	ret = of_changeset_attach_node(&node->cset, node->dt_node);
+	if (ret)
+		goto failed;
+
+	ret = of_changeset_apply(&node->cset);
+	if (ret)
+		goto failed;
+
+	pdev->dev.of_node = node->dt_node;
+
+	mutex_lock(&of_pci_node_lock);
+	list_add(&node->node, &of_pci_node_list);
+	mutex_unlock(&of_pci_node_lock);
+
+	return;
+
+failed:
+	of_pci_free_node(node);
+}
+#endif
+
 #endif /* CONFIG_PCI */
 
 /**
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 49238ddd39ee..1540c4c9a770 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1628,7 +1628,8 @@ static int pci_dma_configure(struct device *dev)
 	bridge = pci_get_host_bridge_device(to_pci_dev(dev));
 
 	if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
-	    bridge->parent->of_node) {
+	    bridge->parent->of_node &&
+	    !of_node_check_flag(bridge->parent->of_node, OF_DYNAMIC)) {
 		ret = of_dma_configure(dev, bridge->parent->of_node, true);
 	} else if (has_acpi_companion(bridge)) {
 		struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 785f31086313..319b79920d40 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -678,6 +678,22 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
 
 #endif /* CONFIG_OF */
 
+#ifdef CONFIG_PCI_OF
+void of_pci_make_dev_node(struct pci_dev *pdev);
+void of_pci_remove_node(struct pci_dev *pdev);
+
+#else
+static inline void
+of_pci_make_dev_node(struct pci_dev *pdev)
+{
+}
+
+static inline void
+of_pci_remove_node(struct pci_dev *pdev);
+{
+}
+#endif /* CONFIG_OF_DYNAMIC */
+
 #ifdef CONFIG_PCIEAER
 void pci_no_aer(void);
 void pci_aer_init(struct pci_dev *dev);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 4944798e75b5..58a81e9ff0ed 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5956,3 +5956,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
 #endif
+
+/*
+ * For PCIe device which have multiple downstream devices, its driver may use
+ * a flattened device tree to describe the downstream devices.
+ * To overlay the flattened device tree, the PCI device and all its ancestor
+ * devices need to have device tree nodes on system base device tree. Thus,
+ * before driver probing, it might need to add a device tree node as the final
+ * fixup.
+ */
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 4c54c75050dc..0eaa9d9a3609 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -23,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
+		of_pci_remove_node(dev);
 
 		pci_dev_assign_added(dev, false);
 	}
-- 
2.27.0

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[kernel test robot]

Hi Lizhi,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on robh/for-next linus/master v6.0-rc3 next-20220829]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lizhi-Hou/Generate-device-tree-node-for-pci-devices/20220830-054614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: hexagon-randconfig-r041-20220829 (https://download.01.org/0day-ci/archive/20220830/202208301225.GaqbZVki-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4c106463674a6f6de085d9a91510c385be4a620d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f78f02388fd4e94a2ce74278c2af4c983b3ed98d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lizhi-Hou/Generate-device-tree-node-for-pci-devices/20220830-054614
        git checkout f78f02388fd4e94a2ce74278c2af4c983b3ed98d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/pci/of.c:16:
>> drivers/pci/pci.h:693:1: error: expected identifier or '('
   {
   ^
   1 error generated.


vim +693 drivers/pci/pci.h

   690	
   691	static inline void
   692	of_pci_remove_node(struct pci_dev *pdev);
 > 693	{
   694	}
   695	#endif /* CONFIG_OF_DYNAMIC */
   696	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[kernel test robot]

Hi Lizhi,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on robh/for-next linus/master v6.0-rc3 next-20220829]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lizhi-Hou/Generate-device-tree-node-for-pci-devices/20220830-054614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-a012-20220829 (https://download.01.org/0day-ci/archive/20220830/202208301237.ndR0szmn-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f78f02388fd4e94a2ce74278c2af4c983b3ed98d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lizhi-Hou/Generate-device-tree-node-for-pci-devices/20220830-054614
        git checkout f78f02388fd4e94a2ce74278c2af4c983b3ed98d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/pci/controller/cadence/pci-j721e.c:23:
>> drivers/pci/controller/cadence/../../pci.h:693:1: error: expected identifier or '('
   {
   ^
   1 error generated.


vim +693 drivers/pci/controller/cadence/../../pci.h

   690	
   691	static inline void
   692	of_pci_remove_node(struct pci_dev *pdev);
 > 693	{
   694	}
   695	#endif /* CONFIG_OF_DYNAMIC */
   696	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[Rob Herring]

On Mon, Aug 29, 2022 at 02:43:37PM -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.
> And the process does not provide a way to discover the hardware peripherals
> been mapped to PCI BARs.

This sentence doesn't make sense.

> For Alveo PCI card, the card firmware provides a
> flattened device tree to describe the hardware peripherals on its BARs.
> And 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. It introduces a
> kernel option. When the option is turned on, the kernel will generate
> device tree nodes for PCI bridges unconditionally. It will also generate
> a device tree node for Xilinx Alveo U50 by using PCI quirks. The generated
> device tree nodes do not have any property. The future patches will add
> necessary properties.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
> Signed-off-by: Max Zhen <max.zhen@amd.com>
> Signed-off-by: Brian Xu <brian.xu@amd.com>
> ---
>  drivers/pci/Kconfig         |  11 ++++
>  drivers/pci/bus.c           |   2 +
>  drivers/pci/msi/irqdomain.c |   6 +-
>  drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c    |   3 +-
>  drivers/pci/pci.h           |  16 ++++++
>  drivers/pci/quirks.c        |  11 ++++
>  drivers/pci/remove.c        |   1 +
>  8 files changed, 153 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 55c028af4bd9..9eca5420330b 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -198,6 +198,17 @@ 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_OF

We already have OF_PCI so this is confusing. Maybe 
'PCI_DYNAMIC_OF_NODES'.


> +	bool "Device tree node for PCI devices"
> +	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/PCIE bridges.
> +
>  choice
>  	prompt "PCI Express hierarchy optimization setting"
>  	default PCIE_BUS_DEFAULT
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 3cef835b375f..f752b804ad1f 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 (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)

Would pci_is_bridge() work here? That would include cardbus, but I think 
that won't matter in practice.

> +		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..19856d42e147 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 (of_node_check_flag(ppnode, OF_DYNAMIC))
> +				ppnode = NULL;
>  		}
>  
>  		/*
> @@ -599,6 +601,110 @@ 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_OF)
> +struct of_pci_node {
> +	struct list_head node;
> +	struct device_node *dt_node;
> +	struct of_changeset cset;
> +};
> +
> +static LIST_HEAD(of_pci_node_list);
> +static DEFINE_MUTEX(of_pci_node_lock);

There is a 'data' pointer in device_node which you could use to store 
the changeset. Then you wouldn't need a list. (though 'data' is rarely 
used and I hoped to get rid of it)

> +
> +static void of_pci_free_node(struct of_pci_node *node)
> +{
> +	of_changeset_destroy(&node->cset);
> +	kfree(node->dt_node->full_name);
> +	if (node->dt_node)
> +		of_node_put(node->dt_node);

You free full_name before freeing the node, so you could have a UAF. 
That needs to be taken care of when releasing the node.

> +	kfree(node);
> +}
> +
> +void of_pci_remove_node(struct pci_dev *pdev)
> +{
> +	struct list_head *ele, *next;
> +	struct of_pci_node *node;
> +
> +	mutex_lock(&of_pci_node_lock);
> +
> +	list_for_each_safe(ele, next, &of_pci_node_list) {
> +		node = list_entry(ele, struct of_pci_node, node);
> +		if (node->dt_node == pdev->dev.of_node) {
> +			list_del(&node->node);
> +			mutex_unlock(&of_pci_node_lock);
> +			of_pci_free_node(node);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&of_pci_node_lock);
> +}

The above bits aren't really particular to PCI, so they probably 
belong in the DT core code. Frank will probably have thoughts on what 
this should look like.

> +
> +void of_pci_make_dev_node(struct pci_dev *pdev)
> +{
> +	const char *pci_type = "dev";
> +	struct device_node *parent;
> +	struct of_pci_node *node;
> +	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)
> +		parent = pdev->bus->dev.of_node;
> +	else
> +		parent = pdev->bus->self->dev.of_node;
> +	if (!parent)
> +		return;
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return;
> +	of_changeset_init(&node->cset);
> +
> +	node->dt_node = of_node_alloc(NULL);
> +	if (!node->dt_node) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}
> +	node->dt_node->parent = parent;
> +
> +	if (pci_is_bridge(pdev))
> +		pci_type = "pci";
> +
> +	node->dt_node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
> +					     PCI_SLOT(pdev->devfn),
> +					     PCI_FUNC(pdev->devfn));
> +	if (!node->dt_node->full_name) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}

Wait, aren't you missing adding properties? You need 'reg', 
'compatbile', and 'device_type = "pci"' for bridges.

> +
> +	ret = of_changeset_attach_node(&node->cset, node->dt_node);
> +	if (ret)
> +		goto failed;
> +
> +	ret = of_changeset_apply(&node->cset);
> +	if (ret)
> +		goto failed;
> +
> +	pdev->dev.of_node = node->dt_node;
> +
> +	mutex_lock(&of_pci_node_lock);
> +	list_add(&node->node, &of_pci_node_list);
> +	mutex_unlock(&of_pci_node_lock);
> +
> +	return;
> +
> +failed:
> +	of_pci_free_node(node);
> +}

Pass in the parent node and node name, and this function is not PCI 
specific either.

> +#endif
> +
>  #endif /* CONFIG_PCI */
>  
>  /**
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 49238ddd39ee..1540c4c9a770 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1628,7 +1628,8 @@ static int pci_dma_configure(struct device *dev)
>  	bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>  
>  	if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
> -	    bridge->parent->of_node) {
> +	    bridge->parent->of_node &&
> +	    !of_node_check_flag(bridge->parent->of_node, OF_DYNAMIC)) {
>  		ret = of_dma_configure(dev, bridge->parent->of_node, true);
>  	} else if (has_acpi_companion(bridge)) {
>  		struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 785f31086313..319b79920d40 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -678,6 +678,22 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>  
>  #endif /* CONFIG_OF */
>  
> +#ifdef CONFIG_PCI_OF
> +void of_pci_make_dev_node(struct pci_dev *pdev);
> +void of_pci_remove_node(struct pci_dev *pdev);
> +
> +#else
> +static inline void
> +of_pci_make_dev_node(struct pci_dev *pdev)
> +{
> +}
> +
> +static inline void
> +of_pci_remove_node(struct pci_dev *pdev);
> +{
> +}
> +#endif /* CONFIG_OF_DYNAMIC */
> +
>  #ifdef CONFIG_PCIEAER
>  void pci_no_aer(void);
>  void pci_aer_init(struct pci_dev *dev);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 4944798e75b5..58a81e9ff0ed 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5956,3 +5956,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
>  #endif
> +
> +/*
> + * For PCIe device which have multiple downstream devices, its driver may use
> + * a flattened device tree to describe the downstream devices.
> + * To overlay the flattened device tree, the PCI device and all its ancestor
> + * devices need to have device tree nodes on system base device tree. Thus,
> + * before driver probing, it might need to add a device tree node as the final
> + * fixup.
> + */
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 4c54c75050dc..0eaa9d9a3609 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -23,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
>  		device_release_driver(&dev->dev);
>  		pci_proc_detach_device(dev);
>  		pci_remove_sysfs_dev_files(dev);
> +		of_pci_remove_node(dev);
>  
>  		pci_dev_assign_added(dev, false);
>  	}
> -- 
> 2.27.0
> 
> 

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[Frank Rowand]

On 9/2/22 13:54, Rob Herring wrote:
> On Mon, Aug 29, 2022 at 02:43:37PM -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.
>> And the process does not provide a way to discover the hardware peripherals
>> been mapped to PCI BARs.

< snip >

> 
> The above bits aren't really particular to PCI, so they probably 
> belong in the DT core code. Frank will probably have thoughts on what 
> this should look like.

< snip >

I will try to look through this patch series later today (Monday 9/12
USA time - I will not be in Dublin for the conferences this week.)

-Frank

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[Frank Rowand]

On 9/12/22 01:33, Frank Rowand wrote:
> On 9/2/22 13:54, Rob Herring wrote:
>> On Mon, Aug 29, 2022 at 02:43:37PM -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.
>>> And the process does not provide a way to discover the hardware peripherals
>>> been mapped to PCI BARs.
> 
> < snip >
> 
>>
>> The above bits aren't really particular to PCI, so they probably 
>> belong in the DT core code. Frank will probably have thoughts on what 
>> this should look like.
> 
> < snip >
> 
> I will try to look through this patch series later today (Monday 9/12
> USA time - I will not be in Dublin for the conferences this week.)
> 
> -Frank

I have collected nearly 500 emails on the history behind this patch and
also another set of patch series that has been trying to achieve some
somewhat similar functionality.  Expect me to take a while to wade through
all of this.

-Frank

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[Frank Rowand]

On 9/13/22 02:03, Frank Rowand wrote:
> On 9/12/22 01:33, Frank Rowand wrote:
>> On 9/2/22 13:54, Rob Herring wrote:
>>> On Mon, Aug 29, 2022 at 02:43:37PM -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.
>>>> And the process does not provide a way to discover the hardware peripherals
>>>> been mapped to PCI BARs.
>>
>> < snip >
>>
>>>
>>> The above bits aren't really particular to PCI, so they probably 
>>> belong in the DT core code. Frank will probably have thoughts on what 
>>> this should look like.
>>
>> < snip >
>>
>> I will try to look through this patch series later today (Monday 9/12
>> USA time - I will not be in Dublin for the conferences this week.)
>>
>> -Frank
> 
> I have collected nearly 500 emails on the history behind this patch and
> also another set of patch series that has been trying to achieve some
> somewhat similar functionality.  Expect me to take a while to wade through
> all of this.

I'm still working at understanding the full picture of patch 2/2.

-Frank

> 
> -Frank

Re: [PATCH RFC 2/2] pci: create device tree node for selected devices

[Lizhi Hou]

On 9/2/22 11:54, Rob Herring wrote:
> On Mon, Aug 29, 2022 at 02:43:37PM -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.
>> And the process does not provide a way to discover the hardware peripherals
>> been mapped to PCI BARs.
> This sentence doesn't make sense.

How about changing it to:

And it does not 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.
>> And 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. It introduces a
>> kernel option. When the option is turned on, the kernel will generate
>> device tree nodes for PCI bridges unconditionally. It will also generate
>> a device tree node for Xilinx Alveo U50 by using PCI quirks. The generated
>> device tree nodes do not have any property. The future patches will add
>> necessary properties.
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
>> Signed-off-by: Max Zhen <max.zhen@amd.com>
>> Signed-off-by: Brian Xu <brian.xu@amd.com>
>> ---
>>   drivers/pci/Kconfig         |  11 ++++
>>   drivers/pci/bus.c           |   2 +
>>   drivers/pci/msi/irqdomain.c |   6 +-
>>   drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci-driver.c    |   3 +-
>>   drivers/pci/pci.h           |  16 ++++++
>>   drivers/pci/quirks.c        |  11 ++++
>>   drivers/pci/remove.c        |   1 +
>>   8 files changed, 153 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index 55c028af4bd9..9eca5420330b 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -198,6 +198,17 @@ 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_OF
> We already have OF_PCI so this is confusing. Maybe
> 'PCI_DYNAMIC_OF_NODES'.
Sure. I will change it to PCI_DYNAMIC_OF_NODES
>
>
>> +	bool "Device tree node for PCI devices"
>> +	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/PCIE bridges.
>> +
>>   choice
>>   	prompt "PCI Express hierarchy optimization setting"
>>   	default PCIE_BUS_DEFAULT
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 3cef835b375f..f752b804ad1f 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 (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
> Would pci_is_bridge() work here? That would include cardbus, but I think
> that won't matter in practice.
ok. I will use pci_is_bridge() here.
>
>> +		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..19856d42e147 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 (of_node_check_flag(ppnode, OF_DYNAMIC))
>> +				ppnode = NULL;
>>   		}
>>   
>>   		/*
>> @@ -599,6 +601,110 @@ 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_OF)
>> +struct of_pci_node {
>> +	struct list_head node;
>> +	struct device_node *dt_node;
>> +	struct of_changeset cset;
>> +};
>> +
>> +static LIST_HEAD(of_pci_node_list);
>> +static DEFINE_MUTEX(of_pci_node_lock);
> There is a 'data' pointer in device_node which you could use to store
> the changeset. Then you wouldn't need a list. (though 'data' is rarely
> used and I hoped to get rid of it)

Ok. So if I understand correctly, in of_pci_removed_node(), it may check 
the flag and assume 'data' is pointing to cset if OF_DYNAMIC is set.

>> +
>> +static void of_pci_free_node(struct of_pci_node *node)
>> +{
>> +	of_changeset_destroy(&node->cset);
>> +	kfree(node->dt_node->full_name);
>> +	if (node->dt_node)
>> +		of_node_put(node->dt_node);
> You free full_name before freeing the node, so you could have a UAF.
> That needs to be taken care of when releasing the node.
Got it. I will fix this.
>
>> +	kfree(node);
>> +}
>> +
>> +void of_pci_remove_node(struct pci_dev *pdev)
>> +{
>> +	struct list_head *ele, *next;
>> +	struct of_pci_node *node;
>> +
>> +	mutex_lock(&of_pci_node_lock);
>> +
>> +	list_for_each_safe(ele, next, &of_pci_node_list) {
>> +		node = list_entry(ele, struct of_pci_node, node);
>> +		if (node->dt_node == pdev->dev.of_node) {
>> +			list_del(&node->node);
>> +			mutex_unlock(&of_pci_node_lock);
>> +			of_pci_free_node(node);
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&of_pci_node_lock);
>> +}
> The above bits aren't really particular to PCI, so they probably
> belong in the DT core code. Frank will probably have thoughts on what
> this should look like.
Sure. Looking forward Frank's comment.
>
>> +
>> +void of_pci_make_dev_node(struct pci_dev *pdev)
>> +{
>> +	const char *pci_type = "dev";
>> +	struct device_node *parent;
>> +	struct of_pci_node *node;
>> +	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)
>> +		parent = pdev->bus->dev.of_node;
>> +	else
>> +		parent = pdev->bus->self->dev.of_node;
>> +	if (!parent)
>> +		return;
>> +
>> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
>> +	if (!node)
>> +		return;
>> +	of_changeset_init(&node->cset);
>> +
>> +	node->dt_node = of_node_alloc(NULL);
>> +	if (!node->dt_node) {
>> +		ret = -ENOMEM;
>> +		goto failed;
>> +	}
>> +	node->dt_node->parent = parent;
>> +
>> +	if (pci_is_bridge(pdev))
>> +		pci_type = "pci";
>> +
>> +	node->dt_node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
>> +					     PCI_SLOT(pdev->devfn),
>> +					     PCI_FUNC(pdev->devfn));
>> +	if (!node->dt_node->full_name) {
>> +		ret = -ENOMEM;
>> +		goto failed;
>> +	}
> Wait, aren't you missing adding properties? You need 'reg',
> 'compatbile', and 'device_type = "pci"' for bridges.
In this patch series nobody consumes the dynamic generated node yet, 
Thus I did not add any property. I will add one or two patches to this 
series for the properties you listed.
>
>> +
>> +	ret = of_changeset_attach_node(&node->cset, node->dt_node);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	ret = of_changeset_apply(&node->cset);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	pdev->dev.of_node = node->dt_node;
>> +
>> +	mutex_lock(&of_pci_node_lock);
>> +	list_add(&node->node, &of_pci_node_list);
>> +	mutex_unlock(&of_pci_node_lock);
>> +
>> +	return;
>> +
>> +failed:
>> +	of_pci_free_node(node);
>> +}
> Pass in the parent node and node name, and this function is not PCI
> specific either.

Ok. How about introducing new functions of_changeset_create_node(), 
of_changeset_create_property_*()  to of/dynamic.c?

So the function could be like:

of_pci_make_dev_node ()

{

     node = of_changeset_create_node (cset, full_name, parent); //alloc  
of_node and add to cset

     of_changeset_create_property_string(cset, node, name, string);  
//alloc of_property and add to cset

     of_changeset_create_property_u32_array(cset, node, name, array, len);

     ....  add more properties;

     of_changeset_apply(cset)

}


Thanks,

Lizhi

>
>> +#endif
>> +
>>   #endif /* CONFIG_PCI */
>>   
>>   /**
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 49238ddd39ee..1540c4c9a770 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -1628,7 +1628,8 @@ static int pci_dma_configure(struct device *dev)
>>   	bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>>   
>>   	if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
>> -	    bridge->parent->of_node) {
>> +	    bridge->parent->of_node &&
>> +	    !of_node_check_flag(bridge->parent->of_node, OF_DYNAMIC)) {
>>   		ret = of_dma_configure(dev, bridge->parent->of_node, true);
>>   	} else if (has_acpi_companion(bridge)) {
>>   		struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 785f31086313..319b79920d40 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -678,6 +678,22 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>>   
>>   #endif /* CONFIG_OF */
>>   
>> +#ifdef CONFIG_PCI_OF
>> +void of_pci_make_dev_node(struct pci_dev *pdev);
>> +void of_pci_remove_node(struct pci_dev *pdev);
>> +
>> +#else
>> +static inline void
>> +of_pci_make_dev_node(struct pci_dev *pdev)
>> +{
>> +}
>> +
>> +static inline void
>> +of_pci_remove_node(struct pci_dev *pdev);
>> +{
>> +}
>> +#endif /* CONFIG_OF_DYNAMIC */
>> +
>>   #ifdef CONFIG_PCIEAER
>>   void pci_no_aer(void);
>>   void pci_aer_init(struct pci_dev *dev);
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 4944798e75b5..58a81e9ff0ed 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -5956,3 +5956,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency
>>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
>>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
>>   #endif
>> +
>> +/*
>> + * For PCIe device which have multiple downstream devices, its driver may use
>> + * a flattened device tree to describe the downstream devices.
>> + * To overlay the flattened device tree, the PCI device and all its ancestor
>> + * devices need to have device tree nodes on system base device tree. Thus,
>> + * before driver probing, it might need to add a device tree node as the final
>> + * fixup.
>> + */
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 4c54c75050dc..0eaa9d9a3609 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -23,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
>>   		device_release_driver(&dev->dev);
>>   		pci_proc_detach_device(dev);
>>   		pci_remove_sysfs_dev_files(dev);
>> +		of_pci_remove_node(dev);
>>   
>>   		pci_dev_assign_added(dev, false);
>>   	}
>> -- 
>> 2.27.0
>>
>>

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Bjorn Helgaas]

On Mon, Aug 29, 2022 at 02:43:35PM -0700, Lizhi Hou wrote:

> Clément Léger (1):
>   of: dynamic: add of_node_alloc()
> 
> Lizhi Hou (1):
>   pci: create device tree node for selected devices

Please capitalize both subject lines to match previous history of the
files involved.

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Lizhi Hou]

Hi Bjorn,


On 9/2/22 13:43, Bjorn Helgaas wrote:
> On Mon, Aug 29, 2022 at 02:43:35PM -0700, Lizhi Hou wrote:
>
>> Clément Léger (1):
>>    of: dynamic: add of_node_alloc()
>>
>> Lizhi Hou (1):
>>    pci: create device tree node for selected devices
> Please capitalize both subject lines to match previous history of the
> files involved.

Sure. Does this subject format look ok?

of: dynamic: Add of_node_alloc()

PCI: Create device tree node for selected devices


Thanks,

Lizhi

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 8/29/22 16:43, Lizhi Hou wrote:
> This patch series introduces OF overlay support for PCI devices which
> primarily addresses two use cases. First, it provides a data driven method
> to describe hardware peripherals that are present in a PCI endpoint and
> hence can be accessed by the PCI host. An example device is Xilinx/AMD
> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
> driver -- often used in SoC platforms -- in a PCI host based system. An
> example device is Microchip LAN9662 Ethernet Controller.
> 
> This patch series consolidates previous efforts to define such an
> infrastructure:
> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> 
> Normally, the PCI core discovers PCI devices and their BARs using the
> PCI enumeration process. However, the process does not provide a way to
> discover the hardware peripherals that are present in a PCI device, and
> which can be accessed through the PCI BARs. Also, the enumeration process
> does not provide a way to associate MSI-X vectors of a PCI device with the
> hardware peripherals that are present in the device. PCI device drivers
> often use header files to describe the hardware peripherals and their
> resources as there is no standard data driven way to do so. This patch
> series proposes to use flattened device tree blob to describe the
> peripherals in a data driven way. Based on previous discussion, using
> device tree overlay is the best way to unflatten the blob and populate
> platform devices. To use device tree overlay, there are three obvious
> problems that need to be resolved.
> 
> First, we need to create a base tree for non-DT system such as x86_64. A
> patch series has been submitted for this:
> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
> 
> Second, a device tree node corresponding to the PCI endpoint is required
> for overlaying the flattened device tree blob for that PCI endpoint.
> Because PCI is a self-discoverable bus, a device tree node is usually not
> created for PCI devices. This series adds support to generate a device
> tree node for a PCI device which advertises itself using PCI quirks
> infrastructure.
> 
> Third, we need to generate device tree nodes for PCI bridges since a child
> PCI endpoint may choose to have a device tree node created.
> 
> This patch series is made up of two patches.
> 
> The first patch is adding OF interface to allocate an OF node. It is copied
> from:
> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
> 
> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
> is turned on, the kernel will generate device tree nodes for all PCI
> bridges unconditionally. The patch also shows how to use the PCI quirks
> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
> a device. Specifically, the patch generates a device tree node for Xilinx
> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
> have any property. Future patches will add the necessary properties.
> 
> Clément Léger (1):
>   of: dynamic: add of_node_alloc()
> 
> Lizhi Hou (1):
>   pci: create device tree node for selected devices
> 
>  drivers/of/dynamic.c        |  50 +++++++++++++----
>  drivers/pci/Kconfig         |  11 ++++
>  drivers/pci/bus.c           |   2 +
>  drivers/pci/msi/irqdomain.c |   6 +-
>  drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c    |   3 +-
>  drivers/pci/pci.h           |  16 ++++++
>  drivers/pci/quirks.c        |  11 ++++
>  drivers/pci/remove.c        |   1 +
>  include/linux/of.h          |   7 +++
>  10 files changed, 200 insertions(+), 13 deletions(-)
> 

The patch description leaves out the most important piece of information.

The device located at the PCI endpoint is implemented via FPGA
   - which is programmed after Linux boots (or somewhere late in the boot process)
      - (A) and thus can not be described by static data available pre-boot because
            it is dynamic (and the FPGA program will often change while the Linux
            kernel is already booted
      - (B) can be described by static data available pre-boot because the FPGA
            program will always be the same for this device on this system

I am not positive what part of what I wrote above is correct and would appreciate
some confirmation of what is correct or incorrect.

-Frank

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Lizhi Hou]

On 9/13/22 00:00, Frank Rowand wrote:
> On 8/29/22 16:43, Lizhi Hou wrote:
>> This patch series introduces OF overlay support for PCI devices which
>> primarily addresses two use cases. First, it provides a data driven method
>> to describe hardware peripherals that are present in a PCI endpoint and
>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>> driver -- often used in SoC platforms -- in a PCI host based system. An
>> example device is Microchip LAN9662 Ethernet Controller.
>>
>> This patch series consolidates previous efforts to define such an
>> infrastructure:
>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>
>> Normally, the PCI core discovers PCI devices and their BARs using the
>> PCI enumeration process. However, the process does not provide a way to
>> discover the hardware peripherals that are present in a PCI device, and
>> which can be accessed through the PCI BARs. Also, the enumeration process
>> does not provide a way to associate MSI-X vectors of a PCI device with the
>> hardware peripherals that are present in the device. PCI device drivers
>> often use header files to describe the hardware peripherals and their
>> resources as there is no standard data driven way to do so. This patch
>> series proposes to use flattened device tree blob to describe the
>> peripherals in a data driven way. Based on previous discussion, using
>> device tree overlay is the best way to unflatten the blob and populate
>> platform devices. To use device tree overlay, there are three obvious
>> problems that need to be resolved.
>>
>> First, we need to create a base tree for non-DT system such as x86_64. A
>> patch series has been submitted for this:
>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>
>> Second, a device tree node corresponding to the PCI endpoint is required
>> for overlaying the flattened device tree blob for that PCI endpoint.
>> Because PCI is a self-discoverable bus, a device tree node is usually not
>> created for PCI devices. This series adds support to generate a device
>> tree node for a PCI device which advertises itself using PCI quirks
>> infrastructure.
>>
>> Third, we need to generate device tree nodes for PCI bridges since a child
>> PCI endpoint may choose to have a device tree node created.
>>
>> This patch series is made up of two patches.
>>
>> The first patch is adding OF interface to allocate an OF node. It is copied
>> from:
>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>
>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>> is turned on, the kernel will generate device tree nodes for all PCI
>> bridges unconditionally. The patch also shows how to use the PCI quirks
>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>> a device. Specifically, the patch generates a device tree node for Xilinx
>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>> have any property. Future patches will add the necessary properties.
>>
>> Clément Léger (1):
>>    of: dynamic: add of_node_alloc()
>>
>> Lizhi Hou (1):
>>    pci: create device tree node for selected devices
>>
>>   drivers/of/dynamic.c        |  50 +++++++++++++----
>>   drivers/pci/Kconfig         |  11 ++++
>>   drivers/pci/bus.c           |   2 +
>>   drivers/pci/msi/irqdomain.c |   6 +-
>>   drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci-driver.c    |   3 +-
>>   drivers/pci/pci.h           |  16 ++++++
>>   drivers/pci/quirks.c        |  11 ++++
>>   drivers/pci/remove.c        |   1 +
>>   include/linux/of.h          |   7 +++
>>   10 files changed, 200 insertions(+), 13 deletions(-)
>>
> The patch description leaves out the most important piece of information.
>
> The device located at the PCI endpoint is implemented via FPGA
>     - which is programmed after Linux boots (or somewhere late in the boot process)
>        - (A) and thus can not be described by static data available pre-boot because
>              it is dynamic (and the FPGA program will often change while the Linux
>              kernel is already booted
>        - (B) can be described by static data available pre-boot because the FPGA
>              program will always be the same for this device on this system
>
> I am not positive what part of what I wrote above is correct and would appreciate
> some confirmation of what is correct or incorrect.

There are 2 series devices rely on this patch:

     1) Xilinx Alveo Accelerator cards (FPGA based device)

     2) lan9662 PCIe card

           please see: 
https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/

For Xilinx Alveo device, it is (A). The FPGA partitions can be 
programmed dynamically after boot.


Thanks,

Lzhi

>
> -Frank

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 9/13/22 12:10, Lizhi Hou wrote:
> 
> On 9/13/22 00:00, Frank Rowand wrote:
>> On 8/29/22 16:43, Lizhi Hou wrote:
>>> This patch series introduces OF overlay support for PCI devices which
>>> primarily addresses two use cases. First, it provides a data driven method
>>> to describe hardware peripherals that are present in a PCI endpoint and
>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>> example device is Microchip LAN9662 Ethernet Controller.
>>>
>>> This patch series consolidates previous efforts to define such an
>>> infrastructure:
>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>
>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>> PCI enumeration process. However, the process does not provide a way to
>>> discover the hardware peripherals that are present in a PCI device, and
>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>> hardware peripherals that are present in the device. PCI device drivers
>>> often use header files to describe the hardware peripherals and their
>>> resources as there is no standard data driven way to do so. This patch
>>> series proposes to use flattened device tree blob to describe the
>>> peripherals in a data driven way. Based on previous discussion, using
>>> device tree overlay is the best way to unflatten the blob and populate
>>> platform devices. To use device tree overlay, there are three obvious
>>> problems that need to be resolved.
>>>
>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>> patch series has been submitted for this:
>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>
>>> Second, a device tree node corresponding to the PCI endpoint is required
>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>> created for PCI devices. This series adds support to generate a device
>>> tree node for a PCI device which advertises itself using PCI quirks
>>> infrastructure.
>>>
>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>> PCI endpoint may choose to have a device tree node created.
>>>
>>> This patch series is made up of two patches.
>>>
>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>> from:
>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>
>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>> is turned on, the kernel will generate device tree nodes for all PCI
>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>> have any property. Future patches will add the necessary properties.
>>>
>>> Clément Léger (1):
>>>    of: dynamic: add of_node_alloc()
>>>
>>> Lizhi Hou (1):
>>>    pci: create device tree node for selected devices
>>>
>>>   drivers/of/dynamic.c        |  50 +++++++++++++----
>>>   drivers/pci/Kconfig         |  11 ++++
>>>   drivers/pci/bus.c           |   2 +
>>>   drivers/pci/msi/irqdomain.c |   6 +-
>>>   drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>   drivers/pci/pci-driver.c    |   3 +-
>>>   drivers/pci/pci.h           |  16 ++++++
>>>   drivers/pci/quirks.c        |  11 ++++
>>>   drivers/pci/remove.c        |   1 +
>>>   include/linux/of.h          |   7 +++
>>>   10 files changed, 200 insertions(+), 13 deletions(-)
>>>
>> The patch description leaves out the most important piece of information.
>>
>> The device located at the PCI endpoint is implemented via FPGA
>>     - which is programmed after Linux boots (or somewhere late in the boot process)
>>        - (A) and thus can not be described by static data available pre-boot because
>>              it is dynamic (and the FPGA program will often change while the Linux
>>              kernel is already booted
>>        - (B) can be described by static data available pre-boot because the FPGA
>>              program will always be the same for this device on this system
>>
>> I am not positive what part of what I wrote above is correct and would appreciate
>> some confirmation of what is correct or incorrect.
> 
> There are 2 series devices rely on this patch:
> 
>     1) Xilinx Alveo Accelerator cards (FPGA based device)
> 
>     2) lan9662 PCIe card
> 
>           please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/

Thanks.  Please include this information in future versions of the patch series.

For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
device tree.  I realize that this suggestion is only a partial solution if one wants to
use hotplug to change system configuration (as opposed to using hotplug only to replace
an existing device (eg a broken device) with another instance of the same device).  I
also realize that this increased the system administration overhead.  On the other hand
an overlay based solution is likely to be fragile and possibly flaky.

> 
> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.

I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
available.  So the answer to my next question may vary by type of card.

Is it expected that the fpga program on a given card will change frequently (eg multiple
times per day), where the changed program results in a new device that would require a
different hardware description in the device tree?

Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
annually), in the same way as device firmware and operating systems are updated on a regular
basis for bug fixes and new functionality?


> 
> 
> Thanks,
> 
> Lzhi
> 
>>
>> -Frank

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Lizhi Hou]

On 9/13/22 10:41, Frank Rowand wrote:
> On 9/13/22 12:10, Lizhi Hou wrote:
>> On 9/13/22 00:00, Frank Rowand wrote:
>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>> This patch series introduces OF overlay support for PCI devices which
>>>> primarily addresses two use cases. First, it provides a data driven method
>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>
>>>> This patch series consolidates previous efforts to define such an
>>>> infrastructure:
>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>
>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>> PCI enumeration process. However, the process does not provide a way to
>>>> discover the hardware peripherals that are present in a PCI device, and
>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>> hardware peripherals that are present in the device. PCI device drivers
>>>> often use header files to describe the hardware peripherals and their
>>>> resources as there is no standard data driven way to do so. This patch
>>>> series proposes to use flattened device tree blob to describe the
>>>> peripherals in a data driven way. Based on previous discussion, using
>>>> device tree overlay is the best way to unflatten the blob and populate
>>>> platform devices. To use device tree overlay, there are three obvious
>>>> problems that need to be resolved.
>>>>
>>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>>> patch series has been submitted for this:
>>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>>
>>>> Second, a device tree node corresponding to the PCI endpoint is required
>>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>>> created for PCI devices. This series adds support to generate a device
>>>> tree node for a PCI device which advertises itself using PCI quirks
>>>> infrastructure.
>>>>
>>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>>> PCI endpoint may choose to have a device tree node created.
>>>>
>>>> This patch series is made up of two patches.
>>>>
>>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>>> from:
>>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>>
>>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>>> is turned on, the kernel will generate device tree nodes for all PCI
>>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>>> have any property. Future patches will add the necessary properties.
>>>>
>>>> Clément Léger (1):
>>>>     of: dynamic: add of_node_alloc()
>>>>
>>>> Lizhi Hou (1):
>>>>     pci: create device tree node for selected devices
>>>>
>>>>    drivers/of/dynamic.c        |  50 +++++++++++++----
>>>>    drivers/pci/Kconfig         |  11 ++++
>>>>    drivers/pci/bus.c           |   2 +
>>>>    drivers/pci/msi/irqdomain.c |   6 +-
>>>>    drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>>    drivers/pci/pci-driver.c    |   3 +-
>>>>    drivers/pci/pci.h           |  16 ++++++
>>>>    drivers/pci/quirks.c        |  11 ++++
>>>>    drivers/pci/remove.c        |   1 +
>>>>    include/linux/of.h          |   7 +++
>>>>    10 files changed, 200 insertions(+), 13 deletions(-)
>>>>
>>> The patch description leaves out the most important piece of information.
>>>
>>> The device located at the PCI endpoint is implemented via FPGA
>>>      - which is programmed after Linux boots (or somewhere late in the boot process)
>>>         - (A) and thus can not be described by static data available pre-boot because
>>>               it is dynamic (and the FPGA program will often change while the Linux
>>>               kernel is already booted
>>>         - (B) can be described by static data available pre-boot because the FPGA
>>>               program will always be the same for this device on this system
>>>
>>> I am not positive what part of what I wrote above is correct and would appreciate
>>> some confirmation of what is correct or incorrect.
>> There are 2 series devices rely on this patch:
>>
>>      1) Xilinx Alveo Accelerator cards (FPGA based device)
>>
>>      2) lan9662 PCIe card
>>
>>            please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> Thanks.  Please include this information in future versions of the patch series.
>
> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
> device tree.  I realize that this suggestion is only a partial solution if one wants to
> use hotplug to change system configuration (as opposed to using hotplug only to replace
> an existing device (eg a broken device) with another instance of the same device).  I
> also realize that this increased the system administration overhead.  On the other hand
> an overlay based solution is likely to be fragile and possibly flaky.
Can you clarify the pre-boot apply approach? How will it work for PCI 
devices?
>
>> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.
> I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
> available.  So the answer to my next question may vary by type of card.
>
> Is it expected that the fpga program on a given card will change frequently (eg multiple
> times per day), where the changed program results in a new device that would require a
> different hardware description in the device tree?

Different images may be loaded to a FPGA partition several times a day. 
The PCI topology (Device IDs, BARs, MSIx, etc) does not change. New IPs 
may appear (and old IPs may disappear) on the BARs when a new image is 
loaded. We would like to use flattened device tree to describe the IPs 
on the BARs.


Thanks,

Lizhi

>
> Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
> annually), in the same way as device firmware and operating systems are updated on a regular
> basis for bug fixes and new functionality?
>
>
>>
>> Thanks,
>>
>> Lzhi
>>
>>> -Frank

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 9/13/22 16:02, Lizhi Hou wrote:
> 
> On 9/13/22 10:41, Frank Rowand wrote:
>> On 9/13/22 12:10, Lizhi Hou wrote:
>>> On 9/13/22 00:00, Frank Rowand wrote:
>>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>>> This patch series introduces OF overlay support for PCI devices which
>>>>> primarily addresses two use cases. First, it provides a data driven method
>>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>>
>>>>> This patch series consolidates previous efforts to define such an
>>>>> infrastructure:
>>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>
>>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>>> PCI enumeration process. However, the process does not provide a way to
>>>>> discover the hardware peripherals that are present in a PCI device, and
>>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>>> hardware peripherals that are present in the device. PCI device drivers
>>>>> often use header files to describe the hardware peripherals and their
>>>>> resources as there is no standard data driven way to do so. This patch
>>>>> series proposes to use flattened device tree blob to describe the
>>>>> peripherals in a data driven way. Based on previous discussion, using
>>>>> device tree overlay is the best way to unflatten the blob and populate
>>>>> platform devices. To use device tree overlay, there are three obvious
>>>>> problems that need to be resolved.
>>>>>
>>>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>>>> patch series has been submitted for this:
>>>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>>>
>>>>> Second, a device tree node corresponding to the PCI endpoint is required
>>>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>>>> created for PCI devices. This series adds support to generate a device
>>>>> tree node for a PCI device which advertises itself using PCI quirks
>>>>> infrastructure.
>>>>>
>>>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>>>> PCI endpoint may choose to have a device tree node created.
>>>>>
>>>>> This patch series is made up of two patches.
>>>>>
>>>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>>>> from:
>>>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>>>
>>>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>>>> is turned on, the kernel will generate device tree nodes for all PCI
>>>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>>>> have any property. Future patches will add the necessary properties.
>>>>>
>>>>> Clément Léger (1):
>>>>>     of: dynamic: add of_node_alloc()
>>>>>
>>>>> Lizhi Hou (1):
>>>>>     pci: create device tree node for selected devices
>>>>>
>>>>>    drivers/of/dynamic.c        |  50 +++++++++++++----
>>>>>    drivers/pci/Kconfig         |  11 ++++
>>>>>    drivers/pci/bus.c           |   2 +
>>>>>    drivers/pci/msi/irqdomain.c |   6 +-
>>>>>    drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>>>    drivers/pci/pci-driver.c    |   3 +-
>>>>>    drivers/pci/pci.h           |  16 ++++++
>>>>>    drivers/pci/quirks.c        |  11 ++++
>>>>>    drivers/pci/remove.c        |   1 +
>>>>>    include/linux/of.h          |   7 +++
>>>>>    10 files changed, 200 insertions(+), 13 deletions(-)
>>>>>
>>>> The patch description leaves out the most important piece of information.
>>>>
>>>> The device located at the PCI endpoint is implemented via FPGA
>>>>      - which is programmed after Linux boots (or somewhere late in the boot process)
>>>>         - (A) and thus can not be described by static data available pre-boot because
>>>>               it is dynamic (and the FPGA program will often change while the Linux
>>>>               kernel is already booted
>>>>         - (B) can be described by static data available pre-boot because the FPGA
>>>>               program will always be the same for this device on this system
>>>>
>>>> I am not positive what part of what I wrote above is correct and would appreciate
>>>> some confirmation of what is correct or incorrect.
>>> There are 2 series devices rely on this patch:
>>>
>>>      1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>
>>>      2) lan9662 PCIe card
>>>
>>>            please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>> Thanks.  Please include this information in future versions of the patch series.
>>
>> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
>> device tree.  I realize that this suggestion is only a partial solution if one wants to
>> use hotplug to change system configuration (as opposed to using hotplug only to replace
>> an existing device (eg a broken device) with another instance of the same device).  I
>> also realize that this increased the system administration overhead.  On the other hand
>> an overlay based solution is likely to be fragile and possibly flaky.
> Can you clarify the pre-boot apply approach? How will it work for PCI devices?
>>
>>> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.
>> I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
>> available.  So the answer to my next question may vary by type of card.
>>
>> Is it expected that the fpga program on a given card will change frequently (eg multiple
>> times per day), where the changed program results in a new device that would require a
>> different hardware description in the device tree?
> 
> Different images may be loaded to a FPGA partition several times a
> day. The PCI topology (Device IDs, BARs, MSIx, etc) does not change.
> New IPs may appear (and old IPs may disappear) on the BARs when a new
> image is loaded. We would like to use flattened device tree to
> describe the IPs on the BARs.

That was kind of a non-answer.  I know that images _may_ change at
some frequency.  I was trying to get a sense of whether the images
were _likely_ to be changing on a frequent basis for these types
of boards, or whether frequent image changes are likely to be a
rare edge use case.

If there is a good design for the 99.999% use case that does not
support the 0.001% use case then it may be better to not create
an inferior design that also supports the 0.001% use case.

I hope that gives a better idea of the reason why I was asking the
question and how the answer could impact design and implementation
decisions.

As a point of reference, some other fpga users have indicated a
desire to change images many times per second.  The current driver
and overlay architecture did not seem to me to be a good match to
that use case (depending on the definition of "many").

-Frank

> 
> Thanks,
> 
> Lizhi
> 
>>
>> Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
>> annually), in the same way as device firmware and operating systems are updated on a regular
>> basis for bug fixes and new functionality?
>>
>>
>>>
>>> Thanks,
>>>
>>> Lzhi
>>>
>>>> -Frank

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Tom Rix]

Frank,

On 9/16/22 7:23 PM, Frank Rowand wrote:
> On 9/13/22 16:02, Lizhi Hou wrote:
>> On 9/13/22 10:41, Frank Rowand wrote:
>>> On 9/13/22 12:10, Lizhi Hou wrote:
>>>> On 9/13/22 00:00, Frank Rowand wrote:
>>>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>>>> This patch series introduces OF overlay support for PCI devices which
>>>>>> primarily addresses two use cases. First, it provides a data driven method
>>>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>>>
>>>>>> This patch series consolidates previous efforts to define such an
>>>>>> infrastructure:
>>>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>>
>>>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>>>> PCI enumeration process. However, the process does not provide a way to
>>>>>> discover the hardware peripherals that are present in a PCI device, and
>>>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>>>> hardware peripherals that are present in the device. PCI device drivers
>>>>>> often use header files to describe the hardware peripherals and their
>>>>>> resources as there is no standard data driven way to do so. This patch
>>>>>> series proposes to use flattened device tree blob to describe the
>>>>>> peripherals in a data driven way. Based on previous discussion, using
>>>>>> device tree overlay is the best way to unflatten the blob and populate
>>>>>> platform devices. To use device tree overlay, there are three obvious
>>>>>> problems that need to be resolved.
>>>>>>
>>>>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>>>>> patch series has been submitted for this:
>>>>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>>>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>>>>
>>>>>> Second, a device tree node corresponding to the PCI endpoint is required
>>>>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>>>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>>>>> created for PCI devices. This series adds support to generate a device
>>>>>> tree node for a PCI device which advertises itself using PCI quirks
>>>>>> infrastructure.
>>>>>>
>>>>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>>>>> PCI endpoint may choose to have a device tree node created.
>>>>>>
>>>>>> This patch series is made up of two patches.
>>>>>>
>>>>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>>>>> from:
>>>>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>>>>
>>>>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>>>>> is turned on, the kernel will generate device tree nodes for all PCI
>>>>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>>>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>>>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>>>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>>>>> have any property. Future patches will add the necessary properties.
>>>>>>
>>>>>> Clément Léger (1):
>>>>>>      of: dynamic: add of_node_alloc()
>>>>>>
>>>>>> Lizhi Hou (1):
>>>>>>      pci: create device tree node for selected devices
>>>>>>
>>>>>>     drivers/of/dynamic.c        |  50 +++++++++++++----
>>>>>>     drivers/pci/Kconfig         |  11 ++++
>>>>>>     drivers/pci/bus.c           |   2 +
>>>>>>     drivers/pci/msi/irqdomain.c |   6 +-
>>>>>>     drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>>>>     drivers/pci/pci-driver.c    |   3 +-
>>>>>>     drivers/pci/pci.h           |  16 ++++++
>>>>>>     drivers/pci/quirks.c        |  11 ++++
>>>>>>     drivers/pci/remove.c        |   1 +
>>>>>>     include/linux/of.h          |   7 +++
>>>>>>     10 files changed, 200 insertions(+), 13 deletions(-)
>>>>>>
>>>>> The patch description leaves out the most important piece of information.
>>>>>
>>>>> The device located at the PCI endpoint is implemented via FPGA
>>>>>       - which is programmed after Linux boots (or somewhere late in the boot process)
>>>>>          - (A) and thus can not be described by static data available pre-boot because
>>>>>                it is dynamic (and the FPGA program will often change while the Linux
>>>>>                kernel is already booted
>>>>>          - (B) can be described by static data available pre-boot because the FPGA
>>>>>                program will always be the same for this device on this system
>>>>>
>>>>> I am not positive what part of what I wrote above is correct and would appreciate
>>>>> some confirmation of what is correct or incorrect.
>>>> There are 2 series devices rely on this patch:
>>>>
>>>>       1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>>
>>>>       2) lan9662 PCIe card
>>>>
>>>>             please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>> Thanks.  Please include this information in future versions of the patch series.
>>>
>>> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
>>> device tree.  I realize that this suggestion is only a partial solution if one wants to
>>> use hotplug to change system configuration (as opposed to using hotplug only to replace
>>> an existing device (eg a broken device) with another instance of the same device).  I
>>> also realize that this increased the system administration overhead.  On the other hand
>>> an overlay based solution is likely to be fragile and possibly flaky.
>> Can you clarify the pre-boot apply approach? How will it work for PCI devices?
>>>> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.
>>> I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
>>> available.  So the answer to my next question may vary by type of card.
>>>
>>> Is it expected that the fpga program on a given card will change frequently (eg multiple
>>> times per day), where the changed program results in a new device that would require a
>>> different hardware description in the device tree?
>> Different images may be loaded to a FPGA partition several times a
>> day. The PCI topology (Device IDs, BARs, MSIx, etc) does not change.
>> New IPs may appear (and old IPs may disappear) on the BARs when a new
>> image is loaded. We would like to use flattened device tree to
>> describe the IPs on the BARs.
> That was kind of a non-answer.  I know that images _may_ change at
> some frequency.  I was trying to get a sense of whether the images
> were _likely_ to be changing on a frequent basis for these types
> of boards, or whether frequent image changes are likely to be a
> rare edge use case.
>
> If there is a good design for the 99.999% use case that does not
> support the 0.001% use case then it may be better to not create
> an inferior design that also supports the 0.001% use case.
>
> I hope that gives a better idea of the reason why I was asking the
> question and how the answer could impact design and implementation
> decisions.
>
> As a point of reference, some other fpga users have indicated a
> desire to change images many times per second.  The current driver
> and overlay architecture did not seem to me to be a good match to
> that use case (depending on the definition of "many").

I would rather we cover 99.999% now.

My understanding is that the subdevices are flexible but fairly static 
and the frequency Lizhi mentions would cover development uses.

In production I would expect the image to change about once a year with 
the same order of magnitude as firmware.

Can you point me to a reference of a user case with high frequency 
images changing that also depends on pci io device changing?

Tom

> -Frank
>
>> Thanks,
>>
>> Lizhi
>>
>>> Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
>>> annually), in the same way as device firmware and operating systems are updated on a regular
>>> basis for bug fixes and new functionality?
>>>
>>>
>>>> Thanks,
>>>>
>>>> Lzhi
>>>>
>>>>> -Frank

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 9/17/22 13:36, Tom Rix wrote:
> Frank,
> 
> On 9/16/22 7:23 PM, Frank Rowand wrote:
>> On 9/13/22 16:02, Lizhi Hou wrote:
>>> On 9/13/22 10:41, Frank Rowand wrote:
>>>> On 9/13/22 12:10, Lizhi Hou wrote:
>>>>> On 9/13/22 00:00, Frank Rowand wrote:
>>>>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>>>>> This patch series introduces OF overlay support for PCI devices which
>>>>>>> primarily addresses two use cases. First, it provides a data driven method
>>>>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>>>>
>>>>>>> This patch series consolidates previous efforts to define such an
>>>>>>> infrastructure:
>>>>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>>>
>>>>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>>>>> PCI enumeration process. However, the process does not provide a way to
>>>>>>> discover the hardware peripherals that are present in a PCI device, and
>>>>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>>>>> hardware peripherals that are present in the device. PCI device drivers
>>>>>>> often use header files to describe the hardware peripherals and their
>>>>>>> resources as there is no standard data driven way to do so. This patch
>>>>>>> series proposes to use flattened device tree blob to describe the
>>>>>>> peripherals in a data driven way. Based on previous discussion, using
>>>>>>> device tree overlay is the best way to unflatten the blob and populate
>>>>>>> platform devices. To use device tree overlay, there are three obvious
>>>>>>> problems that need to be resolved.
>>>>>>>
>>>>>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>>>>>> patch series has been submitted for this:
>>>>>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>>>>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>>>>>
>>>>>>> Second, a device tree node corresponding to the PCI endpoint is required
>>>>>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>>>>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>>>>>> created for PCI devices. This series adds support to generate a device
>>>>>>> tree node for a PCI device which advertises itself using PCI quirks
>>>>>>> infrastructure.
>>>>>>>
>>>>>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>>>>>> PCI endpoint may choose to have a device tree node created.
>>>>>>>
>>>>>>> This patch series is made up of two patches.
>>>>>>>
>>>>>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>>>>>> from:
>>>>>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>>>>>
>>>>>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>>>>>> is turned on, the kernel will generate device tree nodes for all PCI
>>>>>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>>>>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>>>>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>>>>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>>>>>> have any property. Future patches will add the necessary properties.
>>>>>>>
>>>>>>> Clément Léger (1):
>>>>>>>      of: dynamic: add of_node_alloc()
>>>>>>>
>>>>>>> Lizhi Hou (1):
>>>>>>>      pci: create device tree node for selected devices
>>>>>>>
>>>>>>>     drivers/of/dynamic.c        |  50 +++++++++++++----
>>>>>>>     drivers/pci/Kconfig         |  11 ++++
>>>>>>>     drivers/pci/bus.c           |   2 +
>>>>>>>     drivers/pci/msi/irqdomain.c |   6 +-
>>>>>>>     drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>>>>>     drivers/pci/pci-driver.c    |   3 +-
>>>>>>>     drivers/pci/pci.h           |  16 ++++++
>>>>>>>     drivers/pci/quirks.c        |  11 ++++
>>>>>>>     drivers/pci/remove.c        |   1 +
>>>>>>>     include/linux/of.h          |   7 +++
>>>>>>>     10 files changed, 200 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>> The patch description leaves out the most important piece of information.
>>>>>>
>>>>>> The device located at the PCI endpoint is implemented via FPGA
>>>>>>       - which is programmed after Linux boots (or somewhere late in the boot process)
>>>>>>          - (A) and thus can not be described by static data available pre-boot because
>>>>>>                it is dynamic (and the FPGA program will often change while the Linux
>>>>>>                kernel is already booted
>>>>>>          - (B) can be described by static data available pre-boot because the FPGA
>>>>>>                program will always be the same for this device on this system
>>>>>>
>>>>>> I am not positive what part of what I wrote above is correct and would appreciate
>>>>>> some confirmation of what is correct or incorrect.
>>>>> There are 2 series devices rely on this patch:
>>>>>
>>>>>       1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>>>
>>>>>       2) lan9662 PCIe card
>>>>>
>>>>>             please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>> Thanks.  Please include this information in future versions of the patch series.
>>>>
>>>> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
>>>> device tree.  I realize that this suggestion is only a partial solution if one wants to
>>>> use hotplug to change system configuration (as opposed to using hotplug only to replace
>>>> an existing device (eg a broken device) with another instance of the same device).  I
>>>> also realize that this increased the system administration overhead.  On the other hand
>>>> an overlay based solution is likely to be fragile and possibly flaky.
>>> Can you clarify the pre-boot apply approach? How will it work for PCI devices?
>>>>> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.
>>>> I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
>>>> available.  So the answer to my next question may vary by type of card.
>>>>
>>>> Is it expected that the fpga program on a given card will change frequently (eg multiple
>>>> times per day), where the changed program results in a new device that would require a
>>>> different hardware description in the device tree?
>>> Different images may be loaded to a FPGA partition several times a
>>> day. The PCI topology (Device IDs, BARs, MSIx, etc) does not change.
>>> New IPs may appear (and old IPs may disappear) on the BARs when a new
>>> image is loaded. We would like to use flattened device tree to
>>> describe the IPs on the BARs.
>> That was kind of a non-answer.  I know that images _may_ change at
>> some frequency.  I was trying to get a sense of whether the images
>> were _likely_ to be changing on a frequent basis for these types
>> of boards, or whether frequent image changes are likely to be a
>> rare edge use case.
>>
>> If there is a good design for the 99.999% use case that does not
>> support the 0.001% use case then it may be better to not create
>> an inferior design that also supports the 0.001% use case.
>>
>> I hope that gives a better idea of the reason why I was asking the
>> question and how the answer could impact design and implementation
>> decisions.
>>
>> As a point of reference, some other fpga users have indicated a
>> desire to change images many times per second.  The current driver
>> and overlay architecture did not seem to me to be a good match to
>> that use case (depending on the definition of "many").
> 
> I would rather we cover 99.999% now.
> 
> My understanding is that the subdevices are flexible but fairly
> static and the frequency Lizhi mentions would cover development
> uses.
> 
> In production I would expect the image to change about once a year
> with the same order of magnitude as firmware.

Thanks for this info, it helps a lot.

> 
> Can you point me to a reference of a user case with high frequency
> images changing that also depends on pci io device changing?

I actually don't have references to any previous PCI devices that are
based on FPGAs, let alone with a high frequency of images changing.

The Alveo devices are the first such devices that have come to my
attention.  Note that this is a technology space that I do not
follow, so my lack of awareness does not mean much.

I do not remember the specific discussion that was asserting or
desiring a high frequency of image changes for an FPGA.  The
current overlay architecture and overall device tree architecture
would not handle this well and/or robustly because (off the top of
my head, hopefully I'm getting this correct) the live system device
tree does not directly contain all of the associated data - some of
it is contained in the unflattened device tree (FDT) that remains in
memory after unflattening, both in the case of the base system device
tree and overlay device trees.  Some of the device tree data APIs return
pointers to this data in the FDT.  And the API does not provide reference
counting for the data (just reference counting for nodes - and these
reference counts are know to be frequently incorrect).

In general I have very little visibility into the FPGA space so I go
out of my way to notify them before making changes to the overlay
implementation, API, etc; listen carefully to their input; and give
them lots of opportunity to test any resulting changes.

-Frank

> 
> Tom
> 
>> -Frank
>>
>>> Thanks,
>>>
>>> Lizhi
>>>
>>>> Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
>>>> annually), in the same way as device firmware and operating systems are updated on a regular
>>>> basis for bug fixes and new functionality?
>>>>
>>>>
>>>>> Thanks,
>>>>>
>>>>> Lzhi
>>>>>
>>>>>> -Frank
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Sonal Santan]

On 9/19/22 20:12, Frank Rowand wrote:
> On 9/17/22 13:36, Tom Rix wrote:
>> Frank,
>>
>> On 9/16/22 7:23 PM, Frank Rowand wrote:
>>> On 9/13/22 16:02, Lizhi Hou wrote:
>>>> On 9/13/22 10:41, Frank Rowand wrote:
>>>>> On 9/13/22 12:10, Lizhi Hou wrote:
>>>>>> On 9/13/22 00:00, Frank Rowand wrote:
>>>>>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>>>>>> This patch series introduces OF overlay support for PCI devices which
>>>>>>>> primarily addresses two use cases. First, it provides a data driven method
>>>>>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>>>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>>>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>>>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>>>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>>>>>
>>>>>>>> This patch series consolidates previous efforts to define such an
>>>>>>>> infrastructure:
>>>>>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>>>>
>>>>>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>>>>>> PCI enumeration process. However, the process does not provide a way to
>>>>>>>> discover the hardware peripherals that are present in a PCI device, and
>>>>>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>>>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>>>>>> hardware peripherals that are present in the device. PCI device drivers
>>>>>>>> often use header files to describe the hardware peripherals and their
>>>>>>>> resources as there is no standard data driven way to do so. This patch
>>>>>>>> series proposes to use flattened device tree blob to describe the
>>>>>>>> peripherals in a data driven way. Based on previous discussion, using
>>>>>>>> device tree overlay is the best way to unflatten the blob and populate
>>>>>>>> platform devices. To use device tree overlay, there are three obvious
>>>>>>>> problems that need to be resolved.
>>>>>>>>
>>>>>>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>>>>>>> patch series has been submitted for this:
>>>>>>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>>>>>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>>>>>>
>>>>>>>> Second, a device tree node corresponding to the PCI endpoint is required
>>>>>>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>>>>>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>>>>>>> created for PCI devices. This series adds support to generate a device
>>>>>>>> tree node for a PCI device which advertises itself using PCI quirks
>>>>>>>> infrastructure.
>>>>>>>>
>>>>>>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>>>>>>> PCI endpoint may choose to have a device tree node created.
>>>>>>>>
>>>>>>>> This patch series is made up of two patches.
>>>>>>>>
>>>>>>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>>>>>>> from:
>>>>>>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>>>>>>
>>>>>>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>>>>>>> is turned on, the kernel will generate device tree nodes for all PCI
>>>>>>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>>>>>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>>>>>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>>>>>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>>>>>>> have any property. Future patches will add the necessary properties.
>>>>>>>>
>>>>>>>> Clément Léger (1):
>>>>>>>>       of: dynamic: add of_node_alloc()
>>>>>>>>
>>>>>>>> Lizhi Hou (1):
>>>>>>>>       pci: create device tree node for selected devices
>>>>>>>>
>>>>>>>>      drivers/of/dynamic.c        |  50 +++++++++++++----
>>>>>>>>      drivers/pci/Kconfig         |  11 ++++
>>>>>>>>      drivers/pci/bus.c           |   2 +
>>>>>>>>      drivers/pci/msi/irqdomain.c |   6 +-
>>>>>>>>      drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>>>>>>      drivers/pci/pci-driver.c    |   3 +-
>>>>>>>>      drivers/pci/pci.h           |  16 ++++++
>>>>>>>>      drivers/pci/quirks.c        |  11 ++++
>>>>>>>>      drivers/pci/remove.c        |   1 +
>>>>>>>>      include/linux/of.h          |   7 +++
>>>>>>>>      10 files changed, 200 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>> The patch description leaves out the most important piece of information.
>>>>>>>
>>>>>>> The device located at the PCI endpoint is implemented via FPGA
>>>>>>>        - which is programmed after Linux boots (or somewhere late in the boot process)
>>>>>>>           - (A) and thus can not be described by static data available pre-boot because
>>>>>>>                 it is dynamic (and the FPGA program will often change while the Linux
>>>>>>>                 kernel is already booted
>>>>>>>           - (B) can be described by static data available pre-boot because the FPGA
>>>>>>>                 program will always be the same for this device on this system
>>>>>>>
>>>>>>> I am not positive what part of what I wrote above is correct and would appreciate
>>>>>>> some confirmation of what is correct or incorrect.
>>>>>> There are 2 series devices rely on this patch:
>>>>>>
>>>>>>        1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>>>>
>>>>>>        2) lan9662 PCIe card
>>>>>>
>>>>>>              please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>> Thanks.  Please include this information in future versions of the patch series.
>>>>>
>>>>> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
>>>>> device tree.  I realize that this suggestion is only a partial solution if one wants to
>>>>> use hotplug to change system configuration (as opposed to using hotplug only to replace
>>>>> an existing device (eg a broken device) with another instance of the same device).  I
>>>>> also realize that this increased the system administration overhead.  On the other hand
>>>>> an overlay based solution is likely to be fragile and possibly flaky.
>>>> Can you clarify the pre-boot apply approach? How will it work for PCI devices?
>>>>>> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.
>>>>> I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
>>>>> available.  So the answer to my next question may vary by type of card.
>>>>>
>>>>> Is it expected that the fpga program on a given card will change frequently (eg multiple
>>>>> times per day), where the changed program results in a new device that would require a
>>>>> different hardware description in the device tree?
>>>> Different images may be loaded to a FPGA partition several times a
>>>> day. The PCI topology (Device IDs, BARs, MSIx, etc) does not change.
>>>> New IPs may appear (and old IPs may disappear) on the BARs when a new
>>>> image is loaded. We would like to use flattened device tree to
>>>> describe the IPs on the BARs.
>>> That was kind of a non-answer.  I know that images _may_ change at
>>> some frequency.  I was trying to get a sense of whether the images
>>> were _likely_ to be changing on a frequent basis for these types
>>> of boards, or whether frequent image changes are likely to be a
>>> rare edge use case.
>>>
>>> If there is a good design for the 99.999% use case that does not
>>> support the 0.001% use case then it may be better to not create
>>> an inferior design that also supports the 0.001% use case.
>>>
>>> I hope that gives a better idea of the reason why I was asking the
>>> question and how the answer could impact design and implementation
>>> decisions.
>>>
>>> As a point of reference, some other fpga users have indicated a
>>> desire to change images many times per second.  The current driver
>>> and overlay architecture did not seem to me to be a good match to
>>> that use case (depending on the definition of "many").
>>
>> I would rather we cover 99.999% now.
>>
>> My understanding is that the subdevices are flexible but fairly
>> static and the frequency Lizhi mentions would cover development
>> uses.
>>
>> In production I would expect the image to change about once a year
>> with the same order of magnitude as firmware.
> 
> Thanks for this info, it helps a lot.
> 
>>
>> Can you point me to a reference of a user case with high frequency
>> images changing that also depends on pci io device changing?
> 
> I actually don't have references to any previous PCI devices that are
> based on FPGAs, let alone with a high frequency of images changing.
> 
> The Alveo devices are the first such devices that have come to my
> attention.  Note that this is a technology space that I do not
> follow, so my lack of awareness does not mean much.
> 
> I do not remember the specific discussion that was asserting or
> desiring a high frequency of image changes for an FPGA.  The
> current overlay architecture and overall device tree architecture
> would not handle this well and/or robustly because (off the top of
> my head, hopefully I'm getting this correct) the live system device
> tree does not directly contain all of the associated data - some of
> it is contained in the unflattened device tree (FDT) that remains in
> memory after unflattening, both in the case of the base system device
> tree and overlay device trees.  Some of the device tree data APIs return
> pointers to this data in the FDT.  And the API does not provide reference
> counting for the data (just reference counting for nodes - and these
> reference counts are know to be frequently incorrect).
> 
Thanks for pointing out the limitations of the current overlay 
architecture. Can a careful orchestration of overlay creation and tear 
down by each driver address the limitation? I did see another user, 
drivers/pci/hotplug/pnv_php.c, which seems to be using the overlay 
infrastructure in this manner.

What is your suggestion to move forward?

-Sonal

> In general I have very little visibility into the FPGA space so I go
> out of my way to notify them before making changes to the overlay
> implementation, API, etc; listen carefully to their input; and give
> them lots of opportunity to test any resulting changes.
> 
> -Frank
> 
>>
>> Tom
>>
>>> -Frank
>>>
>>>> Thanks,
>>>>
>>>> Lizhi
>>>>
>>>>> Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
>>>>> annually), in the same way as device firmware and operating systems are updated on a regular
>>>>> basis for bug fixes and new functionality?
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Lzhi
>>>>>>
>>>>>>> -Frank
>>
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 9/25/22 22:03, Sonal Santan wrote:
> On 9/19/22 20:12, Frank Rowand wrote:
>> On 9/17/22 13:36, Tom Rix wrote:
>>> Frank,
>>>
>>> On 9/16/22 7:23 PM, Frank Rowand wrote:
>>>> On 9/13/22 16:02, Lizhi Hou wrote:
>>>>> On 9/13/22 10:41, Frank Rowand wrote:
>>>>>> On 9/13/22 12:10, Lizhi Hou wrote:
>>>>>>> On 9/13/22 00:00, Frank Rowand wrote:
>>>>>>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>>>>>>> This patch series introduces OF overlay support for PCI devices which
>>>>>>>>> primarily addresses two use cases. First, it provides a data driven method
>>>>>>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>>>>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>>>>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>>>>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>>>>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>>>>>>
>>>>>>>>> This patch series consolidates previous efforts to define such an
>>>>>>>>> infrastructure:
>>>>>>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>>>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>>>>>
>>>>>>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>>>>>>> PCI enumeration process. However, the process does not provide a way to
>>>>>>>>> discover the hardware peripherals that are present in a PCI device, and
>>>>>>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>>>>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>>>>>>> hardware peripherals that are present in the device. PCI device drivers
>>>>>>>>> often use header files to describe the hardware peripherals and their
>>>>>>>>> resources as there is no standard data driven way to do so. This patch
>>>>>>>>> series proposes to use flattened device tree blob to describe the
>>>>>>>>> peripherals in a data driven way. Based on previous discussion, using
>>>>>>>>> device tree overlay is the best way to unflatten the blob and populate
>>>>>>>>> platform devices. To use device tree overlay, there are three obvious
>>>>>>>>> problems that need to be resolved.
>>>>>>>>>
>>>>>>>>> First, we need to create a base tree for non-DT system such as x86_64. A
>>>>>>>>> patch series has been submitted for this:
>>>>>>>>> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
>>>>>>>>> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>>>>>>>>>
>>>>>>>>> Second, a device tree node corresponding to the PCI endpoint is required
>>>>>>>>> for overlaying the flattened device tree blob for that PCI endpoint.
>>>>>>>>> Because PCI is a self-discoverable bus, a device tree node is usually not
>>>>>>>>> created for PCI devices. This series adds support to generate a device
>>>>>>>>> tree node for a PCI device which advertises itself using PCI quirks
>>>>>>>>> infrastructure.
>>>>>>>>>
>>>>>>>>> Third, we need to generate device tree nodes for PCI bridges since a child
>>>>>>>>> PCI endpoint may choose to have a device tree node created.
>>>>>>>>>
>>>>>>>>> This patch series is made up of two patches.
>>>>>>>>>
>>>>>>>>> The first patch is adding OF interface to allocate an OF node. It is copied
>>>>>>>>> from:
>>>>>>>>> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
>>>>>>>>>
>>>>>>>>> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
>>>>>>>>> is turned on, the kernel will generate device tree nodes for all PCI
>>>>>>>>> bridges unconditionally. The patch also shows how to use the PCI quirks
>>>>>>>>> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
>>>>>>>>> a device. Specifically, the patch generates a device tree node for Xilinx
>>>>>>>>> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
>>>>>>>>> have any property. Future patches will add the necessary properties.
>>>>>>>>>
>>>>>>>>> Clément Léger (1):
>>>>>>>>>       of: dynamic: add of_node_alloc()
>>>>>>>>>
>>>>>>>>> Lizhi Hou (1):
>>>>>>>>>       pci: create device tree node for selected devices
>>>>>>>>>
>>>>>>>>>      drivers/of/dynamic.c        |  50 +++++++++++++----
>>>>>>>>>      drivers/pci/Kconfig         |  11 ++++
>>>>>>>>>      drivers/pci/bus.c           |   2 +
>>>>>>>>>      drivers/pci/msi/irqdomain.c |   6 +-
>>>>>>>>>      drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>      drivers/pci/pci-driver.c    |   3 +-
>>>>>>>>>      drivers/pci/pci.h           |  16 ++++++
>>>>>>>>>      drivers/pci/quirks.c        |  11 ++++
>>>>>>>>>      drivers/pci/remove.c        |   1 +
>>>>>>>>>      include/linux/of.h          |   7 +++
>>>>>>>>>      10 files changed, 200 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>> The patch description leaves out the most important piece of information.
>>>>>>>>
>>>>>>>> The device located at the PCI endpoint is implemented via FPGA
>>>>>>>>        - which is programmed after Linux boots (or somewhere late in the boot process)
>>>>>>>>           - (A) and thus can not be described by static data available pre-boot because
>>>>>>>>                 it is dynamic (and the FPGA program will often change while the Linux
>>>>>>>>                 kernel is already booted
>>>>>>>>           - (B) can be described by static data available pre-boot because the FPGA
>>>>>>>>                 program will always be the same for this device on this system
>>>>>>>>
>>>>>>>> I am not positive what part of what I wrote above is correct and would appreciate
>>>>>>>> some confirmation of what is correct or incorrect.
>>>>>>> There are 2 series devices rely on this patch:
>>>>>>>
>>>>>>>        1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>>>>>
>>>>>>>        2) lan9662 PCIe card
>>>>>>>
>>>>>>>              please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>> Thanks.  Please include this information in future versions of the patch series.
>>>>>>
>>>>>> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
>>>>>> device tree.  I realize that this suggestion is only a partial solution if one wants to
>>>>>> use hotplug to change system configuration (as opposed to using hotplug only to replace
>>>>>> an existing device (eg a broken device) with another instance of the same device).  I
>>>>>> also realize that this increased the system administration overhead.  On the other hand
>>>>>> an overlay based solution is likely to be fragile and possibly flaky.
>>>>> Can you clarify the pre-boot apply approach? How will it work for PCI devices?
>>>>>>> For Xilinx Alveo device, it is (A). The FPGA partitions can be programmed dynamically after boot.
>>>>>> I looked at the Xilinx Alveo web page, and there are a variety of types of Alveo cards
>>>>>> available.  So the answer to my next question may vary by type of card.
>>>>>>
>>>>>> Is it expected that the fpga program on a given card will change frequently (eg multiple
>>>>>> times per day), where the changed program results in a new device that would require a
>>>>>> different hardware description in the device tree?
>>>>> Different images may be loaded to a FPGA partition several times a
>>>>> day. The PCI topology (Device IDs, BARs, MSIx, etc) does not change.
>>>>> New IPs may appear (and old IPs may disappear) on the BARs when a new
>>>>> image is loaded. We would like to use flattened device tree to
>>>>> describe the IPs on the BARs.
>>>> That was kind of a non-answer.  I know that images _may_ change at
>>>> some frequency.  I was trying to get a sense of whether the images
>>>> were _likely_ to be changing on a frequent basis for these types
>>>> of boards, or whether frequent image changes are likely to be a
>>>> rare edge use case.
>>>>
>>>> If there is a good design for the 99.999% use case that does not
>>>> support the 0.001% use case then it may be better to not create
>>>> an inferior design that also supports the 0.001% use case.
>>>>
>>>> I hope that gives a better idea of the reason why I was asking the
>>>> question and how the answer could impact design and implementation
>>>> decisions.
>>>>
>>>> As a point of reference, some other fpga users have indicated a
>>>> desire to change images many times per second.  The current driver
>>>> and overlay architecture did not seem to me to be a good match to
>>>> that use case (depending on the definition of "many").
>>>
>>> I would rather we cover 99.999% now.
>>>
>>> My understanding is that the subdevices are flexible but fairly
>>> static and the frequency Lizhi mentions would cover development
>>> uses.
>>>
>>> In production I would expect the image to change about once a year
>>> with the same order of magnitude as firmware.
>>
>> Thanks for this info, it helps a lot.
>>
>>>
>>> Can you point me to a reference of a user case with high frequency
>>> images changing that also depends on pci io device changing?
>>
>> I actually don't have references to any previous PCI devices that are
>> based on FPGAs, let alone with a high frequency of images changing.
>>
>> The Alveo devices are the first such devices that have come to my
>> attention.  Note that this is a technology space that I do not
>> follow, so my lack of awareness does not mean much.
>>
>> I do not remember the specific discussion that was asserting or
>> desiring a high frequency of image changes for an FPGA.  The
>> current overlay architecture and overall device tree architecture
>> would not handle this well and/or robustly because (off the top of
>> my head, hopefully I'm getting this correct) the live system device
>> tree does not directly contain all of the associated data - some of
>> it is contained in the unflattened device tree (FDT) that remains in
>> memory after unflattening, both in the case of the base system device
>> tree and overlay device trees.  Some of the device tree data APIs return
>> pointers to this data in the FDT.  And the API does not provide reference
>> counting for the data (just reference counting for nodes - and these
>> reference counts are know to be frequently incorrect).
>>
> Thanks for pointing out the limitations of the current overlay
> architecture. Can a careful orchestration of overlay creation and
> tear down by each driver address the limitation? 

No, that is not practical (for example, see the never ending patches
to address broken refcounting -- of_node_get() and of_node_put() usage).
Plus the overlay data in the system device tree is visible to every
driver and subsystem, not just the limited set that appear to be
related to the nodes in the overlay.

> I did see another
> user, drivers/pci/hotplug/pnv_php.c, which seems to be using the
> overlay infrastructure in this manner.

What tree is that in?  And what sections of that file?

> 
> What is your suggestion to move forward?

https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts has a large
but incomplete list of areas to be worked on.

-Frank

> 
> -Sonal
> 
>> In general I have very little visibility into the FPGA space so I go
>> out of my way to notify them before making changes to the overlay
>> implementation, API, etc; listen carefully to their input; and give
>> them lots of opportunity to test any resulting changes.
>>
>> -Frank
>>
>>>
>>> Tom
>>>
>>>> -Frank
>>>>
>>>>> Thanks,
>>>>>
>>>>> Lizhi
>>>>>
>>>>>> Or is the fpga program expected to change on an infrequent basis (eg monthly, quarterly,
>>>>>> annually), in the same way as device firmware and operating systems are updated on a regular
>>>>>> basis for bug fixes and new functionality?
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Lzhi
>>>>>>>
>>>>>>>> -Frank
>>>
>>
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Clément Léger]

Le Tue, 13 Sep 2022 12:41:28 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> >> I am not positive what part of what I wrote above is correct and would appreciate
> >> some confirmation of what is correct or incorrect.  
> > 
> > There are 2 series devices rely on this patch:
> > 
> >     1) Xilinx Alveo Accelerator cards (FPGA based device)
> > 
> >     2) lan9662 PCIe card
> > 
> >           please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/  
> 
> Thanks.  Please include this information in future versions of the patch series.
> 
> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
> device tree.  I realize that this suggestion is only a partial solution if one wants to
> use hotplug to change system configuration (as opposed to using hotplug only to replace
> an existing device (eg a broken device) with another instance of the same device).  I
> also realize that this increased the system administration overhead.  On the other hand
> an overlay based solution is likely to be fragile and possibly flaky.

Again, applying overlays pre-boot is not an acceptable solution. Some
systems are not based on device-tree (x86 platforms with ACPI based
description, and I'm not even sure this is doable by modifying ACPI
tables). PCI is meant to be plug-and-play, so patching the ACPI
tables or device-tree pre-boot is likely not the correct answer to this
problem.

This would also require two different descriptions of the same card
(for ACPI and device-tree) and would require the final user to create a
specific overlay for its device based on the PCI slots the card is
plugged in.

The solution we proposed (Lizhi and I) allows to overcome these
problems and is way easier to use. Fixing the potential bugs that might
exists in the overlay layer seems a way better idea that just pushing
that away to the bootloader level. Moreover, this kind of devices is
likely to be more common with the increasing popularity of FPGA and a
proper solution must be found.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Frank Rowand]

On 10/10/22 03:42, Clément Léger wrote:
> Le Tue, 13 Sep 2022 12:41:28 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>>>> I am not positive what part of what I wrote above is correct and would appreciate
>>>> some confirmation of what is correct or incorrect.  
>>>
>>> There are 2 series devices rely on this patch:
>>>
>>>     1) Xilinx Alveo Accelerator cards (FPGA based device)
>>>
>>>     2) lan9662 PCIe card
>>>
>>>           please see: https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/  
>>
>> Thanks.  Please include this information in future versions of the patch series.
>>
>> For device 2 I have strongly recommended using pre-boot apply of the overlay to the base
>> device tree.  I realize that this suggestion is only a partial solution if one wants to
>> use hotplug to change system configuration (as opposed to using hotplug only to replace
>> an existing device (eg a broken device) with another instance of the same device).  I
>> also realize that this increased the system administration overhead.  On the other hand
>> an overlay based solution is likely to be fragile and possibly flaky.
> 
> Again, applying overlays pre-boot is not an acceptable solution. Some
> systems are not based on device-tree (x86 platforms with ACPI based
> description, and I'm not even sure this is doable by modifying ACPI
> tables). PCI is meant to be plug-and-play, so patching the ACPI
> tables or device-tree pre-boot is likely not the correct answer to this
> problem.
> 


> This would also require two different descriptions of the same card
> (for ACPI and device-tree) and would require the final user to create a
> specific overlay for its device based on the PCI slots the card is
> plugged in.

One of the many missing pieces of overlay support.  There have been several
discussion of how to describe a "socket" in a device tree that a device
could be plugged into, where a single device tree subtree .dtb could be
relocated to one or more different socket locations.  Thus in this
case a single overlay could be relocated to various PCI slots.

I don't expect be getting involved in any future efforts around sockets
(see my following comment for why).

> 
> The solution we proposed (Lizhi and I) allows to overcome these
> problems and is way easier to use. Fixing the potential bugs that might
> exists in the overlay layer seems a way better idea that just pushing

It is not potential bugs.  The current run time overlay implementation is
proof of concept quality and completeness.  It is not production ready.

I got an opportunity for early retirement a couple of weeks ago.  My first
inclination was to continue the same level of device tree maintainership,
but I am quickly realizing that there are other activities that I would
like to devote my time and energy to.  I will continue to support Rob with
minor patch reviews and testing, and potentially finishing up some
improvements to unittest.  On the other hand, bringing run time overlay
support to product quality would be a major investment of my time that I
am not willing to continue.

So I am leaving major overlay issues in the capable hands of Rob.  I may
chime in from time to time when I can do so without requiring too much of
my time.

-Frank

> that away to the bootloader level. Moreover, this kind of devices is
> likely to be more common with the increasing popularity of FPGA and a
> proper solution must be found.
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Clément Léger]

Le Thu, 13 Oct 2022 01:05:26 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> > This would also require two different descriptions of the same card
> > (for ACPI and device-tree) and would require the final user to create a
> > specific overlay for its device based on the PCI slots the card is
> > plugged in.  
> 
> One of the many missing pieces of overlay support.  There have been several
> discussion of how to describe a "socket" in a device tree that a device
> could be plugged into, where a single device tree subtree .dtb could be
> relocated to one or more different socket locations.  Thus in this
> case a single overlay could be relocated to various PCI slots.
> 
> I don't expect be getting involved in any future efforts around sockets
> (see my following comment for why).
> 
> > 
> > The solution we proposed (Lizhi and I) allows to overcome these
> > problems and is way easier to use. Fixing the potential bugs that might
> > exists in the overlay layer seems a way better idea that just pushing  
> 
> It is not potential bugs.  The current run time overlay implementation is
> proof of concept quality and completeness.  It is not production ready.
> 
> I got an opportunity for early retirement a couple of weeks ago.  My first
> inclination was to continue the same level of device tree maintainership,
> but I am quickly realizing that there are other activities that I would
> like to devote my time and energy to.  I will continue to support Rob with
> minor patch reviews and testing, and potentially finishing up some
> improvements to unittest.  On the other hand, bringing run time overlay
> support to product quality would be a major investment of my time that I
> am not willing to continue.

Hi Frank,

This explains your position on the overlay support and I can
certainly understand it ! Regarding the fact that it would enter
"production", the devices we are talking about are not really
widespread yet? This would be a good opportunity to gather feedback
early and improve the support gradually. We could probably even be able
to support improvements in the overlay code if needed I guess.

Thanks for your honest answer,

Clément

> 
> So I am leaving major overlay issues in the capable hands of Rob.  I may
> chime in from time to time when I can do so without requiring too much of
> my time.
> 
> -Frank
> 
> > that away to the bootloader level. Moreover, this kind of devices is
> > likely to be more common with the increasing popularity of FPGA and a
> > proper solution must be found.
> >   



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Frank Rowand]

On 10/13/22 03:02, Clément Léger wrote:
> Le Thu, 13 Oct 2022 01:05:26 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>>> This would also require two different descriptions of the same card
>>> (for ACPI and device-tree) and would require the final user to create a
>>> specific overlay for its device based on the PCI slots the card is
>>> plugged in.  
>>
>> One of the many missing pieces of overlay support.  There have been several
>> discussion of how to describe a "socket" in a device tree that a device
>> could be plugged into, where a single device tree subtree .dtb could be
>> relocated to one or more different socket locations.  Thus in this
>> case a single overlay could be relocated to various PCI slots.
>>
>> I don't expect be getting involved in any future efforts around sockets
>> (see my following comment for why).
>>
>>>
>>> The solution we proposed (Lizhi and I) allows to overcome these
>>> problems and is way easier to use. Fixing the potential bugs that might
>>> exists in the overlay layer seems a way better idea that just pushing  
>>
>> It is not potential bugs.  The current run time overlay implementation is
>> proof of concept quality and completeness.  It is not production ready.
>>
>> I got an opportunity for early retirement a couple of weeks ago.  My first
>> inclination was to continue the same level of device tree maintainership,
>> but I am quickly realizing that there are other activities that I would
>> like to devote my time and energy to.  I will continue to support Rob with
>> minor patch reviews and testing, and potentially finishing up some
>> improvements to unittest.  On the other hand, bringing run time overlay
>> support to product quality would be a major investment of my time that I
>> am not willing to continue.
> 
> Hi Frank,
> 
> This explains your position on the overlay support and I can
> certainly understand it ! Regarding the fact that it would enter

No, my position on the technical aspects of overlay support is totally
unchanged.

The only thing that has changed is that my time will not be available to
assist in future overlay related work.  The burden for this will fall
more on Rob than it has in the past.


> "production", the devices we are talking about are not really
> widespread yet? This would be a good opportunity to gather feedback
> early and improve the support gradually. We could probably even be able
> to support improvements in the overlay code if needed I guess.

That is avoiding my point about the current implementation being
proof of concept.

-Frank

> 
> Thanks for your honest answer,
> 
> Clément
> 
>>
>> So I am leaving major overlay issues in the capable hands of Rob.  I may
>> chime in from time to time when I can do so without requiring too much of
>> my time.
>>
>> -Frank
>>
>>> that away to the bootloader level. Moreover, this kind of devices is
>>> likely to be more common with the increasing popularity of FPGA and a
>>> proper solution must be found.
>>>   
> 
> 
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Rob Herring]

On Thu, Oct 13, 2022 at 12:28 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 10/13/22 03:02, Clément Léger wrote:
> > Le Thu, 13 Oct 2022 01:05:26 -0500,
> > Frank Rowand <frowand.list@gmail.com> a écrit :
> >
> >>> This would also require two different descriptions of the same card
> >>> (for ACPI and device-tree) and would require the final user to create a
> >>> specific overlay for its device based on the PCI slots the card is
> >>> plugged in.
> >>
> >> One of the many missing pieces of overlay support.  There have been several
> >> discussion of how to describe a "socket" in a device tree that a device
> >> could be plugged into, where a single device tree subtree .dtb could be
> >> relocated to one or more different socket locations.  Thus in this
> >> case a single overlay could be relocated to various PCI slots.
> >>
> >> I don't expect be getting involved in any future efforts around sockets
> >> (see my following comment for why).
> >>
> >>>
> >>> The solution we proposed (Lizhi and I) allows to overcome these
> >>> problems and is way easier to use. Fixing the potential bugs that might
> >>> exists in the overlay layer seems a way better idea that just pushing
> >>
> >> It is not potential bugs.  The current run time overlay implementation is
> >> proof of concept quality and completeness.  It is not production ready.
> >>
> >> I got an opportunity for early retirement a couple of weeks ago.  My first
> >> inclination was to continue the same level of device tree maintainership,
> >> but I am quickly realizing that there are other activities that I would
> >> like to devote my time and energy to.  I will continue to support Rob with
> >> minor patch reviews and testing, and potentially finishing up some
> >> improvements to unittest.  On the other hand, bringing run time overlay
> >> support to product quality would be a major investment of my time that I
> >> am not willing to continue.
> >
> > Hi Frank,
> >
> > This explains your position on the overlay support and I can
> > certainly understand it ! Regarding the fact that it would enter
>
> No, my position on the technical aspects of overlay support is totally
> unchanged.
>
> The only thing that has changed is that my time will not be available to
> assist in future overlay related work.  The burden for this will fall
> more on Rob than it has in the past.

s/Rob/someone that steps up to maintain the overlay code/

> > "production", the devices we are talking about are not really
> > widespread yet? This would be a good opportunity to gather feedback
> > early and improve the support gradually. We could probably even be able
> > to support improvements in the overlay code if needed I guess.
>
> That is avoiding my point about the current implementation being
> proof of concept.

I think it would be better to talk in terms of under what conditions
the overlay support is adequate (for production) rather than a blanket
statement that it is not-production ready. A large part of it is
really outside the code itself and related to going from static to
dynamic DT. There are certainly issues, but dynamic DTs have been used
in production for a very long time. However, that usage has been
constrained.

Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Frank Rowand]

On 10/14/22 12:33, Rob Herring wrote:
> On Thu, Oct 13, 2022 at 12:28 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 10/13/22 03:02, Clément Léger wrote:
>>> Le Thu, 13 Oct 2022 01:05:26 -0500,
>>> Frank Rowand <frowand.list@gmail.com> a écrit :
>>>
>>>>> This would also require two different descriptions of the same card
>>>>> (for ACPI and device-tree) and would require the final user to create a
>>>>> specific overlay for its device based on the PCI slots the card is
>>>>> plugged in.
>>>>
>>>> One of the many missing pieces of overlay support.  There have been several
>>>> discussion of how to describe a "socket" in a device tree that a device
>>>> could be plugged into, where a single device tree subtree .dtb could be
>>>> relocated to one or more different socket locations.  Thus in this
>>>> case a single overlay could be relocated to various PCI slots.
>>>>
>>>> I don't expect be getting involved in any future efforts around sockets
>>>> (see my following comment for why).
>>>>
>>>>>
>>>>> The solution we proposed (Lizhi and I) allows to overcome these
>>>>> problems and is way easier to use. Fixing the potential bugs that might
>>>>> exists in the overlay layer seems a way better idea that just pushing
>>>>
>>>> It is not potential bugs.  The current run time overlay implementation is
>>>> proof of concept quality and completeness.  It is not production ready.
>>>>
>>>> I got an opportunity for early retirement a couple of weeks ago.  My first
>>>> inclination was to continue the same level of device tree maintainership,
>>>> but I am quickly realizing that there are other activities that I would
>>>> like to devote my time and energy to.  I will continue to support Rob with
>>>> minor patch reviews and testing, and potentially finishing up some
>>>> improvements to unittest.  On the other hand, bringing run time overlay
>>>> support to product quality would be a major investment of my time that I
>>>> am not willing to continue.
>>>
>>> Hi Frank,
>>>
>>> This explains your position on the overlay support and I can
>>> certainly understand it ! Regarding the fact that it would enter
>>
>> No, my position on the technical aspects of overlay support is totally
>> unchanged.
>>
>> The only thing that has changed is that my time will not be available to
>> assist in future overlay related work.  The burden for this will fall
>> more on Rob than it has in the past.
> 
> s/Rob/someone that steps up to maintain the overlay code/
> 
>>> "production", the devices we are talking about are not really
>>> widespread yet? This would be a good opportunity to gather feedback
>>> early and improve the support gradually. We could probably even be able
>>> to support improvements in the overlay code if needed I guess.
>>
>> That is avoiding my point about the current implementation being
>> proof of concept.
> 


> I think it would be better to talk in terms of under what conditions
> the overlay support is adequate (for production) rather than a blanket
> statement that it is not-production ready. 

I sort of agree.  Use of run time overlays has been narrowly supported
for use by a limited set of very cautious developers in a very constrained
usage.

> A large part of it is
> really outside the code itself and related to going from static to
> dynamic DT. There are certainly issues, but dynamic DTs have been used
> in production for a very long time. However, that usage has been
> constrained.

Yes, to the dynamic DT comments.

When the run time overlay code was added the overlay code used the existing
dynamic DT code as a foundation but did not address the architectural
issues that are exposed by using the dynamic DT code in a less constrained
manner.

> 
> Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Clément Léger]

Le Fri, 14 Oct 2022 13:52:50 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 10/14/22 12:33, Rob Herring wrote:
> > On Thu, Oct 13, 2022 at 12:28 PM Frank Rowand <frowand.list@gmail.com> wrote:  
> >>
> >> On 10/13/22 03:02, Clément Léger wrote:  
> >>> Le Thu, 13 Oct 2022 01:05:26 -0500,
> >>> Frank Rowand <frowand.list@gmail.com> a écrit :
> >>>  
> >>>>> This would also require two different descriptions of the same card
> >>>>> (for ACPI and device-tree) and would require the final user to create a
> >>>>> specific overlay for its device based on the PCI slots the card is
> >>>>> plugged in.  
> >>>>
> >>>> One of the many missing pieces of overlay support.  There have been several
> >>>> discussion of how to describe a "socket" in a device tree that a device
> >>>> could be plugged into, where a single device tree subtree .dtb could be
> >>>> relocated to one or more different socket locations.  Thus in this
> >>>> case a single overlay could be relocated to various PCI slots.
> >>>>
> >>>> I don't expect be getting involved in any future efforts around sockets
> >>>> (see my following comment for why).
> >>>>  
> >>>>>
> >>>>> The solution we proposed (Lizhi and I) allows to overcome these
> >>>>> problems and is way easier to use. Fixing the potential bugs that might
> >>>>> exists in the overlay layer seems a way better idea that just pushing  
> >>>>
> >>>> It is not potential bugs.  The current run time overlay implementation is
> >>>> proof of concept quality and completeness.  It is not production ready.
> >>>>
> >>>> I got an opportunity for early retirement a couple of weeks ago.  My first
> >>>> inclination was to continue the same level of device tree maintainership,
> >>>> but I am quickly realizing that there are other activities that I would
> >>>> like to devote my time and energy to.  I will continue to support Rob with
> >>>> minor patch reviews and testing, and potentially finishing up some
> >>>> improvements to unittest.  On the other hand, bringing run time overlay
> >>>> support to product quality would be a major investment of my time that I
> >>>> am not willing to continue.  
> >>>
> >>> Hi Frank,
> >>>
> >>> This explains your position on the overlay support and I can
> >>> certainly understand it ! Regarding the fact that it would enter  
> >>
> >> No, my position on the technical aspects of overlay support is totally
> >> unchanged.
> >>
> >> The only thing that has changed is that my time will not be available to
> >> assist in future overlay related work.  The burden for this will fall
> >> more on Rob than it has in the past.  
> > 
> > s/Rob/someone that steps up to maintain the overlay code/
> >   
> >>> "production", the devices we are talking about are not really
> >>> widespread yet? This would be a good opportunity to gather feedback
> >>> early and improve the support gradually. We could probably even be able
> >>> to support improvements in the overlay code if needed I guess.  
> >>
> >> That is avoiding my point about the current implementation being
> >> proof of concept.  
> >   
> 
> 
> > I think it would be better to talk in terms of under what conditions
> > the overlay support is adequate (for production) rather than a blanket
> > statement that it is not-production ready.   
> 
> I sort of agree.  Use of run time overlays has been narrowly supported
> for use by a limited set of very cautious developers in a very constrained
> usage.

As a first working point, could we potentially restrict drivers to only
insert an overlay but not remove it ? It would be quite limited, but
as you pointed out, the multiple load/unload (or FPGA reconfiguration)
will only happen during development. Under "normal" condition, we could
expect the FPGA to be configured once during the system runtime. The
same goes for our PCI card which uses an existing SoC, we can probably
assume that it is going to be plugged once for all during the system
runtime.

This would limit the problems that might happen due to dynamic
insertion/removal of the overlay.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH RFC 0/2] Generate device tree node for pci devicesgain,

[Sonal Santan]

On 10/17/22 00:18, Clément Léger wrote:
> Le Fri, 14 Oct 2022 13:52:50 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>> On 10/14/22 12:33, Rob Herring wrote:
>>> On Thu, Oct 13, 2022 at 12:28 PM Frank Rowand <frowand.list@gmail.com> wrote:  
>>>>
>>>> On 10/13/22 03:02, Clément Léger wrote:  
>>>>> Le Thu, 13 Oct 2022 01:05:26 -0500,
>>>>> Frank Rowand <frowand.list@gmail.com> a écrit :
>>>>>  
>>>>>>> This would also require two different descriptions of the same card
>>>>>>> (for ACPI and device-tree) and would require the final user to create a
>>>>>>> specific overlay for its device based on the PCI slots the card is
>>>>>>> plugged in.  
>>>>>>
>>>>>> One of the many missing pieces of overlay support.  There have been several
>>>>>> discussion of how to describe a "socket" in a device tree that a device
>>>>>> could be plugged into, where a single device tree subtree .dtb could be
>>>>>> relocated to one or more different socket locations.  Thus in this
>>>>>> case a single overlay could be relocated to various PCI slots.
>>>>>>
>>>>>> I don't expect be getting involved in any future efforts around sockets
>>>>>> (see my following comment for why).
>>>>>>  
>>>>>>>
>>>>>>> The solution we proposed (Lizhi and I) allows to overcome these
>>>>>>> problems and is way easier to use. Fixing the potential bugs that might
>>>>>>> exists in the overlay layer seems a way better idea that just pushing  
>>>>>>
>>>>>> It is not potential bugs.  The current run time overlay implementation is
>>>>>> proof of concept quality and completeness.  It is not production ready.
>>>>>>
>>>>>> I got an opportunity for early retirement a couple of weeks ago.  My first
>>>>>> inclination was to continue the same level of device tree maintainership,
>>>>>> but I am quickly realizing that there are other activities that I would
>>>>>> like to devote my time and energy to.  I will continue to support Rob with
>>>>>> minor patch reviews and testing, and potentially finishing up some
>>>>>> improvements to unittest.  On the other hand, bringing run time overlay
>>>>>> support to product quality would be a major investment of my time that I
>>>>>> am not willing to continue.  
>>>>>
>>>>> Hi Frank,
>>>>>
>>>>> This explains your position on the overlay support and I can
>>>>> certainly understand it ! Regarding the fact that it would enter  
>>>>
>>>> No, my position on the technical aspects of overlay support is totally
>>>> unchanged.
>>>>
>>>> The only thing that has changed is that my time will not be available to
>>>> assist in future overlay related work.  The burden for this will fall
>>>> more on Rob than it has in the past.  
>>>
>>> s/Rob/someone that steps up to maintain the overlay code/
>>>   
>>>>> "production", the devices we are talking about are not really
>>>>> widespread yet? This would be a good opportunity to gather feedback
>>>>> early and improve the support gradually. We could probably even be able
>>>>> to support improvements in the overlay code if needed I guess.  
>>>>
>>>> That is avoiding my point about the current implementation being
>>>> proof of concept.  
>>>   
>>
>>
>>> I think it would be better to talk in terms of under what conditions
>>> the overlay support is adequate (for production) rather than a blanket
>>> statement that it is not-production ready.   
>>
>> I sort of agree.  Use of run time overlays has been narrowly supported
>> for use by a limited set of very cautious developers in a very constrained
>> usage.
> 
> As a first working point, could we potentially restrict drivers to only
> insert an overlay but not remove it ? It would be quite limited, but
> as you pointed out, the multiple load/unload (or FPGA reconfiguration)
> will only happen during development. Under "normal" condition, we could
> expect the FPGA to be configured once during the system runtime. The
> same goes for our PCI card which uses an existing SoC, we can probably
> assume that it is going to be plugged once for all during the system
> runtime.
> 
> This would limit the problems that might happen due to dynamic
> insertion/removal of the overlay.
> 
We would need "limited" overlay removal support to handle driver unload or device hotplug. Limited removal support will also be needed for Alveo use case in order to handle FPGA reconfiguration in production environment.

-Sonal

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Jeremi Piotrowski]

On Mon, Aug 29, 2022 at 02:43:35PM -0700, Lizhi Hou wrote:
> This patch series introduces OF overlay support for PCI devices which
> primarily addresses two use cases. First, it provides a data driven method
> to describe hardware peripherals that are present in a PCI endpoint and
> hence can be accessed by the PCI host. An example device is Xilinx/AMD
> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
> driver -- often used in SoC platforms -- in a PCI host based system. An
> example device is Microchip LAN9662 Ethernet Controller.
> 
> This patch series consolidates previous efforts to define such an
> infrastructure:
> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> 
> Normally, the PCI core discovers PCI devices and their BARs using the
> PCI enumeration process. However, the process does not provide a way to
> discover the hardware peripherals that are present in a PCI device, and
> which can be accessed through the PCI BARs. Also, the enumeration process
> does not provide a way to associate MSI-X vectors of a PCI device with the
> hardware peripherals that are present in the device. PCI device drivers
> often use header files to describe the hardware peripherals and their
> resources as there is no standard data driven way to do so. This patch
> series proposes to use flattened device tree blob to describe the
> peripherals in a data driven way. Based on previous discussion, using
> device tree overlay is the best way to unflatten the blob and populate
> platform devices. To use device tree overlay, there are three obvious
> problems that need to be resolved.

Hi Lizhi,

We all *love* "have you thought about xxx" questions but I would really like to
get your thoughts on this. An approach to this problem that I have seen in
various devices is to emulate a virtual pcie switch, and expose the "sub
devices" behind that. That way you can carve up the BAR space, each device has
its own config space and mapping of MSI-X vector to device becomes clear. This
approach also integrates well with other kernel infrastructure (IOMMU, hotplug).

This is certainly possible on reprogrammable devices but requires some more
FPGA resources - though I don't believe the added utilization would be
significant. What do you think of this kind of solution?

Jeremi

> 
> First, we need to create a base tree for non-DT system such as x86_64. A
> patch series has been submitted for this:
> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
> 
> Second, a device tree node corresponding to the PCI endpoint is required
> for overlaying the flattened device tree blob for that PCI endpoint.
> Because PCI is a self-discoverable bus, a device tree node is usually not
> created for PCI devices. This series adds support to generate a device
> tree node for a PCI device which advertises itself using PCI quirks
> infrastructure.
> 
> Third, we need to generate device tree nodes for PCI bridges since a child
> PCI endpoint may choose to have a device tree node created.
> 
> This patch series is made up of two patches.
> 
> The first patch is adding OF interface to allocate an OF node. It is copied
> from:
> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
> 
> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
> is turned on, the kernel will generate device tree nodes for all PCI
> bridges unconditionally. The patch also shows how to use the PCI quirks
> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
> a device. Specifically, the patch generates a device tree node for Xilinx
> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
> have any property. Future patches will add the necessary properties.
> 
> Clément Léger (1):
>   of: dynamic: add of_node_alloc()
> 
> Lizhi Hou (1):
>   pci: create device tree node for selected devices
> 
>  drivers/of/dynamic.c        |  50 +++++++++++++----
>  drivers/pci/Kconfig         |  11 ++++
>  drivers/pci/bus.c           |   2 +
>  drivers/pci/msi/irqdomain.c |   6 +-
>  drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c    |   3 +-
>  drivers/pci/pci.h           |  16 ++++++
>  drivers/pci/quirks.c        |  11 ++++
>  drivers/pci/remove.c        |   1 +
>  include/linux/of.h          |   7 +++
>  10 files changed, 200 insertions(+), 13 deletions(-)
> 
> -- 
> 2.27.0
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Rob Herring]

On Wed, Sep 14, 2022 at 8:35 AM Jeremi Piotrowski
<jpiotrowski@linux.microsoft.com> wrote:
>
> On Mon, Aug 29, 2022 at 02:43:35PM -0700, Lizhi Hou wrote:
> > This patch series introduces OF overlay support for PCI devices which
> > primarily addresses two use cases. First, it provides a data driven method
> > to describe hardware peripherals that are present in a PCI endpoint and
> > hence can be accessed by the PCI host. An example device is Xilinx/AMD
> > Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
> > driver -- often used in SoC platforms -- in a PCI host based system. An
> > example device is Microchip LAN9662 Ethernet Controller.
> >
> > This patch series consolidates previous efforts to define such an
> > infrastructure:
> > https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
> > https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> >
> > Normally, the PCI core discovers PCI devices and their BARs using the
> > PCI enumeration process. However, the process does not provide a way to
> > discover the hardware peripherals that are present in a PCI device, and
> > which can be accessed through the PCI BARs. Also, the enumeration process
> > does not provide a way to associate MSI-X vectors of a PCI device with the
> > hardware peripherals that are present in the device. PCI device drivers
> > often use header files to describe the hardware peripherals and their
> > resources as there is no standard data driven way to do so. This patch
> > series proposes to use flattened device tree blob to describe the
> > peripherals in a data driven way. Based on previous discussion, using
> > device tree overlay is the best way to unflatten the blob and populate
> > platform devices. To use device tree overlay, there are three obvious
> > problems that need to be resolved.
>
> Hi Lizhi,
>
> We all *love* "have you thought about xxx" questions but I would really like to
> get your thoughts on this. An approach to this problem that I have seen in
> various devices is to emulate a virtual pcie switch, and expose the "sub
> devices" behind that. That way you can carve up the BAR space, each device has
> its own config space and mapping of MSI-X vector to device becomes clear. This
> approach also integrates well with other kernel infrastructure (IOMMU, hotplug).
>
> This is certainly possible on reprogrammable devices but requires some more
> FPGA resources - though I don't believe the added utilization would be
> significant. What do you think of this kind of solution?

It would integrate easily unless the sub-devices you are targeting
have drivers already which are not PCI drivers. Sure, we could add PCI
support to them, but that could be a lot of churn.

There are also usecases where we don't get to change the h/w.

Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 8/29/22 16:43, Lizhi Hou wrote:
> This patch series introduces OF overlay support for PCI devices which
> primarily addresses two use cases. First, it provides a data driven method
> to describe hardware peripherals that are present in a PCI endpoint and
> hence can be accessed by the PCI host. An example device is Xilinx/AMD
> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
> driver -- often used in SoC platforms -- in a PCI host based system. An
> example device is Microchip LAN9662 Ethernet Controller.
> 
> This patch series consolidates previous efforts to define such an
> infrastructure:
> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> 
> Normally, the PCI core discovers PCI devices and their BARs using the
> PCI enumeration process. However, the process does not provide a way to
> discover the hardware peripherals that are present in a PCI device, and
> which can be accessed through the PCI BARs. Also, the enumeration process
> does not provide a way to associate MSI-X vectors of a PCI device with the
> hardware peripherals that are present in the device. PCI device drivers
> often use header files to describe the hardware peripherals and their
> resources as there is no standard data driven way to do so. This patch> series proposes to use flattened device tree blob to describe the
> peripherals in a data driven way.

> Based on previous discussion, using
> device tree overlay is the best way to unflatten the blob and populate
> platform devices.

I still do not agree with this statement.  The device tree overlay
implementation is very incomplete and should not be used until it
becomes more complete.  No need to debate this right now, but I don't want
to let this go unchallenged.

If there is no base system device tree on an ACPI based system, then I
am not convinced that a mixed ACPI / device tree implementation is
good architecture.  I might be more supportive of using a device tree
description of a PCI device in a detached device tree (not linked to
the system device tree, but instead freestanding).  Unfortunately the
device tree functions assume a single system devicetree, with no concept
of a freestanding tree (eg, if a NULL device tree node is provided to
a function or macro, it often defaults to the root of the system device
tree).  I need to go look at whether the flag OF_DETACHED handles this,
or if it could be leveraged to do so.

> To use device tree overlay, there are three obvious
> problems that need to be resolved.
> 
> First, we need to create a base tree for non-DT system such as x86_64. A
> patch series has been submitted for this:
> https://lore.kernel.org/lkml/20220624034327.2542112-1-frowand.list@gmail.com/
> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
> 
> Second, a device tree node corresponding to the PCI endpoint is required
> for overlaying the flattened device tree blob for that PCI endpoint.
> Because PCI is a self-discoverable bus, a device tree node is usually not
> created for PCI devices. This series adds support to generate a device
> tree node for a PCI device which advertises itself using PCI quirks
> infrastructure.
> 
> Third, we need to generate device tree nodes for PCI bridges since a child
> PCI endpoint may choose to have a device tree node created.
> 
> This patch series is made up of two patches.
> 
> The first patch is adding OF interface to allocate an OF node. It is copied
> from:
> https://lore.kernel.org/lkml/20220620104123.341054-5-clement.leger@bootlin.com/
> 
> The second patch introduces a kernel option, CONFIG_PCI_OF. When the option
> is turned on, the kernel will generate device tree nodes for all PCI
> bridges unconditionally. The patch also shows how to use the PCI quirks
> infrastructure, DECLARE_PCI_FIXUP_FINAL to generate a device tree node for
> a device. Specifically, the patch generates a device tree node for Xilinx
> Alveo U50 PCIe accelerator device. The generated device tree nodes do not
> have any property. Future patches will add the necessary properties.
> 
> Clément Léger (1):
>   of: dynamic: add of_node_alloc()
> 
> Lizhi Hou (1):
>   pci: create device tree node for selected devices
> 
>  drivers/of/dynamic.c        |  50 +++++++++++++----
>  drivers/pci/Kconfig         |  11 ++++
>  drivers/pci/bus.c           |   2 +
>  drivers/pci/msi/irqdomain.c |   6 +-
>  drivers/pci/of.c            | 106 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci-driver.c    |   3 +-
>  drivers/pci/pci.h           |  16 ++++++
>  drivers/pci/quirks.c        |  11 ++++
>  drivers/pci/remove.c        |   1 +
>  include/linux/of.h          |   7 +++
>  10 files changed, 200 insertions(+), 13 deletions(-)
> 

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Rob Herring]

On Fri, Sep 16, 2022 at 6:15 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 8/29/22 16:43, Lizhi Hou wrote:
> > This patch series introduces OF overlay support for PCI devices which
> > primarily addresses two use cases. First, it provides a data driven method
> > to describe hardware peripherals that are present in a PCI endpoint and
> > hence can be accessed by the PCI host. An example device is Xilinx/AMD
> > Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
> > driver -- often used in SoC platforms -- in a PCI host based system. An
> > example device is Microchip LAN9662 Ethernet Controller.
> >
> > This patch series consolidates previous efforts to define such an
> > infrastructure:
> > https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
> > https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> >
> > Normally, the PCI core discovers PCI devices and their BARs using the
> > PCI enumeration process. However, the process does not provide a way to
> > discover the hardware peripherals that are present in a PCI device, and
> > which can be accessed through the PCI BARs. Also, the enumeration process
> > does not provide a way to associate MSI-X vectors of a PCI device with the
> > hardware peripherals that are present in the device. PCI device drivers
> > often use header files to describe the hardware peripherals and their
> > resources as there is no standard data driven way to do so. This patch> series proposes to use flattened device tree blob to describe the
> > peripherals in a data driven way.
>
> > Based on previous discussion, using
> > device tree overlay is the best way to unflatten the blob and populate
> > platform devices.
>
> I still do not agree with this statement.  The device tree overlay
> implementation is very incomplete and should not be used until it
> becomes more complete.  No need to debate this right now, but I don't want
> to let this go unchallenged.

Then we should remove overlay support. The only way it becomes more
complete is having actual users.

But really, whether this is the right solution to the problem is
independent of the state of kernel overlay support.

> If there is no base system device tree on an ACPI based system, then I
> am not convinced that a mixed ACPI / device tree implementation is
> good architecture.

Most/all of this series is needed for a DT system in which the PCI
devices are not populated in the DT.

>  I might be more supportive of using a device tree
> description of a PCI device in a detached device tree (not linked to
> the system device tree, but instead freestanding).  Unfortunately the
> device tree functions assume a single system devicetree, with no concept
> of a freestanding tree (eg, if a NULL device tree node is provided to
> a function or macro, it often defaults to the root of the system device
> tree).  I need to go look at whether the flag OF_DETACHED handles this,
> or if it could be leveraged to do so.

Instead of worrying about a theoretical problem, we should see if
there is an actual problem for a user.

I'm not so worried about DT functions themselves, but places which
have 'if ACPI ... else (DT) ...' paths.

Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Sonal Santan]

On 9/26/22 15:44, Rob Herring wrote:
> On Fri, Sep 16, 2022 at 6:15 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 8/29/22 16:43, Lizhi Hou wrote:
>>> This patch series introduces OF overlay support for PCI devices which
>>> primarily addresses two use cases. First, it provides a data driven method
>>> to describe hardware peripherals that are present in a PCI endpoint and
>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>> example device is Microchip LAN9662 Ethernet Controller.
>>>
>>> This patch series consolidates previous efforts to define such an
>>> infrastructure:
>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>
>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>> PCI enumeration process. However, the process does not provide a way to
>>> discover the hardware peripherals that are present in a PCI device, and
>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>> hardware peripherals that are present in the device. PCI device drivers
>>> often use header files to describe the hardware peripherals and their
>>> resources as there is no standard data driven way to do so. This patch> series proposes to use flattened device tree blob to describe the
>>> peripherals in a data driven way.
>>
>>> Based on previous discussion, using
>>> device tree overlay is the best way to unflatten the blob and populate
>>> platform devices.
>>
>> I still do not agree with this statement.  The device tree overlay
>> implementation is very incomplete and should not be used until it
>> becomes more complete.  No need to debate this right now, but I don't want
>> to let this go unchallenged.
> 
> Then we should remove overlay support. The only way it becomes more
> complete is having actual users.
> 
> But really, whether this is the right solution to the problem is
> independent of the state of kernel overlay support.
> 
>> If there is no base system device tree on an ACPI based system, then I
>> am not convinced that a mixed ACPI / device tree implementation is
>> good architecture.
> 
> Most/all of this series is needed for a DT system in which the PCI
> devices are not populated in the DT.
> 
>>   I might be more supportive of using a device tree
>> description of a PCI device in a detached device tree (not linked to
>> the system device tree, but instead freestanding).  Unfortunately the
>> device tree functions assume a single system devicetree, with no concept
>> of a freestanding tree (eg, if a NULL device tree node is provided to
>> a function or macro, it often defaults to the root of the system device
>> tree).  I need to go look at whether the flag OF_DETACHED handles this,
>> or if it could be leveraged to do so.
> 
> Instead of worrying about a theoretical problem, we should see if
> there is an actual problem for a user.
> 
> I'm not so worried about DT functions themselves, but places which
> have 'if ACPI ... else (DT) ...' paths.
>

Bringing this thread back into focus. Any thoughts on how to move forward?

-Sonal

> Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Rob Herring]

On Fri, Sep 30, 2022 at 2:29 PM Sonal Santan <sonal.santan@amd.com> wrote:
>
> On 9/26/22 15:44, Rob Herring wrote:
> > On Fri, Sep 16, 2022 at 6:15 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 8/29/22 16:43, Lizhi Hou wrote:
> >>> This patch series introduces OF overlay support for PCI devices which
> >>> primarily addresses two use cases. First, it provides a data driven method
> >>> to describe hardware peripherals that are present in a PCI endpoint and
> >>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
> >>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
> >>> driver -- often used in SoC platforms -- in a PCI host based system. An
> >>> example device is Microchip LAN9662 Ethernet Controller.
> >>>
> >>> This patch series consolidates previous efforts to define such an
> >>> infrastructure:
> >>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
> >>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
> >>>
> >>> Normally, the PCI core discovers PCI devices and their BARs using the
> >>> PCI enumeration process. However, the process does not provide a way to
> >>> discover the hardware peripherals that are present in a PCI device, and
> >>> which can be accessed through the PCI BARs. Also, the enumeration process
> >>> does not provide a way to associate MSI-X vectors of a PCI device with the
> >>> hardware peripherals that are present in the device. PCI device drivers
> >>> often use header files to describe the hardware peripherals and their
> >>> resources as there is no standard data driven way to do so. This patch> series proposes to use flattened device tree blob to describe the
> >>> peripherals in a data driven way.
> >>
> >>> Based on previous discussion, using
> >>> device tree overlay is the best way to unflatten the blob and populate
> >>> platform devices.
> >>
> >> I still do not agree with this statement.  The device tree overlay
> >> implementation is very incomplete and should not be used until it
> >> becomes more complete.  No need to debate this right now, but I don't want
> >> to let this go unchallenged.
> >
> > Then we should remove overlay support. The only way it becomes more
> > complete is having actual users.
> >
> > But really, whether this is the right solution to the problem is
> > independent of the state of kernel overlay support.
> >
> >> If there is no base system device tree on an ACPI based system, then I
> >> am not convinced that a mixed ACPI / device tree implementation is
> >> good architecture.
> >
> > Most/all of this series is needed for a DT system in which the PCI
> > devices are not populated in the DT.
> >
> >>   I might be more supportive of using a device tree
> >> description of a PCI device in a detached device tree (not linked to
> >> the system device tree, but instead freestanding).  Unfortunately the
> >> device tree functions assume a single system devicetree, with no concept
> >> of a freestanding tree (eg, if a NULL device tree node is provided to
> >> a function or macro, it often defaults to the root of the system device
> >> tree).  I need to go look at whether the flag OF_DETACHED handles this,
> >> or if it could be leveraged to do so.
> >
> > Instead of worrying about a theoretical problem, we should see if
> > there is an actual problem for a user.
> >
> > I'm not so worried about DT functions themselves, but places which
> > have 'if ACPI ... else (DT) ...' paths.
> >
>
> Bringing this thread back into focus. Any thoughts on how to move forward?

Reviewers raise concerns/issues and the submitters work to address
them or explain why they aren't an issue. The submitter has to push
things forward. That's how the process works.

As I noted, much of this is needed on a DT system with PCI device not
described in DT. So you could split out any ACPI system support to
avoid that concern for example. Enabling others to exercise these
patches may help too. Perhaps use QEMU to create some imaginary
device.

Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Sonal Santan]

On 10/6/22 08:10, Rob Herring wrote:
> On Fri, Sep 30, 2022 at 2:29 PM Sonal Santan <sonal.santan@amd.com> wrote:
>>
>> On 9/26/22 15:44, Rob Herring wrote:
>>> On Fri, Sep 16, 2022 at 6:15 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 8/29/22 16:43, Lizhi Hou wrote:
>>>>> This patch series introduces OF overlay support for PCI devices which
>>>>> primarily addresses two use cases. First, it provides a data driven method
>>>>> to describe hardware peripherals that are present in a PCI endpoint and
>>>>> hence can be accessed by the PCI host. An example device is Xilinx/AMD
>>>>> Alveo PCIe accelerators. Second, it allows reuse of a OF compatible
>>>>> driver -- often used in SoC platforms -- in a PCI host based system. An
>>>>> example device is Microchip LAN9662 Ethernet Controller.
>>>>>
>>>>> This patch series consolidates previous efforts to define such an
>>>>> infrastructure:
>>>>> https://lore.kernel.org/lkml/20220305052304.726050-1-lizhi.hou@xilinx.com/
>>>>> https://lore.kernel.org/lkml/20220427094502.456111-1-clement.leger@bootlin.com/
>>>>>
>>>>> Normally, the PCI core discovers PCI devices and their BARs using the
>>>>> PCI enumeration process. However, the process does not provide a way to
>>>>> discover the hardware peripherals that are present in a PCI device, and
>>>>> which can be accessed through the PCI BARs. Also, the enumeration process
>>>>> does not provide a way to associate MSI-X vectors of a PCI device with the
>>>>> hardware peripherals that are present in the device. PCI device drivers
>>>>> often use header files to describe the hardware peripherals and their
>>>>> resources as there is no standard data driven way to do so. This patch> series proposes to use flattened device tree blob to describe the
>>>>> peripherals in a data driven way.
>>>>
>>>>> Based on previous discussion, using
>>>>> device tree overlay is the best way to unflatten the blob and populate
>>>>> platform devices.
>>>>
>>>> I still do not agree with this statement.  The device tree overlay
>>>> implementation is very incomplete and should not be used until it
>>>> becomes more complete.  No need to debate this right now, but I don't want
>>>> to let this go unchallenged.
>>>
>>> Then we should remove overlay support. The only way it becomes more
>>> complete is having actual users.
>>>
>>> But really, whether this is the right solution to the problem is
>>> independent of the state of kernel overlay support.
>>>
>>>> If there is no base system device tree on an ACPI based system, then I
>>>> am not convinced that a mixed ACPI / device tree implementation is
>>>> good architecture.
>>>
>>> Most/all of this series is needed for a DT system in which the PCI
>>> devices are not populated in the DT.
>>>
>>>>    I might be more supportive of using a device tree
>>>> description of a PCI device in a detached device tree (not linked to
>>>> the system device tree, but instead freestanding).  Unfortunately the
>>>> device tree functions assume a single system devicetree, with no concept
>>>> of a freestanding tree (eg, if a NULL device tree node is provided to
>>>> a function or macro, it often defaults to the root of the system device
>>>> tree).  I need to go look at whether the flag OF_DETACHED handles this,
>>>> or if it could be leveraged to do so.
>>>
>>> Instead of worrying about a theoretical problem, we should see if
>>> there is an actual problem for a user.
>>>
>>> I'm not so worried about DT functions themselves, but places which
>>> have 'if ACPI ... else (DT) ...' paths.
>>>
>>
>> Bringing this thread back into focus. Any thoughts on how to move forward?
> 
> Reviewers raise concerns/issues and the submitters work to address
> them or explain why they aren't an issue. The submitter has to push
> things forward. That's how the process works.
> 
We are working on updating the patch set to address the feedback. The 
design is still based on device tree overlay infrastructure.

> As I noted, much of this is needed on a DT system with PCI device not
> described in DT. So you could split out any ACPI system support to
> avoid that concern for example. Enabling others to exercise these
> patches may help too. Perhaps use QEMU to create some imaginary
> device.
To verify this patch set, in addition to a x86_64/ACPI based system, we 
also have an AARCH64/DT QEMU setup where we have attached a physical 
Alveo device. We haven't run into any ACPI or DTO issues so far.

Perhaps we can introduce this feature in a phased manner where we first 
enable DT based platforms and then enable ACPI based platforms?

-Sonal
> 
> Rob

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Clément Léger]

Le Fri, 7 Oct 2022 15:45:17 -0700,
Sonal Santan <sonal.santan@amd.com> a écrit :

> >> Bringing this thread back into focus. Any thoughts on how to move forward?  
> > 
> > Reviewers raise concerns/issues and the submitters work to address
> > them or explain why they aren't an issue. The submitter has to push
> > things forward. That's how the process works.
> >   

Up to now, there does not seems to be a better solution to this
problem in term of maintainability, reusability and ease of use.

Again, patching the pre-boot description (ACPI or DT) is not an option,
the user is entitled to plug the card in whatever PCI slot he wants.
This is either probbly not possible and ACPI based system and would
require a difficult setup to even try to achieve that. This would also
result in two different description to support the same device.

> We are working on updating the patch set to address the feedback. The 
> design is still based on device tree overlay infrastructure.

Agreed, moreover saying that "the overlay support is fragile" seems to
be a shortcut to do nothing and move all the support outside of the
kernel. If buggy, then it would be better to fix this support (if there
are real problems encountered with it) or kill it by removing it
entirely if not usable (option 1 would of course be preferred).

> 
> > As I noted, much of this is needed on a DT system with PCI device not
> > described in DT. So you could split out any ACPI system support to
> > avoid that concern for example. Enabling others to exercise these
> > patches may help too. Perhaps use QEMU to create some imaginary
> > device.  
> To verify this patch set, in addition to a x86_64/ACPI based system, we 
> also have an AARCH64/DT QEMU setup where we have attached a physical 
> Alveo device. We haven't run into any ACPI or DTO issues so far.

I've been able to use the same patch set with a X86 QEMU system by
attaching my physical PCI card to it. No issues were encountered
(although the usage was rather limited). Gaining some users of this
support would allow to gather more feedback.

> 
> Perhaps we can introduce this feature in a phased manner where we first 
> enable DT based platforms and then enable ACPI based platforms?
> 
> -Sonal
> > 
> > Rob  


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH RFC 0/2] Generate device tree node for pci devices

[Frank Rowand]

On 10/10/22 03:58, Clément Léger wrote:
> Le Fri, 7 Oct 2022 15:45:17 -0700,
> Sonal Santan <sonal.santan@amd.com> a écrit :
> 
>>>> Bringing this thread back into focus. Any thoughts on how to move forward?  
>>>
>>> Reviewers raise concerns/issues and the submitters work to address
>>> them or explain why they aren't an issue. The submitter has to push
>>> things forward. That's how the process works.
>>>   
> 
> Up to now, there does not seems to be a better solution to this
> problem in term of maintainability, reusability and ease of use.
> 
> Again, patching the pre-boot description (ACPI or DT) is not an option,
> the user is entitled to plug the card in whatever PCI slot he wants.
> This is either probbly not possible and ACPI based system and would
> require a difficult setup to even try to achieve that. This would also
> result in two different description to support the same device.
> 
>> We are working on updating the patch set to address the feedback. The 
>> design is still based on device tree overlay infrastructure.
> 
> Agreed, moreover saying that "the overlay support is fragile" seems to
> be a shortcut to do nothing and move all the support outside of the
> kernel. If buggy, then it would be better to fix this support (if there
> are real problems encountered with it) or kill it by removing it
> entirely if not usable (option 1 would of course be preferred).

"Buggy" is true, but not an adequate description.  See my other reply in
this thread a couple of minutes ago regarding "proof of concept".

Rob has suggested removing it at least a couple of times this year.

-Frank

> 
>>
>>> As I noted, much of this is needed on a DT system with PCI device not
>>> described in DT. So you could split out any ACPI system support to
>>> avoid that concern for example. Enabling others to exercise these
>>> patches may help too. Perhaps use QEMU to create some imaginary
>>> device.  
>> To verify this patch set, in addition to a x86_64/ACPI based system, we 
>> also have an AARCH64/DT QEMU setup where we have attached a physical 
>> Alveo device. We haven't run into any ACPI or DTO issues so far.
> 
> I've been able to use the same patch set with a X86 QEMU system by
> attaching my physical PCI card to it. No issues were encountered
> (although the usage was rather limited). Gaining some users of this
> support would allow to gather more feedback.
> 
>>
>> Perhaps we can introduce this feature in a phased manner where we first 
>> enable DT based platforms and then enable ACPI based platforms?
>>
>> -Sonal
>>>
>>> Rob  
> 
>