[U-Boot] [PATCH 04/31] powerpc: 82xx serial: add configurable SMC Rx buffer len

[U-Boot] [PATCH 04/31] powerpc: 82xx serial: add configurable SMC Rx buffer len

[Heiko Schocher]

This patch adds the configuration option CONFIG_SYS_SMC_RXBUFLEN.
With this option it is possible to allow the receive
buffer for the SMC on 82xx to be greater then 1. In case
CONFIG_SYS_SMC_RXBUFLEN == 1 or it is not defined this driver
works as the old version.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 cpu/mpc8260/serial_smc.c |   94 +++++++++++++++++++++++++++++++---------------
 include/configs/mgcoge.h |    1 +
 2 files changed, 64 insertions(+), 31 deletions(-)

diff --git a/cpu/mpc8260/serial_smc.c b/cpu/mpc8260/serial_smc.c
index a6efa66..6825e2e 100644
--- a/cpu/mpc8260/serial_smc.c
+++ b/cpu/mpc8260/serial_smc.c
@@ -64,6 +64,18 @@ DECLARE_GLOBAL_DATA_PTR;

 #endif

+typedef volatile struct serialbuffer {
+	cbd_t	rxbd;		/* Rx BD */
+	cbd_t	txbd;		/* Tx BD */
+#ifdef CONFIG_SYS_SMC_RXBUFLEN
+	uint	rxindex;	/* index for next character to read */
+	volatile uchar	rxbuf[CONFIG_SYS_SMC_RXBUFLEN];/* rx buffers */
+#else
+	volatile uchar	rxbuf[1];	/* rx buffers */
+#endif
+	volatile uchar	txbuf;	/* tx buffers */
+} serialbuffer_t;
+
 /* map rs_table index to baud rate generator index */
 static unsigned char brg_map[] = {
 	6,	/* BRG7 for SMC1 */
@@ -79,9 +91,9 @@ int serial_init (void)
 	volatile immap_t *im = (immap_t *)CONFIG_SYS_IMMR;
 	volatile smc_t *sp;
 	volatile smc_uart_t *up;
-	volatile cbd_t *tbdf, *rbdf;
 	volatile cpm8260_t *cp = &(im->im_cpm);
 	uint	dpaddr;
+	volatile serialbuffer_t *rtx;

 	/* initialize pointers to SMC */

@@ -99,17 +111,21 @@ int serial_init (void)
 	 * damm: allocating space after the two buffers for rx/tx data
 	 */

-	dpaddr = m8260_cpm_dpalloc((2 * sizeof (cbd_t)) + 2, 16);
+	/* allocate
+	 * size of struct serialbuffer with bd rx/tx, buffer rx/tx and rx index
+	 */
+	dpaddr = m8260_cpm_dpalloc((sizeof(serialbuffer_t)), 16);
+
+	rtx = (serialbuffer_t *)&im->im_dprambase[dpaddr];

 	/* Set the physical address of the host memory buffers in
 	 * the buffer descriptors.
 	 */
-	rbdf = (cbd_t *)&im->im_dprambase[dpaddr];
-	rbdf->cbd_bufaddr = (uint) (rbdf+2);
-	rbdf->cbd_sc = 0;
-	tbdf = rbdf + 1;
-	tbdf->cbd_bufaddr = ((uint) (rbdf+2)) + 1;
-	tbdf->cbd_sc = 0;
+	rtx->rxbd.cbd_bufaddr = (uint) &rtx->rxbuf;
+	rtx->rxbd.cbd_sc      = 0;
+
+	rtx->txbd.cbd_bufaddr = (uint) &rtx->txbuf;
+	rtx->txbd.cbd_sc      = 0;

 	/* Set up the uart parameters in the parameter ram.
 	*/
@@ -142,13 +158,21 @@ int serial_init (void)

 	/* Make the first buffer the only buffer.
 	*/
-	tbdf->cbd_sc |= BD_SC_WRAP;
-	rbdf->cbd_sc |= BD_SC_EMPTY | BD_SC_WRAP;
+	rtx->txbd.cbd_sc |= BD_SC_WRAP;
+	rtx->rxbd.cbd_sc |= BD_SC_EMPTY | BD_SC_WRAP;

+#ifdef CONFIG_SYS_SMC_RXBUFLEN
+	/* multi-character receive.
+	*/
+	up->smc_mrblr = CONFIG_SYS_SMC_RXBUFLEN;
+	up->smc_maxidl = 10;
+	rtx->rxindex = 0;
+#else
 	/* Single character receive.
 	*/
 	up->smc_mrblr = 1;
 	up->smc_maxidl = 0;
+#endif

 	/* Initialize Tx/Rx parameters.
 	*/
@@ -183,27 +207,24 @@ serial_setbrg (void)
 void
 serial_putc(const char c)
 {
-	volatile cbd_t		*tbdf;
-	volatile char		*buf;
 	volatile smc_uart_t	*up;
 	volatile immap_t	*im = (immap_t *)CONFIG_SYS_IMMR;
+	volatile serialbuffer_t	*rtx;

 	if (c == '\n')
 		serial_putc ('\r');

 	up = (smc_uart_t *)&(im->im_dprambase[PROFF_SMC]);

-	tbdf = (cbd_t *)&im->im_dprambase[up->smc_tbase];
+	rtx = (serialbuffer_t *)&im->im_dprambase[up->smc_rbase];

 	/* Wait for last character to go.
 	*/
-	buf = (char *)tbdf->cbd_bufaddr;
-	while (tbdf->cbd_sc & BD_SC_READY)
+	while (rtx->txbd.cbd_sc & BD_SC_READY & BD_SC_READY)
 		;
-
-	*buf = c;
-	tbdf->cbd_datlen = 1;
-	tbdf->cbd_sc |= BD_SC_READY;
+	rtx->txbuf = c;
+	rtx->txbd.cbd_datlen = 1;
+	rtx->txbd.cbd_sc |= BD_SC_READY;
 }

 void
@@ -217,39 +238,50 @@ serial_puts (const char *s)
 int
 serial_getc(void)
 {
-	volatile cbd_t		*rbdf;
-	volatile unsigned char	*buf;
 	volatile smc_uart_t	*up;
 	volatile immap_t	*im = (immap_t *)CONFIG_SYS_IMMR;
-	unsigned char		c;
+	volatile serialbuffer_t	*rtx;
+	unsigned char  c;

 	up = (smc_uart_t *)&(im->im_dprambase[PROFF_SMC]);

-	rbdf = (cbd_t *)&im->im_dprambase[up->smc_rbase];
+	rtx = (serialbuffer_t *)&im->im_dprambase[up->smc_rbase];

 	/* Wait for character to show up.
 	*/
-	buf = (unsigned char *)rbdf->cbd_bufaddr;
-	while (rbdf->cbd_sc & BD_SC_EMPTY)
+	while (rtx->rxbd.cbd_sc & BD_SC_EMPTY)
 		;
-	c = *buf;
-	rbdf->cbd_sc |= BD_SC_EMPTY;

+#ifdef CONFIG_SYS_SMC_RXBUFLEN
+	/* the characters are read one by one,
+	 * use the rxindex to know the next char to deliver
+	 */
+	c = *(unsigned char *) (rtx->rxbd.cbd_bufaddr+rtx->rxindex);
+	rtx->rxindex++;
+
+	/* check if all char are readout, then make prepare for next receive */
+	if (rtx->rxindex >= rtx->rxbd.cbd_datlen) {
+		rtx->rxindex = 0;
+	rtx->rxbd.cbd_sc |= BD_SC_EMPTY;
+	}
+#else
+	c = *(unsigned char *) (rtx->rxbd.cbd_bufaddr);
+	rtx->rxbd.cbd_sc |= BD_SC_EMPTY;
+#endif
 	return(c);
 }

 int
 serial_tstc()
 {
-	volatile cbd_t		*rbdf;
 	volatile smc_uart_t	*up;
 	volatile immap_t	*im = (immap_t *)CONFIG_SYS_IMMR;
+	volatile serialbuffer_t	*rtx;

 	up = (smc_uart_t *)&(im->im_dprambase[PROFF_SMC]);
+	rtx = (serialbuffer_t *)&im->im_dprambase[up->smc_rbase];

-	rbdf = (cbd_t *)&im->im_dprambase[up->smc_rbase];
-
-	return(!(rbdf->cbd_sc & BD_SC_EMPTY));
+	return !(rtx->rxbd.cbd_sc & BD_SC_EMPTY);
 }

 #endif	/* CONFIG_CONS_ON_SMC */
diff --git a/include/configs/mgcoge.h b/include/configs/mgcoge.h
index 9416a03..ddb06aa 100644
--- a/include/configs/mgcoge.h
+++ b/include/configs/mgcoge.h
@@ -49,6 +49,7 @@
 #undef  CONFIG_CONS_ON_SCC		/* It's not on SCC           */
 #undef	CONFIG_CONS_NONE		/* It's not on external UART */
 #define CONFIG_CONS_INDEX	2	/* SMC2 is used for console  */
+#define CONFIG_SYS_SMC_RXBUFLEN	16

 /*
  * Select ethernet configuration
-- 
1.6.0.6

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 04/31] powerpc: 82xx serial: add configurable SMC Rx buffer len

[Wolfgang Denk]

Dear Heiko Schocher,

In message <498027A8.3010200@denx.de> you wrote:
> This patch adds the configuration option CONFIG_SYS_SMC_RXBUFLEN.

We discussed this before. I explained that I do not want to have this
added level of complexity in the driver.

Why should I reconsider?


> With this option it is possible to allow the receive
> buffer for the SMC on 82xx to be greater then 1. In case

How is this supposed to work?

Assume you set CONFIG_SYS_SMC_RXBUFLEN to 4, and the user at the
serial console port enters just a single character. When will the
receiver return this character? You might want to explain the
setup...


More technical comments below.

>  cpu/mpc8260/serial_smc.c |   94 +++++++++++++++++++++++++++++++---------------
>  include/configs/mgcoge.h |    1 +
>  2 files changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/cpu/mpc8260/serial_smc.c b/cpu/mpc8260/serial_smc.c
> index a6efa66..6825e2e 100644
> --- a/cpu/mpc8260/serial_smc.c
> +++ b/cpu/mpc8260/serial_smc.c
> @@ -64,6 +64,18 @@ DECLARE_GLOBAL_DATA_PTR;
> 
>  #endif
> 
> +typedef volatile struct serialbuffer {
> +	cbd_t	rxbd;		/* Rx BD */
> +	cbd_t	txbd;		/* Tx BD */
> +#ifdef CONFIG_SYS_SMC_RXBUFLEN
> +	uint	rxindex;	/* index for next character to read */
> +	volatile uchar	rxbuf[CONFIG_SYS_SMC_RXBUFLEN];/* rx buffers */
> +#else
> +	volatile uchar	rxbuf[1];	/* rx buffers */
> +#endif

This ifdef can be avoided if you #define CONFIG_SYS_SMC_RXBUFLEN as 1
as a default value. Makes the code much easir to read.

> -	rbdf->cbd_bufaddr = (uint) (rbdf+2);
> -	rbdf->cbd_sc = 0;
> -	tbdf = rbdf + 1;
> -	tbdf->cbd_bufaddr = ((uint) (rbdf+2)) + 1;
> -	tbdf->cbd_sc = 0;
> +	rtx->rxbd.cbd_bufaddr = (uint) &rtx->rxbuf;
> +	rtx->rxbd.cbd_sc      = 0;
> +
> +	rtx->txbd.cbd_bufaddr = (uint) &rtx->txbuf;
> +	rtx->txbd.cbd_sc      = 0;

The code does not exactly become easier to read. You might consider
using a pointer instead of rtx->rxbd resp. rtx->txbd - this would
probably largely reduce the size of your patch.

> +#ifdef CONFIG_SYS_SMC_RXBUFLEN
> +	/* multi-character receive.
> +	*/

Incorrect multiline comment style. Please fix (also some other
places).

> +	up->smc_mrblr = CONFIG_SYS_SMC_RXBUFLEN;
> +	up->smc_maxidl = 10;
> +	rtx->rxindex = 0;
> +#else
>  	/* Single character receive.
>  	*/
>  	up->smc_mrblr = 1;
>  	up->smc_maxidl = 0;
> +#endif

You might want to explain what the magic constant "10" above means. If
you use a #define for it, you could have it default to 1, and get rid
of another #ifdef block.

> +#ifdef CONFIG_SYS_SMC_RXBUFLEN
> +	/* the characters are read one by one,
> +	 * use the rxindex to know the next char to deliver
> +	 */
> +	c = *(unsigned char *) (rtx->rxbd.cbd_bufaddr+rtx->rxindex);
> +	rtx->rxindex++;
> +
> +	/* check if all char are readout, then make prepare for next receive */
> +	if (rtx->rxindex >= rtx->rxbd.cbd_datlen) {
> +		rtx->rxindex = 0;
> +	rtx->rxbd.cbd_sc |= BD_SC_EMPTY;
> +	}

Indentation wrong.

> +#else
> +	c = *(unsigned char *) (rtx->rxbd.cbd_bufaddr);
> +	rtx->rxbd.cbd_sc |= BD_SC_EMPTY;
> +#endif
>  	return(c);

I think this #ifdef can be largely avoided, too, given that
rtx->rxindex is always 0 in the non-CONFIG_SYS_SMC_RXBUFLEN case.

> diff --git a/include/configs/mgcoge.h b/include/configs/mgcoge.h
> index 9416a03..ddb06aa 100644
> --- a/include/configs/mgcoge.h
> +++ b/include/configs/mgcoge.h
> @@ -49,6 +49,7 @@
>  #undef  CONFIG_CONS_ON_SCC		/* It's not on SCC           */
>  #undef	CONFIG_CONS_NONE		/* It's not on external UART */
>  #define CONFIG_CONS_INDEX	2	/* SMC2 is used for console  */
> +#define CONFIG_SYS_SMC_RXBUFLEN	16

Did you do any performance measurements? How much performance do
you really save this way?

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
Am I your nanny? The kernel is there to support  user  programs,  but
it's a _resource_ handler, not a baby feeder.     - Linus Torvalds in
      <Pine.LNX.3.91.960425074845.22041C-100000@linux.cs.Helsinki.FI>

[U-Boot] [PATCH 04/31] powerpc: 82xx serial: add configurable SMC Rx buffer len

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> In message <498027A8.3010200@denx.de> you wrote:
>> This patch adds the configuration option CONFIG_SYS_SMC_RXBUFLEN.
> 
> We discussed this before. I explained that I do not want to have this
> added level of complexity in the driver.
> 
> Why should I reconsider?

Hups, seems I missed something (Seems I stepped into this subject, after
it was discussed here). I thought that you didnt want such a try, because
it changes a driver that a lot of boards uses, and if this change is
manageable it has a chance to get in mainline. Sorry, I have a look
in the archives ...

>> With this option it is possible to allow the receive
>> buffer for the SMC on 82xx to be greater then 1. In case
> 
> How is this supposed to work?
> 
> Assume you set CONFIG_SYS_SMC_RXBUFLEN to 4, and the user at the
> serial console port enters just a single character. When will the
> receiver return this character? You might want to explain the
> setup...

He will become this character when an idle-timout occurs. Then
the SMC closes the buffer ...

> More technical comments below.
> 
>>  cpu/mpc8260/serial_smc.c |   94 +++++++++++++++++++++++++++++++---------------
>>  include/configs/mgcoge.h |    1 +
>>  2 files changed, 64 insertions(+), 31 deletions(-)
>>
>> diff --git a/cpu/mpc8260/serial_smc.c b/cpu/mpc8260/serial_smc.c
>> index a6efa66..6825e2e 100644
>> --- a/cpu/mpc8260/serial_smc.c
>> +++ b/cpu/mpc8260/serial_smc.c
>> @@ -64,6 +64,18 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>>  #endif
>>
>> +typedef volatile struct serialbuffer {
>> +	cbd_t	rxbd;		/* Rx BD */
>> +	cbd_t	txbd;		/* Tx BD */
>> +#ifdef CONFIG_SYS_SMC_RXBUFLEN
>> +	uint	rxindex;	/* index for next character to read */
>> +	volatile uchar	rxbuf[CONFIG_SYS_SMC_RXBUFLEN];/* rx buffers */
>> +#else
>> +	volatile uchar	rxbuf[1];	/* rx buffers */
>> +#endif
> 
> This ifdef can be avoided if you #define CONFIG_SYS_SMC_RXBUFLEN as 1
> as a default value. Makes the code much easir to read.

OK.

>> -	rbdf->cbd_bufaddr = (uint) (rbdf+2);
>> -	rbdf->cbd_sc = 0;
>> -	tbdf = rbdf + 1;
>> -	tbdf->cbd_bufaddr = ((uint) (rbdf+2)) + 1;
>> -	tbdf->cbd_sc = 0;
>> +	rtx->rxbd.cbd_bufaddr = (uint) &rtx->rxbuf;
>> +	rtx->rxbd.cbd_sc      = 0;
>> +
>> +	rtx->txbd.cbd_bufaddr = (uint) &rtx->txbuf;
>> +	rtx->txbd.cbd_sc      = 0;
> 
> The code does not exactly become easier to read. You might consider
> using a pointer instead of rtx->rxbd resp. rtx->txbd - this would
> probably largely reduce the size of your patch.

OK, try this out.

>> +#ifdef CONFIG_SYS_SMC_RXBUFLEN
>> +	/* multi-character receive.
>> +	*/
> 
> Incorrect multiline comment style. Please fix (also some other
> places).

damn. (I have really problems to see such things, when the code is
not complete from my hand ... and I really tried to see such things)

:-(

>> +	up->smc_mrblr = CONFIG_SYS_SMC_RXBUFLEN;
>> +	up->smc_maxidl = 10;
>> +	rtx->rxindex = 0;
>> +#else
>>  	/* Single character receive.
>>  	*/
>>  	up->smc_mrblr = 1;
>>  	up->smc_maxidl = 0;
>> +#endif
> 
> You might want to explain what the magic constant "10" above means. If
> you use a #define for it, you could have it default to 1, and get rid
> of another #ifdef block.

Yes, a define would be much better.

>> +#ifdef CONFIG_SYS_SMC_RXBUFLEN
>> +	/* the characters are read one by one,
>> +	 * use the rxindex to know the next char to deliver
>> +	 */
>> +	c = *(unsigned char *) (rtx->rxbd.cbd_bufaddr+rtx->rxindex);
>> +	rtx->rxindex++;
>> +
>> +	/* check if all char are readout, then make prepare for next receive */
>> +	if (rtx->rxindex >= rtx->rxbd.cbd_datlen) {
>> +		rtx->rxindex = 0;
>> +	rtx->rxbd.cbd_sc |= BD_SC_EMPTY;
>> +	}
> 
> Indentation wrong.

I fix that.

>> +#else
>> +	c = *(unsigned char *) (rtx->rxbd.cbd_bufaddr);
>> +	rtx->rxbd.cbd_sc |= BD_SC_EMPTY;
>> +#endif
>>  	return(c);
> 
> I think this #ifdef can be largely avoided, too, given that
> rtx->rxindex is always 0 in the non-CONFIG_SYS_SMC_RXBUFLEN case.
> 
>> diff --git a/include/configs/mgcoge.h b/include/configs/mgcoge.h
>> index 9416a03..ddb06aa 100644
>> --- a/include/configs/mgcoge.h
>> +++ b/include/configs/mgcoge.h
>> @@ -49,6 +49,7 @@
>>  #undef  CONFIG_CONS_ON_SCC		/* It's not on SCC           */
>>  #undef	CONFIG_CONS_NONE		/* It's not on external UART */
>>  #define CONFIG_CONS_INDEX	2	/* SMC2 is used for console  */
>> +#define CONFIG_SYS_SMC_RXBUFLEN	16
> 
> Did you do any performance measurements? How much performance do
> you really save this way?

I must have a look in old logs

thanks
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 04/31] powerpc: 82xx serial: add configurable SMC Rx buffer len

[Wolfgang Denk]

Dear Heiko Schocher,

In message <498050AA.6070609@denx.de> you wrote:
> 
> > We discussed this before. I explained that I do not want to have this
> > added level of complexity in the driver.
> > 
> > Why should I reconsider?
> 
> Hups, seems I missed something (Seems I stepped into this subject, after
> it was discussed here). I thought that you didnt want such a try, because
> it changes a driver that a lot of boards uses, and if this change is
> manageable it has a chance to get in mainline. Sorry, I have a look
> in the archives ...

Well, you can see that I actually reviewed the code. If there wasn;t
even a chance I would not have done that ;-)


> > How is this supposed to work?
> > 
> > Assume you set CONFIG_SYS_SMC_RXBUFLEN to 4, and the user at the
> > serial console port enters just a single character. When will the
> > receiver return this character? You might want to explain the
> > setup...
> 
> He will become this character when an idle-timout occurs. Then
> the SMC closes the buffer ...

So it would probably make sense to mention that  you  are  using  the
idle timeout now, right?


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
"The whole world is about three drinks behind."     - Humphrey Bogart