[PATCH v2 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[PATCH v2 1/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.
Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.
Currently, it's just mandatory for arasan,sdhci-5.1.

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

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 | 97 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 75379cb..2c13ef8 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)
@@ -70,6 +72,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
@@ -91,6 +149,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;
 }
 
@@ -122,6 +186,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 */
@@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		goto clk_dis_ahb;
 	}
 
+	sdhci_arasan->phy = NULL;
+	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 = -ENODEV;
+			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
+			goto clk_dis_ahb;
+		}
+
+		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;
+		}
+	}
+
 	host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0);
 	if (IS_ERR(host)) {
 		ret = PTR_ERR(host);
-- 
2.3.7

[PATCH v2 2/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: [PATCH v2 2/2] Documentation: bindings: add description of phy for sdhci-of-arasan

[Sören Brinkmann]

On Mon, 2015-09-14 at 02:29PM +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@rock-chips.com>
Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>

	Sören

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

[Sören Brinkmann]

Hi Shawn,

overall, it looks good to me. I have some questions though.

On Mon, 2015-09-14 at 02:29PM +0800, 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.
> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.
> Currently, it's just mandatory for arasan,sdhci-5.1.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> 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 | 97 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 75379cb..2c13ef8 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)
> @@ -70,6 +72,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);

Just to confirm, are these actions in the error path correct? E.g.
if the power_off() call fails, is it safe to call power_on()? Isn't
the phy still powered on? (this would apply to other error paths too)

> +	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
> @@ -91,6 +149,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;
>  }
>  
> @@ -122,6 +186,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 */
> @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  		goto clk_dis_ahb;
>  	}
>  
> +	sdhci_arasan->phy = NULL;
> +	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 = -ENODEV;
> +			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
> +			goto clk_dis_ahb;
> +		}
> +
> +		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;
> +		}
> +	}

I assume you looked at options for having the error paths in a
consolidated location? I guess this may be the nicest solution since all
of this is in this conditional block?

Feel free to add my
Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>

	Sören

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

[Shawn Lin]

On 2015/9/14 23:07, Sören Brinkmann wrote:
> Hi Shawn,
>
> overall, it looks good to me. I have some questions though.
>
> On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote:

[...]

>> +err_phy_exit:
>> +	phy_init(phy);
>
> Just to confirm, are these actions in the error path correct? E.g.
> if the power_off() call fails, is it safe to call power_on()? Isn't
> the phy still powered on? (this would apply to other error paths too)
>

Cool question!

While writing this, I had read generic phy stuffs deliberately to find a 
solution for a case: how to deal with ping-pong fails? In another word, 
if power_off call fails, then we should call power_on, but unfortunately 
it fails again then we call power_off... so endless nested err 
handling... No answer yet.

So, I assumed two cases happened when power_off call fails:
(1) *real power_off* is done, but some other stuffs in the calling path 
fail, so phy is really power_off in theory. We need to power_on it 
again, but if it fail, we don't know PHY is on or off since we don't 
know power_on fails for what? *real power on* ? some other stuffs?

(2) *real power_off* isn't completed, so indeed it's *still* in power_on 
state. The reason we never need to check the return value of power_on 
cross the err handling is that whether power_on call successfully or 
not, it's always make phy in power_on state.

Now, let's think about case(1).
After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt 
generic phy framework for PHY management, real thing should be like 
that: they NEVER deal with case(1).

It's a trick of sub-phy drivers themself. power_on/off calling path 
return err for two case:
<1>  phy_runtime callback fails. It's after *real power on/off*, so
definitely *real power on/off* is conpleted. That is the case(2) I 
mentioned above.
<2>  sub-phy drivers return err for  phy->ops->power_off(phy); Look
into all the sub-phy drivers twice, we find that they always return 
success for phy->ops->power_off hook. Why? Because all of them
write registers to enable/disable something, always consider things 
going well. Actually if we write value into a register, we have to think 
that it's functional.

Anyway, back to this patch.
Indeed we also write value into arasan phy's register to do 
phy_power_on/off/init/exit to make things work. Right, we return success 
state for all of these them just as all the other sub-phy drivers do.

Feel free to let me know if I make mistakes or misunderstanding above.

>> +	return ret;
>> +}

[...]

>> +		}
>> +	}
>
> I assume you looked at options for having the error paths in a
> consolidated location? I guess this may be the nicest solution since all
> of this is in this conditional block?
>

yep, otherwise we should add some *if* statements to check 
sdhci_arasan->phy cross the err handles. And I intent to strictly limit
the phy stuffs under the scope of arasan,sdhci-5.1 currently.

> Feel free to add my
> Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
>

Thanks, Sören. :)

> 	Sören
>
>
>


-- 
Best Regards
Shawn Lin

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

[Ulf Hansson]

On 14 September 2015 at 08:29, 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.
> Currently, it's just mandatory for arasan,sdhci-5.1.

I am trying to understand how a PHY can be used together with a
MMC/SD/SDIO controller. Normally the card connector doesn't hold any
intelligence, so I wonder if PHY is correctly used here.

Could you try to explain, HW-wise, what role the PHY has for you?

Kind regards
Uffe

>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> 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 | 97 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 75379cb..2c13ef8 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)
> @@ -70,6 +72,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
> @@ -91,6 +149,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;
>  }
>
> @@ -122,6 +186,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 */
> @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>                 goto clk_dis_ahb;
>         }
>
> +       sdhci_arasan->phy = NULL;
> +       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 = -ENODEV;
> +                       dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
> +                       goto clk_dis_ahb;
> +               }
> +
> +               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;
> +               }
> +       }
> +
>         host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0);
>         if (IS_ERR(host)) {
>                 ret = PTR_ERR(host);
> --
> 2.3.7
>
>
> --
> 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
--
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: [PATCH v2 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Shawn Lin]

On 2015/9/17 19:44, Ulf Hansson wrote:
> On 14 September 2015 at 08:29, 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.
>> Currently, it's just mandatory for arasan,sdhci-5.1.
>
> I am trying to understand how a PHY can be used together with a
> MMC/SD/SDIO controller. Normally the card connector doesn't hold any
> intelligence, so I wonder if PHY is correctly used here.
>
> Could you try to explain, HW-wise, what role the PHY has for you?
>

Thanks for comment, Ulf.

It's the first time we introduce phy into mmc subsystem,also it's my 
first time to use phy for emmc in real Soc.
so definitely we need to discuss it deliberately.

 From my view, phy is an active-power, differential sampling and more
well-designed analog *IO component* for ultra-highspeed device to 
guarantee its signal integrity, like USB, UFS, DDR-RAM,MIPI and so on.

(1)Firstly it contains DLL to generate delayline and phase for sampling 
data from mmc devices. When sdhci controller executes tuning and sends 
out tuning sequence, and PHY can auto change its delayline and phase
in order to test if this "sample timing" is okay. If not, CRC err is 
generate back to the controller. Then after SDHCI tune timing for 40 
times, also PHY have scanned all the "sample windows" by trying 
different delayline and phase combination, and finally auto-select the 
best sample timing for the "sample windows". And sdhci HOST CONTROL
2 Register[6:7] is controlled by PHY in my case.

(2)Then phy can programming source/sink impedance to avoid signal 
reflex. Given that JEDEC has come to HS533(highspeed 533MHz), this 
function is imperative to be implemented just like the role of ODT for 
DDR-RAM's PHY.

(3)Phy is integrated with diff pull-up/down resistance value and 
open-drain type for HW design.

(4)Phy can also have some enable-bit to decide whether all mmc 
signal(clk/data/cmd/strobe) can output to the devices or not.


For the card, even for host controller itself, phy can just be regarded 
as a need-to-do-something-before-used GPIO.
So, before we start initializing card, we need to power up PHY, enable 
clk for it and configure all the stuffs. Also we need to power down it 
for power-saving when into suspend. That's all we need to do and that's 
all the generic phy framework had provided by four interface: 
phy_init/phy_exit/phy_power_on/phy_power_off.


Sincerely welcome any comments here to make things better. :)


> Kind regards
> Uffe
>
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> 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 | 97 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 97 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index 75379cb..2c13ef8 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)
>> @@ -70,6 +72,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
>> @@ -91,6 +149,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;
>>   }
>>
>> @@ -122,6 +186,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 */
>> @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>                  goto clk_dis_ahb;
>>          }
>>
>> +       sdhci_arasan->phy = NULL;
>> +       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 = -ENODEV;
>> +                       dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
>> +                       goto clk_dis_ahb;
>> +               }
>> +
>> +               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;
>> +               }
>> +       }
>> +
>>          host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata, 0);
>>          if (IS_ERR(host)) {
>>                  ret = PTR_ERR(host);
>> --
>> 2.3.7
>>
>>
>> --
>> 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

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

[Ulf Hansson]

On 17 September 2015 at 17:20, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2015/9/17 19:44, Ulf Hansson wrote:
>>
>> On 14 September 2015 at 08:29, 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.
>>> Currently, it's just mandatory for arasan,sdhci-5.1.
>>
>>
>> I am trying to understand how a PHY can be used together with a
>> MMC/SD/SDIO controller. Normally the card connector doesn't hold any
>> intelligence, so I wonder if PHY is correctly used here.
>>
>> Could you try to explain, HW-wise, what role the PHY has for you?
>>
>
> Thanks for comment, Ulf.
>
> It's the first time we introduce phy into mmc subsystem,also it's my first
> time to use phy for emmc in real Soc.
> so definitely we need to discuss it deliberately.
>
> From my view, phy is an active-power, differential sampling and more
> well-designed analog *IO component* for ultra-highspeed device to guarantee
> its signal integrity, like USB, UFS, DDR-RAM,MIPI and so on.
>
> (1)Firstly it contains DLL to generate delayline and phase for sampling data
> from mmc devices. When sdhci controller executes tuning and sends out tuning
> sequence, and PHY can auto change its delayline and phase
> in order to test if this "sample timing" is okay. If not, CRC err is
> generate back to the controller. Then after SDHCI tune timing for 40 times,
> also PHY have scanned all the "sample windows" by trying different delayline
> and phase combination, and finally auto-select the best sample timing for
> the "sample windows". And sdhci HOST CONTROL
> 2 Register[6:7] is controlled by PHY in my case.
>
> (2)Then phy can programming source/sink impedance to avoid signal reflex.
> Given that JEDEC has come to HS533(highspeed 533MHz), this function is
> imperative to be implemented just like the role of ODT for DDR-RAM's PHY.
>
> (3)Phy is integrated with diff pull-up/down resistance value and open-drain
> type for HW design.
>
> (4)Phy can also have some enable-bit to decide whether all mmc
> signal(clk/data/cmd/strobe) can output to the devices or not.

Thanks for elaborating. It seems like using phy is justified in your case.

>
>
> For the card, even for host controller itself, phy can just be regarded as a
> need-to-do-something-before-used GPIO.
> So, before we start initializing card, we need to power up PHY, enable clk
> for it and configure all the stuffs. Also we need to power down it for
> power-saving when into suspend. That's all we need to do and that's all the
> generic phy framework had provided by four interface:
> phy_init/phy_exit/phy_power_on/phy_power_off.
>
>
> Sincerely welcome any comments here to make things better. :)
>
>

I will get back to your patch and review it in detail as soon as can.

Kind regards
Uffe

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

[Sören Brinkmann]

On Tue, 2015-09-15 at 09:07AM +0800, Shawn Lin wrote:
> On 2015/9/14 23:07, Sören Brinkmann wrote:
> >Hi Shawn,
> >
> >overall, it looks good to me. I have some questions though.
> >
> >On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote:
> 
> [...]
> 
> >>+err_phy_exit:
> >>+	phy_init(phy);
> >
> >Just to confirm, are these actions in the error path correct? E.g.
> >if the power_off() call fails, is it safe to call power_on()? Isn't
> >the phy still powered on? (this would apply to other error paths too)
> >
> 
> Cool question!
> 
> While writing this, I had read generic phy stuffs deliberately to find a
> solution for a case: how to deal with ping-pong fails? In another word, if
> power_off call fails, then we should call power_on, but unfortunately it
> fails again then we call power_off... so endless nested err handling... No
> answer yet.
> 
> So, I assumed two cases happened when power_off call fails:
> (1) *real power_off* is done, but some other stuffs in the calling path
> fail, so phy is really power_off in theory. We need to power_on it again,
> but if it fail, we don't know PHY is on or off since we don't know power_on
> fails for what? *real power on* ? some other stuffs?
> 
> (2) *real power_off* isn't completed, so indeed it's *still* in power_on
> state. The reason we never need to check the return value of power_on cross
> the err handling is that whether power_on call successfully or not, it's
> always make phy in power_on state.
> 
> Now, let's think about case(1).
> After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt
> generic phy framework for PHY management, real thing should be like that:
> they NEVER deal with case(1).
> 
> It's a trick of sub-phy drivers themself. power_on/off calling path return
> err for two case:
> <1>  phy_runtime callback fails. It's after *real power on/off*, so
> definitely *real power on/off* is conpleted. That is the case(2) I mentioned
> above.
> <2>  sub-phy drivers return err for  phy->ops->power_off(phy); Look
> into all the sub-phy drivers twice, we find that they always return success
> for phy->ops->power_off hook. Why? Because all of them
> write registers to enable/disable something, always consider things going
> well. Actually if we write value into a register, we have to think that it's
> functional.
> 
> Anyway, back to this patch.
> Indeed we also write value into arasan phy's register to do
> phy_power_on/off/init/exit to make things work. Right, we return success
> state for all of these them just as all the other sub-phy drivers do.
> 
> Feel free to let me know if I make mistakes or misunderstanding above.
> 
> >>+	return ret;
> >>+}
> 
> [...]
> 
> >>+		}
> >>+	}
> >
> >I assume you looked at options for having the error paths in a
> >consolidated location? I guess this may be the nicest solution since all
> >of this is in this conditional block?
> >
> 
> yep, otherwise we should add some *if* statements to check sdhci_arasan->phy
> cross the err handles. And I intent to strictly limit
> the phy stuffs under the scope of arasan,sdhci-5.1 currently.
> 
> >Feel free to add my
> >Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >
> 
> Thanks, Sören. :)

Makes all sense to me. Thanks for all the details.

	Sören
--
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