Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i51094778:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1115-g8b801eadce-fm-20221102.001-g8b801ead
Mime-Version: 1.0
Message-Id: <f3378173-2e15-467c-b159-7d523533ffc5@app.fastmail.com>
In-Reply-To: <1800f8ea-2dde-a420-3e99-56237bde1bb0@gmail.com>
References: <20221112102506.34990-1-sven@svenpeter.dev>
 <1800f8ea-2dde-a420-3e99-56237bde1bb0@gmail.com>
Date:   Sat, 12 Nov 2022 23:46:19 +0100
From:   "Sven Peter" <sven@svenpeter.dev>
To:     "Ferry Toth" <fntoth@gmail.com>
Cc:     "Felipe Balbi" <felipe.balbi@linux.intel.com>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        "Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
        "Thinh Nguyen" <Thinh.Nguyen@synopsys.com>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] usb: dwc3: core: configure PHY before initializing host in
 dwc3_set_mode
Content-Type: text/plain
Precedence: bulk

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.


Best,


Sven