linux-kernel - Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

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

Date:	Thu, 6 Jun 2013 02:09:43 +0200
From:	Heiko Stübner <heiko@...ech.de>
To:	linux-arm-kernel@...ts.infradead.org
Cc:	Mike Turquette <mturquette@...aro.org>,
	Matt Sealey <neko@...uhatsu.net>,
	Stephen Boyd <sboyd@...eaurora.org>,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 3/3] clk: dt: binding for basic divider clock

Am Dienstag, 4. Juni 2013, 21:22:43 schrieb Mike Turquette:
> Quoting Matt Sealey (2013-06-04 10:39:53)
> 
> > On Tue, Jun 4, 2013 at 12:11 PM, Stephen Boyd <sboyd@...eaurora.org> 
wrote:
> > > On 06/03/13 10:53, Mike Turquette wrote:
> > >> +Required properties:
> > >> +- compatible : shall be "divider-clock".
> > >> +- #clock-cells : from common clock binding; shall be set to 0.
> > >> +- clocks : link to phandle of parent clock
> > >> +- reg : base address for register controlling adjustable divider
> > >> +- mask : arbitrary bitmask for programming the adjustable divider
> > >> +
> > >> +Optional properties:
> > >> +- clock-output-names : From common clock binding.
> > >> +- table : array of integer pairs defining divisors & bitfield values
> > >> +- shift : number of bits to shift the mask, defaults to 0 if not
> > >> present +- index_one : valid divisor programming starts at 1, not
> > >> zero +- index_power_of_two : valid divisor programming must be a
> > >> power of two +- allow_zero : implies index_one, and programming zero
> > >> results in +  divide-by-one
> > > 
> > > It's preferred that property names have dashes instead of underscores.
> > 
> > I think I have a suggestion or two here..
> > 
> > I mailed one to Mike earlier, I have had some mailing list problems
> > which I think are solved now..
> > 
> > If you provide a mask for programming the divider, surely the "shift"
> > property is therefore redundant. Unless some device has a completely
> > strange way of doing things and uses two seperated bitfields for
> > programming in the same register, the shift value is simply a function
> > of ffs() on the mask, isn't it? It is nice to put the information
> > specifically in the device tree, but where a shift is not specified
> > it'd probably be a good idea to go about deriving that value that way
> > anyway, right, and if we're doing that.. why specify it?
> 
> Shift is optional.  Also I think the integer values in a DTS are
> 32-bits, so if some day someone had a clock driver that needed to write
> to a 64-bit register then shift might be useful there, combined with a
> 32-bit mask.  Or perhaps dtc will have a 64-bit value for that case?  I
> don't know.
> 
> I certainly do see your point about only using the mask, and the
> original clock basic types did this a while back before the shift+width
> style came into vogue.
> 
> If other reviewers also want to use a pure 32-bit mask and no shift then
> I'll drop it in the next round.

I would object to dropping the shift :-)

If you look at the two clk extensions in my rockchip patches you can see that 
they use a different regiment to set values.

Using the lower 16 bits to set the value and the upper 16 bit to mark which 
values are changed, i.e. (mask << shift + 16) | (val << shift).

(I'm not sure, if the naming or solution could be improved though)


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/