From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933640AbeAJN45 (ORCPT + 1 other); Wed, 10 Jan 2018 08:56:57 -0500 Received: from lelnx194.ext.ti.com ([198.47.27.80]:16002 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932275AbeAJN4z (ORCPT ); Wed, 10 Jan 2018 08:56:55 -0500 Subject: Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume To: Felipe Balbi CC: , , , , "linux-stable # = v4 . 13" References: <1515589914-23460-1-git-send-email-rogerq@ti.com> <0c2c7e45-9324-316a-d44b-dd17a3a2c68b@ti.com> <87373dzvmi.fsf@linux.intel.com> From: Roger Quadros Message-ID: <327eac7d-80e2-bbd6-4fb4-98d947335698@ti.com> Date: Wed, 10 Jan 2018 15:56:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <87373dzvmi.fsf@linux.intel.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-GB Content-Transfer-Encoding: 8bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 10/01/18 15:33, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: >> Felipe, >> >> On 10/01/18 15:11, Roger Quadros wrote: >>> The USB PHYs should be requested only once during the life cycle of >>> this driver. >>> >>> As dwc3_core_init() is called during system suspend/resume >>> it will result in multiple calls to dwc3_core_get_phy() which is wrong. >>> >>> To prevent that let's move dwc3_core_get_phy() call >>> outside dwc3_core_init(). >>> >>> Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys") >>> Cc: linux-stable # >= v4.13 >>> Signed-off-by: Roger Quadros >> >> FYI. this patch brings the code back to >> revert 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys") >> revert f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get the PHY") >> >> So looks like this will break ULPI PHY case? >> >> Where do we initialize ULPI PHY, in dwc3_phy_setup()? >> >> if so then 541768b08a40 breaks the ULPI PHY case as well, right? > > indeed, that commit regressed ULPI PHYs :-( > > Seems like it should be more like below: > > @@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc) > dwc->maximum_speed = USB_SPEED_HIGH; > } > > - ret = dwc3_core_get_phy(dwc); > + ret = dwc3_phy_setup(dwc); But can we do a dwc3_phy_setup() without doing the soft reset of the controller first? > if (ret) > goto err0; > > - ret = dwc3_core_soft_reset(dwc); > + ret = dwc3_core_get_phy(dwc); we can get_phy in dwc3_core_init() as it will get called on resume(). This was the $subject of this patch. > if (ret) > goto err0; > > - ret = dwc3_phy_setup(dwc); > + ret = dwc3_core_soft_reset(dwc); > if (ret) > goto err0; > > And maybe we rename dwc3_phy_setup() to dwc3_phy_intf_config() just to > make the name match what the function actually does. Can you check that > it won't regress the case reported by Carlos? If that works, then we > would have to move BOTH dwc3_phy_setup() (dwc3_phy_intf_config()) and > dwc3_core_get_phy() outside of dwc3_core_init(), which would mean > duplicated code in suspend/resume handlers. > > I'm sure we can sort that out in another way; but the proper order is: > > -> initialize ULPI (if necessary) > -> get phy > -> soft reset > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki