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;
MIME-Version: 1.0
References: <20230712014812.3337992-1-quic_skakitap@quicinc.com>
In-Reply-To: <20230712014812.3337992-1-quic_skakitap@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Wed, 12 Jul 2023 17:15:47 +0300
Message-ID: <CAA8EJpqvUH4vhqGsnfKSzbV0xLV70GYuJwdejxQNN37XG68Oww@mail.gmail.com>
Subject: Re: [RESEND] clk: qcom: rcg: Update rcg configuration before enabling it
To:     Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
Cc:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Jagadeesh Kona <quic_jkona@quicinc.com>,
        Taniya Das <quic_tdas@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 12 Jul 2023 at 04:49, Satya Priya Kakitapalli
<quic_skakitap@quicinc.com> wrote:
>
> From: Taniya Das <quic_tdas@quicinc.com>
>
> If rcg is in disabled state when clk_rcg2_shared_set_rate is called, the
> new configuration is written to the configuration register but it won't be
> effective in h/w yet because update bit won't be set if rcg is in disabled
> state. Since the new configuration is not yet updated in h/w, dirty bit of
> configuration register will be set in such case. Clear the dirty bit and
> update the rcg to proper new configuration by setting the update bit before
> enabling the rcg.
>
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>

I think there should be your S-o-B too, as you are resending this patch.

Next, should there be any Fixes tags? Probably this is a fix for 703db1f5da1e ?

> ---
> Resending this patch as there is no review for 2 months.
>
>  drivers/clk/qcom/clk-rcg2.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index e22baf3a7112..b25635feb617 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -982,7 +982,13 @@ static int clk_rcg2_set_force_enable(struct clk_hw *hw)
>  {
>         struct clk_rcg2 *rcg = to_clk_rcg2(hw);
>         const char *name = clk_hw_get_name(hw);
> -       int ret, count;
> +       int ret, count, val;
> +
> +       if (!__clk_is_enabled(hw->clk)) {

There is clk_hw_is_enabled(). Can you consider using it instead?

Also, I'd like to point out the traditional source of controversy,
confusion and troubles. The clk_rcg2_shared_ops does not have the
is_enabled callback. Will this code work as expected in this case?

> +               regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, &val);
> +               if (val & CMD_DIRTY_CFG)
> +                       update_config(rcg);
> +       }
>
>         ret = regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG,
>                                  CMD_ROOT_EN, CMD_ROOT_EN);
> --
> 2.25.1
>


-- 
With best wishes
Dmitry