[reset-control] How to initialize hardware state with the shared reset line?

[reset-control] How to initialize hardware state with the shared reset line?

[Martin Blumenstingl]

Hi Philipp,

On Tue, May 22, 2018 at 4:04 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Martin,
>
> On Mon, 2018-05-21 at 12:40 +0200, Martin Blumenstingl wrote:
>> Hello,
>>
>> On Mon, May 21, 2018 at 3:27 AM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> wrote:
>> > Hi.
>> >
>> >
>> > 2018-05-20 19:57 GMT+09:00 Martin Blumenstingl
>> > <martin.blumenstingl@googlemail.com>:
>> > > Hi,
>> > >
>> > > On Thu, May 10, 2018 at 11:16 AM, Masahiro Yamada
>> > > <yamada.masahiro@socionext.com> wrote:
>> > > [snip]
>> > > > I may be missing something, but
>> > > > one solution might be reset hogging on the
>> > > > reset provider side.  This allows us to describe
>> > > > the initial state of reset lines in the reset controller.
>> > > >
>> > > > The idea for "reset-hog" is similar to:
>> > > >  - "gpio-hog" defined in
>> > > >    Documentation/devicetree/bindings/gpio/gpio.txt
>> > > >  - "assigned-clocks" defined in
>> > > >    Documetation/devicetree/bindings/clock/clock-bindings.txt
>> > > >
>> > > >
>> > > >
>> > > > For example,
>> > > >
>> > > >    reset-controller {
>> > > >             ....
>> > > >
>> > > >             line_a {
>> > > >                   reset-hog;
>> > > >                   resets = <1>;
>> > > >                   reset-assert;
>> > > >             };
>> > > >    }
>> > > >
>> > > >
>> > > > When the reset controller is registered,
>> > > > the reset ID '1' is asserted.
>> > > >
>> > > >
>> > > > So, all reset consumers that share the reset line '1'
>> > > > will start from the asserted state
>> > > > (i.e. defined state machine state).
>> > >
>> > > I wonder if a "reset hog" can be board specific:
>> > > - GPIO hogs are definitely board specific (meson-gxbb-odroidc2.dts for
>> > > example uses it to take the USB hub out of reset)
>> > > - assigned-clock-parents (and the like) can also be board specific (I
>> > > made up a use-case since I don't know of any actual examples: board A
>> > > uses an external XTAL while board B uses some other internal
>> > > clock-source because it doesn't have an external XTAL)
>> > >
>> > > however, can reset lines be board specific? or in other words: do we
>> > > need to describe them in device-tree?
>> >
>> > Indeed.
>> >
>> > I did not come up with board-specific cases.
>> >
>> > The problem we are discussing is SoC-specific,
>> > and reset-controller drivers are definitely SoC-specific.
>> >
>> > So, I think the initial state can be coded in drivers instead of DT.
>>
>> OK, let's also hear Philipp's (reset framework maintainer) opinion on this
>
> I'd like to know if there are other SoC families besides Amlogic Meson
> that potentially could have this problem and about how many of the
> resets that are documented in include/dt-bindings/reset/amlogic,meson*
> we are actually talking. Are all of those initially deasserted and none
> of the connected peripherals have power-on reset mechanisms?
I cannot speak for other SoC families besides Amlogic
Meson8/Meson8b/Meson8m2 and GX (disclaimer: I am a community
contributor, I don't have access to Amlogic's internal datasheets - my
knowledge is based on their public datasheets, their GPL kernel/u-boot
sources and trial and error)

it seems that at least "some" (but I don't know the exact number)
resets are de-asserted by the bootloader
Amlogic's u-boot for example also enables all gate clocks by default

I CC'ed the Amlogic mailing list because I'm not sure if everyone
working on that SoC family is watching the linux-arm-kernel mailing
list

>> > > we could extend struct reset_controller_dev (= reset controller
>> > > driver) if they are not board specific:
>> > > - either assert all reset lines by default except if they are listed
>> > > in a new field (may break backwards compatibility, requires testing of
>> > > all reset controller drivers)
>> >
>> > This is quite simple, but I am afraid there are some cases where the forcible
>> > reset-assert is not preferred.
>> >
>> > For example, the earlycon.  When we use earlycon, we would expect it has been
>> > initialized by a boot-loader, or something.
>> > If it is reset-asserted on the while, the console output
>> > will not be good.
>>
>> indeed, so let's skip this idea
>
> Maybe we should at first add initial reset assertion to the Meson driver
> on a case by case bases?
this seems simple enough to test it - we can still generalize this
later on (either by adding support to the reset framework, DT bindings
or something else)

> We can't add required reset hog DT bindings to the Meson reset
> controller anyway without breaking DT backwards compatibility.
>
>> > > - specify a list of reset lines and their desired state (or to keep it
>> > > easy: specify a list of reset lines that should be asserted)
>> > > (I must admit that this is basically your idea but the definition is
>> > > moved from device-tree to the reset controller driver)
>> >
>> > Yes, I think the list of "reset line ID" and "init state" pairs
>> > would be nicer.
>>
>> $ grep -R "of_reset_n_cells = [^1]" drivers/reset/
>> drivers/reset/reset-berlin.c:   priv->rcdev.of_reset_n_cells = 2;
>> drivers/reset/hisilicon/reset-hi3660.c: rc->rst.of_reset_n_cells = 2;
>> drivers/reset/reset-ti-sci.c:   data->rcdev.of_reset_n_cells = 2;
>> drivers/reset/reset-lantiq.c:   priv->rcdev.of_reset_n_cells = 2;
>>
>> everything else uses only one reset cell
>> from the lantiq reset dt-binding documentation: "The first cell takes
>> the reset set bit and the second cell takes the status bit."
>>
>> I'm not sure what to do with drivers that specify != 1 reset-cell
>> though if we use a simple "init state pair"
>> maybe Philipp can share his opinion on this one as well
>
> See above, so far I am not convinced (either way) whether this should be
> described in the DT at all.
>
>> > > any "chip" specific differences could be expressed by using a
>> > > different of_device_id
>> > >
>> > > one the other hand: your "reset hog" solution looks fine to me if
>> > > reset lines can be board specific
>> > >
>> > > > From the discussion with Martin Blumenstingl
>> > > > (https://lkml.org/lkml/2018/4/28/115),
>> > > > the problem for Amlogic is that
>> > > > the reset line is "de-asserted" by default.
>> > > > If so, the 'reset-hog' would fix the problem,
>> > > > and DWC3 driver would be able to use
>> > > > shared, level reset, I think.
>> > >
>> > > I think you are right: if we could control the initial state then we
>> > > should be able to use level resets
>> >
>> >
>> > Even further, can we drop the shared reset_control_reset() support, maybe?
>> > (in other words, revert commit 7da33a37b48f11)
>>
>> I believe we need to keep this if there's hardware out there:
>> - where the reset controller only supports reset pulses
>> - at least one reset line is shared between multiple devices
>>
>> I didn't have a closer look at the Amlogic Meson6 SoC yet, but I think
>> it matches above criteria. as far as I know:
>> - the USB situation there is similar to Meson8b (USB controllers and
>> PHYs share a reset line)
>> - it uses an older reset controller IP block which does not support
>> level resets (only reset pulses)
>
> See my answer to Masahiro's first mail. I think somebody suggested in
> the past to add a fallback from the deassert to the reset op. I think
> this is something that should work in this case.
this is an interesting idea - it should work for Meson6 (in case
mainline ever gains support for this old SoC)


Regards
Martin

[reset-control] How to initialize hardware state with the shared reset line?

[Masahiro Yamada]

2018-05-25 5:09 GMT+09:00 Martin Blumenstingl
<martin.blumenstingl@googlemail.com>:
> Hi Philipp,
>
> On Tue, May 22, 2018 at 4:04 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> Hi Martin,
>>
>> On Mon, 2018-05-21 at 12:40 +0200, Martin Blumenstingl wrote:
>>> Hello,
>>>
>>> On Mon, May 21, 2018 at 3:27 AM, Masahiro Yamada
>>> <yamada.masahiro@socionext.com> wrote:
>>> > Hi.
>>> >
>>> >
>>> > 2018-05-20 19:57 GMT+09:00 Martin Blumenstingl
>>> > <martin.blumenstingl@googlemail.com>:
>>> > > Hi,
>>> > >
>>> > > On Thu, May 10, 2018 at 11:16 AM, Masahiro Yamada
>>> > > <yamada.masahiro@socionext.com> wrote:
>>> > > [snip]
>>> > > > I may be missing something, but
>>> > > > one solution might be reset hogging on the
>>> > > > reset provider side.  This allows us to describe
>>> > > > the initial state of reset lines in the reset controller.
>>> > > >
>>> > > > The idea for "reset-hog" is similar to:
>>> > > >  - "gpio-hog" defined in
>>> > > >    Documentation/devicetree/bindings/gpio/gpio.txt
>>> > > >  - "assigned-clocks" defined in
>>> > > >    Documetation/devicetree/bindings/clock/clock-bindings.txt
>>> > > >
>>> > > >
>>> > > >
>>> > > > For example,
>>> > > >
>>> > > >    reset-controller {
>>> > > >             ....
>>> > > >
>>> > > >             line_a {
>>> > > >                   reset-hog;
>>> > > >                   resets = <1>;
>>> > > >                   reset-assert;
>>> > > >             };
>>> > > >    }
>>> > > >
>>> > > >
>>> > > > When the reset controller is registered,
>>> > > > the reset ID '1' is asserted.
>>> > > >
>>> > > >
>>> > > > So, all reset consumers that share the reset line '1'
>>> > > > will start from the asserted state
>>> > > > (i.e. defined state machine state).
>>> > >
>>> > > I wonder if a "reset hog" can be board specific:
>>> > > - GPIO hogs are definitely board specific (meson-gxbb-odroidc2.dts for
>>> > > example uses it to take the USB hub out of reset)
>>> > > - assigned-clock-parents (and the like) can also be board specific (I
>>> > > made up a use-case since I don't know of any actual examples: board A
>>> > > uses an external XTAL while board B uses some other internal
>>> > > clock-source because it doesn't have an external XTAL)
>>> > >
>>> > > however, can reset lines be board specific? or in other words: do we
>>> > > need to describe them in device-tree?
>>> >
>>> > Indeed.
>>> >
>>> > I did not come up with board-specific cases.
>>> >
>>> > The problem we are discussing is SoC-specific,
>>> > and reset-controller drivers are definitely SoC-specific.
>>> >
>>> > So, I think the initial state can be coded in drivers instead of DT.
>>>
>>> OK, let's also hear Philipp's (reset framework maintainer) opinion on this
>>
>> I'd like to know if there are other SoC families besides Amlogic Meson
>> that potentially could have this problem and about how many of the
>> resets that are documented in include/dt-bindings/reset/amlogic,meson*
>> we are actually talking. Are all of those initially deasserted and none
>> of the connected peripherals have power-on reset mechanisms?
> I cannot speak for other SoC families besides Amlogic
> Meson8/Meson8b/Meson8m2 and GX (disclaimer: I am a community
> contributor, I don't have access to Amlogic's internal datasheets - my
> knowledge is based on their public datasheets, their GPL kernel/u-boot
> sources and trial and error)
>
> it seems that at least "some" (but I don't know the exact number)
> resets are de-asserted by the bootloader
> Amlogic's u-boot for example also enables all gate clocks by default
>
> I CC'ed the Amlogic mailing list because I'm not sure if everyone
> working on that SoC family is watching the linux-arm-kernel mailing
> list
>
>>> > > we could extend struct reset_controller_dev (= reset controller
>>> > > driver) if they are not board specific:
>>> > > - either assert all reset lines by default except if they are listed
>>> > > in a new field (may break backwards compatibility, requires testing of
>>> > > all reset controller drivers)
>>> >
>>> > This is quite simple, but I am afraid there are some cases where the forcible
>>> > reset-assert is not preferred.
>>> >
>>> > For example, the earlycon.  When we use earlycon, we would expect it has been
>>> > initialized by a boot-loader, or something.
>>> > If it is reset-asserted on the while, the console output
>>> > will not be good.
>>>
>>> indeed, so let's skip this idea
>>
>> Maybe we should at first add initial reset assertion to the Meson driver
>> on a case by case bases?
> this seems simple enough to test it - we can still generalize this
> later on (either by adding support to the reset framework, DT bindings
> or something else)
>
>> We can't add required reset hog DT bindings to the Meson reset
>> controller anyway without breaking DT backwards compatibility.
>>
>>> > > - specify a list of reset lines and their desired state (or to keep it
>>> > > easy: specify a list of reset lines that should be asserted)
>>> > > (I must admit that this is basically your idea but the definition is
>>> > > moved from device-tree to the reset controller driver)
>>> >
>>> > Yes, I think the list of "reset line ID" and "init state" pairs
>>> > would be nicer.
>>>
>>> $ grep -R "of_reset_n_cells = [^1]" drivers/reset/
>>> drivers/reset/reset-berlin.c:   priv->rcdev.of_reset_n_cells = 2;
>>> drivers/reset/hisilicon/reset-hi3660.c: rc->rst.of_reset_n_cells = 2;
>>> drivers/reset/reset-ti-sci.c:   data->rcdev.of_reset_n_cells = 2;
>>> drivers/reset/reset-lantiq.c:   priv->rcdev.of_reset_n_cells = 2;
>>>
>>> everything else uses only one reset cell
>>> from the lantiq reset dt-binding documentation: "The first cell takes
>>> the reset set bit and the second cell takes the status bit."
>>>
>>> I'm not sure what to do with drivers that specify != 1 reset-cell
>>> though if we use a simple "init state pair"
>>> maybe Philipp can share his opinion on this one as well
>>
>> See above, so far I am not convinced (either way) whether this should be
>> described in the DT at all.
>>
>>> > > any "chip" specific differences could be expressed by using a
>>> > > different of_device_id
>>> > >
>>> > > one the other hand: your "reset hog" solution looks fine to me if
>>> > > reset lines can be board specific
>>> > >
>>> > > > From the discussion with Martin Blumenstingl
>>> > > > (https://lkml.org/lkml/2018/4/28/115),
>>> > > > the problem for Amlogic is that
>>> > > > the reset line is "de-asserted" by default.
>>> > > > If so, the 'reset-hog' would fix the problem,
>>> > > > and DWC3 driver would be able to use
>>> > > > shared, level reset, I think.
>>> > >
>>> > > I think you are right: if we could control the initial state then we
>>> > > should be able to use level resets
>>> >
>>> >
>>> > Even further, can we drop the shared reset_control_reset() support, maybe?
>>> > (in other words, revert commit 7da33a37b48f11)
>>>
>>> I believe we need to keep this if there's hardware out there:
>>> - where the reset controller only supports reset pulses
>>> - at least one reset line is shared between multiple devices
>>>
>>> I didn't have a closer look at the Amlogic Meson6 SoC yet, but I think
>>> it matches above criteria. as far as I know:
>>> - the USB situation there is similar to Meson8b (USB controllers and
>>> PHYs share a reset line)
>>> - it uses an older reset controller IP block which does not support
>>> level resets (only reset pulses)
>>
>> See my answer to Masahiro's first mail. I think somebody suggested in
>> the past to add a fallback from the deassert to the reset op. I think
>> this is something that should work in this case.
> this is an interesting idea - it should work for Meson6 (in case
> mainline ever gains support for this old SoC)
>
>



One more thing.


I want to remove reset_control_reset() entirely.


[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.


For all reset consumers,
replace
  reset_control_reset();
with
  reset_control_assert();
  reset_control_deassert();


and deprecate reset_control_reset().

I think this is the right thing to do.



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.



Or, we can keep the reset drivers as they are.
drivers/reset/core.c can take care of the proper fallback logic.






-- 
Best Regards
Masahiro Yamada

[reset-control] How to initialize hardware state with the shared reset line?

[Philipp Zabel]

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