[PATCH v2 0/6] clk: qcom: apss-ipq-pll: various cleanups

[PATCH v2 0/6] clk: qcom: apss-ipq-pll: various cleanups

[Gabor Juhos]

This series contains a few patches to perform some cleanup in the
apss-ipq-pll driver.

The set is based on v6.8 and it depends on the following patches:
  - "clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure"
     Link: https://lore.kernel.org/r/20240315-apss-ipq-pll-ipq5018-hang-v2-1-6fe30ada2009@gmail.com
  - "clk: qcom: clk-alpha-pll: Stromer register cleanup"
     Link: https://lore.kernel.org/r/20240311-alpha-pll-stromer-cleanup-v1-0-f7c0c5607cca@gmail.com

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
  - add a new patch at the end to remove the 'cbf_pll_regs' register map
    from clk-cbf-8996.c
  - change patch 2 to move huayra register map to clk-alpha-pll.c
  - collect Reviewed-by tags
  - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-0-52f795429d5d@gmail.com

---
Gabor Juhos (6):
      clk: qcom: apss-ipq-pll: reuse Stromer reg offsets from 'clk_alpha_pll_regs'
      clk: qcom: apss-ipq-pll: move Huayra register map to 'clk_alpha_pll_regs'
      clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'
      clk: qcom: apss-ipq-pll: constify match data structures
      clk: qcom: apss-ipq-pll: constify clk_init_data structures
      clk: qcom: clk-cbf-8996: use HUAYRA_APPS register map for cbf_pll

 drivers/clk/qcom/apss-ipq-pll.c  | 68 ++++++++++------------------------------
 drivers/clk/qcom/clk-alpha-pll.c | 10 ++++++
 drivers/clk/qcom/clk-alpha-pll.h |  1 +
 drivers/clk/qcom/clk-cbf-8996.c  | 13 +-------
 4 files changed, 28 insertions(+), 64 deletions(-)
---
base-commit: 9b65bf93985e8bd5bc5476beef59ba56f3f99697
change-id: 20240315-apss-ipq-pll-cleanup-a1e99af9d854

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

[PATCH v2 1/6] clk: qcom: apss-ipq-pll: reuse Stromer reg offsets from 'clk_alpha_pll_regs'

[Gabor Juhos]

The register offset array defined locally for the
CLK_ALPHA_PLL_TYPE_STROMER_PLUS is the same as the
entry defined for CLK_ALPHA_PLL_TYPE_STROMER in the
'clk_alpha_pll_regs' array.

To avoid code duplication, remove the local definition
and use the global one instead.

No functional changes.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
 - add Reviewed-by tag from Dmitry
 - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-1-52f795429d5d@gmail.com

Depends on the following patches:
 - "clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure"
   Link: https://lore.kernel.org/r/20240315-apss-ipq-pll-ipq5018-hang-v2-1-6fe30ada2009@gmail.com
 - "clk: qcom: clk-alpha-pll: Stromer register cleanup"
   Link: https://lore.kernel.org/r/20240311-alpha-pll-stromer-cleanup-v1-0-f7c0c5607cca@gmail.com
---
 drivers/clk/qcom/apss-ipq-pll.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index dfffec2f06ae7..ed3e6405f99cb 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -24,17 +24,6 @@ static const u8 ipq_pll_offsets[][PLL_OFF_MAX_REGS] = {
 		[PLL_OFF_TEST_CTL] = 0x30,
 		[PLL_OFF_TEST_CTL_U] = 0x34,
 	},
-	[CLK_ALPHA_PLL_TYPE_STROMER_PLUS] = {
-		[PLL_OFF_L_VAL] = 0x08,
-		[PLL_OFF_ALPHA_VAL] = 0x10,
-		[PLL_OFF_ALPHA_VAL_U] = 0x14,
-		[PLL_OFF_USER_CTL] = 0x18,
-		[PLL_OFF_USER_CTL_U] = 0x1c,
-		[PLL_OFF_CONFIG_CTL] = 0x20,
-		[PLL_OFF_STATUS] = 0x28,
-		[PLL_OFF_TEST_CTL] = 0x30,
-		[PLL_OFF_TEST_CTL_U] = 0x34,
-	},
 };
 
 static struct clk_alpha_pll ipq_pll_huayra = {
@@ -57,12 +46,7 @@ static struct clk_alpha_pll ipq_pll_huayra = {
 
 static struct clk_alpha_pll ipq_pll_stromer = {
 	.offset = 0x0,
-	/*
-	 * Reuse CLK_ALPHA_PLL_TYPE_STROMER_PLUS register offsets.
-	 * Although this is a bit confusing, but the offset values
-	 * are correct nevertheless.
-	 */
-	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS],
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER],
 	.flags = SUPPORTS_DYNAMIC_UPDATE,
 	.clkr = {
 		.enable_reg = 0x0,
@@ -80,7 +64,11 @@ static struct clk_alpha_pll ipq_pll_stromer = {
 
 static struct clk_alpha_pll ipq_pll_stromer_plus = {
 	.offset = 0x0,
-	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS],
+	/*
+	 * The register offsets of the Stromer Plus PLL used in IPQ5332
+	 * are the same as the Stromer PLL's offsets.
+	 */
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER],
 	.flags = SUPPORTS_DYNAMIC_UPDATE,
 	.clkr = {
 		.enable_reg = 0x0,

-- 
2.44.0

Re: [PATCH v2 1/6] clk: qcom: apss-ipq-pll: reuse Stromer reg offsets from 'clk_alpha_pll_regs'

[Konrad Dybcio]

On 21.03.2024 08:49, Gabor Juhos wrote:
> The register offset array defined locally for the
> CLK_ALPHA_PLL_TYPE_STROMER_PLUS is the same as the
> entry defined for CLK_ALPHA_PLL_TYPE_STROMER in the
> 'clk_alpha_pll_regs' array.
> 
> To avoid code duplication, remove the local definition
> and use the global one instead.
> 
> No functional changes.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 2/6] clk: qcom: apss-ipq-pll: move Huayra register map to 'clk_alpha_pll_regs'

[Gabor Juhos]

Move the locally defined Huayra register map to 'clk_alpha_pll_regs'
in order to allow using that by other drivers, like the clk-cbf-8996.

No functional changes.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
 - rework the patch as requested by Dmitry Baryshkov by moving the register
   map into clk-alpha-pll.c instead of keeping that locally
 - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-2-52f795429d5d@gmail.com
---
 drivers/clk/qcom/apss-ipq-pll.c  | 20 +-------------------
 drivers/clk/qcom/clk-alpha-pll.c | 10 ++++++++++
 drivers/clk/qcom/clk-alpha-pll.h |  1 +
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index ed3e6405f99cb..8cf17374a2e2a 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -8,27 +8,9 @@
 
 #include "clk-alpha-pll.h"
 
-/*
- * Even though APSS PLL type is of existing one (like Huayra), its offsets
- * are different from the one mentioned in the clk-alpha-pll.c, since the
- * PLL is specific to APSS, so lets the define the same.
- */
-static const u8 ipq_pll_offsets[][PLL_OFF_MAX_REGS] = {
-	[CLK_ALPHA_PLL_TYPE_HUAYRA] =  {
-		[PLL_OFF_L_VAL] = 0x08,
-		[PLL_OFF_ALPHA_VAL] = 0x10,
-		[PLL_OFF_USER_CTL] = 0x18,
-		[PLL_OFF_CONFIG_CTL] = 0x20,
-		[PLL_OFF_CONFIG_CTL_U] = 0x24,
-		[PLL_OFF_STATUS] = 0x28,
-		[PLL_OFF_TEST_CTL] = 0x30,
-		[PLL_OFF_TEST_CTL_U] = 0x34,
-	},
-};
-
 static struct clk_alpha_pll ipq_pll_huayra = {
 	.offset = 0x0,
-	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_HUAYRA],
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_HUAYRA_APSS],
 	.flags = SUPPORTS_DYNAMIC_UPDATE,
 	.clkr = {
 		.enable_reg = 0x0,
diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 55a77e36dc44c..ba039b2b24a09 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -82,6 +82,16 @@ const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
 		[PLL_OFF_TEST_CTL_U] = 0x20,
 		[PLL_OFF_STATUS] = 0x24,
 	},
+	[CLK_ALPHA_PLL_TYPE_HUAYRA_APSS] =  {
+		[PLL_OFF_L_VAL] = 0x08,
+		[PLL_OFF_ALPHA_VAL] = 0x10,
+		[PLL_OFF_USER_CTL] = 0x18,
+		[PLL_OFF_CONFIG_CTL] = 0x20,
+		[PLL_OFF_CONFIG_CTL_U] = 0x24,
+		[PLL_OFF_STATUS] = 0x28,
+		[PLL_OFF_TEST_CTL] = 0x30,
+		[PLL_OFF_TEST_CTL_U] = 0x34,
+	},
 	[CLK_ALPHA_PLL_TYPE_BRAMMO] =  {
 		[PLL_OFF_L_VAL] = 0x04,
 		[PLL_OFF_ALPHA_VAL] = 0x08,
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index a1a75bb12fe88..d057955138952 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -15,6 +15,7 @@
 enum {
 	CLK_ALPHA_PLL_TYPE_DEFAULT,
 	CLK_ALPHA_PLL_TYPE_HUAYRA,
+	CLK_ALPHA_PLL_TYPE_HUAYRA_APSS,
 	CLK_ALPHA_PLL_TYPE_BRAMMO,
 	CLK_ALPHA_PLL_TYPE_FABIA,
 	CLK_ALPHA_PLL_TYPE_TRION,

-- 
2.44.0

Re: [PATCH v2 2/6] clk: qcom: apss-ipq-pll: move Huayra register map to 'clk_alpha_pll_regs'

[Dmitry Baryshkov]

On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> Move the locally defined Huayra register map to 'clk_alpha_pll_regs'
> in order to allow using that by other drivers, like the clk-cbf-8996.
>
> No functional changes.
>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Changes in v2:
>  - rework the patch as requested by Dmitry Baryshkov by moving the register
>    map into clk-alpha-pll.c instead of keeping that locally
>  - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-2-52f795429d5d@gmail.com
> ---
>  drivers/clk/qcom/apss-ipq-pll.c  | 20 +-------------------
>  drivers/clk/qcom/clk-alpha-pll.c | 10 ++++++++++
>  drivers/clk/qcom/clk-alpha-pll.h |  1 +
>  3 files changed, 12 insertions(+), 19 deletions(-)

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


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/6] clk: qcom: apss-ipq-pll: move Huayra register map to 'clk_alpha_pll_regs'

[Konrad Dybcio]

On 21.03.2024 08:49, Gabor Juhos wrote:
> Move the locally defined Huayra register map to 'clk_alpha_pll_regs'
> in order to allow using that by other drivers, like the clk-cbf-8996.
> 
> No functional changes.
> 
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Gabor Juhos]

The value of the 'pll_type' field of 'struct apps_pll_data'
is used only by the probe function to decide which config
function should be called for the actual PLL. However this
can be derived also from the 'pll' field  which makes the
'pll_type' field redundant.

Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values
are meant to be used as indices to the 'clk_alpha_pll_regs'
array so using those to define the pll type in this driver
is misleading anyway.

Change the probe function to use the 'pll' field to determine
the configuration function to be used, and remove the
'pll_type' field to simplify the code.

No functional changes.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
 - no changes
 - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-3-52f795429d5d@gmail.com
---
 drivers/clk/qcom/apss-ipq-pll.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index 8cf17374a2e2a..816b0d1f8d8c8 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -131,37 +131,31 @@ static const struct alpha_pll_config ipq9574_pll_config = {
 };
 
 struct apss_pll_data {
-	int pll_type;
 	struct clk_alpha_pll *pll;
 	const struct alpha_pll_config *pll_config;
 };
 
 static const struct apss_pll_data ipq5018_pll_data = {
-	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER,
 	.pll = &ipq_pll_stromer,
 	.pll_config = &ipq5018_pll_config,
 };
 
 static struct apss_pll_data ipq5332_pll_data = {
-	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS,
 	.pll = &ipq_pll_stromer_plus,
 	.pll_config = &ipq5332_pll_config,
 };
 
 static struct apss_pll_data ipq8074_pll_data = {
-	.pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA,
 	.pll = &ipq_pll_huayra,
 	.pll_config = &ipq8074_pll_config,
 };
 
 static struct apss_pll_data ipq6018_pll_data = {
-	.pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA,
 	.pll = &ipq_pll_huayra,
 	.pll_config = &ipq6018_pll_config,
 };
 
 static struct apss_pll_data ipq9574_pll_data = {
-	.pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA,
 	.pll = &ipq_pll_huayra,
 	.pll_config = &ipq9574_pll_config,
 };
@@ -194,10 +188,10 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENODEV;
 
-	if (data->pll_type == CLK_ALPHA_PLL_TYPE_HUAYRA)
+	if (data->pll == &ipq_pll_huayra)
 		clk_alpha_pll_configure(data->pll, regmap, data->pll_config);
-	else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER ||
-		 data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS)
+	else if (data->pll == &ipq_pll_stromer ||
+		 data->pll == &ipq_pll_stromer_plus)
 		clk_stromer_pll_configure(data->pll, regmap, data->pll_config);
 
 	ret = devm_clk_register_regmap(dev, &data->pll->clkr);

-- 
2.44.0

Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Dmitry Baryshkov]

On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The value of the 'pll_type' field of 'struct apps_pll_data'
> is used only by the probe function to decide which config
> function should be called for the actual PLL. However this
> can be derived also from the 'pll' field  which makes the
> 'pll_type' field redundant.
>
> Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values
> are meant to be used as indices to the 'clk_alpha_pll_regs'
> array so using those to define the pll type in this driver
> is misleading anyway.
>
> Change the probe function to use the 'pll' field to determine
> the configuration function to be used, and remove the
> 'pll_type' field to simplify the code.

I can't fully appreciate this idea. There can be cases when different
PLL types share the set of ops. I think having a type is more
versatile and makes the code more obvious.

>
> No functional changes.
>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Changes in v2:
>  - no changes
>  - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-3-52f795429d5d@gmail.com
> ---
>  drivers/clk/qcom/apss-ipq-pll.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index 8cf17374a2e2a..816b0d1f8d8c8 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -131,37 +131,31 @@ static const struct alpha_pll_config ipq9574_pll_config = {
>  };
>
>  struct apss_pll_data {
> -       int pll_type;
>         struct clk_alpha_pll *pll;
>         const struct alpha_pll_config *pll_config;
>  };
>
>  static const struct apss_pll_data ipq5018_pll_data = {
> -       .pll_type = CLK_ALPHA_PLL_TYPE_STROMER,
>         .pll = &ipq_pll_stromer,
>         .pll_config = &ipq5018_pll_config,
>  };
>
>  static struct apss_pll_data ipq5332_pll_data = {
> -       .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS,
>         .pll = &ipq_pll_stromer_plus,
>         .pll_config = &ipq5332_pll_config,
>  };
>
>  static struct apss_pll_data ipq8074_pll_data = {
> -       .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA,
>         .pll = &ipq_pll_huayra,
>         .pll_config = &ipq8074_pll_config,
>  };
>
>  static struct apss_pll_data ipq6018_pll_data = {
> -       .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA,
>         .pll = &ipq_pll_huayra,
>         .pll_config = &ipq6018_pll_config,
>  };
>
>  static struct apss_pll_data ipq9574_pll_data = {
> -       .pll_type = CLK_ALPHA_PLL_TYPE_HUAYRA,
>         .pll = &ipq_pll_huayra,
>         .pll_config = &ipq9574_pll_config,
>  };
> @@ -194,10 +188,10 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>         if (!data)
>                 return -ENODEV;
>
> -       if (data->pll_type == CLK_ALPHA_PLL_TYPE_HUAYRA)
> +       if (data->pll == &ipq_pll_huayra)
>                 clk_alpha_pll_configure(data->pll, regmap, data->pll_config);
> -       else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER ||
> -                data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS)
> +       else if (data->pll == &ipq_pll_stromer ||
> +                data->pll == &ipq_pll_stromer_plus)
>                 clk_stromer_pll_configure(data->pll, regmap, data->pll_config);
>
>         ret = devm_clk_register_regmap(dev, &data->pll->clkr);
>
> --
> 2.44.0
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Gabor Juhos]

2024. 03. 21. 11:37 keltezéssel, Dmitry Baryshkov írta:
> On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote:
>>
>> The value of the 'pll_type' field of 'struct apps_pll_data'
>> is used only by the probe function to decide which config
>> function should be called for the actual PLL. However this
>> can be derived also from the 'pll' field  which makes the
>> 'pll_type' field redundant.
>>
>> Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values
>> are meant to be used as indices to the 'clk_alpha_pll_regs'
>> array so using those to define the pll type in this driver
>> is misleading anyway.
>>
>> Change the probe function to use the 'pll' field to determine
>> the configuration function to be used, and remove the
>> 'pll_type' field to simplify the code.
> 
> I can't fully appreciate this idea. There can be cases when different
> PLL types share the set of ops. I think having a type is more
> versatile and makes the code more obvious.

I understand your concerns, but let me explain the reasons about why I have
choosed this implementation in more detail.

The driver declares three distinct clocks for the three different PLL types it
supports. Each one of these clocks are using different register maps and clock
operations which in a whole uniquely identifies the type of the PLL. In contrary
to this, the CLK_ALPHA_PLL_TYPE_* values assigned to 'pll_type' are only
indicating that which register map should be used for the given PLL. However
this is also specified indirectly through the 'regs' member of the clocks so
this is a redundant information.

Additionally, using the CLK_ALPHA_PLL_TYPE_*  for anything other than for
specifying the register map is misleading.  For example, here are some snippets
from the driver before the patch:

static struct clk_alpha_pll ipq_pll_stromer_plus = {
	...
	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER],
	...

static struct apss_pll_data ipq5332_pll_data = {
	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS,
	.pll = &ipq_pll_stromer_plus,
	...

Since it is not obvious at a first glance, one could ask that why the two
CLK_ALPHA_PLL_TYPE_* values are different?

Although my opinion that it is redundant still stand, but I'm not against
keeping the pll_type. However if we keep that, then at least we should use
private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more
obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values.

This solution would be more acceptable?

Regards,
Gabor

Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Dmitry Baryshkov]

On Fri, 22 Mar 2024 at 22:59, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> 2024. 03. 21. 11:37 keltezéssel, Dmitry Baryshkov írta:
> > On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote:
> >>
> >> The value of the 'pll_type' field of 'struct apps_pll_data'
> >> is used only by the probe function to decide which config
> >> function should be called for the actual PLL. However this
> >> can be derived also from the 'pll' field  which makes the
> >> 'pll_type' field redundant.
> >>
> >> Additionally, the CLK_ALPHA_PLL_TYPE_* enumeration values
> >> are meant to be used as indices to the 'clk_alpha_pll_regs'
> >> array so using those to define the pll type in this driver
> >> is misleading anyway.
> >>
> >> Change the probe function to use the 'pll' field to determine
> >> the configuration function to be used, and remove the
> >> 'pll_type' field to simplify the code.
> >
> > I can't fully appreciate this idea. There can be cases when different
> > PLL types share the set of ops. I think having a type is more
> > versatile and makes the code more obvious.
>
> I understand your concerns, but let me explain the reasons about why I have
> choosed this implementation in more detail.
>
> The driver declares three distinct clocks for the three different PLL types it
> supports. Each one of these clocks are using different register maps and clock
> operations which in a whole uniquely identifies the type of the PLL. In contrary
> to this, the CLK_ALPHA_PLL_TYPE_* values assigned to 'pll_type' are only
> indicating that which register map should be used for the given PLL. However
> this is also specified indirectly through the 'regs' member of the clocks so
> this is a redundant information.
>
> Additionally, using the CLK_ALPHA_PLL_TYPE_*  for anything other than for
> specifying the register map is misleading.  For example, here are some snippets
> from the driver before the patch:
>
> static struct clk_alpha_pll ipq_pll_stromer_plus = {
>         ...
>         .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER],
>         ...
>
> static struct apss_pll_data ipq5332_pll_data = {
>         .pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS,
>         .pll = &ipq_pll_stromer_plus,
>         ...
>
> Since it is not obvious at a first glance, one could ask that why the two
> CLK_ALPHA_PLL_TYPE_* values are different?
>
> Although my opinion that it is redundant still stand, but I'm not against
> keeping the pll_type. However if we keep that, then at least we should use
> private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more
> obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values.
>
> This solution would be more acceptable?

This looks like a slight overkill, but yes, it is more acceptable. The
logic should be type => functions, not the other way around.


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Gabor Juhos]

2024. 03. 22. 23:33 keltezéssel, Dmitry Baryshkov írta:

...

>> Although my opinion that it is redundant still stand, but I'm not against
>> keeping the pll_type. However if we keep that, then at least we should use
>> private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more
>> obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values.
>>
>> This solution would be more acceptable?
> 
> This looks like a slight overkill, but yes, it is more acceptable. The
> logic should be type => functions, not the other way around.
> 
> 

If that is overkill, it does not worth the change. I will drop the patch and
send an updated series.

Regards,
Gabor

Re: [PATCH v2 3/6] clk: qcom: apss-ipq-pll: remove 'pll_type' field from struct 'apss_pll_data'

[Dmitry Baryshkov]

On Sun, 24 Mar 2024 at 09:29, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> 2024. 03. 22. 23:33 keltezéssel, Dmitry Baryshkov írta:
>
> ...
>
> >> Although my opinion that it is redundant still stand, but I'm not against
> >> keeping the pll_type. However if we keep that, then at least we should use
> >> private enums (IPQ_APSS_PLL_TYPE_* or similar) for that in order to make it more
> >> obvious that it means a different thing than the CLK_ALPHA_PLL_TYPE_* values.
> >>
> >> This solution would be more acceptable?
> >
> > This looks like a slight overkill, but yes, it is more acceptable. The
> > logic should be type => functions, not the other way around.
> >
> >
>
> If that is overkill, it does not worth the change. I will drop the patch and
> send an updated series.

Either way is fine to me.


-- 
With best wishes
Dmitry

[PATCH v2 4/6] clk: qcom: apss-ipq-pll: constify match data structures

[Gabor Juhos]

The match data structures are used only by the apss_ipq_pll_probe()
function and are never modified so mark those as constant.

No functional changes.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>

---
Changes in v2:
 - add Reviewed-by tag from Dmitry
 - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-4-52f795429d5d@gmail.com
---
 drivers/clk/qcom/apss-ipq-pll.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index 816b0d1f8d8c8..49da67435d4fe 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -140,22 +140,22 @@ static const struct apss_pll_data ipq5018_pll_data = {
 	.pll_config = &ipq5018_pll_config,
 };
 
-static struct apss_pll_data ipq5332_pll_data = {
+static const struct apss_pll_data ipq5332_pll_data = {
 	.pll = &ipq_pll_stromer_plus,
 	.pll_config = &ipq5332_pll_config,
 };
 
-static struct apss_pll_data ipq8074_pll_data = {
+static const struct apss_pll_data ipq8074_pll_data = {
 	.pll = &ipq_pll_huayra,
 	.pll_config = &ipq8074_pll_config,
 };
 
-static struct apss_pll_data ipq6018_pll_data = {
+static const struct apss_pll_data ipq6018_pll_data = {
 	.pll = &ipq_pll_huayra,
 	.pll_config = &ipq6018_pll_config,
 };
 
-static struct apss_pll_data ipq9574_pll_data = {
+static const struct apss_pll_data ipq9574_pll_data = {
 	.pll = &ipq_pll_huayra,
 	.pll_config = &ipq9574_pll_config,
 };

-- 
2.44.0

Re: [PATCH v2 4/6] clk: qcom: apss-ipq-pll: constify match data structures

[Konrad Dybcio]

On 21.03.2024 08:49, Gabor Juhos wrote:
> The match data structures are used only by the apss_ipq_pll_probe()
> function and are never modified so mark those as constant.
> 
> No functional changes.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> 
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 5/6] clk: qcom: apss-ipq-pll: constify clk_init_data structures

[Gabor Juhos]

The clk_init_data structures are never modified, so add const
qualifier to the ones where it is missing.

No functional changes.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
 - no changes
 - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-5-52f795429d5d@gmail.com
---
 drivers/clk/qcom/apss-ipq-pll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index 49da67435d4fe..5bbf329c9bdd1 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -15,7 +15,7 @@ static struct clk_alpha_pll ipq_pll_huayra = {
 	.clkr = {
 		.enable_reg = 0x0,
 		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
+		.hw.init = &(const struct clk_init_data) {
 			.name = "a53pll",
 			.parent_data = &(const struct clk_parent_data) {
 				.fw_name = "xo",
@@ -55,7 +55,7 @@ static struct clk_alpha_pll ipq_pll_stromer_plus = {
 	.clkr = {
 		.enable_reg = 0x0,
 		.enable_mask = BIT(0),
-		.hw.init = &(struct clk_init_data){
+		.hw.init = &(const struct clk_init_data) {
 			.name = "a53pll",
 			.parent_data = &(const struct clk_parent_data) {
 				.fw_name = "xo",

-- 
2.44.0

Re: [PATCH v2 5/6] clk: qcom: apss-ipq-pll: constify clk_init_data structures

[Dmitry Baryshkov]

On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The clk_init_data structures are never modified, so add const
> qualifier to the ones where it is missing.
>
> No functional changes.
>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Changes in v2:
>  - no changes
>  - Link to v1: https://lore.kernel.org/r/20240318-apss-ipq-pll-cleanup-v1-5-52f795429d5d@gmail.com
> ---
>  drivers/clk/qcom/apss-ipq-pll.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

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

-- 
With best wishes
Dmitry

Re: [PATCH v2 5/6] clk: qcom: apss-ipq-pll: constify clk_init_data structures

[Konrad Dybcio]

On 21.03.2024 08:49, Gabor Juhos wrote:
> The clk_init_data structures are never modified, so add const
> qualifier to the ones where it is missing.
> 
> No functional changes.
> 
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 6/6] clk: qcom: clk-cbf-8996: use HUAYRA_APPS register map for cbf_pll

[Gabor Juhos]

The register map used for 'cbf_pll' is the same as the one defined for
the CLK_ALPHA_PLL_TYPE_HUAYRA_APSS indice in the 'clk_alpha_pll_regs'
array.

Drop the local register map and use the global one instead to reduce
code duplication.

No functional changes intended. Compile tested only.

Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes since v1:
  - new patch

Note: Although this patch is not strictly related to the subject of the
series but as the change has been suggested by Dmitry during the review
process it has been added here for completeness.
---
 drivers/clk/qcom/clk-cbf-8996.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
index fe24b4abeab48..76bf523431b85 100644
--- a/drivers/clk/qcom/clk-cbf-8996.c
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -41,17 +41,6 @@ enum {
 
 #define CBF_PLL_OFFSET 0xf000
 
-static const u8 cbf_pll_regs[PLL_OFF_MAX_REGS] = {
-	[PLL_OFF_L_VAL] = 0x08,
-	[PLL_OFF_ALPHA_VAL] = 0x10,
-	[PLL_OFF_USER_CTL] = 0x18,
-	[PLL_OFF_CONFIG_CTL] = 0x20,
-	[PLL_OFF_CONFIG_CTL_U] = 0x24,
-	[PLL_OFF_TEST_CTL] = 0x30,
-	[PLL_OFF_TEST_CTL_U] = 0x34,
-	[PLL_OFF_STATUS] = 0x28,
-};
-
 static struct alpha_pll_config cbfpll_config = {
 	.l = 72,
 	.config_ctl_val = 0x200d4828,
@@ -67,7 +56,7 @@ static struct alpha_pll_config cbfpll_config = {
 
 static struct clk_alpha_pll cbf_pll = {
 	.offset = CBF_PLL_OFFSET,
-	.regs = cbf_pll_regs,
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_HUAYRA_APSS],
 	.flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "cbf_pll",

-- 
2.44.0

Re: [PATCH v2 6/6] clk: qcom: clk-cbf-8996: use HUAYRA_APPS register map for cbf_pll

[Dmitry Baryshkov]

On Thu, 21 Mar 2024 at 09:50, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The register map used for 'cbf_pll' is the same as the one defined for
> the CLK_ALPHA_PLL_TYPE_HUAYRA_APSS indice in the 'clk_alpha_pll_regs'
> array.
>
> Drop the local register map and use the global one instead to reduce
> code duplication.
>
> No functional changes intended. Compile tested only.
>
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Changes since v1:
>   - new patch
>
> Note: Although this patch is not strictly related to the subject of the
> series but as the change has been suggested by Dmitry during the review
> process it has been added here for completeness.
> ---
>  drivers/clk/qcom/clk-cbf-8996.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)

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

-- 
With best wishes
Dmitry