devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
@ 2015-09-11  8:54 Shawn Lin
  2015-09-11  8:55 ` [PATCH 2/2] Documentation: bindings: add description of phy " Shawn Lin
  2015-10-16 12:35 ` [PATCH 1/2] mmc: sdhci-of-arasan: add phy support " Ulf Hansson
  0 siblings, 2 replies; 10+ messages in thread
From: Shawn Lin @ 2015-09-11  8:54 UTC (permalink / raw)
  To: Michal Simek, S?ren Brinkmann, Ulf Hansson
  Cc: linux-mmc, linux-kernel, linux-rockchip, devicetree, Shawn Lin

This patch adds Generic PHY access for sdhci-of-arasan. Driver
can get PHY handler from dt-binding, and power-on/init the PHY.
Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/mmc/host/sdhci-of-arasan.c | 90 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 621c3f4..fdd71c7 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -21,6 +21,7 @@
 
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/phy/phy.h>
 #include "sdhci-pltfm.h"
 
 #define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
@@ -35,6 +36,7 @@
  */
 struct sdhci_arasan_data {
 	struct clk	*clk_ahb;
+	struct phy	*phy;
 };
 
 static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
@@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
 
 #ifdef CONFIG_PM_SLEEP
 /**
+  * sdhci_arasan_suspend_phy - Suspend phy method for the driver
+  * @phy:        Handler of phy structure
+  * Returns 0 on success and error value on error
+  *
+  * Put the phy in a deactive state.
+  */
+static int sdhci_arasan_suspend_phy(struct phy *phy)
+{
+	int ret;
+
+	ret = phy_exit(phy);
+	if (ret < 0)
+		goto err_phy_exit;
+
+	ret = phy_power_off(phy);
+	if (ret < 0)
+		goto err_phy_pwr_off;
+
+	return 0;
+
+err_phy_pwr_off:
+	phy_power_on(phy);
+err_phy_exit:
+	phy_init(phy);
+	return ret;
+}
+
+/**
+  * sdhci_arasan_resume_phy - Resume phy method for the driver
+  * @phy:        Handler of phy structure
+  * Returns 0 on success and error value on error
+  *
+  * Put the phy in a active state.
+  */
+static int sdhci_arasan_resume_phy(struct phy *phy)
+{
+	int ret;
+
+	ret = phy_power_on(phy);
+	if (ret < 0)
+		goto err_phy_pwr_on;
+
+	ret = phy_init(phy);
+	if (ret < 0)
+		goto err_phy_init;
+
+	return 0;
+
+err_phy_init:
+	phy_exit(phy);
+err_phy_pwr_on:
+	phy_power_off(phy);
+	return ret;
+}
+
+/**
  * sdhci_arasan_suspend - Suspend method for the driver
  * @dev:	Address of the device structure
  * Returns 0 on success and error value on error
@@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev)
 	clk_disable(pltfm_host->clk);
 	clk_disable(sdhci_arasan->clk_ahb);
 
+	if (sdhci_arasan->phy) {
+		ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy);
+		if (ret < 0)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev)
 		return ret;
 	}
 
+	if (sdhci_arasan->phy) {
+		ret = sdhci_arasan_resume_phy(sdhci_arasan->phy);
+		if (ret < 0)
+			return ret;
+	}
+
 	return sdhci_resume_host(host);
 }
 #endif /* ! CONFIG_PM_SLEEP */
@@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		goto clk_dis_ahb;
 	}
 
+	sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan");
+	if (!IS_ERR(sdhci_arasan->phy)) {
+		ret = phy_power_on(sdhci_arasan->phy);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "phy_power_on err.\n");
+			phy_power_off(sdhci_arasan->phy);
+			goto clk_dis_ahb;
+		}
+
+		ret = phy_init(sdhci_arasan->phy);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "phy_init err.\n");
+			phy_exit(sdhci_arasan->phy);
+			phy_power_off(sdhci_arasan->phy);
+			goto clk_dis_ahb;
+		}
+	} else {
+		sdhci_arasan->phy = NULL;
+	}
+
 	host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0);
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
-- 
2.3.7



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] Documentation: bindings: add description of phy for sdhci-of-arasan
  2015-09-11  8:54 [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan Shawn Lin
@ 2015-09-11  8:55 ` Shawn Lin
       [not found]   ` <1441961729-10594-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
  2015-10-16 12:35 ` [PATCH 1/2] mmc: sdhci-of-arasan: add phy support " Ulf Hansson
  1 sibling, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2015-09-11  8:55 UTC (permalink / raw)
  To: Michal Simek, S?ren Brinkmann, Ulf Hansson
  Cc: linux-mmc, linux-kernel, linux-rockchip, devicetree, Shawn Lin

This patch adds phys and phy-names for sdhci-of-arasan as optional
properties, and details the example as well.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index da541c3..0264d5f 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -1,11 +1,12 @@
 Device Tree Bindings for the Arasan SDHCI Controller
 
-  The bindings follow the mmc[1], clock[2] and interrupt[3] bindings. Only
-  deviations are documented here.
+  The bindings follow the mmc[1], clock[2], interrupt[3] and phy[4] bindings.
+  Only deviations are documented here.
 
   [1] Documentation/devicetree/bindings/mmc/mmc.txt
   [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
   [3] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+  [4] Documentation/devicetree/bindings/phy/phy-bindings.txt
 
 Required Properties:
   - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
@@ -16,6 +17,9 @@ Required Properties:
   - interrupts: Interrupt specifier
   - interrupt-parent: Phandle for the interrupt controller that services
 		      interrupts for this device.
+Optional Properties:
+  - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
+  - phy-names:  MUST be "phy_arasan".
 
 Example:
 	sdhci@e0100000 {
@@ -25,4 +29,6 @@ Example:
 		clocks = <&clkc 21>, <&clkc 32>;
 		interrupt-parent = <&gic>;
 		interrupts = <0 24 4>;
+		phys = <&emmc_phy>;
+		phy-names = "phy_arasan";
 	} ;
-- 
2.3.7

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Documentation: bindings: add description of phy for sdhci-of-arasan
       [not found]   ` <1441961729-10594-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
@ 2015-09-14  2:47     ` Sören Brinkmann
  2015-09-14  3:02       ` Shawn Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Sören Brinkmann @ 2015-09-14  2:47 UTC (permalink / raw)
  To: Shawn Lin
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA, Michal Simek,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

Hi Shawn,

On Fri, 2015-09-11 at 04:55PM +0800, Shawn Lin wrote:
> This patch adds phys and phy-names for sdhci-of-arasan as optional
> properties, and details the example as well.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index da541c3..0264d5f 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -1,11 +1,12 @@
>  Device Tree Bindings for the Arasan SDHCI Controller
>  
> -  The bindings follow the mmc[1], clock[2] and interrupt[3] bindings. Only
> -  deviations are documented here.
> +  The bindings follow the mmc[1], clock[2], interrupt[3] and phy[4] bindings.
> +  Only deviations are documented here.
>  
>    [1] Documentation/devicetree/bindings/mmc/mmc.txt
>    [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
>    [3] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +  [4] Documentation/devicetree/bindings/phy/phy-bindings.txt
>  
>  Required Properties:
>    - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
> @@ -16,6 +17,9 @@ Required Properties:
>    - interrupts: Interrupt specifier
>    - interrupt-parent: Phandle for the interrupt controller that services
>  		      interrupts for this device.
> +Optional Properties:
> +  - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
> +  - phy-names:  MUST be "phy_arasan".

This might be a dumb question, but, is the external phy actually
optional? Or is it mandatory for certain implementations of the IP.
In the latter case, it should probably be a mandatory property depending
on the compatible string matching an implementation that requires an
external phy (accordingly, the implementation might have to treat it
that way too).

	Sören

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] Documentation: bindings: add description of phy for sdhci-of-arasan
  2015-09-14  2:47     ` Sören Brinkmann
@ 2015-09-14  3:02       ` Shawn Lin
  0 siblings, 0 replies; 10+ messages in thread
From: Shawn Lin @ 2015-09-14  3:02 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
	shawn.lin-TNX95d0MmH7DzftRWevZcw,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA, Michal Simek,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

在 2015/9/14 10:47, Sören Brinkmann 写道:
> Hi Shawn,
>
> On Fri, 2015-09-11 at 04:55PM +0800, Shawn Lin wrote:
>> This patch adds phys and phy-names for sdhci-of-arasan as optional
>> properties, and details the example as well.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index da541c3..0264d5f 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -1,11 +1,12 @@
>>   Device Tree Bindings for the Arasan SDHCI Controller
>>
>> -  The bindings follow the mmc[1], clock[2] and interrupt[3] bindings. Only
>> -  deviations are documented here.
>> +  The bindings follow the mmc[1], clock[2], interrupt[3] and phy[4] bindings.
>> +  Only deviations are documented here.
>>
>>     [1] Documentation/devicetree/bindings/mmc/mmc.txt
>>     [2] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>     [3] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> +  [4] Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>>   Required Properties:
>>     - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
>> @@ -16,6 +17,9 @@ Required Properties:
>>     - interrupts: Interrupt specifier
>>     - interrupt-parent: Phandle for the interrupt controller that services
>>   		      interrupts for this device.
>> +Optional Properties:
>> +  - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
>> +  - phy-names:  MUST be "phy_arasan".
>
> This might be a dumb question, but, is the external phy actually
> optional? Or is it mandatory for certain implementations of the IP.
> In the latter case, it should probably be a mandatory property depending
> on the compatible string matching an implementation that requires an
> external phy (accordingly, the implementation might have to treat it

 From my soc team's reponse, there is no way to bypass controller to IO 
if no phy supported. So it should be the latter case you mentioned for 
arasan.sdhci-5.1.

Thanks for point out it, really helpful.

I will fix it in v2.

> that way too).
>
> 	Sören
>
>
>


-- 
Best Regards
Shawn Lin


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
  2015-09-11  8:54 [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan Shawn Lin
  2015-09-11  8:55 ` [PATCH 2/2] Documentation: bindings: add description of phy " Shawn Lin
@ 2015-10-16 12:35 ` Ulf Hansson
       [not found]   ` <CAPDyKFpwgZSkE+TNehZ1Un=5CfjoEtfvELfrS6t-ZHZPscy3Yg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2015-10-16 12:35 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Michal Simek, S?ren Brinkmann, linux-mmc,
	linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...,
	devicetree@vger.kernel.org

On 11 September 2015 at 10:54, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> This patch adds Generic PHY access for sdhci-of-arasan. Driver
> can get PHY handler from dt-binding, and power-on/init the PHY.
> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
>  drivers/mmc/host/sdhci-of-arasan.c | 90 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 621c3f4..fdd71c7 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -21,6 +21,7 @@
>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/phy/phy.h>
>  #include "sdhci-pltfm.h"
>
>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET   0x2c
> @@ -35,6 +36,7 @@
>   */
>  struct sdhci_arasan_data {
>         struct clk      *clk_ahb;
> +       struct phy      *phy;
>  };
>
>  static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> @@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
>
>  #ifdef CONFIG_PM_SLEEP
>  /**
> +  * sdhci_arasan_suspend_phy - Suspend phy method for the driver
> +  * @phy:        Handler of phy structure
> +  * Returns 0 on success and error value on error
> +  *
> +  * Put the phy in a deactive state.
> +  */
> +static int sdhci_arasan_suspend_phy(struct phy *phy)
> +{
> +       int ret;
> +
> +       ret = phy_exit(phy);
> +       if (ret < 0)
> +               goto err_phy_exit;

This odd to me. First you do phy_exit() then phy_power_off(). It seems
like it should be in the opposite order.

Moreover I wonder why phy_exit() is needed, I expected that to be
called at ->remove() only!?

> +
> +       ret = phy_power_off(phy);
> +       if (ret < 0)
> +               goto err_phy_pwr_off;
> +
> +       return 0;
> +
> +err_phy_pwr_off:
> +       phy_power_on(phy);
> +err_phy_exit:
> +       phy_init(phy);
> +       return ret;
> +}
> +
> +/**
> +  * sdhci_arasan_resume_phy - Resume phy method for the driver
> +  * @phy:        Handler of phy structure
> +  * Returns 0 on success and error value on error
> +  *
> +  * Put the phy in a active state.
> +  */
> +static int sdhci_arasan_resume_phy(struct phy *phy)
> +{
> +       int ret;
> +
> +       ret = phy_power_on(phy);
> +       if (ret < 0)
> +               goto err_phy_pwr_on;
> +
> +       ret = phy_init(phy);
> +       if (ret < 0)
> +               goto err_phy_init;
> +

Similar comment as above.

> +       return 0;
> +
> +err_phy_init:
> +       phy_exit(phy);
> +err_phy_pwr_on:
> +       phy_power_off(phy);
> +       return ret;
> +}
> +
> +/**
>   * sdhci_arasan_suspend - Suspend method for the driver
>   * @dev:       Address of the device structure
>   * Returns 0 on success and error value on error
> @@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev)
>         clk_disable(pltfm_host->clk);
>         clk_disable(sdhci_arasan->clk_ahb);
>
> +       if (sdhci_arasan->phy) {
> +               ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy);
> +               if (ret < 0)
> +                       return ret;
> +       }

This means you will suspend the phy after you have disabled the
clocks. Of course I can't tell whether that okay, but it doesn't
follow the same sequence as in ->probe(). To me that indicates that
either probe or suspend/resume could be broken.

> +
>         return 0;
>  }
>
> @@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev)
>                 return ret;
>         }
>
> +       if (sdhci_arasan->phy) {
> +               ret = sdhci_arasan_resume_phy(sdhci_arasan->phy);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
>         return sdhci_resume_host(host);
>  }
>  #endif /* ! CONFIG_PM_SLEEP */
> @@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>                 goto clk_dis_ahb;
>         }
>
> +       sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan");
> +       if (!IS_ERR(sdhci_arasan->phy)) {

I understand the phy is optional, but you still need to handle the
EPROBE_DEFER case.

Perhaps you should also use devm_phy_optional_get() instead!?

> +               ret = phy_power_on(sdhci_arasan->phy);

This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?

Similar comment applies to phy_exit() and phy_power_off().

> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "phy_power_on err.\n");
> +                       phy_power_off(sdhci_arasan->phy);
> +                       goto clk_dis_ahb;
> +               }
> +
> +               ret = phy_init(sdhci_arasan->phy);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "phy_init err.\n");
> +                       phy_exit(sdhci_arasan->phy);
> +                       phy_power_off(sdhci_arasan->phy);
> +                       goto clk_dis_ahb;
> +               }
> +       } else {

This else isn't needed. When you are about to access the phy you can
check the cookie of it by "!IS_ERR(sdhci_arasan->phy)".

> +               sdhci_arasan->phy = NULL;
> +       }
> +
>         host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0);
>         if (IS_ERR(host)) {
>                 ret = PTR_ERR(host);
> --
> 2.3.7
>
>

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
       [not found]   ` <CAPDyKFpwgZSkE+TNehZ1Un=5CfjoEtfvELfrS6t-ZHZPscy3Yg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2015-10-19  0:51     ` Shawn Lin
       [not found]       ` <56243E85.2040202-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2015-10-19  0:51 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	shawn.lin-TNX95d0MmH7DzftRWevZcw, linux-mmc, Michal Simek,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC..., S?ren Brinkmann

On 2015/10/16 20:35, Ulf Hansson wrote:
> On 11 September 2015 at 10:54, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> This patch adds Generic PHY access for sdhci-of-arasan. Driver
>> can get PHY handler from dt-binding, and power-on/init the PHY.
>> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.
>>
>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>
>>   drivers/mmc/host/sdhci-of-arasan.c | 90 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index 621c3f4..fdd71c7 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -21,6 +21,7 @@
>>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>>   #include "sdhci-pltfm.h"
>>
>>   #define SDHCI_ARASAN_CLK_CTRL_OFFSET   0x2c
>> @@ -35,6 +36,7 @@
>>    */
>>   struct sdhci_arasan_data {
>>          struct clk      *clk_ahb;
>> +       struct phy      *phy;
>>   };
>>
>>   static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
>> @@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
>>
>>   #ifdef CONFIG_PM_SLEEP
>>   /**
>> +  * sdhci_arasan_suspend_phy - Suspend phy method for the driver
>> +  * @phy:        Handler of phy structure
>> +  * Returns 0 on success and error value on error
>> +  *
>> +  * Put the phy in a deactive state.
>> +  */
>> +static int sdhci_arasan_suspend_phy(struct phy *phy)
>> +{
>> +       int ret;
>> +
>> +       ret = phy_exit(phy);
>> +       if (ret < 0)
>> +               goto err_phy_exit;
>
> This odd to me. First you do phy_exit() then phy_power_off(). It seems
> like it should be in the opposite order.
>

yep, there is no need to use phy_exit/init for suspend/resume.
I will do it.

> Moreover I wonder why phy_exit() is needed, I expected that to be
> called at ->remove() only!?
>
>> +
>> +       ret = phy_power_off(phy);
>> +       if (ret < 0)
>> +               goto err_phy_pwr_off;
>> +
>> +       return 0;
>> +
>> +err_phy_pwr_off:
>> +       phy_power_on(phy);
>> +err_phy_exit:
>> +       phy_init(phy);
>> +       return ret;
>> +}
>> +
>> +/**
>> +  * sdhci_arasan_resume_phy - Resume phy method for the driver
>> +  * @phy:        Handler of phy structure
>> +  * Returns 0 on success and error value on error
>> +  *
>> +  * Put the phy in a active state.
>> +  */
>> +static int sdhci_arasan_resume_phy(struct phy *phy)
>> +{
>> +       int ret;
>> +
>> +       ret = phy_power_on(phy);
>> +       if (ret < 0)
>> +               goto err_phy_pwr_on;
>> +
>> +       ret = phy_init(phy);
>> +       if (ret < 0)
>> +               goto err_phy_init;
>> +
>
> Similar comment as above.
>
>> +       return 0;
>> +
>> +err_phy_init:
>> +       phy_exit(phy);
>> +err_phy_pwr_on:
>> +       phy_power_off(phy);
>> +       return ret;
>> +}
>> +
>> +/**
>>    * sdhci_arasan_suspend - Suspend method for the driver
>>    * @dev:       Address of the device structure
>>    * Returns 0 on success and error value on error
>> @@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev)
>>          clk_disable(pltfm_host->clk);
>>          clk_disable(sdhci_arasan->clk_ahb);
>>
>> +       if (sdhci_arasan->phy) {
>> +               ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy);
>> +               if (ret < 0)
>> +                       return ret;
>> +       }
>
> This means you will suspend the phy after you have disabled the
> clocks. Of course I can't tell whether that okay, but it doesn't
> follow the same sequence as in ->probe(). To me that indicates that
> either probe or suspend/resume could be broken.
>

phy has a seperate clk for itself, and it's controlled by phy driver. 
So, there is no any relationship between controller's clk and phy's clk. 
We disable or enable phy's clk internally in 
phy_int/exit/power_off/power_on.

Of course if it makes odd to you, I would put suspend_phy before 
clk_disable.  :)

>> +
>>          return 0;
>>   }
>>
>> @@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev)
>>                  return ret;
>>          }
>>
>> +       if (sdhci_arasan->phy) {
>> +               ret = sdhci_arasan_resume_phy(sdhci_arasan->phy);
>> +               if (ret < 0)
>> +                       return ret;
>> +       }
>> +
>>          return sdhci_resume_host(host);
>>   }
>>   #endif /* ! CONFIG_PM_SLEEP */
>> @@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>                  goto clk_dis_ahb;
>>          }
>>
>> +       sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan");
>> +       if (!IS_ERR(sdhci_arasan->phy)) {
>
> I understand the phy is optional, but you still need to handle the
> EPROBE_DEFER case.
>
> Perhaps you should also use devm_phy_optional_get() instead!?

I already changed it in version-2 [1]. :)
phy is mandatory for sdhci-arasan,5.1.

[1]: https://patchwork.kernel.org/patch/7173251/

>
>> +               ret = phy_power_on(sdhci_arasan->phy);
>
> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?
>
> Similar comment applies to phy_exit() and phy_power_off().

Both are okay. It depends how the phy-driver implement the four 
interfaces. From my case, power_on for arasan's phy driver firstly 
enable phy's clk and open power-domain, then programme phy internal 
registers to activate phy. Without enabling phy's clk and power-domain, 
we cannot do phy_init since phy can't be accessed by CPU.

But here, I think you are right. It does look a bit weird.
I think the better way is to remove phy_power_on here, and let 
phy-driver do power_on in phy_init internally. Similarly in the remove call.


How about?

>
>> +               if (ret < 0) {
>> +                       dev_err(&pdev->dev, "phy_power_on err.\n");
>> +                       phy_power_off(sdhci_arasan->phy);
>> +                       goto clk_dis_ahb;
>> +               }
>> +
>> +               ret = phy_init(sdhci_arasan->phy);
>> +               if (ret < 0) {
>> +                       dev_err(&pdev->dev, "phy_init err.\n");
>> +                       phy_exit(sdhci_arasan->phy);
>> +                       phy_power_off(sdhci_arasan->phy);
>> +                       goto clk_dis_ahb;
>> +               }
>> +       } else {
>
> This else isn't needed. When you are about to access the phy you can
> check the cookie of it by "!IS_ERR(sdhci_arasan->phy)".
>

similarly, I had removed it in v2.

>> +               sdhci_arasan->phy = NULL;
>> +       }
>> +
>>          host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0);
>>          if (IS_ERR(host)) {
>>                  ret = PTR_ERR(host);
>> --
>> 2.3.7
>>
>>
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
Best Regards
Shawn Lin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
       [not found]       ` <56243E85.2040202-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
@ 2015-10-19  7:50         ` Ulf Hansson
  2015-10-19  8:39           ` Shawn Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2015-10-19  7:50 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Michal Simek, S?ren Brinkmann, linux-mmc,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC...,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

[...]

>> I understand the phy is optional, but you still need to handle the
>> EPROBE_DEFER case.
>>
>> Perhaps you should also use devm_phy_optional_get() instead!?
>
>
> I already changed it in version-2 [1]. :)
> phy is mandatory for sdhci-arasan,5.1.
>
> [1]: https://patchwork.kernel.org/patch/7173251/

Oh, apologize for reviewing the old version!

>
>>
>>> +               ret = phy_power_on(sdhci_arasan->phy);
>>
>>
>> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?
>>
>> Similar comment applies to phy_exit() and phy_power_off().
>
>
> Both are okay. It depends how the phy-driver implement the four interfaces.
> From my case, power_on for arasan's phy driver firstly enable phy's clk and
> open power-domain, then programme phy internal registers to activate phy.
> Without enabling phy's clk and power-domain, we cannot do phy_init since phy
> can't be accessed by CPU.
>
> But here, I think you are right. It does look a bit weird.
> I think the better way is to remove phy_power_on here, and let phy-driver do
> power_on in phy_init internally. Similarly in the remove call.

That makes sense to me! I think it would also follow other phy clients
use of the phy API.

What makes me wonder though is from a power management point of view.
*When* do you need to have phy initialized and powered?

1) For a removable card can leave the phy uninitialised or powered
off, but still detect card insertion/removal via GPIO? Is that a valid
scenario for you?

2) Considering the runtime PM case for the sdhci device. Typically you
can gate clocks etc at runtime suspend to save power, but what about
the phy? Can you power off it in runtime suspend?

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
  2015-10-19  7:50         ` Ulf Hansson
@ 2015-10-19  8:39           ` Shawn Lin
       [not found]             ` <5624AC51.9060907-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Shawn Lin @ 2015-10-19  8:39 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: shawn.lin, Michal Simek, S?ren Brinkmann, linux-mmc,
	linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...,
	devicetree@vger.kernel.org

On 2015/10/19 15:50, Ulf Hansson wrote:
> [...]
>
>>> I understand the phy is optional, but you still need to handle the
>>> EPROBE_DEFER case.
>>>
>>> Perhaps you should also use devm_phy_optional_get() instead!?
>>
>>
>> I already changed it in version-2 [1]. :)
>> phy is mandatory for sdhci-arasan,5.1.
>>
>> [1]: https://patchwork.kernel.org/patch/7173251/
>
> Oh, apologize for reviewing the old version!
>
>>
>>>
>>>> +               ret = phy_power_on(sdhci_arasan->phy);
>>>
>>>
>>> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?
>>>
>>> Similar comment applies to phy_exit() and phy_power_off().
>>
>>
>> Both are okay. It depends how the phy-driver implement the four interfaces.
>>  From my case, power_on for arasan's phy driver firstly enable phy's clk and
>> open power-domain, then programme phy internal registers to activate phy.
>> Without enabling phy's clk and power-domain, we cannot do phy_init since phy
>> can't be accessed by CPU.
>>
>> But here, I think you are right. It does look a bit weird.
>> I think the better way is to remove phy_power_on here, and let phy-driver do
>> power_on in phy_init internally. Similarly in the remove call.
>
> That makes sense to me! I think it would also follow other phy clients
> use of the phy API.
>
> What makes me wonder though is from a power management point of view.
> *When* do you need to have phy initialized and powered?
>

Whenever controller need to communicate with card, must init/power_on 
phy firstly.

> 1) For a removable card can leave the phy uninitialised or powered
> off, but still detect card insertion/removal via GPIO? Is that a valid
> scenario for you?
>

Theoretically, it is.  Although my soc don't use phy for removable card, 
I also consider how to handle this case. Should we add a hook
for sdhci_get_cd, and initialize phy if it's non-removeable device or 
removeable card in slot ? Doesn't seem like a good idea.

Also seems not always work if we just use SDHCI_PRESENT_STATE to detect 
card insertion/removal without phy in active state.

> 2) Considering the runtime PM case for the sdhci device. Typically you
> can gate clocks etc at runtime suspend to save power, but what about
> the phy? Can you power off it in runtime suspend?

yes, we can power off it in runtime suspend. So we can append some 
patches later to introduce runtime pm for sdhci-of-arasan?

>
> [...]
>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
       [not found]             ` <5624AC51.9060907-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
@ 2015-10-19 10:54               ` Ulf Hansson
  2015-10-19 14:56                 ` Shawn Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2015-10-19 10:54 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Michal Simek, S?ren Brinkmann, linux-mmc,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC...,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

[...]

>>>
>>>>
>>>>> +               ret = phy_power_on(sdhci_arasan->phy);
>>>>
>>>>
>>>>
>>>> This looks a bit weird. Shouldn't you do phy_init() prior
>>>> phy_power_on()?
>>>>
>>>> Similar comment applies to phy_exit() and phy_power_off().
>>>
>>>
>>>
>>> Both are okay. It depends how the phy-driver implement the four
>>> interfaces.
>>>  From my case, power_on for arasan's phy driver firstly enable phy's clk
>>> and
>>> open power-domain, then programme phy internal registers to activate phy.
>>> Without enabling phy's clk and power-domain, we cannot do phy_init since
>>> phy
>>> can't be accessed by CPU.
>>>
>>> But here, I think you are right. It does look a bit weird.
>>> I think the better way is to remove phy_power_on here, and let phy-driver
>>> do
>>> power_on in phy_init internally. Similarly in the remove call.
>>
>>
>> That makes sense to me! I think it would also follow other phy clients
>> use of the phy API.
>>
>> What makes me wonder though is from a power management point of view.
>> *When* do you need to have phy initialized and powered?
>>
>
> Whenever controller need to communicate with card, must init/power_on phy
> firstly.
>
>> 1) For a removable card can leave the phy uninitialised or powered
>> off, but still detect card insertion/removal via GPIO? Is that a valid
>> scenario for you?
>>
>
> Theoretically, it is.  Although my soc don't use phy for removable card, I
> also consider how to handle this case. Should we add a hook
> for sdhci_get_cd, and initialize phy if it's non-removeable device or
> removeable card in slot ? Doesn't seem like a good idea.
>
> Also seems not always work if we just use SDHCI_PRESENT_STATE to detect card
> insertion/removal without phy in active state.

As you SoC don't use removable card, let's just leave this for now.
Thanks for sharing your thoughts.

>
>> 2) Considering the runtime PM case for the sdhci device. Typically you
>> can gate clocks etc at runtime suspend to save power, but what about
>> the phy? Can you power off it in runtime suspend?
>
>
> yes, we can power off it in runtime suspend. So we can append some patches
> later to introduce runtime pm for sdhci-of-arasan?

Yes, that's fine. I would thus expect that you want to do phy power
off/on from the runtime PM callbacks, right!?

If that's the case I think $subject patch should deal with *both* phy
init and phy power on during ->probe().

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
  2015-10-19 10:54               ` Ulf Hansson
@ 2015-10-19 14:56                 ` Shawn Lin
  0 siblings, 0 replies; 10+ messages in thread
From: Shawn Lin @ 2015-10-19 14:56 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: shawn.lin, Michal Simek, S?ren Brinkmann, linux-mmc,
	linux-kernel@vger.kernel.org, open list:ARM/Rockchip SoC...,
	devicetree@vger.kernel.org

On 2015/10/19 18:54, Ulf Hansson wrote:
> [...]
>

[...]

>
>>
>>> 2) Considering the runtime PM case for the sdhci device. Typically you
>>> can gate clocks etc at runtime suspend to save power, but what about
>>> the phy? Can you power off it in runtime suspend?
>>
>>
>> yes, we can power off it in runtime suspend. So we can append some patches
>> later to introduce runtime pm for sdhci-of-arasan?
>
> Yes, that's fine. I would thus expect that you want to do phy power
> off/on from the runtime PM callbacks, right!?
>
> If that's the case I think $subject patch should deal with *both* phy
> init and phy power on during ->probe().
>

yes. I will do phy power_off/on from runtime pm for the next version of 
$subject patchset.

Thanks.

> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
Best Regards
Shawn Lin


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-10-19 14:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-11  8:54 [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan Shawn Lin
2015-09-11  8:55 ` [PATCH 2/2] Documentation: bindings: add description of phy " Shawn Lin
     [not found]   ` <1441961729-10594-1-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2015-09-14  2:47     ` Sören Brinkmann
2015-09-14  3:02       ` Shawn Lin
2015-10-16 12:35 ` [PATCH 1/2] mmc: sdhci-of-arasan: add phy support " Ulf Hansson
     [not found]   ` <CAPDyKFpwgZSkE+TNehZ1Un=5CfjoEtfvELfrS6t-ZHZPscy3Yg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-10-19  0:51     ` Shawn Lin
     [not found]       ` <56243E85.2040202-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2015-10-19  7:50         ` Ulf Hansson
2015-10-19  8:39           ` Shawn Lin
     [not found]             ` <5624AC51.9060907-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2015-10-19 10:54               ` Ulf Hansson
2015-10-19 14:56                 ` Shawn Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).