[PATCH 0/3] Add and export clk-480m clocks for ehci and ohci on RK3399

[PATCH 0/3] Add and export clk-480m clocks for ehci and ohci on RK3399

[Xing Zheng]

Hi,
  This patches would like to fix the USB suspend block without
the clk-480m clock. Let's add and export them to control them.

Thanks.


William wu (1):
  arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

Xing Zheng (2):
  clk: rockchip: rk3399: add USBPHYx_480M_SRC clock IDs
  clk: rockchip: rk3399: export 480M_SRC clocks id for usbphy0/usbphy1

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 28 ++++++++++++++++++++--------
 drivers/clk/rockchip/clk-rk3399.c        |  4 ++--
 include/dt-bindings/clock/rk3399-cru.h   |  2 ++
 3 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/3] clk: rockchip: rk3399: add USBPHYx_480M_SRC clock IDs

[Xing Zheng]

This patch add two clock IDs for the usb phy 480m source clocks.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

 include/dt-bindings/clock/rk3399-cru.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/dt-bindings/clock/rk3399-cru.h b/include/dt-bindings/clock/rk3399-cru.h
index 220a60f..224daf7 100644
--- a/include/dt-bindings/clock/rk3399-cru.h
+++ b/include/dt-bindings/clock/rk3399-cru.h
@@ -132,6 +132,8 @@
 #define SCLK_RMII_SRC			166
 #define SCLK_PCIEPHY_REF100M		167
 #define SCLK_DDRC			168
+#define SCLK_USBPHY0_480M_SRC		169
+#define SCLK_USBPHY1_480M_SRC		170
 
 #define DCLK_VOP0			180
 #define DCLK_VOP1			181
-- 
2.7.4

Re: [PATCH 1/3] clk: rockchip: rk3399: add USBPHYx_480M_SRC clock IDs

[Doug Anderson]

Hi,

On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> This patch add two clock IDs for the usb phy 480m source clocks.
>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
>
>  include/dt-bindings/clock/rk3399-cru.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/dt-bindings/clock/rk3399-cru.h b/include/dt-bindings/clock/rk3399-cru.h
> index 220a60f..224daf7 100644
> --- a/include/dt-bindings/clock/rk3399-cru.h
> +++ b/include/dt-bindings/clock/rk3399-cru.h
> @@ -132,6 +132,8 @@
>  #define SCLK_RMII_SRC                  166
>  #define SCLK_PCIEPHY_REF100M           167
>  #define SCLK_DDRC                      168
> +#define SCLK_USBPHY0_480M_SRC          169
> +#define SCLK_USBPHY1_480M_SRC          170

As mentioned in the dts patch, I don't think you need these since I'm
under the impression that nobody gets this clock.  I think the USB
Controller get the ungated version of this clock.

-Doug

[PATCH 2/3] clk: rockchip: rk3399: export 480M_SRC clocks id for usbphy0/usbphy1

[Xing Zheng]

This patch exports USBPHYx_480M_SRC clocks for usbphy.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

 drivers/clk/rockchip/clk-rk3399.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 3490887..cf2af4c 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -411,9 +411,9 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 	GATE(SCLK_USB2PHY1_REF, "clk_usb2phy1_ref", "xin24m", CLK_IGNORE_UNUSED,
 			RK3399_CLKGATE_CON(6), 6, GFLAGS),
 
-	GATE(0, "clk_usbphy0_480m_src", "clk_usbphy0_480m", 0,
+	GATE(SCLK_USBPHY0_480M_SRC, "clk_usbphy0_480m_src", "clk_usbphy0_480m", 0,
 			RK3399_CLKGATE_CON(13), 12, GFLAGS),
-	GATE(0, "clk_usbphy1_480m_src", "clk_usbphy1_480m", 0,
+	GATE(SCLK_USBPHY1_480M_SRC, "clk_usbphy1_480m_src", "clk_usbphy1_480m", 0,
 			RK3399_CLKGATE_CON(13), 12, GFLAGS),
 	MUX(0, "clk_usbphy_480m", mux_usbphy_480m_p, 0,
 			RK3399_CLKSEL_CON(14), 6, 1, MFLAGS),
-- 
2.7.4

Re: [PATCH 2/3] clk: rockchip: rk3399: export 480M_SRC clocks id for usbphy0/usbphy1

[Doug Anderson]

Hi,

On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> This patch exports USBPHYx_480M_SRC clocks for usbphy.
>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
>
>  drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

As mentioned in the dts patch, I don't think you need these since I'm
under the impression that nobody gets this clock.  I think the USB
Controller get the ungated version of this clock.

-Doug

[PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Xing Zheng]

From: William wu <wulf@rock-chips.com>

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.

The root cause is that usb2-phy suspended earlier than ehci/ohci
(usb2-phy will be auto suspended if no devices plug-in). and the
clk-480m provided by it was disabled if no module used. However,
some suspend process related ehci/ohci are base on this clock,
so we should refer it into ehci/ohci driver to prevent this case.

Signed-off-by: William wu <wulf@rock-chips.com>
Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index b65c193..228c764 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -315,8 +315,10 @@
 		compatible = "generic-ehci";
 		reg = <0x0 0xfe380000 0x0 0x20000>;
 		interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
-		clock-names = "hclk_host0", "hclk_host0_arb";
+		clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
+			 <&cru SCLK_USBPHY0_480M_SRC>;
+		clock-names = "hclk_host0", "hclk_host0_arb",
+			      "usbphy0_480m";
 		phys = <&u2phy0_host>;
 		phy-names = "usb";
 		status = "disabled";
@@ -326,8 +328,12 @@
 		compatible = "generic-ohci";
 		reg = <0x0 0xfe3a0000 0x0 0x20000>;
 		interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
-		clock-names = "hclk_host0", "hclk_host0_arb";
+		clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
+			 <&cru SCLK_USBPHY0_480M_SRC>;
+		clock-names = "hclk_host0", "hclk_host0_arb",
+			      "usbphy0_480m";
+		phys = <&u2phy0_host>;
+		phy-names = "usb";
 		status = "disabled";
 	};
 
@@ -335,8 +341,10 @@
 		compatible = "generic-ehci";
 		reg = <0x0 0xfe3c0000 0x0 0x20000>;
 		interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>;
-		clock-names = "hclk_host1", "hclk_host1_arb";
+		clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>,
+			 <&cru SCLK_USBPHY1_480M_SRC>;
+		clock-names = "hclk_host1", "hclk_host1_arb",
+			      "usbphy1_480m";
 		phys = <&u2phy1_host>;
 		phy-names = "usb";
 		status = "disabled";
@@ -346,8 +354,12 @@
 		compatible = "generic-ohci";
 		reg = <0x0 0xfe3e0000 0x0 0x20000>;
 		interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>;
-		clock-names = "hclk_host1", "hclk_host1_arb";
+		clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>,
+			 <&cru SCLK_USBPHY1_480M_SRC>;
+		clock-names = "hclk_host1", "hclk_host1_arb",
+			      "usbphy1_480m";
+		phys = <&u2phy1_host>;
+		phy-names = "usb";
 		status = "disabled";
 	};
 
-- 
2.7.4

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Doug Anderson]

Hi,

On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> From: William wu <wulf@rock-chips.com>
>
> We found that the suspend process was blocked when it run into
> ehci/ohci module due to clk-480m of usb2-phy was disabled.
>
> The root cause is that usb2-phy suspended earlier than ehci/ohci
> (usb2-phy will be auto suspended if no devices plug-in).

This is really weird, but I can confirm it is true on my system too
(kernel-4.4 based).  At least I see:

[  208.012065] calling  usb1+ @ 4984, parent: fe380000.usb, cb: usb_dev_suspend
[  208.569112] calling  ff770000.syscon:usb2-phy@e450+ @ 4983, parent:
ff770000.syscon, cb: platform_pm_suspend
[  208.569113] call ff770000.syscon:usb2-phy@e450+ returned 0 after 0 usecs
[  208.569439] calling  fe380000.usb+ @ 4983, parent: platform, cb:
platform_pm_suspend
[  208.569444] call fe380000.usb+ returned 0 after 4 usecs


In general I thought that suspend order was supposed to be related to
probe order.  So if your probe order is A, B, C then your suspend
order would be C, B, A.  ...and we know for sure that the USB PHY
needs to probe _before_ the main USB controller.  If it didn't then
you'd get an EPROBE_DEFER in the USB controller, right?  So that means
that the USB controller should be suspending before its PHY.

Any chance this is somehow related to async probe?  I'm not a huge
expert on async probe but I guess I could imagine things getting
confused if you had a sequence like this:

1. Start USB probe (async)
2. Start PHY probe
3. Finish PHY probe
4. In USB probe, ask for PHY--no problems since PHY probe finished
5. Finish USB probe

The probe order would be USB before PHY even though the USB probe
_depended_ on the PHY probe being finished...  :-/  Anyway, probably
I'm just misunderstanding something and someone can tell me how dumb I
am...

I also notice that the ehci_platform_power_off() function we're
actually making PHY commands right before the same commands that turn
off our clocks.  Presumably those commands aren't really so good to do
if the PHY has already been suspended?

Actually, does the PHY suspend from platform_pm_suspend() actually
even do anything?  It doesn't look like it.  It looks as if all the
PHY cares about is init/exit and on/off...  ...and it looks as if the
PHY should be turned off by the EHCI controller at about the same time
it turns off its clocks...

I haven't fully dug, but is there any chance that things are getting
confused between the OTG PHY and the Host PHY?  Maybe when we turn off
the OTG PHY it turns off something that the host PHY needs?


> and the
> clk-480m provided by it was disabled if no module used. However,
> some suspend process related ehci/ohci are base on this clock,
> so we should refer it into ehci/ohci driver to prevent this case.

Though I don't actually have details about the internals of the chip,
it does seem highly likely that the USB block actually uses this clock
for some things, so it doesn't seem insane (to me) to have the USB
controller request that the clock be on.  So, in general, I don't have
lots of objections to including the USB PHY Clock here.

...but I think you have the wrong clock (please correct me if I'm
wrong).  I think you really wanted your input clock to be
"clk_usbphy0_480m", not "clk_usbphy0_480m_src".  Specifically I
believe there is a gate between the clock outputted by the PHY and the
USB Controller itself.  I'm guessing that the gate is only there
between the PHY and the "clk_usbphy_480m" MUX.

As evidence, I have a totally functioning system right now where
"clk_usbphy0_480m_src" is currently gated.

That means really you should be changing your clocks to this (untested):

               clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
                        <&u2phy0>;

...and then you could drop the other two patches in this series.

===

OK, I actually briefly tested my proposed change and it at least seems
to build and boot OK.  You'd have to test it to make sure it makes
your tests pass...

===

So I guess to summarize all the above:

* It seems to me like there's some deeper root cause and your patch
will at most put a band-aid on it.  Seems like digging out the root
cause is a good idea.

* Though I don't believe it solves the root problem, the idea of the
USB Controller holding onto the PHY clock doesn't seem wrong.

* You're holding onto the wrong clock in your patch--you want the one
before the gate (I think).


-Doug

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Brian Norris]

Hi,

I think Doug is probably right on most accounts, and I haven't
thoroughly investigated all the claims. But a few thoughts:

On Wed, Dec 14, 2016 at 04:10:38PM -0800, Doug Anderson wrote:
> On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> > From: William wu <wulf@rock-chips.com>
> >
> > We found that the suspend process was blocked when it run into
> > ehci/ohci module due to clk-480m of usb2-phy was disabled.
> >
> > The root cause is that usb2-phy suspended earlier than ehci/ohci
> > (usb2-phy will be auto suspended if no devices plug-in).

When you say "suspend" do you mean USB runtime suspend (i.e., auto
suspend) or do you mean system suspend (i.e., driver .suspend()
callbacks)? The latter are empty intentionally for PHY drivers, since
PHY state is managed by the consumer driver (i.e., the controller
driver). And the former doesn't actually disable any clocks AFAIK, so
that's a red herring IIUC.

> This is really weird, but I can confirm it is true on my system too
> (kernel-4.4 based).  At least I see:
> 
> [  208.012065] calling  usb1+ @ 4984, parent: fe380000.usb, cb: usb_dev_suspend
> [  208.569112] calling  ff770000.syscon:usb2-phy@e450+ @ 4983, parent:
> ff770000.syscon, cb: platform_pm_suspend
> [  208.569113] call ff770000.syscon:usb2-phy@e450+ returned 0 after 0 usecs
> [  208.569439] calling  fe380000.usb+ @ 4983, parent: platform, cb:
> platform_pm_suspend
> [  208.569444] call fe380000.usb+ returned 0 after 4 usecs
> 
> 
> In general I thought that suspend order was supposed to be related to
> probe order.  So if your probe order is A, B, C then your suspend
> order would be C, B, A.  ...and we know for sure that the USB PHY
> needs to probe _before_ the main USB controller.  If it didn't then
> you'd get an EPROBE_DEFER in the USB controller, right?  So that means
> that the USB controller should be suspending before its PHY.
> 
> Any chance this is somehow related to async probe?  I'm not a huge
> expert on async probe but I guess I could imagine things getting
> confused if you had a sequence like this:
> 
> 1. Start USB probe (async)
> 2. Start PHY probe
> 3. Finish PHY probe
> 4. In USB probe, ask for PHY--no problems since PHY probe finished
> 5. Finish USB probe
> 
> The probe order would be USB before PHY even though the USB probe
> _depended_ on the PHY probe being finished...  :-/  Anyway, probably
> I'm just misunderstanding something and someone can tell me how dumb I
> am...

That may well be true. There isn't a single defined probe order as soon
as you involve async probe, right? So things get a little fuzzier. Also,
I know if you're talking about async suspend/resume, the driver core
only (until quite recently? [1]) respects parent/child relationships.
But I'm not sure of all the async details right now, and async suspend
isn't typically used for the controllers AFAIK -- just for the
hubs/devices.

Anyway, I don't think that's relevant at all because:

> I also notice that the ehci_platform_power_off() function we're
> actually making PHY commands right before the same commands that turn
> off our clocks.  Presumably those commands aren't really so good to do
> if the PHY has already been suspended?
> 
> Actually, does the PHY suspend from platform_pm_suspend() actually
> even do anything?  It doesn't look like it.  It looks as if all the
> PHY cares about is init/exit and on/off...  ...and it looks as if the
> PHY should be turned off by the EHCI controller at about the same time
> it turns off its clocks...

Right, PHY drivers don't do anything at suspend/resume, since I guess
they presume the consuming driver (the controller) will handle state
transitions (power off, exit).

> I haven't fully dug, but is there any chance that things are getting
> confused between the OTG PHY and the Host PHY?  Maybe when we turn off
> the OTG PHY it turns off something that the host PHY needs?

Random thing I noticed: there seems to be a race in
phy-rockchip-inno-usb2.c, if we're worried about the 480M clock getting
disabled too early. See:

static int rockchip_usb2phy_power_off(struct phy *phy)
{
...
        clk_disable_unprepare(rphy->clk480m);
...
}

static int rockchip_usb2phy_exit(struct phy *phy)
{
        struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);

        if (rport->port_id == USB2PHY_PORT_OTG &&
            rport->mode != USB_DR_MODE_HOST) {
                cancel_delayed_work_sync(&rport->otg_sm_work);
                cancel_delayed_work_sync(&rport->chg_work);
        } else if (rport->port_id == USB2PHY_PORT_HOST)
                cancel_delayed_work_sync(&rport->sm_work);

        return 0;
}

I believe that means any of those work handlers can still be running while
after power_off() -- and therefore can be running after we've disabled the
clock. Might this be your problem?

If so, you're papering that bug by keeping a clock reference in the
controller, which implicitly defers the *actual*
clock_disable_unprepare() until much later.

Brian

[1] commit 9ed9895370ae ("driver core: Functional dependencies tracking
support")

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Brian Norris]

On Wed, Dec 14, 2016 at 04:47:38PM -0800, Brian Norris wrote:
> On Wed, Dec 14, 2016 at 04:10:38PM -0800, Doug Anderson wrote:
> > On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> > > From: William wu <wulf@rock-chips.com>
> > >
> > > We found that the suspend process was blocked when it run into
> > > ehci/ohci module due to clk-480m of usb2-phy was disabled.

One more thing: why is the USB2 PHY relevant to the OHCI controller? And
if it is relevant, why isn't there a PHY phandle for it in
usb_host0_ohci and usb_host1_ohci in rk3399.dtsi? As it stands, your
patch is hacking in USB2 clock references for OHCI, but you're not
actually managing the PHY there at all. Seems like you'd want to do
all-or-nothing if there's a functional dependency between the OHCI
controllers and the USB2 PHYs.

Brian

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Xing Zheng]

// Frank

Hi Doug,  Brain,
     Thanks for the reply.
     Sorry I forgot these patches have been sent earlier, and Frank have 
some explained and discussed with Heiko.
Please see https://patchwork.kernel.org/patch/9255245/
     Perhaps we can move to that patch tree to continue the discussion.

     I think Frank and William will help us to continue checking these.

Thanks

在 2016年12月15日 08:10, Doug Anderson 写道:
> Hi,
>
> On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
>> From: William wu <wulf@rock-chips.com>
>>
>> We found that the suspend process was blocked when it run into
>> ehci/ohci module due to clk-480m of usb2-phy was disabled.
>>
>> The root cause is that usb2-phy suspended earlier than ehci/ohci
>> (usb2-phy will be auto suspended if no devices plug-in).
> This is really weird, but I can confirm it is true on my system too
> (kernel-4.4 based).  At least I see:
>
> [  208.012065] calling  usb1+ @ 4984, parent: fe380000.usb, cb: usb_dev_suspend
> [  208.569112] calling  ff770000.syscon:usb2-phy@e450+ @ 4983, parent:
> ff770000.syscon, cb: platform_pm_suspend
> [  208.569113] call ff770000.syscon:usb2-phy@e450+ returned 0 after 0 usecs
> [  208.569439] calling  fe380000.usb+ @ 4983, parent: platform, cb:
> platform_pm_suspend
> [  208.569444] call fe380000.usb+ returned 0 after 4 usecs
>
>
> In general I thought that suspend order was supposed to be related to
> probe order.  So if your probe order is A, B, C then your suspend
> order would be C, B, A.  ...and we know for sure that the USB PHY
> needs to probe _before_ the main USB controller.  If it didn't then
> you'd get an EPROBE_DEFER in the USB controller, right?  So that means
> that the USB controller should be suspending before its PHY.
>
> Any chance this is somehow related to async probe?  I'm not a huge
> expert on async probe but I guess I could imagine things getting
> confused if you had a sequence like this:
>
> 1. Start USB probe (async)
> 2. Start PHY probe
> 3. Finish PHY probe
> 4. In USB probe, ask for PHY--no problems since PHY probe finished
> 5. Finish USB probe
>
> The probe order would be USB before PHY even though the USB probe
> _depended_ on the PHY probe being finished...  :-/  Anyway, probably
> I'm just misunderstanding something and someone can tell me how dumb I
> am...
>
> I also notice that the ehci_platform_power_off() function we're
> actually making PHY commands right before the same commands that turn
> off our clocks.  Presumably those commands aren't really so good to do
> if the PHY has already been suspended?
>
> Actually, does the PHY suspend from platform_pm_suspend() actually
> even do anything?  It doesn't look like it.  It looks as if all the
> PHY cares about is init/exit and on/off...  ...and it looks as if the
> PHY should be turned off by the EHCI controller at about the same time
> it turns off its clocks...
>
> I haven't fully dug, but is there any chance that things are getting
> confused between the OTG PHY and the Host PHY?  Maybe when we turn off
> the OTG PHY it turns off something that the host PHY needs?
>
>
>> and the
>> clk-480m provided by it was disabled if no module used. However,
>> some suspend process related ehci/ohci are base on this clock,
>> so we should refer it into ehci/ohci driver to prevent this case.
> Though I don't actually have details about the internals of the chip,
> it does seem highly likely that the USB block actually uses this clock
> for some things, so it doesn't seem insane (to me) to have the USB
> controller request that the clock be on.  So, in general, I don't have
> lots of objections to including the USB PHY Clock here.
>
> ...but I think you have the wrong clock (please correct me if I'm
> wrong).  I think you really wanted your input clock to be
> "clk_usbphy0_480m", not "clk_usbphy0_480m_src".  Specifically I
> believe there is a gate between the clock outputted by the PHY and the
> USB Controller itself.  I'm guessing that the gate is only there
> between the PHY and the "clk_usbphy_480m" MUX.
>
> As evidence, I have a totally functioning system right now where
> "clk_usbphy0_480m_src" is currently gated.
>
> That means really you should be changing your clocks to this (untested):
>
>                 clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>                          <&u2phy0>;
>
> ...and then you could drop the other two patches in this series.
>
> ===
>
> OK, I actually briefly tested my proposed change and it at least seems
> to build and boot OK.  You'd have to test it to make sure it makes
> your tests pass...
>
> ===
>
> So I guess to summarize all the above:
>
> * It seems to me like there's some deeper root cause and your patch
> will at most put a band-aid on it.  Seems like digging out the root
> cause is a good idea.
>
> * Though I don't believe it solves the root problem, the idea of the
> USB Controller holding onto the PHY clock doesn't seem wrong.
>
> * You're holding onto the wrong clock in your patch--you want the one
> before the gate (I think).
>
>
> -Doug
>
>
>


-- 
- Xing Zheng

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Brian Norris]

On Thu, Dec 15, 2016 at 10:41:04AM +0800, Xing Zheng wrote:
> // Frank
> 
> Hi Doug,  Brain,
>     Thanks for the reply.
>     Sorry I forgot these patches have been sent earlier, and Frank
> have some explained and discussed with Heiko.
> Please see https://patchwork.kernel.org/patch/9255245/
>     Perhaps we can move to that patch tree to continue the discussion.
> 
>     I think Frank and William will help us to continue checking these.

I only briefly read that discussion, but AFAICT it doesn't actually
address all the comments/quetions we had here. For instance, the
power_off() vs. delayed-work race in your USB2 PHY driver (is that
intentional?). Also, the question of why PHY (auto?)suspend is relevant.

I'll check again tomorrow.

Brian

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Frank Wang]

Hi Brain, Doug and Heiko,

I would like to summarize why this story was constructed.

The ehci/ohci-platform suspend process are blocked due to UTMI clock 
which directly output from usb-phy has been disabled, and why the UTMI 
clock was disabled?

UTMI clock and 480m clock all output from the same internal PLL of 
usb-phy, and there is only one bit can use to control this PLL on or 
off, which we named "otg_commononn"(GRF, offset 0x0e450/0x0e460 bit4 ) 
in RK3399 TRM.

When system boot up, ehci/ohci-platform probe function invoke 
phy_power_on(), further invoke rockchip_usb2phy_power_on() to enable 
480m clock, actually, it sets the otg_commononn bit on, and then usb-phy 
will go to (auto)suspend if there is no devices plug-in after 1 minute, 
the rockchip_usb2phy_power_off() will be invoked and the 480m clock may 
be disabled in the (auto)suspend process. As a result, the otg_commononn 
bit may be turned off, and all output clock of usb-phy will be disabled. 
However, ehci/ohci-platform PM suspend operation (read/write controller 
register) are based on the UTMI clock.

So we introduced "clk_usbphy0_480m_src"/"clk_usbphy1_480m_src" as one 
input clock for ehci/ohci-platform, in this way, the otg_commononn bit 
is not turned off until ehci/ohci-platform go to PM suspend.


BR.
Frank

On 2016/12/15 10:41, Xing Zheng wrote:
> // Frank
>
> Hi Doug,  Brain,
>     Thanks for the reply.
>     Sorry I forgot these patches have been sent earlier, and Frank 
> have some explained and discussed with Heiko.
> Please see https://patchwork.kernel.org/patch/9255245/
>     Perhaps we can move to that patch tree to continue the discussion.
>
>     I think Frank and William will help us to continue checking these.
>
> Thanks
>
> 在 2016年12月15日 08:10, Doug Anderson 写道:
>> Hi,
>>
>> On Wed, Dec 14, 2016 at 2:11 AM, Xing Zheng 
>> <zhengxing@rock-chips.com> wrote:
>>> From: William wu <wulf@rock-chips.com>
>>>
>>> We found that the suspend process was blocked when it run into
>>> ehci/ohci module due to clk-480m of usb2-phy was disabled.
>>>
>>> The root cause is that usb2-phy suspended earlier than ehci/ohci
>>> (usb2-phy will be auto suspended if no devices plug-in).
>> This is really weird, but I can confirm it is true on my system too
>> (kernel-4.4 based).  At least I see:
>>
>> [  208.012065] calling  usb1+ @ 4984, parent: fe380000.usb, cb: 
>> usb_dev_suspend
>> [  208.569112] calling  ff770000.syscon:usb2-phy@e450+ @ 4983, parent:
>> ff770000.syscon, cb: platform_pm_suspend
>> [  208.569113] call ff770000.syscon:usb2-phy@e450+ returned 0 after 0 
>> usecs
>> [  208.569439] calling  fe380000.usb+ @ 4983, parent: platform, cb:
>> platform_pm_suspend
>> [  208.569444] call fe380000.usb+ returned 0 after 4 usecs
>>
>>
>> In general I thought that suspend order was supposed to be related to
>> probe order.  So if your probe order is A, B, C then your suspend
>> order would be C, B, A.  ...and we know for sure that the USB PHY
>> needs to probe _before_ the main USB controller.  If it didn't then
>> you'd get an EPROBE_DEFER in the USB controller, right?  So that means
>> that the USB controller should be suspending before its PHY.
>>
>> Any chance this is somehow related to async probe?  I'm not a huge
>> expert on async probe but I guess I could imagine things getting
>> confused if you had a sequence like this:
>>
>> 1. Start USB probe (async)
>> 2. Start PHY probe
>> 3. Finish PHY probe
>> 4. In USB probe, ask for PHY--no problems since PHY probe finished
>> 5. Finish USB probe
>>
>> The probe order would be USB before PHY even though the USB probe
>> _depended_ on the PHY probe being finished...  :-/  Anyway, probably
>> I'm just misunderstanding something and someone can tell me how dumb I
>> am...
>>
>> I also notice that the ehci_platform_power_off() function we're
>> actually making PHY commands right before the same commands that turn
>> off our clocks.  Presumably those commands aren't really so good to do
>> if the PHY has already been suspended?
>>
>> Actually, does the PHY suspend from platform_pm_suspend() actually
>> even do anything?  It doesn't look like it.  It looks as if all the
>> PHY cares about is init/exit and on/off...  ...and it looks as if the
>> PHY should be turned off by the EHCI controller at about the same time
>> it turns off its clocks...
>>
>> I haven't fully dug, but is there any chance that things are getting
>> confused between the OTG PHY and the Host PHY?  Maybe when we turn off
>> the OTG PHY it turns off something that the host PHY needs?
>>
>>
>>> and the
>>> clk-480m provided by it was disabled if no module used. However,
>>> some suspend process related ehci/ohci are base on this clock,
>>> so we should refer it into ehci/ohci driver to prevent this case.
>> Though I don't actually have details about the internals of the chip,
>> it does seem highly likely that the USB block actually uses this clock
>> for some things, so it doesn't seem insane (to me) to have the USB
>> controller request that the clock be on.  So, in general, I don't have
>> lots of objections to including the USB PHY Clock here.
>>
>> ...but I think you have the wrong clock (please correct me if I'm
>> wrong).  I think you really wanted your input clock to be
>> "clk_usbphy0_480m", not "clk_usbphy0_480m_src".  Specifically I
>> believe there is a gate between the clock outputted by the PHY and the
>> USB Controller itself.  I'm guessing that the gate is only there
>> between the PHY and the "clk_usbphy_480m" MUX.
>>
>> As evidence, I have a totally functioning system right now where
>> "clk_usbphy0_480m_src" is currently gated.
>>
>> That means really you should be changing your clocks to this (untested):
>>
>>                 clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>>                          <&u2phy0>;
>>
>> ...and then you could drop the other two patches in this series.
>>
>> ===
>>
>> OK, I actually briefly tested my proposed change and it at least seems
>> to build and boot OK.  You'd have to test it to make sure it makes
>> your tests pass...
>>
>> ===
>>
>> So I guess to summarize all the above:
>>
>> * It seems to me like there's some deeper root cause and your patch
>> will at most put a band-aid on it.  Seems like digging out the root
>> cause is a good idea.
>>
>> * Though I don't believe it solves the root problem, the idea of the
>> USB Controller holding onto the PHY clock doesn't seem wrong.
>>
>> * You're holding onto the wrong clock in your patch--you want the one
>> before the gate (I think).
>>
>>
>> -Doug
>>
>>
>>
>
>

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Doug Anderson]

Hi,

On Wed, Dec 14, 2016 at 10:41 PM, Frank Wang <frank.wang@rock-chips.com> wrote:
> Hi Brain, Doug and Heiko,
>
> I would like to summarize why this story was constructed.
>
> The ehci/ohci-platform suspend process are blocked due to UTMI clock which
> directly output from usb-phy has been disabled, and why the UTMI clock was
> disabled?
>
> UTMI clock and 480m clock all output from the same internal PLL of usb-phy,
> and there is only one bit can use to control this PLL on or off, which we
> named "otg_commononn"(GRF, offset 0x0e450/0x0e460 bit4 ) in RK3399 TRM.
>
> When system boot up, ehci/ohci-platform probe function invoke
> phy_power_on(), further invoke rockchip_usb2phy_power_on() to enable 480m
> clock, actually, it sets the otg_commononn bit on, and then usb-phy will go
> to (auto)suspend if there is no devices plug-in after 1 minute, the
> rockchip_usb2phy_power_off() will be invoked and the 480m clock may be
> disabled in the (auto)suspend process. As a result, the otg_commononn bit
> may be turned off, and all output clock of usb-phy will be disabled.
> However, ehci/ohci-platform PM suspend operation (read/write controller
> register) are based on the UTMI clock.
>
> So we introduced "clk_usbphy0_480m_src"/"clk_usbphy1_480m_src" as one input
> clock for ehci/ohci-platform, in this way, the otg_commononn bit is not
> turned off until ehci/ohci-platform go to PM suspend.

I still need to digest all of the things that were added to this
thread overnight, but nothing I've seen so far indicates that you need
the post-gated clock.  AKA I still think you need to redo your patch
to replace:

              clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
                       <&cru SCLK_USBPHY0_480M_SRC>;

with:

              clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
                        <&u2phy0>;

Can you please comment on that?

-Doug

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Heiko Stuebner]

Am Donnerstag, 15. Dezember 2016, 08:34:09 CET schrieb Doug Anderson:
> Hi,
> 
> On Wed, Dec 14, 2016 at 10:41 PM, Frank Wang <frank.wang@rock-chips.com> 
wrote:
> > Hi Brain, Doug and Heiko,
> > 
> > I would like to summarize why this story was constructed.
> > 
> > The ehci/ohci-platform suspend process are blocked due to UTMI clock which
> > directly output from usb-phy has been disabled, and why the UTMI clock was
> > disabled?
> > 
> > UTMI clock and 480m clock all output from the same internal PLL of
> > usb-phy,
> > and there is only one bit can use to control this PLL on or off, which we
> > named "otg_commononn"(GRF, offset 0x0e450/0x0e460 bit4 ) in RK3399 TRM.
> > 
> > When system boot up, ehci/ohci-platform probe function invoke
> > phy_power_on(), further invoke rockchip_usb2phy_power_on() to enable 480m
> > clock, actually, it sets the otg_commononn bit on, and then usb-phy will
> > go
> > to (auto)suspend if there is no devices plug-in after 1 minute, the
> > rockchip_usb2phy_power_off() will be invoked and the 480m clock may be
> > disabled in the (auto)suspend process. As a result, the otg_commononn bit
> > may be turned off, and all output clock of usb-phy will be disabled.
> > However, ehci/ohci-platform PM suspend operation (read/write controller
> > register) are based on the UTMI clock.
> > 
> > So we introduced "clk_usbphy0_480m_src"/"clk_usbphy1_480m_src" as one
> > input
> > clock for ehci/ohci-platform, in this way, the otg_commononn bit is not
> > turned off until ehci/ohci-platform go to PM suspend.
> 
> I still need to digest all of the things that were added to this
> thread overnight, but nothing I've seen so far indicates that you need
> the post-gated clock.  AKA I still think you need to redo your patch
> to replace:
> 
>               clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>                        <&cru SCLK_USBPHY0_480M_SRC>;
> 
> with:
> 
>               clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>                         <&u2phy0>;
> 
> Can you please comment on that?

Also, with the change, the ehci will keep the clock (and thus the phy) always 
on. Does the phy-autosuspend even save anything now?

In any case, could we make the clock-names entry sound nicer than usbphy0_480m 
please? bindings/usb/atmel-usb.txt calls its UTMI clock simply "usb_clk", but 
something like "utmi" should also work.
While at it you could also fix up the other clock names to something like 
"host" and "arbiter" or so?.


Heiko

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Doug Anderson]

Hi,

On Thu, Dec 15, 2016 at 10:57 PM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> Hi Heiko, Doug,
>
> On 2016年12月16日 02:18, Heiko Stuebner wrote:
>
> Am Donnerstag, 15. Dezember 2016, 08:34:09 CET schrieb Doug Anderson:
>
>
> I still need to digest all of the things that were added to this
> thread overnight, but nothing I've seen so far indicates that you need
> the post-gated clock.  AKA I still think you need to redo your patch
> to replace:
>
>               clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>                        <&cru SCLK_USBPHY0_480M_SRC>;
>
> with:
>
>               clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>                         <&u2phy0>;
>
> Can you please comment on that?
>
> Also, with the change, the ehci will keep the clock (and thus the phy)
> always
> on. Does the phy-autosuspend even save anything now?
>
> In any case, could we make the clock-names entry sound nicer than
> usbphy0_480m
> please? bindings/usb/atmel-usb.txt calls its UTMI clock simply "usb_clk",
> but
> something like "utmi" should also work.
> While at it you could also fix up the other clock names to something like
> "host" and "arbiter" or so?.
>
>
> Heiko
>
>
> The usbphy related clock tress like this:
>
>
> Actually, at drivers/phy/phy-rockchip-inno-usb2.c, we can only
> enable/disable the master gate via GRF is PHY_PLL, not UTMI_CLK.
>
> And the naming style of the "hclk_host0" keep the name "hclk_host0" on the
> clcok tree diagram:
>
>
> Therefore, could we rename the clock name like this:
> ----
> for usb_host0_ehci and usb_host0_ohci:
>         clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>              <&cru SCLK_U2PHY0>;
>         clock-names = "hclk_host0", "hclk_host0_arb",
>                   "sclk_u2phy0";
>
> for usb_host1_ehci and usb_host1_ohci:
>         clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>,
>              <&cru SCLK_U2PHY1>;
>         clock-names = "hclk_host1", "hclk_host1_arb",
>                   "sclk_u2phy1";
> ----
>
> BTW, the "arb" is an abbreviation for arbiter.

You don't specify what this new "SCLK_U2PHY0" ID is, so it's a little
hard for me to know what you're intending.

...however, I still don't see any reason why you can't just use the
solution I proposed.  Specifying the clock as "<&u2phy0>" is the
correct thing to do.  The input clock to the EHCI driver is exactly
the clock provided by the USB PHY with no gate in between (just as I
said).  There is no reason to somehow buffer it by the cru.  The cru
doesn't see this clock and has no reason to be involved.


> Thanks.

Note that there were many other comments on this thread besides mine.
Are you planning to address any of them?

-Doug

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Xing Zheng]

Hi Heiko, Doug

在 2016年12月17日 01:28, Doug Anderson 写道:
> Hi,
>
> On Thu, Dec 15, 2016 at 10:57 PM, Xing Zheng <zhengxing@rock-chips.com> wrote:
>> Hi Heiko, Doug,
>>
>> On 2016年12月16日 02:18, Heiko Stuebner wrote:
>>
>> Am Donnerstag, 15. Dezember 2016, 08:34:09 CET schrieb Doug Anderson:
>>
>>
>> I still need to digest all of the things that were added to this
>> thread overnight, but nothing I've seen so far indicates that you need
>> the post-gated clock.  AKA I still think you need to redo your patch
>> to replace:
>>
>>                clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>>                         <&cru SCLK_USBPHY0_480M_SRC>;
>>
>> with:
>>
>>                clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>>                          <&u2phy0>;
>>
>> Can you please comment on that?
>>
>> Also, with the change, the ehci will keep the clock (and thus the phy)
>> always
>> on. Does the phy-autosuspend even save anything now?
>>
>> In any case, could we make the clock-names entry sound nicer than
>> usbphy0_480m
>> please? bindings/usb/atmel-usb.txt calls its UTMI clock simply "usb_clk",
>> but
>> something like "utmi" should also work.
>> While at it you could also fix up the other clock names to something like
>> "host" and "arbiter" or so?.
>>
>>
>> Heiko
>>
>>
>> The usbphy related clock tress like this:
>>
>>
>> Actually, at drivers/phy/phy-rockchip-inno-usb2.c, we can only
>> enable/disable the master gate via GRF is PHY_PLL, not UTMI_CLK.
>>
>> And the naming style of the "hclk_host0" keep the name "hclk_host0" on the
>> clcok tree diagram:
>>
>>
>> Therefore, could we rename the clock name like this:
>> ----
>> for usb_host0_ehci and usb_host0_ohci:
>>          clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>>               <&cru SCLK_U2PHY0>;
>>          clock-names = "hclk_host0", "hclk_host0_arb",
>>                    "sclk_u2phy0";
>>
>> for usb_host1_ehci and usb_host1_ohci:
>>          clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>,
>>               <&cru SCLK_U2PHY1>;
>>          clock-names = "hclk_host1", "hclk_host1_arb",
>>                    "sclk_u2phy1";
>> ----
>>
>> BTW, the "arb" is an abbreviation for arbiter.
> You don't specify what this new "SCLK_U2PHY0" ID is, so it's a little
> hard for me to know what you're intending.
>
> ...however, I still don't see any reason why you can't just use the
> solution I proposed.  Specifying the clock as "<&u2phy0>" is the
> correct thing to do.  The input clock to the EHCI driver is exactly
> the clock provided by the USB PHY with no gate in between (just as I
> said).  There is no reason to somehow buffer it by the cru.  The cru
> doesn't see this clock and has no reason to be involved.
>
> Note that there were many other comments on this thread besides mine.
> Are you planning to address any of them?
>
> -Doug
>
>
Done, and have resent the patch.

Thanks.
-- 

- Xing Zheng

Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399

[Heiko Stuebner]

Am Freitag, 16. Dezember 2016, 14:57:01 CET schrieb Xing Zheng:
> Hi Heiko, Doug,
> 
> On 2016年12月16日 02:18, Heiko Stuebner wrote:
> > Am Donnerstag, 15. Dezember 2016, 08:34:09 CET schrieb Doug Anderson:
> >> I still need to digest all of the things that were added to this
> >> thread overnight, but nothing I've seen so far indicates that you need
> >> the post-gated clock.  AKA I still think you need to redo your patch
> >> 
> >> to replace:
> >>                clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> >>                
> >>                         <&cru SCLK_USBPHY0_480M_SRC>;
> >> 
> >> with:
> >>                clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
> >>                
> >>                          <&u2phy0>;
> >> 
> >> Can you please comment on that?
> > 
> > Also, with the change, the ehci will keep the clock (and thus the phy)
> > always on. Does the phy-autosuspend even save anything now?
> > 
> > In any case, could we make the clock-names entry sound nicer than
> > usbphy0_480m please? bindings/usb/atmel-usb.txt calls its UTMI clock
> > simply "usb_clk", but something like "utmi" should also work.
> > While at it you could also fix up the other clock names to something like
> > "host" and "arbiter" or so?.
> > 
> > 
> > Heiko
> 
> The usbphy related clock tress like this:
> 
> 
> Actually, at drivers/phy/phy-rockchip-inno-usb2.c, we can only
> enable/disable the master gate via GRF is PHY_PLL, not UTMI_CLK.
> 
> And the naming style of the "hclk_host0" keep the name "hclk_host0" on
> the clcok tree diagram:
> 
> 
> Therefore, could we rename the clock name like this:
> ----
> for usb_host0_ehci and usb_host0_ohci:
>          clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
>               <&cru SCLK_U2PHY0>;
>          clock-names = "hclk_host0", "hclk_host0_arb",
>                    "sclk_u2phy0";
> 
> for usb_host1_ehci and usb_host1_ohci:
>          clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>,
>               <&cru SCLK_U2PHY1>;
>          clock-names = "hclk_host1", "hclk_host1_arb",
>                    "sclk_u2phy1";
> ----
> 
> BTW, the "arb" is an abbreviation for arbiter.

clock-naming wise, the clock names in devicetree bindings should always 
describe the clock in the context of the peripheral, not the hosts clock-tree.

So if the clock supplies the "arbiter" part, the clock-name should be called 
"arbiter". Same for "utmi" and the host clock that could be named "usbhost" or 
just "host" in the clock-names property.