| Message ID | 20191031185715.15504-1-jeffrey.l.hugo@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | MSM8998 GPUCC Support | expand |
On Thu, 2019-10-31 at 11:57 -0700, Jeffrey Hugo wrote: > Some RCGs (the gfx_3d_src_clk in msm8998 for example) are basically just > some constant ratio from the input across the entire frequency range. It > would be great if we could specify the frequency table as a single entry > constant ratio instead of a long list, ie: > > { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }, > { } > > So, lets support that. [] > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c [] > @@ -29,6 +29,9 @@ struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate) > if (!f) > return NULL; > > + if(!f->freq) > + return f; > + trivia: Space after if before open parenthesis please. Can you please make sure to style check your code with checkpatch before submission? Thanks.
Quoting Jeffrey Hugo (2019-10-31 11:57:15) > Some RCGs (the gfx_3d_src_clk in msm8998 for example) are basically just > some constant ratio from the input across the entire frequency range. It > would be great if we could specify the frequency table as a single entry > constant ratio instead of a long list, ie: > > { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }, > { } > > So, lets support that. > > We need to fix a corner case in qcom_find_freq() where if the freq table > is non-null, but has no frequencies, we end up returning an "entry" before > the table array, which is bad. Then, we need ignore the freq from the > table, and instead base everything on the requested freq. > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- Applied to clk-next and fixed the space thing. I guess ceil/floor rounding isn't a problem?
On Thu, Nov 7, 2019 at 2:43 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Jeffrey Hugo (2019-10-31 11:57:15) > > Some RCGs (the gfx_3d_src_clk in msm8998 for example) are basically just > > some constant ratio from the input across the entire frequency range. It > > would be great if we could specify the frequency table as a single entry > > constant ratio instead of a long list, ie: > > > > { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }, > > { } > > > > So, lets support that. > > > > We need to fix a corner case in qcom_find_freq() where if the freq table > > is non-null, but has no frequencies, we end up returning an "entry" before > > the table array, which is bad. Then, we need ignore the freq from the > > table, and instead base everything on the requested freq. > > > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > --- > > Applied to clk-next and fixed the space thing. I guess ceil/floor > rounding isn't a problem? > Thanks for fixing the nit. Hmm. Looking back at it, floor is only used with the rcg_floor_ops. Right now, you can't use a constant ratio table with rcg_floor_ops - looks like you'd probably hit a null pointer dereference. I'm having trouble seeing how the floor operation would work with this constant ratio idea in a way that would be different than the normal rcg_ops. I think I would say that either you have a good reason for using the constant ratio table, in which case you should be using the normal rcg_ops, or you have a good reason for using floor which is then incompatible with a constant ratio table. If you think that the constant ratio table concept should be applied to floor ops, can you please detail what you expect the behavior to be?
Quoting Jeffrey Hugo (2019-11-07 14:12:09) > On Thu, Nov 7, 2019 at 2:43 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Jeffrey Hugo (2019-10-31 11:57:15) > > > Some RCGs (the gfx_3d_src_clk in msm8998 for example) are basically just > > > some constant ratio from the input across the entire frequency range. It > > > would be great if we could specify the frequency table as a single entry > > > constant ratio instead of a long list, ie: > > > > > > { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }, > > > { } > > > > > > So, lets support that. > > > > > > We need to fix a corner case in qcom_find_freq() where if the freq table > > > is non-null, but has no frequencies, we end up returning an "entry" before > > > the table array, which is bad. Then, we need ignore the freq from the > > > table, and instead base everything on the requested freq. > > > > > > Suggested-by: Stephen Boyd <sboyd@kernel.org> > > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > > --- > > > > Applied to clk-next and fixed the space thing. I guess ceil/floor > > rounding isn't a problem? > > > > Thanks for fixing the nit. > > Hmm. Looking back at it, floor is only used with the rcg_floor_ops. > Right now, you can't use a constant ratio table with rcg_floor_ops - > looks like you'd probably hit a null pointer dereference. I'm having > trouble seeing how the floor operation would work with this constant > ratio idea in a way that would be different than the normal rcg_ops. > I think I would say that either you have a good reason for using the > constant ratio table, in which case you should be using the normal > rcg_ops, or you have a good reason for using floor which is then > incompatible with a constant ratio table. If you think that the > constant ratio table concept should be applied to floor ops, can you > please detail what you expect the behavior to be? I don't think floor ops make sense. I just wanted to make sure that the floor and ceiling stuff in here isn't going to cause problems. Looking again after reading your response I think we're going to be fine.
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c index b98b81ef43a1..5a89ed88cc27 100644 --- a/drivers/clk/qcom/clk-rcg2.c +++ b/drivers/clk/qcom/clk-rcg2.c @@ -220,6 +220,8 @@ static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f, if (clk_flags & CLK_SET_RATE_PARENT) { rate = f->freq; if (f->pre_div) { + if (!rate) + rate = req->rate; rate /= 2; rate *= f->pre_div + 1; } diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index 28ddc747d703..f1a32c5fcb8d 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -29,6 +29,9 @@ struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, unsigned long rate) if (!f) return NULL; + if(!f->freq) + return f; + for (; f->freq; f++) if (rate <= f->freq) return f;
Some RCGs (the gfx_3d_src_clk in msm8998 for example) are basically just some constant ratio from the input across the entire frequency range. It would be great if we could specify the frequency table as a single entry constant ratio instead of a long list, ie: { .src = P_GPUPLL0_OUT_EVEN, .pre_div = 3 }, { } So, lets support that. We need to fix a corner case in qcom_find_freq() where if the freq table is non-null, but has no frequencies, we end up returning an "entry" before the table array, which is bad. Then, we need ignore the freq from the table, and instead base everything on the requested freq. Suggested-by: Stephen Boyd <sboyd@kernel.org> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> --- drivers/clk/qcom/clk-rcg2.c | 2 ++ drivers/clk/qcom/common.c | 3 +++ 2 files changed, 5 insertions(+)