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

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

[Masahiro Yamada]

Hi.


The previous thread was:
"usb: dwc3: support clocks and resets for DWC3 core"
https://patchwork.kernel.org/patch/10349623/


I changed the subject because
I think this is rather reset-control topic than USB.


I am searching for a good way to initialize hardware devices
in the following situation:

 - two or more hardware devices share the same reset line

 - those devices are not reset before booting the kernel
   (i.e. flip flops in RTL are random state at driver probe)

 - the hardware IP is used by various SoCs,
   and this issue only appears only on some.



Specifically, the DWC3 USB IP case is in my mind,
but this should apply to any hardware in general.




Issue in detail
---------------

The DWC3 USB IP is very popular and used by various SoCs
as you see in drivers/usb/dwc3/.

SoCs are using the same RTL as provided by Synopsys.
(SoC vendors could tweak the RTL if they like,
but it would rarely happen because it would just be troublesome)

So, let's assume we use the same IP with the same RTL version.
It is *compatible* in terms of device tree.
In this case, we should avoid differentiating the driver code per-SoC.

In fact, we ended up with
commit ff0a632 ("usb: dwc3: of-simple: add support for shared and
pulsed reset lines")
commit e8284db ("usb: dwc3: of-simple: add support for the Amlogic
Meson GXL and AXG SoCs")


This means, the difference that comes from the reset _provider_
must be implemented in the reset _consumer_.
This is unfortunate.

So, I wonder if we can support it in sub-system level somehow
instead of messing up the reset consumer driver.

 - The reset topology is SoC-dependent.
   A reset line is dedicated for single hardware for some SoCs,
   and shared by multiple hardware for others.

 - The reset policy (pulse reset, level reset, or both of them)
   are SoC-dependent (reset controller dependent).



RTL generally contains state machines.
The initial state of flip flops are undefined on power-on
hence the initial state of state machines are also undefined.

So, digital circuits needs explicit reset in general.

In some cases, the reset is performed before booting the kernel.

 - power-on reset
   (FFs are put into defined state on power-on automatically)

 - a reset controller assert reset lines by default
   (FFs are fixed to defined state.  If a reset consumer
   driver deasserts the reset line, HW starts from the define state)

 - Firmware may reset the hardware before booting the kernel


If nothing above is done, and the reset line is shared by multiple devices,
we do not have a good way to put the hardware into the sane state.


Reset consumers choose the required reset type:

- Shared reset:
    reset_control_assert / reset_control_deassert work like
clock_disable / clock_enable.
    The consumer must be tolerant to the situation where the HW may
not reset-asserted.

- Exclusive reset:
    It is guaranteed reset_control_assert() really asserts the reset line,
    but only one reset consumer grabs the reset line at the same time.


As above, the state machines in digital circuits must start from a
defined state.
So, we want exclusive reset to reset the FFs.
However, this is too much requirement
since it is a reset line is often shared.


How to save such an Amlogic case?


Hogging solve the problem?
--------------------------


I was thinking of a solution.

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


>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 something may be missing from my mind,
but I hope this will prompt the discussion,
and we will find a better idea.

Thanks.

-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

Hi.


The previous thread was:
"usb: dwc3: support clocks and resets for DWC3 core"
https://patchwork.kernel.org/patch/10349623/


I changed the subject because
I think this is rather reset-control topic than USB.


I am searching for a good way to initialize hardware devices
in the following situation:

 - two or more hardware devices share the same reset line

 - those devices are not reset before booting the kernel
   (i.e. flip flops in RTL are random state at driver probe)

 - the hardware IP is used by various SoCs,
   and this issue only appears only on some.



Specifically, the DWC3 USB IP case is in my mind,
but this should apply to any hardware in general.




Issue in detail
---------------

The DWC3 USB IP is very popular and used by various SoCs
as you see in drivers/usb/dwc3/.

SoCs are using the same RTL as provided by Synopsys.
(SoC vendors could tweak the RTL if they like,
but it would rarely happen because it would just be troublesome)

So, let's assume we use the same IP with the same RTL version.
It is *compatible* in terms of device tree.
In this case, we should avoid differentiating the driver code per-SoC.

In fact, we ended up with
commit ff0a632 ("usb: dwc3: of-simple: add support for shared and
pulsed reset lines")
commit e8284db ("usb: dwc3: of-simple: add support for the Amlogic
Meson GXL and AXG SoCs")


This means, the difference that comes from the reset _provider_
must be implemented in the reset _consumer_.
This is unfortunate.

So, I wonder if we can support it in sub-system level somehow
instead of messing up the reset consumer driver.

 - The reset topology is SoC-dependent.
   A reset line is dedicated for single hardware for some SoCs,
   and shared by multiple hardware for others.

 - The reset policy (pulse reset, level reset, or both of them)
   are SoC-dependent (reset controller dependent).



RTL generally contains state machines.
The initial state of flip flops are undefined on power-on
hence the initial state of state machines are also undefined.

So, digital circuits needs explicit reset in general.

In some cases, the reset is performed before booting the kernel.

 - power-on reset
   (FFs are put into defined state on power-on automatically)

 - a reset controller assert reset lines by default
   (FFs are fixed to defined state.  If a reset consumer
   driver deasserts the reset line, HW starts from the define state)

 - Firmware may reset the hardware before booting the kernel


If nothing above is done, and the reset line is shared by multiple devices,
we do not have a good way to put the hardware into the sane state.


Reset consumers choose the required reset type:

- Shared reset:
    reset_control_assert / reset_control_deassert work like
clock_disable / clock_enable.
    The consumer must be tolerant to the situation where the HW may
not reset-asserted.

- Exclusive reset:
    It is guaranteed reset_control_assert() really asserts the reset line,
    but only one reset consumer grabs the reset line at the same time.


As above, the state machines in digital circuits must start from a
defined state.
So, we want exclusive reset to reset the FFs.
However, this is too much requirement
since it is a reset line is often shared.


How to save such an Amlogic case?


Hogging solve the problem?
--------------------------


I was thinking of a solution.

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


>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 something may be missing from my mind,
but I hope this will prompt the discussion,
and we will find a better idea.

Thanks.

-- 
Best Regards
Masahiro Yamada

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

[Martin Blumenstingl]

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?
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)
- 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)

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


Regards
Martin

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

[Martin Blumenstingl]

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?
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)
- 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)

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


Regards
Martin

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

[Masahiro Yamada]

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.


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



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


> 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)


Thanks for your comment!


>
> Regards
> Martin
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

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.


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



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


> 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)


Thanks for your comment!


>
> Regards
> Martin
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

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

[Martin Blumenstingl]

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

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

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

>> 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)

> Thanks for your comment!
you're welcome - thank you for bringing up this topic also :)


Regards
Martin

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

[Martin Blumenstingl]

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

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

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

>> 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)

> Thanks for your comment!
you're welcome - thank you for bringing up this topic also :)


Regards
Martin

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

[Hans de Goede]

Hi,

On 21-05-18 03:27, Masahiro Yamada 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.
> 
> 
>> 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.
> 
> 
> 
>> - 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.
> 
> 
>> 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)

At least one some Allwinner ships shared-reset support accurately
models how the hardware works. It seems that the case you are
trying to fix is actually a special version of shared reset support,
you want shared reset behavior, combined with making sure the reset
is asserted at reset-controller-driver load time.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 21-05-18 03:27, Masahiro Yamada 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.
> 
> 
>> 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.
> 
> 
> 
>> - 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.
> 
> 
>> 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)

At least one some Allwinner ships shared-reset support accurately
models how the hardware works. It seems that the case you are
trying to fix is actually a special version of shared reset support,
you want shared reset behavior, combined with making sure the reset
is asserted at reset-controller-driver load time.

Regards,

Hans

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

[Philipp Zabel]

Hi Masahiro,

On Thu, 2018-05-10 at 18:16 +0900, Masahiro Yamada wrote:
> Hi.
> 
> 
> The previous thread was:
> "usb: dwc3: support clocks and resets for DWC3 core"
> https://patchwork.kernel.org/patch/10349623/
> 
> 
> I changed the subject because
> I think this is rather reset-control topic than USB.

Thank you for taking the time to write this up in such detail.

Indeed we were not expecting hardware that both defaults to deasserted
resets and does not have a power-on reset mechanism for the IP cores, so
yes, the framework currently assumes that all shared reset lines are
initially (or at some point in the past since power-on were) asserted.

I wonder if we are going to see this more often in the future, or
whether the Amlogic Meson SoCs will stay the exception.

> I am searching for a good way to initialize hardware devices
> in the following situation:
> 
>  - two or more hardware devices share the same reset line

So in general, at least during runtime, the driver must not expect reset
assertion to actually work, so the sharing device is not disturbed.

>  - those devices are not reset before booting the kernel
>    (i.e. flip flops in RTL are random state at driver probe)
> 
>  - the hardware IP is used by various SoCs,
>    and this issue only appears only on some.

Unfortunately for configurable IP cores like the DW ones I am not sure
to what extent these can actually be considered identical when
implemented in different SoCs.
I would expect that the hardware IP's basic requirements, like whether
the reset must be asserted at some defined point during operation, or
whether it has to be a pulse of da specific duration, does not change
between implementations.

> Specifically, the DWC3 USB IP case is in my mind,
> but this should apply to any hardware in general.
> 
> 
> 
> 
> Issue in detail
> ---------------
> 
> The DWC3 USB IP is very popular and used by various SoCs
> as you see in drivers/usb/dwc3/.
> 
> SoCs are using the same RTL as provided by Synopsys.
> (SoC vendors could tweak the RTL if they like,
> but it would rarely happen because it would just be troublesome)
> 
> So, let's assume we use the same IP with the same RTL version.
> It is *compatible* in terms of device tree.
> In this case, we should avoid differentiating the driver code per-SoC.
> 
> In fact, we ended up with
> commit ff0a632 ("usb: dwc3: of-simple: add support for shared and
> pulsed reset lines")
> commit e8284db ("usb: dwc3: of-simple: add support for the Amlogic
> Meson GXL and AXG SoCs")

I'm surprised, didn't all Meson SoCs gain level reset support for this
purpose?
I agree this should not be needed in the drivers, and I'd be very happy
if we could make this work on Meson with the dwc3 driver always
requesting shared resets.

> This means, the difference that comes from the reset _provider_
> must be implemented in the reset _consumer_.
> This is unfortunate.

I am not entirely happy about the shared vs. exclusive naming of the
API, but I have no better names. By choosing between the two, the
driver states its requirements of the API, not whether the SoC it is
implemented in actually shares the reset line with other devices.

> So, I wonder if we can support it in sub-system level somehow
> instead of messing up the reset consumer driver.

I'd like to deflect and, hoping that this is an exception, ask whether
we could just do this in the driver?

>  - The reset topology is SoC-dependent.
>    A reset line is dedicated for single hardware for some SoCs,
>    and shared by multiple hardware for others.

In which case the driver must be able to cope with shared reset lines,
so it should use the shared reset API.

>  - The reset policy (pulse reset, level reset, or both of them)
>    are SoC-dependent (reset controller dependent).

While that may be the case for some hardware IP, I don't think it is the
case for the dwc3 driver. It doesn't need to assert the reset at any
time during normal operation, and it works with level resets. The
pulsed/exclusive reset handling really is just a workaround for the
missing power-on reset.

> RTL generally contains state machines.
> The initial state of flip flops are undefined on power-on
> hence the initial state of state machines are also undefined.
> 
> So, digital circuits needs explicit reset in general.
> 
> In some cases, the reset is performed before booting the kernel.
> 
>  - power-on reset
>    (FFs are put into defined state on power-on automatically)
> 
>  - a reset controller assert reset lines by default
>    (FFs are fixed to defined state.  If a reset consumer
>    driver deasserts the reset line, HW starts from the define state)
> 
>  - Firmware may reset the hardware before booting the kernel
> 
> 
> If nothing above is done, and the reset line is shared by multiple devices,
> we do not have a good way to put the hardware into the sane state.
> 
> 
> Reset consumers choose the required reset type:
> 
> - Shared reset:
>     reset_control_assert / reset_control_deassert work like
> clock_disable / clock_enable.
>     The consumer must be tolerant to the situation where the HW may
> not reset-asserted.
> 
> - Exclusive reset:
>     It is guaranteed reset_control_assert() really asserts the reset line,
>     but only one reset consumer grabs the reset line at the same time.

Yes. The last part is a limitation of the current framework
implementation, but one that I'd like to keep as long as possible.

To support shared resets with guaranteed assertion, we'd need a
notification framework with a possibility for drivers to temporarily
suspend their own operations to allow a reset request from a different
device, or to veto reset requests if that is not possible. And drivers
that try to reset need to properly handle failed or deferred reset
requests. That would introduce a level of complexity that I'd very much
like to avoid.

> As above, the state machines in digital circuits must start from a
> defined state.
> So, we want exclusive reset to reset the FFs.
> However, this is too much requirement
> since it is a reset line is often shared.

For missing power-on resets, what we really want is not "exclusive
reset", and not even "guaranteed reset assertion". We want that after
the initial reset_deassert, the reset line is deasserted and the device
has been in reset *at any point in the past* since power-on.

> How to save such an Amlogic case?

I think this case can be solved by either just initializing the reset
lines to asserted in the reset controller driver (in which case the
controller driver needs to know about which lines have to be asserted,
and needs to support assert/deassert ops) or by adding a fallback from
.deassert to .reset for reset controllers that don't have level reset
hardware support. That is, if the first driver calls
reset_control_deassert, the reset controller driver actually issues a
reset pulse.

> Hogging solve the problem?
> --------------------------
> 
> 
> I was thinking of a solution.
> 
> 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).
> 
> 
> 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 something may be missing from my mind,
> but I hope this will prompt the discussion,
> and we will find a better idea.

My question would is: do we need this complexity, or can this be solved
by just adding a workaround in the meson driver? If a lot of SoCs using
the simple reset driver had this requirement, I'd seriously consider
this, but it currently seems to me that this is just an issue with a
single SoC family.

regards
Philipp

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

[Philipp Zabel]

Hi Masahiro,

On Thu, 2018-05-10 at 18:16 +0900, Masahiro Yamada wrote:
> Hi.
> 
> 
> The previous thread was:
> "usb: dwc3: support clocks and resets for DWC3 core"
> https://patchwork.kernel.org/patch/10349623/
> 
> 
> I changed the subject because
> I think this is rather reset-control topic than USB.

Thank you for taking the time to write this up in such detail.

Indeed we were not expecting hardware that both defaults to deasserted
resets and does not have a power-on reset mechanism for the IP cores, so
yes, the framework currently assumes that all shared reset lines are
initially (or at some point in the past since power-on were) asserted.

I wonder if we are going to see this more often in the future, or
whether the Amlogic Meson SoCs will stay the exception.

> I am searching for a good way to initialize hardware devices
> in the following situation:
> 
>  - two or more hardware devices share the same reset line

So in general, at least during runtime, the driver must not expect reset
assertion to actually work, so the sharing device is not disturbed.

>  - those devices are not reset before booting the kernel
>    (i.e. flip flops in RTL are random state at driver probe)
> 
>  - the hardware IP is used by various SoCs,
>    and this issue only appears only on some.

Unfortunately for configurable IP cores like the DW ones I am not sure
to what extent these can actually be considered identical when
implemented in different SoCs.
I would expect that the hardware IP's basic requirements, like whether
the reset must be asserted at some defined point during operation, or
whether it has to be a pulse of da specific duration, does not change
between implementations.

> Specifically, the DWC3 USB IP case is in my mind,
> but this should apply to any hardware in general.
> 
> 
> 
> 
> Issue in detail
> ---------------
> 
> The DWC3 USB IP is very popular and used by various SoCs
> as you see in drivers/usb/dwc3/.
> 
> SoCs are using the same RTL as provided by Synopsys.
> (SoC vendors could tweak the RTL if they like,
> but it would rarely happen because it would just be troublesome)
> 
> So, let's assume we use the same IP with the same RTL version.
> It is *compatible* in terms of device tree.
> In this case, we should avoid differentiating the driver code per-SoC.
> 
> In fact, we ended up with
> commit ff0a632 ("usb: dwc3: of-simple: add support for shared and
> pulsed reset lines")
> commit e8284db ("usb: dwc3: of-simple: add support for the Amlogic
> Meson GXL and AXG SoCs")

I'm surprised, didn't all Meson SoCs gain level reset support for this
purpose?
I agree this should not be needed in the drivers, and I'd be very happy
if we could make this work on Meson with the dwc3 driver always
requesting shared resets.

> This means, the difference that comes from the reset _provider_
> must be implemented in the reset _consumer_.
> This is unfortunate.

I am not entirely happy about the shared vs. exclusive naming of the
API, but I have no better names. By choosing between the two, the
driver?states its requirements of the API, not whether the SoC it is
implemented in actually shares the reset line with other devices.

> So, I wonder if we can support it in sub-system level somehow
> instead of messing up the reset consumer driver.

I'd like to deflect and, hoping that this is an exception, ask whether
we could just do this in the driver?

>  - The reset topology is SoC-dependent.
>    A reset line is dedicated for single hardware for some SoCs,
>    and shared by multiple hardware for others.

In which case the driver must be able to cope with shared reset lines,
so it should use the shared reset API.

>  - The reset policy (pulse reset, level reset, or both of them)
>    are SoC-dependent (reset controller dependent).

While that may be the case for some hardware IP, I don't think it is the
case for the dwc3 driver. It doesn't need to assert the reset at any
time during normal operation, and it works with level resets. The
pulsed/exclusive reset handling really is just a workaround for the
missing power-on reset.

> RTL generally contains state machines.
> The initial state of flip flops are undefined on power-on
> hence the initial state of state machines are also undefined.
> 
> So, digital circuits needs explicit reset in general.
> 
> In some cases, the reset is performed before booting the kernel.
> 
>  - power-on reset
>    (FFs are put into defined state on power-on automatically)
> 
>  - a reset controller assert reset lines by default
>    (FFs are fixed to defined state.  If a reset consumer
>    driver deasserts the reset line, HW starts from the define state)
> 
>  - Firmware may reset the hardware before booting the kernel
> 
> 
> If nothing above is done, and the reset line is shared by multiple devices,
> we do not have a good way to put the hardware into the sane state.
> 
> 
> Reset consumers choose the required reset type:
> 
> - Shared reset:
>     reset_control_assert / reset_control_deassert work like
> clock_disable / clock_enable.
>     The consumer must be tolerant to the situation where the HW may
> not reset-asserted.
> 
> - Exclusive reset:
>     It is guaranteed reset_control_assert() really asserts the reset line,
>     but only one reset consumer grabs the reset line at the same time.

Yes. The last part is a limitation of the current framework
implementation, but one that I'd like to keep as long as possible.

To support shared resets with guaranteed assertion, we'd need a
notification framework with a possibility for drivers to temporarily
suspend their own operations to allow a reset request from a different
device, or to veto reset requests if that is not possible. And drivers
that try to reset need to properly handle failed or deferred reset
requests. That would introduce a level of complexity that I'd very much
like to avoid.

> As above, the state machines in digital circuits must start from a
> defined state.
> So, we want exclusive reset to reset the FFs.
> However, this is too much requirement
> since it is a reset line is often shared.

For missing power-on resets, what we really want is not "exclusive
reset", and not even "guaranteed reset assertion". We want that after
the initial reset_deassert, the reset line is deasserted and the device
has been in reset *at any point in the past* since power-on.

> How to save such an Amlogic case?

I think this case can be solved by either just initializing the reset
lines to asserted in the reset controller driver (in which case the
controller driver needs to know about which lines have to be asserted,
and needs to support assert/deassert ops) or by adding a fallback from
.deassert to .reset for reset controllers that don't have level reset
hardware support. That is, if the first driver calls
reset_control_deassert, the reset controller driver actually issues a
reset pulse.

> Hogging solve the problem?
> --------------------------
> 
> 
> I was thinking of a solution.
> 
> 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).
> 
> 
> 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 something may be missing from my mind,
> but I hope this will prompt the discussion,
> and we will find a better idea.

My question would is: do we need this complexity, or can this be solved
by just adding a workaround in the meson driver? If a lot of SoCs using
the simple reset driver had this requirement, I'd seriously consider
this, but it currently seems to me that this is just an issue with a
single SoC family.

regards
Philipp

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

[Philipp Zabel]

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?

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

regards
Philipp

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

[Philipp Zabel]

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?

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

regards
Philipp

Re: [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?

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

[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

Re: [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?

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

[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

Re: [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

[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

[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