On Wed, Mar 29, 2023 at 10:34:56AM +0200, 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.
>
Ok, good to know.
> > > > 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.
>
> 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.
>
Correct me if I'm wrong. When I look into the code, "snps,is-utmi-l1-suspend"
and "snps,hird-threshold" are used independently of the LPM mode atleast in one
place:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc3/gadget.c#n2867
But I could be completely wrong here as my understanding of the usb stack is not
that great.
> > > > > + 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.
>
Going by this commit message it sounds like it. But...
> 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).
>
If you read my last reply one more time, I didn't reason it based on the vendor
code.
But I hear a contradict reply from Qcom saying that these properties are
required as a part of the DWC3 IP integration with the SoC. I need to recheck
with them again tomorrow.
Also, if these properties are application specific then they shouldn't be in
devicetree atleast :/
- Mani
- Mani
> Johan
--
மணிவண்ணன் சதாசிவம்
