diff mbox

[v3,05/15] phy: rockchip-emmc: Increase lock time allowance

Message ID 1466445414-11974-6-git-send-email-dianders@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Douglas Anderson June 20, 2016, 5:56 p.m. UTC 
Previous PHY code waited a fixed amount of time for the DLL to lock at
power on time.  Unfortunately, the time for the DLL to lock is actually
a bit more dynamic and can be longer if the card clock is slower.

Instead of waiting a fixed 30 us, let's now dynamically wait until the
lock bit gets set.  We'll wait up to 10 ms which should be OK even if
the card clock is at the super slow 100 kHz.

On its own, this change makes the PHY power on code a little more
robust.  Before this change the PHY was relying on the eMMC code to make
sure the PHY was only powered on when the card clock was set to at least
50 MHz before, though this reliance wasn't documented anywhere.

This change will be even more useful in future changes where we actually
need to be able to wait for a DLL lock at slower clock speeds.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
Changes in v3:
- Add collected tags

Changes in v2:
- Indicate that 5.1 ms is calculated (Shawn).

 drivers/phy/phy-rockchip-emmc.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

Guenter Roeck June 20, 2016, 7:29 p.m. UTC | #1 
On Mon, Jun 20, 2016 at 10:56 AM, Douglas Anderson
<dianders@chromium.org> wrote:
> Previous PHY code waited a fixed amount of time for the DLL to lock at
> power on time.  Unfortunately, the time for the DLL to lock is actually
> a bit more dynamic and can be longer if the card clock is slower.
>
> Instead of waiting a fixed 30 us, let's now dynamically wait until the
> lock bit gets set.  We'll wait up to 10 ms which should be OK even if
> the card clock is at the super slow 100 kHz.
>

10 ms active delay (no sleep) is actually quite long. Can this code sleep ?

> On its own, this change makes the PHY power on code a little more
> robust.  Before this change the PHY was relying on the eMMC code to make
> sure the PHY was only powered on when the card clock was set to at least
> 50 MHz before, though this reliance wasn't documented anywhere.
>
> This change will be even more useful in future changes where we actually
> need to be able to wait for a DLL lock at slower clock speeds.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
> Changes in v3:
> - Add collected tags
>
> Changes in v2:
> - Indicate that 5.1 ms is calculated (Shawn).
>
>  drivers/phy/phy-rockchip-emmc.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
> index a69f53630e67..2d059c046978 100644
> --- a/drivers/phy/phy-rockchip-emmc.c
> +++ b/drivers/phy/phy-rockchip-emmc.c
> @@ -85,6 +85,7 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
>  {
>         unsigned int caldone;
>         unsigned int dllrdy;
> +       unsigned long timeout;
>
>         /*
>          * Keep phyctrl_pdb and phyctrl_endll low to allow
> @@ -137,15 +138,26 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
>                                    PHYCTRL_ENDLL_MASK,
>                                    PHYCTRL_ENDLL_SHIFT));
>         /*
> -        * After enable analog DLL circuits, we need an extra 10.2us
> -        * for dll to be ready for work. But according to testing, we
> -        * find some chips need more than 25us.
> +        * After enabling analog DLL circuits docs say that we need 10.2 us if
> +        * our source clock is at 50 MHz and that lock time scales linearly
> +        * with clock speed.  If we are powering on the PHY and the card clock
> +        * is super slow (like 100 kHZ) this could take as long as 5.1 ms as
> +        * per the math: 10.2 us * (50000000 Hz / 100000 Hz) => 5.1 ms
> +        * Hopefully we won't be running at 100 kHz, but we should still make
> +        * sure we wait long enough.
>          */
> -       udelay(30);
> -       regmap_read(rk_phy->reg_base,
> -                   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> -                   &dllrdy);
> -       dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
> +       timeout = jiffies + msecs_to_jiffies(10);
> +       do {
> +               udelay(1);
> +
> +               regmap_read(rk_phy->reg_base,
> +                       rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> +                       &dllrdy);
> +               dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
> +               if (dllrdy == PHYCTRL_DLLRDY_DONE)
> +                       break;
> +       } while (!time_after(jiffies, timeout));
> +
>         if (dllrdy != PHYCTRL_DLLRDY_DONE) {
>                 pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
>                 return -ETIMEDOUT;
> --
> 2.8.0.rc3.226.g39d4020
>
--
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
Douglas Anderson June 20, 2016, 7:36 p.m. UTC | #2 
Hi,

On Mon, Jun 20, 2016 at 12:29 PM, Guenter Roeck <groeck@google.com> wrote:
> On Mon, Jun 20, 2016 at 10:56 AM, Douglas Anderson
> <dianders@chromium.org> wrote:
>> Previous PHY code waited a fixed amount of time for the DLL to lock at
>> power on time.  Unfortunately, the time for the DLL to lock is actually
>> a bit more dynamic and can be longer if the card clock is slower.
>>
>> Instead of waiting a fixed 30 us, let's now dynamically wait until the
>> lock bit gets set.  We'll wait up to 10 ms which should be OK even if
>> the card clock is at the super slow 100 kHz.
>>
>
> 10 ms active delay (no sleep) is actually quite long. Can this code sleep ?

It is expected that in nearly all cases it will be much shorter than
10ms.  The longest expected (at 400kHz) is 1.3 ms and we should only
be probing down to 300, 200, 100 kHz if we are having trouble
communicating.  When running at a normal speed (50 MHz, 100 MHz, etc)
it should be much smaller and closer to 10 us or less.  We could still
try to sleep in some of these cases, but IMHO the extra code
complexity for something like this that should happen very
infrequently (only at bootup or if we decide to re-tune) is probably
not worth it.  Also note that at boot eMMC is (probably) on the
critical path, so there may be some boot speed benefits to continuing
as quickly as possible.

-Doug
--
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
Guenter Roeck June 20, 2016, 7:38 p.m. UTC | #3 
On Mon, Jun 20, 2016 at 12:36 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Mon, Jun 20, 2016 at 12:29 PM, Guenter Roeck <groeck@google.com> wrote:
>> On Mon, Jun 20, 2016 at 10:56 AM, Douglas Anderson
>> <dianders@chromium.org> wrote:
>>> Previous PHY code waited a fixed amount of time for the DLL to lock at
>>> power on time.  Unfortunately, the time for the DLL to lock is actually
>>> a bit more dynamic and can be longer if the card clock is slower.
>>>
>>> Instead of waiting a fixed 30 us, let's now dynamically wait until the
>>> lock bit gets set.  We'll wait up to 10 ms which should be OK even if
>>> the card clock is at the super slow 100 kHz.
>>>
>>
>> 10 ms active delay (no sleep) is actually quite long. Can this code sleep ?
>
> It is expected that in nearly all cases it will be much shorter than
> 10ms.  The longest expected (at 400kHz) is 1.3 ms and we should only
> be probing down to 300, 200, 100 kHz if we are having trouble
> communicating.  When running at a normal speed (50 MHz, 100 MHz, etc)
> it should be much smaller and closer to 10 us or less.  We could still
> try to sleep in some of these cases, but IMHO the extra code
> complexity for something like this that should happen very
> infrequently (only at bootup or if we decide to re-tune) is probably
> not worth it.  Also note that at boot eMMC is (probably) on the
> critical path, so there may be some boot speed benefits to continuing
> as quickly as possible.
>

Makes sense.

Thanks,
Guenter
--
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
diff mbox

Patch

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index a69f53630e67..2d059c046978 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -85,6 +85,7 @@  static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 {
 	unsigned int caldone;
 	unsigned int dllrdy;
+	unsigned long timeout;
 
 	/*
 	 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -137,15 +138,26 @@  static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 				   PHYCTRL_ENDLL_MASK,
 				   PHYCTRL_ENDLL_SHIFT));
 	/*
-	 * After enable analog DLL circuits, we need an extra 10.2us
-	 * for dll to be ready for work. But according to testing, we
-	 * find some chips need more than 25us.
+	 * After enabling analog DLL circuits docs say that we need 10.2 us if
+	 * our source clock is at 50 MHz and that lock time scales linearly
+	 * with clock speed.  If we are powering on the PHY and the card clock
+	 * is super slow (like 100 kHZ) this could take as long as 5.1 ms as
+	 * per the math: 10.2 us * (50000000 Hz / 100000 Hz) => 5.1 ms
+	 * Hopefully we won't be running at 100 kHz, but we should still make
+	 * sure we wait long enough.
 	 */
-	udelay(30);
-	regmap_read(rk_phy->reg_base,
-		    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-		    &dllrdy);
-	dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
+	timeout = jiffies + msecs_to_jiffies(10);
+	do {
+		udelay(1);
+
+		regmap_read(rk_phy->reg_base,
+			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+			&dllrdy);
+		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
+		if (dllrdy == PHYCTRL_DLLRDY_DONE)
+			break;
+	} while (!time_after(jiffies, timeout));
+
 	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
 		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
 		return -ETIMEDOUT;