mbox series

[0/7] usb: dwc3: add support for SC8280XP

Message ID 20220713131340.29401-1-johan+linaro@kernel.org
Headers show
Series usb: dwc3: add support for SC8280XP | expand

Message

Johan Hovold July 13, 2022, 1:13 p.m. UTC 
This series adds the missing devicetree binding for SC8280XP, clean up
the current bindings somewhat and fixes a related driver warning when an
optional wakeup interrupt is missing.

The last four patches fix up the SC8280XP devicetree that's currently in
Bjorn's tree and addresses some DT schema warnings in other Qualcomm
devicetrees and are only included here for completeness.

The binding and driver updates are expected to go though the USB tree,
while the devicetree updates can be picked up by Bjorn once the binding
updates have been merged.

Johan


Johan Hovold (7):
  dt-bindings: usb: qcom,dwc3: add SC8280XP binding
  dt-bindings: usb: qcom,dwc3: refine interrupt requirements
  usb: dwc3: qcom: fix missing optional irq warnings
  arm64: dts: qcom: sc8280xp: fix USB clock order
  arm64: dts: qcom: sc8280xp: fix USB interrupts
  arm64: dts: qcom: sc7280: reorder USB interrupts
  arm64: dts: qcom: reorder USB interrupts

 .../devicetree/bindings/usb/qcom,dwc3.yaml    | 152 ++++++++++++++++--
 arch/arm/boot/dts/qcom-sdx65.dtsi             |  10 +-
 arch/arm64/boot/dts/qcom/sc7280.dtsi          |  15 +-
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi        |  28 ++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  20 ++-
 arch/arm64/boot/dts/qcom/sm8350.dtsi          |  20 ++-
 drivers/usb/dwc3/dwc3-qcom.c                  |   4 +-
 7 files changed, 194 insertions(+), 55 deletions(-)

Comments

Andrew Halaney July 13, 2022, 2 p.m. UTC | #1 
On Wed, Jul 13, 2022 at 03:13:36PM +0200, Johan Hovold wrote:
> Not all platforms have all of the four currently supported wakeup
> interrupts so use the optional irq helpers when looking up interrupts to
> avoid printing error messages when an optional interrupt is not found:
> 
> 	dwc3-qcom a6f8800.usb: error -ENXIO: IRQ hs_phy_irq not found
> 
> Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 77036551987a..c5e482f53e9d 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -490,9 +490,9 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev,
>  	int ret;
>  
>  	if (np)
> -		ret = platform_get_irq_byname(pdev_irq, name);
> +		ret = platform_get_irq_byname_optional(pdev_irq, name);
>  	else
> -		ret = platform_get_irq(pdev_irq, num);
> +		ret = platform_get_irq_optional(pdev_irq, num);
>  
>  	return ret;
>  }
> -- 
> 2.35.1
> 

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Andrew Halaney July 13, 2022, 2:12 p.m. UTC | #2 
On Wed, Jul 13, 2022 at 03:13:38PM +0200, Johan Hovold wrote:
> The two single-port SC8280XP USB controllers do not have an hs_phy_irq
> interrupt. Instead they have a pwr_event interrupt which is distinct
> from the former and not yet supported by the driver.
> 
> Fix the USB node interrupt names so that they match the devicetree
> binding.
> 
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 45cc7d714fd2..4a7aa9992f3a 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1875,8 +1875,10 @@ usb_0: usb@a6f8800 {
>  					      <&pdc 14 IRQ_TYPE_EDGE_BOTH>,
>  					      <&pdc 15 IRQ_TYPE_EDGE_BOTH>,
>  					      <&pdc 138 IRQ_TYPE_LEVEL_HIGH>;
> -			interrupt-names = "hs_phy_irq", "dp_hs_phy_irq",
> -					  "dm_hs_phy_irq", "ss_phy_irq";
> +			interrupt-names = "pwr_event",
> +					  "dp_hs_phy_irq",
> +					  "dm_hs_phy_irq",
> +					  "ss_phy_irq";
>  
>  			power-domains = <&gcc USB30_PRIM_GDSC>;
>  
> @@ -1925,8 +1927,10 @@ usb_1: usb@a8f8800 {
>  					      <&pdc 12 IRQ_TYPE_EDGE_BOTH>,
>  					      <&pdc 13 IRQ_TYPE_EDGE_BOTH>,
>  					      <&pdc 136 IRQ_TYPE_LEVEL_HIGH>;
> -			interrupt-names = "hs_phy_irq", "dp_hs_phy_irq",
> -					  "dm_hs_phy_irq", "ss_phy_irq";
> +			interrupt-names = "pwr_event",
> +					  "dp_hs_phy_irq",
> +					  "dm_hs_phy_irq",
> +					  "ss_phy_irq";

For this specific change to pwr_event:

    Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

That being said, I was reviewing this against the (fairly old)
downstream release I have, and the IRQs defined there look like this:

		interrupts-extended = <&pdc 12 IRQ_TYPE_EDGE_RISING>,
				<&intc GIC_SPI 811 IRQ_TYPE_LEVEL_HIGH>,
				<&pdc 136 IRQ_TYPE_LEVEL_HIGH>,
				<&pdc 13 IRQ_TYPE_EDGE_RISING>;
		interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
				"ss_phy_irq", "dm_hs_phy_irq";

The part I want to highlight is that the "pwr_event" irq downstream maps
to <&intc GIC_SPI 811 IRQ_TYPE_LEVEL_HIGH>, but the current upstream
devicetree I'm looking at has it mapped to <&intc GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>

Do you happen to have any source you can also check to confirm if this
is a bug or not?

Thanks,
Andrew

>  
>  			power-domains = <&gcc USB30_SEC_GDSC>;
>  
> -- 
> 2.35.1
>
Andrew Halaney July 13, 2022, 2:43 p.m. UTC | #3 
On Wed, Jul 13, 2022 at 04:35:22PM +0200, Johan Hovold wrote:
> On Wed, Jul 13, 2022 at 09:12:28AM -0500, Andrew Halaney wrote:
> > On Wed, Jul 13, 2022 at 03:13:38PM +0200, Johan Hovold wrote:
> > > The two single-port SC8280XP USB controllers do not have an hs_phy_irq
> > > interrupt. Instead they have a pwr_event interrupt which is distinct
> > > from the former and not yet supported by the driver.
> > > 
> > > Fix the USB node interrupt names so that they match the devicetree
> > > binding.
> > > 
> > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > index 45cc7d714fd2..4a7aa9992f3a 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > @@ -1875,8 +1875,10 @@ usb_0: usb@a6f8800 {
> > >  					      <&pdc 14 IRQ_TYPE_EDGE_BOTH>,
> > >  					      <&pdc 15 IRQ_TYPE_EDGE_BOTH>,
> > >  					      <&pdc 138 IRQ_TYPE_LEVEL_HIGH>;
> > > -			interrupt-names = "hs_phy_irq", "dp_hs_phy_irq",
> > > -					  "dm_hs_phy_irq", "ss_phy_irq";
> > > +			interrupt-names = "pwr_event",
> > > +					  "dp_hs_phy_irq",
> > > +					  "dm_hs_phy_irq",
> > > +					  "ss_phy_irq";
> > >  
> > >  			power-domains = <&gcc USB30_PRIM_GDSC>;
> > >  
> > > @@ -1925,8 +1927,10 @@ usb_1: usb@a8f8800 {
> > >  					      <&pdc 12 IRQ_TYPE_EDGE_BOTH>,
> > >  					      <&pdc 13 IRQ_TYPE_EDGE_BOTH>,
> > >  					      <&pdc 136 IRQ_TYPE_LEVEL_HIGH>;
> > > -			interrupt-names = "hs_phy_irq", "dp_hs_phy_irq",
> > > -					  "dm_hs_phy_irq", "ss_phy_irq";
> > > +			interrupt-names = "pwr_event",
> > > +					  "dp_hs_phy_irq",
> > > +					  "dm_hs_phy_irq",
> > > +					  "ss_phy_irq";
> > 
> > For this specific change to pwr_event:
> > 
> >     Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> > 
> > That being said, I was reviewing this against the (fairly old)
> > downstream release I have, and the IRQs defined there look like this:
> > 
> > 		interrupts-extended = <&pdc 12 IRQ_TYPE_EDGE_RISING>,
> > 				<&intc GIC_SPI 811 IRQ_TYPE_LEVEL_HIGH>,
> > 				<&pdc 136 IRQ_TYPE_LEVEL_HIGH>,
> > 				<&pdc 13 IRQ_TYPE_EDGE_RISING>;
> > 		interrupt-names = "dp_hs_phy_irq", "pwr_event_irq",
> > 				"ss_phy_irq", "dm_hs_phy_irq";
> > 
> > The part I want to highlight is that the "pwr_event" irq downstream maps
> > to <&intc GIC_SPI 811 IRQ_TYPE_LEVEL_HIGH>, but the current upstream
> > devicetree I'm looking at has it mapped to <&intc GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>
> > 
> > Do you happen to have any source you can also check to confirm if this
> > is a bug or not?
> 
> Good catch! I believe this is another copy-paste error when creating
> base dtsi based on some vendor source (or perhaps an error carried over
> from an earlier version).
> 
> The vendor devicetree I have access to also has 811 here, which matches
> the pattern for usb_0 (dwc3 interrupt + 1).
> 
> Do you mind if I fold a fix for that into a v2 of this patch?
> 
> Thanks for reviewing!

Sounds good, feel free to add my R-B with that change as well!

> 
> Johan
>
Krzysztof Kozlowski July 14, 2022, 10:48 a.m. UTC | #4 
On 13/07/2022 15:13, Johan Hovold wrote:
> Add SC8280XP to the DT schema.
> 
> Note that the SC8280XP controllers use the common set of five clocks and
> an additional set of four interconnect clocks whose purpose is not
> entirely clear at this point.

Thank you for your patch. There is something to discuss/improve.

>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq4019-dwc3
> +              - qcom,ipq6018-dwc3
> +              - qcom,ipq8064-dwc3
> +              - qcom,ipq8074-dwc3
> +              - qcom,msm8953-dwc3
> +              - qcom,msm8994-dwc3
> +              - qcom,msm8996-dwc3
> +              - qcom,msm8998-dwc3
> +              - qcom,qcs404-dwc3
> +              - qcom,sc7180-dwc3
> +              - qcom,sc7280-dwc3
> +              - qcom,sdm660-dwc3
> +              - qcom,sdm845-dwc3
> +              - qcom,sdx55-dwc3
> +              - qcom,sdx65-dwc3
> +              - qcom,sm4250-dwc3
> +              - qcom,sm6115-dwc3
> +              - qcom,sm6125-dwc3
> +              - qcom,sm6350-dwc3
> +              - qcom,sm8150-dwc3
> +              - qcom,sm8250-dwc3
> +              - qcom,sm8350-dwc3
> +              - qcom,sm8450-dwc3
> +    then:
> +      properties:
> +        interrupts:
> +          items:
> +            - description: The interrupt that is asserted
> +                when a wakeup event is received on USB2 bus.
> +            - description: The interrupt that is asserted
> +                when a wakeup event is received on USB3 bus.
> +            - description: Wakeup event on DM line.
> +            - description: Wakeup event on DP line.
> +        interrupt-names:
> +          items:
> +            - const: hs_phy_irq
> +            - const: ss_phy_irq
> +            - const: dm_hs_phy_irq
> +            - const: dp_hs_phy_irq
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sc8280xp-dwc3
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 4
> +        interrupt-names:
> +          items:
> +            - const: pwr_event
> +            - const: dp_hs_phy_irq
> +            - const: dm_hs_phy_irq
> +            - const: ss_phy_irq

Could you keep the closest order to exiting ones? In that case first
entry will differ, but rest at least will be the same.

>  
>  additionalProperties: false
>  


Best regards,
Krzysztof
Krzysztof Kozlowski July 14, 2022, 10:51 a.m. UTC | #5 
On 13/07/2022 15:13, Johan Hovold wrote:
> Add SC8280XP to the DT schema.
> 
> Note that the SC8280XP controllers use the common set of five clocks and
> an additional set of four interconnect clocks whose purpose is not
> entirely clear at this point.
> 
> The set of wakeup interrupts is also different for SC8280XP.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Second patch answers my questions, so:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Krzysztof Kozlowski July 14, 2022, 10:52 a.m. UTC | #6 
On 13/07/2022 15:13, Johan Hovold wrote:
> Fix the USB controller clock order and naming so that they match the
> devicetree binding.
> 
> Note that the driver currently simply enables all clocks in the order
> that they are specified in the devicetree. Reordering the clocks as per
> the binding means that the only explicit ordering constraint found in
> the vendor driver, that cfg_noc should be enabled before the core_clk,
> is now honoured.
> 
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 285a9828c250..45cc7d714fd2 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1855,16 +1855,16 @@ usb_0: usb@a6f8800 {
>  			#size-cells = <2>;
>  			ranges;
>  
> -			clocks = <&gcc GCC_USB30_PRIM_MASTER_CLK>,
> -				 <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
> +			clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
> +				 <&gcc GCC_USB30_PRIM_MASTER_CLK>,
>  				 <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>,
> -				 <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
>  				 <&gcc GCC_USB30_PRIM_SLEEP_CLK>,
> +				 <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
>  				 <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
>  				 <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
>  				 <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
>  				 <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
> -			clock-names = "core", "iface", "bus_aggr", "utmi", "sleep",
> +			clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
>  				      "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";

Your commit title should also include change of naming.

Best regards,
Krzysztof
Krzysztof Kozlowski July 14, 2022, 10:52 a.m. UTC | #7 
On 13/07/2022 15:13, Johan Hovold wrote:
> The two single-port SC8280XP USB controllers do not have an hs_phy_irq
> interrupt. Instead they have a pwr_event interrupt which is distinct
> from the former and not yet supported by the driver.
> 
> Fix the USB node interrupt names so that they match the devicetree
> binding.
> 
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof