Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1254863 > unrolled thread

[PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port Douglas Anderson <dianders@chromium.org> - 2015-10-23 20:30 +0200
    [PATCH 2/4] usb: dwc2: optionally assert phy "port reset" when waking up Douglas Anderson <dianders@chromium.org> - 2015-10-23 20:30 +0200
    [PATCH 4/4] ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset Douglas Anderson <dianders@chromium.org> - 2015-10-23 20:30 +0200
    [PATCH 3/4] ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288 Douglas Anderson <dianders@chromium.org> - 2015-10-23 20:30 +0200
    Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port Heiko Stübner <heiko@sntech.de> - 2015-10-24 14:30 +0200
    Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host"  port Rob Herring <robh@kernel.org> - 2015-10-24 18:40 +0200
      Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port Doug Anderson <dianders@chromium.org> - 2015-10-24 23:30 +0200
        Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port Rob Herring <robh@kernel.org> - 2015-10-27 00:10 +0100
          Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port Doug Anderson <dianders@chromium.org> - 2015-10-27 01:00 +0100
            Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port Doug Anderson <dianders@chromium.org> - 2015-10-27 02:50 +0100

#1254863 — [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port

The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
has a hardware errata that causes everything to get confused when we get
a remote wakeup.  It appears that the "port reset" bit that's in the USB
phy (located in the rk3288 GRF) fixes things up and appears safe to do.

This series of patches exports the "port reset" from the PHY and then
hooks it up to dwc2 through a quirk.

I've tested this series atop a bit of a conglomeration of Heiko's github
"somewhat stable" branch (v4.3-rc3-876-g6509232) but with Greg KH's
usb-next merged in.

These patches currently conflict with patches that I posted previously
to enable USB wakeup from S3, specifically:
* https://patchwork.kernel.org/patch/6727081/
* https://patchwork.kernel.org/patch/6727121/
...those patches no longer apply anyway, so presumably they need to be
reposted and I can do so later atop these patches.


Douglas Anderson (4):
  phy: rockchip-usb: Support the PHY's "port reset"
  usb: dwc2: optionally assert phy "port reset" when waking up
  ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288
  ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset

 .../devicetree/bindings/phy/rockchip-usb-phy.txt   |  6 ++
 Documentation/devicetree/bindings/usb/dwc2.txt     |  7 ++
 arch/arm/boot/dts/rk3288.dtsi                      |  8 +++
 drivers/phy/phy-rockchip-usb.c                     | 74 ++++++++++++++++++++++
 drivers/usb/dwc2/core.h                            |  5 ++
 drivers/usb/dwc2/core_intr.c                       |  7 ++
 drivers/usb/dwc2/platform.c                        | 13 ++++
 7 files changed, 120 insertions(+)

-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1254871 — [PATCH 2/4] usb: dwc2: optionally assert phy "port reset" when waking up

On the rk3288 USB host-only port (the one that's not the OTG-enabled
port) the phy can get into a bad state when a wakeup is asserted (not
just a wakeup from full system suspend but also a wakeup from
autosuspend).  Asserting the "port reset" bit in the PHY upon wakeup
seems to resolve the issue.

Here we add the ability to specify the PHY "port reset" source to dwc2
and add a quirk property that says that we should assert the PHY "port
reset" source when we get a wakeup.

See the patch ("phy: rockchip-usb: Support the PHY's "port reset"") for
some details on the reset that we plan to use.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
---
 Documentation/devicetree/bindings/usb/dwc2.txt |  7 +++++++
 drivers/usb/dwc2/core.h                        |  5 +++++
 drivers/usb/dwc2/core_intr.c                   |  7 +++++++
 drivers/usb/dwc2/platform.c                    | 13 +++++++++++++
 4 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index fd132cb..36c4919 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -17,6 +17,13 @@ Refer to clk/clock-bindings.txt for generic clock consumer properties
 Optional properties:
 - phys: phy provider specifier
 - phy-names: shall be "usb2-phy"
+- snps,need-phy-port-reset-on-wake: if present indicates that we need to assert
+  the PHY "port reset" when we detect a wakeup due to a hardware errata.  If
+  present you must specify a "phy-port-reset" reset.
+
+Resets:
+- phy-port-reset (optional): Asserts the PHY's "port reset".
+
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index a66d3cb..5cd58ab 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -582,8 +582,11 @@ struct dwc2_hregs_backup {
  * @hcd_enabled		Host mode sub-driver initialization indicator.
  * @gadget_enabled	Peripheral mode sub-driver initialization indicator.
  * @ll_hw_enabled	Status of low-level hardware resources.
+ * @need_phy_port_reset_on_wake: Quirk saying that we should assert
+ *                               phy_port_reset on a remote wakeup.
  * @phy:                The otg phy transceiver structure for phy control.
  * @uphy:               The otg phy transceiver structure for old USB phy control.
+ * @phy_port_reset:     Reset control for the PHY's "port reset".
  * @plat:               The platform specific configuration data. This can be removed once
  *                      all SoCs support usb transceiver.
  * @supplies:           Definition of USB power supplies
@@ -710,9 +713,11 @@ struct dwc2_hsotg {
 	unsigned int hcd_enabled:1;
 	unsigned int gadget_enabled:1;
 	unsigned int ll_hw_enabled:1;
+	unsigned int need_phy_port_reset_on_wake:1;
 
 	struct phy *phy;
 	struct usb_phy *uphy;
+	struct reset_control *phy_port_reset;
 	struct dwc2_hsotg_plat *plat;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
 	u32 phyif;
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 27daa42..52e22a5 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -45,6 +45,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/reset.h>
 #include <linux/usb.h>
 
 #include <linux/usb/hcd.h>
@@ -378,6 +379,12 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 			/* Restart the Phy Clock */
 			pcgcctl &= ~PCGCTL_STOPPCLK;
 			dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
+
+			if (hsotg->need_phy_port_reset_on_wake) {
+				reset_control_assert(hsotg->phy_port_reset);
+				reset_control_deassert(hsotg->phy_port_reset);
+			}
+
 			mod_timer(&hsotg->wkp_timer,
 				  jiffies + msecs_to_jiffies(71));
 		} else {
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 5859b0f..2f7b5d0 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -45,6 +45,7 @@
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_data/s3c-hsotg.h>
+#include <linux/reset.h>
 
 #include <linux/usb/of.h>
 
@@ -376,6 +377,18 @@ static int dwc2_driver_probe(struct platform_device *dev)
 			"Configuration mismatch. Forcing peripheral mode\n");
 	}
 
+	hsotg->need_phy_port_reset_on_wake =
+		of_property_read_bool(dev->dev.of_node,
+				      "snps,need-phy-port-reset-on-wake");
+	hsotg->phy_port_reset = devm_reset_control_get(hsotg->dev,
+						       "phy-port-reset");
+	if (IS_ERR(hsotg->phy_port_reset) &&
+	    hsotg->need_phy_port_reset_on_wake) {
+		dev_warn(hsotg->dev, "Missing phy port reset (%ld); skipping\n",
+			 PTR_ERR(hsotg->phy_port_reset));
+		hsotg->need_phy_port_reset_on_wake = false;
+	}
+
 	retval = dwc2_lowlevel_hw_init(hsotg);
 	if (retval)
 		return retval;
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1254872 — [PATCH 4/4] ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset

The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
has a hardware errata that causes everything to get confused when we get
a remote wakeup.  It appears that the "port reset" bit that's in the USB
phy (located in the rk3288 GRF) fixes things up and appears safe to do.

We recently added code to the PHY to expose this reset and code to dwc2
to use it, so now let's hook things up.

Note that we add the PHY port reset to both dwc2 controllers even though
only one has the errata in case we find some other use for this reset
that's unrelated to the current hardware errata.  Only the host port
gets the quirk property, though.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 arch/arm/boot/dts/rk3288.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 4f76805..03de41d 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -496,6 +496,9 @@
 		dr_mode = "host";
 		phys = <&usbphy2>;
 		phy-names = "usb2-phy";
+		resets = <&usbphy2>;
+		reset-names = "phy-port-reset";
+		snps,need-phy-port-reset-on-wake;
 		status = "disabled";
 	};
 
@@ -513,6 +516,8 @@
 		g-use-dma;
 		phys = <&usbphy0>;
 		phy-names = "usb2-phy";
+		resets = <&usbphy0>;
+		reset-names = "phy-port-reset";
 		status = "disabled";
 	};
 
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1254880 — [PATCH 3/4] ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288

As per the change to the rk3288 USB phy driver, we can now enable the
PHYs as reset providers.  Do so.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 arch/arm/boot/dts/rk3288.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 906e938..4f76805 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -778,6 +778,7 @@
 
 		usbphy0: usb-phy0 {
 			#phy-cells = <0>;
+			#reset-cells = <0>;
 			reg = <0x320>;
 			clocks = <&cru SCLK_OTGPHY0>;
 			clock-names = "phyclk";
@@ -785,6 +786,7 @@
 
 		usbphy1: usb-phy1 {
 			#phy-cells = <0>;
+			#reset-cells = <0>;
 			reg = <0x334>;
 			clocks = <&cru SCLK_OTGPHY1>;
 			clock-names = "phyclk";
@@ -792,6 +794,7 @@
 
 		usbphy2: usb-phy2 {
 			#phy-cells = <0>;
+			#reset-cells = <0>;
 			reg = <0x348>;
 			clocks = <&cru SCLK_OTGPHY2>;
 			clock-names = "phyclk";
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255173

Am Freitag, 23. Oktober 2015, 11:28:07 schrieb Douglas Anderson:
> The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
> has a hardware errata that causes everything to get confused when we get
> a remote wakeup.  It appears that the "port reset" bit that's in the USB
> phy (located in the rk3288 GRF) fixes things up and appears safe to do.
> 
> This series of patches exports the "port reset" from the PHY and then
> hooks it up to dwc2 through a quirk.
> 
> I've tested this series atop a bit of a conglomeration of Heiko's github
> "somewhat stable" branch (v4.3-rc3-876-g6509232) but with Greg KH's
> usb-next merged in.

I've been involved in the earlier revisions already, so this version already 
implements the review-comments I had. I've also gave this a spin on 4.3-rc6 
with usb-next merged in on a rk3288-firefly, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>


If the first two patches get picked up, I'll pick the two dts patches 
afterwards.


Thanks
Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255226 — Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port

On 10/23/2015 01:28 PM, Douglas Anderson wrote:
> The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
> has a hardware errata that causes everything to get confused when we get
> a remote wakeup.  It appears that the "port reset" bit that's in the USB
> phy (located in the rk3288 GRF) fixes things up and appears safe to do.
> 
> This series of patches exports the "port reset" from the PHY and then
> hooks it up to dwc2 through a quirk.
> 
> I've tested this series atop a bit of a conglomeration of Heiko's github
> "somewhat stable" branch (v4.3-rc3-876-g6509232) but with Greg KH's
> usb-next merged in.
> 
> These patches currently conflict with patches that I posted previously
> to enable USB wakeup from S3, specifically:
> * https://patchwork.kernel.org/patch/6727081/
> * https://patchwork.kernel.org/patch/6727121/
> ...those patches no longer apply anyway, so presumably they need to be
> reposted and I can do so later atop these patches.
> 
> 
> Douglas Anderson (4):
>   phy: rockchip-usb: Support the PHY's "port reset"
>   usb: dwc2: optionally assert phy "port reset" when waking up
>   ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288
>   ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset
> 
>  .../devicetree/bindings/phy/rockchip-usb-phy.txt   |  6 ++
>  Documentation/devicetree/bindings/usb/dwc2.txt     |  7 ++
>  arch/arm/boot/dts/rk3288.dtsi                      |  8 +++
>  drivers/phy/phy-rockchip-usb.c                     | 74 ++++++++++++++++++++++
>  drivers/usb/dwc2/core.h                            |  5 ++
>  drivers/usb/dwc2/core_intr.c                       |  7 ++
>  drivers/usb/dwc2/platform.c                        | 13 ++++
>  7 files changed, 120 insertions(+)

A DT reset controller seems like a bit of an overkill here. I think this
would be much more simple if we just add a phy reset hook to the phy
subsystem.

Rob

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255248

Rob,

On Sat, Oct 24, 2015 at 11:10 AM, Rob Herring <robh@kernel.org> wrote:
> On 10/23/2015 01:28 PM, Douglas Anderson wrote:
>> The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
>> has a hardware errata that causes everything to get confused when we get
>> a remote wakeup.  It appears that the "port reset" bit that's in the USB
>> phy (located in the rk3288 GRF) fixes things up and appears safe to do.
>>
>> This series of patches exports the "port reset" from the PHY and then
>> hooks it up to dwc2 through a quirk.
>>
>> I've tested this series atop a bit of a conglomeration of Heiko's github
>> "somewhat stable" branch (v4.3-rc3-876-g6509232) but with Greg KH's
>> usb-next merged in.
>>
>> These patches currently conflict with patches that I posted previously
>> to enable USB wakeup from S3, specifically:
>> * https://patchwork.kernel.org/patch/6727081/
>> * https://patchwork.kernel.org/patch/6727121/
>> ...those patches no longer apply anyway, so presumably they need to be
>> reposted and I can do so later atop these patches.
>>
>>
>> Douglas Anderson (4):
>>   phy: rockchip-usb: Support the PHY's "port reset"
>>   usb: dwc2: optionally assert phy "port reset" when waking up
>>   ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288
>>   ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset
>>
>>  .../devicetree/bindings/phy/rockchip-usb-phy.txt   |  6 ++
>>  Documentation/devicetree/bindings/usb/dwc2.txt     |  7 ++
>>  arch/arm/boot/dts/rk3288.dtsi                      |  8 +++
>>  drivers/phy/phy-rockchip-usb.c                     | 74 ++++++++++++++++++++++
>>  drivers/usb/dwc2/core.h                            |  5 ++
>>  drivers/usb/dwc2/core_intr.c                       |  7 ++
>>  drivers/usb/dwc2/platform.c                        | 13 ++++
>>  7 files changed, 120 insertions(+)
>
> A DT reset controller seems like a bit of an overkill here. I think this
> would be much more simple if we just add a phy reset hook to the phy
> subsystem.

Adding a reset hook in the PHY subsystem does seem like a reasonable
idea to me.  I was considering it in an earlier version of this series
that actually used a reset of the PHY to the fix the stuck dwc2.

...but I think that even if the phy subsystem had a reset hook it
wouldn't be the ideal solution here.  When we assert the PHY "port
reset" we're not actually fully resetting the PHY.  We're instead
doing some sort of a more minor "state machine" reset in the PHY.
This appears better (in my case) than resetting the whole PHY because
it doesn't force a de-enumeration / re-enumeration.  Exposing this
more minor reset as a PHY reset seems wrong to me.  ...and it also
precludes us later also exposing the more full reset through the PHY
framework if that later becomes useful.

...we could, of course, re-invent the reset framework (with string or
integral IDs so we can assert different types of resets) within the
PHY framework.  That doesn't seem ideal to me, but if that's what
others want to do then I guess it would be OK...

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256337

On Sat, Oct 24, 2015 at 4:22 PM, Doug Anderson <dianders@chromium.org> wrote:
> Rob,
>
> On Sat, Oct 24, 2015 at 11:10 AM, Rob Herring <robh@kernel.org> wrote:
>> On 10/23/2015 01:28 PM, Douglas Anderson wrote:
>>> The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
>>> has a hardware errata that causes everything to get confused when we get
>>> a remote wakeup.  It appears that the "port reset" bit that's in the USB
>>> phy (located in the rk3288 GRF) fixes things up and appears safe to do.
>>>
>>> This series of patches exports the "port reset" from the PHY and then
>>> hooks it up to dwc2 through a quirk.
>>>
>>> I've tested this series atop a bit of a conglomeration of Heiko's github
>>> "somewhat stable" branch (v4.3-rc3-876-g6509232) but with Greg KH's
>>> usb-next merged in.
>>>
>>> These patches currently conflict with patches that I posted previously
>>> to enable USB wakeup from S3, specifically:
>>> * https://patchwork.kernel.org/patch/6727081/
>>> * https://patchwork.kernel.org/patch/6727121/
>>> ...those patches no longer apply anyway, so presumably they need to be
>>> reposted and I can do so later atop these patches.
>>>
>>>
>>> Douglas Anderson (4):
>>>   phy: rockchip-usb: Support the PHY's "port reset"
>>>   usb: dwc2: optionally assert phy "port reset" when waking up
>>>   ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288
>>>   ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset
>>>
>>>  .../devicetree/bindings/phy/rockchip-usb-phy.txt   |  6 ++
>>>  Documentation/devicetree/bindings/usb/dwc2.txt     |  7 ++
>>>  arch/arm/boot/dts/rk3288.dtsi                      |  8 +++
>>>  drivers/phy/phy-rockchip-usb.c                     | 74 ++++++++++++++++++++++
>>>  drivers/usb/dwc2/core.h                            |  5 ++
>>>  drivers/usb/dwc2/core_intr.c                       |  7 ++
>>>  drivers/usb/dwc2/platform.c                        | 13 ++++
>>>  7 files changed, 120 insertions(+)
>>
>> A DT reset controller seems like a bit of an overkill here. I think this
>> would be much more simple if we just add a phy reset hook to the phy
>> subsystem.
>
> Adding a reset hook in the PHY subsystem does seem like a reasonable
> idea to me.  I was considering it in an earlier version of this series
> that actually used a reset of the PHY to the fix the stuck dwc2.
>
> ...but I think that even if the phy subsystem had a reset hook it
> wouldn't be the ideal solution here.  When we assert the PHY "port
> reset" we're not actually fully resetting the PHY.  We're instead
> doing some sort of a more minor "state machine" reset in the PHY.
> This appears better (in my case) than resetting the whole PHY because
> it doesn't force a de-enumeration / re-enumeration.  Exposing this
> more minor reset as a PHY reset seems wrong to me.  ...and it also
> precludes us later also exposing the more full reset through the PHY
> framework if that later becomes useful.

Doesn't creating a binding also have similar possibility? Maybe an
"attach host" or re-init hook would be more appropriate. I'm sure
there are other such cases where the host and phy need more tight
coupling. I recently had a similar case where there was an interleaved
sequence of phy and host register writes. I managed rework things and
avoid changing the phy interface, but there will certainly be cases
where we can't.

Changing function names in the kernel is easy. Changing the binding
later not so much. Also as we're looking toward dependency handling,
this creates a circular dependency. We'll probably have to deal with
those anyway, but here it seems a bit pointless. We already have a
connection between the host and phy defined. Let's use that and not
define another.

> ...we could, of course, re-invent the reset framework (with string or
> integral IDs so we can assert different types of resets) within the
> PHY framework.  That doesn't seem ideal to me, but if that's what
> others want to do then I guess it would be OK...

I think that should already be possible as you can have multiple
cells. Of course, you have to plan for that with your cell values.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256351

Rob,

On Mon, Oct 26, 2015 at 4:05 PM, Rob Herring <robh@kernel.org> wrote:
>>> A DT reset controller seems like a bit of an overkill here. I think this
>>> would be much more simple if we just add a phy reset hook to the phy
>>> subsystem.
>>
>> Adding a reset hook in the PHY subsystem does seem like a reasonable
>> idea to me.  I was considering it in an earlier version of this series
>> that actually used a reset of the PHY to the fix the stuck dwc2.
>>
>> ...but I think that even if the phy subsystem had a reset hook it
>> wouldn't be the ideal solution here.  When we assert the PHY "port
>> reset" we're not actually fully resetting the PHY.  We're instead
>> doing some sort of a more minor "state machine" reset in the PHY.
>> This appears better (in my case) than resetting the whole PHY because
>> it doesn't force a de-enumeration / re-enumeration.  Exposing this
>> more minor reset as a PHY reset seems wrong to me.  ...and it also
>> precludes us later also exposing the more full reset through the PHY
>> framework if that later becomes useful.
>
> Doesn't creating a binding also have similar possibility? Maybe an
> "attach host" or re-init hook would be more appropriate. I'm sure
> there are other such cases where the host and phy need more tight
> coupling. I recently had a similar case where there was an interleaved
> sequence of phy and host register writes. I managed rework things and
> avoid changing the phy interface, but there will certainly be cases
> where we can't.
>
> Changing function names in the kernel is easy. Changing the binding
> later not so much. Also as we're looking toward dependency handling,
> this creates a circular dependency. We'll probably have to deal with
> those anyway, but here it seems a bit pointless. We already have a
> connection between the host and phy defined. Let's use that and not
> define another.

I'm probably not understanding what you're proposing.  I was imagining
that you were proposing adding into "include/linux/phy/phy.h" a
definition like:

  int phy_reset(struct phy *phy);

If that existed in the PHY interface, it wouldn't be enough for me
since the "reset" that I'm doing isn't a full reset (and there does
exist another, fuller PHY reset on this SoC).  So if we wanted to use
the PHY interface, I'd need either:

  int phy_reset(struct phy *phy, int reset_id);
...or...
  int phy_reset(struct phy *phy, const char *reset_id);

I was expecting that to stay generic you'd somehow need to add a
mapping of these resets into the device tree because a given USB
controller may have different PHYs on different boards and the same
PHY might be used with more than one USB controller.  If you don't add
some type of mapping in the device tree then in _some_ type of include
file you'd need something like:

#define DWC2_RESET_ID_PHY_PORT_RESET  1
#define DWC2_RESET_ID_PHY_FULL_RESET  2
...or...
#define DWC2_RESET_ID_PHY_PORT_RESET  "phy_port_reset"
#define DWC2_RESET_ID_PHY_FULL_RESET  "phy_full_reset"

...presumably you'd expect that to be in a dwc2 header file, to be
included by the "rk3288 USB PHY"?  ...but, oddly enough, the "rk3288
USB PHY" is not specific to dwc2, so having it include a dwc2 header
file is pretty odd.  Specifically on rk3288 there are 3 instances of
the same PHY.  One is hooked up to the dwc2 "OTG" port.  One is hooked
up to the dwc2 "host" port (which doesn't have device functionality
and also has the need for this strange reset quirk).  One is hooked up
to an EHCI port (using the generic EHCI driver).

...so, while I could add a dwc2 specific include into the rk3288 PHY
driver, I'd need to somehow figure out which of the PHYs it applied
to.  For instance, maybe we later find that we need to activate this
reset for the EHCI port.  Now we'll need to include both headers.
...and hopefully there aren't name / number conflicts.

One note: the "full" PHY reset is actually not in the register map of
the PHY.  It is amazingly enough in the CRU (clock reset unit).  So if
we actually exposed the "full" reset through the PHY framework, the
PHY would then turn around and pass it off to the reset framework,
which is how the full PHY reset is exposed from the CRU.


>> ...we could, of course, re-invent the reset framework (with string or
>> integral IDs so we can assert different types of resets) within the
>> PHY framework.  That doesn't seem ideal to me, but if that's what
>> others want to do then I guess it would be OK...
>
> I think that should already be possible as you can have multiple
> cells. Of course, you have to plan for that with your cell values.

I didn't understand this comment.


Basically: to summarize, I think that we have a reset framework that
handles this beautifully and gets all the corner cases down.  Adding a
second link to the PHY seems totally reasonable to me because I could
totally imagine that this reset could be implemented in a totally
different place in some SoCs.  It happens to be in the PHY in my
particular case, but it need not be on all SoCs.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256390

Hi,

On Mon, Oct 26, 2015 at 4:49 PM, Doug Anderson <dianders@chromium.org> wrote:
> One note: the "full" PHY reset is actually not in the register map of
> the PHY.  It is amazingly enough in the CRU (clock reset unit).  So if
> we actually exposed the "full" reset through the PHY framework, the
> PHY would then turn around and pass it off to the reset framework,
> which is how the full PHY reset is exposed from the CRU.

Interestingly enough, it looks like there's a reasonable chance that
we won't be able to use the PHY port reset.  We might have to go back
to the full PHY reset (though it causes de-enumeration and
re-enumeration) for safety, since there might be some side effects of
the "phy port reset".

Probably shouldn't land this series until we can figure that out.
...but in that case we still have the question of how we should export
it.  It would certainly be easy to keep using the reset framework...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web