[3/3] usb: dwc2: prevent core phy initialisation

  From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

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(+)
  

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