| Message ID | 20210222160300.1811121-5-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: venus: Enable 6xx support | expand |
On 2/22/21 6:02 PM, Bryan O'Donoghue wrote: > From: Dikshita Agarwal <dikshita@codeaurora.org> > > Vote for min clk frequency for core clks during prepare and enable clocks > at boot sequence. Without this the controller clock runs at very low value > (9.6MHz) which is not sufficient to boot venus. > > Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/media/platform/qcom/venus/pm_helpers.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c > index 4f5d42662963..767cb00d4b46 100644 > --- a/drivers/media/platform/qcom/venus/pm_helpers.c > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c > @@ -41,10 +41,24 @@ static int core_clks_get(struct venus_core *core) > static int core_clks_enable(struct venus_core *core) > { > const struct venus_resources *res = core->res; > + const struct freq_tbl *freq_tbl = NULL; > + unsigned int freq_tbl_size = 0; > + unsigned long freq = 0; no need to initialize to zero. > unsigned int i; > int ret; > > + freq_tbl = core->res->freq_tbl; > + freq_tbl_size = core->res->freq_tbl_size; could you initialize those at the variables declaration? > + if (!freq_tbl) > + return -EINVAL; > + > + freq = freq_tbl[freq_tbl_size - 1].freq; > + > for (i = 0; i < res->clks_num; i++) { > + ret = clk_set_rate(core->clks[i], freq); I'm not sure that we have to set the rate for all core->clks[i] ? > + if (ret) > + goto err; > + > ret = clk_prepare_enable(core->clks[i]); > if (ret) > goto err; > -- regards, Stan
On 2/23/21 3:25 PM, Stanimir Varbanov wrote: > > > On 2/22/21 6:02 PM, Bryan O'Donoghue wrote: >> From: Dikshita Agarwal <dikshita@codeaurora.org> >> >> Vote for min clk frequency for core clks during prepare and enable clocks >> at boot sequence. Without this the controller clock runs at very low value >> (9.6MHz) which is not sufficient to boot venus. >> >> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> drivers/media/platform/qcom/venus/pm_helpers.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c >> index 4f5d42662963..767cb00d4b46 100644 >> --- a/drivers/media/platform/qcom/venus/pm_helpers.c >> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c >> @@ -41,10 +41,24 @@ static int core_clks_get(struct venus_core *core) >> static int core_clks_enable(struct venus_core *core) >> { >> const struct venus_resources *res = core->res; >> + const struct freq_tbl *freq_tbl = NULL; >> + unsigned int freq_tbl_size = 0; >> + unsigned long freq = 0; > > no need to initialize to zero. > >> unsigned int i; >> int ret; >> >> + freq_tbl = core->res->freq_tbl; >> + freq_tbl_size = core->res->freq_tbl_size; > > could you initialize those at the variables declaration? > >> + if (!freq_tbl) >> + return -EINVAL; >> + >> + freq = freq_tbl[freq_tbl_size - 1].freq; >> + >> for (i = 0; i < res->clks_num; i++) { >> + ret = clk_set_rate(core->clks[i], freq); > > I'm not sure that we have to set the rate for all core->clks[i] ? Confirmed. This produces regressions on db410c (I haven't tested on other platforms yet). > >> + if (ret) >> + goto err; >> + >> ret = clk_prepare_enable(core->clks[i]); >> if (ret) >> goto err; >> >
On 06/03/2021 15:01, Stanimir Varbanov wrote: > Confirmed. This produces regressions on db410c (I haven't tested on > other platforms yet). db410c was broken for me on the reference branch prior to sm8250 additions however AFAICT this change was fine on sdm845. --- bod
On 3/10/21 7:33 PM, Bryan O'Donoghue wrote: > On 06/03/2021 15:01, Stanimir Varbanov wrote: >> >> >> On 2/23/21 3:25 PM, Stanimir Varbanov wrote: >>> >>> >>> On 2/22/21 6:02 PM, Bryan O'Donoghue wrote: >>>> From: Dikshita Agarwal <dikshita@codeaurora.org> >>>> >>>> Vote for min clk frequency for core clks during prepare and enable >>>> clocks >>>> at boot sequence. Without this the controller clock runs at very low >>>> value >>>> (9.6MHz) which is not sufficient to boot venus. >>>> >>>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org> >>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>>> --- >>>> drivers/media/platform/qcom/venus/pm_helpers.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c >>>> b/drivers/media/platform/qcom/venus/pm_helpers.c >>>> index 4f5d42662963..767cb00d4b46 100644 >>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c >>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c >>>> @@ -41,10 +41,24 @@ static int core_clks_get(struct venus_core *core) >>>> static int core_clks_enable(struct venus_core *core) >>>> { >>>> const struct venus_resources *res = core->res; >>>> + const struct freq_tbl *freq_tbl = NULL; >>>> + unsigned int freq_tbl_size = 0; >>>> + unsigned long freq = 0; >>> >>> no need to initialize to zero. >>> >>>> unsigned int i; >>>> int ret; >>>> + freq_tbl = core->res->freq_tbl; >>>> + freq_tbl_size = core->res->freq_tbl_size; >>> >>> could you initialize those at the variables declaration? >>> >>>> + if (!freq_tbl) >>>> + return -EINVAL; >>>> + >>>> + freq = freq_tbl[freq_tbl_size - 1].freq; >>>> + >>>> for (i = 0; i < res->clks_num; i++) { >>>> + ret = clk_set_rate(core->clks[i], freq); >>> >>> I'm not sure that we have to set the rate for all core->clks[i] ? >> >> Confirmed. This produces regressions on db410c (I haven't tested on >> other platforms yet). >> >>> >>>> + if (ret) >>>> + goto err; >>>> + >>>> ret = clk_prepare_enable(core->clks[i]); >>>> if (ret) >>>> goto err; >>>> >>> >> > > OK, I have this now on db410c > > I made a tree out of > > svarbanov-linux-tv/venus-for-next-v5.13 > + > svarbanov-linux-tv/venus-msm8916-fixes - minor fix here integrating on > top of 5.13 > > and then put the sm8250 changes on top of that > > https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=tracking-qcomlt-sm8250-venus-integrated-sm8250 > > > So confirm db410c works up to tag > tracking-qcomlt-sm8250-venus-integrated-sm8250-02+svarbanov-linux-tv/venus-msm8916-fixes > > > I'll work on fixing your feedback on that putative branch. Thanks! I fixed the regression on db410c by set the rate for the core->clks[0] only, e.g. the clock at which the remote processor is running. > > --- > bod -- regards, Stan
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c index 4f5d42662963..767cb00d4b46 100644 --- a/drivers/media/platform/qcom/venus/pm_helpers.c +++ b/drivers/media/platform/qcom/venus/pm_helpers.c @@ -41,10 +41,24 @@ static int core_clks_get(struct venus_core *core) static int core_clks_enable(struct venus_core *core) { const struct venus_resources *res = core->res; + const struct freq_tbl *freq_tbl = NULL; + unsigned int freq_tbl_size = 0; + unsigned long freq = 0; unsigned int i; int ret; + freq_tbl = core->res->freq_tbl; + freq_tbl_size = core->res->freq_tbl_size; + if (!freq_tbl) + return -EINVAL; + + freq = freq_tbl[freq_tbl_size - 1].freq; + for (i = 0; i < res->clks_num; i++) { + ret = clk_set_rate(core->clks[i], freq); + if (ret) + goto err; + ret = clk_prepare_enable(core->clks[i]); if (ret) goto err;