From: Surong Pang <[email protected]>
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 <[email protected]>
---
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
On 12.4.2022 15.25, Surong Pang wrote:
> From: Surong Pang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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
>>> @@ -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 <[email protected]> 于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
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
shared_hcd might be freed already here, but hcd should not be freed
here, because "usb_put_hcd(hcd)" is called later.
Mathias Nyman <[email protected]> 于2022年4月19日周二 22:43写道：
>
> On 12.4.2022 15.25, Surong Pang wrote:
> > From: Surong Pang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
>
>
From: Surong Pang <[email protected]>
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 <[email protected]>
---
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
On Sun, Apr 24, 2022 at 09:57:57AM +0800, Surong Pang wrote:
> From: Surong Pang <[email protected]>
>
> 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 <[email protected]>
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
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