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:0.090 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site