[PATCH v6 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

[Guenter Roeck]

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

> @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);
>>>>>
>>>
>