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

[Manivannan Sadhasivam]

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.
> 
> > 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@xxxxxxxxxxxxxxx # 5.20
> > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > ---
> > >  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.

> > > +				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.

- Mani

> - Mani
> 
> > > +				snps,has-lpm-erratum;
> > > +				tx-fifo-resize;
> > 
> > Same here.
> > 
> > >  				port {
> > >  					usb_0_role_switch: endpoint {
> > 
> > Johan
> 
> -- 
> மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்