Hi Masahiro, On Wed, 2018-05-30 at 14:57 +0900, Masahiro Yamada wrote: > One more thing. > > I want to remove reset_control_reset() entirely. reset_control_reset is for those cases where "the reset controller knows" how to reset us. There are hardware reset controllers that can control a bunch of actual reset signals in the right order and with the right timings necessary for the connected IP cores by triggering a single bit. In that case it wouldn't make much sense to do assert / delay / deassert in the driver, as the information about the delay is contained in the reset controller hardware. > [1] Some reset consumers (e.g. drivers/ata/sata_gemini.c) > use reset_control_reset() to reset the HW. > > [2] Some reset consumers (e.g. drivers/input/keyboard/tegra-kbc.c) > use the combination of reset_control_assert() and reset_control_deassert() > to reset the HW. > > [1] is the only way if the reset controller only supports the pulse reset. > > [2] is the only way if the reset controller only supports the level reset. > > So, this is another strangeness because > the implementation of reset controller > affects reset consumers. > > We do not need [1]. > > [2] is more flexible than [1] because hardware usually specifies > how long the reset line should be kept asserted. This is not always the case. > For all reset consumers, > replace > reset_control_reset(); > with > reset_control_assert(); > reset_control_deassert(); To be honest, it doesn't make sense to me. If the intention in the driver is just to reset our internal state, and we have a system reset controller that can reset us by writing a single bit, I'd prefer to call a reset function over two assert/deassert functions, one of which ends up doing nothing. How about moving in the other direction, and allowing to replace reset_control_assert(rstc); udelay(delay); reset_control_deassert(rstc); and variants with calls like reset_control_reset_udelay(rstc, delay); ? If the reset controller knows better, or can't change the delay in hardware, it may ignore the delay parameter. > and deprecate reset_control_reset(). > > I think this is the right thing to do. I don't think this helps the API, as with that change we have to remove a guarantee it currently makes: This either only works for shared resets or we have to accept that reset_control_assert for exclusive resets does not guarantee to return with the reset line asserted anymore. Also, for drivers that do deassert in probe and assert in remove, we would have to issue the reset in deassert and let assert be the no-op, instead of the other way around. > The reset controller side should be implemented like this: > > If your reset controller only supports the pulse reset, > .deassert hook should be no-op. > .assert hook should pulse the reset > > Then .reset hook should be removed. There is hardware where assert, deassert, and reset are three different operations. See for example the tegra/reset-bpmp.c driver. Both assert / deassert and module reset messages are part of the firmware ABI. > Or, we can keep the reset drivers as they are. > drivers/reset/core.c can take care of the proper fallback logic. I prefer to keep assert, deassert and reset separate for those cases where the hardware actually supports both variants. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
