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

[Guenter Roeck]

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

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