linux-kernel - Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 22 Feb 2022 16:53:29 +0530
From:   Taniya Das <tdas@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for
 GDSCs

Hello Bjorn,

Thanks for your comments.

On 2/22/2022 2:12 AM, Bjorn Andersson wrote:
> On Mon 21 Feb 08:55 PST 2022, Taniya Das wrote:
> 
>> Hi Stephen, Bjorn,
>>
>> Thanks for your comments.
>>
>> On 2/10/2022 12:58 PM, Stephen Boyd wrote:
>>> Quoting Bjorn Andersson (2022-02-09 14:35:08)
>>>> On Wed 09 Feb 11:25 CST 2022, Taniya Das wrote:
>>>>
>>>>> Do not update the transition delay and use the default reset values.
>>>>>
>>>>> Fixes: 45dd0e55317cc ("clk: qcom: Add support for GDSCs)
>>>>> Signed-off-by: Taniya Das <tdas@...eaurora.org>
>>>>> ---
>>>>>    drivers/clk/qcom/gdsc.c | 6 +++++-
>>>>>    drivers/clk/qcom/gdsc.h | 1 +
>>>>>    2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>>>> index 7e1dd8ccfa38..e7b213450640 100644
>>>>> --- a/drivers/clk/qcom/gdsc.c
>>>>> +++ b/drivers/clk/qcom/gdsc.c
>>>>> @@ -380,7 +380,11 @@ static int gdsc_init(struct gdsc *sc)
>>>>>          */
>>>>>         mask = HW_CONTROL_MASK | SW_OVERRIDE_MASK |
>>>>>                EN_REST_WAIT_MASK | EN_FEW_WAIT_MASK | CLK_DIS_WAIT_MASK;
>>>>> -     val = EN_REST_WAIT_VAL | EN_FEW_WAIT_VAL | CLK_DIS_WAIT_VAL;
>>>>> +
>>>>> +     regmap_read(sc->regmap, sc->gdscr, &val);
>>>>> +
>>>>> +     if (!(sc->flags & DEFAULT_TRANSITION_DELAY))
>>>>
>>>> I dug a little bit more into this and noticed that on various platforms
>>>> CLK_DIS_WAIT_VAL for the GPU_CX GDSC is supposed to be 8 (whereas both
>>>> hw default and CLK_DIS_WAIT_VAL is 2).
>>>>
>>
>> Yes, only for certain GPU_CC these would be updated and that too in case the
>> design team suggests. Downstream we would set the value from probe itself,
>> or we can pick these from device tree as required and suggested.
>>
> 
> I don't expect that value to be "configurable", so pushing it to DT
> doesn't seem like the proper solution.
> 
>>>> I'm not able to find anything helpful in the git log describing what the
>>>> value does, but it seems that a "just use hw default" flag won't cut it
>>>> for this scenario.
>>>>
>>
>> This value is used for the number of clock cycles it would wait before the
>> GDSCR ACK signals/halting the clock.
>>
> 
> That makes sense.
> 
>>>
>>> I'd prefer we invert the logic so that we don't need to litter this flag
>>> all over the place. I recall that the wait values were incorrect a long
>>> time ago on early gdsc using designs but hopefully they've been fixed
>>> now and we can simply use the default power on reset (POR) values.
>>
>> I am okay to invert the logic, but I am not sure if they could cause any
>> issues to the older targets. They were broken in HW design long back, but
>> got fixed in most families of target and most GDSCs.
>>
> 
> I don't fancy us having a flag with the purpose of "don't set the
> timings to 2, 8 and 2" and then rely on open coded writes in probe to
> set it to something else where needed.
> 
> So I definitely would prefer to flip this around, to make the cases
> where we want to write different values explicit.
> 
> But as you say, unless we make sure that all existing platforms do write
> 2, 8 and 2 we risk introducing regressions from the current behavior.
> 
>> As mentioned if explicitly they need to be updated, it is best to do from
>> the probe.
>> This was done in SC7180 GPUCC driver.
>>          /* Configure clk_dis_wait for gpu_cx_gdsc */
>>          regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
>>                                          8 << CLK_DIS_WAIT_SHIFT);
> 
> But we call regmap_update_bits() from probe, which sets the CLK_DIS_WAIT
> to 8, then we call qcom_cc_really_probe() which will end up in
> gdsc_init() which replaces that with a 2.
> 
> Perhaps I'm missing something?
> 

It was my miss when I did a cleanup to move the DIS_WAIT_VAL before 
registering the clocks.

>>
>>
>> Please let me know if we are okay to add the invert logic.
>>
> 
> I'm still favoring a scheme where we add 3 integers to struct gdsc and
> in gdsc_init() we check if they are non-zero we write the value to the
> register.
> 
Sure, will update the gdsc_init() to default "2, 8, 2" in case of 
non-zero value.

> Although being a big patch, we could maintain the existing behaviour by
> giving all existing struct gdsc definitions the values 2, 8 and 2 to
> avoid regressions and we (everyone) can then go through the platforms
> one by one and remove the unnecessary assignments - but more
> importantly, double check with downstream if they need a different
> value.
> 

Sure, that would help.

> This will also have the side effect going forward that it will be more
> explicit and people writing gdsc definitions should double check these
> values.
> 
> Regards,
> Bjorn

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--