Hi, On Thursday 30 January 2014 09:49 AM, Vivek Gautam wrote: > Hi Kishon, > > > On Mon, Jan 27, 2014 at 2:27 PM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote: >> Hi, > > Thanks for review. Please find my answers inline below. > >> >> On Monday 20 January 2014 07:12 PM, Vivek Gautam wrote: >>> 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 <gautam.vivek@xxxxxxxxxxx> >>> --- >>> >>> 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] > >>> 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? >>> + 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. > >>> + help >>> + Enable USB 3.0 PHY support for Exynos 5 SoC series >>> + >>> endmenu > [snip] > >>> 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@xxxxxxxxxxx> >>> + * >>> + * 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. > >>> + >>> +#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.. >>> + >>> +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 > >>> + * @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. > >>> + >>> + 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. > >>> + 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. > >>> + >>> + 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. > >>> + 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. > >>> + > [snip] > >>> + >>> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html