Discussion:
[PATCH 3/4] ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288
Douglas Anderson
2015-10-23 18:28:10 UTC
Permalink
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 <***@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
Douglas Anderson
2015-10-23 18:28:08 UTC
Permalink
We'd like to be able to expose the USB PHYs as reset providers. They
can provide one reset that is named in the TRM as a "port reset".

The name in the TRM is a bit confusing since there's a bit in dwc2's
"hprt0" register that's also called "port reset" and this appears to
reset something different, but it's still interesting to expose this one
so we can actually use it.

The TRM gives a little details about the reset that's in the PHY. It
says this will "reset the port transmit and receive logic without
disabling the clocks within the PHY." It goes on to say that the
"transmit and receive finite state machines are reset and the line_state
logic combinatorially reflects the state of the single-ended receivers."

It's expected that will add code to dwc2 in a future patch that will let
dwc2 access this reset. This reset seems to have the ability to unwedge
the dwc2 "host" port when a remote wakeup happens. It may have other
redeeming qualities as well.

Signed-off-by: Douglas Anderson <***@chromium.org>
---
.../devicetree/bindings/phy/rockchip-usb-phy.txt | 6 ++
drivers/phy/phy-rockchip-usb.c | 74 ++++++++++++++++++++++
2 files changed, 80 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
index 826454a..746c035 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
@@ -21,6 +21,11 @@ required properties:
Optional Properties:
- clocks : phandle + clock specifier for the phy clocks
- clock-names: string, clock name, must be "phyclk"
+- #reset-cells: adding this property allows the phy to be
+ specified as a reset source. Asserting this reset will
+ assert the PHY's "port reset" bit. Always set reset-cells
+ to 0. Each PHY only has one reset to provide so no ID is
+ needed.

Example:

@@ -32,6 +37,7 @@ usbphy: phy {

usbphy0: usb-phy0 {
#phy-cells = <0>;
+ #reset-cells = <0>;
reg = <0x320>;
};
};
diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
index 91d6f34..0e17677 100644
--- a/drivers/phy/phy-rockchip-usb.c
+++ b/drivers/phy/phy-rockchip-usb.c
@@ -15,6 +15,7 @@
*/

#include <linux/clk.h>
+#include <linux/delay.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -25,6 +26,7 @@
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
+#include <linux/reset-controller.h>
#include <linux/regmap.h>
#include <linux/mfd/syscon.h>

@@ -36,11 +38,72 @@
#define SIDDQ_ON BIT(13)
#define SIDDQ_OFF (0 << 13)

+#define PORT_RESET_WRITE_ENA BIT(12 + 16)
+#define PORT_RESET_ON BIT(12)
+#define PORT_RESET_OFF (0 << 12)
+
struct rockchip_usb_phy {
unsigned int reg_offset;
struct regmap *reg_base;
struct clk *clk;
struct phy *phy;
+ struct reset_controller_dev rcdev;
+};
+
+static int rockchip_usb_phy_port_reset_on(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ int ret;
+
+ struct rockchip_usb_phy *phy =
+ container_of(rcdev, struct rockchip_usb_phy, rcdev);
+
+ ret = regmap_write(phy->reg_base, phy->reg_offset,
+ PORT_RESET_WRITE_ENA | PORT_RESET_ON);
+
+ /*
+ * The TRM says nothing about how long we need to reset for, but
+ * it seems to work with very little delay.
+ */
+ udelay(1);
+
+ return ret;
+}
+
+static int rockchip_usb_phy_port_reset_off(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ int ret;
+
+ struct rockchip_usb_phy *phy =
+ container_of(rcdev, struct rockchip_usb_phy, rcdev);
+
+ ret = regmap_write(phy->reg_base, phy->reg_offset,
+ PORT_RESET_WRITE_ENA | PORT_RESET_OFF);
+
+ /*
+ * TRM says we should wait 11 PHYCLOCK cycles after deasserting reset
+ * but doesn't say what PHYCLOCK is. Even if it was as slow as 12MHz
+ * that would only be 917 ns though, so we'll delay 1us which should be
+ * total overkill but shouldn't hurt.
+ */
+ udelay(1);
+
+ return ret;
+}
+
+static int rockchip_usb_phy_reset_xlate(struct reset_controller_dev *rcdev,
+ const struct of_phandle_args *reset_spec)
+{
+ if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
+ return -EINVAL;
+
+ return 0;
+}
+
+static struct reset_control_ops rockchip_usb_phy_port_reset_ops = {
+ .assert = rockchip_usb_phy_port_reset_on,
+ .deassert = rockchip_usb_phy_port_reset_off,
};

static int rockchip_usb_phy_power(struct rockchip_usb_phy *phy,
@@ -135,6 +198,17 @@ static int rockchip_usb_phy_probe(struct platform_device *pdev)
err = rockchip_usb_phy_power(rk_phy, 1);
if (err)
return err;
+
+ rk_phy->rcdev.owner = THIS_MODULE;
+ rk_phy->rcdev.nr_resets = 1;
+ rk_phy->rcdev.ops = &rockchip_usb_phy_port_reset_ops;
+ rk_phy->rcdev.of_node = child;
+ rk_phy->rcdev.of_xlate = rockchip_usb_phy_reset_xlate;
+
+ err = reset_controller_register(&rk_phy->rcdev);
+ if (err)
+ dev_warn(dev, "Register reset failed (%d); skipping\n",
+ err);
}

phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
--
2.6.0.rc2.230.g3dd15c0
Douglas Anderson
2015-10-23 18:28:11 UTC
Permalink
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 <***@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
Heiko Stübner
2015-10-24 12:26:21 UTC
Permalink
Post by 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 <***@sntech.de>
Tested-by: Heiko Stuebner <***@sntech.de>


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


Thanks
Heiko
Rob Herring
2015-10-24 15:10:23 UTC
Permalink
Post by 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.
These patches currently conflict with patches that I posted previously
* 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.
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
Doug Anderson
2015-10-24 21:22:22 UTC
Permalink
Rob,
Post by Rob Herring
Post by 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.
These patches currently conflict with patches that I posted previously
* 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.
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
Rob Herring
2015-10-26 23:05:04 UTC
Permalink
Post by Doug Anderson
Rob,
Post by Rob Herring
Post by 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.
These patches currently conflict with patches that I posted previously
* 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.
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.
Post by Doug Anderson
...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
Doug Anderson
2015-10-26 23:49:52 UTC
Permalink
Rob,
Post by Rob Herring
Post by Doug Anderson
Post by Rob Herring
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.
Post by Rob Herring
Post by Doug Anderson
...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
Doug Anderson
2015-10-27 01:43:00 UTC
Permalink
Hi,
Post by Doug Anderson
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...
Loading...