lkml.org 
[lkml]   [2018]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 1/2] phy: rockchip-emmc: retry calpad busy trimming
    Hi,

    This seems like a good idea to me. The fact that there was no polling
    loop here always seemed strange to me, but that's how the original
    code was structured and I personally never saw any problems with it.

    On Wed, Jan 10, 2018 at 2:49 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
    > 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 <shawn.lin@rock-chips.com>
    > Reviewed-by: Brian Norris <briannorris@chromium.org>
    > Tested-by: Caesar Wang <wxt@rock-chips.com>
    > Tested-by: Ziyuan Xu <xzy.xu@rock-chips.com>
    > ---
    >
    > Changes in v2:
    > - propagate the error and print it
    >
    > 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..547b746 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);

    See comments in part 2 of this series, AKA
    <https://patchwork.kernel.org/patch/10154797/>. I think this should
    be "0, 50", not "5, 50". ...or, if you insist, "10, 50"


    > + if (ret) {
    > + pr_err("%s: caldone failed %d.\n", __func__, ret);

    I like this v2 slightly better than Caesar's v2 because you changed
    the word "timeout" to "failed".

    \
     
     \ /
      Last update: 2018-01-14 23:22    [W:2.410 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site