mbox series

[00/11] ARM/arm64: dts: qcom: fix USB wakeup interrupt types

Message ID 20231120164331.8116-1-johan+linaro@kernel.org
Headers show
Series ARM/arm64: dts: qcom: fix USB wakeup interrupt types | expand

Message

Johan Hovold Nov. 20, 2023, 4:43 p.m. UTC 
When testing a recent series that addresses resource leaks in the
Qualcomm dwc3 glue driver [1], I realised that probe deferral can break
wakeup from suspend due to how the wakeup interrupts are currently
requested.

The following series fixes this by no longer overriding the firmware
defined trigger types for the wakeup interrupts:

	https://lore.kernel.org/lkml/20231120161607.7405-1-johan+linaro@kernel.org/

It turns out a number Qualcomm devicetrees have also gotten the trigger
types wrong, something which this series addresses.

Specifically, the HS/SS PHY wakeup interrupts are level triggered while
the DP/DM HS PHY interrupts are edge triggered, and which edge to
trigger on depends both on the use-case and on whether a Low speed or
Full/High speed device is connected.

Fortunately, there should be no dependency between this series and USB
one as all devicetree use the correct trigger type for the HS/SS PHY
interrupts and the HS one has never been armed by Linux anyway. The
DP/DM interrupt trigger types are also updated on suspend currently.

The only exception may be sc7280 where a recent cleanup patch
inadvertently switched the SS and DP trigger types, but that one should
just be backported anyway.

Note that the binding example is updated in the USB driver series
mentioned above.

Johan


[1] https://lore.kernel.org/lkml/20231117173650.21161-1-johan+linaro@kernel.org/


Johan Hovold (11):
  ARM: dts: qcom: sdx55: fix USB wakeup interrupt types
  arm64: dts: qcom: sa8775p: fix USB wakeup interrupt types
  arm64: dts: qcom: sc7180: fix USB wakeup interrupt types
  arm64: dts: qcom: sc7280: fix usb_1 wakeup interrupt types
  arm64: dts: qcom: sc7280: fix usb_2 wakeup interrupt types
  arm64: dts: qcom: sc8180x: fix USB wakeup interrupt types
  arm64: dts: qcom: sdm670: fix USB wakeup interrupt types
  arm64: dts: qcom: sdm845: fix USB wakeup interrupt types
  arm64: dts: qcom: sm6375: fix USB wakeup interrupt types
  arm64: dts: qcom: sm8150: fix USB wakeup interrupt types
  arm64: dts: qcom: sm8550: fix USB wakeup interrupt types

 arch/arm/boot/dts/qcom/qcom-sdx55.dtsi |  4 ++--
 arch/arm64/boot/dts/qcom/sa8775p.dtsi  | 12 ++++++------
 arch/arm64/boot/dts/qcom/sc7180.dtsi   |  4 ++--
 arch/arm64/boot/dts/qcom/sc7280.dtsi   |  8 ++++----
 arch/arm64/boot/dts/qcom/sc8180x.dtsi  |  8 ++++----
 arch/arm64/boot/dts/qcom/sdm670.dtsi   |  4 ++--
 arch/arm64/boot/dts/qcom/sdm845.dtsi   |  8 ++++----
 arch/arm64/boot/dts/qcom/sm6375.dtsi   |  4 ++--
 arch/arm64/boot/dts/qcom/sm8150.dtsi   |  8 ++++----
 arch/arm64/boot/dts/qcom/sm8550.dtsi   |  4 ++--
 10 files changed, 32 insertions(+), 32 deletions(-)

Comments

Richard Acayan Nov. 21, 2023, 2:01 a.m. UTC | #1 
On Mon, Nov 20, 2023 at 05:43:27PM +0100, Johan Hovold wrote:
> The DP/DM wakeup interrupts are edge triggered and which edge to trigger
> on depends on use-case and whether a Low speed or Full/High speed device
> is connected.
>
> Fixes: 07c8ded6e373 ("arm64: dts: qcom: add sdm670 and pixel 3a device trees")
> Cc: stable@vger.kernel.org      # 6.2
> Cc: Richard Acayan <mailingradian@gmail.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Acked-by: Richard Acayan <mailingradian@gmail.com>

>  arch/arm64/boot/dts/qcom/sdm670.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> index ba2043d67370..c873560ae9d5 100644
> --- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
> @@ -1297,8 +1297,8 @@ usb_1: usb@a6f8800 {
>  
>  			interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
>  				     <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 488 IRQ_TYPE_LEVEL_HIGH>,
> -				     <GIC_SPI 489 IRQ_TYPE_LEVEL_HIGH>;
> +				     <GIC_SPI 488 IRQ_TYPE_EDGE_BOTH>,
> +				     <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>;
>  			interrupt-names = "hs_phy_irq", "ss_phy_irq",
>  					  "dm_hs_phy_irq", "dp_hs_phy_irq";
>  
> -- 
> 2.41.0
>
Bjorn Andersson Dec. 8, 2023, 2:55 p.m. UTC | #2 
On Mon, 20 Nov 2023 17:43:20 +0100, Johan Hovold wrote:
> When testing a recent series that addresses resource leaks in the
> Qualcomm dwc3 glue driver [1], I realised that probe deferral can break
> wakeup from suspend due to how the wakeup interrupts are currently
> requested.
> 
> The following series fixes this by no longer overriding the firmware
> defined trigger types for the wakeup interrupts:
> 
> [...]

Applied, thanks!

[01/11] ARM: dts: qcom: sdx55: fix USB wakeup interrupt types
        commit: d0ec3c4c11c3b30e1f2d344973b2a7bf0f986734

Best regards,
Johan Hovold Dec. 11, 2023, 4:39 p.m. UTC | #3 
On Mon, Nov 20, 2023 at 05:43:20PM +0100, Johan Hovold wrote:

> It turns out a number Qualcomm devicetrees have also gotten the trigger
> types wrong, something which this series addresses.
> 
> Specifically, the HS/SS PHY wakeup interrupts are level triggered while
> the DP/DM HS PHY interrupts are edge triggered, and which edge to
> trigger on depends both on the use-case and on whether a Low speed or
> Full/High speed device is connected.
> 
> Fortunately, there should be no dependency between this series and USB
> one as all devicetree use the correct trigger type for the HS/SS PHY
> interrupts and the HS one has never been armed by Linux anyway. The
> DP/DM interrupt trigger types are also updated on suspend currently.

Konrad reported off-list that the sc8180x patch in this series breaks
probe of the dwc3 driver.

Turns out a number of these SoCs were using GIC interrupts for the
DP/DM_HS_PHY interrupts despite the fact that the driver tries to
reconfigure these as IRQ_TYPE_EDGE_FALLING (which the GIC does not
support) to detect disconnect events during suspend.

This is obviously broken and the proper fix is to replace the GIC
interrupts with the corresponding PDC interrupts. I believe Konrad is
digging out the magic numbers at this moment.

The following patches will need a follow-up fix:

>   ARM: dts: qcom: sdx55: fix USB wakeup interrupt types

>   arm64: dts: qcom: sc8180x: fix USB wakeup interrupt types
>   arm64: dts: qcom: sdm670: fix USB wakeup interrupt types
>   arm64: dts: qcom: sdm845: fix USB wakeup interrupt types
>   arm64: dts: qcom: sm6375: fix USB wakeup interrupt types
>   arm64: dts: qcom: sm8150: fix USB wakeup interrupt types

Sorry about not noticing this.

Johan
Krishna Kurapati Dec. 12, 2023, 9:30 a.m. UTC | #4 
On 12/11/2023 10:09 PM, Johan Hovold wrote:
> On Mon, Nov 20, 2023 at 05:43:20PM +0100, Johan Hovold wrote:
> 
>> It turns out a number Qualcomm devicetrees have also gotten the trigger
>> types wrong, something which this series addresses.
>>
>> Specifically, the HS/SS PHY wakeup interrupts are level triggered while
>> the DP/DM HS PHY interrupts are edge triggered, and which edge to
>> trigger on depends both on the use-case and on whether a Low speed or
>> Full/High speed device is connected.
>>
>> Fortunately, there should be no dependency between this series and USB
>> one as all devicetree use the correct trigger type for the HS/SS PHY
>> interrupts and the HS one has never been armed by Linux anyway. The
>> DP/DM interrupt trigger types are also updated on suspend currently.
> 
> Konrad reported off-list that the sc8180x patch in this series breaks
> probe of the dwc3 driver.
> 
> Turns out a number of these SoCs were using GIC interrupts for the
> DP/DM_HS_PHY interrupts despite the fact that the driver tries to
> reconfigure these as IRQ_TYPE_EDGE_FALLING (which the GIC does not
> support) to detect disconnect events during suspend.
> 
> This is obviously broken and the proper fix is to replace the GIC
> interrupts with the corresponding PDC interrupts. I believe Konrad is
> digging out the magic numbers at this moment.
> 
> The following patches will need a follow-up fix:
> 
>>    ARM: dts: qcom: sdx55: fix USB wakeup interrupt types
> 
>>    arm64: dts: qcom: sc8180x: fix USB wakeup interrupt types
>>    arm64: dts: qcom: sdm670: fix USB wakeup interrupt types
>>    arm64: dts: qcom: sdm845: fix USB wakeup interrupt types
>>    arm64: dts: qcom: sm6375: fix USB wakeup interrupt types
>>    arm64: dts: qcom: sm8150: fix USB wakeup interrupt types
> 
Hi Johan,

  If it helps, I tried to dig up the PDC numbers for corresponding 
GIC_SPI vectors:


SM8150:

eud_p0_dpse_int_mx	apps_pdc_irq_out[9]	SYS_apcsQgicSPI[489]
eud_p0_dmse_int_mx    apps_pdc_irq_out[8]	SYS_apcsQgicSPI[488]
qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[6]	SYS_apcsQgicSPI[486]
usb31_power_event_irq	SYS_apcsQgicSPI[130]
usb31_hs_phy_irq	SYS_apcsQgicSPI[131]

interrupts-extended = <&pdc 9 IRQ_TYPE_EDGE_RISING>,
			<&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
			<&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
			<&pdc 8 IRQ_TYPE_EDGE_RISING>;

interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
		"ss_phy_irq", "dm_hs_phy_irq";

--

sdm845-670-usb-common.dtsi

interrupts = <0 489 0>, <0 130 0>, <0 486 0>, <0 488 0>;
interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
		"ss_phy_irq", "dm_hs_phy_irq";

interrupts = <0 491 0>, <0 135 0>, <0 487 0>, <0 490 0>;
interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
		"ss_phy_irq", "dm_hs_phy_irq";

eud_p0_dpse_int_mx	apps_pdc_irq_out[9]	SYS_apssQgicSPI[489]
eud_p0_dmse_int_mx	apps_pdc_irq_out[8]	SYS_apssQgicSPI[488]
eud_p1_dmse_int_mx	apps_pdc_irq_out[10]	SYS_apssQgicSPI[490]
eud_p1_dpse_int_mx	apps_pdc_irq_out[11]	SYS_apssQgicSPI[491]
qmp_usb3_lfps_rxterm_irq	apps_pdc_irq_out[7]	SYS_apssQgicSPI[487]
qmp_usb3_lfps_rxterm_irq	apps_pdc_irq_out[6]	SYS_apssQgicSPI[486]

--

SDX55:

interrupts = <0 157 0>, <0 130 0>, <0 158 0>, <0 198 0>;
interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
		"dm_hs_phy_irq", "ss_phy_irq";

eud_p1_dpse_int_mx	apps_pdc_irq_out[10]	SYS_apcsQgicSPI[157]
eud_p1_dmse_int_mx	apps_pdc_irq_out[11]	SYS_apcsQgicSPI[158]
apps_pdc.gp_irq_mux[31]	apps_pdc_irq_out[51]	SYS_apcsQgicSPI[198]

--

SM6375, I think GIC_SPI is fine but I will try to get back on this.

Sorry for bad formatting.

Regards,
Krishna,
Johan Hovold Dec. 13, 2023, 5:38 p.m. UTC | #5 
On Tue, Dec 12, 2023 at 03:00:07PM +0530, Krishna Kurapati PSSNV wrote:
> On 12/11/2023 10:09 PM, Johan Hovold wrote:
> > On Mon, Nov 20, 2023 at 05:43:20PM +0100, Johan Hovold wrote:

> > Konrad reported off-list that the sc8180x patch in this series breaks
> > probe of the dwc3 driver.
> > 
> > Turns out a number of these SoCs were using GIC interrupts for the
> > DP/DM_HS_PHY interrupts despite the fact that the driver tries to
> > reconfigure these as IRQ_TYPE_EDGE_FALLING (which the GIC does not
> > support) to detect disconnect events during suspend.
> > 
> > This is obviously broken and the proper fix is to replace the GIC
> > interrupts with the corresponding PDC interrupts. I believe Konrad is
> > digging out the magic numbers at this moment.
> > 
> > The following patches will need a follow-up fix:
> > 
> >>    ARM: dts: qcom: sdx55: fix USB wakeup interrupt types
> > 
> >>    arm64: dts: qcom: sc8180x: fix USB wakeup interrupt types
> >>    arm64: dts: qcom: sdm670: fix USB wakeup interrupt types
> >>    arm64: dts: qcom: sdm845: fix USB wakeup interrupt types
> >>    arm64: dts: qcom: sm6375: fix USB wakeup interrupt types
> >>    arm64: dts: qcom: sm8150: fix USB wakeup interrupt types

>   If it helps, I tried to dig up the PDC numbers for corresponding 
> GIC_SPI vectors:

Thanks, Krisha, that helps a lot.

I've sent two series (for arm and arm64) based on yours and Konrad's
input:

	https://lore.kernel.org/lkml/20231213173131.29436-1-johan+linaro@kernel.org/
	https://lore.kernel.org/lkml/20231213173403.29544-1-johan+linaro@kernel.org/

> SM8150:
> 
> eud_p0_dpse_int_mx	apps_pdc_irq_out[9]	SYS_apcsQgicSPI[489]
> eud_p0_dmse_int_mx    apps_pdc_irq_out[8]	SYS_apcsQgicSPI[488]
> qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[6]	SYS_apcsQgicSPI[486]
> usb31_power_event_irq	SYS_apcsQgicSPI[130]
> usb31_hs_phy_irq	SYS_apcsQgicSPI[131]
> 
> interrupts-extended = <&pdc 9 IRQ_TYPE_EDGE_RISING>,
> 			<&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
> 			<&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
> 			<&pdc 8 IRQ_TYPE_EDGE_RISING>;
> 
> interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
> 		"ss_phy_irq", "dm_hs_phy_irq";

Do you have the corresponding numbers also for the second controller on
SM8150? I inferred them from SDM845, but it would good to verify that.

And can someone dig out the corresponding SS PHY interrupt for sc8180x?

Johan
Konrad Dybcio Dec. 13, 2023, 6:39 p.m. UTC | #6 
On 12/12/23 10:30, Krishna Kurapati PSSNV wrote:

[...]

> SM6375, I think GIC_SPI is fine but I will try to get back on this.
interrupts-extended = <&intc GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>,
                                               <&mpm 12 IRQ_TYPE_LEVEL_HIGH>,
                                               <&mpm 93 IRQ_TYPE_EDGE_BOTH>,
                                               <&mpm 94 IRQ_TYPE_EDGE_BOTH>;
                         interrupt-names = "hs_phy_irq",
                                           "ss_phy_irq",
                                           "dm_hs_phy_irq",
                                           "dp_hs_phy_irq";

the mpm node is not yet upstream (I only managed to untangle the
related mess recently), I'll submit this soon.

Thanks Krishna and Johan for looking into this!

Konrad
Johan Hovold Dec. 14, 2023, 7:49 a.m. UTC | #7 
On Wed, Dec 13, 2023 at 07:39:59PM +0100, Konrad Dybcio wrote:
> On 12/12/23 10:30, Krishna Kurapati PSSNV wrote:
> 
> [...]
> 
> > SM6375, I think GIC_SPI is fine but I will try to get back on this.
> interrupts-extended = <&intc GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>,
>                                                <&mpm 12 IRQ_TYPE_LEVEL_HIGH>,
>                                                <&mpm 93 IRQ_TYPE_EDGE_BOTH>,
>                                                <&mpm 94 IRQ_TYPE_EDGE_BOTH>;
>                          interrupt-names = "hs_phy_irq",
>                                            "ss_phy_irq",
>                                            "dm_hs_phy_irq",
>                                            "dp_hs_phy_irq";
> 
> the mpm node is not yet upstream (I only managed to untangle the
> related mess recently), I'll submit this soon.

Perfect, thanks!

Johan