On Fri, Apr 30, 2010 at 5:19 AM, Anatolij Gustschin <agust@denx.de> wrote:
>> How about just doing this?>>>> .init_early = mpc512x_init_diu,>> I thought it should be prepared for adding other code here.> mpc5121_ads_init_early() is generic and could contain other> things as well. I would vote for current version.
Do you have any plans to add any additional code? If not, then I say
skip the middle-man. If someone ever needs to do more, he can always
put that function back.
>> I'm pretty sure the compiler will optimize this to:>>>> temp = (1000000000000UL / pixclock);>>>> so you may as well do it that way.>> ??> 1000000000000 is _not_ UL, but UUL.
That's what I meant. Actually, I think it's ULL. Regardless, I think
the compiler will see the "1000000000 ... * 1000" and just combine
them together. You're not actually outsmarting the compiler.
>> > + err = 100000000;>>>> Why do you assign err to this arbitrary value?>> Dunno. It is Freescale's code and I do not have time to check> and understand each bit of it and to explain it.
*sigh* You're not the first person to modify the DIU driver without
understanding what it all does. I suspect that the original author
should have done this:
err = -1;
because he wanted it to be the largest possible integer.
>> Do you really need to use reserve_bootmem? Have you tried kmalloc or>> alloc_pages_exact()?>> Yes. No, it is too early to use them here.
There was a recent change in the kernel that allows kmalloc to work
earlier than before. Take a look at commit
85355bb272db31a3f2dd99d547eef794805e1319 ("powerpc: Fix mpic alloc
warning").
>> > + mode = in_be32(diu_reg + 0x1c);>> > + if (mode != 1) {>>>> How can in_be32() return a -1?>> It is a 1, not -1. I will use appropriate macro here and also> change to use a struct instead of adding offset to register base.
Sorry, I misread the code. I could have sworn it read "mode != -1"

On Fri, Apr 30, 2010 at 10:08:45AM -0500, Timur Tabi wrote:
> On Fri, Apr 30, 2010 at 5:19 AM, Anatolij Gustschin <agust@denx.de> wrote:> > >> How about just doing this?> >>> >> .init_early = mpc512x_init_diu,> >> > I thought it should be prepared for adding other code here.> > mpc5121_ads_init_early() is generic and could contain other> > things as well. I would vote for current version.> > Do you have any plans to add any additional code? If not, then I say> skip the middle-man. If someone ever needs to do more, he can always> put that function back.> > >> I'm pretty sure the compiler will optimize this to:> >>> >> temp = (1000000000000UL / pixclock);> >>> >> so you may as well do it that way.> >> > ??> > 1000000000000 is _not_ UL, but UUL.> > That's what I meant. Actually, I think it's ULL. Regardless, I think> the compiler will see the "1000000000 ... * 1000" and just combine> them together. You're not actually outsmarting the compiler.
The compiler will do no such thing. That's a valid transformation when
doing pure math, but not when working with integers.
> >> > + err = 100000000;> >>> >> Why do you assign err to this arbitrary value?> >> > Dunno. It is Freescale's code and I do not have time to check> > and understand each bit of it and to explain it.> > *sigh* You're not the first person to modify the DIU driver without> understanding what it all does. I suspect that the original author> should have done this:> > err = -1;> > because he wanted it to be the largest possible integer.
-1 is not the largest possible integer. LONG_MAX, perhaps?
-Scott

On Fri, 30 Apr 2010 10:08:45 -0500
Timur Tabi <timur.tabi@gmail.com> wrote:
> On Fri, Apr 30, 2010 at 5:19 AM, Anatolij Gustschin <agust@denx.de> wrote:> > >> How about just doing this?> >>> >> .init_early = mpc512x_init_diu,> >> > I thought it should be prepared for adding other code here.> > mpc5121_ads_init_early() is generic and could contain other> > things as well. I would vote for current version.> > Do you have any plans to add any additional code? If not, then I say> skip the middle-man. If someone ever needs to do more, he can always> put that function back.
Currently I do not have such plans. Ok will skip them.
...
> >> Do you really need to use reserve_bootmem? Have you tried kmalloc or> >> alloc_pages_exact()?> >> > Yes. No, it is too early to use them here.> > There was a recent change in the kernel that allows kmalloc to work> earlier than before. Take a look at commit> 85355bb272db31a3f2dd99d547eef794805e1319 ("powerpc: Fix mpic alloc> warning").
Thanks. Sorry for my wrong answer above, now I remember the logic
behind this and will try to explain. Actually the reason I do not
use kmalloc() here is that I do not want to _copy_ bitmap data to
newly allocated frame buffer area (It will negatively affect boot
time). Instead I reserve the already configured frame buffer area
so that it won't be destroyed. The starting address of the area
to reserve and also the lenght is passed to reserve_bootmem().
This is the real reason for using reserve_bootmem() here.
I could alloc new bitmap area using allocators, but then I have
to copy the bitmap data (splash image) to newly allocated area
and have to re-configure the descriptors to display from new
bitmap buffer.
Anatolij

On Fri, Apr 30, 2010 at 11:22 AM, Scott Wood <scottwood@freescale.com> wrote:
>> That's what I meant. Actually, I think it's ULL. Regardless, I think>> the compiler will see the "1000000000 ... * 1000" and just combine>> them together. You're not actually outsmarting the compiler.>> The compiler will do no such thing. That's a valid transformation when> doing pure math, but not when working with integers.
I ran some tests, and it appears you're right. I doesn't make a lot
of sense to me, but whatever.
However, "(1000000000 / pixclock) * 1000" produces a result that's
less accurate than "1000000000000ULL / pixclock". Unfortunately, that
math caused a linker problem with __udivdi3 when I tried it, so maybe
you can use do_div() instead?
>> err = -1;>>>> because he wanted it to be the largest possible integer.>> -1 is not the largest possible integer. LONG_MAX, perhaps?
What, you don't like implicit casting of -1 to an unsigned? :-)
Since err is a long integer, LONG_MAX is the better choice.

On Fri, Apr 30, 2010 at 12:00 PM, Anatolij Gustschin <agust@denx.de> wrote:
> Thanks. Sorry for my wrong answer above, now I remember the logic> behind this and will try to explain. Actually the reason I do not> use kmalloc() here is that I do not want to _copy_ bitmap data to> newly allocated frame buffer area (It will negatively affect boot> time). Instead I reserve the already configured frame buffer area> so that it won't be destroyed. The starting address of the area> to reserve and also the lenght is passed to reserve_bootmem().> This is the real reason for using reserve_bootmem() here.> I could alloc new bitmap area using allocators, but then I have> to copy the bitmap data (splash image) to newly allocated area> and have to re-configure the descriptors to display from new> bitmap buffer.
Ok, I understand. Please add this comment to the code, so that no one
else will wonder what you're doing.

Timur Tabi wrote:
> On Fri, Apr 30, 2010 at 11:22 AM, Scott Wood <scottwood@freescale.com> wrote:> >>> That's what I meant. Actually, I think it's ULL. Regardless, I think>>> the compiler will see the "1000000000 ... * 1000" and just combine>>> them together. You're not actually outsmarting the compiler.>> The compiler will do no such thing. That's a valid transformation when>> doing pure math, but not when working with integers.> > I ran some tests, and it appears you're right. I doesn't make a lot> of sense to me, but whatever.> > However, "(1000000000 / pixclock) * 1000" produces a result that's> less accurate than "1000000000000ULL / pixclock".
Precisely, that's what makes it a distinct computation -- as far as the
compiler knows, it could be intentional. Plus, turning it into 64-bit
math would invoke a library call for 64-bit division, which wouldn't be
much of an optimization anyway.
The question is whether the loss of accuracy matters in this case.
>>> err = -1;>>>>>> because he wanted it to be the largest possible integer.>> -1 is not the largest possible integer. LONG_MAX, perhaps?> > What, you don't like implicit casting of -1 to an unsigned? :-)
I like it even less when the variable is signed and it's still supposed
to be larger than positive numbers. :-)
-Scott

On Fri, 30 Apr 2010 15:40:12 -0500
Scott Wood <scottwood@freescale.com> wrote:
> Timur Tabi wrote:> > On Fri, Apr 30, 2010 at 11:22 AM, Scott Wood <scottwood@freescale.com> wrote:> > > >>> That's what I meant. Actually, I think it's ULL. Regardless, I think> >>> the compiler will see the "1000000000 ... * 1000" and just combine> >>> them together. You're not actually outsmarting the compiler.> >> The compiler will do no such thing. That's a valid transformation when> >> doing pure math, but not when working with integers.> > > > I ran some tests, and it appears you're right. I doesn't make a lot> > of sense to me, but whatever.> > > > However, "(1000000000 / pixclock) * 1000" produces a result that's> > less accurate than "1000000000000ULL / pixclock".> > Precisely, that's what makes it a distinct computation -- as far as the > compiler knows, it could be intentional. Plus, turning it into 64-bit > math would invoke a library call for 64-bit division, which wouldn't be > much of an optimization anyway.> > The question is whether the loss of accuracy matters in this case.
Here, this loss of accuracy doesn't matter at all. Max. possible
loss by this current conversion is 999 HZ compared to conversion
using 64-bit division. Further computation tolerates 5% deviation
for pixclock and selects best possible value. This deviation is
by far greater than 999 HZ. It is 156862 HZ for lowest configurable
pixel clock.
Anatolij