| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <028097f3-9056-4c07-a868-4eeac9bc8c94@quicinc.com>
Date: Thu, 7 Dec 2023 21:14:55 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Johan Hovold <johan@...nel.org>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
Conor Dooley <conor+dt@...nel.org>,
<cros-qcom-dts-watchers@...omium.org>, <linux-usb@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<devicetree@...r.kernel.org>, <quic_ppratap@...cinc.com>,
<quic_jackp@...cinc.com>
Subject: Re: [PATCH v2 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in
bindings
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@...8800: 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,
Powered by blists - more mailing lists