linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Doug Anderson <dianders@google.com>
Cc: "Xing Zheng" <zhengxing@rock-chips.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"Heiko Stübner" <heiko@sntech.de>,
	"William wu" <wulf@rock-chips.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Will Deacon" <will.deacon@arm.com>,
	"Caesar Wang" <wxt@rock-chips.com>,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	"Jianqun Xu" <jay.xu@rock-chips.com>,
	"Elaine Zhang" <zhangqing@rock-chips.com>,
	"David Wu" <david.wu@rock-chips.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Dmitry Torokhov" <dmitry.torokhov@gmail.com>
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399
Date: Wed, 14 Dec 2016 16:47:38 -0800	[thread overview]
Message-ID: <20161215004737.GA32652@google.com> (raw)
In-Reply-To: <CAD=FV=UU4JdRjRgEvc_wxprvi6bo46+jd=x2m2QzOe4uJmuRPA@mail.gmail.com>

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")

  reply	other threads:[~2016-12-15  0:47 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-14 10:11 [PATCH 0/3] Add and export clk-480m clocks for ehci and ohci on RK3399 Xing Zheng
2016-12-14 10:11 ` [PATCH 1/3] clk: rockchip: rk3399: add USBPHYx_480M_SRC clock IDs Xing Zheng
2016-12-15  0:27   ` Doug Anderson
2016-12-14 10:11 ` [PATCH 2/3] clk: rockchip: rk3399: export 480M_SRC clocks id for usbphy0/usbphy1 Xing Zheng
2016-12-15  0:28   ` Doug Anderson
2016-12-14 10:11 ` [PATCH 3/3] arm64: dts: rockchip: add clk-480m for ehci and ohci of rk3399 Xing Zheng
2016-12-15  0:10   ` Doug Anderson
2016-12-15  0:47     ` Brian Norris [this message]
2016-12-15  1:18       ` Brian Norris
2016-12-15  2:41     ` Xing Zheng
2016-12-15  3:20       ` Brian Norris
2016-12-15  6:41       ` Frank Wang
2016-12-15 16:34         ` Doug Anderson
2016-12-15 18:18           ` Heiko Stuebner
     [not found]             ` <5853903D.8030605@rock-chips.com>
2016-12-16 17:28               ` Doug Anderson
2016-12-21 10:44                 ` Xing Zheng
2016-12-17  1:20               ` Heiko Stuebner

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20161215004737.GA32652@google.com \
    --to=briannorris@chromium.org \
    --cc=catalin.marinas@arm.com \
    --cc=david.wu@rock-chips.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@google.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=heiko@sntech.de \
    --cc=jay.xu@rock-chips.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=shawn.lin@rock-chips.com \
    --cc=will.deacon@arm.com \
    --cc=wulf@rock-chips.com \
    --cc=wxt@rock-chips.com \
    --cc=zhangqing@rock-chips.com \
    --cc=zhengxing@rock-chips.com \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).