Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Rob Herring]

On Wed, Jan 9, 2019 at 11:31 AM Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
>
> On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote:
> >On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
> >>
> >> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote:
> >> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
> >> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
> >> >> routed to the PDC as interrupts that can be used to wake the system up
> >> >> from deep low power modes and suspend.
> >> >>
> >> >> Cc: devicetree@xxxxxxxxxxxxxxx
> >> >> Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> >> >> ---
> >> >>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> >> index 665aadb5ea28..a522ca46667d 100644
> >> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> >> @@ -29,6 +29,11 @@ SDM845 platform.
> >> >>      Definition: must be 2. Specifying the pin number and flags, as defined
> >> >>                  in <dt-bindings/interrupt-controller/irq.h>
> >> >>
> >> >> +- wakeup-parent:
> >> >> +    Usage: optional
> >> >> +    Value type: <phandle>
> >> >> +    Definition: A phandle to the wakeup interrupt controller for the SoC.
> >> >
> >> >Is this really necessary? Is there more than one possible wakeup-parent
> >> >node?
> >> >
> >> No.  There is only one but depending on the architecture, the wakeup
> >> interrupt controller could be different device like PDC on SDM845 or MPM
> >> on SDM820.
> >>
> >> What do you have in mind? Let me know if you have a better idea than
> >> referencing in DT.
> >
> >If there's only one possibility for a given platform, then you can
> >just use of_find_compatible_node(). I don't think it matters that
> >different platforms have a different device here. It's not going to be
> >a large table and you may need to know the differences if there's not
> >an abstracted interface to it (seems there is in your case).
> The GPIO irqchip would be in hierarchy with the wakeup-parent
> irqchip and no device specific functions would be called directly.
> We could achieve this with compatible strings to the irqchip.
>
> >Alternatively, if the PDC/MPM code knows what interrupt controller it
> >is associated with, then it could setup that relationship and the
> >interrupt controller code could retrieve that. Maybe the stacked
> >domain support doesn't work in that direction (I haven't looked at the
> >irq code much since that was added).
> >
> The PDC/MPM do not know about the association.

Neither does the main interrupt controller. The association is part of
SoC integration. You can describe that association in either direction
and that is sufficient from a DT standpoint. You've probably picked
putting this in the GIC(?) based on what works more easily with the
Linux irqdomain code.

> >However, my main concern is documenting something genericish in a
> >device specific binding. It looks like Tegra is trying to add the same
> >thing, so this needs to be documented in a common place. One question
> >is whether wakeup is the only use or if this should be more generally
> >a secondary interrupt parent?
> >
> Yes, wakeup is the only use of this interrupt parent.

Maybe for you, but I was wondering about this more generally. Should
we encode what the function (e.g. wakeup) is in the property name or
have something like aux-interrupt-controller? Maybe some platforms
have some need for a secondary interrupt-controller which is not
wakeup. Routing interrupts to other cores perhaps?

> It is powered by
> an always-on rail and therefore can detect some interrupts that are
> routed to it even when the GIC is powered off. Though Tegra's
> implementation of the irqchip is a bit different from QCOM, the idea is
> generally the same. It would be helpful, if we could make this a
> generic enough binding.
>
> -- Lina
>