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

[Chris Zhong]

Hi Guenter

On 06/27/2016 12:01 PM, Guenter Roeck wrote:
> On Sun, Jun 26, 2016 at 7:19 PM, Chris Zhong <zyw@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > > > Signed-off-by: Kever Yang <kever.yang@xxxxxxxxxxxxxx>
> > > > >
> > > > > ---
> > > > >
> > > > > 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.

>