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 <[email protected]>
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/[email protected]/
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 <[email protected]> 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 <[email protected]>
>
> 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/[email protected]/
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 <[email protected]> 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 <[email protected]>
> >
> > 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/[email protected]/
>
> 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 <[email protected]> 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 <[email protected]>
>>> 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/[email protected]/
>> 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