From: Roger Quadros <rogerq@ti.com>
To: Kishon Vijay Abraham I <kishon@ti.com>, <tony@atomide.com>
Cc: <nm@ti.com>, <nsekhar@ti.com>, <balbi@ti.com>,
<grygorii.strashko@ti.com>, <linux-kernel@vger.kernel.org>,
<linux-omap@vger.kernel.org>
Subject: Re: [PATCH 1/3] phy: ti-pipe3: fix suspend
Date: Wed, 20 May 2015 16:48:31 +0300 [thread overview]
Message-ID: <555C90AF.4020709@ti.com> (raw)
In-Reply-To: <555C8651.7060805@ti.com>
Kishon,
On 20/05/15 16:04, Kishon Vijay Abraham I wrote:
> Hi Roger,
>
> On Tuesday 12 May 2015 09:37 PM, Roger Quadros wrote:
>> Relying on PM-ops for shutting down PHY clocks was a
>> bad idea since the users (e.g. PCIe/SATA) might not
>> have been suspended by then.
>>
>> The main culprit for not shutting down the clocks was
>> the stray pm_runtime_get() call in probe.
>>
>> Fix the whole thing in the right way by getting rid
>> of that pm_runtime_get() call from probe and
>> removing all PM-ops. It is the sole responsibility
>> of the PHY user to properly turn OFF and de-initialize
>> the PHY as part of its suspend routine.
>>
>> As PHY core serializes init/exit we don't need
>> to use a spinlock in this driver. So get rid of it.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> ---
>> drivers/phy/phy-ti-pipe3.c | 112
>> ++++++++-------------------------------------
>> 1 file changed, 18 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
>> index 53f295c..e13a306 100644
>> --- a/drivers/phy/phy-ti-pipe3.c
>> +++ b/drivers/phy/phy-ti-pipe3.c
>> @@ -28,7 +28,6 @@
>> #include <linux/delay.h>
>> #include <linux/phy/omap_control_phy.h>
>> #include <linux/of_platform.h>
>> -#include <linux/spinlock.h>
>>
>> #define PLL_STATUS 0x00000004
>> #define PLL_GO 0x00000008
>> @@ -83,10 +82,6 @@ struct ti_pipe3 {
>> struct clk *refclk;
>> struct clk *div_clk;
>> struct pipe3_dpll_map *dpll_map;
>> - bool enabled;
>> - spinlock_t lock; /* serialize clock enable/disable */
>> - /* the below flag is needed specifically for SATA */
>> - bool refclk_enabled;
>> };
>>
>> static struct pipe3_dpll_map dpll_map_usb[] = {
>> @@ -137,6 +132,9 @@ static struct pipe3_dpll_params
>> *ti_pipe3_get_dpll_params(struct ti_pipe3 *phy)
>> return NULL;
>> }
>>
>> +static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy);
>> +static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy);
>> +
>> static int ti_pipe3_power_off(struct phy *x)
>> {
>> struct ti_pipe3 *phy = phy_get_drvdata(x);
>> @@ -217,6 +215,7 @@ static int ti_pipe3_init(struct phy *x)
>> u32 val;
>> int ret = 0;
>>
>> + ti_pipe3_enable_clocks(phy);
>> /*
>> * Set pcie_pcs register to 0x96 for proper functioning of phy
>> * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table
>> @@ -277,6 +276,8 @@ static int ti_pipe3_exit(struct phy *x)
>> return -EBUSY;
>> }
>>
>> + ti_pipe3_disable_clocks(phy);
>> +
>> return 0;
>> }
>> static struct phy_ops ops = {
>> @@ -305,8 +306,7 @@ static int ti_pipe3_probe(struct platform_device
>> *pdev)
>> if (!phy)
>> return -ENOMEM;
>>
>> - phy->dev = &pdev->dev;
>> - spin_lock_init(&phy->lock);
>> + phy->dev = &pdev->dev;
>
> this looks more of a cleanup.
ok will get rid of it.
>>
>> if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>> match = of_match_device(ti_pipe3_id_table, &pdev->dev);
>> @@ -402,6 +402,9 @@ static int ti_pipe3_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, phy);
>> pm_runtime_enable(phy->dev);
>> + /* Prevent auto-disable of refclk for SATA PHY due to Errata i783 */
>> + if (of_device_is_compatible(node, "ti,phy-pipe3-sata"))
>> + clk_prepare_enable(phy->refclk);
>>
>> generic_phy = devm_phy_create(phy->dev, NULL, &ops);
>> if (IS_ERR(generic_phy))
>> @@ -413,24 +416,19 @@ static int ti_pipe3_probe(struct platform_device
>> *pdev)
>> if (IS_ERR(phy_provider))
>> return PTR_ERR(phy_provider);
>>
>> - pm_runtime_get(&pdev->dev);
>> -
>> return 0;
>> }
>>
>> static int ti_pipe3_remove(struct platform_device *pdev)
>> {
>> - if (!pm_runtime_suspended(&pdev->dev))
>> - pm_runtime_put(&pdev->dev);
>> pm_runtime_disable(&pdev->dev);
>>
>> return 0;
>> }
>>
>> -#ifdef CONFIG_PM
>> static int ti_pipe3_enable_refclk(struct ti_pipe3 *phy)
>> {
>> - if (!IS_ERR(phy->refclk) && !phy->refclk_enabled) {
>> + if (!IS_ERR(phy->refclk)) {
>> int ret;
>>
>> ret = clk_prepare_enable(phy->refclk);
>> @@ -438,7 +436,6 @@ static int ti_pipe3_enable_refclk(struct ti_pipe3
>> *phy)
>> dev_err(phy->dev, "Failed to enable refclk %d\n", ret);
>> return ret;
>> }
>> - phy->refclk_enabled = true;
>> }
>>
>> return 0;
>> @@ -448,28 +445,21 @@ static void ti_pipe3_disable_refclk(struct
>> ti_pipe3 *phy)
>> {
>> if (!IS_ERR(phy->refclk))
>> clk_disable_unprepare(phy->refclk);
>> -
>> - phy->refclk_enabled = false;
>> }
>>
>> static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
>> {
>> int ret = 0;
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&phy->lock, flags);
>> - if (phy->enabled)
>> - goto err1;
>>
>> ret = ti_pipe3_enable_refclk(phy);
>
> we can enable refclk here itself instead of having a separate function it?
>> if (ret)
>> - goto err1;
>> + return ret;
>>
>> if (!IS_ERR(phy->wkupclk)) {
>> ret = clk_prepare_enable(phy->wkupclk);
>> if (ret) {
>> dev_err(phy->dev, "Failed to enable wkupclk %d\n", ret);
>> - goto err2;
>> + goto disable_refclk;
>> }
>> }
>>
>> @@ -477,96 +467,31 @@ static int ti_pipe3_enable_clocks(struct
>> ti_pipe3 *phy)
>> ret = clk_prepare_enable(phy->div_clk);
>> if (ret) {
>> dev_err(phy->dev, "Failed to enable div_clk %d\n", ret);
>> - goto err3;
>> + goto disable_wkupclk;
>> }
>> }
>>
>> - phy->enabled = true;
>> - spin_unlock_irqrestore(&phy->lock, flags);
>> return 0;
>>
>> -err3:
>> +disable_wkupclk:
>> if (!IS_ERR(phy->wkupclk))
>> clk_disable_unprepare(phy->wkupclk);
>>
>> -err2:
>> - if (!IS_ERR(phy->refclk))
>> - clk_disable_unprepare(phy->refclk);
>> -
>> +disable_refclk:
>> ti_pipe3_disable_refclk(phy);
>> -err1:
>> - spin_unlock_irqrestore(&phy->lock, flags);
>> +
>> return ret;
>> }
>>
>> static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy)
>> {
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&phy->lock, flags);
>> - if (!phy->enabled) {
>> - spin_unlock_irqrestore(&phy->lock, flags);
>> - return;
>> - }
>> -
>> if (!IS_ERR(phy->wkupclk))
>> clk_disable_unprepare(phy->wkupclk);
>> - /* Don't disable refclk for SATA PHY due to Errata i783 */
>> - if (!of_device_is_compatible(phy->dev->of_node,
>> "ti,phy-pipe3-sata"))
>> - ti_pipe3_disable_refclk(phy);
>> + ti_pipe3_disable_refclk(phy);
>
> here too. we can get rid of ti_pipe3_disable_refclk and
> ti_pipe3_enable_refclk.
>
fine.
cheers,
-roger
WARNING: multiple messages have this Message-ID (diff)
From: Roger Quadros <rogerq@ti.com>
To: Kishon Vijay Abraham I <kishon@ti.com>, tony@atomide.com
Cc: nm@ti.com, nsekhar@ti.com, balbi@ti.com,
grygorii.strashko@ti.com, linux-kernel@vger.kernel.org,
linux-omap@vger.kernel.org
Subject: Re: [PATCH 1/3] phy: ti-pipe3: fix suspend
Date: Wed, 20 May 2015 16:48:31 +0300 [thread overview]
Message-ID: <555C90AF.4020709@ti.com> (raw)
In-Reply-To: <555C8651.7060805@ti.com>
Kishon,
On 20/05/15 16:04, Kishon Vijay Abraham I wrote:
> Hi Roger,
>
> On Tuesday 12 May 2015 09:37 PM, Roger Quadros wrote:
>> Relying on PM-ops for shutting down PHY clocks was a
>> bad idea since the users (e.g. PCIe/SATA) might not
>> have been suspended by then.
>>
>> The main culprit for not shutting down the clocks was
>> the stray pm_runtime_get() call in probe.
>>
>> Fix the whole thing in the right way by getting rid
>> of that pm_runtime_get() call from probe and
>> removing all PM-ops. It is the sole responsibility
>> of the PHY user to properly turn OFF and de-initialize
>> the PHY as part of its suspend routine.
>>
>> As PHY core serializes init/exit we don't need
>> to use a spinlock in this driver. So get rid of it.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> ---
>> drivers/phy/phy-ti-pipe3.c | 112
>> ++++++++-------------------------------------
>> 1 file changed, 18 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/phy/phy-ti-pipe3.c b/drivers/phy/phy-ti-pipe3.c
>> index 53f295c..e13a306 100644
>> --- a/drivers/phy/phy-ti-pipe3.c
>> +++ b/drivers/phy/phy-ti-pipe3.c
>> @@ -28,7 +28,6 @@
>> #include <linux/delay.h>
>> #include <linux/phy/omap_control_phy.h>
>> #include <linux/of_platform.h>
>> -#include <linux/spinlock.h>
>>
>> #define PLL_STATUS 0x00000004
>> #define PLL_GO 0x00000008
>> @@ -83,10 +82,6 @@ struct ti_pipe3 {
>> struct clk *refclk;
>> struct clk *div_clk;
>> struct pipe3_dpll_map *dpll_map;
>> - bool enabled;
>> - spinlock_t lock; /* serialize clock enable/disable */
>> - /* the below flag is needed specifically for SATA */
>> - bool refclk_enabled;
>> };
>>
>> static struct pipe3_dpll_map dpll_map_usb[] = {
>> @@ -137,6 +132,9 @@ static struct pipe3_dpll_params
>> *ti_pipe3_get_dpll_params(struct ti_pipe3 *phy)
>> return NULL;
>> }
>>
>> +static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy);
>> +static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy);
>> +
>> static int ti_pipe3_power_off(struct phy *x)
>> {
>> struct ti_pipe3 *phy = phy_get_drvdata(x);
>> @@ -217,6 +215,7 @@ static int ti_pipe3_init(struct phy *x)
>> u32 val;
>> int ret = 0;
>>
>> + ti_pipe3_enable_clocks(phy);
>> /*
>> * Set pcie_pcs register to 0x96 for proper functioning of phy
>> * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table
>> @@ -277,6 +276,8 @@ static int ti_pipe3_exit(struct phy *x)
>> return -EBUSY;
>> }
>>
>> + ti_pipe3_disable_clocks(phy);
>> +
>> return 0;
>> }
>> static struct phy_ops ops = {
>> @@ -305,8 +306,7 @@ static int ti_pipe3_probe(struct platform_device
>> *pdev)
>> if (!phy)
>> return -ENOMEM;
>>
>> - phy->dev = &pdev->dev;
>> - spin_lock_init(&phy->lock);
>> + phy->dev = &pdev->dev;
>
> this looks more of a cleanup.
ok will get rid of it.
>>
>> if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie")) {
>> match = of_match_device(ti_pipe3_id_table, &pdev->dev);
>> @@ -402,6 +402,9 @@ static int ti_pipe3_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, phy);
>> pm_runtime_enable(phy->dev);
>> + /* Prevent auto-disable of refclk for SATA PHY due to Errata i783 */
>> + if (of_device_is_compatible(node, "ti,phy-pipe3-sata"))
>> + clk_prepare_enable(phy->refclk);
>>
>> generic_phy = devm_phy_create(phy->dev, NULL, &ops);
>> if (IS_ERR(generic_phy))
>> @@ -413,24 +416,19 @@ static int ti_pipe3_probe(struct platform_device
>> *pdev)
>> if (IS_ERR(phy_provider))
>> return PTR_ERR(phy_provider);
>>
>> - pm_runtime_get(&pdev->dev);
>> -
>> return 0;
>> }
>>
>> static int ti_pipe3_remove(struct platform_device *pdev)
>> {
>> - if (!pm_runtime_suspended(&pdev->dev))
>> - pm_runtime_put(&pdev->dev);
>> pm_runtime_disable(&pdev->dev);
>>
>> return 0;
>> }
>>
>> -#ifdef CONFIG_PM
>> static int ti_pipe3_enable_refclk(struct ti_pipe3 *phy)
>> {
>> - if (!IS_ERR(phy->refclk) && !phy->refclk_enabled) {
>> + if (!IS_ERR(phy->refclk)) {
>> int ret;
>>
>> ret = clk_prepare_enable(phy->refclk);
>> @@ -438,7 +436,6 @@ static int ti_pipe3_enable_refclk(struct ti_pipe3
>> *phy)
>> dev_err(phy->dev, "Failed to enable refclk %d\n", ret);
>> return ret;
>> }
>> - phy->refclk_enabled = true;
>> }
>>
>> return 0;
>> @@ -448,28 +445,21 @@ static void ti_pipe3_disable_refclk(struct
>> ti_pipe3 *phy)
>> {
>> if (!IS_ERR(phy->refclk))
>> clk_disable_unprepare(phy->refclk);
>> -
>> - phy->refclk_enabled = false;
>> }
>>
>> static int ti_pipe3_enable_clocks(struct ti_pipe3 *phy)
>> {
>> int ret = 0;
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&phy->lock, flags);
>> - if (phy->enabled)
>> - goto err1;
>>
>> ret = ti_pipe3_enable_refclk(phy);
>
> we can enable refclk here itself instead of having a separate function it?
>> if (ret)
>> - goto err1;
>> + return ret;
>>
>> if (!IS_ERR(phy->wkupclk)) {
>> ret = clk_prepare_enable(phy->wkupclk);
>> if (ret) {
>> dev_err(phy->dev, "Failed to enable wkupclk %d\n", ret);
>> - goto err2;
>> + goto disable_refclk;
>> }
>> }
>>
>> @@ -477,96 +467,31 @@ static int ti_pipe3_enable_clocks(struct
>> ti_pipe3 *phy)
>> ret = clk_prepare_enable(phy->div_clk);
>> if (ret) {
>> dev_err(phy->dev, "Failed to enable div_clk %d\n", ret);
>> - goto err3;
>> + goto disable_wkupclk;
>> }
>> }
>>
>> - phy->enabled = true;
>> - spin_unlock_irqrestore(&phy->lock, flags);
>> return 0;
>>
>> -err3:
>> +disable_wkupclk:
>> if (!IS_ERR(phy->wkupclk))
>> clk_disable_unprepare(phy->wkupclk);
>>
>> -err2:
>> - if (!IS_ERR(phy->refclk))
>> - clk_disable_unprepare(phy->refclk);
>> -
>> +disable_refclk:
>> ti_pipe3_disable_refclk(phy);
>> -err1:
>> - spin_unlock_irqrestore(&phy->lock, flags);
>> +
>> return ret;
>> }
>>
>> static void ti_pipe3_disable_clocks(struct ti_pipe3 *phy)
>> {
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&phy->lock, flags);
>> - if (!phy->enabled) {
>> - spin_unlock_irqrestore(&phy->lock, flags);
>> - return;
>> - }
>> -
>> if (!IS_ERR(phy->wkupclk))
>> clk_disable_unprepare(phy->wkupclk);
>> - /* Don't disable refclk for SATA PHY due to Errata i783 */
>> - if (!of_device_is_compatible(phy->dev->of_node,
>> "ti,phy-pipe3-sata"))
>> - ti_pipe3_disable_refclk(phy);
>> + ti_pipe3_disable_refclk(phy);
>
> here too. we can get rid of ti_pipe3_disable_refclk and
> ti_pipe3_enable_refclk.
>
fine.
cheers,
-roger
next prev parent reply other threads:[~2015-05-20 13:48 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-12 16:07 [PATCH 0/3] phy: ti-pipe3: dra7: sata: allow suspend to RAM Roger Quadros
2015-05-12 16:07 ` Roger Quadros
2015-05-12 16:07 ` [PATCH 1/3] phy: ti-pipe3: fix suspend Roger Quadros
2015-05-12 16:07 ` Roger Quadros
2015-05-20 13:04 ` Kishon Vijay Abraham I
2015-05-20 13:04 ` Kishon Vijay Abraham I
2015-05-20 13:48 ` Roger Quadros [this message]
2015-05-20 13:48 ` Roger Quadros
2015-05-12 16:07 ` [PATCH 2/3] phy: ti-pipe3: i783 workaround for SATA lockup after dpll unlock/relock Roger Quadros
2015-05-12 16:07 ` Roger Quadros
2015-05-20 13:19 ` Kishon Vijay Abraham I
2015-05-20 13:19 ` Kishon Vijay Abraham I
2015-05-20 13:47 ` Roger Quadros
2015-05-20 13:47 ` Roger Quadros
2015-05-22 11:34 ` Kishon Vijay Abraham I
2015-05-22 11:34 ` Kishon Vijay Abraham I
2015-05-22 13:58 ` Roger Quadros
2015-05-22 13:58 ` Roger Quadros
2015-05-23 7:22 ` Kishon Vijay Abraham I
2015-05-23 7:22 ` Kishon Vijay Abraham I
2015-05-26 14:20 ` Roger Quadros
2015-05-26 14:20 ` Roger Quadros
2015-06-02 8:17 ` Roger Quadros
2015-06-02 8:17 ` Roger Quadros
2015-05-12 16:07 ` [PATCH 3/3] ARM: dts: dra7: Add syscon-pllreset syscon to SATA PHY Roger Quadros
2015-05-12 16:07 ` Roger Quadros
2015-07-14 10:34 ` Tony Lindgren
2015-07-14 10:39 ` Roger Quadros
2015-07-14 10:39 ` Roger Quadros
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=555C90AF.4020709@ti.com \
--to=rogerq@ti.com \
--cc=balbi@ti.com \
--cc=grygorii.strashko@ti.com \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=nm@ti.com \
--cc=nsekhar@ti.com \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.