[PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[Douglas Anderson]

This re-hashes two older series I posted a long time ago, re-basing
them to mainline.  ...well, technically, atop another dwc2 series I
recently posted:

* usb: dwc2: Another attempt handling rk3288's remote wake quirk
  https://lkml.kernel.org/r/20190416215351.242246-1-dianders@chromium.org
  https://lore.kernel.org/patchwork/cover/1062972/

In general I've tried to add links to each patch pointing to relevant
older discussion.  Here are overall links to the cover letters though.
Note that for the previous "allow wakeup" series the discussion was
scattered a bit between the original post and the repost.

* usb: dwc2: bus suspend/resume that's not hibernate
  https://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org
  https://lore.kernel.org/patchwork/patch/613761/

* dwc2 patches to allow wakeup on Rockchip rk3288
  https://lkml.kernel.org/r/1435017144-2971-1-git-send-email-dianders@chromium.org
  https://lore.kernel.org/patchwork/cover/572944/

* dwc2 patches to allow wakeup on Rockchip rk3288 (REPOST)
  https://lkml.kernel.org/r/1436207224-21849-1-git-send-email-dianders@chromium.org
  https://lore.kernel.org/patchwork/cover/576120/

I'm hoping there's a better chance of these things landing this time
around, but I guess we'll see.  ;-)

In case it's helpful I've put what I tested (which is based on Heiko's
for-next branch and includes patches to enable deep suspend plus two
other s2r fixes) at:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/sandbox/dianders/190417-testing-dwc2-wakeup

Changes in v2:
- Rebased to mainline atop rk3288 remote wake quirk series.
- rk3288-veyron dts patch new for v2.

Douglas Anderson (5):
  usb: dwc2: bus suspend/resume for hosts with
    DWC2_POWER_DOWN_PARAM_NONE
  USB: Export usb_wakeup_enabled_descendants()
  Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
  USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
  ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports

 .../devicetree/bindings/usb/dwc2.txt          |  3 +
 arch/arm/boot/dts/rk3288-veyron.dtsi          |  2 +
 drivers/usb/core/hub.c                        |  7 +-
 drivers/usb/dwc2/core.h                       |  5 ++
 drivers/usb/dwc2/hcd.c                        | 84 ++++++++++++-------
 drivers/usb/dwc2/platform.c                   | 43 +++++++++-
 include/linux/usb/hcd.h                       |  5 ++
 7 files changed, 113 insertions(+), 36 deletions(-)

-- 
2.21.0.593.g511ec345e18-goog

[PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Douglas Anderson]

Some SoCs with a dwc2 USB controller may need to keep the PHY on to
support remote wakeup.  Allow specifying this as a device tree
property.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
For relevant prior discussion on this patch, see:

https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org

I didn't make any changes from the prior version since I never found
out what Rob thought of my previous arguments.  If folks want a
change, perhaps they could choose from these options:

1. Assume that all dwc2 hosts would like to keep their PHY on for
   suspend if there's a USB wakeup enabled, thus we totally drop this
   binding.  This doesn't seem super great to me since I'd bet that
   many devices that use dwc2 weren't designed for USB wakeup (they
   may not keep enough clocks or rails on) so we might be wasting
   power for nothing.
2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
   it more obvious that this property is intended both to document
   that wakeup from suspend is possible and that we need the PHY for
   said wakeup.
3. Rename this property to "snps,can-wakeup-from-suspend" and assume
   it's implicit that if we can wakeup from suspend that we need to
   keep the PHY on.  If/when someone shows that a device exists using
   dwc2 where we can wakeup from suspend without the PHY they can add
   a new property.

Changes in v2: None

 Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index f70f3aee4bfc..1c5e29d23c51 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -37,6 +37,8 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties
 - g-rx-fifo-size: size of rx fifo size in gadget mode.
 - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode.
 - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode.
+- snps,need-phy-for-wake: If present indicates that the phy needs to be left
+                          on for remote wakeup during suspend.
 - snps,reset-phy-on-wake: If present indicates that we need to reset the PHY when
                           we detect a wakeup.  This is due to a hardware errata.
 
@@ -53,4 +55,5 @@ Example:
 		clock-names = "otg";
 		phys = <&usbphy>;
 		phy-names = "usb2-phy";
+		snps,need-phy-for-wake;
         };
-- 
2.21.0.593.g511ec345e18-goog

[PATCH v2 5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports

[Douglas Anderson]

We want to be able to wake from USB if a device is plugged in that
wants remote wakeup.  Enable it on both dwc2 controllers.

NOTE: this is added specifically to veyron and not to rk3288 in
general since it's not known whether all rk3288 boards are designed to
support USB wakeup.  It is plausible that some boards could shut down
important rails in S3.

Also note that currently wakeup doesn't seem to happen unless you use
the "deep" suspend mode (where SDRAM is turned off).  Presumably the
shallow suspend mode is gating some sort of clock that's important but
I couldn't easily figure out how to get it working.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- rk3288-veyron dts patch new for v2.

 arch/arm/boot/dts/rk3288-veyron.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index 1252522392c7..1d8bfed7830c 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -424,6 +424,7 @@
 
 &usb_host1 {
 	status = "okay";
+	snps,need-phy-for-wake;
 };
 
 &usb_otg {
@@ -432,6 +433,7 @@
 	assigned-clocks = <&cru SCLK_USBPHY480M_SRC>;
 	assigned-clock-parents = <&usbphy0>;
 	dr_mode = "host";
+	snps,need-phy-for-wake;
 };
 
 &vopb {
-- 
2.21.0.593.g511ec345e18-goog

Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[Minas Harutyunyan]

Hi Douglas,

On 4/18/2019 4:15 AM, Douglas Anderson wrote:
> This re-hashes two older series I posted a long time ago, re-basing
> them to mainline.  ...well, technically, atop another dwc2 series I
> recently posted:
> 
> * usb: dwc2: Another attempt handling rk3288's remote wake quirk
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_20190416215351.242246-2D1-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=b-IYjc3cgFyZYC_B9zSA_xbSLE2ODwAZX0Png-G4SwA&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_cover_1062972_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=VCNBuDGf2VeFFWUUyE3wUdn-sDnUu0nFipeNpyWPXts&e=
> 
> In general I've tried to add links to each patch pointing to relevant
> older discussion.  Here are overall links to the cover letters though.
> Note that for the previous "allow wakeup" series the discussion was
> scattered a bit between the original post and the repost.
> 
> * usb: dwc2: bus suspend/resume that's not hibernate
>    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=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=FSVMrPFuSM--uXrAZN9GzCEXP60Li7miMsC4ydv6oDQ&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_patch_613761_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=omRYPw4XVgY8Rq2UgJhApk2poeKXWCBc5QsYMlQkqk4&e=
> 
> * dwc2 patches to allow wakeup on Rockchip rk3288
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1435017144-2D2971-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=ep7GoHZcPlQbiOkXlwy9xXZEKdbd4o2erhLSblDL5Rg&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_cover_572944_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=hq4errAA9YvVpHJaVoWGTIHnlwiq1iuadWW1WJavtCI&e=
> 
> * dwc2 patches to allow wakeup on Rockchip rk3288 (REPOST)
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1436207224-2D21849-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=n-nD8EdqtgTyPdPPJ06t2pxyjC1M65g5aXLT1OiuouY&e=
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_patchwork_cover_576120_&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=YkisA-u9q0yewqaRoOWWpps9E2QFh1asgnwkbM1B7mc&e=
> 
> I'm hoping there's a better chance of these things landing this time
> around, but I guess we'll see.  ;-)
> 
> In case it's helpful I've put what I tested (which is based on Heiko's
> for-next branch and includes patches to enable deep suspend plus two
> other s2r fixes) at:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2Blog_refs_sandbox_dianders_190417-2Dtesting-2Ddwc2-2Dwakeup&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=cQBKt4q-qzNVC53rNAwuwplH23V61rHQhhULvdLA0U8&m=tZD-jhA4bSymns9pwqhaT0Ico1SVCYevQaaQclHX8jY&s=ws6kQVDDGxYJnhR693pQP9fYL-PH_TUGzSVmD4u-S9Q&e=
> 
> Changes in v2:
> - Rebased to mainline atop rk3288 remote wake quirk series.
> - rk3288-veyron dts patch new for v2.
> 
> Douglas Anderson (5):
>    usb: dwc2: bus suspend/resume for hosts with
>      DWC2_POWER_DOWN_PARAM_NONE
>    USB: Export usb_wakeup_enabled_descendants()
>    Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
>    USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
>    ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports
> 
>   .../devicetree/bindings/usb/dwc2.txt          |  3 +
>   arch/arm/boot/dts/rk3288-veyron.dtsi          |  2 +
>   drivers/usb/core/hub.c                        |  7 +-
>   drivers/usb/dwc2/core.h                       |  5 ++
>   drivers/usb/dwc2/hcd.c                        | 84 ++++++++++++-------
>   drivers/usb/dwc2/platform.c                   | 43 +++++++++-
>   include/linux/usb/hcd.h                       |  5 ++
>   7 files changed, 113 insertions(+), 36 deletions(-)
> 

Did you consider/reviewed patch series from Artur Petrosyan "[PATCH 
00/14] usb: dwc2: Fix and improve power saving modes" (submitted on 
April 12) which fixing partial power down and hibernation flows for both 
modes: host and device?
I suspect that this both patch series can be in conflict.

Thanks,
Minas

Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[Doug Anderson]

Hi,

On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
<Minas.Harutyunyan@synopsys.com> wrote:
> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
> April 12) which fixing partial power down and hibernation flows for both
> modes: host and device?
> I suspect that this both patch series can be in conflict.

No, I wasn't aware of them.  I'd like to try them out, but it looks
like it's currently impossible because they're not archived anywhere
that I can find.

1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
suggested to CC LKML on all patches.

2. The Linux USB patchwork only has the cover letter plus the first 6
patches.  See <https://patchwork.kernel.org/cover/10898333/>

3. Searching my own archives I only see the cover letter plus the
first 6 patches.

Maybe you have some other pointer to how I can retrieve them?  I guess
I could try the first 6 patches without the later 8 but I'd rather get
the whole set...


-Doug

Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[Artur Petrosyan]

Hi,

On 4/18/2019 19:55, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
> <Minas.Harutyunyan@synopsys.com> wrote:
>> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
>> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
>> April 12) which fixing partial power down and hibernation flows for both
>> modes: host and device?
>> I suspect that this both patch series can be in conflict.
> 
> No, I wasn't aware of them.  I'd like to try them out, but it looks
> like it's currently impossible because they're not archived anywhere
> that I can find.
> 
> 1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
> suggested to CC LKML on all patches.
> 
> 2. The Linux USB patchwork only has the cover letter plus the first 6
> patches.  See <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_cover_10898333_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=rJbofHXqjP3sUBgEikaZ89fRWtsquvhFUkhSPguCiWw&s=_CrI0uolquXIYC5SuoZ1vBs7BM19VfCceI96qZm9kAY&e=>
> 
> 3. Searching my own archives I only see the cover letter plus the
> first 6 patches.
> 
> Maybe you have some other pointer to how I can retrieve them?  I guess
> I could try the first 6 patches without the later 8 but I'd rather get
> the whole set...
> 
> 
> -Doug
>


I have resend the patch set. You can find it with cover letter
"[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."

Let me know if you need any help.

-- 
Regards,
Artur

Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[Artur Petrosyan]

Hi Doug,

On 4/19/2019 15:43, Artur Petrosyan wrote:
> Hi,
> 
> On 4/18/2019 19:55, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
>> <Minas.Harutyunyan@synopsys.com> wrote:
>>> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
>>> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
>>> April 12) which fixing partial power down and hibernation flows for both
>>> modes: host and device?
>>> I suspect that this both patch series can be in conflict.
>>
>> No, I wasn't aware of them.  I'd like to try them out, but it looks
>> like it's currently impossible because they're not archived anywhere
>> that I can find.
>>
>> 1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
>> suggested to CC LKML on all patches.
>>
>> 2. The Linux USB patchwork only has the cover letter plus the first 6
>> patches.  See <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_cover_10898333_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=rJbofHXqjP3sUBgEikaZ89fRWtsquvhFUkhSPguCiWw&s=_CrI0uolquXIYC5SuoZ1vBs7BM19VfCceI96qZm9kAY&e=>
>>
>> 3. Searching my own archives I only see the cover letter plus the
>> first 6 patches.
>>
>> Maybe you have some other pointer to how I can retrieve them?  I guess
>> I could try the first 6 patches without the later 8 but I'd rather get
>> the whole set...
>>
>>
>> -Doug
>>
> 
> 
> I have resend the patch set. You can find it with cover letter
> "[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."
> 
> Let me know if you need any help.
> 

I am so sorry to inform you that the patches have not reached to LKML.
Looks like there is a problem with the kernel mailing list servers or 
our local servers.
Will try to fix this as soon as possible and let you know about it.

-- 
Regards,
Artur

Re: [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron

[Artur Petrosyan]

Hi Doug,

On 4/19/2019 20:44, Artur Petrosyan wrote:
> Hi Doug,
> 
> On 4/19/2019 15:43, Artur Petrosyan wrote:
>> Hi,
>>
>> On 4/18/2019 19:55, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Apr 18, 2019 at 5:41 AM Minas Harutyunyan
>>> <Minas.Harutyunyan@synopsys.com> wrote:
>>>> Did you consider/reviewed patch series from Artur Petrosyan "[PATCH
>>>> 00/14] usb: dwc2: Fix and improve power saving modes" (submitted on
>>>> April 12) which fixing partial power down and hibernation flows for both
>>>> modes: host and device?
>>>> I suspect that this both patch series can be in conflict.
>>>
>>> No, I wasn't aware of them.  I'd like to try them out, but it looks
>>> like it's currently impossible because they're not archived anywhere
>>> that I can find.
>>>
>>> 1. LKML wasn't copied, so I can't find them on lore.kernel.org.  It is
>>> suggested to CC LKML on all patches.
>>>
>>> 2. The Linux USB patchwork only has the cover letter plus the first 6
>>> patches.  See <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_cover_10898333_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=rJbofHXqjP3sUBgEikaZ89fRWtsquvhFUkhSPguCiWw&s=_CrI0uolquXIYC5SuoZ1vBs7BM19VfCceI96qZm9kAY&e=>
>>>
>>> 3. Searching my own archives I only see the cover letter plus the
>>> first 6 patches.
>>>
>>> Maybe you have some other pointer to how I can retrieve them?  I guess
>>> I could try the first 6 patches without the later 8 but I'd rather get
>>> the whole set...
>>>
>>>
>>> -Doug
>>>
>>
>>
>> I have resend the patch set. You can find it with cover letter
>> "[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."
>>
>> Let me know if you need any help.
>>
> 
> I am so sorry to inform you that the patches have not reached to LKML.
> Looks like there is a problem with the kernel mailing list servers or
> our local servers.
> Will try to fix this as soon as possible and let you know about it.
> 

My patches has reached to LKML. You can find them with the patch set 
title "[PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes."

This is the link to the patch set in marc.info
"https://marc.info/?l=linux-usb&m=155570003924638&w=2"
and a link in patchwork.kernel.org
"https://patchwork.kernel.org/project/linux-usb/list/?submitter=180003"

Sorry for the delay.

-- 
Regards,
Artur

Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

Douglas Anderson <dianders@chromium.org> writes:

> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> support remote wakeup.  Allow specifying this as a device tree
> property.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> For relevant prior discussion on this patch, see:
>
> https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
>
> I didn't make any changes from the prior version since I never found
> out what Rob thought of my previous arguments.  If folks want a
> change, perhaps they could choose from these options:
>
> 1. Assume that all dwc2 hosts would like to keep their PHY on for
>    suspend if there's a USB wakeup enabled, thus we totally drop this
>    binding.  This doesn't seem super great to me since I'd bet that
>    many devices that use dwc2 weren't designed for USB wakeup (they
>    may not keep enough clocks or rails on) so we might be wasting
>    power for nothing.
> 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
>    it more obvious that this property is intended both to document
>    that wakeup from suspend is possible and that we need the PHY for
>    said wakeup.
> 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
>    it's implicit that if we can wakeup from suspend that we need to
>    keep the PHY on.  If/when someone shows that a device exists using
>    dwc2 where we can wakeup from suspend without the PHY they can add
>    a new property.
>
> Changes in v2: None
>
>  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
>  1 file changed, 3 insertions(+)

checking file Documentation/devicetree/bindings/usb/dwc2.txt
Hunk #1 FAILED at 37.
Hunk #2 succeeded at 52 (offset -1 lines).
1 out of 2 hunks FAILED
-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Doug Anderson]

Hi,

On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Douglas Anderson <dianders@chromium.org> writes:
>
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)
>
> checking file Documentation/devicetree/bindings/usb/dwc2.txt
> Hunk #1 FAILED at 37.
> Hunk #2 succeeded at 52 (offset -1 lines).
> 1 out of 2 hunks FAILED

Yeah, as Minas pointed out in the cover letter [1] my series conflicts
with Artur's.  I have it on my list to try out his series and see if,
perhaps, it allows me to enable the partial power down and also just
generally rebase.  It's fairly high on my list to do that--hopefully
in the next week.

[1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com

-Doug

Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Doug Anderson]

Hi,

On Thu, Apr 25, 2019 at 11:09 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
> >
> > Douglas Anderson <dianders@chromium.org> writes:
> >
> > > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > > support remote wakeup.  Allow specifying this as a device tree
> > > property.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > For relevant prior discussion on this patch, see:
> > >
> > > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> > >
> > > I didn't make any changes from the prior version since I never found
> > > out what Rob thought of my previous arguments.  If folks want a
> > > change, perhaps they could choose from these options:
> > >
> > > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> > >    suspend if there's a USB wakeup enabled, thus we totally drop this
> > >    binding.  This doesn't seem super great to me since I'd bet that
> > >    many devices that use dwc2 weren't designed for USB wakeup (they
> > >    may not keep enough clocks or rails on) so we might be wasting
> > >    power for nothing.
> > > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> > >    it more obvious that this property is intended both to document
> > >    that wakeup from suspend is possible and that we need the PHY for
> > >    said wakeup.
> > > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> > >    it's implicit that if we can wakeup from suspend that we need to
> > >    keep the PHY on.  If/when someone shows that a device exists using
> > >    dwc2 where we can wakeup from suspend without the PHY they can add
> > >    a new property.
> > >
> > > Changes in v2: None
> > >
> > >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > checking file Documentation/devicetree/bindings/usb/dwc2.txt
> > Hunk #1 FAILED at 37.
> > Hunk #2 succeeded at 52 (offset -1 lines).
> > 1 out of 2 hunks FAILED
>
> Yeah, as Minas pointed out in the cover letter [1] my series conflicts
> with Artur's.  I have it on my list to try out his series and see if,
> perhaps, it allows me to enable the partial power down and also just
> generally rebase.  It's fairly high on my list to do that--hopefully
> in the next week.
>
> [1] https://lkml.kernel.org/r/e4b3cd69-1c91-dfbe-bea7-bbca89ca1348@synopsys.com

Oh, it looks like you didn't apply Artur's patches, though.
Presumably you applied mine first and they won the race and thus I
guess it's up to Artur to rebase his patches atop mine.  This probably
explains why you told him the patches didn't apply.  I'll reply to
that thread.

...so it turns out that when I try now my patches apply fine [2].  I'm
guessing that you perhaps tried to apply these patches before my
"rk3288's remote wake quirk" which would indeed cause conflicts.  I
mentioned the dependency in my cover letter [1] but totally understand
that it's easy to miss stuff like that.  :-)

I'm going to assume you can just go-ahead and try applying patches 3,
4, and 5 in this series again.  If you want me to repost them then
please let me know.

Thanks, and sorry for the hassle.

[1] https://lkml.kernel.org/r/20190418001356.124334-1-dianders@chromium.org

[2] Showing that patches currently apply:

dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ git checkout
linux_usb_balbi/testing/next
HEAD is now at 2e3cfcbbb140 dwc2: gadget: Fix completed transfer size
calculation in DDMA

dianders@tictac2:v4.19 ((2e3cfcbbb140...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063477/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  7273  100  7273    0     0  35827      0 --:--:-- --:--:-- --:--:-- 35827
Applying: Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

dianders@tictac2:v4.19 ((84c34d7b9647...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063478/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  9587  100  9587    0     0  46538      0 --:--:-- --:--:-- --:--:-- 46538
Applying: USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled

dianders@tictac2:v4.19 ((ff5ab1cb16ab...))$ curl -L
https://lore.kernel.org/patchwork/patch/1063479/mbox | git am
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  6045  100  6045    0     0  29778      0 --:--:-- --:--:-- --:--:-- 29778
Applying: ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports

-Doug

Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Rob Herring]

On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote:
> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> support remote wakeup.  Allow specifying this as a device tree
> property.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> For relevant prior discussion on this patch, see:
> 
> https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders@chromium.org
> 
> I didn't make any changes from the prior version since I never found
> out what Rob thought of my previous arguments.  If folks want a
> change, perhaps they could choose from these options:
> 
> 1. Assume that all dwc2 hosts would like to keep their PHY on for
>    suspend if there's a USB wakeup enabled, thus we totally drop this
>    binding.  This doesn't seem super great to me since I'd bet that
>    many devices that use dwc2 weren't designed for USB wakeup (they
>    may not keep enough clocks or rails on) so we might be wasting
>    power for nothing.

1b. Use SoC specific compatible strings to enable/disable remote 
wake-up. We can debate what the default is I guess.

> 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
>    it more obvious that this property is intended both to document
>    that wakeup from suspend is possible and that we need the PHY for
>    said wakeup.
> 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
>    it's implicit that if we can wakeup from suspend that we need to
>    keep the PHY on.  If/when someone shows that a device exists using
>    dwc2 where we can wakeup from suspend without the PHY they can add
>    a new property.
> 
> Changes in v2: None
> 
>  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 

Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 6:23 PM Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> On Wed, Apr 17, 2019 at 05:13:54PM -0700, Douglas Anderson wrote:
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
>
> 1b. Use SoC specific compatible strings to enable/disable remote
> wake-up. We can debate what the default is I guess.

Unfortunately it's more than just SoC.  While you need the SoC to be
able to support this type of wakeup, you also need the board design,
firmware design, regulator design, etc.  ...so I don't think we can
just use the SoC specific compatible string.

In fact, while testing this I found that USB wakeup was totally broken
unless I enabled "deep suspend" mode on my system.  Something about
the clocks / wakeup sources in the shallow suspend totally blocked it
and I couldn't figure out what.

...so I believe it really needs to be something where someone has
said: I tested it out on this board and everything is setup properly
to support USB wakeup.


> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)

Re: [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Doug Anderson]

Hi,

On Thu, Apr 25, 2019 at 5:40 AM Felipe Balbi
<felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>
> Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> writes:
>
> > Some SoCs with a dwc2 USB controller may need to keep the PHY on to
> > support remote wakeup.  Allow specifying this as a device tree
> > property.
> >
> > Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> > For relevant prior discussion on this patch, see:
> >
> > https://lkml.kernel.org/r/1435017144-2971-3-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org
> >
> > I didn't make any changes from the prior version since I never found
> > out what Rob thought of my previous arguments.  If folks want a
> > change, perhaps they could choose from these options:
> >
> > 1. Assume that all dwc2 hosts would like to keep their PHY on for
> >    suspend if there's a USB wakeup enabled, thus we totally drop this
> >    binding.  This doesn't seem super great to me since I'd bet that
> >    many devices that use dwc2 weren't designed for USB wakeup (they
> >    may not keep enough clocks or rails on) so we might be wasting
> >    power for nothing.
> > 2. Rename this property to "snps,wakeup-from-suspend-with-phy" to make
> >    it more obvious that this property is intended both to document
> >    that wakeup from suspend is possible and that we need the PHY for
> >    said wakeup.
> > 3. Rename this property to "snps,can-wakeup-from-suspend" and assume
> >    it's implicit that if we can wakeup from suspend that we need to
> >    keep the PHY on.  If/when someone shows that a device exists using
> >    dwc2 where we can wakeup from suspend without the PHY they can add
> >    a new property.
> >
> > Changes in v2: None
> >
> >  Documentation/devicetree/bindings/usb/dwc2.txt | 3 +++
> >  1 file changed, 3 insertions(+)
>
> checking file Documentation/devicetree/bindings/usb/dwc2.txt
> Hunk #1 FAILED at 37.
> Hunk #2 succeeded at 52 (offset -1 lines).
> 1 out of 2 hunks FAILED

Can you try applying this and the next two patches again?  ...or let
me know that you'd like me to repost?

Thanks!

-Doug