Message-ID: <ef8cd536-fed1-4117-8500-fffb776a57da@seco.com>
Date: Tue, 9 Jan 2024 10:51:34 -0500
From: Sean Anderson <sean.anderson@...o.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>,
 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Banajit Goswami <bgoswami@...cinc.com>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, linux-arm-msm@...r.kernel.org,
 alsa-devel@...a-project.org, linux-sound@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH 2/4] reset: add GPIO-based reset controller

On 1/9/24 04:41, Krzysztof Kozlowski wrote:
> On 05/01/2024 15:31, Philipp Zabel wrote:
>>>>> Sorry, then I don't get what you refer to. The driver calls deassert
>>>>> when it is safe for it to do it, so the driver *knows*. Now, you claim
>>>>> that driver does not know that... core also does not know, so no one knows.
>>>>
>>>> Yes! That is the problem with this design. Someone has to coordinate the
>>>> reset, and it can't be the driver. But the core also doesn't have enough
>>>> information. So no one can do it.
>>>
>>> The point is that the driver coordinates.
>> 
>> Currently the reset controller API supports two types of shared resets.
>> I hope distinguishing the two types and illustrating them helps the
>> discussion:
>> 
>> 1) For devices that just require the reset to be deasserted while they
>> are active, and don't care otherwise, there is the clk-like behavior
>> described in [1].
>> 
>>   requested reset signal via reset_control_deassert/assert():
>>     device A: ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺\⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽/⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
>>     device B: ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺\⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽/⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
>> 
>>   actual reset signal to both devices:
>>               ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺\⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽/⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺
>> 
>> In this scenario, there should be no delays in the reset controller
>> driver. reset_control_deassert() may return as soon as the physical
>> reset signal is deasserted [2]. Any post-deassert delays required by
>> the devices are handled in the device drivers, and they can be
>> different for each device. The devices have to be able to cope with a
>> (much) longer post-deassert delay than expected (e.g. device B in this
>> case). It is assumed that the reset signal is initially asserted.
>> 
>> The reset-gpio patchset supports this.
> 
> Yep! :)
> 
>> 
>> 2) The second type is for devices that require a single reset pulse for
>> initialization, at any time before they become active. This is
>> described in [3].
>> 
>>   requested reset signal via reset_control_reset/rearm():
>>     device A: ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽/⎺⎺\⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
>>     device B: ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽/⎺⎺\⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
>> 
>>   actual reset signal to both devices:
>>               ⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽/⎺⎺\⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽⎽
>> 
>> Here the reset controller needs to know the delay between assertion and
>> deassertion - either baked into the hardware or as a delay call in the
>> .reset callback.
>> 
>> This is not supported by the reset-gpio patchset. It could be
> 
> Yep, as well.
> 
>> implemented via a delay property in the device tree that would have to
>> be the same for all devices sharing the reset line, and by adding the
> 
> Or through dedicated node to which reset-gpio binds, just like in Sean's
> code some years ago. Nothing stops achieving that, except of course
> convincing Rob. The point is that although my design does not solve it,
> it also does not prevent it in the future.

Given this and

> If the reset deassert (or assert, depending what's the default state) is
> triggered in the probe, then it will happen with the probe of the first
> device. If the delays of that reset are not suitable for the second -
> not yet probed - then what do you propose? I have the answer: do not use
> the simple, generic solution. The simple and generic solutions work for
> simple and generic cases.

I think a separate pseudo-device is necessary a generic solution. So I
guess I will revive my patchset.

>> .reset callback to the reset controller driver. The only issue is that
>> the initial state of the reset line should be deasserted, and at
>> reset_control_get() time, when the reset-gpio controller is
>> instantiated, it is not yet known which type the driver will use.
>> 
>> Sharing a reset line between devices of different type is not
>> supported. Unfortunately, this will only fail at
>> reset_control_deassert() / reset_control_reset() time when the second
>> device tries to use the reset control in a different way than the
>> first.
>> 
>> [1] https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.kernel.org%2fdriver%2dapi%2freset.html%23assertion%2dand%2ddeassertion&umid=0ba4c26a-9b7a-4e4b-8dba-ac7f2f194fcd&auth=d807158c60b7d2502abde8a2fc01f40662980862-4bd780a2a258eadb798324d4af563d691f01efb6
>> [2] https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.kernel.org%2fdriver%2dapi%2freset.html%23c.reset%5fcontrol%5fdeassert&umid=0ba4c26a-9b7a-4e4b-8dba-ac7f2f194fcd&auth=d807158c60b7d2502abde8a2fc01f40662980862-d6349120c92e1887c5765842e10b274192584bde
>> [3] https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdocs.kernel.org%2fdriver%2dapi%2freset.html%23triggering&umid=0ba4c26a-9b7a-4e4b-8dba-ac7f2f194fcd&auth=d807158c60b7d2502abde8a2fc01f40662980862-973380c68f114aad02c47e69b5ceb92a23759963
>> 
>>>> For example, say we want to share a reset GPIO between two devices. Each
>>>> device has the following constraints:
>>>>
>>>> device post-assert delay post-deassert delay
>>>> ====== ================= ===================
>>>> A                  500us                 1ms
>>>> B                    1ms               300us
>>>
>>> And now imagine that these values are incompatible between them, so
>>> using 1ms on device A is wrong - too long.
>>>
>>> This is just not doable. You invented some imaginary case to prove that
>>> hardware is broken.
>>>
>>> Now, if we are back to realistic cases - use just the longest reset time.
>> 
>> Right. This all only works if no device has an upper bound to the
>> allowed delays on the shared reset line.
> 
> If device had an upper bound, it would be quite a conflicting design,
> tricky to implement. I don't think we should target such case with
> generic solution.

This is why I had explicit properties for the various durations. That
way the system integrator can go through the reset requirements
and specify something which satisfies all devices.

--Sean

Powered by blists - more mailing lists