Hi Kishon,
Since the Shawn isn't available, I take over this series patches for now.
As the original bug had tracked on https://issuetracker.google.com/71561742.
In some cases, the mmc phy power on failed during booting up.
The log as below:
...
[ 2.375333] rockchip_emmc_phy_power: caldone timeout.
[ 2.377815] phy phy-ff770000.syscon:[email protected]: phy poweron failed --> -110
...
[ 2.489295] mmc0: mmc_select_hs400es failed, error -110
[ 2.489302] mmc0: error -110 whilst initialising MMC card
..
The actual emulate, the wait 5us for calpad busy trimming, that's no enough.
We need give the enough margin for it.
Verified on url =
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4
This series patches can apply and bring up with kernel-next on rk3399 chromebook.
-Caesar
Changes in v2:
- print the return valut with regmap_read_poll_timeout failing.
- As Brian commented on https://patchwork.kernel.org/patch/10139891/,
changed the note and added to print error value with
regmap_read_poll_timeout API.
Shawn Lin (2):
phy: rockchip-emmc: retry calpad busy trimming
phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy
drivers/phy/rockchip/phy-rockchip-emmc.c | 60 +++++++++++++++-----------------
1 file changed, 28 insertions(+), 32 deletions(-)
--
2.7.4
From: Shawn Lin <[email protected]>
It turns out that 5us isn't enough for all cases, so let's
retry some more times to wait for caldone.
Signed-off-by: Shawn Lin <[email protected]>
Tested-by: Ziyuan Xu <[email protected]>
Signed-off-by: Caesar Wang <[email protected]>
---
Changes in v2:
- print the return valut with regmap_read_poll_timeout failing.
drivers/phy/rockchip/phy-rockchip-emmc.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index f1b24f1..574838f 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -76,6 +76,10 @@
#define PHYCTRL_OTAPDLYSEL_MASK 0xf
#define PHYCTRL_OTAPDLYSEL_SHIFT 0x7
+#define PHYCTRL_IS_CALDONE(x) \
+ ((((x) >> PHYCTRL_CALDONE_SHIFT) & \
+ PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+
struct rockchip_emmc_phy {
unsigned int reg_offset;
struct regmap *reg_base;
@@ -90,6 +94,7 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
unsigned long timeout;
+ int ret;
/*
* Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -160,17 +165,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
PHYCTRL_PDB_SHIFT));
/*
- * According to the user manual, it asks driver to
- * wait 5us for calpad busy trimming
+ * According to the user manual, it asks driver to wait 5us for
+ * calpad busy trimming. However it is documented that this value is
+ * PVT(A.K.A process,voltage and temperature) relevant, so some
+ * failure cases are found which indicates we should be more tolerant
+ * to calpad busy trimming.
*/
- udelay(5);
- regmap_read(rk_phy->reg_base,
- rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
- &caldone);
- caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
- if (caldone != PHYCTRL_CALDONE_DONE) {
- pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
- return -ETIMEDOUT;
+ ret = regmap_read_poll_timeout(rk_phy->reg_base,
+ rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+ caldone, PHYCTRL_IS_CALDONE(caldone),
+ 5, 50);
+ if (ret) {
+ pr_err("%s: caldone timeout, ret=%d\n", __func__, ret);
+ return ret;
}
/* Set the frequency of the DLL operation */
--
2.7.4
From: Shawn Lin <[email protected]>
Just use the API instead of open-coding it, no functional change
intended.
Signed-off-by: Shawn Lin <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Signed-off-by: Caesar Wang <[email protected]>
---
Changes in v2:
- As Brian commented on https://patchwork.kernel.org/patch/10139891/,
changed the note and added to print error value with
regmap_read_poll_timeout API.
drivers/phy/rockchip/phy-rockchip-emmc.c | 33 +++++++++++---------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 574838f..343c623 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
#define PHYCTRL_IS_CALDONE(x) \
((((x) >> PHYCTRL_CALDONE_SHIFT) & \
PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+ ((((x) >> PHYCTRL_DLLRDY_SHIFT) & \
+ PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
struct rockchip_emmc_phy {
unsigned int reg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
unsigned int dllrdy;
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
- unsigned long timeout;
int ret;
/*
@@ -217,28 +219,15 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
* NOTE: There appear to be corner cases where the DLL seems to take
* extra long to lock for reasons that aren't understood. In some
* extreme cases we've seen it take up to over 10ms (!). We'll be
- * generous and give it 50ms. We still busy wait here because:
- * - In most cases it should be super fast.
- * - This is not called lots during normal operation so it shouldn't
- * be a power or performance problem to busy wait. We expect it
- * only at boot / resume. In both cases, eMMC is probably on the
- * critical path so busy waiting a little extra time should be OK.
+ * generous and give it 50ms.
*/
- timeout = jiffies + msecs_to_jiffies(50);
- 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;
+ ret = regmap_read_poll_timeout(rk_phy->reg_base,
+ rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+ dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+ 1, 50 * USEC_PER_MSEC);
+ if (ret) {
+ pr_err("%s: dllrdy timeout. ret=%d\n", __func__, ret);
+ return ret;
}
return 0;
--
2.7.4
Hi,
On Tue, Jan 9, 2018 at 11:37 PM, Caesar Wang <[email protected]> wrote:
> From: Shawn Lin <[email protected]>
>
> Just use the API instead of open-coding it, no functional change
> intended.
>
> Signed-off-by: Shawn Lin <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
> Signed-off-by: Caesar Wang <[email protected]>
>
> ---
>
> Changes in v2:
> - As Brian commented on https://patchwork.kernel.org/patch/10139891/,
> changed the note and added to print error value with
> regmap_read_poll_timeout API.
>
> drivers/phy/rockchip/phy-rockchip-emmc.c | 33 +++++++++++---------------------
> 1 file changed, 11 insertions(+), 22 deletions(-)
See comments in Shawn's v2. AKA: <https://patchwork.kernel.org/patch/10154797/>
Hi,
On Tue, Jan 9, 2018 at 11:37 PM, Caesar Wang <[email protected]> wrote:
> From: Shawn Lin <[email protected]>
>
> It turns out that 5us isn't enough for all cases, so let's
> retry some more times to wait for caldone.
>
> Signed-off-by: Shawn Lin <[email protected]>
> Tested-by: Ziyuan Xu <[email protected]>
> Signed-off-by: Caesar Wang <[email protected]>
> ---
>
> Changes in v2:
> - print the return valut with regmap_read_poll_timeout failing.
I agree with Brian that it was quite confusing to see a v2 from both
you and Shawn.
> drivers/phy/rockchip/phy-rockchip-emmc.c | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index f1b24f1..574838f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -76,6 +76,10 @@
> #define PHYCTRL_OTAPDLYSEL_MASK 0xf
> #define PHYCTRL_OTAPDLYSEL_SHIFT 0x7
>
> +#define PHYCTRL_IS_CALDONE(x) \
> + ((((x) >> PHYCTRL_CALDONE_SHIFT) & \
> + PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
> +
> struct rockchip_emmc_phy {
> unsigned int reg_offset;
> struct regmap *reg_base;
> @@ -90,6 +94,7 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
> unsigned int freqsel = PHYCTRL_FREQSEL_200M;
> unsigned long rate;
> unsigned long timeout;
> + int ret;
>
> /*
> * Keep phyctrl_pdb and phyctrl_endll low to allow
> @@ -160,17 +165,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
> PHYCTRL_PDB_SHIFT));
>
> /*
> - * According to the user manual, it asks driver to
> - * wait 5us for calpad busy trimming
> + * According to the user manual, it asks driver to wait 5us for
> + * calpad busy trimming. However it is documented that this value is
> + * PVT(A.K.A process,voltage and temperature) relevant, so some
> + * failure cases are found which indicates we should be more tolerant
> + * to calpad busy trimming.
> */
> - udelay(5);
> - regmap_read(rk_phy->reg_base,
> - rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> - &caldone);
> - caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
> - if (caldone != PHYCTRL_CALDONE_DONE) {
> - pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
> - return -ETIMEDOUT;
> + ret = regmap_read_poll_timeout(rk_phy->reg_base,
> + rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> + caldone, PHYCTRL_IS_CALDONE(caldone),
> + 5, 50);
>
> + if (ret) {
> + pr_err("%s: caldone timeout, ret=%d\n", __func__, ret);
In Shawn's v2, AKA <https://patchwork.kernel.org/patch/10154795/>,
this says "caldone failed", which I like better since not all failures
will be timeouts.