| Message ID | 20230621043628.21485-2-quic_kriskura@quicinc.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, 21 Jun 2023 10:06:19 +0530, Krishna Kurapati wrote: > Add the compatible string for SC8280 Multiport USB controller from > Qualcomm. > > There are 4 power event irq interrupts supported by this controller > (one for each port of multiport). Added all the 4 as non-optional > interrupts for SC8280XP-MP > > Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM > IRQ's related to 4 ports of SC8280XP Teritiary controller. > > Also added ss phy irq for both SS Ports. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > .../devicetree/bindings/usb/qcom,dwc3.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote: > Add the compatible string for SC8280 Multiport USB controller from > Qualcomm. > > There are 4 power event irq interrupts supported by this controller > (one for each port of multiport). Added all the 4 as non-optional > interrupts for SC8280XP-MP > > Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM > IRQ's related to 4 ports of SC8280XP Teritiary controller. > > Also added ss phy irq for both SS Ports. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,sc8280xp-dwc3-mp > + then: > + properties: > + interrupts: You also need minItems 14 here. > + maxItems: 14 > + interrupt-names: And here, I think. > + items: > + - const: dp1_hs_phy_irq > + - const: dm1_hs_phy_irq > + - const: dp2_hs_phy_irq > + - const: dm2_hs_phy_irq > + - const: dp3_hs_phy_irq > + - const: dm4_hs_phy_irq > + - const: dp4_hs_phy_irq > + - const: dm4_hs_phy_irq > + - const: ss1_phy_irq > + - const: ss2_phy_irq > + - const: pwr_event_1 > + - const: pwr_event_2 > + - const: pwr_event_3 > + - const: pwr_event_4 The naming here is inconsistent and interrupts should not have "_irq" suffixes (even if some of the current ones do for historical reasons). I believe these should be named pwr_event_1 dp_hs_phy_1 dm_hs_phy_1 ss_phy_1 pwr_event_2 dp_hs_phy_2 dm_hs_phy_2 ss_phy_2 pwr_event_3 dp_hs_phy_3 dm_hs_phy_3 pwr_event_4 dp_hs_phy_4 dm_hs_phy_4 or similar and be grouped by port while using the the qcom,sc8280xp-dwc ordering for the individual lines. Side note: Please note how the above interrupt properties can also be used to infer the number of HS and SS ports. > + > additionalProperties: false > > examples: Johan
On Tue, Jun 27, 2023 at 01:20:59PM +0200, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote: > > + items: > > + - const: dp1_hs_phy_irq > > + - const: dm1_hs_phy_irq > > + - const: dp2_hs_phy_irq > > + - const: dm2_hs_phy_irq > > + - const: dp3_hs_phy_irq > > + - const: dm4_hs_phy_irq > > + - const: dp4_hs_phy_irq > > + - const: dm4_hs_phy_irq > > + - const: ss1_phy_irq > > + - const: ss2_phy_irq > > + - const: pwr_event_1 > > + - const: pwr_event_2 > > + - const: pwr_event_3 > > + - const: pwr_event_4 > > The naming here is inconsistent and interrupts should not have "_irq" > suffixes (even if some of the current ones do for historical reasons). > > I believe these should be named > > pwr_event_1 > dp_hs_phy_1 > dm_hs_phy_1 > ss_phy_1 > > pwr_event_2 > dp_hs_phy_2 > dm_hs_phy_2 > ss_phy_2 > > pwr_event_3 > dp_hs_phy_3 > dm_hs_phy_3 > > pwr_event_4 > dp_hs_phy_4 > dm_hs_phy_4 > > or similar and be grouped by port while using the the > qcom,sc8280xp-dwc ordering for the individual lines. Perhaps the ordering you suggested is fine too, but I'd probably move the pwr_event ones first to match qcom,sc8280xp-dwc then, that is: pwr_event_1 pwr_event_2 pwr_event_3 pwr_event_4 dp_hs_phy_1 dm_hs_phy_1 dp_hs_phy_2 dm_hs_phy_2 dp_hs_phy_3 dm_hs_phy_3 dp_hs_phy_4 dm_hs_phy_4 ss_phy_1 ss_phy_2 so we have them grouped as pwr_event followed by HS and with SS last. > Side note: Please note how the above interrupt properties can also be > used to infer the number of HS and SS ports. Johan
On 6/27/2023 9:08 PM, Johan Hovold wrote: > On Tue, Jun 27, 2023 at 01:20:59PM +0200, Johan Hovold wrote: >> On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote: > >>> + items: >>> + - const: dp1_hs_phy_irq >>> + - const: dm1_hs_phy_irq >>> + - const: dp2_hs_phy_irq >>> + - const: dm2_hs_phy_irq >>> + - const: dp3_hs_phy_irq >>> + - const: dm4_hs_phy_irq >>> + - const: dp4_hs_phy_irq >>> + - const: dm4_hs_phy_irq >>> + - const: ss1_phy_irq >>> + - const: ss2_phy_irq >>> + - const: pwr_event_1 >>> + - const: pwr_event_2 >>> + - const: pwr_event_3 >>> + - const: pwr_event_4 >> >> The naming here is inconsistent and interrupts should not have "_irq" >> suffixes (even if some of the current ones do for historical reasons). >> >> I believe these should be named >> >> pwr_event_1 >> dp_hs_phy_1 >> dm_hs_phy_1 >> ss_phy_1 >> >> pwr_event_2 >> dp_hs_phy_2 >> dm_hs_phy_2 >> ss_phy_2 >> >> pwr_event_3 >> dp_hs_phy_3 >> dm_hs_phy_3 >> >> pwr_event_4 >> dp_hs_phy_4 >> dm_hs_phy_4 >> >> or similar and be grouped by port while using the the >> qcom,sc8280xp-dwc ordering for the individual lines. > > Perhaps the ordering you suggested is fine too, but I'd probably move > the pwr_event ones first to match qcom,sc8280xp-dwc then, that is: > > pwr_event_1 > pwr_event_2 > pwr_event_3 > pwr_event_4 > dp_hs_phy_1 > dm_hs_phy_1 > dp_hs_phy_2 > dm_hs_phy_2 > dp_hs_phy_3 > dm_hs_phy_3 > dp_hs_phy_4 > dm_hs_phy_4 > ss_phy_1 > ss_phy_2 > > so we have them grouped as pwr_event followed by HS and with SS last. > >> Side note: Please note how the above interrupt properties can also be >> used to infer the number of HS and SS ports. > > Johan Can't we just cleanup all at once later ? Might not be a good idea for some properties in the file to have _irq and for some to not have it. I will modify the order though. Regards, Krishna,
On Mon, Jul 03, 2023 at 12:41:59AM +0530, Krishna Kurapati PSSNV wrote: > On 6/27/2023 9:08 PM, Johan Hovold wrote: > > On Tue, Jun 27, 2023 at 01:20:59PM +0200, Johan Hovold wrote: > >> On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote: > > > >>> + items: > >>> + - const: dp1_hs_phy_irq > >>> + - const: dm1_hs_phy_irq > >>> + - const: dp2_hs_phy_irq > >>> + - const: dm2_hs_phy_irq > >>> + - const: dp3_hs_phy_irq > >>> + - const: dm4_hs_phy_irq > >>> + - const: dp4_hs_phy_irq > >>> + - const: dm4_hs_phy_irq > >>> + - const: ss1_phy_irq > >>> + - const: ss2_phy_irq > >>> + - const: pwr_event_1 > >>> + - const: pwr_event_2 > >>> + - const: pwr_event_3 > >>> + - const: pwr_event_4 > >> > >> The naming here is inconsistent and interrupts should not have "_irq" > >> suffixes (even if some of the current ones do for historical reasons). > >> > >> I believe these should be named > >> > >> pwr_event_1 > >> dp_hs_phy_1 > >> dm_hs_phy_1 > >> ss_phy_1 > >> > >> pwr_event_2 > >> dp_hs_phy_2 > >> dm_hs_phy_2 > >> ss_phy_2 > >> > >> pwr_event_3 > >> dp_hs_phy_3 > >> dm_hs_phy_3 > >> > >> pwr_event_4 > >> dp_hs_phy_4 > >> dm_hs_phy_4 > >> > >> or similar and be grouped by port while using the the > >> qcom,sc8280xp-dwc ordering for the individual lines. > > > > Perhaps the ordering you suggested is fine too, but I'd probably move > > the pwr_event ones first to match qcom,sc8280xp-dwc then, that is: > > > > pwr_event_1 > > pwr_event_2 > > pwr_event_3 > > pwr_event_4 > > dp_hs_phy_1 > > dm_hs_phy_1 > > dp_hs_phy_2 > > dm_hs_phy_2 > > dp_hs_phy_3 > > dm_hs_phy_3 > > dp_hs_phy_4 > > dm_hs_phy_4 > > ss_phy_1 > > ss_phy_2 > > > > so we have them grouped as pwr_event followed by HS and with SS last. > > > >> Side note: Please note how the above interrupt properties can also be > >> used to infer the number of HS and SS ports. > Can't we just cleanup all at once later ? Might not be a good idea for > some properties in the file to have _irq and for some to not have it. I > will modify the order though. No, DT bindings generally need to be as correct as possible from the start as they form an ABI. So please drop the _irq suffix from all of the new indexed names. Johan
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml index d84281926f10..fb3988208b27 100644 --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml @@ -26,6 +26,7 @@ properties: - qcom,sc7180-dwc3 - qcom,sc7280-dwc3 - qcom,sc8280xp-dwc3 + - qcom,sc8280xp-dwc3-mp - qcom,sdm660-dwc3 - qcom,sdm670-dwc3 - qcom,sdm845-dwc3 @@ -262,6 +263,7 @@ allOf: contains: enum: - qcom,sc8280xp-dwc3 + - qcom,sc8280xp-dwc3-mp then: properties: clocks: @@ -455,6 +457,33 @@ allOf: - const: dm_hs_phy_irq - const: ss_phy_irq + - if: + properties: + compatible: + contains: + enum: + - qcom,sc8280xp-dwc3-mp + then: + properties: + interrupts: + maxItems: 14 + interrupt-names: + items: + - const: dp1_hs_phy_irq + - const: dm1_hs_phy_irq + - const: dp2_hs_phy_irq + - const: dm2_hs_phy_irq + - const: dp3_hs_phy_irq + - const: dm4_hs_phy_irq + - const: dp4_hs_phy_irq + - const: dm4_hs_phy_irq + - const: ss1_phy_irq + - const: ss2_phy_irq + - const: pwr_event_1 + - const: pwr_event_2 + - const: pwr_event_3 + - const: pwr_event_4 + additionalProperties: false examples:
Add the compatible string for SC8280 Multiport USB controller from Qualcomm. There are 4 power event irq interrupts supported by this controller (one for each port of multiport). Added all the 4 as non-optional interrupts for SC8280XP-MP Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM IRQ's related to 4 ports of SC8280XP Teritiary controller. Also added ss phy irq for both SS Ports. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- .../devicetree/bindings/usb/qcom,dwc3.yaml | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+)