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@xxxxxxxxxxxxxx>:
> Hi Philipp,
>
> On Tue, May 22, 2018 at 4:04 PM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
>>> > Hi.
>>> >
>>> >
>>> > 2018-05-20 19:57 GMT+09:00 Martin Blumenstingl
>>> > <martin.blumenstingl@xxxxxxxxxxxxxx>:
>>> > > Hi,
>>> > >
>>> > > On Thu, May 10, 2018 at 11:16 AM, Masahiro Yamada
>>> > > <yamada.masahiro@xxxxxxxxxxxxx> 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