[PATCH 0/3] add dynamic PCI device of_node creation for overlay

[PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Clément Léger]

This series adds foundation work to support the lan9662 PCIe card. This
card is meant to be used an ethernet switch with 2 x RJ45 ports and
2 x 2.5G SFPs. The lan966x SoCs can be used in two different ways:

 - It can run Linux by itself, on ARM64 cores included in the SoC. This
   use-case of the lan966x is currently being upstreamed, using a
   traditional Device Tree representation of the lan996x HW blocks [1]
   A number of drivers for the different IPs of the SoC have already
   been merged in upstream Linux.

 - It can be used as a PCIe endpoint, connected to a separate platform
   that acts as the PCIe root complex. In this case, all the devices
   that are embedded on this SoC are exposed through PCIe BARs and the
   ARM64 cores of the SoC are not used. Since this is a PCIe card, it
   can be plugged on any platform, of any architecture supporting PCIe.

The problem that arose is that we want to reuse all the existing OF
compatible drivers that are used when in SoC mode to instantiate the
PCI device when in PCIe endpoint mode.

A previous attempt to tackle this problem was made using fwnode [1].
However, this proved being way too invasive and it required
modifications in both subsystems and drivers to support fwnode. First
series did not lead to a consensus and multiple ideas to support this
use-case were mentioned (ACPI overlay, fwnode, device-tree overlay).
Since it only seemed that fwnode was not a totally silly idea, we
continued on this way.

However, on the series that added fwnode support to the reset subsystem,
Rob Herring mentioned the fact that OF overlay might actually be the
best way to probe PCI devices and populate platform drivers using this
overlay. He also provided a branch containing some commits that helped
to implement this idea on a x86 setup. Due to the dynamic nature of PCI
bus enumeration, some other modifications needs to be applied on the
overlay to apply it correctly. Indeed, it is necessary to modify the
target node of the fragments to apply them correctly on the PCI device
that was probed. Moreover, the 'ranges' must be set according to the
BAR addresses in order to remap devices to the correct PCI addresses.
These modifications are the located into the driver since the remapping
is something that is specific to each driver.

After modifications, this proves to be successful and a full support of
the aforementioned lan966x PCI card was added. The modifications to
support that (apply an overlay on a dynamically created PCI of_node) are
actually minimal and only touches a few places (pci/of.c). This series
contains the 3 commits that are necessary to do that:

- First commit creates the root node if not present on a x86 setup
  without a firmware provided device-tree.
- Second one dynamically creates the PCI bus/device device-tree node
  hierarchy using changeset API.
- Finally a last commit allows to apply an overlay by targeting a
  specific device-tree node.

Other problems that might be considered with this series is the fact
that CONFIG_OF is not enabled by default on x86 configuration and thus
the driver can't be used without rebuilding a complete kernel with
CONFIG_OF=y. In order to fully support this PCIe card and allow lambda
user to use this driver, it would be almost mandatory to enable
CONFIG_OF by default on such setup.

A driver using this support was added and can be seen at [3]. This
driver embeds a builtin overlay and applies it to the live tree using
of_overlay_fdt_apply_to_node(). An interrupt driver is also included and
associated to a node that is added by the overlay. The driver also
insert a specific "ranges" property based on the BAR values which allows
to remap the device-tree node to BAR addresses dynamically. This is
needed to allow applying the overlay without depending on specific
enumeration BAR addresses.

This series was tested on a x86 kernel using CONFIG_OF under a virtual
machine using PCI passthrough.

Link: [1] https://lore.kernel.org/lkml/YhQHqDJvahgriDZK@lunn.ch/t/
Link: [2] https://lore.kernel.org/lkml/20220408174841.34458529@fixe.home/T/
Link: [3] https://github.com/clementleger/linux/tree/lan966x/of_support

Clément Léger (3):
  of: always populate a root node
  PCI: of: create DT nodes for PCI devices if they do not exists
  of: overlay: add of_overlay_fdt_apply_to_node()

 drivers/of/base.c    |  16 +++-
 drivers/of/overlay.c |  21 +++--
 drivers/pci/of.c     | 184 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   |  17 +++-
 4 files changed, 224 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH 1/3] of: always populate a root node

[Clément Léger]

When enabling CONFIG_OF on a platform where of_root is not populated by
firmware, we end up without a root node. In order to apply overlays and
create subnodes of the root node, we need one. This commit creates an
empty root node if not present.

Co-developed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/base.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e7d92b67cb8a..6b8584c39f73 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -177,6 +177,19 @@ void __init of_core_init(void)
 		pr_err("failed to register existing nodes\n");
 		return;
 	}
+
+	if (!of_root) {
+		of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
+		if (!of_root) {
+			mutex_unlock(&of_mutex);
+			pr_err("failed to create root node\n");
+			return;
+		}
+
+		of_root->full_name = "/";
+		of_node_init(of_root);
+	}
+
 	for_each_of_allnodes(np) {
 		__of_attach_node_sysfs(np);
 		if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
@@ -185,8 +198,7 @@ void __init of_core_init(void)
 	mutex_unlock(&of_mutex);
 
 	/* Symlink in /proc as required by userspace ABI */
-	if (of_root)
-		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
 }
 
 static struct property *__of_find_property(const struct device_node *np,
-- 
2.34.1

[PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[Clément Léger]

In order to apply overlays to PCI device nodes, the nodes must first
exist. This commit add support to populate a skeleton tree for PCI bus
and devices. These nodes can then be used by drivers to apply overlays.

Co-developed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/pci/of.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 184 insertions(+)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index cb2e8351c2cc..f2325708726e 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -16,12 +16,194 @@
 #include "pci.h"
 
 #ifdef CONFIG_PCI
+static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
+			       const char *name, const void *value, int length)
+{
+	struct property *prop;
+	int ret = -ENOMEM;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return -ENOMEM;
+
+	prop->name = kstrdup(name, GFP_KERNEL);
+	if (!prop->name)
+		goto out_err;
+
+	if (value) {
+		prop->value = kmemdup(value, length, GFP_KERNEL);
+		if (!prop->value)
+			goto out_err;
+	} else {
+		/*
+		 * Even if the property has no value, it must be set to a
+		 * non-null value since of_get_property() is used to check
+		 * some values that might or not have a values (ranges for
+		 * instance). Moreover, when the node is released, prop->value
+		 * is kfreed so the memory must come from kmalloc.
+		 */
+		prop->value = kmalloc(1, GFP_KERNEL);
+		if (!prop->value)
+			goto out_err;
+	}
+
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	prop->length = length;
+
+	ret = of_changeset_add_property(ocs, np, prop);
+	if (!ret)
+		return 0;
+
+out_err:
+	kfree(prop->value);
+	kfree(prop->name);
+	kfree(prop);
+
+	return ret;
+}
+
+static struct device_node *of_pci_make_node(void)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return NULL;
+
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+	of_node_init(node);
+
+	return node;
+}
+
+static int of_pci_add_cells_props(struct device_node *node,
+				  struct of_changeset *cs, int n_addr_cells,
+				  int n_size_cells)
+{
+	__be32 val;
+	int ret;
+
+	ret = of_pci_add_property(cs, node, "ranges", NULL, 0);
+	if (ret)
+		return ret;
+
+	val = __cpu_to_be32(n_addr_cells);
+	ret = of_pci_add_property(cs, node, "#address-cells", &val,
+				  sizeof(__be32));
+	if (ret)
+		return ret;
+
+	val = __cpu_to_be32(n_size_cells);
+	return of_pci_add_property(cs, node, "#size-cells", &val,
+				   sizeof(__be32));
+}
+
+static int of_pci_add_pci_bus_props(struct device_node *node,
+				    struct of_changeset *cs)
+{
+	int ret;
+
+	ret = of_pci_add_property(cs, node, "device_type", "pci",
+				  strlen("pci") + 1);
+	if (ret)
+		return ret;
+
+	return of_pci_add_cells_props(node, cs, 3, 2);
+}
+
+static void of_pci_make_dev_node(struct pci_dev *dev)
+{
+	static struct of_changeset cs;
+	const char *pci_type = "dev";
+	struct device_node *node;
+	__be32 val[5] = {0};
+	int ret;
+
+	node = of_pci_make_node();
+	if (!node)
+		return;
+
+	node->parent = dev->bus->dev.of_node;
+	of_changeset_init(&cs);
+
+	if (pci_is_bridge(dev)) {
+		ret = of_pci_add_pci_bus_props(node, &cs);
+		if (ret)
+			goto changeset_destroy;
+		pci_type = "pci";
+	}
+
+	node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
+				    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+
+	val[0] = __cpu_to_be32(dev->devfn << 8);
+	val[4] = __cpu_to_be32(SZ_4K);
+	ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_attach_node(&cs, node);
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_apply(&cs);
+	if (ret)
+		goto changeset_destroy;
+
+	dev->dev.of_node = node;
+
+	return;
+
+changeset_destroy:
+	of_changeset_destroy(&cs);
+	kfree(node);
+}
+
+static struct device_node *of_pci_create_root_bus_node(struct pci_bus *bus)
+{
+	static struct of_changeset cs;
+	struct device_node *node;
+	int ret;
+
+	node = of_pci_make_node();
+	if (!node)
+		return NULL;
+
+	node->full_name = "pci";
+	node->parent = of_root;
+
+	of_changeset_init(&cs);
+	ret = of_pci_add_pci_bus_props(node, &cs);
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_attach_node(&cs, node);
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_apply(&cs);
+	if (ret)
+		goto changeset_destroy;
+
+	return node;
+
+changeset_destroy:
+	of_changeset_destroy(&cs);
+	kfree(node);
+
+	return NULL;
+}
+
 void pci_set_of_node(struct pci_dev *dev)
 {
 	if (!dev->bus->dev.of_node)
 		return;
 	dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
 						    dev->devfn);
+	if (!dev->dev.of_node)
+		of_pci_make_dev_node(dev);
 	if (dev->dev.of_node)
 		dev->dev.fwnode = &dev->dev.of_node->fwnode;
 }
@@ -39,6 +221,8 @@ void pci_set_bus_of_node(struct pci_bus *bus)
 
 	if (bus->self == NULL) {
 		node = pcibios_get_phb_of_node(bus);
+		if (!node)
+			node = of_pci_create_root_bus_node(bus);
 	} else {
 		node = of_node_get(bus->self->dev.of_node);
 		if (node && of_property_read_bool(node, "external-facing"))
-- 
2.34.1

[PATCH 3/3] of: overlay: add of_overlay_fdt_apply_to_node()

[Clément Léger]

When using a driver which load an overlay and applies it on the of_node
of device it represent, the target node isn't known in advance. This is
for example the case when applying an overlay from a PCI driver. PCI
cards might be plugged on whatever slot the user want and thus, the
target node of the overlay framgents are unknown. This function allows
to specify the node on which the overlay fragments will be applied.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/of/overlay.c | 21 +++++++++++++--------
 include/linux/of.h   | 17 +++++++++++++----
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index d80160cf34bb..4dabe1b65343 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -730,7 +730,8 @@ static struct device_node *find_target(struct device_node *info_node)
  * detected in @tree, or -ENOSPC if idr_alloc() error.
  */
 static int init_overlay_changeset(struct overlay_changeset *ovcs,
-		const void *fdt, struct device_node *tree)
+		const void *fdt, struct device_node *tree,
+		struct device_node *target)
 {
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
@@ -792,7 +793,10 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 		fragment = &fragments[cnt];
 		fragment->overlay = overlay_node;
-		fragment->target = find_target(node);
+		if (target)
+			fragment->target = target;
+		else
+			fragment->target = find_target(node);
 		if (!fragment->target) {
 			of_node_put(fragment->overlay);
 			ret = -EINVAL;
@@ -877,6 +881,7 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * @fdt:	base of memory allocated to hold the aligned FDT
  * @tree:	Expanded overlay device tree
  * @ovcs_id:	Pointer to overlay changeset id
+ * @target:	Target node to override target-path property value
  *
  * Creates and applies an overlay changeset.
  *
@@ -914,7 +919,7 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  */
 
 static int of_overlay_apply(const void *fdt, struct device_node *tree,
-		int *ovcs_id)
+		int *ovcs_id, struct device_node *target)
 {
 	struct overlay_changeset *ovcs;
 	int ret = 0, ret_revert, ret_tmp;
@@ -947,7 +952,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 	if (ret)
 		goto err_free_tree;
 
-	ret = init_overlay_changeset(ovcs, fdt, tree);
+	ret = init_overlay_changeset(ovcs, fdt, tree, target);
 	if (ret)
 		goto err_free_tree;
 
@@ -1014,8 +1019,8 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 	return ret;
 }
 
-int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
-			 int *ovcs_id)
+int of_overlay_fdt_apply_to_node(const void *overlay_fdt, u32 overlay_fdt_size,
+				 int *ovcs_id, struct device_node *target)
 {
 	void *new_fdt;
 	void *new_fdt_align;
@@ -1053,7 +1058,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 		goto out_free_new_fdt;
 	}
 
-	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
+	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id, target);
 	if (ret < 0) {
 		/*
 		 * new_fdt and overlay_root now belong to the overlay
@@ -1072,7 +1077,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 out:
 	return ret;
 }
-EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
+EXPORT_SYMBOL_GPL(of_overlay_fdt_apply_to_node);
 
 /*
  * Find @np in @tree.
diff --git a/include/linux/of.h b/include/linux/of.h
index 2dc77430a91a..4df653606936 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1566,8 +1566,8 @@ struct of_overlay_notify_data {
 
 #ifdef CONFIG_OF_OVERLAY
 
-int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
-			 int *ovcs_id);
+int of_overlay_fdt_apply_to_node(const void *overlay_fdt, u32 overlay_fdt_size,
+				 int *ovcs_id, struct device_node *target);
 int of_overlay_remove(int *ovcs_id);
 int of_overlay_remove_all(void);
 
@@ -1576,8 +1576,10 @@ int of_overlay_notifier_unregister(struct notifier_block *nb);
 
 #else
 
-static inline int of_overlay_fdt_apply(void *overlay_fdt, u32 overlay_fdt_size,
-				       int *ovcs_id)
+static inline int of_overlay_fdt_apply_to_node(const void *overlay_fdt,
+					       u32 overlay_fdt_size,
+					       int *ovcs_id,
+					       struct device_node *target)
 {
 	return -ENOTSUPP;
 }
@@ -1604,4 +1606,11 @@ static inline int of_overlay_notifier_unregister(struct notifier_block *nb)
 
 #endif
 
+static inline int of_overlay_fdt_apply(const void *overlay_fdt,
+				       u32 overlay_fdt_size, int *ovcs_id)
+{
+	return of_overlay_fdt_apply_to_node(overlay_fdt, overlay_fdt_size,
+					    ovcs_id, NULL);
+}
+
 #endif /* _LINUX_OF_H */
-- 
2.34.1

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[kernel test robot]

Hi "Clément,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.18-rc4 next-20220427]
[cannot apply to robh/for-next]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: microblaze-randconfig-r005-20220427 (https://download.01.org/0day-ci/archive/20220428/202204280121.ObD8tBkn-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.3.0
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/787f8567f04db060522e198fbdc55a805e99b922
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
        git checkout 787f8567f04db060522e198fbdc55a805e99b922
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/pci/

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

All errors (new ones prefixed by >>):

   drivers/pci/of.c: In function 'of_pci_add_property':
>> drivers/pci/of.c:50:9: error: implicit declaration of function 'of_property_set_flag'; did you mean 'of_node_set_flag'? [-Werror=implicit-function-declaration]
      50 |         of_property_set_flag(prop, OF_DYNAMIC);
         |         ^~~~~~~~~~~~~~~~~~~~
         |         of_node_set_flag
>> drivers/pci/of.c:54:15: error: implicit declaration of function 'of_changeset_add_property'; did you mean 'of_pci_add_property'? [-Werror=implicit-function-declaration]
      54 |         ret = of_changeset_add_property(ocs, np, prop);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~
         |               of_pci_add_property
   drivers/pci/of.c: In function 'of_pci_make_dev_node':
>> drivers/pci/of.c:129:9: error: implicit declaration of function 'of_changeset_init' [-Werror=implicit-function-declaration]
     129 |         of_changeset_init(&cs);
         |         ^~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:147:15: error: implicit declaration of function 'of_changeset_attach_node' [-Werror=implicit-function-declaration]
     147 |         ret = of_changeset_attach_node(&cs, node);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:151:15: error: implicit declaration of function 'of_changeset_apply' [-Werror=implicit-function-declaration]
     151 |         ret = of_changeset_apply(&cs);
         |               ^~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:160:9: error: implicit declaration of function 'of_changeset_destroy' [-Werror=implicit-function-declaration]
     160 |         of_changeset_destroy(&cs);
         |         ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +50 drivers/pci/of.c

    17	
    18	#ifdef CONFIG_PCI
    19	static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
    20				       const char *name, const void *value, int length)
    21	{
    22		struct property *prop;
    23		int ret = -ENOMEM;
    24	
    25		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
    26		if (!prop)
    27			return -ENOMEM;
    28	
    29		prop->name = kstrdup(name, GFP_KERNEL);
    30		if (!prop->name)
    31			goto out_err;
    32	
    33		if (value) {
    34			prop->value = kmemdup(value, length, GFP_KERNEL);
    35			if (!prop->value)
    36				goto out_err;
    37		} else {
    38			/*
    39			 * Even if the property has no value, it must be set to a
    40			 * non-null value since of_get_property() is used to check
    41			 * some values that might or not have a values (ranges for
    42			 * instance). Moreover, when the node is released, prop->value
    43			 * is kfreed so the memory must come from kmalloc.
    44			 */
    45			prop->value = kmalloc(1, GFP_KERNEL);
    46			if (!prop->value)
    47				goto out_err;
    48		}
    49	
  > 50		of_property_set_flag(prop, OF_DYNAMIC);
    51	
    52		prop->length = length;
    53	
  > 54		ret = of_changeset_add_property(ocs, np, prop);
    55		if (!ret)
    56			return 0;
    57	
    58	out_err:
    59		kfree(prop->value);
    60		kfree(prop->name);
    61		kfree(prop);
    62	
    63		return ret;
    64	}
    65	
    66	static struct device_node *of_pci_make_node(void)
    67	{
    68		struct device_node *node;
    69	
    70		node = kzalloc(sizeof(*node), GFP_KERNEL);
    71		if (!node)
    72			return NULL;
    73	
    74		of_node_set_flag(node, OF_DYNAMIC);
    75		of_node_set_flag(node, OF_DETACHED);
    76		of_node_init(node);
    77	
    78		return node;
    79	}
    80	
    81	static int of_pci_add_cells_props(struct device_node *node,
    82					  struct of_changeset *cs, int n_addr_cells,
    83					  int n_size_cells)
    84	{
    85		__be32 val;
    86		int ret;
    87	
    88		ret = of_pci_add_property(cs, node, "ranges", NULL, 0);
    89		if (ret)
    90			return ret;
    91	
    92		val = __cpu_to_be32(n_addr_cells);
    93		ret = of_pci_add_property(cs, node, "#address-cells", &val,
    94					  sizeof(__be32));
    95		if (ret)
    96			return ret;
    97	
    98		val = __cpu_to_be32(n_size_cells);
    99		return of_pci_add_property(cs, node, "#size-cells", &val,
   100					   sizeof(__be32));
   101	}
   102	
   103	static int of_pci_add_pci_bus_props(struct device_node *node,
   104					    struct of_changeset *cs)
   105	{
   106		int ret;
   107	
   108		ret = of_pci_add_property(cs, node, "device_type", "pci",
   109					  strlen("pci") + 1);
   110		if (ret)
   111			return ret;
   112	
   113		return of_pci_add_cells_props(node, cs, 3, 2);
   114	}
   115	
   116	static void of_pci_make_dev_node(struct pci_dev *dev)
   117	{
   118		static struct of_changeset cs;
   119		const char *pci_type = "dev";
   120		struct device_node *node;
   121		__be32 val[5] = {0};
   122		int ret;
   123	
   124		node = of_pci_make_node();
   125		if (!node)
   126			return;
   127	
   128		node->parent = dev->bus->dev.of_node;
 > 129		of_changeset_init(&cs);
   130	
   131		if (pci_is_bridge(dev)) {
   132			ret = of_pci_add_pci_bus_props(node, &cs);
   133			if (ret)
   134				goto changeset_destroy;
   135			pci_type = "pci";
   136		}
   137	
   138		node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
   139					    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
   140	
   141		val[0] = __cpu_to_be32(dev->devfn << 8);
   142		val[4] = __cpu_to_be32(SZ_4K);
   143		ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
   144		if (ret)
   145			goto changeset_destroy;
   146	
 > 147		ret = of_changeset_attach_node(&cs, node);
   148		if (ret)
   149			goto changeset_destroy;
   150	
 > 151		ret = of_changeset_apply(&cs);
   152		if (ret)
   153			goto changeset_destroy;
   154	
   155		dev->dev.of_node = node;
   156	
   157		return;
   158	
   159	changeset_destroy:
 > 160		of_changeset_destroy(&cs);
   161		kfree(node);
   162	}
   163	

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

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[kernel test robot]

Hi "Clément,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.18-rc4 next-20220427]
[cannot apply to robh/for-next]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: sparc64-buildonly-randconfig-r003-20220427 (https://download.01.org/0day-ci/archive/20220428/202204280126.T5VOPZdN-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 11.3.0
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/787f8567f04db060522e198fbdc55a805e99b922
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
        git checkout 787f8567f04db060522e198fbdc55a805e99b922
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/pci/

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

All errors (new ones prefixed by >>):

   drivers/pci/of.c: In function 'of_pci_add_property':
>> drivers/pci/of.c:54:15: error: implicit declaration of function 'of_changeset_add_property'; did you mean 'of_pci_add_property'? [-Werror=implicit-function-declaration]
      54 |         ret = of_changeset_add_property(ocs, np, prop);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~
         |               of_pci_add_property
   drivers/pci/of.c: In function 'of_pci_make_dev_node':
>> drivers/pci/of.c:129:9: error: implicit declaration of function 'of_changeset_init' [-Werror=implicit-function-declaration]
     129 |         of_changeset_init(&cs);
         |         ^~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:147:15: error: implicit declaration of function 'of_changeset_attach_node' [-Werror=implicit-function-declaration]
     147 |         ret = of_changeset_attach_node(&cs, node);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:151:15: error: implicit declaration of function 'of_changeset_apply' [-Werror=implicit-function-declaration]
     151 |         ret = of_changeset_apply(&cs);
         |               ^~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:160:9: error: implicit declaration of function 'of_changeset_destroy' [-Werror=implicit-function-declaration]
     160 |         of_changeset_destroy(&cs);
         |         ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +54 drivers/pci/of.c

    17	
    18	#ifdef CONFIG_PCI
    19	static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
    20				       const char *name, const void *value, int length)
    21	{
    22		struct property *prop;
    23		int ret = -ENOMEM;
    24	
    25		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
    26		if (!prop)
    27			return -ENOMEM;
    28	
    29		prop->name = kstrdup(name, GFP_KERNEL);
    30		if (!prop->name)
    31			goto out_err;
    32	
    33		if (value) {
    34			prop->value = kmemdup(value, length, GFP_KERNEL);
    35			if (!prop->value)
    36				goto out_err;
    37		} else {
    38			/*
    39			 * Even if the property has no value, it must be set to a
    40			 * non-null value since of_get_property() is used to check
    41			 * some values that might or not have a values (ranges for
    42			 * instance). Moreover, when the node is released, prop->value
    43			 * is kfreed so the memory must come from kmalloc.
    44			 */
    45			prop->value = kmalloc(1, GFP_KERNEL);
    46			if (!prop->value)
    47				goto out_err;
    48		}
    49	
    50		of_property_set_flag(prop, OF_DYNAMIC);
    51	
    52		prop->length = length;
    53	
  > 54		ret = of_changeset_add_property(ocs, np, prop);
    55		if (!ret)
    56			return 0;
    57	
    58	out_err:
    59		kfree(prop->value);
    60		kfree(prop->name);
    61		kfree(prop);
    62	
    63		return ret;
    64	}
    65	
    66	static struct device_node *of_pci_make_node(void)
    67	{
    68		struct device_node *node;
    69	
    70		node = kzalloc(sizeof(*node), GFP_KERNEL);
    71		if (!node)
    72			return NULL;
    73	
    74		of_node_set_flag(node, OF_DYNAMIC);
    75		of_node_set_flag(node, OF_DETACHED);
    76		of_node_init(node);
    77	
    78		return node;
    79	}
    80	
    81	static int of_pci_add_cells_props(struct device_node *node,
    82					  struct of_changeset *cs, int n_addr_cells,
    83					  int n_size_cells)
    84	{
    85		__be32 val;
    86		int ret;
    87	
    88		ret = of_pci_add_property(cs, node, "ranges", NULL, 0);
    89		if (ret)
    90			return ret;
    91	
    92		val = __cpu_to_be32(n_addr_cells);
    93		ret = of_pci_add_property(cs, node, "#address-cells", &val,
    94					  sizeof(__be32));
    95		if (ret)
    96			return ret;
    97	
    98		val = __cpu_to_be32(n_size_cells);
    99		return of_pci_add_property(cs, node, "#size-cells", &val,
   100					   sizeof(__be32));
   101	}
   102	
   103	static int of_pci_add_pci_bus_props(struct device_node *node,
   104					    struct of_changeset *cs)
   105	{
   106		int ret;
   107	
   108		ret = of_pci_add_property(cs, node, "device_type", "pci",
   109					  strlen("pci") + 1);
   110		if (ret)
   111			return ret;
   112	
   113		return of_pci_add_cells_props(node, cs, 3, 2);
   114	}
   115	
   116	static void of_pci_make_dev_node(struct pci_dev *dev)
   117	{
   118		static struct of_changeset cs;
   119		const char *pci_type = "dev";
   120		struct device_node *node;
   121		__be32 val[5] = {0};
   122		int ret;
   123	
   124		node = of_pci_make_node();
   125		if (!node)
   126			return;
   127	
   128		node->parent = dev->bus->dev.of_node;
 > 129		of_changeset_init(&cs);
   130	
   131		if (pci_is_bridge(dev)) {
   132			ret = of_pci_add_pci_bus_props(node, &cs);
   133			if (ret)
   134				goto changeset_destroy;
   135			pci_type = "pci";
   136		}
   137	
   138		node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
   139					    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
   140	
   141		val[0] = __cpu_to_be32(dev->devfn << 8);
   142		val[4] = __cpu_to_be32(SZ_4K);
   143		ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
   144		if (ret)
   145			goto changeset_destroy;
   146	
 > 147		ret = of_changeset_attach_node(&cs, node);
   148		if (ret)
   149			goto changeset_destroy;
   150	
 > 151		ret = of_changeset_apply(&cs);
   152		if (ret)
   153			goto changeset_destroy;
   154	
   155		dev->dev.of_node = node;
   156	
   157		return;
   158	
   159	changeset_destroy:
 > 160		of_changeset_destroy(&cs);
   161		kfree(node);
   162	}
   163	

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

Re: [PATCH 1/3] of: always populate a root node

[Rob Herring]

On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
> When enabling CONFIG_OF on a platform where of_root is not populated by
> firmware, we end up without a root node. In order to apply overlays and
> create subnodes of the root node, we need one. This commit creates an
> empty root node if not present.

The existing unittest essentially does the same thing for running the 
tests on non-DT systems. It should be modified to use this support 
instead. Maybe that's just removing the unittest code that set of_root.

I expect Frank will have some comments.

> Co-developed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/of/base.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index e7d92b67cb8a..6b8584c39f73 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -177,6 +177,19 @@ void __init of_core_init(void)
>  		pr_err("failed to register existing nodes\n");
>  		return;
>  	}
> +
> +	if (!of_root) {
> +		of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
> +		if (!of_root) {
> +			mutex_unlock(&of_mutex);
> +			pr_err("failed to create root node\n");
> +			return;
> +		}
> +
> +		of_root->full_name = "/";
> +		of_node_init(of_root);
> +	}
> +
>  	for_each_of_allnodes(np) {
>  		__of_attach_node_sysfs(np);
>  		if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> @@ -185,8 +198,7 @@ void __init of_core_init(void)
>  	mutex_unlock(&of_mutex);
>  
>  	/* Symlink in /proc as required by userspace ABI */
> -	if (of_root)
> -		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> +	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>  }
>  
>  static struct property *__of_find_property(const struct device_node *np,
> -- 
> 2.34.1
> 
> 

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[Rob Herring]

On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> In order to apply overlays to PCI device nodes, the nodes must first
> exist. This commit add support to populate a skeleton tree for PCI bus
> and devices. These nodes can then be used by drivers to apply overlays.
> 

While I implemented this creating the nodes as the PCI devices are 
created, I think probably we're going to want to create the device node 
and any needed parent nodes on demand. Otherwise, just turning on 
CONFIG_OF could break platforms.

One potential issue is that fwnode assumes there is either a DT node or 
ACPI node. With this, we have the potential for both. I'm not sure how 
much that's going to be an issue.

> Co-developed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/pci/of.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 184 insertions(+)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index cb2e8351c2cc..f2325708726e 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -16,12 +16,194 @@
>  #include "pci.h"
>  
>  #ifdef CONFIG_PCI
> +static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
> +			       const char *name, const void *value, int length)

Nothing really PCI specific about this function.

The kernel support for creating nodes and properties is pretty poor. We 
should improve it with functions like this (in drivers/of/). Maybe the 
changeset part should be separate though. We have some cases of creating 
properties or nodes already, and whatever new APIs we make those 
cases should be able to use them. And if they are converted, then it can 
be merged sooner rather than when all the PCI parts are ready.

> +{
> +	struct property *prop;
> +	int ret = -ENOMEM;
> +
> +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	prop->name = kstrdup(name, GFP_KERNEL);
> +	if (!prop->name)
> +		goto out_err;
> +
> +	if (value) {
> +		prop->value = kmemdup(value, length, GFP_KERNEL);
> +		if (!prop->value)
> +			goto out_err;
> +	} else {
> +		/*
> +		 * Even if the property has no value, it must be set to a
> +		 * non-null value since of_get_property() is used to check
> +		 * some values that might or not have a values (ranges for
> +		 * instance). Moreover, when the node is released, prop->value
> +		 * is kfreed so the memory must come from kmalloc.
> +		 */
> +		prop->value = kmalloc(1, GFP_KERNEL);
> +		if (!prop->value)
> +			goto out_err;
> +	}
> +
> +	of_property_set_flag(prop, OF_DYNAMIC);
> +
> +	prop->length = length;
> +
> +	ret = of_changeset_add_property(ocs, np, prop);
> +	if (!ret)
> +		return 0;
> +
> +out_err:
> +	kfree(prop->value);
> +	kfree(prop->name);
> +	kfree(prop);
> +
> +	return ret;
> +}
> +
> +static struct device_node *of_pci_make_node(void)
> +{

This isn't PCI specific either.

> +	struct device_node *node;
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return NULL;
> +
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +	of_node_init(node);
> +
> +	return node;
> +}
> +
> +static int of_pci_add_cells_props(struct device_node *node,
> +				  struct of_changeset *cs, int n_addr_cells,
> +				  int n_size_cells)
> +{
> +	__be32 val;
> +	int ret;
> +
> +	ret = of_pci_add_property(cs, node, "ranges", NULL, 0);

The host bridge node is going to need to fill in 'ranges'. Empty ranges 
is not valid when there's a change in number of cells.

The root node also will need "#address-cells" and "#size-cells".
 
> +	if (ret)
> +		return ret;
> +
> +	val = __cpu_to_be32(n_addr_cells);
> +	ret = of_pci_add_property(cs, node, "#address-cells", &val,
> +				  sizeof(__be32));
> +	if (ret)
> +		return ret;
> +
> +	val = __cpu_to_be32(n_size_cells);
> +	return of_pci_add_property(cs, node, "#size-cells", &val,
> +				   sizeof(__be32));
> +}
> +
> +static int of_pci_add_pci_bus_props(struct device_node *node,
> +				    struct of_changeset *cs)
> +{
> +	int ret;
> +
> +	ret = of_pci_add_property(cs, node, "device_type", "pci",
> +				  strlen("pci") + 1);
> +	if (ret)
> +		return ret;
> +
> +	return of_pci_add_cells_props(node, cs, 3, 2);
> +}
> +
> +static void of_pci_make_dev_node(struct pci_dev *dev)
> +{
> +	static struct of_changeset cs;
> +	const char *pci_type = "dev";
> +	struct device_node *node;
> +	__be32 val[5] = {0};
> +	int ret;
> +
> +	node = of_pci_make_node();
> +	if (!node)
> +		return;
> +
> +	node->parent = dev->bus->dev.of_node;
> +	of_changeset_init(&cs);
> +
> +	if (pci_is_bridge(dev)) {
> +		ret = of_pci_add_pci_bus_props(node, &cs);
> +		if (ret)
> +			goto changeset_destroy;
> +		pci_type = "pci";
> +	}
> +
> +	node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
> +				    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> +
> +	val[0] = __cpu_to_be32(dev->devfn << 8);
> +	val[4] = __cpu_to_be32(SZ_4K);
> +	ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_attach_node(&cs, node);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_apply(&cs);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	dev->dev.of_node = node;
> +
> +	return;
> +
> +changeset_destroy:
> +	of_changeset_destroy(&cs);
> +	kfree(node);
> +}
> +
> +static struct device_node *of_pci_create_root_bus_node(struct pci_bus *bus)
> +{
> +	static struct of_changeset cs;
> +	struct device_node *node;
> +	int ret;
> +
> +	node = of_pci_make_node();
> +	if (!node)
> +		return NULL;
> +
> +	node->full_name = "pci";
> +	node->parent = of_root;
> +
> +	of_changeset_init(&cs);
> +	ret = of_pci_add_pci_bus_props(node, &cs);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_attach_node(&cs, node);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_apply(&cs);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	return node;
> +
> +changeset_destroy:
> +	of_changeset_destroy(&cs);
> +	kfree(node);
> +
> +	return NULL;
> +}
> +
>  void pci_set_of_node(struct pci_dev *dev)
>  {
>  	if (!dev->bus->dev.of_node)
>  		return;
>  	dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
>  						    dev->devfn);
> +	if (!dev->dev.of_node)
> +		of_pci_make_dev_node(dev);
>  	if (dev->dev.of_node)
>  		dev->dev.fwnode = &dev->dev.of_node->fwnode;
>  }
> @@ -39,6 +221,8 @@ void pci_set_bus_of_node(struct pci_bus *bus)
>  
>  	if (bus->self == NULL) {
>  		node = pcibios_get_phb_of_node(bus);
> +		if (!node)
> +			node = of_pci_create_root_bus_node(bus);
>  	} else {
>  		node = of_node_get(bus->self->dev.of_node);
>  		if (node && of_property_read_bool(node, "external-facing"))
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/3] of: always populate a root node

[Clément Léger]

Le Tue, 3 May 2022 08:45:11 -0500,
Rob Herring <robh@kernel.org> a écrit :

> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
> > When enabling CONFIG_OF on a platform where of_root is not populated by
> > firmware, we end up without a root node. In order to apply overlays and
> > create subnodes of the root node, we need one. This commit creates an
> > empty root node if not present.  
> 
> The existing unittest essentially does the same thing for running the 
> tests on non-DT systems. It should be modified to use this support 
> instead. Maybe that's just removing the unittest code that set of_root.

Acked, I'll try the unit test on my system.

> 
> I expect Frank will have some comments.
> 
> > Co-developed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/of/base.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index e7d92b67cb8a..6b8584c39f73 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -177,6 +177,19 @@ void __init of_core_init(void)
> >  		pr_err("failed to register existing nodes\n");
> >  		return;
> >  	}
> > +
> > +	if (!of_root) {
> > +		of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
> > +		if (!of_root) {
> > +			mutex_unlock(&of_mutex);
> > +			pr_err("failed to create root node\n");
> > +			return;
> > +		}
> > +
> > +		of_root->full_name = "/";
> > +		of_node_init(of_root);
> > +	}
> > +
> >  	for_each_of_allnodes(np) {
> >  		__of_attach_node_sysfs(np);
> >  		if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
> > @@ -185,8 +198,7 @@ void __init of_core_init(void)
> >  	mutex_unlock(&of_mutex);
> >  
> >  	/* Symlink in /proc as required by userspace ABI */
> > -	if (of_root)
> > -		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> > +	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> >  }
> >  
> >  static struct property *__of_find_property(const struct device_node *np,
> > -- 
> > 2.34.1
> > 
> >   



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

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[Clément Léger]

Le Tue, 3 May 2022 09:12:06 -0500,
Rob Herring <robh@kernel.org> a écrit :

> On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> > In order to apply overlays to PCI device nodes, the nodes must first
> > exist. This commit add support to populate a skeleton tree for PCI bus
> > and devices. These nodes can then be used by drivers to apply overlays.
> >   
> 
> While I implemented this creating the nodes as the PCI devices are 
> created, I think probably we're going to want to create the device node 
> and any needed parent nodes on demand. Otherwise, just turning on 
> CONFIG_OF could break platforms.

Ok, so this creation would potentially be done on request from some PCI
driver that want to apply it's overlay on the tree. Should I actually
add some function such as of_pci_apply_overlay() which would create the
PCI node tree if not present and apply the overlay to the of_node that
is associated to the PCIe device ?

> 
> One potential issue is that fwnode assumes there is either a DT node or 
> ACPI node. With this, we have the potential for both. I'm not sure how 
> much that's going to be an issue.

Not sure either but that's better not to play with that.

> 
> > Co-developed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/pci/of.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 184 insertions(+)
> > 
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index cb2e8351c2cc..f2325708726e 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -16,12 +16,194 @@
> >  #include "pci.h"
> >  
> >  #ifdef CONFIG_PCI
> > +static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
> > +			       const char *name, const void *value, int length)  
> 
> Nothing really PCI specific about this function.
> 
> The kernel support for creating nodes and properties is pretty poor. We 
> should improve it with functions like this (in drivers/of/). Maybe the 
> changeset part should be separate though. We have some cases of creating 
> properties or nodes already, and whatever new APIs we make those 
> cases should be able to use them. And if they are converted, then it can 
> be merged sooner rather than when all the PCI parts are ready.

Ok, so this will be done as a first separate series to add property
creation then.

> > +
> > +static int of_pci_add_cells_props(struct device_node *node,
> > +				  struct of_changeset *cs, int n_addr_cells,
> > +				  int n_size_cells)
> > +{
> > +	__be32 val;
> > +	int ret;
> > +
> > +	ret = of_pci_add_property(cs, node, "ranges", NULL, 0);  
> 
> The host bridge node is going to need to fill in 'ranges'. Empty ranges 
> is not valid when there's a change in number of cells.

Ok, wasn't aware of that. If I understand, I'll need to obtain the
range of PCI addresses that are behind the bridge to fill in this
ranges property right ?

> 
> The root node also will need "#address-cells" and "#size-cells".
>  

Ok.


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

Re: [PATCH 1/3] of: always populate a root node

[Frank Rowand]

On 5/3/22 08:45, Rob Herring wrote:
> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
>> When enabling CONFIG_OF on a platform where of_root is not populated by
>> firmware, we end up without a root node. In order to apply overlays and
>> create subnodes of the root node, we need one. This commit creates an
>> empty root node if not present.
> 
> The existing unittest essentially does the same thing for running the 
> tests on non-DT systems. It should be modified to use this support 
> instead. Maybe that's just removing the unittest code that set of_root.
> 
> I expect Frank will have some comments.
> 

< snip >

This patch series is next on my list, after what I am currently working
on (updating the .dts -> .dtso patch).  I may get to this today, but
more likely it will be tomorrow.

-Frank

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[Bjorn Helgaas]

In subject:

  PCI: of: Create DT nodes ... if they do not exist

The subject could be read as saying that you're going to create DT
nodes before the PCI devices exist, but I think you mean that when we
enumerate a PCI devices, we're *always* going to create a DT node for
it, even if the DT didn't mention it.

Maybe something like:

  PCI: of: Create DT node for every PCI device

although I see Rob thinks this should be done on demand instead of
doing it for every device, which sounds sensible to me.

On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> In order to apply overlays to PCI device nodes, the nodes must first
> exist. This commit add support to populate a skeleton tree for PCI bus
> and devices. These nodes can then be used by drivers to apply overlays.

s/This commit add support/Add support/

Bjorn

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[Clément Léger]

Le Tue, 3 May 2022 17:53:53 -0500,
Bjorn Helgaas <helgaas@kernel.org> a écrit :

> In subject:
> 
>   PCI: of: Create DT nodes ... if they do not exist
> 
> The subject could be read as saying that you're going to create DT
> nodes before the PCI devices exist, but I think you mean that when we
> enumerate a PCI devices, we're *always* going to create a DT node for
> it, even if the DT didn't mention it.

Hi Bjorn,

Indeed ! I'll modify that.

> 
> Maybe something like:
> 
>   PCI: of: Create DT node for every PCI device
> 
> although I see Rob thinks this should be done on demand instead of
> doing it for every device, which sounds sensible to me.

Agreed, I'll rework this series.

Thanks,

> 
> On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> > In order to apply overlays to PCI device nodes, the nodes must first
> > exist. This commit add support to populate a skeleton tree for PCI bus
> > and devices. These nodes can then be used by drivers to apply overlays.  
> 
> s/This commit add support/Add support/
> 
> Bjorn



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

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Frank Rowand]

On 4/27/22 04:44, Clément Léger wrote:
> This series adds foundation work to support the lan9662 PCIe card. This
> card is meant to be used an ethernet switch with 2 x RJ45 ports and
> 2 x 2.5G SFPs. The lan966x SoCs can be used in two different ways:
> 
>  - It can run Linux by itself, on ARM64 cores included in the SoC. This
>    use-case of the lan966x is currently being upstreamed, using a
>    traditional Device Tree representation of the lan996x HW blocks [1]
>    A number of drivers for the different IPs of the SoC have already
>    been merged in upstream Linux.
> 
>  - It can be used as a PCIe endpoint, connected to a separate platform
>    that acts as the PCIe root complex. In this case, all the devices
>    that are embedded on this SoC are exposed through PCIe BARs and the
>    ARM64 cores of the SoC are not used. Since this is a PCIe card, it
>    can be plugged on any platform, of any architecture supporting PCIe.
> 
> The problem that arose is that we want to reuse all the existing OF
> compatible drivers that are used when in SoC mode to instantiate the
> PCI device when in PCIe endpoint mode.
> 
> A previous attempt to tackle this problem was made using fwnode [1].
> However, this proved being way too invasive and it required
> modifications in both subsystems and drivers to support fwnode. First
> series did not lead to a consensus and multiple ideas to support this
> use-case were mentioned (ACPI overlay, fwnode, device-tree overlay).
> Since it only seemed that fwnode was not a totally silly idea, we
> continued on this way.
> 
> However, on the series that added fwnode support to the reset subsystem,
> Rob Herring mentioned the fact that OF overlay might actually be the
> best way to probe PCI devices and populate platform drivers using this
> overlay. He also provided a branch containing some commits that helped

I need to go look at the various email threads mentioned above before I
continue reading this patch series.

I do have serious concerns with this approach.  I need to investigate
more fully before I can determine whether the concerns are addressed
sufficiently.

To give some background to my longstanding response to similar proposals,
here is my old statement from https://elinux.org/Device_Tree_Reference:

   Overlays
   Mainline Linux Support
   Run time overlay apply and run time overlay remove from user space are
   not supported in the mainline kernel. There   are out of tree patches
   to implement this feature via an overlay manager. The overlay manager
   is used successfully by many users for specific overlays on specific
   boards with specific environments and use cases. However, there are many
   issues with the Linux kernel overlay implementation due to incomplete and
   incorrect code. The overlay manager has not been accepted in mainline due
   to these issues. Once these issues are resolved, it is expected that some
   method of run time overlay apply and overlay removal from user space will
   be supported by the Linux kernel.

   There is a possibility that overlay apply and overlay remove support could
   be phased in slowly, feature by feature, as specific issues are resolved.

Those are my words, not Rob's, but I thought that Rob was somewhat in
agreement with those ideas.  Apparently either I misunderstood his
thoughts, or his thoughts have evolved, since you say that he suggested
overlays in one of the above email threads, and you list him as a
co-developer.

In the next line of the elinux info above, I provide a link to more
detailed information:

   Frank's thoughts on what is needed to complete basic overlay support

The link goes to:

   https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts

That page provides an incomplete list of issues to be resolved, and
a list of "what has been completed".

Please read through the elinux.org page to understand the basis of
my concerns.

If after reading through the related email threads, and this thread,
I agree that overlays are a good approach, I am already aware of areas
that I will have specific comments about on the patches in this thread.

-Frank

> to implement this idea on a x86 setup. Due to the dynamic nature of PCI
> bus enumeration, some other modifications needs to be applied on the
> overlay to apply it correctly. Indeed, it is necessary to modify the
> target node of the fragments to apply them correctly on the PCI device
> that was probed. Moreover, the 'ranges' must be set according to the
> BAR addresses in order to remap devices to the correct PCI addresses.
> These modifications are the located into the driver since the remapping
> is something that is specific to each driver.
> 
> After modifications, this proves to be successful and a full support of
> the aforementioned lan966x PCI card was added. The modifications to
> support that (apply an overlay on a dynamically created PCI of_node) are
> actually minimal and only touches a few places (pci/of.c). This series
> contains the 3 commits that are necessary to do that:
> 
> - First commit creates the root node if not present on a x86 setup
>   without a firmware provided device-tree.
> - Second one dynamically creates the PCI bus/device device-tree node
>   hierarchy using changeset API.
> - Finally a last commit allows to apply an overlay by targeting a
>   specific device-tree node.
> 
> Other problems that might be considered with this series is the fact
> that CONFIG_OF is not enabled by default on x86 configuration and thus
> the driver can't be used without rebuilding a complete kernel with
> CONFIG_OF=y. In order to fully support this PCIe card and allow lambda
> user to use this driver, it would be almost mandatory to enable
> CONFIG_OF by default on such setup.
> 
> A driver using this support was added and can be seen at [3]. This
> driver embeds a builtin overlay and applies it to the live tree using
> of_overlay_fdt_apply_to_node(). An interrupt driver is also included and
> associated to a node that is added by the overlay. The driver also
> insert a specific "ranges" property based on the BAR values which allows
> to remap the device-tree node to BAR addresses dynamically. This is
> needed to allow applying the overlay without depending on specific
> enumeration BAR addresses.
> 
> This series was tested on a x86 kernel using CONFIG_OF under a virtual
> machine using PCI passthrough.
> 
> Link: [1] https://lore.kernel.org/lkml/YhQHqDJvahgriDZK@lunn.ch/t/
> Link: [2] https://lore.kernel.org/lkml/20220408174841.34458529@fixe.home/T/
> Link: [3] https://github.com/clementleger/linux/tree/lan966x/of_support
> 
> Clément Léger (3):
>   of: always populate a root node
>   PCI: of: create DT nodes for PCI devices if they do not exists
>   of: overlay: add of_overlay_fdt_apply_to_node()
> 
>  drivers/of/base.c    |  16 +++-
>  drivers/of/overlay.c |  21 +++--
>  drivers/pci/of.c     | 184 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h   |  17 +++-
>  4 files changed, 224 insertions(+), 14 deletions(-)
> 

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Clément Léger]

Le Fri, 6 May 2022 13:33:22 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 4/27/22 04:44, Clément Léger wrote:
> > This series adds foundation work to support the lan9662 PCIe card.
> > This card is meant to be used an ethernet switch with 2 x RJ45
> > ports and 2 x 2.5G SFPs. The lan966x SoCs can be used in two
> > different ways:
> > 
> >  - It can run Linux by itself, on ARM64 cores included in the SoC.
> > This use-case of the lan966x is currently being upstreamed, using a
> >    traditional Device Tree representation of the lan996x HW blocks
> > [1] A number of drivers for the different IPs of the SoC have
> > already been merged in upstream Linux.
> > 
> >  - It can be used as a PCIe endpoint, connected to a separate
> > platform that acts as the PCIe root complex. In this case, all the
> > devices that are embedded on this SoC are exposed through PCIe BARs
> > and the ARM64 cores of the SoC are not used. Since this is a PCIe
> > card, it can be plugged on any platform, of any architecture
> > supporting PCIe.
> > 
> > The problem that arose is that we want to reuse all the existing OF
> > compatible drivers that are used when in SoC mode to instantiate the
> > PCI device when in PCIe endpoint mode.
> > 
> > A previous attempt to tackle this problem was made using fwnode [1].
> > However, this proved being way too invasive and it required
> > modifications in both subsystems and drivers to support fwnode.
> > First series did not lead to a consensus and multiple ideas to
> > support this use-case were mentioned (ACPI overlay, fwnode,
> > device-tree overlay). Since it only seemed that fwnode was not a
> > totally silly idea, we continued on this way.
> > 
> > However, on the series that added fwnode support to the reset
> > subsystem, Rob Herring mentioned the fact that OF overlay might
> > actually be the best way to probe PCI devices and populate platform
> > drivers using this overlay. He also provided a branch containing
> > some commits that helped  
> 
> I need to go look at the various email threads mentioned above before
> I continue reading this patch series.
> 
> I do have serious concerns with this approach.  I need to investigate
> more fully before I can determine whether the concerns are addressed
> sufficiently.
> 
> To give some background to my longstanding response to similar
> proposals, here is my old statement from
> https://elinux.org/Device_Tree_Reference:
> 
>    Overlays
>    Mainline Linux Support
>    Run time overlay apply and run time overlay remove from user space
> are not supported in the mainline kernel. There   are out of tree
> patches to implement this feature via an overlay manager. The overlay
> manager is used successfully by many users for specific overlays on
> specific boards with specific environments and use cases. However,
> there are many issues with the Linux kernel overlay implementation
> due to incomplete and incorrect code. The overlay manager has not
> been accepted in mainline due to these issues. Once these issues are
> resolved, it is expected that some method of run time overlay apply
> and overlay removal from user space will be supported by the Linux
> kernel.
> 
>    There is a possibility that overlay apply and overlay remove
> support could be phased in slowly, feature by feature, as specific
> issues are resolved.

Hi Frank,

This work uses the kernel space interface (of_overlay_fdt_apply())
and the device tree overlay is builtin the driver. This interface was
used until recently by rcu-dcar driver. While the only user (sic),
this seems to work pretty well and I was able to use it successfully.

Moreover, this support targets at using this with PCI devices. This
devices are really well contained and do not interfere with other
devices. This actually consists in adding a complete subtree into the
existing device-tree and thus it limits the interactions between
potentially platform provided devices and PCI ones.

Clément

> 
> Those are my words, not Rob's, but I thought that Rob was somewhat in
> agreement with those ideas.  Apparently either I misunderstood his
> thoughts, or his thoughts have evolved, since you say that he
> suggested overlays in one of the above email threads, and you list
> him as a co-developer.
> 
> In the next line of the elinux info above, I provide a link to more
> detailed information:
> 
>    Frank's thoughts on what is needed to complete basic overlay
> support
> 
> The link goes to:
> 
>    https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts
> 
> That page provides an incomplete list of issues to be resolved, and
> a list of "what has been completed".
> 
> Please read through the elinux.org page to understand the basis of
> my concerns.
> 
> If after reading through the related email threads, and this thread,
> I agree that overlays are a good approach, I am already aware of areas
> that I will have specific comments about on the patches in this
> thread.
> 
> -Frank
> 
> > to implement this idea on a x86 setup. Due to the dynamic nature of
> > PCI bus enumeration, some other modifications needs to be applied
> > on the overlay to apply it correctly. Indeed, it is necessary to
> > modify the target node of the fragments to apply them correctly on
> > the PCI device that was probed. Moreover, the 'ranges' must be set
> > according to the BAR addresses in order to remap devices to the
> > correct PCI addresses. These modifications are the located into the
> > driver since the remapping is something that is specific to each
> > driver.
> > 
> > After modifications, this proves to be successful and a full
> > support of the aforementioned lan966x PCI card was added. The
> > modifications to support that (apply an overlay on a dynamically
> > created PCI of_node) are actually minimal and only touches a few
> > places (pci/of.c). This series contains the 3 commits that are
> > necessary to do that:
> > 
> > - First commit creates the root node if not present on a x86 setup
> >   without a firmware provided device-tree.
> > - Second one dynamically creates the PCI bus/device device-tree node
> >   hierarchy using changeset API.
> > - Finally a last commit allows to apply an overlay by targeting a
> >   specific device-tree node.
> > 
> > Other problems that might be considered with this series is the fact
> > that CONFIG_OF is not enabled by default on x86 configuration and
> > thus the driver can't be used without rebuilding a complete kernel
> > with CONFIG_OF=y. In order to fully support this PCIe card and
> > allow lambda user to use this driver, it would be almost mandatory
> > to enable CONFIG_OF by default on such setup.
> > 
> > A driver using this support was added and can be seen at [3]. This
> > driver embeds a builtin overlay and applies it to the live tree
> > using of_overlay_fdt_apply_to_node(). An interrupt driver is also
> > included and associated to a node that is added by the overlay. The
> > driver also insert a specific "ranges" property based on the BAR
> > values which allows to remap the device-tree node to BAR addresses
> > dynamically. This is needed to allow applying the overlay without
> > depending on specific enumeration BAR addresses.
> > 
> > This series was tested on a x86 kernel using CONFIG_OF under a
> > virtual machine using PCI passthrough.
> > 
> > Link: [1] https://lore.kernel.org/lkml/YhQHqDJvahgriDZK@lunn.ch/t/
> > Link: [2]
> > https://lore.kernel.org/lkml/20220408174841.34458529@fixe.home/T/
> > Link: [3]
> > https://github.com/clementleger/linux/tree/lan966x/of_support
> > 
> > Clément Léger (3):
> >   of: always populate a root node
> >   PCI: of: create DT nodes for PCI devices if they do not exists
> >   of: overlay: add of_overlay_fdt_apply_to_node()
> > 
> >  drivers/of/base.c    |  16 +++-
> >  drivers/of/overlay.c |  21 +++--
> >  drivers/pci/of.c     | 184
> > +++++++++++++++++++++++++++++++++++++++++++ include/linux/of.h   |
> > 17 +++- 4 files changed, 224 insertions(+), 14 deletions(-)
> >   
> 

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Frank Rowand]

On 5/9/22 07:16, Clément Léger wrote:
> Le Fri, 6 May 2022 13:33:22 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>> On 4/27/22 04:44, Clément Léger wrote:
>>> This series adds foundation work to support the lan9662 PCIe card.
>>> This card is meant to be used an ethernet switch with 2 x RJ45
>>> ports and 2 x 2.5G SFPs. The lan966x SoCs can be used in two
>>> different ways:
>>>
>>>  - It can run Linux by itself, on ARM64 cores included in the SoC.
>>> This use-case of the lan966x is currently being upstreamed, using a
>>>    traditional Device Tree representation of the lan996x HW blocks
>>> [1] A number of drivers for the different IPs of the SoC have
>>> already been merged in upstream Linux.
>>>
>>>  - It can be used as a PCIe endpoint, connected to a separate
>>> platform that acts as the PCIe root complex. In this case, all the
>>> devices that are embedded on this SoC are exposed through PCIe BARs
>>> and the ARM64 cores of the SoC are not used. Since this is a PCIe
>>> card, it can be plugged on any platform, of any architecture
>>> supporting PCIe.
>>>
>>> The problem that arose is that we want to reuse all the existing OF
>>> compatible drivers that are used when in SoC mode to instantiate the
>>> PCI device when in PCIe endpoint mode.
>>>
>>> A previous attempt to tackle this problem was made using fwnode [1].
>>> However, this proved being way too invasive and it required
>>> modifications in both subsystems and drivers to support fwnode.
>>> First series did not lead to a consensus and multiple ideas to
>>> support this use-case were mentioned (ACPI overlay, fwnode,
>>> device-tree overlay). Since it only seemed that fwnode was not a
>>> totally silly idea, we continued on this way.
>>>
>>> However, on the series that added fwnode support to the reset
>>> subsystem, Rob Herring mentioned the fact that OF overlay might
>>> actually be the best way to probe PCI devices and populate platform
>>> drivers using this overlay. He also provided a branch containing
>>> some commits that helped  
>>
>> I need to go look at the various email threads mentioned above before
>> I continue reading this patch series.
>>
>> I do have serious concerns with this approach.  I need to investigate
>> more fully before I can determine whether the concerns are addressed
>> sufficiently.
>>
>> To give some background to my longstanding response to similar
>> proposals, here is my old statement from
>> https://elinux.org/Device_Tree_Reference:
>>
>>    Overlays
>>    Mainline Linux Support
>>    Run time overlay apply and run time overlay remove from user space
>> are not supported in the mainline kernel. There   are out of tree
>> patches to implement this feature via an overlay manager. The overlay
>> manager is used successfully by many users for specific overlays on
>> specific boards with specific environments and use cases. However,
>> there are many issues with the Linux kernel overlay implementation
>> due to incomplete and incorrect code. The overlay manager has not
>> been accepted in mainline due to these issues. Once these issues are
>> resolved, it is expected that some method of run time overlay apply
>> and overlay removal from user space will be supported by the Linux
>> kernel.
>>
>>    There is a possibility that overlay apply and overlay remove
>> support could be phased in slowly, feature by feature, as specific
>> issues are resolved.
> 
> Hi Frank,
> 
> This work uses the kernel space interface (of_overlay_fdt_apply())
> and the device tree overlay is builtin the driver. This interface was
> used until recently by rcu-dcar driver. While the only user (sic),
> this seems to work pretty well and I was able to use it successfully.

Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
was explicitly recognized as a grandfathered exception, and not an
example for other users.  It was finally removed in 5.18-rc1.

You may have used of_overlay_fdt_apply() in a specific use case at
a specific kernel version, but if you read through the references
I provided you will find that applying overlays after the kernel
boots is a fragile endeavor, with expectations of bugs and problems
being exposed as usage is changed (simple example is that my adding
some overlay notifier unittests exposed yet another memory leak).

The reference that I provided also shows how the overlay code is
being improved over time.  Even with improvements, it will remain
fragile.

> 
> Moreover, this support targets at using this with PCI devices. This
> devices are really well contained and do not interfere with other
> devices. This actually consists in adding a complete subtree into the
> existing device-tree and thus it limits the interactions between
> potentially platform provided devices and PCI ones.

Yes, that it is very important that you have described this fact, both
here and in other emails.  Thank you for that information, it does help
understanding the alternatives.

I've hesitated in recommending a specific solution before better
understanding the architecture of your pcie board and drivers, but
I've delayed too long, so I am going to go ahead and mention one
possibility at the risk of not yet fully understanding the situation.

On the surface, it appears that your need might be well met by having
a base devicetree that describes all of the pcie nodes, but with each
node having a status of "disabled" so that they will not be used.
Have a devicetree overlay describing the pcie card (as you proposed),
where the overlay also includes a status of "ok" for the pcie node.
Applying the overlay, with a method of redirecting the target to a
specific pcie node would change the status of the pcie node to enable
its use.  (You have already proposed a patch to modify of_overlay_fdt_apply()
to allow a modified target, so not a new concept from me.)  My suggestion
is to apply the overlay devicetree to the base devicetree before the
combined FDT devicetree is passed to the kernel at boot.  The overlay
apply could be done by several different entities.  It could be before
the bootloader executes, it could be done by the bootloader, it could
be done by a shim between the bootloader and the kernel.  This method
avoids all of the issues of applying an overlay to a running system
that I find problematic.  It is also a method used by the U-boot
bootloader, as an example.

The other big issue is mixing ACPI and devicetree on a single system.
Historically, the Linux devicetree community has not been receptive
to the ides of that mixture.  Your example might be a specific case
where the two can be isolated from each other, or maybe not.  (For
disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
ACPI and devicetree is a recipe for disaster in the general case.

More to come later as I finish reading through the various threads.

-Frank

> 
> Clément
> 
>>
>> Those are my words, not Rob's, but I thought that Rob was somewhat in
>> agreement with those ideas.  Apparently either I misunderstood his
>> thoughts, or his thoughts have evolved, since you say that he
>> suggested overlays in one of the above email threads, and you list
>> him as a co-developer.
>>
>> In the next line of the elinux info above, I provide a link to more
>> detailed information:
>>
>>    Frank's thoughts on what is needed to complete basic overlay
>> support
>>
>> The link goes to:
>>
>>    https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts
>>
>> That page provides an incomplete list of issues to be resolved, and
>> a list of "what has been completed".
>>
>> Please read through the elinux.org page to understand the basis of
>> my concerns.
>>
>> If after reading through the related email threads, and this thread,
>> I agree that overlays are a good approach, I am already aware of areas
>> that I will have specific comments about on the patches in this
>> thread.
>>
>> -Frank
>>
>>> to implement this idea on a x86 setup. Due to the dynamic nature of
>>> PCI bus enumeration, some other modifications needs to be applied
>>> on the overlay to apply it correctly. Indeed, it is necessary to
>>> modify the target node of the fragments to apply them correctly on
>>> the PCI device that was probed. Moreover, the 'ranges' must be set
>>> according to the BAR addresses in order to remap devices to the
>>> correct PCI addresses. These modifications are the located into the
>>> driver since the remapping is something that is specific to each
>>> driver.
>>>
>>> After modifications, this proves to be successful and a full
>>> support of the aforementioned lan966x PCI card was added. The
>>> modifications to support that (apply an overlay on a dynamically
>>> created PCI of_node) are actually minimal and only touches a few
>>> places (pci/of.c). This series contains the 3 commits that are
>>> necessary to do that:
>>>
>>> - First commit creates the root node if not present on a x86 setup
>>>   without a firmware provided device-tree.
>>> - Second one dynamically creates the PCI bus/device device-tree node
>>>   hierarchy using changeset API.
>>> - Finally a last commit allows to apply an overlay by targeting a
>>>   specific device-tree node.
>>>
>>> Other problems that might be considered with this series is the fact
>>> that CONFIG_OF is not enabled by default on x86 configuration and
>>> thus the driver can't be used without rebuilding a complete kernel
>>> with CONFIG_OF=y. In order to fully support this PCIe card and
>>> allow lambda user to use this driver, it would be almost mandatory
>>> to enable CONFIG_OF by default on such setup.
>>>
>>> A driver using this support was added and can be seen at [3]. This
>>> driver embeds a builtin overlay and applies it to the live tree
>>> using of_overlay_fdt_apply_to_node(). An interrupt driver is also
>>> included and associated to a node that is added by the overlay. The
>>> driver also insert a specific "ranges" property based on the BAR
>>> values which allows to remap the device-tree node to BAR addresses
>>> dynamically. This is needed to allow applying the overlay without
>>> depending on specific enumeration BAR addresses.
>>>
>>> This series was tested on a x86 kernel using CONFIG_OF under a
>>> virtual machine using PCI passthrough.
>>>
>>> Link: [1] https://lore.kernel.org/lkml/YhQHqDJvahgriDZK@lunn.ch/t/
>>> Link: [2]
>>> https://lore.kernel.org/lkml/20220408174841.34458529@fixe.home/T/
>>> Link: [3]
>>> https://github.com/clementleger/linux/tree/lan966x/of_support
>>>
>>> Clément Léger (3):
>>>   of: always populate a root node
>>>   PCI: of: create DT nodes for PCI devices if they do not exists
>>>   of: overlay: add of_overlay_fdt_apply_to_node()
>>>
>>>  drivers/of/base.c    |  16 +++-
>>>  drivers/of/overlay.c |  21 +++--
>>>  drivers/pci/of.c     | 184
>>> +++++++++++++++++++++++++++++++++++++++++++ include/linux/of.h   |
>>> 17 +++- 4 files changed, 224 insertions(+), 14 deletions(-)
>>>   
>>
> 

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Clément Léger]

Le Mon, 9 May 2022 10:56:36 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> > Hi Frank,
> > 
> > This work uses the kernel space interface (of_overlay_fdt_apply())
> > and the device tree overlay is builtin the driver. This interface
> > was used until recently by rcu-dcar driver. While the only user
> > (sic), this seems to work pretty well and I was able to use it
> > successfully.  
> 
> Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
> was explicitly recognized as a grandfathered exception, and not an
> example for other users.  It was finally removed in 5.18-rc1.

I noticed that unfortunately.

> 
> You may have used of_overlay_fdt_apply() in a specific use case at
> a specific kernel version, but if you read through the references
> I provided you will find that applying overlays after the kernel
> boots is a fragile endeavor, with expectations of bugs and problems
> being exposed as usage is changed (simple example is that my adding
> some overlay notifier unittests exposed yet another memory leak).
> 
> The reference that I provided also shows how the overlay code is
> being improved over time.  Even with improvements, it will remain
> fragile.

Acked.

> 
> > 
> > Moreover, this support targets at using this with PCI devices. This
> > devices are really well contained and do not interfere with other
> > devices. This actually consists in adding a complete subtree into
> > the existing device-tree and thus it limits the interactions between
> > potentially platform provided devices and PCI ones.  
> 
> Yes, that it is very important that you have described this fact, both
> here and in other emails.  Thank you for that information, it does
> help understanding the alternatives.
> 
> I've hesitated in recommending a specific solution before better
> understanding the architecture of your pcie board and drivers, but
> I've delayed too long, so I am going to go ahead and mention one
> possibility at the risk of not yet fully understanding the situation.
> 
> On the surface, it appears that your need might be well met by having
> a base devicetree that describes all of the pcie nodes, but with each
> node having a status of "disabled" so that they will not be used.
> Have a devicetree overlay describing the pcie card (as you proposed),
> where the overlay also includes a status of "ok" for the pcie node.
> Applying the overlay, with a method of redirecting the target to a
> specific pcie node would change the status of the pcie node to enable
> its use.  (You have already proposed a patch to modify
> of_overlay_fdt_apply() to allow a modified target, so not a new
> concept from me.)  My suggestion is to apply the overlay devicetree
> to the base devicetree before the combined FDT devicetree is passed
> to the kernel at boot.  The overlay apply could be done by several
> different entities.  It could be before the bootloader executes, it
> could be done by the bootloader, it could be done by a shim between
> the bootloader and the kernel.  This method avoids all of the issues
> of applying an overlay to a running system that I find problematic.
> It is also a method used by the U-boot bootloader, as an example.

Ok, that is actually possible on a system that is given a device-tree
by the bootloader. But on a system that is desrcibed using ACPI (such
as the x86), this is much more difficult (at least to my knowledge)...
We want this feature to be easy to use for the end user. Adding such
configuration which also differs between various architecture is
clearly not so easy to setup.

Moreover, since the PCI is meant to be "Plug and Play", such
configuration would completely break that. If the user switches the
PCIe card from one slot to another, the bootloader configuration will
need to be modified. This seems a big no way for me (and for the user).

> 
> The other big issue is mixing ACPI and devicetree on a single system.
> Historically, the Linux devicetree community has not been receptive
> to the ides of that mixture.  Your example might be a specific case
> where the two can be isolated from each other, or maybe not.  (For
> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
> ACPI and devicetree is a recipe for disaster in the general case.

Agreed, on that fact, it did raised some eyebrows, and it was for that
specific concern that initially, I proposed the fwnode solution.
Honestly, the fwnode conversion represent a lot of work (hundreds of
lines easily) + requires a conversion of all the subsystem that are not
fwnode ready (spoiler: almost all of them are not ready). 

After implementing Rob's solution, the device-tree overlay really seems
the cleaner to me and requires much less modifications.

> 
> More to come later as I finish reading through the various threads.

Ok, thanks for your time !

Clément

> 
> -Frank

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Andy Shevchenko]

On Mon, May 09, 2022 at 06:09:17PM +0200, Clément Léger wrote:
> Le Mon, 9 May 2022 10:56:36 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :

...

> > On the surface, it appears that your need might be well met by having
> > a base devicetree that describes all of the pcie nodes, but with each
> > node having a status of "disabled" so that they will not be used.
> > Have a devicetree overlay describing the pcie card (as you proposed),
> > where the overlay also includes a status of "ok" for the pcie node.
> > Applying the overlay, with a method of redirecting the target to a
> > specific pcie node would change the status of the pcie node to enable
> > its use.  (You have already proposed a patch to modify
> > of_overlay_fdt_apply() to allow a modified target, so not a new
> > concept from me.)  My suggestion is to apply the overlay devicetree
> > to the base devicetree before the combined FDT devicetree is passed
> > to the kernel at boot.  The overlay apply could be done by several
> > different entities.  It could be before the bootloader executes, it
> > could be done by the bootloader, it could be done by a shim between
> > the bootloader and the kernel.  This method avoids all of the issues
> > of applying an overlay to a running system that I find problematic.
> > It is also a method used by the U-boot bootloader, as an example.
> 
> Ok, that is actually possible on a system that is given a device-tree
> by the bootloader. But on a system that is desrcibed using ACPI (such
> as the x86), this is much more difficult (at least to my knowledge)...
> We want this feature to be easy to use for the end user. Adding such
> configuration which also differs between various architecture is
> clearly not so easy to setup.
> 
> Moreover, since the PCI is meant to be "Plug and Play", such
> configuration would completely break that. If the user switches the
> PCIe card from one slot to another, the bootloader configuration will
> need to be modified. This seems a big no way for me (and for the user).

The main problem here is that Linux does not support hotplugging for the
devices behind non-hotpluggable buses. You need to develop something to
say that the device tree (in terms of hardware) can morph at run-time
transparently to the user. I think the closest one is what FPGA does,
or at least should do.

> > The other big issue is mixing ACPI and devicetree on a single system.
> > Historically, the Linux devicetree community has not been receptive
> > to the ides of that mixture.  Your example might be a specific case
> > where the two can be isolated from each other, or maybe not.  (For
> > disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
> > ACPI and devicetree is a recipe for disaster in the general case.
> 
> Agreed, on that fact, it did raised some eyebrows, and it was for that
> specific concern that initially, I proposed the fwnode solution.
> Honestly, the fwnode conversion represent a lot of work (hundreds of
> lines easily) + requires a conversion of all the subsystem that are not
> fwnode ready (spoiler: almost all of them are not ready). 

In either case you need to provide a format that would be suitable for
DT-based as well as ACPI-based platforms.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Rob Herring]

On Mon, May 9, 2022 at 10:56 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 5/9/22 07:16, Clément Léger wrote:
> > Le Fri, 6 May 2022 13:33:22 -0500,
> > Frank Rowand <frowand.list@gmail.com> a écrit :
> >
> >> On 4/27/22 04:44, Clément Léger wrote:
> >>> This series adds foundation work to support the lan9662 PCIe card.
> >>> This card is meant to be used an ethernet switch with 2 x RJ45
> >>> ports and 2 x 2.5G SFPs. The lan966x SoCs can be used in two
> >>> different ways:
> >>>
> >>>  - It can run Linux by itself, on ARM64 cores included in the SoC.
> >>> This use-case of the lan966x is currently being upstreamed, using a
> >>>    traditional Device Tree representation of the lan996x HW blocks
> >>> [1] A number of drivers for the different IPs of the SoC have
> >>> already been merged in upstream Linux.
> >>>
> >>>  - It can be used as a PCIe endpoint, connected to a separate
> >>> platform that acts as the PCIe root complex. In this case, all the
> >>> devices that are embedded on this SoC are exposed through PCIe BARs
> >>> and the ARM64 cores of the SoC are not used. Since this is a PCIe
> >>> card, it can be plugged on any platform, of any architecture
> >>> supporting PCIe.
> >>>
> >>> The problem that arose is that we want to reuse all the existing OF
> >>> compatible drivers that are used when in SoC mode to instantiate the
> >>> PCI device when in PCIe endpoint mode.
> >>>
> >>> A previous attempt to tackle this problem was made using fwnode [1].
> >>> However, this proved being way too invasive and it required
> >>> modifications in both subsystems and drivers to support fwnode.
> >>> First series did not lead to a consensus and multiple ideas to
> >>> support this use-case were mentioned (ACPI overlay, fwnode,
> >>> device-tree overlay). Since it only seemed that fwnode was not a
> >>> totally silly idea, we continued on this way.
> >>>
> >>> However, on the series that added fwnode support to the reset
> >>> subsystem, Rob Herring mentioned the fact that OF overlay might
> >>> actually be the best way to probe PCI devices and populate platform
> >>> drivers using this overlay. He also provided a branch containing
> >>> some commits that helped
> >>
> >> I need to go look at the various email threads mentioned above before
> >> I continue reading this patch series.
> >>
> >> I do have serious concerns with this approach.  I need to investigate
> >> more fully before I can determine whether the concerns are addressed
> >> sufficiently.
> >>
> >> To give some background to my longstanding response to similar
> >> proposals, here is my old statement from
> >> https://elinux.org/Device_Tree_Reference:
> >>
> >>    Overlays
> >>    Mainline Linux Support
> >>    Run time overlay apply and run time overlay remove from user space
> >> are not supported in the mainline kernel. There   are out of tree
> >> patches to implement this feature via an overlay manager. The overlay
> >> manager is used successfully by many users for specific overlays on
> >> specific boards with specific environments and use cases. However,
> >> there are many issues with the Linux kernel overlay implementation
> >> due to incomplete and incorrect code. The overlay manager has not
> >> been accepted in mainline due to these issues. Once these issues are
> >> resolved, it is expected that some method of run time overlay apply
> >> and overlay removal from user space will be supported by the Linux
> >> kernel.
> >>
> >>    There is a possibility that overlay apply and overlay remove
> >> support could be phased in slowly, feature by feature, as specific
> >> issues are resolved.
> >
> > Hi Frank,
> >
> > This work uses the kernel space interface (of_overlay_fdt_apply())
> > and the device tree overlay is builtin the driver. This interface was
> > used until recently by rcu-dcar driver. While the only user (sic),
> > this seems to work pretty well and I was able to use it successfully.
>
> Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
> was explicitly recognized as a grandfathered exception, and not an
> example for other users.  It was finally removed in 5.18-rc1.

What API are folks supposed to use exactly? That's the only API to
apply an overlay. I thought the FPGA mgr code was using it too, but
it's not. It doesn't look to me like the upstream code there even
works as nothing applies the overlays AFAICT. If there are no in
kernel users applying overlays, then let's remove the overlay code. I
hear it has lots of problems.

I am *way* more comfortable with driver specific applying of overlays
than any generic mechanism. I don't think we'll ever have a generic
mechanism. At least not one that doesn't end up with the same usage
constraints driver specific cases would have.


> You may have used of_overlay_fdt_apply() in a specific use case at
> a specific kernel version, but if you read through the references
> I provided you will find that applying overlays after the kernel
> boots is a fragile endeavor, with expectations of bugs and problems
> being exposed as usage is changed (simple example is that my adding
> some overlay notifier unittests exposed yet another memory leak).

The exception being specific drivers that are only applying overlays
isolated to their device as this usecase is. The usecase here is
entirely self-contained. The only base tree is only what's needed to
represent the PCI device.

> The reference that I provided also shows how the overlay code is
> being improved over time.  Even with improvements, it will remain
> fragile.
>
> >
> > Moreover, this support targets at using this with PCI devices. This
> > devices are really well contained and do not interfere with other
> > devices. This actually consists in adding a complete subtree into the
> > existing device-tree and thus it limits the interactions between
> > potentially platform provided devices and PCI ones.
>
> Yes, that it is very important that you have described this fact, both
> here and in other emails.  Thank you for that information, it does help
> understanding the alternatives.
>
> I've hesitated in recommending a specific solution before better
> understanding the architecture of your pcie board and drivers, but
> I've delayed too long, so I am going to go ahead and mention one
> possibility at the risk of not yet fully understanding the situation.
>
> On the surface, it appears that your need might be well met by having
> a base devicetree that describes all of the pcie nodes, but with each
> node having a status of "disabled" so that they will not be used.
> Have a devicetree overlay describing the pcie card (as you proposed),
> where the overlay also includes a status of "ok" for the pcie node.
> Applying the overlay, with a method of redirecting the target to a
> specific pcie node would change the status of the pcie node to enable
> its use.  (You have already proposed a patch to modify of_overlay_fdt_apply()
> to allow a modified target, so not a new concept from me.)  My suggestion
> is to apply the overlay devicetree to the base devicetree before the
> combined FDT devicetree is passed to the kernel at boot.  The overlay
> apply could be done by several different entities.  It could be before
> the bootloader executes, it could be done by the bootloader, it could
> be done by a shim between the bootloader and the kernel.  This method
> avoids all of the issues of applying an overlay to a running system
> that I find problematic.  It is also a method used by the U-boot
> bootloader, as an example.

Adding a layer, the solution to all problems...

I don't think that's a workable solution unless all the components are
in one party's control. Given the desire to work on ACPI and DT based
systems, that doesn't sound like the case here.

> The other big issue is mixing ACPI and devicetree on a single system.
> Historically, the Linux devicetree community has not been receptive
> to the ides of that mixture.  Your example might be a specific case
> where the two can be isolated from each other, or maybe not.  (For
> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
> ACPI and devicetree is a recipe for disaster in the general case.

The idea here is what is described by ACPI and DT are disjoint which I
think we can enforce. Enforcement comes from fwnode assuming it has
either an ACPI or a DT handle, but not both.

> More to come later as I finish reading through the various threads.

There is also the Xilinx folks wanting to support their PCI FPGA card
with DT for the FPGA contents on both ACPI and DT systems.

Rob

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Frank Rowand]

On 5/9/22 11:09, Clément Léger wrote:
> Le Mon, 9 May 2022 10:56:36 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>>> Hi Frank,
>>>
>>> This work uses the kernel space interface (of_overlay_fdt_apply())
>>> and the device tree overlay is builtin the driver. This interface
>>> was used until recently by rcu-dcar driver. While the only user
>>> (sic), this seems to work pretty well and I was able to use it
>>> successfully.  
>>
>> Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
>> was explicitly recognized as a grandfathered exception, and not an
>> example for other users.  It was finally removed in 5.18-rc1.
> 
> I noticed that unfortunately.
> 
>>
>> You may have used of_overlay_fdt_apply() in a specific use case at
>> a specific kernel version, but if you read through the references
>> I provided you will find that applying overlays after the kernel
>> boots is a fragile endeavor, with expectations of bugs and problems
>> being exposed as usage is changed (simple example is that my adding
>> some overlay notifier unittests exposed yet another memory leak).
>>
>> The reference that I provided also shows how the overlay code is
>> being improved over time.  Even with improvements, it will remain
>> fragile.
> 
> Acked.
> 
>>
>>>
>>> Moreover, this support targets at using this with PCI devices. This
>>> devices are really well contained and do not interfere with other
>>> devices. This actually consists in adding a complete subtree into
>>> the existing device-tree and thus it limits the interactions between
>>> potentially platform provided devices and PCI ones.  
>>
>> Yes, that it is very important that you have described this fact, both
>> here and in other emails.  Thank you for that information, it does
>> help understanding the alternatives.
>>
>> I've hesitated in recommending a specific solution before better
>> understanding the architecture of your pcie board and drivers, but
>> I've delayed too long, so I am going to go ahead and mention one
>> possibility at the risk of not yet fully understanding the situation.
>>
>> On the surface, it appears that your need might be well met by having
>> a base devicetree that describes all of the pcie nodes, but with each
>> node having a status of "disabled" so that they will not be used.
>> Have a devicetree overlay describing the pcie card (as you proposed),
>> where the overlay also includes a status of "ok" for the pcie node.
>> Applying the overlay, with a method of redirecting the target to a
>> specific pcie node would change the status of the pcie node to enable
>> its use.  (You have already proposed a patch to modify
>> of_overlay_fdt_apply() to allow a modified target, so not a new
>> concept from me.)  My suggestion is to apply the overlay devicetree
>> to the base devicetree before the combined FDT devicetree is passed
>> to the kernel at boot.  The overlay apply could be done by several
>> different entities.  It could be before the bootloader executes, it
>> could be done by the bootloader, it could be done by a shim between
>> the bootloader and the kernel.  This method avoids all of the issues
>> of applying an overlay to a running system that I find problematic.
>> It is also a method used by the U-boot bootloader, as an example.
> 

Apologies if my following questions have already been answered in the
other threads...

> Ok, that is actually possible on a system that is given a device-tree
> by the bootloader. But on a system that is desrcibed using ACPI (such
> as the x86), this is much more difficult (at least to my knowledge)...
> We want this feature to be easy to use for the end user. Adding such
> configuration which also differs between various architecture is
> clearly not so easy to setup.

Are you trying to make your card work on any ACPI based system (x86,
x86-64, etc)?  Or do you have a specific model of computer that you
want to make this work on for a specific customer or appliance?

If for many arbitrary systems, can you limit it to one architecture
or sub-architecture?

> 
> Moreover, since the PCI is meant to be "Plug and Play", such
> configuration would completely break that. If the user switches the
> PCIe card from one slot to another, the bootloader configuration will
> need to be modified. This seems a big no way for me (and for the user).

Yes.  I was envisioning the pre-bootloader, bootloader, or Linux pre-boot
shim dynamically determining the slot containing the card, and applying
the overlay devicetree to the base devicetree, retargeting the overlay
to the proper location, before the Linux boot.

The base devicetree would be for a specific type of machine or family
of machines, just as is the case for all devicetree based systems.

> 
>>
>> The other big issue is mixing ACPI and devicetree on a single system.
>> Historically, the Linux devicetree community has not been receptive
>> to the ides of that mixture.  Your example might be a specific case
>> where the two can be isolated from each other, or maybe not.  (For
>> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
>> ACPI and devicetree is a recipe for disaster in the general case.
> 
> Agreed, on that fact, it did raised some eyebrows, and it was for that
> specific concern that initially, I proposed the fwnode solution.
> Honestly, the fwnode conversion represent a lot of work (hundreds of
> lines easily) + requires a conversion of all the subsystem that are not
> fwnode ready (spoiler: almost all of them are not ready). 
> 
> After implementing Rob's solution, the device-tree overlay really seems
> the cleaner to me and requires much less modifications.
> 
>>
>> More to come later as I finish reading through the various threads.
> 
> Ok, thanks for your time !

Your welcome.  I'll keep looking deeper into the previous threads.

-Frank

> 
> Clément
> 
>>
>> -Frank
> 
> .

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Frank Rowand]

On 5/9/22 12:00, Andy Shevchenko wrote:
> On Mon, May 09, 2022 at 06:09:17PM +0200, Clément Léger wrote:
>> Le Mon, 9 May 2022 10:56:36 -0500,
>> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
> ...
> 
>>> On the surface, it appears that your need might be well met by having
>>> a base devicetree that describes all of the pcie nodes, but with each
>>> node having a status of "disabled" so that they will not be used.
>>> Have a devicetree overlay describing the pcie card (as you proposed),
>>> where the overlay also includes a status of "ok" for the pcie node.
>>> Applying the overlay, with a method of redirecting the target to a
>>> specific pcie node would change the status of the pcie node to enable
>>> its use.  (You have already proposed a patch to modify
>>> of_overlay_fdt_apply() to allow a modified target, so not a new
>>> concept from me.)  My suggestion is to apply the overlay devicetree
>>> to the base devicetree before the combined FDT devicetree is passed
>>> to the kernel at boot.  The overlay apply could be done by several
>>> different entities.  It could be before the bootloader executes, it
>>> could be done by the bootloader, it could be done by a shim between
>>> the bootloader and the kernel.  This method avoids all of the issues
>>> of applying an overlay to a running system that I find problematic.
>>> It is also a method used by the U-boot bootloader, as an example.
>>
>> Ok, that is actually possible on a system that is given a device-tree
>> by the bootloader. But on a system that is desrcibed using ACPI (such
>> as the x86), this is much more difficult (at least to my knowledge)...
>> We want this feature to be easy to use for the end user. Adding such
>> configuration which also differs between various architecture is
>> clearly not so easy to setup.
>>
>> Moreover, since the PCI is meant to be "Plug and Play", such
>> configuration would completely break that. If the user switches the
>> PCIe card from one slot to another, the bootloader configuration will
>> need to be modified. This seems a big no way for me (and for the user).
> 
> The main problem here is that Linux does not support hotplugging for the
> devices behind non-hotpluggable buses. You need to develop something to
> say that the device tree (in terms of hardware) can morph at run-time
> transparently to the user. I think the closest one is what FPGA does,
> or at least should do.

That is something I was not aware of yet.  Is the card in question a
hotpluggable card?  Do the systems that you anticipate plugging the
card into support hotplug?

-Frank

> 
>>> The other big issue is mixing ACPI and devicetree on a single system.
>>> Historically, the Linux devicetree community has not been receptive
>>> to the ides of that mixture.  Your example might be a specific case
>>> where the two can be isolated from each other, or maybe not.  (For
>>> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
>>> ACPI and devicetree is a recipe for disaster in the general case.
>>
>> Agreed, on that fact, it did raised some eyebrows, and it was for that
>> specific concern that initially, I proposed the fwnode solution.
>> Honestly, the fwnode conversion represent a lot of work (hundreds of
>> lines easily) + requires a conversion of all the subsystem that are not
>> fwnode ready (spoiler: almost all of them are not ready). 
> 
> In either case you need to provide a format that would be suitable for
> DT-based as well as ACPI-based platforms.
> 

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Frank Rowand]

On 5/9/22 13:36, Rob Herring wrote:
> On Mon, May 9, 2022 at 10:56 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 5/9/22 07:16, Clément Léger wrote:
>>> Le Fri, 6 May 2022 13:33:22 -0500,
>>> Frank Rowand <frowand.list@gmail.com> a écrit :
>>>
>>>> On 4/27/22 04:44, Clément Léger wrote:
>>>>> This series adds foundation work to support the lan9662 PCIe card.
>>>>> This card is meant to be used an ethernet switch with 2 x RJ45
>>>>> ports and 2 x 2.5G SFPs. The lan966x SoCs can be used in two
>>>>> different ways:
>>>>>
>>>>>  - It can run Linux by itself, on ARM64 cores included in the SoC.
>>>>> This use-case of the lan966x is currently being upstreamed, using a
>>>>>    traditional Device Tree representation of the lan996x HW blocks
>>>>> [1] A number of drivers for the different IPs of the SoC have
>>>>> already been merged in upstream Linux.
>>>>>
>>>>>  - It can be used as a PCIe endpoint, connected to a separate
>>>>> platform that acts as the PCIe root complex. In this case, all the
>>>>> devices that are embedded on this SoC are exposed through PCIe BARs
>>>>> and the ARM64 cores of the SoC are not used. Since this is a PCIe
>>>>> card, it can be plugged on any platform, of any architecture
>>>>> supporting PCIe.
>>>>>
>>>>> The problem that arose is that we want to reuse all the existing OF
>>>>> compatible drivers that are used when in SoC mode to instantiate the
>>>>> PCI device when in PCIe endpoint mode.
>>>>>
>>>>> A previous attempt to tackle this problem was made using fwnode [1].
>>>>> However, this proved being way too invasive and it required
>>>>> modifications in both subsystems and drivers to support fwnode.
>>>>> First series did not lead to a consensus and multiple ideas to
>>>>> support this use-case were mentioned (ACPI overlay, fwnode,
>>>>> device-tree overlay). Since it only seemed that fwnode was not a
>>>>> totally silly idea, we continued on this way.
>>>>>
>>>>> However, on the series that added fwnode support to the reset
>>>>> subsystem, Rob Herring mentioned the fact that OF overlay might
>>>>> actually be the best way to probe PCI devices and populate platform
>>>>> drivers using this overlay. He also provided a branch containing
>>>>> some commits that helped
>>>>
>>>> I need to go look at the various email threads mentioned above before
>>>> I continue reading this patch series.
>>>>
>>>> I do have serious concerns with this approach.  I need to investigate
>>>> more fully before I can determine whether the concerns are addressed
>>>> sufficiently.
>>>>
>>>> To give some background to my longstanding response to similar
>>>> proposals, here is my old statement from
>>>> https://elinux.org/Device_Tree_Reference:
>>>>
>>>>    Overlays
>>>>    Mainline Linux Support
>>>>    Run time overlay apply and run time overlay remove from user space
>>>> are not supported in the mainline kernel. There   are out of tree
>>>> patches to implement this feature via an overlay manager. The overlay
>>>> manager is used successfully by many users for specific overlays on
>>>> specific boards with specific environments and use cases. However,
>>>> there are many issues with the Linux kernel overlay implementation
>>>> due to incomplete and incorrect code. The overlay manager has not
>>>> been accepted in mainline due to these issues. Once these issues are
>>>> resolved, it is expected that some method of run time overlay apply
>>>> and overlay removal from user space will be supported by the Linux
>>>> kernel.
>>>>
>>>>    There is a possibility that overlay apply and overlay remove
>>>> support could be phased in slowly, feature by feature, as specific
>>>> issues are resolved.
>>>
>>> Hi Frank,
>>>
>>> This work uses the kernel space interface (of_overlay_fdt_apply())
>>> and the device tree overlay is builtin the driver. This interface was
>>> used until recently by rcu-dcar driver. While the only user (sic),
>>> this seems to work pretty well and I was able to use it successfully.
>>
>> Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
>> was explicitly recognized as a grandfathered exception, and not an
>> example for other users.  It was finally removed in 5.18-rc1.
> 
> What API are folks supposed to use exactly? That's the only API to
> apply an overlay.

Yes, that is the API to designed to be used if overlays are applied
after the kernel has booted.

> I thought the FPGA mgr code was using it too, but

That was my understanding too.

> it's not. It doesn't look to me like the upstream code there even
> works as nothing applies the overlays AFAICT. If there are no in
> kernel users applying overlays, then let's remove the overlay code. I
> hear it has lots of problems.

I would not object to doing that.  But I suspect there are other people
who might.  I much prefer that overlays be applied before boot.

> 
> I am *way* more comfortable with driver specific applying of overlays
> than any generic mechanism. I don't think we'll ever have a generic
> mechanism. At least not one that doesn't end up with the same usage
> constraints driver specific cases would have.

Yes, generic use leads to a lot of the complications of implementing
runtime overlays.

> 
> 
>> You may have used of_overlay_fdt_apply() in a specific use case at
>> a specific kernel version, but if you read through the references
>> I provided you will find that applying overlays after the kernel
>> boots is a fragile endeavor, with expectations of bugs and problems
>> being exposed as usage is changed (simple example is that my adding
>> some overlay notifier unittests exposed yet another memory leak).
> 
> The exception being specific drivers that are only applying overlays
> isolated to their device as this usecase is. The usecase here is
> entirely self-contained. The only base tree is only what's needed to
> represent the PCI device.

This example might be the best example of such a use case.  As it is
claimed that the drivers for the devices on the board do not access
anything on the host other than the pcie slot.

> 
>> The reference that I provided also shows how the overlay code is
>> being improved over time.  Even with improvements, it will remain
>> fragile.
>>
>>>
>>> Moreover, this support targets at using this with PCI devices. This
>>> devices are really well contained and do not interfere with other
>>> devices. This actually consists in adding a complete subtree into the
>>> existing device-tree and thus it limits the interactions between
>>> potentially platform provided devices and PCI ones.
>>
>> Yes, that it is very important that you have described this fact, both
>> here and in other emails.  Thank you for that information, it does help
>> understanding the alternatives.
>>
>> I've hesitated in recommending a specific solution before better
>> understanding the architecture of your pcie board and drivers, but
>> I've delayed too long, so I am going to go ahead and mention one
>> possibility at the risk of not yet fully understanding the situation.
>>
>> On the surface, it appears that your need might be well met by having
>> a base devicetree that describes all of the pcie nodes, but with each
>> node having a status of "disabled" so that they will not be used.
>> Have a devicetree overlay describing the pcie card (as you proposed),
>> where the overlay also includes a status of "ok" for the pcie node.
>> Applying the overlay, with a method of redirecting the target to a
>> specific pcie node would change the status of the pcie node to enable
>> its use.  (You have already proposed a patch to modify of_overlay_fdt_apply()
>> to allow a modified target, so not a new concept from me.)  My suggestion
>> is to apply the overlay devicetree to the base devicetree before the
>> combined FDT devicetree is passed to the kernel at boot.  The overlay
>> apply could be done by several different entities.  It could be before
>> the bootloader executes, it could be done by the bootloader, it could
>> be done by a shim between the bootloader and the kernel.  This method
>> avoids all of the issues of applying an overlay to a running system
>> that I find problematic.  It is also a method used by the U-boot
>> bootloader, as an example.
> 
> Adding a layer, the solution to all problems...

< insert xkcd reference here >  :-)

> 
> I don't think that's a workable solution unless all the components are
> in one party's control. Given the desire to work on ACPI and DT based
> systems, that doesn't sound like the case here.

That is the motivation behind my questions of how generic or targeted
the use of this one specific card is.

A pre-boot shim that discovers the card and applies the overlay could
be somewhat generic to ACPI systems.

Is the overlay approach also being proposed for DT based systems?

> 
>> The other big issue is mixing ACPI and devicetree on a single system.
>> Historically, the Linux devicetree community has not been receptive
>> to the ides of that mixture.  Your example might be a specific case
>> where the two can be isolated from each other, or maybe not.  (For
>> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
>> ACPI and devicetree is a recipe for disaster in the general case.
> 
> The idea here is what is described by ACPI and DT are disjoint which I
> think we can enforce. Enforcement comes from fwnode assuming it has
> either an ACPI or a DT handle, but not both.

I thought the intent was to use DT API drivers, not fwnode API drivers.
And I thought the card was not to be described by ACPI, so there would
not be any ACPI fwnode info for the card _and_ fwnode is somewhat
irrelevant since the drivers are using the DT API.  So the enforcement
you mention could be implemented by the overlay apply code verifying
that there is no ACPI fwnode for any DT node in the overlay.  So not
an _assumption_, but a testable rule.

That is a long way of essentially agreeing with you.

> 
>> More to come later as I finish reading through the various threads.
> 
> There is also the Xilinx folks wanting to support their PCI FPGA card
> with DT for the FPGA contents on both ACPI and DT systems.

Is that the XRT Alveo driver infrastructure thread?  I was not cc:ed
on that thread, and just recently stumbled upon it.  Yet another
thread that is on my to-read list.

> 
> Rob

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Andy Shevchenko]

On Mon, May 9, 2022 at 10:36 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 5/9/22 12:00, Andy Shevchenko wrote:
> > On Mon, May 09, 2022 at 06:09:17PM +0200, Clément Léger wrote:
> >> Le Mon, 9 May 2022 10:56:36 -0500,
> >> Frank Rowand <frowand.list@gmail.com> a écrit :
> >
> > ...
> >
> >>> On the surface, it appears that your need might be well met by having
> >>> a base devicetree that describes all of the pcie nodes, but with each
> >>> node having a status of "disabled" so that they will not be used.
> >>> Have a devicetree overlay describing the pcie card (as you proposed),
> >>> where the overlay also includes a status of "ok" for the pcie node.
> >>> Applying the overlay, with a method of redirecting the target to a
> >>> specific pcie node would change the status of the pcie node to enable
> >>> its use.  (You have already proposed a patch to modify
> >>> of_overlay_fdt_apply() to allow a modified target, so not a new
> >>> concept from me.)  My suggestion is to apply the overlay devicetree
> >>> to the base devicetree before the combined FDT devicetree is passed
> >>> to the kernel at boot.  The overlay apply could be done by several
> >>> different entities.  It could be before the bootloader executes, it
> >>> could be done by the bootloader, it could be done by a shim between
> >>> the bootloader and the kernel.  This method avoids all of the issues
> >>> of applying an overlay to a running system that I find problematic.
> >>> It is also a method used by the U-boot bootloader, as an example.
> >>
> >> Ok, that is actually possible on a system that is given a device-tree
> >> by the bootloader. But on a system that is desrcibed using ACPI (such
> >> as the x86), this is much more difficult (at least to my knowledge)...
> >> We want this feature to be easy to use for the end user. Adding such
> >> configuration which also differs between various architecture is
> >> clearly not so easy to setup.
> >>
> >> Moreover, since the PCI is meant to be "Plug and Play", such
> >> configuration would completely break that. If the user switches the
> >> PCIe card from one slot to another, the bootloader configuration will
> >> need to be modified. This seems a big no way for me (and for the user).
> >
> > The main problem here is that Linux does not support hotplugging for the
> > devices behind non-hotpluggable buses. You need to develop something to
> > say that the device tree (in terms of hardware) can morph at run-time
> > transparently to the user. I think the closest one is what FPGA does,
> > or at least should do.
>
> That is something I was not aware of yet.  Is the card in question a
> hotpluggable card?  Do the systems that you anticipate plugging the
> card into support hotplug?

Any PCIe card is potentially hotpluggable (seems nobody actually cares
in 90%+ drivers in the Linux kernel). But what I have heard in a
thread (not this one IIRC) is that the card may have pluggable modules
and it would be nice to change configuration and notify OS somehow. I
might be mistaken if it's the case here or not.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Clément Léger]

Le Mon, 9 May 2022 15:07:18 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> > Ok, that is actually possible on a system that is given a
> > device-tree by the bootloader. But on a system that is desrcibed
> > using ACPI (such as the x86), this is much more difficult (at least
> > to my knowledge)... We want this feature to be easy to use for the
> > end user. Adding such configuration which also differs between
> > various architecture is clearly not so easy to setup.  
> 
> Are you trying to make your card work on any ACPI based system (x86,
> x86-64, etc)?  Or do you have a specific model of computer that you
> want to make this work on for a specific customer or appliance?

There is no particular appliance/architecture targeted. This card should
work with any system that can support PCIe.

> 
> If for many arbitrary systems, can you limit it to one architecture
> or sub-architecture?

Previous answer does rule out this one.

> 
> > 
> > Moreover, since the PCI is meant to be "Plug and Play", such
> > configuration would completely break that. If the user switches the
> > PCIe card from one slot to another, the bootloader configuration
> > will need to be modified. This seems a big no way for me (and for
> > the user).  
> 
> Yes.  I was envisioning the pre-bootloader, bootloader, or Linux
> pre-boot shim dynamically determining the slot containing the card,
> and applying the overlay devicetree to the base devicetree,
> retargeting the overlay to the proper location, before the Linux boot.

Ok, this is however not doable on many already existing platforms since
it would require to adapt this system for whatever bootloader is
present on the platform. Moreover, AFAIK some platforms bootchain (x86)
are hardly modifiable.

> 
> The base devicetree would be for a specific type of machine or family
> of machines, just as is the case for all devicetree based systems.
> 
> >   
> >>
> >> The other big issue is mixing ACPI and devicetree on a single
> >> system. Historically, the Linux devicetree community has not been
> >> receptive to the ides of that mixture.  Your example might be a
> >> specific case where the two can be isolated from each other, or
> >> maybe not.  (For disclosure, I am essentially ACPI ignorant.)  I
> >> suspect that mixing ACPI and devicetree is a recipe for disaster
> >> in the general case.  
> > 
> > Agreed, on that fact, it did raised some eyebrows, and it was for
> > that specific concern that initially, I proposed the fwnode
> > solution. Honestly, the fwnode conversion represent a lot of work
> > (hundreds of lines easily) + requires a conversion of all the
> > subsystem that are not fwnode ready (spoiler: almost all of them
> > are not ready). 
> > 
> > After implementing Rob's solution, the device-tree overlay really
> > seems the cleaner to me and requires much less modifications.
> >   
> >>
> >> More to come later as I finish reading through the various
> >> threads.  
> > 
> > Ok, thanks for your time !  
> 
> Your welcome.  I'll keep looking deeper into the previous threads.
> 
> -Frank
> 
> > 
> > Clément
> >   
> >>
> >> -Frank  
> > 
> > .  
> 

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Christoph Hellwig]

On Mon, May 09, 2022 at 10:40:12PM +0200, Andy Shevchenko wrote:
> > hotpluggable card?  Do the systems that you anticipate plugging the
> > card into support hotplug?
> 
> Any PCIe card is potentially hotpluggable (seems nobody actually cares
> in 90%+ drivers in the Linux kernel). But what I have heard in a
> thread (not this one IIRC) is that the card may have pluggable modules
> and it would be nice to change configuration and notify OS somehow. I
> might be mistaken if it's the case here or not.

Well.  M.2 for example is not hotpluggable, as are soldered on BGA
devices or a lot of not quite PCIe devices that actually sit on CPUs
or shipset components.  But for all but the last category an upstream
bridge could still be hot plugged, so not supporting it in drivers is
indeed generally speaking a bad idea.

Re: [PATCH 0/3] add dynamic PCI device of_node creation for overlay

[Rob Herring]

On Mon, May 09, 2022 at 03:35:45PM -0500, Frank Rowand wrote:
> On 5/9/22 13:36, Rob Herring wrote:
> > On Mon, May 9, 2022 at 10:56 AM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 5/9/22 07:16, Clément Léger wrote:
> >>> Le Fri, 6 May 2022 13:33:22 -0500,
> >>> Frank Rowand <frowand.list@gmail.com> a écrit :
> >>>
> >>>> On 4/27/22 04:44, Clément Léger wrote:
> >>>>> This series adds foundation work to support the lan9662 PCIe card.
> >>>>> This card is meant to be used an ethernet switch with 2 x RJ45
> >>>>> ports and 2 x 2.5G SFPs. The lan966x SoCs can be used in two
> >>>>> different ways:

[...]

> >>> This work uses the kernel space interface (of_overlay_fdt_apply())
> >>> and the device tree overlay is builtin the driver. This interface was
> >>> used until recently by rcu-dcar driver. While the only user (sic),
> >>> this seems to work pretty well and I was able to use it successfully.
> >>
> >> Yes, of_overlay_fdt_apply() was used by one driver.  But that driver
> >> was explicitly recognized as a grandfathered exception, and not an
> >> example for other users.  It was finally removed in 5.18-rc1.
> > 
> > What API are folks supposed to use exactly? That's the only API to
> > apply an overlay.
> 
> Yes, that is the API to designed to be used if overlays are applied
> after the kernel has booted.
> 
> > I thought the FPGA mgr code was using it too, but
> 
> That was my understanding too.
> 
> > it's not. It doesn't look to me like the upstream code there even
> > works as nothing applies the overlays AFAICT. If there are no in
> > kernel users applying overlays, then let's remove the overlay code. I
> > hear it has lots of problems.
> 
> I would not object to doing that.  But I suspect there are other people
> who might.  I much prefer that overlays be applied before boot.

Strictly speaking, we don't keep APIs with no users. Perhaps removing or 
threatening to remove would spur on some work in your overlay issues 
list. :)


> >>> Moreover, this support targets at using this with PCI devices. This
> >>> devices are really well contained and do not interfere with other
> >>> devices. This actually consists in adding a complete subtree into the
> >>> existing device-tree and thus it limits the interactions between
> >>> potentially platform provided devices and PCI ones.
> >>
> >> Yes, that it is very important that you have described this fact, both
> >> here and in other emails.  Thank you for that information, it does help
> >> understanding the alternatives.
> >>
> >> I've hesitated in recommending a specific solution before better
> >> understanding the architecture of your pcie board and drivers, but
> >> I've delayed too long, so I am going to go ahead and mention one
> >> possibility at the risk of not yet fully understanding the situation.
> >>
> >> On the surface, it appears that your need might be well met by having
> >> a base devicetree that describes all of the pcie nodes, but with each
> >> node having a status of "disabled" so that they will not be used.
> >> Have a devicetree overlay describing the pcie card (as you proposed),
> >> where the overlay also includes a status of "ok" for the pcie node.
> >> Applying the overlay, with a method of redirecting the target to a
> >> specific pcie node would change the status of the pcie node to enable
> >> its use.  (You have already proposed a patch to modify of_overlay_fdt_apply()
> >> to allow a modified target, so not a new concept from me.)  My suggestion
> >> is to apply the overlay devicetree to the base devicetree before the
> >> combined FDT devicetree is passed to the kernel at boot.  The overlay
> >> apply could be done by several different entities.  It could be before
> >> the bootloader executes, it could be done by the bootloader, it could
> >> be done by a shim between the bootloader and the kernel.  This method
> >> avoids all of the issues of applying an overlay to a running system
> >> that I find problematic.  It is also a method used by the U-boot
> >> bootloader, as an example.
> > 
> > Adding a layer, the solution to all problems...
> 
> < insert xkcd reference here >  :-)
> 
> > 
> > I don't think that's a workable solution unless all the components are
> > in one party's control. Given the desire to work on ACPI and DT based
> > systems, that doesn't sound like the case here.
> 
> That is the motivation behind my questions of how generic or targeted
> the use of this one specific card is.
> 
> A pre-boot shim that discovers the card and applies the overlay could
> be somewhat generic to ACPI systems.
> 
> Is the overlay approach also being proposed for DT based systems?

Yes, the intent is it would work for either.

Another usecase I have in mind are USB to serial chips with downstream 
GPIOs, I2C, SPI, etc. where you want to describe downstream devices. And 
then you plug in more than 1...

> >> The other big issue is mixing ACPI and devicetree on a single system.
> >> Historically, the Linux devicetree community has not been receptive
> >> to the ides of that mixture.  Your example might be a specific case
> >> where the two can be isolated from each other, or maybe not.  (For
> >> disclosure, I am essentially ACPI ignorant.)  I suspect that mixing
> >> ACPI and devicetree is a recipe for disaster in the general case.
> > 
> > The idea here is what is described by ACPI and DT are disjoint which I
> > think we can enforce. Enforcement comes from fwnode assuming it has
> > either an ACPI or a DT handle, but not both.
> 
> I thought the intent was to use DT API drivers, not fwnode API drivers.

Yes.

> And I thought the card was not to be described by ACPI, so there would
> not be any ACPI fwnode info for the card _and_ fwnode is somewhat
> irrelevant since the drivers are using the DT API.

The card would not be described, but the PCI host bridge would likely 
be and that may be where the conflict is. Though I think PCI hosts are 
described differently from devices that end up with a fwnode handle in 
ACPI. 

My current thinking is the whole PCI bus structure from the card 
device up the tree needs to be created in the base DT. Maybe we could 
create just a fake parent for the device and avoid the above issue, but 
I'd guess that would just create other issues.

>  So the enforcement
> you mention could be implemented by the overlay apply code verifying
> that there is no ACPI fwnode for any DT node in the overlay.  So not
> an _assumption_, but a testable rule.

I think we'd need something where ever device fwnode ptrs are getting 
set. Not entirely sure yet.

> That is a long way of essentially agreeing with you.
> 
> > 
> >> More to come later as I finish reading through the various threads.
> > 
> > There is also the Xilinx folks wanting to support their PCI FPGA card
> > with DT for the FPGA contents on both ACPI and DT systems.
> 
> Is that the XRT Alveo driver infrastructure thread?  I was not cc:ed
> on that thread, and just recently stumbled upon it.  Yet another
> thread that is on my to-read list.

Yes, and I've had a call about it in the 'system DT' call. 

Rob

Re: [PATCH 1/3] of: always populate a root node

[Frank Rowand]

On 5/3/22 08:45, Rob Herring wrote:
> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:
>> When enabling CONFIG_OF on a platform where of_root is not populated by
>> firmware, we end up without a root node. In order to apply overlays and
>> create subnodes of the root node, we need one. This commit creates an
>> empty root node if not present.
> 
> The existing unittest essentially does the same thing for running the 
> tests on non-DT systems. It should be modified to use this support 
> instead. Maybe that's just removing the unittest code that set of_root.
> 
> I expect Frank will have some comments.

My preference would be for unflatten_and_copy_device_tree() to
use a compiled in FDT that only contains a root node, in the
case that no valid device tree is found (in other words,
"if (!initial_boot_params)".

unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
after unflattening the device tree passed into the booting kernel.  This
step is needed for a specific portion of the unittests.

I'm still looking at the bigger picture of using overlays for the PCIe
card, so more comments will be coming about that bigger picture.

-Frank

> 
>> Co-developed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
>> ---
>>  drivers/of/base.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index e7d92b67cb8a..6b8584c39f73 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -177,6 +177,19 @@ void __init of_core_init(void)
>>  		pr_err("failed to register existing nodes\n");
>>  		return;
>>  	}
>> +
>> +	if (!of_root) {
>> +		of_root = kzalloc(sizeof(*of_root), GFP_KERNEL);
>> +		if (!of_root) {
>> +			mutex_unlock(&of_mutex);
>> +			pr_err("failed to create root node\n");
>> +			return;
>> +		}
>> +
>> +		of_root->full_name = "/";
>> +		of_node_init(of_root);
>> +	}
>> +
>>  	for_each_of_allnodes(np) {
>>  		__of_attach_node_sysfs(np);
>>  		if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
>> @@ -185,8 +198,7 @@ void __init of_core_init(void)
>>  	mutex_unlock(&of_mutex);
>>  
>>  	/* Symlink in /proc as required by userspace ABI */
>> -	if (of_root)
>> -		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>> +	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>>  }
>>  
>>  static struct property *__of_find_property(const struct device_node *np,
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH 1/3] of: always populate a root node

[Clément Léger]

Le Mon, 16 May 2022 23:11:03 -0400,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 5/3/22 08:45, Rob Herring wrote:
> > On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:  
> >> When enabling CONFIG_OF on a platform where of_root is not populated by
> >> firmware, we end up without a root node. In order to apply overlays and
> >> create subnodes of the root node, we need one. This commit creates an
> >> empty root node if not present.  
> > 
> > The existing unittest essentially does the same thing for running the 
> > tests on non-DT systems. It should be modified to use this support 
> > instead. Maybe that's just removing the unittest code that set of_root.
> > 
> > I expect Frank will have some comments.  
> 
> My preference would be for unflatten_and_copy_device_tree() to
> use a compiled in FDT that only contains a root node, in the
> case that no valid device tree is found (in other words,
> "if (!initial_boot_params)".

Ok, so basically, instead of creating the root node manually, you
expect a device-tree which contains the following to be builtin the
kernel and unflattened if needed:

/ {

};

Maybe "chosen" and "aliases" nodes should also be provided as empty
nodes since the unittest are creating them anyway and the core DT code
also uses them.

Thanks,

Clément

> 
> unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
> after unflattening the device tree passed into the booting kernel.  This
> step is needed for a specific portion of the unittests.
> 
> I'm still looking at the bigger picture of using overlays for the PCIe
> card, so more comments will be coming about that bigger picture.
> 
> -Frank
> 


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

Re: [PATCH 1/3] of: always populate a root node

[Frank Rowand]

On 5/17/22 02:37, Clément Léger wrote:
> Le Mon, 16 May 2022 23:11:03 -0400,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>> On 5/3/22 08:45, Rob Herring wrote:
>>> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:  
>>>> When enabling CONFIG_OF on a platform where of_root is not populated by
>>>> firmware, we end up without a root node. In order to apply overlays and
>>>> create subnodes of the root node, we need one. This commit creates an
>>>> empty root node if not present.  
>>>
>>> The existing unittest essentially does the same thing for running the 
>>> tests on non-DT systems. It should be modified to use this support 
>>> instead. Maybe that's just removing the unittest code that set of_root.
>>>
>>> I expect Frank will have some comments.  
>>
>> My preference would be for unflatten_and_copy_device_tree() to
>> use a compiled in FDT that only contains a root node, in the
>> case that no valid device tree is found (in other words,
>> "if (!initial_boot_params)".
> 
> Ok, so basically, instead of creating the root node manually, you
> expect a device-tree which contains the following to be builtin the
> kernel and unflattened if needed:
> 
> / {
> 
> };

Yes.  If you agree with this I can create a patch to implement it.  I think
it is useful even stand alone from the rest of the series.

> 
> Maybe "chosen" and "aliases" nodes should also be provided as empty
> nodes since the unittest are creating them anyway and the core DT code
> also uses them.

No. Unittest does not create both of them (I'm pretty sure, but I'm not
going to double check).  If I recall correctly, unittest adds a property
in one of those two nodes, and thus implicitly creates the node if not
already present.  Unittest does populate internal pointers to those two
nodes if the nodes are present (otherwise the pointers will have the
value of null).  There is no need for the nodes to be present if empty.

-Frank

> 
> Thanks,
> 
> Clément
> 
>>
>> unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
>> after unflattening the device tree passed into the booting kernel.  This
>> step is needed for a specific portion of the unittests.
>>
>> I'm still looking at the bigger picture of using overlays for the PCIe
>> card, so more comments will be coming about that bigger picture.
>>
>> -Frank
>>
> 
> 

Re: [PATCH 1/3] of: always populate a root node

[Clément Léger]

Le Tue, 17 May 2022 11:03:41 -0400,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 5/17/22 02:37, Clément Léger wrote:
> > Le Mon, 16 May 2022 23:11:03 -0400,
> > Frank Rowand <frowand.list@gmail.com> a écrit :
> >   
> >> On 5/3/22 08:45, Rob Herring wrote:  
> >>> On Wed, Apr 27, 2022 at 11:45:00AM +0200, Clément Léger wrote:    
> >>>> When enabling CONFIG_OF on a platform where of_root is not populated by
> >>>> firmware, we end up without a root node. In order to apply overlays and
> >>>> create subnodes of the root node, we need one. This commit creates an
> >>>> empty root node if not present.    
> >>>
> >>> The existing unittest essentially does the same thing for running the 
> >>> tests on non-DT systems. It should be modified to use this support 
> >>> instead. Maybe that's just removing the unittest code that set of_root.
> >>>
> >>> I expect Frank will have some comments.    
> >>
> >> My preference would be for unflatten_and_copy_device_tree() to
> >> use a compiled in FDT that only contains a root node, in the
> >> case that no valid device tree is found (in other words,
> >> "if (!initial_boot_params)".  
> > 
> > Ok, so basically, instead of creating the root node manually, you
> > expect a device-tree which contains the following to be builtin the
> > kernel and unflattened if needed:
> > 
> > / {
> > 
> > };  
> 
> Yes.  If you agree with this I can create a patch to implement it.  I think
> it is useful even stand alone from the rest of the series.

If you want to implement this, feel free to do so, I'll (at least) be
able to test it.

> 
> > 
> > Maybe "chosen" and "aliases" nodes should also be provided as empty
> > nodes since the unittest are creating them anyway and the core DT code
> > also uses them.  
> 
> No. Unittest does not create both of them (I'm pretty sure, but I'm not
> going to double check).  If I recall correctly, unittest adds a property
> in one of those two nodes, and thus implicitly creates the node if not
> already present.  Unittest does populate internal pointers to those two
> nodes if the nodes are present (otherwise the pointers will have the
> value of null).  There is no need for the nodes to be present if empty.

Acked, makes sense.

Clément

> 
> -Frank
> 
> > 
> > Thanks,
> > 
> > Clément
> >   
> >>
> >> unflatten_and_copy_device_tree() calls unittest_unflatten_overlay_base()
> >> after unflattening the device tree passed into the booting kernel.  This
> >> step is needed for a specific portion of the unittests.
> >>
> >> I'm still looking at the bigger picture of using overlays for the PCIe
> >> card, so more comments will be coming about that bigger picture.
> >>
> >> -Frank
> >>  
> > 
> >   
> 



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

Re: [PATCH 2/3] PCI: of: create DT nodes for PCI devices if they do not exists

[Lizhi Hou]

On 5/4/22 06:43, Clément Léger wrote:
> Le Tue, 3 May 2022 17:53:53 -0500,
> Bjorn Helgaas <helgaas@kernel.org> a écrit :
>
>> In subject:
>>
>>    PCI: of: Create DT nodes ... if they do not exist
>>
>> The subject could be read as saying that you're going to create DT
>> nodes before the PCI devices exist, but I think you mean that when we
>> enumerate a PCI devices, we're *always* going to create a DT node for
>> it, even if the DT didn't mention it.
> Hi Bjorn,
>
> Indeed ! I'll modify that.

Linking the dynamic generated dt node to PCIe device through 
pci_dev->dev.of_node may cause issues. Kernel and driver code may check 
of_node pointer and run complete different code path if of_node is not 
NULL.

For example:  in of_irq_parse_pci(): 
https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/pci/of.c#L492

I encountered different issues when I tried to create a prototype. And I 
have sent all may questions/thoughts through 
https://lore.kernel.org/lkml/79e4c876-e5a4-861f-cfbc-c75ed1a07ddd@xilinx.com/#t

I am wondering what would be the right way to resolve it?


Thanks,

Lizhi

>
>> Maybe something like:
>>
>>    PCI: of: Create DT node for every PCI device
>>
>> although I see Rob thinks this should be done on demand instead of
>> doing it for every device, which sounds sensible to me.
> Agreed, I'll rework this series.
>
> Thanks,
>
>> On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
>>> In order to apply overlays to PCI device nodes, the nodes must first
>>> exist. This commit add support to populate a skeleton tree for PCI bus
>>> and devices. These nodes can then be used by drivers to apply overlays.
>> s/This commit add support/Add support/
>>
>> Bjorn
>
>