Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday, July 12, 2016 11:18:39 AM CEST Michael Turquette wrote:
> Quoting Arnd Bergmann (2016-07-12 01:51:36)
> > On Monday, July 11, 2016 3:00:13 PM CEST Michael Turquette wrote:
> > > Quoting Arnd Bergmann (2016-07-11 13:21:17)
> > > > On Thursday, July 7, 2016 7:10:30 PM CEST Michael Turquette wrote:
> > A similar problem is the patch below
> > that was just added: what in the world does the clk driver care about
> > the settings that the bootloader sets? If something comes from the
> > bootloader, the driver should get it from the DT rather than hardcode it.
> 
> Clock drivers are hard. Strictly speaking, any clock that is consumed
> and controlled by a clock consumer driver in Linux should *not* need the
> kind of hack you see below in the clock consumer driver. The best fix is
> for a display driver to call clk_get() and clk_set_rate() on the clock.

Makes sense.

> However there are two examples of where this doesn't work:
> 
> 1) There is no consumer driver (e.g. DDR)
> 2) Support is coming for the consumer driver Some Day(tm), but we want
> things to work for now.

I have some vague memory that we talked about initializing clocks
from values in DT at some point, which would avoid the need for
hardcoding them in the driver. Am I misremembering that, or is it
something that just never happened?

> The hi6220 clk provider driver was already setting the PLLs, and the
> patch below merely tweaks the chosen rate. That's why I accepted it.
> Clearly it would be better for the PLL rate to set by the display
> driver.

Right, my comment wasn't about the fact that you merged this patch
on top, but rather about the fact that the driver started out by
hardcoding clockrates that are assumed to match whatever the boot
loader sets.

	Arnd