lkml.org 
[lkml]   [2023]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] clk: qcom: common: Handle invalid index error
From
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

\
 
 \ /
  Last update: 2023-04-17 13:51    [W:0.046 / U:1.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site