* [PATCH 0/2] add fwnode support to reset subsystem
@ 2022-03-23 9:50 Clément Léger
2022-03-23 9:50 ` [PATCH 1/2] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Clément Léger @ 2022-03-23 9:50 UTC (permalink / raw)
To: Philipp Zabel, Rob Herring, Frank Rowand
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.
[1] https://lore.kernel.org/netdev/YhPSkz8+BIcdb72R@smile.fi.intel.com/T/
Clément Léger (2):
of: add function to convert fwnode_reference_args to of_phandle_args
reset: add support for fwnode
drivers/reset/core.c | 91 ++++++++++++++++++++++++--------
include/linux/of.h | 18 +++++++
include/linux/reset-controller.h | 14 ++++-
include/linux/reset.h | 14 +++++
4 files changed, 114 insertions(+), 23 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] of: add function to convert fwnode_reference_args to of_phandle_args 2022-03-23 9:50 [PATCH 0/2] add fwnode support to reset subsystem Clément Léger @ 2022-03-23 9:50 ` Clément Léger 2022-03-23 9:50 ` [PATCH 2/2] reset: add support for fwnode Clément Léger 2022-03-23 15:07 ` [PATCH 0/2] add fwnode support to reset subsystem Philipp Zabel 2 siblings, 0 replies; 12+ messages in thread From: Clément Léger @ 2022-03-23 9:50 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Frank Rowand 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] 12+ messages in thread
* [PATCH 2/2] reset: add support for fwnode 2022-03-23 9:50 [PATCH 0/2] add fwnode support to reset subsystem Clément Léger 2022-03-23 9:50 ` [PATCH 1/2] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger @ 2022-03-23 9:50 ` Clément Léger 2022-03-23 15:29 ` Philipp Zabel 2022-03-23 15:07 ` [PATCH 0/2] add fwnode support to reset subsystem Philipp Zabel 2 siblings, 1 reply; 12+ messages in thread From: Clément Léger @ 2022-03-23 9:50 UTC (permalink / raw) To: Philipp Zabel, Rob Herring, Frank Rowand 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 | 91 ++++++++++++++++++++++++-------- include/linux/reset-controller.h | 14 ++++- include/linux/reset.h | 14 +++++ 3 files changed, 96 insertions(+), 23 deletions(-) diff --git a/drivers/reset/core.c b/drivers/reset/core.c index 61e688882643..f014da03b7c1 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> @@ -70,26 +71,49 @@ static const char *rcdev_name(struct reset_controller_dev *rcdev) 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 +122,21 @@ 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) { + 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); + index = fwnode_property_match_string(fwnode, "reset-names", id); if (index == -EILSEQ) 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 * @@ -945,6 +989,9 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id, 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..292552003d11 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,16 @@ 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`. + * to :c:func:`fwnode_of_reset_xlate`. + * @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`. * @nr_resets: number of reset controls in this reset controller device */ struct reset_controller_dev { @@ -73,9 +81,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] 12+ messages in thread
* Re: [PATCH 2/2] reset: add support for fwnode 2022-03-23 9:50 ` [PATCH 2/2] reset: add support for fwnode Clément Léger @ 2022-03-23 15:29 ` Philipp Zabel 2022-03-23 16:21 ` Clément Léger 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2022-03-23 15:29 UTC (permalink / raw) To: Clément Léger, Rob Herring, Frank Rowand Cc: Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree On Mi, 2022-03-23 at 10:50 +0100, Clément Léger wrote: [...] > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > index 61e688882643..f014da03b7c1 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> > @@ -70,26 +71,49 @@ static const char *rcdev_name(struct > reset_controller_dev *rcdev) > if (rcdev->of_node) > return rcdev->of_node->full_name; Could the above be removed, since reset_controller_register() set rcdev->fwnode to of_fwnode_handle(rcdev->of_node) earlier? > > + if (rcdev->fwnode) > + return fwnode_get_name(rcdev->fwnode); > + > return NULL; > } > [...] > > /** > @@ -98,9 +122,21 @@ 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) { > + rcdev->fwnode_xlate = fwnode_of_reset_xlate; It should be documented that .fwnode_xlate/.fwnode_reset_n_cells are ignored if .of_xlate is set. > + 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); > + index = fwnode_property_match_string(fwnode, "reset-names", id); > if (index == -EILSEQ) > return ERR_PTR(index); I don't think this is good enough any more. At least -ENOMEM is added as a possible error return code by this change. [...] > @@ -945,6 +989,9 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id, > if (dev->of_node) > return __of_reset_control_get(dev->of_node, id, index, shared, > optional, acquired); Could the above be removed, given that __of_reset_control_get() just wraps __fwnode_reset_control_get(), which is called right below: > + 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..292552003d11 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,16 @@ 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`. > + * to :c:func:`fwnode_of_reset_xlate`. > + * @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`. This should mention that .fwnode_xlate is ignored/overwritten when .of_xlate is set. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] reset: add support for fwnode 2022-03-23 15:29 ` Philipp Zabel @ 2022-03-23 16:21 ` Clément Léger 2022-03-24 9:19 ` Clément Léger 2022-03-24 9:44 ` Philipp Zabel 0 siblings, 2 replies; 12+ messages in thread From: Clément Léger @ 2022-03-23 16:21 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree Le Wed, 23 Mar 2022 16:29:41 +0100, Philipp Zabel <p.zabel@pengutronix.de> a écrit : > On Mi, 2022-03-23 at 10:50 +0100, Clément Léger wrote: > [...] > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > index 61e688882643..f014da03b7c1 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> > > @@ -70,26 +71,49 @@ static const char *rcdev_name(struct > > reset_controller_dev *rcdev) > > if (rcdev->of_node) > > return rcdev->of_node->full_name; > > Could the above be removed, since reset_controller_register() set > rcdev->fwnode to of_fwnode_handle(rcdev->of_node) earlier? Yes, this should work in all cases, the only difference is that fwnode_get_name() returns the basename of the of_node full_name field. This is potentially a change from what was displayed before. If you are ok with that, I'll drop these lines. [...] > > + } > > + > > + if (rcdev->of_xlate) { > > + rcdev->fwnode_xlate = fwnode_of_reset_xlate; > > It should be documented that .fwnode_xlate/.fwnode_reset_n_cells are > ignored if .of_xlate is set. Acked. [...] > > if (id) { > > - index = of_property_match_string(node, > > - "reset-names", id); > > + index = fwnode_property_match_string(fwnode, "reset-names", id); > > if (index == -EILSEQ) > > return ERR_PTR(index); > > I don't think this is good enough any more. At least -ENOMEM is added > as a possible error return code by this change. Yes indeed, errors are clearly not correctly handled anymore. At least -EILSEQ won't be triggered. > > [...] > > @@ -945,6 +989,9 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id, > > if (dev->of_node) > > return __of_reset_control_get(dev->of_node, id, index, shared, > > optional, acquired); > > Could the above be removed, given that __of_reset_control_get() just > wraps __fwnode_reset_control_get(), which is called right below: Oh yes, sorry for that. It can clearly be removed. [...] > > * @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`. > > + * to :c:func:`fwnode_of_reset_xlate`. > > + * @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`. > > This should mention that .fwnode_xlate is ignored/overwritten when > .of_xlate is set. Acked. > > > regards > Philipp Regards, -- Clément Léger, Embedded Linux and Kernel engineer at Bootlin https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] reset: add support for fwnode 2022-03-23 16:21 ` Clément Léger @ 2022-03-24 9:19 ` Clément Léger 2022-03-24 9:39 ` Philipp Zabel 2022-03-24 9:44 ` Philipp Zabel 1 sibling, 1 reply; 12+ messages in thread From: Clément Léger @ 2022-03-24 9:19 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree Le Wed, 23 Mar 2022 17:21:21 +0100, Clément Léger <clement.leger@bootlin.com> a écrit : > > > if (id) { > > > - index = of_property_match_string(node, > > > - "reset-names", id); > > > + index = fwnode_property_match_string(fwnode, "reset-names", id); > > > if (index == -EILSEQ) > > > return ERR_PTR(index); > > > > I don't think this is good enough any more. At least -ENOMEM is added > > as a possible error return code by this change. > > Yes indeed, errors are clearly not correctly handled anymore. At least > -EILSEQ won't be triggered. > > By the way, even after looking at this more carefully, I'm not sure to understand why there is a special handling for -EILSEQ ? From what I understand, EILSEQ is returned in case the device tree is malformed (string longer than returned property length) but why is it handled differently in this case ? Thanks, -- Clément Léger, Embedded Linux and Kernel engineer at Bootlin https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] reset: add support for fwnode 2022-03-24 9:19 ` Clément Léger @ 2022-03-24 9:39 ` Philipp Zabel 0 siblings, 0 replies; 12+ messages in thread From: Philipp Zabel @ 2022-03-24 9:39 UTC (permalink / raw) To: Clément Léger Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree Hi Clément, On Do, 2022-03-24 at 10:19 +0100, Clément Léger wrote: > Le Wed, 23 Mar 2022 17:21:21 +0100, > Clément Léger <clement.leger@bootlin.com> a écrit : > > > > > if (id) { > > > > - index = of_property_match_string(node, > > > > - "reset-names", > > > > id); > > > > + index = fwnode_property_match_string(fwnode, > > > > "reset-names", id); > > > > if (index == -EILSEQ) > > > > return ERR_PTR(index); > > > > > > I don't think this is good enough any more. At least -ENOMEM is added > > > as a possible error return code by this change. > > > > Yes indeed, errors are clearly not correctly handled anymore. At least > > -EILSEQ won't be triggered. > > > > > By the way, even after looking at this more carefully, I'm not sure to > understand why there is a special handling for -EILSEQ ? From what I > understand, EILSEQ is returned in case the device tree is malformed > (string longer than returned property length) but why is it handled > differently in this case ? of_property_match_string() can return four error codes: -EINVAL (no reset-names property in node) -EILSEQ (real error) -ENODATA (id is not found in reset-names) -ENOSYS (CONFIG_OF is not set) -EINVAL, -ENODATA and -ENOSYS map to -ENOENT (no reset control corresponding to id found), or no error if it is optional. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] reset: add support for fwnode 2022-03-23 16:21 ` Clément Léger 2022-03-24 9:19 ` Clément Léger @ 2022-03-24 9:44 ` Philipp Zabel 1 sibling, 0 replies; 12+ messages in thread From: Philipp Zabel @ 2022-03-24 9:44 UTC (permalink / raw) To: Clément Léger Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree On Mi, 2022-03-23 at 17:21 +0100, Clément Léger wrote: > Le Wed, 23 Mar 2022 16:29:41 +0100, > Philipp Zabel <p.zabel@pengutronix.de> a écrit : > > > On Mi, 2022-03-23 at 10:50 +0100, Clément Léger wrote: > > [...] > > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > > index 61e688882643..f014da03b7c1 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> > > > @@ -70,26 +71,49 @@ static const char *rcdev_name(struct > > > reset_controller_dev *rcdev) > > > if (rcdev->of_node) > > > return rcdev->of_node->full_name; > > > > Could the above be removed, since reset_controller_register() set > > rcdev->fwnode to of_fwnode_handle(rcdev->of_node) earlier? > > Yes, this should work in all cases, the only difference is that > fwnode_get_name() returns the basename of the of_node full_name > field. > This is potentially a change from what was displayed before. If you are > ok with that, I'll drop these lines. Yes, that should be fine. rcdev_name() is currently only used in warnings that let driver developers know which reset controller they used incorrectly. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] add fwnode support to reset subsystem 2022-03-23 9:50 [PATCH 0/2] add fwnode support to reset subsystem Clément Léger 2022-03-23 9:50 ` [PATCH 1/2] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger 2022-03-23 9:50 ` [PATCH 2/2] reset: add support for fwnode Clément Léger @ 2022-03-23 15:07 ` Philipp Zabel 2022-03-23 16:05 ` Clément Léger 2 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2022-03-23 15:07 UTC (permalink / raw) To: Clément Léger, Rob Herring, Frank Rowand Cc: Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree Hi Clément, On Mi, 2022-03-23 at 10:50 +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. Could you explain the purpose of this a little? From the referenced mail it looks like this would be intended allow to register reset controllers via software node? Are there any real systems where this would be useful? > For the moment ACPI node support is excluded from the fwnode support > to avoid creating an unspecified ACPI reset device description. Are there any plans or ongoing discussions to specify such a description in the future? Right now I'm only aware of the ACPI _RST method as used by this patch: [1] https://lore.kernel.org/all/20220307135626.16673-1-kyarlagadda@nvidia.com/ > 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. I would prefer not to have to switch all those small DT-only reset controller drivers all over the tree from of_node to fwnode. On the other hand, I think it would be good to avoid the direct of_node assignment, possibly by letting devm_reset_controller_register() initialize of_node or fwnode from the device for most cases, and by adding of_reset_controller_register() and fwnode_reset_controller_register() variants that take the node as an argument for the rest. That could allow to eventually get rid of the of_node pointer. For those drivers that provide their own .of_xlate, I'm not sure it would make sense to force them to use .fwnode_xlate if they don't already have a reason to use fwnode on their own. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] add fwnode support to reset subsystem 2022-03-23 15:07 ` [PATCH 0/2] add fwnode support to reset subsystem Philipp Zabel @ 2022-03-23 16:05 ` Clément Léger 2022-03-24 10:08 ` Philipp Zabel 0 siblings, 1 reply; 12+ messages in thread From: Clément Léger @ 2022-03-23 16:05 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree Le Wed, 23 Mar 2022 16:07:26 +0100, Philipp Zabel <p.zabel@pengutronix.de> a écrit : > Hi Clément, > > On Mi, 2022-03-23 at 10:50 +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. > > Could you explain the purpose of this a little? From the referenced > mail it looks like this would be intended allow to register reset > controllers via software node? Are there any real systems where this > would be useful? Hi Philipp and thanks for reviewing this series. As you noticed, the initial goal of the primary series was to add fwnode support in order to allow registering devices with software nodes. Since a lot of subsystem are of-centric, It was needed to modify them to use fwnode and thus accept the use of software nodes. The device I'm trying to support is a PCIe card that uses a lan9662 SoC. This card is meant to be used an ethernet switch with 2 x RJ45 ports and 2 x 10G SFPs. The lan966x SoCs can be used in two different ways: - It can run Linux by itself, on ARM64 cores included in the SoC. This use-case of the lan966x is currently being upstreamed, using a traditional Device Tree representation of the lan996x HW blocks [1] A number of drivers for the different IPs of the SoC have already been merged in upstream Linux. - It can be used as a PCIe endpoint, connected to a separate platform that acts as the PCIe root complex. In this case, all the devices that are embedded on this SoC are exposed through PCIe BARs and the ARM64 cores of the SoC are not used. Since this is a PCIe card, it can be plugged on any platform, of any architecture supporting PCIe. Appart from adding software node support, the fwnode API would also allow to add ACPI support more easily later. > > > For the moment ACPI node support is excluded from the fwnode support > > to avoid creating an unspecified ACPI reset device description. > > Are there any plans or ongoing discussions to specify such a > description in the future? Right now I'm only aware of the ACPI _RST > method as used by this patch: > > [1] https://lore.kernel.org/all/20220307135626.16673-1-kyarlagadda@nvidia.com/ > On that side, I must say I'm not really competent regarding ACPI which I do not know enough to answer you on that point. The discussions we had with Mark Brown regarding fwnode ACPI support pointed out the fact that we should not create unwanted ACPI support by using the same descriptions/specifications that exists for the device-tree. In order to avoid that, we suggested to explicitely left out ACPI with this fwnode support. This will allow to specify that support later and integrate it in the subsystem that have been converted to fwnode. > > 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. > > I would prefer not to have to switch all those small DT-only reset > controller drivers all over the tree from of_node to fwnode. That makes sense. > On the other hand, I think it would be good to avoid the direct of_node > assignment, possibly by letting devm_reset_controller_register() > initialize of_node or fwnode from the device for most cases, and by > adding of_reset_controller_register() and > fwnode_reset_controller_register() variants that take the node as an > argument for the rest. > That could allow to eventually get rid of the of_node pointer. Ok, I see that. Do you want this to be done in this series ? > > For those drivers that provide their own .of_xlate, I'm not sure it > would make sense to force them to use .fwnode_xlate if they don't > already have a reason to use fwnode on their own. No indeed and that's why I added the fwnode_xlate -> of_xlate translation function, this will allow to keep the existing of_xlate support. > > regards > Philipp Regards, -- Clément Léger, Embedded Linux and Kernel engineer at Bootlin https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] add fwnode support to reset subsystem 2022-03-23 16:05 ` Clément Léger @ 2022-03-24 10:08 ` Philipp Zabel 2022-03-24 10:16 ` Clément Léger 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2022-03-24 10:08 UTC (permalink / raw) To: Clément Léger Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree On Mi, 2022-03-23 at 17:05 +0100, Clément Léger wrote: [...] > > As you noticed, the initial goal of the primary series was to add > fwnode support in order to allow registering devices with software > nodes. Since a lot of subsystem are of-centric, It was needed to > modify them to use fwnode and thus accept the use of software nodes. > > The device I'm trying to support is a PCIe card that uses a lan9662 > SoC. This card is meant to be used an ethernet switch with 2 x RJ45 > ports and 2 x 10G SFPs. The lan966x SoCs can be used in two different > ways: > > - It can run Linux by itself, on ARM64 cores included in the SoC. This > use-case of the lan966x is currently being upstreamed, using a > traditional Device Tree representation of the lan996x HW blocks [1] > A number of drivers for the different IPs of the SoC have already > been merged in upstream Linux. > > - It can be used as a PCIe endpoint, connected to a separate platform > that acts as the PCIe root complex. In this case, all the devices > that are embedded on this SoC are exposed through PCIe BARs and the > ARM64 cores of the SoC are not used. Since this is a PCIe card, it > can be plugged on any platform, of any architecture supporting PCIe. > > Appart from adding software node support, the fwnode API would also > allow to add ACPI support more easily later. Thank you for the explanation. So this would be used by the sparx5 switch reset driver to provide the microchip,lan966x-switch-reset controller via software node? If that needs to be converted to fwnode anyway, it would be nice to include the conversion in this series as an example. [...] > On that side, I must say I'm not really competent regarding ACPI > which I do not know enough to answer you on that point. > > The discussions we had with Mark Brown regarding fwnode ACPI support > pointed out the fact that we should not create unwanted ACPI support > by using the same descriptions/specifications that exists for the > device-tree. In order to avoid that, we suggested to explicitely left > out ACPI with this fwnode support. This will allow to specify that > support later and integrate it in the subsystem that have been > converted to fwnode. Ok. > > > > On the other hand, I think it would be good to avoid the direct of_node > > assignment, possibly by letting devm_reset_controller_register() > > initialize of_node or fwnode from the device for most cases, and by > > adding of_reset_controller_register() and > > fwnode_reset_controller_register() variants that take the node as an > > argument for the rest. > > That could allow to eventually get rid of the of_node pointer. > > Ok, I see that. Do you want this to be done in this series ? Just thinking out loudly, before starting to drop the rcdev->of_node assigment from drivers en masse, I'd like to use the opportunity and turn reset_controller_register() and friends into macros that provide the module owner as a parameter, so the explicit rcdev->owner = THIS_MODULE assignment can be removed from the drivers as well. I think that is better done separately. > > For those drivers that provide their own .of_xlate, I'm not sure it > > would make sense to force them to use .fwnode_xlate if they don't > > already have a reason to use fwnode on their own. > > No indeed and that's why I added the fwnode_xlate -> of_xlate > translation function, this will allow to keep the existing of_xlate > support. Ok. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] add fwnode support to reset subsystem 2022-03-24 10:08 ` Philipp Zabel @ 2022-03-24 10:16 ` Clément Léger 0 siblings, 0 replies; 12+ messages in thread From: Clément Léger @ 2022-03-24 10:16 UTC (permalink / raw) To: Philipp Zabel Cc: Rob Herring, Frank Rowand, Thomas Petazzoni, Alexandre Belloni, Allan Nielsen, linux-kernel, devicetree Le Thu, 24 Mar 2022 11:08:15 +0100, Philipp Zabel <p.zabel@pengutronix.de> a écrit : > > - It can be used as a PCIe endpoint, connected to a separate platform > > that acts as the PCIe root complex. In this case, all the devices > > that are embedded on this SoC are exposed through PCIe BARs and the > > ARM64 cores of the SoC are not used. Since this is a PCIe card, it > > can be plugged on any platform, of any architecture supporting PCIe. > > > > Appart from adding software node support, the fwnode API would also > > allow to add ACPI support more easily later. > > Thank you for the explanation. So this would be used by the sparx5 > switch reset driver to provide the microchip,lan966x-switch-reset > controller via software node? Exactly. > > If that needs to be converted to fwnode anyway, it would be nice to > include the conversion in this series as an example. Yes indeed, the sparx5 driver was modified in my private tree. I will change it to use fwnode. > > [...] > > On that side, I must say I'm not really competent regarding ACPI > > which I do not know enough to answer you on that point. > > > > The discussions we had with Mark Brown regarding fwnode ACPI support > > pointed out the fact that we should not create unwanted ACPI support > > by using the same descriptions/specifications that exists for the > > device-tree. In order to avoid that, we suggested to explicitely left > > out ACPI with this fwnode support. This will allow to specify that > > support later and integrate it in the subsystem that have been > > converted to fwnode. > > Ok. > > > > > > > On the other hand, I think it would be good to avoid the direct of_node > > > assignment, possibly by letting devm_reset_controller_register() > > > initialize of_node or fwnode from the device for most cases, and by > > > adding of_reset_controller_register() and > > > fwnode_reset_controller_register() variants that take the node as an > > > argument for the rest. > > > That could allow to eventually get rid of the of_node pointer. > > > > Ok, I see that. Do you want this to be done in this series ? > > Just thinking out loudly, before starting to drop the > rcdev->of_node assigment from drivers en masse, I'd like to use the > opportunity and turn reset_controller_register() and friends into > macros that provide the module owner as a parameter, so the explicit > rcdev->owner = THIS_MODULE assignment can be removed from the drivers > as well. Indeed, that seems like a good thing to do, direct assignments are often a pain to change all other the place. BTW, once drivers are converted to avoid direct assignment of the of_node field, it will be removable, the fwnode field will be sufficient for all operations. Thanks, Clément -- Clément Léger, Embedded Linux and Kernel engineer at Bootlin https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-03-24 10:18 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-23 9:50 [PATCH 0/2] add fwnode support to reset subsystem Clément Léger 2022-03-23 9:50 ` [PATCH 1/2] of: add function to convert fwnode_reference_args to of_phandle_args Clément Léger 2022-03-23 9:50 ` [PATCH 2/2] reset: add support for fwnode Clément Léger 2022-03-23 15:29 ` Philipp Zabel 2022-03-23 16:21 ` Clément Léger 2022-03-24 9:19 ` Clément Léger 2022-03-24 9:39 ` Philipp Zabel 2022-03-24 9:44 ` Philipp Zabel 2022-03-23 15:07 ` [PATCH 0/2] add fwnode support to reset subsystem Philipp Zabel 2022-03-23 16:05 ` Clément Léger 2022-03-24 10:08 ` Philipp Zabel 2022-03-24 10:16 ` 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).