Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <028097f3-9056-4c07-a868-4eeac9bc8c94@quicinc.com>
Date:   Thu, 7 Dec 2023 21:14:55 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/6] dt-bindings: usb: dwc3: Clean up hs_phy_irq in
 bindings
To:     Johan Hovold <johan@kernel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Wesley Cheng <quic_wcheng@quicinc.com>,
        Conor Dooley <conor+dt@kernel.org>,
        <cros-qcom-dts-watchers@chromium.org>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <quic_ppratap@quicinc.com>,
        <quic_jackp@quicinc.com>
References: <20231204100950.28712-1-quic_kriskura@quicinc.com>
 <20231204100950.28712-2-quic_kriskura@quicinc.com>
 <ZXHjXGEbdtbCiOck@hovoldconsulting.com>
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <ZXHjXGEbdtbCiOck@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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,