Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
To:     surong pang <surong.pang@gmail.com>
Cc:     mathias.nyman@intel.com, Greg KH <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Orson.Zhai@unisoc.com, yunguo.wu@unisoc.com
References: <20220412122524.26966-1-surong.pang@gmail.com>
 <610871b2-1707-dfba-868f-4ddecc4d554d@linux.intel.com>
 <CAEDbmAT=fZ-kpn13sW4KjB9RuFb_6T4j_eripR54NZ3UciZfqA@mail.gmail.com>
 <d6df23a0-6539-f955-5241-5cdfcaa4eca4@linux.intel.com>
 <CAEDbmAT3SoSsEmTkELSYoykGN+AuPgi2N11V2YwKuaC3nKMEmQ@mail.gmail.com>
From:   Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: Re: [PATCH V1 1/1] usb/host: Let usb phy shutdown later
Message-ID: <e05ec742-c3dc-df7c-c5d7-29358d0a7081@linux.intel.com>
Date:   Fri, 22 Apr 2022 14:59:01 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <CAEDbmAT3SoSsEmTkELSYoykGN+AuPgi2N11V2YwKuaC3nKMEmQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 22.4.2022 13.43, surong pang wrote:
>>>> @@ -398,6 +397,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>>>>       clk_disable_unprepare(clk);
>>>>       clk_disable_unprepare(reg_clk);
>>>> +    usb_phy_shutdown(hcd->usb_phy);
>>>>       usb_put_hcd(hcd);
> 
> Is it ok to put usb_phy_shutdown before usb_put_hcd(hcd)? hcd is
> released at usb_put_hcd.

yes, above looks good.

> 
> UNISOC DWC3 phy is not divided  USB 2.0/3.0 phy clearly.  Yes, it's
> UNISOC's issue.
> It UNISOC's dtsi: phys = <&ssphy>, <&ssphy>;
> If to shutdown phy too earlier,  it will cost 10s timeout to do xhci_reset.
> usb_remmove_hcd  --> usb_stop_hcd --> xhci_stop --> xhci_reset  -->
> xhci_handshake(&xhci->op_regs->command, CMD_RESET, 0, 10 * 1000 *1000)
> 
> I want to know this change is acceptable or not?
> 
> hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
> Why in xhci_plat_remove, just to shutdown "usb-phy"[0], not to
> shutdown "usb-phy"[1] ?

xhci-plat.c only takes one phy at index 0, so we only shutdowns that one.

Looks like usb core hcd code has better phy handling when adding and
removing hcds. It supports multiple phys.
If possible use that instead.

See drivers/usb/core/hcd.c usb_add_hcd()

Thanks
-Mathias