| Message ID | 20230325165217.31069-2-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: qcom: Allow runtime PM | expand |
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. 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? Did you confirm that the below makes any sense or has this just been copied verbatim from the vendor devicetree (it looks like that)? 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. > + 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. > + snps,has-lpm-erratum; > + tx-fifo-resize; Same here. > port { > usb_0_role_switch: endpoint { Johan
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@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. > > > + 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. - Mani > > + snps,has-lpm-erratum; > > + tx-fifo-resize; > > Same here. > > > port { > > usb_0_role_switch: endpoint { > > Johan
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@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. > > > + 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 > > -- > மணிவண்ணன் சதாசிவம்
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). Johan
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
On Wed, Mar 29, 2023 at 01:24:27PM +0200, Konrad Dybcio wrote: > On 29.03.2023 10:34, Johan Hovold wrote: > > 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? We should the add the bits that are really needed with a proper descriptions of what they do (like all commit messages should). Besides the commit message, the problem here is that these have just been copied from some vendor kernel and some properties are conflicting (e.g. both disabling LPM and configuring LPM settings) while others appear to be application specific. Johan
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@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. > 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
On Wed, Mar 29, 2023 at 06:53:43PM +0530, Manivannan Sadhasivam wrote: > 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: > > > > > > 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. Yeah, it's not that obvious from just looking at the code, but L1 (and BESL) are USB2 LPM concepts and if you disable LPM then there is no need to override these values in the BOS descriptor either (as is done in dwc3_gadget_config_params() and bos_desc()). > > > > > > + 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. I was referring to the fact that these properties had been copied from the vendor dtsi and seemingly so without further review or justification in the commit message (e.g. to explain the inconsistencies). > 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 :/ I fully agree with that. Johan
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; + snps,is-utmi-l1-suspend; + snps,dis-u1-entry-quirk; + snps,dis-u2-entry-quirk; + snps,has-lpm-erratum; + tx-fifo-resize; port { usb_0_role_switch: endpoint { @@ -3100,6 +3107,13 @@ usb_1_dwc3: usb@a800000 { iommus = <&apps_smmu 0x860 0x0>; phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>; phy-names = "usb2-phy", "usb3-phy"; + snps,hird-threshold = /bits/ 8 <0x0>; + snps,usb2-gadget-lpm-disable; + snps,is-utmi-l1-suspend; + snps,dis-u1-entry-quirk; + snps,dis-u2-entry-quirk; + snps,has-lpm-erratum; + tx-fifo-resize; port { usb_1_role_switch: endpoint {
Add missing quirks for the USB DWC3 IP. 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(+)