| Message ID | 1501179565-26466-3-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 27, 2017 at 8:19 PM, Timur Tabi <timur@codeaurora.org> wrote: > To support sparse GPIO maps, pinctrl-msm client drivers can specify > that a given GPIO has a pin count of zero. These GPIOs will be > considered "hidden". Any attempt to claim the GPIO will fail, and they > will not be listed in debugfs. > > During a kexec shutdown, machine_kexec_mask_interrupts() will attempt > to disable all IRQs, even those that aren't enabled. This includes > GPIOs that are unavailable (npins == 0), so add a check to the irq mask > and unmask functions. > > Signed-off-by: Timur Tabi <timur@codeaurora.org> I'm waiting for Björn's review of the two remaining patches. Yours, Linus Walleij
On 07/31/2017 08:36 AM, Linus Walleij wrote: >> To support sparse GPIO maps, pinctrl-msm client drivers can specify >> that a given GPIO has a pin count of zero. These GPIOs will be >> considered "hidden". Any attempt to claim the GPIO will fail, and they >> will not be listed in debugfs. >> >> During a kexec shutdown, machine_kexec_mask_interrupts() will attempt >> to disable all IRQs, even those that aren't enabled. This includes >> GPIOs that are unavailable (npins == 0), so add a check to the irq mask >> and unmask functions. >> >> Signed-off-by: Timur Tabi<timur@codeaurora.org> > I'm waiting for Björn's review of the two remaining patches. Björn, do you have time to review these patches? I'm hoping to get them into 4.14.
During a kexec shutdown, machine_kexec_mask_interrupts() will attempt
to disable all IRQs that are registered. If gpio is not available,
don't register IRQs and it won't be in the irq_desc list it walks through.
If gpio is unavailable, perhaps a more correct fix that covers more is
to not calling gpiochip_irqchip_add() to register msm_gpio_irq_chip in
msm_gpio_init().
When registering interrupt for msm_qpio_irq_chip the following are
registered, not just irq_mask and irq_unmask.
static struct irq_chip msm_gpio_irq_chip = {
.name = "msmgpio",
.irq_mask = msm_gpio_irq_mask,
.irq_unmask = msm_gpio_irq_unmask,
.irq_ack = msm_gpio_irq_ack,
.irq_set_type = msm_gpio_irq_set_type,
.irq_set_wake = msm_gpio_irq_set_wake,
};
Technically the same check added in msm_gpio_irq_mask() and
msm_gpio_irq_unmask() should be added in msm_gpio_irq_ack(),
msm_gpio_irq_set_type(), and msm_gpio_irq_set_wake() if it's registered
with irq domain.
To cover more bases, the more correct way is to not register interrupt
with irq domain at all if gpio is unavailable.
On 08/09/2017 02:02 PM, Timur Tabi wrote:
> On 07/31/2017 08:36 AM, Linus Walleij wrote:
>>> To support sparse GPIO maps, pinctrl-msm client drivers can specify
>>> that a given GPIO has a pin count of zero. These GPIOs will be
>>> considered "hidden". Any attempt to claim the GPIO will fail, and they
>>> will not be listed in debugfs.
>>>
>>> During a kexec shutdown, machine_kexec_mask_interrupts() will attempt
>>> to disable all IRQs, even those that aren't enabled. This includes
>>> GPIOs that are unavailable (npins == 0), so add a check to the irq mask
>>> and unmask functions.
>>>
>>> Signed-off-by: Timur Tabi<timur@codeaurora.org>
>
>> I'm waiting for Björn's review of the two remaining patches.
>
> Björn, do you have time to review these patches? I'm hoping to get them
> into 4.14.
>
On 08/16/2017 01:10 PM, Jiandi An wrote: > > Technically the same check added in msm_gpio_irq_mask() and > msm_gpio_irq_unmask() should be added in msm_gpio_irq_ack(), > msm_gpio_irq_set_type(), and msm_gpio_irq_set_wake() if it's registered > with irq domain. I assume that if the GPIO is never unmasked, then msm_gpio_irq_ack() will never be called. msm_gpio_irq_set_type() and msm_gpio_irq_set_wake() might be called, so I can add checks for those functions. I'm hoping that won't be necessary, however. The GPIO and IRQ code is too entangled for me to figure out whether unclaimed GPIOs can still have their interrupts programmed.
On 08/16/2017 01:32 PM, Timur Tabi wrote: > On 08/16/2017 01:10 PM, Jiandi An wrote: >> >> Technically the same check added in msm_gpio_irq_mask() and >> msm_gpio_irq_unmask() should be added in msm_gpio_irq_ack(), >> msm_gpio_irq_set_type(), and msm_gpio_irq_set_wake() if it's >> registered with irq domain. > > I assume that if the GPIO is never unmasked, then msm_gpio_irq_ack() > will never be called. > > msm_gpio_irq_set_type() and msm_gpio_irq_set_wake() might be called, so > I can add checks for those functions. I'm hoping that won't be > necessary, however. The GPIO and IRQ code is too entangled for me to > figure out whether unclaimed GPIOs can still have their interrupts > programmed. > That is why the suggestion is instead of patching in each of the op function of registered irq_chip for the unavailable gpio that won't have use for all that anyways, simply don't register irq for the unavailable gpio. What's the use of registering irq if you know the gpio is unavailable and not going to use any of the irq_chip functions. It's unnecessarily adding an irq_desc in the list. Technically machine_kexec_mask_interrupts() is not blindly disable all IRQs. If you don't register irq, it won't be in the irq_desc list.
On 08/16/2017 01:32 PM, Timur Tabi wrote: > On 08/16/2017 01:10 PM, Jiandi An wrote: >> >> Technically the same check added in msm_gpio_irq_mask() and >> msm_gpio_irq_unmask() should be added in msm_gpio_irq_ack(), >> msm_gpio_irq_set_type(), and msm_gpio_irq_set_wake() if it's >> registered with irq domain. > > I assume that if the GPIO is never unmasked, then msm_gpio_irq_ack() > will never be called. > > msm_gpio_irq_set_type() and msm_gpio_irq_set_wake() might be called, so > I can add checks for those functions. I'm hoping that won't be > necessary, however. The GPIO and IRQ code is too entangled for me to > figure out whether unclaimed GPIOs can still have their interrupts > programmed. > That is why the suggestion is instead of patching in each of the op function of registered irq_chip for the unavailable gpio that won't have use for all that anyways, simply don't register irq for the unavailable gpio. What's the use of registering irq if you know the gpio is unavailable and not going to use any of the irq_chip functions. It's unnecessarily adding an irq_desc in the list. Technically machine_kexec_mask_interrupts() is not blindly disable all IRQs. If you don't register irq, it won't be in the irq_desc list.
On 08/16/2017 02:31 PM, Jiandi An wrote: > That is why the suggestion is instead of patching in each of the op > function of registered irq_chip for the unavailable gpio that won't > have use for all that anyways, simply don't register irq for the > unavailable gpio Ah, it looks like maybe I can update gpio_chip.irq_valid_mask after calling gpiochip_add_data(). I'll try it.
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 273badd..6b4f353 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -494,6 +494,11 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, }; g = &pctrl->soc->groups[offset]; + + /* If the GPIO group has no pins, then don't show it. */ + if (!g->npins) + return; + ctl_reg = readl(pctrl->regs + g->ctl_reg); is_out = !!(ctl_reg & BIT(g->oe_bit)); @@ -503,7 +508,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func); seq_printf(s, " %dmA", msm_regval_to_drive(drive)); - seq_printf(s, " %s", pulls[pull]); + seq_printf(s, " %s\n", pulls[pull]); } static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) @@ -511,23 +516,30 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) unsigned gpio = chip->base; unsigned i; - for (i = 0; i < chip->ngpio; i++, gpio++) { + for (i = 0; i < chip->ngpio; i++, gpio++) msm_gpio_dbg_show_one(s, NULL, chip, i, gpio); - seq_puts(s, "\n"); - } } #else #define msm_gpio_dbg_show NULL #endif +/* If the GPIO has no pins, then treat it as unavailable. */ +static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset) +{ + struct msm_pinctrl *pctrl = gpiochip_get_data(chip); + const struct msm_pingroup *g = &pctrl->soc->groups[offset]; + + return g->npins ? 0 : -ENODEV; +} + static struct gpio_chip msm_gpio_template = { .direction_input = msm_gpio_direction_input, .direction_output = msm_gpio_direction_output, .get_direction = msm_gpio_get_direction, .get = msm_gpio_get, .set = msm_gpio_set, - .request = gpiochip_generic_request, + .request = msm_gpio_request, .free = gpiochip_generic_free, .dbg_show = msm_gpio_dbg_show, }; @@ -586,6 +598,10 @@ static void msm_gpio_irq_mask(struct irq_data *d) g = &pctrl->soc->groups[d->hwirq]; + /* If there no pins, then this GPIO is unavailable */ + if (!g->npins) + return; + raw_spin_lock_irqsave(&pctrl->lock, flags); val = readl(pctrl->regs + g->intr_cfg_reg); @@ -607,6 +623,10 @@ static void msm_gpio_irq_unmask(struct irq_data *d) g = &pctrl->soc->groups[d->hwirq]; + /* If there no pins, then this GPIO is unavailable */ + if (!g->npins) + return; + raw_spin_lock_irqsave(&pctrl->lock, flags); val = readl(pctrl->regs + g->intr_cfg_reg);
To support sparse GPIO maps, pinctrl-msm client drivers can specify that a given GPIO has a pin count of zero. These GPIOs will be considered "hidden". Any attempt to claim the GPIO will fail, and they will not be listed in debugfs. During a kexec shutdown, machine_kexec_mask_interrupts() will attempt to disable all IRQs, even those that aren't enabled. This includes GPIOs that are unavailable (npins == 0), so add a check to the irq mask and unmask functions. Signed-off-by: Timur Tabi <timur@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)