Re: [PATCH 1/3] [ARM] Translate delay.S into (mostly) C

[Stephen Boyd <sboyd@codeaurora.org>]

 On 10/05/2010 10:22 AM, Daniel Walker wrote:
> You shouldn't really reference this series in this comment. You have to
> look at this as a changeset comment. So you really want to describe what
> this change is doing not what the over all series is doing.
>
> It would be better to say something like,
>
> "We're implementing this in C to give us further flexibility in allowing
> overrides."
>
> But don't references "next patch" or "this series" , but you can do that
> in the intro email.
>

Why not? This is a common thing to do when multiple patches are related
to one topic. Doing a git log and searching for "next patch" shows
others doing the same.

>>
>>  $ scripts/bloat-o-meter vmlinux.orig vmlinux.new
>>  add/remove: 0/0 grow/shrink: 2/0 up/down: 12/0 (12)
>>  function                                     old     new   delta
>>  __udelay                                      48      56      +8
>>  __const_udelay                                40      44      +4
>
> I think the "size" command might be better for this type of stuff. It
> should give you the same output (or similar).. It's just used more
> frequently. 

Ok.

$ size vmlinux.orig
   text    data     bss     dec     hex filename
6533503  530232 1228296 8292031  7e86bf vmlinux.orig
$ size vmlinux.new
   text    data     bss     dec     hex filename
6533607  530232 1228296 8292135  7e8727 vmlinux.new

I should mention GCC decided to inline __delay() into __udelay() and
__const_udelay() and also decided to inline __const_udelay() into
__delay() meaning we lost the function interleaving the assembly file
had. I'll make a note of that and sorry for being misleading.

> Is this an -Os kernel, or -O2 ?

-Os.

>> +/*
>> + * 0 <= xloops <= 0x7fffff06
>> + * loops_per_jiffy <= 0x01ffffff (max. 3355 bogomips)
>
> As long as your doing a re-write may as well add some real text here,
> and for the others.

Any suggestions on what to add? I hoped the original comments would be
good enough already considering the code is unchanged.

>> + */
>> +void __const_udelay(unsigned long xloops)
>> +{
>> +	unsigned long lpj;
>> +	unsigned long loops;
>> +
>> +	xloops >>= 14;			/* max = 0x01ffffff */
>> +	lpj = loops_per_jiffy >> 10;	/* max = 0x0001ffff */
>> +	loops = lpj * xloops;		/* max = 0x00007fff */
>> +	loops >>= 6;			/* max = 2^32-1 */
>> +
>> +	if (loops)
>> +		__delay(loops);
>
> likely/unlikely ?

likely. Although I'm doubtful on how much it will help considering the
assembly and the code are equivalent already for this part of the code.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.