linux-kernel - Re: [PATCH 1/2] clk: bcm2835: Support for clock parent selection

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h9kvw2jp.fsf@eliezer.anholt.net>
Date:	Mon, 09 Nov 2015 08:38:50 -0800
From:	Eric Anholt <eric@...olt.net>
To:	Remi Pommarel <repk@...plefau.lt>
Cc:	Stephen Warren <swarren@...dotorg.org>, Lee Jones <lee@...nel.org>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	linux-rpi-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] clk: bcm2835: Support for clock parent selection

Remi Pommarel <repk@...plefau.lt> writes:

> Hi,
>
> On Wed, Nov 04, 2015 at 06:03:31PM -0800, Eric Anholt wrote:
>
> [...]
>
>> 
>> It looks like you've dropped the use of the divisor off of the PLL
>> channel when setting a rate.  That seems bad for all the other clocks in
>> the system, and a feature we couldn't lose.
>
> Sorry, but I'm not sure to understand your point here. Are you afraid
> that clocks such as PWM, H264, etc, have lost the ability to divide the
> rate from the PLL or oscillator clock they cosume as source ?
>
> If so, I think it's ok. If I'm not wrong here, clk_set_rate() first
> calls clk->determinate_rate() then calls clk->set_rate(). This patch
> makes bcm2835_clock_determine_source() to only select the parent to use
> and does not set the clock's rate itself. The clock's rate is set later
> on when bcm2835_clock_set_parent() is called.
>
> bcm2835_clock_set_parent() still divides the parent rate so we are not
> loosing this feature here.

I see.  You're leaving req->rate as-is, so that it gets passed back in
on the set_rate() call.  Since you've chosen only a parent with a
rate greater than ours, we know we'll be able to divide into it.

This has the downside that anything using the min/max rate clamping
doesn't get to know before setting that we might be out of bounds.

> It would probably be better to have bcm2835_clock_determine_source()
> selects the parent by choosing the one that provides the rate which,
> after being divided, generates the highest but lower rate out of the
> PWM clock itself.
>
> Moreover, if you agree with the above modification I see no reason to
> not call it "bcm2835_clock_determine_rate"

This sounds like what the function should be doing.

Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)