Hi,
On Fri, Dec 27, 2013 at 04:11:21PM +0800, Peter Chen wrote:
> > > > > > > Add clock enable/disable at .set_suspend if the PHY has
> > > > > > > suspend requirement, it can be benefit of power saving for
> > > > > > > phy and the whole system (parent clock may also be disabled).
> > > > > > >
> > > > > > > Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > drivers/usb/phy/phy-generic.c | 10 ++++++++++
> > > > > > > 1 files changed, 10 insertions(+), 0 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
> > > > > > > index aa6d37b..4b20a97 100644
> > > > > > > --- a/drivers/usb/phy/phy-generic.c
> > > > > > > +++ b/drivers/usb/phy/phy-generic.c
> > > > > > > @@ -65,6 +65,16 @@ EXPORT_SYMBOL(usb_nop_xceiv_unregister);
> > > > > > >
> > > > > > > static int nop_set_suspend(struct usb_phy *x, int suspend)
> > > > > > > {
> > > > > > > + struct usb_phy_gen_xceiv *nop = dev_get_drvdata(x->dev);
> > > > > > > +
> > > > > > > + if (IS_ERR(nop->clk))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + if (suspend)
> > > > > > > + clk_disable_unprepare(nop->clk);
> > > > > > > + else
> > > > > > > + clk_prepare_enable(nop->clk);
> > > > > >
> > > > > > heh, this is why I don't like clk_enable_prepare().
> > > > > > usb_phy_set_suspend() can be called from atomic context.
> > > > > >
> > > > >
> > > > > Oh, then, how can we disable/enable phy's clock at runtime/system-side
> > > > > pm? Create another APIs like usb_phy_set_clk()?
> > > >
> > > > clk_prepare() on probe, clk_unprepare() on remove,
> > > > clk_enable()/disable() on set_suspend()
> > > >
> > >
> > > It will keep phy's clock prepare_count is always 1, assume one
> > > use case, the pll clk is the parent of phy clk, it will
> > > cause the pll clk can't be unprepared during the runtime, it
> > > may cause the pll can't be powered off during the runtime.
> >
> > if you power off PHY's PLL, how will you ever notice a remote wakeup
> > event ? How will you ever wakeup at all ?
>
> Wakeup logic is covered by 32K OSC. Once the USB wakeup occurs,
> the PLL can be powered on automatically.
>
> In fact, in order to satisfy 1ms remote wakeup timing, some SoCs
> can send resume signal only with 32K OSC (At system suspend mode,
> the 24M OSC is off).
all right, so I see we need to extend .set_suspend() a bit.
.set_suspend() can be called from bus suspend IRQ and maybe the problem
comes from that mixed sematics of placing PHY in low power mode due to
system suspend or runtime pm, and decreasing power consumption because
USB bus has gone into bus suspend.
Looks like we need to fix that up before I can take this patch. If you
want, I can work on that for v3.15 but if you're willing to put the
work, go for it ;-)
--
balbi
Attachment:
signature.asc
Description: Digital signature
