blob-cvs-commit

CVS: blob/src/blob ledasm.S,1.3,1.4

[Christopher H. <ch...@us...>]

Update of /cvsroot/blob/blob/src/blob
In directory usw-pr-cvs1:/tmp/cvs-serv24870

Modified Files:
	ledasm.S 
Log Message:
prefix register offsets with _ so as to not conflict with sa1100.h defines

Index: ledasm.S
===================================================================
RCS file: /cvsroot/blob/blob/src/blob/ledasm.S,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -d -r1.3 -r1.4
--- ledasm.S	28 Oct 2001 20:29:05 -0000	1.3
+++ ledasm.S	25 Jul 2002 17:24:36 -0000	1.4
@@ -33,16 +33,15 @@
 #endif
 
 #include <blob/arch.h>
-
-
+#include <blob/sa1100.h>
 
 .text
 
 LED:		.long LED_GPIO
 GPIO_BASE:	.long 0x90040000
-#define	GPDR	0x00000004
-#define GPSR	0x00000008
-#define GPCR	0x0000000c
+#define	_GPDR	0x00000004
+#define _GPSR	0x00000008
+#define _GPCR	0x0000000c
 
 
 
@@ -54,8 +53,8 @@
 ledinit:
 	ldr	r0, GPIO_BASE
 	ldr	r1, LED
-	str	r1, [r0, #GPDR]		/* LED GPIO is output */
-	str	r1, [r0, #GPSR]		/* turn LED on */
+	str	r1, [r0, #_GPDR]		/* LED GPIO is output */
+	str	r1, [r0, #_GPSR]		/* turn LED on */
 	mov	pc, lr
 
 
@@ -66,7 +65,7 @@
 led_on:
 	ldr	r0, GPIO_BASE
 	ldr	r1, LED
-	str	r1, [r0, #GPSR]
+	str	r1, [r0, #_GPSR]
 	mov	pc, lr
 
 
@@ -77,7 +76,7 @@
 led_off:
 	ldr	r0, GPIO_BASE
 	ldr	r1, LED
-	str	r1, [r0, #GPCR]
+	str	r1, [r0, #_GPCR]
 	mov	pc, lr
 
 

Re: CVS: blob/src/blob ledasm.S,1.3,1.4

[Erik M. <J.A...@it...>]

On Thu, Jul 25, 2002 at 10:24:38AM -0700, Christopher Hoover wrote:
> Update of /cvsroot/blob/blob/src/blob
> In directory usw-pr-cvs1:/tmp/cvs-serv24870
> 
> Modified Files:
> 	ledasm.S 
> Log Message:
> prefix register offsets with _ so as to not conflict with sa1100.h defines
> 
> Index: ledasm.S
> ===================================================================
> RCS file: /cvsroot/blob/blob/src/blob/ledasm.S,v
> retrieving revision 1.3
> retrieving revision 1.4
> diff -u -d -r1.3 -r1.4
> --- ledasm.S	28 Oct 2001 20:29:05 -0000	1.3
> +++ ledasm.S	25 Jul 2002 17:24:36 -0000	1.4
> @@ -33,16 +33,15 @@
>  #endif
>  
>  #include <blob/arch.h>
> -
> -
> +#include <blob/sa1100.h>
   ^^^^^^^^^^^^^^^^^^^^^^^^
Unnecessary include.

>  .text
>  
>  LED:		.long LED_GPIO
>  GPIO_BASE:	.long 0x90040000
> -#define	GPDR	0x00000004
> +#define	_GPDR	0x00000004

Unnecessary cosmetical difference.

It already helps a *lot* when you don't include blob/sa1100.h in the
first place. We don't need it in assembly, I'd rather keep the stuff
readable instead of having unnecessary underscores before every
identifier. Please back out this patch (same for all the other .S
patches).


Erik

-- 
J.A.K. (Erik) Mouw
Email: J.A...@it...
WWW: http://www-ict.its.tudelft.nl/~erik/

RE: CVS: blob/src/blob ledasm.S,1.3,1.4

[Christopher H. <ch...@mu...>]

If you configure and compile for badge4, you'll find that including
sa1100.h in ledasm.S is necessary as badge4.h defines LED_GPIO as
GPIO_GPIOxx.  That seems more perspicuous than a hex constant or even an
expression (1<<x).

But this isn't important and isn't the real reason I made those changes
-- 

I've got some additional assembly that I haven't checked in that depends
on sa1100.h.  Using that header is cleaner than continually duplicating
register bases and offsets already defined in sa1100.h.

Or is it just the _ prefix you don't like?

-ch

> -----Original Message-----
> From: blo...@li... 
> [mailto:blo...@li...] On 
> Behalf Of Erik Mouw
> Sent: Thursday, July 25, 2002 4:27 PM
> To: Christopher Hoover
> Cc: blo...@li...
> Subject: Re: CVS: blob/src/blob ledasm.S,1.3,1.4
> 
> 
> On Thu, Jul 25, 2002 at 10:24:38AM -0700, Christopher Hoover wrote:
> > Update of /cvsroot/blob/blob/src/blob
> > In directory usw-pr-cvs1:/tmp/cvs-serv24870
> > 
> > Modified Files:
> > 	ledasm.S
> > Log Message:
> > prefix register offsets with _ so as to not conflict with 
> sa1100.h defines
> > 
> > Index: ledasm.S 
> > ===================================================================
> > RCS file: /cvsroot/blob/blob/src/blob/ledasm.S,v
> > retrieving revision 1.3
> > retrieving revision 1.4
> > diff -u -d -r1.3 -r1.4
> > --- ledasm.S	28 Oct 2001 20:29:05 -0000	1.3
> > +++ ledasm.S	25 Jul 2002 17:24:36 -0000	1.4
> > @@ -33,16 +33,15 @@
> >  #endif
> >  
> >  #include <blob/arch.h>
> > -
> > -
> > +#include <blob/sa1100.h>
>    ^^^^^^^^^^^^^^^^^^^^^^^^
> Unnecessary include.
> 
> >  .text
> >  
> >  LED:		.long LED_GPIO
> >  GPIO_BASE:	.long 0x90040000
> > -#define	GPDR	0x00000004
> > +#define	_GPDR	0x00000004
> 
> Unnecessary cosmetical difference.
> 
> It already helps a *lot* when you don't include blob/sa1100.h 
> in the first place. We don't need it in assembly, I'd rather 
> keep the stuff readable instead of having unnecessary 
> underscores before every identifier. Please back out this 
> patch (same for all the other .S patches).
> 
> 
> Erik
> 
> -- 
> J.A.K. (Erik) Mouw
> Email: J.A...@it...
> WWW: http://www-ict.its.tudelft.nl/~erik/
> 
> 
> -------------------------------------------------------
> This sf.net email is sponsored by: Jabber - The world's 
> fastest growing 
> real-time communications platform! Don't just IM. Build it in! 
> http://www.jabber.com/osdn/xim 
> _______________________________________________
> blob-cvs-commit mailing list blo...@li...
> https://lists.sourceforge.net/lists/listinfo/blob-cvs-commit
> 

Re: CVS: blob/src/blob ledasm.S,1.3,1.4

[Erik M. <J.A...@it...>]

On Thu, Jul 25, 2002 at 04:56:44PM -0700, Christopher Hoover wrote:
> 
> If you configure and compile for badge4, you'll find that including
> sa1100.h in ledasm.S is necessary as badge4.h defines LED_GPIO as
> GPIO_GPIOxx.  That seems more perspicuous than a hex constant or even an
> expression (1<<x).

See my previous message. IMHO the advantage of cleaner code outweighs
the disadvantage of having to use 1<<x.

> But this isn't important and isn't the real reason I made those changes
> -- 
> 
> I've got some additional assembly that I haven't checked in that depends
> on sa1100.h.

What kind of additional assembly needs to be in the first stage loader?
Unless it does some very creepy things with memory banks, it can be
easily put in the platform specific C files.

The first stage loader is one of the hardest parts of blob *because*
it's written in assembly. I did my utmost to get most of the platform
dependencies out of the first stage loader into the second stage
loader. The second stage loader is easier to understand because it's
written in C.

> Using that header is cleaner than continually duplicating
> register bases and offsets already defined in sa1100.h.

The main problem of SA-1100.h is that it doesn't use offsets. Therefore
it's useless for assembly (see my previous message).

> Or is it just the _ prefix you don't like?

I don't like changes to fragile parts of the code, unless there is a
very good reason for it. So far I haven't seen that reason.


Erik
[who goes zzz right now]

-- 
J.A.K. (Erik) Mouw
Email: J.A...@it...
WWW: http://www-ict.its.tudelft.nl/~erik/