linux-kernel - Re: [reset-control] How to initialize hardware state with the shared reset line?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1528191453.4074.4.camel@pengutronix.de>
Date:   Tue, 05 Jun 2018 11:37:33 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     Rob Herring <robh+dt@...nel.org>, Lee Jones <lee.jones@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        DTML <devicetree@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-amlogic@...ts.infradead.org
Subject: Re: [reset-control] How to initialize hardware state with the
 shared reset line?

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