Hi Andy,

On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:

On Sun, Sep 26, 2021 at 5:36 PM Sven Peter [off-list ref] wrote:

quoted

On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:

quoted

On Sun, Sep 26, 2021 at 2:56 PM Sven Peter [off-list ref] wrote:

quoted

On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:

quoted

quoted

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

quoted

quoted

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