Messages in this thread | | | Date | Tue, 22 Feb 2022 16:53:29 +0530 | Subject | Re: [v1 1/2] clk: qcom: gdsc: Use the default transition delay for GDSCs | From | Taniya Das <> |
| |
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.
--
| |