[v2] ARC: HSDK: improve reset driver

As for today HSDK reset driver implements only
.reset() callback.

In case of driver which implements one of standard
reset controller usage pattern
(call *_deassert() in probe(), call *_assert() in remove())
that leads to inoperability of this reset driver.

Improve HSDK reset driver by calling .reset() callback inside of
.deassert() callback to avoid each reset controller
user adaptation for work with both reset methods
(reset() and {.assert() & .deassert()} pair)

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
Changes v1->v2:
 * Don't call hsdk_reset_reset in .assert callback.

 drivers/reset/reset-hsdk.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Hi Eugeniy,

thank you for the update.

On Fri, 2018-09-28 at 19:28 +0300, Eugeniy Paltsev wrote:
> As for today HSDK reset driver implements only
> .reset() callback.
> 
> In case of driver which implements one of standard
> reset controller usage pattern
> (call *_deassert() in probe(), call *_assert() in remove())
> that leads to inoperability of this reset driver.
> 
> Improve HSDK reset driver by calling .reset() callback inside of
> .deassert() callback to avoid each reset controller
> user adaptation for work with both reset methods
> (reset() and {.assert() & .deassert()} pair)
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
> Changes v1->v2:
>  * Don't call hsdk_reset_reset in .assert callback.
> 
>  drivers/reset/reset-hsdk.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/reset/reset-hsdk.c b/drivers/reset/reset-hsdk.c
> index 8bce391c6943..399440f197c5 100644
> --- a/drivers/reset/reset-hsdk.c
> +++ b/drivers/reset/reset-hsdk.c
> @@ -84,8 +84,21 @@ static int hsdk_reset_reset(struct reset_controller_dev *rcdev,
>  	return ret;
>  }
>  
> +static int hsdk_reset_dummy(struct reset_controller_dev *rcd, unsigned long id)
> +{
> +	return 0;
> +}
> +
> +/*
> + * Doing real reset from .assert isn't necessary/useful here. So we pass
> + * 'hsdk_reset_dummy' to .assert callback to prevent -ENOTSUPP returning by
> + * reset_control_assert() to make happy drivers which check
> + * reset_control_{assert | deassert} return status.
> + */
>  static const struct reset_control_ops hsdk_reset_ops = {
>  	.reset	= hsdk_reset_reset,
> +	.assert = hsdk_reset_dummy,

Is this part really necessary though? reset_control_assert already
returns 0 in shared mode, so the only thing this does is make
reset_control_assert return 0 instead of -ENOTSUPP in exclusive mode.

The documentation states that calling reset_control_assert "on an
exclusive reset controller guarantees that the reset will be asserted."
Since this is clearly not the case with this driver, it is appropriate
to keep returning an error in this case.

If there is a driver that requests an exclusive reset control, calls
reset_control_assert, and then checks the error value to see whether
asserting the reset succeeded, it should be made aware that
we couldn't actually assert the reset line as requested. If the driver
can continue operation even though the reset line was not asserted,
it should ignore the error.

So if you need to hide this error, I'd like to know the actual case that
is fixed by this, to see if we can't fix it in a better way.

> +	.deassert = hsdk_reset_reset,

This part is fine.

>  };
>  
>  static int hsdk_reset_probe(struct platform_device *pdev)

regards
Philipp

On Fri, 2018-10-12 at 14:08 +0200, Philipp Zabel wrote:
> Hi Eugeniy,
> 
> thank you for the update.
> 
> On Fri, 2018-09-28 at 19:28 +0300, Eugeniy Paltsev wrote:
> > As for today HSDK reset driver implements only
> > .reset() callback.
> > 
> > In case of driver which implements one of standard
> > reset controller usage pattern
> > (call *_deassert() in probe(), call *_assert() in remove())
> > that leads to inoperability of this reset driver.
> > 
> > Improve HSDK reset driver by calling .reset() callback inside of
> > .deassert() callback to avoid each reset controller
> > user adaptation for work with both reset methods
> > (reset() and {.assert() & .deassert()} pair)
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> > Changes v1->v2:
> >  * Don't call hsdk_reset_reset in .assert callback.
> > 
> >  drivers/reset/reset-hsdk.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/reset/reset-hsdk.c b/drivers/reset/reset-hsdk.c
> > index 8bce391c6943..399440f197c5 100644
> > --- a/drivers/reset/reset-hsdk.c
> > +++ b/drivers/reset/reset-hsdk.c
> > @@ -84,8 +84,21 @@ static int hsdk_reset_reset(struct reset_controller_dev *rcdev,
> >  	return ret;
> >  }
> >  
> > +static int hsdk_reset_dummy(struct reset_controller_dev *rcd, unsigned long id)
> > +{
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Doing real reset from .assert isn't necessary/useful here. So we pass
> > + * 'hsdk_reset_dummy' to .assert callback to prevent -ENOTSUPP returning by
> > + * reset_control_assert() to make happy drivers which check
> > + * reset_control_{assert | deassert} return status.
> > + */
> >  static const struct reset_control_ops hsdk_reset_ops = {
> >  	.reset	= hsdk_reset_reset,
> > +	.assert = hsdk_reset_dummy,
> 
> Is this part really necessary though? reset_control_assert already
> returns 0 in shared mode, so the only thing this does is make
> reset_control_assert return 0 instead of -ENOTSUPP in exclusive mode.
> 
> The documentation states that calling reset_control_assert "on an
> exclusive reset controller guarantees that the reset will be asserted."
> Since this is clearly not the case with this driver, it is appropriate
> to keep returning an error in this case.
> 
> If there is a driver that requests an exclusive reset control, calls
> reset_control_assert, and then checks the error value to see whether
> asserting the reset succeeded, it should be made aware that
> we couldn't actually assert the reset line as requested. If the driver
> can continue operation even though the reset line was not asserted,
> it should ignore the error.
> 
> So if you need to hide this error, I'd like to know the actual case that
> is fixed by this, to see if we can't fix it in a better way.

Ok, I can drop it in my case as it will work fine with certain drivers:
(several drivers use shared reset control, other drivers use exclusive reset
control but don't check reset_control_assert() return value)

I simply want to say that this wouldn't work in all cases (without changes
in driver which use reset control).

I'll respin patch with this part dropped.

> > +	.deassert = hsdk_reset_reset,
> 
> This part is fine.
> 
> >  };
> >  
> >  static int hsdk_reset_probe(struct platform_device *pdev)
> 
> regards
> Philipp

Hi Eugeniy,

On Wed, 2018-10-17 at 13:54 +0000, Eugeniy Paltsev wrote:
[...]
> > The documentation states that calling reset_control_assert "on an
> > exclusive reset controller guarantees that the reset will be asserted."
> > Since this is clearly not the case with this driver, it is appropriate
> > to keep returning an error in this case.
> > 
> > If there is a driver that requests an exclusive reset control, calls
> > reset_control_assert, and then checks the error value to see whether
> > asserting the reset succeeded, it should be made aware that
> > we couldn't actually assert the reset line as requested. If the driver
> > can continue operation even though the reset line was not asserted,
> > it should ignore the error.
> > 
> > So if you need to hide this error, I'd like to know the actual case that
> > is fixed by this, to see if we can't fix it in a better way.
> 
> Ok, I can drop it in my case as it will work fine with certain drivers:
> (several drivers use shared reset control, other drivers use exclusive reset
> control but don't check reset_control_assert() return value)
>
> I simply want to say that this wouldn't work in all cases (without changes
> in driver which use reset control).

Ok, if there is ever such a case, please let me know.

regards
Philipp