Messages in this thread | | | Date | Mon, 17 Apr 2023 14:49:27 +0300 | Subject | Re: [PATCH 1/2] clk: qcom: common: Handle invalid index error | From | Dmitry Baryshkov <> |
| |
On 17/04/2023 07:40, Taniya Das wrote: > Hi Dmitry, > > Thanks for the comments. > > > On 3/3/2023 4:14 PM, Dmitry Baryshkov wrote: >> On Fri, 3 Mar 2023 at 11:30, Taniya Das <quic_tdas@quicinc.com> wrote: >>> >>> Introduce start_index to handle invalid index error >>> seen when there are two clock descriptors assigned >>> to the same clock controller. >> >> Please provide details of the exact case that you are trying to solve >> (this might go to the cover letter). I think the commit message is >> slightly misleading here. Are you trying to add error messages or to >> prevent them from showing up? >> > > We are trying to avoid error messages from showing up. > >> I'm asking because error messages do not seem to correspond to patch >> 2. You add start_index to make the kernel warn for the clock indices >> less than LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC = 4, while quoted >> messages show indices 5,6,7. >> > > Right, we want the kernel to warn if the clock index is less than > start_index,
This is arguable but logical. Usually we do not warn for absent clocks.
> along with that we also want to handle the case where > num_rclks is uninitialized because of same clock descriptor being > assigned to two clock controllers.
Hmm, but num_rclks is always initialized, isn't it? In the worst case it will default to 0 meaning that
> Earlier Invalid index error was showing up for valid indices 5,6,7 > because of the simple if check(idx >= num_rclks), hence we enhanced the > checks to handle the above case and compare the index to the start_index > + num_rclks, instead of simply comparing it with num_clks.
This is not a part of the patch and it will be incorrect anyway, since num_rclks = desc->num_clks = ARRAY_SIZE(some_cc_clocks).
Checking idx against `start_index + num_rclks` will allow one to get clocks after the end of rclks array.
For lpass_audio_cc_sc7280_desc num_rclks should get the value of 16, as the last entry in llpass_audio_cc_sc7280_clocks has index LPASS_AUDIO_CC_RX_MCLK_CLK_SRC = 15.
My analysis might be completely wrong, but I can only assume that somehow wrong clock controller got used. Could you please give it a try with the https://lore.kernel.org/linux-clk/20230417114659.137535-1-dmitry.baryshkov@linaro.org/ being applied?
> >> Nit: please don't overwrap the commit message, the recommended line >> width is about 72-77 chars. >> > > Done. > >>> >>> [ 3.600604] qcom_cc_clk_hw_get: invalid index 5 >>> [ 3.625251] qcom_cc_clk_hw_get: invalid index 6 >>> [ 3.648190] qcom_cc_clk_hw_get: invalid index 7 >> >>> >>> Fixes: 120c15528390 ("clk: qcom: Migrate to clk_hw based registration >>> and OF APIs") >>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>> --- >>> drivers/clk/qcom/common.c | 12 ++++++++---- >>> drivers/clk/qcom/common.h | 1 + >>> 2 files changed, 9 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c >>> index 75f09e6e057e..0e80535b61f2 100644 >>> --- a/drivers/clk/qcom/common.c >>> +++ b/drivers/clk/qcom/common.c >>> @@ -21,6 +21,7 @@ struct qcom_cc { >>> struct qcom_reset_controller reset; >>> struct clk_regmap **rclks; >>> size_t num_rclks; >>> + u32 rclks_start_index; >>> }; >>> >>> const >>> @@ -226,12 +227,13 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct >>> of_phandle_args *clkspec, >>> struct qcom_cc *cc = data; >>> unsigned int idx = clkspec->args[0]; >>> >>> - if (idx >= cc->num_rclks) { >>> + if (idx >= cc->rclks_start_index && idx < cc->num_rclks) >>> + return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL; >>> + else if (idx < cc->rclks_start_index && idx >= cc->num_rclks) >>> pr_err("%s: invalid index %u\n", __func__, idx); >>> - return ERR_PTR(-EINVAL); >>> - } >>> >>> - return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL; >>> + return ERR_PTR(-EINVAL); >>> + >>> } >>> >>> int qcom_cc_really_probe(struct platform_device *pdev, >>> @@ -281,6 +283,8 @@ int qcom_cc_really_probe(struct platform_device >>> *pdev, >>> >>> cc->rclks = rclks; >>> cc->num_rclks = num_clks; >>> + if (desc->start_index) >>> + cc->rclks_start_index = desc->start_index; >>> >>> qcom_cc_drop_protected(dev, cc); >>> >>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h >>> index 9c8f7b798d9f..924f36af55b3 100644 >>> --- a/drivers/clk/qcom/common.h >>> +++ b/drivers/clk/qcom/common.h >>> @@ -23,6 +23,7 @@ struct qcom_cc_desc { >>> const struct regmap_config *config; >>> struct clk_regmap **clks; >>> size_t num_clks; >>> + u32 start_index; >>> const struct qcom_reset_map *resets; >>> size_t num_resets; >>> struct gdsc **gdscs; >>> -- >>> 2.17.1 >>> >> >> >> -- >> With best wishes >> >> >> >> Dmitry >
-- With best wishes Dmitry
| |