Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20220321231548.14276-5-ansuelsmth@gmail.com>
References: <20220321231548.14276-1-ansuelsmth@gmail.com> <20220321231548.14276-5-ansuelsmth@gmail.com>
Subject: Re: [PATCH v6 04/18] clk: qcom: clk-hfpll: use poll_timeout macro
From:   Stephen Boyd <sboyd@kernel.org>
To:     Andy Gross <agross@kernel.org>,
        Ansuel Smith <ansuelsmth@gmail.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Thu, 24 Mar 2022 18:09:08 -0700
User-Agent: alot/0.10
Message-Id: <20220325010910.77277C340ED@smtp.kernel.org>
Precedence: bulk

Quoting Ansuel Smith (2022-03-21 16:15:34)
> Use regmap_read_poll_timeout macro instead of do-while structure.

Please add a note that this isn't an equivalent translation. Now we
timeout if the PLL fails to lock whereas before we would loop
potentially forever.

> diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
> index e847d586a73a..a4e347eb4d4d 100644
> --- a/drivers/clk/qcom/clk-hfpll.c
> +++ b/drivers/clk/qcom/clk-hfpll.c
> @@ -12,6 +12,8 @@
>  #include "clk-regmap.h"
>  #include "clk-hfpll.h"
> =20
> +#define HFPLL_BUSY_WAIT_TIMEOUT        100

We don't really need a define for this.

> +
>  #define PLL_OUTCTRL    BIT(0)
>  #define PLL_BYPASSNL   BIT(1)
>  #define PLL_RESET_N    BIT(2)
> @@ -72,13 +74,12 @@ static void __clk_hfpll_enable(struct clk_hw *hw)
>         regmap_update_bits(regmap, hd->mode_reg, PLL_RESET_N, PLL_RESET_N=
);
> =20
>         /* Wait for PLL to lock. */
> -       if (hd->status_reg) {
> -               do {
> -                       regmap_read(regmap, hd->status_reg, &val);
> -               } while (!(val & BIT(hd->lock_bit)));
> -       } else {
> +       if (hd->status_reg)
> +               regmap_read_poll_timeout(regmap, hd->status_reg, val,
> +                                        !(val & BIT(hd->lock_bit)), USEC=
_PER_MSEC * 2,

Why are we waiting between reads?

> +                                        HFPLL_BUSY_WAIT_TIMEOUT * USEC_P=
ER_MSEC);
> +       else
>                 udelay(60);
> -       }
>