[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Ron Madrid]

This patch will create a configuration option for a minimum configuration for
the ns16550 serial driver at drivers/serial/ns16550.c which will exclude all
functions with exception of NS16550_putc and NS16550_init.  This will be used
primarily to save space and remove unused code from builds in which space is
limited.

Signed-off-by: Ron Madrid <ron_madrid@sbcglobal.net>
---
 README                   |    7 +++++++
 drivers/serial/ns16550.c |    4 ++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/README b/README
index 132650e..afea6a6 100644
--- a/README
+++ b/README
@@ -2434,6 +2434,13 @@ use the "saveenv" command to store a valid environment.
 - CONFIG_SYS_64BIT_STRTOUL:
 		Adds simple_strtoull that returns a 64bit value
 
+- CONFIG_NS16550_MIN_FUNCTIONS:
+		Define this if you desire to only have use of the NS16550_init
+		and NS16550_putc functions for the serial driver located at
+		drivers/serial/ns16550.c.  This option is useful for saving
+		space for already greatly restricted images, including but not
+		limited to NAND_SPL configurations.
+
 Low Level (hardware related) configuration options:
 ---------------------------------------------------
 
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 93c2243..1b347e9 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -39,6 +39,7 @@ void NS16550_init (NS16550_t com_port, int baud_divisor)
 #endif
 }
 
+#ifndef CONFIG_NS16550_MIN_FUNCTIONS
 void NS16550_reinit (NS16550_t com_port, int baud_divisor)
 {
 	com_port->ier = 0x00;
@@ -53,6 +54,7 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
 	com_port->dlm = (baud_divisor >> 8) & 0xff;
 	com_port->lcr = LCRVAL;
 }
+#endif /* CONFIG_NS16550_MIN_FUNCTIONS */
 
 void NS16550_putc (NS16550_t com_port, char c)
 {
@@ -60,6 +62,7 @@ void NS16550_putc (NS16550_t com_port, char c)
 	com_port->thr = c;
 }
 
+#ifndef CONFIG_NS16550_MIN_FUNCTIONS
 char NS16550_getc (NS16550_t com_port)
 {
 	while ((com_port->lsr & LSR_DR) == 0) {
@@ -76,4 +79,5 @@ int NS16550_tstc (NS16550_t com_port)
 	return ((com_port->lsr & LSR_DR) != 0);
 }
 
+#endif /* CONFIG_NS16550_MIN_FUNCTIONS */
 #endif
-- 
1.5.5.1

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Scott Wood]

Kim Phillips wrote:
> So this patch gives this when CONFIG_NS16550_MIN_FUNCTIONS is set:
> 
> /home/r1aaha/git/u-boot/drivers/serial/serial.c:229: undefined reference to `NS16550_reinit'
> drivers/serial/libserial.a(serial.o): In function `_serial_tstc':
> /home/r1aaha/git/u-boot/drivers/serial/serial.c:220: undefined reference to `NS16550_tstc'
> drivers/serial/libserial.a(serial.o): In function `_serial_getc':
> /home/r1aaha/git/u-boot/drivers/serial/serial.c:214: undefined reference to `NS16550_getc'
> make: *** [u-boot] Error 1

So don't define that symbol if you're going to be using the functions 
that we're trying to trim away.

> this is my proposed fix:
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 1b347e9..9c0055f 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -54,6 +54,8 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
>         com_port->dlm = (baud_divisor >> 8) & 0xff;
>         com_port->lcr = LCRVAL;
>  }
> +#else
> +void NS16550_reinit (NS16550_t com_port, int baud_divisor) { }

No, this adds bytes.  The whole purpose of this is to cut away bytes for 
NAND bootstraps.

-Scott

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Kim Phillips]

On Tue, 17 Feb 2009 12:14:17 -0800
Ron Madrid <ron_madrid@sbcglobal.net> wrote:

> +#ifndef CONFIG_NS16550_MIN_FUNCTIONS
>  void NS16550_reinit (NS16550_t com_port, int baud_divisor)
>  {
>  	com_port->ier = 0x00;
> @@ -53,6 +54,7 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
>  	com_port->dlm = (baud_divisor >> 8) & 0xff;
>  	com_port->lcr = LCRVAL;
>  }
> +#endif /* CONFIG_NS16550_MIN_FUNCTIONS */

So this patch gives this when CONFIG_NS16550_MIN_FUNCTIONS is set:

/home/r1aaha/git/u-boot/drivers/serial/serial.c:229: undefined reference to `NS16550_reinit'
drivers/serial/libserial.a(serial.o): In function `_serial_tstc':
/home/r1aaha/git/u-boot/drivers/serial/serial.c:220: undefined reference to `NS16550_tstc'
drivers/serial/libserial.a(serial.o): In function `_serial_getc':
/home/r1aaha/git/u-boot/drivers/serial/serial.c:214: undefined reference to `NS16550_getc'
make: *** [u-boot] Error 1

this is my proposed fix:

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 1b347e9..9c0055f 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -54,6 +54,8 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
        com_port->dlm = (baud_divisor >> 8) & 0xff;
        com_port->lcr = LCRVAL;
 }
+#else
+void NS16550_reinit (NS16550_t com_port, int baud_divisor) { }
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
 
 void NS16550_putc (NS16550_t com_port, char c)
@@ -79,5 +81,8 @@ int NS16550_tstc (NS16550_t com_port)
        return ((com_port->lsr & LSR_DR) != 0);
 }
 
+#else
+char NS16550_getc (NS16550_t com_port) { }
+int NS16550_tstc (NS16550_t com_port) { }
 #endif /* CONFIG_NS16550_MIN_FUNCTIONS */
 #endif

(not sure if the latter _getc and _tstc functions should be individually
#ifdef-wrapped)

Anyway, this patch gets my ack if the fix is also get applied, because
it gets rid of the current SIMPC8313 NAND bootstrap too big build
failure.

Thanks,

Kim

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Wolfgang Denk]

Dear Kim Phillips,

In message <20090218122152.7fbe1e25.kim.phillips@freescale.com> you wrote:
>
...
> this is my proposed fix:

In addition to the unnecessary overhead for the memory footprint there
are other problems with that patch:

> +#else
> +char NS16550_getc (NS16550_t com_port) { }
> +int NS16550_tstc (NS16550_t com_port) { }

If these functions are supposed to return "char" resp. "int", you will
get casty compiler warnings with an empty function body.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Any excuse will serve a tyrant."                             - Aesop

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Kim Phillips]

On Wed, 18 Feb 2009 12:21:52 -0600
Kim Phillips <kim.phillips@freescale.com> wrote:

> On Tue, 17 Feb 2009 12:14:17 -0800
> Ron Madrid <ron_madrid@sbcglobal.net> wrote:
> 
> > +#ifndef CONFIG_NS16550_MIN_FUNCTIONS

sorry, I forgot to mention that this needs to be a CONFIG_SYS_, i.e,
CONFIG_SYS_NS16550_MIN_FUNCTIONS.

Kim

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Ron Madrid]

--- On Wed, 2/18/09, Kim Phillips <kim.phillips@freescale.com> wrote:

> From: Kim Phillips <kim.phillips@freescale.com>
> Subject: Re: [U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions
> To: "Ron Madrid" <ron_madrid@sbcglobal.net>, u-boot at lists.denx.de
> Date: Wednesday, February 18, 2009, 10:30 AM
> On Wed, 18 Feb 2009 12:21:52 -0600
> Kim Phillips <kim.phillips@freescale.com> wrote:
> 
> > On Tue, 17 Feb 2009 12:14:17 -0800
> > Ron Madrid <ron_madrid@sbcglobal.net> wrote:
> > 
> > > +#ifndef CONFIG_NS16550_MIN_FUNCTIONS
> 
> sorry, I forgot to mention that this needs to be a
> CONFIG_SYS_, i.e,
> CONFIG_SYS_NS16550_MIN_FUNCTIONS.

My mistake.  I was to understand that the CONFIG_SYS_ flavor of options was
for hardware specific defines, that is an option due to a hardware need,
not a simple exclusion of driver functions.  I can redo this patch though.

Ron

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Scott Wood]

Kim Phillips wrote:
> diff --git a/include/ns16550.h b/include/ns16550.h
> index e6ade61..dbaacfc 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -154,6 +154,12 @@ typedef volatile struct NS16550 *NS16550_t;
>  
>  void	NS16550_init   (NS16550_t com_port, int baud_divisor);
>  void	NS16550_putc   (NS16550_t com_port, char c);
> +#ifndef CONFIG_SYS_NS16550_MIN_FUNCTIONS
>  char	NS16550_getc   (NS16550_t com_port);
>  int	NS16550_tstc   (NS16550_t com_port);
>  void	NS16550_reinit (NS16550_t com_port, int baud_divisor);
> +#else
> +#define NS16550_getc(com_port) 0
> +#define NS16550_tstc(com_port) 0
> +#define NS16550_reinit(com_port, baud_divisor)
> +#endif

I think it would be better to get the error than to silently nop these 
functions out.  The point is to eliminate them when they're not used at 
all, such as in the AND bootstrap.

Plus, the above definitions could cause unused variable warnings if 
nothing else consumes the arguments to the macros.

-Scott

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Kim Phillips]

On Wed, 18 Feb 2009 19:29:27 +0100
Wolfgang Denk <wd@denx.de> wrote:

> > +#else
> > +char NS16550_getc (NS16550_t com_port) { }
> > +int NS16550_tstc (NS16550_t com_port) { }
> 
> If these functions are supposed to return "char" resp. "int", you will
> get casty compiler warnings with an empty function body.

I didn't see them, but anyway, please consider this patch (u-boot-spl
is 32 bytes smaller than previous empty-fn-body based patch):

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Ron Madrid]

--- On Wed, 2/18/09, Kim Phillips <kim.phillips@freescale.com> wrote:
> diff --git a/include/configs/SIMPC8313.h
> b/include/configs/SIMPC8313.h
> index f62a705..6219722 100644
> --- a/include/configs/SIMPC8313.h
> +++ b/include/configs/SIMPC8313.h
> @@ -205,6 +205,7 @@
>  #define CONFIG_SYS_NS16550
>  #define CONFIG_SYS_NS16550_SERIAL
>  #define CONFIG_SYS_NS16550_REG_SIZE	1
> +#define CONFIG_SYS_NS16550_MIN_FUNCTIONS

If you case the CONFIG_SYS_NS16550_MIN_FUNCTIONS within a #ifdef
CONFIG_NAND_SPL you don't need the following definitions.

> +#ifndef CONFIG_SYS_NS16550_MIN_FUNCTIONS
>  char	NS16550_getc   (NS16550_t com_port);
>  int	NS16550_tstc   (NS16550_t com_port);
>  void	NS16550_reinit (NS16550_t com_port, int
> baud_divisor);
> +#else
> +#define NS16550_getc(com_port) 0
> +#define NS16550_tstc(com_port) 0
> +#define NS16550_reinit(com_port, baud_divisor)
> +#endif

I was going to send a patch after this one was accepted that takes care of
the SIMPC8313 build.  Should I have sent the two of them together?  I
thought that they would be considered too different.  It would be this
below.

diff --git a/include/configs/SIMPC8313.h b/include/configs/SIMPC8313.h
index a616236..59e43be 100644
--- a/include/configs/SIMPC8313.h
+++ b/include/configs/SIMPC8313.h
@@ -212,6 +212,10 @@
 #define CONFIG_SYS_NS16550_COM1		(CONFIG_SYS_IMMR+0x4500)
 #define CONFIG_SYS_NS16550_COM2		(CONFIG_SYS_IMMR+0x4600)
 
+#ifdef CONFIG_NAND_SPL
+#define CONFIG_NS16550_MIN_FUNCTIONS
+#endif
+
 /* Use the HUSH parser */
 #define CONFIG_SYS_HUSH_PARSER
 #define CONFIG_SYS_PROMPT_HUSH_PS2	"> "

If you'd like, I can send this one now or you could put the new config
option inside of a #ifdef CONFIG_NAND_SPL.

Ron

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Kim Phillips]

On Wed, 18 Feb 2009 12:02:02 -0800 (PST)
Ron Madrid <ron_madrid@sbcglobal.net> wrote:

> --- On Wed, 2/18/09, Kim Phillips <kim.phillips@freescale.com> wrote:
> > diff --git a/include/configs/SIMPC8313.h
> > b/include/configs/SIMPC8313.h
> > index f62a705..6219722 100644
> > --- a/include/configs/SIMPC8313.h
> > +++ b/include/configs/SIMPC8313.h
> > @@ -205,6 +205,7 @@
> >  #define CONFIG_SYS_NS16550
> >  #define CONFIG_SYS_NS16550_SERIAL
> >  #define CONFIG_SYS_NS16550_REG_SIZE	1
> > +#define CONFIG_SYS_NS16550_MIN_FUNCTIONS
> 
> If you case the CONFIG_SYS_NS16550_MIN_FUNCTIONS within a #ifdef
> CONFIG_NAND_SPL you don't need the following definitions.
> 
> > +#ifndef CONFIG_SYS_NS16550_MIN_FUNCTIONS
> >  char	NS16550_getc   (NS16550_t com_port);
> >  int	NS16550_tstc   (NS16550_t com_port);
> >  void	NS16550_reinit (NS16550_t com_port, int
> > baud_divisor);
> > +#else
> > +#define NS16550_getc(com_port) 0
> > +#define NS16550_tstc(com_port) 0
> > +#define NS16550_reinit(com_port, baud_divisor)
> > +#endif
> 
> I was going to send a patch after this one was accepted that takes care of
> the SIMPC8313 build.  Should I have sent the two of them together?  I
> thought that they would be considered too different.  It would be this
> below.
> 
> diff --git a/include/configs/SIMPC8313.h b/include/configs/SIMPC8313.h
> index a616236..59e43be 100644
> --- a/include/configs/SIMPC8313.h
> +++ b/include/configs/SIMPC8313.h
> @@ -212,6 +212,10 @@
>  #define CONFIG_SYS_NS16550_COM1		(CONFIG_SYS_IMMR+0x4500)
>  #define CONFIG_SYS_NS16550_COM2		(CONFIG_SYS_IMMR+0x4600)
>  
> +#ifdef CONFIG_NAND_SPL
> +#define CONFIG_NS16550_MIN_FUNCTIONS

I think you're right - this really needs to be a CONFIG_ without SYS_.
Sorry.

> If you'd like, I can send this one now or you could put the new config
> option inside of a #ifdef CONFIG_NAND_SPL.

one more time:

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Wolfgang Denk]

Dear Ron,

In message <294428.23511.qm@web83508.mail.sp1.yahoo.com> you wrote:
> --- On Wed, 2/18/09, Kim Phillips <kim.phillips@freescale.com> wrote:
...
> > sorry, I forgot to mention that this needs to be a
> > CONFIG_SYS_, i.e,
> > CONFIG_SYS_NS16550_MIN_FUNCTIONS.
> 
> My mistake.  I was to understand that the CONFIG_SYS_ flavor of options was
> for hardware specific defines, that is an option due to a hardware need,
> not a simple exclusion of driver functions.  I can redo this patch though.

You are correct. This is a CONFIG_ thingy, not a CONFIG_SYS_ one.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Eureka!                                                 -- Archimedes

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Wolfgang Denk]

Dear Ron Madrid,

In message <82808.89665.qm@web83505.mail.sp1.yahoo.com> you wrote:
>
> I was going to send a patch after this one was accepted that takes care of
> the SIMPC8313 build.  Should I have sent the two of them together?  I
> thought that they would be considered too different.  It would be this
> below.

They should go together, or we have problems when bisecting.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Life. Don't talk to me about life.      - Marvin the Paranoid Android

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Kim Phillips]

On Wed, 18 Feb 2009 23:11:30 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Ron Madrid,
> 
> In message <82808.89665.qm@web83505.mail.sp1.yahoo.com> you wrote:
> >
> > I was going to send a patch after this one was accepted that takes care of
> > the SIMPC8313 build.  Should I have sent the two of them together?  I
> > thought that they would be considered too different.  It would be this
> > below.
> 
> They should go together, or we have problems when bisecting.

not really a problem, just giving bisect a miniscule dent in the number
of iterations it would have to go through to resolve a problem.

I've resent the patch with sCONFIG_SYS/CONFIG_/g and merged the enable
on the board changes in this email thread.  Let me know if you want it
to go through the mpc83xx tree.

Kim

[U-Boot] [PATCH v2] Create configuration option for restricted ns16550 functions

[Wolfgang Denk]

Dear Kim Phillips,

In message <20090218172300.bba957f2.kim.phillips@freescale.com> you wrote:
> 
> I've resent the patch with sCONFIG_SYS/CONFIG_/g and merged the enable
> on the board changes in this email thread.  Let me know if you want it
> to go through the mpc83xx tree.

Not needed, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Hindsight is an exact science.