Re: [v3 PATCH 3/5] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong <zyw@rock-chips.com>]

Hi Guenter

On 06/27/2016 12:01 PM, Guenter Roeck wrote:
> On Sun, Jun 26, 2016 at 7:19 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>> Hi Heiko
>>
>>
>> On 06/25/2016 03:39 AM, Heiko Stuebner wrote:
>>> Am Donnerstag, 23. Juni 2016, 18:27:52 schrieb Kishon Vijay Abraham I:
>>>> Hi,
>>>>
>>>> On Thursday 23 June 2016 06:21 PM, Chris Zhong wrote:
>>>>> Add a PHY provider driver for the rk3399 SoC Type-c PHY. The USB
>>>>> Type-C PHY is designed to support the USB3 and DP applications. The
>>>>> PHY basically has two main components: USB3 and DisplyPort. USB3
>>>>> operates in SuperSpeed mode and the DP can operate at RBR, HBR and
>>>>> HBR2 data rates.
>>>>>
>>>>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - remove the phy framework(Kishon Vijay Abraham I)
>>>>> - add parentheses around the macro
>>>>> - use a single space between type and name
>>>>> - add spaces after opening and before closing braces.
>>>>> - use u16 for register value
>>>>> - remove type-c phy header file
>>>>> - CodingStyle optimization
>>>>> - use some cable extcon to get type-c port information
>>>>> - add a extcon to notify Display Port
>>>>>
>>>>> Changes in v2:
>>>>> - select RESET_CONTROLLER
>>>>> - alphabetic order
>>>>> - modify some spelling mistakes
>>>>> - make mode cleaner
>>>>> - use bool for enable/disable
>>>>> - check all of the return value
>>>>> - return a better err number
>>>>> - use more readx_poll_timeout()
>>>>> - clk_disable_unprepare(tcphy->clk_ref);
>>>>> - remove unuse functions, rockchip_typec_phy_power_on/off
>>>>> - remove unnecessary typecast from void *
>>>>> - use dts node to distinguish between phys.
>>>>>
>>>>> Changes in v1:
>>>>> - update the licence note
>>>>> - init core clock to 50MHz
>>>>> - use extcon API
>>>>> - remove unused global
>>>>> - add some comments for magic num
>>>>> - change usleep_range(1000, 2000) tousleep_range(1000, 1050)
>>>>> - remove __func__ from dev_err
>>>>> - return err number when get clk failed
>>>>> - remove ADDR_ADJ define
>>>>> - use devm_clk_get(&pdev->dev, "tcpdcore")
>>>>>
>>>>>    drivers/phy/Kconfig              |    8 +
>>>>>    drivers/phy/Makefile             |    1 +
>>>>>    drivers/phy/phy-rockchip-typec.c | 1027
>>>>>    ++++++++++++++++++++++++++++++++++++++ 3 files changed, 1036
>>>>>    insertions(+)
>>>>>    create mode 100644 drivers/phy/phy-rockchip-typec.c
>>>>>
>>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>>>> index 26566db..ec87b3a 100644
>>>>> --- a/drivers/phy/Kconfig
>>>>> +++ b/drivers/phy/Kconfig
>>>>> @@ -351,6 +351,14 @@ config PHY_ROCKCHIP_DP
>>>>>
>>>>>          help
>>>>>
>>>>>            Enable this to support the Rockchip Display Port PHY.
>>>>>
>>>>> +config PHY_ROCKCHIP_TYPEC
>>>>> +       tristate "Rockchip TYPEC PHY Driver"
>>>>> +       depends on ARCH_ROCKCHIP && OF
>>>>> +       select GENERIC_PHY
>>>> Why? None of the generic PHY API's are used here. Why do you want select
>>>> generic PHY?
>>> I'm actually wondering, why there are no phy ops to start and stop the py.
>>> Right now the device seems to be on and handling all the extcon notifies
>>> all
>>> the time even if no-one is using the phy.
>>>
>>> There are two users of this phy, the dp-controller as well as some usb
>>> component. The phy framework is nicely refcounted, so can handle any
>>> number
>>> of phy users and somehow I'd expect the phy to do nothing (and try to not
>>> consume power) if neither the dp-controller nor the usb-part is actually
>>> running.
>>>
>>> It may very well be my ignorance, but Chris could you explain, if there is
>>> a
>>> reason for this?
>>>
>>>
>>> Thanks
>>> Heiko
>>>
>>>
>> It is good idea, The USB3 and DP controller detect the extcon cable state:
>> USB/USB HOST/DP. If a Type-C device plugged, call phy power on, the phy
>> driver get all the state of extcon: dfp, ufp, dp, flip, pin assignment, and
>> then finish the phy init. So the phy driver need not register extcon
>> notification.
>> But this mechanism allows phy driver only focus plug/unplug event, if some
>> other things happen, such as data role change, the phy will not be notified.
>> I'm not sure if this situation exists.
> Sorry, I think I am lost (again).
>
> All roles can change on the fly. Role changes can be triggered from
> user space, or by the remote partner. If we restrict such role
> changes, we would have to reject all locally triggered role change
> requests, as well as all role change requests from the remote, after
> the initial connection was established. I don't really see the point
> of doing that, though. Wasn't this what the notifications were all
> about ?
>
> Guenter
>
The following are the all 3 cases about data role changing:

1. "USB host only" mode(power_count ==1), switch to "USB device" mode
we can re-init phy by phy_power_off and then phy_power_on

2. "USB host + DP" mode(power_count ==2), switch to "USB device" mode
The phy can not support ufp+dp, so it is reasonable to do nothing. 
Moreover, does this situation exist?

3. "USB device" mode (power_count ==1), switch to "USB host only" or 
"USB host + DP"
it is similar with case 1, dp or usb host can re-init the phy again.

>