Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()
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 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 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-aouledam...@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
Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()
On Mon, 2020-11-09 at 11:21 -0500, Jim Quinlan wrote: > On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel 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 > > > > 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-aouledam...@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
Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()
On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel 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
>
> 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-aouledam...@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)
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()
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
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-aouledam...@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)
[PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()
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
---
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)
--
2.17.1
smime.p7s
Description: S/MIME Cryptographic Signature
