Re: [PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support

From: Thierry Reding <thierry.reding@gmail.com>
To: Rob Herring <robh@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	devicetree@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support
Date: Wed, 28 Nov 2018 11:44:36 +0100	[thread overview]
Message-ID: <20181128104436.GA22559@ulmo> (raw)
In-Reply-To: <20181015144612.GA23929@bogus>

[-- Attachment #1: Type: text/plain, Size: 3217 bytes --]

On Mon, Oct 15, 2018 at 09:46:12AM -0500, Rob Herring wrote:
> On Fri, Sep 21, 2018 at 12:25:44PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Tegra186 and later have some top-level controls for wake events in the
> > power management controller (PMC). In order to enable the system to wake
> > up from low power states, additional registers in the PMC need to be
> > programmed. Add a wakeup-parent property to establish this relationship
> > between the GPIO controller and the PMC.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../devicetree/bindings/gpio/nvidia,tegra186-gpio.txt      | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> > index adff16c71d21..cbb51a8990c3 100644
> > --- a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> > @@ -127,6 +127,11 @@ Required properties:
> >              - 8: Active low level-sensitive.
> >              Valid combinations are 1, 2, 3, 4, 8.
> >  
> > +Optional properties:
> > +- wake-parent
> > +    A phandle to the Power Management Controller (PMC) that contains top-
> > +    level controls to enable the wake-up capabilities of some GPIOs.
> > +
> >  Example:
> >  
> >  #include <dt-bindings/interrupt-controller/irq.h>
> > @@ -148,6 +153,7 @@ gpio@2200000 {
> >  	#gpio-cells = <2>;
> >  	interrupt-controller;
> >  	#interrupt-cells = <2>;
> > +	wakeup-parent = <&pmc>;
> >  };
> >  
> >  gpio@c2f0000 {
> > @@ -162,4 +168,5 @@ gpio@c2f0000 {
> >  	#gpio-cells = <2>;
> >  	interrupt-controller;
> >  	#interrupt-cells = <2>;
> > +	wakeup-parent = <&pmc>;
> 
> If all the GPIO instances point to the same PMC and have no per instance 
> data, why do you need this in DT? You can just search for the compatible 
> node.

That would be slightly annoying to do. I mean, we'd have to somehow
construct the compatible string that we're looking for. I guess we could
get around this mostly by just looking for a device matching one of the
entries in a "pmc_gpio_of_match" table. That would potentially match a
Tegra194 GPIO against a Tegra186 PMC, but that should never happen in
practice because it'd be a bug in the DT.

Although that somewhat depends on exactly what we mean by "compatible".
Technically the Tegra194 PMC is compatible with the Tegra186 PMC in
terms of register layout and so on. However, since both chips have
undergone quite some changes with regards to the pins they expose, the
set of wake events exposed on Tegra186 and Tegra194 varies wildly. So if
that means that the compatible needs to be different (I think it should)
then we could go with the OF match table approach.

Actually, thinking about it some more, even if we had a Tegra186
fallback compatible string in the Tegra194 PMC's device tree node, we'd
still be matching on the correct instance, and therefore get the right
IRQ domain for the hierarchy.

I'll rework the patches accordingly.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]