linux-kernel - Re: [PATCH 1/5] clk: berlin: add support for berlin plls

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]

Message-ID: <20140321154539.GD6443@piout.net>
Date:	Fri, 21 Mar 2014 16:45:39 +0100
From:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc:	Mike Turquette <mturquette@...aro.org>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Antoine Ténart 
	<antoine.tenart@...e-electrons.com>
Subject: Re: [PATCH 1/5] clk: berlin: add support for berlin plls

[all commentis I agree on are snipped]

On 21/03/2014 at 13:49:32 +0100, Sebastian Hesselbarth wrote :
> On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> obj-y += pll.o
> obj-$(CONFIG_MACH_BERLIN_BG2)	+= pll-berlin2.o
> obj-$(CONFIG_MACH_BERLIN_BG2CD)	+= pll-berlin2.o
> obj-$(CONFIG_MACH_BERLIN_BG2Q)	+= pll-berlin2q.o
> 
> Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
> arch/arm/mach-berlin/Kconfig. Can you spin a patch?
> 

I will do that.

> >+static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> >+
> >+static struct berlin_pllmap berlin_pll_map = {
> >+	.vcodiv = vcodiv_berlin2,
> >+	.fbdiv_mask = 0x1FF,
> >+	.rfdiv_mask = 0x1F,
> >+	.divsel_mask = 0xF,
> 
> divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
> 9, and common pll driver below does not know how many valid vcodiv
> values are passed. That can become dangerous..
> 
> I'd rather extend vcodiv_berlin2 to full divsel range and provide
> safe (=1) divisiors. This way wrong/new register values will only
> break clock frequency derived.
> 

Good catch ! Then, what about simply shrinking the mask so that we don't
overflow the table. We'll put it back to its supposed real value whant
we know what are the remaining divisors (my guess is that they are already
all listed here). I would say that we are getting the divisor wrong if
divsel > 8 anyway.

> >+	.fbdiv_shift = 6,
> >+	.rfdiv_shift = 1,
> >+	.divsel_shift = 7,
> 
> Have .foo_mask and .foo_shift together?
> 

This will make the struct larger but I don't really have an opinion.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/