| Message ID | 20201106191743.40457-2-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ata: ahci_brcm: Fix use of BCM7216 reset controller | expand |
Hi Jim, On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote: > Before, only control_reset() was implemented. However, the reset core only > invokes control_reset() once in its lifetime. Because we need it to invoke > control_reset() again after resume out of S2 or S3, we have switched to > putting the reset functionality into the control_deassert() method and > having an empty control_assert() method. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> You are switching to the wrong abstraction to work around a deficiency of the reset controller framework. Instead, it would be better to allow to "reactivate" shared pulsed resets so they can be triggered again. Could you please have a look at [1], which tries to implement this with a new API call, and check if this can fix your problem? If so, it would be great if you could coordinate with Amjad to see this fixed in the core. [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/ regards Philipp > --- > drivers/reset/reset-brcmstb-rescal.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c > index b6f074d6a65f..1f54ae4f91fe 100644 > --- a/drivers/reset/reset-brcmstb-rescal.c > +++ b/drivers/reset/reset-brcmstb-rescal.c > @@ -20,8 +20,8 @@ struct brcm_rescal_reset { > struct reset_controller_dev rcdev; > }; > > -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev, > - unsigned long id) > +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > { > struct brcm_rescal_reset *data = > container_of(rcdev, struct brcm_rescal_reset, rcdev); > @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev, > return 0; > } > > +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + return 0; > +} > + > static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, > const struct of_phandle_args *reset_spec) > { > @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, > } > > static const struct reset_control_ops brcm_rescal_reset_ops = { > - .reset = brcm_rescal_reset_set, > + .deassert = brcm_rescal_reset_deassert, > + .assert = brcm_rescal_reset_assert, > }; > > static int brcm_rescal_reset_probe(struct platform_device *pdev)
On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Jim, > > On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote: > > Before, only control_reset() was implemented. However, the reset core only > > invokes control_reset() once in its lifetime. Because we need it to invoke > > control_reset() again after resume out of S2 or S3, we have switched to > > putting the reset functionality into the control_deassert() method and > > having an empty control_assert() method. > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > You are switching to the wrong abstraction to work around a deficiency > of the reset controller framework. Instead, it would be better to allow > to "reactivate" shared pulsed resets so they can be triggered again. True. > > > Could you please have a look at [1], which tries to implement this with > a new API call, and check if this can fix your problem? If so, it would > be great if you could coordinate with Amjad to see this fixed in the > core. > > [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/ Yes, this would work for our usage. Amjad please let me know if I can help here. The only "nit" I have is that I favor the name 'unreset' over 'resettable' but truly I don't care one way or the other. Thanks and kind regards, Jim Quinaln Broadcom STB > > > > regards > Philipp > > > --- > > drivers/reset/reset-brcmstb-rescal.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c > > index b6f074d6a65f..1f54ae4f91fe 100644 > > --- a/drivers/reset/reset-brcmstb-rescal.c > > +++ b/drivers/reset/reset-brcmstb-rescal.c > > @@ -20,8 +20,8 @@ struct brcm_rescal_reset { > > struct reset_controller_dev rcdev; > > }; > > > > -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev, > > - unsigned long id) > > +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev, > > + unsigned long id) > > { > > struct brcm_rescal_reset *data = > > container_of(rcdev, struct brcm_rescal_reset, rcdev); > > @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev, > > return 0; > > } > > > > +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + return 0; > > +} > > + > > static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, > > const struct of_phandle_args *reset_spec) > > { > > @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, > > } > > > > static const struct reset_control_ops brcm_rescal_reset_ops = { > > - .reset = brcm_rescal_reset_set, > > + .deassert = brcm_rescal_reset_deassert, > > + .assert = brcm_rescal_reset_assert, > > }; > > > > static int brcm_rescal_reset_probe(struct platform_device *pdev)
On Mon, 2020-11-09 at 11:21 -0500, Jim Quinlan wrote: > On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Jim, > > > > On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote: > > > Before, only control_reset() was implemented. However, the reset core only > > > invokes control_reset() once in its lifetime. Because we need it to invoke > > > control_reset() again after resume out of S2 or S3, we have switched to > > > putting the reset functionality into the control_deassert() method and > > > having an empty control_assert() method. > > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > > > You are switching to the wrong abstraction to work around a deficiency > > of the reset controller framework. Instead, it would be better to allow > > to "reactivate" shared pulsed resets so they can be triggered again. > > True. > > > > Could you please have a look at [1], which tries to implement this with > > a new API call, and check if this can fix your problem? If so, it would > > be great if you could coordinate with Amjad to see this fixed in the > > core. > > > > [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/ > > Yes, this would work for our usage. Amjad please let me know if I can > help here. The only "nit" I have is that I favor the name 'unreset' > over 'resettable' but truly I don't care one way or the other. Both unreset and resettable are adjectives, maybe it would be better to have an imperative verb like the other API functions. I would have liked reset_control_trigger/rearm as a pair, but I can't find anything I like that fits with the somewhat unfortunate reset_control_reset name in my mind. In that sense, I don't have a preference one way or the other either. regards Philipp
Hi everyone, On 09/11/2020 18:25, Philipp Zabel wrote: > On Mon, 2020-11-09 at 11:21 -0500, Jim Quinlan wrote: >> On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: >>> Hi Jim, >>> >>> On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote: >>>> Before, only control_reset() was implemented. However, the reset core only >>>> invokes control_reset() once in its lifetime. Because we need it to invoke >>>> control_reset() again after resume out of S2 or S3, we have switched to >>>> putting the reset functionality into the control_deassert() method and >>>> having an empty control_assert() method. >>>> >>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> >>> You are switching to the wrong abstraction to work around a deficiency >>> of the reset controller framework. Instead, it would be better to allow >>> to "reactivate" shared pulsed resets so they can be triggered again. >> True. >>> Could you please have a look at [1], which tries to implement this with >>> a new API call, and check if this can fix your problem? If so, it would >>> be great if you could coordinate with Amjad to see this fixed in the >>> core. >>> >>> [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/ >> Yes, this would work for our usage. Amjad please let me know if I can >> help here. The only "nit" I have is that I favor the name 'unreset' >> over 'resettable' but truly I don't care one way or the other. My pleasure, I will send a V2 soon of the patch, when it is done, please let me know if I can add anything that would suit best your use case as well. > Both unreset and resettable are adjectives, maybe it would be better to > have an imperative verb like the other API functions. I would have liked > reset_control_trigger/rearm as a pair, but I can't find anything I like > that fits with the somewhat unfortunate reset_control_reset name in my > mind. > In that sense, I don't have a preference one way or the other either. I think reset_control_rearm would be a very good candidate, resettable is quite representative but I think it is best to keep using verbs for the sake of homogeneity > > regards > Philipp Sincerely, Amjad
diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c index b6f074d6a65f..1f54ae4f91fe 100644 --- a/drivers/reset/reset-brcmstb-rescal.c +++ b/drivers/reset/reset-brcmstb-rescal.c @@ -20,8 +20,8 @@ struct brcm_rescal_reset { struct reset_controller_dev rcdev; }; -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev, - unsigned long id) +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) { struct brcm_rescal_reset *data = container_of(rcdev, struct brcm_rescal_reset, rcdev); @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev, return 0; } +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return 0; +} + static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, const struct of_phandle_args *reset_spec) { @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev, } static const struct reset_control_ops brcm_rescal_reset_ops = { - .reset = brcm_rescal_reset_set, + .deassert = brcm_rescal_reset_deassert, + .assert = brcm_rescal_reset_assert, }; static int brcm_rescal_reset_probe(struct platform_device *pdev)
Before, only control_reset() was implemented. However, the reset core only invokes control_reset() once in its lifetime. Because we need it to invoke control_reset() again after resume out of S2 or S3, we have switched to putting the reset functionality into the control_deassert() method and having an empty control_assert() method. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/reset/reset-brcmstb-rescal.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)