| Message ID | 20231025-topic-sm8650-upstream-tlmm-v1-2-4e3d84a3a46b@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: qcom: Introduce Pinctrl/GPIO for SM8650 | expand |
On 25/10/2023 09:33, Neil Armstrong wrote: > New platforms uses a new set of bits to control the wakeirq > delivery to the PDC block. > > The intr_wakeup_present_bit indicates if the GPIO supports > wakeirq and intr_wakeup_enable_bit enables wakeirq delivery > to the PDC block. > > While the name seems to imply this only enables wakeup events, > it is required to allow interrupts events to the PDC block. > > Enable this bit in the irq resource request/free if: > - gpio is in wakeirq map > - has the intr_wakeup_present_bit > - the intr_wakeup_enable_bit is set > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Wed, Oct 25, 2023 at 09:33:52AM +0200, Neil Armstrong wrote: > New platforms uses a new set of bits to control the wakeirq > delivery to the PDC block. > > The intr_wakeup_present_bit indicates if the GPIO supports > wakeirq and intr_wakeup_enable_bit enables wakeirq delivery > to the PDC block. > > While the name seems to imply this only enables wakeup events, > it is required to allow interrupts events to the PDC block. > > Enable this bit in the irq resource request/free if: > - gpio is in wakeirq map > - has the intr_wakeup_present_bit > - the intr_wakeup_enable_bit is set > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 32 ++++++++++++++++++++++++++++++++ > drivers/pinctrl/qcom/pinctrl-msm.h | 5 +++++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 395040346d0f..2489a9ac8455 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -1196,6 +1196,7 @@ static int msm_gpio_irq_reqres(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]; > int ret; > > if (!try_module_get(gc->owner)) > @@ -1221,6 +1222,24 @@ static int msm_gpio_irq_reqres(struct irq_data *d) > */ > irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY); > > + /* > + * If the wakeup_enable bit is present and marked as available for the > + * requested GPIO, it should be enabled when the GPIO is marked as > + * wake irq in order to allow the interrupt event to be transfered to > + * the PDC HW. > + * While the name implies only the wakeup event, it's also required for > + * the interrupt event. > + */ > + if (test_bit(d->hwirq, pctrl->skip_wake_irqs) && g->intr_wakeup_present_bit) { > + u32 intr_cfg; > + > + intr_cfg = msm_readl_intr_cfg(pctrl, g); > + if (intr_cfg & BIT(g->intr_wakeup_present_bit)) { > + intr_cfg |= BIT(g->intr_wakeup_enable_bit); > + msm_writel_intr_cfg(intr_cfg, pctrl, g); If I understand correctly, the two modified functions are hooked straight into the irq_chip, which would imply that nothing prevent concurrent execution of this and the other accessors of intr_cfg. If I'm reading this correctly, I think we should perform this read-modify-write under the spinlock. Regards, Bjorn > + } > + } > + > return 0; > out: > module_put(gc->owner); > @@ -1230,6 +1249,19 @@ static int msm_gpio_irq_reqres(struct irq_data *d) > static void msm_gpio_irq_relres(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]; > + > + /* Disable the wakeup_enable bit if it has been set in msm_gpio_irq_reqres() */ > + if (test_bit(d->hwirq, pctrl->skip_wake_irqs) && g->intr_wakeup_present_bit) { > + u32 intr_cfg; > + > + intr_cfg = msm_readl_intr_cfg(pctrl, g); > + if (intr_cfg & BIT(g->intr_wakeup_present_bit)) { > + intr_cfg &= ~BIT(g->intr_wakeup_enable_bit); > + msm_writel_intr_cfg(intr_cfg, pctrl, g); > + } > + } > > gpiochip_unlock_as_irq(gc, d->hwirq); > module_put(gc->owner); > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h > index 4968d08a384d..63852ed70295 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.h > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h > @@ -58,6 +58,9 @@ struct pinctrl_pin_desc; > * @intr_enable_bit: Offset in @intr_cfg_reg for enabling the interrupt for this group. > * @intr_status_bit: Offset in @intr_status_reg for reading and acking the interrupt > * status. > + * @intr_wakeup_present_bit: Offset in @intr_target_reg specifying the GPIO can generate > + * wakeup events. > + * @intr_wakeup_enable_bit: Offset in @intr_target_reg to enable wakeup events for the GPIO. > * @intr_target_bit: Offset in @intr_target_reg for configuring the interrupt routing. > * @intr_target_width: Number of bits used for specifying interrupt routing target. > * @intr_target_kpss_val: Value in @intr_target_bit for specifying that the interrupt from > @@ -100,6 +103,8 @@ struct msm_pingroup { > unsigned intr_status_bit:5; > unsigned intr_ack_high:1; > > + unsigned intr_wakeup_present_bit:5; > + unsigned intr_wakeup_enable_bit:5; > unsigned intr_target_bit:5; > unsigned intr_target_width:5; > unsigned intr_target_kpss_val:5; > > -- > 2.34.1 >
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 395040346d0f..2489a9ac8455 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -1196,6 +1196,7 @@ static int msm_gpio_irq_reqres(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]; int ret; if (!try_module_get(gc->owner)) @@ -1221,6 +1222,24 @@ static int msm_gpio_irq_reqres(struct irq_data *d) */ irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY); + /* + * If the wakeup_enable bit is present and marked as available for the + * requested GPIO, it should be enabled when the GPIO is marked as + * wake irq in order to allow the interrupt event to be transfered to + * the PDC HW. + * While the name implies only the wakeup event, it's also required for + * the interrupt event. + */ + if (test_bit(d->hwirq, pctrl->skip_wake_irqs) && g->intr_wakeup_present_bit) { + u32 intr_cfg; + + intr_cfg = msm_readl_intr_cfg(pctrl, g); + if (intr_cfg & BIT(g->intr_wakeup_present_bit)) { + intr_cfg |= BIT(g->intr_wakeup_enable_bit); + msm_writel_intr_cfg(intr_cfg, pctrl, g); + } + } + return 0; out: module_put(gc->owner); @@ -1230,6 +1249,19 @@ static int msm_gpio_irq_reqres(struct irq_data *d) static void msm_gpio_irq_relres(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]; + + /* Disable the wakeup_enable bit if it has been set in msm_gpio_irq_reqres() */ + if (test_bit(d->hwirq, pctrl->skip_wake_irqs) && g->intr_wakeup_present_bit) { + u32 intr_cfg; + + intr_cfg = msm_readl_intr_cfg(pctrl, g); + if (intr_cfg & BIT(g->intr_wakeup_present_bit)) { + intr_cfg &= ~BIT(g->intr_wakeup_enable_bit); + msm_writel_intr_cfg(intr_cfg, pctrl, g); + } + } gpiochip_unlock_as_irq(gc, d->hwirq); module_put(gc->owner); diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h index 4968d08a384d..63852ed70295 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.h +++ b/drivers/pinctrl/qcom/pinctrl-msm.h @@ -58,6 +58,9 @@ struct pinctrl_pin_desc; * @intr_enable_bit: Offset in @intr_cfg_reg for enabling the interrupt for this group. * @intr_status_bit: Offset in @intr_status_reg for reading and acking the interrupt * status. + * @intr_wakeup_present_bit: Offset in @intr_target_reg specifying the GPIO can generate + * wakeup events. + * @intr_wakeup_enable_bit: Offset in @intr_target_reg to enable wakeup events for the GPIO. * @intr_target_bit: Offset in @intr_target_reg for configuring the interrupt routing. * @intr_target_width: Number of bits used for specifying interrupt routing target. * @intr_target_kpss_val: Value in @intr_target_bit for specifying that the interrupt from @@ -100,6 +103,8 @@ struct msm_pingroup { unsigned intr_status_bit:5; unsigned intr_ack_high:1; + unsigned intr_wakeup_present_bit:5; + unsigned intr_wakeup_enable_bit:5; unsigned intr_target_bit:5; unsigned intr_target_width:5; unsigned intr_target_kpss_val:5;
New platforms uses a new set of bits to control the wakeirq delivery to the PDC block. The intr_wakeup_present_bit indicates if the GPIO supports wakeirq and intr_wakeup_enable_bit enables wakeirq delivery to the PDC block. While the name seems to imply this only enables wakeup events, it is required to allow interrupts events to the PDC block. Enable this bit in the irq resource request/free if: - gpio is in wakeirq map - has the intr_wakeup_present_bit - the intr_wakeup_enable_bit is set Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 32 ++++++++++++++++++++++++++++++++ drivers/pinctrl/qcom/pinctrl-msm.h | 5 +++++ 2 files changed, 37 insertions(+)