| Message ID | 20241021-alpha-mode-cleanup-v1-0-55df8ed73645@gmail.com |
|---|---|
| Headers | show |
| Series | clk: qcom: remove superfluous alpha settings from PLL configs | expand |
On Mon, Oct 21, 2024 at 10:21:57PM +0200, Gabor Juhos wrote: > Since neither 'alpha' nor 'alpha_hi' is defined in the configuration, > those will be initialized with zero values implicitly. By using zero > alpha values, the output rate of the PLL will be the same whether > alpha mode is enabled or not. > > Remove the superfluous initialization of the 'alpha_en_mask' member > to make it clear that enabling alpha mode is not required to get the > desired output rate. > > No functional changes, the initial rate of the PLL is the same both > before and after the patch. After going through DISPCC changes, I think the whole series is incorrect: these PLL can change the rate (e.g. to facilitate CPU frequency changes). Normally PLL ops do not check the alpha_en bit when changing the rate, so the driver might try to set the PLL to the rate which requires alpha value, while the alpha_en bit isn't set. > > Tested on TP-Link Archer AX55 v1 (IPQ5018). > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > --- > drivers/clk/qcom/apss-ipq-pll.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c > index e8632db2c542806e9527a22b54fe169e3e398a7a..dec2a5019cc77bf60142a86453883e336afc860f 100644 > --- a/drivers/clk/qcom/apss-ipq-pll.c > +++ b/drivers/clk/qcom/apss-ipq-pll.c > @@ -73,7 +73,6 @@ static const struct alpha_pll_config ipq5018_pll_config = { > .main_output_mask = BIT(0), > .aux_output_mask = BIT(1), > .early_output_mask = BIT(3), > - .alpha_en_mask = BIT(24), > .status_val = 0x3, > .status_mask = GENMASK(10, 8), > .lock_det = BIT(2), > > -- > 2.47.0 >
2024. 10. 25. 8:24 keltezéssel, Dmitry Baryshkov írta: > On Mon, Oct 21, 2024 at 10:21:57PM +0200, Gabor Juhos wrote: >> Since neither 'alpha' nor 'alpha_hi' is defined in the configuration, >> those will be initialized with zero values implicitly. By using zero >> alpha values, the output rate of the PLL will be the same whether >> alpha mode is enabled or not. >> >> Remove the superfluous initialization of the 'alpha_en_mask' member >> to make it clear that enabling alpha mode is not required to get the >> desired output rate. >> >> No functional changes, the initial rate of the PLL is the same both >> before and after the patch. > > After going through DISPCC changes, I think the whole series is > incorrect: these PLL can change the rate (e.g. to facilitate CPU > frequency changes). Normally PLL ops do not check the alpha_en bit when > changing the rate, so the driver might try to set the PLL to the rate > which requires alpha value, while the alpha_en bit isn't set. Both clk_alpha_pll_stromer_set_rate() which is used for IPQ5018 (patch 1), and clk_alpha_pll_stromer_plus_set_rate() used for IPQ5332 (patch 2) sets the ALPHA_EN bit unconditionally. For the PLLs affected by the remaining patches, clk_alpha_pll_set_rate() is used which also unconditionally sets the ALPHA_EN bit via __clk_alpha_pll_set_rate(). I have created the patches after analysing the side effects of [1]. Due to the bug described in that change, the clk_alpha_pll_configure() function in the current kernel never sets the ALPHA_EN bit in the USER_CTL register. This means that setting 'alpha_en_mask' in the configurations has no effect actually. So, if we assume that the affected PLLs are working correctly now, it is not because the 'alpha_en_mask' is specifed in the configuration but due to the fact that the set_rate op sets the ALPHA_EN bit. At least, I came to this after the analysis. [1] https://lore.kernel.org/r/20241021-fix-alpha-mode-config-v1-1-f32c254e02bc@gmail.com Regards, Gabor
On Fri, Oct 25, 2024 at 09:20:37AM +0300, Dmitry Baryshkov wrote: > On Mon, Oct 21, 2024 at 10:22:01PM +0200, Gabor Juhos wrote: > > Since both the 'alpha' and 'alpha_hi' members of the configuration is > > initialized (the latter is implicitly) with zero values, the output > > rate of the PLL will be the same whether alpha mode is enabled or not. > > > > Remove the initialization of the alpha* members to make it clear that > > the alpha mode is not required to get the desired output rate. > > > > No functional changes intended, compile tested only. > > > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > > --- > > drivers/clk/qcom/dispcc-sm6115.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/clk/qcom/dispcc-sm6115.c b/drivers/clk/qcom/dispcc-sm6115.c > > index 939887f82ecc3da21a5f26168c3161aa8cfeb3cb..2b236d52b29fe72b8979da85c8bd4bfd1db54c0b 100644 > > --- a/drivers/clk/qcom/dispcc-sm6115.c > > +++ b/drivers/clk/qcom/dispcc-sm6115.c > > @@ -48,8 +48,6 @@ static const struct pll_vco spark_vco[] = { > > /* 768MHz configuration */ > > static const struct alpha_pll_config disp_cc_pll0_config = { > > .l = 0x28, > > - .alpha = 0x0, > > - .alpha_en_mask = BIT(24), > > NAK, this isn't a fixed rate PLL. clk_alpha_pll_set_rate() hadnles this bit. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > .vco_val = 0x2 << 20, > > .vco_mask = GENMASK(21, 20), > > .main_output_mask = BIT(0), > > > > -- > > 2.47.0 > > > > -- > With best wishes > Dmitry
There are several alpha PLL configurations, where the alpha mode is enabled even if it is not strictly required in order to get the desired output rate of the PLL. This small series removes those superfluous settings. Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> --- Gabor Juhos (5): clk: qcom: apss-ipq-pll: drop 'alpha_en_mask' from IPQ5018 PLL config clk: qcom: apps-ipq-pll: drop 'alpha_en_mask' from IPQ5332 PLL config clk: qcom: gcc-ipq6018: remove alpha values from NSS Crypto PLL's config clk: qcom: dispcc-qcm2290: remove alpha values from disp_cc_pll0_config clk: qcom: dispcc-sm6115: remove alpha values from disp_cc_pll0_config drivers/clk/qcom/apss-ipq-pll.c | 3 +-- drivers/clk/qcom/dispcc-qcm2290.c | 2 -- drivers/clk/qcom/dispcc-sm6115.c | 2 -- drivers/clk/qcom/gcc-ipq6018.c | 4 +--- 4 files changed, 2 insertions(+), 9 deletions(-) --- base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc change-id: 20241021-alpha-mode-cleanup-661b544644d7 Best regards,