Hi, On Thu, Jan 30, 2014 at 11:48 AM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote: > 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? Ok, will change this. >>>> + 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.. right, will change the nomenclature driver-wide (including function names too). >>>> + >>>> +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 -- Best Regards Vivek Gautam Samsung R&D Institute, Bangalore India -- 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