| Message ID | 1640153383-21036-1-git-send-email-quic_c_sanm@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v5] usb: host: xhci-plat: Set XHCI_SKIP_PHY_INIT quirk for DWC3 controller | expand |
On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: > Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > Runtime suspend of phy drivers was failing from DWC3 driver as runtime > usage value is 2 because the phy is initialized from DWC3 and HCD core. > DWC3 manages phy in their core drivers. Set this quirk to avoid phy > initialization in HCD core. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > --- > v5: > Added comment to explain the change done. > v4: > Changed pdev->dev.parent->of_node to sysdev->of_node > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index c1edcc9..e6014d4 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct platform_device *pdev) > &xhci->imod_interval); > } > > + /* > + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > + * DWC3 manages phy in their core drivers. Set this quirk to avoid phy > + * initialization in HCD core. > + */ > + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) > + xhci->quirks |= XHCI_SKIP_PHY_INIT; > + Why is this function caring about dwc3 stuff? Shoudn't this be a "generic" device property instead of this device-specific one? thanks, greg k-h
On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: > On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: >> Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >> Runtime suspend of phy drivers was failing from DWC3 driver as runtime >> usage value is 2 because the phy is initialized from DWC3 and HCD core. >> DWC3 manages phy in their core drivers. Set this quirk to avoid phy >> initialization in HCD core. >> >> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >> --- >> v5: >> Added comment to explain the change done. >> v4: >> Changed pdev->dev.parent->of_node to sysdev->of_node >> >> drivers/usb/host/xhci-plat.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >> index c1edcc9..e6014d4 100644 >> --- a/drivers/usb/host/xhci-plat.c >> +++ b/drivers/usb/host/xhci-plat.c >> @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct platform_device *pdev) >> &xhci->imod_interval); >> } >> >> + /* >> + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >> + * DWC3 manages phy in their core drivers. Set this quirk to avoid phy >> + * initialization in HCD core. >> + */ >> + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) >> + xhci->quirks |= XHCI_SKIP_PHY_INIT; >> + > Why is this function caring about dwc3 stuff? Shoudn't this be a > "generic" device property instead of this device-specific one? > > thanks, > > greg k-h This quirk is set only if required for some controllers (eg: dwc3 & cdns3). Please check below commit. https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ Regards Sandeep
Hi Greg, On 1/7/2022 10:27 AM, Sandeep Maheswaram wrote: > > On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: >> On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: >>> Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >>> Runtime suspend of phy drivers was failing from DWC3 driver as runtime >>> usage value is 2 because the phy is initialized from DWC3 and HCD core. >>> DWC3 manages phy in their core drivers. Set this quirk to avoid phy >>> initialization in HCD core. >>> >>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>> --- >>> v5: >>> Added comment to explain the change done. >>> v4: >>> Changed pdev->dev.parent->of_node to sysdev->of_node >>> >>> drivers/usb/host/xhci-plat.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/usb/host/xhci-plat.c >>> b/drivers/usb/host/xhci-plat.c >>> index c1edcc9..e6014d4 100644 >>> --- a/drivers/usb/host/xhci-plat.c >>> +++ b/drivers/usb/host/xhci-plat.c >>> @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct >>> platform_device *pdev) >>> &xhci->imod_interval); >>> } >>> + /* >>> + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >>> + * DWC3 manages phy in their core drivers. Set this quirk to >>> avoid phy >>> + * initialization in HCD core. >>> + */ >>> + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) >>> + xhci->quirks |= XHCI_SKIP_PHY_INIT; >>> + >> Why is this function caring about dwc3 stuff? Shoudn't this be a >> "generic" device property instead of this device-specific one? >> >> thanks, >> >> greg k-h > > This quirk is set only if required for some controllers (eg: dwc3 & > cdns3). > > Please check below commit. > > https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ > > > > Regards > > Sandeep > > Please let me know if anything more info needed for this patch. Regards Sandeep
On Mon, Jan 24, 2022 at 11:01:07AM +0530, Sandeep Maheswaram wrote: > Hi Greg, > > On 1/7/2022 10:27 AM, Sandeep Maheswaram wrote: > > > > On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: > > > On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: > > > > Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > > > > Runtime suspend of phy drivers was failing from DWC3 driver as runtime > > > > usage value is 2 because the phy is initialized from DWC3 and HCD core. > > > > DWC3 manages phy in their core drivers. Set this quirk to avoid phy > > > > initialization in HCD core. > > > > > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > > --- > > > > v5: > > > > Added comment to explain the change done. > > > > v4: > > > > Changed pdev->dev.parent->of_node to sysdev->of_node > > > > > > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/usb/host/xhci-plat.c > > > > b/drivers/usb/host/xhci-plat.c > > > > index c1edcc9..e6014d4 100644 > > > > --- a/drivers/usb/host/xhci-plat.c > > > > +++ b/drivers/usb/host/xhci-plat.c > > > > @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct > > > > platform_device *pdev) > > > > &xhci->imod_interval); > > > > } > > > > + /* > > > > + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > > > > + * DWC3 manages phy in their core drivers. Set this quirk > > > > to avoid phy > > > > + * initialization in HCD core. > > > > + */ > > > > + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) > > > > + xhci->quirks |= XHCI_SKIP_PHY_INIT; > > > > + > > > Why is this function caring about dwc3 stuff? Shoudn't this be a > > > "generic" device property instead of this device-specific one? > > > > > > thanks, > > > > > > greg k-h > > > > This quirk is set only if required for some controllers (eg: dwc3 & > > cdns3). > > > > Please check below commit. > > > > https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ > > > > > > > > Regards > > > > Sandeep > > > > > Please let me know if anything more info needed for this patch. The merge window ended a few hours ago, give me a few days to catch up with my pending email queue that I have to review: $ mdfrm -c ~/mail/todo/ 2254 messages in /home/gregkh/mail/todo/ thanks, greg k-h
On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: > > On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: > > On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: > > > Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > > > Runtime suspend of phy drivers was failing from DWC3 driver as runtime > > > usage value is 2 because the phy is initialized from DWC3 and HCD core. > > > DWC3 manages phy in their core drivers. Set this quirk to avoid phy > > > initialization in HCD core. > > > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > --- > > > v5: > > > Added comment to explain the change done. > > > v4: > > > Changed pdev->dev.parent->of_node to sysdev->of_node > > > > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > > index c1edcc9..e6014d4 100644 > > > --- a/drivers/usb/host/xhci-plat.c > > > +++ b/drivers/usb/host/xhci-plat.c > > > @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct platform_device *pdev) > > > &xhci->imod_interval); > > > } > > > + /* > > > + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > > > + * DWC3 manages phy in their core drivers. Set this quirk to avoid phy > > > + * initialization in HCD core. > > > + */ > > > + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) > > > + xhci->quirks |= XHCI_SKIP_PHY_INIT; > > > + > > Why is this function caring about dwc3 stuff? Shoudn't this be a > > "generic" device property instead of this device-specific one? > > > > thanks, > > > > greg k-h > > This quirk is set only if required for some controllers (eg: dwc3 & cdns3). > > Please check below commit. > > https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ That commit has nothing to do with a specific "dwc3" quirk anywhere. Why not set this flag in the specific platform xhci driver instead where it belongs? thanks, greg k-h
On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote: > On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: >> On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: >>> On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: >>>> Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >>>> Runtime suspend of phy drivers was failing from DWC3 driver as runtime >>>> usage value is 2 because the phy is initialized from DWC3 and HCD core. >>>> DWC3 manages phy in their core drivers. Set this quirk to avoid phy >>>> initialization in HCD core. >>>> >>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>> --- >>>> v5: >>>> Added comment to explain the change done. >>>> v4: >>>> Changed pdev->dev.parent->of_node to sysdev->of_node >>>> >>>> drivers/usb/host/xhci-plat.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >>>> index c1edcc9..e6014d4 100644 >>>> --- a/drivers/usb/host/xhci-plat.c >>>> +++ b/drivers/usb/host/xhci-plat.c >>>> @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct platform_device *pdev) >>>> &xhci->imod_interval); >>>> } >>>> + /* >>>> + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >>>> + * DWC3 manages phy in their core drivers. Set this quirk to avoid phy >>>> + * initialization in HCD core. >>>> + */ >>>> + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) >>>> + xhci->quirks |= XHCI_SKIP_PHY_INIT; >>>> + >>> Why is this function caring about dwc3 stuff? Shoudn't this be a >>> "generic" device property instead of this device-specific one? >>> >>> thanks, >>> >>> greg k-h >> This quirk is set only if required for some controllers (eg: dwc3 & cdns3). >> >> Please check below commit. >> >> https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ > That commit has nothing to do with a specific "dwc3" quirk anywhere. > Why not set this flag in the specific platform xhci driver instead where > it belongs? > > thanks, > > greg k-h There is no specific xhci platform driver for dwc3 controllers. dwc3 controllers use xhci-plat driver . We can add this quirk in usb/dwc3/host.c as cdns3 does but that requires tying dwc3 and xhci driver . https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ Regards Sandeep
Hi Greg, On 1/27/2022 10:28 AM, Sandeep Maheswaram wrote: > > On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote: >> On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: >>> On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: >>>> On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: >>>>> Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >>>>> Runtime suspend of phy drivers was failing from DWC3 driver as >>>>> runtime >>>>> usage value is 2 because the phy is initialized from DWC3 and HCD >>>>> core. >>>>> DWC3 manages phy in their core drivers. Set this quirk to avoid phy >>>>> initialization in HCD core. >>>>> >>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>>> --- >>>>> v5: >>>>> Added comment to explain the change done. >>>>> v4: >>>>> Changed pdev->dev.parent->of_node to sysdev->of_node >>>>> >>>>> drivers/usb/host/xhci-plat.c | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/host/xhci-plat.c >>>>> b/drivers/usb/host/xhci-plat.c >>>>> index c1edcc9..e6014d4 100644 >>>>> --- a/drivers/usb/host/xhci-plat.c >>>>> +++ b/drivers/usb/host/xhci-plat.c >>>>> @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct >>>>> platform_device *pdev) >>>>> &xhci->imod_interval); >>>>> } >>>>> + /* >>>>> + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization >>>>> twice. >>>>> + * DWC3 manages phy in their core drivers. Set this quirk to >>>>> avoid phy >>>>> + * initialization in HCD core. >>>>> + */ >>>>> + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) >>>>> + xhci->quirks |= XHCI_SKIP_PHY_INIT; >>>>> + >>>> Why is this function caring about dwc3 stuff? Shoudn't this be a >>>> "generic" device property instead of this device-specific one? >>>> >>>> thanks, >>>> >>>> greg k-h >>> This quirk is set only if required for some controllers (eg: dwc3 & >>> cdns3). >>> >>> Please check below commit. >>> >>> https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ >>> >> That commit has nothing to do with a specific "dwc3" quirk anywhere. >> Why not set this flag in the specific platform xhci driver instead where >> it belongs? >> >> thanks, >> >> greg k-h > > There is no specific xhci platform driver for dwc3 controllers. > > dwc3 controllers use xhci-plat driver . > > We can add this quirk in usb/dwc3/host.c as cdns3 does but that > requires tying dwc3 and xhci driver . > > https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ > > > Regards > > Sandeep > > Can you suggest any other method to set this quirk for dwc3 controllers. Regards Sandeep
On Tue, Feb 08, 2022 at 03:34:22PM +0530, Sandeep Maheswaram wrote: > Hi Greg, > > On 1/27/2022 10:28 AM, Sandeep Maheswaram wrote: > > > > On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote: > > > On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: > > > > On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: > > > > > On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: > > > > > > Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > > > > > > Runtime suspend of phy drivers was failing from DWC3 > > > > > > driver as runtime > > > > > > usage value is 2 because the phy is initialized from > > > > > > DWC3 and HCD core. > > > > > > DWC3 manages phy in their core drivers. Set this quirk to avoid phy > > > > > > initialization in HCD core. > > > > > > > > > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > > > > --- > > > > > > v5: > > > > > > Added comment to explain the change done. > > > > > > v4: > > > > > > Changed pdev->dev.parent->of_node to sysdev->of_node > > > > > > > > > > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > diff --git a/drivers/usb/host/xhci-plat.c > > > > > > b/drivers/usb/host/xhci-plat.c > > > > > > index c1edcc9..e6014d4 100644 > > > > > > --- a/drivers/usb/host/xhci-plat.c > > > > > > +++ b/drivers/usb/host/xhci-plat.c > > > > > > @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct > > > > > > platform_device *pdev) > > > > > > &xhci->imod_interval); > > > > > > } > > > > > > + /* > > > > > > + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy > > > > > > initialization twice. > > > > > > + * DWC3 manages phy in their core drivers. Set this > > > > > > quirk to avoid phy > > > > > > + * initialization in HCD core. > > > > > > + */ > > > > > > + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) > > > > > > + xhci->quirks |= XHCI_SKIP_PHY_INIT; > > > > > > + > > > > > Why is this function caring about dwc3 stuff? Shoudn't this be a > > > > > "generic" device property instead of this device-specific one? > > > > > > > > > > thanks, > > > > > > > > > > greg k-h > > > > This quirk is set only if required for some controllers (eg: > > > > dwc3 & cdns3). > > > > > > > > Please check below commit. > > > > > > > > https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ > > > > > > > That commit has nothing to do with a specific "dwc3" quirk anywhere. > > > Why not set this flag in the specific platform xhci driver instead where > > > it belongs? > > > > > > thanks, > > > > > > greg k-h > > > > There is no specific xhci platform driver for dwc3 controllers. > > > > dwc3 controllers use xhci-plat driver . > > > > We can add this quirk in usb/dwc3/host.c as cdns3 does but that requires > > tying dwc3 and xhci driver . > > > > https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ > > > > > > Regards > > > > Sandeep > > > > > Can you suggest any other method to set this quirk for dwc3 controllers. No idea, sorry.
On Tue, Feb 08, 2022 at 11:11:53AM +0100, Greg Kroah-Hartman wrote: > On Tue, Feb 08, 2022 at 03:34:22PM +0530, Sandeep Maheswaram wrote: > > Hi Greg, > > > > On 1/27/2022 10:28 AM, Sandeep Maheswaram wrote: > > > > > > On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote: > > > > On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: > > > > > On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: > > > > > > On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: > > > > > > > Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > > > > > > > Runtime suspend of phy drivers was failing from DWC3 > > > > > > > driver as runtime > > > > > > > usage value is 2 because the phy is initialized from > > > > > > > DWC3 and HCD core. > > > > > > > DWC3 manages phy in their core drivers. Set this quirk to avoid phy > > > > > > > initialization in HCD core. > > > > > > > > > > > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > > > > > > --- > > > > > > > v5: > > > > > > > Added comment to explain the change done. > > > > > > > v4: > > > > > > > Changed pdev->dev.parent->of_node to sysdev->of_node > > > > > > > > > > > > > > drivers/usb/host/xhci-plat.c | 8 ++++++++ > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/usb/host/xhci-plat.c > > > > > > > b/drivers/usb/host/xhci-plat.c > > > > > > > index c1edcc9..e6014d4 100644 > > > > > > > --- a/drivers/usb/host/xhci-plat.c > > > > > > > +++ b/drivers/usb/host/xhci-plat.c > > > > > > > @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct > > > > > > > platform_device *pdev) > > > > > > > &xhci->imod_interval); > > > > > > > } > > > > > > > + /* > > > > > > > + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy > > > > > > > initialization twice. > > > > > > > + * DWC3 manages phy in their core drivers. Set this > > > > > > > quirk to avoid phy > > > > > > > + * initialization in HCD core. > > > > > > > + */ > > > > > > > + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) > > > > > > > + xhci->quirks |= XHCI_SKIP_PHY_INIT; > > > > > > > + > > > > > > Why is this function caring about dwc3 stuff? Shoudn't this be a > > > > > > "generic" device property instead of this device-specific one? > > > > > > > > > > > > thanks, > > > > > > > > > > > > greg k-h > > > > > This quirk is set only if required for some controllers (eg: > > > > > dwc3 & cdns3). > > > > > > > > > > Please check below commit. > > > > > > > > > > https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ > > > > > > > > > That commit has nothing to do with a specific "dwc3" quirk anywhere. > > > > Why not set this flag in the specific platform xhci driver instead where > > > > it belongs? > > > > > > > > thanks, > > > > > > > > greg k-h > > > > > > There is no specific xhci platform driver for dwc3 controllers. > > > > > > dwc3 controllers use xhci-plat driver . > > > > > > We can add this quirk in usb/dwc3/host.c as cdns3 does but that requires > > > tying dwc3 and xhci driver . > > > > > > https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ > > > > > > > > > Regards > > > > > > Sandeep > > > > > > > > Can you suggest any other method to set this quirk for dwc3 controllers. > > No idea, sorry. Sandeep, I agree with Greg's comments here. The compatible based check to detect dwc3 controller is not elegant. Your proposal of adding a device tree param is overkill, I believe. Greg already gave us a pointer here [1] which I feel is the best approach going forward. We know that xhci-plat is being used by drivers like dwc3, cdns3 and these drivers need to expose their xhci quirks. As Greg suggested, why can't we move xhci quirks definition to include/linux/usb/xhci-quriks.h and directly access from the glue drivers? The attached is the patch (completely untested) for your reference. It will prepare the setup for you to add the private data and quirks in the dwc3 host glue driver. Thanks, Pavan [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/
On 2/9/2022 11:23 AM, Pavan Kondeti wrote: > On Tue, Feb 08, 2022 at 11:11:53AM +0100, Greg Kroah-Hartman wrote: >> On Tue, Feb 08, 2022 at 03:34:22PM +0530, Sandeep Maheswaram wrote: >>> Hi Greg, >>> >>> On 1/27/2022 10:28 AM, Sandeep Maheswaram wrote: >>>> On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote: >>>>> On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: >>>>>> On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: >>>>>>> On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram wrote: >>>>>>>> Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. >>>>>>>> Runtime suspend of phy drivers was failing from DWC3 >>>>>>>> driver as runtime >>>>>>>> usage value is 2 because the phy is initialized from >>>>>>>> DWC3 and HCD core. >>>>>>>> DWC3 manages phy in their core drivers. Set this quirk to avoid phy >>>>>>>> initialization in HCD core. >>>>>>>> >>>>>>>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> >>>>>>>> --- >>>>>>>> v5: >>>>>>>> Added comment to explain the change done. >>>>>>>> v4: >>>>>>>> Changed pdev->dev.parent->of_node to sysdev->of_node >>>>>>>> >>>>>>>> drivers/usb/host/xhci-plat.c | 8 ++++++++ >>>>>>>> 1 file changed, 8 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/usb/host/xhci-plat.c >>>>>>>> b/drivers/usb/host/xhci-plat.c >>>>>>>> index c1edcc9..e6014d4 100644 >>>>>>>> --- a/drivers/usb/host/xhci-plat.c >>>>>>>> +++ b/drivers/usb/host/xhci-plat.c >>>>>>>> @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct >>>>>>>> platform_device *pdev) >>>>>>>> &xhci->imod_interval); >>>>>>>> } >>>>>>>> + /* >>>>>>>> + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy >>>>>>>> initialization twice. >>>>>>>> + * DWC3 manages phy in their core drivers. Set this >>>>>>>> quirk to avoid phy >>>>>>>> + * initialization in HCD core. >>>>>>>> + */ >>>>>>>> + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) >>>>>>>> + xhci->quirks |= XHCI_SKIP_PHY_INIT; >>>>>>>> + >>>>>>> Why is this function caring about dwc3 stuff? Shoudn't this be a >>>>>>> "generic" device property instead of this device-specific one? >>>>>>> >>>>>>> thanks, >>>>>>> >>>>>>> greg k-h >>>>>> This quirk is set only if required for some controllers (eg: >>>>>> dwc3 & cdns3). >>>>>> >>>>>> Please check below commit. >>>>>> >>>>>> https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ >>>>>> >>>>> That commit has nothing to do with a specific "dwc3" quirk anywhere. >>>>> Why not set this flag in the specific platform xhci driver instead where >>>>> it belongs? >>>>> >>>>> thanks, >>>>> >>>>> greg k-h >>>> There is no specific xhci platform driver for dwc3 controllers. >>>> >>>> dwc3 controllers use xhci-plat driver . >>>> >>>> We can add this quirk in usb/dwc3/host.c as cdns3 does but that requires >>>> tying dwc3 and xhci driver . >>>> >>>> https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ >>>> >>>> >>>> Regards >>>> >>>> Sandeep >>>> >>>> >>> Can you suggest any other method to set this quirk for dwc3 controllers. >> No idea, sorry. > Sandeep, > > I agree with Greg's comments here. The compatible based check to detect dwc3 > controller is not elegant. Your proposal of adding a device tree param is > overkill, I believe. > > Greg already gave us a pointer here [1] which I feel is the best approach going > forward. We know that xhci-plat is being used by drivers like dwc3, cdns3 and > these drivers need to expose their xhci quirks. As Greg suggested, why can't > we move xhci quirks definition to include/linux/usb/xhci-quriks.h and directly > access from the glue drivers? The attached is the patch (completely untested) > for your reference. It will prepare the setup for you to add the private data > and quirks in the dwc3 host glue driver. > > Thanks, > Pavan > > [1] > https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ > Thanks Pavan..will test the patch.
Hi Sandeep, On Thu, Feb 10, 2022 at 11:18:19AM +0530, Sandeep Maheswaram wrote: > Hi Pavan, > > On 2/9/2022 11:31 AM, Sandeep Maheswaram wrote: > > > >On 2/9/2022 11:23 AM, Pavan Kondeti wrote: > >>On Tue, Feb 08, 2022 at 11:11:53AM +0100, Greg Kroah-Hartman wrote: > >>>On Tue, Feb 08, 2022 at 03:34:22PM +0530, Sandeep Maheswaram wrote: > >>>>Hi Greg, > >>>> > >>>>On 1/27/2022 10:28 AM, Sandeep Maheswaram wrote: > >>>>>On 1/26/2022 5:55 PM, Greg Kroah-Hartman wrote: > >>>>>>On Fri, Jan 07, 2022 at 10:27:59AM +0530, Sandeep Maheswaram wrote: > >>>>>>>On 1/6/2022 7:55 PM, Greg Kroah-Hartman wrote: > >>>>>>>>On Wed, Dec 22, 2021 at 11:39:43AM +0530, Sandeep Maheswaram > >>>>>>>>wrote: > >>>>>>>>>Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. > >>>>>>>>>Runtime suspend of phy drivers was failing from DWC3 > >>>>>>>>>driver as runtime > >>>>>>>>>usage value is 2 because the phy is initialized from > >>>>>>>>>DWC3 and HCD core. > >>>>>>>>>DWC3 manages phy in their core drivers. Set this quirk to > >>>>>>>>>avoid phy > >>>>>>>>>initialization in HCD core. > >>>>>>>>> > >>>>>>>>>Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > >>>>>>>>>--- > >>>>>>>>>v5: > >>>>>>>>>Added comment to explain the change done. > >>>>>>>>>v4: > >>>>>>>>>Changed pdev->dev.parent->of_node to sysdev->of_node > >>>>>>>>> > >>>>>>>>> drivers/usb/host/xhci-plat.c | 8 ++++++++ > >>>>>>>>> 1 file changed, 8 insertions(+) > >>>>>>>>> > >>>>>>>>>diff --git a/drivers/usb/host/xhci-plat.c > >>>>>>>>>b/drivers/usb/host/xhci-plat.c > >>>>>>>>>index c1edcc9..e6014d4 100644 > >>>>>>>>>--- a/drivers/usb/host/xhci-plat.c > >>>>>>>>>+++ b/drivers/usb/host/xhci-plat.c > >>>>>>>>>@@ -327,6 +327,14 @@ static int xhci_plat_probe(struct > >>>>>>>>>platform_device *pdev) > >>>>>>>>>&xhci->imod_interval); > >>>>>>>>> } > >>>>>>>>>+ /* > >>>>>>>>>+ * Set XHCI_SKIP_PHY_INIT quirk to avoid phy > >>>>>>>>>initialization twice. > >>>>>>>>>+ * DWC3 manages phy in their core drivers. Set this > >>>>>>>>>quirk to avoid phy > >>>>>>>>>+ * initialization in HCD core. > >>>>>>>>>+ */ > >>>>>>>>>+ if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) > >>>>>>>>>+ xhci->quirks |= XHCI_SKIP_PHY_INIT; > >>>>>>>>>+ > >>>>>>>>Why is this function caring about dwc3 stuff? Shoudn't this be a > >>>>>>>>"generic" device property instead of this device-specific one? > >>>>>>>> > >>>>>>>>thanks, > >>>>>>>> > >>>>>>>>greg k-h > >>>>>>>This quirk is set only if required for some controllers (eg: > >>>>>>>dwc3 & cdns3). > >>>>>>> > >>>>>>>Please check below commit. > >>>>>>> > >>>>>>>https://lore.kernel.org/all/20200918131752.16488-5-mathias.nyman@linux.intel.com/ > >>>>>>> > >>>>>>> > >>>>>>That commit has nothing to do with a specific "dwc3" quirk anywhere. > >>>>>>Why not set this flag in the specific platform xhci driver > >>>>>>instead where > >>>>>>it belongs? > >>>>>> > >>>>>>thanks, > >>>>>> > >>>>>>greg k-h > >>>>>There is no specific xhci platform driver for dwc3 controllers. > >>>>> > >>>>>dwc3 controllers use xhci-plat driver . > >>>>> > >>>>>We can add this quirk in usb/dwc3/host.c as cdns3 does but that > >>>>>requires > >>>>>tying dwc3 and xhci driver . > >>>>> > >>>>>https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ > >>>>> > >>>>> > >>>>> > >>>>>Regards > >>>>> > >>>>>Sandeep > >>>>> > >>>>> > >>>>Can you suggest any other method to set this quirk for dwc3 > >>>>controllers. > >>>No idea, sorry. > >>Sandeep, > >> > >>I agree with Greg's comments here. The compatible based check to detect > >>dwc3 > >>controller is not elegant. Your proposal of adding a device tree param > >>is > >>overkill, I believe. > >> > >>Greg already gave us a pointer here [1] which I feel is the best > >>approach going > >>forward. We know that xhci-plat is being used by drivers like dwc3, > >>cdns3 and > >>these drivers need to expose their xhci quirks. As Greg suggested, why > >>can't > >>we move xhci quirks definition to include/linux/usb/xhci-quriks.h and > >>directly > >>access from the glue drivers? The attached is the patch (completely > >>untested) > >>for your reference. It will prepare the setup for you to add the private > >>data > >>and quirks in the dwc3 host glue driver. > >> > >>Thanks, > >>Pavan > >> > >>[1] > >>https://patchwork.kernel.org/project/linux-arm-msm/patch/1633946518-13906-1-git-send-email-sanm@codeaurora.org/ > >> > >> > >Thanks Pavan..will test the patch. > > Tested your patch. It is working fine along with the attached changes. > Your patch looks good to me. Feel free to send the two patches together. Thanks, Pavan
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index c1edcc9..e6014d4 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -327,6 +327,14 @@ static int xhci_plat_probe(struct platform_device *pdev) &xhci->imod_interval); } + /* + * Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. + * DWC3 manages phy in their core drivers. Set this quirk to avoid phy + * initialization in HCD core. + */ + if (of_device_is_compatible(sysdev->of_node, "snps,dwc3")) + xhci->quirks |= XHCI_SKIP_PHY_INIT; + hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0); if (IS_ERR(hcd->usb_phy)) { ret = PTR_ERR(hcd->usb_phy);
Set XHCI_SKIP_PHY_INIT quirk to avoid phy initialization twice. Runtime suspend of phy drivers was failing from DWC3 driver as runtime usage value is 2 because the phy is initialized from DWC3 and HCD core. DWC3 manages phy in their core drivers. Set this quirk to avoid phy initialization in HCD core. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- v5: Added comment to explain the change done. v4: Changed pdev->dev.parent->of_node to sysdev->of_node drivers/usb/host/xhci-plat.c | 8 ++++++++ 1 file changed, 8 insertions(+)