pinctrl: qcom: Handle broken PDC dual edge case on sc7180

Commit Message

Doug Anderson July 8, 2020, 9:16 p.m. UTC

As per Qualcomm, there is a PDC hardware issue (with the specific IP
rev that exists on sc7180) that causes the PDC not to work properly
when configured to handle dual edges.

Let's work around this by emulating only ever letting our parent see
requests for single edge interrupts on affected hardware.

Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Douglas Anderson <[e-mail]>
---
As far as I can tell everything here should work and the limited
testing I'm able to give it shows that, in fact, I can detect both
edges.

Please give this an extra thorough review since it's trying to find
the exact right place to insert this code and I'm not massively
familiar with all the frameworks.

If someone has hardware where it's easy to stress test this that'd be
wonderful too.  The board I happen to have in front of me doesn't have
any easy-to-toggle GPIOs where I can just poke a button or a switch to
generate edges.  My testing was done by hacking the "write protect"
GPIO on my board into gpio-keys as a dual-edge interrupt and then
sending commands to our security chip to toggle it--not exactly great
for testing to make sure there are no race conditions if the interrupt
bounces a lot.

 drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
 drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
 3 files changed, 85 insertions(+)

Comments

kernel test robot July 9, 2020, 12:52 a.m. UTC | #1

Hi Douglas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pinctrl/devel]
[also build test WARNING on v5.8-rc4 next-20200708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Douglas-Anderson/pinctrl-qcom-Handle-broken-PDC-dual-edge-case-on-sc7180/20200709-051816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/pinctrl/qcom/pinctrl-msm.c:914:6: warning: no previous prototype for 'msm_gpio_handle_dual_edge_parent_irq' [-Wmissing-prototypes]
     914 | void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/msm_gpio_handle_dual_edge_parent_irq +914 drivers/pinctrl/qcom/pinctrl-msm.c

   913	
 > 914	void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
   915	{
   916		struct irq_data	*d = &desc->irq_data;
   917	
   918		/* Make sure we're primed for the next edge */
   919		msm_gpio_update_dual_edge_parent(d);
   920	
   921		/* Pass on to the normal interrupt handler */
   922		handle_fasteoi_irq(desc);
   923	}
   924	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Doug Anderson July 9, 2020, 1:55 p.m. UTC | #2

Hi,

On Wed, Jul 8, 2020 at 5:53 PM kernel test robot <[e-mail]> wrote:
>
> Hi Douglas,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on pinctrl/devel]
> [also build test WARNING on v5.8-rc4 next-20200708]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use  as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/0day-ci/linux/commits/Douglas-Anderson/pinctrl-qcom-Handle-broken-PDC-dual-edge-case-on-sc7180/20200709-051816
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
> config: arm-defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[e-mail]>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/pinctrl/qcom/pinctrl-msm.c:914:6: warning: no previous prototype for 'msm_gpio_handle_dual_edge_parent_irq' [-Wmissing-prototypes]
>      914 | void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
>          |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> vim +/msm_gpio_handle_dual_edge_parent_irq +914 drivers/pinctrl/qcom/pinctrl-msm.c
>
>    913
>  > 914  void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)

This is just because I'm missing a "static".

For now I'll hold-off on reposting and wait for additional feedback.
If I don't hear anything for a little while I'll re-post with this
fix.

-Doug

Bjorn Andersson July 9, 2020, 3 p.m. UTC | #3

On Wed 08 Jul 14:16 PDT 2020, Douglas Anderson wrote:

> As per Qualcomm, there is a PDC hardware issue (with the specific IP
> rev that exists on sc7180) that causes the PDC not to work properly
> when configured to handle dual edges.
> 
> Let's work around this by emulating only ever letting our parent see
> requests for single edge interrupts on affected hardware.
> 
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Douglas Anderson <[e-mail]>

Looks good to me, but did even less testing than you.

Reviewed-by: Bjorn Andersson <[e-mail]>

Regards,
Bjorn

> ---
> As far as I can tell everything here should work and the limited
> testing I'm able to give it shows that, in fact, I can detect both
> edges.
> 
> Please give this an extra thorough review since it's trying to find
> the exact right place to insert this code and I'm not massively
> familiar with all the frameworks.
> 
> If someone has hardware where it's easy to stress test this that'd be
> wonderful too.  The board I happen to have in front of me doesn't have
> any easy-to-toggle GPIOs where I can just poke a button or a switch to
> generate edges.  My testing was done by hacking the "write protect"
> GPIO on my board into gpio-keys as a dual-edge interrupt and then
> sending commands to our security chip to toggle it--not exactly great
> for testing to make sure there are no race conditions if the interrupt
> bounces a lot.
> 
>  drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
>  drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
>  drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
>  3 files changed, 85 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 83b7d64bc4c1..45ca09ebb7b3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  }
>  
> +/**
> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> + * @d: The irq dta.
> + *
> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> + * normally handled by the parent irqchip.  The logic here is slightly
> + * different due to what's easy to do with our parent, but in principle it's
> + * the same.
> + */
> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +	unsigned long flags;
> +	int loop_limit = 100;
> +	unsigned int val;
> +	unsigned int type;
> +
> +	/* Read the value and make a guess about what edge we need to catch */
> +	val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +	type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	do {
> +		/* Set the parent to catch the next edge */
> +		irq_chip_set_type_parent(d, type);
> +
> +		/*
> +		 * Possibly the line changed between when we last read "val"
> +		 * (and decided what edge we needed) and when set the edge.
> +		 * If the value didn't change (or changed and then changed
> +		 * back) then we're done.
> +		 */
> +		val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +		if (type == IRQ_TYPE_EDGE_RISING) {
> +			if (!val)
> +				break;
> +			type = IRQ_TYPE_EDGE_FALLING;
> +		} else if (type == IRQ_TYPE_EDGE_FALLING) {
> +			if (val)
> +				break;
> +			type = IRQ_TYPE_EDGE_RISING;
> +		}
> +	} while (loop_limit-- > 0);
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (!loop_limit)
> +		dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> +}
> +
> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> +{
> +	struct irq_data	*d = &desc->irq_data;
> +
> +	/* Make sure we're primed for the next edge */
> +	msm_gpio_update_dual_edge_parent(d);
> +
> +	/* Pass on to the normal interrupt handler */
> +	handle_fasteoi_irq(desc);
> +}
> +
> +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> +						       unsigned int type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> +	return type == IRQ_TYPE_EDGE_BOTH &&
> +	       pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> +	       test_bit(d->hwirq, pctrl->skip_wake_irqs);
> +}
> +
>  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  	unsigned long flags;
>  	u32 val;
>  
> +	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> +		irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> +		msm_gpio_update_dual_edge_parent(d);
> +
> +		return 0;
> +	}
> +
>  	if (d->parent_data)
>  		irq_chip_set_type_parent(d, type);
>  
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9452da18a78b..7486fe08eb9b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
>   * @pull_no_keeper: The SoC does not support keeper bias.
>   * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>   * @nwakeirq_map:   The number of entries in @wakeirq_map
> + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> + *                            to be aware that their parent can't handle dual
> + *                            edge interrupts.
>   */
>  struct msm_pinctrl_soc_data {
>  	const struct pinctrl_pin_desc *pins;
> @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
>  	const int *reserved_gpios;
>  	const struct msm_gpio_wakeirq_map *wakeirq_map;
>  	unsigned int nwakeirq_map;
> +	bool wakeirq_dual_edge_errata;
>  };
>  
>  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> index 1b6465a882f2..1d9acad3c1ce 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
>  	.ntiles = ARRAY_SIZE(sc7180_tiles),
>  	.wakeirq_map = sc7180_pdc_map,
>  	.nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> +	.wakeirq_dual_edge_errata = true,
>  };
>  
>  static int sc7180_pinctrl_probe(struct platform_device *pdev)
> -- 
> 2.27.0.383.g050319c2ae-goog
>

Rajendra Nayak July 10, 2020, 4:27 a.m. UTC | #4

On 7/9/2020 2:46 AM, Douglas Anderson wrote:
> As per Qualcomm, there is a PDC hardware issue (with the specific IP
> rev that exists on sc7180) that causes the PDC not to work properly
> when configured to handle dual edges.
> 
> Let's work around this by emulating only ever letting our parent see
> requests for single edge interrupts on affected hardware.
> 
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Douglas Anderson <[e-mail]>

Thanks Doug, this looks like a much better solution than what I was
proposing :)

Reviewed-by: Rajendra Nayak <[e-mail]>

> ---
> As far as I can tell everything here should work and the limited
> testing I'm able to give it shows that, in fact, I can detect both
> edges.
> 
> Please give this an extra thorough review since it's trying to find
> the exact right place to insert this code and I'm not massively
> familiar with all the frameworks.
> 
> If someone has hardware where it's easy to stress test this that'd be
> wonderful too.  The board I happen to have in front of me doesn't have
> any easy-to-toggle GPIOs where I can just poke a button or a switch to
> generate edges.  My testing was done by hacking the "write protect"
> GPIO on my board into gpio-keys as a dual-edge interrupt and then
> sending commands to our security chip to toggle it--not exactly great
> for testing to make sure there are no race conditions if the interrupt
> bounces a lot.
> 
>   drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
>   drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
>   drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
>   3 files changed, 85 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 83b7d64bc4c1..45ca09ebb7b3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>   	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>   }
>   
> +/**
> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> + * @d: The irq dta.
> + *
> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> + * normally handled by the parent irqchip.  The logic here is slightly
> + * different due to what's easy to do with our parent, but in principle it's
> + * the same.
> + */
> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +	unsigned long flags;
> +	int loop_limit = 100;
> +	unsigned int val;
> +	unsigned int type;
> +
> +	/* Read the value and make a guess about what edge we need to catch */
> +	val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +	type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
> +	do {
> +		/* Set the parent to catch the next edge */
> +		irq_chip_set_type_parent(d, type);
> +
> +		/*
> +		 * Possibly the line changed between when we last read "val"
> +		 * (and decided what edge we needed) and when set the edge.
> +		 * If the value didn't change (or changed and then changed
> +		 * back) then we're done.
> +		 */
> +		val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +		if (type == IRQ_TYPE_EDGE_RISING) {
> +			if (!val)
> +				break;
> +			type = IRQ_TYPE_EDGE_FALLING;
> +		} else if (type == IRQ_TYPE_EDGE_FALLING) {
> +			if (val)
> +				break;
> +			type = IRQ_TYPE_EDGE_RISING;
> +		}
> +	} while (loop_limit-- > 0);
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (!loop_limit)
> +		dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> +}
> +
> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> +{
> +	struct irq_data	*d = &desc->irq_data;
> +
> +	/* Make sure we're primed for the next edge */
> +	msm_gpio_update_dual_edge_parent(d);
> +
> +	/* Pass on to the normal interrupt handler */
> +	handle_fasteoi_irq(desc);
> +}
> +
> +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> +						       unsigned int type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> +	return type == IRQ_TYPE_EDGE_BOTH &&
> +	       pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> +	       test_bit(d->hwirq, pctrl->skip_wake_irqs);
> +}
> +
>   static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   {
>   	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	unsigned long flags;
>   	u32 val;
>   
> +	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> +		irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> +		msm_gpio_update_dual_edge_parent(d);
> +
> +		return 0;
> +	}
> +
>   	if (d->parent_data)
>   		irq_chip_set_type_parent(d, type);
>   
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9452da18a78b..7486fe08eb9b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
>    * @pull_no_keeper: The SoC does not support keeper bias.
>    * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>    * @nwakeirq_map:   The number of entries in @wakeirq_map
> + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> + *                            to be aware that their parent can't handle dual
> + *                            edge interrupts.
>    */
>   struct msm_pinctrl_soc_data {
>   	const struct pinctrl_pin_desc *pins;
> @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
>   	const int *reserved_gpios;
>   	const struct msm_gpio_wakeirq_map *wakeirq_map;
>   	unsigned int nwakeirq_map;
> +	bool wakeirq_dual_edge_errata;
>   };
>   
>   extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> index 1b6465a882f2..1d9acad3c1ce 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
>   	.ntiles = ARRAY_SIZE(sc7180_tiles),
>   	.wakeirq_map = sc7180_pdc_map,
>   	.nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> +	.wakeirq_dual_edge_errata = true,
>   };
>   
>   static int sc7180_pinctrl_probe(struct platform_device *pdev)
>

Maulik Shah July 10, 2020, 5:09 a.m. UTC | #5

Hi Doug,

On 7/9/2020 2:46 AM, Douglas Anderson wrote:
> As per Qualcomm, there is a PDC hardware issue (with the specific IP
> rev that exists on sc7180) that causes the PDC not to work properly
> when configured to handle dual edges.
>
> Let's work around this by emulating only ever letting our parent see
> requests for single edge interrupts on affected hardware.
>
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Douglas Anderson <[e-mail]>
> ---
> As far as I can tell everything here should work and the limited
> testing I'm able to give it shows that, in fact, I can detect both
> edges.
>
> Please give this an extra thorough review since it's trying to find
> the exact right place to insert this code and I'm not massively
> familiar with all the frameworks.
>
> If someone has hardware where it's easy to stress test this that'd be
> wonderful too.  The board I happen to have in front of me doesn't have
> any easy-to-toggle GPIOs where I can just poke a button or a switch to
> generate edges.  My testing was done by hacking the "write protect"
> GPIO on my board into gpio-keys as a dual-edge interrupt and then
> sending commands to our security chip to toggle it--not exactly great
> for testing to make sure there are no race conditions if the interrupt
> bounces a lot.
>
>   drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
>   drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
>   drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
>   3 files changed, 85 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 83b7d64bc4c1..45ca09ebb7b3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>   	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>   }
>   
> +/**
> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> + * @d: The irq dta.
> + *
> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> + * normally handled by the parent irqchip.  The logic here is slightly
> + * different due to what's easy to do with our parent, but in principle it's
> + * the same.
> + */
> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +	unsigned long flags;
> +	int loop_limit = 100;
> +	unsigned int val;
> +	unsigned int type;
> +
> +	/* Read the value and make a guess about what edge we need to catch */
> +	val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +	type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);
can you please move this spinlock covering above two lines as well?
> +	do {
> +		/* Set the parent to catch the next edge */
> +		irq_chip_set_type_parent(d, type);
> +
> +		/*
> +		 * Possibly the line changed between when we last read "val"
> +		 * (and decided what edge we needed) and when set the edge.
> +		 * If the value didn't change (or changed and then changed
> +		 * back) then we're done.
> +		 */
> +		val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +		if (type == IRQ_TYPE_EDGE_RISING) {
> +			if (!val)
> +				break;
> +			type = IRQ_TYPE_EDGE_FALLING;
> +		} else if (type == IRQ_TYPE_EDGE_FALLING) {
> +			if (val)
> +				break;
> +			type = IRQ_TYPE_EDGE_RISING;
> +		}
> +	} while (loop_limit-- > 0);
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (!loop_limit)
> +		dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");

you will never enter this if condtion since loop_limit will become 
negative value in above do..while loop.

need to update this check to if (loop_limit <= 0)

other than above comment this change looks good to me.

Reviewed-by: Maulik Shah <[e-mail]>

Tested-by: Maulik Shah <[e-mail]>

Thanks,
Maulik

> +}
> +
> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> +{
> +	struct irq_data	*d = &desc->irq_data;
> +
> +	/* Make sure we're primed for the next edge */
> +	msm_gpio_update_dual_edge_parent(d);
> +
> +	/* Pass on to the normal interrupt handler */
> +	handle_fasteoi_irq(desc);
> +}
> +
> +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> +						       unsigned int type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> +	return type == IRQ_TYPE_EDGE_BOTH &&
> +	       pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> +	       test_bit(d->hwirq, pctrl->skip_wake_irqs);
> +}
> +
>   static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   {
>   	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>   	unsigned long flags;
>   	u32 val;
>   
> +	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> +		irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> +		msm_gpio_update_dual_edge_parent(d);
> +
> +		return 0;
> +	}
> +
>   	if (d->parent_data)
>   		irq_chip_set_type_parent(d, type);
>   
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9452da18a78b..7486fe08eb9b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
>    * @pull_no_keeper: The SoC does not support keeper bias.
>    * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>    * @nwakeirq_map:   The number of entries in @wakeirq_map
> + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> + *                            to be aware that their parent can't handle dual
> + *                            edge interrupts.
>    */
>   struct msm_pinctrl_soc_data {
>   	const struct pinctrl_pin_desc *pins;
> @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
>   	const int *reserved_gpios;
>   	const struct msm_gpio_wakeirq_map *wakeirq_map;
>   	unsigned int nwakeirq_map;
> +	bool wakeirq_dual_edge_errata;
>   };
>   
>   extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> index 1b6465a882f2..1d9acad3c1ce 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
>   	.ntiles = ARRAY_SIZE(sc7180_tiles),
>   	.wakeirq_map = sc7180_pdc_map,
>   	.nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> +	.wakeirq_dual_edge_errata = true,
>   };
>   
>   static int sc7180_pinctrl_probe(struct platform_device *pdev)

Marc Zyngier July 10, 2020, 9:03 a.m. UTC | #6

Hi Doug,

On Wed, 08 Jul 2020 22:16:25 +0100,
Douglas Anderson <[e-mail]> wrote:
> 
> As per Qualcomm, there is a PDC hardware issue (with the specific IP
> rev that exists on sc7180) that causes the PDC not to work properly
> when configured to handle dual edges.
> 
> Let's work around this by emulating only ever letting our parent see
> requests for single edge interrupts on affected hardware.
> 
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Douglas Anderson <[e-mail]>
> ---
> As far as I can tell everything here should work and the limited
> testing I'm able to give it shows that, in fact, I can detect both
> edges.
> 
> Please give this an extra thorough review since it's trying to find
> the exact right place to insert this code and I'm not massively
> familiar with all the frameworks.
> 
> If someone has hardware where it's easy to stress test this that'd be
> wonderful too.  The board I happen to have in front of me doesn't have
> any easy-to-toggle GPIOs where I can just poke a button or a switch to
> generate edges.  My testing was done by hacking the "write protect"
> GPIO on my board into gpio-keys as a dual-edge interrupt and then
> sending commands to our security chip to toggle it--not exactly great
> for testing to make sure there are no race conditions if the interrupt
> bounces a lot.

This looks positively awful (the erratum, not the patch). Is there an
actual description of the problem, outlining the circumstances that
triggers this issue? The PDC really never fails to disappoint...

> 
>  drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
>  drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
>  drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
>  3 files changed, 85 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 83b7d64bc4c1..45ca09ebb7b3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  }
>  
> +/**
> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> + * @d: The irq dta.
> + *
> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> + * normally handled by the parent irqchip.  The logic here is slightly
> + * different due to what's easy to do with our parent, but in principle it's
> + * the same.
> + */
> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +	unsigned long flags;
> +	int loop_limit = 100;

I guess this is a "finger up in the air" type of limit?

> +	unsigned int val;
> +	unsigned int type;
> +
> +	/* Read the value and make a guess about what edge we need to catch */
> +	val = msm_readl_io(pctrl, g) & BIT(g->in_bit);

What does this value represent? The input line value?

> +	type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +
> +	raw_spin_lock_irqsave(&pctrl->lock, flags);

What is this lock protecting you against? In both cases, you are
already under the irq_desc lock, with interrupts disabled.

> +	do {
> +		/* Set the parent to catch the next edge */
> +		irq_chip_set_type_parent(d, type);
> +
> +		/*
> +		 * Possibly the line changed between when we last read "val"
> +		 * (and decided what edge we needed) and when set the edge.
> +		 * If the value didn't change (or changed and then changed
> +		 * back) then we're done.
> +		 */

If the line changed, shouldn't you actually inject a new interrupt
altogether? By changing the polarity more than once, you are
effectively loosing edges that should have triggered an interrupt.

> +		val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +		if (type == IRQ_TYPE_EDGE_RISING) {
> +			if (!val)
> +				break;
> +			type = IRQ_TYPE_EDGE_FALLING;
> +		} else if (type == IRQ_TYPE_EDGE_FALLING) {
> +			if (val)
> +				break;
> +			type = IRQ_TYPE_EDGE_RISING;
> +		}
> +	} while (loop_limit-- > 0);
> +	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (!loop_limit)
> +		dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> +}
> +
> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> +{
> +	struct irq_data	*d = &desc->irq_data;
> +
> +	/* Make sure we're primed for the next edge */
> +	msm_gpio_update_dual_edge_parent(d);

I would have expected this to happen on EOI or ACK, rather than before
the flow is actually handled, once you have told the interrupt
controller that you were dealing with this interrupt.

> +
> +	/* Pass on to the normal interrupt handler */
> +	handle_fasteoi_irq(desc);

Is that the right flow? It seems that the current code is using
handle_edge_irq. I guess it has been broken so far, and that this
patch actually fixes it by forcing a fasteoi flow...

> +}
> +
> +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> +						       unsigned int type)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> +	return type == IRQ_TYPE_EDGE_BOTH &&
> +	       pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> +	       test_bit(d->hwirq, pctrl->skip_wake_irqs);
> +}
> +
>  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  	unsigned long flags;
>  	u32 val;
>  
> +	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> +		irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> +		msm_gpio_update_dual_edge_parent(d);
> +
> +		return 0;
> +	}
> +
>  	if (d->parent_data)
>  		irq_chip_set_type_parent(d, type);
>  
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9452da18a78b..7486fe08eb9b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
>   * @pull_no_keeper: The SoC does not support keeper bias.
>   * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>   * @nwakeirq_map:   The number of entries in @wakeirq_map
> + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> + *                            to be aware that their parent can't handle dual
> + *                            edge interrupts.
>   */
>  struct msm_pinctrl_soc_data {
>  	const struct pinctrl_pin_desc *pins;
> @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
>  	const int *reserved_gpios;
>  	const struct msm_gpio_wakeirq_map *wakeirq_map;
>  	unsigned int nwakeirq_map;
> +	bool wakeirq_dual_edge_errata;
>  };
>  
>  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> index 1b6465a882f2..1d9acad3c1ce 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
>  	.ntiles = ARRAY_SIZE(sc7180_tiles),
>  	.wakeirq_map = sc7180_pdc_map,
>  	.nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> +	.wakeirq_dual_edge_errata = true,
>  };
>  
>  static int sc7180_pinctrl_probe(struct platform_device *pdev)
> -- 
> 2.27.0.383.g050319c2ae-goog
> 
> 

Thanks,

	M.

Doug Anderson July 10, 2020, 4:10 p.m. UTC | #7

Hi,

On Fri, Jul 10, 2020 at 2:03 AM Marc Zyngier <[e-mail]> wrote:
>
> Hi Doug,
>
> On Wed, 08 Jul 2020 22:16:25 +0100,
> Douglas Anderson <[e-mail]> wrote:
> >
> > As per Qualcomm, there is a PDC hardware issue (with the specific IP
> > rev that exists on sc7180) that causes the PDC not to work properly
> > when configured to handle dual edges.
> >
> > Let's work around this by emulating only ever letting our parent see
> > requests for single edge interrupts on affected hardware.
> >
> > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> > Signed-off-by: Douglas Anderson <[e-mail]>
> > ---
> > As far as I can tell everything here should work and the limited
> > testing I'm able to give it shows that, in fact, I can detect both
> > edges.
> >
> > Please give this an extra thorough review since it's trying to find
> > the exact right place to insert this code and I'm not massively
> > familiar with all the frameworks.
> >
> > If someone has hardware where it's easy to stress test this that'd be
> > wonderful too.  The board I happen to have in front of me doesn't have
> > any easy-to-toggle GPIOs where I can just poke a button or a switch to
> > generate edges.  My testing was done by hacking the "write protect"
> > GPIO on my board into gpio-keys as a dual-edge interrupt and then
> > sending commands to our security chip to toggle it--not exactly great
> > for testing to make sure there are no race conditions if the interrupt
> > bounces a lot.
>
> This looks positively awful (the erratum, not the patch). Is there an
> actual description of the problem, outlining the circumstances that
> triggers this issue? The PDC really never fails to disappoint...

Hopefully someone from Qualcomm can chime in here.  My entire
knowledge of the errata comes from:

https://lore.kernel.org/r/[e-mail]

...and I tried to copy the exact phrasing that Rajendra gave.


> >  drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
> >  drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
> >  drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
> >  3 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 83b7d64bc4c1..45ca09ebb7b3 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
> >       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >  }
> >
> > +/**
> > + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> > + * @d: The irq dta.
> > + *
> > + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> > + * normally handled by the parent irqchip.  The logic here is slightly
> > + * different due to what's easy to do with our parent, but in principle it's
> > + * the same.
> > + */
> > +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > +     const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> > +     unsigned long flags;
> > +     int loop_limit = 100;
>
> I guess this is a "finger up in the air" type of limit?

Yes, the same "finger up in the air" as
msm_gpio_update_dual_edge_pos() in the same file.  My function comment
refers to the other function to try to tie them together at least a
little.


> > +     unsigned int val;
> > +     unsigned int type;
> > +
> > +     /* Read the value and make a guess about what edge we need to catch */
> > +     val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
>
> What does this value represent? The input line value?

Yes.  Coped from msm_gpio_update_dual_edge_pos().


> > +     type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > +
> > +     raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> What is this lock protecting you against? In both cases, you are
> already under the irq_desc lock, with interrupts disabled.

We are?  I put a breakpoint when the IRQ hits and did a bt.  I see
this (I happen to be on 5.4 at the moment, so hopefully the same as
mainline):

 kgdb_breakpoint+0x3c/0x74
 msm_gpio_update_dual_edge_parent+0x58/0x17c
 msm_gpio_handle_dual_edge_parent_irq+0x1c/0x30
 __handle_domain_irq+0x84/0xc4
 gic_handle_irq+0x170/0x220
 el1_irq+0xd0/0x180

I think the stack is missing a few things due to aggressive inlining
from my compiler, so the true backtrace would be:

msm_gpio_handle_dual_edge_parent_irq()
generic_handle_irq_desc()
generic_handle_irq()
__handle_domain_irq()
handle_domain_irq()
gic_handle_irq()

The first place that got the "desc" was generic_handle_irq() and it
got it via irq_to_desc().  That doesn't seem to do any locking.  Then
generic_handle_irq_desc() just calls a function pointer so no locking
there either.

...ah, but maybe what you're saying is that
msm_gpio_handle_dual_edge_parent_irq() should be holding "desc->lock"
around the call to msm_gpio_update_dual_edge_parent()?  I can do that.


> > +     do {
> > +             /* Set the parent to catch the next edge */
> > +             irq_chip_set_type_parent(d, type);
> > +
> > +             /*
> > +              * Possibly the line changed between when we last read "val"
> > +              * (and decided what edge we needed) and when set the edge.
> > +              * If the value didn't change (or changed and then changed
> > +              * back) then we're done.
> > +              */
>
> If the line changed, shouldn't you actually inject a new interrupt
> altogether? By changing the polarity more than once, you are
> effectively loosing edges that should have triggered an interrupt.

Are you sure this is needed?  My understanding of edge triggered
interrupts is that until the interrupt handler is called that all
edges can be coalesced into a single interrupt.  It's only after the
interrupt handler is called that it's important to capture new edges.
So if you have this:

a) Be busy processing another unrelated interrupt
b) 5 edges happen on the line
c) Other interrupt finishes
d) Edge interrupt is acked and handler is called

You'll only get one call to the interrupt handler even though there
were 5 edges, right?  It's only important that you queue another
interrupt if that interrupt happens after the true interrupt handler
(the one acting on the edge) has started.

...actually, in theory you'll get _either_ one or two calls to the
interrupt handler depending on timing, since the above could also
happen as:

a) Be busy processing another unrelated interrupt
b) 4 edges happen on the line
c) Other interrupt finishes
d) Edge interrupt is acked and ...
e) 1 more edge happens on the line
f) ...handler is called
g) Edge interrupt is acked and handler is called


As long as msm_gpio_update_dual_edge_parent() is called _before_ the
true interrupt handler is called then what I have should be fine,
right?


> > +             val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > +             if (type == IRQ_TYPE_EDGE_RISING) {
> > +                     if (!val)
> > +                             break;
> > +                     type = IRQ_TYPE_EDGE_FALLING;
> > +             } else if (type == IRQ_TYPE_EDGE_FALLING) {
> > +                     if (val)
> > +                             break;
> > +                     type = IRQ_TYPE_EDGE_RISING;
> > +             }
> > +     } while (loop_limit-- > 0);
> > +     raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > +     if (!loop_limit)
> > +             dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> > +}
> > +
> > +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> > +{
> > +     struct irq_data *d = &desc->irq_data;
> > +
> > +     /* Make sure we're primed for the next edge */
> > +     msm_gpio_update_dual_edge_parent(d);
>
> I would have expected this to happen on EOI or ACK, rather than before
> the flow is actually handled, once you have told the interrupt
> controller that you were dealing with this interrupt.

Having it on Ack would be ideal, but it appears that the Ack function
isn't called in this case.  That's only called if our handler is
handle_edge_irq() or handle_level_irq().  See more below.

...I'm pretty sure I don't want it on EOI.  Specifically, if I did it
on EOI then I think I _would_ need to re-queue another interrupt if an
edge came in msm_gpio_update_dual_edge_parent().  Doing all the edge
adjustment before calling the true interrupt handler avoids all that.


> > +
> > +     /* Pass on to the normal interrupt handler */
> > +     handle_fasteoi_irq(desc);
>
> Is that the right flow? It seems that the current code is using
> handle_edge_irq. I guess it has been broken so far, and that this
> patch actually fixes it by forcing a fasteoi flow...

The code today only uses handle_level_irq() / handle_edge_irq() if
"skip_wake_irqs" wasn't set for this IRQ.  In the case that
"skip_wake_irqs" wasn't set then it leaves the handler alone.  I
definitely had a hard time following all the flow and interactions
between the pinctrl, PDC, and the GICv3 but I definitely did confirm
that handle_fasteoi_irq() was the handler that was running when
"skip_wake_irqs" was set before I stuck mine in the middle.

I believe how things work today with the "skip_wake_irqs" case is
that, for the most part, the pinctrl driver stays out of the way for
setting up and handling IRQs and just passes some calls onto its
parent (the PDC).  The PDC driver is actually quite minimal.  There's
no "Ack" in there and no calls to set an IRQ handler--it seems to just
rely on the GICv3 doing all that.  It looks there is an implicit Ack
as part of gic_handle_irq() since reading the IAR counts as an Ack.


So to try to sum up my understanding:

1. In the case of "skip_wake_irqs" today there is no acking / handling
code that is part of pinctrl-msm or the PDC.  They just configure
things to direct to the GICv3.

2. For my workaround I just need to make sure to intercept myself and
prime the next edge _before_ the end-user interrupt handler gets
called.  If edges are coalesced before the end-user interrupt handler
is called then that's OK.


I'll await your reply before sending out the next version.  Thanks
much for all your time looking at this!


> > +}
> > +
> > +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> > +                                                    unsigned int type)
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > +
> > +     return type == IRQ_TYPE_EDGE_BOTH &&
> > +            pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> > +            test_bit(d->hwirq, pctrl->skip_wake_irqs);
> > +}
> > +
> >  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> >  {
> >       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> >       unsigned long flags;
> >       u32 val;
> >
> > +     if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> > +             irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> > +             msm_gpio_update_dual_edge_parent(d);
> > +
> > +             return 0;
> > +     }
> > +
> >       if (d->parent_data)
> >               irq_chip_set_type_parent(d, type);
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> > index 9452da18a78b..7486fe08eb9b 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> > @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
> >   * @pull_no_keeper: The SoC does not support keeper bias.
> >   * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
> >   * @nwakeirq_map:   The number of entries in @wakeirq_map
> > + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> > + *                            to be aware that their parent can't handle dual
> > + *                            edge interrupts.
> >   */
> >  struct msm_pinctrl_soc_data {
> >       const struct pinctrl_pin_desc *pins;
> > @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
> >       const int *reserved_gpios;
> >       const struct msm_gpio_wakeirq_map *wakeirq_map;
> >       unsigned int nwakeirq_map;
> > +     bool wakeirq_dual_edge_errata;
> >  };
> >
> >  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> > index 1b6465a882f2..1d9acad3c1ce 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> > @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
> >       .ntiles = ARRAY_SIZE(sc7180_tiles),
> >       .wakeirq_map = sc7180_pdc_map,
> >       .nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> > +     .wakeirq_dual_edge_errata = true,
> >  };
> >
> >  static int sc7180_pinctrl_probe(struct platform_device *pdev)
> > --
> > 2.27.0.383.g050319c2ae-goog
> >
> >
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Doug Anderson July 10, 2020, 4:11 p.m. UTC | #8

Hi,

On Thu, Jul 9, 2020 at 10:09 PM Maulik Shah <[e-mail]> wrote:
>
> Hi Doug,
>
> On 7/9/2020 2:46 AM, Douglas Anderson wrote:
> > As per Qualcomm, there is a PDC hardware issue (with the specific IP
> > rev that exists on sc7180) that causes the PDC not to work properly
> > when configured to handle dual edges.
> >
> > Let's work around this by emulating only ever letting our parent see
> > requests for single edge interrupts on affected hardware.
> >
> > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> > Signed-off-by: Douglas Anderson <[e-mail]>
> > ---
> > As far as I can tell everything here should work and the limited
> > testing I'm able to give it shows that, in fact, I can detect both
> > edges.
> >
> > Please give this an extra thorough review since it's trying to find
> > the exact right place to insert this code and I'm not massively
> > familiar with all the frameworks.
> >
> > If someone has hardware where it's easy to stress test this that'd be
> > wonderful too.  The board I happen to have in front of me doesn't have
> > any easy-to-toggle GPIOs where I can just poke a button or a switch to
> > generate edges.  My testing was done by hacking the "write protect"
> > GPIO on my board into gpio-keys as a dual-edge interrupt and then
> > sending commands to our security chip to toggle it--not exactly great
> > for testing to make sure there are no race conditions if the interrupt
> > bounces a lot.
> >
> >   drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
> >   drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
> >   drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
> >   3 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 83b7d64bc4c1..45ca09ebb7b3 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
> >       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >   }
> >
> > +/**
> > + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> > + * @d: The irq dta.
> > + *
> > + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> > + * normally handled by the parent irqchip.  The logic here is slightly
> > + * different due to what's easy to do with our parent, but in principle it's
> > + * the same.
> > + */
> > +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > +     const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> > +     unsigned long flags;
> > +     int loop_limit = 100;
> > +     unsigned int val;
> > +     unsigned int type;
> > +
> > +     /* Read the value and make a guess about what edge we need to catch */
> > +     val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > +     type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > +
> > +     raw_spin_lock_irqsave(&pctrl->lock, flags);
> can you please move this spinlock covering above two lines as well?

I could, but it wouldn't accomplish anything would it?  We're reading
the status of the pin straight from memory mapped registers.  Grabbing
a spinlock won't stop the pin from toggling.

...but from Mark's review I'll likely change the locking a bit anyway.


> > +     do {
> > +             /* Set the parent to catch the next edge */
> > +             irq_chip_set_type_parent(d, type);
> > +
> > +             /*
> > +              * Possibly the line changed between when we last read "val"
> > +              * (and decided what edge we needed) and when set the edge.
> > +              * If the value didn't change (or changed and then changed
> > +              * back) then we're done.
> > +              */
> > +             val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > +             if (type == IRQ_TYPE_EDGE_RISING) {
> > +                     if (!val)
> > +                             break;
> > +                     type = IRQ_TYPE_EDGE_FALLING;
> > +             } else if (type == IRQ_TYPE_EDGE_FALLING) {
> > +                     if (val)
> > +                             break;
> > +                     type = IRQ_TYPE_EDGE_RISING;
> > +             }
> > +     } while (loop_limit-- > 0);
> > +     raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > +     if (!loop_limit)
> > +             dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
>
> you will never enter this if condtion since loop_limit will become
> negative value in above do..while loop.
>
> need to update this check to if (loop_limit <= 0)
>
> other than above comment this change looks good to me.

Good catch, thanks!



> Reviewed-by: Maulik Shah <[e-mail]>
>
> Tested-by: Maulik Shah <[e-mail]>
>
> Thanks,
> Maulik
>
> > +}
> > +
> > +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> > +{
> > +     struct irq_data *d = &desc->irq_data;
> > +
> > +     /* Make sure we're primed for the next edge */
> > +     msm_gpio_update_dual_edge_parent(d);
> > +
> > +     /* Pass on to the normal interrupt handler */
> > +     handle_fasteoi_irq(desc);
> > +}
> > +
> > +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> > +                                                    unsigned int type)
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > +
> > +     return type == IRQ_TYPE_EDGE_BOTH &&
> > +            pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> > +            test_bit(d->hwirq, pctrl->skip_wake_irqs);
> > +}
> > +
> >   static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> >   {
> >       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> >       unsigned long flags;
> >       u32 val;
> >
> > +     if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> > +             irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> > +             msm_gpio_update_dual_edge_parent(d);
> > +
> > +             return 0;
> > +     }
> > +
> >       if (d->parent_data)
> >               irq_chip_set_type_parent(d, type);
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> > index 9452da18a78b..7486fe08eb9b 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> > @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
> >    * @pull_no_keeper: The SoC does not support keeper bias.
> >    * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
> >    * @nwakeirq_map:   The number of entries in @wakeirq_map
> > + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> > + *                            to be aware that their parent can't handle dual
> > + *                            edge interrupts.
> >    */
> >   struct msm_pinctrl_soc_data {
> >       const struct pinctrl_pin_desc *pins;
> > @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
> >       const int *reserved_gpios;
> >       const struct msm_gpio_wakeirq_map *wakeirq_map;
> >       unsigned int nwakeirq_map;
> > +     bool wakeirq_dual_edge_errata;
> >   };
> >
> >   extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> > index 1b6465a882f2..1d9acad3c1ce 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> > @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
> >       .ntiles = ARRAY_SIZE(sc7180_tiles),
> >       .wakeirq_map = sc7180_pdc_map,
> >       .nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> > +     .wakeirq_dual_edge_errata = true,
> >   };
> >
> >   static int sc7180_pinctrl_probe(struct platform_device *pdev)
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Marc Zyngier July 11, 2020, 9:16 a.m. UTC | #9

On Fri, 10 Jul 2020 17:10:55 +0100,
Doug Anderson <[e-mail]> wrote:
> 
> Hi,
> 
> On Fri, Jul 10, 2020 at 2:03 AM Marc Zyngier <[e-mail]> wrote:
> >
> > Hi Doug,

[...]

> >
> > > +     type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > > +
> > > +     raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > What is this lock protecting you against? In both cases, you are
> > already under the irq_desc lock, with interrupts disabled.
> 
> We are?  I put a breakpoint when the IRQ hits and did a bt.  I see
> this (I happen to be on 5.4 at the moment, so hopefully the same as
> mainline):
> 
>  kgdb_breakpoint+0x3c/0x74
>  msm_gpio_update_dual_edge_parent+0x58/0x17c
>  msm_gpio_handle_dual_edge_parent_irq+0x1c/0x30
>  __handle_domain_irq+0x84/0xc4
>  gic_handle_irq+0x170/0x220
>  el1_irq+0xd0/0x180
> 
> I think the stack is missing a few things due to aggressive inlining
> from my compiler, so the true backtrace would be:
> 
> msm_gpio_handle_dual_edge_parent_irq()
> generic_handle_irq_desc()
> generic_handle_irq()
> __handle_domain_irq()
> handle_domain_irq()
> gic_handle_irq()
> 
> The first place that got the "desc" was generic_handle_irq() and it
> got it via irq_to_desc().  That doesn't seem to do any locking.  Then
> generic_handle_irq_desc() just calls a function pointer so no locking
> there either.
> 
> ...ah, but maybe what you're saying is that
> msm_gpio_handle_dual_edge_parent_irq() should be holding "desc->lock"
> around the call to msm_gpio_update_dual_edge_parent()?  I can do that.

No, I mentally did a fast-forward to moving this hack into the irq
flow, rather than doing before entering the flow. handle_fasteoi_irq
will take the lock, but obviously not with the current state of this
patch.

>
> 
> > > +     do {
> > > +             /* Set the parent to catch the next edge */
> > > +             irq_chip_set_type_parent(d, type);
> > > +
> > > +             /*
> > > +              * Possibly the line changed between when we last read "val"
> > > +              * (and decided what edge we needed) and when set the edge.
> > > +              * If the value didn't change (or changed and then changed
> > > +              * back) then we're done.
> > > +              */
> >
> > If the line changed, shouldn't you actually inject a new interrupt
> > altogether? By changing the polarity more than once, you are
> > effectively loosing edges that should have triggered an interrupt.
> 
> Are you sure this is needed?  My understanding of edge triggered
> interrupts is that until the interrupt handler is called that all
> edges can be coalesced into a single interrupt.

It really depends on whether the edges are semantically different, and
I'm not sure you can decide this at the interrupt controller
level. The core IRQ code doesn't give you a way to discriminate
between those, but endpoint drivers could, and could get terminally
confused if the see two rising edges without a falling edge in
between.

> It's only after the
> interrupt handler is called that it's important to capture new edges.
> So if you have this:
> 
> a) Be busy processing another unrelated interrupt
> b) 5 edges happen on the line
> c) Other interrupt finishes
> d) Edge interrupt is acked and handler is called
> 
> You'll only get one call to the interrupt handler even though there
> were 5 edges, right?  It's only important that you queue another
> interrupt if that interrupt happens after the true interrupt handler
> (the one acting on the edge) has started.
> 
> ...actually, in theory you'll get _either_ one or two calls to the
> interrupt handler depending on timing, since the above could also
> happen as:
> 
> a) Be busy processing another unrelated interrupt
> b) 4 edges happen on the line
> c) Other interrupt finishes
> d) Edge interrupt is acked and ...
> e) 1 more edge happens on the line
> f) ...handler is called
> g) Edge interrupt is acked and handler is called
> 
> 
> As long as msm_gpio_update_dual_edge_parent() is called _before_ the
> true interrupt handler is called then what I have should be fine,
> right?

I don't disagree with any of that, except that being fine at the
irqchip level doesn't necessarily mean being fine at the endpoint
driver level. On the other hand, the HW looks terminally broken, so
maybe it doesn't matter as the drivers will have to be written with
this limitation in mind...

> 
> > > +             val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > > +             if (type == IRQ_TYPE_EDGE_RISING) {
> > > +                     if (!val)
> > > +                             break;
> > > +                     type = IRQ_TYPE_EDGE_FALLING;
> > > +             } else if (type == IRQ_TYPE_EDGE_FALLING) {
> > > +                     if (val)
> > > +                             break;
> > > +                     type = IRQ_TYPE_EDGE_RISING;
> > > +             }
> > > +     } while (loop_limit-- > 0);
> > > +     raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > > +
> > > +     if (!loop_limit)
> > > +             dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> > > +}
> > > +
> > > +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> > > +{
> > > +     struct irq_data *d = &desc->irq_data;
> > > +
> > > +     /* Make sure we're primed for the next edge */
> > > +     msm_gpio_update_dual_edge_parent(d);
> >
> > I would have expected this to happen on EOI or ACK, rather than before
> > the flow is actually handled, once you have told the interrupt
> > controller that you were dealing with this interrupt.
> 
> Having it on Ack would be ideal, but it appears that the Ack function
> isn't called in this case.  That's only called if our handler is
> handle_edge_irq() or handle_level_irq().  See more below.

Easily fixed, see further down.

> 
> ...I'm pretty sure I don't want it on EOI.  Specifically, if I did it
> on EOI then I think I _would_ need to re-queue another interrupt if an
> edge came in msm_gpio_update_dual_edge_parent().  Doing all the edge
> adjustment before calling the true interrupt handler avoids all
> that.

Requeuing interrupts would be fine, and we have the retrigger callback
for that. This can be used when you want to support level interrupts,
but your interrupt controller only supports edge. Something similar
could be done to deal with dual edge interrupts.

> 
> 
> > > +
> > > +     /* Pass on to the normal interrupt handler */
> > > +     handle_fasteoi_irq(desc);
> >
> > Is that the right flow? It seems that the current code is using
> > handle_edge_irq. I guess it has been broken so far, and that this
> > patch actually fixes it by forcing a fasteoi flow...
> 
> The code today only uses handle_level_irq() / handle_edge_irq() if
> "skip_wake_irqs" wasn't set for this IRQ.  In the case that
> "skip_wake_irqs" wasn't set then it leaves the handler alone.  I
> definitely had a hard time following all the flow and interactions
> between the pinctrl, PDC, and the GICv3 but I definitely did confirm
> that handle_fasteoi_irq() was the handler that was running when
> "skip_wake_irqs" was set before I stuck mine in the middle.

OK.

> I believe how things work today with the "skip_wake_irqs" case is
> that, for the most part, the pinctrl driver stays out of the way for
> setting up and handling IRQs and just passes some calls onto its
> parent (the PDC).  The PDC driver is actually quite minimal.  There's
> no "Ack" in there and no calls to set an IRQ handler--it seems to just
> rely on the GICv3 doing all that.  It looks there is an implicit Ack
> as part of gic_handle_irq() since reading the IAR counts as an Ack.
> 
> 
> So to try to sum up my understanding:
> 
> 1. In the case of "skip_wake_irqs" today there is no acking / handling
> code that is part of pinctrl-msm or the PDC.  They just configure
> things to direct to the GICv3.
> 
> 2. For my workaround I just need to make sure to intercept myself and
> prime the next edge _before_ the end-user interrupt handler gets
> called.  If edges are coalesced before the end-user interrupt handler
> is called then that's OK.
> 
> 
> I'll await your reply before sending out the next version.  Thanks
> much for all your time looking at this!

So here are my suggestions:

- Move your dual edge hack to the irq_ack callback

- On detecting a dual edge interrupt, switch to the
  handle_fasteoi_ack_irq flow, which will call the irq_ack callback

- Get rid of the now useless locking

I bet you could reuse some of the existing hacks, though I haven't
looked too hard because it is Saturday and this code really makes my
eyes bleed.

Thanks,

	M.

Cheng-yi Chiang July 13, 2020, 11:19 a.m. UTC | #10

On Thu, Jul 9, 2020 at 5:16 AM Douglas Anderson <[e-mail]> wrote:
>
> As per Qualcomm, there is a PDC hardware issue (with the specific IP
> rev that exists on sc7180) that causes the PDC not to work properly
> when configured to handle dual edges.
>
> Let's work around this by emulating only ever letting our parent see
> requests for single edge interrupts on affected hardware.
>
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Douglas Anderson <[e-mail]>

Tested-by: Cheng-Yi Chiang <[e-mail]>

>
> ---
> As far as I can tell everything here should work and the limited
> testing I'm able to give it shows that, in fact, I can detect both
> edges.
>
> Please give this an extra thorough review since it's trying to find
> the exact right place to insert this code and I'm not massively
> familiar with all the frameworks.
>
> If someone has hardware where it's easy to stress test this that'd be
> wonderful too.  The board I happen to have in front of me doesn't have
> any easy-to-toggle GPIOs where I can just poke a button or a switch to
> generate edges.  My testing was done by hacking the "write protect"
> GPIO on my board into gpio-keys as a dual-edge interrupt and then
> sending commands to our security chip to toggle it--not exactly great
> for testing to make sure there are no race conditions if the interrupt
> bounces a lot.
>
>  drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
>  drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
>  drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
>  3 files changed, 85 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 83b7d64bc4c1..45ca09ebb7b3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  }
>
> +/**
> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
> + * @d: The irq dta.
> + *
> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
> + * normally handled by the parent irqchip.  The logic here is slightly
> + * different due to what's easy to do with our parent, but in principle it's
> + * the same.
> + */
> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +       const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> +       unsigned long flags;
> +       int loop_limit = 100;
> +       unsigned int val;
> +       unsigned int type;
> +
> +       /* Read the value and make a guess about what edge we need to catch */
> +       val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +       type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +
> +       raw_spin_lock_irqsave(&pctrl->lock, flags);
> +       do {
> +               /* Set the parent to catch the next edge */
> +               irq_chip_set_type_parent(d, type);
> +
> +               /*
> +                * Possibly the line changed between when we last read "val"
> +                * (and decided what edge we needed) and when set the edge.
> +                * If the value didn't change (or changed and then changed
> +                * back) then we're done.
> +                */
> +               val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> +               if (type == IRQ_TYPE_EDGE_RISING) {
> +                       if (!val)
> +                               break;
> +                       type = IRQ_TYPE_EDGE_FALLING;
> +               } else if (type == IRQ_TYPE_EDGE_FALLING) {
> +                       if (val)
> +                               break;
> +                       type = IRQ_TYPE_EDGE_RISING;
> +               }
> +       } while (loop_limit-- > 0);
> +       raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +       if (!loop_limit)
> +               dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> +}
> +
> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> +{
> +       struct irq_data *d = &desc->irq_data;
> +
> +       /* Make sure we're primed for the next edge */
> +       msm_gpio_update_dual_edge_parent(d);
> +
> +       /* Pass on to the normal interrupt handler */
> +       handle_fasteoi_irq(desc);
> +}
> +
> +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
> +                                                      unsigned int type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> +       return type == IRQ_TYPE_EDGE_BOTH &&
> +              pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
> +              test_bit(d->hwirq, pctrl->skip_wake_irqs);
> +}
> +
>  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>         unsigned long flags;
>         u32 val;
>
> +       if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
> +               irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
> +               msm_gpio_update_dual_edge_parent(d);
> +
> +               return 0;
> +       }
> +
>         if (d->parent_data)
>                 irq_chip_set_type_parent(d, type);
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9452da18a78b..7486fe08eb9b 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
>   * @pull_no_keeper: The SoC does not support keeper bias.
>   * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>   * @nwakeirq_map:   The number of entries in @wakeirq_map
> + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
> + *                            to be aware that their parent can't handle dual
> + *                            edge interrupts.
>   */
>  struct msm_pinctrl_soc_data {
>         const struct pinctrl_pin_desc *pins;
> @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
>         const int *reserved_gpios;
>         const struct msm_gpio_wakeirq_map *wakeirq_map;
>         unsigned int nwakeirq_map;
> +       bool wakeirq_dual_edge_errata;
>  };
>
>  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> index 1b6465a882f2..1d9acad3c1ce 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
> @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
>         .ntiles = ARRAY_SIZE(sc7180_tiles),
>         .wakeirq_map = sc7180_pdc_map,
>         .nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
> +       .wakeirq_dual_edge_errata = true,
>  };
>
>  static int sc7180_pinctrl_probe(struct platform_device *pdev)
> --
> 2.27.0.383.g050319c2ae-goog
>

Maulik Shah July 13, 2020, 2:25 p.m. UTC | #11

Hi,

On 7/10/2020 9:40 PM, Doug Anderson wrote:
> Hi,
>
> On Fri, Jul 10, 2020 at 2:03 AM Marc Zyngier <[e-mail]> wrote:
>> Hi Doug,
>>
>> On Wed, 08 Jul 2020 22:16:25 +0100,
>> Douglas Anderson <[e-mail]> wrote:
>>> As per Qualcomm, there is a PDC hardware issue (with the specific IP
>>> rev that exists on sc7180) that causes the PDC not to work properly
>>> when configured to handle dual edges.
>>>
>>> Let's work around this by emulating only ever letting our parent see
>>> requests for single edge interrupts on affected hardware.
>>>
>>> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
>>> Signed-off-by: Douglas Anderson <[e-mail]>
>>> ---
>>> As far as I can tell everything here should work and the limited
>>> testing I'm able to give it shows that, in fact, I can detect both
>>> edges.
>>>
>>> Please give this an extra thorough review since it's trying to find
>>> the exact right place to insert this code and I'm not massively
>>> familiar with all the frameworks.
>>>
>>> If someone has hardware where it's easy to stress test this that'd be
>>> wonderful too.  The board I happen to have in front of me doesn't have
>>> any easy-to-toggle GPIOs where I can just poke a button or a switch to
>>> generate edges.  My testing was done by hacking the "write protect"
>>> GPIO on my board into gpio-keys as a dual-edge interrupt and then
>>> sending commands to our security chip to toggle it--not exactly great
>>> for testing to make sure there are no race conditions if the interrupt
>>> bounces a lot.
>> This looks positively awful (the erratum, not the patch). Is there an
>> actual description of the problem, outlining the circumstances that
>> triggers this issue? The PDC really never fails to disappoint...
> Hopefully someone from Qualcomm can chime in here.  My entire
> knowledge of the errata comes from:
>
> https://lore.kernel.org/r/[e-mail]
>
> ...and I tried to copy the exact phrasing that Rajendra gave.
PDC irqchip not detecting dual edge interrupts is not really a bug. PDC 
closely works in hierarchy with
GIC and GIC also doesn't support dual edge IRQs.  Dual edge feature is 
added in later HW revisions to support
similar usecases.

Regardless of dual edge feature,  as i mentioned in below link
if the drivers requests the intended edge of the interrupt then it works 
as expected.
(Drivers never need/use both the edges at a same time).
but if we are not intending to change/update each driver which use dual 
edge, this patch takes care of
same by requesting only one edge at a time, albeit its done from irqchip 
driver as opposed to client driver.

https://lore.kernel.org/linux-arm-msm/[e-mail]/

Thanks,
Maulik
>
>>>   drivers/pinctrl/qcom/pinctrl-msm.c    | 80 +++++++++++++++++++++++++++
>>>   drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
>>>   drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
>>>   3 files changed, 85 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> index 83b7d64bc4c1..45ca09ebb7b3 100644
>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>>>        raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>>   }
>>>
>>> +/**
>>> + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
>>> + * @d: The irq dta.
>>> + *
>>> + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
>>> + * normally handled by the parent irqchip.  The logic here is slightly
>>> + * different due to what's easy to do with our parent, but in principle it's
>>> + * the same.
>>> + */
>>> +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
>>> +{
>>> +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>> +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>> +     const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
>>> +     unsigned long flags;
>>> +     int loop_limit = 100;
>> I guess this is a "finger up in the air" type of limit?
> Yes, the same "finger up in the air" as
> msm_gpio_update_dual_edge_pos() in the same file.  My function comment
> refers to the other function to try to tie them together at least a
> little.
>
>
>>> +     unsigned int val;
>>> +     unsigned int type;
>>> +
>>> +     /* Read the value and make a guess about what edge we need to catch */
>>> +     val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
>> What does this value represent? The input line value?
> Yes.  Coped from msm_gpio_update_dual_edge_pos().
>
>
>>> +     type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
>>> +
>>> +     raw_spin_lock_irqsave(&pctrl->lock, flags);
>> What is this lock protecting you against? In both cases, you are
>> already under the irq_desc lock, with interrupts disabled.
> We are?  I put a breakpoint when the IRQ hits and did a bt.  I see
> this (I happen to be on 5.4 at the moment, so hopefully the same as
> mainline):
>
>   kgdb_breakpoint+0x3c/0x74
>   msm_gpio_update_dual_edge_parent+0x58/0x17c
>   msm_gpio_handle_dual_edge_parent_irq+0x1c/0x30
>   __handle_domain_irq+0x84/0xc4
>   gic_handle_irq+0x170/0x220
>   el1_irq+0xd0/0x180
>
> I think the stack is missing a few things due to aggressive inlining
> from my compiler, so the true backtrace would be:
>
> msm_gpio_handle_dual_edge_parent_irq()
> generic_handle_irq_desc()
> generic_handle_irq()
> __handle_domain_irq()
> handle_domain_irq()
> gic_handle_irq()
>
> The first place that got the "desc" was generic_handle_irq() and it
> got it via irq_to_desc().  That doesn't seem to do any locking.  Then
> generic_handle_irq_desc() just calls a function pointer so no locking
> there either.
>
> ...ah, but maybe what you're saying is that
> msm_gpio_handle_dual_edge_parent_irq() should be holding "desc->lock"
> around the call to msm_gpio_update_dual_edge_parent()?  I can do that.
>
>
>>> +     do {
>>> +             /* Set the parent to catch the next edge */
>>> +             irq_chip_set_type_parent(d, type);
>>> +
>>> +             /*
>>> +              * Possibly the line changed between when we last read "val"
>>> +              * (and decided what edge we needed) and when set the edge.
>>> +              * If the value didn't change (or changed and then changed
>>> +              * back) then we're done.
>>> +              */
>> If the line changed, shouldn't you actually inject a new interrupt
>> altogether? By changing the polarity more than once, you are
>> effectively loosing edges that should have triggered an interrupt.
> Are you sure this is needed?  My understanding of edge triggered
> interrupts is that until the interrupt handler is called that all
> edges can be coalesced into a single interrupt.  It's only after the
> interrupt handler is called that it's important to capture new edges.
> So if you have this:
>
> a) Be busy processing another unrelated interrupt
> b) 5 edges happen on the line
> c) Other interrupt finishes
> d) Edge interrupt is acked and handler is called
>
> You'll only get one call to the interrupt handler even though there
> were 5 edges, right?  It's only important that you queue another
> interrupt if that interrupt happens after the true interrupt handler
> (the one acting on the edge) has started.
>
> ...actually, in theory you'll get _either_ one or two calls to the
> interrupt handler depending on timing, since the above could also
> happen as:
>
> a) Be busy processing another unrelated interrupt
> b) 4 edges happen on the line
> c) Other interrupt finishes
> d) Edge interrupt is acked and ...
> e) 1 more edge happens on the line
> f) ...handler is called
> g) Edge interrupt is acked and handler is called
>
>
> As long as msm_gpio_update_dual_edge_parent() is called _before_ the
> true interrupt handler is called then what I have should be fine,
> right?
>
>
>>> +             val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
>>> +             if (type == IRQ_TYPE_EDGE_RISING) {
>>> +                     if (!val)
>>> +                             break;
>>> +                     type = IRQ_TYPE_EDGE_FALLING;
>>> +             } else if (type == IRQ_TYPE_EDGE_FALLING) {
>>> +                     if (val)
>>> +                             break;
>>> +                     type = IRQ_TYPE_EDGE_RISING;
>>> +             }
>>> +     } while (loop_limit-- > 0);
>>> +     raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>> +
>>> +     if (!loop_limit)
>>> +             dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
>>> +}
>>> +
>>> +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
>>> +{
>>> +     struct irq_data *d = &desc->irq_data;
>>> +
>>> +     /* Make sure we're primed for the next edge */
>>> +     msm_gpio_update_dual_edge_parent(d);
>> I would have expected this to happen on EOI or ACK, rather than before
>> the flow is actually handled, once you have told the interrupt
>> controller that you were dealing with this interrupt.
> Having it on Ack would be ideal, but it appears that the Ack function
> isn't called in this case.  That's only called if our handler is
> handle_edge_irq() or handle_level_irq().  See more below.
>
> ...I'm pretty sure I don't want it on EOI.  Specifically, if I did it
> on EOI then I think I _would_ need to re-queue another interrupt if an
> edge came in msm_gpio_update_dual_edge_parent().  Doing all the edge
> adjustment before calling the true interrupt handler avoids all that.
>
>
>>> +
>>> +     /* Pass on to the normal interrupt handler */
>>> +     handle_fasteoi_irq(desc);
>> Is that the right flow? It seems that the current code is using
>> handle_edge_irq. I guess it has been broken so far, and that this
>> patch actually fixes it by forcing a fasteoi flow...
> The code today only uses handle_level_irq() / handle_edge_irq() if
> "skip_wake_irqs" wasn't set for this IRQ.  In the case that
> "skip_wake_irqs" wasn't set then it leaves the handler alone.  I
> definitely had a hard time following all the flow and interactions
> between the pinctrl, PDC, and the GICv3 but I definitely did confirm
> that handle_fasteoi_irq() was the handler that was running when
> "skip_wake_irqs" was set before I stuck mine in the middle.
>
> I believe how things work today with the "skip_wake_irqs" case is
> that, for the most part, the pinctrl driver stays out of the way for
> setting up and handling IRQs and just passes some calls onto its
> parent (the PDC).  The PDC driver is actually quite minimal.  There's
> no "Ack" in there and no calls to set an IRQ handler--it seems to just
> rely on the GICv3 doing all that.  It looks there is an implicit Ack
> as part of gic_handle_irq() since reading the IAR counts as an Ack.
>
>
> So to try to sum up my understanding:
>
> 1. In the case of "skip_wake_irqs" today there is no acking / handling
> code that is part of pinctrl-msm or the PDC.  They just configure
> things to direct to the GICv3.
>
> 2. For my workaround I just need to make sure to intercept myself and
> prime the next edge _before_ the end-user interrupt handler gets
> called.  If edges are coalesced before the end-user interrupt handler
> is called then that's OK.
>
>
> I'll await your reply before sending out the next version.  Thanks
> much for all your time looking at this!
>
>
>>> +}
>>> +
>>> +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
>>> +                                                    unsigned int type)
>>> +{
>>> +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>> +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>> +
>>> +     return type == IRQ_TYPE_EDGE_BOTH &&
>>> +            pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
>>> +            test_bit(d->hwirq, pctrl->skip_wake_irqs);
>>> +}
>>> +
>>>   static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>>   {
>>>        struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>> @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>>        unsigned long flags;
>>>        u32 val;
>>>
>>> +     if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
>>> +             irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
>>> +             msm_gpio_update_dual_edge_parent(d);
>>> +
>>> +             return 0;
>>> +     }
>>> +
>>>        if (d->parent_data)
>>>                irq_chip_set_type_parent(d, type);
>>>
>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
>>> index 9452da18a78b..7486fe08eb9b 100644
>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
>>> @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
>>>    * @pull_no_keeper: The SoC does not support keeper bias.
>>>    * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>>>    * @nwakeirq_map:   The number of entries in @wakeirq_map
>>> + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
>>> + *                            to be aware that their parent can't handle dual
>>> + *                            edge interrupts.
>>>    */
>>>   struct msm_pinctrl_soc_data {
>>>        const struct pinctrl_pin_desc *pins;
>>> @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
>>>        const int *reserved_gpios;
>>>        const struct msm_gpio_wakeirq_map *wakeirq_map;
>>>        unsigned int nwakeirq_map;
>>> +     bool wakeirq_dual_edge_errata;
>>>   };
>>>
>>>   extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
>>> index 1b6465a882f2..1d9acad3c1ce 100644
>>> --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
>>> +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
>>> @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
>>>        .ntiles = ARRAY_SIZE(sc7180_tiles),
>>>        .wakeirq_map = sc7180_pdc_map,
>>>        .nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
>>> +     .wakeirq_dual_edge_errata = true,
>>>   };
>>>
>>>   static int sc7180_pinctrl_probe(struct platform_device *pdev)
>>> --
>>> 2.27.0.383.g050319c2ae-goog
>>>
>>>
>> Thanks,
>>
>>          M.
>>
>> --
>> Without deviation from the norm, progress is not possible.

Doug Anderson July 13, 2020, 3:27 p.m. UTC | #12

Hi,

On Sat, Jul 11, 2020 at 2:16 AM Marc Zyngier <[e-mail]> wrote:
>
> On Fri, 10 Jul 2020 17:10:55 +0100,
> Doug Anderson <[e-mail]> wrote:
> >
> > Hi,
> >
> > On Fri, Jul 10, 2020 at 2:03 AM Marc Zyngier <[e-mail]> wrote:
> > >
> > > Hi Doug,
>
> [...]
>
> > >
> > > > +     type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > > > +
> > > > +     raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > > What is this lock protecting you against? In both cases, you are
> > > already under the irq_desc lock, with interrupts disabled.
> >
> > We are?  I put a breakpoint when the IRQ hits and did a bt.  I see
> > this (I happen to be on 5.4 at the moment, so hopefully the same as
> > mainline):
> >
> >  kgdb_breakpoint+0x3c/0x74
> >  msm_gpio_update_dual_edge_parent+0x58/0x17c
> >  msm_gpio_handle_dual_edge_parent_irq+0x1c/0x30
> >  __handle_domain_irq+0x84/0xc4
> >  gic_handle_irq+0x170/0x220
> >  el1_irq+0xd0/0x180
> >
> > I think the stack is missing a few things due to aggressive inlining
> > from my compiler, so the true backtrace would be:
> >
> > msm_gpio_handle_dual_edge_parent_irq()
> > generic_handle_irq_desc()
> > generic_handle_irq()
> > __handle_domain_irq()
> > handle_domain_irq()
> > gic_handle_irq()
> >
> > The first place that got the "desc" was generic_handle_irq() and it
> > got it via irq_to_desc().  That doesn't seem to do any locking.  Then
> > generic_handle_irq_desc() just calls a function pointer so no locking
> > there either.
> >
> > ...ah, but maybe what you're saying is that
> > msm_gpio_handle_dual_edge_parent_irq() should be holding "desc->lock"
> > around the call to msm_gpio_update_dual_edge_parent()?  I can do that.
>
> No, I mentally did a fast-forward to moving this hack into the irq
> flow, rather than doing before entering the flow. handle_fasteoi_irq
> will take the lock, but obviously not with the current state of this
> patch.
>
> >
> >
> > > > +     do {
> > > > +             /* Set the parent to catch the next edge */
> > > > +             irq_chip_set_type_parent(d, type);
> > > > +
> > > > +             /*
> > > > +              * Possibly the line changed between when we last read "val"
> > > > +              * (and decided what edge we needed) and when set the edge.
> > > > +              * If the value didn't change (or changed and then changed
> > > > +              * back) then we're done.
> > > > +              */
> > >
> > > If the line changed, shouldn't you actually inject a new interrupt
> > > altogether? By changing the polarity more than once, you are
> > > effectively loosing edges that should have triggered an interrupt.
> >
> > Are you sure this is needed?  My understanding of edge triggered
> > interrupts is that until the interrupt handler is called that all
> > edges can be coalesced into a single interrupt.
>
> It really depends on whether the edges are semantically different, and
> I'm not sure you can decide this at the interrupt controller
> level. The core IRQ code doesn't give you a way to discriminate
> between those, but endpoint drivers could, and could get terminally
> confused if the see two rising edges without a falling edge in
> between.

I have added discussion about this in the commit message for v2.
Hopefully it looks OK.  NOTE: it's actually quite common for pinctrl
hardware to only support single edge and require dual edge emulation
in solftware.  I think there are at least 4-5 examples that I found
pretty easily.  ...so I think any drivers that are expecting dual
edges to come from an external pin will have code to account for this.


> > It's only after the
> > interrupt handler is called that it's important to capture new edges.
> > So if you have this:
> >
> > a) Be busy processing another unrelated interrupt
> > b) 5 edges happen on the line
> > c) Other interrupt finishes
> > d) Edge interrupt is acked and handler is called
> >
> > You'll only get one call to the interrupt handler even though there
> > were 5 edges, right?  It's only important that you queue another
> > interrupt if that interrupt happens after the true interrupt handler
> > (the one acting on the edge) has started.
> >
> > ...actually, in theory you'll get _either_ one or two calls to the
> > interrupt handler depending on timing, since the above could also
> > happen as:
> >
> > a) Be busy processing another unrelated interrupt
> > b) 4 edges happen on the line
> > c) Other interrupt finishes
> > d) Edge interrupt is acked and ...
> > e) 1 more edge happens on the line
> > f) ...handler is called
> > g) Edge interrupt is acked and handler is called
> >
> >
> > As long as msm_gpio_update_dual_edge_parent() is called _before_ the
> > true interrupt handler is called then what I have should be fine,
> > right?
>
> I don't disagree with any of that, except that being fine at the
> irqchip level doesn't necessarily mean being fine at the endpoint
> driver level. On the other hand, the HW looks terminally broken, so
> maybe it doesn't matter as the drivers will have to be written with
> this limitation in mind...
>
> >
> > > > +             val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > > > +             if (type == IRQ_TYPE_EDGE_RISING) {
> > > > +                     if (!val)
> > > > +                             break;
> > > > +                     type = IRQ_TYPE_EDGE_FALLING;
> > > > +             } else if (type == IRQ_TYPE_EDGE_FALLING) {
> > > > +                     if (val)
> > > > +                             break;
> > > > +                     type = IRQ_TYPE_EDGE_RISING;
> > > > +             }
> > > > +     } while (loop_limit-- > 0);
> > > > +     raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > > > +
> > > > +     if (!loop_limit)
> > > > +             dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
> > > > +}
> > > > +
> > > > +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
> > > > +{
> > > > +     struct irq_data *d = &desc->irq_data;
> > > > +
> > > > +     /* Make sure we're primed for the next edge */
> > > > +     msm_gpio_update_dual_edge_parent(d);
> > >
> > > I would have expected this to happen on EOI or ACK, rather than before
> > > the flow is actually handled, once you have told the interrupt
> > > controller that you were dealing with this interrupt.
> >
> > Having it on Ack would be ideal, but it appears that the Ack function
> > isn't called in this case.  That's only called if our handler is
> > handle_edge_irq() or handle_level_irq().  See more below.
>
> Easily fixed, see further down.
>
> >
> > ...I'm pretty sure I don't want it on EOI.  Specifically, if I did it
> > on EOI then I think I _would_ need to re-queue another interrupt if an
> > edge came in msm_gpio_update_dual_edge_parent().  Doing all the edge
> > adjustment before calling the true interrupt handler avoids all
> > that.
>
> Requeuing interrupts would be fine, and we have the retrigger callback
> for that. This can be used when you want to support level interrupts,
> but your interrupt controller only supports edge. Something similar
> could be done to deal with dual edge interrupts.
>
> >
> >
> > > > +
> > > > +     /* Pass on to the normal interrupt handler */
> > > > +     handle_fasteoi_irq(desc);
> > >
> > > Is that the right flow? It seems that the current code is using
> > > handle_edge_irq. I guess it has been broken so far, and that this
> > > patch actually fixes it by forcing a fasteoi flow...
> >
> > The code today only uses handle_level_irq() / handle_edge_irq() if
> > "skip_wake_irqs" wasn't set for this IRQ.  In the case that
> > "skip_wake_irqs" wasn't set then it leaves the handler alone.  I
> > definitely had a hard time following all the flow and interactions
> > between the pinctrl, PDC, and the GICv3 but I definitely did confirm
> > that handle_fasteoi_irq() was the handler that was running when
> > "skip_wake_irqs" was set before I stuck mine in the middle.
>
> OK.
>
> > I believe how things work today with the "skip_wake_irqs" case is
> > that, for the most part, the pinctrl driver stays out of the way for
> > setting up and handling IRQs and just passes some calls onto its
> > parent (the PDC).  The PDC driver is actually quite minimal.  There's
> > no "Ack" in there and no calls to set an IRQ handler--it seems to just
> > rely on the GICv3 doing all that.  It looks there is an implicit Ack
> > as part of gic_handle_irq() since reading the IAR counts as an Ack.
> >
> >
> > So to try to sum up my understanding:
> >
> > 1. In the case of "skip_wake_irqs" today there is no acking / handling
> > code that is part of pinctrl-msm or the PDC.  They just configure
> > things to direct to the GICv3.
> >
> > 2. For my workaround I just need to make sure to intercept myself and
> > prime the next edge _before_ the end-user interrupt handler gets
> > called.  If edges are coalesced before the end-user interrupt handler
> > is called then that's OK.
> >
> >
> > I'll await your reply before sending out the next version.  Thanks
> > much for all your time looking at this!
>
> So here are my suggestions:
>
> - Move your dual edge hack to the irq_ack callback
>
> - On detecting a dual edge interrupt, switch to the
>   handle_fasteoi_ack_irq flow, which will call the irq_ack callback
>
> - Get rid of the now useless locking

OK, see how v2 looks to you.  Sending it out right after I hit "Send'
on this message.


> I bet you could reuse some of the existing hacks, though I haven't
> looked too hard because it is Saturday and this code really makes my
> eyes bleed.

Thanks again for all your help!  For now I'll keep the code separate.
While I agree that it could be unified with the existing
msm_gpio_update_dual_edge_pos() function, doing so would mean:

1. Changing the logic of msm_gpio_update_dual_edge_pos() slightly.
That function does things that we can't quite do as easily.  For
instance, it reads the current config and queries whether an interrupt
is pending both of which didn't seem easy to do in my case.  I think
it would be fine/possible to change msm_gpio_update_dual_edge_pos() to
be more like my function, but I haven't quantified if it would be
worse or better performance.

2. Doing do would require an extra level of abstraction and I'm not
100% sure it's worth it.

If folks want me to work on trying to unify the two let me know and I
can do a v3.


-Doug

Patch

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 83b7d64bc4c1..45ca09ebb7b3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -860,6 +860,79 @@  static void msm_gpio_irq_ack(struct irq_data *d)
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
+/**
+ * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs handled by parent.
+ * @d: The irq dta.
+ *
+ * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that are
+ * normally handled by the parent irqchip.  The logic here is slightly
+ * different due to what's easy to do with our parent, but in principle it's
+ * the same.
+ */
+static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
+	unsigned long flags;
+	int loop_limit = 100;
+	unsigned int val;
+	unsigned int type;
+
+	/* Read the value and make a guess about what edge we need to catch */
+	val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
+	type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	do {
+		/* Set the parent to catch the next edge */
+		irq_chip_set_type_parent(d, type);
+
+		/*
+		 * Possibly the line changed between when we last read "val"
+		 * (and decided what edge we needed) and when set the edge.
+		 * If the value didn't change (or changed and then changed
+		 * back) then we're done.
+		 */
+		val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
+		if (type == IRQ_TYPE_EDGE_RISING) {
+			if (!val)
+				break;
+			type = IRQ_TYPE_EDGE_FALLING;
+		} else if (type == IRQ_TYPE_EDGE_FALLING) {
+			if (val)
+				break;
+			type = IRQ_TYPE_EDGE_RISING;
+		}
+	} while (loop_limit-- > 0);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	if (!loop_limit)
+		dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
+}
+
+void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
+{
+	struct irq_data	*d = &desc->irq_data;
+
+	/* Make sure we're primed for the next edge */
+	msm_gpio_update_dual_edge_parent(d);
+
+	/* Pass on to the normal interrupt handler */
+	handle_fasteoi_irq(desc);
+}
+
+static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
+						       unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+	return type == IRQ_TYPE_EDGE_BOTH &&
+	       pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
+	       test_bit(d->hwirq, pctrl->skip_wake_irqs);
+}
+
 static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -868,6 +941,13 @@  static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	u32 val;
 
+	if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
+		irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
+		msm_gpio_update_dual_edge_parent(d);
+
+		return 0;
+	}
+
 	if (d->parent_data)
 		irq_chip_set_type_parent(d, type);
 
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9452da18a78b..7486fe08eb9b 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -113,6 +113,9 @@  struct msm_gpio_wakeirq_map {
  * @pull_no_keeper: The SoC does not support keeper bias.
  * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
  * @nwakeirq_map:   The number of entries in @wakeirq_map
+ * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
+ *                            to be aware that their parent can't handle dual
+ *                            edge interrupts.
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -128,6 +131,7 @@  struct msm_pinctrl_soc_data {
 	const int *reserved_gpios;
 	const struct msm_gpio_wakeirq_map *wakeirq_map;
 	unsigned int nwakeirq_map;
+	bool wakeirq_dual_edge_errata;
 };
 
 extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
index 1b6465a882f2..1d9acad3c1ce 100644
--- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
+++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
@@ -1147,6 +1147,7 @@  static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
 	.ntiles = ARRAY_SIZE(sc7180_tiles),
 	.wakeirq_map = sc7180_pdc_map,
 	.nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
+	.wakeirq_dual_edge_errata = true,
 };
 
 static int sc7180_pinctrl_probe(struct platform_device *pdev)