diff mbox series

[v5,1/3] clk: qcom: Allow constant ratio freq tables for rcg

Message ID 20191031185715.15504-1-jeffrey.l.hugo@gmail.com
State New, archived
Headers show
Series MSM8998 GPUCC Support | expand

Commit Message

Jeffrey Hugo Oct. 31, 2019, 6:57 p.m. UTC
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(+)

Comments

Joe Perches Nov. 4, 2019, 6:04 p.m. UTC | #1
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.
Stephen Boyd Nov. 7, 2019, 9:43 p.m. UTC | #2
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?
Jeffrey Hugo Nov. 7, 2019, 10:12 p.m. UTC | #3
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?
Stephen Boyd Nov. 8, 2019, 6:44 a.m. UTC | #4
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 mbox series

Patch

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;