On Thu, 2010-10-28 at 14:19 -0700, Stephen Boyd wrote:
> +
> +static void (*delay_fn)(unsigned long) = delay_loop;
> +
> +void set_delay_fn(void (*fn)(unsigned long))
> +{
> + delay_fn = fn;
> +}
This needs to be a static inline in the header file.
> +/*
> + * loops = usecs * HZ * loops_per_jiffy / 1000000
> + */
> +void __delay(unsigned long loops)
> +{
> + delay_fn(loops);
> +}
> EXPORT_SYMBOL(__delay);
Can we make this static inline also? I'm sure about the module issues..
Daniel
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 11/04/2010 12:30 PM, Daniel Walker wrote:
> On Thu, 2010-10-28 at 14:19 -0700, Stephen Boyd wrote:
>> +
>> +static void (*delay_fn)(unsigned long) = delay_loop;
>> +
>> +void set_delay_fn(void (*fn)(unsigned long))
>> +{
>> + delay_fn = fn;
>> +}
>
> This needs to be a static inline in the header file.
Wouldn't that mean delay_fn needs to be exposed in the header file too?
I like the fact that it's static and scoped to this file.
>> +/*
>> + * loops = usecs * HZ * loops_per_jiffy / 1000000
>> + */
>> +void __delay(unsigned long loops)
>> +{
>> + delay_fn(loops);
>> +}
>> EXPORT_SYMBOL(__delay);
>
> Can we make this static inline also? I'm sure about the module issues..
Do you mean in the header file or in this file?
I think it won't work because there actually needs to be a __delay
symbol and it can't just be inlined away at all the call sites.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Thu, 2010-11-04 at 13:58 -0700, Stephen Boyd wrote:
> On 11/04/2010 12:30 PM, Daniel Walker wrote:
> > On Thu, 2010-10-28 at 14:19 -0700, Stephen Boyd wrote:
> >> +
> >> +static void (*delay_fn)(unsigned long) = delay_loop;
> >> +
> >> +void set_delay_fn(void (*fn)(unsigned long))
> >> +{
> >> + delay_fn = fn;
> >> +}
> >
> > This needs to be a static inline in the header file.
>
> Wouldn't that mean delay_fn needs to be exposed in the header file too?
> I like the fact that it's static and scoped to this file.
Yeah you would need to make an extern for that. Why is it better to have
it static in this file?
If you make it inline you should have smaller code size since all your
doing is "delay_fn = fn;" and that's not expected to be used much (if at
all).
> >> +/*
> >> + * loops = usecs * HZ * loops_per_jiffy / 1000000
> >> + */
> >> +void __delay(unsigned long loops)
> >> +{
> >> + delay_fn(loops);
> >> +}
> >> EXPORT_SYMBOL(__delay);
> >
> > Can we make this static inline also? I'm sure about the module issues..
>
> Do you mean in the header file or in this file?
header file.
> I think it won't work because there actually needs to be a __delay
> symbol and it can't just be inlined away at all the call sites.
It would be inlined into delay_fn(loops); . So it's calling a function.
I think you can export the delay_fn symbol and use it that way.
Daniel
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 11/04/2010 02:16 PM, Daniel Walker wrote:
> On Thu, 2010-11-04 at 13:58 -0700, Stephen Boyd wrote:
>> Wouldn't that mean delay_fn needs to be exposed in the header file too?
>> I like the fact that it's static and scoped to this file.
>
> Yeah you would need to make an extern for that. Why is it better to have
> it static in this file?
>
> If you make it inline you should have smaller code size since all your
> doing is "delay_fn = fn;" and that's not expected to be used much (if at
> all).
>
Ok. Doing that increases the size of my vmlinux.
$ size vmlinux.orig vmlinux.new
text data bss dec hex filename
7091426 594512 1244648 8930586 88451a vmlinux.orig
7091514 594512 1244648 8930674 884572 vmlinux.new
Here's the interdiff.
diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
index 5c6b9a3..ef0ea48 100644
--- a/arch/arm/include/asm/delay.h
+++ b/arch/arm/include/asm/delay.h
@@ -8,7 +8,20 @@
#include <asm/param.h> /* HZ */
-extern void __delay(unsigned long loops);
+extern void (*delay_fn)(unsigned long);
+
+static inline void set_delay_fn(void (*fn)(unsigned long))
+{
+ delay_fn = fn;
+}
+
+/*
+ * loops = usecs * HZ * loops_per_jiffy / 1000000
+ */
+static inline void __delay(unsigned long loops)
+{
+ delay_fn(loops);
+}
/*
* This function intentionally does not exist; if you see references to
@@ -40,7 +53,6 @@ extern void __const_udelay(unsigned long);
__const_udelay((n) * ((2199023U*HZ)>>11))) : \
__udelay(n))
-extern void set_delay_fn(void (*fn)(unsigned long));
extern void read_current_timer_delay_loop(unsigned long loops);
#endif /* defined(_ARM_DELAY_H) */
diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index 59fdf64..a4b1ecd 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -44,20 +44,8 @@ void read_current_timer_delay_loop(unsigned long loops)
}
#endif
-static void (*delay_fn)(unsigned long) = delay_loop;
+void (*delay_fn)(unsigned long) = delay_loop;
-void set_delay_fn(void (*fn)(unsigned long))
-{
- delay_fn = fn;
-}
-
-/*
- * loops = usecs * HZ * loops_per_jiffy / 1000000
- */
-void __delay(unsigned long loops)
-{
- delay_fn(loops);
-}
EXPORT_SYMBOL(__delay);
/*
Do you get similar results?
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Fri, 2010-11-05 at 14:51 -0700, Stephen Boyd wrote:
> On 11/04/2010 02:16 PM, Daniel Walker wrote:
> > On Thu, 2010-11-04 at 13:58 -0700, Stephen Boyd wrote:
> >> Wouldn't that mean delay_fn needs to be exposed in the header file too?
> >> I like the fact that it's static and scoped to this file.
> >
> > Yeah you would need to make an extern for that. Why is it better to have
> > it static in this file?
> >
> > If you make it inline you should have smaller code size since all your
> > doing is "delay_fn = fn;" and that's not expected to be used much (if at
> > all).
> >
>
> Ok. Doing that increases the size of my vmlinux.
>
> $ size vmlinux.orig vmlinux.new
> text data bss dec hex filename
> 7091426 594512 1244648 8930586 88451a vmlinux.orig
> 7091514 594512 1244648 8930674 884572 vmlinux.new
This is what I get,
text data bss dec hex filename
2168427 104288 186176 2458891 25850b ../build-test/vmlinux.orig
2168379 104288 186176 2458843 2584db ../build-test/vmlinux.new
Your patch has something wrong with it, which I fixed. Details below,
> Here's the interdiff.
>
> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
> index 5c6b9a3..ef0ea48 100644
> --- a/arch/arm/include/asm/delay.h
> +++ b/arch/arm/include/asm/delay.h
> @@ -8,7 +8,20 @@
>
> #include <asm/param.h> /* HZ */
>
> -extern void __delay(unsigned long loops);
> +extern void (*delay_fn)(unsigned long);
> +
> +static inline void set_delay_fn(void (*fn)(unsigned long))
> +{
> + delay_fn = fn;
> +}
> +
> +/*
> + * loops = usecs * HZ * loops_per_jiffy / 1000000
> + */
> +static inline void __delay(unsigned long loops)
> +{
> + delay_fn(loops);
> +}
>
> /*
> * This function intentionally does not exist; if you see references to
> @@ -40,7 +53,6 @@ extern void __const_udelay(unsigned long);
> __const_udelay((n) * ((2199023U*HZ)>>11))) : \
> __udelay(n))
>
> -extern void set_delay_fn(void (*fn)(unsigned long));
> extern void read_current_timer_delay_loop(unsigned long loops);
>
> #endif /* defined(_ARM_DELAY_H) */
> diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
> index 59fdf64..a4b1ecd 100644
> --- a/arch/arm/lib/delay.c
> +++ b/arch/arm/lib/delay.c
> @@ -44,20 +44,8 @@ void read_current_timer_delay_loop(unsigned long loops)
> }
> #endif
>
> -static void (*delay_fn)(unsigned long) = delay_loop;
> +void (*delay_fn)(unsigned long) = delay_loop;
>
> -void set_delay_fn(void (*fn)(unsigned long))
> -{
> - delay_fn = fn;
> -}
> -
> -/*
> - * loops = usecs * HZ * loops_per_jiffy / 1000000
> - */
> -void __delay(unsigned long loops)
> -{
> - delay_fn(loops);
> -}
> EXPORT_SYMBOL(__delay);
You need to modify this EXPORT_SYMBOL to delay_fn since __delay doesn't
exist anymore.
Daniel
--
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.
On 11/05/2010 04:43 PM, Daniel Walker wrote:
> On Fri, 2010-11-05 at 14:51 -0700, Stephen Boyd wrote:
>> Ok. Doing that increases the size of my vmlinux.
>>
>> $ size vmlinux.orig vmlinux.new
>> text data bss dec hex filename
>> 7091426 594512 1244648 8930586 88451a vmlinux.orig
>> 7091514 594512 1244648 8930674 884572 vmlinux.new
>
> This is what I get,
>
> text data bss dec hex filename
> 2168427 104288 186176 2458891 25850b ../build-test/vmlinux.orig
> 2168379 104288 186176 2458843 2584db ../build-test/vmlinux.new
>
> Your patch has something wrong with it, which I fixed. Details below,
>
[snip]
>> - */
>> -void __delay(unsigned long loops)
>> -{
>> - delay_fn(loops);
>> -}
>> EXPORT_SYMBOL(__delay);
>
> You need to modify this EXPORT_SYMBOL to delay_fn since __delay doesn't
> exist anymore.
Wait. Doesn't this mean we're exporting delay_fn instead of __delay now?
i.e. the symbol name has changed and modules can no longer call __delay?
That sounds bad.
If I make that change, my kernel size is exactly the same before and
after. It may sound like a win since you got a decrease and I got a net
zero, but I'm not sure since the symbol has changed. I could make
__delay a function pointer and assign it directly but I'm not very
interested to expose a function pointer to modules allowing them to
modify it at any time (easily). Actually, I should probably mark
set_delay_fn __init so it gets thrown away after init when its far too
late to switch the delay function anyway. That would give you the space
savings you want and allow me to keep the delay_fn static to delay.c
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Fri, 2010-11-05 at 20:36 -0700, Stephen Boyd wrote:
> On 11/05/2010 04:43 PM, Daniel Walker wrote:
> > On Fri, 2010-11-05 at 14:51 -0700, Stephen Boyd wrote:
> >> Ok. Doing that increases the size of my vmlinux.
> >>
> >> $ size vmlinux.orig vmlinux.new
> >> text data bss dec hex filename
> >> 7091426 594512 1244648 8930586 88451a vmlinux.orig
> >> 7091514 594512 1244648 8930674 884572 vmlinux.new
> >
> > This is what I get,
> >
> > text data bss dec hex filename
> > 2168427 104288 186176 2458891 25850b ../build-test/vmlinux.orig
> > 2168379 104288 186176 2458843 2584db ../build-test/vmlinux.new
> >
> > Your patch has something wrong with it, which I fixed. Details below,
> >
> [snip]
> >> - */
> >> -void __delay(unsigned long loops)
> >> -{
> >> - delay_fn(loops);
> >> -}
> >> EXPORT_SYMBOL(__delay);
> >
> > You need to modify this EXPORT_SYMBOL to delay_fn since __delay doesn't
> > exist anymore.
>
> Wait. Doesn't this mean we're exporting delay_fn instead of __delay now?
> i.e. the symbol name has changed and modules can no longer call __delay?
> That sounds bad.
The modules would just call the new symbol. It would be a problem for
binary modules, but we don't really cater to binary modules. Like you
suggest below you could change the name to __delay().
> If I make that change, my kernel size is exactly the same before and
> after. It may sound like a win since you got a decrease and I got a net
> zero, but I'm not sure since the symbol has changed. I could make
I can't imagine how it's a net zero change for you. The change is
removing two global functions.
> __delay a function pointer and assign it directly but I'm not very
> interested to expose a function pointer to modules allowing them to
> modify it at any time (easily). Actually, I should probably mark
> set_delay_fn __init so it gets thrown away after init when its far too
> late to switch the delay function anyway. That would give you the space
> savings you want and allow me to keep the delay_fn static to delay.c
I don't think we need to protect other code authors to that degree. You
could put down a comment letting people know it's bad to alter __delay
after bootup .. I doubt this API will be used all that often..
Marking it __init only saves run time space it doesn't reduce the image
size. However, since set_delay_fn is likely to be used in __init
sections already, and it's inline, means the code is likely to get
removed in that case too. So doing it the way I'm suggesting give you a
smaller image size, and smaller runtime size.
Daniel
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.