From: Quentin Schulz <[email protected]>
From: Bin Yang <[email protected]>
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 <[email protected]>
Signed-off-by: Frank Wang <[email protected]>
Signed-off-by: Quentin Schulz <[email protected]>
---
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;
+
/*
* Finish generic HCD initialization and start the HCD. This function
* allocates the DMA buffer pool, registers the USB bus, requests the
--
b4 0.10.1
On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
> From: Quentin Schulz <[email protected]>
>
> From: Bin Yang <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Frank Wang <[email protected]>
> Signed-off-by: Quentin Schulz <[email protected]>
> ---
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Frank Wang <[email protected]>
>> Signed-off-by: Quentin Schulz <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Frank Wang <[email protected]>
>>>> Signed-off-by: Quentin Schulz <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Frank Wang <[email protected]>
> > > Signed-off-by: Quentin Schulz <[email protected]>
> > > ---
> > > 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