Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller

[Rob Herring <robh@kernel.org>]

On Thu, Jan 3, 2019 at 1:31 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 1/3/19 11:19 AM, Rob Herring wrote:
> > On Thu, Jan 03, 2019 at 10:53:25AM -0800, Florian Fainelli wrote:
> >> On 1/3/19 9:41 AM, Rob Herring wrote:
> >>> On Thu, Dec 20, 2018 at 05:34:08PM -0800, Florian Fainelli wrote:
> >>>> Add a binding document for the Broadcom STB reset controller, also known
> >>>> as SW_INIT-style reset controller.
> >>>>
> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >>>> ---
> >>>>  .../devicetree/bindings/reset/brcm,reset.txt  | 27 +++++++++++++++++++
> >>>>  1 file changed, 27 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/reset/brcm,reset.txt b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>>> new file mode 100644
> >>>> index 000000000000..6e5341b4f891
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/reset/brcm,reset.txt
> >>>> @@ -0,0 +1,27 @@
> >>>> +Broadcom STB SW_INIT-style reset controller
> >>>> +===========================================
> >>>> +
> >>>> +Broadcom STB SoCs have a SW_INIT-style reset controller with separate
> >>>> +SET/CLEAR/STATUS registers and possibly multiple banks, each of 32 bit
> >>>> +reset lines.
> >>>> +
> >>>> +Please also refer to reset.txt in this directory for common reset
> >>>> +controller binding usage.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: should be brcm,brcmstb-reset
> >>>> +- reg: register base and length
> >>>> +- #reset-cells: must be set to 1
> >>>> +
> >>>> +Example:
> >>>> +
> >>>> +  reset: reset-controller@8404318 {
> >>>> +          compatible = "brcm,brcmstb-reset";
> >>>> +          reg = <0x8404318 0x30>;
> >>>
> >>> Based on this address, should this be a sub-node of something else? Or
> >>> not even a sub-node and just make the parent be a reset provider?
> >>
> >> The reset controller is part of a larger "sundry" node which has a
> >> collection of functionality, from pinmux/pinctrl, reset controller,
> >> spare bits, chicken bits, anything the designers forgot to put somewhere
> >> else and decided to put there.
> >>
> >> If there is one thing consistent though is that given a set of 32-bit
> >> register groups, they have a self contained functionality such that you
> >> can break up the larger "sundry" space into smaller sub-blocks which
> >> have one an only one functionality. Do you think this warrants a
> >> different representation in Device Tree?
> >
> > With pinctrl in the mix, you're going to need sub-nodes anyways. So just
> > define what this is a sub-node of.
>
> pinctrl is not necessarily something I want the kernel to control, since
> we have a high level scripting language without our bootloader that
> makes sure pinctrl is properly configured from early boot on all the way
> to the kernel, and preserved across suspend/resume states.
> pinctrl-single does work, and was occasionally used. Everything else is
> typically muxes that the kernel does not need to touch/see/be aware of.

That's good. I'd rather see more platforms do that rather than have
the kernel handle it. OTOH, bootloaders often use DT too, so maybe who
handles pin muxing is irrelevant.

> > Also, I'd prefer to have complete example for the "sundry" node and
> > child nodes than partial examples spread across the tree.
>
> I am afraid I can't provide that example because the sundry node is
> really changing from chip to chip, and there is a gazillion of things in
> there that the kernel typically does not even touch, like
> pinmuxing/pinctrl, various mux selections etc. I could provide the
> following example if that is what you are requesting?:
>
>
> sun-top-ctrl: simple-bus@8404000 {
>         compatible = "brcm,brcmstb-sun-top-ctrl", "simple-bus";
>         reg = <0x8404000 0x708>;
>
>         reset: reset-controller@318 {
>                 compatible = "brcm,brcmstb-reset";
>                 reg = <0x318 0x30>;
>                 #reset-cells = <1>;
>         };
> };
>
> Would that be what you expect to see?

The problem is with this alone, you should just move #reset-cells to
the parent and remove the child node. That's all you really need from
a DT perspective. But if this is really a separate block that's reused
from chip to chip, then a separate node is fine. Typically in these
situations, I just can't tell whether it's that or just the
convenience of creating nodes for every kernel driver.

Rob