[PATCH 0/3] pinctrl: msm interrupt and muxing fixes

[PATCH 0/3] pinctrl: msm interrupt and muxing fixes

[Stephen Boyd]

Here's a collection of pinctrl fixes for the qcom driver that
make things a little smoother for DT writers while also fixing
a problem seen with level triggered interrupts.

The first patch fixes an issue where we always see one extra level
triggered interrupt when the interrupt triggers. The second and third
patches make things nice for DT writers so they don't have to explicitly
mux out pins as 'GPIO' function and as 'input' instead of output so that
interrupts work properly and also makes sure that a gpio is muxed out
properly to 'GPIO' function when a gpio is requested by gpiod_request()
and friends.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Doug Anderson <dianders@chromium.org>

Stephen Boyd (3):
  pinctrl: msm: Really mask level interrupts to prevent latching
  pinctrl: msm: Mux out gpio function with gpio_request()
  pinctrl: msm: Configure interrupts as input and gpio mode

 drivers/pinctrl/qcom/pinctrl-msm.c | 70 ++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

-- 
Sent by a computer through tubes

[PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Stephen Boyd]

The interrupt controller hardware in this pin controller has two status
enable bits. The first "normal" status enable bit enables or disables
the summary interrupt line being raised when a gpio interrupt triggers
and the "raw" status enable bit allows or prevents the hardware from
latching an interrupt into the status register for a gpio interrupt.
Currently we just toggle the "normal" status enable bit in the mask and
unmask ops so that the summary irq interrupt going to the CPU's
interrupt controller doesn't trigger for the masked gpio interrupt.

For a level triggered interrupt, the flow would be as follows: the pin
controller sees the interrupt, latches the status into the status
register, raises the summary irq to the CPU, summary irq handler runs
and calls handle_level_irq(), handle_level_irq() masks and acks the gpio
interrupt, the interrupt handler runs, and finally unmask the interrupt.
When the interrupt handler completes, we expect that the interrupt line
level will go back to the deasserted state so the genirq code can unmask
the interrupt without it triggering again.

If we only mask the interrupt by clearing the "normal" status enable bit
then we'll ack the interrupt but it will continue to show up as pending
in the status register because the raw status bit is enabled, the
hardware hasn't deasserted the line, and thus the asserted state latches
into the status register again. When the hardware deasserts the
interrupt the pin controller still thinks there is a pending unserviced
level interrupt because it latched it earlier. This behavior causes
software to see an extra interrupt for level type interrupts each time
the interrupt is handled.

Let's fix this by clearing the raw status enable bit for level type
interrupts so that the hardware stops latching the status of the
interrupt after we ack it. We don't do this for edge type interrupts
because it seems that toggling the raw status enable bit for edge type
interrupts causes spurious edge interrupts.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 0e22f52b2a19..3563c4394837 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -626,6 +626,19 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = readl(pctrl->regs + g->intr_cfg_reg);
+	/*
+	 * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
+	 * are still asserted to re-latch after we ack them. Clear the raw
+	 * status enable bit too so the interrupt can't even latch into the
+	 * hardware while it's masked, but only do this for level interrupts
+	 * because edge interrupts have a problem with the raw status bit
+	 * toggling and causing spurious interrupts.
+	 */
+	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
+		val &= ~BIT(g->intr_raw_status_bit);
+		writel(val, pctrl->regs + g->intr_cfg_reg);
+	}
+
 	val &= ~BIT(g->intr_enable_bit);
 	writel(val, pctrl->regs + g->intr_cfg_reg);
 
@@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = readl(pctrl->regs + g->intr_cfg_reg);
+	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
+		val |= BIT(g->intr_raw_status_bit);
+		writel(val, pctrl->regs + g->intr_cfg_reg);
+	}
 	val |= BIT(g->intr_enable_bit);
 	writel(val, pctrl->regs + g->intr_cfg_reg);
 
-- 
Sent by a computer through tubes

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Doug Anderson]

Hi,

On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> The interrupt controller hardware in this pin controller has two status
> enable bits. The first "normal" status enable bit enables or disables
> the summary interrupt line being raised when a gpio interrupt triggers
> and the "raw" status enable bit allows or prevents the hardware from
> latching an interrupt into the status register for a gpio interrupt.
> Currently we just toggle the "normal" status enable bit in the mask and
> unmask ops so that the summary irq interrupt going to the CPU's
> interrupt controller doesn't trigger for the masked gpio interrupt.
>
> For a level triggered interrupt, the flow would be as follows: the pin
> controller sees the interrupt, latches the status into the status
> register, raises the summary irq to the CPU, summary irq handler runs
> and calls handle_level_irq(), handle_level_irq() masks and acks the gpio
> interrupt, the interrupt handler runs, and finally unmask the interrupt.
> When the interrupt handler completes, we expect that the interrupt line
> level will go back to the deasserted state so the genirq code can unmask
> the interrupt without it triggering again.
>
> If we only mask the interrupt by clearing the "normal" status enable bit
> then we'll ack the interrupt but it will continue to show up as pending
> in the status register because the raw status bit is enabled, the
> hardware hasn't deasserted the line, and thus the asserted state latches
> into the status register again. When the hardware deasserts the
> interrupt the pin controller still thinks there is a pending unserviced
> level interrupt because it latched it earlier. This behavior causes
> software to see an extra interrupt for level type interrupts each time
> the interrupt is handled.
>
> Let's fix this by clearing the raw status enable bit for level type
> interrupts so that the hardware stops latching the status of the
> interrupt after we ack it. We don't do this for edge type interrupts
> because it seems that toggling the raw status enable bit for edge type
> interrupts causes spurious edge interrupts.
>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..3563c4394837 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -626,6 +626,19 @@ static void msm_gpio_irq_mask(struct irq_data *d)
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>         val = readl(pctrl->regs + g->intr_cfg_reg);
> +       /*
> +        * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
> +        * are still asserted to re-latch after we ack them. Clear the raw

As I understand it acking a level-triggered interrupt is not totally
sensible anyway. IMO the fundamental "brokenness" of the MSM pin
controller is that it's latching level-triggered interrupts in the
first place when it shouldn't be. Instead of latching level triggered
interrupts (and requiring an Ack), the controller should simply stop
asserting its interrupt when the level goes away (if you want, you can
think of this in your mind as the controller auto-acking its own
interrupt when the level goes away).

The reason things aren't more broken than they are is that the Linux
interrupt core still Acks level-triggered interrupts (it just does it
at the beginning instead of the end).  IMO this is not fundamentally
something that the Linux interrupt core needs to do, but it does it
anyway (maybe there are a bunch of interrupt controllers that still
need this even though it's strange?)

Of course, maybe someone can tell me that I'm wrong and everyone else
expects that level-triggered interrupts are fundamentally supposed to
be Acked but that they are only supposed to re-latch if they
transition away from their current level and then back again?


> +        * status enable bit too so the interrupt can't even latch into the
> +        * hardware while it's masked, but only do this for level interrupts
> +        * because edge interrupts have a problem with the raw status bit
> +        * toggling and causing spurious interrupts.

Even if there was no spurious interrupt it would be wrong to use
RAW_STATUS_EN for edge-triggered interrupts.  For edge-triggered
interrupts it's important to continue to latch assertions even when
masked.



> +        */
> +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> +               val &= ~BIT(g->intr_raw_status_bit);
> +               writel(val, pctrl->regs + g->intr_cfg_reg);

Do you know if it's important to do a 2nd write here, or could this be
combined with the next writel()?

> +       }
> +
>         val &= ~BIT(g->intr_enable_bit);
>         writel(val, pctrl->regs + g->intr_cfg_reg);
>
> @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>         val = readl(pctrl->regs + g->intr_cfg_reg);
> +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> +               val |= BIT(g->intr_raw_status_bit);
> +               writel(val, pctrl->regs + g->intr_cfg_reg);

Same question about whether this could be combined with the next
writel().  ...although I could imagine that the answer might be
different for mask and unmask.

...if it can be combined, you can totally get rid of the "if" test and
always "OR" in the bit, right?


Despite the above comments, I'm in favor of this patch.  I'd be
curious about whether we can remove the two writel() calls, but I'd
only suggest the comments be changed if someone else agrees with me
about the fundamental nature of level-triggered interrupts with
regards to Acking.  :-P

-Doug

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Stephen Boyd]

Quoting Doug Anderson (2018-06-18 15:43:06)
> 
> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > +        */
> > +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> > +               val &= ~BIT(g->intr_raw_status_bit);
> > +               writel(val, pctrl->regs + g->intr_cfg_reg);
> 
> Do you know if it's important to do a 2nd write here, or could this be
> combined with the next writel()?

I haven't tried combining the writes. It felt safer to keep them split
up so that both bits don't toggle at the same time, but I don't know if
it actually matters.

> 
> > +       }
> > +
> >         val &= ~BIT(g->intr_enable_bit);
> >         writel(val, pctrl->regs + g->intr_cfg_reg);
> >
> > @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> >         val = readl(pctrl->regs + g->intr_cfg_reg);
> > +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> > +               val |= BIT(g->intr_raw_status_bit);
> > +               writel(val, pctrl->regs + g->intr_cfg_reg);
> 
> Same question about whether this could be combined with the next
> writel().  ...although I could imagine that the answer might be
> different for mask and unmask.

We probably need someone from qcom side to determine if these can be
combined. I can give it a try and see if anything goes wrong but my
confidence level will only be anecdotal. It's worth a shot.

> 
> ...if it can be combined, you can totally get rid of the "if" test and
> always "OR" in the bit, right?

Yes.

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Doug Anderson]

Hi,

On Mon, Jun 18, 2018 at 4:28 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Doug Anderson (2018-06-18 15:43:06)
>>
>> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> > +        */
>> > +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
>> > +               val &= ~BIT(g->intr_raw_status_bit);
>> > +               writel(val, pctrl->regs + g->intr_cfg_reg);
>>
>> Do you know if it's important to do a 2nd write here, or could this be
>> combined with the next writel()?
>
> I haven't tried combining the writes. It felt safer to keep them split
> up so that both bits don't toggle at the same time, but I don't know if
> it actually matters.

Maybe I'm a glutton for punishment, but I'd say go for it, unless
someone from Qualcomm says "no way".

In the very least in the "unmask" case it seems pretty safe.  IMHO if
re-enabling the "raw" status caused a glitch we'd already be hitting
problems.  Specifically the glitch would end up getting latched
(whee!) and then we'd unmask and see the glitch anyway.

...and actually for the "mask" case it seems like you've written it
the less-safe way anyway.  We know that masking can't cause some sort
of glitch (since that's the old code), but I guess we don't know
whether disabling the "raw" status could cause a glitch.  To be the
absolutely safest you'd do the new disable of the "raw" status _after_
the old masking.  ...but as per above I'd just go whole hog and
combine them.  :-P

As with everything I write, feel free to tell me I'm being stupid and
I'll try to shut up.  ;-)


-Doug

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Stephen Boyd]

Quoting Doug Anderson (2018-06-18 16:38:27)
> Hi,
> 
> On Mon, Jun 18, 2018 at 4:28 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> > Quoting Doug Anderson (2018-06-18 15:43:06)
> >>
> >> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> >>
> >> > +        */
> >> > +       if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> >> > +               val &= ~BIT(g->intr_raw_status_bit);
> >> > +               writel(val, pctrl->regs + g->intr_cfg_reg);
> >>
> >> Do you know if it's important to do a 2nd write here, or could this be
> >> combined with the next writel()?
> >
> > I haven't tried combining the writes. It felt safer to keep them split
> > up so that both bits don't toggle at the same time, but I don't know if
> > it actually matters.
> 
> Maybe I'm a glutton for punishment, but I'd say go for it, unless
> someone from Qualcomm says "no way".
> 
> In the very least in the "unmask" case it seems pretty safe.  IMHO if
> re-enabling the "raw" status caused a glitch we'd already be hitting
> problems.  Specifically the glitch would end up getting latched
> (whee!) and then we'd unmask and see the glitch anyway.
> 
> ...and actually for the "mask" case it seems like you've written it
> the less-safe way anyway.  We know that masking can't cause some sort
> of glitch (since that's the old code), but I guess we don't know
> whether disabling the "raw" status could cause a glitch.  To be the
> absolutely safest you'd do the new disable of the "raw" status _after_
> the old masking.  ...but as per above I'd just go whole hog and
> combine them.  :-P
> 
> As with everything I write, feel free to tell me I'm being stupid and
> I'll try to shut up.  ;-)
> 

I've tested it and it seems to work by combining the writes in mask and
unmask. I will resend it with the combination tomorrow or the next day
in case anyone else has comments on this series.

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Bjorn Andersson]

On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:

> The interrupt controller hardware in this pin controller has two status
> enable bits. The first "normal" status enable bit enables or disables
> the summary interrupt line being raised when a gpio interrupt triggers
> and the "raw" status enable bit allows or prevents the hardware from
> latching an interrupt into the status register for a gpio interrupt.
> Currently we just toggle the "normal" status enable bit in the mask and
> unmask ops so that the summary irq interrupt going to the CPU's
> interrupt controller doesn't trigger for the masked gpio interrupt.
> 
> For a level triggered interrupt, the flow would be as follows: the pin
> controller sees the interrupt, latches the status into the status
> register, raises the summary irq to the CPU, summary irq handler runs
> and calls handle_level_irq(), handle_level_irq() masks and acks the gpio
> interrupt, the interrupt handler runs, and finally unmask the interrupt.
> When the interrupt handler completes, we expect that the interrupt line
> level will go back to the deasserted state so the genirq code can unmask
> the interrupt without it triggering again.
> 
> If we only mask the interrupt by clearing the "normal" status enable bit
> then we'll ack the interrupt but it will continue to show up as pending
> in the status register because the raw status bit is enabled, the
> hardware hasn't deasserted the line, and thus the asserted state latches
> into the status register again. When the hardware deasserts the
> interrupt the pin controller still thinks there is a pending unserviced
> level interrupt because it latched it earlier. This behavior causes
> software to see an extra interrupt for level type interrupts each time
> the interrupt is handled.
> 
> Let's fix this by clearing the raw status enable bit for level type
> interrupts so that the hardware stops latching the status of the
> interrupt after we ack it. We don't do this for edge type interrupts
> because it seems that toggling the raw status enable bit for edge type
> interrupts causes spurious edge interrupts.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..3563c4394837 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -626,6 +626,19 @@ static void msm_gpio_irq_mask(struct irq_data *d)
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	/*
> +	 * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
> +	 * are still asserted to re-latch after we ack them. Clear the raw
> +	 * status enable bit too so the interrupt can't even latch into the
> +	 * hardware while it's masked, but only do this for level interrupts
> +	 * because edge interrupts have a problem with the raw status bit
> +	 * toggling and causing spurious interrupts.
> +	 */
> +	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> +		val &= ~BIT(g->intr_raw_status_bit);
> +		writel(val, pctrl->regs + g->intr_cfg_reg);
> +	}
> +
>  	val &= ~BIT(g->intr_enable_bit);
>  	writel(val, pctrl->regs + g->intr_cfg_reg);
>  
> @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> +		val |= BIT(g->intr_raw_status_bit);
> +		writel(val, pctrl->regs + g->intr_cfg_reg);
> +	}
>  	val |= BIT(g->intr_enable_bit);
>  	writel(val, pctrl->regs + g->intr_cfg_reg);

I looked at the TLMM documentation, which states that the status bit
should be cleared after handling the interrupt and this driver used to
do this.

But Timur managed to hit the race where we lost edge triggered
interrupts with this behavior, so we changed it in the following commit:

a6566710adaa ("pinctrl: qcom: Don't clear status bit on irq_unmask")


But the reason that I had this in the driver originally is that msm-3.10
does this (clear status bit in unmask), so perhaps the appropriate way
to solve is to follow the documentation and the downstream driver and
ack the interrupt in unmask - but do so only for level triggered
interrupts?

Regards,
Bjorn

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Doug Anderson]

Hi,

On Tue, Jun 19, 2018 at 11:45 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
>
>> The interrupt controller hardware in this pin controller has two status
>> enable bits. The first "normal" status enable bit enables or disables
>> the summary interrupt line being raised when a gpio interrupt triggers
>> and the "raw" status enable bit allows or prevents the hardware from
>> latching an interrupt into the status register for a gpio interrupt.
>> Currently we just toggle the "normal" status enable bit in the mask and
>> unmask ops so that the summary irq interrupt going to the CPU's
>> interrupt controller doesn't trigger for the masked gpio interrupt.
>>
>> For a level triggered interrupt, the flow would be as follows: the pin
>> controller sees the interrupt, latches the status into the status
>> register, raises the summary irq to the CPU, summary irq handler runs
>> and calls handle_level_irq(), handle_level_irq() masks and acks the gpio
>> interrupt, the interrupt handler runs, and finally unmask the interrupt.
>> When the interrupt handler completes, we expect that the interrupt line
>> level will go back to the deasserted state so the genirq code can unmask
>> the interrupt without it triggering again.
>>
>> If we only mask the interrupt by clearing the "normal" status enable bit
>> then we'll ack the interrupt but it will continue to show up as pending
>> in the status register because the raw status bit is enabled, the
>> hardware hasn't deasserted the line, and thus the asserted state latches
>> into the status register again. When the hardware deasserts the
>> interrupt the pin controller still thinks there is a pending unserviced
>> level interrupt because it latched it earlier. This behavior causes
>> software to see an extra interrupt for level type interrupts each time
>> the interrupt is handled.
>>
>> Let's fix this by clearing the raw status enable bit for level type
>> interrupts so that the hardware stops latching the status of the
>> interrupt after we ack it. We don't do this for edge type interrupts
>> because it seems that toggling the raw status enable bit for edge type
>> interrupts causes spurious edge interrupts.
>>
>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Cc: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> ---
>>  drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 0e22f52b2a19..3563c4394837 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -626,6 +626,19 @@ static void msm_gpio_irq_mask(struct irq_data *d)
>>       raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>>       val = readl(pctrl->regs + g->intr_cfg_reg);
>> +     /*
>> +      * Leaving the RAW_STATUS_EN bit enabled causes level interrupts that
>> +      * are still asserted to re-latch after we ack them. Clear the raw
>> +      * status enable bit too so the interrupt can't even latch into the
>> +      * hardware while it's masked, but only do this for level interrupts
>> +      * because edge interrupts have a problem with the raw status bit
>> +      * toggling and causing spurious interrupts.
>> +      */
>> +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
>> +             val &= ~BIT(g->intr_raw_status_bit);
>> +             writel(val, pctrl->regs + g->intr_cfg_reg);
>> +     }
>> +
>>       val &= ~BIT(g->intr_enable_bit);
>>       writel(val, pctrl->regs + g->intr_cfg_reg);
>>
>> @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
>>       raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>>       val = readl(pctrl->regs + g->intr_cfg_reg);
>> +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
>> +             val |= BIT(g->intr_raw_status_bit);
>> +             writel(val, pctrl->regs + g->intr_cfg_reg);
>> +     }
>>       val |= BIT(g->intr_enable_bit);
>>       writel(val, pctrl->regs + g->intr_cfg_reg);
>
> I looked at the TLMM documentation, which states that the status bit
> should be cleared after handling the interrupt and this driver used to
> do this.
>
> But Timur managed to hit the race where we lost edge triggered
> interrupts with this behavior, so we changed it in the following commit:
>
> a6566710adaa ("pinctrl: qcom: Don't clear status bit on irq_unmask")
>
>
> But the reason that I had this in the driver originally is that msm-3.10
> does this (clear status bit in unmask), so perhaps the appropriate way
> to solve is to follow the documentation and the downstream driver and
> ack the interrupt in unmask - but do so only for level triggered
> interrupts?

LOL.  Stephen and I had an offline discussion about this and acking in
the unmask was my preferred solution too.  It matches what I did to
solve the same problem on a different controller back in 2013.  See
commit 5a68e7a748c0 ("pinctrl: exynos: ack level-triggered interrupts
before unmasking").  Stephen was of the opinion that it was cleaner to
fully disable the interrupt and that the "ack on unmask" was a bit of
a hack.  I figured that both worked OK and so I didn't insist.

Anyway, either is fine with me w/ a slight personal preference to
acking in unmask.

-Doug

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Stephen Boyd]

Quoting Bjorn Andersson (2018-06-19 23:45:09)
> On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> > @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
> >       raw_spin_lock_irqsave(&pctrl->lock, flags);
> >  
> >       val = readl(pctrl->regs + g->intr_cfg_reg);
> > +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> > +             val |= BIT(g->intr_raw_status_bit);
> > +             writel(val, pctrl->regs + g->intr_cfg_reg);
> > +     }
> >       val |= BIT(g->intr_enable_bit);
> >       writel(val, pctrl->regs + g->intr_cfg_reg);
> 
> I looked at the TLMM documentation, which states that the status bit
> should be cleared after handling the interrupt and this driver used to
> do this.

Nice!

> 
> But Timur managed to hit the race where we lost edge triggered
> interrupts with this behavior, so we changed it in the following commit:
> 
> a6566710adaa ("pinctrl: qcom: Don't clear status bit on irq_unmask")
> 
> 
> But the reason that I had this in the driver originally is that msm-3.10
> does this (clear status bit in unmask), so perhaps the appropriate way
> to solve is to follow the documentation and the downstream driver and
> ack the interrupt in unmask - but do so only for level triggered
> interrupts?
> 

Clearing the status bit (basically acking the gpio irq) can be done in
unmask for level triggered interrupts. That works and as you say it's
even documented.

I didn't implement that because it felt better to prevent the status
from latching in the hardware while the interrupt is masked. My
understanding of irq mask semantics is that the interrupt shouldn't be
"pending" during the time between mask and unmask and clearing the raw
status allows us to do that properly without messing with the status bit
on the unmask path. It also means that the ack operation really does ack
the irq status bit and cause it to go away. I suppose there is one case
where I'm wrong though, and that is when the irq is unmasked on irq
startup where we don't want to see a spurious latched level interrupt
that occurred before we booted.

That problem may be possible with bad bootloaders that are leaving some
status bit latched in there, but also we would want to fix that for edge
type interrupts too, so we would need to clear the status bit regardless
of the level on irq startup and hope an edge isn't lost on startup.

Re: [PATCH 1/3] pinctrl: msm: Really mask level interrupts to prevent latching

[Bjorn Andersson]

On Thu 21 Jun 08:14 PDT 2018, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2018-06-19 23:45:09)
> > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> > > @@ -647,6 +660,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
> > >       raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >  
> > >       val = readl(pctrl->regs + g->intr_cfg_reg);
> > > +     if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK) {
> > > +             val |= BIT(g->intr_raw_status_bit);
> > > +             writel(val, pctrl->regs + g->intr_cfg_reg);
> > > +     }
> > >       val |= BIT(g->intr_enable_bit);
> > >       writel(val, pctrl->regs + g->intr_cfg_reg);
> > 
> > I looked at the TLMM documentation, which states that the status bit
> > should be cleared after handling the interrupt and this driver used to
> > do this.
> 
> Nice!
> 
> > 
> > But Timur managed to hit the race where we lost edge triggered
> > interrupts with this behavior, so we changed it in the following commit:
> > 
> > a6566710adaa ("pinctrl: qcom: Don't clear status bit on irq_unmask")
> > 
> > 
> > But the reason that I had this in the driver originally is that msm-3.10
> > does this (clear status bit in unmask), so perhaps the appropriate way
> > to solve is to follow the documentation and the downstream driver and
> > ack the interrupt in unmask - but do so only for level triggered
> > interrupts?
> > 
> 
> Clearing the status bit (basically acking the gpio irq) can be done in
> unmask for level triggered interrupts. That works and as you say it's
> even documented.
> 
> I didn't implement that because it felt better to prevent the status
> from latching in the hardware while the interrupt is masked. My
> understanding of irq mask semantics is that the interrupt shouldn't be
> "pending" during the time between mask and unmask and clearing the raw
> status allows us to do that properly without messing with the status bit
> on the unmask path. It also means that the ack operation really does ack
> the irq status bit and cause it to go away. I suppose there is one case
> where I'm wrong though, and that is when the irq is unmasked on irq
> startup where we don't want to see a spurious latched level interrupt
> that occurred before we booted.
> 

I took another pass through the irq code and I agree, while the late ack
does solve the problem it's more intuitive if we can prevent the
latching.

> That problem may be possible with bad bootloaders that are leaving some
> status bit latched in there, but also we would want to fix that for edge
> type interrupts too, so we would need to clear the status bit regardless
> of the level on irq startup and hope an edge isn't lost on startup.
> 

I would prefer that we handle that case explicitly.


There are some concerns from msm-3.10 regarding writing the intr-raw bit
at the same time as the other bits, but I think that relates to the fact
that the downstream driver used to configure and enabled raw-state in
the same write. So please respin v2 as you planned.

Regards,
Bjorn

[PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

We rely on devices to use pinmuxing configurations in DT to select the
GPIO function (function 0) if they're going to use the gpio in GPIO
mode. Let's simplify things for driver authors by implementing
gpio_request_enable() for this pinctrl driver to mux out the GPIO
function when the gpio is use from gpiolib.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 3563c4394837..eacfc5b85f7f 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
+				   struct pinctrl_gpio_range *range,
+				   unsigned offset)
+{
+	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
+
+	/* No funcs? Probably ACPI so can't do anything here */
+	if (!g->nfuncs)
+		return 0;
+
+	/* For now assume function 0 is GPIO because it always is */
+	return msm_pinmux_set_mux(pctldev, 0, offset);
+}
+
 static const struct pinmux_ops msm_pinmux_ops = {
 	.request		= msm_pinmux_request,
 	.get_functions_count	= msm_get_functions_count,
 	.get_function_name	= msm_get_function_name,
 	.get_function_groups	= msm_get_function_groups,
+	.gpio_request_enable	= msm_pinmux_request_gpio,
 	.set_mux		= msm_pinmux_set_mux,
 };
 
-- 
Sent by a computer through tubes

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Doug Anderson]

Hi,

On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> We rely on devices to use pinmuxing configurations in DT to select the
> GPIO function (function 0) if they're going to use the gpio in GPIO
> mode. Let's simplify things for driver authors by implementing
> gpio_request_enable() for this pinctrl driver to mux out the GPIO
> function when the gpio is use from gpiolib.
>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 3563c4394837..eacfc5b85f7f 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>         return 0;
>  }
>
> +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
> +                                  struct pinctrl_gpio_range *range,
> +                                  unsigned offset)
> +{
> +       struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       const struct msm_pingroup *g = &pctrl->soc->groups[offset];
> +
> +       /* No funcs? Probably ACPI so can't do anything here */
> +       if (!g->nfuncs)
> +               return 0;

Is there a reason why you'd want to return 0 instead of some sort of
error code?  Wouldn't you want to know that this pin can't be a GPIO?
Another non-ACPI example is sdc2 on sdm845 and it seems like you'd
want to know if someone tried to set one of those as a GPIO.

...oh, but I guess ufs_reset also has no funcs but it still probably
wants to use the GPIO framework to write something.  Hrmmm...  Maybe
check if either in_bit or out_bit is not -1?


> +
> +       /* For now assume function 0 is GPIO because it always is */
> +       return msm_pinmux_set_mux(pctldev, 0, offset);

nit: should you be consistent with msm_pinmux_set_mux() and call it
"group" instead of "offset"?  It looks like it's supposed to be the
same thing...


-Doug

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Doug Anderson (2018-06-18 16:54:49)
> Hi,
> 
> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> > We rely on devices to use pinmuxing configurations in DT to select the
> > GPIO function (function 0) if they're going to use the gpio in GPIO
> > mode. Let's simplify things for driver authors by implementing
> > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > function when the gpio is use from gpiolib.
> >
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 3563c4394837..eacfc5b85f7f 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >         return 0;
> >  }
> >
> > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
> > +                                  struct pinctrl_gpio_range *range,
> > +                                  unsigned offset)
> > +{
> > +       struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +       const struct msm_pingroup *g = &pctrl->soc->groups[offset];
> > +
> > +       /* No funcs? Probably ACPI so can't do anything here */
> > +       if (!g->nfuncs)
> > +               return 0;
> 
> Is there a reason why you'd want to return 0 instead of some sort of
> error code?  Wouldn't you want to know that this pin can't be a GPIO?

On ACPI there aren't any functions and thus all pins are GPIO mode and
only GPIO mode if they're used as GPIOs. At least that's my
understanding of how the ACPI version of this driver works.

> Another non-ACPI example is sdc2 on sdm845 and it seems like you'd
> want to know if someone tried to set one of those as a GPIO.
> 
> ...oh, but I guess ufs_reset also has no funcs but it still probably
> wants to use the GPIO framework to write something.  Hrmmm...  Maybe
> check if either in_bit or out_bit is not -1?

ufs_reset and sdc2 aren't in the GPIO chip's numberspace so I don't
think we need to care? At least I can't convince myself that those pins
would eventually call into the this function. We could check if offset
is greater than ngpios for the chip but that seems useless if higher
layers are handling this already.

> 
> 
> > +
> > +       /* For now assume function 0 is GPIO because it always is */
> > +       return msm_pinmux_set_mux(pctldev, 0, offset);
> 
> nit: should you be consistent with msm_pinmux_set_mux() and call it
> "group" instead of "offset"?  It looks like it's supposed to be the
> same thing...
> 

This is copying the generic one and the function pointer prototype, so I
think it's best to leave it as 'offset'.

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Doug Anderson]

Hi,

On Tue, Jun 19, 2018 at 2:18 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Doug Anderson (2018-06-18 16:54:49)
>> Hi,
>>
>> On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
>> > We rely on devices to use pinmuxing configurations in DT to select the
>> > GPIO function (function 0) if they're going to use the gpio in GPIO
>> > mode. Let's simplify things for driver authors by implementing
>> > gpio_request_enable() for this pinctrl driver to mux out the GPIO
>> > function when the gpio is use from gpiolib.
>> >
>> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>> > Cc: Doug Anderson <dianders@chromium.org>
>> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> > ---
>> >  drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++
>> >  1 file changed, 16 insertions(+)
>> >
>> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> > index 3563c4394837..eacfc5b85f7f 100644
>> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>> >         return 0;
>> >  }
>> >
>> > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
>> > +                                  struct pinctrl_gpio_range *range,
>> > +                                  unsigned offset)
>> > +{
>> > +       struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> > +       const struct msm_pingroup *g = &pctrl->soc->groups[offset];
>> > +
>> > +       /* No funcs? Probably ACPI so can't do anything here */
>> > +       if (!g->nfuncs)
>> > +               return 0;
>>
>> Is there a reason why you'd want to return 0 instead of some sort of
>> error code?  Wouldn't you want to know that this pin can't be a GPIO?
>
> On ACPI there aren't any functions and thus all pins are GPIO mode and
> only GPIO mode if they're used as GPIOs. At least that's my
> understanding of how the ACPI version of this driver works.

OK.  I have no understanding of how the ACPI version of this driver
works, so your understanding is much more likely to be right than
mine.  I guess this is just "pinctrl-qdf2xxx.c"?


>> Another non-ACPI example is sdc2 on sdm845 and it seems like you'd
>> want to know if someone tried to set one of those as a GPIO.
>>
>> ...oh, but I guess ufs_reset also has no funcs but it still probably
>> wants to use the GPIO framework to write something.  Hrmmm...  Maybe
>> check if either in_bit or out_bit is not -1?
>
> ufs_reset and sdc2 aren't in the GPIO chip's numberspace so I don't
> think we need to care? At least I can't convince myself that those pins
> would eventually call into the this function. We could check if offset
> is greater than ngpios for the chip but that seems useless if higher
> layers are handling this already.

Ah, I see what you mean.  These pins do have numbers in the code:

PINCTRL_PIN(150, "SDC2_CLK"),
PINCTRL_PIN(151, "SDC2_CMD"),
PINCTRL_PIN(152, "SDC2_DATA"),
PINCTRL_PIN(153, "UFS_RESET"),

...but those are effectively made up numbers and they are all past the
"ngpios" (150).  ...and the higher level code seems to be already
checking that.


OK, thought I've already proven my cluelessness about this driver,
FWIW this patch makes sense to me now so FWIW:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

-Doug

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Doug Anderson (2018-06-19 14:38:57)
> On Tue, Jun 19, 2018 at 2:18 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> > Quoting Doug Anderson (2018-06-18 16:54:49)
> >>
> >> Is there a reason why you'd want to return 0 instead of some sort of
> >> error code?  Wouldn't you want to know that this pin can't be a GPIO?
> >
> > On ACPI there aren't any functions and thus all pins are GPIO mode and
> > only GPIO mode if they're used as GPIOs. At least that's my
> > understanding of how the ACPI version of this driver works.
> 
> OK.  I have no understanding of how the ACPI version of this driver
> works, so your understanding is much more likely to be right than
> mine.  I guess this is just "pinctrl-qdf2xxx.c"?
> 

Yes that's the single ACPI driver.

> 
> >> Another non-ACPI example is sdc2 on sdm845 and it seems like you'd
> >> want to know if someone tried to set one of those as a GPIO.
> >>
> >> ...oh, but I guess ufs_reset also has no funcs but it still probably
> >> wants to use the GPIO framework to write something.  Hrmmm...  Maybe
> >> check if either in_bit or out_bit is not -1?
> >
> > ufs_reset and sdc2 aren't in the GPIO chip's numberspace so I don't
> > think we need to care? At least I can't convince myself that those pins
> > would eventually call into the this function. We could check if offset
> > is greater than ngpios for the chip but that seems useless if higher
> > layers are handling this already.
> 
> Ah, I see what you mean.  These pins do have numbers in the code:
> 
> PINCTRL_PIN(150, "SDC2_CLK"),
> PINCTRL_PIN(151, "SDC2_CMD"),
> PINCTRL_PIN(152, "SDC2_DATA"),
> PINCTRL_PIN(153, "UFS_RESET"),
> 
> ...but those are effectively made up numbers and they are all past the
> "ngpios" (150).  ...and the higher level code seems to be already
> checking that.

Right. Hopefully that saves us from this trouble.

> 
> 
> OK, thought I've already proven my cluelessness about this driver,
> FWIW this patch makes sense to me now so FWIW:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 

Thanks!

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Bjorn Andersson]

On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:

> We rely on devices to use pinmuxing configurations in DT to select the
> GPIO function (function 0) if they're going to use the gpio in GPIO
> mode. Let's simplify things for driver authors by implementing
> gpio_request_enable() for this pinctrl driver to mux out the GPIO
> function when the gpio is use from gpiolib.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 3563c4394837..eacfc5b85f7f 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	return 0;
>  }
>  
> +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
> +				   struct pinctrl_gpio_range *range,
> +				   unsigned offset)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
> +
> +	/* No funcs? Probably ACPI so can't do anything here */
> +	if (!g->nfuncs)
> +		return 0;
> +
> +	/* For now assume function 0 is GPIO because it always is */
> +	return msm_pinmux_set_mux(pctldev, 0, offset);
> +}
> +
>  static const struct pinmux_ops msm_pinmux_ops = {
>  	.request		= msm_pinmux_request,
>  	.get_functions_count	= msm_get_functions_count,
>  	.get_function_name	= msm_get_function_name,
>  	.get_function_groups	= msm_get_function_groups,
> +	.gpio_request_enable	= msm_pinmux_request_gpio,
>  	.set_mux		= msm_pinmux_set_mux,
>  };
>  
> -- 
> Sent by a computer through tubes
> 

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Bjorn Andersson]

On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:

> On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> 
> > We rely on devices to use pinmuxing configurations in DT to select the
> > GPIO function (function 0) if they're going to use the gpio in GPIO
> > mode. Let's simplify things for driver authors by implementing
> > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > function when the gpio is use from gpiolib.
> > 
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 

On second thought, while reading patch 3, when would this be used?

While both patch 2 and 3 are convenient ways to get around the annoyance
of having to specify a pinmux state both patches then ends up relying on
some default pinconf state; which I think is bad.


Further more in situations like i2c-qup (downstream), where the pins are
requested as gpios in order to "bitbang" a reset this would mean that
the driver has to counter the convenience; by either switching in the
default pinmux at the end of probe or postponing the gpio_request() to
the invocation of reset and then, after issuing the gpio_release,
switching in the default pinmux explicitly again.


So I'm not sure we want this.

Regards,
Bjorn

> Regards,
> Bjorn
> 
> > Cc: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/pinctrl/qcom/pinctrl-msm.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 3563c4394837..eacfc5b85f7f 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -176,11 +176,27 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> >  	return 0;
> >  }
> >  
> > +static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
> > +				   struct pinctrl_gpio_range *range,
> > +				   unsigned offset)
> > +{
> > +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +	const struct msm_pingroup *g = &pctrl->soc->groups[offset];
> > +
> > +	/* No funcs? Probably ACPI so can't do anything here */
> > +	if (!g->nfuncs)
> > +		return 0;
> > +
> > +	/* For now assume function 0 is GPIO because it always is */
> > +	return msm_pinmux_set_mux(pctldev, 0, offset);
> > +}
> > +
> >  static const struct pinmux_ops msm_pinmux_ops = {
> >  	.request		= msm_pinmux_request,
> >  	.get_functions_count	= msm_get_functions_count,
> >  	.get_function_name	= msm_get_function_name,
> >  	.get_function_groups	= msm_get_function_groups,
> > +	.gpio_request_enable	= msm_pinmux_request_gpio,
> >  	.set_mux		= msm_pinmux_set_mux,
> >  };
> >  
> > -- 
> > Sent by a computer through tubes
> > 

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Linus Walleij]

On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
> > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> >
> > > We rely on devices to use pinmuxing configurations in DT to select the
> > > GPIO function (function 0) if they're going to use the gpio in GPIO
> > > mode. Let's simplify things for driver authors by implementing
> > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > > function when the gpio is use from gpiolib.
> > >
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
(...)
> While both patch 2 and 3 are convenient ways to get around the annoyance
> of having to specify a pinmux state both patches then ends up relying on
> some default pinconf state; which I think is bad.

Nothing stops you from setting up a default conf in
this callback though.

But admittedly this call was added for simpler hardware.

> Further more in situations like i2c-qup (downstream), where the pins are
> requested as gpios in order to "bitbang" a reset this would mean that
> the driver has to counter the convenience; by either switching in the
> default pinmux at the end of probe or postponing the gpio_request() to
> the invocation of reset and then, after issuing the gpio_release,
> switching in the default pinmux explicitly again.

That's a bigger problem. If the system is using device and GPIO
mode orthogonally, it'd be good to leave like this.

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Linus Walleij (2018-06-28 07:25:46)
> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
> > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> > >
> > > > We rely on devices to use pinmuxing configurations in DT to select the
> > > > GPIO function (function 0) if they're going to use the gpio in GPIO
> > > > mode. Let's simplify things for driver authors by implementing
> > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > > > function when the gpio is use from gpiolib.
> > > >
> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >
> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> (...)
> > While both patch 2 and 3 are convenient ways to get around the annoyance
> > of having to specify a pinmux state both patches then ends up relying on
> > some default pinconf state; which I think is bad.

What default state are we relying on? The reset state of the pins? I'm
very confused by this statement. These last two patches are making sure
that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO
mode. If this code is not in place, then we'll allow drivers to request
a GPIO but pinmuxing won't be called to actually mux that pin to GPIO
mode unless they have a DT conf for function = "gpio". That seems
entirely unexpected and easy to mess up.

> 
> Nothing stops you from setting up a default conf in
> this callback though.
> 
> But admittedly this call was added for simpler hardware.
> 
> > Further more in situations like i2c-qup (downstream), where the pins are
> > requested as gpios in order to "bitbang" a reset this would mean that
> > the driver has to counter the convenience; by either switching in the
> > default pinmux at the end of probe or postponing the gpio_request() to
> > the invocation of reset and then, after issuing the gpio_release,
> > switching in the default pinmux explicitly again.
> 
> That's a bigger problem. If the system is using device and GPIO
> mode orthogonally, it'd be good to leave like this.
> 

Doesn't that driver need to explicitly mux GPIO mode vs. device mode
right now? So having gpio_request() do the muxing to GPIO mode and then
explicit muxing of the pin to device mode would be what we have after
this patch, while before this patch we would have mux to GPIO, request
GPIO (nop), mux to device. We saved an explicit pinmux call?

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Doug Anderson]

Hi,

On Thu, Jun 28, 2018 at 10:14 AM, Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Linus Walleij (2018-06-28 07:25:46)
>> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>> > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
>> > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
>> > >
>> > > > We rely on devices to use pinmuxing configurations in DT to select the
>> > > > GPIO function (function 0) if they're going to use the gpio in GPIO
>> > > > mode. Let's simplify things for driver authors by implementing
>> > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
>> > > > function when the gpio is use from gpiolib.
>> > > >
>> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>> > >
>> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> (...)
>> > While both patch 2 and 3 are convenient ways to get around the annoyance
>> > of having to specify a pinmux state both patches then ends up relying on
>> > some default pinconf state; which I think is bad.
>
> What default state are we relying on? The reset state of the pins? I'm
> very confused by this statement. These last two patches are making sure
> that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO
> mode. If this code is not in place, then we'll allow drivers to request
> a GPIO but pinmuxing won't be called to actually mux that pin to GPIO
> mode unless they have a DT conf for function = "gpio". That seems
> entirely unexpected and easy to mess up.
>
>>
>> Nothing stops you from setting up a default conf in
>> this callback though.
>>
>> But admittedly this call was added for simpler hardware.
>>
>> > Further more in situations like i2c-qup (downstream), where the pins are
>> > requested as gpios in order to "bitbang" a reset this would mean that
>> > the driver has to counter the convenience; by either switching in the
>> > default pinmux at the end of probe or postponing the gpio_request() to
>> > the invocation of reset and then, after issuing the gpio_release,
>> > switching in the default pinmux explicitly again.
>>
>> That's a bigger problem. If the system is using device and GPIO
>> mode orthogonally, it'd be good to leave like this.
>>
>
> Doesn't that driver need to explicitly mux GPIO mode vs. device mode
> right now? So having gpio_request() do the muxing to GPIO mode and then
> explicit muxing of the pin to device mode would be what we have after
> this patch, while before this patch we would have mux to GPIO, request
> GPIO (nop), mux to device. We saved an explicit pinmux call?

After reading these replies I'm slightly worried about some of the
stuff Bjorn is worried about too.  In Bjorn's example I think that the
"default" state of the pins is probably i2c-mode and that it might
switch to GPIO mode only when it needs to do the special unwedge of
the i2c port.

So maybe the i2c driver does this:

1. "Default" pinmux state of i2c is i2c-mode, "unwedge" pinmux state
is gpio-mode.

2. In probe, request the GPIOs for use later in case we need to
unwedge.  Don't use them right now since we don't need to unwedge.
Assumes pinmux state stays as "default".

3. When you decide you need to unwedge the bus, pinmux to the special
i2c mode and drive the pins you requested in probe.  Then pinmux back.


...I think your patch will break this because when you request the
GPIOs you'll have an implicit (and unexpected?) pinmux transition.

What do you think?


NOTE: I think even if we don't want patch #2, we might still want at
least part of patch #3?  It feels like if you request a GPIO as an IRQ
that it should be OK to at least automatically change it to an
input...


-Doug

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Doug Anderson (2018-06-28 11:45:30)
> Hi,
> 
> On Thu, Jun 28, 2018 at 10:14 AM, Stephen Boyd <swboyd@chromium.org> wrote:
> > Quoting Linus Walleij (2018-06-28 07:25:46)
> >> On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
> >> <bjorn.andersson@linaro.org> wrote:
> >> > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
> >> > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> >> > >
> >> > > > We rely on devices to use pinmuxing configurations in DT to select the
> >> > > > GPIO function (function 0) if they're going to use the gpio in GPIO
> >> > > > mode. Let's simplify things for driver authors by implementing
> >> > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> >> > > > function when the gpio is use from gpiolib.
> >> > > >
> >> > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> >> > >
> >> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >> (...)
> >> > While both patch 2 and 3 are convenient ways to get around the annoyance
> >> > of having to specify a pinmux state both patches then ends up relying on
> >> > some default pinconf state; which I think is bad.
> >
> > What default state are we relying on? The reset state of the pins? I'm
> > very confused by this statement. These last two patches are making sure
> > that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO
> > mode. If this code is not in place, then we'll allow drivers to request
> > a GPIO but pinmuxing won't be called to actually mux that pin to GPIO
> > mode unless they have a DT conf for function = "gpio". That seems
> > entirely unexpected and easy to mess up.
> >
> >>
> >> Nothing stops you from setting up a default conf in
> >> this callback though.
> >>
> >> But admittedly this call was added for simpler hardware.
> >>
> >> > Further more in situations like i2c-qup (downstream), where the pins are
> >> > requested as gpios in order to "bitbang" a reset this would mean that
> >> > the driver has to counter the convenience; by either switching in the
> >> > default pinmux at the end of probe or postponing the gpio_request() to
> >> > the invocation of reset and then, after issuing the gpio_release,
> >> > switching in the default pinmux explicitly again.
> >>
> >> That's a bigger problem. If the system is using device and GPIO
> >> mode orthogonally, it'd be good to leave like this.
> >>
> >
> > Doesn't that driver need to explicitly mux GPIO mode vs. device mode
> > right now? So having gpio_request() do the muxing to GPIO mode and then
> > explicit muxing of the pin to device mode would be what we have after
> > this patch, while before this patch we would have mux to GPIO, request
> > GPIO (nop), mux to device. We saved an explicit pinmux call?
> 
> After reading these replies I'm slightly worried about some of the
> stuff Bjorn is worried about too.  In Bjorn's example I think that the
> "default" state of the pins is probably i2c-mode and that it might
> switch to GPIO mode only when it needs to do the special unwedge of
> the i2c port.
> 
> So maybe the i2c driver does this:
> 
> 1. "Default" pinmux state of i2c is i2c-mode, "unwedge" pinmux state
> is gpio-mode.
> 
> 2. In probe, request the GPIOs for use later in case we need to
> unwedge.  Don't use them right now since we don't need to unwedge.
> Assumes pinmux state stays as "default".
> 
> 3. When you decide you need to unwedge the bus, pinmux to the special
> i2c mode and drive the pins you requested in probe.  Then pinmux back.
> 
> 
> ...I think your patch will break this because when you request the
> GPIOs you'll have an implicit (and unexpected?) pinmux transition.
> 
> What do you think?
> 
> 

I could do with some more clarity from Linus in the "Drivers needing
both pin control and GPIOs" section of
Documentation/driver-api/pinctl.rst but I read that section as stating
that the GPIO driver needs to mux the pin as a GPIO by requesting the
pinctrl backend to do so, unless the hardware overrides the muxed
function selection when the GPIO is used, without involving pinctrl
software.

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Linus Walleij]

On Mon, Jul 2, 2018 at 7:56 PM Stephen Boyd <swboyd@chromium.org> wrote:

> I could do with some more clarity from Linus in the "Drivers needing
> both pin control and GPIOs" section of
> Documentation/driver-api/pinctl.rst but I read that section as stating
> that the GPIO driver needs to mux the pin as a GPIO by requesting the
> pinctrl backend to do so, unless the hardware overrides the muxed
> function selection when the GPIO is used, without involving pinctrl
> software.

Yeah that text is especially terse :/

What it says (or what I meant to say) is that there is a choice
between letting the pin control and GPIO functionality on the
same pin be handled orthogonally or implementing these
gpio_*() callbacks into the pin control backend, but in either case
the two APIs must be used in sequence:
pin control setting comes first, second the GPIO subsystem can
request the GPIO line.

I'll see if I can clarify.

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Linus Walleij (2018-07-09 06:54:01)
> On Mon, Jul 2, 2018 at 7:56 PM Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > I could do with some more clarity from Linus in the "Drivers needing
> > both pin control and GPIOs" section of
> > Documentation/driver-api/pinctl.rst but I read that section as stating
> > that the GPIO driver needs to mux the pin as a GPIO by requesting the
> > pinctrl backend to do so, unless the hardware overrides the muxed
> > function selection when the GPIO is used, without involving pinctrl
> > software.
> 
> Yeah that text is especially terse :/
> 
> What it says (or what I meant to say) is that there is a choice
> between letting the pin control and GPIO functionality on the
> same pin be handled orthogonally or implementing these
> gpio_*() callbacks into the pin control backend, but in either case
> the two APIs must be used in sequence:
> pin control setting comes first, second the GPIO subsystem can
> request the GPIO line.
> 
> I'll see if I can clarify.
> 

Ok. Is my interpretation correct though? The fundamental question here
is if gpio_request() should remux the GPIO for the GPIO function or if
drivers are expected to have pinmux settings to use their pin as a GPIO.

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Linus Walleij]

On Mon, Jul 9, 2018 at 5:37 PM Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Linus Walleij (2018-07-09 06:54:01)
> > On Mon, Jul 2, 2018 at 7:56 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > I could do with some more clarity from Linus in the "Drivers needing
> > > both pin control and GPIOs" section of
> > > Documentation/driver-api/pinctl.rst but I read that section as stating
> > > that the GPIO driver needs to mux the pin as a GPIO by requesting the
> > > pinctrl backend to do so, unless the hardware overrides the muxed
> > > function selection when the GPIO is used, without involving pinctrl
> > > software.
> >
> > Yeah that text is especially terse :/
> >
> > What it says (or what I meant to say) is that there is a choice
> > between letting the pin control and GPIO functionality on the
> > same pin be handled orthogonally or implementing these
> > gpio_*() callbacks into the pin control backend, but in either case
> > the two APIs must be used in sequence:
> > pin control setting comes first, second the GPIO subsystem can
> > request the GPIO line.
> >
> > I'll see if I can clarify.
>
> Ok. Is my interpretation correct though? The fundamental question here
> is if gpio_request() should remux the GPIO for the GPIO function or if
> drivers are expected to have pinmux settings to use their pin as a GPIO.

It's an either/or situation.

So there are two ways to do it, as the gpio_request() callback to
pinctrl_gpio_request() etc are not compulsory to implement.

For any one specific system, it is either done such that gpio_request()
does it by calling down to pinctrl_gpio_request() and talking to the
pinctrl back-end, OR the pin muxing is done as a side dish
without any interaction with the GPIO subsystem.

So pick one...

I know this is not very consistent. Sorry for the inconvenience :(

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Bjorn Andersson]

On Thu 28 Jun 10:14 PDT 2018, Stephen Boyd wrote:

> Quoting Linus Walleij (2018-06-28 07:25:46)
> > On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
> > > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> > > >
> > > > > We rely on devices to use pinmuxing configurations in DT to select the
> > > > > GPIO function (function 0) if they're going to use the gpio in GPIO
> > > > > mode. Let's simplify things for driver authors by implementing
> > > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > > > > function when the gpio is use from gpiolib.
> > > > >
> > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > >
> > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > (...)
> > > While both patch 2 and 3 are convenient ways to get around the annoyance
> > > of having to specify a pinmux state both patches then ends up relying on
> > > some default pinconf state; which I think is bad.
> 
> What default state are we relying on? The reset state of the pins? I'm
> very confused by this statement. These last two patches are making sure
> that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO
> mode.

Yes and this is convenient, as the TLMM is both multiplexor and gpio
controller this is probably what people would expect. However looking at
the downstream code people don't think this way (i.e. many drivers calls
gpio_request() to get some sort of exclusive access to its pins - not
to request gpio to be the muxed function).

But my concern is related to pinconf, not pinmux - automating pinmuxing
doesn't change the fact that the systems integrator should make sure to
configure appropriate pull properties on the pins.

> If this code is not in place, then we'll allow drivers to request
> a GPIO but pinmuxing won't be called to actually mux that pin to GPIO
> mode unless they have a DT conf for function = "gpio". That seems
> entirely unexpected and easy to mess up.
> 

I do agree with this, but the opposite doesn't feel crystal clear
either.

> > 
> > Nothing stops you from setting up a default conf in
> > this callback though.
> > 
> > But admittedly this call was added for simpler hardware.
> > 
> > > Further more in situations like i2c-qup (downstream), where the pins are
> > > requested as gpios in order to "bitbang" a reset this would mean that
> > > the driver has to counter the convenience; by either switching in the
> > > default pinmux at the end of probe or postponing the gpio_request() to
> > > the invocation of reset and then, after issuing the gpio_release,
> > > switching in the default pinmux explicitly again.
> > 
> > That's a bigger problem. If the system is using device and GPIO
> > mode orthogonally, it'd be good to leave like this.
> > 
> 
> Doesn't that driver need to explicitly mux GPIO mode vs. device mode
> right now? So having gpio_request() do the muxing to GPIO mode and then
> explicit muxing of the pin to device mode would be what we have after
> this patch, while before this patch we would have mux to GPIO, request
> GPIO (nop), mux to device. We saved an explicit pinmux call?
> 

It's currently possible to call gpio_request() in the i2c driver's probe
function and then mux when needed. With this patch you would have to
either follow the gpio_request with a mux-to-default or defer the
gpio_request until the point where they today would have an explicit
mux-to-gpio.

Regards,
Bjorn

Re: [PATCH 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Stephen Boyd]

Quoting Bjorn Andersson (2018-07-02 12:09:22)
> On Thu 28 Jun 10:14 PDT 2018, Stephen Boyd wrote:
> 
> > Quoting Linus Walleij (2018-06-28 07:25:46)
> > > On Fri, Jun 22, 2018 at 8:29 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > > On Fri 22 Jun 10:58 PDT 2018, Bjorn Andersson wrote:
> > > > > On Mon 18 Jun 13:52 PDT 2018, Stephen Boyd wrote:
> > > > >
> > > > > > We rely on devices to use pinmuxing configurations in DT to select the
> > > > > > GPIO function (function 0) if they're going to use the gpio in GPIO
> > > > > > mode. Let's simplify things for driver authors by implementing
> > > > > > gpio_request_enable() for this pinctrl driver to mux out the GPIO
> > > > > > function when the gpio is use from gpiolib.
> > > > > >
> > > > > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > >
> > > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > (...)
> > > > While both patch 2 and 3 are convenient ways to get around the annoyance
> > > > of having to specify a pinmux state both patches then ends up relying on
> > > > some default pinconf state; which I think is bad.
> > 
> > What default state are we relying on? The reset state of the pins? I'm
> > very confused by this statement. These last two patches are making sure
> > that when a GPIO is requested as a GPIO or IRQ, it's really in GPIO
> > mode.
> 
> Yes and this is convenient, as the TLMM is both multiplexor and gpio
> controller this is probably what people would expect. However looking at
> the downstream code people don't think this way (i.e. many drivers calls
> gpio_request() to get some sort of exclusive access to its pins - not
> to request gpio to be the muxed function).
> 
> But my concern is related to pinconf, not pinmux - automating pinmuxing
> doesn't change the fact that the systems integrator should make sure to
> configure appropriate pull properties on the pins.

Ok. Pinconf wouldn't change here, and I would expect system integrators
to set pinconfs in their DTS board files like always.

> 
> > > 
> > > Nothing stops you from setting up a default conf in
> > > this callback though.
> > > 
> > > But admittedly this call was added for simpler hardware.
> > > 
> > > > Further more in situations like i2c-qup (downstream), where the pins are
> > > > requested as gpios in order to "bitbang" a reset this would mean that
> > > > the driver has to counter the convenience; by either switching in the
> > > > default pinmux at the end of probe or postponing the gpio_request() to
> > > > the invocation of reset and then, after issuing the gpio_release,
> > > > switching in the default pinmux explicitly again.
> > > 
> > > That's a bigger problem. If the system is using device and GPIO
> > > mode orthogonally, it'd be good to leave like this.
> > > 
> > 
> > Doesn't that driver need to explicitly mux GPIO mode vs. device mode
> > right now? So having gpio_request() do the muxing to GPIO mode and then
> > explicit muxing of the pin to device mode would be what we have after
> > this patch, while before this patch we would have mux to GPIO, request
> > GPIO (nop), mux to device. We saved an explicit pinmux call?
> > 
> 
> It's currently possible to call gpio_request() in the i2c driver's probe
> function and then mux when needed. With this patch you would have to
> either follow the gpio_request with a mux-to-default or defer the
> gpio_request until the point where they today would have an explicit
> mux-to-gpio.
> 

Ok, got it. I don't know if we can do any better though, so the question
is if gpio_request() muxing the pin for gpio operation is matching
driver expectations or not. Also, see my reply to Doug here on the same
topic. The documentation file isn't crystal clear to me. If Linus can
clear things up a bit I think we can all get on the same page.

[PATCH 3/3] pinctrl: msm: Configure interrupts as input and gpio mode

[Stephen Boyd]

When requesting a gpio as an interrupt, we should make sure to mux the
pin as the GPIO function and configure it to be an input so that various
functions or output signals don't affect the interrupt state of the pin.
So far, we've relied on pinmux configurations in DT to handle this, but
let's explicitly configure this in the code so that DT implementers
don't have to get this part right.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index eacfc5b85f7f..d3c107d6c05e 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -822,6 +822,41 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	return 0;
 }
 
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	int ret;
+
+	if (!try_module_get(gc->owner))
+		return -ENODEV;
+
+	ret = msm_pinmux_request_gpio(pctrl->pctrl, NULL, d->hwirq);
+	if (ret)
+		goto out;
+	msm_gpio_direction_input(gc, d->hwirq);
+
+	if (gpiochip_lock_as_irq(gc, d->hwirq)) {
+		dev_err(gc->parent,
+			"unable to lock HW IRQ %lu for IRQ\n",
+			d->hwirq);
+		ret = -EINVAL;
+		goto out;
+	}
+	return 0;
+out:
+	module_put(gc->owner);
+	return ret;
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	gpiochip_unlock_as_irq(gc, d->hwirq);
+	module_put(gc->owner);
+}
+
 static void msm_gpio_irq_handler(struct irq_desc *desc)
 {
 	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
@@ -920,6 +955,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
-- 
Sent by a computer through tubes

Re: [PATCH 3/3] pinctrl: msm: Configure interrupts as input and gpio mode

[Doug Anderson]

Hi,

On Mon, Jun 18, 2018 at 1:52 PM, Stephen Boyd <swboyd@chromium.org> wrote:
> When requesting a gpio as an interrupt, we should make sure to mux the
> pin as the GPIO function and configure it to be an input so that various
> functions or output signals don't affect the interrupt state of the pin.
> So far, we've relied on pinmux configurations in DT to handle this, but
> let's explicitly configure this in the code so that DT implementers
> don't have to get this part right.
>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 37 ++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)

This seems like a reasonable idea to me.  I'm not a huge fan of all
the boilerplate code copied from gpiochip_irq_reqres() and
gpiochip_irq_relres(), but it looks like that's the way it's done at
the moment.  Thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>