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

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

[Vivek Gautam]

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

[PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250

[Vivek Gautam]

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

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 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

[PATCH 2/2] ARM: Exynos5250: Enabling USB 3.0 phy for samsung-usbphy driver

[Vivek Gautam]

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

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 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

Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250

[Sylwester Nawrocki]

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

>
> Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com>
> ---
>   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.

> +
> +#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.

> +#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;
?

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 ?

>   	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);
>   }
>
> +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() ?

> +
> +	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.

> +
> +	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 ?

> +		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.

> +	}

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 ?

> +
>   	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

Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250

[Vivek Gautam]

Hi,


On Wed, Nov 7, 2012 at 5:10 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
>
> 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

Sure, will correct this.

>
>
>>
>> Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com>
>> ---
>>   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.

>>
>> +
>> +#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)
>
>
> ?

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

>>
>> +               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.

>
>> +#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.

>
>> +#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.

>
>>         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.

>>
>>   }
>>
>> +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() ?
>
>> +
>> +       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.
>
>> +
>> +       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.

>> +               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.

>> +       }
>
>
> 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.
>
>> +
>>         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

Re: [PATCH 2/2] ARM: Exynos5250: Enabling USB 3.0 phy for samsung-usbphy driver

[Sylwester Nawrocki]

Hi Vivek,

On 11/06/2012 04:36 PM, Vivek Gautam wrote:
> Adding base address information and required platform data
> support for enabling USB DRD phy on exynos5250 SOC.
>
> Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com>
> ---
>   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)

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.

> +
>   #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);

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

Re: [PATCH 1/2] USB: PHY: Add support for USB 3.0 phy for exynos5250

[Sylwester Nawrocki]

On 11/07/2012 02:35 PM, Vivek Gautam wrote:
>>> @@ -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.

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.

>>> +/*
>>> + * 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