devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
To: Douglas Anderson
	<dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
	ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	kishon-l0cyMroinI0@public.gmane.org,
	Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org,
	adrian.hunter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org,
	soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH 02/11] mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes
Date: Mon, 13 Jun 2016 16:08:54 +0800	[thread overview]
Message-ID: <ae4409b0-48a5-e30f-a244-7a28bd7a9daa@rock-chips.com> (raw)
In-Reply-To: <1465339484-969-3-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

在 2016/6/8 6:44, Douglas Anderson 写道:
> In commit 802ac39a5566 ("mmc: sdhci-of-arasan: fix set_clock when a phy
> is supported") we added code to power the PHY off and on whenever the
> clock was changed but we avoided doing the power cycle code when the
> clock was low speed.  Let's now do it always.
>
> Although there may be other reasons for power cycling the PHY when the
> clock changes, one of the main reasons is that we need to give the DLL a
> chance to re-lock with the new clock.
>
> One of the things that the DLL is for is tuning the Receive Clock in
> HS200 mode and STRB in HS400 mode.  Thus it is clear that we should make
> sure we power cycle the PHY (and wait for the DLL to lock) when we know
> we'll be in one of these two speed modes.  That's what the original code
> did, though it used the clock rate rather than the speed mode.  However,
> even in speed modes other than HS200,/HS400 the DLL is used for
> something since it can be clearly observed that the PHY doesn't function
> properly if you leave the DLL off.
>
> Although it appears less important to power cycle the PHY and wait for
> the DLL to lock when not in HS200/HS400 modes (no bugs were reported),
> it still seems wise to let the locking always happen nevertheless.
>

 From the design doc, there is no need to off/on phy when not in
HS200/400, but maybe someone will limit the clk freq by assigning
max-frequency in DT when in HS200/400, which will make things worse.

So your patch looks sane.

> Note: as part of this, we make sure that we never try to turn the PHY on
> when the clock is off (when the clock rate is 0).  The PHY cannot work
> when the clock is off since its DLL can't lock.
>
> This change requires ("phy: rockchip-emmc: Increase lock time
> allowance") and will cause problems if picked without that change.
>
> Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 533e2bcb10bc..3ff1711077c2 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -35,11 +35,13 @@
>  /**
>   * struct sdhci_arasan_data
>   * @clk_ahb:	Pointer to the AHB clock
> - * @phy: Pointer to the generic phy
> + * @phy:	Pointer to the generic phy
> + * @phy_on:	True if the PHY is turned on.
>   */
>  struct sdhci_arasan_data {
>  	struct clk	*clk_ahb;
>  	struct phy	*phy;
> +	bool		phy_on;
>  };
>
>  static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> @@ -61,12 +63,10 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> -	bool ctrl_phy = false;
>
> -	if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy)))
> -		ctrl_phy = true;
> +	if (sdhci_arasan->phy_on && !IS_ERR(sdhci_arasan->phy)) {
> +		sdhci_arasan->phy_on = false;
>
> -	if (ctrl_phy) {
>  		spin_unlock_irq(&host->lock);
>  		phy_power_off(sdhci_arasan->phy);
>  		spin_lock_irq(&host->lock);
> @@ -74,7 +74,9 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>
>  	sdhci_set_clock(host, clock);
>
> -	if (ctrl_phy) {
> +	if (host->mmc->actual_clock && !IS_ERR(sdhci_arasan->phy)) {
> +		sdhci_arasan->phy_on = true;
> +
>  		spin_unlock_irq(&host->lock);
>  		phy_power_on(sdhci_arasan->phy);
>  		spin_lock_irq(&host->lock);
> @@ -257,12 +259,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  			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;
> -		}
> -

Because there is too much dependency between phy and controller, so
previous solution is to setup clk by firmware. The same case for
suspend and resume.

Look really good to do it more legit.

>  		host->mmc_host_ops.hs400_enhanced_strobe =
>  					sdhci_arasan_hs400_enhanced_strobe;
>  	}
> @@ -275,9 +271,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>
>  err_add_host:
>  	if (!IS_ERR(sdhci_arasan->phy))
> -		phy_power_off(sdhci_arasan->phy);
> -err_phy_power:
> -	if (!IS_ERR(sdhci_arasan->phy))
>  		phy_exit(sdhci_arasan->phy);
>  clk_disable_all:
>  	clk_disable_unprepare(clk_xin);
>


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

  parent reply	other threads:[~2016-06-13  8:08 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-07 22:44 [PATCH 0/11] Changes to support 150 MHz eMMC on rk3399 Douglas Anderson
     [not found] ` <1465339484-969-1-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-06-07 22:44   ` [PATCH 01/11] phy: rockchip-emmc: Increase lock time allowance Douglas Anderson
     [not found]     ` <1465339484-969-2-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-06-13  7:58       ` Shawn Lin
     [not found]         ` <3e19ff54-ee4a-c208-e137-1c0f8022f6b3-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-06-13 23:07           ` Doug Anderson
2016-06-07 22:44   ` [PATCH 02/11] mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes Douglas Anderson
     [not found]     ` <1465339484-969-3-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-06-13  8:08       ` Shawn Lin [this message]
2016-06-13 23:06         ` Doug Anderson
2016-06-07 22:44   ` [PATCH 03/11] Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs Douglas Anderson
2016-06-08 20:17     ` Rob Herring
2016-06-13  8:18     ` Shawn Lin
     [not found]       ` <45a7e8c7-5bd4-8c40-004a-b8906eff881a-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-06-13  9:32         ` Heiko Stübner
2016-06-13 23:07       ` Doug Anderson
2016-06-07 22:44   ` [PATCH 07/11] mmc: sdhci-of-arasan: Add ability to export card clock Douglas Anderson
2016-06-07 22:44   ` [PATCH 08/11] Documentation: phy: Let the rockchip eMMC PHY get an exported " Douglas Anderson
2016-06-10 13:36     ` Rob Herring
2016-06-13 23:05       ` Doug Anderson
2016-06-07 22:44   ` [PATCH 10/11] phy: rockchip-emmc: Minor code cleanup in rockchip_emmc_phy_power_off() Douglas Anderson
2016-06-13  8:56     ` Shawn Lin
2016-06-13 23:05       ` Doug Anderson
2016-06-07 22:44 ` [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399 Douglas Anderson
2016-06-13  8:36   ` Shawn Lin
     [not found]     ` <f5dcc018-bd60-87a7-798b-efc261e443dd-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-06-13 23:06       ` Doug Anderson
2016-06-14  0:14         ` Shawn Lin
     [not found]           ` <47a2dcd9-9c3c-8fde-2be0-40e305c25e8d-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-06-14  0:43             ` Doug Anderson
     [not found]               ` <CAD=FV=UL5tU8RWtHF=-pE8SA0jUcBsqQDOU0BrruXOF7yGh5xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-06-14  0:59                 ` Shawn Lin
2016-06-14  2:13                   ` Doug Anderson
2016-06-16  1:06                     ` Shawn Lin
2016-06-07 22:44 ` [PATCH 05/11] arm64: dts: rockchip: Add soc-ctl-syscon to sdhci for rk3399 Douglas Anderson
2016-06-07 22:44 ` [PATCH 06/11] Documentation: mmc: sdhci-of-arasan: Add ability to export card clock Douglas Anderson
2016-06-08 20:19   ` Rob Herring
2016-06-08 20:52     ` Doug Anderson
2016-06-10 13:10       ` Rob Herring
2016-06-13 23:05         ` Doug Anderson
2016-06-07 22:44 ` [PATCH 09/11] phy: rockchip-emmc: Set phyctrl_frqsel based on " Douglas Anderson
     [not found]   ` <1465339484-969-10-git-send-email-dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-06-13  8:54     ` Shawn Lin
2016-06-13 23:05       ` Doug Anderson
2016-06-14  0:24         ` Shawn Lin
     [not found]           ` <036b0349-8343-f5de-7215-5a0843ebc6a9-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-06-14  0:45             ` Doug Anderson
2016-06-07 22:44 ` [PATCH 11/11] arm64: dts: rockchip: Provide emmcclk to PHY for rk3399 Douglas Anderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ae4409b0-48a5-e30f-a244-7a28bd7a9daa@rock-chips.com \
    --to=shawn.lin-tnx95d0mmh7dzftrwevzcw@public.gmane.org \
    --cc=adrian.hunter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org \
    --cc=kishon-l0cyMroinI0@public.gmane.org \
    --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org \
    --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org \
    --cc=ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    --cc=xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org \
    --subject='Re: [PATCH 02/11] mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox