Message ID | 1456368085-19979-1-git-send-email-b18965@freescale.com |
---|---|
State | Accepted |
Commit | 055457ef25357d1921a8ed13ebfd32c526f12106 |
Delegated to: | Simon Glass |
Headers | show |
On Thu, Feb 25, 2016 at 10:41 AM, Alison Wang <b18965@freescale.com> wrote: > In general, a carriage return needs to execute before a line feed. > The patch is to change some serial drivers based on this rule, such > as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. > > Signed-off-by: Alison Wang <alison.wang@nxp.com> > --- > drivers/serial/serial_mxc.c | 8 ++++---- > drivers/serial/serial_pxa.c | 8 ++++---- > drivers/serial/serial_s3c24x0.c | 8 ++++---- > drivers/serial/usbtty.c | 7 ++++--- > 4 files changed, 16 insertions(+), 15 deletions(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> I guess dm serial drivers will be in another patch? Regards, Bin
Hi, Bin, > On Thu, Feb 25, 2016 at 10:41 AM, Alison Wang <b18965@freescale.com> > wrote: > > In general, a carriage return needs to execute before a line feed. > > The patch is to change some serial drivers based on this rule, such as > > serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. > > > > Signed-off-by: Alison Wang <alison.wang@nxp.com> > > --- > > drivers/serial/serial_mxc.c | 8 ++++---- > > drivers/serial/serial_pxa.c | 8 ++++---- > > drivers/serial/serial_s3c24x0.c | 8 ++++---- > > drivers/serial/usbtty.c | 7 ++++--- > > 4 files changed, 16 insertions(+), 15 deletions(-) > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > I guess dm serial drivers will be in another patch? > [Alison Wang] Yes, two patches. Best Regards, Alison Wang
Hi Alison, On Thu, Feb 25, 2016 at 11:02 AM, Huan Wang <alison.wang@nxp.com> wrote: > Hi, Bin, > >> On Thu, Feb 25, 2016 at 10:41 AM, Alison Wang <b18965@freescale.com> >> wrote: >> > In general, a carriage return needs to execute before a line feed. >> > The patch is to change some serial drivers based on this rule, such as >> > serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. >> > >> > Signed-off-by: Alison Wang <alison.wang@nxp.com> >> > --- >> > drivers/serial/serial_mxc.c | 8 ++++---- >> > drivers/serial/serial_pxa.c | 8 ++++---- >> > drivers/serial/serial_s3c24x0.c | 8 ++++---- >> > drivers/serial/usbtty.c | 7 ++++--- >> > 4 files changed, 16 insertions(+), 15 deletions(-) >> > >> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> >> I guess dm serial drivers will be in another patch? >> > [Alison Wang] Yes, two patches. > I would suggest in the future you send all these 3 patches (serial-ulass, legacy serial, dm serial) in a series. This helps people to track. Regards, Bin
Hi, Bin, > On Thu, Feb 25, 2016 at 11:02 AM, Huan Wang <alison.wang@nxp.com> wrote: > > Hi, Bin, > > > >> On Thu, Feb 25, 2016 at 10:41 AM, Alison Wang <b18965@freescale.com> > >> wrote: > >> > In general, a carriage return needs to execute before a line feed. > >> > The patch is to change some serial drivers based on this rule, such > >> > as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. > >> > > >> > Signed-off-by: Alison Wang <alison.wang@nxp.com> > >> > --- > >> > drivers/serial/serial_mxc.c | 8 ++++---- > >> > drivers/serial/serial_pxa.c | 8 ++++---- > >> > drivers/serial/serial_s3c24x0.c | 8 ++++---- > >> > drivers/serial/usbtty.c | 7 ++++--- > >> > 4 files changed, 16 insertions(+), 15 deletions(-) > >> > > >> > >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > >> > >> I guess dm serial drivers will be in another patch? > >> > > [Alison Wang] Yes, two patches. > > > > I would suggest in the future you send all these 3 patches (serial-ulass, > legacy serial, dm serial) in a series. This helps people to track. > [Alison Wang] dm serial? Sorry, I may misunderstand you. I only have 2 patches, one is for serial-ulass(that's my meaning about dm serial), the other is for legacy serial. I didn't find such error in other dm serial drivers. Best Regards, Alison Wang
Hi Alison, On Thu, Feb 25, 2016 at 12:58 PM, Huan Wang <alison.wang@nxp.com> wrote: > Hi, Bin, > >> On Thu, Feb 25, 2016 at 11:02 AM, Huan Wang <alison.wang@nxp.com> wrote: >> > Hi, Bin, >> > >> >> On Thu, Feb 25, 2016 at 10:41 AM, Alison Wang <b18965@freescale.com> >> >> wrote: >> >> > In general, a carriage return needs to execute before a line feed. >> >> > The patch is to change some serial drivers based on this rule, such >> >> > as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. >> >> > >> >> > Signed-off-by: Alison Wang <alison.wang@nxp.com> >> >> > --- >> >> > drivers/serial/serial_mxc.c | 8 ++++---- >> >> > drivers/serial/serial_pxa.c | 8 ++++---- >> >> > drivers/serial/serial_s3c24x0.c | 8 ++++---- >> >> > drivers/serial/usbtty.c | 7 ++++--- >> >> > 4 files changed, 16 insertions(+), 15 deletions(-) >> >> > >> >> >> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> >> >> >> I guess dm serial drivers will be in another patch? >> >> >> > [Alison Wang] Yes, two patches. >> > >> >> I would suggest in the future you send all these 3 patches (serial-ulass, >> legacy serial, dm serial) in a series. This helps people to track. >> > [Alison Wang] dm serial? Sorry, I may misunderstand you. I only have 2 patches, one is > for serial-ulass(that's my meaning about dm serial), the other is for legacy serial. > I didn't find such error in other dm serial drivers. > serial_lpuart.c and serial_arc.c has this handling, and they should be removed. Regards, Bin
This is a request for more information. On 02/24/2016 06:41 PM, Alison Wang wrote: > In general, a carriage return needs to execute before a line feed. > The patch is to change some serial drivers based on this rule, such > as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. You write "In general, a carriage return needs to execute before a line feed. The patch is to change some serial drivers based on this rule" Please provide a reference. I'd probably benefit from deeper knowledge of this subject. I had not heard this rule before. Thank you, Jim > > Signed-off-by: Alison Wang <alison.wang@nxp.com> > --- > drivers/serial/serial_mxc.c | 8 ++++---- > drivers/serial/serial_pxa.c | 8 ++++---- > drivers/serial/serial_s3c24x0.c | 8 ++++---- > drivers/serial/usbtty.c | 7 ++++--- > 4 files changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c > index 51485c0..1563bb3 100644 > --- a/drivers/serial/serial_mxc.c > +++ b/drivers/serial/serial_mxc.c > @@ -164,15 +164,15 @@ static int mxc_serial_getc(void) > > static void mxc_serial_putc(const char c) > { > + /* If \n, also do \r */ > + if (c == '\n') > + serial_putc('\r'); > + > __REG(UART_PHYS + UTXD) = c; > > /* wait for transmitter to be ready */ > while (!(__REG(UART_PHYS + UTS) & UTS_TXEMPTY)) > WATCHDOG_RESET(); > - > - /* If \n, also do \r */ > - if (c == '\n') > - serial_putc ('\r'); > } > > /* > diff --git a/drivers/serial/serial_pxa.c b/drivers/serial/serial_pxa.c > index 8fbcc10..1eb19ec 100644 > --- a/drivers/serial/serial_pxa.c > +++ b/drivers/serial/serial_pxa.c > @@ -156,6 +156,10 @@ void pxa_putc_dev(unsigned int uart_index, const char c) > { > struct pxa_uart_regs *uart_regs; > > + /* If \n, also do \r */ > + if (c == '\n') > + pxa_putc_dev(uart_index, '\r'); > + > uart_regs = pxa_uart_index_to_regs(uart_index); > if (!uart_regs) > hang(); > @@ -163,10 +167,6 @@ void pxa_putc_dev(unsigned int uart_index, const char c) > while (!(readl(&uart_regs->lsr) & LSR_TEMT)) > WATCHDOG_RESET(); > writel(c, &uart_regs->thr); > - > - /* If \n, also do \r */ > - if (c == '\n') > - pxa_putc_dev (uart_index,'\r'); > } > > /* > diff --git a/drivers/serial/serial_s3c24x0.c b/drivers/serial/serial_s3c24x0.c > index d4e7df2..0f0878a 100644 > --- a/drivers/serial/serial_s3c24x0.c > +++ b/drivers/serial/serial_s3c24x0.c > @@ -135,14 +135,14 @@ static void _serial_putc(const char c, const int dev_index) > { > struct s3c24x0_uart *uart = s3c24x0_get_base_uart(dev_index); > > + /* If \n, also do \r */ > + if (c == '\n') > + serial_putc('\r'); > + > while (!(readl(&uart->utrstat) & 0x2)) > /* wait for room in the tx FIFO */ ; > > writeb(c, &uart->utxh); > - > - /* If \n, also do \r */ > - if (c == '\n') > - serial_putc('\r'); > } > > static inline void serial_putc_dev(unsigned int dev_index, const char c) > diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c > index 75f0ec3..2e19813 100644 > --- a/drivers/serial/usbtty.c > +++ b/drivers/serial/usbtty.c > @@ -434,11 +434,12 @@ void usbtty_putc(struct stdio_dev *dev, const char c) > if (!usbtty_configured ()) > return; > > - buf_push (&usbtty_output, &c, 1); > /* If \n, also do \r */ > if (c == '\n') > buf_push (&usbtty_output, "\r", 1); > > + buf_push(&usbtty_output, &c, 1); > + > /* Poll at end to handle new data... */ > if ((usbtty_output.size + 2) >= usbtty_output.totalsize) { > usbtty_poll (); > @@ -498,8 +499,8 @@ void usbtty_puts(struct stdio_dev *dev, const char *str) > n = next_nl_pos (str); > > if (str[n] == '\n') { > - __usbtty_puts (str, n + 1); > - __usbtty_puts ("\r", 1); > + __usbtty_puts("\r", 1); > + __usbtty_puts(str, n + 1); > str += (n + 1); > len -= (n + 1); > } else { >
On 02/25/2016 03:41 AM, Alison Wang wrote: > In general, a carriage return needs to execute before a line feed. > The patch is to change some serial drivers based on this rule, such > as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. > > Signed-off-by: Alison Wang <alison.wang@nxp.com> Why isn't this CR-LF stuff handled in common code instead ? It'd be much better than having billion copies of the exact same logic (some of them more buggy than the others) in drivers.
Hi Marek, On Fri, Feb 26, 2016 at 1:55 AM, Marek Vasut <marex@denx.de> wrote: > On 02/25/2016 03:41 AM, Alison Wang wrote: >> In general, a carriage return needs to execute before a line feed. >> The patch is to change some serial drivers based on this rule, such >> as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. >> >> Signed-off-by: Alison Wang <alison.wang@nxp.com> > > Why isn't this CR-LF stuff handled in common code instead ? It'd be much > better than having billion copies of the exact same logic (some of them > more buggy than the others) in drivers. > The CR-LF stuff is already handled in the common code, but only for the DM version, see serial-uclass.c. This patch is to fix non-DM version serial drivers. When those drivers get converted to DM, these handling should be removed. Regards, Bin
Hi Marek, On 25 February 2016 at 10:55, Marek Vasut <marex@denx.de> wrote: > On 02/25/2016 03:41 AM, Alison Wang wrote: >> In general, a carriage return needs to execute before a line feed. >> The patch is to change some serial drivers based on this rule, such >> as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. >> >> Signed-off-by: Alison Wang <alison.wang@nxp.com> > > Why isn't this CR-LF stuff handled in common code instead ? It'd be much > better than having billion copies of the exact same logic (some of them > more buggy than the others) in drivers. > With driver model it is. Regards, Simon
On 02/26/2016 02:56 AM, Simon Glass wrote: > Hi Marek, > > On 25 February 2016 at 10:55, Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> wrote: >> On 02/25/2016 03:41 AM, Alison Wang wrote: >>> In general, a carriage return needs to execute before a line feed. >>> The patch is to change some serial drivers based on this rule, such >>> as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. >>> >>> Signed-off-by: Alison Wang <alison.wang@nxp.com > <mailto:alison.wang@nxp.com>> >> >> Why isn't this CR-LF stuff handled in common code instead ? It'd be much >> better than having billion copies of the exact same logic (some of them >> more buggy than the others) in drivers. >> > > With driver model it is. OK, thanks.
Hi, Bin, > On Thu, Feb 25, 2016 at 11:02 AM, Huan Wang <alison.wang@nxp.com> wrote: > > Hi, Bin, > > > >> On Thu, Feb 25, 2016 at 10:41 AM, Alison Wang <b18965@freescale.com> > >> wrote: > >> > In general, a carriage return needs to execute before a line feed. > >> > The patch is to change some serial drivers based on this rule, such > >> > as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. > >> > > >> > Signed-off-by: Alison Wang <alison.wang@nxp.com> > >> > --- > >> > drivers/serial/serial_mxc.c | 8 ++++---- > >> > drivers/serial/serial_pxa.c | 8 ++++---- > >> > drivers/serial/serial_s3c24x0.c | 8 ++++---- > >> > drivers/serial/usbtty.c | 7 ++++--- > >> > 4 files changed, 16 insertions(+), 15 deletions(-) > >> > > >> > >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > >> > >> I guess dm serial drivers will be in another patch? > >> > > [Alison Wang] Yes, two patches. > > > > I would suggest in the future you send all these 3 patches (serial-ulass, > legacy serial, dm serial) in a series. This helps people to track. > [Alison Wang] Sorry to reply late. I will send a series. Thanks for your advice. Best Regards, Alison Wang
Hi, Jim, > -----Original Message----- > From: James Chargin [mailto:jimccrown@gmail.com] > Sent: Thursday, February 25, 2016 10:19 PM > To: u-boot@lists.denx.de; b18965@freescale.com > Subject: Re: [U-Boot] [PATCH] serial: Move carriage return before line > feed for some serial drivers > > This is a request for more information. > > On 02/24/2016 06:41 PM, Alison Wang wrote: > > In general, a carriage return needs to execute before a line feed. > > The patch is to change some serial drivers based on this rule, such as > > serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. > > You write "In general, a carriage return needs to execute before a line > feed. The patch is to change some serial drivers based on this rule" > > Please provide a reference. I'd probably benefit from deeper knowledge > of this subject. I had not heard this rule before. > [Alison Wang] I think you could refer to https://en.wikipedia.org/wiki/Newline#History . Best Regards, Alison Wang
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index 51485c0..1563bb3 100644 --- a/drivers/serial/serial_mxc.c +++ b/drivers/serial/serial_mxc.c @@ -164,15 +164,15 @@ static int mxc_serial_getc(void) static void mxc_serial_putc(const char c) { + /* If \n, also do \r */ + if (c == '\n') + serial_putc('\r'); + __REG(UART_PHYS + UTXD) = c; /* wait for transmitter to be ready */ while (!(__REG(UART_PHYS + UTS) & UTS_TXEMPTY)) WATCHDOG_RESET(); - - /* If \n, also do \r */ - if (c == '\n') - serial_putc ('\r'); } /* diff --git a/drivers/serial/serial_pxa.c b/drivers/serial/serial_pxa.c index 8fbcc10..1eb19ec 100644 --- a/drivers/serial/serial_pxa.c +++ b/drivers/serial/serial_pxa.c @@ -156,6 +156,10 @@ void pxa_putc_dev(unsigned int uart_index, const char c) { struct pxa_uart_regs *uart_regs; + /* If \n, also do \r */ + if (c == '\n') + pxa_putc_dev(uart_index, '\r'); + uart_regs = pxa_uart_index_to_regs(uart_index); if (!uart_regs) hang(); @@ -163,10 +167,6 @@ void pxa_putc_dev(unsigned int uart_index, const char c) while (!(readl(&uart_regs->lsr) & LSR_TEMT)) WATCHDOG_RESET(); writel(c, &uart_regs->thr); - - /* If \n, also do \r */ - if (c == '\n') - pxa_putc_dev (uart_index,'\r'); } /* diff --git a/drivers/serial/serial_s3c24x0.c b/drivers/serial/serial_s3c24x0.c index d4e7df2..0f0878a 100644 --- a/drivers/serial/serial_s3c24x0.c +++ b/drivers/serial/serial_s3c24x0.c @@ -135,14 +135,14 @@ static void _serial_putc(const char c, const int dev_index) { struct s3c24x0_uart *uart = s3c24x0_get_base_uart(dev_index); + /* If \n, also do \r */ + if (c == '\n') + serial_putc('\r'); + while (!(readl(&uart->utrstat) & 0x2)) /* wait for room in the tx FIFO */ ; writeb(c, &uart->utxh); - - /* If \n, also do \r */ - if (c == '\n') - serial_putc('\r'); } static inline void serial_putc_dev(unsigned int dev_index, const char c) diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c index 75f0ec3..2e19813 100644 --- a/drivers/serial/usbtty.c +++ b/drivers/serial/usbtty.c @@ -434,11 +434,12 @@ void usbtty_putc(struct stdio_dev *dev, const char c) if (!usbtty_configured ()) return; - buf_push (&usbtty_output, &c, 1); /* If \n, also do \r */ if (c == '\n') buf_push (&usbtty_output, "\r", 1); + buf_push(&usbtty_output, &c, 1); + /* Poll at end to handle new data... */ if ((usbtty_output.size + 2) >= usbtty_output.totalsize) { usbtty_poll (); @@ -498,8 +499,8 @@ void usbtty_puts(struct stdio_dev *dev, const char *str) n = next_nl_pos (str); if (str[n] == '\n') { - __usbtty_puts (str, n + 1); - __usbtty_puts ("\r", 1); + __usbtty_puts("\r", 1); + __usbtty_puts(str, n + 1); str += (n + 1); len -= (n + 1); } else {
In general, a carriage return needs to execute before a line feed. The patch is to change some serial drivers based on this rule, such as serial_mxc.c, serial_pxa.c, serial_s3c24x0.c and usbtty.c. Signed-off-by: Alison Wang <alison.wang@nxp.com> --- drivers/serial/serial_mxc.c | 8 ++++---- drivers/serial/serial_pxa.c | 8 ++++---- drivers/serial/serial_s3c24x0.c | 8 ++++---- drivers/serial/usbtty.c | 7 ++++--- 4 files changed, 16 insertions(+), 15 deletions(-)