Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1592818308-23001-1-git-send-email-mkshah@codeaurora.org> <1592818308-23001-6-git-send-email-mkshah@codeaurora.org>
In-Reply-To: <1592818308-23001-6-git-send-email-mkshah@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 13 Jul 2020 15:17:11 -0700
Message-ID: <CAD=FV=X=0yqDMeWSUAM4D6o_6JsOU4hm8Q2XFbOSEVQcRzRN6A@mail.gmail.com>
Subject: Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init
To:     Maulik Shah <mkshah@codeaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Marc Zyngier <maz@kernel.org>,
        LinusW <linus.walleij@linaro.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Evan Green <evgreen@chromium.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Andy Gross <agross@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Lina Iyer <ilina@codeaurora.org>,
        Srinivas Rao L <lsrao@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.