Re: [PATCH v1 1/1] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Sep 27, 2021 at 12:27 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> On 9/27/21 11:00 AM, Andy Shevchenko wrote:
> > On Mon, Sep 27, 2021 at 8:46 AM Sven Peter <sven@xxxxxxxxxxxxx> wrote:
> >> On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:
> >>> On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@xxxxxxxxxxxxx> wrote:
> >>>> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
> >>>>> On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@xxxxxxxxxxxxx> wrote:
> >>>>>> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:

...

> >>> Hmm... Dunno the details, but at least AOP GPIO is definitely ca[able
> >>> of waking a system from a deep sleep (that's what SUS == suspend do on
> >>> Intel). Haven't checked if you implemented ->irq_set_wake() callbacks,
> >>> though.
> >>
> >> I don't think Joey implemented the set_wake callback because we didn't
> >> even consider that the AOP GPIOs might be able to wake the system from
> >> deep sleep. I'll see if I can figure out some details about that though.
> >
> > I have checked Intel drivers and above mentioned do not implement
> > ->irq_set_wake() callback. Hmm... Maybe Hans can share his thoughts
> > why it's so
> > (note, the Skylake and newest are all based on pinctrl-intel.c which
> > implements it. So does Merrifield) and if we also need to consider
> > adding it.
>
> Bay Trail and Cherry Trail always use suspend2idle, which means any
> IRQ is a wake IRQ, since the CPU is in a S0ix (deep-idle, rather
> then full suspended) state.
>
> Drivers still need to make irq_set_irq_wake() calls
> though, to avoid the IRQ code disabling the IRQ on suspend.
>
> To allow those calls to succeed the baytrail and cherryview
> pinctrl drivers set IRQCHIP_SKIP_SET_WAKE in their irqchip.flags.
>
> There are also some more standard (non tablet targetting)
> CPUs which are using the same GPIO IP block, e.g.
> the Celeron N2840 uses the pinctrl-baytrail.c code but
> laptops using this will typically use normal S3 suspend.
>
> I assume that in this case configuring which IRQs are wakeup
> sources is actually controlled the BIOS, since S3 suspend is
> heavily BIOS assisted. I would not even be surprised if the
> BIOS even completely reprograms all IRQ settings (including
> IRQs left enabled) on suspend.

Thank you, Hans!
I will try not to forget this, but it makes me wonder if we may
collect this kind of information in some README-like file inside the
source code.

-- 
With Best Regards,
Andy Shevchenko