Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
WARNING: multiple messages have this Message-ID
From: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
To: Douglas Anderson <dianders@chromium.org>,
	Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>,
	Felipe Balbi <felipe.balbi@linux.intel.com>,
	"heiko@sntech.de" <heiko@sntech.de>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	"amstan@chromium.org" <amstan@chromium.org>,
	"linux-rockchip@lists.infradead.org"
	<linux-rockchip@lists.infradead.org>,
	William Wu <william.wu@rock-chips.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Stefan Wahren <stefan.wahren@i2se.com>,
	Randy Li <ayaka@soulik.info>,
	"zyw@rock-chips.com" <zyw@rock-chips.com>,
	"mka@chromium.org" <mka@chromium.org>,
	"ryandcase@chromium.org" <ryandcase@chromium.org>,
	Amelie Delaunay <amelie.delaunay@st.com>,
	"jwerner@chromium.org" <jwerner@chromium.org>,
	"dinguyen@opensource.altera.com" <dinguyen@opensource.altera.com>,
	Elaine Zhang <zhangqing@rock-chips.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
Date: Mon, 29 Apr 2019 08:43:34 +0000
Message-ID: <SN1PR12MB243108D1EF3239EC4F730ACDA7390@SN1PR12MB2431.namprd12.prod.outlook.com> (raw)

Hi,

On 4/18/2019 04:15, Douglas Anderson wrote:
> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> because apparently it broke the Altera SOCFPGA.
> 
> With all the changes that have happened to dwc2 in the meantime, it's
> possible that the Altera SOCFPGA will just magically work with this
> change now.  ...and it would be good to get bus suspend/resume
> implemented.
> 
> This change is a forward port of one that's been living in the Chrome
> OS 3.14 kernel tree.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch was last posted at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> 
> ...and appears to have died the death of silence.  Maybe it could get
> some bake time in linuxnext if we can't find any proactive testing?
> 
> I will also freely admit that I don't know tons about the theory
> behind this patch.  I'm mostly just re-hashing the original commit
> from Kever that was reverted since:
> * Turning on partial power down on rk3288 doesn't "just work".  I
>    don't get hotplug events.  This is despite dwc2 auto-detecting that
>    we are power optimized.
What do you mean by doesn't "just work" ? It seem to me that even after 
adding this patch you don't get issues fixed.
You mention that you don't get the hotplug events. Please provide dwc2 
debug logs and register dumps on this issue.

> * If we don't do something like this commit we don't get into as low
>    of a power mode.
> 
> Changes in v2: None
> 
>   drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++----------------
>   1 file changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e272d020012e..978232a9e4a8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	unsigned long flags;
>   	int ret = 0;
>   	u32 hprt0;
> +	u32 pcgctl;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
>   
> @@ -4497,7 +4498,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)
 >   		goto skip_power_saving;
 >

"hsotg->params.power_down" is assigned to "DWC2_POWER_DOWN_PARAM_NONE = 
0" if there is no hibernation or partial power down supported by the 
core or power saving features are disabled by 
"hsotg->params.power_saving = false" , "DWC2_POWER_DOWN_PARAM_PARTIAL" 
if core supports partial power down, "DWC2_POWER_DOWN_PARAM_HIBERNATION 
" if the core supports hibernation

When you check "if (hsotg->params.power_down > 
DWC2_POWER_DOWN_PARAM_PARTIAL)" you are saying that "skip_power_saving" 
only in that case when core supports Hibernation. But what if core 
doesn't support both hibernation and partial power down and the 
"hsotg->params.power_down" value us equal to 
"DWC2_POWER_DOWN_PARAM_NONE" which is 0.

With this implementation driver will program entering to suspend when 
core doesn't support both hibernation and partial power down.

>   	/*
> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	 */
>   	if (!hsotg->bus_suspended) {
>   		hprt0 = dwc2_read_hprt0(hsotg);
> -		hprt0 |= HPRT0_SUSP;
> -		hprt0 &= ~HPRT0_PWR;
> -		dwc2_writel(hsotg, hprt0, HPRT0);
> -		spin_unlock_irqrestore(&hsotg->lock, flags);
> -		dwc2_vbus_supply_exit(hsotg);
> -		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (hprt0 & HPRT0_CONNSTS) { > +			hprt0 |= HPRT0_SUSP;
Here you set "HPRT0_SUSP" bit but what if core doesn't support both 
hibernation and Partial Power down assuming that 
hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0.
> +			if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
You make one checking of hsotg->params.power_down mode here.
> +				hprt0 &= ~HPRT0_PWR;
> +			dwc2_writel(hsotg, hprt0, HPRT0);
> +		}
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
another checking of power_down mode here.
> +			spin_unlock_irqrestore(&hsotg->lock, flags);
> +			dwc2_vbus_supply_exit(hsotg);
> +			spin_lock_irqsave(&hsotg->lock, flags);
> +		} else {
> +			pcgctl = readl(hsotg->regs + PCGCTL);
> +			pcgctl |= PCGCTL_STOPPCLK;
> +			writel(pcgctl, hsotg->regs + PCGCTL);
"PCGCTL_STOPPCLK" bit is set only when core enters to partial power 
down. So here if hsotg->params.power_down is not equal to 
DWC2_POWER_DOWN_PARAM_PARTIAL and is DWC2_POWER_DOWN_PARAM_NONE the the 
bit will be set.
> +		}
>   	}
>   
> -	/* Enter partial_power_down */
> -	ret = dwc2_enter_partial_power_down(hsotg);
> -	if (ret) {
> -		if (ret != -ENOTSUPP)
> -			dev_err(hsotg->dev,
> -				"enter partial_power_down failed\n");
> -		goto skip_power_saving;
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
one more power_down mode checking here.
I understand that those checking are to make sure that we got partial 
power down mode enabled but before this patch it was done with one checking.
> +		/* Enter partial_power_down */
> +		ret = dwc2_enter_partial_power_down(hsotg);
> +		if (ret) {
> +			if (ret != -ENOTSUPP)
> +				dev_err(hsotg->dev,
> +					"enter partial_power_down failed\n");
> +			goto skip_power_saving;
> +		}
> +
> +		/* After entering partial_power_down, hardware is no more accessible */
> +		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>   	}
>   
>   	/* Ask phy to be suspended */
> @@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* After entering partial_power_down, hardware is no more accessible */
> -	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   skip_power_saving:
>   	hsotg->lx_state = DWC2_L2;
>   unlock:
> @@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   {
>   	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>   	unsigned long flags;
> +	u32 pcgctl;
>   	int ret = 0;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   	if (hsotg->lx_state != DWC2_L2)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
>   		hsotg->lx_state = DWC2_L0;
>   		goto unlock;
>   	}
>   
> -	/*
> -	 * Set HW accessible bit before powering on the controller
> -	 * since an interrupt may rise.
> -	 */
> -	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   	/*
>   	 * Enable power if not already done.
>   	 * This must not be spinlocked since duration
> @@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* Exit partial_power_down */
> -	ret = dwc2_exit_partial_power_down(hsotg, true);
> -	if (ret && (ret != -ENOTSUPP))
> -		dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +		/*
> +		 * Set HW accessible bit before powering on the controller
> +		 * since an interrupt may rise.
> +		 */
> +		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +
> +
you leave an odd blank line here.  Please delete it.
> +		/* Exit partial_power_down */
> +		ret = dwc2_exit_partial_power_down(hsotg, true);
> +		if (ret && (ret != -ENOTSUPP))
> +			dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	} else {
> +		pcgctl = readl(hsotg->regs + PCGCTL);
> +		pcgctl &= ~PCGCTL_STOPPCLK;
> +		writel(pcgctl, hsotg->regs + PCGCTL);

Here if core doesn't support both hibernation and partial power down 
and "hsotg->params.power_down" is equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0 then "PCGCTL_STOPPCLK" bit is unset.

> +	}
>   
>   	hsotg->lx_state = DWC2_L0;
>   
> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_unlock_irqrestore(&hsotg->lock, flags);
>   		dwc2_port_resume(hsotg);
>   	} else {
> -		dwc2_vbus_supply_init(hsotg);
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +			dwc2_vbus_supply_init(hsotg);
>   
> -		/* Wait for controller to correctly update D+/D- level */
> -		usleep_range(3000, 5000);
> +			/* Wait for controller to correctly update D+/D- level */
> +			usleep_range(3000, 5000);
> +		}
>   
>   		/*
>   		 * Clear Port Enable and Port Status changes.
> 

I have tested the patch on HAPS-DX. With this patch or without it when I 
have a device connected core  enters to partial power down and doesn't 
exit from it. So I cannot use the device.

From: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
To: Douglas Anderson <dianders@chromium.org>,
	Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>,
	Felipe Balbi <felipe.balbi@linux.intel.com>,
	"heiko@sntech.de" <heiko@sntech.de>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	"amstan@chromium.org" <amstan@chromium.org>,
	"linux-rockchip@lists.infradead.org" 
	<linux-rockchip@lists.infradead.org>,
	William Wu <william.wu@rock-chips.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Stefan Wahren <stefan.wahren@i2se.com>,
	Randy Li <ayaka@soulik.info>,
	"zyw@rock-chips.com" <zyw@rock-chips.com>,
	"mka@chromium.org" <mka@chromium.org>,
	"ryandcase@chromium.org" <ryandcase@chromium.org>,
	Amelie Delaunay <amelie.delaunay@st.com>,
	"jwerner@chromium.org" <jwerner@chromium.org>,
	"dinguyen@opensource.altera.com" <dinguyen@opensource.altera.com>,
	"Elaine Zhang" <zhangqing@rock-chips.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
Date: Mon, 29 Apr 2019 08:43:34 +0000
Message-ID: <SN1PR12MB243108D1EF3239EC4F730ACDA7390@SN1PR12MB2431.namprd12.prod.outlook.com> (raw)
Message-ID: <20190429084334.k_Q7osCeTMNWiMpVPCy3whFWgGuYnjN25rxrowUwgtc@z> (raw)
In-Reply-To: <20190418001356.124334-2-dianders@chromium.org>

Hi,

On 4/18/2019 04:15, Douglas Anderson wrote:
> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
> suspend/resume for dwc2") on ToT.  That commit was reverted in commit
> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
> because apparently it broke the Altera SOCFPGA.
> 
> With all the changes that have happened to dwc2 in the meantime, it's
> possible that the Altera SOCFPGA will just magically work with this
> change now.  ...and it would be good to get bus suspend/resume
> implemented.
> 
> This change is a forward port of one that's been living in the Chrome
> OS 3.14 kernel tree.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch was last posted at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=
> 
> ...and appears to have died the death of silence.  Maybe it could get
> some bake time in linuxnext if we can't find any proactive testing?
> 
> I will also freely admit that I don't know tons about the theory
> behind this patch.  I'm mostly just re-hashing the original commit
> from Kever that was reverted since:
> * Turning on partial power down on rk3288 doesn't "just work".  I
>    don't get hotplug events.  This is despite dwc2 auto-detecting that
>    we are power optimized.
What do you mean by doesn't "just work" ? It seem to me that even after 
adding this patch you don't get issues fixed.
You mention that you don't get the hotplug events. Please provide dwc2 
debug logs and register dumps on this issue.

> * If we don't do something like this commit we don't get into as low
>    of a power mode.
> 
> Changes in v2: None
> 
>   drivers/usb/dwc2/hcd.c | 84 ++++++++++++++++++++++++++----------------
>   1 file changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e272d020012e..978232a9e4a8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4482,6 +4482,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	unsigned long flags;
>   	int ret = 0;
>   	u32 hprt0;
> +	u32 pcgctl;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
>   
> @@ -4497,7 +4498,7 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)
 >   		goto skip_power_saving;
 >

"hsotg->params.power_down" is assigned to "DWC2_POWER_DOWN_PARAM_NONE = 
0" if there is no hibernation or partial power down supported by the 
core or power saving features are disabled by 
"hsotg->params.power_saving = false" , "DWC2_POWER_DOWN_PARAM_PARTIAL" 
if core supports partial power down, "DWC2_POWER_DOWN_PARAM_HIBERNATION 
" if the core supports hibernation

When you check "if (hsotg->params.power_down > 
DWC2_POWER_DOWN_PARAM_PARTIAL)" you are saying that "skip_power_saving" 
only in that case when core supports Hibernation. But what if core 
doesn't support both hibernation and partial power down and the 
"hsotg->params.power_down" value us equal to 
"DWC2_POWER_DOWN_PARAM_NONE" which is 0.

With this implementation driver will program entering to suspend when 
core doesn't support both hibernation and partial power down.

>   	/*
> @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   	 */
>   	if (!hsotg->bus_suspended) {
>   		hprt0 = dwc2_read_hprt0(hsotg);
> -		hprt0 |= HPRT0_SUSP;
> -		hprt0 &= ~HPRT0_PWR;
> -		dwc2_writel(hsotg, hprt0, HPRT0);
> -		spin_unlock_irqrestore(&hsotg->lock, flags);
> -		dwc2_vbus_supply_exit(hsotg);
> -		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (hprt0 & HPRT0_CONNSTS) { > +			hprt0 |= HPRT0_SUSP;
Here you set "HPRT0_SUSP" bit but what if core doesn't support both 
hibernation and Partial Power down assuming that 
hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0.
> +			if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL)
You make one checking of hsotg->params.power_down mode here.
> +				hprt0 &= ~HPRT0_PWR;
> +			dwc2_writel(hsotg, hprt0, HPRT0);
> +		}
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
another checking of power_down mode here.
> +			spin_unlock_irqrestore(&hsotg->lock, flags);
> +			dwc2_vbus_supply_exit(hsotg);
> +			spin_lock_irqsave(&hsotg->lock, flags);
> +		} else {
> +			pcgctl = readl(hsotg->regs + PCGCTL);
> +			pcgctl |= PCGCTL_STOPPCLK;
> +			writel(pcgctl, hsotg->regs + PCGCTL);
"PCGCTL_STOPPCLK" bit is set only when core enters to partial power 
down. So here if hsotg->params.power_down is not equal to 
DWC2_POWER_DOWN_PARAM_PARTIAL and is DWC2_POWER_DOWN_PARAM_NONE the the 
bit will be set.
> +		}
>   	}
>   
> -	/* Enter partial_power_down */
> -	ret = dwc2_enter_partial_power_down(hsotg);
> -	if (ret) {
> -		if (ret != -ENOTSUPP)
> -			dev_err(hsotg->dev,
> -				"enter partial_power_down failed\n");
> -		goto skip_power_saving;
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
one more power_down mode checking here.
I understand that those checking are to make sure that we got partial 
power down mode enabled but before this patch it was done with one checking.
> +		/* Enter partial_power_down */
> +		ret = dwc2_enter_partial_power_down(hsotg);
> +		if (ret) {
> +			if (ret != -ENOTSUPP)
> +				dev_err(hsotg->dev,
> +					"enter partial_power_down failed\n");
> +			goto skip_power_saving;
> +		}
> +
> +		/* After entering partial_power_down, hardware is no more accessible */
> +		clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>   	}
>   
>   	/* Ask phy to be suspended */
> @@ -4530,9 +4545,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* After entering partial_power_down, hardware is no more accessible */
> -	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   skip_power_saving:
>   	hsotg->lx_state = DWC2_L2;
>   unlock:
> @@ -4545,6 +4557,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   {
>   	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
>   	unsigned long flags;
> +	u32 pcgctl;
>   	int ret = 0;
>   
>   	spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4555,17 +4568,11 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   	if (hsotg->lx_state != DWC2_L2)
>   		goto unlock;
>   
> -	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +	if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
>   		hsotg->lx_state = DWC2_L0;
>   		goto unlock;
>   	}
>   
> -	/*
> -	 * Set HW accessible bit before powering on the controller
> -	 * since an interrupt may rise.
> -	 */
> -	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
>   	/*
>   	 * Enable power if not already done.
>   	 * This must not be spinlocked since duration
> @@ -4577,10 +4584,23 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_lock_irqsave(&hsotg->lock, flags);
>   	}
>   
> -	/* Exit partial_power_down */
> -	ret = dwc2_exit_partial_power_down(hsotg, true);
> -	if (ret && (ret != -ENOTSUPP))
> -		dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +		/*
> +		 * Set HW accessible bit before powering on the controller
> +		 * since an interrupt may rise.
> +		 */
> +		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +
> +
you leave an odd blank line here.  Please delete it.
> +		/* Exit partial_power_down */
> +		ret = dwc2_exit_partial_power_down(hsotg, true);
> +		if (ret && (ret != -ENOTSUPP))
> +			dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +	} else {
> +		pcgctl = readl(hsotg->regs + PCGCTL);
> +		pcgctl &= ~PCGCTL_STOPPCLK;
> +		writel(pcgctl, hsotg->regs + PCGCTL);

Here if core doesn't support both hibernation and partial power down 
and "hsotg->params.power_down" is equal to "DWC2_POWER_DOWN_PARAM_NONE" 
which is 0 then "PCGCTL_STOPPCLK" bit is unset.

> +	}
>   
>   	hsotg->lx_state = DWC2_L0;
>   
> @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>   		spin_unlock_irqrestore(&hsotg->lock, flags);
>   		dwc2_port_resume(hsotg);
>   	} else {
> -		dwc2_vbus_supply_init(hsotg);
> +		if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> +			dwc2_vbus_supply_init(hsotg);
>   
> -		/* Wait for controller to correctly update D+/D- level */
> -		usleep_range(3000, 5000);
> +			/* Wait for controller to correctly update D+/D- level */
> +			usleep_range(3000, 5000);
> +		}
>   
>   		/*
>   		 * Clear Port Enable and Port Status changes.
> 

I have tested the patch on HAPS-DX. With this patch or without it when I 
have a device connected core  enters to partial power down and doesn't 
exit from it. So I cannot use the device.

-- 
Regards,
Artur

         reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-18  0:13 [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Douglas Anderson
2019-04-18  0:13 ` [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Doug Anderson
2019-04-18  0:13   ` [PATCH v2 1/5] " Douglas Anderson
2019-04-29  8:43   ` Artur Petrosyan [this message]
2019-04-29  8:43     ` Artur Petrosyan
2019-04-29 17:33     ` [v2,1/5] " Doug Anderson
2019-04-29 17:33       ` [PATCH v2 1/5] " Doug Anderson
2019-04-30  6:05       ` [v2,1/5] " Artur Petrosyan
2019-04-30  6:05         ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-01  1:51         ` [v2,1/5] " Doug Anderson
2019-05-01  1:51           ` [PATCH v2 1/5] " Doug Anderson
2019-05-03  8:20           ` [v2,1/5] " Artur Petrosyan
2019-05-03  8:20             ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
2019-05-03 15:03               ` [PATCH v2 1/5] " Doug Anderson
2019-05-07  7:26               ` Artur Petrosyan
2019-05-01 23:58   ` [v2,1/5] " Doug Anderson
2019-05-01 23:58     ` [PATCH v2 1/5] " Doug Anderson
2019-05-03  8:25     ` [v2,1/5] " Artur Petrosyan
2019-05-03  8:25       ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
2019-05-03 15:07         ` [PATCH v2 1/5] " Doug Anderson
2019-05-07  7:05         ` Artur Petrosyan
2019-04-18  0:13 ` [v2,2/5] USB: Export usb_wakeup_enabled_descendants() Doug Anderson
2019-04-18  0:13   ` [PATCH v2 2/5] " Douglas Anderson
2019-04-18  0:13 ` [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB Doug Anderson
2019-04-18  0:13   ` [PATCH v2 3/5] " Douglas Anderson
2019-04-25 12:40   ` [v2,3/5] " Felipe Balbi
2019-04-25 12:40     ` [PATCH v2 3/5] " Felipe Balbi
2019-04-25 18:09     ` [v2,3/5] " Doug Anderson
2019-04-25 18:09       ` [PATCH v2 3/5] " Doug Anderson
2019-04-25 19:58       ` [v2,3/5] " Doug Anderson
2019-04-25 19:58         ` [PATCH v2 3/5] " Doug Anderson
2019-05-02 18:36     ` [v2,3/5] " Doug Anderson
2019-05-02 18:36       ` [PATCH v2 3/5] " Doug Anderson
2019-04-30  1:23   ` [v2,3/5] " Rob Herring
2019-04-30  1:23     ` [PATCH v2 3/5] " Rob Herring
2019-04-30  5:25     ` [v2,3/5] " Doug Anderson
2019-04-30  5:25       ` [PATCH v2 3/5] " Doug Anderson
2019-04-18  0:13 ` [v2,4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled Doug Anderson
2019-04-18  0:13   ` [PATCH v2 4/5] " Douglas Anderson
2019-04-25 12:41   ` [v2,4/5] " Felipe Balbi
2019-04-25 12:41     ` [PATCH v2 4/5] " Felipe Balbi
2019-04-18  0:13 ` [v2,5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports Doug Anderson
2019-04-18  0:13   ` [PATCH v2 5/5] " Douglas Anderson
2019-04-18 12:40 ` [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Minas Harutyunyan
2019-04-18 15:54   ` Doug Anderson
2019-04-19 11:43     ` Artur Petrosyan
2019-04-19 16:44       ` Artur Petrosyan
2019-04-22 15:50         ` Artur Petrosyan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=SN1PR12MB243108D1EF3239EC4F730ACDA7390@SN1PR12MB2431.namprd12.prod.outlook.com \
    --to=arthur.petrosyan@synopsys.com \
    --cc=Minas.Harutyunyan@synopsys.com \
    --cc=amelie.delaunay@st.com \
    --cc=amstan@chromium.org \
    --cc=ayaka@soulik.info \
    --cc=dianders@chromium.org \
    --cc=dinguyen@opensource.altera.com \
    --cc=felipe.balbi@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko@sntech.de \
    --cc=jwerner@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=ryandcase@chromium.org \
    --cc=stefan.wahren@i2se.com \
    --cc=stern@rowland.harvard.edu \
    --cc=william.wu@rock-chips.com \
    --cc=zhangqing@rock-chips.com \
    --cc=zyw@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git