| Message ID | 20251006-reset-gpios-swnodes-v1-3-6d3325b9af42@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | reset: rework reset-gpios handling | expand |
On Mon, Oct 06, 2025 at 03:00:18PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > At the moment software nodes can only reference other software nodes. > This is a limitation for devices created, for instance, on the auxiliary > bus with a dynamic software node attached which cannot reference devices > the firmware node of which is "real" (as an OF node or otherwise). > > Make it possible for a software node to reference all firmware nodes in > addition to static software nodes. To that end: use a union of different > pointers in struct software_node_ref_args and add an enum indicating > what kind of reference given instance of it is. Rework the helper macros > and deprecate the existing ones whose names don't indicate the reference > type. > > Software node graphs remain the same, as in: the remote endpoints still > have to be software nodes. ... > +enum software_node_ref_type { > + /* References a software node. */ > + SOFTWARE_NODE_REF_SWNODE = 0, I don't see why we need an explicit value here. > + /* References a firmware node. */ > + SOFTWARE_NODE_REF_FWNODE, > +}; ... > /** > * struct software_node_ref_args - Reference property with additional arguments > - * @node: Reference to a software node > + * @swnode: Reference to a software node > + * @fwnode: Alternative reference to a firmware node handle > * @nargs: Number of elements in @args array > * @args: Integer arguments > */ > struct software_node_ref_args { > - const struct software_node *node; > + enum software_node_ref_type type; > + union { > + const struct software_node *swnode; > + struct fwnode_handle *fwnode; > + }; Can't we always have an fwnode reference? > unsigned int nargs; > u64 args[NR_FWNODE_REFERENCE_ARGS]; > };
On Sat, Oct 18, 2025 at 7:34 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Oct 06, 2025 at 03:00:18PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > At the moment software nodes can only reference other software nodes. > > This is a limitation for devices created, for instance, on the auxiliary > > bus with a dynamic software node attached which cannot reference devices > > the firmware node of which is "real" (as an OF node or otherwise). > > > > Make it possible for a software node to reference all firmware nodes in > > addition to static software nodes. To that end: use a union of different > > pointers in struct software_node_ref_args and add an enum indicating > > what kind of reference given instance of it is. Rework the helper macros > > and deprecate the existing ones whose names don't indicate the reference > > type. > > > > Software node graphs remain the same, as in: the remote endpoints still > > have to be software nodes. > > ... > > > +enum software_node_ref_type { > > + /* References a software node. */ > > + SOFTWARE_NODE_REF_SWNODE = 0, > > > I don't see why we need an explicit value here. > It was to make it clear, this is the default value and it's the one used in older code with the legacy macros. I can drop it, it's no big deal. > > + /* References a firmware node. */ > > + SOFTWARE_NODE_REF_FWNODE, > > +}; > > ... > > > /** > > * struct software_node_ref_args - Reference property with additional arguments > > - * @node: Reference to a software node > > + * @swnode: Reference to a software node > > + * @fwnode: Alternative reference to a firmware node handle > > * @nargs: Number of elements in @args array > > * @args: Integer arguments > > */ > > struct software_node_ref_args { > > - const struct software_node *node; > > + enum software_node_ref_type type; > > + union { > > + const struct software_node *swnode; > > + struct fwnode_handle *fwnode; > > + }; > > Can't we always have an fwnode reference? > Unfortunately no. A const struct software_node is not yet a full fwnode, it's just a template that becomes an actual firmware node when it's registered with the swnode framework. However in order to allow creating a graph of software nodes before we register them, we need a way to reference those templates and then look them up internally in swnode code. Bart > > unsigned int nargs; > > u64 args[NR_FWNODE_REFERENCE_ARGS]; > > }; > > -- > With Best Regards, > Andy Shevchenko > > >
On Mon, Oct 20, 2025 at 10:06:43AM +0200, Bartosz Golaszewski wrote: > On Sat, Oct 18, 2025 at 7:34 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Oct 06, 2025 at 03:00:18PM +0200, Bartosz Golaszewski wrote: ... > > > +enum software_node_ref_type { > > > + /* References a software node. */ > > > + SOFTWARE_NODE_REF_SWNODE = 0, > > > > I don't see why we need an explicit value here. > > It was to make it clear, this is the default value and it's the one > used in older code with the legacy macros. I can drop it, it's no big > deal. Usually when we assign a default value(s) in enum, it should be justified. The common mistake here (not this case) is to use autoincrement feature with some of the values explicitly defined for the enums that reflect the HW bits / states, which obviously makes code fragile and easy to break. > > > + /* References a firmware node. */ > > > + SOFTWARE_NODE_REF_FWNODE, > > > +}; ... > > > /** > > > * struct software_node_ref_args - Reference property with additional arguments > > > - * @node: Reference to a software node > > > + * @swnode: Reference to a software node > > > + * @fwnode: Alternative reference to a firmware node handle > > > * @nargs: Number of elements in @args array > > > * @args: Integer arguments > > > */ > > > struct software_node_ref_args { > > > - const struct software_node *node; > > > + enum software_node_ref_type type; > > > + union { > > > + const struct software_node *swnode; > > > + struct fwnode_handle *fwnode; > > > + }; > > > > Can't we always have an fwnode reference? > > Unfortunately no. A const struct software_node is not yet a full > fwnode, it's just a template that becomes an actual firmware node when > it's registered with the swnode framework. However in order to allow > creating a graph of software nodes before we register them, we need a > way to reference those templates and then look them up internally in > swnode code. Strange that you need this way. The IPU3 bridge driver (that creates a graph of fwnodes at run-time for being consumed by the respective parts of v4l2 framework) IIRC has no such issue. Why your case is different? > > > unsigned int nargs; > > > u64 args[NR_FWNODE_REFERENCE_ARGS]; > > > };
On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > Can't we always have an fwnode reference? > > > > Unfortunately no. A const struct software_node is not yet a full > > fwnode, it's just a template that becomes an actual firmware node when > > it's registered with the swnode framework. However in order to allow > > creating a graph of software nodes before we register them, we need a > > way to reference those templates and then look them up internally in > > swnode code. > > Strange that you need this way. The IPU3 bridge driver (that creates a graph of > fwnodes at run-time for being consumed by the respective parts of v4l2 > framework) IIRC has no such issue. Why your case is different? > From what I can tell the ipu-bridge driver only references software nodes (as struct software_node) from other software nodes. I need to reference ANY implementation of firmware node from a software node. Bartosz
Hi Bartosz, Andy, On Mon, Oct 20, 2025 at 01:26:59PM +0200, Bartosz Golaszewski wrote: > On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > Can't we always have an fwnode reference? > > > > > > Unfortunately no. A const struct software_node is not yet a full > > > fwnode, it's just a template that becomes an actual firmware node when > > > it's registered with the swnode framework. However in order to allow > > > creating a graph of software nodes before we register them, we need a > > > way to reference those templates and then look them up internally in > > > swnode code. > > > > Strange that you need this way. The IPU3 bridge driver (that creates a graph of > > fwnodes at run-time for being consumed by the respective parts of v4l2 > > framework) IIRC has no such issue. Why your case is different? > > > > From what I can tell the ipu-bridge driver only references software > nodes (as struct software_node) from other software nodes. I need to > reference ANY implementation of firmware node from a software node. Yes, the IPU bridge only references software nodes. I might use two distinct pointers instead of an union and an integer field that tells which type is the right one. I don't expect more such cases here; it's either a software node or an fwnode handle (ACPI or OF node).
On Tue, 21 Oct 2025 08:54:22 +0200, Sakari Ailus <sakari.ailus@linux.intel.com> said: > Hi Bartosz, Andy, > > On Mon, Oct 20, 2025 at 01:26:59PM +0200, Bartosz Golaszewski wrote: >> On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > >> > > > >> > > > Can't we always have an fwnode reference? >> > > >> > > Unfortunately no. A const struct software_node is not yet a full >> > > fwnode, it's just a template that becomes an actual firmware node when >> > > it's registered with the swnode framework. However in order to allow >> > > creating a graph of software nodes before we register them, we need a >> > > way to reference those templates and then look them up internally in >> > > swnode code. >> > >> > Strange that you need this way. The IPU3 bridge driver (that creates a graph of >> > fwnodes at run-time for being consumed by the respective parts of v4l2 >> > framework) IIRC has no such issue. Why your case is different? >> > >> >> From what I can tell the ipu-bridge driver only references software >> nodes (as struct software_node) from other software nodes. I need to >> reference ANY implementation of firmware node from a software node. > > Yes, the IPU bridge only references software nodes. > > I might use two distinct pointers instead of an union and an integer field > that tells which type is the right one. I don't expect more such cases > here; it's either a software node or an fwnode handle (ACPI or OF node). > Like: struct software_node_ref_args { const struct software_node *swnode; struct fwnode_handle *fwnode; unsigned int nargs; u64 args[NR_FWNODE_REFERENCE_ARGS]; }; And then if swnode is NULL then assume fwnode must not be? I'm not sure if it's necessarily better but I don't have a strong opinion on this either. Bartosz
On Tue, Oct 21, 2025 at 02:06:36AM -0700, Bartosz Golaszewski wrote: > On Tue, 21 Oct 2025 08:54:22 +0200, Sakari Ailus > <sakari.ailus@linux.intel.com> said: > > On Mon, Oct 20, 2025 at 01:26:59PM +0200, Bartosz Golaszewski wrote: > >> On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: ... > >> > > > Can't we always have an fwnode reference? > >> > > > >> > > Unfortunately no. A const struct software_node is not yet a full > >> > > fwnode, it's just a template that becomes an actual firmware node when > >> > > it's registered with the swnode framework. However in order to allow > >> > > creating a graph of software nodes before we register them, we need a > >> > > way to reference those templates and then look them up internally in > >> > > swnode code. > >> > > >> > Strange that you need this way. The IPU3 bridge driver (that creates a graph of > >> > fwnodes at run-time for being consumed by the respective parts of v4l2 > >> > framework) IIRC has no such issue. Why your case is different? > >> > >> From what I can tell the ipu-bridge driver only references software > >> nodes (as struct software_node) from other software nodes. I need to > >> reference ANY implementation of firmware node from a software node. > > > > Yes, the IPU bridge only references software nodes. > > > > I might use two distinct pointers instead of an union and an integer field > > that tells which type is the right one. I don't expect more such cases > > here; it's either a software node or an fwnode handle (ACPI or OF node). > > Like: > > struct software_node_ref_args { > const struct software_node *swnode; > struct fwnode_handle *fwnode; > unsigned int nargs; > u64 args[NR_FWNODE_REFERENCE_ARGS]; > }; > > And then if swnode is NULL then assume fwnode must not be? > > I'm not sure if it's necessarily better but I don't have a strong opinion on > this either. At least it is slightly closer to what I ideally want to have (but not in this design seems), so +1 to Sakari's proposal.
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index a60ba4327db8b967034b296b73c948aa5746a094..b7e91c60c1d51b167adc88afb0f06feeeccf900c 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -535,9 +535,19 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode, ref_array = prop->pointer; ref = &ref_array[index]; - refnode = software_node_fwnode(ref->node); - if (!refnode) - return -ENOENT; + switch (ref->type) { + case SOFTWARE_NODE_REF_SWNODE: + refnode = software_node_fwnode(ref->swnode); + if (!refnode) + return -ENOENT; + break; + case SOFTWARE_NODE_REF_FWNODE: + refnode = ref->fwnode; + break; + default: + WARN_ON(1); + return -EINVAL; + } if (nargs_prop) { error = fwnode_property_read_u32_array(refnode, nargs_prop, @@ -634,7 +644,10 @@ software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode) ref = prop->pointer; - return software_node_get(software_node_fwnode(ref[0].node)); + if (ref->type != SOFTWARE_NODE_REF_SWNODE) + return NULL; + + return software_node_get(software_node_fwnode(ref[0].swnode)); } static struct fwnode_handle * diff --git a/include/linux/property.h b/include/linux/property.h index 50b26589dd70d1756f3b8644255c24a011e2617c..af72f579e8026cee2456b0983819e7a4bf0c6805 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -353,25 +353,50 @@ fwnode_property_string_array_count(const struct fwnode_handle *fwnode, struct software_node; +enum software_node_ref_type { + /* References a software node. */ + SOFTWARE_NODE_REF_SWNODE = 0, + /* References a firmware node. */ + SOFTWARE_NODE_REF_FWNODE, +}; + /** * struct software_node_ref_args - Reference property with additional arguments - * @node: Reference to a software node + * @swnode: Reference to a software node + * @fwnode: Alternative reference to a firmware node handle * @nargs: Number of elements in @args array * @args: Integer arguments */ struct software_node_ref_args { - const struct software_node *node; + enum software_node_ref_type type; + union { + const struct software_node *swnode; + struct fwnode_handle *fwnode; + }; unsigned int nargs; u64 args[NR_FWNODE_REFERENCE_ARGS]; }; -#define SOFTWARE_NODE_REFERENCE(_ref_, ...) \ +#define __SOFTWARE_NODE_REF(_ref, _type, _node, ...) \ (const struct software_node_ref_args) { \ - .node = _ref_, \ + .type = _type, \ + ._node = _ref, \ .nargs = COUNT_ARGS(__VA_ARGS__), \ .args = { __VA_ARGS__ }, \ } +#define SOFTWARE_NODE_REF_SWNODE(_ref, ...) \ + __SOFTWARE_NODE_REF(_ref, SOFTWARE_NODE_REF_SWNODE, \ + swnode, __VA_ARGS__) + +#define SOFTWARE_NODE_REF_FWNODE(_ref, ...) \ + __SOFTWARE_NODE_REF(_ref, SOFTWARE_NODE_REF_FWNODE, \ + fwnode, __VA_ARGS__) + +/* DEPRECATED, use SOFTWARE_NODE_REF_SWNODE() instead. */ +#define SOFTWARE_NODE_REFERENCE(_ref, ...) \ + SOFTWARE_NODE_REF_SWNODE(_ref, __VA_ARGS__) + /** * struct property_entry - "Built-in" device property representation. * @name: Name of the property. @@ -463,14 +488,26 @@ struct property_entry { #define PROPERTY_ENTRY_STRING(_name_, _val_) \ __PROPERTY_ENTRY_ELEMENT(_name_, str, STRING, _val_) -#define PROPERTY_ENTRY_REF(_name_, _ref_, ...) \ +#define __PROPERTY_ENTRY_REF(_type, _name, _ref, ...) \ (struct property_entry) { \ - .name = _name_, \ + .name = _name, \ .length = sizeof(struct software_node_ref_args), \ .type = DEV_PROP_REF, \ - { .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), }, \ + { .pointer = &_type(_ref, ##__VA_ARGS__), }, \ } +#define PROPERTY_ENTRY_REF_SWNODE(_name, _ref, ...) \ + __PROPERTY_ENTRY_REF(SOFTWARE_NODE_REF_SWNODE, \ + _name, _ref, __VA_ARGS__) + +#define PROPERTY_ENTRY_REF_FWNODE(_name, _ref, ...) \ + __PROPERTY_ENTRY_REF(SOFTWARE_NODE_REF_FWNODE, \ + _name, _ref, __VA_ARGS__) + +/* DEPRECATED, use PROPERTY_ENTRY_REF_SWNODE() instead. */ +#define PROPERTY_ENTRY_REF(_name, _ref, ...) \ + PROPERTY_ENTRY_REF_SWNODE(_name, _ref, __VA_ARGS__) + #define PROPERTY_ENTRY_BOOL(_name_) \ (struct property_entry) { \ .name = _name_, \