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

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:

quoted

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 <redacted>
Signed-off-by: Kever Yang <redacted>

---

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       | 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 0000000..230e074
--- /dev/null
+++ b/drivers/phy/phy-rockchip-typec.c

<snip>

quoted

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

no ops?

quoted

+};
+
+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, &tcphy->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)
+			return;
+
+		if (tcphy->mode & (MODE_UFP_USB | MODE_DFP_USB))
+			tcphy_usb3_init(tcphy);
+
+		if (tcphy->mode & MODE_DFP_DP) {
+			ret = tcphy_dp_init(tcphy);
+
+			/* set hpd sign for DP, if DP phy is ready */
+			if (!ret) {
+				regmap_write(tcphy->grf_regs, GRF_SOC_CON26,
+					     DPTX_HPD_SEL_MASK | DPTX_HPD_SEL);
+				tcphy->hpd_status = 1;
+			}
+		}
+	}
+}
+
+static int tcphy_get_param(struct device *dev,
+			   struct usb3phy_reg *reg,
+			   const char *name)
+{
+	int ret, buffer[3];
+
+	ret = of_property_read_u32_array(dev->of_node, name, buffer, 3);
+	if (ret) {
+		dev_err(dev, "Can not parse %s\n", name);
+		return ret;
+	}
+
+	reg->offset = buffer[0];
+	reg->enable_bit = buffer[1];
+	reg->write_enable = buffer[2];
+	return 0;
+}
+
+static int tcphy_parse_dt(struct rockchip_typec_phy *tcphy,
+			  struct device *dev)
+{
+	struct rockchip_usb3phy_port_cfg *cfg = &tcphy->port_cfgs;
+	int ret;
+
+	ret = tcphy_get_param(dev, &cfg->typec_conn_dir,
+			      "rockchip,typec_conn_dir");
+	if (ret)
+		return ret;
+
+	ret = tcphy_get_param(dev, &cfg->usb3tousb2_en,
+			      "rockchip,usb3tousb2_en");
+	if (ret)
+		return ret;
+
+	ret = tcphy_get_param(dev, &cfg->external_psm,
+			      "rockchip,external_psm");
+	if (ret)
+		return ret;
+
+	ret = tcphy_get_param(dev, &cfg->pipe_status,
+			      "rockchip,pipe_status");
+	if (ret)
+		return ret;
+
+	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
+			      "rockchip,uphy_dp_sel");
+	if (ret)
+		return ret;
+
+	tcphy->grf_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
+							  "rockchip,grf");
+	if (IS_ERR(tcphy->grf_regs)) {
+		dev_err(dev, "could not find grf dt node\n");
+		return PTR_ERR(tcphy->grf_regs);
+	}
+
+	tcphy->clk_core = devm_clk_get(dev, "tcpdcore");
+	if (IS_ERR(tcphy->clk_core)) {
+		dev_err(dev, "could not get uphy core clock\n");
+		return PTR_ERR(tcphy->clk_core);
+	}
+
+	tcphy->clk_ref = devm_clk_get(dev, "tcpdphy_ref");
+	if (IS_ERR(tcphy->clk_ref)) {
+		dev_err(dev, "could not get uphy ref clock\n");
+		return PTR_ERR(tcphy->clk_ref);
+	}
+
+	tcphy->phy_rst = devm_reset_control_get(dev, "tcphy");
+	if (IS_ERR(tcphy->phy_rst)) {
+		dev_err(dev, "no phy_rst reset control found\n");
+		return PTR_ERR(tcphy->phy_rst);
+	}
+
+	tcphy->pipe_rst = devm_reset_control_get(dev, "tcphy_pipe");
+	if (IS_ERR(tcphy->pipe_rst)) {
+		dev_err(dev, "no pipe_rst reset control found\n");
+		return PTR_ERR(tcphy->pipe_rst);
+	}
+
+	tcphy->uphy_rst = devm_reset_control_get(dev, "uphy_tcphy");
+	if (IS_ERR(tcphy->uphy_rst)) {
+		dev_err(dev, "no uphy_rst reset control found\n");
+		return PTR_ERR(tcphy->uphy_rst);
+	}
+
+	return 0;
+}
+
+static int rockchip_typec_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_typec_phy *tcphy;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int ret;
+
+	tcphy = devm_kzalloc(dev, sizeof(*tcphy), GFP_KERNEL);
+	if (!tcphy)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tcphy->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(tcphy->base)) {
+		dev_err(dev, "failed to remap phy regs\n");
+		return PTR_ERR(tcphy->base);
+	}
+
+	tcphy_parse_dt(tcphy, dev);
+
+	tcphy->dev = dev;
+	platform_set_drvdata(pdev, tcphy);
+
+	tcphy->mode = MODE_DISCONNECT;
+
+	tcphy->phy = devm_phy_create(dev, NULL, &rockchip_tcphy_ops);

your phy_ops doesn't have any ops populated. Is there a reason to use the phy
framework?

Thanks
Kishon

Yes, I am thinking about it, remove it would be better.