Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Tomasz

Thanks for your comments.
I will modify all the the part of snip. Please find my reply in the 
following.


On 06/18/2016 12:06 AM, Tomasz Figa wrote:

Hi Chris,


[snip]


+struct phy_reg {
+   int value;
+   int addr;
+};
+
+struct phy_reg usb_pll_cfg[] = {
+   {0xf0,  CMN_PLL0_VCOCAL_INIT},

CodingStyle: Please add spaces after opening and before closing braces.


+   {0x18,  CMN_PLL0_VCOCAL_ITER},
+   {0xd0,  CMN_PLL0_INTDIV},
+   {0x4a4a,CMN_PLL0_FRACDIV},
+   {0x34,  CMN_PLL0_HIGH_THR},
+   {0x1ee, CMN_PLL0_SS_CTRL1},
+   {0x7f03,CMN_PLL0_SS_CTRL2},
+   {0x20,  CMN_PLL0_DSM_DIAG},
+   {0, CMN_DIAG_PLL0_OVRD},
+   {0, CMN_DIAG_PLL0_FBH_OVRD},
+   {0, CMN_DIAG_PLL0_FBL_OVRD},
+   {0x7,   CMN_DIAG_PLL0_V2I_TUNE},
+   {0x45,  CMN_DIAG_PLL0_CP_TUNE},
+   {0x8,   CMN_DIAG_PLL0_LF_PROG},

It would be generally much, much better if these magic numbers were
dissected into particular bitfields and defined using macros, if
possible... The same applies to all other magic numbers in this file.
This magic number is very hard to describe, it is a initialization 
sequence from vendor.

Their names are very close to the description.
From spec of cdn type-c phy:
Iteration wait timer value
pll_fb_div_integer value: Value of the pll_fb_div_integer signal.
pll_fb_div_fractional: Value of the pll_fb_div_fractional signal.
pll_fb_div_high_theshold: Value of the pll_fb_div_high_threshold signal.
...


+};
+
+struct phy_reg dp_pll_cfg[] = {

[snip]

+static void tcphy_cfg_usb_pll(struct rockchip_typec_phy *tcphy)
+{
+   u32 rdata;
+   u32 i;
+
+   /*
+* Selects which PLL clock will be driven on the analog high speed
+* clock 0: PLL 0 div 1.
+*/
+   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
+   writel(rdata & 0xfffc, tcphy->base + CMN_DIAG_HSCLK_SEL);

This mask looks suspiciously. Is clearing bits 31-16 and 1-0 what we
want here? I'd advise for manipulating the value in separate line and
then only calling writel() with the final value already in the
variable.
Yes, the register valid length is 16 bits, but the they are stored with 
32bit.

readl will return 0 in higher 16bit + valid value in lower 16bit.
and writel will ignore the higher 16bit.





+
+   /* load the configuration of PLL0 */
+   for (i = 0; i < ARRAY_SIZE(usb_pll_cfg); i++)
+   writel(usb_pll_cfg[i].value, tcphy->base + usb_pll_cfg[i].addr);
+}
+
+static void tcphy_cfg_dp_pll(struct rockchip_typec_phy *tcphy)
+{
+   u32 rdata;
+   u32 i;
+
+   /* set the default mode to RBR */
+   writel(DP_PLL_CLOCK_ENABLE | DP_PLL_ENABLE | DP_PLL_DATA_RATE_RBR,
+  tcphy->base + DP_CLK_CTL);

This looks (and is understandable) much better than magic numbers in
other parts of this file!


+
+   /*
+* Selects which PLL clock will be driven on the analog high speed
+* clock 1: PLL 1 div 2.
+*/
+   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
+   rdata = (rdata & 0xffcf) | 0x30;

If the & operation here is used to clear a bitfield, please use the
negative notation, e.g.

rdata &= ~0x30;
rdata |= 0x30;

(By the way, the AND NOT and OR with the same value is what the code
above does, which would make sense if the bitfield mask was defined by
a macro, but doesn't make any sense with magic numbers.)

It looks like the registers are 16-bit. Should they really be accessed
with readl() and not readw()? If they are accessed with readl(), what
is returned in most significant 16 bits and what should be written
there?

I will use macro here at next version



+   writel(rdata, tcphy->base + CMN_DIAG_HSCLK_SEL);
+
+   /* load the configuration of PLL1 */
+   for (i = 0; i < ARRAY_SIZE(dp_pll_cfg); i++)
+   writel(dp_pll_cfg[i].value, tcphy->base + dp_pll_cfg[i].addr);
+}

[snip]

+   }
+
+   ret = clk_prepare_enable(tcphy->clk_ref);
+   if (ret) {
+   dev_err(tcphy->dev, "Failed to prepare_enable ref clock\n");
+   goto clk_ref_failed;
+   }

[snip]

+static void tcphy_phy_deinit(struct rockchip_typec_phy *tcphy)
+{
+   clk_disable_unprepare(tcphy->clk_core);
+   clk_disable_unprepare(tcphy->clk_ref);
+}
+
+static const struct phy_ops rockchip_tcphy_ops = {
+   .owner  = THIS_MODULE,

Hmm, if there is no phy ops, how the phy consumer drivers request the
PHY to do anything?
There is no consumer call this phy, the power on and power off are 
called by notification.

So I am going to delete this ops next version.

+};
+
+static int tcphy_pd_event(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{

[snip]

+static int tcphy_get_param(struct device *dev,
+  struct usb3phy_reg *reg,
+  const char 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Tomasz

Thanks for your comments.
I will modify all the the part of snip. Please find my reply in the 
following.


On 06/18/2016 12:06 AM, Tomasz Figa wrote:

Hi Chris,


[snip]


+struct phy_reg {
+   int value;
+   int addr;
+};
+
+struct phy_reg usb_pll_cfg[] = {
+   {0xf0,  CMN_PLL0_VCOCAL_INIT},

CodingStyle: Please add spaces after opening and before closing braces.


+   {0x18,  CMN_PLL0_VCOCAL_ITER},
+   {0xd0,  CMN_PLL0_INTDIV},
+   {0x4a4a,CMN_PLL0_FRACDIV},
+   {0x34,  CMN_PLL0_HIGH_THR},
+   {0x1ee, CMN_PLL0_SS_CTRL1},
+   {0x7f03,CMN_PLL0_SS_CTRL2},
+   {0x20,  CMN_PLL0_DSM_DIAG},
+   {0, CMN_DIAG_PLL0_OVRD},
+   {0, CMN_DIAG_PLL0_FBH_OVRD},
+   {0, CMN_DIAG_PLL0_FBL_OVRD},
+   {0x7,   CMN_DIAG_PLL0_V2I_TUNE},
+   {0x45,  CMN_DIAG_PLL0_CP_TUNE},
+   {0x8,   CMN_DIAG_PLL0_LF_PROG},

It would be generally much, much better if these magic numbers were
dissected into particular bitfields and defined using macros, if
possible... The same applies to all other magic numbers in this file.
This magic number is very hard to describe, it is a initialization 
sequence from vendor.

Their names are very close to the description.
From spec of cdn type-c phy:
Iteration wait timer value
pll_fb_div_integer value: Value of the pll_fb_div_integer signal.
pll_fb_div_fractional: Value of the pll_fb_div_fractional signal.
pll_fb_div_high_theshold: Value of the pll_fb_div_high_threshold signal.
...


+};
+
+struct phy_reg dp_pll_cfg[] = {

[snip]

+static void tcphy_cfg_usb_pll(struct rockchip_typec_phy *tcphy)
+{
+   u32 rdata;
+   u32 i;
+
+   /*
+* Selects which PLL clock will be driven on the analog high speed
+* clock 0: PLL 0 div 1.
+*/
+   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
+   writel(rdata & 0xfffc, tcphy->base + CMN_DIAG_HSCLK_SEL);

This mask looks suspiciously. Is clearing bits 31-16 and 1-0 what we
want here? I'd advise for manipulating the value in separate line and
then only calling writel() with the final value already in the
variable.
Yes, the register valid length is 16 bits, but the they are stored with 
32bit.

readl will return 0 in higher 16bit + valid value in lower 16bit.
and writel will ignore the higher 16bit.





+
+   /* load the configuration of PLL0 */
+   for (i = 0; i < ARRAY_SIZE(usb_pll_cfg); i++)
+   writel(usb_pll_cfg[i].value, tcphy->base + usb_pll_cfg[i].addr);
+}
+
+static void tcphy_cfg_dp_pll(struct rockchip_typec_phy *tcphy)
+{
+   u32 rdata;
+   u32 i;
+
+   /* set the default mode to RBR */
+   writel(DP_PLL_CLOCK_ENABLE | DP_PLL_ENABLE | DP_PLL_DATA_RATE_RBR,
+  tcphy->base + DP_CLK_CTL);

This looks (and is understandable) much better than magic numbers in
other parts of this file!


+
+   /*
+* Selects which PLL clock will be driven on the analog high speed
+* clock 1: PLL 1 div 2.
+*/
+   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
+   rdata = (rdata & 0xffcf) | 0x30;

If the & operation here is used to clear a bitfield, please use the
negative notation, e.g.

rdata &= ~0x30;
rdata |= 0x30;

(By the way, the AND NOT and OR with the same value is what the code
above does, which would make sense if the bitfield mask was defined by
a macro, but doesn't make any sense with magic numbers.)

It looks like the registers are 16-bit. Should they really be accessed
with readl() and not readw()? If they are accessed with readl(), what
is returned in most significant 16 bits and what should be written
there?

I will use macro here at next version



+   writel(rdata, tcphy->base + CMN_DIAG_HSCLK_SEL);
+
+   /* load the configuration of PLL1 */
+   for (i = 0; i < ARRAY_SIZE(dp_pll_cfg); i++)
+   writel(dp_pll_cfg[i].value, tcphy->base + dp_pll_cfg[i].addr);
+}

[snip]

+   }
+
+   ret = clk_prepare_enable(tcphy->clk_ref);
+   if (ret) {
+   dev_err(tcphy->dev, "Failed to prepare_enable ref clock\n");
+   goto clk_ref_failed;
+   }

[snip]

+static void tcphy_phy_deinit(struct rockchip_typec_phy *tcphy)
+{
+   clk_disable_unprepare(tcphy->clk_core);
+   clk_disable_unprepare(tcphy->clk_ref);
+}
+
+static const struct phy_ops rockchip_tcphy_ops = {
+   .owner  = THIS_MODULE,

Hmm, if there is no phy ops, how the phy consumer drivers request the
PHY to do anything?
There is no consumer call this phy, the power on and power off are 
called by notification.

So I am going to delete this ops next version.

+};
+
+static int tcphy_pd_event(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{

[snip]

+static int tcphy_get_param(struct device *dev,
+  struct usb3phy_reg *reg,
+  const char 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Guenter

On 06/18/2016 11:45 PM, Guenter Roeck wrote:

Hi Chris,

On Mon, Jun 13, 2016 at 2:39 AM, 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.


I started integrating the driver with our code.
Doing so, I realized a problem in the way you are using extcon.

[ ... ]


+
+static int tcphy_pd_event(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{
+   struct rockchip_typec_phy *tcphy;
+   struct extcon_dev *edev = priv;
+   int value = edev->state;
+   int mode;
+   u8 is_plugged, dfp;
+
+   tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
+
+   is_plugged = GET_PLUGGED(value);
+   tcphy->flip = GET_FLIP(value);
+   dfp = GET_DFP(value);
+   tcphy->map = GET_PIN_MAP(value);
+

I don't think it is a good idea to use the extcon 'state' field like
this. I don't even think it is possible.

The state is supposed to be a bit mask, each bit indicating if a
specific connector (or functionality) on the cable is attached. The
extcon notifier code walks through this bit mask and determines based
on changed bits if the notifier should be called. So the notifier in
this case would only be called if bit 1 (EXTCON_USB) of 'state' has
changed, but not if one of the other bits has changed. One would have
to define 32 "virtual" cables, one for each bit, for this to work, and
then you would have to register a notifier for each of the bits. That
would not really make sense.

Of course, that makes using the extcon notifier quite useless for our
purpose, since we need the callback not only if a cable has been
attached or deattached, but also if some secondary state changes. I
don't really know myself how to solve the problem; I'll need to think
about it some more. Maybe we can add a callback into the type-c
infrastructure code and somehow tie into that code, but I don't know
yet if that is feasible either.

Guenter



Yes, currently, we can get the notification only when bit 0 change.
So the phy driver can know plug/unplug event.
if we need more trigger, how about set the BIT 0 for changed flag.

state = extcon_get_cable_state

state = ~state | is_plugged | flip | dfp | map

extcon_set_state(state)

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Guenter

On 06/18/2016 11:45 PM, Guenter Roeck wrote:

Hi Chris,

On Mon, Jun 13, 2016 at 2:39 AM, 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.


I started integrating the driver with our code.
Doing so, I realized a problem in the way you are using extcon.

[ ... ]


+
+static int tcphy_pd_event(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{
+   struct rockchip_typec_phy *tcphy;
+   struct extcon_dev *edev = priv;
+   int value = edev->state;
+   int mode;
+   u8 is_plugged, dfp;
+
+   tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
+
+   is_plugged = GET_PLUGGED(value);
+   tcphy->flip = GET_FLIP(value);
+   dfp = GET_DFP(value);
+   tcphy->map = GET_PIN_MAP(value);
+

I don't think it is a good idea to use the extcon 'state' field like
this. I don't even think it is possible.

The state is supposed to be a bit mask, each bit indicating if a
specific connector (or functionality) on the cable is attached. The
extcon notifier code walks through this bit mask and determines based
on changed bits if the notifier should be called. So the notifier in
this case would only be called if bit 1 (EXTCON_USB) of 'state' has
changed, but not if one of the other bits has changed. One would have
to define 32 "virtual" cables, one for each bit, for this to work, and
then you would have to register a notifier for each of the bits. That
would not really make sense.

Of course, that makes using the extcon notifier quite useless for our
purpose, since we need the callback not only if a cable has been
attached or deattached, but also if some secondary state changes. I
don't really know myself how to solve the problem; I'll need to think
about it some more. Maybe we can add a callback into the type-c
infrastructure code and somehow tie into that code, but I don't know
yet if that is feasible either.

Guenter



Yes, currently, we can get the notification only when bit 0 change.
So the phy driver can know plug/unplug event.
if we need more trigger, how about set the BIT 0 for changed flag.

state = extcon_get_cable_state

state = ~state | is_plugged | flip | dfp | map

extcon_set_state(state)

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Kishon

On 06/17/2016 08:54 PM, Kishon Vijay Abraham I wrote:


On Monday 13 June 2016 03:09 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 
Signed-off-by: Kever Yang 

---

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(>dev, "tcpdcore")

  drivers/phy/Kconfig|   8 +
  drivers/phy/Makefile   |   1 +
  drivers/phy/phy-rockchip-typec.c   | 952 +
  include/linux/phy/phy-rockchip-typec.h |  20 +
  4 files changed, 981 insertions(+)
  create mode 100644 drivers/phy/phy-rockchip-typec.c
  create mode 100644 include/linux/phy/phy-rockchip-typec.h

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
+   select RESET_CONTROLLER
+   help
+ Enable this to support the Rockchip USB TYPEC PHY.
+
  config PHY_ST_SPEAR1310_MIPHY
tristate "ST SPEAR1310-MIPHY driver"
select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 24596a9..91fa413 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)   += 
phy-qcom-apq8064-sata.o
  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
  obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
+obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)   += phy-qcom-ipq806x-sata.o
  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)  += phy-spear1310-miphy.o
  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)  += phy-spear1340-miphy.o
diff --git a/drivers/phy/phy-rockchip-typec.c b/drivers/phy/phy-rockchip-typec.c
new file mode 100644
index 000..230e074
--- /dev/null
+++ b/drivers/phy/phy-rockchip-typec.c




+static const struct phy_ops rockchip_tcphy_ops = {
+   .owner  = THIS_MODULE,

no ops?

+};
+
+static int tcphy_pd_event(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{
+   struct rockchip_typec_phy *tcphy;
+   struct extcon_dev *edev = priv;
+   int value = edev->state;
+   int mode;
+   u8 is_plugged, dfp;
+
+   tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
+
+   is_plugged = GET_PLUGGED(value);
+   tcphy->flip = GET_FLIP(value);
+   dfp = GET_DFP(value);
+   tcphy->map = GET_PIN_MAP(value);
+
+   if (is_plugged) {
+   if (!dfp)
+   mode = MODE_UFP_USB;
+   else if (tcphy->map & (PIN_MAP_B | PIN_MAP_D | PIN_MAP_F))
+   mode = MODE_DFP_USB | MODE_DFP_DP;
+   else if (tcphy->map & (PIN_MAP_A | PIN_MAP_C | PIN_MAP_E))
+   mode = MODE_DFP_DP;
+   else
+   mode = MODE_DFP_USB;
+   } else {
+   mode = MODE_DISCONNECT;
+   }
+
+   if (tcphy->mode != mode) {
+   tcphy->mode = mode;
+   schedule_delayed_work_on(0, >event_wq, 0);
+   }
+
+   return 0;
+}
+
+static void tcphy_event_wq(struct work_struct *work)
+{
+   struct rockchip_typec_phy *tcphy;
+   int ret;
+
+   tcphy = container_of(work, struct rockchip_typec_phy, event_wq.work);
+
+   if (tcphy->mode == MODE_DISCONNECT) {
+   tcphy_phy_deinit(tcphy);
+   /* remove hpd sign for DP */
+   if (tcphy->hpd_status) {
+   regmap_write(tcphy->grf_regs, GRF_SOC_CON26,
+DPTX_HPD_SEL_MASK | DPTX_HPD_DEL);
+   tcphy->hpd_status = 0;
+   }
+   } else {
+   ret = tcphy_phy_init(tcphy);

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Kishon

On 06/17/2016 08:54 PM, Kishon Vijay Abraham I wrote:


On Monday 13 June 2016 03:09 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 
Signed-off-by: Kever Yang 

---

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(>dev, "tcpdcore")

  drivers/phy/Kconfig|   8 +
  drivers/phy/Makefile   |   1 +
  drivers/phy/phy-rockchip-typec.c   | 952 +
  include/linux/phy/phy-rockchip-typec.h |  20 +
  4 files changed, 981 insertions(+)
  create mode 100644 drivers/phy/phy-rockchip-typec.c
  create mode 100644 include/linux/phy/phy-rockchip-typec.h

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
+   select RESET_CONTROLLER
+   help
+ Enable this to support the Rockchip USB TYPEC PHY.
+
  config PHY_ST_SPEAR1310_MIPHY
tristate "ST SPEAR1310-MIPHY driver"
select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 24596a9..91fa413 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)   += 
phy-qcom-apq8064-sata.o
  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
  obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
+obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)   += phy-qcom-ipq806x-sata.o
  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)  += phy-spear1310-miphy.o
  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)  += phy-spear1340-miphy.o
diff --git a/drivers/phy/phy-rockchip-typec.c b/drivers/phy/phy-rockchip-typec.c
new file mode 100644
index 000..230e074
--- /dev/null
+++ b/drivers/phy/phy-rockchip-typec.c




+static const struct phy_ops rockchip_tcphy_ops = {
+   .owner  = THIS_MODULE,

no ops?

+};
+
+static int tcphy_pd_event(struct notifier_block *nb,
+ unsigned long event, void *priv)
+{
+   struct rockchip_typec_phy *tcphy;
+   struct extcon_dev *edev = priv;
+   int value = edev->state;
+   int mode;
+   u8 is_plugged, dfp;
+
+   tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
+
+   is_plugged = GET_PLUGGED(value);
+   tcphy->flip = GET_FLIP(value);
+   dfp = GET_DFP(value);
+   tcphy->map = GET_PIN_MAP(value);
+
+   if (is_plugged) {
+   if (!dfp)
+   mode = MODE_UFP_USB;
+   else if (tcphy->map & (PIN_MAP_B | PIN_MAP_D | PIN_MAP_F))
+   mode = MODE_DFP_USB | MODE_DFP_DP;
+   else if (tcphy->map & (PIN_MAP_A | PIN_MAP_C | PIN_MAP_E))
+   mode = MODE_DFP_DP;
+   else
+   mode = MODE_DFP_USB;
+   } else {
+   mode = MODE_DISCONNECT;
+   }
+
+   if (tcphy->mode != mode) {
+   tcphy->mode = mode;
+   schedule_delayed_work_on(0, >event_wq, 0);
+   }
+
+   return 0;
+}
+
+static void tcphy_event_wq(struct work_struct *work)
+{
+   struct rockchip_typec_phy *tcphy;
+   int ret;
+
+   tcphy = container_of(work, struct rockchip_typec_phy, event_wq.work);
+
+   if (tcphy->mode == MODE_DISCONNECT) {
+   tcphy_phy_deinit(tcphy);
+   /* remove hpd sign for DP */
+   if (tcphy->hpd_status) {
+   regmap_write(tcphy->grf_regs, GRF_SOC_CON26,
+DPTX_HPD_SEL_MASK | DPTX_HPD_DEL);
+   tcphy->hpd_status = 0;
+   }
+   } else {
+   ret = tcphy_phy_init(tcphy);
+   if (ret)
+  

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Guenter Roeck]

Hi Chris,

On Mon, Jun 13, 2016 at 2:39 AM, 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.
>

I started integrating the driver with our code.
Doing so, I realized a problem in the way you are using extcon.

[ ... ]

> +
> +static int tcphy_pd_event(struct notifier_block *nb,
> + unsigned long event, void *priv)
> +{
> +   struct rockchip_typec_phy *tcphy;
> +   struct extcon_dev *edev = priv;
> +   int value = edev->state;
> +   int mode;
> +   u8 is_plugged, dfp;
> +
> +   tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
> +
> +   is_plugged = GET_PLUGGED(value);
> +   tcphy->flip = GET_FLIP(value);
> +   dfp = GET_DFP(value);
> +   tcphy->map = GET_PIN_MAP(value);
> +

I don't think it is a good idea to use the extcon 'state' field like
this. I don't even think it is possible.

The state is supposed to be a bit mask, each bit indicating if a
specific connector (or functionality) on the cable is attached. The
extcon notifier code walks through this bit mask and determines based
on changed bits if the notifier should be called. So the notifier in
this case would only be called if bit 1 (EXTCON_USB) of 'state' has
changed, but not if one of the other bits has changed. One would have
to define 32 "virtual" cables, one for each bit, for this to work, and
then you would have to register a notifier for each of the bits. That
would not really make sense.

Of course, that makes using the extcon notifier quite useless for our
purpose, since we need the callback not only if a cable has been
attached or deattached, but also if some secondary state changes. I
don't really know myself how to solve the problem; I'll need to think
about it some more. Maybe we can add a callback into the type-c
infrastructure code and somehow tie into that code, but I don't know
yet if that is feasible either.

Guenter

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Guenter Roeck]

Hi Chris,

On Mon, Jun 13, 2016 at 2:39 AM, 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.
>

I started integrating the driver with our code.
Doing so, I realized a problem in the way you are using extcon.

[ ... ]

> +
> +static int tcphy_pd_event(struct notifier_block *nb,
> + unsigned long event, void *priv)
> +{
> +   struct rockchip_typec_phy *tcphy;
> +   struct extcon_dev *edev = priv;
> +   int value = edev->state;
> +   int mode;
> +   u8 is_plugged, dfp;
> +
> +   tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
> +
> +   is_plugged = GET_PLUGGED(value);
> +   tcphy->flip = GET_FLIP(value);
> +   dfp = GET_DFP(value);
> +   tcphy->map = GET_PIN_MAP(value);
> +

I don't think it is a good idea to use the extcon 'state' field like
this. I don't even think it is possible.

The state is supposed to be a bit mask, each bit indicating if a
specific connector (or functionality) on the cable is attached. The
extcon notifier code walks through this bit mask and determines based
on changed bits if the notifier should be called. So the notifier in
this case would only be called if bit 1 (EXTCON_USB) of 'state' has
changed, but not if one of the other bits has changed. One would have
to define 32 "virtual" cables, one for each bit, for this to work, and
then you would have to register a notifier for each of the bits. That
would not really make sense.

Of course, that makes using the extcon notifier quite useless for our
purpose, since we need the callback not only if a cable has been
attached or deattached, but also if some secondary state changes. I
don't really know myself how to solve the problem; I'll need to think
about it some more. Maybe we can add a callback into the type-c
infrastructure code and somehow tie into that code, but I don't know
yet if that is feasible either.

Guenter

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Tomasz Figa]

Hi Chris,

On Mon, Jun 13, 2016 at 6:39 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.

Please see my comments inline.

[snip]

> diff --git a/drivers/phy/phy-rockchip-typec.c 
> b/drivers/phy/phy-rockchip-typec.c
> new file mode 100644
> index 000..230e074
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-typec.c
> @@ -0,0 +1,952 @@
[snip]
> +
> +#define XCVR_PSM_RCTRL(n)  ((0x4001 | (n << 9)) << 2)

It's not very safe to not have parentheses around the macro argument
dereference. Please add to all macros, such as below:

#define XCVR_PSM_RCTRL(n)  ((0x4001 | ((n) << 9)) << 2)

> +#define XCVR_PSM_CAL_TMR(n)((0x4002 | (n << 9)) << 2)
> +#define XCVR_PSM_A0IN_TMR(n)   ((0x4003 | (n << 9)) << 2)
> +#define TX_TXCC_CAL_SCLR_MULT(n)   ((0x4047 | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_00(n)   ((0x404c | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_01(n)   ((0x404d | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_10(n)   ((0x404e | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_11(n)   ((0x404f | (n << 9)) << 2)
[snip]
> +#define PHY_MODE_SET_TIMEOUT   100
> +
> +#defineMODE_DISCONNECT 0
> +#defineMODE_UFP_USBBIT(0)
> +#defineMODE_DFP_USBBIT(1)
> +#defineMODE_DFP_DP BIT(2)

CodingStyle: Why is there a tab between #define and name?

> +
> +struct usb3phy_reg {
> +   u32 offset;
> +   u32 enable_bit;
> +   u32 write_enable;

CodingStyle: I believe it's generally preferable to just use a single
space between type and name. Also applies to other structs in this
file.

> +};
> +
> +struct rockchip_usb3phy_port_cfg {
> +   struct usb3phy_reg  typec_conn_dir;
> +   struct usb3phy_reg  usb3tousb2_en;
[snip]
> +struct phy_reg {
> +   int value;
> +   int addr;
> +};
> +
> +struct phy_reg usb_pll_cfg[] = {
> +   {0xf0,  CMN_PLL0_VCOCAL_INIT},

CodingStyle: Please add spaces after opening and before closing braces.

> +   {0x18,  CMN_PLL0_VCOCAL_ITER},
> +   {0xd0,  CMN_PLL0_INTDIV},
> +   {0x4a4a,CMN_PLL0_FRACDIV},
> +   {0x34,  CMN_PLL0_HIGH_THR},
> +   {0x1ee, CMN_PLL0_SS_CTRL1},
> +   {0x7f03,CMN_PLL0_SS_CTRL2},
> +   {0x20,  CMN_PLL0_DSM_DIAG},
> +   {0, CMN_DIAG_PLL0_OVRD},
> +   {0, CMN_DIAG_PLL0_FBH_OVRD},
> +   {0, CMN_DIAG_PLL0_FBL_OVRD},
> +   {0x7,   CMN_DIAG_PLL0_V2I_TUNE},
> +   {0x45,  CMN_DIAG_PLL0_CP_TUNE},
> +   {0x8,   CMN_DIAG_PLL0_LF_PROG},

It would be generally much, much better if these magic numbers were
dissected into particular bitfields and defined using macros, if
possible... The same applies to all other magic numbers in this file.

> +};
> +
> +struct phy_reg dp_pll_cfg[] = {
[snip]
> +static void tcphy_cfg_usb_pll(struct rockchip_typec_phy *tcphy)
> +{
> +   u32 rdata;
> +   u32 i;
> +
> +   /*
> +* Selects which PLL clock will be driven on the analog high speed
> +* clock 0: PLL 0 div 1.
> +*/
> +   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
> +   writel(rdata & 0xfffc, tcphy->base + CMN_DIAG_HSCLK_SEL);

This mask looks suspiciously. Is clearing bits 31-16 and 1-0 what we
want here? I'd advise for manipulating the value in separate line and
then only calling writel() with the final value already in the
variable.

> +
> +   /* load the configuration of PLL0 */
> +   for (i = 0; i < ARRAY_SIZE(usb_pll_cfg); i++)
> +   writel(usb_pll_cfg[i].value, tcphy->base + 
> usb_pll_cfg[i].addr);
> +}
> +
> +static void tcphy_cfg_dp_pll(struct rockchip_typec_phy *tcphy)
> +{
> +   u32 rdata;
> +   u32 i;
> +
> +   /* set the default mode to RBR */
> +   writel(DP_PLL_CLOCK_ENABLE | DP_PLL_ENABLE | DP_PLL_DATA_RATE_RBR,
> +  tcphy->base + DP_CLK_CTL);

This looks (and is understandable) much better than magic numbers in
other parts of this file!

> +
> +   /*
> +* Selects which PLL clock will be driven on the analog high speed
> +* clock 1: PLL 1 div 2.
> +*/
> +   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
> +   rdata = (rdata & 0xffcf) | 0x30;

If the & operation here is used to clear a bitfield, please use the
negative notation, e.g.

rdata &= ~0x30;
rdata |= 0x30;

(By the way, the AND NOT and OR with the same value is what the code
above does, which would make sense if the bitfield mask was defined by
a macro, but doesn't make any sense with magic numbers.)

It 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Tomasz Figa]

Hi Chris,

On Mon, Jun 13, 2016 at 6:39 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.

Please see my comments inline.

[snip]

> diff --git a/drivers/phy/phy-rockchip-typec.c 
> b/drivers/phy/phy-rockchip-typec.c
> new file mode 100644
> index 000..230e074
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-typec.c
> @@ -0,0 +1,952 @@
[snip]
> +
> +#define XCVR_PSM_RCTRL(n)  ((0x4001 | (n << 9)) << 2)

It's not very safe to not have parentheses around the macro argument
dereference. Please add to all macros, such as below:

#define XCVR_PSM_RCTRL(n)  ((0x4001 | ((n) << 9)) << 2)

> +#define XCVR_PSM_CAL_TMR(n)((0x4002 | (n << 9)) << 2)
> +#define XCVR_PSM_A0IN_TMR(n)   ((0x4003 | (n << 9)) << 2)
> +#define TX_TXCC_CAL_SCLR_MULT(n)   ((0x4047 | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_00(n)   ((0x404c | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_01(n)   ((0x404d | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_10(n)   ((0x404e | (n << 9)) << 2)
> +#define TX_TXCC_CPOST_MULT_11(n)   ((0x404f | (n << 9)) << 2)
[snip]
> +#define PHY_MODE_SET_TIMEOUT   100
> +
> +#defineMODE_DISCONNECT 0
> +#defineMODE_UFP_USBBIT(0)
> +#defineMODE_DFP_USBBIT(1)
> +#defineMODE_DFP_DP BIT(2)

CodingStyle: Why is there a tab between #define and name?

> +
> +struct usb3phy_reg {
> +   u32 offset;
> +   u32 enable_bit;
> +   u32 write_enable;

CodingStyle: I believe it's generally preferable to just use a single
space between type and name. Also applies to other structs in this
file.

> +};
> +
> +struct rockchip_usb3phy_port_cfg {
> +   struct usb3phy_reg  typec_conn_dir;
> +   struct usb3phy_reg  usb3tousb2_en;
[snip]
> +struct phy_reg {
> +   int value;
> +   int addr;
> +};
> +
> +struct phy_reg usb_pll_cfg[] = {
> +   {0xf0,  CMN_PLL0_VCOCAL_INIT},

CodingStyle: Please add spaces after opening and before closing braces.

> +   {0x18,  CMN_PLL0_VCOCAL_ITER},
> +   {0xd0,  CMN_PLL0_INTDIV},
> +   {0x4a4a,CMN_PLL0_FRACDIV},
> +   {0x34,  CMN_PLL0_HIGH_THR},
> +   {0x1ee, CMN_PLL0_SS_CTRL1},
> +   {0x7f03,CMN_PLL0_SS_CTRL2},
> +   {0x20,  CMN_PLL0_DSM_DIAG},
> +   {0, CMN_DIAG_PLL0_OVRD},
> +   {0, CMN_DIAG_PLL0_FBH_OVRD},
> +   {0, CMN_DIAG_PLL0_FBL_OVRD},
> +   {0x7,   CMN_DIAG_PLL0_V2I_TUNE},
> +   {0x45,  CMN_DIAG_PLL0_CP_TUNE},
> +   {0x8,   CMN_DIAG_PLL0_LF_PROG},

It would be generally much, much better if these magic numbers were
dissected into particular bitfields and defined using macros, if
possible... The same applies to all other magic numbers in this file.

> +};
> +
> +struct phy_reg dp_pll_cfg[] = {
[snip]
> +static void tcphy_cfg_usb_pll(struct rockchip_typec_phy *tcphy)
> +{
> +   u32 rdata;
> +   u32 i;
> +
> +   /*
> +* Selects which PLL clock will be driven on the analog high speed
> +* clock 0: PLL 0 div 1.
> +*/
> +   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
> +   writel(rdata & 0xfffc, tcphy->base + CMN_DIAG_HSCLK_SEL);

This mask looks suspiciously. Is clearing bits 31-16 and 1-0 what we
want here? I'd advise for manipulating the value in separate line and
then only calling writel() with the final value already in the
variable.

> +
> +   /* load the configuration of PLL0 */
> +   for (i = 0; i < ARRAY_SIZE(usb_pll_cfg); i++)
> +   writel(usb_pll_cfg[i].value, tcphy->base + 
> usb_pll_cfg[i].addr);
> +}
> +
> +static void tcphy_cfg_dp_pll(struct rockchip_typec_phy *tcphy)
> +{
> +   u32 rdata;
> +   u32 i;
> +
> +   /* set the default mode to RBR */
> +   writel(DP_PLL_CLOCK_ENABLE | DP_PLL_ENABLE | DP_PLL_DATA_RATE_RBR,
> +  tcphy->base + DP_CLK_CTL);

This looks (and is understandable) much better than magic numbers in
other parts of this file!

> +
> +   /*
> +* Selects which PLL clock will be driven on the analog high speed
> +* clock 1: PLL 1 div 2.
> +*/
> +   rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
> +   rdata = (rdata & 0xffcf) | 0x30;

If the & operation here is used to clear a bitfield, please use the
negative notation, e.g.

rdata &= ~0x30;
rdata |= 0x30;

(By the way, the AND NOT and OR with the same value is what the code
above does, which would make sense if the bitfield mask was defined by
a macro, but doesn't make any sense with magic numbers.)

It looks like the 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Kishon Vijay Abraham I]

On Monday 13 June 2016 03:09 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 
> Signed-off-by: Kever Yang 
> 
> ---
> 
> 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(>dev, "tcpdcore")
> 
>  drivers/phy/Kconfig|   8 +
>  drivers/phy/Makefile   |   1 +
>  drivers/phy/phy-rockchip-typec.c   | 952 
> +
>  include/linux/phy/phy-rockchip-typec.h |  20 +
>  4 files changed, 981 insertions(+)
>  create mode 100644 drivers/phy/phy-rockchip-typec.c
>  create mode 100644 include/linux/phy/phy-rockchip-typec.h
> 
> 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
> + select RESET_CONTROLLER
> + help
> +   Enable this to support the Rockchip USB TYPEC PHY.
> +
>  config PHY_ST_SPEAR1310_MIPHY
>   tristate "ST SPEAR1310-MIPHY driver"
>   select GENERIC_PHY
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 24596a9..91fa413 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_QCOM_APQ8064_SATA) += 
> phy-qcom-apq8064-sata.o
>  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
>  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
>  obj-$(CONFIG_PHY_ROCKCHIP_DP)+= phy-rockchip-dp.o
> +obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
>  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)  += phy-qcom-ipq806x-sata.o
>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY) += phy-spear1310-miphy.o
>  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) += phy-spear1340-miphy.o
> diff --git a/drivers/phy/phy-rockchip-typec.c 
> b/drivers/phy/phy-rockchip-typec.c
> new file mode 100644
> index 000..230e074
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-typec.c



> +static const struct phy_ops rockchip_tcphy_ops = {
> + .owner  = THIS_MODULE,

no ops?
> +};
> +
> +static int tcphy_pd_event(struct notifier_block *nb,
> +   unsigned long event, void *priv)
> +{
> + struct rockchip_typec_phy *tcphy;
> + struct extcon_dev *edev = priv;
> + int value = edev->state;
> + int mode;
> + u8 is_plugged, dfp;
> +
> + tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
> +
> + is_plugged = GET_PLUGGED(value);
> + tcphy->flip = GET_FLIP(value);
> + dfp = GET_DFP(value);
> + tcphy->map = GET_PIN_MAP(value);
> +
> + if (is_plugged) {
> + if (!dfp)
> + mode = MODE_UFP_USB;
> + else if (tcphy->map & (PIN_MAP_B | PIN_MAP_D | PIN_MAP_F))
> + mode = MODE_DFP_USB | MODE_DFP_DP;
> + else if (tcphy->map & (PIN_MAP_A | PIN_MAP_C | PIN_MAP_E))
> + mode = MODE_DFP_DP;
> + else
> + mode = MODE_DFP_USB;
> + } else {
> + mode = MODE_DISCONNECT;
> + }
> +
> + if (tcphy->mode != mode) {
> + tcphy->mode = mode;
> + schedule_delayed_work_on(0, >event_wq, 0);
> + }
> +
> + return 0;
> +}
> +
> +static void tcphy_event_wq(struct work_struct *work)
> +{
> + struct rockchip_typec_phy *tcphy;
> + int ret;
> +
> + tcphy = container_of(work, struct rockchip_typec_phy, event_wq.work);
> +
> + if (tcphy->mode == MODE_DISCONNECT) {
> + tcphy_phy_deinit(tcphy);
> + /* remove hpd sign for DP */
> + if (tcphy->hpd_status) {
> + regmap_write(tcphy->grf_regs, GRF_SOC_CON26,
> +  DPTX_HPD_SEL_MASK | DPTX_HPD_DEL);
> + 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Kishon Vijay Abraham I]

On Monday 13 June 2016 03:09 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 
> Signed-off-by: Kever Yang 
> 
> ---
> 
> 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(>dev, "tcpdcore")
> 
>  drivers/phy/Kconfig|   8 +
>  drivers/phy/Makefile   |   1 +
>  drivers/phy/phy-rockchip-typec.c   | 952 
> +
>  include/linux/phy/phy-rockchip-typec.h |  20 +
>  4 files changed, 981 insertions(+)
>  create mode 100644 drivers/phy/phy-rockchip-typec.c
>  create mode 100644 include/linux/phy/phy-rockchip-typec.h
> 
> 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
> + select RESET_CONTROLLER
> + help
> +   Enable this to support the Rockchip USB TYPEC PHY.
> +
>  config PHY_ST_SPEAR1310_MIPHY
>   tristate "ST SPEAR1310-MIPHY driver"
>   select GENERIC_PHY
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 24596a9..91fa413 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_QCOM_APQ8064_SATA) += 
> phy-qcom-apq8064-sata.o
>  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
>  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
>  obj-$(CONFIG_PHY_ROCKCHIP_DP)+= phy-rockchip-dp.o
> +obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
>  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)  += phy-qcom-ipq806x-sata.o
>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY) += phy-spear1310-miphy.o
>  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) += phy-spear1340-miphy.o
> diff --git a/drivers/phy/phy-rockchip-typec.c 
> b/drivers/phy/phy-rockchip-typec.c
> new file mode 100644
> index 000..230e074
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-typec.c



> +static const struct phy_ops rockchip_tcphy_ops = {
> + .owner  = THIS_MODULE,

no ops?
> +};
> +
> +static int tcphy_pd_event(struct notifier_block *nb,
> +   unsigned long event, void *priv)
> +{
> + struct rockchip_typec_phy *tcphy;
> + struct extcon_dev *edev = priv;
> + int value = edev->state;
> + int mode;
> + u8 is_plugged, dfp;
> +
> + tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
> +
> + is_plugged = GET_PLUGGED(value);
> + tcphy->flip = GET_FLIP(value);
> + dfp = GET_DFP(value);
> + tcphy->map = GET_PIN_MAP(value);
> +
> + if (is_plugged) {
> + if (!dfp)
> + mode = MODE_UFP_USB;
> + else if (tcphy->map & (PIN_MAP_B | PIN_MAP_D | PIN_MAP_F))
> + mode = MODE_DFP_USB | MODE_DFP_DP;
> + else if (tcphy->map & (PIN_MAP_A | PIN_MAP_C | PIN_MAP_E))
> + mode = MODE_DFP_DP;
> + else
> + mode = MODE_DFP_USB;
> + } else {
> + mode = MODE_DISCONNECT;
> + }
> +
> + if (tcphy->mode != mode) {
> + tcphy->mode = mode;
> + schedule_delayed_work_on(0, >event_wq, 0);
> + }
> +
> + return 0;
> +}
> +
> +static void tcphy_event_wq(struct work_struct *work)
> +{
> + struct rockchip_typec_phy *tcphy;
> + int ret;
> +
> + tcphy = container_of(work, struct rockchip_typec_phy, event_wq.work);
> +
> + if (tcphy->mode == MODE_DISCONNECT) {
> + tcphy_phy_deinit(tcphy);
> + /* remove hpd sign for DP */
> + if (tcphy->hpd_status) {
> + regmap_write(tcphy->grf_regs, GRF_SOC_CON26,
> +  DPTX_HPD_SEL_MASK | DPTX_HPD_DEL);
> + tcphy->hpd_status = 0;
> + }
> + } 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Kever Yang]

Hi Chris,

On 06/13/2016 05:39 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 
Signed-off-by: Kever Yang 

---

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(>dev, "tcpdcore")

  drivers/phy/Kconfig|   8 +
  drivers/phy/Makefile   |   1 +
  drivers/phy/phy-rockchip-typec.c   | 952 +
  include/linux/phy/phy-rockchip-typec.h |  20 +
  4 files changed, 981 insertions(+)
  create mode 100644 drivers/phy/phy-rockchip-typec.c
  create mode 100644 include/linux/phy/phy-rockchip-typec.h

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
+   select RESET_CONTROLLER
+   help
+ Enable this to support the Rockchip USB TYPEC PHY.
+
  config PHY_ST_SPEAR1310_MIPHY
tristate "ST SPEAR1310-MIPHY driver"
select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 24596a9..91fa413 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)   += 
phy-qcom-apq8064-sata.o
  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
  obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
+obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)   += phy-qcom-ipq806x-sata.o
  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)  += phy-spear1310-miphy.o
  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)  += phy-spear1340-miphy.o
diff --git a/drivers/phy/phy-rockchip-typec.c b/drivers/phy/phy-rockchip-typec.c
new file mode 100644
index 000..230e074
--- /dev/null
+++ b/drivers/phy/phy-rockchip-typec.c
@@ -0,0 +1,952 @@
+/*
+ * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
+ * Author: Chris Zhong 
+ * Kever Yang 
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define CMN_SSM_BANDGAP(0x21 << 2)
+#define CMN_SSM_BIAS   (0x22 << 2)
+#define CMN_PLLSM0_PLLEN   (0x29 << 2)
+#define CMN_PLLSM0_PLLPRE  (0x2a << 2)
+#define CMN_PLLSM0_PLLVREF (0x2b << 2)
+#define CMN_PLLSM0_PLLLOCK (0x2c << 2)
+#define CMN_PLLSM1_PLLEN   (0x31 << 2)
+#define CMN_PLLSM1_PLLPRE  (0x32 << 2)
+#define CMN_PLLSM1_PLLVREF (0x33 << 2)
+#define CMN_PLLSM1_PLLLOCK (0x34 << 2)
+#define CMN_PLLSM1_USER_DEF_CTRL   (0x37 << 2)
+#define CMN_ICAL_OVRD  (0xc1 << 2)
+#define CMN_PLL0_VCOCAL_OVRD   (0x83 << 2)
+#define CMN_PLL0_VCOCAL_INIT   (0x84 << 2)
+#define CMN_PLL0_VCOCAL_ITER   (0x85 << 2)
+#define CMN_PLL0_LOCK_REFCNT_START (0x90 << 2)
+#define CMN_PLL0_LOCK_PLLCNT_START (0x92 << 2)
+#define CMN_PLL0_LOCK_PLLCNT_THR   (0x93 << 2)
+#define CMN_PLL0_INTDIV(0x94 << 2)
+#define CMN_PLL0_FRACDIV   (0x95 << 2)
+#define CMN_PLL0_HIGH_THR  (0x96 << 2)
+#define 

Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Kever Yang]

Hi Chris,

On 06/13/2016 05:39 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 
Signed-off-by: Kever Yang 

---

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(>dev, "tcpdcore")

  drivers/phy/Kconfig|   8 +
  drivers/phy/Makefile   |   1 +
  drivers/phy/phy-rockchip-typec.c   | 952 +
  include/linux/phy/phy-rockchip-typec.h |  20 +
  4 files changed, 981 insertions(+)
  create mode 100644 drivers/phy/phy-rockchip-typec.c
  create mode 100644 include/linux/phy/phy-rockchip-typec.h

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
+   select RESET_CONTROLLER
+   help
+ Enable this to support the Rockchip USB TYPEC PHY.
+
  config PHY_ST_SPEAR1310_MIPHY
tristate "ST SPEAR1310-MIPHY driver"
select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 24596a9..91fa413 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)   += 
phy-qcom-apq8064-sata.o
  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
  obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
  obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
+obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)   += phy-qcom-ipq806x-sata.o
  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)  += phy-spear1310-miphy.o
  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)  += phy-spear1340-miphy.o
diff --git a/drivers/phy/phy-rockchip-typec.c b/drivers/phy/phy-rockchip-typec.c
new file mode 100644
index 000..230e074
--- /dev/null
+++ b/drivers/phy/phy-rockchip-typec.c
@@ -0,0 +1,952 @@
+/*
+ * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
+ * Author: Chris Zhong 
+ * Kever Yang 
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define CMN_SSM_BANDGAP(0x21 << 2)
+#define CMN_SSM_BIAS   (0x22 << 2)
+#define CMN_PLLSM0_PLLEN   (0x29 << 2)
+#define CMN_PLLSM0_PLLPRE  (0x2a << 2)
+#define CMN_PLLSM0_PLLVREF (0x2b << 2)
+#define CMN_PLLSM0_PLLLOCK (0x2c << 2)
+#define CMN_PLLSM1_PLLEN   (0x31 << 2)
+#define CMN_PLLSM1_PLLPRE  (0x32 << 2)
+#define CMN_PLLSM1_PLLVREF (0x33 << 2)
+#define CMN_PLLSM1_PLLLOCK (0x34 << 2)
+#define CMN_PLLSM1_USER_DEF_CTRL   (0x37 << 2)
+#define CMN_ICAL_OVRD  (0xc1 << 2)
+#define CMN_PLL0_VCOCAL_OVRD   (0x83 << 2)
+#define CMN_PLL0_VCOCAL_INIT   (0x84 << 2)
+#define CMN_PLL0_VCOCAL_ITER   (0x85 << 2)
+#define CMN_PLL0_LOCK_REFCNT_START (0x90 << 2)
+#define CMN_PLL0_LOCK_PLLCNT_START (0x92 << 2)
+#define CMN_PLL0_LOCK_PLLCNT_THR   (0x93 << 2)
+#define CMN_PLL0_INTDIV(0x94 << 2)
+#define CMN_PLL0_FRACDIV   (0x95 << 2)
+#define CMN_PLL0_HIGH_THR  (0x96 << 2)
+#define CMN_PLL0_DSM_DIAG  (0x97 << 2)
+#define CMN_PLL0_SS_CTRL1  (0x98 << 2)