Re: [PATCH v3 3/3] clk: qcom: Add display clock controller driver for SDM845

[Stephen Boyd]

Quoting spanda@xxxxxxxxxxxxxx (2018-07-13 01:25:49)
> On 2018-07-13 01:11, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-07-12 10:21:33)
> >> ++ Display driver team,
> >> 
> >> On 7/9/2018 8:36 PM, Stephen Boyd wrote:
> >> > Quoting Taniya Das (2018-07-09 02:34:07)
> >> >>
> >> >>
> >> >> On 7/9/2018 1:07 PM, Stephen Boyd wrote:
> >> >>> Quoting Taniya Das (2018-07-09 00:07:21)
> >> >>>>
> >> >>>>
> >> >>>> On 7/9/2018 11:46 AM, Stephen Boyd wrote:
> >> >>>>>>
> >> >>>>>>     > Why is the nocache flag needed? Applies to all clks in this file.
> >> >>>>>>     >
> >> >>>>>>
> >> >>>>>> This flag is required for all RCGs whose PLLs are controlled outside the
> >> >>>>>> clock controller. The display code would require the recalculated rate
> >> >>>>>> always.
> >> >>>>>
> >> >>>>> Right. Why is the PLL controlled outside of the clock controller? The
> >> >>>>> rate should propagate upward to the PLL from here, so who's going
> >> >>>>> outside of that?
> >> >>>>>
> >> >>>> The DSI0/1 PLL are not part of the display clock controller, but in the
> >> >>>> display subsystem which are managed by the DRM drivers. When DRM drivers
> >> >>>> query for the rate clock driver should always return the non cached rates.
> >> >>>
> >> >>> Why? Is the DSI PLL changing rate all the time, randomly, without going
> >> >>> through the clk APIs to do so?
> >> >>>
> >> >>
> >> >> Hmm, I am afraid I do not have an answer for this, but this was the
> >> >> requirement to always return the non cached rates from the clock driver.
> >> >>
> >> >
> >> > Ok. Who knows about this requirement? Can we add someone from the
> >> > display driver to understand more?
> >> >
> >> As per my discussions offline with the display teams,
> >> 
> >> There is a use-case where the clock framework is unaware of the PLL 
> >> VCO
> >> frequency change and thus the drivers would query to get the actual HW
> >> frequency rather than the cached one.
> >> 
> >> Do you think keeping these flags would have any impact other than 
> >> always
> >> getting the non-cached rates?
> >> 
> > 
> > The flag will make it so clk_get_rate() works in spite of something
> > changing the frequency behind the framework's back, but I want to
> > understand what and why it's changing without framework involvement. We
> > shouldn't need the flag here, because this flag is typically for clks
> > that are controlled by some other entity that the kernel doesn't have
> > control over. In this case, it seems like we have full control of the
> > clk tree for the display PLL down to this clk, so it should be 
> > perfectly
> > fine to not have this flag. The presence of the flag means that the
> > display driver is doing something wrong.
> 
> These clocks are sourced from DSI PLL. In dsi command mode there is an 
> idle use case when there
> is no activity on display, we switch of the source DSI PLL to save power 
> by calling clk_disable_unprepare().
> In this scenario if some client queries the clk_get_rate(), then if 
> NO_CACHE flag is used the clk
> driver will return the cached rate which is some non-zero value set when 
> we last called clk_set_rate(),
> before enabling the clock, whereas actually in HW this clk is disabled. 

If the clk is disabled in hardware it doesn't mean clk_get_rate() should
return 0 Hz. The frequency of the clk is set to something, so we should
return what that frequency is by reading the hardware.

> So we used the NO_CACHE flag
> for the call to land in clk_recalc_rate and we return zero if the PLL is 
> disabled.

This is super wrong. If the PLL is disabled it still has some frequency
configured.