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