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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <5159238.dTvZjzNLNR@wuerfel>
Date:	Thu, 14 Jul 2016 16:30:59 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Michael Turquette <mturquette@...libre.com>
Cc:	John Stultz <john.stultz@...aro.org>,
	Olof Johansson <olof@...om.net>,
	lkml <linux-kernel@...r.kernel.org>,
	"arm@...nel.org" <arm@...nel.org>,
	Stephen Boyd <sboyd@...eaurora.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>, Wei Xu <xuwei5@...ilicon.com>,
	Guodong Xu <guodong.xu@...aro.org>,
	Zhangfei Gao <zhangfei.gao@...aro.org>
Subject: Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220

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