Message-ID: <12a93817-0d3c-b35c-2e02-91f8ed2ee2a6@ti.com>
Date:   Thu, 27 Oct 2016 01:26:24 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Vivek Gautam <vivek.gautam@...eaurora.org>, <robh+dt@...nel.org>,
        <mark.rutland@....com>
CC:     <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH 1/2] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom
 chips

Hi,

On Wednesday 19 October 2016 04:13 PM, Vivek Gautam wrote:
> PHY transceiver driver for QUSB2 phy controller that provides
> HighSpeed functionality for DWC3 controller present on
> Qualcomm chipsets.
> 
> This driver is based on phy-msm-qusb driver available in
> msm-4.4 kernel @codeaurora[1]
> 
> [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/log/?h=caf/3.18/msm-3.18

I'd prefer only LKML links in commit msg. This can be part of the comment
section below ---.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@...eaurora.org>
> Cc: Kishon Vijay Abraham I <kishon@...com>
> ---
>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt     |  37 ++
>  drivers/phy/Kconfig                                |  10 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/phy-qcom-qusb2.c                       | 577 +++++++++++++++++++++
>  4 files changed, 625 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>  create mode 100644 drivers/phy/phy-qcom-qusb2.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> new file mode 100644
> index 0000000..97c9ce7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> @@ -0,0 +1,37 @@
> +Qualcomm QUSB2 phy controller
> +=============================
> +
> +QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
> +
> +Required properties:
> + - compatible: compatible list, contains "qcom,msm8996-qusb2-phy".
> + - reg: offset and length of the PHY register set.
> + - #phy-cells: must be 0.
> +
> + - clocks: a list of phandles and clock-specifier pairs,
> +	   one for each entry in clock-names.
> + - clock-names: must be "cfg_ahb" for phy config clock,
> +			"ref_clk" for 19.2 MHz ref clk,
> +			"ref_clk_src" reference clock source.
> +			"iface" for phy interface clock (Optional).
> +
> + - vdd-phy-supply: Phandle to a regulator supply to PHY core block.
> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> + - vdda-phy-dpdm: Phandle to 3.1V regulator supply to Dp/Dm port signals.
> +
> + - resets: a list of phandles and reset controller specifier pairs,
> +	   one for each entry in reset-names.
> + - reset-names: must be "phy" for reset of phy block.
> +
> +Optional properties:
> + - nvmem-cells: a list of phandles to nvmem cells that contain fused
> +		tuning parameters for qusb2 phy, one for each entry
> +		in nvmem-cell-names.
> + - nvmem-cell-names: must be "tune2_hstx_trim_efuse" for cell containing
> +		     HS Tx trim value.
> + - qcom,hstx-trim-bit-offset: bit offset within nvmem cell for
> +			      HS Tx trim value.
> + - qcom,hstx-trim-bit-len: bit length of HS Tx trim value within nvmem cell.
> +
> + - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
> + - qcom,phy-clk-scheme: Offset to TCSR_PHY_CLK_SCHEME_SEL register.
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index fe00f91..5547984 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -446,6 +446,16 @@ config PHY_STIH41X_USB
>  	  Enable this to support the USB transceiver that is part of
>  	  STMicroelectronics STiH41x SoC series.
>  
> +config PHY_QCOM_QUSB2
> +	tristate "Qualcomm QUSB2 PHY Driver"
> +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the HighSpeed QUSB2 PHY transceiver for USB
> +	  controllers on Qualcomm chips. This driver supports the high-speed
> +	  PHY which is usually paired with either the ChipIdea or Synopsys DWC3
> +	  USB IPs on MSM SOCs.
> +
>  config PHY_QCOM_UFS
>  	tristate "Qualcomm UFS PHY driver"
>  	depends on OF && ARCH_QCOM
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index a534cf5..848489d 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
>  obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
>  obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
>  obj-$(CONFIG_PHY_STIH41X_USB)		+= phy-stih41x-usb.o
> +obj-$(CONFIG_PHY_QCOM_QUSB2) 	+= phy-qcom-qusb2.o
>  obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs.o
>  obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-20nm.o
>  obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-14nm.o
> diff --git a/drivers/phy/phy-qcom-qusb2.c b/drivers/phy/phy-qcom-qusb2.c
> new file mode 100644
> index 0000000..058f661
> --- /dev/null
> +++ b/drivers/phy/phy-qcom-qusb2.c
> @@ -0,0 +1,577 @@
> +/*
> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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 <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define QUSB2PHY_PLL_TEST		0x04
> +#define CLK_REF_SEL			BIT(7)
> +
> +#define QUSB2PHY_PLL_TUNE		0x08
> +#define QUSB2PHY_PLL_USER_CTL1		0x0c
> +#define QUSB2PHY_PLL_USER_CTL2		0x10
> +#define QUSB2PHY_PLL_AUTOPGM_CTL1	0x1c
> +#define QUSB2PHY_PLL_PWR_CTRL		0x18
> +
> +#define QUSB2PHY_PLL_STATUS		0x38
> +#define PLL_LOCKED			BIT(5)
> +
> +#define QUSB2PHY_PORT_TUNE1             0x80
> +
> +#define QUSB2PHY_PORT_TUNE2             0x84
> +/* TUNE2's high nibble value read from efuse */
> +#define TUNE2_HIGH_NIBBLE_VAL(val, pos, mask)	((val >> pos) & mask)
> +
> +#define QUSB2PHY_PORT_TUNE3             0x88
> +#define QUSB2PHY_PORT_TUNE4             0x8C
> +#define QUSB2PHY_PORT_TUNE5		0x90tx
> +#define QUSB2PHY_PORT_TEST2		0x9c
> +
> +#define QUSB2PHY_PORT_POWERDOWN		0xB4
> +#define CLAMP_N_EN			BIT(5)
> +#define FREEZIO_N			BIT(1)
> +#define POWER_DOWN			BIT(0)
> +
> +#define QUSB2PHY_REFCLK_ENABLE		BIT(0)
> +
> +#define PHY_CLK_SCHEME_SEL		BIT(0)
> +
> +struct qusb2_phy_init_tbl {
> +	unsigned int reg_offset;
> +	unsigned int cfg_val;
> +};
> +#define QCOM_QUSB2_PHY_INIT_CFG(reg, val) \
> +	{				\
> +		.reg_offset = reg,	\
> +		.cfg_val = val,		\
> +	}
> +
> +static struct qusb2_phy_init_tbl msm8996_phy_init_tbl[] = {
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE1, 0xF8),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE2, 0xB3),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE3, 0x83),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE4, 0xC0),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_TUNE, 0x30),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL1, 0x79),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL2, 0x21),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TEST2, 0x14),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_AUTOPGM_CTL1, 0x9F),
> +	QCOM_QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00),
> +};
we should generalize this so that it could be used for any phys. All these
tuning parameters can come from dt.

phy,tx-swing = <reg_offset> <swing value>;
phy,rx-swing = <reg_offset> <swing value>;
phy,tx-compensation = <reg_offset> <swing value>;
phy,rx-compensation = <reg_offset> <swing value>;
..

We have to identify all the calibration parameters and add a dt property for
each parameter. The phy-core can just use these properties to calibrate or tune
the PHY.

> +
> +struct qusb2_phy_init_cfg {
> +	struct qusb2_phy_init_tbl *phy_init_tbl;
> +	int phy_init_tbl_sz;
> +};
> +
> +const struct qusb2_phy_init_cfg msm8996_phy_init_cfg = {
> +	.phy_init_tbl = msm8996_phy_init_tbl,
> +	.phy_init_tbl_sz = ARRAY_SIZE(msm8996_phy_init_tbl),
> +};
> +
> +/**
> + * struct qusb2_phy: Structure holding qusb2 phy attributes.
> + *
> + * @phy: pointer to generic phy.
> + * @base: pointer to iomapped memory space for qubs2 phy.
> + *
> + * @cfg_ahb_clk: pointer to AHB2PHY interface clock.
> + * @ref_clk: pointer to reference clock.
> + * @ref_clk_src: pointer to source to reference clock.
> + * @iface_src: pointer to phy interface clock.
> + * @clk_enabled: check if clocks are enabled.

Why is this needed? IMO this should not be used. clk framework can handle this
better since it maintains a reference count for clk_enable/clk_disable.
> + *
> + * @phy_reset: Pointer to phy reset control
> + *
> + * @vdda_phy: vdd supply to the phy core block.
> + * @vdda_pll: 1.8V vdd supply to ref_clk block.
> + * @vdda_phy_dpdm: 3.1V vdd supply to Dp/Dm port signals.
> + * @pwr_enabled: check if the regulators are enabled.

same here.
> + * @tcsr: pointer to TCSR syscon register map.
> + * @clk_scheme_offset: offset to PHY_CLK_SCHEME register in TCSR map.
> + * @cfg: phy initialization config data
> + */
> +struct qusb2_phy {
> +	struct phy *phy;
> +	void __iomem *base;
> +
> +	struct clk *cfg_ahb_clk;
> +	struct clk *ref_clk;
> +	struct clk *ref_clk_src;
> +	struct clk *iface_clk;
> +	bool clk_enabled;
> +
> +	struct reset_control *phy_reset;
> +
> +	struct regulator *vdd_phy;
> +	struct regulator *vdda_pll;
> +	struct regulator *vdda_phy_dpdm;
> +	bool pwr_enabled;
> +
> +	struct regmap *tcsr;
> +	unsigned int clk_scheme_offset;
> +
> +	const struct qusb2_phy_init_cfg *cfg;
> +};
> +
> +static inline void qusb2_set_bits(void __iomem *reg, u32 mask)

mask is misleading. just use val.
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(reg);
> +	val |= mask;
> +	writel_relaxed(val, reg);
> +
> +	/* Ensure above write is completed */
> +	mb();
> +}
> +
> +static inline void qusb2_clr_bits(void __iomem *reg, u32 mask)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(reg);
> +	val &= ~mask;
> +	writel_relaxed(val, reg);
> +
> +	/* Ensure above write is completed */
> +	mb();
> +}
> +
> +static void qcom_qusb2_phy_configure(void __iomem *base,
> +				struct qusb2_phy_init_tbl init_tbl[],
> +				int init_tbl_sz)
> +{
> +	int i;
> +
> +	for (i = 0; i < init_tbl_sz; i++) {
> +		writel_relaxed(init_tbl[i].cfg_val,
> +				base + init_tbl[i].reg_offset);
> +	}
> +
> +	/* flush buffered writes */
> +	mb();
> +}
> +
> +static void qusb2_phy_enable_clocks(struct qusb2_phy *qphy, bool on)
> +{
> +	dev_dbg(&qphy->phy->dev, "%s(): clocks_enabled:%d on:%d\n",
> +			__func__, qphy->clk_enabled, on);

unnecessary debug print IMO, given that there is another debug print before the
function returns.
> +
> +	if (!qphy->clk_enabled && on) {
> +		clk_prepare_enable(qphy->cfg_ahb_clk);
> +		if (qphy->iface_clk)
> +			clk_prepare_enable(qphy->iface_clk);
> +		clk_prepare_enable(qphy->ref_clk_src);
> +		qphy->clk_enabled = true;
> +	}
> +
> +	if (qphy->clk_enabled && !on) {
> +		clk_disable_unprepare(qphy->ref_clk_src);
> +		if (qphy->iface_clk)
> +			clk_disable_unprepare(qphy->iface_clk);
> +		clk_disable_unprepare(qphy->cfg_ahb_clk);
> +		qphy->clk_enabled = false;
> +	}
> +
> +	dev_dbg(&qphy->phy->dev, "%s(): clocks_enabled:%d\n", __func__,
> +						qphy->clk_enabled);

even this should be dev_vdbg.
> +}
> +
> +static int qusb2_phy_enable_power(struct qusb2_phy *qphy, bool on)
> +{
> +	int ret = 0;
> +	struct device *dev = &qphy->phy->dev;
> +
> +	dev_dbg(dev, "%s(): turn %s regulators. power_enabled:%d\n",
> +			__func__, on ? "on" : "off", qphy->pwr_enabled);
> +
> +	if (qphy->pwr_enabled == on)
> +		return 0;
> +
> +	if (!on)
> +		goto disable_vdda_phy_dpdm;
> +
> +	ret = regulator_enable(qphy->vdd_phy);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vdd-phy:%d\n", ret);
> +		goto err_vdd_phy;
> +	}
> +
> +	ret = regulator_enable(qphy->vdda_pll);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vdda-pll:%d\n", ret);
> +		goto disable_vdd_phy;
> +	}
> +
> +	ret = regulator_enable(qphy->vdda_phy_dpdm);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vdda-phy-dpdm:%d\n", ret);
> +		goto disable_vdda_pll;
> +	}
> +
> +	qphy->pwr_enabled = true;
> +	dev_dbg(dev, "%s() regulators are turned on.\n", __func__);
> +
> +	return ret;
> +
> +disable_vdda_phy_dpdm:
> +	regulator_disable(qphy->vdda_phy_dpdm);
> +disable_vdda_pll:
> +	regulator_disable(qphy->vdda_pll);
> +disable_vdd_phy:
> +	regulator_disable(qphy->vdd_phy);
> +err_vdd_phy:
> +	qphy->pwr_enabled = false;
> +	dev_dbg(dev, "%s() regulators are turned off.\n", __func__);
> +	return ret;
> +}
> +
> +/*
> + * Fetches HS Tx tuning value from e-fuse and sets QUSB2PHY_PORT_TUNE2
> + * register.
> + * For any error case, skip setting the value and use the default value.
> + */
> +static int qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
> +{
> +	struct device *dev = &qphy->phy->dev;
> +	struct nvmem_cell *cell;
> +	unsigned int bit_mask;
> +	u32 tune2_val;
> +	int bit_offset = 0;
> +	int bit_len = 0;
> +	ssize_t len;
> +	u32 *val;
> +	int ret;
> +
> +	/*
> +	 * Read EFUSE register having TUNE2 parameter's high nibble.
> +	 * If efuse register shows value as 0x0, or if we fail to find
> +	 * a valid efuse register settings, then use default value
> +	 * as 0xB for high nibble that we have already set while
> +	 * configuring phy.
> +	 */
> +	cell = devm_nvmem_cell_get(dev, "tune2_hstx_trim_efuse");
> +	if (IS_ERR(cell)) {
> +		if (PTR_ERR(cell) == -EPROBE_DEFER)
> +			return PTR_ERR(cell);
> +		goto skip;
> +	}
> +
> +	val = (u32 *)nvmem_cell_read(cell, &len);
> +	if (!IS_ERR(val)) {
> +		ret = of_property_read_u32(dev->of_node,
> +						"qcom,hstx-trim-bit-offset",
> +						&bit_offset);
> +		if (ret) {
> +			dev_dbg(dev, "hstx-trim bit offset is invalid.\n");
> +			goto skip;
> +		}
> +
> +		ret = of_property_read_u32(dev->of_node,
> +						"qcom,hstx-trim-bit-len",
> +						&bit_len);
> +		if (ret) {
> +			dev_dbg(dev, "hstx-trim bit length is invalid.\n");
> +			goto skip;
> +		}
> +
> +		bit_mask = (1 << bit_len) - 1;
> +		dev_dbg(dev,
> +			"%s(): efuse value:%x, bit_mask:%x, bit_offset: %d\n",
> +			__func__, val[0], bit_mask, bit_offset);
> +		tune2_val = TUNE2_HIGH_NIBBLE_VAL(val[0], bit_offset, bit_mask);
> +		if (!tune2_val) {
> +			dev_dbg(dev, "hstx-trim bit length is invalid.\n");
> +			goto skip;
> +		}
> +
> +		/* Fused TUNE2 value is the higher nibble only */
> +		tune2_val = tune2_val << 0x4;
> +		qusb2_set_bits(qphy->base + QUSB2PHY_PORT_TUNE2, tune2_val);
> +	} else {
> +		dev_dbg(dev, "failed reading hs-tx trim value: %d\n", ret);
> +	}
> +
> +skip:
> +	return 0;
> +}
> +
> +static int qusb2_phy_init(struct phy *phy)
> +{
> +	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> +	unsigned int reset_val;
> +	unsigned int clk_scheme;
> +	bool is_se_clk = false;
> +	int ret;
> +
> +	dev_info(&phy->dev, "Initializing QUSB2 phy\n");

dev_vdbg here..
> +
> +	ret = qusb2_phy_enable_power(qphy, true);
> +	if (ret)
> +		return ret;

shouldn't this be done in power_on?
> +
> +	qusb2_phy_enable_clocks(qphy, true);
> +
> +	/* Perform phy reset */
> +	ret = reset_control_assert(qphy->phy_reset);
> +	if (ret) {
> +		dev_err(&phy->dev, "Failed to assert phy_reset\n");
> +		return ret;
> +	}
> +	usleep_range(100, 150);

add a comment for the above delay.
> +	ret = reset_control_deassert(qphy->phy_reset);
> +	if (ret) {
> +		dev_err(&phy->dev, "Failed to de-assert phy_reset\n");
> +		return ret;
> +	}
> +
> +	/* Disable the PHY */
> +	qusb2_set_bits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
> +			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
> +
> +	/* save reset value to override based on clk scheme */
> +	reset_val = readl_relaxed(qphy->base + QUSB2PHY_PLL_TEST);
> +
> +	qcom_qusb2_phy_configure(qphy->base, qphy->cfg->phy_init_tbl,
> +				qphy->cfg->phy_init_tbl_sz);
> +
> +	/* Check for efuse value for tuning the PHY */
> +	ret = qusb2_phy_set_tune2_param(qphy);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable the PHY */
> +	qusb2_clr_bits(qphy->base + QUSB2PHY_PORT_POWERDOWN, POWER_DOWN);
> +
> +	/* Require to get phy pll lock successfully */
> +	usleep_range(150, 160);
> +
> +	/*
> +	 * Use differential clk by default; later we read TCSR_PHY_CLK_SCHEME
> +	 * register to check if Single-ended clock scheme is selected. If yes,
> +	 * then disable differential ref_clk and use single-ended clock.
> +	 */
> +
> +	if (qphy->tcsr) {
> +		ret = regmap_read(qphy->tcsr, qphy->clk_scheme_offset,
> +							&clk_scheme);
> +		/* is SE-clk available ? */
> +		if (clk_scheme & PHY_CLK_SCHEME_SEL) {
> +			dev_dbg(&phy->dev, "%s: select single-ended clk src\n",
> +								__func__);
> +			is_se_clk = true;
> +		}
> +	}
> +
> +	if (is_se_clk) {
> +		reset_val |= CLK_REF_SEL;
> +		clk_disable_unprepare(qphy->ref_clk);
> +	} else {
> +		reset_val &= ~CLK_REF_SEL;
> +		clk_prepare_enable(qphy->ref_clk);
> +	}
> +
> +	writel_relaxed(reset_val, qphy->base + QUSB2PHY_PLL_TEST);
> +
> +	/* Make sure that above write is completed to get PLL source clock */
> +	wmb();
> +
> +	/* Required to get PHY PLL lock successfully */
> +	usleep_range(100, 110);
> +
> +	if (!(readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS) &
> +					PLL_LOCKED)) {
> +		dev_err(&phy->dev, "QUSB PHY PLL LOCK fails:%x\n",
> +			readb_relaxed(qphy->base + QUSB2PHY_PLL_STATUS));
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qusb2_phy_exit(struct phy *phy)
> +{
> +	struct qusb2_phy *qphy = phy_get_drvdata(phy);
> +
> +	/* Disable the PHY */
> +	qusb2_set_bits(qphy->base + QUSB2PHY_PORT_POWERDOWN,
> +			CLAMP_N_EN | FREEZIO_N | POWER_DOWN);
> +
> +	qusb2_phy_enable_clocks(qphy, false);
> +	qusb2_phy_enable_power(qphy, false);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops qusb2_phy_gen_ops = {
> +	.init		= qusb2_phy_init,
> +	.exit		= qusb2_phy_exit,

you can add callbacks for power_on and power_off and add then enable clocks and
power here?

> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct of_device_id qusb2_phy_of_match_table[] = {
> +	{
> +		.compatible	= "qcom,msm8996-qusb2-phy",
> +		.data		= &msm8996_phy_init_cfg,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qusb2_phy_of_match_table);
> +
> +static int qusb2_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qusb2_phy *qphy;
> +	struct phy_provider *phy_provider;
> +	struct phy *generic_phy;
> +	const struct of_device_id *match;
> +	struct resource *res;
> +	int ret = 0;
> +
> +	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> +	if (!qphy)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;

the above check is not required. ioremap will take care.
> +	qphy->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(qphy->base))
> +		return PTR_ERR(qphy->base);
> +
> +	qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb_clk");
> +	if (IS_ERR(qphy->cfg_ahb_clk)) {
> +		ret = PTR_ERR(qphy->cfg_ahb_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get cfg_ahb_clk\n");
> +		return ret;
> +	}
> +
> +	qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
> +	if (IS_ERR(qphy->ref_clk_src)) {
> +		qphy->ref_clk_src = NULL;
> +		ret = PTR_ERR(qphy->ref_clk_src);
> +		if (ret != -EPROBE_DEFER)
> +			dev_dbg(dev, "clk get failed for ref_clk_src\n");

don't we have to return on PROBE_DEFER?
> +	}
> +
> +	qphy->ref_clk = devm_clk_get(dev, "ref_clk");
> +	if (IS_ERR(qphy->ref_clk)) {
> +		qphy->ref_clk = NULL;
> +		ret = PTR_ERR(qphy->ref_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_dbg(dev, "clk get failed for ref_clk\n");

same here.
> +	} else {
> +		clk_set_rate(qphy->ref_clk, 19200000);
> +	}
> +
> +	qphy->iface_clk = devm_clk_get(dev, "iface_clk");
> +	if (IS_ERR(qphy->iface_clk)) {
> +		qphy->iface_clk = NULL;
> +		ret = PTR_ERR(qphy->iface_clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_dbg(dev, "clk get failed for iface_clk\n");

here..
> +	}
> +
> +	qphy->phy_reset = devm_reset_control_get(&pdev->dev, "phy");
> +	if (IS_ERR(qphy->phy_reset)) {
> +		dev_err(dev, "failed to get phy core reset\n");
> +		return PTR_ERR(qphy->phy_reset);
> +	}
> +
> +	qphy->vdd_phy = devm_regulator_get(dev, "vdd-phy");
> +	if (IS_ERR(qphy->vdd_phy)) {
> +		dev_err(dev, "unable to get vdd-phy supply\n");
> +		return PTR_ERR(qphy->vdd_phy);
> +	}
> +
> +	qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
> +	if (IS_ERR(qphy->vdda_pll)) {
> +		dev_err(dev, "unable to get vdda-pll supply\n");
> +		return PTR_ERR(qphy->vdda_pll);
> +	}
> +
> +	qphy->vdda_phy_dpdm = devm_regulator_get(dev, "vdda-phy-dpdm");
> +	if (IS_ERR(qphy->vdda_phy_dpdm)) {
> +		dev_err(dev, "unable to get vdda-phy-dpdm supply\n");
> +		return PTR_ERR(qphy->vdda_phy_dpdm);
> +	}
> +
> +	/* Get the specific init parameters of QMP phy */
> +	match = of_match_node(qusb2_phy_of_match_table, dev->of_node);
> +	qphy->cfg = match->data;
> +
> +	qphy->tcsr = syscon_regmap_lookup_by_phandle(dev->of_node,
> +							"qcom,tcsr-syscon");
> +	if (!IS_ERR(qphy->tcsr)) {
> +		ret = of_property_read_u32(dev->of_node, "qcom,phy-clk-scheme",
> +						&qphy->clk_scheme_offset);
> +		if (ret) {
> +			dev_err(dev, "phy-clk-scheme reg offset is invalid\n");
> +			qphy->tcsr = NULL;
> +			return ret;
> +		}
> +	} else {
> +		dev_dbg(dev, "Failed to lookup TCSR regmap\n");
> +		qphy->tcsr = NULL;
> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		ret = PTR_ERR(phy_provider);
> +		dev_err(dev, "%s: failed to register phy %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
> +	if (IS_ERR(generic_phy)) {
> +		ret = PTR_ERR(generic_phy);
> +		dev_err(dev, "%s: failed to create phy %d\n", __func__, ret);
> +		generic_phy = NULL;
> +		return ret;
> +	}

phy should be created before registering the PHY.
> +	qphy->phy = generic_phy;
> +
> +	dev_set_drvdata(dev, qphy);
> +	phy_set_drvdata(generic_phy, qphy);

this should be done before phy_provider_register.

Thanks
Kishon

Powered by blists - more mailing lists