All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets
@ 2024-02-25 17:32 Gabor Juhos
  2024-02-25 17:32 ` [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk' Gabor Juhos
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Gabor Juhos @ 2024-02-25 17:32 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel, Gabor Juhos

The purpose of this small series is to fix some, presumably wrong
register offsets in the 'gcc-ipq5018' driver.

The patches are based on v6.8-rc5.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Gabor Juhos (3):
      clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
      clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk'
      clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset

 drivers/clk/qcom/gcc-ipq5018.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20240224-gcc-ipq5018-register-fixes-394905520fda

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
  2024-02-25 17:32 [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Gabor Juhos
@ 2024-02-25 17:32 ` Gabor Juhos
  2024-02-25 21:00   ` Dmitry Baryshkov
  2024-02-26  9:53   ` Kathiravan Thirumoorthy
  2024-02-25 17:32 ` [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk' Gabor Juhos
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Gabor Juhos @ 2024-02-25 17:32 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel, Gabor Juhos

The value of the 'enable_reg' field in the 'gcc_gmac0_sys_clk'
clock definition seems wrong as it is greater than the
'max_register' value defined in the regmap configuration.
Additionally, all other gmac specific branch clock definitions
within the driver uses the same value both for the 'enable_reg'
and for the 'halt_reg' fields.

Due to the lack of documentation the correct value is not known.
Looking into the downstream driver does not help either, as that
uses the same (presumably wrong) value [1].

Nevertheless, change the 'enable_reg' field of 'gcc_gmac0_sys_clk'
to use the value from the 'halt_reg' field so it follows the pattern
used in other gmac clock definitions. The change is based on the
assumption that the register layout of this clock is the same
as the other gmac clocks.

1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L1889

Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/clk/qcom/gcc-ipq5018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
index 4aba47e8700d2..cef9a1e7c9fdb 100644
--- a/drivers/clk/qcom/gcc-ipq5018.c
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -1754,7 +1754,7 @@ static struct clk_branch gcc_gmac0_sys_clk = {
 	.halt_check = BRANCH_HALT_DELAY,
 	.halt_bit = 31,
 	.clkr = {
-		.enable_reg = 0x683190,
+		.enable_reg = 0x68190,
 		.enable_mask = BIT(0),
 		.hw.init = &(struct clk_init_data) {
 			.name = "gcc_gmac0_sys_clk",

-- 
2.43.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk'
  2024-02-25 17:32 [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Gabor Juhos
  2024-02-25 17:32 ` [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk' Gabor Juhos
@ 2024-02-25 17:32 ` Gabor Juhos
  2024-02-25 21:00   ` Dmitry Baryshkov
  2024-02-26  9:56   ` Kathiravan Thirumoorthy
  2024-02-25 17:32 ` [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset Gabor Juhos
  2024-03-04 21:28 ` [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Bjorn Andersson
  3 siblings, 2 replies; 13+ messages in thread
From: Gabor Juhos @ 2024-02-25 17:32 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel, Gabor Juhos

The following table shows the values of the 'halt_reg' and the
'enable_reg' fields from the pcie clocks defined in the current
driver:

  clock                        halt_reg    enable_reg

  gcc_pcie0_ahb_clk            0x75010     0x75010
  gcc_pcie0_aux_clk            0x75014     0x75014
  gcc_pcie0_axi_m_clk          0x75008     0x75008
  gcc_pcie0_axi_s_bridge_clk   0x75048     0x75048
  gcc_pcie0_axi_s_clk          0x7500c     0x7500c
  gcc_pcie0_pipe_clk           0x75018     0x75018

  gcc_pcie1_ahb_clk            0x76010     0x76010
  gcc_pcie1_aux_clk            0x76014     0x76014
  gcc_pcie1_axi_m_clk          0x76008     0x76008
  gcc_pcie1_axi_s_bridge_clk   0x76048     0x76048
  gcc_pcie1_axi_s_clk          0x7600c     0x7600c
  gcc_pcie1_pipe_clk                 8*    0x76018

Based on the table, it is quite likely that the pcie0 and the pci1
clocks are using the same register layout, however it seems that
the value of the 'halt_reg' field in the 'gcc_pcie1_pipe_clk' clock
is wrong.

In the downstream driver [1], the same '0x76018' value is used for
both the 'halt_reg' and for the 'enable_reg' fields of the
'gcc_pcie1_pipe_clk' clock.

Update the current driver to use the same value used downstream as
probably that is the correct value.

1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L2316

Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/clk/qcom/gcc-ipq5018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
index cef9a1e7c9fdb..5e81cfa77293a 100644
--- a/drivers/clk/qcom/gcc-ipq5018.c
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -2180,7 +2180,7 @@ static struct clk_branch gcc_pcie1_axi_s_clk = {
 };
 
 static struct clk_branch gcc_pcie1_pipe_clk = {
-	.halt_reg = 8,
+	.halt_reg = 0x76018,
 	.halt_check = BRANCH_HALT_DELAY,
 	.halt_bit = 31,
 	.clkr = {

-- 
2.43.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset
  2024-02-25 17:32 [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Gabor Juhos
  2024-02-25 17:32 ` [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk' Gabor Juhos
  2024-02-25 17:32 ` [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk' Gabor Juhos
@ 2024-02-25 17:32 ` Gabor Juhos
  2024-02-25 21:00   ` Dmitry Baryshkov
  2024-02-26  9:59   ` Kathiravan Thirumoorthy
  2024-03-04 21:28 ` [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Bjorn Andersson
  3 siblings, 2 replies; 13+ messages in thread
From: Gabor Juhos @ 2024-02-25 17:32 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel, Gabor Juhos

The current register offset used for the GCC_UBI0_AXI_ARES reset
seems wrong. Or at least, the downstream driver uses [1] the same
offset which is used for other the GCC_UBI0_*_ARES resets.

Change the code to use the same offset used in the downstream
driver and also specify the reset bit explicitly to use the
same format as the followup entries.

1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L3773

Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/clk/qcom/gcc-ipq5018.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
index 5e81cfa77293a..e2bd54826a4ce 100644
--- a/drivers/clk/qcom/gcc-ipq5018.c
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -3632,7 +3632,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = {
 	[GCC_SYSTEM_NOC_BCR] = { 0x26000, 0 },
 	[GCC_TCSR_BCR] = { 0x28000, 0 },
 	[GCC_TLMM_BCR] = { 0x34000, 0 },
-	[GCC_UBI0_AXI_ARES] = { 0x680},
+	[GCC_UBI0_AXI_ARES] = { 0x68010, 0 },
 	[GCC_UBI0_AHB_ARES] = { 0x68010, 1 },
 	[GCC_UBI0_NC_AXI_ARES] = { 0x68010, 2 },
 	[GCC_UBI0_DBG_ARES] = { 0x68010, 3 },

-- 
2.43.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
  2024-02-25 17:32 ` [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk' Gabor Juhos
@ 2024-02-25 21:00   ` Dmitry Baryshkov
  2024-02-26 16:53     ` Gabor Juhos
  2024-02-26  9:53   ` Kathiravan Thirumoorthy
  1 sibling, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2024-02-25 21:00 UTC (permalink / raw)
  To: Gabor Juhos
  Cc: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran, linux-arm-msm, linux-clk, linux-kernel

On Sun, 25 Feb 2024 at 19:33, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The value of the 'enable_reg' field in the 'gcc_gmac0_sys_clk'
> clock definition seems wrong as it is greater than the
> 'max_register' value defined in the regmap configuration.
> Additionally, all other gmac specific branch clock definitions
> within the driver uses the same value both for the 'enable_reg'
> and for the 'halt_reg' fields.
>
> Due to the lack of documentation the correct value is not known.
> Looking into the downstream driver does not help either, as that
> uses the same (presumably wrong) value [1].
>
> Nevertheless, change the 'enable_reg' field of 'gcc_gmac0_sys_clk'
> to use the value from the 'halt_reg' field so it follows the pattern
> used in other gmac clock definitions. The change is based on the
> assumption that the register layout of this clock is the same
> as the other gmac clocks.
>
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L1889
>
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>  drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

--
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk'
  2024-02-25 17:32 ` [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk' Gabor Juhos
@ 2024-02-25 21:00   ` Dmitry Baryshkov
  2024-02-26  9:56   ` Kathiravan Thirumoorthy
  1 sibling, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2024-02-25 21:00 UTC (permalink / raw)
  To: Gabor Juhos
  Cc: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran, linux-arm-msm, linux-clk, linux-kernel

On Sun, 25 Feb 2024 at 19:33, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The following table shows the values of the 'halt_reg' and the
> 'enable_reg' fields from the pcie clocks defined in the current
> driver:
>
>   clock                        halt_reg    enable_reg
>
>   gcc_pcie0_ahb_clk            0x75010     0x75010
>   gcc_pcie0_aux_clk            0x75014     0x75014
>   gcc_pcie0_axi_m_clk          0x75008     0x75008
>   gcc_pcie0_axi_s_bridge_clk   0x75048     0x75048
>   gcc_pcie0_axi_s_clk          0x7500c     0x7500c
>   gcc_pcie0_pipe_clk           0x75018     0x75018
>
>   gcc_pcie1_ahb_clk            0x76010     0x76010
>   gcc_pcie1_aux_clk            0x76014     0x76014
>   gcc_pcie1_axi_m_clk          0x76008     0x76008
>   gcc_pcie1_axi_s_bridge_clk   0x76048     0x76048
>   gcc_pcie1_axi_s_clk          0x7600c     0x7600c
>   gcc_pcie1_pipe_clk                 8*    0x76018
>
> Based on the table, it is quite likely that the pcie0 and the pci1
> clocks are using the same register layout, however it seems that
> the value of the 'halt_reg' field in the 'gcc_pcie1_pipe_clk' clock
> is wrong.
>
> In the downstream driver [1], the same '0x76018' value is used for
> both the 'halt_reg' and for the 'enable_reg' fields of the
> 'gcc_pcie1_pipe_clk' clock.
>
> Update the current driver to use the same value used downstream as
> probably that is the correct value.
>
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L2316
>
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>  drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset
  2024-02-25 17:32 ` [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset Gabor Juhos
@ 2024-02-25 21:00   ` Dmitry Baryshkov
  2024-02-26  9:59   ` Kathiravan Thirumoorthy
  1 sibling, 0 replies; 13+ messages in thread
From: Dmitry Baryshkov @ 2024-02-25 21:00 UTC (permalink / raw)
  To: Gabor Juhos
  Cc: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran, linux-arm-msm, linux-clk, linux-kernel

On Sun, 25 Feb 2024 at 19:34, Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The current register offset used for the GCC_UBI0_AXI_ARES reset
> seems wrong. Or at least, the downstream driver uses [1] the same
> offset which is used for other the GCC_UBI0_*_ARES resets.
>
> Change the code to use the same offset used in the downstream
> driver and also specify the reset bit explicitly to use the
> same format as the followup entries.
>
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L3773
>
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>  drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
  2024-02-25 17:32 ` [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk' Gabor Juhos
  2024-02-25 21:00   ` Dmitry Baryshkov
@ 2024-02-26  9:53   ` Kathiravan Thirumoorthy
  2024-02-26 16:54     ` Gabor Juhos
  1 sibling, 1 reply; 13+ messages in thread
From: Kathiravan Thirumoorthy @ 2024-02-26  9:53 UTC (permalink / raw)
  To: Gabor Juhos, Bjorn Andersson, Konrad Dybcio, Michael Turquette,
	Stephen Boyd, Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel



On 2/25/2024 11:02 PM, Gabor Juhos wrote:
> The value of the 'enable_reg' field in the 'gcc_gmac0_sys_clk'
> clock definition seems wrong as it is greater than the
> 'max_register' value defined in the regmap configuration.
> Additionally, all other gmac specific branch clock definitions
> within the driver uses the same value both for the 'enable_reg'
> and for the 'halt_reg' fields.
> 
> Due to the lack of documentation the correct value is not known.
> Looking into the downstream driver does not help either, as that
> uses the same (presumably wrong) value [1].
> 
> Nevertheless, change the 'enable_reg' field of 'gcc_gmac0_sys_clk'
> to use the value from the 'halt_reg' field so it follows the pattern
> used in other gmac clock definitions. The change is based on the
> assumption that the register layout of this clock is the same
> as the other gmac clocks.
> 
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L1889


Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>


> 
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>   drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
> index 4aba47e8700d2..cef9a1e7c9fdb 100644
> --- a/drivers/clk/qcom/gcc-ipq5018.c
> +++ b/drivers/clk/qcom/gcc-ipq5018.c
> @@ -1754,7 +1754,7 @@ static struct clk_branch gcc_gmac0_sys_clk = {
>   	.halt_check = BRANCH_HALT_DELAY,
>   	.halt_bit = 31,
>   	.clkr = {
> -		.enable_reg = 0x683190,
> +		.enable_reg = 0x68190,
>   		.enable_mask = BIT(0),
>   		.hw.init = &(struct clk_init_data) {
>   			.name = "gcc_gmac0_sys_clk",
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk'
  2024-02-25 17:32 ` [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk' Gabor Juhos
  2024-02-25 21:00   ` Dmitry Baryshkov
@ 2024-02-26  9:56   ` Kathiravan Thirumoorthy
  1 sibling, 0 replies; 13+ messages in thread
From: Kathiravan Thirumoorthy @ 2024-02-26  9:56 UTC (permalink / raw)
  To: Gabor Juhos, Bjorn Andersson, Konrad Dybcio, Michael Turquette,
	Stephen Boyd, Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel



On 2/25/2024 11:02 PM, Gabor Juhos wrote:
> The following table shows the values of the 'halt_reg' and the
> 'enable_reg' fields from the pcie clocks defined in the current
> driver:
> 
>    clock                        halt_reg    enable_reg
> 
>    gcc_pcie0_ahb_clk            0x75010     0x75010
>    gcc_pcie0_aux_clk            0x75014     0x75014
>    gcc_pcie0_axi_m_clk          0x75008     0x75008
>    gcc_pcie0_axi_s_bridge_clk   0x75048     0x75048
>    gcc_pcie0_axi_s_clk          0x7500c     0x7500c
>    gcc_pcie0_pipe_clk           0x75018     0x75018
> 
>    gcc_pcie1_ahb_clk            0x76010     0x76010
>    gcc_pcie1_aux_clk            0x76014     0x76014
>    gcc_pcie1_axi_m_clk          0x76008     0x76008
>    gcc_pcie1_axi_s_bridge_clk   0x76048     0x76048
>    gcc_pcie1_axi_s_clk          0x7600c     0x7600c
>    gcc_pcie1_pipe_clk                 8*    0x76018
> 
> Based on the table, it is quite likely that the pcie0 and the pci1
> clocks are using the same register layout, however it seems that
> the value of the 'halt_reg' field in the 'gcc_pcie1_pipe_clk' clock
> is wrong.
> 
> In the downstream driver [1], the same '0x76018' value is used for
> both the 'halt_reg' and for the 'enable_reg' fields of the
> 'gcc_pcie1_pipe_clk' clock.
> 
> Update the current driver to use the same value used downstream as
> probably that is the correct value.
> 
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L2316


Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>


> 
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>   drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
> index cef9a1e7c9fdb..5e81cfa77293a 100644
> --- a/drivers/clk/qcom/gcc-ipq5018.c
> +++ b/drivers/clk/qcom/gcc-ipq5018.c
> @@ -2180,7 +2180,7 @@ static struct clk_branch gcc_pcie1_axi_s_clk = {
>   };
>   
>   static struct clk_branch gcc_pcie1_pipe_clk = {
> -	.halt_reg = 8,
> +	.halt_reg = 0x76018,
>   	.halt_check = BRANCH_HALT_DELAY,
>   	.halt_bit = 31,
>   	.clkr = {
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset
  2024-02-25 17:32 ` [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset Gabor Juhos
  2024-02-25 21:00   ` Dmitry Baryshkov
@ 2024-02-26  9:59   ` Kathiravan Thirumoorthy
  1 sibling, 0 replies; 13+ messages in thread
From: Kathiravan Thirumoorthy @ 2024-02-26  9:59 UTC (permalink / raw)
  To: Gabor Juhos, Bjorn Andersson, Konrad Dybcio, Michael Turquette,
	Stephen Boyd, Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel



On 2/25/2024 11:02 PM, Gabor Juhos wrote:
> The current register offset used for the GCC_UBI0_AXI_ARES reset
> seems wrong. Or at least, the downstream driver uses [1] the same
> offset which is used for other the GCC_UBI0_*_ARES resets.
> 
> Change the code to use the same offset used in the downstream
> driver and also specify the reset bit explicitly to use the
> same format as the followup entries.
> 
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L3773


Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>


> 
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>   drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
> index 5e81cfa77293a..e2bd54826a4ce 100644
> --- a/drivers/clk/qcom/gcc-ipq5018.c
> +++ b/drivers/clk/qcom/gcc-ipq5018.c
> @@ -3632,7 +3632,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = {
>   	[GCC_SYSTEM_NOC_BCR] = { 0x26000, 0 },
>   	[GCC_TCSR_BCR] = { 0x28000, 0 },
>   	[GCC_TLMM_BCR] = { 0x34000, 0 },
> -	[GCC_UBI0_AXI_ARES] = { 0x680},
> +	[GCC_UBI0_AXI_ARES] = { 0x68010, 0 },
>   	[GCC_UBI0_AHB_ARES] = { 0x68010, 1 },
>   	[GCC_UBI0_NC_AXI_ARES] = { 0x68010, 2 },
>   	[GCC_UBI0_DBG_ARES] = { 0x68010, 3 },
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
  2024-02-25 21:00   ` Dmitry Baryshkov
@ 2024-02-26 16:53     ` Gabor Juhos
  0 siblings, 0 replies; 13+ messages in thread
From: Gabor Juhos @ 2024-02-26 16:53 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran, linux-arm-msm, linux-clk, linux-kernel

2024. 02. 25. 22:00 keltezéssel, Dmitry Baryshkov írta:
> On Sun, 25 Feb 2024 at 19:33, Gabor Juhos <j4g8y7@gmail.com> wrote:
>>
>> The value of the 'enable_reg' field in the 'gcc_gmac0_sys_clk'
>> clock definition seems wrong as it is greater than the
>> 'max_register' value defined in the regmap configuration.
>> Additionally, all other gmac specific branch clock definitions
>> within the driver uses the same value both for the 'enable_reg'
>> and for the 'halt_reg' fields.
>>
>> Due to the lack of documentation the correct value is not known.
>> Looking into the downstream driver does not help either, as that
>> uses the same (presumably wrong) value [1].
>>
>> Nevertheless, change the 'enable_reg' field of 'gcc_gmac0_sys_clk'
>> to use the value from the 'halt_reg' field so it follows the pattern
>> used in other gmac clock definitions. The change is based on the
>> assumption that the register layout of this clock is the same
>> as the other gmac clocks.
>>
>> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L1889
>>
>> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>> ---
>>  drivers/clk/qcom/gcc-ipq5018.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thank you for the review!

Regards,
Gabor

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
  2024-02-26  9:53   ` Kathiravan Thirumoorthy
@ 2024-02-26 16:54     ` Gabor Juhos
  0 siblings, 0 replies; 13+ messages in thread
From: Gabor Juhos @ 2024-02-26 16:54 UTC (permalink / raw)
  To: Kathiravan Thirumoorthy, Bjorn Andersson, Konrad Dybcio,
	Michael Turquette, Stephen Boyd, Gokul Sriram Palanisamy,
	Varadarajan Narayanan, Sricharan Ramabadhran
  Cc: linux-arm-msm, linux-clk, linux-kernel

2024. 02. 26. 10:53 keltezéssel, Kathiravan Thirumoorthy írta:
> 
> 
> On 2/25/2024 11:02 PM, Gabor Juhos wrote:
>> The value of the 'enable_reg' field in the 'gcc_gmac0_sys_clk'
>> clock definition seems wrong as it is greater than the
>> 'max_register' value defined in the regmap configuration.
>> Additionally, all other gmac specific branch clock definitions
>> within the driver uses the same value both for the 'enable_reg'
>> and for the 'halt_reg' fields.
>>
>> Due to the lack of documentation the correct value is not known.
>> Looking into the downstream driver does not help either, as that
>> uses the same (presumably wrong) value [1].
>>
>> Nevertheless, change the 'enable_reg' field of 'gcc_gmac0_sys_clk'
>> to use the value from the 'halt_reg' field so it follows the pattern
>> used in other gmac clock definitions. The change is based on the
>> assumption that the register layout of this clock is the same
>> as the other gmac clocks.
>>
>> 1.
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r4/drivers/clk/qcom/gcc-ipq5018.c?ref_type=heads#L1889
> 
> 
> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>

Thank you for the review!

Regards,
Gabor

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets
  2024-02-25 17:32 [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Gabor Juhos
                   ` (2 preceding siblings ...)
  2024-02-25 17:32 ` [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset Gabor Juhos
@ 2024-03-04 21:28 ` Bjorn Andersson
  3 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2024-03-04 21:28 UTC (permalink / raw)
  To: Konrad Dybcio, Michael Turquette, Stephen Boyd,
	Gokul Sriram Palanisamy, Varadarajan Narayanan,
	Sricharan Ramabadhran, Gabor Juhos
  Cc: linux-arm-msm, linux-clk, linux-kernel


On Sun, 25 Feb 2024 18:32:53 +0100, Gabor Juhos wrote:
> The purpose of this small series is to fix some, presumably wrong
> register offsets in the 'gcc-ipq5018' driver.
> 
> The patches are based on v6.8-rc5.
> 
> 

Applied, thanks!

[1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk'
      commit: f982adcc1b1c02a3114f68ac73c811cbfabe90fa
[2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk'
      commit: 11b752ac5a07cbfd95592fac5237a02f45662926
[3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset
      commit: 7d474b43087aa356d714d39870c90d77fc6f1186

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-03-04 21:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-25 17:32 [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Gabor Juhos
2024-02-25 17:32 ` [PATCH 1/3] clk: qcom: gcc-ipq5018: fix 'enable_reg' offset of 'gcc_gmac0_sys_clk' Gabor Juhos
2024-02-25 21:00   ` Dmitry Baryshkov
2024-02-26 16:53     ` Gabor Juhos
2024-02-26  9:53   ` Kathiravan Thirumoorthy
2024-02-26 16:54     ` Gabor Juhos
2024-02-25 17:32 ` [PATCH 2/3] clk: qcom: gcc-ipq5018: fix 'halt_reg' offset of 'gcc_pcie1_pipe_clk' Gabor Juhos
2024-02-25 21:00   ` Dmitry Baryshkov
2024-02-26  9:56   ` Kathiravan Thirumoorthy
2024-02-25 17:32 ` [PATCH 3/3] clk: qcom: gcc-ipq5018: fix register offset for GCC_UBI0_AXI_ARES reset Gabor Juhos
2024-02-25 21:00   ` Dmitry Baryshkov
2024-02-26  9:59   ` Kathiravan Thirumoorthy
2024-03-04 21:28 ` [PATCH 0/3] clk: qcom: gcc-ipq5018: fix some register offsets Bjorn Andersson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.