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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 9/27/21 11:00 AM, Andy Shevchenko wrote:
> +Cc: Hans (just for a bit offtopic comment below)
> 
> 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:
> 
> ...
> 
>>>> I'd prefer to have a single compatible and get the npins from some
>>>> property and I don't think that's necessarily over-generalizing.
>>>> AFAICT Apple has been using the exact same MMIO interface for years
>>>> and I'd expect them to continue using it in the future. The only thing
>>>> that seems to change is the number of pins available and their assignment.
>>>> If we just have a single compatible we can likely support the M1X/2 or
>>>> however Apple calls the next SoCs with just a simple DTB change without
>>>> touching any driver code.
>>>
>>> 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.

Regards,

Hans