From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 28 Jan 2009 13:33:46 +0100 Subject: [U-Boot] [PATCH 04/31] powerpc: 82xx serial: add configurable SMC Rx buffer len In-Reply-To: <20090128115717.65E21832E416@gemini.denx.de> References: <498027A8.3010200@denx.de> <20090128115717.65E21832E416@gemini.denx.de> Message-ID: <498050AA.6070609@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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