Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 29.03.2023 10:34, Johan Hovold wrote:
> On Wed, Mar 29, 2023 at 10:56:00AM +0530, Manivannan Sadhasivam wrote:
>> On Tue, Mar 28, 2023 at 03:09:03PM +0530, Manivannan Sadhasivam wrote:
>>> On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
>>>> On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
>>>>> Add missing quirks for the USB DWC3 IP.
>>>>
>>>> This is not an acceptable commit message generally and certainly not for
>>>> something that you have tagged for stable.
>>>>
>>>> At a minimum, you need to describe why these are needed and what the
>>>> impact is.
>>>>
>>>
>>> I can certainly improve the commit message. But usually the quirks are copied
>>> from the downstream devicetree where qualcomm engineers would've added them
>>> based on the platform requirements.
>>>
>>>> Also, why are you sending as part of a series purporting to enable
>>>> runtime PM when it appears to be all about optimising specific gadget
>>>> applications?
>>>>
>>>
>>> It's not related to this series I agree but just wanted to group it with a
>>> series touching usb so that it won't get lost.
>>>
>>> I could respin it separately though in v2.
> 
> That's also generally best for USB patches as Greg expects series to be
> merged through a single tree.
> 
>>>> Did you confirm that the below makes any sense or has this just been
>>>> copied verbatim from the vendor devicetree (it looks like that)?
>>>>
>>>
>>> As you've mentioned, most of the quirks are for gadget mode which is not
>>> supported by the upstream supported boards. So I haven't really tested them but
>>> for I assumed that Qcom engineers did.
>>>
>>>> The fact that almost none of the qcom SoCs sets these also indicates
>>>> that something is not right here.
>>>>
>>>>> Cc: stable@vger.kernel.org # 5.20
>>>>> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> index 0d02599d8867..266a94c712aa 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
>>>>>  				iommus = <&apps_smmu 0x820 0x0>;
>>>>>  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
>>>>>  				phy-names = "usb2-phy", "usb3-phy";
>>>>> +				snps,hird-threshold = /bits/ 8 <0x0>;
>>>>> +				snps,usb2-gadget-lpm-disable;
>>>>
>>>> Here you are disabling LPM for gadget mode, which makes most of the
>>>> other properties entirely pointless.
>>
>> Checked with Qcom on these quirks. So this one is just disabling lpm for USB2
>> and rest of the quirks below are for SS/SSP modes.
> 
> No, snps,hird-threshold is for USB2 LPM and so is
> snps,is-utmi-l1-suspend and snps,has-lpm-erratum as you'll see if you
> look at the implementation.
> 
>>>>> +				snps,is-utmi-l1-suspend;
>>>>> +				snps,dis-u1-entry-quirk;
>>>>> +				snps,dis-u2-entry-quirk;
>>>>
>>>> These appear to be used to optimise certain gadget application and
>>>> likely not something that should be set in a dtsi.
>>>>
>>>
>>> I will cross check these with Qcom and respin accordingly.
>>>
>>
>> These quirks are needed as per the DWC IP integration with this SoC it seems.
>> But I got the point that these don't add any values for host only
>> configurations. At the same time, these quirks still hold true for the SoC even
>> if not exercised.
>>
>> So I think we should keep these in the dtsi itself.
> 
> Please take a closer look at the quirks you're enabling first. Commit
> 729dcffd1ed3 ("usb: dwc3: gadget: Add support for disabling U1 and U2
> entries") which added 
> 
>>>>> +				snps,dis-u1-entry-quirk;
>>>>> +				snps,dis-u2-entry-quirk;
> 
> explicitly mentions
> 
> 	Gadget applications may have a requirement to disable the U1 and U2
> 	entry based on the usecase.
> 
> which sounds like something that needs to be done in a per board dts at
> least.
> 
> Perhaps keeping all of these in in the dtsi is correct, but that's going
> to need some more motivation than simply that some vendor does so (as
> they often do all sorts of things they should not).
I'm looking at the DWC3 code and admittedly I don't understand much,
but is there any harm to keeping them? What if somebody decides to
plug in a laptop as a gadget device?

Konrad

> 
> Johan