Hi Hans, Am Mittwoch, den 18.11.2015, 11:38 +0100 schrieb Hans de Goede: > 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, We have to accommodate both. > and it seems to me that the short-pulse case should use reset_control_reset. reset_control_reset can only be used if the reset controller knows the necessary reset pulse duration itself. > Maybe we need to provide a default implementation of reset_control_reset which > does the pulse when no implementation is provided by the driver ? I have thought about adding a version that takes a delay parameter, but then you'd end up with udelay and msleep variants. > 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 Yes, that is a good idea. And disallow reset_control_get (non-shared) on an already in-use control. > 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) Maybe add a new structure between reset_controller_dev and reset_control, or keep the reset_control structures themselves on a list and hand them out again for reset_control_get_shared and do the reference counting there? Otherwise the reset_controller_dev would have to allocate a (possibly huge, sparse) array of reference counters. > 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 Sounds good, though I'd prefer WARN_ON over BUG_ON. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
