Hi Frank,
On Sun, Jun 19, 2016 at 8:32 PM, Frank Wang <frank.wang@xxxxxxxxxxxxxx> wrote:
> Hi Heiko & Guenter,
>
>
> 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,
>>>
>>>
>>> On 2016/6/17 21:20, Guenter Roeck wrote:
>>>>
>>>> Hi Frank,
>>>>
>>>> On 06/16/2016 11:43 PM, Frank Wang wrote:
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> On 2016/6/17 12:59, Guenter Roeck wrote:
>>>>>>
>>>>>> On 06/16/2016 07:09 PM, Frank Wang wrote:
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>> +
>>>>>>> +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;
>>>>>>> +
>>>>>>
>>>>>> If suspend can be called multiple times, resume can be called
>>>>>> 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 ?
>>>>>>
>>>>> Well, what you said is reasonable, How does something like below?
>>>>>
>>>>> @@ -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;
>>>>>
>>>> Why does it start in suspended mode ? That seems odd.
>>>>
>>> This is an initialization. Using above design which make 'suspended' as a
>>> 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.
>>
>> I had to re-read the entire patch.
>>
>> 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?
>
Other phy drivers name the functions _power_off and _power_on and
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
> @Heiko Stübner. Hey Heiko, what is your unique perceptions? ;-)
>
>
> BR.
> Frank
>
>
>>
>>> 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);
>>>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
