Re: [PATCH v2] reset: add support for non-DT systems
2018-02-17 3:01 GMT+01:00 David Lechner : > On 02/13/2018 12:39 PM, Bartosz Golaszewski wrote: >> >> From: Bartosz Golaszewski >> >> The reset framework only supports device-tree. There are some platforms >> however, which need to use it even in legacy, board-file based mode. >> >> An example of such architecture is the DaVinci family of SoCs which >> supports both device tree and legacy boot modes and we don't want to >> introduce any regressions. >> >> We're currently working on converting the platform from its hand-crafted >> clock API to using the common clock framework. Part of the overhaul will >> be representing the chip's power sleep controller's reset lines using >> the reset framework. >> >> This changeset extends the core reset code with a new field in the >> reset controller struct which contains an array of lookup entries. Each >> entry contains the device name and an additional, optional identifier >> string. >> >> Drivers can register a set of reset lines using this lookup table and >> concerned devices can access them using the regular reset_control API. >> >> This new function is only called as a fallback in case the of_node >> field is NULL and doesn't change anything for current users. >> >> Tested with a dummy reset driver with several lookup entries. >> >> An example lookup table can look like this: >> >> static const struct reset_lookup foobar_reset_lookup[] = { >> [FOO_RESET] = { .dev = "foo", .id = "foo_id" }, >> [BAR_RESET] = { .dev = "bar", .id = NULL }, >> { } >> }; >> >> where FOO_RESET and BAR_RESET will correspond with the id parameters >> of reset callbacks. >> >> Cc: Sekhar Nori >> Cc: Kevin Hilman >> Cc: David Lechner >> Signed-off-by: Bartosz Golaszewski >> --- >> v1 -> v2: >> - renamed the new function to __reset_control_get_from_lookup() >> - added a missing break; when a matching entry is found >> - rearranged the code in __reset_control_get() - we can no longer get to >> the >>return at the bottom, so remove it and return from >>__reset_control_get_from_lookup() if __of_reset_control_get() fails >> - return -ENOENT from reset_contol_get() if we can't find a matching >> entry, >>prevously returned -EINVAL referred to the fact that we passed a device >>without the of_node which is no longer an error condition >> - add a comment about needing a sentinel in the lookup table >> >> drivers/reset/core.c | 40 >> +++- >> include/linux/reset-controller.h | 14 ++ >> 2 files changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index da4292e9de97..b104a0c5c511 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c >> @@ -493,6 +493,44 @@ struct reset_control *__of_reset_control_get(struct >> device_node *node, >> } >> EXPORT_SYMBOL_GPL(__of_reset_control_get); >> +static struct reset_control * >> +__reset_control_get_from_lookup(struct device *dev, const char *id, >> + bool shared, bool optional) >> +{ >> + struct reset_controller_dev *rcdev; >> + const char *dev_id = dev_name(dev); >> + struct reset_control *rstc = NULL; >> + const struct reset_lookup *lookup; >> + int index; >> + >> + mutex_lock(&reset_list_mutex); >> + >> + list_for_each_entry(rcdev, &reset_controller_list, list) { >> + if (!rcdev->lookup) >> + continue; >> + >> + lookup = rcdev->lookup; >> + for (index = 0; lookup->dev; index++, lookup++) {> + >> if (strcmp(dev_id, lookup->dev)) >> + continue; >> + >> + if ((!id && !lookup->id) || >> + (id && lookup->id && !strcmp(id, lookup->id))) >> { >> + rstc = __reset_control_get_internal(rcdev, >> + index, >> shared); >> + break; >> + } >> + } >> + } > > > > This method of determining the index is not very useful. In the case of the > DSP > reset on OMAP-L138, the index *must* be the LPSC module domain number, which > is > 15. This would require us to create 15 dummy entries in the rcdev->lookup > array > so that we get the correct index in order to get the correct reset control. > > I think it would be better to just store the index in struct reset_lookup. > > Another option would be to require the length of lookup to be > rcdev->nr_resets > instead of using a sentinel. Indeed. Please take a look at v3. Thanks, Bartosz
Re: [PATCH v2] reset: add support for non-DT systems
On 02/13/2018 12:39 PM, Bartosz Golaszewski wrote: From: Bartosz Golaszewski The reset framework only supports device-tree. There are some platforms however, which need to use it even in legacy, board-file based mode. An example of such architecture is the DaVinci family of SoCs which supports both device tree and legacy boot modes and we don't want to introduce any regressions. We're currently working on converting the platform from its hand-crafted clock API to using the common clock framework. Part of the overhaul will be representing the chip's power sleep controller's reset lines using the reset framework. This changeset extends the core reset code with a new field in the reset controller struct which contains an array of lookup entries. Each entry contains the device name and an additional, optional identifier string. Drivers can register a set of reset lines using this lookup table and concerned devices can access them using the regular reset_control API. This new function is only called as a fallback in case the of_node field is NULL and doesn't change anything for current users. Tested with a dummy reset driver with several lookup entries. An example lookup table can look like this: static const struct reset_lookup foobar_reset_lookup[] = { [FOO_RESET] = { .dev = "foo", .id = "foo_id" }, [BAR_RESET] = { .dev = "bar", .id = NULL }, { } }; where FOO_RESET and BAR_RESET will correspond with the id parameters of reset callbacks. Cc: Sekhar Nori Cc: Kevin Hilman Cc: David Lechner Signed-off-by: Bartosz Golaszewski --- v1 -> v2: - renamed the new function to __reset_control_get_from_lookup() - added a missing break; when a matching entry is found - rearranged the code in __reset_control_get() - we can no longer get to the return at the bottom, so remove it and return from __reset_control_get_from_lookup() if __of_reset_control_get() fails - return -ENOENT from reset_contol_get() if we can't find a matching entry, prevously returned -EINVAL referred to the fact that we passed a device without the of_node which is no longer an error condition - add a comment about needing a sentinel in the lookup table drivers/reset/core.c | 40 +++- include/linux/reset-controller.h | 14 ++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/drivers/reset/core.c b/drivers/reset/core.c index da4292e9de97..b104a0c5c511 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -493,6 +493,44 @@ struct reset_control *__of_reset_control_get(struct device_node *node, } EXPORT_SYMBOL_GPL(__of_reset_control_get); +static struct reset_control * +__reset_control_get_from_lookup(struct device *dev, const char *id, + bool shared, bool optional) +{ + struct reset_controller_dev *rcdev; + const char *dev_id = dev_name(dev); + struct reset_control *rstc = NULL; + const struct reset_lookup *lookup; + int index; + + mutex_lock(&reset_list_mutex); + + list_for_each_entry(rcdev, &reset_controller_list, list) { + if (!rcdev->lookup) + continue; + + lookup = rcdev->lookup; + for (index = 0; lookup->dev; index++, lookup++) {> + if (strcmp(dev_id, lookup->dev)) + continue; + + if ((!id && !lookup->id) || + (id && lookup->id && !strcmp(id, lookup->id))) { + rstc = __reset_control_get_internal(rcdev, + index, shared); + break; + } + } + } This method of determining the index is not very useful. In the case of the DSP reset on OMAP-L138, the index *must* be the LPSC module domain number, which is 15. This would require us to create 15 dummy entries in the rcdev->lookup array so that we get the correct index in order to get the correct reset control. I think it would be better to just store the index in struct reset_lookup. Another option would be to require the length of lookup to be rcdev->nr_resets instead of using a sentinel.
Re: [PATCH v2] reset: add support for non-DT systems
On Wednesday 14 February 2018 12:09 AM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > The reset framework only supports device-tree. There are some platforms > however, which need to use it even in legacy, board-file based mode. > > An example of such architecture is the DaVinci family of SoCs which > supports both device tree and legacy boot modes and we don't want to > introduce any regressions. > > We're currently working on converting the platform from its hand-crafted > clock API to using the common clock framework. Part of the overhaul will > be representing the chip's power sleep controller's reset lines using > the reset framework. > > This changeset extends the core reset code with a new field in the > reset controller struct which contains an array of lookup entries. Each > entry contains the device name and an additional, optional identifier > string. > > Drivers can register a set of reset lines using this lookup table and > concerned devices can access them using the regular reset_control API. > > This new function is only called as a fallback in case the of_node > field is NULL and doesn't change anything for current users. > > Tested with a dummy reset driver with several lookup entries. > > An example lookup table can look like this: > > static const struct reset_lookup foobar_reset_lookup[] = { > [FOO_RESET] = { .dev = "foo", .id = "foo_id" }, > [BAR_RESET] = { .dev = "bar", .id = NULL }, > { } > }; > > where FOO_RESET and BAR_RESET will correspond with the id parameters > of reset callbacks. > > Cc: Sekhar Nori > Cc: Kevin Hilman > Cc: David Lechner > Signed-off-by: Bartosz Golaszewski This should really help get rid of the private davinci_clk_reset_assert() API. Thanks for getting this done. I noticed that the array based API still remains DT only. Its not a concern for DaVinci so it can probably be supported when its really needed. Reviewed-by: Sekhar Nori Thanks, Sekhar
Re: [PATCH v2] reset: add support for non-DT systems
On Wed, Feb 14, 2018 at 10:59 AM, Bartosz Golaszewski wrote: > 2018-02-13 20:17 GMT+01:00 Andy Shevchenko : >> On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski wrote: >>> + for (index = 0; lookup->dev; index++, lookup++) { >>> + if (strcmp(dev_id, lookup->dev)) >>> + continue; >>> + >>> + if ((!id && !lookup->id) || >>> + (id && lookup->id && !strcmp(id, lookup->id))) { >>> + rstc = __reset_control_get_internal(rcdev, >>> + index, >>> shared); >>> + break; >>> + } >> >> Wouldn't be slightly more readable >> >> if (id && lookup->id) { >> if (strcmp(id, lookup->id)) >> continue; >> } else if (id || lookup->id) { >> continue; >> } >> >> rstc = __reset_control_get_internal(rcdev, index, shared); >> break; >> > > You'd get less indentations, yes but I wanted to emphasize the > condition on which we want to stop in this line. It doesn't matter (indentation) here, esp. taking into consideration that you already have another condition by which you continue the loop. My proposal adds consistency and from my point of view makes easier to parse. You check all false conditions in a row, end up with a true one. >>> + if (!rstc) >>> + return optional ? NULL : ERR_PTR(-ENOENT); >> >> Isn't simpler >> >> if (!optional && !rstc) // or other way around, depending which might >> be more offten >> return ERR_PTR(...); >> > > IMO it's just a matter of taste. I think in my proposal it's more straight forward. Easier to parse by reader. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] reset: add support for non-DT systems
2018-02-13 20:17 GMT+01:00 Andy Shevchenko : > On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski wrote: >> From: Bartosz Golaszewski >> >> The reset framework only supports device-tree. There are some platforms >> however, which need to use it even in legacy, board-file based mode. >> >> An example of such architecture is the DaVinci family of SoCs which >> supports both device tree and legacy boot modes and we don't want to >> introduce any regressions. >> >> We're currently working on converting the platform from its hand-crafted >> clock API to using the common clock framework. Part of the overhaul will >> be representing the chip's power sleep controller's reset lines using >> the reset framework. >> >> This changeset extends the core reset code with a new field in the >> reset controller struct which contains an array of lookup entries. Each >> entry contains the device name and an additional, optional identifier >> string. >> >> Drivers can register a set of reset lines using this lookup table and >> concerned devices can access them using the regular reset_control API. >> >> This new function is only called as a fallback in case the of_node >> field is NULL and doesn't change anything for current users. >> >> Tested with a dummy reset driver with several lookup entries. >> >> An example lookup table can look like this: >> >> static const struct reset_lookup foobar_reset_lookup[] = { >> [FOO_RESET] = { .dev = "foo", .id = "foo_id" }, >> [BAR_RESET] = { .dev = "bar", .id = NULL }, >> { } >> }; >> >> where FOO_RESET and BAR_RESET will correspond with the id parameters >> of reset callbacks. > >> +static struct reset_control * >> +__reset_control_get_from_lookup(struct device *dev, const char *id, >> + bool shared, bool optional) >> +{ >> + struct reset_controller_dev *rcdev; >> + const char *dev_id = dev_name(dev); >> + struct reset_control *rstc = NULL; >> + const struct reset_lookup *lookup; >> + int index; >> + >> + mutex_lock(&reset_list_mutex); >> + >> + list_for_each_entry(rcdev, &reset_controller_list, list) { >> + if (!rcdev->lookup) >> + continue; >> + >> + lookup = rcdev->lookup; > >> + for (index = 0; lookup->dev; index++, lookup++) { >> + if (strcmp(dev_id, lookup->dev)) >> + continue; >> + >> + if ((!id && !lookup->id) || >> + (id && lookup->id && !strcmp(id, lookup->id))) { >> + rstc = __reset_control_get_internal(rcdev, >> + index, >> shared); >> + break; >> + } > > Wouldn't be slightly more readable > > if (id && lookup->id) { > if (strcmp(id, lookup->id)) > continue; > } else if (id || lookup->id) { > continue; > } > > rstc = __reset_control_get_internal(rcdev, index, shared); > break; > You'd get less indentations, yes but I wanted to emphasize the condition on which we want to stop in this line. >> + } >> + } >> + >> + mutex_unlock(&reset_list_mutex); >> + > >> + if (!rstc) >> + return optional ? NULL : ERR_PTR(-ENOENT); > > Isn't simpler > > if (!optional && !rstc) // or other way around, depending which might > be more offten > return ERR_PTR(...); > IMO it's just a matter of taste. Thanks, Bartosz
Re: [PATCH v2] reset: add support for non-DT systems
On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > The reset framework only supports device-tree. There are some platforms > however, which need to use it even in legacy, board-file based mode. > > An example of such architecture is the DaVinci family of SoCs which > supports both device tree and legacy boot modes and we don't want to > introduce any regressions. > > We're currently working on converting the platform from its hand-crafted > clock API to using the common clock framework. Part of the overhaul will > be representing the chip's power sleep controller's reset lines using > the reset framework. > > This changeset extends the core reset code with a new field in the > reset controller struct which contains an array of lookup entries. Each > entry contains the device name and an additional, optional identifier > string. > > Drivers can register a set of reset lines using this lookup table and > concerned devices can access them using the regular reset_control API. > > This new function is only called as a fallback in case the of_node > field is NULL and doesn't change anything for current users. > > Tested with a dummy reset driver with several lookup entries. > > An example lookup table can look like this: > > static const struct reset_lookup foobar_reset_lookup[] = { > [FOO_RESET] = { .dev = "foo", .id = "foo_id" }, > [BAR_RESET] = { .dev = "bar", .id = NULL }, > { } > }; > > where FOO_RESET and BAR_RESET will correspond with the id parameters > of reset callbacks. > +static struct reset_control * > +__reset_control_get_from_lookup(struct device *dev, const char *id, > + bool shared, bool optional) > +{ > + struct reset_controller_dev *rcdev; > + const char *dev_id = dev_name(dev); > + struct reset_control *rstc = NULL; > + const struct reset_lookup *lookup; > + int index; > + > + mutex_lock(&reset_list_mutex); > + > + list_for_each_entry(rcdev, &reset_controller_list, list) { > + if (!rcdev->lookup) > + continue; > + > + lookup = rcdev->lookup; > + for (index = 0; lookup->dev; index++, lookup++) { > + if (strcmp(dev_id, lookup->dev)) > + continue; > + > + if ((!id && !lookup->id) || > + (id && lookup->id && !strcmp(id, lookup->id))) { > + rstc = __reset_control_get_internal(rcdev, > + index, > shared); > + break; > + } Wouldn't be slightly more readable if (id && lookup->id) { if (strcmp(id, lookup->id)) continue; } else if (id || lookup->id) { continue; } rstc = __reset_control_get_internal(rcdev, index, shared); break; > + } > + } > + > + mutex_unlock(&reset_list_mutex); > + > + if (!rstc) > + return optional ? NULL : ERR_PTR(-ENOENT); Isn't simpler if (!optional && !rstc) // or other way around, depending which might be more offten return ERR_PTR(...); > + return rstc; > +} -- With Best Regards, Andy Shevchenko
[PATCH v2] reset: add support for non-DT systems
From: Bartosz Golaszewski The reset framework only supports device-tree. There are some platforms however, which need to use it even in legacy, board-file based mode. An example of such architecture is the DaVinci family of SoCs which supports both device tree and legacy boot modes and we don't want to introduce any regressions. We're currently working on converting the platform from its hand-crafted clock API to using the common clock framework. Part of the overhaul will be representing the chip's power sleep controller's reset lines using the reset framework. This changeset extends the core reset code with a new field in the reset controller struct which contains an array of lookup entries. Each entry contains the device name and an additional, optional identifier string. Drivers can register a set of reset lines using this lookup table and concerned devices can access them using the regular reset_control API. This new function is only called as a fallback in case the of_node field is NULL and doesn't change anything for current users. Tested with a dummy reset driver with several lookup entries. An example lookup table can look like this: static const struct reset_lookup foobar_reset_lookup[] = { [FOO_RESET] = { .dev = "foo", .id = "foo_id" }, [BAR_RESET] = { .dev = "bar", .id = NULL }, { } }; where FOO_RESET and BAR_RESET will correspond with the id parameters of reset callbacks. Cc: Sekhar Nori Cc: Kevin Hilman Cc: David Lechner Signed-off-by: Bartosz Golaszewski --- v1 -> v2: - renamed the new function to __reset_control_get_from_lookup() - added a missing break; when a matching entry is found - rearranged the code in __reset_control_get() - we can no longer get to the return at the bottom, so remove it and return from __reset_control_get_from_lookup() if __of_reset_control_get() fails - return -ENOENT from reset_contol_get() if we can't find a matching entry, prevously returned -EINVAL referred to the fact that we passed a device without the of_node which is no longer an error condition - add a comment about needing a sentinel in the lookup table drivers/reset/core.c | 40 +++- include/linux/reset-controller.h | 14 ++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/drivers/reset/core.c b/drivers/reset/core.c index da4292e9de97..b104a0c5c511 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -493,6 +493,44 @@ struct reset_control *__of_reset_control_get(struct device_node *node, } EXPORT_SYMBOL_GPL(__of_reset_control_get); +static struct reset_control * +__reset_control_get_from_lookup(struct device *dev, const char *id, + bool shared, bool optional) +{ + struct reset_controller_dev *rcdev; + const char *dev_id = dev_name(dev); + struct reset_control *rstc = NULL; + const struct reset_lookup *lookup; + int index; + + mutex_lock(&reset_list_mutex); + + list_for_each_entry(rcdev, &reset_controller_list, list) { + if (!rcdev->lookup) + continue; + + lookup = rcdev->lookup; + for (index = 0; lookup->dev; index++, lookup++) { + if (strcmp(dev_id, lookup->dev)) + continue; + + if ((!id && !lookup->id) || + (id && lookup->id && !strcmp(id, lookup->id))) { + rstc = __reset_control_get_internal(rcdev, + index, shared); + break; + } + } + } + + mutex_unlock(&reset_list_mutex); + + if (!rstc) + return optional ? NULL : ERR_PTR(-ENOENT); + + return rstc; +} + struct reset_control *__reset_control_get(struct device *dev, const char *id, int index, bool shared, bool optional) { @@ -500,7 +538,7 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id, return __of_reset_control_get(dev->of_node, id, index, shared, optional); - return optional ? NULL : ERR_PTR(-EINVAL); + return __reset_control_get_from_lookup(dev, id, shared, optional); } EXPORT_SYMBOL_GPL(__reset_control_get); diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h index adb88f8cefbc..4cc52b4a4d27 100644 --- a/include/linux/reset-controller.h +++ b/include/linux/reset-controller.h @@ -22,6 +22,17 @@ struct reset_control_ops { int (*status)(struct reset_controller_dev *rcdev, unsigned long id); }; +/** + * struct reset_lookup - a single entry in a reset lookup table + * + * @dev: name of the device associated with this reset + * @id: additional reset identifier (if the device uses multiple reset lines)