| Message ID | 1592818308-23001-6-git-send-email-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip: qcom: pdc: Introduce irq_set_wake call | expand |
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. 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.
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.
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; }
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(+)