devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Shawn Lin <shawn.lin@rock-chips.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Heiko Stuebner <heiko@sntech.de>,
	Rob Herring <robh+dt@kernel.org>,
	Ziyuan Xu <xzy.xu@rock-chips.com>,
	Brian Norris <briannorris@chromium.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Michal Simek <michal.simek@xilinx.com>,
	soren.brinkmann@xilinx.com,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.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:06:47 -0700	[thread overview]
Message-ID: <CAD=FV=V8GumWP0aKQdCyXBEdUcjg7c5BKLu5smUYuZW3C5DHmA@mail.gmail.com> (raw)
In-Reply-To: <ae4409b0-48a5-e30f-a244-7a28bd7a9daa@rock-chips.com>

Hi,

On Mon, Jun 13, 2016 at 1:08 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 在 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@chromium.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.

Thanks!  I don't see a Reviewed-by tag so I'm not adding it.  If you'd
like your tag added, please let me know and I will add it if I send
out another version.


-Doug

  reply	other threads:[~2016-06-13 23:06 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
2016-06-13 23:06         ` Doug Anderson [this message]
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='CAD=FV=V8GumWP0aKQdCyXBEdUcjg7c5BKLu5smUYuZW3C5DHmA@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=adrian.hunter@intel.com \
    --cc=briannorris@chromium.org \
    --cc=devicetree@vger.kernel.org \
    --cc=heiko@sntech.de \
    --cc=kishon@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=michal.simek@xilinx.com \
    --cc=robh+dt@kernel.org \
    --cc=shawn.lin@rock-chips.com \
    --cc=soren.brinkmann@xilinx.com \
    --cc=ulf.hansson@linaro.org \
    --cc=xzy.xu@rock-chips.com \
    --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