| Message ID | 20241021-alpha-mode-cleanup-v1-1-55df8ed73645@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 10:05:04PM +0200, Gabor Juhos wrote: > 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. Ack. Please mention in the commit message that it's safe to drop the alpha_en bit, because it will get reset by the set_rate function. > > [1] > https://lore.kernel.org/r/20241021-fix-alpha-mode-config-v1-1-f32c254e02bc@gmail.com > > Regards, > Gabor > >
2024. 10. 26. 20:55 keltezéssel, Dmitry Baryshkov írta: > On Fri, Oct 25, 2024 at 10:05:04PM +0200, Gabor Juhos wrote: >> 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. > > Ack. Please mention in the commit message that it's safe to drop the > alpha_en bit, because it will get reset by the set_rate function. Ok.
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),
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. 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(-)