[PATCH 0/2] Adding USB 3.0 DRD-phy support for exynos5250

This patchset is based on the work for USB 2.0 host phy support
for exynos5250
https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-November/021915.html

Based on 'usb-next' branch.
Tested on smdk5250 with following patch-series:
https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-November/021923.html
http://www.spinics.net/lists/linux-usb/msg73857.html

Vivek Gautam (2):
  USB: PHY: Add support for USB 3.0 phy for exynos5250
  ARM: Exynos5250: Enabling USB 3.0 phy for samsung-usbphy driver

 arch/arm/boot/dts/exynos5250.dtsi            |    3 +-
 arch/arm/mach-exynos/include/mach/regs-pmu.h |    4 +
 arch/arm/mach-exynos/setup-usb-phy.c         |    9 +
 drivers/usb/phy/samsung-usbphy.c             |  337 ++++++++++++++++++++++++++
 include/linux/usb/samsung_usb_phy.h          |    1 +
 5 files changed, 353 insertions(+), 1 deletions(-)

-- 
1.7.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Adding support for USB3.0 phy for dwc3 controller on
exynso5250 SOC.

Signed-off-by: Vivek Gautam <redacted>
---
 drivers/usb/phy/samsung-usbphy.c    |  337 +++++++++++++++++++++++++++++++++++
 include/linux/usb/samsung_usb_phy.h |    1 +
 2 files changed, 338 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 3b4863d..e3b5fb1 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c

@@ -167,6 +167,99 @@
 
 #define EXYNOS5_PHY_OTG_TUNE			(0x40)
 
+/* USB 3.0: DRD */
+#define EXYNOS5_DRD_LINKSYSTEM			(0x04)
+
+#define LINKSYSTEM_FLADJ_MASK			(0x3f << 1)
+#define LINKSYSTEM_FLADJ(_x)			((_x) << 1)
+#define LINKSYSTEM_XHCI_VERSION_CONTROL		(1 << 27)
+
+#define EXYNOS5_DRD_PHYUTMI			(0x08)
+
+#define PHYUTMI_OTGDISABLE			(1 << 6)
+#define PHYUTMI_FORCESUSPEND			(1 << 1)
+#define PHYUTMI_FORCESLEEP			(1 << 0)
+
+#define EXYNOS5_DRD_PHYPIPE			(0x0C)
+
+#define EXYNOS5_DRD_PHYCLKRST			(0x10)
+
+#define PHYCLKRST_SSC_REFCLKSEL_MASK		(0xff << 23)
+#define PHYCLKRST_SSC_REFCLKSEL(_x)		((_x) << 23)
+
+#define PHYCLKRST_SSC_RANGE_MASK		(0x03 << 21)
+#define PHYCLKRST_SSC_RANGE(_x)			((_x) << 21)
+
+#define PHYCLKRST_SSC_EN			(1 << 20)
+#define PHYCLKRST_REF_SSP_EN			(1 << 19)
+#define PHYCLKRST_REF_CLKDIV2			(1 << 18)
+
+#define PHYCLKRST_MPLL_MULTIPLIER_MASK		(0x7f << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER(_x)		((_x) << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF 	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x19)
+#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF 	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x02)
+#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x68)
+#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x7d)
+#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF 	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x02)
+
+#define PHYCLKRST_FSEL_MASK			(0x3f << 5)
+#define PHYCLKRST_FSEL(_x)			((_x) << 5)
+#define PHYCLKRST_FSEL_PAD_100MHZ		\
+		PHYCLKRST_FSEL(0x27)
+#define PHYCLKRST_FSEL_PAD_24MHZ		\
+		PHYCLKRST_FSEL(0x2a)
+#define PHYCLKRST_FSEL_PAD_20MHZ		\
+		PHYCLKRST_FSEL(0x31)
+#define PHYCLKRST_FSEL_PAD_19_2MHZ		\
+		PHYCLKRST_FSEL(0x38)
+
+#define PHYCLKRST_RETENABLEN			(1 << 4)
+
+#define PHYCLKRST_REFCLKSEL_MASK		(0x03 << 2)
+#define PHYCLKRST_REFCLKSEL(_x)			((_x) << 2)
+#define PHYCLKRST_REFCLKSEL_PAD_REFCLK		\
+		PHYCLKRST_REFCLKSEL(2)
+#define PHYCLKRST_REFCLKSEL_EXT_REFCLK		\
+		PHYCLKRST_REFCLKSEL(3)
+
+#define PHYCLKRST_PORTRESET			(1 << 1)
+#define PHYCLKRST_COMMONONN			(1 << 0)
+
+#define EXYNOS5_DRD_PHYREG0			(0x14)
+#define EXYNOS5_DRD_PHYREG1			(0x18)
+
+#define EXYNOS5_DRD_PHYPARAM0			(0x1C)
+
+#define PHYPARAM0_REF_USE_PAD			(0x1 << 31)
+#define PHYPARAM0_REF_LOSLEVEL_MASK		(0x1f << 26)
+#define PHYPARAM0_REF_LOSLEVEL			(0x9 << 26)
+
+#define EXYNOS5_DRD_PHYPARAM1			(0x20)
+
+#define PHYPARAM1_PCS_TXDEEMPH_MASK		(0x1f << 0)
+#define PHYPARAM1_PCS_TXDEEMPH			(0x1C)
+
+#define EXYNOS5_DRD_PHYTERM			(0x24)
+
+#define EXYNOS5_DRD_PHYTEST			(0x28)
+
+#define PHYTEST_POWERDOWN_SSP			(1 << 3)
+#define PHYTEST_POWERDOWN_HSP			(1 << 2)
+
+#define EXYNOS5_DRD_PHYADP			(0x2C)
+
+#define EXYNOS5_DRD_PHYBATCHG			(0x30)
+
+#define PHYBATCHG_UTMI_CLKSEL			(0x1 << 2)
+
+#define EXYNOS5_DRD_PHYRESUME			(0x34)
+#define EXYNOS5_DRD_LINKPORT			(0x44)
+
 #ifndef MHZ
 #define MHZ (1000*1000)
 #endif

@@ -180,10 +273,12 @@ enum samsung_cpu_type {
 /*
  * struct samsung_usbphy - transceiver driver state
  * @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
  * @plat: platform data
  * @dev: The parent device supplied to the probe function
  * @clk: usb phy clock
  * @regs: usb phy register memory base
+ * @regs_phy3: usb 3.0 phy register memory base
  * @ref_clk_freq: reference clock frequency selection
  * @cpu_type: machine identifier
  * @phy_type: It keeps track of the PHY type.

@@ -191,10 +286,12 @@ enum samsung_cpu_type {
  */
 struct samsung_usbphy {
 	struct usb_phy	phy;
+	struct usb_phy	phy3;
 	struct samsung_usbphy_data *plat;
 	struct device	*dev;
 	struct clk	*clk;
 	void __iomem	*regs;
+	void __iomem	*regs_phy3;
 	int		ref_clk_freq;
 	int		cpu_type;
 	enum samsung_usb_phy_type phy_type;

@@ -202,6 +299,7 @@ struct samsung_usbphy {
 };
 
 #define phy_to_sphy(x)		container_of((x), struct samsung_usbphy, phy)
+#define phy3_to_sphy(x)		container_of((x), struct samsung_usbphy, phy3)
 
 /*
  * PHYs are different for USB Device and USB Host. Controllers can make

@@ -287,12 +385,120 @@ static int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy)
 	return refclk_freq;
 }
 
+/*
+ * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock form clock core.
+ */
+static u32 exynos5_usbphy3_set_clock(struct samsung_usbphy *sphy)
+{
+	u32 reg;
+	u32 refclk;
+
+	refclk = sphy->ref_clk_freq;
+
+	reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK |
+		PHYCLKRST_FSEL(refclk);
+
+	switch (refclk) {
+	case HOST_CTRL0_FSEL_CLKSEL_50M:
+		reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF |
+			PHYCLKRST_SSC_REFCLKSEL(0x00));
+		break;
+	case HOST_CTRL0_FSEL_CLKSEL_20M:
+		reg |= (PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF |
+			PHYCLKRST_SSC_REFCLKSEL(0x00));
+		break;
+	case HOST_CTRL0_FSEL_CLKSEL_19200K:
+		reg |= (PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF |
+			PHYCLKRST_SSC_REFCLKSEL(0x88));
+		break;
+	case HOST_CTRL0_FSEL_CLKSEL_24M:
+	default:
+		reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF |
+			PHYCLKRST_SSC_REFCLKSEL(0x88));
+		break;
+	}
+
+	return reg;
+}
+
 static int exynos5_phyhost_is_on(void *regs)
 {
 	return (readl(regs + EXYNOS5_PHY_HOST_CTRL0) &
 				HOST_CTRL0_SIDDQ) ? 0 : 1;
 }
 
+static int samsung_exynos5_usbphy3_enable(struct samsung_usbphy *sphy)
+{
+	void __iomem *regs = sphy->regs_phy3;
+	u32 phyparam0;
+	u32 phyparam1;
+	u32 linksystem;
+	u32 phybatchg;
+	u32 phytest;
+	u32 phyclkrst;
+
+	/* Reset USB 3.0 PHY */
+	writel(0x0, regs + EXYNOS5_DRD_PHYREG0);
+
+	phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
+	/* Select PHY CLK source */
+	phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
+	/* Set Loss-of-Signal Detector sensitivity */
+	phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
+	phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
+	writel(phyparam0, regs + EXYNOS5_DRD_PHYPARAM0);
+
+	writel(0x0, regs + EXYNOS5_DRD_PHYRESUME);
+
+	/*
+	 * Setting the Frame length Adj value[6:1] to default 0x20
+	 * See xHCI 1.0 spec, 5.2.4
+	 */
+	linksystem = LINKSYSTEM_XHCI_VERSION_CONTROL |
+			LINKSYSTEM_FLADJ(0x20);
+	writel(linksystem, regs + EXYNOS5_DRD_LINKSYSTEM);
+
+	phyparam1 = readl(regs + EXYNOS5_DRD_PHYPARAM1);
+	/* Set Tx De-Emphasis level */
+	phyparam1 &= ~PHYPARAM1_PCS_TXDEEMPH_MASK;
+	phyparam1 |= PHYPARAM1_PCS_TXDEEMPH;
+	writel(phyparam1, regs + EXYNOS5_DRD_PHYPARAM1);
+
+	phybatchg = readl(regs + EXYNOS5_DRD_PHYBATCHG);
+	phybatchg |= PHYBATCHG_UTMI_CLKSEL;
+	writel(phybatchg, regs + EXYNOS5_DRD_PHYBATCHG);
+
+	/* PHYTEST POWERDOWN Control */
+	phytest = readl(regs + EXYNOS5_DRD_PHYTEST);
+	phytest &= ~(PHYTEST_POWERDOWN_SSP |
+			PHYTEST_POWERDOWN_HSP);
+	writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
+
+	/* UTMI Power Control */
+	writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
+
+	phyclkrst = exynos5_usbphy3_set_clock(sphy);
+
+	phyclkrst |= PHYCLKRST_PORTRESET |
+			/* Digital power supply in normal operating mode */
+			PHYCLKRST_RETENABLEN |
+			/* Enable ref clock for SS function */
+			PHYCLKRST_REF_SSP_EN |
+			/* Enable spread spectrum */
+			PHYCLKRST_SSC_EN |
+			/* Power down HS Bias and PLL blocks in suspend mode */
+			PHYCLKRST_COMMONONN;
+
+	writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+	udelay(10);
+
+	phyclkrst &= ~(PHYCLKRST_PORTRESET);
+	writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+	return 0;
+}
+
 static void samsung_exynos5_usbphy_enable(struct samsung_usbphy *sphy)
 {
 	void __iomem *regs = sphy->regs;

@@ -432,6 +638,32 @@ static void samsung_usbphy_enable(struct samsung_usbphy *sphy)
 	writel(rstcon, regs + SAMSUNG_RSTCON);
 }
 
+static void samsung_exynos5_usbphy3_disable(struct samsung_usbphy *sphy)
+{
+	u32 phyutmi;
+	u32 phyclkrst;
+	u32 phytest;
+	void __iomem *regs = sphy->regs_phy3;
+
+	phyutmi = PHYUTMI_OTGDISABLE |
+			PHYUTMI_FORCESUSPEND |
+			PHYUTMI_FORCESLEEP;
+	writel(phyutmi, regs + EXYNOS5_DRD_PHYUTMI);
+
+	/* Resetting the PHYCLKRST enable bits to reduce leakage current */
+	phyclkrst = readl(regs + EXYNOS5_DRD_PHYCLKRST);
+	phyclkrst &= ~(PHYCLKRST_REF_SSP_EN |
+			PHYCLKRST_SSC_EN |
+			PHYCLKRST_COMMONONN);
+	writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+	/* Control PHYTEST to remove leakage current */
+	phytest = readl(regs + EXYNOS5_DRD_PHYTEST);
+	phytest |= (PHYTEST_POWERDOWN_SSP |
+			PHYTEST_POWERDOWN_HSP);
+	writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
+}
+
 static void samsung_exynos5_usbphy_disable(struct samsung_usbphy *sphy)
 {
 	void __iomem *regs = sphy->regs;

@@ -491,6 +723,76 @@ static void samsung_usbphy_disable(struct samsung_usbphy *sphy)
 /*
  * The function passed to the usb driver for phy initialization
  */
+static int samsung_usbphy3_init(struct usb_phy *phy3)
+{
+	struct samsung_usbphy *sphy;
+	int ret = 0;
+
+	sphy = phy3_to_sphy(phy3);
+
+	if (sphy->cpu_type != TYPE_EXYNOS5250) {
+		dev_err(sphy->dev, "Not a valid cpu_type for USB 3.0\n");
+		return -ENODEV;
+	}
+
+	/* setting default phy-type for USB 3.0 */
+	samsung_usbphy_set_type(&sphy->phy3, USB_PHY_TYPE_DRD);
+
+	/* Enable the phy clock */
+	ret = clk_prepare_enable(sphy->clk);
+	if (ret) {
+		dev_err(sphy->dev, "%s: clk_prepare_enable failed\n", __func__);
+		return ret;
+	}
+
+	/* Disable phy isolation */
+	if (sphy->plat && sphy->plat->pmu_isolation)
+		sphy->plat->pmu_isolation(false, sphy->phy_type);
+
+	/* Initialize usb phy registers */
+	samsung_exynos5_usbphy3_enable(sphy);
+
+	/* Disable the phy clock */
+	clk_disable_unprepare(sphy->clk);
+
+	return ret;
+}
+
+/*
+ * The function passed to the usb driver for phy shutdown
+ */
+static void samsung_usbphy3_shutdown(struct usb_phy *phy3)
+{
+	struct samsung_usbphy *sphy;
+
+	sphy = phy3_to_sphy(phy3);
+
+	if (sphy->cpu_type != TYPE_EXYNOS5250) {
+		dev_err(sphy->dev, "Not a valid cpu_type for USB 3.0\n");
+		return;
+	}
+
+	/* setting default phy-type for USB 3.0 */
+	samsung_usbphy_set_type(&sphy->phy3, USB_PHY_TYPE_DRD);
+
+	if (clk_prepare_enable(sphy->clk)) {
+		dev_err(sphy->dev, "%s: clk_prepare_enable failed\n", __func__);
+		return;
+	}
+
+	/* De-initialize usb phy registers */
+	samsung_exynos5_usbphy3_disable(sphy);
+
+	/* Enable phy isolation */
+	if (sphy->plat && sphy->plat->pmu_isolation)
+		sphy->plat->pmu_isolation(true, sphy->phy_type);
+
+	clk_disable_unprepare(sphy->clk);
+}
+
+/*
+ * The function passed to the usb driver for phy initialization
+ */
 static int samsung_usbphy_init(struct usb_phy *phy)
 {
 	struct samsung_usbphy *sphy;

@@ -570,6 +872,8 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *phy_mem;
 	void __iomem	*phy_base;
+	struct resource *phy3_mem;
+	void __iomem	*phy3_base = NULL;
 	struct clk *clk;
 	int	ret = 0;
 

@@ -618,7 +922,38 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
 
 	sphy->clk = clk;
 
+	if (sphy->cpu_type == TYPE_EXYNOS5250) {
+		phy3_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!phy3_mem) {
+			dev_err(dev, "%s: missing mem resource\n", __func__);
+			return -ENODEV;
+		}
+
+		phy3_base = devm_request_and_ioremap(dev, phy3_mem);
+		if (!phy3_base) {
+			dev_err(dev, "%s: register mapping failed\n", __func__);
+			return -ENXIO;
+		}
+	}
+
+	sphy->regs_phy3		= phy3_base;
+	sphy->phy3.dev		= sphy->dev;
+	sphy->phy3.label	= "samsung-usbphy3";
+	sphy->phy3.init		= samsung_usbphy3_init;
+	sphy->phy3.shutdown	= samsung_usbphy3_shutdown;
+
 	ret = usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
+	if (ret)
+		return ret;
+
+	if (sphy->cpu_type != TYPE_EXYNOS5250) {
+		dev_warn(dev, "Not a valid cpu_type for USB 3.0\n");
+	} else {
+		ret = usb_add_phy(&sphy->phy3, USB_PHY_TYPE_USB3);
+		if (ret)
+			return ret;
+	}
+
 	return ret;
 }
 

@@ -627,6 +962,8 @@ static int __exit samsung_usbphy_remove(struct platform_device *pdev)
 	struct samsung_usbphy *sphy = platform_get_drvdata(pdev);
 
 	usb_remove_phy(&sphy->phy);
+	if (sphy->cpu_type == TYPE_EXYNOS5250)
+		usb_remove_phy(&sphy->phy3);
 
 	return 0;
 }

diff --git a/include/linux/usb/samsung_usb_phy.h b/include/linux/usb/samsung_usb_phy.h
index 8c05462..ed6f6c4 100644
--- a/include/linux/usb/samsung_usb_phy.h
+++ b/include/linux/usb/samsung_usb_phy.h

@@ -16,6 +16,7 @@ enum samsung_usb_phy_type
 {
 	USB_PHY_TYPE_DEVICE,
 	USB_PHY_TYPE_HOST,
+	USB_PHY_TYPE_DRD,
 };
 
 #ifdef CONFIG_SAMSUNG_USBPHY

-- 
1.7.6.5

Adding base address information and required platform data
support for enabling USB DRD phy on exynos5250 SOC.

Signed-off-by: Vivek Gautam <redacted>
---
 arch/arm/boot/dts/exynos5250.dtsi            |    3 ++-
 arch/arm/mach-exynos/include/mach/regs-pmu.h |    4 ++++
 arch/arm/mach-exynos/setup-usb-phy.c         |    9 +++++++++
 3 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 82bf042..51693af 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi

@@ -220,7 +220,8 @@
 
 	usbphy {
 		compatible = "samsung,exynos5250-usbphy";
-		reg = <0x12130000 0x100>;
+		reg = <0x12130000 0x100>,
+		      <0x12100000 0x100>;
 	};
 
 	amba {

diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index d4e392b..67132b4 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h

@@ -39,6 +39,10 @@
 #define S5P_HDMI_PHY_CONTROL			S5P_PMUREG(0x0700)
 #define S5P_HDMI_PHY_ENABLE			(1 << 0)
 
+/* only for EXYNOS5250*/
+#define S5P_USBDRD_PHY_CONTROL			S5P_PMUREG(0x0704)
+#define S5P_USBDRD_PHY_ENABLE			(1 << 0)
+
 #define S5P_DAC_PHY_CONTROL			S5P_PMUREG(0x070C)
 #define S5P_DAC_PHY_ENABLE			(1 << 0)
 

diff --git a/arch/arm/mach-exynos/setup-usb-phy.c b/arch/arm/mach-exynos/setup-usb-phy.c
index 6c768e0..5e46fdd 100644
--- a/arch/arm/mach-exynos/setup-usb-phy.c
+++ b/arch/arm/mach-exynos/setup-usb-phy.c

@@ -238,6 +238,15 @@ void s5p_usb_phy_pmu_isolation(int on, int type)
 			writel(readl(S5P_USBHOST_PHY_CONTROL)
 				| S5P_USBHOST_PHY_ENABLE,
 					S5P_USBHOST_PHY_CONTROL);
+	} else if (type == USB_PHY_TYPE_DRD) {
+		if (on)
+			writel(readl(S5P_USBDRD_PHY_CONTROL)
+				& ~S5P_USBDRD_PHY_ENABLE,
+					S5P_USBDRD_PHY_CONTROL);
+		else
+			writel(readl(S5P_USBDRD_PHY_CONTROL)
+				| S5P_USBDRD_PHY_ENABLE,
+					S5P_USBDRD_PHY_CONTROL);
 	} else {
 		if (on)
 			writel(readl(S5P_USBDEVICE_PHY_CONTROL)

-- 
1.7.6.5

Hi,

I have a few comments. Please see below...

On 11/06/2012 04:36 PM, Vivek Gautam wrote:

Adding support for USB3.0 phy for dwc3 controller on
exynso5250 SOC.

exynso -> exynos

Would be nice to put all hex numbers in lower case.

+
+#define EXYNOS5_DRD_PHYCLKRST			(0x10)
+
+#define PHYCLKRST_SSC_REFCLKSEL_MASK		(0xff<<  23)
+#define PHYCLKRST_SSC_REFCLKSEL(_x)		((_x)<<  23)
+
+#define PHYCLKRST_SSC_RANGE_MASK		(0x03<<  21)
+#define PHYCLKRST_SSC_RANGE(_x)			((_x)<<  21)
+
+#define PHYCLKRST_SSC_EN			(1<<  20)
+#define PHYCLKRST_REF_SSP_EN			(1<<  19)
+#define PHYCLKRST_REF_CLKDIV2			(1<<  18)
+
+#define PHYCLKRST_MPLL_MULTIPLIER_MASK		(0x7f<<  11)
+#define PHYCLKRST_MPLL_MULTIPLIER(_x)		((_x)<<  11)

Is this macro-definition going to be used anywhere else except the
5 definitions below ? Is this really helpful ? In anything else than
forcing you to use questionable line breaking below ?

+#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF 	\

How about simply defining it as

#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF	(0x19 << 11)

?

+		PHYCLKRST_MPLL_MULTIPLIER(0x19)
+#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF 	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x02)
+#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x68)
+#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x7d)
+#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF 	\
+		PHYCLKRST_MPLL_MULTIPLIER(0x02)
+
+#define PHYCLKRST_FSEL_MASK			(0x3f<<  5)
+#define PHYCLKRST_FSEL(_x)			((_x)<<  5)

Ditto.

+#define PHYCLKRST_FSEL_PAD_100MHZ		\
+		PHYCLKRST_FSEL(0x27)
+#define PHYCLKRST_FSEL_PAD_24MHZ		\
+		PHYCLKRST_FSEL(0x2a)
+#define PHYCLKRST_FSEL_PAD_20MHZ		\
+		PHYCLKRST_FSEL(0x31)
+#define PHYCLKRST_FSEL_PAD_19_2MHZ		\
+		PHYCLKRST_FSEL(0x38)
+
+#define PHYCLKRST_RETENABLEN			(1<<  4)
+
+#define PHYCLKRST_REFCLKSEL_MASK		(0x03<<  2)
+#define PHYCLKRST_REFCLKSEL(_x)			((_x)<<  2)

Ditto.

Wouldn't it be better to create a new data structure for USB 3.0
and embedding it here, rather than adding multiple fields with "3"
suffix ? E.g.

	struct {
		void __iomem	*phy_regs;
		struct usb_phy	phy;
	} usb3;
?

And why do you need to duplicate those fields in first place ?
I guess phy and phy3 are dependant and you can't register 2 PHYs
separately ?

Just do:

	return !(readl(regs + EXYNOS5_PHY_HOST_CTRL0) &	HOST_CTRL0_SIDDQ);
or:
	u32 reg = readl(regs + EXYNOS5_PHY_HOST_CTRL0);
	return !(reg & HOST_CTRL0_SIDDQ);

  }

+static int samsung_exynos5_usbphy3_enable(struct samsung_usbphy *sphy)
+{
+	void __iomem *regs = sphy->regs_phy3;
+	u32 phyparam0;
+	u32 phyparam1;
+	u32 linksystem;
+	u32 phybatchg;
+	u32 phytest;
+	u32 phyclkrst;
+
+	/* Reset USB 3.0 PHY */
+	writel(0x0, regs + EXYNOS5_DRD_PHYREG0);
+
+	phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
+	/* Select PHY CLK source */
+	phyparam0&= ~PHYPARAM0_REF_USE_PAD;
+	/* Set Loss-of-Signal Detector sensitivity */
+	phyparam0&= ~PHYPARAM0_REF_LOSLEVEL_MASK;
+	phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
+	writel(phyparam0, regs + EXYNOS5_DRD_PHYPARAM0);
+
+	writel(0x0, regs + EXYNOS5_DRD_PHYRESUME);
+
+	/*
+	 * Setting the Frame length Adj value[6:1] to default 0x20
+	 * See xHCI 1.0 spec, 5.2.4
+	 */
+	linksystem = LINKSYSTEM_XHCI_VERSION_CONTROL |
+			LINKSYSTEM_FLADJ(0x20);
+	writel(linksystem, regs + EXYNOS5_DRD_LINKSYSTEM);
+
+	phyparam1 = readl(regs + EXYNOS5_DRD_PHYPARAM1);
+	/* Set Tx De-Emphasis level */
+	phyparam1&= ~PHYPARAM1_PCS_TXDEEMPH_MASK;
+	phyparam1 |= PHYPARAM1_PCS_TXDEEMPH;
+	writel(phyparam1, regs + EXYNOS5_DRD_PHYPARAM1);
+
+	phybatchg = readl(regs + EXYNOS5_DRD_PHYBATCHG);
+	phybatchg |= PHYBATCHG_UTMI_CLKSEL;
+	writel(phybatchg, regs + EXYNOS5_DRD_PHYBATCHG);
+
+	/* PHYTEST POWERDOWN Control */
+	phytest = readl(regs + EXYNOS5_DRD_PHYTEST);
+	phytest&= ~(PHYTEST_POWERDOWN_SSP |
+			PHYTEST_POWERDOWN_HSP);
+	writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
+
+	/* UTMI Power Control */
+	writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
+
+	phyclkrst = exynos5_usbphy3_set_clock(sphy);
+
+	phyclkrst |= PHYCLKRST_PORTRESET |
+			/* Digital power supply in normal operating mode */
+			PHYCLKRST_RETENABLEN |
+			/* Enable ref clock for SS function */
+			PHYCLKRST_REF_SSP_EN |
+			/* Enable spread spectrum */
+			PHYCLKRST_SSC_EN |
+			/* Power down HS Bias and PLL blocks in suspend mode */
+			PHYCLKRST_COMMONONN;
+
+	writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+	udelay(10);

usleep_range() ?

This assignment could be done at the declaration time above.

+
+	if (sphy->cpu_type != TYPE_EXYNOS5250) {

How about adding some flag to struct samsung_usbphy telling whether PHY 3.0
is used or not, rather than having checks for all future cpu types all
over in this driver ?

2 redundant lines here.

+	}

The above code looks completely out of order. What's the point of 
initialising
sphy->phy3 and then not using it when sphy->cpu_type != TYPE_EXYNOS5250 ?

Thanks,
Sylwester

Hi,


On Wed, Nov 7, 2012 at 5:10 AM, Sylwester Nawrocki
[off-list ref] wrote:

Hi,

I have a few comments. Please see below...


On 11/06/2012 04:36 PM, Vivek Gautam wrote:

quoted

Adding support for USB3.0 phy for dwc3 controller on
exynso5250 SOC.

exynso -> exynos

Sure, will correct this.

quoted

Signed-off-by: Vivek Gautam<redacted>
---
  drivers/usb/phy/samsung-usbphy.c    |  337 +++++++++++++++++++++++++++++++++++
  include/linux/usb/samsung_usb_phy.h |    1 +
  2 files changed, 338 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index 3b4863d..e3b5fb1 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c

@@ -167,6 +167,99 @@

  #define EXYNOS5_PHY_OTG_TUNE                  (0x40)

+/* USB 3.0: DRD */
+#define EXYNOS5_DRD_LINKSYSTEM                 (0x04)
+
+#define LINKSYSTEM_FLADJ_MASK                  (0x3f<<  1)
+#define LINKSYSTEM_FLADJ(_x)                   ((_x)<<  1)
+#define LINKSYSTEM_XHCI_VERSION_CONTROL                (1<<  27)
+
+#define EXYNOS5_DRD_PHYUTMI                    (0x08)
+
+#define PHYUTMI_OTGDISABLE                     (1<<  6)
+#define PHYUTMI_FORCESUSPEND                   (1<<  1)
+#define PHYUTMI_FORCESLEEP                     (1<<  0)
+
+#define EXYNOS5_DRD_PHYPIPE                    (0x0C)

Would be nice to put all hex numbers in lower case.

Sure, will put the hex numbers in sync.

quoted

+
+#define EXYNOS5_DRD_PHYCLKRST                  (0x10)
+
+#define PHYCLKRST_SSC_REFCLKSEL_MASK           (0xff<<  23)
+#define PHYCLKRST_SSC_REFCLKSEL(_x)            ((_x)<<  23)
+
+#define PHYCLKRST_SSC_RANGE_MASK               (0x03<<  21)
+#define PHYCLKRST_SSC_RANGE(_x)                        ((_x)<<  21)
+
+#define PHYCLKRST_SSC_EN                       (1<<  20)
+#define PHYCLKRST_REF_SSP_EN                   (1<<  19)
+#define PHYCLKRST_REF_CLKDIV2                  (1<<  18)
+
+#define PHYCLKRST_MPLL_MULTIPLIER_MASK         (0x7f<<  11)
+#define PHYCLKRST_MPLL_MULTIPLIER(_x)          ((_x)<<  11)

Is this macro-definition going to be used anywhere else except the
5 definitions below ? Is this really helpful ? In anything else than
forcing you to use questionable line breaking below ?

quoted

+#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF   \

How about simply defining it as

#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF    (0x19 << 11)


?

Yes, we can write the way as suggested. Will amend this.

quoted

+               PHYCLKRST_MPLL_MULTIPLIER(0x19)
+#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF      \
+               PHYCLKRST_MPLL_MULTIPLIER(0x02)
+#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF    \
+               PHYCLKRST_MPLL_MULTIPLIER(0x68)
+#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF    \
+               PHYCLKRST_MPLL_MULTIPLIER(0x7d)
+#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF         \
+               PHYCLKRST_MPLL_MULTIPLIER(0x02)
+
+#define PHYCLKRST_FSEL_MASK                    (0x3f<<  5)
+#define PHYCLKRST_FSEL(_x)                     ((_x)<<  5)

Ditto.

Will amend this.

quoted

+#define PHYCLKRST_FSEL_PAD_100MHZ              \
+               PHYCLKRST_FSEL(0x27)
+#define PHYCLKRST_FSEL_PAD_24MHZ               \
+               PHYCLKRST_FSEL(0x2a)
+#define PHYCLKRST_FSEL_PAD_20MHZ               \
+               PHYCLKRST_FSEL(0x31)
+#define PHYCLKRST_FSEL_PAD_19_2MHZ             \
+               PHYCLKRST_FSEL(0x38)
+
+#define PHYCLKRST_RETENABLEN                   (1<<  4)
+
+#define PHYCLKRST_REFCLKSEL_MASK               (0x03<<  2)
+#define PHYCLKRST_REFCLKSEL(_x)                        ((_x)<<  2)

Ditto.

Will amend this.

quoted

+#define PHYCLKRST_REFCLKSEL_PAD_REFCLK         \
+               PHYCLKRST_REFCLKSEL(2)
+#define PHYCLKRST_REFCLKSEL_EXT_REFCLK         \
+               PHYCLKRST_REFCLKSEL(3)
+
+#define PHYCLKRST_PORTRESET                    (1<<  1)
+#define PHYCLKRST_COMMONONN                    (1<<  0)
+
+#define EXYNOS5_DRD_PHYREG0                    (0x14)
+#define EXYNOS5_DRD_PHYREG1                    (0x18)
+
+#define EXYNOS5_DRD_PHYPARAM0                  (0x1C)
+
+#define PHYPARAM0_REF_USE_PAD                  (0x1<<  31)
+#define PHYPARAM0_REF_LOSLEVEL_MASK            (0x1f<<  26)
+#define PHYPARAM0_REF_LOSLEVEL                 (0x9<<  26)
+
+#define EXYNOS5_DRD_PHYPARAM1                  (0x20)
+
+#define PHYPARAM1_PCS_TXDEEMPH_MASK            (0x1f<<  0)
+#define PHYPARAM1_PCS_TXDEEMPH                 (0x1C)
+
+#define EXYNOS5_DRD_PHYTERM                    (0x24)
+
+#define EXYNOS5_DRD_PHYTEST                    (0x28)
+
+#define PHYTEST_POWERDOWN_SSP                  (1<<  3)
+#define PHYTEST_POWERDOWN_HSP                  (1<<  2)
+
+#define EXYNOS5_DRD_PHYADP                     (0x2C)
+
+#define EXYNOS5_DRD_PHYBATCHG                  (0x30)
+
+#define PHYBATCHG_UTMI_CLKSEL                  (0x1<<  2)
+
+#define EXYNOS5_DRD_PHYRESUME                  (0x34)
+#define EXYNOS5_DRD_LINKPORT                   (0x44)
+
  #ifndef MHZ
  #define MHZ (1000*1000)
  #endif

@@ -180,10 +273,12 @@ enum samsung_cpu_type {
  /*
   * struct samsung_usbphy - transceiver driver state
   * @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
   * @plat: platform data
   * @dev: The parent device supplied to the probe function
   * @clk: usb phy clock
   * @regs: usb phy register memory base
+ * @regs_phy3: usb 3.0 phy register memory base
   * @ref_clk_freq: reference clock frequency selection
   * @cpu_type: machine identifier
   * @phy_type: It keeps track of the PHY type.

@@ -191,10 +286,12 @@ enum samsung_cpu_type {
   */
  struct samsung_usbphy {
        struct usb_phy  phy;
+       struct usb_phy  phy3;
        struct samsung_usbphy_data *plat;
        struct device   *dev;
        struct clk      *clk;
        void __iomem    *regs;
+       void __iomem    *regs_phy3;

Wouldn't it be better to create a new data structure for USB 3.0
and embedding it here, rather than adding multiple fields with "3"
suffix ? E.g.

        struct {
                void __iomem    *phy_regs;
                struct usb_phy  phy;
        } usb3;
?

Yes, thanks for suggesting. This way things will look clearer.
Will update this.

And why do you need to duplicate those fields in first place ?
I guess phy and phy3 are dependant and you can't register 2 PHYs
separately ?

Controllers like DWC3 needs two different PHYs. One of USB2 type and
one of USB3 type. So we needed to register two separate PHYs.

quoted

        int             ref_clk_freq;
        int             cpu_type;
        enum samsung_usb_phy_type phy_type;

@@ -202,6 +299,7 @@ struct samsung_usbphy {
  };

  #define phy_to_sphy(x)                container_of((x), struct samsung_usbphy, phy)
+#define phy3_to_sphy(x)                container_of((x), struct samsung_usbphy, phy3)

  /*
   * PHYs are different for USB Device and USB Host. Controllers can make

@@ -287,12 +385,120 @@ static int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy)
        return refclk_freq;
  }

+/*
+ * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock form clock core.
+ */
+static u32 exynos5_usbphy3_set_clock(struct samsung_usbphy *sphy)
+{
+       u32 reg;
+       u32 refclk;
+
+       refclk = sphy->ref_clk_freq;
+
+       reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK |
+               PHYCLKRST_FSEL(refclk);
+
+       switch (refclk) {
+       case HOST_CTRL0_FSEL_CLKSEL_50M:
+               reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF |
+                       PHYCLKRST_SSC_REFCLKSEL(0x00));
+               break;
+       case HOST_CTRL0_FSEL_CLKSEL_20M:
+               reg |= (PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF |
+                       PHYCLKRST_SSC_REFCLKSEL(0x00));
+               break;
+       case HOST_CTRL0_FSEL_CLKSEL_19200K:
+               reg |= (PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF |
+                       PHYCLKRST_SSC_REFCLKSEL(0x88));
+               break;
+       case HOST_CTRL0_FSEL_CLKSEL_24M:
+       default:
+               reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF |
+                       PHYCLKRST_SSC_REFCLKSEL(0x88));
+               break;
+       }
+
+       return reg;
+}
+
  static int exynos5_phyhost_is_on(void *regs)
  {
        return (readl(regs + EXYNOS5_PHY_HOST_CTRL0)&
                                HOST_CTRL0_SIDDQ) ? 0 : 1;

Just do:

        return !(readl(regs + EXYNOS5_PHY_HOST_CTRL0) & HOST_CTRL0_SIDDQ);
or:
        u32 reg = readl(regs + EXYNOS5_PHY_HOST_CTRL0);
        return !(reg & HOST_CTRL0_SIDDQ);

Sure, will amend this.

quoted

  }

+static int samsung_exynos5_usbphy3_enable(struct samsung_usbphy *sphy)
+{
+       void __iomem *regs = sphy->regs_phy3;
+       u32 phyparam0;
+       u32 phyparam1;
+       u32 linksystem;
+       u32 phybatchg;
+       u32 phytest;
+       u32 phyclkrst;
+
+       /* Reset USB 3.0 PHY */
+       writel(0x0, regs + EXYNOS5_DRD_PHYREG0);
+
+       phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
+       /* Select PHY CLK source */
+       phyparam0&= ~PHYPARAM0_REF_USE_PAD;

+       /* Set Loss-of-Signal Detector sensitivity */
+       phyparam0&= ~PHYPARAM0_REF_LOSLEVEL_MASK;

+       phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
+       writel(phyparam0, regs + EXYNOS5_DRD_PHYPARAM0);
+
+       writel(0x0, regs + EXYNOS5_DRD_PHYRESUME);
+
+       /*
+        * Setting the Frame length Adj value[6:1] to default 0x20
+        * See xHCI 1.0 spec, 5.2.4
+        */
+       linksystem = LINKSYSTEM_XHCI_VERSION_CONTROL |
+                       LINKSYSTEM_FLADJ(0x20);
+       writel(linksystem, regs + EXYNOS5_DRD_LINKSYSTEM);
+
+       phyparam1 = readl(regs + EXYNOS5_DRD_PHYPARAM1);
+       /* Set Tx De-Emphasis level */
+       phyparam1&= ~PHYPARAM1_PCS_TXDEEMPH_MASK;

+       phyparam1 |= PHYPARAM1_PCS_TXDEEMPH;
+       writel(phyparam1, regs + EXYNOS5_DRD_PHYPARAM1);
+
+       phybatchg = readl(regs + EXYNOS5_DRD_PHYBATCHG);
+       phybatchg |= PHYBATCHG_UTMI_CLKSEL;
+       writel(phybatchg, regs + EXYNOS5_DRD_PHYBATCHG);
+
+       /* PHYTEST POWERDOWN Control */
+       phytest = readl(regs + EXYNOS5_DRD_PHYTEST);
+       phytest&= ~(PHYTEST_POWERDOWN_SSP |

+                       PHYTEST_POWERDOWN_HSP);
+       writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
+
+       /* UTMI Power Control */
+       writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);
+
+       phyclkrst = exynos5_usbphy3_set_clock(sphy);
+
+       phyclkrst |= PHYCLKRST_PORTRESET |
+                       /* Digital power supply in normal operating mode */
+                       PHYCLKRST_RETENABLEN |
+                       /* Enable ref clock for SS function */
+                       PHYCLKRST_REF_SSP_EN |
+                       /* Enable spread spectrum */
+                       PHYCLKRST_SSC_EN |
+                       /* Power down HS Bias and PLL blocks in suspend mode */
+                       PHYCLKRST_COMMONONN;
+
+       writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+       udelay(10);

usleep_range() ?

quoted

+
+       phyclkrst&= ~(PHYCLKRST_PORTRESET);

+       writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+       return 0;
+}
+
  static void samsung_exynos5_usbphy_enable(struct samsung_usbphy *sphy)
  {
        void __iomem *regs = sphy->regs;

@@ -432,6 +638,32 @@ static void samsung_usbphy_enable(struct samsung_usbphy *sphy)
        writel(rstcon, regs + SAMSUNG_RSTCON);
  }

+static void samsung_exynos5_usbphy3_disable(struct samsung_usbphy *sphy)
+{
+       u32 phyutmi;
+       u32 phyclkrst;
+       u32 phytest;
+       void __iomem *regs = sphy->regs_phy3;
+
+       phyutmi = PHYUTMI_OTGDISABLE |
+                       PHYUTMI_FORCESUSPEND |
+                       PHYUTMI_FORCESLEEP;
+       writel(phyutmi, regs + EXYNOS5_DRD_PHYUTMI);
+
+       /* Resetting the PHYCLKRST enable bits to reduce leakage current */
+       phyclkrst = readl(regs + EXYNOS5_DRD_PHYCLKRST);
+       phyclkrst&= ~(PHYCLKRST_REF_SSP_EN |

+                       PHYCLKRST_SSC_EN |
+                       PHYCLKRST_COMMONONN);
+       writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST);
+
+       /* Control PHYTEST to remove leakage current */
+       phytest = readl(regs + EXYNOS5_DRD_PHYTEST);
+       phytest |= (PHYTEST_POWERDOWN_SSP |
+                       PHYTEST_POWERDOWN_HSP);
+       writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
+}
+
  static void samsung_exynos5_usbphy_disable(struct samsung_usbphy *sphy)
  {
        void __iomem *regs = sphy->regs;

@@ -491,6 +723,76 @@ static void samsung_usbphy_disable(struct samsung_usbphy *sphy)
  /*
   * The function passed to the usb driver for phy initialization
   */
+static int samsung_usbphy3_init(struct usb_phy *phy3)
+{
+       struct samsung_usbphy *sphy;
+       int ret = 0;
+
+       sphy = phy3_to_sphy(phy3);
+
+       if (sphy->cpu_type != TYPE_EXYNOS5250) {
+               dev_err(sphy->dev, "Not a valid cpu_type for USB 3.0\n");
+               return -ENODEV;
+       }
+
+       /* setting default phy-type for USB 3.0 */
+       samsung_usbphy_set_type(&sphy->phy3, USB_PHY_TYPE_DRD);
+
+       /* Enable the phy clock */
+       ret = clk_prepare_enable(sphy->clk);
+       if (ret) {
+               dev_err(sphy->dev, "%s: clk_prepare_enable failed\n", __func__);
+               return ret;
+       }
+
+       /* Disable phy isolation */
+       if (sphy->plat&&  sphy->plat->pmu_isolation)

+               sphy->plat->pmu_isolation(false, sphy->phy_type);
+
+       /* Initialize usb phy registers */
+       samsung_exynos5_usbphy3_enable(sphy);
+
+       /* Disable the phy clock */
+       clk_disable_unprepare(sphy->clk);
+
+       return ret;
+}
+
+/*
+ * The function passed to the usb driver for phy shutdown
+ */
+static void samsung_usbphy3_shutdown(struct usb_phy *phy3)
+{
+       struct samsung_usbphy *sphy;
+
+       sphy = phy3_to_sphy(phy3);

This assignment could be done at the declaration time above.

Yes, sure will change this.

quoted

+
+       if (sphy->cpu_type != TYPE_EXYNOS5250) {

How about adding some flag to struct samsung_usbphy telling whether PHY 3.0
is used or not, rather than having checks for all future cpu types all
over in this driver ?

Will update this accordingly.

quoted

+               dev_err(sphy->dev, "Not a valid cpu_type for USB 3.0\n");
+               return;
+       }
+
+       /* setting default phy-type for USB 3.0 */
+       samsung_usbphy_set_type(&sphy->phy3, USB_PHY_TYPE_DRD);
+
+       if (clk_prepare_enable(sphy->clk)) {
+               dev_err(sphy->dev, "%s: clk_prepare_enable failed\n", __func__);
+               return;
+       }
+
+       /* De-initialize usb phy registers */
+       samsung_exynos5_usbphy3_disable(sphy);
+
+       /* Enable phy isolation */
+       if (sphy->plat&&  sphy->plat->pmu_isolation)

+               sphy->plat->pmu_isolation(true, sphy->phy_type);
+
+       clk_disable_unprepare(sphy->clk);
+}
+
+/*
+ * The function passed to the usb driver for phy initialization
+ */
  static int samsung_usbphy_init(struct usb_phy *phy)
  {
        struct samsung_usbphy *sphy;

@@ -570,6 +872,8 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
        struct device *dev =&pdev->dev;

        struct resource *phy_mem;
        void __iomem    *phy_base;
+       struct resource *phy3_mem;
+       void __iomem    *phy3_base = NULL;
        struct clk *clk;
        int     ret = 0;

@@ -618,7 +922,38 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)

        sphy->clk = clk;

+       if (sphy->cpu_type == TYPE_EXYNOS5250) {
+               phy3_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+               if (!phy3_mem) {
+                       dev_err(dev, "%s: missing mem resource\n", __func__);
+                       return -ENODEV;
+               }
+
+               phy3_base = devm_request_and_ioremap(dev, phy3_mem);
+               if (!phy3_base) {
+                       dev_err(dev, "%s: register mapping failed\n", __func__);
+                       return -ENXIO;
+               }
+       }
+
+       sphy->regs_phy3         = phy3_base;
+       sphy->phy3.dev          = sphy->dev;
+       sphy->phy3.label        = "samsung-usbphy3";
+       sphy->phy3.init         = samsung_usbphy3_init;
+       sphy->phy3.shutdown     = samsung_usbphy3_shutdown;
+
        ret = usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
+       if (ret)
+               return ret;
+
+       if (sphy->cpu_type != TYPE_EXYNOS5250) {
+               dev_warn(dev, "Not a valid cpu_type for USB 3.0\n");
+       } else {
+               ret = usb_add_phy(&sphy->phy3, USB_PHY_TYPE_USB3);
+               if (ret)
+                       return ret;

2 redundant lines here.

Will two returns under if return not error codes ? The last return
actually returns success.
If still it needs modification, will do that.

quoted

+       }

The above code looks completely out of order. What's the point of initialising
sphy->phy3 and then not using it when sphy->cpu_type != TYPE_EXYNOS5250 ?

True, will put the initialization under a check.

quoted

+
        return ret;
  }

@@ -627,6 +962,8 @@ static int __exit samsung_usbphy_remove(struct platform_device *pdev)
        struct samsung_usbphy *sphy = platform_get_drvdata(pdev);

        usb_remove_phy(&sphy->phy);
+       if (sphy->cpu_type == TYPE_EXYNOS5250)
+               usb_remove_phy(&sphy->phy3);

        return 0;
  }

diff --git a/include/linux/usb/samsung_usb_phy.h b/include/linux/usb/samsung_usb_phy.h
index 8c05462..ed6f6c4 100644
--- a/include/linux/usb/samsung_usb_phy.h
+++ b/include/linux/usb/samsung_usb_phy.h

@@ -16,6 +16,7 @@ enum samsung_usb_phy_type
  {
        USB_PHY_TYPE_DEVICE,
        USB_PHY_TYPE_HOST,
+       USB_PHY_TYPE_DRD,
  };

  #ifdef CONFIG_SAMSUNG_USBPHY

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in

the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Thanks & Regards
Vivek

Hi Vivek,

On 11/06/2012 04:36 PM, Vivek Gautam wrote:

Hmm, couldn't it be added to your usbphy node above and then this register
left for the usb phy driver to do ioremap and control it directly ? Rather
than relying on the platform data callback ? I hoped this static mapping
can be dropped once there is a proper usb phy driver in place. AFAIU
arch/arm/mach-exynos/setup-usb-phy.c is supposed to be a non-dt only thing.

This is horrible coding style IMHO BTW. Why not just do

		u32 reg = readl(S5P_USBDRD_PHY_CONTROL);
		if (on)
			reg &= ~S5P_USBDRD_PHY_ENABLE;
		else
			reg |= ~S5P_USBDRD_PHY_ENABLE;
		writel(reg, S5P_USBDRD_PHY_CONTROL);

Or to create some read/modify/write helper ? Anyway, I suppose this whole
setup-usb-phy.c file is going to be removed, once exynos is completely dt
only.

+		else
+			writel(readl(S5P_USBDRD_PHY_CONTROL)
+				| S5P_USBDRD_PHY_ENABLE,
+					S5P_USBDRD_PHY_CONTROL);
  	} else {
  		if (on)
  			writel(readl(S5P_USBDEVICE_PHY_CONTROL)

--

Thanks,
Sylwester

On 11/07/2012 02:35 PM, Vivek Gautam wrote:

quoted

quoted

@@ -180,10 +273,12 @@ enum samsung_cpu_type {
   /*
    * struct samsung_usbphy - transceiver driver state
    * @phy: transceiver structure
+ * @phy3: transceiver structure for USB 3.0
    * @plat: platform data
    * @dev: The parent device supplied to the probe function
    * @clk: usb phy clock
    * @regs: usb phy register memory base
+ * @regs_phy3: usb 3.0 phy register memory base
    * @ref_clk_freq: reference clock frequency selection
    * @cpu_type: machine identifier
    * @phy_type: It keeps track of the PHY type.

@@ -191,10 +286,12 @@ enum samsung_cpu_type {
    */
   struct samsung_usbphy {
         struct usb_phy  phy;
+       struct usb_phy  phy3;
         struct samsung_usbphy_data *plat;
         struct device   *dev;
         struct clk      *clk;
         void __iomem    *regs;
+       void __iomem    *regs_phy3;

Wouldn't it be better to create a new data structure for USB 3.0
and embedding it here, rather than adding multiple fields with "3"
suffix ? E.g.

         struct {
                 void __iomem    *phy_regs;
                 struct usb_phy  phy;
         } usb3;
?

Yes, thanks for suggesting. This way things will look clearer.
Will update this.

quoted

And why do you need to duplicate those fields in first place ?
I guess phy and phy3 are dependant and you can't register 2 PHYs
separately ?

Controllers like DWC3 needs two different PHYs. One of USB2 type and
one of USB3 type. So we needed to register two separate PHYs.

OK, I was just wondering if there is any dependency between those two phys
so you need to aggregate them in one struct samsung_usbphy, rather than
creating two separate struct samsung_usbphy objects for them.

quoted

quoted

+/*
+ * The function passed to the usb driver for phy initialization
+ */
   static int samsung_usbphy_init(struct usb_phy *phy)
   {
         struct samsung_usbphy *sphy;

@@ -570,6 +872,8 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
         struct device *dev =&pdev->dev;

         struct resource *phy_mem;
         void __iomem    *phy_base;
+       struct resource *phy3_mem;
+       void __iomem    *phy3_base = NULL;
         struct clk *clk;
         int     ret = 0;

@@ -618,7 +922,38 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)

         sphy->clk = clk;

+       if (sphy->cpu_type == TYPE_EXYNOS5250) {
+               phy3_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+               if (!phy3_mem) {
+                       dev_err(dev, "%s: missing mem resource\n", __func__);
+                       return -ENODEV;
+               }
+
+               phy3_base = devm_request_and_ioremap(dev, phy3_mem);
+               if (!phy3_base) {
+                       dev_err(dev, "%s: register mapping failed\n", __func__);
+                       return -ENXIO;
+               }
+       }
+
+       sphy->regs_phy3         = phy3_base;
+       sphy->phy3.dev          = sphy->dev;
+       sphy->phy3.label        = "samsung-usbphy3";
+       sphy->phy3.init         = samsung_usbphy3_init;
+       sphy->phy3.shutdown     = samsung_usbphy3_shutdown;
+
         ret = usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
+       if (ret)
+               return ret;
+
+       if (sphy->cpu_type != TYPE_EXYNOS5250) {
+               dev_warn(dev, "Not a valid cpu_type for USB 3.0\n");
+       } else {
+               ret = usb_add_phy(&sphy->phy3, USB_PHY_TYPE_USB3);
+               if (ret)
+                       return ret;

2 redundant lines here.

Will two returns under if return not error codes ? The last return
actually returns success.
If still it needs modification, will do that.

It's up to you how you structure it. The last return returns whatever
value ret has. I can't see what is an advantage of doing something like:

	if (ret)
		return ret;

	return ret;
--

Thanks,
Sylwester