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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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 -> ...

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.

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).

-- 
With Best Regards,
Andy Shevchenko