Hi Cheng-yi,
[adding Maxime, devicetree to Cc:, the old discussion about GPIO resets
in [4] has never been resolved]
On Tue, 2018-10-09 at 21:46 +0800, Cheng-yi Chiang wrote:
> +reset controller maintainer Philipp
>
> Hi Mark,
> Sorry for the late reply. It took me a while to investigate reset
> controller and its possible usage. I would like to figure out the
> proper way of reset handling because it is common to have a shared
> reset line for two max98927 codecs for left and right channels.
> Without supporting this usage, a simple reset-gpios change for single
> codec would not be useful, and perhaps will be duplicated if reset
> controller is a better way.
>
> Hi Philipp,
> I would like to seek your advice about whether we can use reset
> controller to support the use case where multiple devices share the
> same reset line.
>
> Let me summarize the use case:
> There are two max98927 codecs sharing the same reset line.
> The reset line is controlled by a GPIO.
> The goal is to toggle reset line once and only once.
>
> There was a similar scenario in tlv320aic3x codec driver [1].
> A static list is used in the codec driver so probe function can know
> whether it is needed to toggle reset line.
> Mark pointed out that it is not suitable to handle the shared reset
> line inside codec driver.
> A point is that this only works for multiple devices using the same
> device driver.
> He suggested to look into reset controller so I searched through the
> usage for common reset line.
>
> Here I found a shared reset line use case [2].
> With the patch [2], reset_control_reset can support this “reset once
> and for all” use case.
This patch was applied as 7da33a37b48f. So the reset framework has
support for shared reset controls where only the first reset request
actually causes a reset pulse, and all others are no-ops.
> However, I found that there are some missing pieces:
>
> Let’s assume there are two codecs c1 and c2 sharing a reset line
> controlled by GPIO.
>
> c1’s spec:
> Hold time: The minimum time to assert the reset line is t_a1.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d1.
>
> c2’s spec:
> Hold time: The minimum time to assert the reset line is t_a2.
> Release time: The minimum time to access the chip after deassert of
> the reset line is t_d2.
>
> For both c1 and c2 to work properly, we need a reset controller that
> can assert the reset line for
> T = max(t_a1, t_a2).
>
> 1. We need reset controller to support GPIO.
I still don't like the idea of describing a separate gpio reset
controller "device" in DT very much, as this is really just a software
abstraction, not actual hardware. For example:
gpio_reset: reset-controller {
compatible = "gpio-reset";
reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>,
<&gpio1
16 GPIO_ACTIVE_HIGH>;
reset-delays-us = <10000>, <500>;
};
c1 {
resets = <&gpio_reset 0>; /* maps to <&gpio0 15 ...> */
};
c2 {
resets = <&gpio_reset 0>;
};
What I would like better would be to let the consumers keep their reset-
gpios bindings, but add an optional hold time override in the DT:
c1 {
reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
reset-delays-us = <10000>;
};
c2 {
reset-gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
re
set-delays-us = <10000>;
};
The reset framework could create a reset_control backed by a gpio
instead of a rcdev. I have an unfinished patch for this, but without the
sharing requirement adding the reset framework abstraction between gpiod
and drivers never seemed really worth it.
> 2. We need to specify hold time T to reset controller in device tree
> so it knows that it needs hold reset line for that long in its
> implementation of reset_control_reset.
Agreed. Ideally, I'd like to make this optional, and have the drivers
continue to provide the hold time if they think they know it.
> 3. Assuming we have 1 and 2 in place. In codec driver of c1, it can
> call reset_control_reset and wait for t_a1 + t_d1. In codec driver of
> c2, it can call reset_control_reset and wait for t_a2 + t_d2.
The reset framework should wait for the configured assertion time,
max(t_a1, t_a2). The drivers only should only have to wait for
t_d1 / t_d2 after reset_control_reset returns.
> We need to wait for hold time + release time because
> triggered_count is increased before reset ops is called. When the
> second driver finds that triggered_count is 1 and skip the real reset
> operation, reset ops might just begin doing its work a short time ago.
That is a good point. Maybe the reset framework should just wait for the
hold time even for the second reset. Another possibility would be to
serialize them with a mutex.
> I am not sure whether we would need a flag in reset controller to
> mark that "reset is done". When driver sees this flag is done, it can
> just wait for release time instead of hold time + release time.
Let's not complicate the drivers with this too much. I think
reset_control_reset should guarantee that the reset line is not asserted
anymore upon return.
> And I found that you already solved 1 and mentioned the
> possible usage of 2 in [3].
> There were discussion about letting device driver to deal with
> reset-gpios by itself in [4], but it seems that reset controller can
> better deal with the shared reset line situation.
> Maybe we could revive the patch of GPIO support for reset controller ?
The discussion with Maxime in [4] hasn't really been resolved. I still
think the reset-gpios property should be kept, and the reset framework
could adapt to it. This is something the device tree maintainers' input
would be welcome on.
> Please let me know what direction I should look into.
> Thanks a lot!
>
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2010-November/033246.html
> https://patchwork.kernel.org/patch/9424123/
>
> [2] https://patchwork.kernel.org/patch/9424123/
>
> [3] https://lore.kernel.org/patchwork/patch/455839/
>
> [4] https://patchwork.kernel.org/patch/3978121/
regards
Philipp
