Patchwork [U-Boot,v2,4/7] Serial: ns16550: Add support for CONFIG_SYS_NS16550_IER macro

login
register
mail settings
Submitter Prafulla Wadaskar
Date Nov. 30, 2010, 11:02 a.m.
Message ID <1291114965-17100-5-git-send-email-prafulla@marvell.com>
Download mbox | patch
Permalink /patch/73569/
State Changes Requested
Headers show

Comments

Prafulla Wadaskar - Nov. 30, 2010, 11:02 a.m.
On some processors this ier register configuration is different
for ex. Marvell Armada100

This patch introduce CONFIG_SYS_NS16550_IER macro support to
unconditionally initialize this register.

Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
---
 drivers/serial/ns16550.c |    4 ++--
 include/ns16550.h        |    4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)
Wolfgang Denk - Dec. 1, 2010, 7:44 a.m.
Dear Prafulla Wadaskar,

In message <1291114965-17100-5-git-send-email-prafulla@marvell.com> you wrote:
> On some processors this ier register configuration is different
> for ex. Marvell Armada100
> 
> This patch introduce CONFIG_SYS_NS16550_IER macro support to
> unconditionally initialize this register.

Sorry, but I don't like an implementation detail:

>  void NS16550_init (NS16550_t com_port, int baud_divisor)
>  {
> -	serial_out(0x00, &com_port->ier);
> +	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
>  #if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
>  	serial_out(0x7, &com_port->mdr1);	/* mode select reset TL16C750*/
>  #endif
> @@ -52,7 +52,7 @@ void NS16550_init (NS16550_t com_port, int baud_divisor)
>  #ifndef CONFIG_NS16550_MIN_FUNCTIONS
>  void NS16550_reinit (NS16550_t com_port, int baud_divisor)
>  {
> -	serial_out(0x00, &com_port->ier);
> +	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
>  	serial_out(UART_LCR_BKSE | UART_LCRVAL, &com_port->lcr);
>  	serial_out(0, &com_port->dll);
>  	serial_out(0, &com_port->dlm);
> diff --git a/include/ns16550.h b/include/ns16550.h
> index 9ea81e9..e9cb449 100644
> --- a/include/ns16550.h
> +++ b/include/ns16550.h
> @@ -33,6 +33,10 @@
>  	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
>  #endif
>  
> +#ifndef CONFIG_SYS_NS16550_IER
> +#define CONFIG_SYS_NS16550_IER	0x00
> +#endif /* CONFIG_SYS_NS16550_IER */

As this macro is only used in drivers/serial/ns16550.c, it makes no
sense to me to move this to ns16550.h - this makes the code just
harder to read.  Please move this #ifndef... to
drivers/serial/ns16550.c

Also, please add a description of the new CONFIG_ variable to the
README file.

Thanks.

Best regards,

Wolfgang Denk
Prafulla Wadaskar - Dec. 1, 2010, 7:52 a.m.
> -----Original Message-----
> From: Wolfgang Denk [mailto:wd@denx.de]
> Sent: Wednesday, December 01, 2010 1:15 PM
> To: Prafulla Wadaskar
> Cc: u-boot@lists.denx.de; Eric Miao; Manas Saksena; Lei Wen; Yu Tang;
> Ashish Karkare; Kiran Vedere; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH v2 4/7] Serial: ns16550: Add support for
> CONFIG_SYS_NS16550_IER macro
> 
> Dear Prafulla Wadaskar,
> 
> In message <1291114965-17100-5-git-send-email-prafulla@marvell.com> you
> wrote:
> > On some processors this ier register configuration is different
> > for ex. Marvell Armada100
> >
> > This patch introduce CONFIG_SYS_NS16550_IER macro support to
> > unconditionally initialize this register.
> 
> Sorry, but I don't like an implementation detail:
> 
> >  void NS16550_init (NS16550_t com_port, int baud_divisor)
> >  {
> > -	serial_out(0x00, &com_port->ier);
> > +	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
> >  #if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
> >  	serial_out(0x7, &com_port->mdr1);	/* mode select reset TL16C750*/
> >  #endif
> > @@ -52,7 +52,7 @@ void NS16550_init (NS16550_t com_port, int
> baud_divisor)
> >  #ifndef CONFIG_NS16550_MIN_FUNCTIONS
> >  void NS16550_reinit (NS16550_t com_port, int baud_divisor)
> >  {
> > -	serial_out(0x00, &com_port->ier);
> > +	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
> >  	serial_out(UART_LCR_BKSE | UART_LCRVAL, &com_port->lcr);
> >  	serial_out(0, &com_port->dll);
> >  	serial_out(0, &com_port->dlm);
> > diff --git a/include/ns16550.h b/include/ns16550.h
> > index 9ea81e9..e9cb449 100644
> > --- a/include/ns16550.h
> > +++ b/include/ns16550.h
> > @@ -33,6 +33,10 @@
> >  	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
> >  #endif
> >
> > +#ifndef CONFIG_SYS_NS16550_IER
> > +#define CONFIG_SYS_NS16550_IER	0x00
> > +#endif /* CONFIG_SYS_NS16550_IER */
> 
> As this macro is only used in drivers/serial/ns16550.c, it makes no
> sense to me to move this to ns16550.h - this makes the code just
> harder to read.  Please move this #ifndef... to
> drivers/serial/ns16550.c

Ack, will be done in v3

> 
> Also, please add a description of the new CONFIG_ variable to the
> README file.

Ack,

Thanks and regards..
Prafulla . .

Patch

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 32f24de..8dcfcb9 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -26,7 +26,7 @@ 
 
 void NS16550_init (NS16550_t com_port, int baud_divisor)
 {
-	serial_out(0x00, &com_port->ier);
+	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
 #if defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)
 	serial_out(0x7, &com_port->mdr1);	/* mode select reset TL16C750*/
 #endif
@@ -52,7 +52,7 @@  void NS16550_init (NS16550_t com_port, int baud_divisor)
 #ifndef CONFIG_NS16550_MIN_FUNCTIONS
 void NS16550_reinit (NS16550_t com_port, int baud_divisor)
 {
-	serial_out(0x00, &com_port->ier);
+	serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);
 	serial_out(UART_LCR_BKSE | UART_LCRVAL, &com_port->lcr);
 	serial_out(0, &com_port->dll);
 	serial_out(0, &com_port->dlm);
diff --git a/include/ns16550.h b/include/ns16550.h
index 9ea81e9..e9cb449 100644
--- a/include/ns16550.h
+++ b/include/ns16550.h
@@ -33,6 +33,10 @@ 
 	unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
 #endif
 
+#ifndef CONFIG_SYS_NS16550_IER
+#define CONFIG_SYS_NS16550_IER	0x00
+#endif /* CONFIG_SYS_NS16550_IER */
+
 struct NS16550 {
 	UART_REG(rbr);		/* 0 */
 	UART_REG(ier);		/* 1 */