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;
Date:   Mon, 12 Jun 2023 12:17:53 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Konrad Dybcio <konrad.dybcio@linaro.org>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] clk: qcom: gcc-msm8996: Use read-only RCG ops for
 RPM bus clocks
Message-ID: <ZIbw0dEdFNFcC0Hr@gerhold.net>
References: <20230612-topic-rcg2_ro-v1-0-e7d824aeb628@linaro.org>
 <20230612-topic-rcg2_ro-v1-2-e7d824aeb628@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230612-topic-rcg2_ro-v1-2-e7d824aeb628@linaro.org>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, Jun 12, 2023 at 11:22:48AM +0200, Konrad Dybcio wrote:
> The config/periph/system NoC clocks are wholly controlled by the
> RPM firmware and Linux should never ever alter their configuration.
> 
> Switch them over to read-only ops to avoid that.
> 
> Fixes: b1e010c0730a ("clk: qcom: Add MSM8996 Global Clock Control (GCC) driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/clk/qcom/gcc-msm8996.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
> index 5e44d1bcca9e..588e3b67657a 100644
> --- a/drivers/clk/qcom/gcc-msm8996.c
> +++ b/drivers/clk/qcom/gcc-msm8996.c
> @@ -264,7 +264,7 @@ static struct clk_rcg2 system_noc_clk_src = {
>  		.name = "system_noc_clk_src",
>  		.parent_data = gcc_xo_gpll0_gpll0_early_div,
>  		.num_parents = ARRAY_SIZE(gcc_xo_gpll0_gpll0_early_div),
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_ro_ops,
>  	},
>  };
>  
> @@ -284,7 +284,7 @@ static struct clk_rcg2 config_noc_clk_src = {
>  		.name = "config_noc_clk_src",
>  		.parent_data = gcc_xo_gpll0,
>  		.num_parents = ARRAY_SIZE(gcc_xo_gpll0),
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_ro_ops,
>  	},
>  };
>  
> @@ -306,7 +306,7 @@ static struct clk_rcg2 periph_noc_clk_src = {
>  		.name = "periph_noc_clk_src",
>  		.parent_data = gcc_xo_gpll0,
>  		.num_parents = ARRAY_SIZE(gcc_xo_gpll0),
> -		.ops = &clk_rcg2_ops,
> +		.ops = &clk_rcg2_ro_ops,
>  	},
>  };
>  

If the rcg is read-only then it shouldn't need a .freq_tbl (the
.recalc_rate() works without it). These clocks have one for some reason.

Maybe we should also set .flags = CLK_GET_RATE_NOCACHE to ensure that
clk_get_rate() doesn't return some stale value? Although I'm not sure if
it's enough to set it here or if it would need to be set on all child
clocks as well.

Thanks,
Stephan