Re: [linux-sunxi] Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants
On 4 January 2016 at 21:39, Philipp Zabel wrote: > Am Samstag, den 19.12.2015, 11:55 +0100 schrieb Hans de Goede: >> On 18-12-15 12:08, Maxime Ripard wrote: >> > - If the reset line is in a !exclusive use with more than 1 user, >> >the refcount is modified and an error is returned to notify that >> >it didn't happen. >> >> Also ack, except for returning the error, if a driver has used >> reset_control_get_shared, it should simply be aware that doing an assert >> might not necessarily actually assert the line, just like doing a clk-disable >> does not really necessary disable the clock, etc. If a driver is not prepared >> to deal with this, it should simply not use reset_control_get_shared. >> >> I see returning an error if the assert did not happen due to other users / >> deassert_count != 0 as inconsistent compared to how clks, regulators and phys >> handle this, these all simply return success in this case. > > I wouldn't want drivers to have to differentiate between relevant and > irrelevant error codes, so in the clock-like refcounting use case > reset_assert should not return an error if it just correctly decremented > the refcount. I'd still prefer to have separate API for the counted > must_deassert/may_assert vs the exclusive must_assert/must_deassert use > cases, but I just can't think of a good name. > Maybe something along the lines of assert_now or assert_sync. It should be possible to call on shared line and then get an error when the operation is blocked by other user. The driver may not really care. Depending on the hardware the line can be shared on one device and exclusive on another. The driver may just let the line go when the device is powered off. And it may require a reset cycle when it detects the device is hosed and return an error when the reset fails for whatever reason. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants
Am Samstag, den 19.12.2015, 11:55 +0100 schrieb Hans de Goede: > On 18-12-15 12:08, Maxime Ripard wrote: [...] > > I guess we could also have something like this: > > > >* The driver gets the reference to the reset line using > > reset_control_get or its shared variant. > > > > - If you call reset_control_get on a free line, it succeeds, and > >marks the line in exclusive use. > > - If you call reset_control_get on a busy line, it fails with > >EBUSY > > > > - If you call the shared variant on a free line, it succeeds > > - If you call the shared variant on a busy exclusive line, it > >fails with EBUSY > > - If you call the shared variant on a busy !exclusive line, it > >succeeds. >> > >* The customer driver can then call reset_control_assert / deassert: > > > > - If the reset line is in exclusive use, the assertion happens > >right away, it succeeds and returns 0. > > > > - If the reset line is in a !exclusive use, but with a single > >user, the assertion happens right away, it succeeds and returns > >0. > > Ack for all of the above, this is what I had in mind at first, but I started > with a more lightweight version as I'm lazy :) If Philipp likes this > suggestion I can rework my patch-set into this. Seconded, this all sounds good to me. > > - If the reset line is in a !exclusive use with more than 1 user, > >the refcount is modified and an error is returned to notify that > >it didn't happen. > > Also ack, except for returning the error, if a driver has used > reset_control_get_shared, it should simply be aware that doing an assert > might not necessarily actually assert the line, just like doing a clk-disable > does not really necessary disable the clock, etc. If a driver is not prepared > to deal with this, it should simply not use reset_control_get_shared. > > I see returning an error if the assert did not happen due to other users / > deassert_count != 0 as inconsistent compared to how clks, regulators and phys > handle this, these all simply return success in this case. I wouldn't want drivers to have to differentiate between relevant and irrelevant error codes, so in the clock-like refcounting use case reset_assert should not return an error if it just correctly decremented the refcount. I'd still prefer to have separate API for the counted must_deassert/may_assert vs the exclusive must_assert/must_deassert use cases, but I just can't think of a good name. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH v2 1/3] reset: Add shared reset_control_[de]assert variants
Hi,
On 18-12-15 12:08, Maxime Ripard wrote:
On Wed, Dec 16, 2015 at 12:21:48PM +0100, Philipp Zabel wrote:
Hi Maxime,
Am Mittwoch, den 16.12.2015, 11:29 +0100 schrieb Maxime Ripard:
On Mon, Dec 14, 2015 at 10:50:55AM +0100, Philipp Zabel wrote:
Am Montag, den 14.12.2015, 10:36 +0100 schrieb Maxime Ripard:
Hi,
On Fri, Dec 11, 2015 at 04:41:58PM +0100, Hans de Goede wrote:
diff --git a/include/linux/reset.h b/include/linux/reset.h
index c4c097d..1cca8ce 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -11,6 +11,8 @@ int reset_control_reset(struct reset_control *rstc);
int reset_control_assert(struct reset_control *rstc);
int reset_control_deassert(struct reset_control *rstc);
int reset_control_status(struct reset_control *rstc);
+int reset_control_assert_shared(struct reset_control *rstc);
+int reset_control_deassert_shared(struct reset_control *rstc);
Shouldn't that be handled in reset_control_get directly?
I think I see your point now. Maybe we should add a flags parameter to
reset_control_get and/or wrap it in two versions,
reset_control_get_exclusive and reset_control_get_shared (or just add
the _shared variant). Then reset_control_get(_exclusive) could return
-EBUSY if a reset line is already in use.
I guess the current assumption was that reset_control_get was
exclusive.
So we could have something like:
reset_control_get (..) {
return __reset_control_get(.., 0);
}
reset_control_get_shared (..) {
return __reset_control_get(.., RESET_SHARED);
}
And all the logic shared between the two, without exposing any flag
(that would change the prototype and require to fix every callers).
This is about different expectations of the caller.
A driver calling reset_control_assert expects the reset line to be
asserted after the call.
Is that behaviour documented explicitly somewhere?
/**
* reset_control_assert - asserts the reset line
* @rstc: reset controller
*/
Yeah, but it's not said when it would happen, or if it happens
synchronously.
Also, that expected behavior matches the function name, which I like.
So I still welcome adding new API calls for the shared/refcounting
variant.
A driver calling reset_control_assert_shared
just signals that it doesn't care about the state of the reset line
anymore.
We could just as well call the two new functions
reset_control_deassert_get and reset_control_deassert_put.
What happens if you mix them? What happens if you have several drivers
ignoring this API?
The core should give useful error messages and disallow non-shared reset
calls on shared lines.
I guess we could also have something like this:
* The driver gets the reference to the reset line using
reset_control_get or its shared variant.
- If you call reset_control_get on a free line, it succeeds, and
marks the line in exclusive use.
- If you call reset_control_get on a busy line, it fails with
EBUSY
- If you call the shared variant on a free line, it succeeds
- If you call the shared variant on a busy exclusive line, it
fails with EBUSY
- If you call the shared variant on a busy !exclusive line, it
succeeds.
* The customer driver can then call reset_control_assert / deassert:
- If the reset line is in exclusive use, the assertion happens
right away, it succeeds and returns 0.
- If the reset line is in a !exclusive use, but with a single
user, the assertion happens right away, it succeeds and returns
0.
Ack for all of the above, this is what I had in mind at first, but I started
with a more lightweight version as I'm lazy :) If Philipp likes this
suggestion I can rework my patch-set into this.
- If the reset line is in a !exclusive use with more than 1 user,
the refcount is modified and an error is returned to notify that
it didn't happen.
Also ack, except for returning the error, if a driver has used
reset_control_get_shared, it should simply be aware that doing an assert
might not necessarily actually assert the line, just like doing a clk-disable
does not really necessary disable the clock, etc. If a driver is not prepared
to deal with this, it should simply not use reset_control_get_shared.
I see returning an error if the assert did not happen due to other users /
deassert_count != 0 as inconsistent compared to how clks, regulators and phys
handle this, these all simply return success in this case.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
