Re: [PATCH 2/4] clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845

[Amit Nischal]

On 2018-08-08 11:28, Stephen Boyd wrote:
> Quoting Jordan Crouse (2018-08-06 08:04:37)
> > On Mon, Aug 06, 2018 at 02:37:18PM +0530, Amit Nischal wrote:
> > > On 2018-08-03 04:14, Stephen Boyd wrote:
> > > >Quoting Amit Nischal (2018-07-30 04:28:56)
> > > >>On 2018-07-25 12:28, Stephen Boyd wrote:
> > > >>>
> > > >>> Ok. Sounds good! Is the rate range call really needed? It can't be
> > > >>> determined in the PLL code with some table or avoided by making sure
> > > >>> GPU
> > > >>> uses OPP table with only approved frequencies?
> > > >>>
> > > >>
> > > >>Currently fabia PLL code does not have any table to check this and
> > > >>intention
> > > >>was to avoid relying on the client to call set_rate with only approved
> > > >>frequencies so we have added the set_rate_range() call in the GPUCC
> > > >>driver
> > > >>in order to set the rate range.
> > > >>
> > > >
> > > >But GPU will use OPP so it doesn't seem like it really buys us anything
> > > >here. And it really doesn't matter when the clk driver implementation
> > > >doesn't use the min/max to clamp the values of the round_rate()
> > > >call. Is
> > > >that being done here? I need to double check. I would be more convinced
> > > >if the implementation was looking at min/max to constrain the rate
> > > >requested.
> > > >
> > >
> > > So our understanding is that GPU(client) driver will always call the
> > > set_rate with approved frequencies only and we can completely rely
> > > on the
> > > client. Is our understanding is correct?
> >
> >
> > First: on sdm845 the software doesn't set the GPU clocks - we rely on 
> > the GMU
> > firmware to do that on our behalf but for the GPU at least this is an 
> > academic
> > exercise.
>
> So what is this GPU clk driver for then?
>
> > But that said: traditionally we've expected that the clock driver 
> > correctly
> > clamp the requested rate to the correct values. In the past we have 
> > taken
> > advantage of this and we may in the future. I don't think it is 
> > reasonable
> > to require the leaf driver to only pass "approved" frequencies 
> > especially
> > since we depend on our own OPP table that may or may not be similar to 
> > the
> > one used by the clock driver.
>
> Ok. Sounds like things can't be kept in sync between the clk driver and
> the OPP tables. Why is that hard to do?
>
> Either way, I'd be fine if the code actually used the frequency limits
> to round the rate to something within range, but I don't recall seeing
> that being done here. So if the min/max limits stay, the clk driver
> should round to within that range.

Thanks Stephen for your suggestion. I have modified the existing
determine_rate() op to use the min/max limits and round the requested
rate so that it stays withing the set_rate range. I will submit the
same in the next patch series.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html