lkml.org 
[lkml]   [2023]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in bindings
    From

    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,

    \
     
     \ /
      Last update: 2023-12-07 16:45    [W:2.428 / U:0.852 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site