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

[PATCH v3 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.

The discussion never really completed on the previous thread so I'm just
resending these patches to restart the conversation. In the cases
where a driver needs to do both pinctrl muxing for some non-gpio
function and also GPIO control they'll need to explicitly mux the pins
at the right time. If we force them to mux the pins into the function
mode after requesting the GPIO at boot then we'll be better off because
it will force the code to mux out the function or GPIO explicitly all
the time.

We will have a case in the near future where the UART driver will want
to mux the RX pin into GPIO mode so it can get a wakeup interrupt during
suspend path and then swizzle the pin back into QUP/UART mode when the
wakeup interrupt isn't necessary anymore. In this case, I imagine the
driver will request the pin as an interrupt during probe, that will
convert the GPIO into an irq and mux it out as a GPIO function input
pin, disable that IRQ because it's only needed at suspend time, and then
need to explicitly mux the device into "UART" mode before finishing
driver probe. Then when it goes into suspend, it will need to remux the
pin as a GPIO function input pin, enable the irq, and wait for wakeup.
On resume, it will disable the irq and remux the pin as "UART" mode.

Changes from v2:
 * Better comment in patch#1 to describe this stuff
 * Squashed the raw status bit part into the same write in mask path
   based on suggestion from Doug Andersson

Changes from v1:
 * Squashed the raw status bit part into the same write in unmask path
   based on suggestion from Doug Andersson

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 | 77 ++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

-- 
Sent by a computer through tubes

[PATCH v3 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 | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 2155a30c282b..5d72ffad32c2 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -634,6 +634,29 @@ 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);
+	/*
+	 * There are two bits that control interrupt forwarding to the CPU. The
+	 * RAW_STATUS_EN bit causes the level or edge sensed on the line to be
+	 * latched into the interrupt status register when the hardware detects
+	 * an irq that it's configured for (either edge for edge type or level
+	 * for level type irq). The 'non-raw' status enable bit causes the
+	 * hardware to assert the summary interrupt to the CPU if the latched
+	 * status bit is set. There's a bug though, the edge detection logic
+	 * seems to have a problem where toggling the RAW_STATUS_EN bit may
+	 * cause the status bit to latch spuriously when there isn't any edge
+	 * so we can't touch that bit for edge type irqs and we have to keep
+	 * the bit set anyway so that edges are latched while the line is masked.
+	 *
+	 * To make matters more complicated, leaving the RAW_STATUS_EN bit
+	 * enabled all the time causes level interrupts to re-latch into the
+	 * status register because the level is still present on the line after
+	 * we ack it. We clear the raw status enable bit during mask here and
+	 * set the bit on unmask so the interrupt can't latch into the hardware
+	 * while it's masked.
+	 */
+	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
+		val &= ~BIT(g->intr_raw_status_bit);
+
 	val &= ~BIT(g->intr_enable_bit);
 	writel(val, pctrl->regs + g->intr_cfg_reg);
 
@@ -655,6 +678,7 @@ 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);
+	val |= BIT(g->intr_raw_status_bit);
 	val |= BIT(g->intr_enable_bit);
 	writel(val, pctrl->regs + g->intr_cfg_reg);
 
-- 
Sent by a computer through tubes

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

[Doug Anderson]

Hi,

On Thu, Aug 16, 2018 at 1:06 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 | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

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

NOTE: IMO we should land this fix even if we continue to have debate
on patch #2 and #3 since this fixes a definite problem.

-Doug

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

[Linus Walleij]

On Thu, Aug 16, 2018 at 10:49 PM Doug Anderson <dianders@chromium.org> wrote:
> On Thu, Aug 16, 2018 at 1:06 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 | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> NOTE: IMO we should land this fix even if we continue to have debate
> on patch #2 and #3 since this fixes a definite problem.

OK makes sense, I guess I'll queue this for fixes
once v4.19-rc1 is out.

Would be nice to also get Bjorn's buy-in on it!

Yours,
Linus Walleij

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

[Bjorn Andersson]

On Thu 16 Aug 13:06 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>

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

Regards,
Bjorn

> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2155a30c282b..5d72ffad32c2 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -634,6 +634,29 @@ 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);
> +	/*
> +	 * There are two bits that control interrupt forwarding to the CPU. The
> +	 * RAW_STATUS_EN bit causes the level or edge sensed on the line to be
> +	 * latched into the interrupt status register when the hardware detects
> +	 * an irq that it's configured for (either edge for edge type or level
> +	 * for level type irq). The 'non-raw' status enable bit causes the
> +	 * hardware to assert the summary interrupt to the CPU if the latched
> +	 * status bit is set. There's a bug though, the edge detection logic
> +	 * seems to have a problem where toggling the RAW_STATUS_EN bit may
> +	 * cause the status bit to latch spuriously when there isn't any edge
> +	 * so we can't touch that bit for edge type irqs and we have to keep
> +	 * the bit set anyway so that edges are latched while the line is masked.
> +	 *
> +	 * To make matters more complicated, leaving the RAW_STATUS_EN bit
> +	 * enabled all the time causes level interrupts to re-latch into the
> +	 * status register because the level is still present on the line after
> +	 * we ack it. We clear the raw status enable bit during mask here and
> +	 * set the bit on unmask so the interrupt can't latch into the hardware
> +	 * while it's masked.
> +	 */
> +	if (irqd_get_trigger_type(d) & IRQ_TYPE_LEVEL_MASK)
> +		val &= ~BIT(g->intr_raw_status_bit);
> +
>  	val &= ~BIT(g->intr_enable_bit);
>  	writel(val, pctrl->regs + g->intr_cfg_reg);
>  
> @@ -655,6 +678,7 @@ 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);
> +	val |= BIT(g->intr_raw_status_bit);
>  	val |= BIT(g->intr_enable_bit);
>  	writel(val, pctrl->regs + g->intr_cfg_reg);
>  
> -- 
> Sent by a computer through tubes
> 

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

[Linus Walleij]

On Thu, Aug 16, 2018 at 10:06 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>

This patch applied for fixes with Doug and Bjorn's ACKs.

I suppose you will respin the two others and obtain buy-in from
the same people for these.

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Aug 29, 2018 at 9:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> This patch applied for fixes with Doug and Bjorn's ACKs.
>
> I suppose you will respin the two others and obtain buy-in from
> the same people for these.

Scrap that. I saw Bjorn has ACKed the two others so applied them for
next (v4.20).

Yours,
Linus Walleij

[PATCH v3 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 5d72ffad32c2..793504057ad0 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 v3 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Bjorn Andersson]

On Thu 16 Aug 13:06 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>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

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

Regards,
Bjorn

> ---
>  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 5d72ffad32c2..793504057ad0 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 v3 2/3] pinctrl: msm: Mux out gpio function with gpio_request()

[Linus Walleij]

On Thu, Aug 16, 2018 at 10:06 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>

Patch applied for v4.20 with Bjorn's ACK.

Yours,
Linus Walleij

[PATCH v3 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 793504057ad0..defed34d32b0 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -837,6 +837,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);
@@ -935,6 +970,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 v3 3/3] pinctrl: msm: Configure interrupts as input and gpio mode

[Bjorn Andersson]

On Thu 16 Aug 13:06 PDT 2018, Stephen Boyd 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>

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

Regards,
Bjorn

> ---
>  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 793504057ad0..defed34d32b0 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -837,6 +837,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);
> @@ -935,6 +970,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 v3 3/3] pinctrl: msm: Configure interrupts as input and gpio mode

[Linus Walleij]

On Thu, Aug 16, 2018 at 10:06 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>

Patch applied for v4.20 with Bjorn's ACK.

Yours,
Linus Walleij

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

[Niklas Cassel]

Hello Stephen,

I'm getting this warning on dragonboard 820c (msm8996) when booting linux-next:

[    3.211575] WARNING: CPU: 1 PID: 1 at drivers/pinctrl/qcom/pinctrl-msm.c:164 msm_pinmux_set_mux+0xc8/0x150
[    3.212127] l28: ramp_delay not set
[    3.215146] Modules linked in:
[    3.215168] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W         4.19.0-rc2-next-2018904-00004-gd8c6804d9e9b #54
[    3.215175] Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
[    3.215184] pstate: 60400005 (nZCv daif +PAN -UAO)
[    3.215197] pc : msm_pinmux_set_mux+0xc8/0x150
[    3.215206] lr : msm_pinmux_set_mux+0x30/0x150
[    3.215213] sp : ffff00000803b880
[    3.224815] l28: 925 mV
[    3.228149] x29: ffff00000803b880 x28: ffff00000978a22c
[    3.228172] x27: ffff000009f22000 x26: ffff000009640764
[    3.228193] x25: 0000000000000000
[    3.241956] x24: ffff8000d93f5f18
[    3.241971] x23: ffff8000d93fec18 x22: 0000000000000000
[    3.241992] x21: ffff000009100d10 x20: 000000000000003c
[    3.252891] x19: 000000000000000a x18: ffffffffffffffff
[    3.252913] x17: 00000000000017c7 x16: ffff000009fdaac0
[    3.252935] x15: ffff000009cbe1c8
[    3.257467] l29: supplied by regulator-dummy
[    3.261743] x14: ffff8000d85fea14
[    3.261757] x13: ffff8000d85fea13 x12: ffff8000d9f488b0
[    3.261778] x11: 0000000005f5e0ff x10: 0000000000000040
[    3.267734] x9 : ffff8000d91bae18 x8 : ffff8000d981efe8
[    3.267755] x7 : ffff8000d981f128 x6 : 000000000000000a
[    3.267776] x5 : 0000000000000040
[    3.273347] l29: Bringing 0uV into 2800000-2800000uV
[    3.278323] x4 : ffffffffffffffff
[    3.278338] x3 : 000000000000000a x2 : 00000000ffffffc6
[    3.278359] x1 : ffff000009d3eec8 x0 : 00000000000000f3
[    3.282026] l29: ramp_delay not set
[    3.292566] Call trace:
[    3.292581]  msm_pinmux_set_mux+0xc8/0x150
[    3.292590]  msm_pinmux_request_gpio+0x5c/0x68
[    3.292599]  pin_request+0x154/0x2c0
[    3.292608]  pinmux_request_gpio+0x60/0xa0
[    3.296080] l29: 2800 mV
[    3.301503]  pinctrl_gpio_request+0x148/0x1d8
[    3.301514]  gpiochip_generic_request+0x2c/0x38
[    3.301523]  gpiod_request_commit+0xc4/0x1a0
[    3.301532]  gpiod_request+0x6c/0x110
[    3.301544]  gpiod_get_index+0x134/0x360
[    3.322166]  devm_gpiod_get_index+0x64/0xc0
[    3.322174]  devm_gpiod_get_optional+0x38/0x58
[    3.322185]  qcom_pcie_probe+0xa4/0x230
[    3.322197]  platform_drv_probe+0x58/0xa8
[    3.322207]  really_probe+0x280/0x3d8
[    3.322215]  driver_probe_device+0x60/0x148
[    3.322224]  __driver_attach+0x144/0x148
[    3.322233]  bus_for_each_dev+0x84/0xd8
[    3.322241]  driver_attach+0x30/0x40
[    3.322250]  bus_add_driver+0x234/0x2a8
[    3.322258]  driver_register+0x64/0x110
[    3.322267]  __platform_driver_register+0x54/0x60
[    3.322279]  qcom_pcie_driver_init+0x20/0x28
[    3.322290]  do_one_initcall+0x94/0x3f8
[    3.322301]  kernel_init_freeable+0x47c/0x528
[    3.322315]  kernel_init+0x18/0x110
[    3.322323]  ret_from_fork+0x10/0x1c


If I revert patch 2/3 and 3/3 in this series, the warning goes away.


Kind regards,
Niklas

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

[Stephen Boyd]

Quoting Niklas Cassel (2018-09-12 03:34:31)
> Hello Stephen,
> 
> I'm getting this warning on dragonboard 820c (msm8996) when booting linux-next:
> 
> [    3.211575] WARNING: CPU: 1 PID: 1 at drivers/pinctrl/qcom/pinctrl-msm.c:164 msm_pinmux_set_mux+0xc8/0x150
> [    3.212127] l28: ramp_delay not set

Thanks for testing. I think the problem is I messed up the function 0
and enum 0. Can you try this patch?

--8<---
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 1684b2da09d5..b925b8feac95 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -188,7 +188,7 @@ static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
 		return 0;
 
 	/* For now assume function 0 is GPIO because it always is */
-	return msm_pinmux_set_mux(pctldev, 0, offset);
+	return msm_pinmux_set_mux(pctldev, g->funcs[0], offset);
 }
 
 static const struct pinmux_ops msm_pinmux_ops = {

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

[Niklas Cassel]

On Mon, Oct 01, 2018 at 11:46:57AM -0700, Stephen Boyd wrote:
> Quoting Niklas Cassel (2018-09-12 03:34:31)
> > Hello Stephen,
> > 
> > I'm getting this warning on dragonboard 820c (msm8996) when booting linux-next:
> > 
> > [    3.211575] WARNING: CPU: 1 PID: 1 at drivers/pinctrl/qcom/pinctrl-msm.c:164 msm_pinmux_set_mux+0xc8/0x150
> > [    3.212127] l28: ramp_delay not set
> 
> Thanks for testing. I think the problem is I messed up the function 0
> and enum 0. Can you try this patch?

Hello Stephen,

This patch does indeed remove the warning.
Thanks!

Kind regards,
Niklas

> 
> --8<---
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1684b2da09d5..b925b8feac95 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -188,7 +188,7 @@ static int msm_pinmux_request_gpio(struct pinctrl_dev *pctldev,
>  		return 0;
>  
>  	/* For now assume function 0 is GPIO because it always is */
> -	return msm_pinmux_set_mux(pctldev, 0, offset);
> +	return msm_pinmux_set_mux(pctldev, g->funcs[0], offset);
>  }
>  
>  static const struct pinmux_ops msm_pinmux_ops = {