Hi Heiko,
On 2016/6/20 12:56, Guenter Roeck wrote:
> Hi Frank,
>
> On Sun, Jun 19, 2016 at 8:32 PM, Frank Wang <frank.wang at rock-chips.com> 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 at rock-chips.com>
>>> 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 at rock-chips.com>
>>>>>>>> Suggested-by: Guenter Roeck <linux at roeck-us.net>
>>>>>>>> Suggested-by: Doug Anderson <dianders at chromium.org>
>>>>>>>> Reviewed-by: Heiko Stuebner <heiko at sntech.de>
>>>>>>>> Tested-by: Heiko Stuebner <heiko at sntech.de>
>>>>>>>> ---
>>>>>>>
>>>>>>> [ ... ]
>>>>>>>
>>>>>>>> +
>>>>>>>> +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
As Guenter mentioned above, I doped out two solutions, one is that keep
current process but renaming *_resume/*_suspend to
*_power_on/*_power_off; another is that do not assign power_on/power_off
functions for phy_ops at phy creating time, then, shorten
_SCHEDULE_DELAY_ delay time less that 10 Seconds, and the phy-port
suspend/resume mechanism depend on _sm_work_ completely.
So which is the better way from your view? or would you like to give
other unique perceptions please?
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);
>>>>>>
