Subject: [PATCH 3/7] drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework

From: [email-masked] (Kishon Vijay Abraham I)
Date: Mon, 14 Oct 2013 14:49:34 +0530

Hi Roger,

On Friday 11 October 2013 08:32 PM, Roger Quadros wrote:
> On 09/16/2013 10:37 AM, Roger Quadros wrote:
>> On 09/16/2013 06:01 AM, Kishon Vijay Abraham I wrote:
>>> On Thursday 12 September 2013 04:49 PM, Roger Quadros wrote:
>>>> Hi Kishon,
>>>>
>>>> On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote:
>>>>> Adapted omap-usb3 PHY driver to Generic PHY Framework and moved phy-omap-usb3
>>>>> driver in drivers/usb/phy to drivers/phy and also renamed the file to
>>>>> phy-omap-pipe3 since this same driver will be used for SATA PHY and
>>>>> PCIE PHY.
>>>>
>>>> I would suggest to split the renaming and PHY adaptation into 2 separate patches.
>>>
>>> Alright.
>>>>
>>>>>
>>>>> Signed-off-by: Kishon Vijay Abraham I <[email-masked]>
>>>>> ---
>>>>>   Documentation/devicetree/bindings/usb/usb-phy.txt  |    5 +-
>>>>>   drivers/phy/Kconfig                                |   10 +
>>>>>   drivers/phy/Makefile                               |    1 +
>>>>>   .../phy/phy-omap-usb3.c => phy/phy-omap-pipe3.c}   |  206 +++++++++++---------
>>>>
>>>> how about naming it to phy-ti-pipe3.c as it is used on OMAP as well as non-OMAP e.g. DRA7.
>>>
>>> hmm.. I thought it would be consistent with other PHY drivers (phy-omap-usb2). Moreover DRA7 is OMAP based platform ;-) Maybe we should reserve that for later?
>>
>> OK. Up to you.
>>
>>>>
>>>>>   drivers/usb/phy/Kconfig                            |   11 --
>>>>>   drivers/usb/phy/Makefile                           |    1 -
>>>>>   include/linux/phy/omap_pipe3.h                     |   52 +++++
>>>>>   7 files changed, 177 insertions(+), 109 deletions(-)
>>>>>   rename drivers/{usb/phy/phy-omap-usb3.c => phy/phy-omap-pipe3.c} (57%)
>>>>>   create mode 100644 include/linux/phy/omap_pipe3.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/usb-phy.txt b/Documentation/devicetree/bindings/usb/usb-phy.txt
>>>>> index c0245c8..36bdb17 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/usb-phy.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-phy.txt
>>>>> @@ -21,10 +21,11 @@ usb2phy at 4a0ad080 {
>>>>>       #phy-cells = <0>;
>>>>>   };
>>>>>
>>>>> -OMAP USB3 PHY
>>>>> +OMAP PIPE3 PHY
>>>>>
>>>>>   Required properties:
>>>>> - - compatible: Should be "ti,omap-usb3"
>>>>> + - compatible: Should be "ti,omap-usb3", "ti,omap-pipe3", "ti,omap-sata"
>>>>> +   or "ti,omap-pcie"
>>>>>    - reg : Address and length of the register set for the device.
>>>>>    - reg-names: The names of the register addresses corresponding to the registers
>>>>>      filled in "reg".
>>>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>>>> index ac239ac..5c2e7a0 100644
>>>>> --- a/drivers/phy/Kconfig
>>>>> +++ b/drivers/phy/Kconfig
>>>>> @@ -27,6 +27,16 @@ config OMAP_USB2
>>>>>         The USB OTG controller communicates with the comparator using this
>>>>>         driver.
>>>>>
>>>>> +config OMAP_PIPE3
>>>>> +    tristate "OMAP PIPE3 PHY Driver"
>>>>> +    select GENERIC_PHY
>>>>> +    select OMAP_CONTROL_USB
>>>> how about
>>>>     depends on OMAP_CONTROL_USB
>>>
>>> From whatever I could make out from comments of Greg in my Generic PHY Framework, it's better to do a select of dependent modules instead of depends on.
>>
>> You can use select, provided the item you are selecting doesn't depend on anything else.
>> As OMAP_CONTROL_USB depends on ARCH_OMAP2PLUS, your configuration will fail if a user enables
>> OMAP_PIPE3 on non OMAP configuration.
>>
>> Further, in this case since it is OMAP related driver, there is no point in showing/building it
>> if OMAP platform is not selected, so you at least need [depends on "ARCH_OMAP2PLUS"] to fix
>> both issue I mentioned.
>>
>>>>
>>>> Also, if this is TI/OMAP it might as well depend on ARCH_OMAP.
>>>>
>>>>> +    help
>>>>> +      Enable this to support the PIPE3 PHY that is part of SOC. This
>>>>
>>>> worth mentioning TI OMAP/DRA SoCs.
>>>
>>> right.
>>>>
>>>>> +      driver takes care of all the PHY functionality apart from comparator.
>>>>> +      This driver interacts with the "OMAP Control PHY Driver" to power
>>>>> +      on/off the PHY.
>>>>> +
>>>>>   config TWL4030_USB
>>>>>       tristate "TWL4030 USB Transceiver Driver"
>>>>>       depends on TWL4030_CORE && REGULATOR_TWL4030 && USB_MUSB_OMAP2PLUS
>>>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>>>>> index 0dd8a98..48bf9f2 100644
>>>>> --- a/drivers/phy/Makefile
>>>>> +++ b/drivers/phy/Makefile
>>>>> @@ -4,4 +4,5 @@
>>>>>
>>>>>   obj-$(CONFIG_GENERIC_PHY)    += phy-core.o
>>>>>   obj-$(CONFIG_OMAP_USB2)        += phy-omap-usb2.o
>>>>> +obj-$(CONFIG_OMAP_PIPE3)    += phy-omap-pipe3.o
>>>>>   obj-$(CONFIG_TWL4030_USB)    += phy-twl4030-usb.o
>>>>> diff --git a/drivers/usb/phy/phy-omap-usb3.c b/drivers/phy/phy-omap-pipe3.c
>>>>> similarity index 57%
>>>>> rename from drivers/usb/phy/phy-omap-usb3.c
>>>>> rename to drivers/phy/phy-omap-pipe3.c
>>>>> index 4004f82..ee9a901 100644
>>>>> --- a/drivers/usb/phy/phy-omap-usb3.c
>>>>> +++ b/drivers/phy/phy-omap-pipe3.c
>>>>> @@ -1,5 +1,5 @@
>>>>>   /*
>>>>> - * omap-usb3 - USB PHY, talking to dwc3 controller in OMAP.
>>>>> + * omap-pipe3 - PHY driver for SATA, USB and PCIE in OMAP platforms
>>>>>    *
>>>>>    * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>>>>>    * This program is free software; you can redistribute it and/or modify
>>>>> @@ -19,7 +19,8 @@
>>>>>   #include <linux/module.h>
>>>>>   #include <linux/platform_device.h>
>>>>>   #include <linux/slab.h>
>>>>> -#include <linux/usb/omap_usb.h>
>>>>> +#include <linux/phy/omap_pipe3.h>
>>>>> +#include <linux/phy/phy.h>
>>>>>   #include <linux/of.h>
>>>>>   #include <linux/clk.h>
>>>>>   #include <linux/err.h>
>>>>> @@ -52,17 +53,17 @@
>>>>>
>>>>>   /*
>>>>>    * This is an Empirical value that works, need to confirm the actual
>>>>> - * value required for the USB3PHY_PLL_CONFIGURATION2.PLL_IDLE status
>>>>> - * to be correctly reflected in the USB3PHY_PLL_STATUS register.
>>>>> + * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
>>>>> + * to be correctly reflected in the PIPE3PHY_PLL_STATUS register.
>>>>>    */
>>>>>   # define PLL_IDLE_TIME  100;
>>>>>
>>>>> -struct usb_dpll_map {
>>>>> +struct pipe3_dpll_map {
>>>>>       unsigned long rate;
>>>>> -    struct usb_dpll_params params;
>>>>> +    struct pipe3_dpll_params params;
>>>>>   };
>>>>>
>>>>> -static struct usb_dpll_map dpll_map[] = {
>>>>> +static struct pipe3_dpll_map dpll_map[] = {
>>>>>       {12000000, {1250, 5, 4, 20, 0} },    /* 12 MHz */
>>>>>       {16800000, {3125, 20, 4, 20, 0} },    /* 16.8 MHz */
>>>>>       {19200000, {1172, 8, 4, 20, 65537} },    /* 19.2 MHz */
>>>>> @@ -71,7 +72,7 @@ static struct usb_dpll_map dpll_map[] = {
>>>>>       {38400000, {3125, 47, 4, 20, 92843} },    /* 38.4 MHz */
>>>>>   };
>>>>>
>>>>> -static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
>>>>> +static struct pipe3_dpll_params *omap_pipe3_get_dpll_params(unsigned long rate)
>>>>>   {
>>>>>       int i;
>>>>>
>>>>> @@ -83,110 +84,113 @@ static struct usb_dpll_params *omap_usb3_get_dpll_params(unsigned long rate)
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> -static int omap_usb3_suspend(struct usb_phy *x, int suspend)
>>>>> +static int omap_pipe3_power_off(struct phy *x)
>>>>>   {
>>>>> -    struct omap_usb *phy = phy_to_omapusb(x);
>>>>> -    int    val;
>>>>> +    struct omap_pipe3 *phy = phy_get_drvdata(x);
>>>>> +    int val;
>>>>>       int timeout = PLL_IDLE_TIME;
>>>>>
>>>>> -    if (suspend && !phy->is_suspended) {
>>>>> -        val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>>>> -        val |= PLL_IDLE;
>>>>> -        omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>>>> -
>>>>> -        do {
>>>>> -            val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
>>>>> -            if (val & PLL_TICOPWDN)
>>>>> -                break;
>>>>> -            udelay(1);
>>>>> -        } while (--timeout);
>>>>> -
>>>>> -        omap_control_usb_phy_power(phy->control_dev, 0);
>>>>> -
>>>>> -        phy->is_suspended    = 1;
>>>>> -    } else if (!suspend && phy->is_suspended) {
>>>>> -        phy->is_suspended    = 0;
>>>>> -
>>>>> -        val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>>>> -        val &= ~PLL_IDLE;
>>>>> -        omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>>>> -
>>>>> -        do {
>>>>> -            val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
>>>>> -            if (!(val & PLL_TICOPWDN))
>>>>> -                break;
>>>>> -            udelay(1);
>>>>> -        } while (--timeout);
>>>>> -    }
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>>>> +    val |= PLL_IDLE;
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>>>> +
>>>>> +    do {
>>>>> +        val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>>>>> +        if (val & PLL_TICOPWDN)
>>>>> +            break;
>>>>> +        udelay(1);
>>>>
>>>> Is it better to sleep instead of udelay?
>>>
>>> hmm.. yeah, I guess this function wont be called from interrupt context.
>>>>
>>>>> +    } while (--timeout);
>>>>> +
>>>>> +    omap_control_usb_phy_power(phy->control_dev, 0);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int omap_pipe3_power_on(struct phy *x)
>>>>> +{
>>>>> +    struct omap_pipe3 *phy = phy_get_drvdata(x);
>>>>> +    int val;
>>>>> +    int timeout = PLL_IDLE_TIME;
>>>>> +
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>>>> +    val &= ~PLL_IDLE;
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>>>> +
>>>>> +    do {
>>>>> +        val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>>>>> +        if (!(val & PLL_TICOPWDN))
>>>>> +            break;
>>>>> +        udelay(1);
>>>>
>>>> here too.
>>>
>>> Ok.
>>>>
>>>>> +    } while (--timeout);
>>>>>
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> -static void omap_usb_dpll_relock(struct omap_usb *phy)
>>>>> +static void omap_pipe3_dpll_relock(struct omap_pipe3 *phy)
>>>>>   {
>>>>>       u32        val;
>>>>>       unsigned long    timeout;
>>>>>
>>>>> -    omap_usb_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_GO, SET_PLL_GO);
>>>>>
>>>>>       timeout = jiffies + msecs_to_jiffies(20);
>>>>>       do {
>>>>> -        val = omap_usb_readl(phy->pll_ctrl_base, PLL_STATUS);
>>>>> +        val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
>>>>>           if (val & PLL_LOCK)
>>>>>               break;
>>>>>       } while (!WARN_ON(time_after(jiffies, timeout)));
>>>>>   }
>>>>>
>>>>> -static int omap_usb_dpll_lock(struct omap_usb *phy)
>>>>> +static int omap_pipe3_dpll_lock(struct omap_pipe3 *phy)
>>>>>   {
>>>>>       u32            val;
>>>>>       unsigned long        rate;
>>>>> -    struct usb_dpll_params *dpll_params;
>>>>> +    struct pipe3_dpll_params *dpll_params;
>>>>>
>>>>>       rate = clk_get_rate(phy->sys_clk);
>>>>> -    dpll_params = omap_usb3_get_dpll_params(rate);
>>>>> +    dpll_params = omap_pipe3_get_dpll_params(rate);
>>>>>       if (!dpll_params) {
>>>>>           dev_err(phy->dev,
>>>>>                 "No DPLL configuration for %lu Hz SYS CLK\n", rate);
>>>>>           return -EINVAL;
>>>>>       }
>>>>>
>>>>> -    val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>>>>>       val &= ~PLL_REGN_MASK;
>>>>>       val |= dpll_params->n << PLL_REGN_SHIFT;
>>>>> -    omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>>>>>
>>>>> -    val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
>>>>>       val &= ~PLL_SELFREQDCO_MASK;
>>>>>       val |= dpll_params->freq << PLL_SELFREQDCO_SHIFT;
>>>>> -    omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
>>>>>
>>>>> -    val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION1);
>>>>>       val &= ~PLL_REGM_MASK;
>>>>>       val |= dpll_params->m << PLL_REGM_SHIFT;
>>>>> -    omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION1, val);
>>>>>
>>>>> -    val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION4);
>>>>>       val &= ~PLL_REGM_F_MASK;
>>>>>       val |= dpll_params->mf << PLL_REGM_F_SHIFT;
>>>>> -    omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION4, val);
>>>>>
>>>>> -    val = omap_usb_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
>>>>> +    val = omap_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION3);
>>>>>       val &= ~PLL_SD_MASK;
>>>>>       val |= dpll_params->sd << PLL_SD_SHIFT;
>>>>> -    omap_usb_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
>>>>> +    omap_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION3, val);
>>>>>
>>>>> -    omap_usb_dpll_relock(phy);
>>>>> +    omap_pipe3_dpll_relock(phy);
>>>>>
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> -static int omap_usb3_init(struct usb_phy *x)
>>>>> +static int omap_pipe3_init(struct phy *x)
>>>>>   {
>>>>> -    struct omap_usb    *phy = phy_to_omapusb(x);
>>>>> +    struct omap_pipe3 *phy = phy_get_drvdata(x);
>>>>>       int ret;
>>>>>
>>>>> -    ret = omap_usb_dpll_lock(phy);
>>>>> +    ret = omap_pipe3_dpll_lock(phy);
>>>>>       if (ret)
>>>>>           return ret;
>>>>>
>>>>> @@ -195,9 +199,18 @@ static int omap_usb3_init(struct usb_phy *x)
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> -static int omap_usb3_probe(struct platform_device *pdev)
>>>>> +static struct phy_ops ops = {
>>>>> +    .init        = omap_pipe3_init,
>>>>> +    .power_on    = omap_pipe3_power_on,
>>>>> +    .power_off    = omap_pipe3_power_off,
>>>>> +    .owner        = THIS_MODULE,
>>>>> +};
>>>>> +
>>>>> +static int omap_pipe3_probe(struct platform_device *pdev)
>>>>>   {
>>>>> -    struct omap_usb *phy;
>>>>> +    struct omap_pipe3 *phy;
>>>>> +    struct phy *generic_phy;
>>>>> +    struct phy_provider *phy_provider;
>>>>>       struct resource *res;
>>>>>       struct device_node *node = pdev->dev.of_node;
>>>>>       struct device_node *control_node;
>>>>> @@ -208,7 +221,7 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>>>>
>>>>>       phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
>>>>>       if (!phy) {
>>>>> -        dev_err(&pdev->dev, "unable to alloc mem for OMAP USB3 PHY\n");
>>>>> +        dev_err(&pdev->dev, "unable to alloc mem for OMAP PIPE3 PHY\n");
>>>>>           return -ENOMEM;
>>>>>       }
>>>>>
>>>>> @@ -219,13 +232,6 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>>>>
>>>>>       phy->dev        = &pdev->dev;
>>>>>
>>>>> -    phy->phy.dev        = phy->dev;
>>>>> -    phy->phy.label        = "omap-usb3";
>>>>> -    phy->phy.init        = omap_usb3_init;
>>>>> -    phy->phy.set_suspend    = omap_usb3_suspend;
>>>>> -    phy->phy.type        = USB_PHY_TYPE_USB3;
>>>>> -
>>>>> -    phy->is_suspended    = 1;
>>>>>       phy->wkupclk = devm_clk_get(phy->dev, "usb_phy_cm_clk32k");
>>>>
>>>> As this is no longer USB specific, we need to make the clock binding generic as well.
>>>
>>> right. Its also needed when we have the same driver work for sata and pcie. Maybe do that in a separate patch?
>>
>> OK. up to you.
>>>>
>>>>>       if (IS_ERR(phy->wkupclk)) {
>>>>>           dev_err(&pdev->dev, "unable to get usb_phy_cm_clk32k\n");
>>>>> @@ -251,6 +257,11 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>>>>           dev_err(&pdev->dev, "Failed to get control device phandle\n");
>>>>>           return -EINVAL;
>>>>>       }
>>>>> +    phy_provider = devm_of_phy_provider_register(phy->dev,
>>>>> +            of_phy_simple_xlate);
>>>>> +    if (IS_ERR(phy_provider))
>>>>> +        return PTR_ERR(phy_provider);
>>>>> +
>>>>>       control_pdev = of_find_device_by_node(control_node);
>>>>>       if (!control_pdev) {
>>>>>           dev_err(&pdev->dev, "Failed to get control device\n");
>>>>> @@ -260,23 +271,27 @@ static int omap_usb3_probe(struct platform_device *pdev)
>>>>>       phy->control_dev = &control_pdev->dev;
>>>>>
>>>>>       omap_control_usb_phy_power(phy->control_dev, 0);
>>>>> -    usb_add_phy_dev(&phy->phy);
>>>>>
>>>>>       platform_set_drvdata(pdev, phy);
>>>>> -
>>>>>       pm_runtime_enable(phy->dev);
>>>>> +
>>>>> +    generic_phy = devm_phy_create(phy->dev, &ops, NULL);
>>>>> +    if (IS_ERR(generic_phy))
>>>>> +        return PTR_ERR(generic_phy);
>>>>> +
>>>>> +    phy_set_drvdata(generic_phy, phy);
>>>>> +
>>>>>       pm_runtime_get(&pdev->dev);
>>>>>
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> -static int omap_usb3_remove(struct platform_device *pdev)
>>>>> +static int omap_pipe3_remove(struct platform_device *pdev)
>>>>>   {
>>>>> -    struct omap_usb *phy = platform_get_drvdata(pdev);
>>>>> +    struct omap_pipe3 *phy = platform_get_drvdata(pdev);
>>>>>
>>>>>       clk_unprepare(phy->wkupclk);
>>>>>       clk_unprepare(phy->optclk);
>>>>> -    usb_remove_phy(&phy->phy);
>>>>>       if (!pm_runtime_suspended(&pdev->dev))
>>>>>           pm_runtime_put(&pdev->dev);
>>>>>       pm_runtime_disable(&pdev->dev);
>>>>> @@ -286,10 +301,9 @@ static int omap_usb3_remove(struct platform_device *pdev)
>>>>>
>>>>>   #ifdef CONFIG_PM_RUNTIME
>>>>>
>>>>> -static int omap_usb3_runtime_suspend(struct device *dev)
>>>>> +static int omap_pipe3_runtime_suspend(struct device *dev)
>>>>>   {
>>>>> -    struct platform_device    *pdev = to_platform_device(dev);
>>>>> -    struct omap_usb    *phy = platform_get_drvdata(pdev);
>>>>> +    struct omap_pipe3    *phy = dev_get_drvdata(dev);
>>>>>
>>>>>       clk_disable(phy->wkupclk);
>>>>>       clk_disable(phy->optclk);
>>>>> @@ -297,11 +311,10 @@ static int omap_usb3_runtime_suspend(struct device *dev)
>>>>>       return 0;
>>>>>   }
>>>>>
>>>>> -static int omap_usb3_runtime_resume(struct device *dev)
>>>>> +static int omap_pipe3_runtime_resume(struct device *dev)
>>>>>   {
>>>>>       u32 ret = 0;
>>>>> -    struct platform_device    *pdev = to_platform_device(dev);
>>>>> -    struct omap_usb    *phy = platform_get_drvdata(pdev);
>>>>> +    struct omap_pipe3    *phy = dev_get_drvdata(dev);
>>>>>
>>>>>       ret = clk_enable(phy->optclk);
>>>>>       if (ret) {
>>>>> @@ -324,38 +337,41 @@ err1:
>>>>>       return ret;
>>>>>   }
>>>>>
>>>>> -static const struct dev_pm_ops omap_usb3_pm_ops = {
>>>>> -    SET_RUNTIME_PM_OPS(omap_usb3_runtime_suspend, omap_usb3_runtime_resume,
>>>>> -        NULL)
>>>>> +static const struct dev_pm_ops omap_pipe3_pm_ops = {
>>>>> +    SET_RUNTIME_PM_OPS(omap_pipe3_runtime_suspend,
>>>>> +        omap_pipe3_runtime_resume, NULL)
>>>>>   };
>>>>>
>>>>> -#define DEV_PM_OPS     (&omap_usb3_pm_ops)
>>>>> +#define DEV_PM_OPS     (&omap_pipe3_pm_ops)
>>>>>   #else
>>>>>   #define DEV_PM_OPS     NULL
>>>>>   #endif
>>>>>
>>>>>   #ifdef CONFIG_OF
>>>>> -static const struct of_device_id omap_usb3_id_table[] = {
>>>>> +static const struct of_device_id omap_pipe3_id_table[] = {
>>>>> +    { .compatible = "ti,omap-pipe3" },
>>>>
>>>> why do you need "omap-pipe3", isn't sata, pcie and usb3 sufficient?
>>>
>>> I thought it would be better if everyone uses omap-pipe3 and added pcie, sata if there are any specific settings (for pcie or sata) that should be done.
>>
>> We can always add specialized options later when needed. AFAIK just the
>> DPLL data is different among the different PHYs.
>>>>
>>>>> +    { .compatible = "ti,omap-sata" },
>>>>> +    { .compatible = "ti,omap-pcie" },
>>>>>       { .compatible = "ti,omap-usb3" },
>>
>> I think compatible strings should be improved to indicate that it is a PHY.
>>
>> e.g. "ti,omap-phy-sata" or just "ti,pipe3-phy-sata"
>>
> 
> Please remove "ti,omap-pcie" for now, you can add it later whenever you add
> dpll settings for pcie.
> 
> Also, please change the newly added compatible strings to
> 
> "ti,phy-pipe3-usb3" and "ti,phy-pipe3-sata"

No, I think we should have omap in the compatible since this PHY is specific to
OMAP.
> 
> Also you need to rearrange the patches so that the DT binding document is first
> moded from usb/ to phy/ and then the change is done for pipe3.

Ok.

Thanks
Kishon