| Message ID | 20240225-gcc-ipq5018-register-fixes-v1-1-3c191404d9f0@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f982adcc1b1c02a3114f68ac73c811cbfabe90fa |
| Headers | show |
| Series | clk: qcom: gcc-ipq5018: fix some register offsets | expand |
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
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", >
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
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
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",
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(-)