* [PATCH v2 0/3] add fwnode support to reset subsystem @ 2022-03-24 14:12 Clément Léger 2022-03-24 14:12 ` [PATCH v2 1/3] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Clément Léger @ 2022-03-24 14:12 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver Cc: Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/3] of: add function to convert fwnode_reference_args to of_phandle_args 2022-03-24 14:12 [PATCH v2 0/3] add fwnode support to reset subsystem Clément Léger @ 2022-03-24 14:12 ` Clément Léger 2022-03-24 14:12 ` [PATCH v2 2/3] reset: add support for fwnode Clément Léger ` (2 subsequent siblings) 3 siblings, 0 replies; 19+ messages in thread From: Clément Léger @ 2022-03-24 14:12 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver Cc: Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/3] reset: add support for fwnode 2022-03-24 14:12 [PATCH v2 0/3] add fwnode support to reset subsystem Clément Léger 2022-03-24 14:12 ` [PATCH v2 1/3] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger @ 2022-03-24 14:12 ` Clément Léger 2022-03-24 14:12 ` [PATCH v2 3/3] reset: mchp: sparx5: set fwnode field of reset controller Clément Léger 2022-04-04 17:41 ` [PATCH v2 0/3] add fwnode support to reset subsystem Rob Herring 3 siblings, 0 replies; 19+ messages in thread From: Clément Léger @ 2022-03-24 14:12 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver Cc: Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/3] reset: mchp: sparx5: set fwnode field of reset controller 2022-03-24 14:12 [PATCH v2 0/3] add fwnode support to reset subsystem Clément Léger 2022-03-24 14:12 ` [PATCH v2 1/3] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger 2022-03-24 14:12 ` [PATCH v2 2/3] reset: add support for fwnode Clément Léger @ 2022-03-24 14:12 ` Clément Léger 2022-04-04 17:41 ` [PATCH v2 0/3] add fwnode support to reset subsystem Rob Herring 3 siblings, 0 replies; 19+ messages in thread From: Clément Léger @ 2022-03-24 14:12 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver Cc: Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-03-24 14:12 [PATCH v2 0/3] add fwnode support to reset subsystem Clément Léger ` (2 preceding siblings ...) 2022-03-24 14:12 ` [PATCH v2 3/3] reset: mchp: sparx5: set fwnode field of reset controller Clément Léger @ 2022-04-04 17:41 ` Rob Herring 2022-04-05 7:24 ` Clément Léger 3 siblings, 1 reply; 19+ messages in thread From: Rob Herring @ 2022-04-04 17:41 UTC (permalink / raw) To: Clément Léger Cc: Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-04 17:41 ` [PATCH v2 0/3] add fwnode support to reset subsystem Rob Herring @ 2022-04-05 7:24 ` Clément Léger 2022-04-05 14:47 ` Rob Herring 0 siblings, 1 reply; 19+ messages in thread From: Clément Léger @ 2022-04-05 7:24 UTC (permalink / raw) To: Rob Herring Cc: Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-05 7:24 ` Clément Léger @ 2022-04-05 14:47 ` Rob Herring 2022-04-05 15:51 ` Clément Léger 0 siblings, 1 reply; 19+ messages in thread From: Rob Herring @ 2022-04-05 14:47 UTC (permalink / raw) To: Clément Léger, Lizhi Hou, Sonal Santan Cc: Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini + 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/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-05 14:47 ` Rob Herring @ 2022-04-05 15:51 ` Clément Léger 2022-04-05 17:11 ` Rob Herring 0 siblings, 1 reply; 19+ messages in thread From: Clément Léger @ 2022-04-05 15:51 UTC (permalink / raw) To: Rob Herring Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, UNGLinuxDriver, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-05 15:51 ` Clément Léger @ 2022-04-05 17:11 ` Rob Herring 2022-04-05 21:28 ` Rob Herring 2022-04-06 7:40 ` Clément Léger 0 siblings, 2 replies; 19+ messages in thread From: Rob Herring @ 2022-04-05 17:11 UTC (permalink / raw) To: Clément Léger Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-05 17:11 ` Rob Herring @ 2022-04-05 21:28 ` Rob Herring 2022-04-06 7:52 ` Clément Léger 2022-04-06 7:40 ` Clément Léger 1 sibling, 1 reply; 19+ messages in thread From: Rob Herring @ 2022-04-05 21:28 UTC (permalink / raw) To: Clément Léger Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-05 21:28 ` Rob Herring @ 2022-04-06 7:52 ` Clément Léger 2022-04-06 13:04 ` Rob Herring 0 siblings, 1 reply; 19+ messages in thread From: Clément Léger @ 2022-04-06 7:52 UTC (permalink / raw) To: Rob Herring Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-06 7:52 ` Clément Léger @ 2022-04-06 13:04 ` Rob Herring 0 siblings, 0 replies; 19+ messages in thread From: Rob Herring @ 2022-04-06 13:04 UTC (permalink / raw) To: Clément Léger Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-05 17:11 ` Rob Herring 2022-04-05 21:28 ` Rob Herring @ 2022-04-06 7:40 ` Clément Léger 2022-04-06 13:19 ` Rob Herring 1 sibling, 1 reply; 19+ messages in thread From: Clément Léger @ 2022-04-06 7:40 UTC (permalink / raw) To: Rob Herring Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-06 7:40 ` Clément Léger @ 2022-04-06 13:19 ` Rob Herring 2022-04-06 13:33 ` Alexandre Belloni 2022-04-08 15:48 ` Clément Léger 0 siblings, 2 replies; 19+ messages in thread From: Rob Herring @ 2022-04-06 13:19 UTC (permalink / raw) To: Clément Léger Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-06 13:19 ` Rob Herring @ 2022-04-06 13:33 ` Alexandre Belloni 2022-04-06 13:36 ` Clément Léger 2022-04-08 15:48 ` Clément Léger 1 sibling, 1 reply; 19+ messages in thread From: Alexandre Belloni @ 2022-04-06 13:33 UTC (permalink / raw) To: Rob Herring Cc: Clément Léger, Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-06 13:33 ` Alexandre Belloni @ 2022-04-06 13:36 ` Clément Léger 0 siblings, 0 replies; 19+ messages in thread From: Clément Léger @ 2022-04-06 13:36 UTC (permalink / raw) To: Alexandre Belloni Cc: Rob Herring, Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-06 13:19 ` Rob Herring 2022-04-06 13:33 ` Alexandre Belloni @ 2022-04-08 15:48 ` Clément Léger 2022-04-25 10:21 ` Philipp Zabel 1 sibling, 1 reply; 19+ messages in thread From: Clément Léger @ 2022-04-08 15:48 UTC (permalink / raw) To: Rob Herring Cc: Lizhi Hou, Sonal Santan, Philipp Zabel, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-08 15:48 ` Clément Léger @ 2022-04-25 10:21 ` Philipp Zabel 2022-04-25 11:18 ` Clément Léger 0 siblings, 1 reply; 19+ messages in thread From: Philipp Zabel @ 2022-04-25 10:21 UTC (permalink / raw) To: Clément Léger, Rob Herring Cc: Lizhi Hou, Sonal Santan, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] add fwnode support to reset subsystem 2022-04-25 10:21 ` Philipp Zabel @ 2022-04-25 11:18 ` Clément Léger 0 siblings, 0 replies; 19+ messages in thread From: Clément Léger @ 2022-04-25 11:18 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Lizhi Hou, Sonal Santan, Frank Rowand, Lars Povlsen, Steen Hegelund, Microchip Linux Driver Support, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree, Stefano Stabellini, Hans de Goede, Mark Brown 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-04-25 11:19 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-24 14:12 [PATCH v2 0/3] add fwnode support to reset subsystem Clément Léger 2022-03-24 14:12 ` [PATCH v2 1/3] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger 2022-03-24 14:12 ` [PATCH v2 2/3] reset: add support for fwnode Clément Léger 2022-03-24 14:12 ` [PATCH v2 3/3] reset: mchp: sparx5: set fwnode field of reset controller Clément Léger 2022-04-04 17:41 ` [PATCH v2 0/3] add fwnode support to reset subsystem Rob Herring 2022-04-05 7:24 ` Clément Léger 2022-04-05 14:47 ` Rob Herring 2022-04-05 15:51 ` Clément Léger 2022-04-05 17:11 ` Rob Herring 2022-04-05 21:28 ` Rob Herring 2022-04-06 7:52 ` Clément Léger 2022-04-06 13:04 ` Rob Herring 2022-04-06 7:40 ` Clément Léger 2022-04-06 13:19 ` Rob Herring 2022-04-06 13:33 ` Alexandre Belloni 2022-04-06 13:36 ` Clément Léger 2022-04-08 15:48 ` Clément Léger 2022-04-25 10:21 ` Philipp Zabel 2022-04-25 11:18 ` Clément Léger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).