[PATCH V1 1/1] usb/host: Let usb phy shutdown later

[PATCH V1 1/1] usb/host: Let usb phy shutdown later

[Surong Pang]

From: Surong Pang <surong.pang@unisoc.com>

Let usb phy shutdown later in xhci_plat_remove function.
Some phy driver doesn't divide 3.0/2.0 very clear.
If calls usb_phy_shutdown earlier than usb_remove_hcd(hcd),
It will case 10s cmd timeout issue.

Call usb phy shutdown later has better compatibility.

Signed-off-by: Surong Pang <surong.pang@unisoc.com>
---
 drivers/usb/host/xhci-plat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 649ffd861b44..dc73a81cbe9b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -390,7 +390,6 @@ static int xhci_plat_remove(struct platform_device *dev)
 
 	usb_remove_hcd(shared_hcd);
 	xhci->shared_hcd = NULL;
-	usb_phy_shutdown(hcd->usb_phy);
 
 	usb_remove_hcd(hcd);
 	usb_put_hcd(shared_hcd);
@@ -398,6 +397,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	clk_disable_unprepare(clk);
 	clk_disable_unprepare(reg_clk);
 	usb_put_hcd(hcd);
+	usb_phy_shutdown(hcd->usb_phy);
 
 	pm_runtime_disable(&dev->dev);
 	pm_runtime_put_noidle(&dev->dev);
-- 
2.17.1

Re: [PATCH V1 1/1] usb/host: Let usb phy shutdown later

[Mathias Nyman]

On 12.4.2022 15.25, Surong Pang wrote:
> From: Surong Pang <surong.pang@unisoc.com>
> 
> Let usb phy shutdown later in xhci_plat_remove function.
> Some phy driver doesn't divide 3.0/2.0 very clear.
> If calls usb_phy_shutdown earlier than usb_remove_hcd(hcd),
> It will case 10s cmd timeout issue.
> 
> Call usb phy shutdown later has better compatibility.
> 
> Signed-off-by: Surong Pang <surong.pang@unisoc.com>
> ---
>  drivers/usb/host/xhci-plat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 649ffd861b44..dc73a81cbe9b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -390,7 +390,6 @@ static int xhci_plat_remove(struct platform_device *dev)
>  
>  	usb_remove_hcd(shared_hcd);
>  	xhci->shared_hcd = NULL;
> -	usb_phy_shutdown(hcd->usb_phy);
>  
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(shared_hcd);
> @@ -398,6 +397,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	clk_disable_unprepare(clk);
>  	clk_disable_unprepare(reg_clk);
>  	usb_put_hcd(hcd);
> +	usb_phy_shutdown(hcd->usb_phy);

hcd might be freed already here.
maybe call usb_phy_shutdown(hcd->usb_phy) before usb_put_hcd(hcd)

-Mathias

Re: [PATCH V1 1/1] usb/host: Let usb phy shutdown later

[surong pang]

shared_hcd might be freed already here, but hcd should not be freed
here, because "usb_put_hcd(hcd)" is called later.

Mathias Nyman <mathias.nyman@linux.intel.com> 于2022年4月19日周二 22:43写道：
>
> On 12.4.2022 15.25, Surong Pang wrote:
> > From: Surong Pang <surong.pang@unisoc.com>
> >
> > Let usb phy shutdown later in xhci_plat_remove function.
> > Some phy driver doesn't divide 3.0/2.0 very clear.
> > If calls usb_phy_shutdown earlier than usb_remove_hcd(hcd),
> > It will case 10s cmd timeout issue.
> >
> > Call usb phy shutdown later has better compatibility.
> >
> > Signed-off-by: Surong Pang <surong.pang@unisoc.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 649ffd861b44..dc73a81cbe9b 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -390,7 +390,6 @@ static int xhci_plat_remove(struct platform_device *dev)
> >
> >       usb_remove_hcd(shared_hcd);
> >       xhci->shared_hcd = NULL;
> > -     usb_phy_shutdown(hcd->usb_phy);
> >
> >       usb_remove_hcd(hcd);
> >       usb_put_hcd(shared_hcd);
> > @@ -398,6 +397,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >       clk_disable_unprepare(clk);
> >       clk_disable_unprepare(reg_clk);
> >       usb_put_hcd(hcd);
> > +     usb_phy_shutdown(hcd->usb_phy);
>
> hcd might be freed already here.
> maybe call usb_phy_shutdown(hcd->usb_phy) before usb_put_hcd(hcd)
>
> -Mathias
>
>

Re: [PATCH V1 1/1] usb/host: Let usb phy shutdown later

[Mathias Nyman]

On 22.4.2022 5.10, surong pang wrote:
> shared_hcd might be freed already here, but hcd should not be freed
> here, because "usb_put_hcd(hcd)" is called later.

To me it still looks like this patch calls usb_phy_shutdown(hcd->usb_phy) _after_
usb_put_hcd(hcd):

>>> @@ -398,6 +397,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>>>       clk_disable_unprepare(clk);
>>>       clk_disable_unprepare(reg_clk);
>>>       usb_put_hcd(hcd);
>>> +     usb_phy_shutdown(hcd->usb_phy);


shared hcd was freed even earlier, before disabling the clocks.

Thanks
Mathias

Re: [PATCH V1 1/1] usb/host: Let usb phy shutdown later

[surong pang]

>>> @@ -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.

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] ?

Mathias Nyman <mathias.nyman@linux.intel.com> 于2022年4月22日周五 15:51写道：
>
> On 22.4.2022 5.10, surong pang wrote:
> > shared_hcd might be freed already here, but hcd should not be freed
> > here, because "usb_put_hcd(hcd)" is called later.
>
> To me it still looks like this patch calls usb_phy_shutdown(hcd->usb_phy) _after_
> usb_put_hcd(hcd):
>
> >>> @@ -398,6 +397,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> >>>       clk_disable_unprepare(clk);
> >>>       clk_disable_unprepare(reg_clk);
> >>>       usb_put_hcd(hcd);
> >>> +     usb_phy_shutdown(hcd->usb_phy);
>
>
> shared hcd was freed even earlier, before disabling the clocks.
>
> Thanks
> Mathias

Re: [PATCH V1 1/1] usb/host: Let usb phy shutdown later

[Mathias Nyman]

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

[PATCH V2] usb/host: Let usb phy shutdown later

[Surong Pang]

From: Surong Pang <surong.pang@unisoc.com>

Let usb phy shutdown later in xhci_plat_remove function.
Some phy driver doesn't divide 3.0/2.0 very clear.
If calls usb_phy_shutdown earlier than usb_remove_hcd(hcd),
It will case 10s cmd timeout issue.

Call usb phy shutdown later has better compatibility.

Signed-off-by: Surong Pang <surong.pang@unisoc.com>
---
 drivers/usb/host/xhci-plat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 649ffd861b44..fe492ed99cb7 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -390,13 +390,13 @@ static int xhci_plat_remove(struct platform_device *dev)
 
 	usb_remove_hcd(shared_hcd);
 	xhci->shared_hcd = NULL;
-	usb_phy_shutdown(hcd->usb_phy);
 
 	usb_remove_hcd(hcd);
 	usb_put_hcd(shared_hcd);
 
 	clk_disable_unprepare(clk);
 	clk_disable_unprepare(reg_clk);
+	usb_phy_shutdown(hcd->usb_phy);
 	usb_put_hcd(hcd);
 
 	pm_runtime_disable(&dev->dev);
-- 
2.17.1

Re: [PATCH V2] usb/host: Let usb phy shutdown later

[Greg KH]

On Sun, Apr 24, 2022 at 09:57:57AM +0800, Surong Pang wrote:
> From: Surong Pang <surong.pang@unisoc.com>
> 
> Let usb phy shutdown later in xhci_plat_remove function.
> Some phy driver doesn't divide 3.0/2.0 very clear.
> If calls usb_phy_shutdown earlier than usb_remove_hcd(hcd),
> It will case 10s cmd timeout issue.
> 
> Call usb phy shutdown later has better compatibility.
> 
> Signed-off-by: Surong Pang <surong.pang@unisoc.com>

The subject should say "xhci-plat", right?

> ---
>  drivers/usb/host/xhci-plat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 649ffd861b44..fe492ed99cb7 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -390,13 +390,13 @@ static int xhci_plat_remove(struct platform_device *dev)
>  
>  	usb_remove_hcd(shared_hcd);
>  	xhci->shared_hcd = NULL;
> -	usb_phy_shutdown(hcd->usb_phy);
>  
>  	usb_remove_hcd(hcd);
>  	usb_put_hcd(shared_hcd);
>  
>  	clk_disable_unprepare(clk);
>  	clk_disable_unprepare(reg_clk);
> +	usb_phy_shutdown(hcd->usb_phy);
>  	usb_put_hcd(hcd);

Does this fix a specific commit id?

thanks,

greg k-h

Re: [PATCH V2] usb/host: Let usb phy shutdown later

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Apr 28, 2022 at 02:26:27PM +0800, surong pang wrote:
> Dear Greg,
> No,  It's just a patch to call usb_phy_shutdown later.

I do not understand this response, sorry, as I have no context here.

Also, please fix your email client to not send html email, the mailing
lists reject mail written that way.

thanks,

greg k-h

Re: [PATCH V2] xhci-plat: Let usb phy shutdown later

[surong pang]

Dear Greg,
Sorry for html email response.
Yes, The subject should say "xhci-plat". And it didn't fix a specific commit id.

surong pang <surong.pang@gmail.com> 于2022年4月28日周四 14:26写道：
>
> Dear Greg,
> No,  It's just a patch to call usb_phy_shutdown later.
>
>
> Greg KH <gregkh@linuxfoundation.org> 于2022年4月26日周二 19:53写道：
>>
>> On Sun, Apr 24, 2022 at 09:57:57AM +0800, Surong Pang wrote:
>> > From: Surong Pang <surong.pang@unisoc.com>
>> >
>> > Let usb phy shutdown later in xhci_plat_remove function.
>> > Some phy driver doesn't divide 3.0/2.0 very clear.
>> > If calls usb_phy_shutdown earlier than usb_remove_hcd(hcd),
>> > It will case 10s cmd timeout issue.
>> >
>> > Call usb phy shutdown later has better compatibility.
>> >
>> > Signed-off-by: Surong Pang <surong.pang@unisoc.com>
>>
>> The subject should say "xhci-plat", right?
>>
>> > ---
>> >  drivers/usb/host/xhci-plat.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> > index 649ffd861b44..fe492ed99cb7 100644
>> > --- a/drivers/usb/host/xhci-plat.c
>> > +++ b/drivers/usb/host/xhci-plat.c
>> > @@ -390,13 +390,13 @@ static int xhci_plat_remove(struct platform_device *dev)
>> >
>> >       usb_remove_hcd(shared_hcd);
>> >       xhci->shared_hcd = NULL;
>> > -     usb_phy_shutdown(hcd->usb_phy);
>> >
>> >       usb_remove_hcd(hcd);
>> >       usb_put_hcd(shared_hcd);
>> >
>> >       clk_disable_unprepare(clk);
>> >       clk_disable_unprepare(reg_clk);
>> > +     usb_phy_shutdown(hcd->usb_phy);
>> >       usb_put_hcd(hcd);
>>
>> Does this fix a specific commit id?
>>
>> thanks,
>>
>> greg k-h