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

[Wolfgang Denk]

Dear Heiko Schocher,

In message <498027A8.3010200 at 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 at linux.cs.Helsinki.FI>