On Wed, Nov 18, 2015 at 11:38:17AM +0100, Hans de Goede wrote: > Hi, > > On 18-11-15 10:46, Philipp Zabel wrote: > >Hi Hans, > > > >Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: > >>On 16-11-15 18:01, Philipp Zabel wrote: > >>>If there are two devices sharing the same reset line that is initially > >>>held asserted, do the two drivers somehow have to synchronize before > >>>releasing the reset together? > >> > >>Not to my knowledge, I suggest that we simply treat this same as > >>regulators / clocks where the first one to enable it actually gets > >>it enabled (de-asserted in case of a reset), and the last one > >>to disable (assert) it (so dropping the usage counter back to 0) leads > >>to it being asserted again. > >> > >>This seems to work well enough for clocks / regulators and phys, and > >>at least for my use-case it should work fine for the shared reset too > >>(I will test to make sure of course). > >> > >>So from my pov a simple counter should suffice, does that work for you ? > > > >I don't quite understand what counting will help with, then. The first > >driver to call reset_control_deassert will deassert the reset, whether > >you count or not. > >But if the two drivers have deasserted an initially asserted reset, a > >reset_control_assert for one of them will silently fail. > > Correct, which is what we want, although I would not call it silently > fail I would call it a nop as it is intended. > > >I fear the deassertion count maps well to the case where resets are used > >just like clocks (when inactive modules are kept in reset), but I'm not > >sure this is useful in the case of resets that are kept deasserted most > >of the time, only to be asserted for a short pulse. Maybe we have to > >differentiate between the two cases? > > Ack. I think that the "just like clocks" case is the more common one, and > it seems to me that the short-pulse case should use reset_control_reset. > > Maybe we need to provide a default implementation of reset_control_reset which > does the pulse when no implementation is provided by the driver ? > > Although that brings the question with it what to do with the deassert_count in > that case, as some drivers may also use that for the initial deassert... I guess > we could document to not do that if you want to assure that no other drivers > muck with the reset-line ... > > Hmm, this is getting messy pretty quickly. New proposal: > > 1) Add a concept of shared resets, adding: reset_control_get_shared and > devm_reset_control_get_shared functions, which set a shared bool > in struct reset_control Maybe we can simply be a bit more generic and add a version taking flags as an argument, the fact that the reset line is shared being one of these flags. I guess both these versions have the issue that eventually every driver might use the shared variant (since we'd need to use it if one board or SoC is using a shared reset line, pretty much the same situation than for the interrupts), which would reduce the effectiveness of reset_control_get would not be able to catch exclusive use of the reset line. > 2) Add int refcnt to struct reset_controller_dev, which gets > incremented/decremented on reset_control_get/reset_control_put > do a BUG_ON on refcnt == 1 in the get functions when the non-shared > variant gets called (this is optional but probably a good extra > check) > > 3) Do the whole deassert_count thingy only when the shared bool is true > > 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true I don't think silently failing (at least from the driver point of view) is the right approach. You've used clocks as an example, but there's a quite significant difference between clocks and reset lines: you never really care if the clock is actually disabled or not while when you want to put the device in reset, you usually expect it to be actually reset when the function returns (to deal with unrecoverable hardware errors for example). And drivers can always ignore the return code if they don't care. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
