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

From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>,
	Sven Peter <sven@svenpeter.dev>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Joey Gouly <joey.gouly@arm.com>,
	Andy Shevchenko <andriy.shevchenko@intel.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Hector Martin <marcan@marcan.st>, Marc Zyngier <maz@kernel.org>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	nd <nd@arm.com>, Stan Skowronek <stan@corellium.com>
Subject: Re: [PATCH v1 1/1] pinctrl: add pinctrl/GPIO driver for Apple SoCs
Date: Mon, 27 Sep 2021 11:27:01 +0200	[thread overview]
Message-ID: <7f8e0480-c6af-8a09-86d6-6234429ffa68@redhat.com> (raw)
In-Reply-To: <CAHp75Vc+uA-G2+aT_U10BDo8krhahYgTSWdxQt8iffn9angL_w@mail.gmail.com>

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@svenpeter.dev> wrote:
>> On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:
>>> On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@svenpeter.dev> wrote:
>>>> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
>>>>> On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> 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