On Wed, Feb 06, 2019 at 03:46:30PM +0100, Philipp Zabel wrote: > Hi Thierry, > > On Wed, 2019-02-06 at 12:38 +0100, Thierry Reding wrote: > [...] > > I see, that's a very good point. So it sounds indeed like we'll need to > > add some sort of implicit acquire to request_exclusive() to make this > > work. > > See below. > > > However, that's going to make the API a bit awkward to use, because in > > order to release the implicit acquisition the driver would have to do a > > release() that is, from a driver's point of view, unbalanced. > > That is true. We could make reset_control_acquire on an already acquired > control a no-op though, so this could look like: > > reset_control_get_exclusive() // implicitly acquires, may fail > reset_control_acquire() // optional, just makes acquire explicit (no-op) > reset_control_assert/deassert() > reset_control_release() I don't think we can do that. Otherwise, how do we make sure the acquire from a second driver isn't a no-op as well? > > or > > reset_control_get_exclusive_released() // does not implicitly acquire > res > et_control_acquire() // necessary, may fail > reset_control_assert/deassert > () > reset_control_release() Mhm... that makes sense. > > Also, if > > we implicitly acquire() in request_exclusive(), aren't we going to get > > into a situation where both driver A and driver B will try to acquire on > > request and one of them is going to fail? > > That is true, at least one of the drivers must request the control as > initially released, otherwise probing may fail depending on whether the > other driver currently acquired the reset control. Yeah. I think that this would really only be a temporary situation. Eventually drivers that deal with temporarily exclusive resets should transition to the acquire/release protocol in all consumer drivers. But yeah, the implicit acquire would make sure that existing code continues to work and the assumptions of the API hold true. > > > Also, how to decide at driver B request_exclusive() time whether or not > > > to throw a warning? We the core doesn't know at this point whether > > > driver B will use acquire()/release(). > > > > My suggestion was not to throw a warning a request_exclusive() time, but > > at acquire() time. > > I'd like to keep the warning in case of two drivers that never call > _acquire(). I assume this is error case that can easily happen by > writing a wrong phandle argument in the device tree. Yeah, makes sense. > [...] > > > > That's at least the way that it works on Tegra. Not sure if that is > > > > universally applicable, though it seems logical to me from a hardware > > > > point of view. It may not be required to keep the reset asserted while > > > > the power domain is turned off, but I'd be surprised if there was a > > > > requirement that it be deasserted while the power domain is off. > > > > > > There is hardware that doesn't allow to control the level of the reset > > > line, providing only a self clearing bit that can be used to trigger a > > > reset pulse (via reset_control_reset()). > > > > I don't think that really matters. The important part here is ownership. > > If the power domain owns the reset control upon turning off, no driver > > whose device is under that power domain should be handling the reset > > anyway. The device should only be able to take back control of the reset > > after the power domain has powered on again. > > Ok, that makes sense. > > > > > > > Hm... actually this means that power domains would have to request the > > > > > > reset controls and call reset_control_acquire() during probe. That way > > > > > > when the domain is powered on it will release the reset and free it up > > > > > > for the SOR driver to use. > > > > > > > > > > If you can guarantee that the power domain is powered up and releases > > > > > the reset control before the SOR driver tries to request the reset line, > > > > > that would work. > > > > > > > > Well, that's the part that the acquire/release protocol would enforce. > > > > > > Only if both drivers use it. I'd prefer the API to provide consistent > > > behaviour even if one of the drivers doesn't use acquire/release. > > > > I'm not sure how to solve that, unless we make the API asymmetric. So if > > an exclusive reset is implicitly acquired at request time, you need to > > have an unbalanced release() to release it for a second driver to use. > > Correct. > > > But you also don't know which one of the drivers will get the implicit > > acquire, so the second driver to request the reset control is already > > going to fail to acquire() at request time. Or if we build in some smart > > mechanism to only acquire() for the first driver to request, we'll end > > up with a situation where neither driver knows whether they own the > > reset control or not. > > That's why I suggested there should be a new method to explicitly get a > reset control without the implicit acquire step. That's more or less the > same as what you suggest below, just by a different name. Okay, great. > > To be honest, I have no idea if that could even work. It's like if you > > pass down a lock to two users but you don't tell them if it's locked or > > not. What are they supposed to do with that lock? None of them know who > > owns it, so the purpose of the lock is completely defeated. > > > > The simplest alternative would be to introduce some sort of flag that > > would enable the acquire/release protocol. Drivers that want to share > > exclusive resets this way would then explicitly request using this flag > > and have to implement the acquire/release protocol if they do. All > > consumers will get a released reset control and have to call acquire() > > on it first to gain exclusive access to it. > > ^ This. I currently call this reset_control_get_exclusive_released. > > > > > If your device shares a reset control with a power domain, this should > > > > work fine, provided that the device driver uses the reset only between > > > > (or during) ->runtime_resume() and ->runtime_suspend(). > > > > > > There should be a clear error path in case drivers don't use the reset > > > control only during suspend/resume as expected. > > > > There are two things that I imagine could work. I think in general we > > should return an error (-EBUSY) if we call acquire() on a reset control > > that's already acquired. > > Agreed. > > > Something else that may make sense is to allow > > acquire() to block if the consumers wish to do that. This has the usual > > caveats in that it could block on consumer indefinitely if another > > consumer never releases the reset control. > > I'd rather not open that can of worms. > > > On the other hand, what's a consumer to do if they get -EBUSY? If they > > use this protocol, then surely they do so because they won't work > > without access to the reset control, so if they get -EBUSY, presumably > > there's no way forward for them. > > > > Perhaps that means that such a condition is actually a bug and we should > > throw a big warning to make sure it gets addressed. Maybe this means the > > users of this protocol need to be extra cautious about sequencing, and > > that would be, in my opinion, a good argument in favour of adding an > > extra flag for this type of reset control. > > I agree with all of this. In the case we are currently talking about > (driver/pm domain), reset_control_acquire() returning -EBUSY is a bug. > > [...] > > Perhaps one alternative would be for all consumers to be modified with > > an explicit acquire() and release() pair before the new semantics take > > effect. But that seems a bit daunting: > > > > $ git grep -n reset_control_get | wc -l > > 381 > > Even if it was easy to change, I wouldn't want every driver to have to > deal with the acquire/release dance just because in some special cases > we have to be extra careful about who controls the reset line at what > time. Yeah, makes sense. > [...] > > I'm beginning to wonder if we're on a wild goose chase here. We've been > > focusing on exclusive resets here, but we're really talking about shared > > resets here. What we really want to do is share resets across multiple > > users and then allow one user to take exclusive control over the reset > > for a limited amount of time. > > I suppose you could just use shared resets and patch the reset > controller driver to initially assert these resets. But that would again > make it work by accident. > If at any point you want to call reset_control_assert() and expect this > to have an effect, shared reset controls are not the right method. > > > Would it be any easier to implement this on top of shared resets? Given > > the deassert count this would probably be difficult to do. Users of the > > shared reset could be at any point in their code execution when another > > user decides to acquire and assert/deassert. > > Right. As soon as any shared reset control user has called deassert, > there is no sane way to explicitly assert the reset from another driver. > We'd have to build some kind of notification framework for reset > requests and then argue about whether to give other drivers veto power > or force them to store internal state and cease all operations. > > > Hm... so I don't think the implicit acquire step during request would > > work because of the race conditions I mentioned above. The only other > > solution that I can think of is to have an explicit way to mark these > > resets as "exclusively shared" and require users of such resets to > > implement the acquire/release protocol. > > I agree. Here is an (untested) patch to illustrate how I thought > handling 'acquired' state could work: Very nice, just a couple of comments and questions inline. > ----------8<---------- > From: Philipp Zabel