Hi,


On Thu, Jan 30, 2014 at 11:48 AM, Kishon Vijay Abraham I [off-list ref] wrote:

Hi,

On Thursday 30 January 2014 09:49 AM, Vivek Gautam wrote:

quoted

Hi Kishon,


On Mon, Jan 27, 2014 at 2:27 PM, Kishon Vijay Abraham I [off-list ref] wrote:

quoted

Hi,

Thanks for review. Please find my answers inline below.

quoted

On Monday 20 January 2014 07:12 PM, Vivek Gautam wrote:

quoted

Add a new driver for the USB 3.0 PHY on Exynos5 series of SoCs.
The new driver uses the generic PHY framework and will interact
with DWC3 controller present on Exynos5 series of SoCs.
Thereby, removing old phy-samsung-usb3 driver and related code
used untill now which was based on usb/phy framework.

Signed-off-by: Vivek Gautam <redacted>
---

Changes from v2:
1) Added support for multiple PHYs (UTMI+ and PIPE3) and
   related changes in the driver structuring.
2) Added a xlate function to get the required phy out of
   number of PHYs in mutiple PHY scenerio.
3) Changed the names of few structures and variables to
   have a clearer meaning.
4) Added 'usb3phy_config' structure to take care of mutiple
   phys for a SoC having 'exynos5_usb3phy_drv_data' driver data.
5) Not deleting support for old driver 'phy-samsung-usb3' until
   required support for generic phy is added to DWC3.

 .../devicetree/bindings/phy/samsung-phy.txt        |   49 ++
 drivers/phy/Kconfig                                |    8 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/phy-exynos5-usb3.c                     |  621 ++++++++++++++++++++
 4 files changed, 679 insertions(+)
 create mode 100644 drivers/phy/phy-exynos5-usb3.c

[snip]

quoted

quoted

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 330ef2d..32f9f38 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig

@@ -51,4 +51,12 @@ config PHY_EXYNOS_DP_VIDEO
      help
        Support for Display Port PHY found on Samsung EXYNOS SoCs.

+config PHY_EXYNOS5_USB3

This shouldn't be USB3 since this driver has support for both USB2 and USB3.
maybe just PHY_EXYNOS5_USB?

Ok, will change this.

quoted

quoted

quoted

+     tristate "Exynos5 SoC series USB 3.0 PHY driver"
+     depends on ARCH_EXYNOS5
+     select GENERIC_PHY
+     select MFD_SYSCON

add depends on 'HAS_IOMEM'. Someone reported getting
undefined reference to `devm_ioremap_resource' with it.

Ok will add it.

quoted

quoted

+     help
+       Enable USB 3.0 PHY support for Exynos 5 SoC series
+
 endmenu

[snip]

quoted

quoted

diff --git a/drivers/phy/phy-exynos5-usb3.c b/drivers/phy/phy-exynos5-usb3.c
new file mode 100644
index 0000000..24efed0
--- /dev/null
+++ b/drivers/phy/phy-exynos5-usb3.c

@@ -0,0 +1,621 @@
+/*
+ * Samsung EXYNOS5 SoC series USB 3.0 PHY driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Vivek Gautam <gautam.vivek@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+/* Exynos USB PHY registers */
+#define EXYNOS5_FSEL_9MHZ6           0x0
+#define EXYNOS5_FSEL_10MHZ           0x1
+#define EXYNOS5_FSEL_12MHZ           0x2
+#define EXYNOS5_FSEL_19MHZ2          0x3
+#define EXYNOS5_FSEL_20MHZ           0x4
+#define EXYNOS5_FSEL_24MHZ           0x5
+#define EXYNOS5_FSEL_50MHZ           0x7
+
+/* EXYNOS5: USB 3.0 DRD PHY registers */
+#define EXYNOS5_DRD_LINKSYSTEM                       (0x04)
+
+#define LINKSYSTEM_FLADJ_MASK                        (0x3f << 1)
+#define LINKSYSTEM_FLADJ(_x)                 ((_x) << 1)
+#define LINKSYSTEM_XHCI_VERSION_CONTROL              (0x1 << 27)
+
+#define EXYNOS5_DRD_PHYUTMI                  (0x08)
+
+#define PHYUTMI_OTGDISABLE                   (0x1 << 6)
+#define PHYUTMI_FORCESUSPEND                 (0x1 << 1)
+#define PHYUTMI_FORCESLEEP                   (0x1 << 0)

use BIT macro here and below?

Ok.

quoted

quoted

+
+#define EXYNOS5_DRD_PHYPIPE                  (0x0c)
+
+#define EXYNOS5_DRD_PHYCLKRST                        (0x10)
+
+#define PHYCLKRST_EN_UTMISUSPEND             (0x1 << 31)
+
+#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                     (0x1 << 20)
+#define PHYCLKRST_REF_SSP_EN                 (0x1 << 19)
+#define PHYCLKRST_REF_CLKDIV2                        (0x1 << 18)
+
+#define PHYCLKRST_MPLL_MULTIPLIER_MASK               (0x7f << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19 << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_50M_REF    (0x32 << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68 << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d << 11)
+#define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF       (0x02 << 11)
+
+#define PHYCLKRST_FSEL_MASK                  (0x3f << 5)
+#define PHYCLKRST_FSEL(_x)                   ((_x) << 5)
+#define PHYCLKRST_FSEL_PAD_100MHZ            (0x27 << 5)
+#define PHYCLKRST_FSEL_PAD_24MHZ             (0x2a << 5)
+#define PHYCLKRST_FSEL_PAD_20MHZ             (0x31 << 5)
+#define PHYCLKRST_FSEL_PAD_19_2MHZ           (0x38 << 5)
+
+#define PHYCLKRST_RETENABLEN                 (0x1 << 4)
+
+#define PHYCLKRST_REFCLKSEL_MASK             (0x03 << 2)
+#define PHYCLKRST_REFCLKSEL_PAD_REFCLK               (0x2 << 2)
+#define PHYCLKRST_REFCLKSEL_EXT_REFCLK               (0x3 << 2)
+
+#define PHYCLKRST_PORTRESET                  (0x1 << 1)
+#define PHYCLKRST_COMMONONN                  (0x1 << 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                        (0x1 << 3)
+#define PHYTEST_POWERDOWN_HSP                        (0x1 << 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)
+
+/* Power isolation defined in power management unit */
+#define EXYNOS5_USB3DRD_PHY_PMU_REG_OFFSET   (0x704)
+#define EXYNOS5_USB3DRD_PMU_ISOL             (1 << 0)
+
+#define KHZ  1000
+#define MHZ  (KHZ * KHZ)
+
+enum exynos5_usb3phy_id {
+     EXYNOS5_USB3PHY_UTMI,
+     EXYNOS5_USB3PHY_PIPE3,
+     EXYNOS5_USB3PHYS_NUM,
+};

here and all the structure names below shouldn't have usb3 in their names since
this is not just a 'usb3' phy driver..

right, will change the nomenclature driver-wide (including function names too).

quoted

quoted

quoted

+
+struct usb3phy_config {
+     u32 id;
+     u32 reg_pmu_offset;
+     void (*phy_isol)(struct phy *phy, u32 on);
+};
+
+struct exynos5_usb3phy_drv_data {
+     bool has_usb30_sclk;
+     bool has_multi_controller;
+     const struct usb3phy_config *phy_cfg;
+};
+
+/**
+ * struct exynos5_usb3phy_driver - driver data for USB 3.0 PHY

Is this really a driver data? I think it should be just exynos5_usb3phy.

Yes, not a driver data, rather just 'exynos_usb3phy' structure. will
modify the name

quoted

quoted

+ * @dev: pointer to device instance of this platform device
+ * @reg_phy: usb phy controller register memory base
+ * @clk: phy clock for register access
+ * @usb30_sclk: additional special clock for phy operations
+ * @drv_data: pointer to SoC level driver data structure
+ * @phys[]: array for 'EXYNOS5_USB3PHYS_NUM' number of PHY
+ *       instances each with its 'phy' and 'phy_cfg'.
+ * @extrefclk: frequency select settings when using 'separate
+ *          reference clocks' for SS and HS operations
+ * @rate: rate of reference clock to PHY block
+ * @channel: number of PHY channels present in SoC
+ */
+struct exynos5_usb3phy_driver {
+     struct device *dev;
+     void __iomem *reg_phy;
+     struct clk *clk;
+     struct clk *usb30_sclk;
+     const struct exynos5_usb3phy_drv_data *drv_data;
+     struct phy_usb_instance {
+             struct phy *phy;
+             u32 index;
+             struct regmap *reg_isol;
+             const struct usb3phy_config *phy_cfg;
+     } phys[EXYNOS5_USB3PHYS_NUM];
+     u32 extrefclk;
+     unsigned long rate;
+     u32 channel;
+};
+
+#define to_usb3phy_driver(inst) \
+     container_of((inst), struct exynos5_usb3phy_driver, \
+                  phys[(inst)->index]);
+
+/*
+ * exynos5_rate_to_clk() converts the supplied clock rate to the value that
+ * can be written to the phy register.
+ */
+static u32 exynos5_rate_to_clk(unsigned long rate)
+{
+     unsigned int clksel;
+
+     /* EXYNOS5_FSEL_MASK */
+
+     switch (rate) {
+     case 9600 * KHZ:
+             clksel = EXYNOS5_FSEL_9MHZ6;
+             break;
+     case 10 * MHZ:
+             clksel = EXYNOS5_FSEL_10MHZ;
+             break;
+     case 12 * MHZ:
+             clksel = EXYNOS5_FSEL_12MHZ;
+             break;
+     case 19200 * KHZ:
+             clksel = EXYNOS5_FSEL_19MHZ2;
+             break;
+     case 20 * MHZ:
+             clksel = EXYNOS5_FSEL_20MHZ;
+             break;
+     case 24 * MHZ:
+             clksel = EXYNOS5_FSEL_24MHZ;
+             break;
+     case 50 * MHZ:
+             clksel = EXYNOS5_FSEL_50MHZ;
+             break;
+     default:
+             clksel = -EINVAL;
+     }
+
+     return clksel;
+}
+
+static void exynos5_usb3phy_isol(struct phy *phy, unsigned int on)
+{
+     u32 pmu_offset;
+     struct phy_usb_instance *inst = phy_get_drvdata(phy);
+     struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
+
+     pmu_offset = inst->phy_cfg->reg_pmu_offset;
+     if (!inst->reg_isol)
+             return;
+
+     switch (drv->channel) {
+     case 1:
+             /* Channel 1 is at 0x708 offset */
+             pmu_offset += sizeof(&pmu_offset);
+             break;
+     case 0:
+     default:
+             /* Channel 0 is at 0x704 offset */
+             break;
+     }

This can be in a simple 'if' stmt no?

What if there are systems with more channels? In that case also we
will have to fall back to a switch-case statement ?

right.

quoted

quoted

quoted

+
+     regmap_update_bits(inst->reg_isol, pmu_offset,
+                        EXYNOS5_USB3DRD_PMU_ISOL, ~on);
+}
+
+/*
+ * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
+ */
+static u32 exynos5_usb3phy_set_refclk(struct exynos5_usb3phy_driver *drv)
+{
+     u32 reg;
+
+     reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK |
+           PHYCLKRST_FSEL(drv->extrefclk);
+
+     switch (drv->extrefclk) {
+     case EXYNOS5_FSEL_50MHZ:
+             reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF |
+                     PHYCLKRST_SSC_REFCLKSEL(0x00));
+             break;
+     case EXYNOS5_FSEL_24MHZ:
+             reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF |
+                     PHYCLKRST_SSC_REFCLKSEL(0x88));
+             break;
+     case EXYNOS5_FSEL_20MHZ:
+             reg |= (PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF |
+                     PHYCLKRST_SSC_REFCLKSEL(0x00));
+             break;
+     case EXYNOS5_FSEL_19MHZ2:
+             reg |= (PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF |
+                     PHYCLKRST_SSC_REFCLKSEL(0x88));
+             break;
+     default:
+             dev_dbg(drv->dev, "unsupported ref clk\n");
+             break;
+     }
+
+     return reg;
+}
+
+static int exynos5_usb3phy_init(struct phy *phy)
+{
+     int ret;
+     u32 phyparam0;
+     u32 phyparam1;
+     u32 linksystem;
+     u32 phybatchg;
+     u32 phytest;
+     u32 phyclkrst;

instead you can define a single variable 'u32 reg' for register read and writes.

Right.

quoted

quoted

+     struct phy_usb_instance *inst = phy_get_drvdata(phy);
+     struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
+
+     ret = clk_prepare_enable(drv->clk);
+     if (ret)
+             return ret;
+
+     drv->extrefclk = exynos5_rate_to_clk(drv->rate);
+     if (drv->extrefclk == -EINVAL) {
+             dev_err(drv->dev, "Clock rate (%ld) not supported\n",
+                                             drv->rate);
+             return -EINVAL;
+     }
+
+     /* Reset USB 3.0 PHY */
+     writel(0x0, drv->reg_phy + EXYNOS5_DRD_PHYREG0);
+
+     phyparam0 = readl(drv->reg_phy + 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, drv->reg_phy + EXYNOS5_DRD_PHYPARAM0);
+
+     writel(0x0, drv->reg_phy + 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, drv->reg_phy + EXYNOS5_DRD_LINKSYSTEM);
+
+     phyparam1 = readl(drv->reg_phy + EXYNOS5_DRD_PHYPARAM1);
+     /* Set Tx De-Emphasis level */
+     phyparam1 &= ~PHYPARAM1_PCS_TXDEEMPH_MASK;
+     phyparam1 |= PHYPARAM1_PCS_TXDEEMPH;
+     writel(phyparam1, drv->reg_phy + EXYNOS5_DRD_PHYPARAM1);
+
+     phybatchg = readl(drv->reg_phy + EXYNOS5_DRD_PHYBATCHG);
+     phybatchg |= PHYBATCHG_UTMI_CLKSEL;
+     writel(phybatchg, drv->reg_phy + EXYNOS5_DRD_PHYBATCHG);
+
+     /* PHYTEST POWERDOWN Control */
+     phytest = readl(drv->reg_phy + EXYNOS5_DRD_PHYTEST);
+     phytest &= ~(PHYTEST_POWERDOWN_SSP |
+                  PHYTEST_POWERDOWN_HSP);
+     writel(phytest, drv->reg_phy + EXYNOS5_DRD_PHYTEST);
+
+     /* UTMI Power Control */
+     writel(PHYUTMI_OTGDISABLE, drv->reg_phy + EXYNOS5_DRD_PHYUTMI);

All these UTMI configuration should be done in usb2 init.

Ok, will move this to separate function.

quoted

quoted

+
+     phyclkrst = exynos5_usb3phy_set_refclk(drv);
+
+     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, drv->reg_phy + EXYNOS5_DRD_PHYCLKRST);
+
+     udelay(10);
+
+     phyclkrst &= ~PHYCLKRST_PORTRESET;
+     writel(phyclkrst, drv->reg_phy + EXYNOS5_DRD_PHYCLKRST);
+
+     clk_disable_unprepare(drv->clk);
+
+     return 0;
+}
+
+static int exynos5_usb3phy_exit(struct phy *phy)
+{
+     int ret;
+     u32 phyutmi;
+     u32 phyclkrst;
+     u32 phytest;

same here..

right, will do it.

quoted

quoted

+     struct phy_usb_instance *inst = phy_get_drvdata(phy);
+     struct exynos5_usb3phy_driver *drv = to_usb3phy_driver(inst);
+
+     ret = clk_prepare_enable(drv->clk);
+     if (ret)
+             return ret;
+
+     phyutmi = PHYUTMI_OTGDISABLE |
+               PHYUTMI_FORCESUSPEND |
+               PHYUTMI_FORCESLEEP;
+     writel(phyutmi, drv->reg_phy + EXYNOS5_DRD_PHYUTMI);

here too.. UTMI configuration should be part of USB2.

ok.

quoted

quoted

+

[snip]

quoted

quoted

+
+static struct phy_ops exynos5_usb3phy_ops = {
+     .init           = exynos5_usb3phy_init,
+     .exit           = exynos5_usb3phy_exit,
+     .power_on       = exynos5_usb3phy_power_on,
+     .power_off      = exynos5_usb3phy_power_off,
+     .owner          = THIS_MODULE,
+};
+
+const struct usb3phy_config exynos5_usb3phy_cfg[] = {
+     {
+             .id             = EXYNOS5_USB3PHY_UTMI,

This should be USB2 no?

Actually the thought was to have similar naming for enums.
EXYNOS5_USB3PHY_UTMI
EXYNOS5_USB3PHY_PIPE3

Since the entire driver was going that way.
But will change these to a more common name
EXYNOS5_DRDPHY_UTMI
EXYNOS5_DRDPHY_PIPE3,
in the same fashion the register names are defined.
Will that be fine ?

Yeah.

Thanks
Kishon

-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India