Hi Frank,
On Sun, Jun 19, 2016 at 8:32 PM, Frank Wang <frank.wang@xxxxxxxxxxxxxx> wrote:
Hi Heiko & Guenter,Other phy drivers name the functions _power_off and _power_on and
On 2016/6/20 11:00, Guenter Roeck wrote:
On Sun, Jun 19, 2016 at 6:27 PM, Frank Wang <frank.wang@xxxxxxxxxxxxxx>
wrote:
Hi Guenter,I had to re-read the entire patch.
On 2016/6/17 21:20, Guenter Roeck wrote:
Hi Frank,This is an initialization. Using above design which make 'suspended' as a
On 06/16/2016 11:43 PM, Frank Wang wrote:
Hi Guenter,Why does it start in suspended mode ? That seems odd.
On 2016/6/17 12:59, Guenter Roeck wrote:
On 06/16/2016 07:09 PM, Frank Wang wrote:Well, what you said is reasonable, How does something like below?
The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
than rk3288 and before, and most of phy-related registers are also
different from the past, so a new phy driver is required necessarily.
Signed-off-by: Frank Wang <frank.wang@xxxxxxxxxxxxxx>
Suggested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
Suggested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>
Tested-by: Heiko Stuebner <heiko@xxxxxxxxx>
---
[ ... ]
+If suspend can be called multiple times, resume can be called
+static int rockchip_usb2phy_resume(struct phy *phy)
+{
+ struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
+ struct rockchip_usb2phy *rphy =
dev_get_drvdata(phy->dev.parent);
+ int ret;
+
+ dev_dbg(&rport->phy->dev, "port resume\n");
+
+ ret = clk_prepare_enable(rphy->clk480m);
+ if (ret)
+ return ret;
+
multiple times as well. Doesn't this cause a clock imbalance
if you call clk_prepare_enable() multiple times on resume,
but clk_disable_unprepare() only once on suspend ?
@@ -307,6 +307,9 @@ static int rockchip_usb2phy_resume(struct phy *phy)
dev_dbg(&rport->phy->dev, "port resume\n");
+ if (!rport->suspended)
+ return 0;
+
ret = clk_prepare_enable(rphy->clk480m);
if (ret)
return ret;
@@ -327,12 +330,16 @@ static int rockchip_usb2phy_suspend(struct phy
*phy)
dev_dbg(&rport->phy->dev, "port suspend\n");
+ if (rport->suspended)
+ return 0;
+
ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
if (ret)
return ret;
rport->suspended = true;
clk_disable_unprepare(rphy->clk480m);
+
return 0;
}
@@ -485,6 +492,7 @@ static int rockchip_usb2phy_host_port_init(struct
rockchip_usb2phy *rphy,
rport->port_id = USB2PHY_PORT_HOST;
rport->port_cfg =
&rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
+ rport->suspended = true;
condition both in *_usb2phy_resume and *_usb2phy_suspend, I believe if it
is
not initialized as suspended mode, the first resume process will be
skipped.
Turns out my problem was one of terminology. Using "suspend" and
"resume" to me suggested the common use of suspend and resume
functions. That is not the case here. After mentally replacing
"suspend" with "power_off" and "resume" with "power_on", you are
right, no problem exists. Sorry for the noise.
Maybe it would be useful to replace "resume" with "power_on" and
"suspend" with "power_off" in the function and variable names to
reduce confusion and misunderstandings.
Thanks,
Guenter
Well, it does have a bits confusion, however, the phy-port always just goes
to suspend and resume mode (Not power off and power on) in a fact. So must
it be renamed?
avoid the confusion. The callbacks are named .power_off and .power_on,
which gives a clear indication of its intended purpose. Other drivers
implementing suspend/resume (such as the omap usb phy driver) tie
those functions not into the power_off/power_on callbacks, but into
the driver's suspend/resume callbacks. At least the omap driver has
separate power management functions.
Do the functions _have_ to be renamed ? Surely not. But, if the
functions are really suspend/resume functions and not
power_off/power_on functions, maybe they should tie to the
suspend/resume functions and not register themselves as
power_off/power_on functions ?
Thanks,
Guenter
Theoretically, the phy-port in suspended mode make sense when it is at
start
time, then the upper layer controller will invoke phy_power_on (See
phy-core.c), and it further call back *_usb2phy_resume to make phy-port
work
properly.
So could you tell me what make you feeling odd or would you like to give
another appropriate way please? :-)
BR.
Frank
mutex_init(&rport->mutex);
INIT_DELAYED_WORK(&rport->sm_work, rockchip_usb2phy_sm_work);