| Message ID | 1453918516-3078-4-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/27/2016 11:15 AM, Hans de Goede wrote: > In some SoCs some hw-blocks share a reset control. Add support for this > setup by adding new: > > reset_control_get_shared() > devm_reset_control_get_shared() > devm_reset_control_get_shared_by_index() > > methods to get a reset_control. Note that this patch omits adding of_ > variants, if these are needed later they can be easily added. > > This patch also changes the behavior of the existing exclusive > reset_control_get() variants, if these are now called more then once > for the same reset_control they will return -EBUSY. To catch existing > drivers triggering this error (there should not be any) a WARN_ON(1) > is added in this path. > > When a reset_control is shared, the behavior of reset_control_assert / > deassert is changed, for shared reset_controls these will work like the > clock-enable/disable and regulator-on/off functions. They will keep a > deassert_count, and only (re-)assert the reset after reset_control_assert > has been called as many times as reset_control_deassert was called. > > Calling reset_control_assert without first calling reset_control_deassert > is not allowed on a shared reset control. Calling reset_control_reset is > also not allowed on a shared reset control. Hmmm. Do you have some examples of how drivers are supposed to co-ordinate use of the shared reset? Reference counting implies all the drivers need to get together and all assert and de-assert the reset in unison when some event happens. That seems difficult to co-ordinate. Instead, would it be better to require some central device to exclusively get the reset, and manage it. That device could then react to appropriate events to manage the reset. In this scheme, we wouldn't need the concept of shared resets at all. However, depending on why/when the reset needed to be re-asserted at run-time, this scheme wouldn't eliminate the co-ordination issue, but equally I don't believe it makes it any worse.
Hi, On 01/28/2016 09:20 PM, Stephen Warren wrote: > > On 01/27/2016 11:15 AM, Hans de Goede wrote: >> In some SoCs some hw-blocks share a reset control. Add support for this >> setup by adding new: >> >> reset_control_get_shared() >> devm_reset_control_get_shared() >> devm_reset_control_get_shared_by_index() >> >> methods to get a reset_control. Note that this patch omits adding of_ >> variants, if these are needed later they can be easily added. >> >> This patch also changes the behavior of the existing exclusive >> reset_control_get() variants, if these are now called more then once >> for the same reset_control they will return -EBUSY. To catch existing >> drivers triggering this error (there should not be any) a WARN_ON(1) >> is added in this path. >> >> When a reset_control is shared, the behavior of reset_control_assert / >> deassert is changed, for shared reset_controls these will work like the >> clock-enable/disable and regulator-on/off functions. They will keep a >> deassert_count, and only (re-)assert the reset after reset_control_assert >> has been called as many times as reset_control_deassert was called. >> >> Calling reset_control_assert without first calling reset_control_deassert >> is not allowed on a shared reset control. Calling reset_control_reset is >> also not allowed on a shared reset control. > > Hmmm. Do you have some examples of how drivers are supposed to co-ordinate use of the shared reset? Reference counting implies all the drivers need to get together and all assert and de-assert the reset in unison when some event happens. That seems difficult to co-ordinate. Right, for now this is only intended for drivers which need to de-assert the reset and probe time and (re-)assert it when unbound (and maybe for suspend / resume, in which case the driver should be able to handle the reset never heaving been asserted on resume). This is also why reset_control_reset is not supported for shared resets. I hope that we will never see a case where a reset is actually needed for error handling (so used outside of probe / suspend cases) and it is shared. > Instead, would it be better to require some central device to exclusively get the reset, and manage it. That device could then react to appropriate events to manage the reset. In this scheme, we wouldn't need the concept of shared resets at all. However, depending on why/when the reset needed to be re-asserted at run-time, this scheme wouldn't eliminate the co-ordination issue, but equally I don't believe it makes it any worse. If we ever need this kind of complexity, then yes a central service, with callbacks in to drivers requesting them to prepare for handling reset (like how usb does this) might be the best solution, but lets cross that bridge when we get there. Regards, Hans
On 01/28/2016 11:18 PM, Hans de Goede wrote: > Hi, > > On 01/28/2016 09:20 PM, Stephen Warren wrote: >> >> On 01/27/2016 11:15 AM, Hans de Goede wrote: >>> In some SoCs some hw-blocks share a reset control. Add support for this >>> setup by adding new: >>> >>> reset_control_get_shared() >>> devm_reset_control_get_shared() >>> devm_reset_control_get_shared_by_index() >>> >>> methods to get a reset_control. Note that this patch omits adding of_ >>> variants, if these are needed later they can be easily added. >>> >>> This patch also changes the behavior of the existing exclusive >>> reset_control_get() variants, if these are now called more then once >>> for the same reset_control they will return -EBUSY. To catch existing >>> drivers triggering this error (there should not be any) a WARN_ON(1) >>> is added in this path. >>> >>> When a reset_control is shared, the behavior of reset_control_assert / >>> deassert is changed, for shared reset_controls these will work like the >>> clock-enable/disable and regulator-on/off functions. They will keep a >>> deassert_count, and only (re-)assert the reset after >>> reset_control_assert >>> has been called as many times as reset_control_deassert was called. >>> >>> Calling reset_control_assert without first calling >>> reset_control_deassert >>> is not allowed on a shared reset control. Calling reset_control_reset is >>> also not allowed on a shared reset control. >> >> Hmmm. Do you have some examples of how drivers are supposed to >> co-ordinate use of the shared reset? Reference counting implies all >> the drivers need to get together and all assert and de-assert the >> reset in unison when some event happens. That seems difficult to >> co-ordinate. > > Right, for now this is only intended for drivers which need to de-assert > the > reset and probe time and (re-)assert it when unbound (and maybe for > suspend / resume, in which case the driver should be able to handle the > reset > never heaving been asserted on resume). This is also why > reset_control_reset > is not supported for shared resets. > > I hope that we will never see a case where a reset is actually needed > for error handling (so used outside of probe / suspend cases) and it is > shared. OK, that sounds reasonable. I guess the assumption is that the reset starts out asserted, and the first de-assert clears the reset signal. What if the default HW state has the reset de-asserted; is there any way for a driver to guarantee that the first deassert call is actually a deassert rather than a no-op? That would be useful so that drivers could assume their module's registers were reset due to the deassert call. >> Instead, would it be better to require some central device to >> exclusively get the reset, and manage it. That device could then react >> to appropriate events to manage the reset. In this scheme, we wouldn't >> need the concept of shared resets at all. However, depending on >> why/when the reset needed to be re-asserted at run-time, this scheme >> wouldn't eliminate the co-ordination issue, but equally I don't >> believe it makes it any worse. > > If we ever need this kind of complexity, then yes a central service, with > callbacks in to drivers requesting them to prepare for handling > reset (like how usb does this) might be the best solution, but lets cross > that bridge when we get there. OK. Perhaps it's a good idea to spell out this usage model assumption explicitly somewhere? Perhaps it already is and I simply didn't look in enough detail.
Hi Hans, On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote: > In some SoCs some hw-blocks share a reset control. Add support for this > setup by adding new: > > reset_control_get_shared() > devm_reset_control_get_shared() > devm_reset_control_get_shared_by_index() > > methods to get a reset_control. Note that this patch omits adding of_ > variants, if these are needed later they can be easily added. > > This patch also changes the behavior of the existing exclusive > reset_control_get() variants, if these are now called more then once > for the same reset_control they will return -EBUSY. To catch existing > drivers triggering this error (there should not be any) a WARN_ON(1) > is added in this path. Which is a good thing. > When a reset_control is shared, the behavior of reset_control_assert / > deassert is changed, for shared reset_controls these will work like the > clock-enable/disable and regulator-on/off functions. They will keep a > deassert_count, and only (re-)assert the reset after reset_control_assert > has been called as many times as reset_control_deassert was called. > > Calling reset_control_assert without first calling reset_control_deassert > is not allowed on a shared reset control. Calling reset_control_reset is > also not allowed on a shared reset control. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> All three patches look very nice. I'll give them a test-drive next week. So far I have one small issue, and I like Stephens suggestion of elaborating on how shared resets are to be used a bit more. > --- > drivers/reset/core.c | 61 ++++++++++++++++++++++++++++++++------- > include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++---------- > 2 files changed, 114 insertions(+), 26 deletions(-) > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > index e439ae2..5554585 100644 > --- a/drivers/reset/core.c > +++ b/drivers/reset/core.c > @@ -8,6 +8,7 @@ > * the Free Software Foundation; either version 2 of the License, or > * (at your option) any later version. > */ > +#include <linux/atomic.h> > #include <linux/device.h> > #include <linux/err.h> > #include <linux/export.h> > @@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list); > * @id: ID of the reset controller in the reset > * controller device > * @refcnt: Number of gets of this reset_control > + * @shared: Is this a shared (1), or an exclusive (0) reset_control? > + * @deassert_cnt: Number of times this reset line has been deasserted > */ > struct reset_control { > struct reset_controller_dev *rcdev; > struct list_head list; > unsigned int id; > unsigned int refcnt; > + int shared; Could we make this an enum reset_control_type type; enum reset_control_type { RESET_CONTROL_EXCLUSIVE, RESET_CONTROL_SHARED, }; instead? [...] > @@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status); > > static struct reset_control *__reset_control_get( > struct reset_controller_dev *rcdev, > - unsigned int index) > + unsigned int index, int shared) > { > struct reset_control *rstc; > > @@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get( > > list_for_each_entry(rstc, &rcdev->reset_control_head, list) { > if (rstc->id == index) { > + if (!rstc->shared || !shared) { Then this would have to be if ((rstc->type == RESET_CONTROL_EXCLUSIVE) || (type == RESET_CONTROL_EXCLUSIVE)) { ... [...] > @@ -78,7 +78,8 @@ static inline struct reset_control *__devm_reset_control_get( > #endif /* CONFIG_RESET_CONTROLLER */ > > /** > - * reset_control_get - Lookup and obtain a reference to a reset controller. > + * reset_control_get - Lookup and obtain an exclusive reference to a > + * reset controller. > * @dev: device to be reset by the controller > * @id: reset line name > * > @@ -92,17 +93,34 @@ static inline struct reset_control *__must_check reset_control_get( > #ifndef CONFIG_RESET_CONTROLLER > WARN_ON(1); > #endif > - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0); > + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); ... but these would't be as opaque: return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, RESET_CONTROL_EXCLUSIVE); [...] > /** > - * of_reset_control_get - Lookup and obtain a reference to a reset controller. > + * reset_control_get_shared - Lookup and obtain a shared reference to a > + * reset controller. > + * @dev: device to be reset by the controller > + * @id: reset line name > + * > + * Returns a struct reset_control or IS_ERR() condition containing errno. How about addressing Stephen's suggestion by extending this kerneldoc comment a bit? > + * Use of id names is optional. > + */ > +static inline struct reset_control *reset_control_get_shared( > + struct device *dev, const char *id) > +{ > + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1); return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, RESET_CONTROL_SHARED); [...] best regards Philipp
Hi, On 01/30/2016 12:38 PM, Philipp Zabel wrote: > Hi Hans, > > On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote: >> In some SoCs some hw-blocks share a reset control. Add support for this >> setup by adding new: >> >> reset_control_get_shared() >> devm_reset_control_get_shared() >> devm_reset_control_get_shared_by_index() >> >> methods to get a reset_control. Note that this patch omits adding of_ >> variants, if these are needed later they can be easily added. >> >> This patch also changes the behavior of the existing exclusive >> reset_control_get() variants, if these are now called more then once >> for the same reset_control they will return -EBUSY. To catch existing >> drivers triggering this error (there should not be any) a WARN_ON(1) >> is added in this path. > > Which is a good thing. > >> When a reset_control is shared, the behavior of reset_control_assert / >> deassert is changed, for shared reset_controls these will work like the >> clock-enable/disable and regulator-on/off functions. They will keep a >> deassert_count, and only (re-)assert the reset after reset_control_assert >> has been called as many times as reset_control_deassert was called. >> >> Calling reset_control_assert without first calling reset_control_deassert >> is not allowed on a shared reset control. Calling reset_control_reset is >> also not allowed on a shared reset control. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > All three patches look very nice. I'll give them a test-drive next week. > So far I have one small issue, and I like Stephens suggestion of > elaborating on how shared resets are to be used a bit more. I can do that, where would you like me to put this, a doxygen style comment in include/linux/reset.h ? Regards, Hans > >> --- >> drivers/reset/core.c | 61 ++++++++++++++++++++++++++++++++------- >> include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++---------- >> 2 files changed, 114 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index e439ae2..5554585 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c >> @@ -8,6 +8,7 @@ >> * the Free Software Foundation; either version 2 of the License, or >> * (at your option) any later version. >> */ >> +#include <linux/atomic.h> >> #include <linux/device.h> >> #include <linux/err.h> >> #include <linux/export.h> >> @@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list); >> * @id: ID of the reset controller in the reset >> * controller device >> * @refcnt: Number of gets of this reset_control >> + * @shared: Is this a shared (1), or an exclusive (0) reset_control? >> + * @deassert_cnt: Number of times this reset line has been deasserted >> */ >> struct reset_control { >> struct reset_controller_dev *rcdev; >> struct list_head list; >> unsigned int id; >> unsigned int refcnt; >> + int shared; > > Could we make this an > enum reset_control_type type; > > enum reset_control_type { > RESET_CONTROL_EXCLUSIVE, > RESET_CONTROL_SHARED, > }; > instead? > > [...] >> @@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status); >> >> static struct reset_control *__reset_control_get( >> struct reset_controller_dev *rcdev, >> - unsigned int index) >> + unsigned int index, int shared) >> { >> struct reset_control *rstc; >> >> @@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get( >> >> list_for_each_entry(rstc, &rcdev->reset_control_head, list) { >> if (rstc->id == index) { >> + if (!rstc->shared || !shared) { > > Then this would have to be > > if ((rstc->type == RESET_CONTROL_EXCLUSIVE) || > (type == RESET_CONTROL_EXCLUSIVE)) { > ... > > [...] >> @@ -78,7 +78,8 @@ static inline struct reset_control *__devm_reset_control_get( >> #endif /* CONFIG_RESET_CONTROLLER */ >> >> /** >> - * reset_control_get - Lookup and obtain a reference to a reset controller. >> + * reset_control_get - Lookup and obtain an exclusive reference to a >> + * reset controller. >> * @dev: device to be reset by the controller >> * @id: reset line name >> * >> @@ -92,17 +93,34 @@ static inline struct reset_control *__must_check reset_control_get( >> #ifndef CONFIG_RESET_CONTROLLER >> WARN_ON(1); >> #endif >> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0); >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); > > ... but these would't be as opaque: > > return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, > RESET_CONTROL_EXCLUSIVE); > > [...] >> /** >> - * of_reset_control_get - Lookup and obtain a reference to a reset controller. >> + * reset_control_get_shared - Lookup and obtain a shared reference to a >> + * reset controller. >> + * @dev: device to be reset by the controller >> + * @id: reset line name >> + * >> + * Returns a struct reset_control or IS_ERR() condition containing errno. > > How about addressing Stephen's suggestion by extending this kerneldoc comment a > bit? > >> + * Use of id names is optional. >> + */ >> +static inline struct reset_control *reset_control_get_shared( >> + struct device *dev, const char *id) >> +{ >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1); > > return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, > RESET_CONTROL_SHARED); > > [...] > > best regards > Philipp >
Hi Hans, Am Sonntag, den 31.01.2016, 10:12 +0100 schrieb Hans de Goede: > Hi, > > On 01/30/2016 12:38 PM, Philipp Zabel wrote: > > Hi Hans, > > > > On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote: > >> In some SoCs some hw-blocks share a reset control. Add support for this > >> setup by adding new: > >> > >> reset_control_get_shared() > >> devm_reset_control_get_shared() > >> devm_reset_control_get_shared_by_index() > >> > >> methods to get a reset_control. Note that this patch omits adding of_ > >> variants, if these are needed later they can be easily added. > >> > >> This patch also changes the behavior of the existing exclusive > >> reset_control_get() variants, if these are now called more then once > >> for the same reset_control they will return -EBUSY. To catch existing > >> drivers triggering this error (there should not be any) a WARN_ON(1) > >> is added in this path. > > > > Which is a good thing. > > > >> When a reset_control is shared, the behavior of reset_control_assert / > >> deassert is changed, for shared reset_controls these will work like the > >> clock-enable/disable and regulator-on/off functions. They will keep a > >> deassert_count, and only (re-)assert the reset after reset_control_assert > >> has been called as many times as reset_control_deassert was called. > >> > >> Calling reset_control_assert without first calling reset_control_deassert > >> is not allowed on a shared reset control. Calling reset_control_reset is > >> also not allowed on a shared reset control. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > All three patches look very nice. I'll give them a test-drive next week. > > So far I have one small issue, and I like Stephens suggestion of > > elaborating on how shared resets are to be used a bit more. > > I can do that, where would you like me to put this, a doxygen style comment > in include/linux/reset.h ? Sorry for the delay. I had just thought of extending the reset_control_get_shared comment below, but an overview comment in reset.h would be fine, too. [...] > >> /** > >> - * of_reset_control_get - Lookup and obtain a reference to a reset controller. > >> + * reset_control_get_shared - Lookup and obtain a shared reference to a > >> + * reset controller. > >> + * @dev: device to be reset by the controller > >> + * @id: reset line name > >> + * > >> + * Returns a struct reset_control or IS_ERR() condition containing errno. > > > > How about addressing Stephen's suggestion by extending this kerneldoc comment a > > bit? ^^^ this. Now that I think of it, the reset_control_assert function comment should explicitly state that for shared resets the drivers can't expect their registers and internal state to be reset, but must be prepared for this to happen. best regards Philipp
Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede: [...] > @@ -94,9 +99,16 @@ EXPORT_SYMBOL_GPL(reset_controller_unregister); > /** > * reset_control_reset - reset the controlled device > * @rstc: reset controller > + * > + * Calling this on a shared reset controller is an error. > */ > int reset_control_reset(struct reset_control *rstc) > { > + if (rstc->shared) { > + WARN_ON(1); > + return -EINVAL; Occurrences of if (condition) { WARN_ON(1); ... } could be changed to if (WARN_ON(condition)) { ... } regards Philipp
Hi, On 05-02-16 10:08, Philipp Zabel wrote: > Am Mittwoch, den 27.01.2016, 19:15 +0100 schrieb Hans de Goede: > [...] >> @@ -94,9 +99,16 @@ EXPORT_SYMBOL_GPL(reset_controller_unregister); >> /** >> * reset_control_reset - reset the controlled device >> * @rstc: reset controller >> + * >> + * Calling this on a shared reset controller is an error. >> */ >> int reset_control_reset(struct reset_control *rstc) >> { >> + if (rstc->shared) { >> + WARN_ON(1); >> + return -EINVAL; > > Occurrences of > if (condition) { > WARN_ON(1); > ... > } > could be changed to > if (WARN_ON(condition)) { > ... > } Good one, will fix for v4 (v2 of the second attempt at this). Regards, Hans
diff --git a/drivers/reset/core.c b/drivers/reset/core.c index e439ae2..5554585 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -8,6 +8,7 @@ * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. */ +#include <linux/atomic.h> #include <linux/device.h> #include <linux/err.h> #include <linux/export.h> @@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list); * @id: ID of the reset controller in the reset * controller device * @refcnt: Number of gets of this reset_control + * @shared: Is this a shared (1), or an exclusive (0) reset_control? + * @deassert_cnt: Number of times this reset line has been deasserted */ struct reset_control { struct reset_controller_dev *rcdev; struct list_head list; unsigned int id; unsigned int refcnt; + int shared; + atomic_t deassert_count; }; /** @@ -94,9 +99,16 @@ EXPORT_SYMBOL_GPL(reset_controller_unregister); /** * reset_control_reset - reset the controlled device * @rstc: reset controller + * + * Calling this on a shared reset controller is an error. */ int reset_control_reset(struct reset_control *rstc) { + if (rstc->shared) { + WARN_ON(1); + return -EINVAL; + } + if (rstc->rcdev->ops->reset) return rstc->rcdev->ops->reset(rstc->rcdev, rstc->id); @@ -107,26 +119,47 @@ EXPORT_SYMBOL_GPL(reset_control_reset); /** * reset_control_assert - asserts the reset line * @rstc: reset controller + * + * Calling this on an exclusive reset controller guarantees that the reset + * will be asserted. When called on a shared reset controller the line may + * still be deasserted, as long as other users keep it so. */ int reset_control_assert(struct reset_control *rstc) { - if (rstc->rcdev->ops->assert) - return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id); + if (!rstc->rcdev->ops->assert) + return -ENOTSUPP; - return -ENOTSUPP; + if (rstc->shared) { + if (atomic_read(&rstc->deassert_count) == 0) { + WARN_ON(1); + return -EINVAL; + } + + if (atomic_dec_return(&rstc->deassert_count) != 0) + return 0; + } + + return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id); } EXPORT_SYMBOL_GPL(reset_control_assert); /** * reset_control_deassert - deasserts the reset line * @rstc: reset controller + * + * After calling this function, the reset is guaranteed to be deasserted. */ int reset_control_deassert(struct reset_control *rstc) { - if (rstc->rcdev->ops->deassert) - return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id); + if (!rstc->rcdev->ops->deassert) + return -ENOTSUPP; - return -ENOTSUPP; + if (rstc->shared) { + if (atomic_inc_return(&rstc->deassert_count) != 1) + return 0; + } + + return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id); } EXPORT_SYMBOL_GPL(reset_control_deassert); @@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status); static struct reset_control *__reset_control_get( struct reset_controller_dev *rcdev, - unsigned int index) + unsigned int index, int shared) { struct reset_control *rstc; @@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get( list_for_each_entry(rstc, &rcdev->reset_control_head, list) { if (rstc->id == index) { + if (!rstc->shared || !shared) { + WARN_ON(1); + return ERR_PTR(-EBUSY); + } rstc->refcnt++; return rstc; } @@ -170,6 +207,7 @@ static struct reset_control *__reset_control_get( list_add(&rstc->list, &rcdev->reset_control_head); rstc->id = index; rstc->refcnt = 1; + rstc->shared = shared; return rstc; } @@ -188,7 +226,7 @@ static void __reset_control_put(struct reset_control *rstc) } struct reset_control *__of_reset_control_get(struct device_node *node, - const char *id, int index) + const char *id, int index, int shared) { struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER); struct reset_controller_dev *r, *rcdev; @@ -233,7 +271,7 @@ struct reset_control *__of_reset_control_get(struct device_node *node, } /* reset_list_mutex also protects the rcdev's reset_control list */ - rstc = __reset_control_get(rcdev, rstc_id); + rstc = __reset_control_get(rcdev, rstc_id, shared); mutex_unlock(&reset_list_mutex); @@ -263,7 +301,7 @@ static void devm_reset_control_release(struct device *dev, void *res) } struct reset_control *__devm_reset_control_get(struct device *dev, - const char *id, int index) + const char *id, int index, int shared) { struct reset_control **ptr, *rstc; @@ -272,7 +310,8 @@ struct reset_control *__devm_reset_control_get(struct device *dev, if (!ptr) return ERR_PTR(-ENOMEM); - rstc = __of_reset_control_get(dev ? dev->of_node : NULL, id, index); + rstc = __of_reset_control_get(dev ? dev->of_node : NULL, + id, index, shared); if (!IS_ERR(rstc)) { *ptr = rstc; devres_add(dev, ptr); diff --git a/include/linux/reset.h b/include/linux/reset.h index 1bb69a2..cb35e83 100644 --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -13,10 +13,10 @@ int reset_control_deassert(struct reset_control *rstc); int reset_control_status(struct reset_control *rstc); struct reset_control *__of_reset_control_get(struct device_node *node, - const char *id, int index); + const char *id, int index, int shared); void reset_control_put(struct reset_control *rstc); struct reset_control *__devm_reset_control_get(struct device *dev, - const char *id, int index); + const char *id, int index, int shared); int __must_check device_reset(struct device *dev); @@ -63,14 +63,14 @@ static inline int device_reset_optional(struct device *dev) static inline struct reset_control *__of_reset_control_get( struct device_node *node, - const char *id, int index) + const char *id, int index, int shared) { return ERR_PTR(-EINVAL); } static inline struct reset_control *__devm_reset_control_get( struct device *dev, - const char *id, int index) + const char *id, int index, int shared) { return ERR_PTR(-EINVAL); } @@ -78,7 +78,8 @@ static inline struct reset_control *__devm_reset_control_get( #endif /* CONFIG_RESET_CONTROLLER */ /** - * reset_control_get - Lookup and obtain a reference to a reset controller. + * reset_control_get - Lookup and obtain an exclusive reference to a + * reset controller. * @dev: device to be reset by the controller * @id: reset line name * @@ -92,17 +93,34 @@ static inline struct reset_control *__must_check reset_control_get( #ifndef CONFIG_RESET_CONTROLLER WARN_ON(1); #endif - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0); + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); } static inline struct reset_control *reset_control_get_optional( struct device *dev, const char *id) { - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0); + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); } /** - * of_reset_control_get - Lookup and obtain a reference to a reset controller. + * reset_control_get_shared - Lookup and obtain a shared reference to a + * reset controller. + * @dev: device to be reset by the controller + * @id: reset line name + * + * Returns a struct reset_control or IS_ERR() condition containing errno. + * + * Use of id names is optional. + */ +static inline struct reset_control *reset_control_get_shared( + struct device *dev, const char *id) +{ + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1); +} + +/** + * of_reset_control_get - Lookup and obtain an exclusive reference to a + * reset controller. * @node: device to be reset by the controller * @id: reset line name * @@ -113,12 +131,12 @@ static inline struct reset_control *reset_control_get_optional( static inline struct reset_control *of_reset_control_get( struct device_node *node, const char *id) { - return __of_reset_control_get(node, id, 0); + return __of_reset_control_get(node, id, 0, 0); } /** - * of_reset_control_get_by_index - Lookup and obtain a reference to a reset - * controller by index. + * of_reset_control_get_by_index - Lookup and obtain an exclusive reference to + * a reset controller by index. * @node: device to be reset by the controller * @index: index of the reset controller * @@ -129,7 +147,7 @@ static inline struct reset_control *of_reset_control_get( static inline struct reset_control *of_reset_control_get_by_index( struct device_node *node, int index) { - return __of_reset_control_get(node, NULL, index); + return __of_reset_control_get(node, NULL, index, 0); } /** @@ -147,13 +165,13 @@ static inline struct reset_control *__must_check devm_reset_control_get( #ifndef CONFIG_RESET_CONTROLLER WARN_ON(1); #endif - return __devm_reset_control_get(dev, id, 0); + return __devm_reset_control_get(dev, id, 0, 0); } static inline struct reset_control *devm_reset_control_get_optional( struct device *dev, const char *id) { - return __devm_reset_control_get(dev, id, 0); + return __devm_reset_control_get(dev, id, 0, 0); } /** @@ -168,7 +186,38 @@ static inline struct reset_control *devm_reset_control_get_optional( static inline struct reset_control *devm_reset_control_get_by_index( struct device *dev, int index) { - return __devm_reset_control_get(dev, NULL, index); + return __devm_reset_control_get(dev, NULL, index, 0); +} + +/** + * devm_reset_control_get_shared - resource managed reset_control_get_shared() + * @dev: device to be reset by the controller + * @id: reset line name + * + * Managed reset_control_get_shared(). For reset controllers returned from + * this function, reset_control_put() is called automatically on driver detach. + * See reset_control_get_shared() for more information. + */ +static inline struct reset_control *devm_reset_control_get_shared( + struct device *dev, const char *id) +{ + return __devm_reset_control_get(dev, id, 0, 1); +} + +/** + * devm_reset_control_get_shared_by_index - resource managed + * reset_control_get_shared + * @dev: device to be reset by the controller + * @index: index of the reset controller + * + * Managed reset_control_get_shared(). For reset controllers returned from + * this function, reset_control_put() is called automatically on driver detach. + * See reset_control_get_shared() for more information. + */ +static inline struct reset_control *devm_reset_control_get_shared_by_index( + struct device *dev, int index) +{ + return __devm_reset_control_get(dev, NULL, index, 1); } #endif
In some SoCs some hw-blocks share a reset control. Add support for this setup by adding new: reset_control_get_shared() devm_reset_control_get_shared() devm_reset_control_get_shared_by_index() methods to get a reset_control. Note that this patch omits adding of_ variants, if these are needed later they can be easily added. This patch also changes the behavior of the existing exclusive reset_control_get() variants, if these are now called more then once for the same reset_control they will return -EBUSY. To catch existing drivers triggering this error (there should not be any) a WARN_ON(1) is added in this path. When a reset_control is shared, the behavior of reset_control_assert / deassert is changed, for shared reset_controls these will work like the clock-enable/disable and regulator-on/off functions. They will keep a deassert_count, and only (re-)assert the reset after reset_control_assert has been called as many times as reset_control_deassert was called. Calling reset_control_assert without first calling reset_control_deassert is not allowed on a shared reset control. Calling reset_control_reset is also not allowed on a shared reset control. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/reset/core.c | 61 ++++++++++++++++++++++++++++++++------- include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 114 insertions(+), 26 deletions(-)