[RESEND PATCH v5 0/2] Add phy support for arasan,sdhci-5.1

[RESEND PATCH v5 0/2] Add phy support for arasan,sdhci-5.1

[Shawn Lin]

Hello Ulf and Adrian,

Sorry for delay the new version for this topic. Actually I have no
platform available to test phy stuff since v4. Now, I can rework it.:)
I remove rpm support from v5 because this patchset just need to focus
on phy stuff, which will make the changes more precise. Another reason
is that phy_power_on take too much time from rpm, however the power profit
gained from phy_power_off is little due to the low power mechanism with the
phy itself observed from experiments.

v4 can be found here:
https://patchwork.kernel.org/patch/7463251/

Note:
Phy is mandatory requirement for arasan,sdhci-5.1, so we introduce
generic phy support for sdhci-of-arasan.


Shawn Lin (2):
  Documentation: bindings: add description of phy for sdhci-of-arasan
  mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

 .../devicetree/bindings/mmc/arasan,sdhci.txt       | 20 ++++++-
 drivers/mmc/host/sdhci-of-arasan.c                 | 66 ++++++++++++++++++++--
 2 files changed, 80 insertions(+), 6 deletions(-)

-- 
2.3.7


--
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

[RESEND PATCH v5 1/2] Documentation: bindings: add description of phy for sdhci-of-arasan

[Shawn Lin]

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

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

---

Changes in v2:
- Keep phy as a mandatory requirement for arasan,sdhci-5.1

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

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index da541c3..31b35c3 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
@@ -17,6 +18,10 @@ Required Properties:
   - interrupt-parent: Phandle for the interrupt controller that services
 		      interrupts for this device.
 
+Required Properties for "arasan,sdhci-5.1":
+  - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
+  - phy-names:  MUST be "phy_arasan".
+
 Example:
 	sdhci@e0100000 {
 		compatible = "arasan,sdhci-8.9a";
@@ -26,3 +31,14 @@ Example:
 		interrupt-parent = <&gic>;
 		interrupts = <0 24 4>;
 	} ;
+
+	sdhci@e2800000 {
+		compatible = "arasan,sdhci-5.1";
+		reg = <0xe2800000 0x1000>;
+		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&cru 8>, <&cru 18>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 24 4>;
+		phys = <&emmc_phy>;
+		phy-names = "phy_arasan";
+	} ;
-- 
2.3.7

Re: [RESEND PATCH v5 1/2] Documentation: bindings: add description of phy for sdhci-of-arasan

[Rob Herring]

On Fri, Feb 19, 2016 at 10:19:47AM +0800, Shawn Lin wrote:
> This patch adds phys and phy-names for sdhci-of-arasan as required
> properties for arasan,sdhci-5.1, and details the example as well.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> 
> ---
> 
> Changes in v2:
> - Keep phy as a mandatory requirement for arasan,sdhci-5.1
> 
>  .../devicetree/bindings/mmc/arasan,sdhci.txt         | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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

[RESEND PATCH v5 2/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[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.
Currently, it's just mandatory for arasan,sdhci-5.1.

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

Serise-changes: 5
- remove rpm support from this serise
- fix some err case for handling phy

Serise-changes: 4
- remove suspend/resume_phy
- add err handle label for system PM callback
- fix comments of struct sdhci_arasan_data and system PM
  callback suggested by Michal

Serise-changes: 3
- remove phy_init/exit for suspend/resume
- adjust phy_int/power_on seq to make code more reasonable
- simplify suspend/resume_phy

Serise-changes: 2
- Keep phy as a mandatory requirement for arasan,sdhci-5.1

---

Changes in v2: None

 drivers/mmc/host/sdhci-of-arasan.c | 66 +++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 5d9fdb3..cc40b1d 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
@@ -32,9 +33,11 @@
 /**
  * struct sdhci_arasan_data
  * @clk_ahb:	Pointer to the AHB clock
+ * @phy: Pointer to the generic phy
  */
 struct sdhci_arasan_data {
 	struct clk	*clk_ahb;
+	struct phy	*phy;
 };
 
 static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
@@ -72,7 +75,7 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
 /**
  * sdhci_arasan_suspend - Suspend method for the driver
  * @dev:	Address of the device structure
- * Returns 0 on success and error value on error
+ * Return: 0 on success and error value on error
  *
  * Put the device in a low power state.
  */
@@ -88,6 +91,14 @@ static int sdhci_arasan_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (!IS_ERR(sdhci_arasan->phy)) {
+		ret = phy_power_off(sdhci_arasan->phy);
+		if (ret) {
+			dev_err(dev, "Cannot power off phy.\n");
+			return ret;
+		}
+	}
+
 	clk_disable(pltfm_host->clk);
 	clk_disable(sdhci_arasan->clk_ahb);
 
@@ -97,7 +108,7 @@ static int sdhci_arasan_suspend(struct device *dev)
 /**
  * sdhci_arasan_resume - Resume method for the driver
  * @dev:	Address of the device structure
- * Returns 0 on success and error value on error
+ * Return: 0 on success and error value on error
  *
  * Resume operation after suspend
  */
@@ -118,11 +129,24 @@ static int sdhci_arasan_resume(struct device *dev)
 	ret = clk_enable(pltfm_host->clk);
 	if (ret) {
 		dev_err(dev, "Cannot enable SD clock.\n");
-		clk_disable(sdhci_arasan->clk_ahb);
-		return ret;
+		goto err_clk_en;
+	}
+
+	if (!IS_ERR(sdhci_arasan->phy)) {
+		ret = phy_power_on(sdhci_arasan->phy);
+		if (ret) {
+			dev_err(dev, "Cannot power on phy.\n");
+			goto err_phy_power;
+		}
 	}
 
 	return sdhci_resume_host(host);
+
+err_phy_power:
+	clk_disable(pltfm_host->clk);
+err_clk_en:
+	clk_disable(sdhci_arasan->clk_ahb);
+	return ret;
 }
 #endif /* ! CONFIG_PM_SLEEP */
 
@@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		goto clk_disable_all;
 	}
 
+	sdhci_arasan->phy = ERR_PTR(-ENODEV);
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "arasan,sdhci-5.1")) {
+		sdhci_arasan->phy = devm_phy_get(&pdev->dev,
+						 "phy_arasan");
+		if (IS_ERR(sdhci_arasan->phy)) {
+			ret = PTR_ERR(sdhci_arasan->phy);
+			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
+			goto clk_disable_all;
+		}
+
+		ret = phy_init(sdhci_arasan->phy);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "phy_init err.\n");
+			goto clk_disable_all;
+		}
+
+		ret = phy_power_on(sdhci_arasan->phy);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "phy_power_on err.\n");
+			goto err_phy_power;
+		}
+	}
+
 	ret = sdhci_add_host(host);
 	if (ret)
 		goto err_pltfm_free;
@@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	return 0;
 
 err_pltfm_free:
+	if (!IS_ERR(sdhci_arasan->phy))
+		phy_power_off(sdhci_arasan->phy);
 	sdhci_pltfm_free(pdev);
+err_phy_power:
+	if (!IS_ERR(sdhci_arasan->phy))
+		phy_exit(sdhci_arasan->phy);
 clk_disable_all:
 	clk_disable_unprepare(clk_xin);
 clk_dis_ahb:
@@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;
 
+	if (!IS_ERR(sdhci_arasan->phy)) {
+		phy_power_off(sdhci_arasan->phy);
+		phy_exit(sdhci_arasan->phy);
+	}
+
 	clk_disable_unprepare(sdhci_arasan->clk_ahb);
 
 	return sdhci_pltfm_unregister(pdev);
-- 
2.3.7

Re: [RESEND PATCH v5 2/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Adrian Hunter]

On 19/02/16 04:20, Shawn Lin 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.
> Currently, it's just mandatory for arasan,sdhci-5.1.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

Sorry for the slow reply.  Was on vacation and am still catching up.

First, the patch does not apply cleanly so you will have to re-base I am afraid.

> 
> Serise-changes: 5
> - remove rpm support from this serise
> - fix some err case for handling phy
> 
> Serise-changes: 4
> - remove suspend/resume_phy
> - add err handle label for system PM callback
> - fix comments of struct sdhci_arasan_data and system PM
>   callback suggested by Michal
> 
> Serise-changes: 3
> - remove phy_init/exit for suspend/resume
> - adjust phy_int/power_on seq to make code more reasonable
> - simplify suspend/resume_phy
> 
> Serise-changes: 2
> - Keep phy as a mandatory requirement for arasan,sdhci-5.1
> 
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 66 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 5d9fdb3..cc40b1d 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
> @@ -32,9 +33,11 @@
>  /**
>   * struct sdhci_arasan_data
>   * @clk_ahb:	Pointer to the AHB clock
> + * @phy: Pointer to the generic phy
>   */
>  struct sdhci_arasan_data {
>  	struct clk	*clk_ahb;
> +	struct phy	*phy;
>  };
>  
>  static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> @@ -72,7 +75,7 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
>  /**
>   * sdhci_arasan_suspend - Suspend method for the driver
>   * @dev:	Address of the device structure
> - * Returns 0 on success and error value on error
> + * Return: 0 on success and error value on error

Really should be a separate patch for the comment fixes.

>   *
>   * Put the device in a low power state.
>   */
> @@ -88,6 +91,14 @@ static int sdhci_arasan_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	if (!IS_ERR(sdhci_arasan->phy)) {
> +		ret = phy_power_off(sdhci_arasan->phy);
> +		if (ret) {
> +			dev_err(dev, "Cannot power off phy.\n");

I would expect you ought to call sdhci_resume_host() here.

> +			return ret;
> +		}
> +	}
> +
>  	clk_disable(pltfm_host->clk);
>  	clk_disable(sdhci_arasan->clk_ahb);
>  
> @@ -97,7 +108,7 @@ static int sdhci_arasan_suspend(struct device *dev)
>  /**
>   * sdhci_arasan_resume - Resume method for the driver
>   * @dev:	Address of the device structure
> - * Returns 0 on success and error value on error
> + * Return: 0 on success and error value on error
>   *
>   * Resume operation after suspend
>   */
> @@ -118,11 +129,24 @@ static int sdhci_arasan_resume(struct device *dev)
>  	ret = clk_enable(pltfm_host->clk);
>  	if (ret) {
>  		dev_err(dev, "Cannot enable SD clock.\n");
> -		clk_disable(sdhci_arasan->clk_ahb);
> -		return ret;
> +		goto err_clk_en;
> +	}
> +
> +	if (!IS_ERR(sdhci_arasan->phy)) {
> +		ret = phy_power_on(sdhci_arasan->phy);
> +		if (ret) {
> +			dev_err(dev, "Cannot power on phy.\n");
> +			goto err_phy_power;
> +		}
>  	}
>  
>  	return sdhci_resume_host(host);
> +
> +err_phy_power:
> +	clk_disable(pltfm_host->clk);
> +err_clk_en:
> +	clk_disable(sdhci_arasan->clk_ahb);

I don't know that disabling clocks is necessarily the right thing to do if
the resume fails.  You might want to consider what happens if the system
tries to use the device when it is in that state.  It's been a long time
since I did any ARM work but aren't you risking bus errors.  Might just as
well not bother and save a few lines of code.

> +	return ret;
>  }
>  #endif /* ! CONFIG_PM_SLEEP */
>  
> @@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  		goto clk_disable_all;
>  	}
>  
> +	sdhci_arasan->phy = ERR_PTR(-ENODEV);
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "arasan,sdhci-5.1")) {
> +		sdhci_arasan->phy = devm_phy_get(&pdev->dev,
> +						 "phy_arasan");
> +		if (IS_ERR(sdhci_arasan->phy)) {
> +			ret = PTR_ERR(sdhci_arasan->phy);
> +			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
> +			goto clk_disable_all;
> +		}
> +
> +		ret = phy_init(sdhci_arasan->phy);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "phy_init err.\n");
> +			goto clk_disable_all;
> +		}
> +
> +		ret = phy_power_on(sdhci_arasan->phy);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "phy_power_on err.\n");
> +			goto err_phy_power;
> +		}
> +	}
> +
>  	ret = sdhci_add_host(host);
>  	if (ret)
>  		goto err_pltfm_free;
> @@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_pltfm_free:
> +	if (!IS_ERR(sdhci_arasan->phy))
> +		phy_power_off(sdhci_arasan->phy);
>  	sdhci_pltfm_free(pdev);
> +err_phy_power:
> +	if (!IS_ERR(sdhci_arasan->phy))

Looks like you are using sdhci_arasan after it has been free'd by
sdhci_pltfm_free().

Also there seem to be cases where sdhci_arasan_probe() can exit after
sdhci_pltfm_init() without calling sdhci_pltfm_free().

> +		phy_exit(sdhci_arasan->phy);
>  clk_disable_all:
>  	clk_disable_unprepare(clk_xin);
>  clk_dis_ahb:
> @@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;
>  
> +	if (!IS_ERR(sdhci_arasan->phy)) {
> +		phy_power_off(sdhci_arasan->phy);
> +		phy_exit(sdhci_arasan->phy);
> +	}
> +

When you re-base, take care to keep this chunk above sdhci_pltfm_unregister().

>  	clk_disable_unprepare(sdhci_arasan->clk_ahb);
>  
>  	return sdhci_pltfm_unregister(pdev);
> 

Re: [RESEND PATCH v5 2/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Shawn Lin]

Hi Adrian,

On 2016/3/4 17:14, Adrian Hunter wrote:
> On 19/02/16 04:20, Shawn Lin wrote:
>> This patch adds Generic PHY access for sdhci-of-arasan. Driver

[...]

>>   /**
>>    * sdhci_arasan_suspend - Suspend method for the driver
>>    * @dev:	Address of the device structure
>> - * Returns 0 on success and error value on error
>> + * Return: 0 on success and error value on error
>
> Really should be a separate patch for the comment fixes.

yep, I won't touch it when I rebase my patch for this topic.

>
>>    *
>>    * Put the device in a low power state.
>>    */
>> @@ -88,6 +91,14 @@ static int sdhci_arasan_suspend(struct device *dev)
>>   	if (ret)
>>   		return ret;
>>
>> +	if (!IS_ERR(sdhci_arasan->phy)) {
>> +		ret = phy_power_off(sdhci_arasan->phy);
>> +		if (ret) {
>> +			dev_err(dev, "Cannot power off phy.\n");
>
> I would expect you ought to call sdhci_resume_host() here.

oh, got it.

>
>> +			return ret;
>> +		}
>> +	}
>> +
>>   	clk_disable(pltfm_host->clk);
>>   	clk_disable(sdhci_arasan->clk_ahb);
>>
>> @@ -97,7 +108,7 @@ static int sdhci_arasan_suspend(struct device *dev)
>>   /**
>>    * sdhci_arasan_resume - Resume method for the driver
>>    * @dev:	Address of the device structure
>> - * Returns 0 on success and error value on error
>> + * Return: 0 on success and error value on error
>>    *
>>    * Resume operation after suspend
>>    */
>> @@ -118,11 +129,24 @@ static int sdhci_arasan_resume(struct device *dev)
>>   	ret = clk_enable(pltfm_host->clk);
>>   	if (ret) {
>>   		dev_err(dev, "Cannot enable SD clock.\n");
>> -		clk_disable(sdhci_arasan->clk_ahb);
>> -		return ret;
>> +		goto err_clk_en;
>> +	}
>> +
>> +	if (!IS_ERR(sdhci_arasan->phy)) {
>> +		ret = phy_power_on(sdhci_arasan->phy);
>> +		if (ret) {
>> +			dev_err(dev, "Cannot power on phy.\n");
>> +			goto err_phy_power;
>> +		}
>>   	}
>>
>>   	return sdhci_resume_host(host);
>> +
>> +err_phy_power:
>> +	clk_disable(pltfm_host->clk);
>> +err_clk_en:
>> +	clk_disable(sdhci_arasan->clk_ahb);
>
> I don't know that disabling clocks is necessarily the right thing to do if
> the resume fails.  You might want to consider what happens if the system
> tries to use the device when it is in that state.  It's been a long time
> since I did any ARM work but aren't you risking bus errors.  Might just as
> well not bother and save a few lines of code.
>

Firstly, I really just follow the orginal err handle.
It's risking bus errors if disabling ahb clk, but ahb clk is most very
likely to shared by many controllers. So regarding to the clk reference
count, the ahb_clk is not *really* disabled and just decrease the
reference count. How about keep these handles here?


>> +	return ret;
>>   }
>>   #endif /* ! CONFIG_PM_SLEEP */
>>
>> @@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>   		goto clk_disable_all;
>>   	}
>>
>> +	sdhci_arasan->phy = ERR_PTR(-ENODEV);
>> +	if (of_device_is_compatible(pdev->dev.of_node,
>> +				    "arasan,sdhci-5.1")) {
>> +		sdhci_arasan->phy = devm_phy_get(&pdev->dev,
>> +						 "phy_arasan");
>> +		if (IS_ERR(sdhci_arasan->phy)) {
>> +			ret = PTR_ERR(sdhci_arasan->phy);
>> +			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
>> +			goto clk_disable_all;
>> +		}
>> +
>> +		ret = phy_init(sdhci_arasan->phy);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "phy_init err.\n");
>> +			goto clk_disable_all;
>> +		}
>> +
>> +		ret = phy_power_on(sdhci_arasan->phy);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "phy_power_on err.\n");
>> +			goto err_phy_power;
>> +		}
>> +	}
>> +
>>   	ret = sdhci_add_host(host);
>>   	if (ret)
>>   		goto err_pltfm_free;
>> @@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>   	return 0;
>>
>>   err_pltfm_free:
>> +	if (!IS_ERR(sdhci_arasan->phy))
>> +		phy_power_off(sdhci_arasan->phy);
>>   	sdhci_pltfm_free(pdev);
>> +err_phy_power:
>> +	if (!IS_ERR(sdhci_arasan->phy))
>
> Looks like you are using sdhci_arasan after it has been free'd by
> sdhci_pltfm_free().
>
> Also there seem to be cases where sdhci_arasan_probe() can exit after
> sdhci_pltfm_init() without calling sdhci_pltfm_free().

Good catch, will fix it.

>
>> +		phy_exit(sdhci_arasan->phy);
>>   clk_disable_all:
>>   	clk_disable_unprepare(clk_xin);
>>   clk_dis_ahb:
>> @@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>   	struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;
>>
>> +	if (!IS_ERR(sdhci_arasan->phy)) {
>> +		phy_power_off(sdhci_arasan->phy);
>> +		phy_exit(sdhci_arasan->phy);
>> +	}
>> +
>
> When you re-base, take care to keep this chunk above sdhci_pltfm_unregister().

sure.

Thanks for reviewing it.

>
>>   	clk_disable_unprepare(sdhci_arasan->clk_ahb);
>>
>>   	return sdhci_pltfm_unregister(pdev);
>>
>
>
>
>


-- 
Best Regards
Shawn Lin

--
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

Re: [RESEND PATCH v5 2/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Adrian Hunter]

On 04/03/16 12:24, Shawn Lin wrote:
> Hi Adrian,
> 
> On 2016/3/4 17:14, Adrian Hunter wrote:
>> On 19/02/16 04:20, Shawn Lin wrote:
>>> This patch adds Generic PHY access for sdhci-of-arasan. Driver
> 
> [...]
> 
>>>   /**
>>>    * sdhci_arasan_suspend - Suspend method for the driver
>>>    * @dev:    Address of the device structure
>>> - * Returns 0 on success and error value on error
>>> + * Return: 0 on success and error value on error
>>
>> Really should be a separate patch for the comment fixes.
> 
> yep, I won't touch it when I rebase my patch for this topic.
> 
>>
>>>    *
>>>    * Put the device in a low power state.
>>>    */
>>> @@ -88,6 +91,14 @@ static int sdhci_arasan_suspend(struct device *dev)
>>>       if (ret)
>>>           return ret;
>>>
>>> +    if (!IS_ERR(sdhci_arasan->phy)) {
>>> +        ret = phy_power_off(sdhci_arasan->phy);
>>> +        if (ret) {
>>> +            dev_err(dev, "Cannot power off phy.\n");
>>
>> I would expect you ought to call sdhci_resume_host() here.
> 
> oh, got it.
> 
>>
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>>       clk_disable(pltfm_host->clk);
>>>       clk_disable(sdhci_arasan->clk_ahb);
>>>
>>> @@ -97,7 +108,7 @@ static int sdhci_arasan_suspend(struct device *dev)
>>>   /**
>>>    * sdhci_arasan_resume - Resume method for the driver
>>>    * @dev:    Address of the device structure
>>> - * Returns 0 on success and error value on error
>>> + * Return: 0 on success and error value on error
>>>    *
>>>    * Resume operation after suspend
>>>    */
>>> @@ -118,11 +129,24 @@ static int sdhci_arasan_resume(struct device *dev)
>>>       ret = clk_enable(pltfm_host->clk);
>>>       if (ret) {
>>>           dev_err(dev, "Cannot enable SD clock.\n");
>>> -        clk_disable(sdhci_arasan->clk_ahb);
>>> -        return ret;
>>> +        goto err_clk_en;
>>> +    }
>>> +
>>> +    if (!IS_ERR(sdhci_arasan->phy)) {
>>> +        ret = phy_power_on(sdhci_arasan->phy);
>>> +        if (ret) {
>>> +            dev_err(dev, "Cannot power on phy.\n");
>>> +            goto err_phy_power;
>>> +        }
>>>       }
>>>
>>>       return sdhci_resume_host(host);
>>> +
>>> +err_phy_power:
>>> +    clk_disable(pltfm_host->clk);
>>> +err_clk_en:
>>> +    clk_disable(sdhci_arasan->clk_ahb);
>>
>> I don't know that disabling clocks is necessarily the right thing to do if
>> the resume fails.  You might want to consider what happens if the system
>> tries to use the device when it is in that state.  It's been a long time
>> since I did any ARM work but aren't you risking bus errors.  Might just as
>> well not bother and save a few lines of code.
>>
> 
> Firstly, I really just follow the orginal err handle.
> It's risking bus errors if disabling ahb clk, but ahb clk is most very
> likely to shared by many controllers. So regarding to the clk reference
> count, the ahb_clk is not *really* disabled and just decrease the
> reference count. How about keep these handles here?

I would remove the error handling on the grounds that it does not do
anything useful.

> 
> 
>>> +    return ret;
>>>   }
>>>   #endif /* ! CONFIG_PM_SLEEP */
>>>
>>> @@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device
>>> *pdev)
>>>           goto clk_disable_all;
>>>       }
>>>
>>> +    sdhci_arasan->phy = ERR_PTR(-ENODEV);
>>> +    if (of_device_is_compatible(pdev->dev.of_node,
>>> +                    "arasan,sdhci-5.1")) {
>>> +        sdhci_arasan->phy = devm_phy_get(&pdev->dev,
>>> +                         "phy_arasan");
>>> +        if (IS_ERR(sdhci_arasan->phy)) {
>>> +            ret = PTR_ERR(sdhci_arasan->phy);
>>> +            dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
>>> +            goto clk_disable_all;
>>> +        }
>>> +
>>> +        ret = phy_init(sdhci_arasan->phy);
>>> +        if (ret < 0) {
>>> +            dev_err(&pdev->dev, "phy_init err.\n");
>>> +            goto clk_disable_all;
>>> +        }
>>> +
>>> +        ret = phy_power_on(sdhci_arasan->phy);
>>> +        if (ret < 0) {
>>> +            dev_err(&pdev->dev, "phy_power_on err.\n");
>>> +            goto err_phy_power;
>>> +        }
>>> +    }
>>> +
>>>       ret = sdhci_add_host(host);
>>>       if (ret)
>>>           goto err_pltfm_free;
>>> @@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device
>>> *pdev)
>>>       return 0;
>>>
>>>   err_pltfm_free:
>>> +    if (!IS_ERR(sdhci_arasan->phy))
>>> +        phy_power_off(sdhci_arasan->phy);
>>>       sdhci_pltfm_free(pdev);
>>> +err_phy_power:
>>> +    if (!IS_ERR(sdhci_arasan->phy))
>>
>> Looks like you are using sdhci_arasan after it has been free'd by
>> sdhci_pltfm_free().
>>
>> Also there seem to be cases where sdhci_arasan_probe() can exit after
>> sdhci_pltfm_init() without calling sdhci_pltfm_free().
> 
> Good catch, will fix it.
> 
>>
>>> +        phy_exit(sdhci_arasan->phy);
>>>   clk_disable_all:
>>>       clk_disable_unprepare(clk_xin);
>>>   clk_dis_ahb:
>>> @@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct
>>> platform_device *pdev)
>>>       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>       struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;
>>>
>>> +    if (!IS_ERR(sdhci_arasan->phy)) {
>>> +        phy_power_off(sdhci_arasan->phy);
>>> +        phy_exit(sdhci_arasan->phy);
>>> +    }
>>> +
>>
>> When you re-base, take care to keep this chunk above
>> sdhci_pltfm_unregister().
> 
> sure.
> 
> Thanks for reviewing it.
> 
>>
>>>       clk_disable_unprepare(sdhci_arasan->clk_ahb);
>>>
>>>       return sdhci_pltfm_unregister(pdev);
>>>
>>
>>
>>
>>
> 
> 

Re: [RESEND PATCH v5 0/2] Add phy support for arasan,sdhci-5.1

[Ulf Hansson]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

Den 19 feb 2016 3:24 fm skrev "Shawn Lin" <shawn.lin@rock-chips.com>:
>
>
> Hello Ulf and Adrian,
>
> Sorry for delay the new version for this topic. Actually I have no
> platform available to test phy stuff since v4. Now, I can rework it.:)
> I remove rpm support from v5 because this patchset just need to focus
> on phy stuff, which will make the changes more precise. Another reason
> is that phy_power_on take too much time from rpm, however the power profit
> gained from phy_power_off is little due to the low power mechanism with
the
> phy itself observed from experiments.
>
> v4 can be found here:
> https://patchwork.kernel.org/patch/7463251/
>
> Note:
> Phy is mandatory requirement for arasan,sdhci-5.1, so we introduce
> generic phy support for sdhci-of-arasan.
>
>
> Shawn Lin (2):
>   Documentation: bindings: add description of phy for sdhci-of-arasan
>   mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
>
>  .../devicetree/bindings/mmc/arasan,sdhci.txt       | 20 ++++++-
>  drivers/mmc/host/sdhci-of-arasan.c                 | 66
++++++++++++++++++++--
>  2 files changed, 80 insertions(+), 6 deletions(-)
>
> --
> 2.3.7
>
>

This looks good to me! When acks has been received I am ready to queue it
for next.

Kind regards
Uffe

[-- Attachment #2: Type: text/html, Size: 1724 bytes --]

Re: [RESEND PATCH v5 0/2] Add phy support for arasan,sdhci-5.1

[Shawn Lin]

在 2016/2/20 6:23, Ulf Hansson 写道:
>
> Den 19 feb 2016 3:24 fm skrev "Shawn Lin" <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org
> <mailto:shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>>:
>  >
>  >
>  > Hello Ulf and Adrian,
>  >
>  > Sorry for delay the new version for this topic. Actually I have no
>  > platform available to test phy stuff since v4. Now, I can rework it.:)
>  > I remove rpm support from v5 because this patchset just need to focus
>  > on phy stuff, which will make the changes more precise. Another reason
>  > is that phy_power_on take too much time from rpm, however the power
> profit
>  > gained from phy_power_off is little due to the low power mechanism
> with the
>  > phy itself observed from experiments.
>  >
>  > v4 can be found here:
>  > https://patchwork.kernel.org/patch/7463251/
>  >
>  > Note:
>  > Phy is mandatory requirement for arasan,sdhci-5.1, so we introduce
>  > generic phy support for sdhci-of-arasan.
>  >
>  >
>  > Shawn Lin (2):
>  >   Documentation: bindings: add description of phy for sdhci-of-arasan
>  >   mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
>  >
>  >  .../devicetree/bindings/mmc/arasan,sdhci.txt       | 20 ++++++-
>  >  drivers/mmc/host/sdhci-of-arasan.c                 | 66
> ++++++++++++++++++++--
>  >  2 files changed, 80 insertions(+), 6 deletions(-)
>  >
>  > --
>  > 2.3.7
>  >
>  >
>
> This looks good to me! When acks has been received I am ready to queue
> it for next.
>

Hi Adrian,

	Any chance to look at this patch? I assume Ulf need your ack for
sdhci* patch. :)

> Kind regards
> Uffe
>


-- 
Best Regards
Shawn Lin

--
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

Re: [RESEND PATCH v5 0/2] Add phy support for arasan,sdhci-5.1

[Adrian Hunter]

On 04/03/16 03:27, Shawn Lin wrote:
> 在 2016/2/20 6:23, Ulf Hansson 写道:
>>
>> Den 19 feb 2016 3:24 fm skrev "Shawn Lin" <shawn.lin@rock-chips.com
>> <mailto:shawn.lin@rock-chips.com>>:
>>  >
>>  >
>>  > Hello Ulf and Adrian,
>>  >
>>  > Sorry for delay the new version for this topic. Actually I have no
>>  > platform available to test phy stuff since v4. Now, I can rework it.:)
>>  > I remove rpm support from v5 because this patchset just need to focus
>>  > on phy stuff, which will make the changes more precise. Another reason
>>  > is that phy_power_on take too much time from rpm, however the power
>> profit
>>  > gained from phy_power_off is little due to the low power mechanism
>> with the
>>  > phy itself observed from experiments.
>>  >
>>  > v4 can be found here:
>>  > https://patchwork.kernel.org/patch/7463251/
>>  >
>>  > Note:
>>  > Phy is mandatory requirement for arasan,sdhci-5.1, so we introduce
>>  > generic phy support for sdhci-of-arasan.
>>  >
>>  >
>>  > Shawn Lin (2):
>>  >   Documentation: bindings: add description of phy for sdhci-of-arasan
>>  >   mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
>>  >
>>  >  .../devicetree/bindings/mmc/arasan,sdhci.txt       | 20 ++++++-
>>  >  drivers/mmc/host/sdhci-of-arasan.c                 | 66
>> ++++++++++++++++++++--
>>  >  2 files changed, 80 insertions(+), 6 deletions(-)
>>  >
>>  > --
>>  > 2.3.7
>>  >
>>  >
>>
>> This looks good to me! When acks has been received I am ready to queue
>> it for next.
>>
> 
> Hi Adrian,
> 
>     Any chance to look at this patch? I assume Ulf need your ack for
> sdhci* patch. :)

Just sent some comments.