[PATCH v2 0/3] add fwnode support to reset subsystem

[PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

This series is part of a larger series which aims at adding fwnode
support in multiple subsystems [1]. The goal of this series was to
add support for software node in various subsystem but in a first
time only the fwnode support had gained consensus and will be added
to multiple subsystems.

For the moment ACPI node support is excluded from the fwnode support
to avoid creating an unspecified ACPI reset device description. With
these modifications, both driver that uses the fwnode_ API or the of_
API to register the reset controller will be usable by consumer
whatever the type of node that is used.

One question raised by this series is that I'm not sure if all reset
drivers should be modified to use the new fwnode support or keep the
existing device-tree support. Maintainer advice on that particular
question will be welcome.

This series was tested on a sparx5 board (pcb134).

[1] https://lore.kernel.org/netdev/YhPSkz8+BIcdb72R@smile.fi.intel.com/T/

---

Changes in V2:
- Updated comment specifying that either one of fwnode_xlate or of_xlate
  should provided and add check to return EINVAL if both are set
- Use only fwnode node name in rcdev_name() which also works for of_node
- Remove of_node handling case in __reset_control_get()
- Handle new error values -EOVERFLOW and -ENOMEM (returned by swnode
  implementation) for fwnode_property_match_string()
- Add sparx5 driver fwnode conversion

Clément Léger (3):
  of: add function to convert fwnode_reference_args to of_phandle_args
  reset: add support for fwnode
  reset: mchp: sparx5: set fwnode field of reset controller

 drivers/reset/core.c                   | 100 ++++++++++++++++++-------
 drivers/reset/reset-microchip-sparx5.c |   3 +-
 include/linux/of.h                     |  18 +++++
 include/linux/reset-controller.h       |  20 ++++-
 include/linux/reset.h                  |  14 ++++
 5 files changed, 123 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] of: add function to convert fwnode_reference_args to of_phandle_args

[Clément Léger]

Add of_phandle_args_from_fwnode_reference_args() which convert a struct
fwnode_reference_args to an of_phandle_args. This is used to maintain
compatibility for device-tree specifiers translation functions that are
used by various subsystem.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 include/linux/of.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 2dc77430a91a..1f6c8927c5ff 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -175,6 +175,18 @@ static inline bool is_of_node(const struct fwnode_handle *fwnode)
 			&__of_fwnode_handle_node->fwnode : NULL;	\
 	})
 
+static inline void
+of_phandle_args_from_fwnode_reference_args(struct of_phandle_args *of_args,
+					   const struct fwnode_reference_args *fwnode_args)
+{
+	int i;
+
+	of_args->np = to_of_node(fwnode_args->fwnode);
+	of_args->args_count = fwnode_args->nargs;
+	for (i = 0; i < fwnode_args->nargs; i++)
+		of_args->args[i] = fwnode_args->args[i];
+}
+
 static inline bool of_have_populated_dt(void)
 {
 	return of_root != NULL;
@@ -543,6 +555,12 @@ static inline struct device_node *of_find_node_with_property(
 
 #define of_fwnode_handle(node) NULL
 
+static inline void
+of_phandle_args_from_fwnode_reference_args(struct of_phandle_args *of_args,
+					   const struct fwnode_reference_args *fwnode_args)
+{
+}
+
 static inline bool of_have_populated_dt(void)
 {
 	return false;
-- 
2.34.1

[PATCH v2 2/3] reset: add support for fwnode

[Clément Léger]

In order to abstract the firmware node used by the reset subsystem,
switch to fwnode API. This will allow the subsystem to work with
all nodes that are supported by the fwnode API. ACPI node is for the
moment excluded from this support until the reset device description
is specified.

Existing device-tree support is kept and a fwnode_of_reset_xlate()
function has been added to translate fwnode reference args to a
device-tree spec for xlate function.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/reset/core.c             | 100 ++++++++++++++++++++++---------
 include/linux/reset-controller.h |  20 ++++++-
 include/linux/reset.h            |  14 +++++
 3 files changed, 104 insertions(+), 30 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 61e688882643..54fb4c9e4f7b 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -4,6 +4,7 @@
  *
  * Copyright 2013 Philipp Zabel, Pengutronix
  */
+#include <linux/acpi.h>
 #include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -67,29 +68,49 @@ static const char *rcdev_name(struct reset_controller_dev *rcdev)
 	if (rcdev->dev)
 		return dev_name(rcdev->dev);
 
-	if (rcdev->of_node)
-		return rcdev->of_node->full_name;
+	if (rcdev->fwnode)
+		return fwnode_get_name(rcdev->fwnode);
 
 	return NULL;
 }
 
 /**
- * of_reset_simple_xlate - translate reset_spec to the reset line number
+ * fwnode_reset_simple_xlate - translate reset_spec to the reset line number
  * @rcdev: a pointer to the reset controller device
- * @reset_spec: reset line specifier as found in the device tree
+ * @rargs: fwnode reset line specifier
  *
- * This static translation function is used by default if of_xlate in
+ * This static translation function is used by default if fwnode_xlate in
  * :c:type:`reset_controller_dev` is not set. It is useful for all reset
  * controllers with 1:1 mapping, where reset lines can be indexed by number
  * without gaps.
  */
-static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
-				 const struct of_phandle_args *reset_spec)
+static int fwnode_reset_simple_xlate(struct reset_controller_dev *rcdev,
+				     const struct fwnode_reference_args *rargs)
 {
-	if (reset_spec->args[0] >= rcdev->nr_resets)
+	if (rargs->args[0] >= rcdev->nr_resets)
 		return -EINVAL;
 
-	return reset_spec->args[0];
+	return rargs->args[0];
+}
+
+/**
+ * fwnode_of_reset_xlate - convert fwnode reference args for of_xlate call
+ * @rcdev: a pointer to the reset controller device
+ * @rargs: fwnode reset line specifier
+ *
+ * This static translation function is used by default for fwnode_xlate if
+ * of_xlate in * :c:type:`reset_controller_dev` is set. It allows to keep
+ * compatibility with device-tree compatible reset drivers by converting the
+ * fwnode_reference_args to of_phandle_args and calling of_xlate.
+ */
+static int fwnode_of_reset_xlate(struct reset_controller_dev *rcdev,
+				 const struct fwnode_reference_args *rargs)
+{
+	struct of_phandle_args of_args;
+
+	of_phandle_args_from_fwnode_reference_args(&of_args, rargs);
+
+	return rcdev->of_xlate(rcdev, &of_args);
 }
 
 /**
@@ -98,9 +119,24 @@ static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
  */
 int reset_controller_register(struct reset_controller_dev *rcdev)
 {
-	if (!rcdev->of_xlate) {
-		rcdev->of_reset_n_cells = 1;
-		rcdev->of_xlate = of_reset_simple_xlate;
+	if (!rcdev->fwnode) {
+		rcdev->fwnode = of_fwnode_handle(rcdev->of_node);
+	} else {
+		if (is_acpi_node(rcdev->fwnode))
+			return -EINVAL;
+	}
+
+	if (rcdev->of_xlate) {
+		if (rcdev->fwnode_xlate)
+			return -EINVAL;
+
+		rcdev->fwnode_xlate = fwnode_of_reset_xlate;
+		rcdev->fwnode_reset_n_cells = rcdev->of_reset_n_cells;
+	}
+
+	if (!rcdev->fwnode_xlate) {
+		rcdev->fwnode_reset_n_cells = 1;
+		rcdev->fwnode_xlate = fwnode_reset_simple_xlate;
 	}
 
 	INIT_LIST_HEAD(&rcdev->reset_control_head);
@@ -810,29 +846,28 @@ static void __reset_control_put_internal(struct reset_control *rstc)
 }
 
 struct reset_control *
-__of_reset_control_get(struct device_node *node, const char *id, int index,
-		       bool shared, bool optional, bool acquired)
+__fwnode_reset_control_get(struct fwnode_handle *fwnode, const char *id,
+			   int index, bool shared, bool optional, bool acquired)
 {
 	struct reset_control *rstc;
 	struct reset_controller_dev *r, *rcdev;
-	struct of_phandle_args args;
+	struct fwnode_reference_args args;
 	int rstc_id;
 	int ret;
 
-	if (!node)
+	if (!fwnode || is_acpi_node(fwnode))
 		return ERR_PTR(-EINVAL);
 
 	if (id) {
-		index = of_property_match_string(node,
-						 "reset-names", id);
-		if (index == -EILSEQ)
+		index = fwnode_property_match_string(fwnode, "reset-names", id);
+		if (index == -EILSEQ || index == -ENOMEM || index == -EOVERFLOW)
 			return ERR_PTR(index);
 		if (index < 0)
 			return optional ? NULL : ERR_PTR(-ENOENT);
 	}
 
-	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
-					 index, &args);
+	ret = fwnode_property_get_reference_args(fwnode, "resets", "#reset-cells",
+						 0, index, &args);
 	if (ret == -EINVAL)
 		return ERR_PTR(ret);
 	if (ret)
@@ -841,7 +876,7 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
 	mutex_lock(&reset_list_mutex);
 	rcdev = NULL;
 	list_for_each_entry(r, &reset_controller_list, list) {
-		if (args.np == r->of_node) {
+		if (args.fwnode == r->fwnode) {
 			rcdev = r;
 			break;
 		}
@@ -852,12 +887,12 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
 		goto out;
 	}
 
-	if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
+	if (WARN_ON(args.nargs != rcdev->fwnode_reset_n_cells)) {
 		rstc = ERR_PTR(-EINVAL);
 		goto out;
 	}
 
-	rstc_id = rcdev->of_xlate(rcdev, &args);
+	rstc_id = rcdev->fwnode_xlate(rcdev, &args);
 	if (rstc_id < 0) {
 		rstc = ERR_PTR(rstc_id);
 		goto out;
@@ -868,10 +903,19 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
 
 out:
 	mutex_unlock(&reset_list_mutex);
-	of_node_put(args.np);
+	fwnode_handle_put(args.fwnode);
 
 	return rstc;
 }
+EXPORT_SYMBOL_GPL(__fwnode_reset_control_get);
+
+struct reset_control *
+__of_reset_control_get(struct device_node *node, const char *id, int index,
+		       bool shared, bool optional, bool acquired)
+{
+	return __fwnode_reset_control_get(of_fwnode_handle(node), id, index,
+					  shared, optional, acquired);
+}
 EXPORT_SYMBOL_GPL(__of_reset_control_get);
 
 static struct reset_controller_dev *
@@ -942,9 +986,9 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id,
 	if (WARN_ON(shared && acquired))
 		return ERR_PTR(-EINVAL);
 
-	if (dev->of_node)
-		return __of_reset_control_get(dev->of_node, id, index, shared,
-					      optional, acquired);
+	if (dev_fwnode(dev))
+		return __fwnode_reset_control_get(dev_fwnode(dev), id, index,
+						  shared, optional, acquired);
 
 	return __reset_control_get_from_lookup(dev, id, shared, optional,
 					       acquired);
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index 0fa4f60e1186..c0ba4dae2e90 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -24,7 +24,9 @@ struct reset_control_ops {
 
 struct module;
 struct device_node;
+struct fwnode_handle;
 struct of_phandle_args;
+struct fwnode_reference_args;
 
 /**
  * struct reset_control_lookup - represents a single lookup entry
@@ -60,10 +62,20 @@ struct reset_control_lookup {
  * @reset_control_head: head of internal list of requested reset controls
  * @dev: corresponding driver model device struct
  * @of_node: corresponding device tree node as phandle target
+ * @fwnode: corresponding firmware node as reference target
  * @of_reset_n_cells: number of cells in reset line specifiers
  * @of_xlate: translation function to translate from specifier as found in the
- *            device tree to id as given to the reset control ops, defaults
- *            to :c:func:`of_reset_simple_xlate`.
+ *            device tree to id as given to the reset control ops. If set,
+ *            fwnode_xlate will be set with :c:func:`fwnode_of_reset_xlate`.
+ *            Note that only either one of of_xlate or fwnode_xlate should be
+ *            provided but not both.
+ * @fwnode_reset_n_cells: number of cells in reset line reference specifiers
+ * @fwnode_xlate: translation function to translate from reference specifier as
+ *                found in the firmware node description to id as given to the
+ *                reset control ops, defaults to
+ *                :c:func:`fwnode_reset_simple_xlate`.
+ *                Note that only either one of of_xlate or fwnode_xlate should
+ *                be provided but not both.
  * @nr_resets: number of reset controls in this reset controller device
  */
 struct reset_controller_dev {
@@ -73,9 +85,13 @@ struct reset_controller_dev {
 	struct list_head reset_control_head;
 	struct device *dev;
 	struct device_node *of_node;
+	struct fwnode_handle *fwnode;
 	int of_reset_n_cells;
 	int (*of_xlate)(struct reset_controller_dev *rcdev,
 			const struct of_phandle_args *reset_spec);
+	int fwnode_reset_n_cells;
+	int (*fwnode_xlate)(struct reset_controller_dev *rcdev,
+			    const struct fwnode_reference_args *reset_spec);
 	unsigned int nr_resets;
 };
 
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 8a21b5756c3e..0f0fe9408180 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -8,6 +8,7 @@
 
 struct device;
 struct device_node;
+struct fwnode_handle;
 struct reset_control;
 
 /**
@@ -41,6 +42,10 @@ int reset_control_bulk_deassert(int num_rstcs, struct reset_control_bulk_data *r
 int reset_control_bulk_acquire(int num_rstcs, struct reset_control_bulk_data *rstcs);
 void reset_control_bulk_release(int num_rstcs, struct reset_control_bulk_data *rstcs);
 
+struct reset_control *__fwnode_reset_control_get(struct fwnode_handle *node,
+						 const char *id, int index,
+						 bool shared, bool optional,
+						 bool acquired);
 struct reset_control *__of_reset_control_get(struct device_node *node,
 				     const char *id, int index, bool shared,
 				     bool optional, bool acquired);
@@ -114,6 +119,15 @@ static inline int __device_reset(struct device *dev, bool optional)
 	return optional ? 0 : -ENOTSUPP;
 }
 
+static inline
+struct reset_control *__fwnode_reset_control_get(struct fwnode_handle *node,
+						 const char *id, int index,
+						 bool shared, bool optional,
+						 bool acquired)
+{
+	return optional ? NULL : ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct reset_control *__of_reset_control_get(
 					struct device_node *node,
 					const char *id, int index, bool shared,
-- 
2.34.1

[PATCH v2 3/3] reset: mchp: sparx5: set fwnode field of reset controller

[Clément Léger]

Now that reset subsystem has fwnode support, use the fwnode field of
the rcdev to setup the fwnode to be used for the controller.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/reset/reset-microchip-sparx5.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/reset/reset-microchip-sparx5.c b/drivers/reset/reset-microchip-sparx5.c
index 00b612a0effa..eb648b40dffd 100644
--- a/drivers/reset/reset-microchip-sparx5.c
+++ b/drivers/reset/reset-microchip-sparx5.c
@@ -101,7 +101,6 @@ static int mchp_sparx5_map_io(struct platform_device *pdev, int index,
 
 static int mchp_sparx5_reset_probe(struct platform_device *pdev)
 {
-	struct device_node *dn = pdev->dev.of_node;
 	struct mchp_reset_context *ctx;
 	int err;
 
@@ -119,7 +118,7 @@ static int mchp_sparx5_reset_probe(struct platform_device *pdev)
 	ctx->rcdev.owner = THIS_MODULE;
 	ctx->rcdev.nr_resets = 1;
 	ctx->rcdev.ops = &sparx5_reset_ops;
-	ctx->rcdev.of_node = dn;
+	ctx->rcdev.fwnode = dev_fwnode(&pdev->dev);
 	ctx->props = device_get_match_data(&pdev->dev);
 
 	return devm_reset_controller_register(&pdev->dev, &ctx->rcdev);
-- 
2.34.1

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Rob Herring]

On Thu, Mar 24, 2022 at 03:12:34PM +0100, Clément Léger wrote:
> This series is part of a larger series which aims at adding fwnode
> support in multiple subsystems [1]. The goal of this series was to
> add support for software node in various subsystem but in a first
> time only the fwnode support had gained consensus and will be added
> to multiple subsystems.

The goal is describing a solution. What is the problem?

What's the scenario where you have a reset provider not described by 
firmware providing resets to devices (consumers) also not described by 
firmware.

> For the moment ACPI node support is excluded from the fwnode support
> to avoid creating an unspecified ACPI reset device description. With
> these modifications, both driver that uses the fwnode_ API or the of_
> API to register the reset controller will be usable by consumer
> whatever the type of node that is used.

Good, because controlling reset lines directly isn't how the ACPI device 
model works AFAIK.

> One question raised by this series is that I'm not sure if all reset
> drivers should be modified to use the new fwnode support or keep the
> existing device-tree support. Maintainer advice on that particular
> question will be welcome.

That would be pointless churn IMO. Why do we need to convert drivers 
which the vast majority will never use anything but DT?

Rob

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Mon, 4 Apr 2022 12:41:37 -0500,
Rob Herring <robh@kernel.org> a écrit :

> On Thu, Mar 24, 2022 at 03:12:34PM +0100, Clément Léger wrote:
> > This series is part of a larger series which aims at adding fwnode
> > support in multiple subsystems [1]. The goal of this series was to
> > add support for software node in various subsystem but in a first
> > time only the fwnode support had gained consensus and will be added
> > to multiple subsystems.  
> 
> The goal is describing a solution. What is the problem?
> 
> What's the scenario where you have a reset provider not described by 
> firmware providing resets to devices (consumers) also not described by 
> firmware.

Hi Rob, there was a link attached to this series since there was a
previous one that was sent which described the problem. Here is a link
to the same thread but to a specific message which clarifies the
problem and the solutions that were mentionned by other maintainers
(ACPI overlays, DT overlays, software nodes and so on):

https://lore.kernel.org/netdev/20220224154040.2633a4e4@fixe.home/

> 
> > For the moment ACPI node support is excluded from the fwnode support
> > to avoid creating an unspecified ACPI reset device description. With
> > these modifications, both driver that uses the fwnode_ API or the of_
> > API to register the reset controller will be usable by consumer
> > whatever the type of node that is used.  
> 
> Good, because controlling reset lines directly isn't how the ACPI device 
> model works AFAIK.

This was based on Mark Brown feedback.

> 
> > One question raised by this series is that I'm not sure if all reset
> > drivers should be modified to use the new fwnode support or keep the
> > existing device-tree support. Maintainer advice on that particular
> > question will be welcome.  
> 
> That would be pointless churn IMO. Why do we need to convert drivers 
> which the vast majority will never use anything but DT?

To have a single interface to maintain and to remove duplicated fields
(of_node, fwnode, fwnode_xlate, of_xlate) from reset controller struct.


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

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Rob Herring]

+ some Xilinx folks

On Tue, Apr 05, 2022 at 09:24:34AM +0200, Clément Léger wrote:
> Le Mon, 4 Apr 2022 12:41:37 -0500,
> Rob Herring <robh@kernel.org> a écrit :
> 
> > On Thu, Mar 24, 2022 at 03:12:34PM +0100, Clément Léger wrote:
> > > This series is part of a larger series which aims at adding fwnode
> > > support in multiple subsystems [1]. The goal of this series was to
> > > add support for software node in various subsystem but in a first
> > > time only the fwnode support had gained consensus and will be added
> > > to multiple subsystems.  
> > 
> > The goal is describing a solution. What is the problem?
> > 
> > What's the scenario where you have a reset provider not described by 
> > firmware providing resets to devices (consumers) also not described by 
> > firmware.
> 
> Hi Rob, there was a link attached to this series since there was a
> previous one that was sent which described the problem. Here is a link
> to the same thread but to a specific message which clarifies the
> problem and the solutions that were mentionned by other maintainers
> (ACPI overlays, DT overlays, software nodes and so on):
> 
> https://lore.kernel.org/netdev/20220224154040.2633a4e4@fixe.home/

Thanks, but your commit message should explain the problem. The problem 
is not subsystems don't support fwnode.

This is the exact same problem the Xilinx folks are trying to solve with 
their PCIe FPGA cards[1] (and that is not really a v1). They need to 
describe h/w downstream from a 'discoverable' device. Their case is 
further complicated with the dynamic nature of FPGAs. It's also not just 
PCIe. Another usecase is describing downstream devices on USB FTDI 
serial chips which can have GPIO, I2C, SPI downstream. And then you want 
to plug in 10 of those.

I don't think swnodes are going to scale for these usecases. We moved 
h/w description out of the kernel for a reason. Why are we adding that 
back in a new form? The complexity for what folks want to describe is 
only going to increase.

I think DT overlays is the right (or only) solution here. Of course the 
DT maintainer would say that. Actually, I would be happier to not have 
to support overlays in the kernel.

I've told the Xilinx folks the same thing, but I would separate this 
into 2 parts. First is just h/w work in a DT based system. Second is 
creating a base tree an overlay can be applied to. The first part should 
be pretty straightforward. We already have PCI bus bindings. The only 
tricky part is getting address translation working from leaf device thru 
the PCI bus to host bus, but support for that should all be in place 
(given we support ISA buses off of PCI bus). The second part will 
require generating PCI DT nodes at runtime. That may be needed for both 
DT and ACPI systems as we don't always describe all the PCI hierarchy in 
DT. That could work either by the PCI subsystem creating nodes as it
populates devices or your driver could make a request to populate nodes 
for its hierarchy. That's not a hard problem to solve. That's what 
OpenFirmware implementations do already.

Rob


https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Tue, 5 Apr 2022 09:47:20 -0500,
Rob Herring <robh@kernel.org> a écrit :

> + some Xilinx folks
> 
> On Tue, Apr 05, 2022 at 09:24:34AM +0200, Clément Léger wrote:
> > Le Mon, 4 Apr 2022 12:41:37 -0500,
> > Rob Herring <robh@kernel.org> a écrit :
> >   
> > > On Thu, Mar 24, 2022 at 03:12:34PM +0100, Clément Léger wrote:  
> > > > This series is part of a larger series which aims at adding fwnode
> > > > support in multiple subsystems [1]. The goal of this series was to
> > > > add support for software node in various subsystem but in a first
> > > > time only the fwnode support had gained consensus and will be added
> > > > to multiple subsystems.    
> > > 
> > > The goal is describing a solution. What is the problem?
> > > 
> > > What's the scenario where you have a reset provider not described by 
> > > firmware providing resets to devices (consumers) also not described by 
> > > firmware.  
> > 
> > Hi Rob, there was a link attached to this series since there was a
> > previous one that was sent which described the problem. Here is a link
> > to the same thread but to a specific message which clarifies the
> > problem and the solutions that were mentionned by other maintainers
> > (ACPI overlays, DT overlays, software nodes and so on):
> > 
> > https://lore.kernel.org/netdev/20220224154040.2633a4e4@fixe.home/  
> 
> Thanks, but your commit message should explain the problem. The problem 
> is not subsystems don't support fwnode.
> 
> This is the exact same problem the Xilinx folks are trying to solve with 
> their PCIe FPGA cards[1] (and that is not really a v1). They need to 
> describe h/w downstream from a 'discoverable' device. Their case is 
> further complicated with the dynamic nature of FPGAs. It's also not just 
> PCIe. Another usecase is describing downstream devices on USB FTDI 
> serial chips which can have GPIO, I2C, SPI downstream. And then you want 
> to plug in 10 of those.

I also tried loading an overlay from a driver on an ACPI based system.
Their patch is (I guess) targeting the specific problem that there is
no base DT when using ACPI. However, Mark Brown feedback was not to
mix OF and ACPI:

"That seems like it's opening a can of worms that might be best left
closed."

But I would be interested to know how the Xilinx guys are doing that
on x86/ACPI based system.

> 
> I don't think swnodes are going to scale for these usecases. We moved 
> h/w description out of the kernel for a reason. Why are we adding that 
> back in a new form? The complexity for what folks want to describe is 
> only going to increase.
> 
> I think DT overlays is the right (or only) solution here. Of course the 
> DT maintainer would say that. Actually, I would be happier to not have 
> to support overlays in the kernel.

DT overlay might work on DT based system. If I'm going to plug the card
on an ACPI based platform (x86), I also want that card to work
seamlessly without requiring the user to create an ACPI overlay.

If you proposal was to use DT overlays on an ACPI based system, doing
so would also require to "plug" the PCI subystem when described with
ACPI to "probe" DT overlays describing PCI devices, not sure this is
something trivial and it would be PCI centric.

> 
> I've told the Xilinx folks the same thing, but I would separate this 
> into 2 parts. First is just h/w work in a DT based system. Second is 
> creating a base tree an overlay can be applied to. The first part should 
> be pretty straightforward. We already have PCI bus bindings. The only 
> tricky part is getting address translation working from leaf device thru 
> the PCI bus to host bus, but support for that should all be in place 
> (given we support ISA buses off of PCI bus). The second part will 
> require generating PCI DT nodes at runtime. That may be needed for both 
> DT and ACPI systems as we don't always describe all the PCI hierarchy
> in DT. 

But then, if the driver generate the nodes, it will most probably
have to describe the nodes by hardcoding them right ? Or probably load
some dtbo from the FS. If so, I would then have to describe the card
for both ACPI and DT. How is that better than using a single software
node description for both ACPI/OF based systems ? Or maybe I missed
something, but the device description won't come out of thin air I
guess.

Also, when saying "That may be needed for both DT and ACPI systems", do
you actually meant that ACPI overlay should be described for ACPI based
systems and DT overlays for DT based ones ? If so, some subsystems do
not even support ACPI (reset for instance which is need for my
PCI card but that is not the only one). So how to accomodate both ? This
would result in having 2 separate descriptions for ACPI and OF and
potentially non working with ACPI description.

Software nodes have the advantage of being independent from the
description systems used (ACPI/OF). If switching susbsystems to use
fwnode, this would also allows to accomodate easily for all nodes types
and potentially factorize some code.

> That could work either by the PCI subsystem creating nodes as it
> populates devices or your driver could make a request to populate nodes 
> for its hierarchy. That's not a hard problem to solve. That's what 
> OpenFirmware implementations do already.

This would also require to get address translation working with ACPI
based systems since the PCI bus isn't described with DT on such
systems. I'm not sure how trivial it is. Or it would require to add PCI
root complex entries into the device-tree to allow adress translation
to work using the existing system probably.

>
> 
> Rob
> 
> 
> https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/

Looking at the feedback of the previous series that I mentionned,
absolutely nobody agreed on the solution to be adopted. I asked for a
consensus but I only got an answer from Hans de Goede which was ok
with the fwnode way. I would be really glad to have some consensus on
that in order to implement a final solution (and if the OF overlays is
the one to be used, I'll use it).

Thanks,

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

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Rob Herring]

On Tue, Apr 5, 2022 at 10:52 AM Clément Léger <clement.leger@bootlin.com> wrote:
>
> Le Tue, 5 Apr 2022 09:47:20 -0500,
> Rob Herring <robh@kernel.org> a écrit :
>
> > + some Xilinx folks
> >
> > On Tue, Apr 05, 2022 at 09:24:34AM +0200, Clément Léger wrote:
> > > Le Mon, 4 Apr 2022 12:41:37 -0500,
> > > Rob Herring <robh@kernel.org> a écrit :
> > >
> > > > On Thu, Mar 24, 2022 at 03:12:34PM +0100, Clément Léger wrote:
> > > > > This series is part of a larger series which aims at adding fwnode
> > > > > support in multiple subsystems [1]. The goal of this series was to
> > > > > add support for software node in various subsystem but in a first
> > > > > time only the fwnode support had gained consensus and will be added
> > > > > to multiple subsystems.
> > > >
> > > > The goal is describing a solution. What is the problem?
> > > >
> > > > What's the scenario where you have a reset provider not described by
> > > > firmware providing resets to devices (consumers) also not described by
> > > > firmware.
> > >
> > > Hi Rob, there was a link attached to this series since there was a
> > > previous one that was sent which described the problem. Here is a link
> > > to the same thread but to a specific message which clarifies the
> > > problem and the solutions that were mentionned by other maintainers
> > > (ACPI overlays, DT overlays, software nodes and so on):
> > >
> > > https://lore.kernel.org/netdev/20220224154040.2633a4e4@fixe.home/
> >
> > Thanks, but your commit message should explain the problem. The problem
> > is not subsystems don't support fwnode.
> >
> > This is the exact same problem the Xilinx folks are trying to solve with
> > their PCIe FPGA cards[1] (and that is not really a v1). They need to
> > describe h/w downstream from a 'discoverable' device. Their case is
> > further complicated with the dynamic nature of FPGAs. It's also not just
> > PCIe. Another usecase is describing downstream devices on USB FTDI
> > serial chips which can have GPIO, I2C, SPI downstream. And then you want
> > to plug in 10 of those.
>
> I also tried loading an overlay from a driver on an ACPI based system.
> Their patch is (I guess) targeting the specific problem that there is
> no base DT when using ACPI. However, Mark Brown feedback was not to
> mix OF and ACPI:

I agree there. I don't think we should use DT bindings in ACPI tables
which is already happening. In this case, I think what's described by
ACPI and DT must be completely disjoint. I think that's the case here
as everything is downstream of the PCIe device.

> "That seems like it's opening a can of worms that might be best left
> closed."
>
> But I would be interested to know how the Xilinx guys are doing that
> on x86/ACPI based system.

They aren't, yet...


> > I don't think swnodes are going to scale for these usecases. We moved
> > h/w description out of the kernel for a reason. Why are we adding that
> > back in a new form? The complexity for what folks want to describe is
> > only going to increase.
> >
> > I think DT overlays is the right (or only) solution here. Of course the
> > DT maintainer would say that. Actually, I would be happier to not have
> > to support overlays in the kernel.
>
> DT overlay might work on DT based system. If I'm going to plug the card
> on an ACPI based platform (x86), I also want that card to work
> seamlessly without requiring the user to create an ACPI overlay.

I agree, it should work the same way for the user.

> If you proposal was to use DT overlays on an ACPI based system, doing
> so would also require to "plug" the PCI subystem when described with
> ACPI to "probe" DT overlays describing PCI devices, not sure this is
> something trivial and it would be PCI centric.

Yes, this is the 2nd part I describe. I don't think there's any way to
avoid this being bus specific because bus specific nodes have to be
created.


> > I've told the Xilinx folks the same thing, but I would separate this
> > into 2 parts. First is just h/w work in a DT based system. Second is
> > creating a base tree an overlay can be applied to. The first part should
> > be pretty straightforward. We already have PCI bus bindings. The only
> > tricky part is getting address translation working from leaf device thru
> > the PCI bus to host bus, but support for that should all be in place
> > (given we support ISA buses off of PCI bus). The second part will
> > require generating PCI DT nodes at runtime. That may be needed for both
> > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > in DT.
>
> But then, if the driver generate the nodes, it will most probably
> have to describe the nodes by hardcoding them right ?

No, the kernel already maintains its own tree of devices. You just
need to use that to generate the tree. That's really not much more
than nodes with a 'reg' property encoding the device and function
numbers.

We already support matching a PCI device to a DT node. The PCI
subsystem checks if there is a corresponding DT node for each PCI
device created and sets the of_node pointer if there is. For
OpenFirmware systems (PPC), there always is a node. For FDT, we
generally don't have a node unless there are additional
non-discoverable properties. Hikey960 is an example with PCI device
nodes in the DT as it has a soldered down PCIe switch with downstream
devices and non-discoverable properties (e.g. reset GPIO for each
port).

> Or probably load
> some dtbo from the FS. If so, I would then have to describe the card
> for both ACPI and DT. How is that better than using a single software
> node description for both ACPI/OF based systems ? Or maybe I missed
> something, but the device description won't come out of thin air I
> guess.

What you would have to load is a DT overlay describing all your
downstream devices.

We support DTBs (including DTBOs) built into the kernel already, so
whether it's built into the kernel or in the FS is up to you really.

> Also, when saying "That may be needed for both DT and ACPI systems", do
> you actually meant that ACPI overlay should be described for ACPI based
> systems and DT overlays for DT based ones ?

No, as I said: "I think DT overlays is the right (or only) solution
here." ACPI overlays doesn't seem like a workable solution because it
can't describe your downstream devices.

The reason generating nodes may be needed on DT systems as well is
that all PCI devices are not described in DT systems either.

> If so, some subsystems do
> not even support ACPI (reset for instance which is need for my
> PCI card but that is not the only one). So how to accomodate both ? This
> would result in having 2 separate descriptions for ACPI and OF and
> potentially non working with ACPI description.
>
> Software nodes have the advantage of being independent from the
> description systems used (ACPI/OF). If switching susbsystems to use
> fwnode, this would also allows to accomodate easily for all nodes types
> and potentially factorize some code.

It's not independent. You are effectively creating the DT nodes with C
code. Are these not DT bindings:

> static const struct property_entry ddr_clk_props[] = {
>         PROPERTY_ENTRY_U32("clock-frequency", 30000000),
>         PROPERTY_ENTRY_U32("#clock-cells", 0),
>         {}
> };

Sure looks like DT bindings to me. I don't think moving them into the
kernel as sw nodes avoids any of the potential pitfalls of mixing ACPI
and DT. For example, what happens when you have a downstream sw node
device that wants to do DMA allocations and transfers? I suspect that
sw nodes can't really handle more than trivial cases.


> > That could work either by the PCI subsystem creating nodes as it
> > populates devices or your driver could make a request to populate nodes
> > for its hierarchy. That's not a hard problem to solve. That's what
> > OpenFirmware implementations do already.
>
> This would also require to get address translation working with ACPI
> based systems since the PCI bus isn't described with DT on such
> systems. I'm not sure how trivial it is. Or it would require to add PCI
> root complex entries into the device-tree to allow adress translation
> to work using the existing system probably.

It would require all that most likely. Maybe there's some shortcuts we
can take. All the necessary information is maintained by the kernel
already. Normally it's populated from the firmware into the kernel
structures. But here we need the opposite direction.


> > https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/
>
> Looking at the feedback of the previous series that I mentionned,
> absolutely nobody agreed on the solution to be adopted. I asked for a
> consensus but I only got an answer from Hans de Goede which was ok
> with the fwnode way. I would be really glad to have some consensus on
> that in order to implement a final solution (and if the OF overlays is
> the one to be used, I'll use it).

Yes, that's a challenge, but buried in some patch series is not going
to get you there. I am trying to widen the discussion because it is a
problem that's been on my radar for some time.

Rob

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Rob Herring]

On Tue, Apr 5, 2022 at 12:11 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Apr 5, 2022 at 10:52 AM Clément Léger <clement.leger@bootlin.com> wrote:
> >
> > Le Tue, 5 Apr 2022 09:47:20 -0500,
> > Rob Herring <robh@kernel.org> a écrit :
> >
> > > + some Xilinx folks
> > >
> > > On Tue, Apr 05, 2022 at 09:24:34AM +0200, Clément Léger wrote:
> > > > Le Mon, 4 Apr 2022 12:41:37 -0500,
> > > > Rob Herring <robh@kernel.org> a écrit :
> > > >
> > > > > On Thu, Mar 24, 2022 at 03:12:34PM +0100, Clément Léger wrote:
> > > > > > This series is part of a larger series which aims at adding fwnode
> > > > > > support in multiple subsystems [1]. The goal of this series was to
> > > > > > add support for software node in various subsystem but in a first
> > > > > > time only the fwnode support had gained consensus and will be added
> > > > > > to multiple subsystems.
> > > > >
> > > > > The goal is describing a solution. What is the problem?
> > > > >
> > > > > What's the scenario where you have a reset provider not described by
> > > > > firmware providing resets to devices (consumers) also not described by
> > > > > firmware.
> > > >
> > > > Hi Rob, there was a link attached to this series since there was a
> > > > previous one that was sent which described the problem. Here is a link
> > > > to the same thread but to a specific message which clarifies the
> > > > problem and the solutions that were mentionned by other maintainers
> > > > (ACPI overlays, DT overlays, software nodes and so on):
> > > >
> > > > https://lore.kernel.org/netdev/20220224154040.2633a4e4@fixe.home/
> > >
> > > Thanks, but your commit message should explain the problem. The problem
> > > is not subsystems don't support fwnode.
> > >
> > > This is the exact same problem the Xilinx folks are trying to solve with
> > > their PCIe FPGA cards[1] (and that is not really a v1). They need to
> > > describe h/w downstream from a 'discoverable' device. Their case is
> > > further complicated with the dynamic nature of FPGAs. It's also not just
> > > PCIe. Another usecase is describing downstream devices on USB FTDI
> > > serial chips which can have GPIO, I2C, SPI downstream. And then you want
> > > to plug in 10 of those.
> >
> > I also tried loading an overlay from a driver on an ACPI based system.
> > Their patch is (I guess) targeting the specific problem that there is
> > no base DT when using ACPI. However, Mark Brown feedback was not to
> > mix OF and ACPI:
>
> I agree there. I don't think we should use DT bindings in ACPI tables
> which is already happening. In this case, I think what's described by
> ACPI and DT must be completely disjoint. I think that's the case here
> as everything is downstream of the PCIe device.
>
> > "That seems like it's opening a can of worms that might be best left
> > closed."
> >
> > But I would be interested to know how the Xilinx guys are doing that
> > on x86/ACPI based system.
>
> They aren't, yet...
>
>
> > > I don't think swnodes are going to scale for these usecases. We moved
> > > h/w description out of the kernel for a reason. Why are we adding that
> > > back in a new form? The complexity for what folks want to describe is
> > > only going to increase.
> > >
> > > I think DT overlays is the right (or only) solution here. Of course the
> > > DT maintainer would say that. Actually, I would be happier to not have
> > > to support overlays in the kernel.
> >
> > DT overlay might work on DT based system. If I'm going to plug the card
> > on an ACPI based platform (x86), I also want that card to work
> > seamlessly without requiring the user to create an ACPI overlay.
>
> I agree, it should work the same way for the user.
>
> > If you proposal was to use DT overlays on an ACPI based system, doing
> > so would also require to "plug" the PCI subystem when described with
> > ACPI to "probe" DT overlays describing PCI devices, not sure this is
> > something trivial and it would be PCI centric.
>
> Yes, this is the 2nd part I describe. I don't think there's any way to
> avoid this being bus specific because bus specific nodes have to be
> created.
>
>
> > > I've told the Xilinx folks the same thing, but I would separate this
> > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > creating a base tree an overlay can be applied to. The first part should
> > > be pretty straightforward. We already have PCI bus bindings. The only
> > > tricky part is getting address translation working from leaf device thru
> > > the PCI bus to host bus, but support for that should all be in place
> > > (given we support ISA buses off of PCI bus). The second part will
> > > require generating PCI DT nodes at runtime. That may be needed for both
> > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > in DT.
> >
> > But then, if the driver generate the nodes, it will most probably
> > have to describe the nodes by hardcoding them right ?
>
> No, the kernel already maintains its own tree of devices. You just
> need to use that to generate the tree. That's really not much more
> than nodes with a 'reg' property encoding the device and function
> numbers.
>
> We already support matching a PCI device to a DT node. The PCI
> subsystem checks if there is a corresponding DT node for each PCI
> device created and sets the of_node pointer if there is. For
> OpenFirmware systems (PPC), there always is a node. For FDT, we
> generally don't have a node unless there are additional
> non-discoverable properties. Hikey960 is an example with PCI device
> nodes in the DT as it has a soldered down PCIe switch with downstream
> devices and non-discoverable properties (e.g. reset GPIO for each
> port).

Here's a quick and dirty implementation creating DT nodes for PCI devices:

git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt/pop-pci-nodes

Rob

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Tue, 5 Apr 2022 12:11:51 -0500,
Rob Herring <robh@kernel.org> a écrit :

> On Tue, Apr 5, 2022 at 10:52 AM Clément Léger <clement.leger@bootlin.com> wrote:
> >
> > Le Tue, 5 Apr 2022 09:47:20 -0500,
> > Rob Herring <robh@kernel.org> a écrit :
> >  

[...]

> >
> > I also tried loading an overlay from a driver on an ACPI based system.
> > Their patch is (I guess) targeting the specific problem that there is
> > no base DT when using ACPI. However, Mark Brown feedback was not to
> > mix OF and ACPI:  
> 
> I agree there. I don't think we should use DT bindings in ACPI tables
> which is already happening. In this case, I think what's described by
> ACPI and DT must be completely disjoint. I think that's the case here
> as everything is downstream of the PCIe device.

Yes, there is no references to the host devices (at least in my case).

> 
> > "That seems like it's opening a can of worms that might be best left
> > closed."
> >
> > But I would be interested to know how the Xilinx guys are doing that
> > on x86/ACPI based system.  
> 
> They aren't, yet...

Ok...

[...]

> 
> 
> > > I've told the Xilinx folks the same thing, but I would separate this
> > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > creating a base tree an overlay can be applied to. The first part should
> > > be pretty straightforward. We already have PCI bus bindings. The only
> > > tricky part is getting address translation working from leaf device thru
> > > the PCI bus to host bus, but support for that should all be in place
> > > (given we support ISA buses off of PCI bus). The second part will
> > > require generating PCI DT nodes at runtime. That may be needed for both
> > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > in DT.  
> >
> > But then, if the driver generate the nodes, it will most probably
> > have to describe the nodes by hardcoding them right ?  
> 
> No, the kernel already maintains its own tree of devices. You just
> need to use that to generate the tree. That's really not much more
> than nodes with a 'reg' property encoding the device and function
> numbers.

Just to clarified a point, my PCI device exposes multiple peripherals
behind one single PCI function.

To be sure I understood what you are suggesting, you propose to create
a DT node from the PCI driver that has been probed dynamically
matching this same PCI device with a 'reg' property. I also think
this would requires to generate some 'pci-ranges' to remap the
downstream devices that are described in the DTBO, finally, load the
overlay to be apply under this newly created node. Is that right ?

> 
> We already support matching a PCI device to a DT node. The PCI
> subsystem checks if there is a corresponding DT node for each PCI
> device created and sets the of_node pointer if there is. For
> OpenFirmware systems (PPC), there always is a node. For FDT, we
> generally don't have a node unless there are additional
> non-discoverable properties. Hikey960 is an example with PCI device
> nodes in the DT as it has a soldered down PCIe switch with downstream
> devices and non-discoverable properties (e.g. reset GPIO for each
> port).
> 
> > Or probably load
> > some dtbo from the FS. If so, I would then have to describe the card
> > for both ACPI and DT. How is that better than using a single software
> > node description for both ACPI/OF based systems ? Or maybe I missed
> > something, but the device description won't come out of thin air I
> > guess.  
> 
> What you would have to load is a DT overlay describing all your
> downstream devices.
> 
> We support DTBs (including DTBOs) built into the kernel already, so
> whether it's built into the kernel or in the FS is up to you really.

Indeed.

> 
> > Also, when saying "That may be needed for both DT and ACPI systems", do
> > you actually meant that ACPI overlay should be described for ACPI based
> > systems and DT overlays for DT based ones ?  
> 
> No, as I said: "I think DT overlays is the right (or only) solution
> here." ACPI overlays doesn't seem like a workable solution because it
> can't describe your downstream devices.

Ok, so you are actually really suggesting to use OF overlays on ACPI
based systems. If so, I'm ok with that.

> 
> The reason generating nodes may be needed on DT systems as well is
> that all PCI devices are not described in DT systems either.
> 
> > If so, some subsystems do
> > not even support ACPI (reset for instance which is need for my
> > PCI card but that is not the only one). So how to accomodate both ? This
> > would result in having 2 separate descriptions for ACPI and OF and
> > potentially non working with ACPI description.
> >
> > Software nodes have the advantage of being independent from the
> > description systems used (ACPI/OF). If switching susbsystems to use
> > fwnode, this would also allows to accomodate easily for all nodes types
> > and potentially factorize some code.  
> 
> It's not independent. You are effectively creating the DT nodes with C
> code. Are these not DT bindings:
> 
> > static const struct property_entry ddr_clk_props[] = {
> >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),
> >         PROPERTY_ENTRY_U32("#clock-cells", 0),
> >         {}
> > };  
> 
> Sure looks like DT bindings to me. I don't think moving them into the
> kernel as sw nodes avoids any of the potential pitfalls of mixing ACPI
> and DT. For example, what happens when you have a downstream sw node
> device that wants to do DMA allocations and transfers? I suspect that
> sw nodes can't really handle more than trivial cases.

When integrating with fwnode, the subsystem support will be almost the
same as the one with OF. But I agree that it requires certain level of
subsystem modifications to support fwnode.

> 
> 
> > > That could work either by the PCI subsystem creating nodes as it
> > > populates devices or your driver could make a request to populate nodes
> > > for its hierarchy. That's not a hard problem to solve. That's what
> > > OpenFirmware implementations do already.  
> >
> > This would also require to get address translation working with ACPI
> > based systems since the PCI bus isn't described with DT on such
> > systems. I'm not sure how trivial it is. Or it would require to add PCI
> > root complex entries into the device-tree to allow adress translation
> > to work using the existing system probably.  
> 
> It would require all that most likely. Maybe there's some shortcuts we
> can take. All the necessary information is maintained by the kernel
> already. Normally it's populated from the firmware into the kernel
> structures. But here we need the opposite direction.
> 
> 
> > > https://lore.kernel.org/lkml/20220216050056.311496-1-lizhi.hou@xilinx.com/  
> >
> > Looking at the feedback of the previous series that I mentionned,
> > absolutely nobody agreed on the solution to be adopted. I asked for a
> > consensus but I only got an answer from Hans de Goede which was ok
> > with the fwnode way. I would be really glad to have some consensus on
> > that in order to implement a final solution (and if the OF overlays is
> > the one to be used, I'll use it).  
> 
> Yes, that's a challenge, but buried in some patch series is not going
> to get you there.

Sorry, indeed, you were not on the series were the discussion took
place. I'll think about that next time.

> I am trying to widen the discussion because it is a
> problem that's been on my radar for some time.

Thanks for the proposal, maybe we can achieve something that will suit
everybody and solve the current problems.

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

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Tue, 5 Apr 2022 16:28:02 -0500,
Rob Herring <robh@kernel.org> a écrit :


> >
> > No, the kernel already maintains its own tree of devices. You just
> > need to use that to generate the tree. That's really not much more
> > than nodes with a 'reg' property encoding the device and function
> > numbers.
> >
> > We already support matching a PCI device to a DT node. The PCI
> > subsystem checks if there is a corresponding DT node for each PCI
> > device created and sets the of_node pointer if there is. For
> > OpenFirmware systems (PPC), there always is a node. For FDT, we
> > generally don't have a node unless there are additional
> > non-discoverable properties. Hikey960 is an example with PCI device
> > nodes in the DT as it has a soldered down PCIe switch with downstream
> > devices and non-discoverable properties (e.g. reset GPIO for each
> > port).  
> 
> Here's a quick and dirty implementation creating DT nodes for PCI devices:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt/pop-pci-nodes

Ok, thanks, after looking at the branch, it appears that you expect the
PCI nodes matching the probed PCI devices should be created by the PCI
subsystem itself. My previous comment saying that the node would be
created by the PCI driver itself is then wrong and I understand what
you meant.

Then, there is still a bit of magic to do to correctly fill the ranges
for translation and then the driver "simply" have to load the dtbo and
apply it with of_overlay_fdt_apply().

> 
> Rob



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

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Rob Herring]

On Wed, Apr 06, 2022 at 09:52:13AM +0200, Clément Léger wrote:
> Le Tue, 5 Apr 2022 16:28:02 -0500,
> Rob Herring <robh@kernel.org> a écrit :
> 
> 
> > >
> > > No, the kernel already maintains its own tree of devices. You just
> > > need to use that to generate the tree. That's really not much more
> > > than nodes with a 'reg' property encoding the device and function
> > > numbers.
> > >
> > > We already support matching a PCI device to a DT node. The PCI
> > > subsystem checks if there is a corresponding DT node for each PCI
> > > device created and sets the of_node pointer if there is. For
> > > OpenFirmware systems (PPC), there always is a node. For FDT, we
> > > generally don't have a node unless there are additional
> > > non-discoverable properties. Hikey960 is an example with PCI device
> > > nodes in the DT as it has a soldered down PCIe switch with downstream
> > > devices and non-discoverable properties (e.g. reset GPIO for each
> > > port).  
> > 
> > Here's a quick and dirty implementation creating DT nodes for PCI devices:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt/pop-pci-nodes
> 
> Ok, thanks, after looking at the branch, it appears that you expect the
> PCI nodes matching the probed PCI devices should be created by the PCI
> subsystem itself. My previous comment saying that the node would be
> created by the PCI driver itself is then wrong and I understand what
> you meant.

As I said before, the driver could create its node and all the parents. 
I went with the other option partly because I didn't have a particular 
driver. That has the advantage of only creating necessary nodes and 
provides a way to trigger doing so. The issue with it is the timing of 
when the node gets set (after parent devices have probed).

> Then, there is still a bit of magic to do to correctly fill the ranges
> for translation and then the driver "simply" have to load the dtbo and
> apply it with of_overlay_fdt_apply().

The host bridge resource list should have all the information needed.

Rob

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Rob Herring]

On Wed, Apr 06, 2022 at 09:40:19AM +0200, Clément Léger wrote:
> Le Tue, 5 Apr 2022 12:11:51 -0500,
> Rob Herring <robh@kernel.org> a écrit :
> 
> > On Tue, Apr 5, 2022 at 10:52 AM Clément Léger <clement.leger@bootlin.com> wrote:
> > >
> > > Le Tue, 5 Apr 2022 09:47:20 -0500,
> > > Rob Herring <robh@kernel.org> a écrit :
> > >  
> 
> [...]
> 
> > >
> > > I also tried loading an overlay from a driver on an ACPI based system.
> > > Their patch is (I guess) targeting the specific problem that there is
> > > no base DT when using ACPI. However, Mark Brown feedback was not to
> > > mix OF and ACPI:  
> > 
> > I agree there. I don't think we should use DT bindings in ACPI tables
> > which is already happening. In this case, I think what's described by
> > ACPI and DT must be completely disjoint. I think that's the case here
> > as everything is downstream of the PCIe device.
> 
> Yes, there is no references to the host devices (at least in my case).
> 
> > 
> > > "That seems like it's opening a can of worms that might be best left
> > > closed."
> > >
> > > But I would be interested to know how the Xilinx guys are doing that
> > > on x86/ACPI based system.  
> > 
> > They aren't, yet...
> 
> Ok...
> 
> [...]
> 
> > 
> > 
> > > > I've told the Xilinx folks the same thing, but I would separate this
> > > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > > creating a base tree an overlay can be applied to. The first part should
> > > > be pretty straightforward. We already have PCI bus bindings. The only
> > > > tricky part is getting address translation working from leaf device thru
> > > > the PCI bus to host bus, but support for that should all be in place
> > > > (given we support ISA buses off of PCI bus). The second part will
> > > > require generating PCI DT nodes at runtime. That may be needed for both
> > > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > > in DT.  
> > >
> > > But then, if the driver generate the nodes, it will most probably
> > > have to describe the nodes by hardcoding them right ?  
> > 
> > No, the kernel already maintains its own tree of devices. You just
> > need to use that to generate the tree. That's really not much more
> > than nodes with a 'reg' property encoding the device and function
> > numbers.
> 
> Just to clarified a point, my PCI device exposes multiple peripherals
> behind one single PCI function.

Right. I would expect your PCI device DT node to have a 'simple-bus' 
child node with all those peripherals. And maybe there's other nodes 
like fixed-clocks, etc.

> To be sure I understood what you are suggesting, you propose to create
> a DT node from the PCI driver that has been probed dynamically
> matching this same PCI device with a 'reg' property. I also think
> this would requires to generate some 'pci-ranges' to remap the
> downstream devices that are described in the DTBO, finally, load the
> overlay to be apply under this newly created node. Is that right ?

Right. You'll need to take the BAR address(es) for the device and stick 
those into 'ranges' to translate offsets to BAR+offset.

Rob

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Alexandre Belloni]

On 06/04/2022 08:19:16-0500, Rob Herring wrote:
> > > > > I've told the Xilinx folks the same thing, but I would separate this
> > > > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > > > creating a base tree an overlay can be applied to. The first part should
> > > > > be pretty straightforward. We already have PCI bus bindings. The only
> > > > > tricky part is getting address translation working from leaf device thru
> > > > > the PCI bus to host bus, but support for that should all be in place
> > > > > (given we support ISA buses off of PCI bus). The second part will
> > > > > require generating PCI DT nodes at runtime. That may be needed for both
> > > > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > > > in DT.  
> > > >
> > > > But then, if the driver generate the nodes, it will most probably
> > > > have to describe the nodes by hardcoding them right ?  
> > > 
> > > No, the kernel already maintains its own tree of devices. You just
> > > need to use that to generate the tree. That's really not much more
> > > than nodes with a 'reg' property encoding the device and function
> > > numbers.
> > 
> > Just to clarified a point, my PCI device exposes multiple peripherals
> > behind one single PCI function.
> 
> Right. I would expect your PCI device DT node to have a 'simple-bus' 
> child node with all those peripherals. And maybe there's other nodes 
> like fixed-clocks, etc.
> 
> > To be sure I understood what you are suggesting, you propose to create
> > a DT node from the PCI driver that has been probed dynamically
> > matching this same PCI device with a 'reg' property. I also think
> > this would requires to generate some 'pci-ranges' to remap the
> > downstream devices that are described in the DTBO, finally, load the
> > overlay to be apply under this newly created node. Is that right ?
> 
> Right. You'll need to take the BAR address(es) for the device and stick 
> those into 'ranges' to translate offsets to BAR+offset.
> 

Last time I tried that, this was not working well because it means that
the ranges property of the device depends on the host machine...

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Wed, 6 Apr 2022 15:33:46 +0200,
Alexandre Belloni <alexandre.belloni@bootlin.com> a écrit :

> On 06/04/2022 08:19:16-0500, Rob Herring wrote:
> > > > > > I've told the Xilinx folks the same thing, but I would separate this
> > > > > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > > > > creating a base tree an overlay can be applied to. The first part should
> > > > > > be pretty straightforward. We already have PCI bus bindings. The only
> > > > > > tricky part is getting address translation working from leaf device thru
> > > > > > the PCI bus to host bus, but support for that should all be in place
> > > > > > (given we support ISA buses off of PCI bus). The second part will
> > > > > > require generating PCI DT nodes at runtime. That may be needed for both
> > > > > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > > > > in DT.    
> > > > >
> > > > > But then, if the driver generate the nodes, it will most probably
> > > > > have to describe the nodes by hardcoding them right ?    
> > > > 
> > > > No, the kernel already maintains its own tree of devices. You just
> > > > need to use that to generate the tree. That's really not much more
> > > > than nodes with a 'reg' property encoding the device and function
> > > > numbers.  
> > > 
> > > Just to clarified a point, my PCI device exposes multiple peripherals
> > > behind one single PCI function.  
> > 
> > Right. I would expect your PCI device DT node to have a 'simple-bus' 
> > child node with all those peripherals. And maybe there's other nodes 
> > like fixed-clocks, etc.
> >   
> > > To be sure I understood what you are suggesting, you propose to create
> > > a DT node from the PCI driver that has been probed dynamically
> > > matching this same PCI device with a 'reg' property. I also think
> > > this would requires to generate some 'pci-ranges' to remap the
> > > downstream devices that are described in the DTBO, finally, load the
> > > overlay to be apply under this newly created node. Is that right ?  
> > 
> > Right. You'll need to take the BAR address(es) for the device and stick 
> > those into 'ranges' to translate offsets to BAR+offset.
> >   
> 
> Last time I tried that, this was not working well because it means that
> the ranges property of the device depends on the host machine...
> 

Yes, but we can actually resolve that dynamically. The ranges property
that is inserted is inserted in the top node (the PCI device one).
Since this node will be created by "us" (the kernel/driver), we can
insert whatever we want. And most probably, we'll insert a specific BAR
remapping. The underlying nodes will then be loaded from the overlay
and merged in the top PCI device node (At least I guess ;)). These
nodes don't need any "machine dependent" addresses, only realtive
addresses from the top node.


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

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Wed, 6 Apr 2022 08:19:16 -0500,
Rob Herring <robh@kernel.org> a écrit :

> >   
> > > 
> > >   
> > > > > I've told the Xilinx folks the same thing, but I would separate this
> > > > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > > > creating a base tree an overlay can be applied to. The first part should
> > > > > be pretty straightforward. We already have PCI bus bindings. The only
> > > > > tricky part is getting address translation working from leaf device thru
> > > > > the PCI bus to host bus, but support for that should all be in place
> > > > > (given we support ISA buses off of PCI bus). The second part will
> > > > > require generating PCI DT nodes at runtime. That may be needed for both
> > > > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > > > in DT.    
> > > >
> > > > But then, if the driver generate the nodes, it will most probably
> > > > have to describe the nodes by hardcoding them right ?    
> > > 
> > > No, the kernel already maintains its own tree of devices. You just
> > > need to use that to generate the tree. That's really not much more
> > > than nodes with a 'reg' property encoding the device and function
> > > numbers.  
> > 
> > Just to clarified a point, my PCI device exposes multiple peripherals
> > behind one single PCI function.  
> 
> Right. I would expect your PCI device DT node to have a 'simple-bus' 
> child node with all those peripherals. And maybe there's other nodes 
> like fixed-clocks, etc.
> 
> > To be sure I understood what you are suggesting, you propose to create
> > a DT node from the PCI driver that has been probed dynamically
> > matching this same PCI device with a 'reg' property. I also think
> > this would requires to generate some 'pci-ranges' to remap the
> > downstream devices that are described in the DTBO, finally, load the
> > overlay to be apply under this newly created node. Is that right ?  
> 
> Right. You'll need to take the BAR address(es) for the device and stick 
> those into 'ranges' to translate offsets to BAR+offset.

Hi Rob,

I got something working (address translation, probing and so on) using
what you started. I switch to using changeset however, I'm not sure that
it make sense for property creation since the node has not yet been
added to the tree. Attaching the node with changeset however seems
to make sense. But I'm no expert here, so any advise is welcome.

Based on what we said, I created a PCI driver which uses a builtin
overlay. In order to be able to apply the overlay on the correct PCI
node -the one on which the card was plugged) and thus be totally plug
and play, the 'target-path' property is patched using direct fdt
function and replaced the target with the PCI device node path.
I don't see any other way to do that before applying the overlay since
of_overlay_fdt_apply() takes a fdt blob as input.

The driver also insert correct ranges into the PCI device in order to
translate the downstream node addresses to BAR addresses. It seems
reasonnable to assume that this depends on the driver and thus should
not be done by the PCI of core at all.

Finally, the driver probes the newly added childs using
of_platform_populate(). With all of that, the address translation
and the probing works correctly and the platform devices are created.
There is still a few things to fix such as the following:

[ 2830.324773] OF: overlay: WARNING: memory leak will occur if overlay
removed, property: /pci/pci@2,6/dev@0,0/compatible

But it seems like this is something that works and would allow to
support various use cases. From what I see, it should also work on
other platforms. Major advantage of that over fwnode is that the
changes are pretty small and relatively contained.

However, one point that might be a bit of a problem is enabling
CONFIG_OF on x86 for instance. While it seems to work, is there any
potential concerns about this ? Moreover, ideally, I would want the
driver to "select OF" since without that, the driver won't be visible.
Or should it "depends on OF" but thus, it would be almost mandatory to
enable CONFIG_OF on x86 kernels if we want to support this driver
without the need to recompile a kernel.

Thanks,

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

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Philipp Zabel]

Hi Clément,

On Fr, 2022-04-08 at 17:48 +0200, Clément Léger wrote:
[...]
> > > > > > I've told the Xilinx folks the same thing, but I would separate this
> > > > > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > > > > creating a base tree an overlay can be applied to. The first part should
> > > > > > be pretty straightforward. We already have PCI bus bindings. The only
> > > > > > tricky part is getting address translation working from leaf device thru
> > > > > > the PCI bus to host bus, but support for that should all be in place
> > > > > > (given we support ISA buses off of PCI bus). The second part will
> > > > > > require generating PCI DT nodes at runtime. That may be needed for both
> > > > > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > > > > in DT.    
> > > > > 
> > > > > But then, if the driver generate the nodes, it will most probably
> > > > > have to describe the nodes by hardcoding them right ?    
> > > > 
> > > > No, the kernel already maintains its own tree of devices. You just
> > > > need to use that to generate the tree. That's really not much more
> > > > than nodes with a 'reg' property encoding the device and function
> > > > numbers.  
> > > 
> > > Just to clarified a point, my PCI device exposes multiple peripherals
> > > behind one single PCI function.  
> > 
> > Right. I would expect your PCI device DT node to have a 'simple-bus' 
> > child node with all those peripherals. And maybe there's other nodes 
> > like fixed-clocks, etc.
> > 
> > > To be sure I understood what you are suggesting, you propose to create
> > > a DT node from the PCI driver that has been probed dynamically
> > > matching this same PCI device with a 'reg' property. I also think
> > > this would requires to generate some 'pci-ranges' to remap the
> > > downstream devices that are described in the DTBO, finally, load the
> > > overlay to be apply under this newly created node. Is that right ?  
> > 
> > Right. You'll need to take the BAR address(es) for the device and stick 
> > those into 'ranges' to translate offsets to BAR+offset.
> 
> Hi Rob,
> 
> I got something working (address translation, probing and so on) using
> what you started. I switch to using changeset however, I'm not sure that
> it make sense for property creation since the node has not yet been
> added to the tree. Attaching the node with changeset however seems
> to make sense. But I'm no expert here, so any advise is welcome.
>
> Based on what we said, I created a PCI driver which uses a builtin
> overlay. In order to be able to apply the overlay on the correct PCI
> node -the one on which the card was plugged) and thus be totally plug
> and play, the 'target-path' property is patched using direct fdt
> function and replaced the target with the PCI device node path.
> I don't see any other way to do that before applying the overlay since
> of_overlay_fdt_apply() takes a fdt blob as input.
> 
> The driver also insert correct ranges into the PCI device in order to
> translate the downstream node addresses to BAR addresses. It seems
> reasonnable to assume that this depends on the driver and thus should
> not be done by the PCI of core at all.
> 
> Finally, the driver probes the newly added childs using
> of_platform_populate(). With all of that, the address translation
> and the probing works correctly and the platform devices are created.
> There is still a few things to fix such as the following:
> 
> [ 2830.324773] OF: overlay: WARNING: memory leak will occur if overlay
> removed, property: /pci/pci@2,6/dev@0,0/compatible
> 
> But it seems like this is something that works and would allow to
> support various use cases. From what I see, it should also work on
> other platforms. Major advantage of that over fwnode is that the
> changes are pretty small and relatively contained.

Could you show this off somewhere?

From this I take that fwnode support in the reset subsystem is not of
use to you anymore. I'll postpone taking your patches then, until they
are needed.

regards
Philipp

Re: [PATCH v2 0/3] add fwnode support to reset subsystem

[Clément Léger]

Le Mon, 25 Apr 2022 12:21:15 +0200,
Philipp Zabel <p.zabel@pengutronix.de> a écrit :

> Hi Clément,
> 
> On Fr, 2022-04-08 at 17:48 +0200, Clément Léger wrote:
> [...]
> > > > > > > I've told the Xilinx folks the same thing, but I would separate this
> > > > > > > into 2 parts. First is just h/w work in a DT based system. Second is
> > > > > > > creating a base tree an overlay can be applied to. The first part should
> > > > > > > be pretty straightforward. We already have PCI bus bindings. The only
> > > > > > > tricky part is getting address translation working from leaf device thru
> > > > > > > the PCI bus to host bus, but support for that should all be in place
> > > > > > > (given we support ISA buses off of PCI bus). The second part will
> > > > > > > require generating PCI DT nodes at runtime. That may be needed for both
> > > > > > > DT and ACPI systems as we don't always describe all the PCI hierarchy
> > > > > > > in DT.      
> > > > > > 
> > > > > > But then, if the driver generate the nodes, it will most probably
> > > > > > have to describe the nodes by hardcoding them right ?      
> > > > > 
> > > > > No, the kernel already maintains its own tree of devices. You just
> > > > > need to use that to generate the tree. That's really not much more
> > > > > than nodes with a 'reg' property encoding the device and function
> > > > > numbers.    
> > > > 
> > > > Just to clarified a point, my PCI device exposes multiple peripherals
> > > > behind one single PCI function.    
> > > 
> > > Right. I would expect your PCI device DT node to have a 'simple-bus' 
> > > child node with all those peripherals. And maybe there's other nodes 
> > > like fixed-clocks, etc.
> > >   
> > > > To be sure I understood what you are suggesting, you propose to create
> > > > a DT node from the PCI driver that has been probed dynamically
> > > > matching this same PCI device with a 'reg' property. I also think
> > > > this would requires to generate some 'pci-ranges' to remap the
> > > > downstream devices that are described in the DTBO, finally, load the
> > > > overlay to be apply under this newly created node. Is that right ?    
> > > 
> > > Right. You'll need to take the BAR address(es) for the device and stick 
> > > those into 'ranges' to translate offsets to BAR+offset.  
> > 
> > Hi Rob,
> > 
> > I got something working (address translation, probing and so on) using
> > what you started. I switch to using changeset however, I'm not sure that
> > it make sense for property creation since the node has not yet been
> > added to the tree. Attaching the node with changeset however seems
> > to make sense. But I'm no expert here, so any advise is welcome.
> >
> > Based on what we said, I created a PCI driver which uses a builtin
> > overlay. In order to be able to apply the overlay on the correct PCI
> > node -the one on which the card was plugged) and thus be totally plug
> > and play, the 'target-path' property is patched using direct fdt
> > function and replaced the target with the PCI device node path.
> > I don't see any other way to do that before applying the overlay since
> > of_overlay_fdt_apply() takes a fdt blob as input.
> > 
> > The driver also insert correct ranges into the PCI device in order to
> > translate the downstream node addresses to BAR addresses. It seems
> > reasonnable to assume that this depends on the driver and thus should
> > not be done by the PCI of core at all.
> > 
> > Finally, the driver probes the newly added childs using
> > of_platform_populate(). With all of that, the address translation
> > and the probing works correctly and the platform devices are created.
> > There is still a few things to fix such as the following:
> > 
> > [ 2830.324773] OF: overlay: WARNING: memory leak will occur if overlay
> > removed, property: /pci/pci@2,6/dev@0,0/compatible
> > 
> > But it seems like this is something that works and would allow to
> > support various use cases. From what I see, it should also work on
> > other platforms. Major advantage of that over fwnode is that the
> > changes are pretty small and relatively contained.  
> 
> Could you show this off somewhere?
> 
> From this I take that fwnode support in the reset subsystem is not of
> use to you anymore. I'll postpone taking your patches then, until they
> are needed.
> 
> regards
> Philipp

Hi Philip,

Sorry for the lack of asnwer. Indeed, the fwnode support can be left
out. I'm preparing the patches for contribution for this OF overlay
thing, and everything regarding the subsystem have been removed. You
can look at commits [1], [2], [3] and at a driver using them [4]. This
allows the subsystem to be kept entirely without a single modification
(except for the PCI/OF one).

Thanks,

Clément


[1]
https://github.com/clementleger/linux/commit/248d7f25951f90cf5647d295e1c5051cc72ed970
[2]
https://github.com/clementleger/linux/commit/bf3c4c0749e5110b6a58ac9622cafc10872ed17f
[3]
https://github.com/clementleger/linux/commit/8764a2e386fdede73991415277b95e79553622f3
[4]
https://github.com/clementleger/linux/commit/7c66a1ad8f3fadfb538c410f192c9573c66338d5




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