[PATCH v6 0/3] DWC3 USB support for Qualcomm platform

[PATCH v6 0/3] DWC3 USB support for Qualcomm platform

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Hi,

This is sixth version of MSM USB3 drivers patches.

Changes since v5:
* devicetree bindings descriptions fixes 
* Fixed NULL pointer dereferences in dev_prink's
* Removed extra space in "sleep " clock name

Changes since v4:
* Substitute references to "wc3" with just "dw" in USB PHY drivers and
  file names. This is to indicate that the PHY's are DesignWare, but
  not necessarily related to DWC3 IP core. 

Changes since v3:
* Remove "_clk" suffix from clock names
* Clarify required child node for qcom,dwc3
* Fix comments in functions headers
* Use dbg instead err in drivers probe functions.

Changes since v2:
* Several improvements in devicetree bindings description
* Disable regulators in glue layer if there is error during 
  ioremap.

Changes since first version:
* Split devicetree bindings description file to separate patch
* Address comments for device bindings description
* Fix typo in 'gdsc' requlator name.

These patches add basic support for USB3.0 controllers found
on MSM platforms. USB3.0 core is based on Synopsys DesignWare 
SuperSpeed IP. 

Generated on top of Felipe 'testing' branch.

Ivan T. Ivanov (3):
  usb: dwc3: msm: Add device tree binding information
  usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's
  usb: dwc3: Add Qualcomm DWC3 glue layer driver

 .../devicetree/bindings/usb/msm-ssusb.txt          |  105 ++++++
 drivers/usb/dwc3/Kconfig                           |    8 +
 drivers/usb/dwc3/Makefile                          |    1 +
 drivers/usb/dwc3/dwc3-msm.c                        |  168 +++++++++
 drivers/usb/phy/Kconfig                            |   11 +
 drivers/usb/phy/Makefile                           |    2 +
 drivers/usb/phy/phy-msm-dw-hs.c                    |  329 +++++++++++++++++
 drivers/usb/phy/phy-msm-dw-ss.c                    |  375 ++++++++++++++++++++
 8 files changed, 999 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt
 create mode 100644 drivers/usb/dwc3/dwc3-msm.c
 create mode 100644 drivers/usb/phy/phy-msm-dw-hs.c
 create mode 100644 drivers/usb/phy/phy-msm-dw-ss.c

-- 
1.7.9.5

[PATCH v6 1/3] usb: dwc3: msm: Add device tree binding information

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

MSM USB3.0 core wrapper consist of USB3.0 IP from Synopsys
(SNPS) and HS, SS PHY's control and configuration registers.

It could operate in device mode (SS, HS, FS) and host
mode (SS, HS, FS, LS).

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
---
 .../devicetree/bindings/usb/msm-ssusb.txt          |  105 ++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt

diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
new file mode 100644
index 0000000..a71bf00
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
@@ -0,0 +1,105 @@
+MSM SuperSpeed DWC3 USB SoC controller
+
+
+MSM DW Highspeed USB PHY
+========================
+Required properities:
+- compatible:	should contain "qcom,dw-hsphy";
+- reg:			offset and length of the register set in the memory map
+- clocks:		A list of phandle + clock-specifier pairs for the
+				clocks listed in clock-names
+- clock-names:	Should contain the following:
+  "xo"			External reference clock
+  "sleep_a"		Sleep clock, used when USB3 core goes into low
+				power mode (U3).
+- v1p8-supply:	phandle to the regulator for the 1.8v supply to HSPHY.
+- v3p3-supply:	phandle to the regulator for the 3.3v supply to HSPHY.
+- vbus-supply:	phandle to the regulator for the vbus supply for host
+		mode.
+- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY
+                digital circuit operation.
+
+MSM DW Superspeed USB PHY
+=========================
+Required properities:
+- compatible:	should contain "qcom,dw-ssphy";
+- reg:			offset and length of the register set in the memory map
+- clocks:		A list of phandle + clock-specifier pairs for the
+				clocks listed in clock-names
+- clock-names:	Should contain the following:
+  "xo"			External reference clock
+  "ref"			Reference clock - used in host mode.
+- v1p8-supply:	phandle to the regulator for the 1.8v supply to HSPHY.
+- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY
+                digital circuit operation.
+
+MSM DWC3 controller wrapper
+===========================
+Required properties:
+- compatible:	should contain "qcom,dwc3"
+- reg:			offset and length of the TCSR register for routing USB
+				signals to either picoPHY0 or picoPHY1.
+- clocks:		A list of phandle + clock-specifier pairs for the
+				clocks listed in clock-names
+- clock-names:	Should contain the following:
+  "core"		Master/Core clock, have to be >= 125 MHz for SS
+				operation and >= 60MHz for HS operation
+  "iface"		System bus AXI clock
+  "sleep"		Sleep clock, used when USB3 core goes into low
+				power mode (U3).
+  "utmi"		Generated by HSPHY. Used to clock the low power
+				parts of thr HS Link layer.
+Optional regulator:
+- gdsc-supply:	phandle to the regulator from globally distributed
+				switch controller
+Required child node:
+A child node must exist to represent the core DWC3 IP block. The name of
+the node is not important. The content of the node is defined in dwc3.txt.
+
+Example device nodes:
+
+	dw_hsphy: phy@f92f8800 {
+		compatible = "qcom,dw-hsphy";
+		reg = <0xf92f8800 0x30>;
+
+		clocks = <&cxo>, <&usb2a_phy_sleep_cxc>;
+		clock-names = "xo", "sleep_a";
+
+		vbus-supply = <&supply>;
+		vddcx-supply = <&supply>;
+		v1p8-supply = <&supply>;
+		v3p3-supply = <&supply>;
+	};
+
+	dw_ssphy: phy@f92f8830 {
+		compatible = "qcom,dw-ssphy";
+		reg = <0xf92f8830 0x30>;
+
+		clocks = <&cxo>, <&usb30_mock_utmi_cxc>;
+		clock-names = "xo", "ref";
+
+		vddcx-supply = <&supply>;
+		v1p8-supply = <&supply>;
+	};
+
+	usb@fd4ab000 {
+		compatible = "qcom,dwc3";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0xfd4ab000 0x4>;
+
+		clocks = <&usb30_master_cxc>, <&sys_noc_usb3_axi_cxc>,
+				<&usb30_sleep_cxc>, <&usb30_mock_utmi_cxc>;
+		clock-names = "core", "iface", "sleep", "utmi";
+
+		gdsc-supply = <&supply>;
+
+		ranges;
+		dwc3@f9200000 {
+			compatible = "snps,dwc3";
+			reg = <0xf9200000 0xcd00>;
+			interrupts = <0 131>;
+			usb-phy = <&dw_hsphy>, <&dw_ssphy>;
+			tx-fifo-resize;
+		};
+	};
-- 
1.7.9.5

[PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

These drivers handles control and configuration of the HS
and SS USB PHY transceivers. They are part of the driver
which manage Synopsys DesignWare USB3 controller stack
inside Qualcomm SoC's.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/usb/phy/Kconfig         |   11 ++
 drivers/usb/phy/Makefile        |    2 +
 drivers/usb/phy/phy-msm-dw-hs.c |  329 ++++++++++++++++++++++++++++++++++
 drivers/usb/phy/phy-msm-dw-ss.c |  375 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 717 insertions(+)
 create mode 100644 drivers/usb/phy/phy-msm-dw-hs.c
 create mode 100644 drivers/usb/phy/phy-msm-dw-ss.c

diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index d5589f9..bbb2d0e 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -214,6 +214,17 @@ config USB_RCAR_PHY
 	  To compile this driver as a module, choose M here: the
 	  module will be called phy-rcar-usb.
 
+config USB_MSM_DW_PHYS
+	tristate "Qualcomm USB controller DW PHY's wrappers support"
+	depends on (USB || USB_GADGET) && ARCH_MSM
+	select USB_PHY
+	help
+	  Enable this to support the DW USB PHY transceivers on MSM chips
+	  with DWC3 USB core. It handles PHY initialization, clock
+	  management required after resetting the hardware and power
+	  management. This driver is required even for peripheral only or
+	  host only mode configurations.
+
 config USB_ULPI
 	bool "Generic ULPI Transceiver Driver"
 	depends on ARM
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index 2135e85..4813eb5 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -26,6 +26,8 @@ obj-$(CONFIG_USB_EHCI_TEGRA)		+= phy-tegra-usb.o
 obj-$(CONFIG_USB_GPIO_VBUS)		+= phy-gpio-vbus-usb.o
 obj-$(CONFIG_USB_ISP1301)		+= phy-isp1301.o
 obj-$(CONFIG_USB_MSM_OTG)		+= phy-msm-usb.o
+obj-$(CONFIG_USB_MSM_DW_PHYS)		+= phy-msm-dw-hs.o
+obj-$(CONFIG_USB_MSM_DW_PHYS)		+= phy-msm-dw-ss.o
 obj-$(CONFIG_USB_MV_OTG)		+= phy-mv-usb.o
 obj-$(CONFIG_USB_MXS_PHY)		+= phy-mxs-usb.o
 obj-$(CONFIG_USB_RCAR_PHY)		+= phy-rcar-usb.o
diff --git a/drivers/usb/phy/phy-msm-dw-hs.c b/drivers/usb/phy/phy-msm-dw-hs.c
new file mode 100644
index 0000000..d29c1f1
--- /dev/null
+++ b/drivers/usb/phy/phy-msm-dw-hs.c
@@ -0,0 +1,329 @@
+/* Copyright (c) 2013, Code Aurora Forum. 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/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/phy.h>
+
+/**
+ *  USB QSCRATCH Hardware registers
+ */
+#define QSCRATCH_CTRL_REG		(0x04)
+#define QSCRATCH_GENERAL_CFG		(0x08)
+#define PHY_CTRL_REG			(0x10)
+#define PARAMETER_OVERRIDE_X_REG	(0x14)
+#define CHARGING_DET_CTRL_REG		(0x18)
+#define CHARGING_DET_OUTPUT_REG		(0x1c)
+#define ALT_INTERRUPT_EN_REG		(0x20)
+#define PHY_IRQ_STAT_REG		(0x24)
+#define CGCTL_REG			(0x28)
+
+#define PHY_3P3_VOL_MIN			3050000 /* uV */
+#define PHY_3P3_VOL_MAX			3300000 /* uV */
+#define PHY_3P3_HPM_LOAD		16000	/* uA */
+
+#define PHY_1P8_VOL_MIN			1800000 /* uV */
+#define PHY_1P8_VOL_MAX			1800000 /* uV */
+#define PHY_1P8_HPM_LOAD		19000	/* uA */
+
+/* TODO: these are suspicious */
+#define USB_VDDCX_NO			1	/* index */
+#define USB_VDDCX_MIN			5	/* index */
+#define USB_VDDCX_MAX			7	/* index */
+
+struct msm_dw_hs_phy {
+	struct usb_phy		phy;
+	void __iomem		*base;
+	struct device		*dev;
+
+	struct clk		*xo_clk;
+	struct clk		*sleep_a_clk;
+
+	struct regulator	*v3p3;
+	struct regulator	*v1p8;
+	struct regulator	*vddcx;
+	struct regulator	*vbus;
+};
+
+#define	phy_to_dw_phy(x)	container_of((x), struct msm_dw_hs_phy, phy)
+
+
+/**
+ * Write register.
+ *
+ * @base - MSM DWC3 PHY base virtual address.
+ * @offset - register offset.
+ * @val - value to write.
+ */
+static inline void msm_dw_hs_write(void __iomem *base, u32 offset, u32 val)
+{
+	iowrite32(val, base + offset);
+}
+
+/**
+ * Write register and read back masked value to confirm it is written
+ *
+ * @base - MSM DWC3 PHY base virtual address.
+ * @offset - register offset.
+ * @mask - register bitmask specifying what should be updated
+ * @val - value to write.
+ */
+static inline void msm_dw_hs_write_readback(void __iomem *base, u32 offset,
+					    const u32 mask, u32 val)
+{
+	u32 write_val, tmp = ioread32(base + offset);
+
+	tmp &= ~mask;		/* retain other bits */
+	write_val = tmp | val;
+
+	iowrite32(write_val, base + offset);
+
+	/* Read back to see if val was written */
+	tmp = ioread32(base + offset);
+	tmp &= mask;		/* clear other bits */
+
+	if (tmp != val)
+		pr_err("write: %x to QSCRATCH: %x FAILED\n", val, offset);
+}
+
+static void msm_dw_hs_phy_shutdown(struct usb_phy *x)
+{
+	struct msm_dw_hs_phy *phy = phy_to_dw_phy(x);
+	int ret;
+
+	ret = regulator_set_voltage(phy->v3p3, 0, PHY_3P3_VOL_MAX);
+	if (ret)
+		dev_err(phy->dev, "cannot set voltage for v3p3\n");
+
+	ret = regulator_set_voltage(phy->v1p8, 0, PHY_1P8_VOL_MAX);
+	if (ret)
+		dev_err(phy->dev, "cannot set voltage for v1p8\n");
+
+	ret = regulator_disable(phy->v1p8);
+	if (ret)
+		dev_err(phy->dev, "cannot disable v1p8\n");
+
+	ret = regulator_disable(phy->v3p3);
+	if (ret)
+		dev_err(phy->dev, "cannot disable v3p3\n");
+
+	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_NO, USB_VDDCX_MAX);
+	if (ret)
+		dev_err(phy->dev, "cannot set voltage for vddcx\n");
+
+	ret = regulator_disable(phy->vddcx);
+	if (ret)
+		dev_err(phy->dev, "cannot enable vddcx\n");
+
+	clk_disable_unprepare(phy->sleep_a_clk);
+}
+
+static int msm_dw_hs_phy_init(struct usb_phy *x)
+{
+	struct msm_dw_hs_phy	*phy = phy_to_dw_phy(x);
+	int ret;
+
+	clk_prepare_enable(phy->sleep_a_clk);
+
+	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX);
+	if (ret) {
+		dev_err(phy->dev, "cannot set voltage for vddcx\n");
+		return ret;
+	}
+
+	ret = regulator_enable(phy->vddcx);
+	if (ret) {
+		dev_err(phy->dev, "cannot enable vddcx\n");
+		return ret;
+	}
+
+	ret = regulator_set_voltage(phy->v3p3, PHY_3P3_VOL_MIN,
+				    PHY_3P3_VOL_MAX);
+	if (ret) {
+		dev_err(phy->dev, "cannot set voltage for v3p3\n");
+		return ret;
+	}
+
+	ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN,
+				    PHY_1P8_VOL_MAX);
+	if (ret) {
+		dev_err(phy->dev, "cannot set voltage for v1p8\n");
+		return ret;
+	}
+
+	ret = regulator_set_optimum_mode(phy->v1p8, PHY_1P8_HPM_LOAD);
+	if (ret < 0) {
+		dev_err(phy->dev, "cannot set HPM of regulator v1p8\n");
+		return ret;
+	}
+
+	ret = regulator_enable(phy->v1p8);
+	if (ret) {
+		dev_err(phy->dev, "cannot enable v1p8\n");
+		return ret;
+	}
+
+	ret = regulator_set_optimum_mode(phy->v3p3, PHY_3P3_HPM_LOAD);
+	if (ret < 0) {
+		dev_err(phy->dev, "cannot set HPM of regulator v3p3\n");
+		return ret;
+	}
+
+	ret = regulator_enable(phy->v3p3);
+	if (ret) {
+		dev_err(phy->dev, "cannot enable v3p3\n");
+		return ret;
+	}
+
+	/*
+	 * HSPHY Initialization: Enable UTMI clock and clamp enable HVINTs,
+	 * and disable RETENTION (power-on default is ENABLED)
+	 */
+	msm_dw_hs_write(phy->base, PHY_CTRL_REG, 0x5220bb2);
+	usleep_range(2000, 2200);
+
+	/*
+	 * write HSPHY init value to QSCRATCH reg to set HSPHY parameters like
+	 * VBUS valid threshold, disconnect valid threshold, DC voltage level,
+	 * preempasis and rise/fall time.
+	 */
+	msm_dw_hs_write_readback(phy->base, PARAMETER_OVERRIDE_X_REG,
+				0x03ffffff, 0x00d191a4);
+
+	/* Disable (bypass) VBUS and ID filters */
+	msm_dw_hs_write(phy->base, QSCRATCH_GENERAL_CFG, 0x78);
+
+	return 0;
+}
+
+static int msm_dw_hs_phy_set_vbus(struct usb_phy *x, int on)
+{
+	struct msm_dw_hs_phy	*phy = phy_to_dw_phy(x);
+	int ret;
+
+	if (IS_ERR(phy->vbus))
+		return on ? PTR_ERR(phy->vbus) : 0;
+
+	if (on)
+		ret = regulator_enable(phy->vbus);
+	else
+		ret = regulator_disable(phy->vbus);
+
+	if (ret)
+		dev_err(x->dev, "Cannot %s Vbus\n", on ? "set" : "off");
+	return ret;
+}
+
+static int msm_dw_hs_probe(struct platform_device *pdev)
+{
+	struct msm_dw_hs_phy	*phy;
+	struct resource		*res;
+	void __iomem		*base;
+
+	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, phy);
+
+	phy->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(phy->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	phy->vddcx = devm_regulator_get(phy->dev, "vddcx");
+	if (IS_ERR(phy->vddcx)) {
+		dev_dbg(phy->dev, "cannot get vddcx\n");
+		return  PTR_ERR(phy->vddcx);
+	}
+
+	phy->v3p3 = devm_regulator_get(phy->dev, "v3p3");
+	if (IS_ERR(phy->v3p3)) {
+		dev_dbg(phy->dev, "cannot get v3p3\n");
+		return PTR_ERR(phy->v3p3);
+	}
+
+	phy->v1p8 = devm_regulator_get(phy->dev, "v1p8");
+	if (IS_ERR(phy->v1p8)) {
+		dev_dbg(phy->dev, "cannot get v1p8\n");
+		return PTR_ERR(phy->v1p8);
+	}
+
+	phy->vbus = devm_regulator_get(phy->dev, "vbus");
+	if (IS_ERR(phy->vbus))
+		dev_dbg(phy->dev, "Failed to get vbus\n");
+
+	phy->xo_clk = devm_clk_get(phy->dev, "xo");
+	if (IS_ERR(phy->xo_clk)) {
+		dev_dbg(phy->dev, "cannot get TCXO buffer handle\n");
+		return PTR_ERR(phy->xo_clk);
+	}
+
+	phy->sleep_a_clk = devm_clk_get(phy->dev, "sleep_a");
+	if (IS_ERR(phy->sleep_a_clk)) {
+		dev_dbg(phy->dev, "failed to get sleep_a clock\n");
+		return PTR_ERR(phy->sleep_a_clk);
+	}
+
+	clk_prepare_enable(phy->xo_clk);
+
+	phy->base		= base;
+	phy->phy.dev		= phy->dev;
+	phy->phy.label		= "msm-dw-hsphy";
+	phy->phy.init		= msm_dw_hs_phy_init;
+	phy->phy.shutdown	= msm_dw_hs_phy_shutdown;
+	phy->phy.set_vbus	= msm_dw_hs_phy_set_vbus;
+	phy->phy.type		= USB_PHY_TYPE_USB2;
+	phy->phy.state          = OTG_STATE_UNDEFINED;
+
+	usb_add_phy_dev(&phy->phy);
+
+	return 0;
+}
+
+static int msm_dw_hs_remove(struct platform_device *pdev)
+{
+	struct msm_dw_hs_phy *phy = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(phy->xo_clk);
+	usb_remove_phy(&phy->phy);
+	return 0;
+}
+
+static const struct of_device_id msm_dw_hs_id_table[] = {
+	{ .compatible = "qcom,dw-hsphy" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, msm_dw_hs_id_table);
+
+static struct platform_driver msm_dw_hs_driver = {
+	.probe		= msm_dw_hs_probe,
+	.remove		= msm_dw_hs_remove,
+	.driver		= {
+		.name	= "msm-dw-hsphy",
+		.owner	= THIS_MODULE,
+		.of_match_table = msm_dw_hs_id_table,
+	},
+};
+
+module_platform_driver(msm_dw_hs_driver);
+
+MODULE_ALIAS("platform:msm-dw-hsphy");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("DesignWare USB3 MSM HSPHY driver");
diff --git a/drivers/usb/phy/phy-msm-dw-ss.c b/drivers/usb/phy/phy-msm-dw-ss.c
new file mode 100644
index 0000000..6734fa1
--- /dev/null
+++ b/drivers/usb/phy/phy-msm-dw-ss.c
@@ -0,0 +1,375 @@
+/* Copyright (c) 2013, Code Aurora Forum. 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/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/phy.h>
+
+/**
+ *  USB QSCRATCH Hardware registers
+ */
+#define PHY_CTRL_REG			(0x00)
+#define PHY_PARAM_CTRL_1		(0x04)
+#define PHY_PARAM_CTRL_2		(0x08)
+#define CR_PROTOCOL_DATA_IN_REG		(0x0c)
+#define CR_PROTOCOL_DATA_OUT_REG	(0x10)
+#define CR_PROTOCOL_CAP_ADDR_REG	(0x14)
+#define CR_PROTOCOL_CAP_DATA_REG	(0x18)
+#define CR_PROTOCOL_READ_REG		(0x1c)
+#define CR_PROTOCOL_WRITE_REG		(0x20)
+
+#define PHY_1P8_VOL_MIN			1800000 /* uV */
+#define PHY_1P8_VOL_MAX			1800000 /* uV */
+#define PHY_1P8_HPM_LOAD		23000	/* uA */
+
+/* TODO: these are suspicious */
+#define USB_VDDCX_NO			1	/* index */
+#define USB_VDDCX_MIN			5	/* index */
+#define USB_VDDCX_MAX			7	/* index */
+
+struct msm_dw_ss_phy {
+	struct usb_phy		phy;
+	void __iomem		*base;
+	struct device		*dev;
+
+	struct regulator	*v1p8;
+	struct regulator	*vddcx;
+
+	struct clk		*xo_clk;
+	struct clk		*ref_clk;
+};
+
+#define	phy_to_dw_phy(x)	container_of((x), struct msm_dw_ss_phy, phy)
+
+
+/**
+ * Write register
+ *
+ * @base - MSM DWC3 PHY base virtual address.
+ * @offset - register offset.
+ * @val - value to write.
+ */
+static inline void msm_dw_ss_write(void __iomem *base, u32 offset, u32 val)
+{
+	iowrite32(val, base + offset);
+}
+
+/**
+ * Write register and read back masked value to confirm it is written
+ *
+ * @base - MSM DWC3 PHY base virtual address.
+ * @offset - register offset.
+ * @mask - register bitmask specifying what should be updated
+ * @val - value to write.
+ */
+static inline void msm_dw_ss_write_readback(void __iomem *base, u32 offset,
+					    const u32 mask, u32 val)
+{
+	u32 write_val, tmp = ioread32(base + offset);
+
+	tmp &= ~mask;		/* retain other bits */
+	write_val = tmp | val;
+
+	iowrite32(write_val, base + offset);
+
+	/* Read back to see if val was written */
+	tmp = ioread32(base + offset);
+	tmp &= mask;		/* clear other bits */
+
+	if (tmp != val)
+		pr_err("write: %x to QSCRATCH: %x FAILED\n", val, offset);
+}
+
+/**
+ * Write SSPHY register
+ *
+ * @base - MSM DWC3 PHY base virtual address.
+ * @addr - SSPHY address to write.
+ * @val - value to write.
+ */
+static void msm_dw_ss_write_phycreg(void __iomem *base, u32 addr, u32 val)
+{
+	iowrite32(addr, base + CR_PROTOCOL_DATA_IN_REG);
+	iowrite32(0x1, base + CR_PROTOCOL_CAP_ADDR_REG);
+	while (ioread32(base + CR_PROTOCOL_CAP_ADDR_REG))
+		cpu_relax();
+
+	iowrite32(val, base + CR_PROTOCOL_DATA_IN_REG);
+	iowrite32(0x1, base + CR_PROTOCOL_CAP_DATA_REG);
+	while (ioread32(base + CR_PROTOCOL_CAP_DATA_REG))
+		cpu_relax();
+
+	iowrite32(0x1, base + CR_PROTOCOL_WRITE_REG);
+	while (ioread32(base + CR_PROTOCOL_WRITE_REG))
+		cpu_relax();
+}
+
+/**
+ * Read SSPHY register.
+ *
+ * @base - MSM DWC3 PHY base virtual address.
+ * @addr - SSPHY address to read.
+ */
+static u32 msm_dw_ss_read_phycreg(void __iomem *base, u32 addr)
+{
+	bool first_read = true;
+
+	iowrite32(addr, base + CR_PROTOCOL_DATA_IN_REG);
+	iowrite32(0x1, base + CR_PROTOCOL_CAP_ADDR_REG);
+	while (ioread32(base + CR_PROTOCOL_CAP_ADDR_REG))
+		cpu_relax();
+
+	/*
+	 * Due to hardware bug, first read of SSPHY register might be
+	 * incorrect. Hence as workaround, SW should perform SSPHY register
+	 * read twice, but use only second read and ignore first read.
+	 */
+retry:
+	iowrite32(0x1, base + CR_PROTOCOL_READ_REG);
+	while (ioread32(base + CR_PROTOCOL_READ_REG))
+		cpu_relax();
+
+	if (first_read) {
+		ioread32(base + CR_PROTOCOL_DATA_OUT_REG);
+		first_read = false;
+		goto retry;
+	}
+
+	return ioread32(base + CR_PROTOCOL_DATA_OUT_REG);
+}
+
+static void msm_dw_ss_phy_shutdown(struct usb_phy *x)
+{
+	struct msm_dw_ss_phy *phy = phy_to_dw_phy(x);
+	int ret;
+
+	/* Sequence to put SSPHY in low power state:
+	 * 1. Clear REF_PHY_EN in PHY_CTRL_REG
+	 * 2. Clear REF_USE_PAD in PHY_CTRL_REG
+	 * 3. Set TEST_POWERED_DOWN in PHY_CTRL_REG to enable PHY retention
+	 * 4. Disable SSPHY ref clk
+	 */
+	msm_dw_ss_write_readback(phy->base, PHY_CTRL_REG, (1 << 8), 0x0);
+	msm_dw_ss_write_readback(phy->base, PHY_CTRL_REG, (1 << 28), 0x0);
+	msm_dw_ss_write_readback(phy->base, PHY_CTRL_REG,
+				    (1 << 26), (1 << 26));
+
+	usleep_range(1000, 1200);
+	clk_disable_unprepare(phy->ref_clk);
+
+	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_NO, USB_VDDCX_MAX);
+	if (ret)
+		dev_err(phy->dev, "cannot set voltage for vddcx\n");
+
+	regulator_disable(phy->vddcx);
+
+	ret = regulator_set_voltage(phy->v1p8, 0, PHY_1P8_VOL_MAX);
+	if (ret)
+		dev_err(phy->dev, "cannot set v1p8\n");
+
+	regulator_disable(phy->v1p8);
+}
+
+static int msm_dw_ss_phy_init(struct usb_phy *x)
+{
+	struct msm_dw_ss_phy *phy = phy_to_dw_phy(x);
+	u32 data = 0;
+	int ret;
+
+	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX);
+	if (ret) {
+		dev_err(phy->dev, "cannot set voltage for vddcx\n");
+		return ret;
+	}
+
+	ret = regulator_enable(phy->vddcx);
+	if (ret) {
+		dev_err(phy->dev, "cannot enable vddcx\n");
+		return ret;
+	}
+
+	ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN,
+				    PHY_1P8_VOL_MAX);
+	if (ret) {
+		regulator_disable(phy->vddcx);
+		dev_err(phy->dev, "cannot set v1p8\n");
+		return ret;
+	}
+
+	ret = regulator_enable(phy->v1p8);
+	if (ret) {
+		regulator_disable(phy->vddcx);
+		dev_err(phy->dev, "cannot enable v1p8\n");
+		return ret;
+	}
+
+	clk_prepare_enable(phy->ref_clk);
+	usleep_range(1000, 1200);
+
+	/* SSPHY: Use ref_clk from pads and set its parameters */
+	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002);
+	msleep(30);
+	/* Assert SSPHY reset */
+	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210082);
+	usleep_range(2000, 2200);
+	/* De-assert SSPHY reset - power and ref_clock must be ON */
+	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002);
+	usleep_range(2000, 2200);
+	/* Ref clock must be stable now, enable ref clock for HS mode */
+	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210102);
+	usleep_range(2000, 2200);
+
+	/*
+	 * WORKAROUND: There is SSPHY suspend bug due to which USB enumerates
+	 * in HS mode instead of SS mode. Workaround it by asserting
+	 * LANE0.TX_ALT_BLOCK.EN_ALT_BUS to enable TX to use alt bus mode
+	 */
+	data = msm_dw_ss_read_phycreg(phy->base, 0x102d);
+	data |= (1 << 7);
+	msm_dw_ss_write_phycreg(phy->base, 0x102D, data);
+
+	data = msm_dw_ss_read_phycreg(phy->base, 0x1010);
+	data &= ~0xff0;
+	data |= 0x20;
+	msm_dw_ss_write_phycreg(phy->base, 0x1010, data);
+
+	/*
+	 * Fix RX Equalization setting as follows
+	 * LANE0.RX_OVRD_IN_HI. RX_EQ_EN set to 0
+	 * LANE0.RX_OVRD_IN_HI.RX_EQ_EN_OVRD set to 1
+	 * LANE0.RX_OVRD_IN_HI.RX_EQ set to 3
+	 * LANE0.RX_OVRD_IN_HI.RX_EQ_OVRD set to 1
+	 */
+	data = msm_dw_ss_read_phycreg(phy->base, 0x1006);
+	data &= ~(1 << 6);
+	data |= (1 << 7);
+	data &= ~(0x7 << 8);
+	data |= (0x3 << 8);
+	data |= (0x1 << 11);
+	msm_dw_ss_write_phycreg(phy->base, 0x1006, data);
+
+	/*
+	 * Set EQ and TX launch amplitudes as follows
+	 * LANE0.TX_OVRD_DRV_LO.PREEMPH set to 22
+	 * LANE0.TX_OVRD_DRV_LO.AMPLITUDE set to 127
+	 * LANE0.TX_OVRD_DRV_LO.EN set to 1.
+	 */
+	data = msm_dw_ss_read_phycreg(phy->base, 0x1002);
+	data &= ~0x3f80;
+	data |= (0x16 << 7);
+	data &= ~0x7f;
+	data |= (0x7f | (1 << 14));
+	msm_dw_ss_write_phycreg(phy->base, 0x1002, data);
+
+	/*
+	 * Set the QSCRATCH PHY_PARAM_CTRL1 parameters as follows
+	 * TX_FULL_SWING [26:20] amplitude to 127
+	 * TX_DEEMPH_3_5DB [13:8] to 22
+	 * LOS_BIAS [2:0] to 0x5
+	 */
+	msm_dw_ss_write_readback(phy->base, PHY_PARAM_CTRL_1,
+				   0x07f03f07, 0x07f01605);
+	return 0;
+}
+
+static int msm_dw_ss_probe(struct platform_device *pdev)
+{
+	struct msm_dw_ss_phy	*phy;
+	struct resource		*res;
+	void __iomem		*base;
+
+	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, phy);
+
+	phy->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(phy->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	phy->vddcx = devm_regulator_get(phy->dev, "vddcx");
+	if (IS_ERR(phy->vddcx)) {
+		dev_dbg(phy->dev, "cannot get vddcx\n");
+		return  PTR_ERR(phy->vddcx);
+	}
+
+	phy->v1p8 = devm_regulator_get(phy->dev, "v1p8");
+	if (IS_ERR(phy->v1p8)) {
+		dev_dbg(phy->dev, "cannot get v1p8\n");
+		return  PTR_ERR(phy->v1p8);
+	}
+
+	phy->xo_clk = devm_clk_get(phy->dev, "xo");
+	if (IS_ERR(phy->xo_clk)) {
+		dev_dbg(phy->dev, "cannot get TCXO buffer handle\n");
+		return PTR_ERR(phy->xo_clk);
+	}
+
+	phy->ref_clk = devm_clk_get(phy->dev, "ref");
+	if (IS_ERR(phy->ref_clk)) {
+		dev_dbg(phy->dev, "cannot get ref clock handle\n");
+		return PTR_ERR(phy->ref_clk);
+	}
+
+	clk_prepare_enable(phy->xo_clk);
+
+	phy->base		= base;
+	phy->phy.dev		= phy->dev;
+	phy->phy.label		= "msm-dw-ssphy";
+	phy->phy.init		= msm_dw_ss_phy_init;
+	phy->phy.shutdown       = msm_dw_ss_phy_shutdown;
+	phy->phy.type		= USB_PHY_TYPE_USB3;
+
+	usb_add_phy_dev(&phy->phy);
+
+	return 0;
+}
+
+static int msm_dw_ss_remove(struct platform_device *pdev)
+{
+	struct msm_dw_ss_phy *phy = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(phy->xo_clk);
+	usb_remove_phy(&phy->phy);
+	return 0;
+}
+
+static const struct of_device_id msm_dw_ss_id_table[] = {
+	{ .compatible = "qcom,dw-ssphy" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, msm_dw_ss_id_table);
+
+static struct platform_driver msm_dw_ss_driver = {
+	.probe		= msm_dw_ss_probe,
+	.remove		= msm_dw_ss_remove,
+	.driver		= {
+		.name	= "msm-dw-ssphy",
+		.owner	= THIS_MODULE,
+		.of_match_table = msm_dw_ss_id_table,
+	},
+};
+
+module_platform_driver(msm_dw_ss_driver);
+
+MODULE_ALIAS("platform:msm-dw-ssphy");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("DesignWare USB3 MSM SSPHY driver");
-- 
1.7.9.5

[PATCH v6 3/3] usb: dwc3: Add Qualcomm DWC3 glue layer driver

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

DWC3 glue layer is hardware layer around Synopsys DesignWare
USB3 core. Its purpose is to supply Synopsys IP with required
clocks, voltages and interface it with the rest of the SoC.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/usb/dwc3/Kconfig    |    8 +++
 drivers/usb/dwc3/Makefile   |    1 +
 drivers/usb/dwc3/dwc3-msm.c |  168 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 drivers/usb/dwc3/dwc3-msm.c

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 70fc430..4c7b5a4 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -59,6 +59,14 @@ config USB_DWC3_EXYNOS
 	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
 	  say 'Y' or 'M' if you have one such device.
 
+config USB_DWC3_MSM
+       tristate "Qualcomm MSM/APQ Platforms"
+       default USB_DWC3
+       select USB_MSM_DWC3_PHYS
+       help
+         Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside,
+         say 'Y' or 'M' if you have one such device.
+
 config USB_DWC3_PCI
 	tristate "PCIe-based Platforms"
 	depends on PCI
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index dd17601..a90de66 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -31,4 +31,5 @@ endif
 
 obj-$(CONFIG_USB_DWC3_OMAP)		+= dwc3-omap.o
 obj-$(CONFIG_USB_DWC3_EXYNOS)		+= dwc3-exynos.o
+obj-$(CONFIG_USB_DWC3_MSM)		+= dwc3-msm.o
 obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c
new file mode 100644
index 0000000..1d73f92
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-msm.c
@@ -0,0 +1,168 @@
+/* Copyright (c) 2013, 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/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/phy.h>
+
+struct dwc3_msm {
+	struct device		*dev;
+
+	struct clk		*core_clk;
+	struct clk		*iface_clk;
+	struct clk		*sleep_clk;
+	struct clk		*utmi_clk;
+
+	struct regulator	*gdsc;
+};
+
+static int dwc3_msm_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct dwc3_msm *mdwc;
+	struct resource *res;
+	void __iomem *tcsr;
+	int ret = 0;
+
+	mdwc = devm_kzalloc(&pdev->dev, sizeof(*mdwc), GFP_KERNEL);
+	if (!mdwc)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mdwc);
+
+	mdwc->dev = &pdev->dev;
+
+	mdwc->gdsc = devm_regulator_get(mdwc->dev, "gdsc");
+
+	mdwc->core_clk = devm_clk_get(mdwc->dev, "core");
+	if (IS_ERR(mdwc->core_clk)) {
+		dev_dbg(mdwc->dev, "failed to get core clock\n");
+		return PTR_ERR(mdwc->core_clk);
+	}
+
+	mdwc->iface_clk = devm_clk_get(mdwc->dev, "iface");
+	if (IS_ERR(mdwc->iface_clk)) {
+		dev_dbg(mdwc->dev, "failed to get iface clock\n");
+		return PTR_ERR(mdwc->iface_clk);
+	}
+
+	mdwc->sleep_clk = devm_clk_get(mdwc->dev, "sleep");
+	if (IS_ERR(mdwc->sleep_clk)) {
+		dev_dbg(mdwc->dev, "failed to get sleep clock\n");
+		return  PTR_ERR(mdwc->sleep_clk);
+	}
+
+	mdwc->utmi_clk = devm_clk_get(mdwc->dev, "utmi");
+	if (IS_ERR(mdwc->utmi_clk)) {
+		dev_dbg(mdwc->dev, "failed to get utmi clock\n");
+		return  PTR_ERR(mdwc->utmi_clk);
+	}
+
+	if (!IS_ERR(mdwc->gdsc)) {
+		ret = regulator_enable(mdwc->gdsc);
+		if (ret)
+			dev_err(mdwc->dev, "cannot enable gdsc\n");
+	}
+
+	/*
+	 * DWC3 Core requires its CORE CLK (aka master / bus clk) to
+	 * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode.
+	 */
+	clk_set_rate(mdwc->core_clk, 125000000);
+	clk_prepare_enable(mdwc->core_clk);
+	clk_prepare_enable(mdwc->iface_clk);
+	clk_prepare_enable(mdwc->sleep_clk);
+	clk_prepare_enable(mdwc->utmi_clk);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	tcsr = devm_ioremap_resource(mdwc->dev, res);
+	if (!tcsr) {
+		ret = PTR_ERR(tcsr);
+		goto dis_clks;
+	}
+
+	/*
+	 * Primary USB port is shared between USB2 and USB3 controllers.
+	 * By default this port is routed to USB2. Select primary port for
+	 * USB3, this will automatically move USB2 to secondary port.
+	 */
+	writel(0x1, tcsr);
+
+	ret = of_platform_populate(node, NULL, NULL, mdwc->dev);
+	if (ret) {
+		dev_dbg(mdwc->dev, "failed to add create dwc3 core\n");
+		writel(0x0, tcsr);
+		goto dis_clks;
+	}
+
+	return 0;
+
+dis_clks:
+	clk_disable_unprepare(mdwc->utmi_clk);
+	clk_disable_unprepare(mdwc->sleep_clk);
+	clk_disable_unprepare(mdwc->iface_clk);
+	clk_disable_unprepare(mdwc->core_clk);
+	if (!IS_ERR(mdwc->gdsc)) {
+		ret = regulator_disable(mdwc->gdsc);
+		if (ret)
+			dev_dbg(mdwc->dev, "cannot disable gdsc\n");
+	}
+
+	return ret;
+}
+
+static int dwc3_msm_remove(struct platform_device *pdev)
+{
+	struct dwc3_msm	*mdwc = platform_get_drvdata(pdev);
+	int ret;
+
+	clk_disable_unprepare(mdwc->utmi_clk);
+	clk_disable_unprepare(mdwc->sleep_clk);
+	clk_disable_unprepare(mdwc->iface_clk);
+	clk_disable_unprepare(mdwc->core_clk);
+
+	if (!IS_ERR(mdwc->gdsc)) {
+		ret = regulator_disable(mdwc->gdsc);
+		if (ret)
+			dev_dbg(mdwc->dev, "cannot disable gdsc\n");
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_dwc3_match[] = {
+	{ .compatible = "qcom,dwc3" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_dwc3_match);
+
+static struct platform_driver dwc3_msm_driver = {
+	.probe		= dwc3_msm_probe,
+	.remove		= dwc3_msm_remove,
+	.driver		= {
+		.name	= "msm-dwc3",
+		.owner	= THIS_MODULE,
+		.of_match_table	= of_dwc3_match,
+	},
+};
+
+module_platform_driver(dwc3_msm_driver);
+
+MODULE_ALIAS("platform:msm-dwc3");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("DesignWare USB3 MSM Glue Layer");
-- 
1.7.9.5

Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's

[Stanimir Varbanov]

Hi Ivan,

Few comments below.

On 10/07/2013 10:44 AM, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> These drivers handles control and configuration of the HS
> and SS USB PHY transceivers. They are part of the driver
> which manage Synopsys DesignWare USB3 controller stack
> inside Qualcomm SoC's.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  drivers/usb/phy/Kconfig         |   11 ++
>  drivers/usb/phy/Makefile        |    2 +
>  drivers/usb/phy/phy-msm-dw-hs.c |  329 ++++++++++++++++++++++++++++++++++
>  drivers/usb/phy/phy-msm-dw-ss.c |  375 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 717 insertions(+)
>  create mode 100644 drivers/usb/phy/phy-msm-dw-hs.c
>  create mode 100644 drivers/usb/phy/phy-msm-dw-ss.c
> 
> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> index d5589f9..bbb2d0e 100644
> --- a/drivers/usb/phy/Kconfig
> +++ b/drivers/usb/phy/Kconfig
> @@ -214,6 +214,17 @@ config USB_RCAR_PHY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called phy-rcar-usb.
>  
> +config USB_MSM_DW_PHYS
> +	tristate "Qualcomm USB controller DW PHY's wrappers support"
> +	depends on (USB || USB_GADGET) && ARCH_MSM
> +	select USB_PHY
> +	help
> +	  Enable this to support the DW USB PHY transceivers on MSM chips
> +	  with DWC3 USB core. It handles PHY initialization, clock
> +	  management required after resetting the hardware and power
> +	  management. This driver is required even for peripheral only or
> +	  host only mode configurations.
> +
>  config USB_ULPI
>  	bool "Generic ULPI Transceiver Driver"
>  	depends on ARM
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index 2135e85..4813eb5 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -26,6 +26,8 @@ obj-$(CONFIG_USB_EHCI_TEGRA)		+= phy-tegra-usb.o
>  obj-$(CONFIG_USB_GPIO_VBUS)		+= phy-gpio-vbus-usb.o
>  obj-$(CONFIG_USB_ISP1301)		+= phy-isp1301.o
>  obj-$(CONFIG_USB_MSM_OTG)		+= phy-msm-usb.o
> +obj-$(CONFIG_USB_MSM_DW_PHYS)		+= phy-msm-dw-hs.o
> +obj-$(CONFIG_USB_MSM_DW_PHYS)		+= phy-msm-dw-ss.o
>  obj-$(CONFIG_USB_MV_OTG)		+= phy-mv-usb.o
>  obj-$(CONFIG_USB_MXS_PHY)		+= phy-mxs-usb.o
>  obj-$(CONFIG_USB_RCAR_PHY)		+= phy-rcar-usb.o
> diff --git a/drivers/usb/phy/phy-msm-dw-hs.c b/drivers/usb/phy/phy-msm-dw-hs.c
> new file mode 100644
> index 0000000..d29c1f1
> --- /dev/null
> +++ b/drivers/usb/phy/phy-msm-dw-hs.c
> @@ -0,0 +1,329 @@
> +/* Copyright (c) 2013, Code Aurora Forum. 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/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/phy.h>
> +
> +/**
> + *  USB QSCRATCH Hardware registers
> + */
> +#define QSCRATCH_CTRL_REG		(0x04)
> +#define QSCRATCH_GENERAL_CFG		(0x08)
> +#define PHY_CTRL_REG			(0x10)
> +#define PARAMETER_OVERRIDE_X_REG	(0x14)
> +#define CHARGING_DET_CTRL_REG		(0x18)
> +#define CHARGING_DET_OUTPUT_REG		(0x1c)
> +#define ALT_INTERRUPT_EN_REG		(0x20)
> +#define PHY_IRQ_STAT_REG		(0x24)
> +#define CGCTL_REG			(0x28)
> +

Please remove braces above and below.

> +#define PHY_3P3_VOL_MIN			3050000 /* uV */
> +#define PHY_3P3_VOL_MAX			3300000 /* uV */
> +#define PHY_3P3_HPM_LOAD		16000	/* uA */
> +
> +#define PHY_1P8_VOL_MIN			1800000 /* uV */
> +#define PHY_1P8_VOL_MAX			1800000 /* uV */
> +#define PHY_1P8_HPM_LOAD		19000	/* uA */
> +
> +/* TODO: these are suspicious */
> +#define USB_VDDCX_NO			1	/* index */
> +#define USB_VDDCX_MIN			5	/* index */
> +#define USB_VDDCX_MAX			7	/* index */
> +
> +struct msm_dw_hs_phy {
> +	struct usb_phy		phy;
> +	void __iomem		*base;
> +	struct device		*dev;
> +
> +	struct clk		*xo_clk;
> +	struct clk		*sleep_a_clk;
> +
> +	struct regulator	*v3p3;
> +	struct regulator	*v1p8;
> +	struct regulator	*vddcx;
> +	struct regulator	*vbus;
> +};
> +
> +#define	phy_to_dw_phy(x)	container_of((x), struct msm_dw_hs_phy, phy)

I think that using tab after #define is uncommon, isn't it?

> +
> +
> +/**
> + * Write register.
> + *
> + * @base - MSM DWC3 PHY base virtual address.
> + * @offset - register offset.
> + * @val - value to write.
> + */
> +static inline void msm_dw_hs_write(void __iomem *base, u32 offset, u32 val)

Why inline? here and below

> +{
> +	iowrite32(val, base + offset);
> +}
> +
> +/**
> + * Write register and read back masked value to confirm it is written
> + *
> + * @base - MSM DWC3 PHY base virtual address.
> + * @offset - register offset.
> + * @mask - register bitmask specifying what should be updated
> + * @val - value to write.
> + */
> +static inline void msm_dw_hs_write_readback(void __iomem *base, u32 offset,
> +					    const u32 mask, u32 val)
> +{
> +	u32 write_val, tmp = ioread32(base + offset);
> +
> +	tmp &= ~mask;		/* retain other bits */
> +	write_val = tmp | val;
> +
> +	iowrite32(write_val, base + offset);
> +
> +	/* Read back to see if val was written */
> +	tmp = ioread32(base + offset);
> +	tmp &= mask;		/* clear other bits */
> +
> +	if (tmp != val)
> +		pr_err("write: %x to QSCRATCH: %x FAILED\n", val, offset);

You should use dev_err() here. Or better remove the error printing and
make the function to return int.

> +}
> +
> +static void msm_dw_hs_phy_shutdown(struct usb_phy *x)
> +{
> +	struct msm_dw_hs_phy *phy = phy_to_dw_phy(x);
> +	int ret;
> +
> +	ret = regulator_set_voltage(phy->v3p3, 0, PHY_3P3_VOL_MAX);
> +	if (ret)
> +		dev_err(phy->dev, "cannot set voltage for v3p3\n");
> +
> +	ret = regulator_set_voltage(phy->v1p8, 0, PHY_1P8_VOL_MAX);
> +	if (ret)
> +		dev_err(phy->dev, "cannot set voltage for v1p8\n");
> +
> +	ret = regulator_disable(phy->v1p8);
> +	if (ret)
> +		dev_err(phy->dev, "cannot disable v1p8\n");
> +
> +	ret = regulator_disable(phy->v3p3);
> +	if (ret)
> +		dev_err(phy->dev, "cannot disable v3p3\n");
> +
> +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_NO, USB_VDDCX_MAX);
> +	if (ret)
> +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> +
> +	ret = regulator_disable(phy->vddcx);
> +	if (ret)
> +		dev_err(phy->dev, "cannot enable vddcx\n");
> +
> +	clk_disable_unprepare(phy->sleep_a_clk);
> +}
> +
> +static int msm_dw_hs_phy_init(struct usb_phy *x)
> +{
> +	struct msm_dw_hs_phy	*phy = phy_to_dw_phy(x);

Using tab after struct name is uncommon IMO.

> +	int ret;
> +
> +	clk_prepare_enable(phy->sleep_a_clk);
> +
> +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(phy->vddcx);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot enable vddcx\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_set_voltage(phy->v3p3, PHY_3P3_VOL_MIN,
> +				    PHY_3P3_VOL_MAX);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot set voltage for v3p3\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN,
> +				    PHY_1P8_VOL_MAX);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot set voltage for v1p8\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_set_optimum_mode(phy->v1p8, PHY_1P8_HPM_LOAD);
> +	if (ret < 0) {
> +		dev_err(phy->dev, "cannot set HPM of regulator v1p8\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(phy->v1p8);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot enable v1p8\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_set_optimum_mode(phy->v3p3, PHY_3P3_HPM_LOAD);
> +	if (ret < 0) {
> +		dev_err(phy->dev, "cannot set HPM of regulator v3p3\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(phy->v3p3);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot enable v3p3\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * HSPHY Initialization: Enable UTMI clock and clamp enable HVINTs,
> +	 * and disable RETENTION (power-on default is ENABLED)
> +	 */
> +	msm_dw_hs_write(phy->base, PHY_CTRL_REG, 0x5220bb2);
> +	usleep_range(2000, 2200);
> +
> +	/*
> +	 * write HSPHY init value to QSCRATCH reg to set HSPHY parameters like
> +	 * VBUS valid threshold, disconnect valid threshold, DC voltage level,
> +	 * preempasis and rise/fall time.
> +	 */
> +	msm_dw_hs_write_readback(phy->base, PARAMETER_OVERRIDE_X_REG,
> +				0x03ffffff, 0x00d191a4);
> +
> +	/* Disable (bypass) VBUS and ID filters */
> +	msm_dw_hs_write(phy->base, QSCRATCH_GENERAL_CFG, 0x78);
> +
> +	return 0;
> +}
> +
> +static int msm_dw_hs_phy_set_vbus(struct usb_phy *x, int on)
> +{
> +	struct msm_dw_hs_phy	*phy = phy_to_dw_phy(x);
> +	int ret;
> +
> +	if (IS_ERR(phy->vbus))
> +		return on ? PTR_ERR(phy->vbus) : 0;
> +
> +	if (on)
> +		ret = regulator_enable(phy->vbus);
> +	else
> +		ret = regulator_disable(phy->vbus);
> +
> +	if (ret)
> +		dev_err(x->dev, "Cannot %s Vbus\n", on ? "set" : "off");

This looks like debug code. Should the error being handled by the upper
layers? I'd remove this.

> +	return ret;
> +}
> +
> +static int msm_dw_hs_probe(struct platform_device *pdev)
> +{
> +	struct msm_dw_hs_phy	*phy;
> +	struct resource		*res;
> +	void __iomem		*base;
> +
> +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, phy);
> +
> +	phy->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(phy->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	phy->vddcx = devm_regulator_get(phy->dev, "vddcx");
> +	if (IS_ERR(phy->vddcx)) {
> +		dev_dbg(phy->dev, "cannot get vddcx\n");
> +		return  PTR_ERR(phy->vddcx);
> +	}
> +
> +	phy->v3p3 = devm_regulator_get(phy->dev, "v3p3");
> +	if (IS_ERR(phy->v3p3)) {
> +		dev_dbg(phy->dev, "cannot get v3p3\n");
> +		return PTR_ERR(phy->v3p3);
> +	}
> +
> +	phy->v1p8 = devm_regulator_get(phy->dev, "v1p8");
> +	if (IS_ERR(phy->v1p8)) {
> +		dev_dbg(phy->dev, "cannot get v1p8\n");
> +		return PTR_ERR(phy->v1p8);
> +	}
> +
> +	phy->vbus = devm_regulator_get(phy->dev, "vbus");
> +	if (IS_ERR(phy->vbus))
> +		dev_dbg(phy->dev, "Failed to get vbus\n");
> +
> +	phy->xo_clk = devm_clk_get(phy->dev, "xo");
> +	if (IS_ERR(phy->xo_clk)) {
> +		dev_dbg(phy->dev, "cannot get TCXO buffer handle\n");
> +		return PTR_ERR(phy->xo_clk);
> +	}
> +
> +	phy->sleep_a_clk = devm_clk_get(phy->dev, "sleep_a");

What means the "_a" in clock name?

> +	if (IS_ERR(phy->sleep_a_clk)) {
> +		dev_dbg(phy->dev, "failed to get sleep_a clock\n");
> +		return PTR_ERR(phy->sleep_a_clk);
> +	}
> +
> +	clk_prepare_enable(phy->xo_clk);
> +
> +	phy->base		= base;
> +	phy->phy.dev		= phy->dev;
> +	phy->phy.label		= "msm-dw-hsphy";
> +	phy->phy.init		= msm_dw_hs_phy_init;
> +	phy->phy.shutdown	= msm_dw_hs_phy_shutdown;
> +	phy->phy.set_vbus	= msm_dw_hs_phy_set_vbus;
> +	phy->phy.type		= USB_PHY_TYPE_USB2;
> +	phy->phy.state          = OTG_STATE_UNDEFINED;
> +
> +	usb_add_phy_dev(&phy->phy);

this function returns int, why you don't checked it?

> +
> +	return 0;
> +}
> +
> +static int msm_dw_hs_remove(struct platform_device *pdev)
> +{
> +	struct msm_dw_hs_phy *phy = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(phy->xo_clk);
> +	usb_remove_phy(&phy->phy);
> +	return 0;
> +}
> +
> +static const struct of_device_id msm_dw_hs_id_table[] = {
> +	{ .compatible = "qcom,dw-hsphy" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, msm_dw_hs_id_table);
> +
> +static struct platform_driver msm_dw_hs_driver = {
> +	.probe		= msm_dw_hs_probe,
> +	.remove		= msm_dw_hs_remove,
> +	.driver		= {
> +		.name	= "msm-dw-hsphy",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = msm_dw_hs_id_table,
> +	},
> +};
> +
> +module_platform_driver(msm_dw_hs_driver);
> +
> +MODULE_ALIAS("platform:msm-dw-hsphy");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("DesignWare USB3 MSM HSPHY driver");
> diff --git a/drivers/usb/phy/phy-msm-dw-ss.c b/drivers/usb/phy/phy-msm-dw-ss.c
> new file mode 100644
> index 0000000..6734fa1
> --- /dev/null
> +++ b/drivers/usb/phy/phy-msm-dw-ss.c
> @@ -0,0 +1,375 @@
> +/* Copyright (c) 2013, Code Aurora Forum. 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/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/phy.h>
> +
> +/**
> + *  USB QSCRATCH Hardware registers
> + */
> +#define PHY_CTRL_REG			(0x00)
> +#define PHY_PARAM_CTRL_1		(0x04)
> +#define PHY_PARAM_CTRL_2		(0x08)
> +#define CR_PROTOCOL_DATA_IN_REG		(0x0c)
> +#define CR_PROTOCOL_DATA_OUT_REG	(0x10)
> +#define CR_PROTOCOL_CAP_ADDR_REG	(0x14)
> +#define CR_PROTOCOL_CAP_DATA_REG	(0x18)
> +#define CR_PROTOCOL_READ_REG		(0x1c)
> +#define CR_PROTOCOL_WRITE_REG		(0x20)

Remove braces.

> +
> +#define PHY_1P8_VOL_MIN			1800000 /* uV */
> +#define PHY_1P8_VOL_MAX			1800000 /* uV */
> +#define PHY_1P8_HPM_LOAD		23000	/* uA */
> +
> +/* TODO: these are suspicious */
> +#define USB_VDDCX_NO			1	/* index */
> +#define USB_VDDCX_MIN			5	/* index */
> +#define USB_VDDCX_MAX			7	/* index */
> +
> +struct msm_dw_ss_phy {
> +	struct usb_phy		phy;
> +	void __iomem		*base;
> +	struct device		*dev;
> +
> +	struct regulator	*v1p8;
> +	struct regulator	*vddcx;
> +
> +	struct clk		*xo_clk;
> +	struct clk		*ref_clk;
> +};
> +
> +#define	phy_to_dw_phy(x)	container_of((x), struct msm_dw_ss_phy, phy)
> +
> +
> +/**
> + * Write register
> + *
> + * @base - MSM DWC3 PHY base virtual address.
> + * @offset - register offset.
> + * @val - value to write.
> + */
> +static inline void msm_dw_ss_write(void __iomem *base, u32 offset, u32 val)
> +{
> +	iowrite32(val, base + offset);
> +}
> +
> +/**
> + * Write register and read back masked value to confirm it is written
> + *
> + * @base - MSM DWC3 PHY base virtual address.
> + * @offset - register offset.
> + * @mask - register bitmask specifying what should be updated
> + * @val - value to write.
> + */
> +static inline void msm_dw_ss_write_readback(void __iomem *base, u32 offset,
> +					    const u32 mask, u32 val)
> +{
> +	u32 write_val, tmp = ioread32(base + offset);
> +
> +	tmp &= ~mask;		/* retain other bits */
> +	write_val = tmp | val;
> +
> +	iowrite32(write_val, base + offset);
> +
> +	/* Read back to see if val was written */
> +	tmp = ioread32(base + offset);
> +	tmp &= mask;		/* clear other bits */
> +
> +	if (tmp != val)
> +		pr_err("write: %x to QSCRATCH: %x FAILED\n", val, offset);
> +}
> +
> +/**
> + * Write SSPHY register
> + *
> + * @base - MSM DWC3 PHY base virtual address.
> + * @addr - SSPHY address to write.
> + * @val - value to write.
> + */
> +static void msm_dw_ss_write_phycreg(void __iomem *base, u32 addr, u32 val)
> +{
> +	iowrite32(addr, base + CR_PROTOCOL_DATA_IN_REG);
> +	iowrite32(0x1, base + CR_PROTOCOL_CAP_ADDR_REG);
> +	while (ioread32(base + CR_PROTOCOL_CAP_ADDR_REG))
> +		cpu_relax();
> +
> +	iowrite32(val, base + CR_PROTOCOL_DATA_IN_REG);
> +	iowrite32(0x1, base + CR_PROTOCOL_CAP_DATA_REG);
> +	while (ioread32(base + CR_PROTOCOL_CAP_DATA_REG))
> +		cpu_relax();
> +
> +	iowrite32(0x1, base + CR_PROTOCOL_WRITE_REG);
> +	while (ioread32(base + CR_PROTOCOL_WRITE_REG))
> +		cpu_relax();
> +}
> +
> +/**
> + * Read SSPHY register.
> + *
> + * @base - MSM DWC3 PHY base virtual address.
> + * @addr - SSPHY address to read.
> + */
> +static u32 msm_dw_ss_read_phycreg(void __iomem *base, u32 addr)
> +{
> +	bool first_read = true;
> +
> +	iowrite32(addr, base + CR_PROTOCOL_DATA_IN_REG);
> +	iowrite32(0x1, base + CR_PROTOCOL_CAP_ADDR_REG);
> +	while (ioread32(base + CR_PROTOCOL_CAP_ADDR_REG))
> +		cpu_relax();
> +
> +	/*
> +	 * Due to hardware bug, first read of SSPHY register might be
> +	 * incorrect. Hence as workaround, SW should perform SSPHY register
> +	 * read twice, but use only second read and ignore first read.
> +	 */
> +retry:
> +	iowrite32(0x1, base + CR_PROTOCOL_READ_REG);
> +	while (ioread32(base + CR_PROTOCOL_READ_REG))
> +		cpu_relax();
> +
> +	if (first_read) {
> +		ioread32(base + CR_PROTOCOL_DATA_OUT_REG);
> +		first_read = false;
> +		goto retry;
> +	}
> +
> +	return ioread32(base + CR_PROTOCOL_DATA_OUT_REG);
> +}
> +
> +static void msm_dw_ss_phy_shutdown(struct usb_phy *x)
> +{
> +	struct msm_dw_ss_phy *phy = phy_to_dw_phy(x);
> +	int ret;
> +
> +	/* Sequence to put SSPHY in low power state:
> +	 * 1. Clear REF_PHY_EN in PHY_CTRL_REG
> +	 * 2. Clear REF_USE_PAD in PHY_CTRL_REG
> +	 * 3. Set TEST_POWERED_DOWN in PHY_CTRL_REG to enable PHY retention
> +	 * 4. Disable SSPHY ref clk
> +	 */
> +	msm_dw_ss_write_readback(phy->base, PHY_CTRL_REG, (1 << 8), 0x0);
> +	msm_dw_ss_write_readback(phy->base, PHY_CTRL_REG, (1 << 28), 0x0);
> +	msm_dw_ss_write_readback(phy->base, PHY_CTRL_REG,
> +				    (1 << 26), (1 << 26));
> +
> +	usleep_range(1000, 1200);
> +	clk_disable_unprepare(phy->ref_clk);
> +
> +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_NO, USB_VDDCX_MAX);
> +	if (ret)
> +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> +
> +	regulator_disable(phy->vddcx);
> +
> +	ret = regulator_set_voltage(phy->v1p8, 0, PHY_1P8_VOL_MAX);
> +	if (ret)
> +		dev_err(phy->dev, "cannot set v1p8\n");
> +
> +	regulator_disable(phy->v1p8);
> +}
> +
> +static int msm_dw_ss_phy_init(struct usb_phy *x)
> +{
> +	struct msm_dw_ss_phy *phy = phy_to_dw_phy(x);
> +	u32 data = 0;
> +	int ret;
> +
> +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(phy->vddcx);
> +	if (ret) {
> +		dev_err(phy->dev, "cannot enable vddcx\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN,
> +				    PHY_1P8_VOL_MAX);
> +	if (ret) {
> +		regulator_disable(phy->vddcx);
> +		dev_err(phy->dev, "cannot set v1p8\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(phy->v1p8);
> +	if (ret) {
> +		regulator_disable(phy->vddcx);
> +		dev_err(phy->dev, "cannot enable v1p8\n");
> +		return ret;
> +	}
> +
> +	clk_prepare_enable(phy->ref_clk);
> +	usleep_range(1000, 1200);
> +
> +	/* SSPHY: Use ref_clk from pads and set its parameters */
> +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002);

Defines?

> +	msleep(30);
> +	/* Assert SSPHY reset */
> +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210082);
> +	usleep_range(2000, 2200);
> +	/* De-assert SSPHY reset - power and ref_clock must be ON */
> +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002);
> +	usleep_range(2000, 2200);
> +	/* Ref clock must be stable now, enable ref clock for HS mode */
> +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210102);
> +	usleep_range(2000, 2200);

You can extract the bits and add defines with names got from comments.

> +
> +	/*
> +	 * WORKAROUND: There is SSPHY suspend bug due to which USB enumerates
> +	 * in HS mode instead of SS mode. Workaround it by asserting
> +	 * LANE0.TX_ALT_BLOCK.EN_ALT_BUS to enable TX to use alt bus mode
> +	 */
> +	data = msm_dw_ss_read_phycreg(phy->base, 0x102d);
> +	data |= (1 << 7);
> +	msm_dw_ss_write_phycreg(phy->base, 0x102D, data);
> +

Defines? Also you mixing uppper and lower case in hex numberes, use
lower case.

> +	data = msm_dw_ss_read_phycreg(phy->base, 0x1010);
> +	data &= ~0xff0;
> +	data |= 0x20;
> +	msm_dw_ss_write_phycreg(phy->base, 0x1010, data);
> +
> +	/*
> +	 * Fix RX Equalization setting as follows
> +	 * LANE0.RX_OVRD_IN_HI. RX_EQ_EN set to 0
> +	 * LANE0.RX_OVRD_IN_HI.RX_EQ_EN_OVRD set to 1
> +	 * LANE0.RX_OVRD_IN_HI.RX_EQ set to 3
> +	 * LANE0.RX_OVRD_IN_HI.RX_EQ_OVRD set to 1
> +	 */
> +	data = msm_dw_ss_read_phycreg(phy->base, 0x1006);

Defines?

> +	data &= ~(1 << 6);
> +	data |= (1 << 7);
> +	data &= ~(0x7 << 8);
> +	data |= (0x3 << 8);
> +	data |= (0x1 << 11);
> +	msm_dw_ss_write_phycreg(phy->base, 0x1006, data);

You mixing two styles. The fisrt is using magic numbers combined with
comment and the second style is mixing magic bits and masks. Could you
use one defines on all places?

> +
> +	/*
> +	 * Set EQ and TX launch amplitudes as follows
> +	 * LANE0.TX_OVRD_DRV_LO.PREEMPH set to 22
> +	 * LANE0.TX_OVRD_DRV_LO.AMPLITUDE set to 127
> +	 * LANE0.TX_OVRD_DRV_LO.EN set to 1.
> +	 */
> +	data = msm_dw_ss_read_phycreg(phy->base, 0x1002);
> +	data &= ~0x3f80;
> +	data |= (0x16 << 7);
> +	data &= ~0x7f;
> +	data |= (0x7f | (1 << 14));
> +	msm_dw_ss_write_phycreg(phy->base, 0x1002, data);
> +
> +	/*
> +	 * Set the QSCRATCH PHY_PARAM_CTRL1 parameters as follows
> +	 * TX_FULL_SWING [26:20] amplitude to 127
> +	 * TX_DEEMPH_3_5DB [13:8] to 22
> +	 * LOS_BIAS [2:0] to 0x5
> +	 */
> +	msm_dw_ss_write_readback(phy->base, PHY_PARAM_CTRL_1,
> +				   0x07f03f07, 0x07f01605);
> +	return 0;
> +}
> +
> +static int msm_dw_ss_probe(struct platform_device *pdev)
> +{
> +	struct msm_dw_ss_phy	*phy;
> +	struct resource		*res;
> +	void __iomem		*base;
> +
> +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, phy);
> +
> +	phy->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(phy->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	phy->vddcx = devm_regulator_get(phy->dev, "vddcx");
> +	if (IS_ERR(phy->vddcx)) {
> +		dev_dbg(phy->dev, "cannot get vddcx\n");
> +		return  PTR_ERR(phy->vddcx);
> +	}
> +
> +	phy->v1p8 = devm_regulator_get(phy->dev, "v1p8");
> +	if (IS_ERR(phy->v1p8)) {
> +		dev_dbg(phy->dev, "cannot get v1p8\n");
> +		return  PTR_ERR(phy->v1p8);
> +	}
> +
> +	phy->xo_clk = devm_clk_get(phy->dev, "xo");
> +	if (IS_ERR(phy->xo_clk)) {
> +		dev_dbg(phy->dev, "cannot get TCXO buffer handle\n");
> +		return PTR_ERR(phy->xo_clk);
> +	}
> +
> +	phy->ref_clk = devm_clk_get(phy->dev, "ref");
> +	if (IS_ERR(phy->ref_clk)) {
> +		dev_dbg(phy->dev, "cannot get ref clock handle\n");
> +		return PTR_ERR(phy->ref_clk);
> +	}
> +
> +	clk_prepare_enable(phy->xo_clk);
> +
> +	phy->base		= base;
> +	phy->phy.dev		= phy->dev;
> +	phy->phy.label		= "msm-dw-ssphy";
> +	phy->phy.init		= msm_dw_ss_phy_init;
> +	phy->phy.shutdown       = msm_dw_ss_phy_shutdown;
> +	phy->phy.type		= USB_PHY_TYPE_USB3;
> +
> +	usb_add_phy_dev(&phy->phy);

Check the error, please.

> +
> +	return 0;
> +}
> +
> +static int msm_dw_ss_remove(struct platform_device *pdev)
> +{
> +	struct msm_dw_ss_phy *phy = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(phy->xo_clk);
> +	usb_remove_phy(&phy->phy);
> +	return 0;
> +}
> +
> +static const struct of_device_id msm_dw_ss_id_table[] = {
> +	{ .compatible = "qcom,dw-ssphy" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, msm_dw_ss_id_table);
> +
> +static struct platform_driver msm_dw_ss_driver = {
> +	.probe		= msm_dw_ss_probe,
> +	.remove		= msm_dw_ss_remove,
> +	.driver		= {
> +		.name	= "msm-dw-ssphy",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = msm_dw_ss_id_table,
> +	},
> +};
> +
> +module_platform_driver(msm_dw_ss_driver);
> +
> +MODULE_ALIAS("platform:msm-dw-ssphy");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("DesignWare USB3 MSM SSPHY driver");
> 

regards,
Stan

Re: [PATCH v6 3/3] usb: dwc3: Add Qualcomm DWC3 glue layer driver

[Stanimir Varbanov]

Hi Ivan,

Minor comments below.

On 10/07/2013 10:44 AM, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> DWC3 glue layer is hardware layer around Synopsys DesignWare
> USB3 core. Its purpose is to supply Synopsys IP with required
> clocks, voltages and interface it with the rest of the SoC.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  drivers/usb/dwc3/Kconfig    |    8 +++
>  drivers/usb/dwc3/Makefile   |    1 +
>  drivers/usb/dwc3/dwc3-msm.c |  168 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 drivers/usb/dwc3/dwc3-msm.c
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 70fc430..4c7b5a4 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -59,6 +59,14 @@ config USB_DWC3_EXYNOS
>  	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
>  	  say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_MSM
> +       tristate "Qualcomm MSM/APQ Platforms"
> +       default USB_DWC3
> +       select USB_MSM_DWC3_PHYS
> +       help
> +         Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside,
> +         say 'Y' or 'M' if you have one such device.
> +
>  config USB_DWC3_PCI
>  	tristate "PCIe-based Platforms"
>  	depends on PCI
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index dd17601..a90de66 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -31,4 +31,5 @@ endif
>  
>  obj-$(CONFIG_USB_DWC3_OMAP)		+= dwc3-omap.o
>  obj-$(CONFIG_USB_DWC3_EXYNOS)		+= dwc3-exynos.o
> +obj-$(CONFIG_USB_DWC3_MSM)		+= dwc3-msm.o
>  obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
> diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c
> new file mode 100644
> index 0000000..1d73f92
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-msm.c
> @@ -0,0 +1,168 @@
> +/* Copyright (c) 2013, 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/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/phy.h>
> +
> +struct dwc3_msm {
> +	struct device		*dev;
> +
> +	struct clk		*core_clk;
> +	struct clk		*iface_clk;
> +	struct clk		*sleep_clk;
> +	struct clk		*utmi_clk;
> +
> +	struct regulator	*gdsc;
> +};
> +
> +static int dwc3_msm_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct dwc3_msm *mdwc;
> +	struct resource *res;
> +	void __iomem *tcsr;
> +	int ret = 0;
> +
> +	mdwc = devm_kzalloc(&pdev->dev, sizeof(*mdwc), GFP_KERNEL);
> +	if (!mdwc)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, mdwc);
> +
> +	mdwc->dev = &pdev->dev;
> +
> +	mdwc->gdsc = devm_regulator_get(mdwc->dev, "gdsc");
> +
> +	mdwc->core_clk = devm_clk_get(mdwc->dev, "core");
> +	if (IS_ERR(mdwc->core_clk)) {
> +		dev_dbg(mdwc->dev, "failed to get core clock\n");
> +		return PTR_ERR(mdwc->core_clk);
> +	}
> +
> +	mdwc->iface_clk = devm_clk_get(mdwc->dev, "iface");
> +	if (IS_ERR(mdwc->iface_clk)) {
> +		dev_dbg(mdwc->dev, "failed to get iface clock\n");
> +		return PTR_ERR(mdwc->iface_clk);
> +	}
> +
> +	mdwc->sleep_clk = devm_clk_get(mdwc->dev, "sleep");
> +	if (IS_ERR(mdwc->sleep_clk)) {
> +		dev_dbg(mdwc->dev, "failed to get sleep clock\n");
> +		return  PTR_ERR(mdwc->sleep_clk);
> +	}
> +
> +	mdwc->utmi_clk = devm_clk_get(mdwc->dev, "utmi");
> +	if (IS_ERR(mdwc->utmi_clk)) {
> +		dev_dbg(mdwc->dev, "failed to get utmi clock\n");
> +		return  PTR_ERR(mdwc->utmi_clk);
> +	}

I'm not sure that those dev_dbg() are useful at all.

> +
> +	if (!IS_ERR(mdwc->gdsc)) {
> +		ret = regulator_enable(mdwc->gdsc);
> +		if (ret)
> +			dev_err(mdwc->dev, "cannot enable gdsc\n");
> +	}
> +
> +	/*
> +	 * DWC3 Core requires its CORE CLK (aka master / bus clk) to
> +	 * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode.
> +	 */
> +	clk_set_rate(mdwc->core_clk, 125000000);
> +	clk_prepare_enable(mdwc->core_clk);
> +	clk_prepare_enable(mdwc->iface_clk);
> +	clk_prepare_enable(mdwc->sleep_clk);
> +	clk_prepare_enable(mdwc->utmi_clk);

All function calls above could return errors. Aren't those clocks fatal?

regadrs,
Stan

Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's

[Ivan T. Ivanov]

Hi Stan,

On Mon, 2013-10-07 at 12:22 +0300, Stanimir Varbanov wrote: 
> Hi Ivan,
> 
> Few comments below.
> 
> On 10/07/2013 10:44 AM, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > These drivers handles control and configuration of the HS
> > and SS USB PHY transceivers. They are part of the driver
> > which manage Synopsys DesignWare USB3 controller stack
> > inside Qualcomm SoC's.
> > 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> >  drivers/usb/phy/Kconfig         |   11 ++
> >  drivers/usb/phy/Makefile        |    2 +
> >  drivers/usb/phy/phy-msm-dw-hs.c |  329 ++++++++++++++++++++++++++++++++++
> >  drivers/usb/phy/phy-msm-dw-ss.c |  375 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 717 insertions(+)
> >  create mode 100644 drivers/usb/phy/phy-msm-dw-hs.c
> >  create mode 100644 drivers/usb/phy/phy-msm-dw-ss.c
> > 
> > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
> > index d5589f9..bbb2d0e 100644
> > --- a/drivers/usb/phy/Kconfig
> > +++ b/drivers/usb/phy/Kconfig
> > @@ -214,6 +214,17 @@ config USB_RCAR_PHY
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called phy-rcar-usb.
> >  
> > +config USB_MSM_DW_PHYS
> > +	tristate "Qualcomm USB controller DW PHY's wrappers support"
> > +	depends on (USB || USB_GADGET) && ARCH_MSM
> > +	select USB_PHY
> > +	help
> > +	  Enable this to support the DW USB PHY transceivers on MSM chips
> > +	  with DWC3 USB core. It handles PHY initialization, clock
> > +	  management required after resetting the hardware and power
> > +	  management. This driver is required even for peripheral only or
> > +	  host only mode configurations.
> > +
> >  config USB_ULPI
> >  	bool "Generic ULPI Transceiver Driver"
> >  	depends on ARM
> > diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> > index 2135e85..4813eb5 100644
> > --- a/drivers/usb/phy/Makefile
> > +++ b/drivers/usb/phy/Makefile
> > @@ -26,6 +26,8 @@ obj-$(CONFIG_USB_EHCI_TEGRA)		+= phy-tegra-usb.o
> >  obj-$(CONFIG_USB_GPIO_VBUS)		+= phy-gpio-vbus-usb.o
> >  obj-$(CONFIG_USB_ISP1301)		+= phy-isp1301.o
> >  obj-$(CONFIG_USB_MSM_OTG)		+= phy-msm-usb.o
> > +obj-$(CONFIG_USB_MSM_DW_PHYS)		+= phy-msm-dw-hs.o
> > +obj-$(CONFIG_USB_MSM_DW_PHYS)		+= phy-msm-dw-ss.o
> >  obj-$(CONFIG_USB_MV_OTG)		+= phy-mv-usb.o
> >  obj-$(CONFIG_USB_MXS_PHY)		+= phy-mxs-usb.o
> >  obj-$(CONFIG_USB_RCAR_PHY)		+= phy-rcar-usb.o
> > diff --git a/drivers/usb/phy/phy-msm-dw-hs.c b/drivers/usb/phy/phy-msm-dw-hs.c
> > new file mode 100644
> > index 0000000..d29c1f1
> > --- /dev/null
> > +++ b/drivers/usb/phy/phy-msm-dw-hs.c
> > @@ -0,0 +1,329 @@
> > +/* Copyright (c) 2013, Code Aurora Forum. 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/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/usb/phy.h>
> > +
> > +/**
> > + *  USB QSCRATCH Hardware registers
> > + */
> > +#define QSCRATCH_CTRL_REG		(0x04)
> > +#define QSCRATCH_GENERAL_CFG		(0x08)
> > +#define PHY_CTRL_REG			(0x10)
> > +#define PARAMETER_OVERRIDE_X_REG	(0x14)
> > +#define CHARGING_DET_CTRL_REG		(0x18)
> > +#define CHARGING_DET_OUTPUT_REG		(0x1c)
> > +#define ALT_INTERRUPT_EN_REG		(0x20)
> > +#define PHY_IRQ_STAT_REG		(0x24)
> > +#define CGCTL_REG			(0x28)
> > +
> 
> Please remove braces above and below.

ok

> 
> > +#define PHY_3P3_VOL_MIN			3050000 /* uV */
> > +#define PHY_3P3_VOL_MAX			3300000 /* uV */
> > +#define PHY_3P3_HPM_LOAD		16000	/* uA */
> > +
> > +#define PHY_1P8_VOL_MIN			1800000 /* uV */
> > +#define PHY_1P8_VOL_MAX			1800000 /* uV */
> > +#define PHY_1P8_HPM_LOAD		19000	/* uA */
> > +
> > +/* TODO: these are suspicious */
> > +#define USB_VDDCX_NO			1	/* index */
> > +#define USB_VDDCX_MIN			5	/* index */
> > +#define USB_VDDCX_MAX			7	/* index */
> > +
> > +struct msm_dw_hs_phy {
> > +	struct usb_phy		phy;
> > +	void __iomem		*base;
> > +	struct device		*dev;
> > +
> > +	struct clk		*xo_clk;
> > +	struct clk		*sleep_a_clk;
> > +
> > +	struct regulator	*v3p3;
> > +	struct regulator	*v1p8;
> > +	struct regulator	*vddcx;
> > +	struct regulator	*vbus;
> > +};
> > +
> > +#define	phy_to_dw_phy(x)	container_of((x), struct msm_dw_hs_phy, phy)
> 
> I think that using tab after #define is uncommon, isn't it?

Not in this case. I see both patterns used.

> 
> > +
> > +
> > +/**
> > + * Write register.
> > + *
> > + * @base - MSM DWC3 PHY base virtual address.
> > + * @offset - register offset.
> > + * @val - value to write.
> > + */
> > +static inline void msm_dw_hs_write(void __iomem *base, u32 offset, u32 val)
> 
> Why inline? here and below
> 

Hm, because I will like function to be inlined??

> > +{
> > +	iowrite32(val, base + offset);
> > +}
> > +
> > +/**
> > + * Write register and read back masked value to confirm it is written
> > + *
> > + * @base - MSM DWC3 PHY base virtual address.
> > + * @offset - register offset.
> > + * @mask - register bitmask specifying what should be updated
> > + * @val - value to write.
> > + */
> > +static inline void msm_dw_hs_write_readback(void __iomem *base, u32 offset,
> > +					    const u32 mask, u32 val)
> > +{
> > +	u32 write_val, tmp = ioread32(base + offset);
> > +
> > +	tmp &= ~mask;		/* retain other bits */
> > +	write_val = tmp | val;
> > +
> > +	iowrite32(write_val, base + offset);
> > +
> > +	/* Read back to see if val was written */
> > +	tmp = ioread32(base + offset);
> > +	tmp &= mask;		/* clear other bits */
> > +
> > +	if (tmp != val)
> > +		pr_err("write: %x to QSCRATCH: %x FAILED\n", val, offset);
> 
> You should use dev_err() here. Or better remove the error printing and
> make the function to return int.

I change it to dev_err(). This could happen only if clocks are
not properly setup.

> 
> > +}
> > +
> > +static void msm_dw_hs_phy_shutdown(struct usb_phy *x)
> > +{
> > +	struct msm_dw_hs_phy *phy = phy_to_dw_phy(x);
> > +	int ret;
> > +
> > +	ret = regulator_set_voltage(phy->v3p3, 0, PHY_3P3_VOL_MAX);
> > +	if (ret)
> > +		dev_err(phy->dev, "cannot set voltage for v3p3\n");
> > +
> > +	ret = regulator_set_voltage(phy->v1p8, 0, PHY_1P8_VOL_MAX);
> > +	if (ret)
> > +		dev_err(phy->dev, "cannot set voltage for v1p8\n");
> > +
> > +	ret = regulator_disable(phy->v1p8);
> > +	if (ret)
> > +		dev_err(phy->dev, "cannot disable v1p8\n");
> > +
> > +	ret = regulator_disable(phy->v3p3);
> > +	if (ret)
> > +		dev_err(phy->dev, "cannot disable v3p3\n");
> > +
> > +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_NO, USB_VDDCX_MAX);
> > +	if (ret)
> > +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> > +
> > +	ret = regulator_disable(phy->vddcx);
> > +	if (ret)
> > +		dev_err(phy->dev, "cannot enable vddcx\n");
> > +
> > +	clk_disable_unprepare(phy->sleep_a_clk);
> > +}
> > +
> > +static int msm_dw_hs_phy_init(struct usb_phy *x)
> > +{
> > +	struct msm_dw_hs_phy	*phy = phy_to_dw_phy(x);
> 
> Using tab after struct name is uncommon IMO. 

Take a look at OMAP's USB PHY driver, on which thes driver are based.

> 
> > +	int ret;

Important point is to consistent, and in this case I am not :-)
Will remove variables indentation.

> > +
> > +	clk_prepare_enable(phy->sleep_a_clk);
> > +
> > +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(phy->vddcx);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot enable vddcx\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_set_voltage(phy->v3p3, PHY_3P3_VOL_MIN,
> > +				    PHY_3P3_VOL_MAX);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot set voltage for v3p3\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN,
> > +				    PHY_1P8_VOL_MAX);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot set voltage for v1p8\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_set_optimum_mode(phy->v1p8, PHY_1P8_HPM_LOAD);
> > +	if (ret < 0) {
> > +		dev_err(phy->dev, "cannot set HPM of regulator v1p8\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(phy->v1p8);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot enable v1p8\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_set_optimum_mode(phy->v3p3, PHY_3P3_HPM_LOAD);
> > +	if (ret < 0) {
> > +		dev_err(phy->dev, "cannot set HPM of regulator v3p3\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(phy->v3p3);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot enable v3p3\n");
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * HSPHY Initialization: Enable UTMI clock and clamp enable HVINTs,
> > +	 * and disable RETENTION (power-on default is ENABLED)
> > +	 */
> > +	msm_dw_hs_write(phy->base, PHY_CTRL_REG, 0x5220bb2);
> > +	usleep_range(2000, 2200);
> > +
> > +	/*
> > +	 * write HSPHY init value to QSCRATCH reg to set HSPHY parameters like
> > +	 * VBUS valid threshold, disconnect valid threshold, DC voltage level,
> > +	 * preempasis and rise/fall time.
> > +	 */
> > +	msm_dw_hs_write_readback(phy->base, PARAMETER_OVERRIDE_X_REG,
> > +				0x03ffffff, 0x00d191a4);
> > +
> > +	/* Disable (bypass) VBUS and ID filters */
> > +	msm_dw_hs_write(phy->base, QSCRATCH_GENERAL_CFG, 0x78);
> > +
> > +	return 0;
> > +}
> > +
> > +static int msm_dw_hs_phy_set_vbus(struct usb_phy *x, int on)
> > +{
> > +	struct msm_dw_hs_phy	*phy = phy_to_dw_phy(x);
> > +	int ret;
> > +
> > +	if (IS_ERR(phy->vbus))
> > +		return on ? PTR_ERR(phy->vbus) : 0;
> > +
> > +	if (on)
> > +		ret = regulator_enable(phy->vbus);
> > +	else
> > +		ret = regulator_disable(phy->vbus);
> > +
> > +	if (ret)
> > +		dev_err(x->dev, "Cannot %s Vbus\n", on ? "set" : "off");
> 
> This looks like debug code. Should the error being handled by the upper
> layers? I'd remove this.

I could lower this to debug, but will prefer to keep message here.

> 
> > +	return ret;
> > +}
> > +
> > +static int msm_dw_hs_probe(struct platform_device *pdev)
> > +{
> > +	struct msm_dw_hs_phy	*phy;
> > +	struct resource		*res;
> > +	void __iomem		*base;
> > +
> > +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> > +	if (!phy)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, phy);
> > +
> > +	phy->dev = &pdev->dev;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(phy->dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	phy->vddcx = devm_regulator_get(phy->dev, "vddcx");
> > +	if (IS_ERR(phy->vddcx)) {
> > +		dev_dbg(phy->dev, "cannot get vddcx\n");
> > +		return  PTR_ERR(phy->vddcx);
> > +	}
> > +
> > +	phy->v3p3 = devm_regulator_get(phy->dev, "v3p3");
> > +	if (IS_ERR(phy->v3p3)) {
> > +		dev_dbg(phy->dev, "cannot get v3p3\n");
> > +		return PTR_ERR(phy->v3p3);
> > +	}
> > +
> > +	phy->v1p8 = devm_regulator_get(phy->dev, "v1p8");
> > +	if (IS_ERR(phy->v1p8)) {
> > +		dev_dbg(phy->dev, "cannot get v1p8\n");
> > +		return PTR_ERR(phy->v1p8);
> > +	}
> > +
> > +	phy->vbus = devm_regulator_get(phy->dev, "vbus");
> > +	if (IS_ERR(phy->vbus))
> > +		dev_dbg(phy->dev, "Failed to get vbus\n");
> > +
> > +	phy->xo_clk = devm_clk_get(phy->dev, "xo");
> > +	if (IS_ERR(phy->xo_clk)) {
> > +		dev_dbg(phy->dev, "cannot get TCXO buffer handle\n");
> > +		return PTR_ERR(phy->xo_clk);
> > +	}
> > +
> > +	phy->sleep_a_clk = devm_clk_get(phy->dev, "sleep_a");
> 
> What means the "_a" in clock name?

They are 2 PHY's on 8074 chip. This drivers is supposed to 
operate on PHY 0, thus sleep_a. PHY 1 is using sleep_b. Take a look 
here http://www.spinics.net/lists/arm-kernel/msg276945.html


> 
> > +	if (IS_ERR(phy->sleep_a_clk)) {
> > +		dev_dbg(phy->dev, "failed to get sleep_a clock\n");
> > +		return PTR_ERR(phy->sleep_a_clk);
> > +	}
> > +
> > +	clk_prepare_enable(phy->xo_clk);
> > +
> > +	phy->base		= base;
> > +	phy->phy.dev		= phy->dev;
> > +	phy->phy.label		= "msm-dw-hsphy";
> > +	phy->phy.init		= msm_dw_hs_phy_init;
> > +	phy->phy.shutdown	= msm_dw_hs_phy_shutdown;
> > +	phy->phy.set_vbus	= msm_dw_hs_phy_set_vbus;
> > +	phy->phy.type		= USB_PHY_TYPE_USB2;
> > +	phy->phy.state          = OTG_STATE_UNDEFINED;
> > +
> > +	usb_add_phy_dev(&phy->phy);
> 
> this function returns int, why you don't checked it?
> 

Functions will fail only if we do not pass correct 'dev' instance, 
which could not happen here. Anyway will check for error.

> > +
> > +	return 0;
> > +}

<snip>

> > +
> > +static struct platform_driver msm_dw_hs_driver = {
> > +	.probe		= msm_dw_hs_probe,
> > +	.remove		= msm_dw_hs_remove,
> > +	.driver		= {
> > +		.name	= "msm-dw-hsphy",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = msm_dw_hs_id_table,
> > +	},
> > +};
> > +
> > +module_platform_driver(msm_dw_hs_driver);
> > +
> > +MODULE_ALIAS("platform:msm-dw-hsphy");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("DesignWare USB3 MSM HSPHY driver");
> > diff --git a/drivers/usb/phy/phy-msm-dw-ss.c b/drivers/usb/phy/phy-msm-dw-ss.c
> > new file mode 100644
> > index 0000000..6734fa1
> > --- /dev/null
> > +++ b/drivers/usb/phy/phy-msm-dw-ss.c
> > @@ -0,0 +1,375 @@
> > +/* Copyright (c) 2013, Code Aurora Forum. 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/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/usb/phy.h>
> > +
> > +/**
> > + *  USB QSCRATCH Hardware registers
> > + */
> > +#define PHY_CTRL_REG			(0x00)
> > +#define PHY_PARAM_CTRL_1		(0x04)
> > +#define PHY_PARAM_CTRL_2		(0x08)
> > +#define CR_PROTOCOL_DATA_IN_REG		(0x0c)
> > +#define CR_PROTOCOL_DATA_OUT_REG	(0x10)
> > +#define CR_PROTOCOL_CAP_ADDR_REG	(0x14)
> > +#define CR_PROTOCOL_CAP_DATA_REG	(0x18)
> > +#define CR_PROTOCOL_READ_REG		(0x1c)
> > +#define CR_PROTOCOL_WRITE_REG		(0x20)
> 
> Remove braces.

sure.

> 

<snip>

> > +
> > +static int msm_dw_ss_phy_init(struct usb_phy *x)
> > +{
> > +	struct msm_dw_ss_phy *phy = phy_to_dw_phy(x);
> > +	u32 data = 0;
> > +	int ret;
> > +
> > +	ret = regulator_set_voltage(phy->vddcx, USB_VDDCX_MIN, USB_VDDCX_MAX);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot set voltage for vddcx\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(phy->vddcx);
> > +	if (ret) {
> > +		dev_err(phy->dev, "cannot enable vddcx\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_set_voltage(phy->v1p8, PHY_1P8_VOL_MIN,
> > +				    PHY_1P8_VOL_MAX);
> > +	if (ret) {
> > +		regulator_disable(phy->vddcx);
> > +		dev_err(phy->dev, "cannot set v1p8\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(phy->v1p8);
> > +	if (ret) {
> > +		regulator_disable(phy->vddcx);
> > +		dev_err(phy->dev, "cannot enable v1p8\n");
> > +		return ret;
> > +	}
> > +
> > +	clk_prepare_enable(phy->ref_clk);
> > +	usleep_range(1000, 1200);
> > +
> > +	/* SSPHY: Use ref_clk from pads and set its parameters */
> > +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002);
> 
> Defines?
> 
> > +	msleep(30);
> > +	/* Assert SSPHY reset */
> > +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210082);
> > +	usleep_range(2000, 2200);
> > +	/* De-assert SSPHY reset - power and ref_clock must be ON */
> > +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210002);
> > +	usleep_range(2000, 2200);
> > +	/* Ref clock must be stable now, enable ref clock for HS mode */
> > +	msm_dw_ss_write(phy->base, PHY_CTRL_REG, 0x10210102);
> > +	usleep_range(2000, 2200);
> 
> You can extract the bits and add defines with names got from comments.

You know as much as I know, suggestions for define names?

> 
> > +
> > +	/*
> > +	 * WORKAROUND: There is SSPHY suspend bug due to which USB enumerates
> > +	 * in HS mode instead of SS mode. Workaround it by asserting
> > +	 * LANE0.TX_ALT_BLOCK.EN_ALT_BUS to enable TX to use alt bus mode
> > +	 */
> > +	data = msm_dw_ss_read_phycreg(phy->base, 0x102d);
> > +	data |= (1 << 7);
> > +	msm_dw_ss_write_phycreg(phy->base, 0x102D, data);
> > +
> 
> Defines? 

ok. this one is easy :-)

> Also you mixing uppper and lower case in hex numberes, use
> lower case.
> 

will fix it.

> > +	data = msm_dw_ss_read_phycreg(phy->base, 0x1010);
> > +	data &= ~0xff0;
> > +	data |= 0x20;
> > +	msm_dw_ss_write_phycreg(phy->base, 0x1010, data);
> > +
> > +	/*
> > +	 * Fix RX Equalization setting as follows
> > +	 * LANE0.RX_OVRD_IN_HI. RX_EQ_EN set to 0
> > +	 * LANE0.RX_OVRD_IN_HI.RX_EQ_EN_OVRD set to 1
> > +	 * LANE0.RX_OVRD_IN_HI.RX_EQ set to 3
> > +	 * LANE0.RX_OVRD_IN_HI.RX_EQ_OVRD set to 1
> > +	 */
> > +	data = msm_dw_ss_read_phycreg(phy->base, 0x1006);
> 
> Defines?

Ok this is also easy, but will #define make code easy to read. 
We have such nice comment here.

> 
> > +	data &= ~(1 << 6);
> > +	data |= (1 << 7);
> > +	data &= ~(0x7 << 8);
> > +	data |= (0x3 << 8);
> > +	data |= (0x1 << 11);
> > +	msm_dw_ss_write_phycreg(phy->base, 0x1006, data);
> 
> You mixing two styles. The first is using magic numbers combined with
> comment and the second style is mixing magic bits and masks. Could you
> use one defines on all places?

Will try to make it consistent.

> 
> > +
> > +	/*
> > +	 * Set EQ and TX launch amplitudes as follows
> > +	 * LANE0.TX_OVRD_DRV_LO.PREEMPH set to 22
> > +	 * LANE0.TX_OVRD_DRV_LO.AMPLITUDE set to 127
> > +	 * LANE0.TX_OVRD_DRV_LO.EN set to 1.
> > +	 */
> > +	data = msm_dw_ss_read_phycreg(phy->base, 0x1002);
> > +	data &= ~0x3f80;
> > +	data |= (0x16 << 7);
> > +	data &= ~0x7f;
> > +	data |= (0x7f | (1 << 14));
> > +	msm_dw_ss_write_phycreg(phy->base, 0x1002, data);
> > +
> > +	/*
> > +	 * Set the QSCRATCH PHY_PARAM_CTRL1 parameters as follows
> > +	 * TX_FULL_SWING [26:20] amplitude to 127
> > +	 * TX_DEEMPH_3_5DB [13:8] to 22
> > +	 * LOS_BIAS [2:0] to 0x5
> > +	 */
> > +	msm_dw_ss_write_readback(phy->base, PHY_PARAM_CTRL_1,
> > +				   0x07f03f07, 0x07f01605);
> > +	return 0;
> > +}
> > +
> > +static int msm_dw_ss_probe(struct platform_device *pdev)
> > +{
> > +	struct msm_dw_ss_phy	*phy;
> > +	struct resource		*res;
> > +	void __iomem		*base;
> > +
> > +	phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
> > +	if (!phy)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, phy);
> > +
> > +	phy->dev = &pdev->dev;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base = devm_ioremap_resource(phy->dev, res);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	phy->vddcx = devm_regulator_get(phy->dev, "vddcx");
> > +	if (IS_ERR(phy->vddcx)) {
> > +		dev_dbg(phy->dev, "cannot get vddcx\n");
> > +		return  PTR_ERR(phy->vddcx);
> > +	}
> > +
> > +	phy->v1p8 = devm_regulator_get(phy->dev, "v1p8");
> > +	if (IS_ERR(phy->v1p8)) {
> > +		dev_dbg(phy->dev, "cannot get v1p8\n");
> > +		return  PTR_ERR(phy->v1p8);
> > +	}
> > +
> > +	phy->xo_clk = devm_clk_get(phy->dev, "xo");
> > +	if (IS_ERR(phy->xo_clk)) {
> > +		dev_dbg(phy->dev, "cannot get TCXO buffer handle\n");
> > +		return PTR_ERR(phy->xo_clk);
> > +	}
> > +
> > +	phy->ref_clk = devm_clk_get(phy->dev, "ref");
> > +	if (IS_ERR(phy->ref_clk)) {
> > +		dev_dbg(phy->dev, "cannot get ref clock handle\n");
> > +		return PTR_ERR(phy->ref_clk);
> > +	}
> > +
> > +	clk_prepare_enable(phy->xo_clk);
> > +
> > +	phy->base		= base;
> > +	phy->phy.dev		= phy->dev;
> > +	phy->phy.label		= "msm-dw-ssphy";
> > +	phy->phy.init		= msm_dw_ss_phy_init;
> > +	phy->phy.shutdown       = msm_dw_ss_phy_shutdown;
> > +	phy->phy.type		= USB_PHY_TYPE_USB3;
> > +
> > +	usb_add_phy_dev(&phy->phy);
> 
> Check the error, please.

Same comment.

> 
> > +
> > +	return 0;
> > +}
> > +

<snip> 
> > 
> 
> regards,
> Stan

Thanks,
Ivan

Re: [PATCH v6 3/3] usb: dwc3: Add Qualcomm DWC3 glue layer driver

[Ivan T. Ivanov]

Hi Stan, 

On Mon, 2013-10-07 at 12:28 +0300, Stanimir Varbanov wrote: 
> Hi Ivan,
> 
> Minor comments below.
> 
> On 10/07/2013 10:44 AM, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > DWC3 glue layer is hardware layer around Synopsys DesignWare
> > USB3 core. Its purpose is to supply Synopsys IP with required
> > clocks, voltages and interface it with the rest of the SoC.
> > 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> >  drivers/usb/dwc3/Kconfig    |    8 +++
> >  drivers/usb/dwc3/Makefile   |    1 +
> >  drivers/usb/dwc3/dwc3-msm.c |  168 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 177 insertions(+)
> >  create mode 100644 drivers/usb/dwc3/dwc3-msm.c
> > 
> > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > index 70fc430..4c7b5a4 100644
> > --- a/drivers/usb/dwc3/Kconfig
> > +++ b/drivers/usb/dwc3/Kconfig
> > @@ -59,6 +59,14 @@ config USB_DWC3_EXYNOS
> >  	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
> >  	  say 'Y' or 'M' if you have one such device.
> >  
> > +config USB_DWC3_MSM
> > +       tristate "Qualcomm MSM/APQ Platforms"
> > +       default USB_DWC3
> > +       select USB_MSM_DWC3_PHYS
> > +       help
> > +         Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside,
> > +         say 'Y' or 'M' if you have one such device.
> > +
> >  config USB_DWC3_PCI
> >  	tristate "PCIe-based Platforms"
> >  	depends on PCI
> > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > index dd17601..a90de66 100644
> > --- a/drivers/usb/dwc3/Makefile
> > +++ b/drivers/usb/dwc3/Makefile
> > @@ -31,4 +31,5 @@ endif
> >  
> >  obj-$(CONFIG_USB_DWC3_OMAP)		+= dwc3-omap.o
> >  obj-$(CONFIG_USB_DWC3_EXYNOS)		+= dwc3-exynos.o
> > +obj-$(CONFIG_USB_DWC3_MSM)		+= dwc3-msm.o
> >  obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
> > diff --git a/drivers/usb/dwc3/dwc3-msm.c b/drivers/usb/dwc3/dwc3-msm.c
> > new file mode 100644
> > index 0000000..1d73f92
> > --- /dev/null
> > +++ b/drivers/usb/dwc3/dwc3-msm.c
> > @@ -0,0 +1,168 @@
> > +/* Copyright (c) 2013, 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/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/usb/phy.h>
> > +
> > +struct dwc3_msm {
> > +	struct device		*dev;
> > +
> > +	struct clk		*core_clk;
> > +	struct clk		*iface_clk;
> > +	struct clk		*sleep_clk;
> > +	struct clk		*utmi_clk;
> > +
> > +	struct regulator	*gdsc;
> > +};
> > +
> > +static int dwc3_msm_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *node = pdev->dev.of_node;
> > +	struct dwc3_msm *mdwc;
> > +	struct resource *res;
> > +	void __iomem *tcsr;
> > +	int ret = 0;
> > +
> > +	mdwc = devm_kzalloc(&pdev->dev, sizeof(*mdwc), GFP_KERNEL);
> > +	if (!mdwc)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, mdwc);
> > +
> > +	mdwc->dev = &pdev->dev;
> > +
> > +	mdwc->gdsc = devm_regulator_get(mdwc->dev, "gdsc");
> > +
> > +	mdwc->core_clk = devm_clk_get(mdwc->dev, "core");
> > +	if (IS_ERR(mdwc->core_clk)) {
> > +		dev_dbg(mdwc->dev, "failed to get core clock\n");
> > +		return PTR_ERR(mdwc->core_clk);
> > +	}
> > +
> > +	mdwc->iface_clk = devm_clk_get(mdwc->dev, "iface");
> > +	if (IS_ERR(mdwc->iface_clk)) {
> > +		dev_dbg(mdwc->dev, "failed to get iface clock\n");
> > +		return PTR_ERR(mdwc->iface_clk);
> > +	}
> > +
> > +	mdwc->sleep_clk = devm_clk_get(mdwc->dev, "sleep");
> > +	if (IS_ERR(mdwc->sleep_clk)) {
> > +		dev_dbg(mdwc->dev, "failed to get sleep clock\n");
> > +		return  PTR_ERR(mdwc->sleep_clk);
> > +	}
> > +
> > +	mdwc->utmi_clk = devm_clk_get(mdwc->dev, "utmi");
> > +	if (IS_ERR(mdwc->utmi_clk)) {
> > +		dev_dbg(mdwc->dev, "failed to get utmi clock\n");
> > +		return  PTR_ERR(mdwc->utmi_clk);
> > +	}
> 
> I'm not sure that those dev_dbg() are useful at all.

They are, if you deal with WIP clocks and regulators implementations,
which is the case for this platform.

> 
> > +
> > +	if (!IS_ERR(mdwc->gdsc)) {
> > +		ret = regulator_enable(mdwc->gdsc);
> > +		if (ret)
> > +			dev_err(mdwc->dev, "cannot enable gdsc\n");
> > +	}
> > +
> > +	/*
> > +	 * DWC3 Core requires its CORE CLK (aka master / bus clk) to
> > +	 * run at 125Mhz in SSUSB mode and >60MHZ for HSUSB mode.
> > +	 */
> > +	clk_set_rate(mdwc->core_clk, 125000000);
> > +	clk_prepare_enable(mdwc->core_clk);
> > +	clk_prepare_enable(mdwc->iface_clk);
> > +	clk_prepare_enable(mdwc->sleep_clk);
> > +	clk_prepare_enable(mdwc->utmi_clk);
> 
> All function calls above could return errors. Aren't those clocks fatal?

Yes, errors are fatal, I will add checks here.

> 
> regadrs,
> Stan
> 

Thanks, 
Ivan

Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's

[Mark Rutland]

[...]

> > > +   phy->sleep_a_clk = devm_clk_get(phy->dev, "sleep_a");
> >
> > What means the "_a" in clock name?
> 
> They are 2 PHY's on 8074 chip. This drivers is supposed to
> operate on PHY 0, thus sleep_a. PHY 1 is using sleep_b. Take a look
> here http://www.spinics.net/lists/arm-kernel/msg276945.html

When you say two PHYs, do you mean the HS PHY and the SS PHY?

Or are there two HS PHYs? If so, would the other HS PHY have a sleep_b clock?

The clock-names property should describe the clocks from the view of the device
itself. As we're describing the PHY in isolation, rather than a big block that
contains the PHY, the presesnce or absence of other PHYs should not affect the
name. If the "_a" suffix is only to differentiate the instance of PHY, it
should be dropped.

Thanks,
Mark.

Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's

[Ivan T. Ivanov]

Hi, 

On Tue, 2013-10-22 at 15:32 +0100, Mark Rutland wrote: 
> [...]
> 
> > > > +   phy->sleep_a_clk = devm_clk_get(phy->dev, "sleep_a");
> > >
> > > What means the "_a" in clock name?
> > 
> > They are 2 PHY's on 8074 chip. This drivers is supposed to
> > operate on PHY 0, thus sleep_a. PHY 1 is using sleep_b. Take a look
> > here http://www.spinics.net/lists/arm-kernel/msg276945.html
> 
> When you say two PHYs, do you mean the HS PHY and the SS PHY?
> 
> Or are there two HS PHYs? If so, would the other HS PHY have a sleep_b clock?

I think that this is the case.

> 
> The clock-names property should describe the clocks from the view of the device
> itself. As we're describing the PHY in isolation, rather than a big block that
> contains the PHY, the presesnce or absence of other PHYs should not affect the
> name. If the "_a" suffix is only to differentiate the instance of PHY, it
> should be dropped.


Ok. This was left from earlier platform code. I will change it.

Thanks,
Ivan

> 
> Thanks,
> Mark.
> --