From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> To: Alexandre Belloni <alexandre.belloni@free-electrons.com> Cc: "Mike Turquette" <mturquette@linaro.org>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Antoine Ténart" <antoine.tenart@free-electrons.com> Subject: Re: [PATCH 1/5] clk: berlin: add support for berlin plls Date: Fri, 21 Mar 2014 17:03:52 +0100 [thread overview] Message-ID: <532C62E8.2070803@gmail.com> (raw) In-Reply-To: <20140321154539.GD6443@piout.net> On 03/21/2014 04:45 PM, Alexandre Belloni wrote: > [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. Hmm, maybe I should look up valid vcodiv myself, but your vcodiv_berlin2 has 9 values and I guess they are all valid, aren't they? The next possible, larger mask where 0-8 fits in, is 0xf. You used that above and that reveals 16 possible indices. The only option for shrinking the table that I see, would be min/max allowed indices, but that is as useful as having a slightly larger table. >>> + .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. Maybe, I wasn't clear enough. Just assign .foo_mask and .foo_shift in subsequent lines of code, i.e. static struct berlin_pllmap berlin_pll_map = { .vcodiv = vcodiv_berlin2, .fbdiv_mask = 0x1FF, .fbdiv_shift = 6, .rfdiv_mask = 0x1F, .rfdiv_shift = 1, .divsel_mask = 0xF, .divsel_shift = 7, }; Sebastian
WARNING: multiple messages have this Message-ID (diff)
From: sebastian.hesselbarth@gmail.com (Sebastian Hesselbarth) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/5] clk: berlin: add support for berlin plls Date: Fri, 21 Mar 2014 17:03:52 +0100 [thread overview] Message-ID: <532C62E8.2070803@gmail.com> (raw) In-Reply-To: <20140321154539.GD6443@piout.net> On 03/21/2014 04:45 PM, Alexandre Belloni wrote: > [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. Hmm, maybe I should look up valid vcodiv myself, but your vcodiv_berlin2 has 9 values and I guess they are all valid, aren't they? The next possible, larger mask where 0-8 fits in, is 0xf. You used that above and that reveals 16 possible indices. The only option for shrinking the table that I see, would be min/max allowed indices, but that is as useful as having a slightly larger table. >>> + .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. Maybe, I wasn't clear enough. Just assign .foo_mask and .foo_shift in subsequent lines of code, i.e. static struct berlin_pllmap berlin_pll_map = { .vcodiv = vcodiv_berlin2, .fbdiv_mask = 0x1FF, .fbdiv_shift = 6, .rfdiv_mask = 0x1F, .rfdiv_shift = 1, .divsel_mask = 0xF, .divsel_shift = 7, }; Sebastian
next prev parent reply other threads:[~2014-03-21 15:58 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-03-21 11:43 [PATCH 0/5] berlin: initial support for the clocks Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 11:43 ` [PATCH 1/5] clk: berlin: add support for berlin plls Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 12:49 ` Sebastian Hesselbarth 2014-03-21 12:49 ` Sebastian Hesselbarth 2014-03-21 15:45 ` Alexandre Belloni 2014-03-21 15:45 ` Alexandre Belloni 2014-03-21 15:58 ` Alexandre Belloni 2014-03-21 15:58 ` Alexandre Belloni 2014-03-21 16:03 ` Sebastian Hesselbarth [this message] 2014-03-21 16:03 ` Sebastian Hesselbarth 2014-03-21 16:01 ` Alexandre Belloni 2014-03-21 16:01 ` Alexandre Belloni 2014-03-21 15:56 ` Alexandre Belloni 2014-03-21 15:56 ` Alexandre Belloni 2014-03-21 16:04 ` Sebastian Hesselbarth 2014-03-21 16:04 ` Sebastian Hesselbarth 2014-03-21 11:43 ` [PATCH 2/5] clk: berlin: add berlin clocks DT bindings documentation Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 11:53 ` Mark Rutland 2014-03-21 11:53 ` Mark Rutland 2014-03-21 12:16 ` Sebastian Hesselbarth 2014-03-21 12:16 ` Sebastian Hesselbarth 2014-03-21 11:43 ` [PATCH 3/5] ARM: berlin/dt: add cpupll and syspll support to BG2Q Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 12:11 ` Sebastian Hesselbarth 2014-03-21 12:11 ` Sebastian Hesselbarth 2014-03-21 12:17 ` Alexandre Belloni 2014-03-21 12:17 ` Alexandre Belloni 2014-03-21 12:29 ` Sebastian Hesselbarth 2014-03-21 12:29 ` Sebastian Hesselbarth 2014-03-21 14:54 ` Alexandre Belloni 2014-03-21 14:54 ` Alexandre Belloni 2014-03-21 11:43 ` [PATCH 4/5] ARM: berlin/dt: add cpupll and syspll support to BG2CD Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 12:13 ` Sebastian Hesselbarth 2014-03-21 12:13 ` Sebastian Hesselbarth 2014-03-21 11:43 ` [PATCH 5/5] ARM: berlin/dt: add cpupll and syspll support to BG2 Alexandre Belloni 2014-03-21 11:43 ` Alexandre Belloni 2014-03-21 12:13 ` Sebastian Hesselbarth 2014-03-21 12:13 ` Sebastian Hesselbarth
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=532C62E8.2070803@gmail.com \ --to=sebastian.hesselbarth@gmail.com \ --cc=alexandre.belloni@free-electrons.com \ --cc=antoine.tenart@free-electrons.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mturquette@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.