| Message ID | 20221206-dwc2-gadget-dual-role-v1-3-36515e1092cd@theobroma-systems.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc2: fix USB peripheral role in dual-role mode on PX30 | expand |
On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > From: Bin Yang <yangbin@rock-chips.com> Can't have multiple "From:" lines, odd. I'll try to fix this up on my end...
On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote: > From: Bin Yang <yangbin@rock-chips.com> > > The usb phys need to be controlled dynamically on some Rockchip SoCs. > So set the new HCD flag which prevents USB core from trying to manage > our phys. > > Signed-off-by: Bin Yang <yangbin@rock-chips.com> > Signed-off-by: Frank Wang <frank.wang@rock-chips.com> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > --- > drivers/usb/dwc2/hcd.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 657f1f659ffaf..757a66fa32fa8 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) > if (!IS_ERR_OR_NULL(hsotg->uphy)) > otg_set_host(hsotg->uphy->otg, &hcd->self); > > + /* > + * do not manage the PHY state in the HCD core, instead let the driver > + * handle this (for example if the PHY can only be turned on after a > + * specific event) > + */ > + hcd->skip_phy_initialization = 1; Wait, doesn't this mess with the phy logic for all other chips that use this IP block? Have you tested this on other systems? I'd like some verification first before taking this change as it seems very specific-platform. thanks, greg k-h
Hi Greg, On 12/8/22 16:53, Greg Kroah-Hartman wrote: > On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote: >> From: Bin Yang <yangbin@rock-chips.com> >> >> The usb phys need to be controlled dynamically on some Rockchip SoCs. >> So set the new HCD flag which prevents USB core from trying to manage >> our phys. >> >> Signed-off-by: Bin Yang <yangbin@rock-chips.com> >> Signed-off-by: Frank Wang <frank.wang@rock-chips.com> >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> --- >> drivers/usb/dwc2/hcd.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index 657f1f659ffaf..757a66fa32fa8 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) >> if (!IS_ERR_OR_NULL(hsotg->uphy)) >> otg_set_host(hsotg->uphy->otg, &hcd->self); >> >> + /* >> + * do not manage the PHY state in the HCD core, instead let the driver >> + * handle this (for example if the PHY can only be turned on after a >> + * specific event) >> + */ >> + hcd->skip_phy_initialization = 1; > > Wait, doesn't this mess with the phy logic for all other chips that use > this IP block? Have you tested this on other systems? > I have not. I asked this in the cover-letter but I guess I should have made the patch series an RFC for this reason? > I'd like some verification first before taking this change as it seems > very specific-platform. > There's already some platform-specific callbacks for the driver (see dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too early, before hcd structure is actually allocated. So we either need to use some "proxy"/shadow variable in dwc2_core_params and then update it right after hcd gets allocated or have another platform-specific callback only for hcd (post-)initialization. Nothing too fancy so shouldn't take too long to implement. Any preference? Something else? Also on a side note, after further testing, USB peripheral mode in dual-role mode does not seem to be entirely fixed with this patch series, I still have occasional locks. But considering that it absolutely didn't work before, it is some kind of progress. There are also some issues related to USB host mode in dual-role mode but I saw those happening before the patch series too. I'll see what I can do, really frustrating to work with IPs for which there's no documentation :/ Cheers, Quentin
On Fri, Dec 09, 2022 at 12:15:34PM +0100, Quentin Schulz wrote: > Hi Greg, > > On 12/8/22 16:53, Greg Kroah-Hartman wrote: > > On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote: > > > From: Bin Yang <yangbin@rock-chips.com> > > > > > > The usb phys need to be controlled dynamically on some Rockchip SoCs. > > > So set the new HCD flag which prevents USB core from trying to manage > > > our phys. > > > > > > Signed-off-by: Bin Yang <yangbin@rock-chips.com> > > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com> > > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > > --- > > > drivers/usb/dwc2/hcd.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > > > index 657f1f659ffaf..757a66fa32fa8 100644 > > > --- a/drivers/usb/dwc2/hcd.c > > > +++ b/drivers/usb/dwc2/hcd.c > > > @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) > > > if (!IS_ERR_OR_NULL(hsotg->uphy)) > > > otg_set_host(hsotg->uphy->otg, &hcd->self); > > > + /* > > > + * do not manage the PHY state in the HCD core, instead let the driver > > > + * handle this (for example if the PHY can only be turned on after a > > > + * specific event) > > > + */ > > > + hcd->skip_phy_initialization = 1; > > > > Wait, doesn't this mess with the phy logic for all other chips that use > > this IP block? Have you tested this on other systems? > > > > I have not. I asked this in the cover-letter but I guess I should have made > the patch series an RFC for this reason? Ah, should I drop the first 2 in this series that I already applied? > > I'd like some verification first before taking this change as it seems > > very specific-platform. > > > > There's already some platform-specific callbacks for the driver (see > dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too > early, before hcd structure is actually allocated. So we either need to use > some "proxy"/shadow variable in dwc2_core_params and then update it right > after hcd gets allocated or have another platform-specific callback only for > hcd (post-)initialization. > > Nothing too fancy so shouldn't take too long to implement. Any preference? > Something else? Which ever you think would be simplest. thanks, greg k-h
Hi Greg, On 12/9/22 13:13, Greg Kroah-Hartman wrote: > On Fri, Dec 09, 2022 at 12:15:34PM +0100, Quentin Schulz wrote: >> Hi Greg, >> >> On 12/8/22 16:53, Greg Kroah-Hartman wrote: >>> On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote: >>>> From: Bin Yang <yangbin@rock-chips.com> >>>> >>>> The usb phys need to be controlled dynamically on some Rockchip SoCs. >>>> So set the new HCD flag which prevents USB core from trying to manage >>>> our phys. >>>> >>>> Signed-off-by: Bin Yang <yangbin@rock-chips.com> >>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com> >>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>> --- >>>> drivers/usb/dwc2/hcd.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >>>> index 657f1f659ffaf..757a66fa32fa8 100644 >>>> --- a/drivers/usb/dwc2/hcd.c >>>> +++ b/drivers/usb/dwc2/hcd.c >>>> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) >>>> if (!IS_ERR_OR_NULL(hsotg->uphy)) >>>> otg_set_host(hsotg->uphy->otg, &hcd->self); >>>> + /* >>>> + * do not manage the PHY state in the HCD core, instead let the driver >>>> + * handle this (for example if the PHY can only be turned on after a >>>> + * specific event) >>>> + */ >>>> + hcd->skip_phy_initialization = 1; >>> >>> Wait, doesn't this mess with the phy logic for all other chips that use >>> this IP block? Have you tested this on other systems? >>> >> >> I have not. I asked this in the cover-letter but I guess I should have made >> the patch series an RFC for this reason? > > Ah, should I drop the first 2 in this series that I already applied? > Up to you. I need the three patches to have it (somewhat) working, so only merging the first two isn't going to improve the current situation much for me. I don't mind carrying them over for a v2 (or how many versions are needed). >>> I'd like some verification first before taking this change as it seems >>> very specific-platform. >>> >> >> There's already some platform-specific callbacks for the driver (see >> dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too >> early, before hcd structure is actually allocated. So we either need to use >> some "proxy"/shadow variable in dwc2_core_params and then update it right >> after hcd gets allocated or have another platform-specific callback only for >> hcd (post-)initialization. >> >> Nothing too fancy so shouldn't take too long to implement. Any preference? >> Something else? > > Which ever you think would be simplest. > Got it. Cheers, Quentin
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 657f1f659ffaf..757a66fa32fa8 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) if (!IS_ERR_OR_NULL(hsotg->uphy)) otg_set_host(hsotg->uphy->otg, &hcd->self); + /* + * do not manage the PHY state in the HCD core, instead let the driver + * handle this (for example if the PHY can only be turned on after a + * specific event) + */ + hcd->skip_phy_initialization = 1; + /* * Finish generic HCD initialization and start the HCD. This function * allocates the DMA buffer pool, registers the USB bus, requests the