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

Taniya Das
2022-02-22T11:23:40Z

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@codeaurora.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.

--