LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Rob Herring <robh@kernel.org>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Brian Norris <computersforpeace@gmail.com>,
	Gregory Fong <gregory.0xf0@gmail.com>,
	"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
	<bcm-kernel-feedback-list@broadcom.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller
Date: Thu, 3 Jan 2019 16:54:20 -0600
Message-ID: <CAL_JsqKpFYnnAY-feAeJ-Va5yS7JH34FOzvBmTi4gMV=kjbCVQ@mail.gmail.com> (raw)
In-Reply-To: <bcfe4df0-8928-90e2-6460-1391320af9be@gmail.com>

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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-21  1:34 [PATCH 0/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
2018-12-21  1:34 ` [PATCH 1/2] dt-bindings: reset: Add document for Broadcom STB reset controller Florian Fainelli
2018-12-31 23:13   ` Scott Branden
2019-01-02 20:38     ` Florian Fainelli
2019-01-03 17:41   ` Rob Herring
2019-01-03 18:53     ` Florian Fainelli
2019-01-03 19:19       ` Rob Herring
2019-01-03 19:31         ` Florian Fainelli
2019-01-03 22:54           ` Rob Herring [this message]
2019-01-11 18:51             ` Florian Fainelli
2018-12-21  1:34 ` [PATCH 2/2] reset: Add Broadcom STB SW_INIT reset controller driver Florian Fainelli
2019-01-02 10:44   ` Philipp Zabel
2019-01-02 20:38     ` Florian Fainelli

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL_JsqKpFYnnAY-feAeJ-Va5yS7JH34FOzvBmTi4gMV=kjbCVQ@mail.gmail.com' \
    --to=robh@kernel.org \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=computersforpeace@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=gregory.0xf0@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=p.zabel@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox