Re: [PATCH] usb: dwc3: core: configure PHY before initializing host in dwc3_set_mode

["Sven Peter" <sven@svenpeter.dev>]

On Sun, Nov 13, 2022, at 00:07, Ferry Toth wrote:
> Hi,
>
> Op 12-11-2022 om 23:46 schreef Sven Peter:
>> Hi Ferry,
>> 
>> On Sat, Nov 12, 2022, at 23:15, Ferry Toth wrote:
>>> Hi Sven,
>>>
>>> Op 12-11-2022 om 11:25 schreef Sven Peter:
>>>> Usually, first the PHY is set to the correct mode and then the host or
>>>> device side of the controller is initialized afterwards. Otherwise a PHY
>>>> that's already used has to be reconfigured.
>>>> dwc3_core_init_mode() does this correctly for both host and device and
>>>> __dwc3_set_mode() does it correctly when switching to device mode.
>>>> When setting up host mode however it first initializes xhci and only
>>>> then changes the PHY's mode. Let's also do the operations in the correct
>>>> order here.
>>>>
>>>> Fixes: 958d1a4c40dd ("usb: dwc3: core: program PHY for proper DRD modes")
>>>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 9 +++++----
>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index ad4d644e21a4..759d23d908fa 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -212,14 +212,15 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>    
>>>>    	switch (dwc->desired_dr_role) {
>>>>    	case DWC3_GCTL_PRTCAP_HOST:
>>>> +		if (dwc->usb2_phy)
>>>> +			otg_set_vbus(dwc->usb2_phy->otg, true);
>>>> +		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>>> +		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>>> +
>>>>    		ret = dwc3_host_init(dwc);
>>>>    		if (ret) {
>>>>    			dev_err(dwc->dev, "failed to initialize host\n");
>>>>    		} else {
>>>> -			if (dwc->usb2_phy)
>>>> -				otg_set_vbus(dwc->usb2_phy->otg, true);
>>>> -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>>> -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>>>    			if (dwc->dis_split_quirk) {
>>>>    				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
>>>>    				reg |= DWC3_GUCTL3_SPLITDISABLE;
>>> This patch breaks usb host mode on Intel Merrifield platform. I am
>>> testing this on top of v6.0 +
>>> * my 2 "usb: dwc3: core: defer probe on ulpi_read_id timeout" patches
>>> (otherwise tusb1210 will not be probed on this platform)
>>> * Revert "usb: dwc3: disable USB core PHY management" (with/without this
>>> patch makes no difference)
>>> * usb: dwc3: Do not get extcon device when usb-role-switch is used
>>> (with/without this patch makes no difference)
>>>
>>> ftrace shows tusb1210 is indeed still probed, nevertheless in host mode
>>> it seems vbus is not powered as my connected smsc95xx based hub is not
>>> active (seems not plugged).
>>>
>>> Flipping the switch to device mode gadgets work fine.
>>>
>>> Could it be dwc3_host_init() needs to be called prior to otg_set_vbus()?
>> 
>> Huh, thanks for testing!
>> 
>> For the platform I'm working on I need to set the phy mode before dwc3_host_init()
>> and then found the same code further below in dwc3_core_init_mode,
>> 
>> static int dwc3_core_init_mode(struct dwc3 *dwc)
>> {
>> [...]
>> 	case USB_DR_MODE_HOST:
>> 		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
>> 
>> 		if (dwc->usb2_phy)
>> 			otg_set_vbus(dwc->usb2_phy->otg, true);
>> 		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>> 		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>> 
>> 		ret = dwc3_host_init(dwc);
>> 		if (ret)
>> 			return dev_err_probe(dev, ret, "failed to initialize host\n");
>> 		break;
>> [...]
>> }
>> 
>> so I'm quite surprised it causes issue in __dwc3_set_mode now.
>> If otg_set_vbus indeed needs to happen after dwc3_host_init it should probably
>> also be called afterwards in dwc3_core_init_mode as well.
>
> Maybe there is a difference between the case USB_DR_MODE_HOST above and 
> case USB_DR_MODE_OTG touched by this patch?

Would be surprising but who knows. I'll see if I can move more of the PHY
initialization on my platform to the power_on callback so that it'll also
work there with the current code.
I just thought this was going to fix a bug but if it breaks things it's better
to just drop this patch.


Thanks,


Sven