Hi Tomasz, On Sun, Nov 10, 2013 at 7:38 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: > Hi Vivek, > > On Thursday 31 of October 2013 13:15:41 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. >> >> Signed-off-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx> >> --- >> .../devicetree/bindings/phy/samsung-phy.txt | 20 + >> drivers/phy/Kconfig | 7 + >> drivers/phy/Makefile | 1 + >> drivers/phy/phy-exynos5-usb3.c | 562 ++++++++++++++++++++ >> 4 files changed, 590 insertions(+), 0 deletions(-) >> create mode 100644 drivers/phy/phy-exynos5-usb3.c >> >> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt >> index c0fccaa..9b5c111 100644 >> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt >> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt >> @@ -20,3 +20,23 @@ Required properties: >> - compatible : should be "samsung,exynos5250-dp-video-phy"; >> - reg : offset and length of the Display Port PHY register set; >> - #phy-cells : from the generic PHY bindings, must be 0; >> + >> +Samsung Exynos5 SoC seiries USB 3.0 PHY controller > > typo: s/seiries/series/ will correct it. > >> +-------------------------------------------------- >> + >> +Required properties: >> +- compatible : >> + should be "samsung,exynos5250-usb3phy" for exynos5250 SoC >> + should be "samsung,exynos5420-usb3phy" for exynos5420 SoC > > I'd slightly change this into something like this: > > - compatible: Should be set to one of following supported values: > - "samsung,exynos5250-usb3phy" - for Exynos5250 SoC, > - "samsung,exynos5420-usb3phy" - for Exynos5420 SoC. sure, will make it as suggested. > >> +- reg : Register offset and length array >> + - first field corresponds to USB 3.0 PHY register set; >> + - second field corresponds to PHY power isolation register >> + present in PMU; > > For consistency and to make things more future-proof, you should consider > using the PMU indirectly, through the syscon interface, as done in Leela > Krishna Amudala's series[1] in case of watchdog driver. Right that's a better way to do. But this will again introduce the register offset arithmetic once again. And in case of multiple USB 3.0 PHY controllers (like for Exynos5420), we would need to take extra care of each such offset, by having provision for aliases for the usb3phy nodes and then setting required offset before doing isolation. For Exynos5420 USB3.0 PHY channel 0 is controlled by 0x10040704; and USB3.0 PHY channel 1 is controlled by 0x10040708. > > I will tell Kamil to do the same for USB 2.0 PHY as well. > > [1] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/24652 > >> +- clocks: Clock IDs array as required by the controller >> +- clock-names: names of clocks correseponding to IDs in the clock property; >> + Required clocks: >> + - first clock is main PHY clock (same as USB 3.0 controller IP clock) >> + - second clock is reference clock (usually crystal clock) >> + optional clock: >> + - third clock is special clock used by PHY for operation > > Is this clock really optional? It looks like it's required for Exynos5420. Yes, this clock is an additional clock for Exynos5420 rather then being just optional > If so, you should instead change this to: > > "Additional clocks required for Exynos5420:" Ok will change this. > > Also you have not specified names of the clocks, just what they are. > Please remember that those are input names, so you can imagine them as > names of clock input pins of the IP block, not SoC-level clock names. So you mean, similar to what driver requests (clocks with their input names) ? will add clock names. > >> +- #phy-cells : from the generic PHY bindings, must be 0; > > I'd also add an example of correct USB 3.0 PHY device tree node here. Sorry, forgot to add an example of the device node :-) will add one. > >> diff --git a/drivers/phy/phy-exynos5-usb3.c b/drivers/phy/phy-exynos5-usb3.c >> new file mode 100644 >> index 0000000..b9a2674 >> --- /dev/null >> +++ b/drivers/phy/phy-exynos5-usb3.c >> @@ -0,0 +1,562 @@ > [snip] >> +#define EXYNOS5_DRD_PHYRESUME (0x34) >> +#define EXYNOS5_DRD_LINKPORT (0x44) >> + >> + > > nit: Duplicate blank line. will remove it. > >> +/* Isolation, configured in the power management unit */ >> +#define EXYNOS5_USB_ISOL_DRD (1 << 0) >> + >> +#define CLKSEL_ERROR -1 > > What's this? Hmm..i shouldn't be defining error codes out of blue, will remove it. > >> + >> +#ifndef KHZ >> +#define KHZ 1000 >> +#endif >> + >> +#ifndef MHZ >> +#define MHZ (KHZ * KHZ) >> +#endif > > Do you really need the #ifndef's above? You are right. #ifndef not really needed, since no header included here have these definitions. Although for samsung i can see they are defined in arch/arm/plat-samsung/include/plat/cpu.h; and i am sure we don't want to include that here. > >> + >> +enum samsung_cpu_type { >> + TYPE_EXYNOS5250, >> + TYPE_EXYNOS5420, >> +}; > > Instead of using this kind of enumeration, I'd rather introduce a struct > that describes the differences between all supported types. Will drop these, anyways they are not being used anywhere. > >> + >> +enum usb3phy_state { >> + STATE_OFF, >> + STATE_ON, >> +}; > > Hmm, isn't it a simple boolean value - false and true? Right :-) > >> + >> +struct usb3phy_config { >> + enum samsung_cpu_type cpu; >> + bool has_sclk_usbphy30; >> +}; > > Oh, you already have such struct, so there is even a bigger reason to drop > the samsung_cpu_type enum above. Right, i created this structure to make distinction between various cpu types. And moreover the "samsung_cpu_type" enumerations are not being used anywhere right now. I will drop the same. > >> + >> +struct usb3phy_instance { >> + char *label; >> + struct usb3phy_driver *drv; >> + struct phy *phy; >> + enum usb3phy_state state; >> + u32 clk; >> + unsigned long rate; >> +}; > > You seem to have just one instance in this driver. Do you really > need this struct? Right, i was hoping to get a comment on this ;-) Will move the field to "usb3phy_driver" structure. > >> + >> +struct usb3phy_driver { >> + struct device *dev; >> + void __iomem *reg_phy; >> + void __iomem *reg_isol; >> + struct clk *clk; >> + struct clk *sclk_usbphy30; >> + struct usb3phy_instance instance; > > Fields from that struct could be simply moved here. Hmm, will move them here. > >> +}; >> + >> +/* >> + * 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 = CLKSEL_ERROR; >> + } >> + >> + return clksel; >> +} >> + >> +static void exynos5_usb3phy_isol(struct usb3phy_instance *inst, bool on) >> +{ >> + struct usb3phy_driver *drv = inst->drv; >> + u32 tmp; >> + >> + if (!drv->reg_isol) >> + return; >> + >> + tmp = readl(drv->reg_isol); >> + if (on) >> + tmp &= ~EXYNOS5_USB_ISOL_DRD; >> + else >> + tmp |= EXYNOS5_USB_ISOL_DRD; >> + writel(tmp, drv->reg_isol); >> +} >> + >> +/* >> + * Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core. >> + */ >> +static u32 exynos5_usb3phy_set_refclk(struct usb3phy_instance *inst) >> +{ >> + u32 reg; >> + u32 refclk; >> + >> + refclk = inst->clk; >> + >> + reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK | >> + PHYCLKRST_FSEL(refclk); >> + >> + switch (refclk) { > > If I'm reading this correctly, you are switching a value returned by > another switch before (in exynos5_rate_to_clk()), which is only used in > this function. We are not only switching the refclk but also setting "PHYCLKRST_FSEL(__x)". Below i have given a brief explanation about the PHYCLKRST register settings required by PHY for different input reference clock frequencies. > > You could instead drop the exynos5_rate_to_clk() function completely and > simply make a switch over the real clock frequency here. > >> + case EXYNOS5_FSEL_50MHZ: >> + reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF | >> + PHYCLKRST_SSC_REFCLKSEL(0x00)); >> + 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; >> + case EXYNOS5_FSEL_24MHZ: >> + default: > > This switch does not seem to handle all the cases handled by > exynos5_rate_to_clk(). Does it mean that the value for 24 MHz refclk can > be used for all the remaining cases or they are not supported? I got the PHY databook from our H/w team. Looking into that i can find that there are two ways the operational clocks are generated by PHY block from input reference clock for super-speed and high-speed operations : 1) either using shared reference clock for HS and SS operations (when refclksel [3:2] = 0x10 in PHYCLKRST register of USB 3.0 PHY of Exynos5250) 2) or using separate reference clock for HS and SS operations (when refclksel [3:2] = 0x11 in PHYCLKRST register of USB 3.0 PHY of Exynos5250) Both approaches have different settings for PHYCLKRST register. Right now we are using the second approach. So, as given in databook - the bit settings used for High-speed are defined for input reference clock freq ranging from 9.6 MHz to 50MHz, as given by exynos5_rate_to_clk() function. Only bit setting needs to be done in this case is PHYCLKRST_FSEL(__x). - And, for super-speed the bit settings are defined for input reference clock freq ranging from 19.2 MHz to 200MHz. Bit settings to be done in this case are PHYCLKRST_MPLL_MULTIPLIER_** and PHYCLKRST_SSC_REFCLKSEL(__x). So basically exynos5_usb3phy_set_refclk() function includes a subset of input reference clock freq as defined by exynos5_rate_to_clk() function. Hope this explanation makes things a bit clear. > >> + reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF | >> + PHYCLKRST_SSC_REFCLKSEL(0x88)); >> + break; >> + } >> + >> + return reg; >> +} > [snip] >> +static int exynos5_usb3phy_power_on(struct phy *phy) >> +{ >> + struct usb3phy_instance *inst = phy_get_drvdata(phy); >> + struct usb3phy_driver *drv = inst->drv; >> + int ret; >> + >> + dev_info(drv->dev, "Request to power_on \"%s\" usb phy\n", >> + inst->label); > > dev_dbg() sure > >> + >> + if (drv->sclk_usbphy30) > > This check is incorrect. A NULL pointer is a correct value for a clock > as defined by Common Clock Framework, so IS_ERR() must be used here > instead. Right, will instead check of IS_ERR(drv->sclk_usbphy30), and act accrodingly. > >> + clk_prepare_enable(drv->sclk_usbphy30); > > Missing error check. Hmm, will add one. > >> + >> + ret = clk_prepare_enable(drv->clk); >> + if (ret) >> + return ret; >> + > > All the code starting here... > >> + if (inst->state == STATE_ON) { >> + dev_err(drv->dev, "usb phy \"%s\" already on", >> + inst->label); >> + ret = -ENODEV; >> + goto err0; >> + } >> + >> + inst->state = STATE_ON; > > ...and ending here, can be safely removed, since the PHY framework already > provides reference counting for PHYs. Sure will remove this chunk of code for state checking. > >> + >> + /* Initialize the PHY and power it ON */ >> + exynos5_usb3phy_init(inst); >> + exynos5_usb3phy_isol(inst, 0); >> + >> +err0: >> + clk_disable_unprepare(drv->clk); > > Is this clock needed for USB PHY operation or just for register accesses? This clock is required for register accesses. The operational clocks are derived out of reference clock given to PHY block, which comes from XXTI (FIN_PLL). > >> + >> + return ret; >> +} >> + >> +static int exynos5_usb3phy_power_off(struct phy *phy) >> +{ >> + struct usb3phy_instance *inst = phy_get_drvdata(phy); >> + struct usb3phy_driver *drv = inst->drv; >> + int ret; >> + >> + dev_info(drv->dev, "Request to power_off \"%s\" usb phy\n", >> + inst->label); > > dev_dbg() ok > >> + ret = clk_prepare_enable(drv->clk); >> + if (ret) >> + return ret; >> + >> + if (inst->state == STATE_OFF) { >> + dev_err(drv->dev, "usb phy \"%s\" already off", >> + inst->label); >> + ret = -ENODEV; >> + goto err0; >> + } >> + >> + inst->state = STATE_OFF; > > Same comment about the PHY framework already doing reference counting. Sure will remove this chunk of code for state checking. > >> + >> + /* Power-off the PHY and de-initialize it */ >> + exynos5_usb3phy_isol(inst, 1); >> + exynos5_usb3phy_exit(inst); >> + >> +err0: >> + clk_disable_unprepare(drv->clk); >> + if (drv->sclk_usbphy30) >> + clk_disable_unprepare(drv->sclk_usbphy30); >> + >> + return ret; >> +} >> + >> +static struct phy_ops exynos5_usb3phy_ops = { >> + .power_on = exynos5_usb3phy_power_on, >> + .power_off = exynos5_usb3phy_power_off, >> + .owner = THIS_MODULE, >> +}; >> + >> +static const struct of_device_id exynos5_usb3phy_of_match[]; > > What about simply moving the definition here instead using a forward > declaration? Ok, will move the of_device_id struct definition here. I just thought i would be cleaner to place it below probe () ;-) > >> + >> +static int exynos5_usb3phy_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct usb3phy_driver *drv; >> + struct usb3phy_instance *inst; >> + struct phy_provider *phy_provider; >> + struct resource *res; >> + struct clk *clk; >> + const struct of_device_id *match; >> + const struct usb3phy_config *cfg; >> + > > Just to be safe, you should check if pdev->dev.of_node is not NULL here, > to make sure that the driver got instantiated from device tree. Sure will add check for pdev->dev.of_node; and will return from probe with -ENODEV in case we don't find any node since Exynos5 and above are anyways DT enabled systems. > >> + match = of_match_node(exynos5_usb3phy_of_match, pdev->dev.of_node); >> + if (!match) { >> + dev_err(dev, "of_match_node() failed\n"); >> + return -EINVAL; >> + } >> + cfg = match->data; >> + if (!cfg) { > > I don't think this is possible. Hmm, will remove this check completely. > >> + dev_err(dev, "Failed to get configuration\n"); >> + return -EINVAL; >> + } >> + >> + drv = devm_kzalloc(dev, sizeof(struct usb3phy_driver), GFP_KERNEL); > > sizeof(*drv) sure > >> + if (!drv) { >> + dev_err(dev, "Failed to allocate memory\n"); > > kmalloc() and friends already print an error message for you. sorry i could not find the referred error message, can you please point me to that ? AFAICS most of the people print error messages if kmalloc() and friends failed. I tried finding until include/linux/slab.h. > >> + return -ENOMEM; >> + } >> + >> + dev_set_drvdata(dev, drv); >> + drv->dev = dev; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + drv->reg_phy = devm_ioremap_resource(dev, res); >> + if (IS_ERR(drv->reg_phy)) { >> + dev_err(dev, "Failed to map register memory (phy)\n"); >> + return PTR_ERR(drv->reg_phy); >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + drv->reg_isol = devm_ioremap_resource(dev, res); >> + if (IS_ERR(drv->reg_isol)) { >> + dev_err(dev, "Failed to map register memory (isolation)\n"); >> + return PTR_ERR(drv->reg_isol); >> + } >> + >> + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); >> + if (IS_ERR(phy_provider)) { >> + dev_err(drv->dev, "Failed to register phy provider\n"); >> + return PTR_ERR(phy_provider); >> + } > > The provider should be registered as the last thing in the sequence, as > the driver must be ready for handling PHY requests as soon as > of_phy_provider_register() returns. Sure, will move this at the end of probe(). > >> + >> + drv->clk = devm_clk_get(dev, "phy"); >> + if (IS_ERR(drv->clk)) { >> + dev_err(dev, "Failed to get clock of phy controller\n"); >> + return PTR_ERR(drv->clk); >> + } >> + >> + /* >> + * Exysno5420 SoC has an additional special clock, used for >> + * for USB 3.0 PHY operation, this clock goes to the PHY block >> + * as a reference clock to clock generation block of the controller. >> + */ >> + if (cfg->has_sclk_usbphy30) { >> + drv->sclk_usbphy30 = devm_clk_get(dev, "sclk_usbphy30"); >> + if (IS_ERR(drv->clk)) { >> + dev_err(dev, "Failed to get phy reference clock\n"); >> + return PTR_ERR(drv->clk); >> + } > > Seems like this clock is required for Exynos5420 indeed, as opposed to > what the DT binding documentation says. Yes, this is clock is required for Exynos5420. I will modify the device tree documentation further to avoid any confusion. > >> + } >> + >> + inst = &drv->instance; >> + inst->drv = drv; > > This pointer does not seem to be needed. Ok will drop it. In fact i am going to drop the instance thing entirely, so this makes sense. > >> + inst->label = "usb3drd"; > > Do you need this label at all? not really when we use phy's name string directly. ;-) will drop this. > >> + >> + dev_info(dev, "Creating phy \"%s\"\n", inst->label); > > dev_dbg() sure > >> + inst->phy = devm_phy_create(dev, &exynos5_usb3phy_ops, NULL); >> + if (IS_ERR(inst->phy)) { >> + dev_err(drv->dev, "Failed to create usb3phy \"%s\"\n", >> + inst->label); >> + return PTR_ERR(inst->phy); >> + } >> + >> + phy_set_drvdata(inst->phy, inst); >> + >> + clk = clk_get(dev, inst->label); > > Since this driver provides only a single PHY, I think you should simply > use the clock name directly. Ok will use the name string directly. > >> + if (IS_ERR(clk)) { >> + dev_err(dev, "Failed to get clock of \"%s\" phy\n", >> + inst->label); >> + return PTR_ERR(clk); >> + } >> + >> + inst->rate = clk_get_rate(clk); >> + >> + inst->clk = exynos5_rate_to_clk(inst->rate); >> + if (inst->clk == CLKSEL_ERROR) { >> + dev_err(dev, "Clock rate (%ld) not supported\n", >> + inst->rate); >> + clk_put(clk); >> + return -EINVAL; >> + } >> + clk_put(clk); > > Technically this should happen at the time of calling PHY enable, while > a reference to the clock should be kept through the whole PHY lifetime. > In addition, the clock should be prepare_enabled before it is used. This is actually the FIN_PLL (XXTI) clock which is being used as reference clock to the PHY block, which has its own clock generator. On knowing the rate of this FIN_PLL, the driver would like to program the PHY registers (PHYCLKRST in particular), which contain settings for generating different operational clocks used by PHY for high-speed and super-speed operations. > > So, to recall, here you could call devm_clk_get(...). Then in > exynos5_usb3phy_power_on(), call clk_prepare_enable() on it, in > exynos5_usb3phy_set_refclk() call clk_get_rate() to get its frequency > and finally clk_disable_unprepare() it in exynos5_usb3phy_power_off(). So do we really need to call prepare_enable() and disable_unprepare() over this Fin_PLL clock ? I can move this to the place where PHY is enabled. > > Best regards, > Tomasz Thanks Tomasz for reviewing this. I will address your comments and update the new patch-set soon. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards Vivek Gautam Samsung R&D Institute, Bangalore India -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |