Re: [PATCH v3 3/3] arm64: dts: qcom: shikra: enable WCN SW_CTRL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: qcom: shikra: unconfigured trigger type on wcn3988-pmu wake IRQ
--

commit 0d919d176f5987f0cfa1c7fc5b73e582dabc0bf6
Author: Linghui Wu <linghui.wu@oss.qualcomm.com>

arm64: dts: qcom: shikra: enable WCN SW_CTRL

This commit marks the wcn3988-pmu node as a wakeup source on the Shikra
CQM, CQS, and IQS SOMs so the power sequencing driver registers the
SW_CTRL GPIO as a dedicated wake IRQ.

> diff --git a/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi b/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi
> index 95eb7255fb227..c7b894b6126c1 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi
> @@ -102,6 +102,7 @@ wcn3988-pmu {
>  		pinctrl-names = "default";
>  
>  		swctrl-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
> +		wakeup-source;
>  
>  		vddio-supply = <&pm4125_l7>;
>  		vddxo-supply = <&pm4125_l13>;
> diff --git a/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi b/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi
> index ebc3052d57a99..5edcabd9dae17 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi
> @@ -85,6 +85,7 @@ wcn3988-pmu {
>  		pinctrl-names = "default";
>  
>  		swctrl-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
> +		wakeup-source;

[Severity: High]
Does enabling wakeup-source on wcn3988-pmu leave the TLMM GPIO 88 wake IRQ
without a configured trigger type and stuck on handle_bad_irq?

When wakeup-source is present, pwrseq_qcom_wcn_probe() in
drivers/power/sequencing/pwrseq-qcom-wcn.c maps swctrl-gpios to an IRQ and
registers it as a dedicated wake IRQ:

    wake_irq = gpiod_to_irq(swctrl_gpio);
    if (wake_irq < 0)
            return dev_err_probe(dev, wake_irq,
                                 "Failed to get the SW_CTRL wake IRQ\n");

    ret = devm_device_init_wakeup(dev);
    if (ret)
            return dev_err_probe(dev, ret,
                                 "Failed to enable wakeup\n");

    ret = dev_pm_set_dedicated_wake_irq(dev, wake_irq);

In drivers/pinctrl/qcom/pinctrl-msm.c, msm_gpio_init() initializes the GPIO
IRQ chip with no default trigger type and handle_bad_irq:

    girq->default_type = IRQ_TYPE_NONE;
    girq->handler = handle_bad_irq;

Because tlmm uses a hierarchical IRQ domain with wakeup-parent = <&mpm>,
gpiod_to_irq() -> gpiochip_to_irq() creates the mapping with IRQ_TYPE_NONE.
Then dev_pm_set_dedicated_wake_irq() -> __dev_pm_set_dedicated_wake_irq()
in drivers/base/power/wakeirq.c requests the IRQ without any
IRQF_TRIGGER_* flags:

    err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
                               IRQF_ONESHOT | IRQF_NO_AUTOEN,
                               wirq->name, wirq);

In __setup_irq(), because neither the request flags nor the existing
irq_data specifies any trigger bits, __irq_set_trigger() is skipped and
neither msm_gpio_irq_set_type() nor qcom_mpm_set_type() is called. As a
result, the TLMM and MPM trigger registers remain unconfigured and
desc->handle_irq stays set to handle_bad_irq instead of handle_level_irq
or handle_edge_irq.

Should the trigger type be configured before or when registering the wake
IRQ, or specified via an interrupts property?

>  
>  		vddio-supply = <&pm8150_s4>;
>  		vddxo-supply = <&pm8150_l12>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019093911.3199891-1-linghui.wu@oss.qualcomm.com?part=3