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

[Jordan Crouse]

On Tue, Aug 07, 2018 at 10:58:19PM -0700, 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?

That is a good question. There is a hodgepodge of clocks for the GMU, GPU and
SMMU and I'm not sure which ones are provided by the various clk drivers. This
isn't my area of expertise.

The GMU uses:

GPU_CC_CX_GMU_CLK
GPU_CC_CXO_CLK
GCC_DDRSS_GPU_AXI_CLK
GCC_GPU_MEMNOC_GFX_CLK

These are controlled by the GMU device.

The SMMU uses:

GCC_GPU_CFG_AHB_CLK
GCC_DDRSS_GPU_AXI_CLK
GCC_GPU_MEMNOC_GFX_CLK

These should be controlled by the SMMU device.

Downstream defines these drivers for the GPU but we don't get/prepare/use
them for the GPU device - I think they are there in case the GMU isn't
working or is disabled for some other reason.

GPU_CC_GX_GFX3D_CLK
GPU_CC_CXO_CLK
GCC_DDRSS_GPU_AXI_CLK
GCC_GPU_MEMNOC_GFX_CLK
GPU_CC_CX_GMU_CLK

> > 
> > 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?

Again, not my area of expertise. Traditionally the leaf driver is responsible
for setting its own OPP table. I'm not sure if the clock driver can or should
be in the role of switching up the table. We've always assumed that the clk
driver will sanity check whatever we ask it for.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project