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

["Sven Peter" <sven@xxxxxxxxxxxxx>]

Hi Andy,

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 think npins should be known from the compatible (we know that
>> >> > this version of the SoC has so and so many pins) and the base
>> >> > should always be 0? It's not like we have several pin controllers
>> >> > of this type in the SoC is it?
>> >>
>> >> I've just checked: Looks like there are four different pin controllers of
>> >> this type with a different number of pins each in Apple's device tree for
>> >> the M1.
>> >
>> > So we need to understand this hardware: is this something like
>> > south/north/east/west, so the pins are oriented around the chip?
>> >
>> > I would suspect they should then be compatibles:
>> > apple,t8103-pinctrl-north, apple,t8103-pinctrl
>> > apple,t8103-pinctrl-south, apple,t8103-pinctrl
>> > apple,t8103-pinctrl-west, apple,t8103-pinctrl
>> > apple,t8103-pinctrl-east, apple,t8103-pinctrl
>> >
>> > going from specific to general signifying that we know which one
>> > we are dealing with and then we know the npins etc.
>>
>> Apple calls those four controllers "gpio", "nub-gpio", "smc-gpio"
>> and "aop-gpio". SMC is their system management controller and AOP
>> is their "always-on processor". No idea what "nub-gpio" is.
>
> It's similar to what we have in Baytrail/Cherrytrail.
> AOP -> SUS
> SMC -> ...

Interesting! I'll take a look at those.
 
>
> nub is probably related to some type of hub (or maybe simple typo, or
> typo on purpose?).
>
>> > This also gives a normal grouping of functions associated with
>> > pins and the portion of the SoC, see for
>> > example drivers/pinctrl/intel/pinctrl-broxton.c.
>> >
>> > This shows that it might have been a bad idea to define the
>> > pins as opaque, because now that is hiding the fact that
>> > these pins are grouped in four distinct sets.
>> > APPLE_PINMUX(pin, func)
>> >
>> > Should rather have been APPLE_PINMUX(cardinal, pin, func)
>> > where cardinal would be 0..3 for north, south, west, east.
>> >
>> > This is a bit of guessing actually, but we should definitely
>> > try to make the driver reflect the reality and not over-generalize.
>> > If these four blocks in the SoC are different, they should have
>> > different compatible strings, because they are not, by
>> > definition, compatible.
>>
>> 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.

>
> And I don't know if the pin's in the rest of the GPIO blocks have the
> wake-source capable pins. Also I don't know if it's fine to have one
> compatible string if you really know that the difference is the amount
> of pins and nothing else (like crucial properties being changed).

Yeah, I don't know either. Another thing we could do is have the base
compatible just support the maximum number of pins supported by the MMIO
space and only limit that (and possibly add wake-capable pins or other
different properties if there are any) for the more specific ones.


Best,


Sven