Messages in this thread | | | Date | Thu, 7 Dec 2023 21:14:55 +0530 | Subject | Re: [PATCH v2 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings | From | Krishna Kurapati PSSNV <> |
| |
Hi Johan. Thanks for the review.
> > And say something about the SS PHY interrupt being treated as optional > as there are SoCs with multiple controllers where only some supports SS. > > As Krzysztof mentioned you should also add something to motivate why > this de-facto ABI breakage by reordering interrupts is justified and > safe in this case. > Sure. Let me come up with a good reason why this breakage is needed.
> > Try to use uppercase 'PHY' consistently in text throughout the series. > ACK.
>> + - qusb2_phy:: SoCs with QUSB2 PHY do not have separate DP/DM IRQs and >> + expose only a single IRQ whose behavior can be modified >> + by the QUSB2PHY_INTR_CTRL register. The required DPSE/ >> + DMSE configuration is done in QUSB2PHY_INTR_CTRL register >> + of phy address space. >> + - {dp/dm}_hs_phy_irq:: These IRQ's directly reflect changes on the DP/ >> + DM pads of the SoC. These are used for wakeup >> + only on SoCs with non-QUSBb2 targets with > > QUSB2 typo > >> + exception of SDM670/SDM845/SM6350. >> + - ss_phy_irq:: When in super speed mode of operation, interrupts are > > Capitalise 'Super Speed' > >> + received when a wakeup event is received on ss_phy_irq. > > The description as it stands sounds circular. And this one is only used > for remote wakeup right? > Yes. It is used for remote wakeup. Mentioning it as wakeup event should be changed ?
>> + - hs_phY_irq:: Apart from DP/DM/QUSB2 Phy interrupts, there is > > s/phY/phy/ > > Perhaps rephrase to sound less like a commit message and to make it a > bit more concise. > > But this is already an improvement over the current descriptions which > are too terse and not even correct. > >> + hs_phy_irq which is not triggered by default and its >> + functionality is mutually exclusive to that of >> + {dp/dm}_hs_phy_irq and qusb2_phy_irq. >> + - pwr_event:: Used for wakeup based on other power events. > > I'm not sure about the free text description of these (format etc), but > at least this avoid repeating the descriptions for each permutation. > > Perhaps the DT maintainers can chime in here. > > I think you should reorder them to match the permutations below though. > Sure. Will reorder them as per permutations.
>> + minItems: 2 >> + maxItems: 5 >> >> interrupt-names: >> - minItems: 1 >> - maxItems: 4 >> + minItems: 2 >> + maxItems: 5 >> >> qcom,select-utmi-as-pipe-clk: >> description: >> @@ -359,116 +377,54 @@ allOf: >> compatible: >> contains: >> enum: >> - - qcom,ipq4019-dwc3 >> + - qcom,ipq5018-dwc3 >> - qcom,ipq6018-dwc3 >> - - qcom,ipq8064-dwc3 >> - qcom,ipq8074-dwc3 >> - - qcom,msm8994-dwc3 >> - - qcom,qcs404-dwc3 >> - - qcom,sc7180-dwc3 >> - - qcom,sdm670-dwc3 >> - - qcom,sdm845-dwc3 >> - - qcom,sdx55-dwc3 >> - - qcom,sdx65-dwc3 >> - - qcom,sdx75-dwc3 >> - - qcom,sm4250-dwc3 >> - - qcom,sm6350-dwc3 >> - - qcom,sm8150-dwc3 >> - - qcom,sm8250-dwc3 >> - - qcom,sm8350-dwc3 >> - - qcom,sm8450-dwc3 >> - - qcom,sm8550-dwc3 >> - - qcom,sm8650-dwc3 >> + - qcom,msm8953-dwc3 >> + - qcom,msm8998-dwc3 >> + - qcom,qcm2290-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 >> + - const: pwr_event >> + - const: qusb2_phy >> + - const: ss_phy_irq (optional) > > You should not include the string "(optional)" here. It was only a > notation I used when we discussed this earlier. > > The fact that these are optional should be expressed using min/maxItems > as I mentioned earlier. For the above SoCs that would be > > minItems: 2 > maxItems: 3 > >> @@ -522,12 +490,13 @@ examples: >> <&gcc GCC_USB30_PRIM_MASTER_CLK>; >> assigned-clock-rates = <19200000>, <150000000>; >> >> - interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>, >> - <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>, >> + interrupts = <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>, >> <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"; >> + <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "pwr_event", "hs_phy_irq", >> + "dp_hs_phy_irq", "dm_hs_phy_irq", "ss_phy_irq"; > > Perhaps you should align the continuation line here too. > >> >> power-domains = <&gcc USB30_PRIM_GDSC>; > > Also have you set up the tools so that you can verify your bindings > before posing them? I assume the above wouldn't pass (e.g. due to the > "(optional)" strings). > > There's some more details here: > > https://docs.kernel.org/devicetree/bindings/writing-schema.html > > under "Running checks".
I did do a dt-binding check and got the following line as well:
DTC_CHK Documentation/devicetree/bindings/usb/qcom,dwc3.example.dtb /local/mnt/workspace/sriramd/upstream/torvalds/linux/Documentation/devicetree/bindings/usb/qcom,dwc3.example.dtb: usb@a6f8800: interrupt-names:4: 'ss_phy_irq (optional)' was expected From schema:
I ignored this because it isn't a warning and the example dtb too was generated. Will remove the optional thing in v3.
Regards, Krishna,
| |