[PATCH 0/5] clk: qcom: remove superfluous alpha settings from PLL configs

[PATCH 0/5] clk: qcom: remove superfluous alpha settings from PLL configs

[Gabor Juhos]

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,
-- 
Gabor Juhos <j4g8y7@gmail.com>

[PATCH 1/5] clk: qcom: apss-ipq-pll: drop 'alpha_en_mask' from IPQ5018 PLL config

[Gabor Juhos]

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(-)

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

Re: [PATCH 1/5] clk: qcom: apss-ipq-pll: drop 'alpha_en_mask' from IPQ5018 PLL config

[Dmitry Baryshkov]

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
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] clk: qcom: apss-ipq-pll: drop 'alpha_en_mask' from IPQ5018 PLL config

[Gabor Juhos]

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

Re: [PATCH 1/5] clk: qcom: apss-ipq-pll: drop 'alpha_en_mask' from IPQ5018 PLL config

[Dmitry Baryshkov]

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
> 
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] clk: qcom: apss-ipq-pll: drop 'alpha_en_mask' from IPQ5018 PLL config

[Gabor Juhos]

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.

[PATCH 2/5] clk: qcom: apps-ipq-pll: drop 'alpha_en_mask' from IPQ5332 PLL config

[Gabor Juhos]

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.

While at it, also add a comment to indicate the frequency the PLL
runs at with the current configuration.

No functional changes, the initial rate of the PLL is the same both
before and after the patch.

Tested on Xiaomi Router BE3600 2.5G (IPQ5312, out-of-tree board).

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/clk/qcom/apss-ipq-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index dec2a5019cc77bf60142a86453883e336afc860f..d6c1aea7e9e1e50a8d7561ce352feac4e76fb1e3 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -79,13 +79,13 @@ static const struct alpha_pll_config ipq5018_pll_config = {
 	.test_ctl_hi_val = 0x00400003,
 };
 
+/* 1.080 GHz configuration */
 static const struct alpha_pll_config ipq5332_pll_config = {
 	.l = 0x2d,
 	.config_ctl_val = 0x4001075b,
 	.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

Re: [PATCH 2/5] clk: qcom: apps-ipq-pll: drop 'alpha_en_mask' from IPQ5332 PLL config

[Dmitry Baryshkov]

On Mon, Oct 21, 2024 at 10:21:58PM +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.
> 
> While at it, also add a comment to indicate the frequency the PLL
> runs at with the current configuration.
> 
> No functional changes, the initial rate of the PLL is the same both
> before and after the patch.
> 
> Tested on Xiaomi Router BE3600 2.5G (IPQ5312, out-of-tree board).
> 
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>  drivers/clk/qcom/apss-ipq-pll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 3/5] clk: qcom: gcc-ipq6018: remove alpha values from NSS Crypto PLL's config

[Gabor Juhos]

Since both the 'alpha' and 'alpha_hi' members of the configuration is
initialized 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
alpha mode is not required to get the desired output rate.

While at it, also add a comment to indicate the frequency the PLL runs
at with the current configuration.

No functional changes, the PLL runs at 1.2 GHz both before and after
the change.

Tested on Xiaomi Mi Router AX1800 (IPQ6018, out-of-tree board).

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/clk/qcom/gcc-ipq6018.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c
index ab0f7fc665a9790dd8edba0cf4b86c5c672a337d..d861191b0c85ccc105ac0e62d7a68210c621fc13 100644
--- a/drivers/clk/qcom/gcc-ipq6018.c
+++ b/drivers/clk/qcom/gcc-ipq6018.c
@@ -4194,10 +4194,9 @@ static const struct alpha_pll_config ubi32_pll_config = {
 	.test_ctl_hi_val = 0x4000,
 };
 
+/* 1200 MHz configuration */
 static const struct alpha_pll_config nss_crypto_pll_config = {
 	.l = 0x32,
-	.alpha = 0x0,
-	.alpha_hi = 0x0,
 	.config_ctl_val = 0x4001055b,
 	.main_output_mask = BIT(0),
 	.pre_div_val = 0x0,
@@ -4206,7 +4205,6 @@ static const struct alpha_pll_config nss_crypto_pll_config = {
 	.post_div_mask = GENMASK(11, 8),
 	.vco_mask = GENMASK(21, 20),
 	.vco_val = 0x0,
-	.alpha_en_mask = BIT(24),
 };
 
 static struct clk_hw *gcc_ipq6018_hws[] = {

-- 
2.47.0

Re: [PATCH 3/5] clk: qcom: gcc-ipq6018: remove alpha values from NSS Crypto PLL's config

[Dmitry Baryshkov]

On Mon, Oct 21, 2024 at 10:21:59PM +0200, Gabor Juhos wrote:
> Since both the 'alpha' and 'alpha_hi' members of the configuration is
> initialized 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
> alpha mode is not required to get the desired output rate.
> 
> While at it, also add a comment to indicate the frequency the PLL runs
> at with the current configuration.
> 
> No functional changes, the PLL runs at 1.2 GHz both before and after
> the change.
> 
> Tested on Xiaomi Mi Router AX1800 (IPQ6018, out-of-tree board).
> 
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>  drivers/clk/qcom/gcc-ipq6018.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c
> index ab0f7fc665a9790dd8edba0cf4b86c5c672a337d..d861191b0c85ccc105ac0e62d7a68210c621fc13 100644
> --- a/drivers/clk/qcom/gcc-ipq6018.c
> +++ b/drivers/clk/qcom/gcc-ipq6018.c
> @@ -4194,10 +4194,9 @@ static const struct alpha_pll_config ubi32_pll_config = {
>  	.test_ctl_hi_val = 0x4000,
>  };
>  
> +/* 1200 MHz configuration */
>  static const struct alpha_pll_config nss_crypto_pll_config = {
>  	.l = 0x32,
> -	.alpha = 0x0,
> -	.alpha_hi = 0x0,

I'd say this serves documentation purposes: zero alpha value

>  	.config_ctl_val = 0x4001055b,
>  	.main_output_mask = BIT(0),
>  	.pre_div_val = 0x0,
> @@ -4206,7 +4205,6 @@ static const struct alpha_pll_config nss_crypto_pll_config = {
>  	.post_div_mask = GENMASK(11, 8),
>  	.vco_mask = GENMASK(21, 20),
>  	.vco_val = 0x0,
> -	.alpha_en_mask = BIT(24),

This is okay

>  };
>  
>  static struct clk_hw *gcc_ipq6018_hws[] = {
> 
> -- 
> 2.47.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/5] clk: qcom: gcc-ipq6018: remove alpha values from NSS Crypto PLL's config

[Gabor Juhos]

2024. 10. 25. 8:18 keltezéssel, Dmitry Baryshkov írta:
> On Mon, Oct 21, 2024 at 10:21:59PM +0200, Gabor Juhos wrote:
>> Since both the 'alpha' and 'alpha_hi' members of the configuration is
>> initialized 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
>> alpha mode is not required to get the desired output rate.
>>
>> While at it, also add a comment to indicate the frequency the PLL runs
>> at with the current configuration.
>>
>> No functional changes, the PLL runs at 1.2 GHz both before and after
>> the change.
>>
>> Tested on Xiaomi Mi Router AX1800 (IPQ6018, out-of-tree board).
>>
>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>> ---
>>  drivers/clk/qcom/gcc-ipq6018.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c
>> index ab0f7fc665a9790dd8edba0cf4b86c5c672a337d..d861191b0c85ccc105ac0e62d7a68210c621fc13 100644
>> --- a/drivers/clk/qcom/gcc-ipq6018.c
>> +++ b/drivers/clk/qcom/gcc-ipq6018.c
>> @@ -4194,10 +4194,9 @@ static const struct alpha_pll_config ubi32_pll_config = {
>>  	.test_ctl_hi_val = 0x4000,
>>  };
>>  
>> +/* 1200 MHz configuration */
>>  static const struct alpha_pll_config nss_crypto_pll_config = {
>>  	.l = 0x32,
>> -	.alpha = 0x0,
>> -	.alpha_hi = 0x0,
> 
> I'd say this serves documentation purposes: zero alpha value

For me, setting 'alpha_en_mask' means that the alpha values will be used. If it
is not set, then it means that the alpha values are getting ignored. If those
will be ignored eventually, specifying even zero alpha values explicitly is
pointless in my opinion.

If we really need that for documentation purposes, the comment before the
configuration can be changed to indicate that alpha values are not needed.

> 
>>  	.config_ctl_val = 0x4001055b,
>>  	.main_output_mask = BIT(0),
>>  	.pre_div_val = 0x0,
>> @@ -4206,7 +4205,6 @@ static const struct alpha_pll_config nss_crypto_pll_config = {
>>  	.post_div_mask = GENMASK(11, 8),
>>  	.vco_mask = GENMASK(21, 20),
>>  	.vco_val = 0x0,
>> -	.alpha_en_mask = BIT(24),
> 
> This is okay

Regards,
Gabor

Re: [PATCH 3/5] clk: qcom: gcc-ipq6018: remove alpha values from NSS Crypto PLL's config

[Dmitry Baryshkov]

On Fri, Oct 25, 2024 at 10:06:55PM +0200, Gabor Juhos wrote:
> 2024. 10. 25. 8:18 keltezéssel, Dmitry Baryshkov írta:
> > On Mon, Oct 21, 2024 at 10:21:59PM +0200, Gabor Juhos wrote:
> >> Since both the 'alpha' and 'alpha_hi' members of the configuration is
> >> initialized 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
> >> alpha mode is not required to get the desired output rate.
> >>
> >> While at it, also add a comment to indicate the frequency the PLL runs
> >> at with the current configuration.
> >>
> >> No functional changes, the PLL runs at 1.2 GHz both before and after
> >> the change.
> >>
> >> Tested on Xiaomi Mi Router AX1800 (IPQ6018, out-of-tree board).
> >>
> >> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> >> ---
> >>  drivers/clk/qcom/gcc-ipq6018.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c
> >> index ab0f7fc665a9790dd8edba0cf4b86c5c672a337d..d861191b0c85ccc105ac0e62d7a68210c621fc13 100644
> >> --- a/drivers/clk/qcom/gcc-ipq6018.c
> >> +++ b/drivers/clk/qcom/gcc-ipq6018.c
> >> @@ -4194,10 +4194,9 @@ static const struct alpha_pll_config ubi32_pll_config = {
> >>  	.test_ctl_hi_val = 0x4000,
> >>  };
> >>  
> >> +/* 1200 MHz configuration */
> >>  static const struct alpha_pll_config nss_crypto_pll_config = {
> >>  	.l = 0x32,
> >> -	.alpha = 0x0,
> >> -	.alpha_hi = 0x0,
> > 
> > I'd say this serves documentation purposes: zero alpha value
> 
> For me, setting 'alpha_en_mask' means that the alpha values will be used. If it
> is not set, then it means that the alpha values are getting ignored. If those
> will be ignored eventually, specifying even zero alpha values explicitly is
> pointless in my opinion.

Ack, it matches the behaviour by several other CLK drivers.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> 
> If we really need that for documentation purposes, the comment before the
> configuration can be changed to indicate that alpha values are not needed.
> 
> > 
> >>  	.config_ctl_val = 0x4001055b,
> >>  	.main_output_mask = BIT(0),
> >>  	.pre_div_val = 0x0,
> >> @@ -4206,7 +4205,6 @@ static const struct alpha_pll_config nss_crypto_pll_config = {
> >>  	.post_div_mask = GENMASK(11, 8),
> >>  	.vco_mask = GENMASK(21, 20),
> >>  	.vco_val = 0x0,
> >> -	.alpha_en_mask = BIT(24),
> > 
> > This is okay
> 
> Regards,
> Gabor

-- 
With best wishes
Dmitry

[PATCH 4/5] clk: qcom: dispcc-qcm2290: remove alpha values from disp_cc_pll0_config

[Gabor Juhos]

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-qcm2290.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
index 449ffea2295d3760f40abe8b1195e9022f46a9b0..d7bb1399e1022afc68e45ee335d615d4a5be5add 100644
--- a/drivers/clk/qcom/dispcc-qcm2290.c
+++ b/drivers/clk/qcom/dispcc-qcm2290.c
@@ -40,8 +40,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),
 	.vco_val = 0x2 << 20,
 	.vco_mask = GENMASK(21, 20),
 	.main_output_mask = BIT(0),

-- 
2.47.0

Re: [PATCH 4/5] clk: qcom: dispcc-qcm2290: remove alpha values from disp_cc_pll0_config

[Dmitry Baryshkov]

On Mon, Oct 21, 2024 at 10:22:00PM +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-qcm2290.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
> index 449ffea2295d3760f40abe8b1195e9022f46a9b0..d7bb1399e1022afc68e45ee335d615d4a5be5add 100644
> --- a/drivers/clk/qcom/dispcc-qcm2290.c
> +++ b/drivers/clk/qcom/dispcc-qcm2290.c
> @@ -40,8 +40,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 pll isn't fixed rate.

>  	.vco_val = 0x2 << 20,
>  	.vco_mask = GENMASK(21, 20),
>  	.main_output_mask = BIT(0),
> 
> -- 
> 2.47.0
> 

-- 
With best wishes
Dmitry

[PATCH 5/5] clk: qcom: dispcc-sm6115: remove alpha values from disp_cc_pll0_config

[Gabor Juhos]

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),
 	.vco_val = 0x2 << 20,
 	.vco_mask = GENMASK(21, 20),
 	.main_output_mask = BIT(0),

-- 
2.47.0

Re: [PATCH 5/5] clk: qcom: dispcc-sm6115: remove alpha values from disp_cc_pll0_config

[Dmitry Baryshkov]

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.

>  	.vco_val = 0x2 << 20,
>  	.vco_mask = GENMASK(21, 20),
>  	.main_output_mask = BIT(0),
> 
> -- 
> 2.47.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/5] clk: qcom: dispcc-sm6115: remove alpha values from disp_cc_pll0_config

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry