Re: [PATCH 1/2] pinctrl: qcom: msm8226: Add MPM pin mappings

[Stephan Gerhold <stephan@gerhold.net>]

On Sat, Sep 23, 2023 at 11:32:47AM +0200, Luca Weiss wrote:
> Hi Matti,
> 
> On Samstag, 23. September 2023 00:40:26 CEST Matti Lehtimäki wrote:
> > Add pin <-> wakeirq mappings to allow for waking up the AP from sleep
> > through MPM-connected pins.
> > 
> > Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
> > ---
> >  drivers/pinctrl/qcom/pinctrl-msm8226.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > b/drivers/pinctrl/qcom/pinctrl-msm8226.c index 994619840a70..1e46a9ab382f
> > 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm8226.c
> > @@ -612,6 +612,16 @@ static const struct msm_pingroup msm8226_groups[] = {
> > 
> >  #define NUM_GPIO_PINGROUPS 117
> > 
> > +static const struct msm_gpio_wakeirq_map msm8226_mpm_map[] = {
> > +	{ 1, 3 }, { 4, 4 }, { 5, 5 }, { 9, 6 }, { 13, 7 }, { 17, 8 },
> 
> I'm not really convinced this is the correct order of values...
> 
> Let's look at downstream:
> 
>   qcom,gpio-map = <3  1>,
>                   <4  4 >,
>                   <5  5 >,
>                   <6  9 >,
>                   [...]
> 
> From Documentation/devicetree/bindings/arm/msm/mpm.txt downstream:
> 
>   Each tuple represents a MPM pin and which GIC interrupt is routed to it.
> 
> So first is pin number, second is interrupt number.
> 
> And check mainline: 
> 
>   /**
>    * struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
>    * @gpio:          The GPIOs that are wakeup capable
>    * @wakeirq:       The interrupt at the always-on interrupt controller
>    */
>   struct msm_gpio_wakeirq_map {
>   	unsigned int gpio;
>   	unsigned int wakeirq;
>   };
> 
> So here we also have the order pin-interrupt, not the reverse order.
> 
> Therefore I believe the order in this patch is incorrect, and it should rather 
> be:
> 
>   { 3, 1 }, { 4, 4 }, { 5, 5 }, { 6, 9 }, { 7, 13 }, { 8, 17 },
>   [...]
> 
> Or do you think I'm missing something?
> 

Yes :)

Let's look at the later entries:

> > +	{ 21, 9 }, { 27, 10 }, { 29, 11 }, { 31, 12 }, { 33, 13 }, { 35, 14 
> },
> > +	{ 37, 15 }, { 38, 16 }, { 39, 17 }, { 41, 18 }, { 46, 19 }, { 48, 20 
> },
> > +	{ 49, 21 }, { 50, 22 }, { 51, 23 }, { 52, 24 }, { 54, 25 }, { 62, 26 
> },
> > +	{ 63, 27 }, { 64, 28 }, { 65, 29 }, { 66, 30 }, { 67, 31 }, { 68, 32 
> },
> > +	{ 69, 33 }, { 71, 34 }, { 72, 35 }, { 106, 36 }, { 107, 37 },
> > +	{ 108, 38 }, { 109, 39 }, { 110, 40 }, { 111, 54 }, { 113, 55 },
> > +};
> > +

For example: { 113, 55 }, i.e. { .gpio = 113, .wakeirq = 55 }.

MSM8226 has GPIOs 0-116 and 64 MPM pins/interrupts. The order in this
patch is the only one that can be correct because the definition would
be invalid the other way around. 113 must be the GPIO number because it
is larger than the 64 available MPM interrupt pins. :)

Thanks,
Stephan