[v3,5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init

Message ID 1592818308-23001-6-git-send-email-mkshah@codeaurora.org
State New
Headers show
Series
  • [v3,1/5] pinctrl: qcom: Remove irq_disable callback from msmgpio irqchip
Related show

Commit Message

Maulik Shah June 22, 2020, 9:31 a.m.
Clear previous kernel's configuration during init by resetting
all interrupts in enable bank to zero.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Maulik Shah July 14, 2020, 11:01 a.m. | #1
Hi,

On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,

>

> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@codeaurora.org> wrote:

>> Clear previous kernel's configuration during init by resetting

>> all interrupts in enable bank to zero.

>>

>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

>> ---

>>   drivers/irqchip/qcom-pdc.c | 13 +++++++++++++

>>   1 file changed, 13 insertions(+)

>>

>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c

>> index 8beb6f7..11a9d3a 100644

>> --- a/drivers/irqchip/qcom-pdc.c

>> +++ b/drivers/irqchip/qcom-pdc.c

>> @@ -19,6 +19,7 @@

>>   #include <linux/slab.h>

>>   #include <linux/types.h>

>>

>> +#define PDC_MAX_IRQS_PER_REG   32

>>   #define PDC_MAX_IRQS           168

>>   #define PDC_MAX_GPIO_IRQS      256

>>

>> @@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {

>>   static int pdc_setup_pin_mapping(struct device_node *np)

>>   {

>>          int ret, n;

>> +       u32 reg, max_regs, max_pins = 0;

>>

>>          n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));

>>          if (n <= 0 || n % 3)

>> @@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)

>>                                                   &pdc_region[n].cnt);

>>                  if (ret)

>>                          return ret;

>> +               max_pins += pdc_region[n].cnt;

>>          }

>>

>> +       if (max_pins > PDC_MAX_IRQS)

>> +               return -EINVAL;

>> +

>> +       max_regs = max_pins / PDC_MAX_IRQS_PER_REG;

>> +       if (max_pins % PDC_MAX_IRQS_PER_REG)

>> +               max_regs++;

> nit: max_regs = DIV_ROUND_UP(max_pins, PDC_MAX_IRQS_PER_REG)

>

>

>> +       for (reg = 0; reg < max_regs; reg++)

>> +               pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);

> This doesn't feel correct to me, but maybe I'm misunderstanding the

> hardware (I don't think I have access to a reference manual).  Looking

> at the example in the bindings, I see:

>

> qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;

>

> In that example we have mappings for PDC ports:

> 0 - 93 (count = 94)

> 94 - 108 (count = 15)

> 115 - 121 (count = 7)

>

> Notice the slight discontinuity there.  I presume that discontinuity

> is normal / allowed?  If so, if there is enough of it then I think

> your math could be wrong, though with the example you get lucky and it

> works out OK.  It's easy to see the problem with a slightly different

> example:  Imagine that you had this:

>

> 0 - 33 (count = 34)

> 94 - 108 (count = 15)

> 115 - 121 (count = 7)

>

> ...now max_pins = 56 and max_regs = 2.  So you'll init reg 0 and 1.

> ...but (IIUC) you actually should be initting 0, 1, 2, and 3.


Right, Thanks for cacthing this. I will fix in next revision.

Thanks,
Maulik

> I have no idea what might be in those discontinuous ranges and if it's

> always OK to clear, but (assuming it is) one fix is to put your

> clearing loop _inside_ the other "for" loop in this function, AKA:

>

> for (reg = pdc_region[n].pin_base / PDC_MAX_IRQS_PER_REG;

>       reg < DIV_ROUND_UP(pdc_region[n].pin_base + pdc_region[n].cnt),

>                          PDC_MAX_IRQS_PER_REG)

>       reg++)

>

> ...or another option is to keep track of the max "pin_base + cnt" and

> loop from 0 to there?  I just don't know your hardware well enough to

> tell which would be right.


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Patch

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 8beb6f7..11a9d3a 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -19,6 +19,7 @@ 
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#define PDC_MAX_IRQS_PER_REG	32
 #define PDC_MAX_IRQS		168
 #define PDC_MAX_GPIO_IRQS	256
 
@@ -339,6 +340,7 @@  static const struct irq_domain_ops qcom_pdc_gpio_ops = {
 static int pdc_setup_pin_mapping(struct device_node *np)
 {
 	int ret, n;
+	u32 reg, max_regs, max_pins = 0;
 
 	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
 	if (n <= 0 || n % 3)
@@ -367,8 +369,19 @@  static int pdc_setup_pin_mapping(struct device_node *np)
 						 &pdc_region[n].cnt);
 		if (ret)
 			return ret;
+		max_pins += pdc_region[n].cnt;
 	}
 
+	if (max_pins > PDC_MAX_IRQS)
+		return -EINVAL;
+
+	max_regs = max_pins / PDC_MAX_IRQS_PER_REG;
+	if (max_pins % PDC_MAX_IRQS_PER_REG)
+		max_regs++;
+
+	for (reg = 0; reg < max_regs; reg++)
+		pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);
+
 	return 0;
 }