* [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment @ 2018-04-04 15:48 Ulrich Hecht 2018-04-24 15:15 ` Geert Uytterhoeven 2019-03-27 18:35 ` Eugeniu Rosca 0 siblings, 2 replies; 18+ messages in thread From: Ulrich Hecht @ 2018-04-04 15:48 UTC (permalink / raw) To: linux-renesas-soc, wsa, geert Cc: linux-serial, magnus.damm, greg, Ulrich Hecht HSCIF has facilities that allow moving the RX sampling point by between -8 and 7 sampling cycles (one sampling cycles equals 1/15 of a bit by default) to improve the error margin in case of slightly mismatched bit rates between sender and receiver. This patch tries to determine if shifting the sampling point can improve the error margin and will enable it if so. Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> --- This revision dumps the sysfs interface and works out the optimal shift on its own. It also moves setting of the HSSRR register back to its original location. CU Uli drivers/tty/serial/sh-sci.c | 65 +++++++++++++++++++++++++++++---------------- drivers/tty/serial/sh-sci.h | 4 +++ 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index fdbbff5..5d61654 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2390,6 +2390,27 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, uart_update_timeout(port, termios->c_cflag, baud); + /* byte size and parity */ + switch (termios->c_cflag & CSIZE) { + case CS5: + bits = 7; + break; + case CS6: + bits = 8; + break; + case CS7: + bits = 9; + break; + default: + bits = 10; + break; + } + + if (termios->c_cflag & CSTOPB) + bits++; + if (termios->c_cflag & PARENB) + bits++; + if (best_clk >= 0) { if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) switch (srr + 1) { @@ -2406,8 +2427,27 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, serial_port_out(port, SCSCR, scr_val | s->hscif_tot); serial_port_out(port, SCSMR, smr_val); serial_port_out(port, SCBRR, brr); - if (sci_getreg(port, HSSRR)->size) - serial_port_out(port, HSSRR, srr | HSCIF_SRE); + if (sci_getreg(port, HSSRR)->size) { + unsigned int hssrr = srr | HSCIF_SRE; + /* Calculate deviation from intended rate at the + * center of the last stop bit in sampling clocks. + */ + int last_stop = bits * 2 - 1; + int deviation = min_err * srr * last_stop / 2 / baud; + + if (abs(deviation) >= 2) { + /* At least two sampling clocks off at the + * last stop bit; we can increase the error + * margin by shifting the sampling point. + */ + int shift = min(-8, max(7, deviation / 2)); + + hssrr |= (shift << HSCIF_SRHP_SHIFT) & + HSCIF_SRHP_MASK; + hssrr |= HSCIF_SRDE; + } + serial_port_out(port, HSSRR, hssrr); + } /* Wait one bit interval */ udelay((1000000 + (baud - 1)) / baud); @@ -2474,27 +2514,6 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, * value obtained by this formula is too small. Therefore, if the value * is smaller than 20ms, use 20ms as the timeout value for DMA. */ - /* byte size and parity */ - switch (termios->c_cflag & CSIZE) { - case CS5: - bits = 7; - break; - case CS6: - bits = 8; - break; - case CS7: - bits = 9; - break; - default: - bits = 10; - break; - } - - if (termios->c_cflag & CSTOPB) - bits++; - if (termios->c_cflag & PARENB) - bits++; - s->rx_frame = (10000 * bits) / (baud / 100); #ifdef CONFIG_SERIAL_SH_SCI_DMA s->rx_timeout = s->buf_len_rx * 2 * s->rx_frame; diff --git a/drivers/tty/serial/sh-sci.h b/drivers/tty/serial/sh-sci.h index a5f792f..0b9e804 100644 --- a/drivers/tty/serial/sh-sci.h +++ b/drivers/tty/serial/sh-sci.h @@ -130,6 +130,10 @@ enum { /* HSSRR HSCIF */ #define HSCIF_SRE BIT(15) /* Sampling Rate Register Enable */ +#define HSCIF_SRDE BIT(14) /* Sampling Point Register Enable */ + +#define HSCIF_SRHP_SHIFT 8 +#define HSCIF_SRHP_MASK 0x0f00 /* SCPCR (Serial Port Control Register), SCIFA/SCIFB only */ #define SCPCR_RTSC BIT(4) /* Serial Port RTS# Pin / Output Pin */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2018-04-04 15:48 [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment Ulrich Hecht @ 2018-04-24 15:15 ` Geert Uytterhoeven 2019-03-27 18:35 ` Eugeniu Rosca 1 sibling, 0 replies; 18+ messages in thread From: Geert Uytterhoeven @ 2018-04-24 15:15 UTC (permalink / raw) To: Ulrich Hecht Cc: Linux-Renesas, Wolfram Sang, linux-serial, Magnus Damm, Greg KH Hi Ulrich, On Wed, Apr 4, 2018 at 5:48 PM, Ulrich Hecht <ulrich.hecht+renesas@gmail.com> wrote: > HSCIF has facilities that allow moving the RX sampling point by between > -8 and 7 sampling cycles (one sampling cycles equals 1/15 of a bit > by default) to improve the error margin in case of slightly mismatched > bit rates between sender and receiver. > > This patch tries to determine if shifting the sampling point can improve > the error margin and will enable it if so. > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> > --- > This revision dumps the sysfs interface and works out the optimal shift > on its own. It also moves setting of the HSSRR register back to its > original location. Thanks for the update! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> One suggestion for improvement below. > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2406,8 +2427,27 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, > serial_port_out(port, SCSCR, scr_val | s->hscif_tot); > serial_port_out(port, SCSMR, smr_val); > serial_port_out(port, SCBRR, brr); > - if (sci_getreg(port, HSSRR)->size) > - serial_port_out(port, HSSRR, srr | HSCIF_SRE); > + if (sci_getreg(port, HSSRR)->size) { > + unsigned int hssrr = srr | HSCIF_SRE; > + /* Calculate deviation from intended rate at the > + * center of the last stop bit in sampling clocks. > + */ > + int last_stop = bits * 2 - 1; > + int deviation = min_err * srr * last_stop / 2 / baud; DIV_ROUND_CLOSEST()? > + > + if (abs(deviation) >= 2) { > + /* At least two sampling clocks off at the > + * last stop bit; we can increase the error > + * margin by shifting the sampling point. > + */ > + int shift = min(-8, max(7, deviation / 2)); > + > + hssrr |= (shift << HSCIF_SRHP_SHIFT) & > + HSCIF_SRHP_MASK; > + hssrr |= HSCIF_SRDE; > + } > + serial_port_out(port, HSSRR, hssrr); > + } > > /* Wait one bit interval */ > udelay((1000000 + (baud - 1)) / baud); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2018-04-04 15:48 [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment Ulrich Hecht 2018-04-24 15:15 ` Geert Uytterhoeven @ 2019-03-27 18:35 ` Eugeniu Rosca 2019-03-28 8:33 ` Wolfram Sang 2019-03-28 9:24 ` Geert Uytterhoeven 1 sibling, 2 replies; 18+ messages in thread From: Eugeniu Rosca @ 2019-03-27 18:35 UTC (permalink / raw) To: Ulrich Hecht, wsa, geert, linux-renesas-soc Cc: linux-serial, Dirk Behme, Eugeniu Rosca, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, magnus.damm, greg Dear Renesas Open Source team, We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and the latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], which mirrors v4.18-rc1 commit [4] in mainline. JFYI, quite far away in the delivery chain, we've received below report: > With this patch [2-4] there are reports about broken data > communication with 115200 baud with SXM module. Reverting > this patch results in successful communication, again. While this scarce information barely helps anybody, I thought that sharing it with you might be beneficial in case you collect several reports linked to this specific commit in future, meaning it potentially adds a regression. Also, if you are aware of any userland changes that might be required/assumed by this patch or in case you have any alternative ideas how to avoid reverting this patch, your feedback would be very appreciated. [1] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tag/?h=rcar-3.9.3 [2] https://patchwork.kernel.org/patch/10322809/ [3] https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=e395fc813539 [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=63ba1e00f178 Best regards, Eugeniu. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-27 18:35 ` Eugeniu Rosca @ 2019-03-28 8:33 ` Wolfram Sang 2019-03-28 9:24 ` Geert Uytterhoeven 1 sibling, 0 replies; 18+ messages in thread From: Wolfram Sang @ 2019-03-28 8:33 UTC (permalink / raw) To: Eugeniu Rosca Cc: Ulrich Hecht, geert, linux-renesas-soc, linux-serial, Dirk Behme, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, magnus.damm, greg [-- Attachment #1: Type: text/plain, Size: 701 bytes --] Hi Eugeniu, > JFYI, quite far away in the delivery chain, we've received below report: > > > With this patch [2-4] there are reports about broken data > > communication with 115200 baud with SXM module. Reverting > > this patch results in successful communication, again. > > While this scarce information barely helps anybody, I thought that > sharing it with you might be beneficial in case you collect several > reports linked to this specific commit in future, meaning it potentially > adds a regression. Thank you for this report! True, the information is scarce, but we will see what we can do with it. If we find something, I'll report back. Kind regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-27 18:35 ` Eugeniu Rosca 2019-03-28 8:33 ` Wolfram Sang @ 2019-03-28 9:24 ` Geert Uytterhoeven 2019-03-28 10:16 ` Dirk Behme 1 sibling, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2019-03-28 9:24 UTC (permalink / raw) To: Eugeniu Rosca Cc: Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Dirk Behme, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH Hi Eugeniu, On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and the > latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], which > mirrors v4.18-rc1 commit [4] in mainline. > > JFYI, quite far away in the delivery chain, we've received below report: > > > With this patch [2-4] there are reports about broken data > > communication with 115200 baud with SXM module. Reverting > > this patch results in successful communication, again. > > While this scarce information barely helps anybody, I thought that > sharing it with you might be beneficial in case you collect several > reports linked to this specific commit in future, meaning it potentially > adds a regression. > > Also, if you are aware of any userland changes that might be > required/assumed by this patch or in case you have any alternative > ideas how to avoid reverting this patch, your feedback would be very > appreciated. Thanks for your report! [TLDR: skip to the suggested fix below; I only noticed the bug after writing the below paragraphs, which are still useful questions to let us reproduce the issue] Which SoC are you using? I assume this is on a custom board, as Salvator-X(S) and ULCB have external SCIF clock crystals, which allow to use a perfect 115200 bps, hence the affected code path is not exercised: sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 sh-sci e6550000.serial: Using clk scif for 115200+0 bps Does your board have an external SCIF clock? Which frequency? Can you check the clock values and deviation for your configuration, by changing the calls to print the above information from dev_dbg() to dev_info()? Does adding the DIV_ROUND_CLOSEST(), as suggested in my review of the posted patch, help? Perhaps the sampling point shift is inverted? Does -shift work better? [possible solution] > + int shift = min(-8, max(7, deviation / 2)); Oops, min and max are exchanged! I guess using int shift = clamp(deviation / 2, -8, 7) instead fixes the issue? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-28 9:24 ` Geert Uytterhoeven @ 2019-03-28 10:16 ` Dirk Behme 2019-03-28 11:30 ` Dirk Behme 0 siblings, 1 reply; 18+ messages in thread From: Dirk Behme @ 2019-03-28 10:16 UTC (permalink / raw) To: Geert Uytterhoeven, Eugeniu Rosca Cc: Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH Hi Geert, On 28.03.2019 10:24, Geert Uytterhoeven wrote: > Hi Eugeniu, > > On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: >> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and the >> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], which >> mirrors v4.18-rc1 commit [4] in mainline. >> >> JFYI, quite far away in the delivery chain, we've received below report: >> >>> With this patch [2-4] there are reports about broken data >>> communication with 115200 baud with SXM module. Reverting >>> this patch results in successful communication, again. >> >> While this scarce information barely helps anybody, I thought that >> sharing it with you might be beneficial in case you collect several >> reports linked to this specific commit in future, meaning it potentially >> adds a regression. >> >> Also, if you are aware of any userland changes that might be >> required/assumed by this patch or in case you have any alternative >> ideas how to avoid reverting this patch, your feedback would be very >> appreciated. > > Thanks for your report! > > [TLDR: skip to the suggested fix below; I only noticed the bug after > writing the below paragraphs, which are still useful questions to > let us reproduce the issue] > > Which SoC are you using? > I assume this is on a custom board, as Salvator-X(S) and ULCB have > external SCIF clock crystals, which allow to use a perfect 115200 bps, > hence the affected code path is not exercised: > > sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 > sh-sci e6550000.serial: Using clk scif for 115200+0 bps > > Does your board have an external SCIF clock? Which frequency? > Can you check the clock values and deviation for your configuration, by > changing the calls to print the above information from dev_dbg() to > dev_info()? > > Does adding the DIV_ROUND_CLOSEST(), as suggested in my review > of the posted patch, help? > Perhaps the sampling point shift is inverted? Does -shift work better? > > [possible solution] > >> + int shift = min(-8, max(7, deviation / 2)); > > Oops, min and max are exchanged! > > I guess using > > int shift = clamp(deviation / 2, -8, 7) > > instead fixes the issue? Uh, that was fast :) Many thanks! We will test this as fast as possible! But due to the long delivery chain Eugeniu mentioned this will take some time. I'll try my best to come back to you as fast as possible. Thanks again! Dirk ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-28 10:16 ` Dirk Behme @ 2019-03-28 11:30 ` Dirk Behme 2019-03-29 7:05 ` Dirk Behme 0 siblings, 1 reply; 18+ messages in thread From: Dirk Behme @ 2019-03-28 11:30 UTC (permalink / raw) To: Geert Uytterhoeven, Eugeniu Rosca Cc: Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH [-- Attachment #1: Type: text/plain, Size: 2759 bytes --] On 28.03.2019 11:16, Dirk Behme wrote: > Hi Geert, > > On 28.03.2019 10:24, Geert Uytterhoeven wrote: >> Hi Eugeniu, >> >> On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> >> wrote: >>> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and the >>> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], which >>> mirrors v4.18-rc1 commit [4] in mainline. >>> >>> JFYI, quite far away in the delivery chain, we've received below report: >>> >>>> With this patch [2-4] there are reports about broken data >>>> communication with 115200 baud with SXM module. Reverting >>>> this patch results in successful communication, again. >>> >>> While this scarce information barely helps anybody, I thought that >>> sharing it with you might be beneficial in case you collect several >>> reports linked to this specific commit in future, meaning it potentially >>> adds a regression. >>> >>> Also, if you are aware of any userland changes that might be >>> required/assumed by this patch or in case you have any alternative >>> ideas how to avoid reverting this patch, your feedback would be very >>> appreciated. >> >> Thanks for your report! >> >> [TLDR: skip to the suggested fix below; I only noticed the bug after >> writing the below paragraphs, which are still useful questions to >> let us reproduce the issue] >> >> Which SoC are you using? >> I assume this is on a custom board, as Salvator-X(S) and ULCB have >> external SCIF clock crystals, which allow to use a perfect 115200 bps, >> hence the affected code path is not exercised: >> >> sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 >> sh-sci e6550000.serial: Using clk scif for 115200+0 bps >> >> Does your board have an external SCIF clock? Which frequency? >> Can you check the clock values and deviation for your configuration, by >> changing the calls to print the above information from dev_dbg() to >> dev_info()? >> >> Does adding the DIV_ROUND_CLOSEST(), as suggested in my review >> of the posted patch, help? >> Perhaps the sampling point shift is inverted? Does -shift work better? >> >> [possible solution] >> >>> + int shift = min(-8, max(7, deviation >>> / 2)); >> >> Oops, min and max are exchanged! >> >> I guess using >> >> int shift = clamp(deviation / 2, -8, 7) >> >> instead fixes the issue? > > > Uh, that was fast :) Many thanks! > > We will test this as fast as possible! But due to the long delivery > chain Eugeniu mentioned this will take some time. I'll try my best to > come back to you as fast as possible. Just for the archives: We are testing the attached patch. Best regards Dirk [-- Attachment #2: 0001-serial-sh-sci-Fix-HSCIF-RX-sampling-point-adjustment.patch --] [-- Type: text/plain, Size: 1242 bytes --] From 32cde852a03fa7fbb450d5665785b1ff390d3867 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven <geert@linux-m68k.org> Date: Thu, 28 Mar 2019 12:10:58 +0100 Subject: [PATCH] serial: sh-sci: Fix HSCIF RX sampling point adjustment In commit 63ba1e00f178 ("serial: sh-sci: Support for HSCIF RX sampling point adjustment") min and max are exchanged. Fix this. Fixes: 63ba1e00f178 ("serial: sh-sci: Support for HSCIF RX sampling point adjustment") Cc: <stable@vger.kernel.org> # v4.18+ Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> --- drivers/tty/serial/sh-sci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index eba08cd892e5..69896d586a29 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2446,7 +2446,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, * last stop bit; we can increase the error * margin by shifting the sampling point. */ - int shift = min(-8, max(7, deviation / 2)); + int shift = clamp(deviation / 2, -8, 7); hssrr |= (shift << HSCIF_SRHP_SHIFT) & HSCIF_SRHP_MASK; -- 2.20.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-28 11:30 ` Dirk Behme @ 2019-03-29 7:05 ` Dirk Behme 2019-03-29 9:46 ` Ulrich Hecht 2019-03-29 9:46 ` Geert Uytterhoeven 0 siblings, 2 replies; 18+ messages in thread From: Dirk Behme @ 2019-03-29 7:05 UTC (permalink / raw) To: Geert Uytterhoeven, Eugeniu Rosca Cc: Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH Hi Geert, On 28.03.2019 12:30, Dirk Behme wrote: > On 28.03.2019 11:16, Dirk Behme wrote: >> Hi Geert, >> >> On 28.03.2019 10:24, Geert Uytterhoeven wrote: >>> Hi Eugeniu, >>> >>> On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> >>> wrote: >>>> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and >>>> the >>>> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], >>>> which >>>> mirrors v4.18-rc1 commit [4] in mainline. >>>> >>>> JFYI, quite far away in the delivery chain, we've received below >>>> report: >>>> >>>>> With this patch [2-4] there are reports about broken data >>>>> communication with 115200 baud with SXM module. Reverting >>>>> this patch results in successful communication, again. >>>> >>>> While this scarce information barely helps anybody, I thought that >>>> sharing it with you might be beneficial in case you collect several >>>> reports linked to this specific commit in future, meaning it >>>> potentially >>>> adds a regression. >>>> >>>> Also, if you are aware of any userland changes that might be >>>> required/assumed by this patch or in case you have any alternative >>>> ideas how to avoid reverting this patch, your feedback would be very >>>> appreciated. >>> >>> Thanks for your report! >>> >>> [TLDR: skip to the suggested fix below; I only noticed the bug after >>> writing the below paragraphs, which are still useful >>> questions to >>> let us reproduce the issue] >>> >>> Which SoC are you using? >>> I assume this is on a custom board, as Salvator-X(S) and ULCB have >>> external SCIF clock crystals, which allow to use a perfect 115200 bps, >>> hence the affected code path is not exercised: >>> >>> sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 >>> sh-sci e6550000.serial: Using clk scif for 115200+0 bps >>> >>> Does your board have an external SCIF clock? Which frequency? >>> Can you check the clock values and deviation for your configuration, by >>> changing the calls to print the above information from dev_dbg() to >>> dev_info()? >>> >>> Does adding the DIV_ROUND_CLOSEST(), as suggested in my review >>> of the posted patch, help? >>> Perhaps the sampling point shift is inverted? Does -shift work better? >>> >>> [possible solution] >>> >>>> + int shift = min(-8, max(7, deviation >>>> / 2)); >>> >>> Oops, min and max are exchanged! >>> >>> I guess using >>> >>> int shift = clamp(deviation / 2, -8, 7) >>> >>> instead fixes the issue? >> >> >> Uh, that was fast :) Many thanks! >> >> We will test this as fast as possible! But due to the long delivery >> chain Eugeniu mentioned this will take some time. I'll try my best to >> come back to you as fast as possible. > > > Just for the archives: We are testing the attached patch. * Testing the patch [5] - int shift = min(-8, max(7, deviation / 2)); + int shift = clamp(deviation / 2, -8, 7); does *not* fix our issue. Or in other words: Testing was *not* successful. * However, from review point of view we think that it fixes a serious bug. So maybe it should be applied, anyhow? * Using strace we managed to get some more information about the usage of the serial port [6]. With this, we are talking about 57600 and not 115200 * Switching to dev_info() [7] as requested above we get [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, base_baud = 0) is a hscif [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps * We are talking about a custom r8a7796 board Best regards Dirk [5] https://patchwork.kernel.org/patch/10322809/#22554931 [6] [pid 4715] newfstatat(AT_FDCWD, "/dev/ttySC3", {st_mode=S_IFCHR|0777, st_rdev=makedev(204, 11), ...}, 0) = 0 [pid 4715] openat(AT_FDCWD, "/dev/ttySC3", O_RDWR) = 9 [pid 4715] ioctl(9, TCGETS, {B9600 opost isig icanon echo ...}) = 0 [pid 4715] ioctl(9, TCFLSH, TCIFLUSH) = 0 [pid 4715] ioctl(9, SNDCTL_TMR_START or TCSETS, {B57600 -opost -isig -icanon -echo ...}) = 0 [pid 4715] ioctl(9, TIOCMGET, [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4715] ioctl(9, TIOCMSET, [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4715] ioctl(9, TIOCMGET, [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4715] ioctl(9, TIOCMSET, [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4715] nanosleep({0, 250000000}, NULL) = 0 [pid 4715] mmap(NULL, 8388608, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0xffffbc192000 [pid 4715] mprotect(0xffffbc192000, 4096, PROT_NONE) = 0 [pid 4715] clone(child_stack=0xffffbc990af0, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0xffffbc9912b0, tls=0xffffbc9918d0, child_tidptr=0xffffbc9912b0) = 4720 strace: Process 4720 attached [pid 4715] futex(0xffff200082cc, FUTEX_WAIT_BITSET_PRIVATE, 1, {442, 798479296}, ffffffff <unfinished ...> [pid 4720] set_robust_list(0xffffbc9912c0, 24) = 0 [pid 4720] prctl(PR_SET_NAME, "link_thread\0\0\0\0\0") = 0 [pid 4720] mmap(NULL, 8388608, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0xffff27800000 [pid 4720] mmap(0xffff24000000, 67108864, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_NORESERVE, -1, 0) = 0xffff1c000000 [pid 4720] mprotect(0xffff1c000000, 135168, PROT_READ|PROT_WRITE) = 0 [pid 4720] mprotect(0xffff27800000, 4096, PROT_NONE) = 0 [pid 4720] clone(strace: Process 4721 attached child_stack=0xffff27ffeaf0, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0xffff27fff2b0, tls=0xffff27fff8d0, child_tidptr=0xffff27fff2b0) = 4721 [pid 4721] set_robust_list(0xffff27fff2c0, 24 <unfinished ...> [pid 4720] ioctl(9, TIOCMGET <unfinished ...> [pid 4721] <... set_robust_list resumed> ) = 0 [pid 4720] <... ioctl resumed> , [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4721] prctl(PR_SET_NAME, "link_reader\0\0\0\0\0" <unfinished ...> [pid 4720] ioctl(9, TIOCMSET, [TIOCM_DTR|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR] <unfinished ...> [pid 4721] <... prctl resumed> ) = 0 [pid 4720] <... ioctl resumed> ) = 0 [pid 4721] ppoll([{fd=9, events=POLLIN}], 1, {1, 0}, NULL, 0 <unfinished ...> [pid 4720] nanosleep({0, 250000000}, NULL) = 0 [pid 4720] ioctl(9, TIOCMGET, [TIOCM_DTR|TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4720] ioctl(9, TIOCMSET, [TIOCM_CTS|TIOCM_CAR|TIOCM_DSR]) = 0 [pid 4720] nanosleep({0, 100000000}, NULL) = 0 [pid 4720] ioctl(9, TCGETS, {B57600 -opost -isig -icanon -echo ...}) = 0 [pid 4720] ioctl(9, TCFLSH, TCIFLUSH) = 0 [pid 4720] ioctl(9, SNDCTL_TMR_START or TCSETS, {B57600 -opost -isig -icanon -echo ...}) = 0 [pid 4720] write(9, "\336\306\0\0\0\4\0\0\0\0\304\250", 12) = 12 [pid 4720] futex(0xffff2000824c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0xffff20008248, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1 [pid 4716] <... futex resumed> ) = 0 [pid 4720] futex(0xffff20008d9c, FUTEX_WAIT_PRIVATE, 1, NULL <unfinished ...> [pid 4716] futex(0xffff20008210, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4716] futex(0xffff2000824c, FUTEX_WAIT_BITSET_PRIVATE, 5, {383, 226656729}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 4716] futex(0xffff20008210, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4716] futex(0xffff20008d9c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0xffff20008d98, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1 [pid 4720] <... futex resumed> ) = 0 [pid 4716] futex(0xffff2000824c, FUTEX_WAIT_BITSET_PRIVATE, 7, {393, 151155511}, ffffffff <unfinished ...> [pid 4720] futex(0xffff20008d60, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4720] write(9, "\336\306\0\0\0\4\0\0\0\0\304\250", 12) = 12 [pid 4720] futex(0xffff2000824c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0xffff20008248, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1 [pid 4720] futex(0xffff20008d9c, FUTEX_WAIT_PRIVATE, 3, NULL <unfinished ...> [pid 4716] <... futex resumed> ) = 0 [pid 4716] futex(0xffff20008210, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4716] futex(0xffff2000824c, FUTEX_WAIT_BITSET_PRIVATE, 9, {383, 301617693}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 4716] futex(0xffff20008210, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4716] futex(0xffff20008d9c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0xffff20008d98, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1 [pid 4716] futex(0xffff2000824c, FUTEX_WAIT_BITSET_PRIVATE, 11, {393, 150961530}, ffffffff <unfinished ...> [pid 4720] <... futex resumed> ) = 0 [pid 4720] futex(0xffff20008d60, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4720] write(9, "\336\306\0\0\0\4\0\0\0\0\304\250", 12) = 12 [pid 4720] futex(0xffff2000824c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0xffff20008248, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1 [pid 4716] <... futex resumed> ) = 0 [pid 4720] futex(0xffff20008d9c, FUTEX_WAIT_PRIVATE, 5, NULL <unfinished ...> [pid 4716] futex(0xffff20008210, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4716] futex(0xffff2000824c, FUTEX_WAIT_BITSET_PRIVATE, 13, {383, 377427852}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 4716] futex(0xffff20008210, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 4716] futex(0xffff20008d9c, FUTEX_WAKE_OP_PRIVATE, 1, 1, 0xffff20008d98, {FUTEX_OP_SET, 0, FUTEX_OP_CMP_GT, 1}) = 1 [pid 4720] <... futex resumed> ) = 0 [pid 4716] futex(0xffff2000824c, FUTEX_WAIT_BITSET_PRIVATE, 15, {393, 150762113}, ffffffff <unfinished ...> [pid 4720] futex(0xffff20008d60, FUTEX_WAKE_PRIVATE, 1) = 0 ... [7] From 9a3c199f02cb66cb67e93e0824b03b622ab3b024 Mon Sep 17 00:00:00 2001 From: Dirk Behme <dirk.behme@de.bosch.com> Date: Fri, 29 Mar 2019 06:39:31 +0100 Subject: [PATCH] serial: sh-sci: Enable debug output Enable some debug output as requested by Geert in https://patchwork.kernel.org/patch/10322809/#22554727 Change-Id: Icd2f97138516a0e40726fa1ccd50a69abb57cb76 Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> --- drivers/tty/serial/sh-sci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index eba08cd892e5..32210cf2413c 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2161,7 +2161,7 @@ static int sci_brg_calc(struct sci_port *s, unsigned int bps, break; } - dev_dbg(s->port.dev, "BRG: %u%+d bps using DL %u SR %u\n", bps, + dev_info(s->port.dev, "BRG: %u%+d bps using DL %u SR %u\n", bps, min_err, *dlr, *srr + 1); return min_err; } @@ -2376,7 +2376,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, done: if (best_clk >= 0) - dev_dbg(port->dev, "Using clk %pC for %u%+d bps\n", + dev_info(port->dev, "Using clk %pC for %u%+d bps\n", s->clks[best_clk], baud, min_err); sci_port_enable(s); -- 2.20.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 7:05 ` Dirk Behme @ 2019-03-29 9:46 ` Ulrich Hecht 2019-03-29 9:56 ` Geert Uytterhoeven 2019-03-29 11:17 ` Dirk Behme 2019-03-29 9:46 ` Geert Uytterhoeven 1 sibling, 2 replies; 18+ messages in thread From: Ulrich Hecht @ 2019-03-29 9:46 UTC (permalink / raw) To: Dirk Behme, Geert Uytterhoeven, Eugeniu Rosca Cc: Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH > On March 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: > > > Hi Geert, > > On 28.03.2019 12:30, Dirk Behme wrote: > > On 28.03.2019 11:16, Dirk Behme wrote: > * Testing the patch [5] > > - int shift = min(-8, max(7, deviation / 2)); > + int shift = clamp(deviation / 2, -8, 7); > > does *not* fix our issue. Or in other words: Testing was *not* successful. > > * However, from review point of view we think that it fixes a serious > bug. So maybe it should be applied, anyhow? It should. > > * Using strace we managed to get some more information about the usage > of the serial port [6]. With this, we are talking about 57600 and not 115200 > > * Switching to dev_info() [7] as requested above we get > > [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, > base_baud = 0) is a hscif > [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 > [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps > [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 > [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps > > * We are talking about a custom r8a7796 board Reviewing the code, I have found another potential issue: the SRR (sampling rate) value is not validated; only values from 7 to 31 are permissible, according to the datasheet. The values in the debug output above would be fine, though. So, for clarification: Is there a problem at 9600/57600 bps, or does it only occur at 115200 bps? CU Uli ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 9:46 ` Ulrich Hecht @ 2019-03-29 9:56 ` Geert Uytterhoeven 2019-03-29 10:35 ` Ulrich Hecht 2019-03-29 11:17 ` Dirk Behme 1 sibling, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2019-03-29 9:56 UTC (permalink / raw) To: Ulrich Hecht Cc: Dirk Behme, Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH Hi Uli, On Fri, Mar 29, 2019 at 10:46 AM Ulrich Hecht <uli@fpond.eu> wrote: > Reviewing the code, I have found another potential issue: the SRR (sampling rate) value is not validated; only values from 7 to 31 are permissible, according to the datasheet. The values in the debug output above would be fine, though. Isn't that handled by .sampling_rate_mask = SCI_SR_RANGE(8, 32), ? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 9:56 ` Geert Uytterhoeven @ 2019-03-29 10:35 ` Ulrich Hecht 2019-03-29 11:01 ` Ulrich Hecht 0 siblings, 1 reply; 18+ messages in thread From: Ulrich Hecht @ 2019-03-29 10:35 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Dirk Behme, Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH > On March 29, 2019 at 10:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > Hi Uli, > > On Fri, Mar 29, 2019 at 10:46 AM Ulrich Hecht <uli@fpond.eu> wrote: > > Reviewing the code, I have found another potential issue: the SRR (sampling rate) value is not validated; only values from 7 to 31 are permissible, according to the datasheet. The values in the debug output above would be fine, though. > > Isn't that handled by > > .sampling_rate_mask = SCI_SR_RANGE(8, 32), > > ? Having crawled through the related macros, I can confirm that it is. CU Uli ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 10:35 ` Ulrich Hecht @ 2019-03-29 11:01 ` Ulrich Hecht 0 siblings, 0 replies; 18+ messages in thread From: Ulrich Hecht @ 2019-03-29 11:01 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Dirk Behme, Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH > On March 29, 2019 at 11:35 AM Ulrich Hecht <uli@fpond.eu> wrote: > > > > > On March 29, 2019 at 10:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > Hi Uli, > > > > On Fri, Mar 29, 2019 at 10:46 AM Ulrich Hecht <uli@fpond.eu> wrote: > > > Reviewing the code, I have found another potential issue: the SRR (sampling rate) value is not validated; only values from 7 to 31 are permissible, according to the datasheet. The values in the debug output above would be fine, though. > > > > Isn't that handled by > > > > .sampling_rate_mask = SCI_SR_RANGE(8, 32), > > > > ? > > Having crawled through the related macros, I can confirm that it is. There is, however, another restriction that is not checked for: The sampling point shift must not exceed half of the sampling rate in either direction. This could happen ATM if the sampling rate is below 16 and the deviation from the desired bit rate is quite large. That is not so in the case for which debug output has been provided, though, in which the bit rate error is so small that shifting isn't enabled to begin with. I would thus still like to know the numbers for 115200 bps. CU Uli ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 9:46 ` Ulrich Hecht 2019-03-29 9:56 ` Geert Uytterhoeven @ 2019-03-29 11:17 ` Dirk Behme 1 sibling, 0 replies; 18+ messages in thread From: Dirk Behme @ 2019-03-29 11:17 UTC (permalink / raw) To: Ulrich Hecht, Geert Uytterhoeven, Eugeniu Rosca Cc: Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH On 29.03.2019 10:46, Ulrich Hecht wrote: > >> On March 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: >> >> >> Hi Geert, >> >> On 28.03.2019 12:30, Dirk Behme wrote: >>> On 28.03.2019 11:16, Dirk Behme wrote: >> * Testing the patch [5] >> >> - int shift = min(-8, max(7, deviation / 2)); >> + int shift = clamp(deviation / 2, -8, 7); >> >> does *not* fix our issue. Or in other words: Testing was *not* successful. >> >> * However, from review point of view we think that it fixes a serious >> bug. So maybe it should be applied, anyhow? > > It should. > >> >> * Using strace we managed to get some more information about the usage >> of the serial port [6]. With this, we are talking about 57600 and not 115200 >> >> * Switching to dev_info() [7] as requested above we get >> >> [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, >> base_baud = 0) is a hscif >> [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 >> [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps >> [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 >> [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps >> >> * We are talking about a custom r8a7796 board > > Reviewing the code, I have found another potential issue: the SRR (sampling rate) value is not validated; only values from 7 to 31 are permissible, according to the datasheet. The values in the debug output above would be fine, though. > > So, for clarification: Is there a problem at 9600/57600 bps, or does it only occur at 115200 bps? The 115200 bps in the initial report was wrong (sorry!). Looking at the strace output provided to my understanding the issue happens with 57600 bps (it somehow starts with 9600 bps and switches to 57600 bps, then). I have to think about 115200 bps as currently we have only a precompiled binary to test with (the one the strace is from). Best regards Dirk ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 7:05 ` Dirk Behme 2019-03-29 9:46 ` Ulrich Hecht @ 2019-03-29 9:46 ` Geert Uytterhoeven 2019-03-29 12:13 ` Dirk Behme 1 sibling, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2019-03-29 9:46 UTC (permalink / raw) To: Dirk Behme Cc: Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH Hi Dirk, On Fri, Mar 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: > On 28.03.2019 12:30, Dirk Behme wrote: > > On 28.03.2019 11:16, Dirk Behme wrote: > >> On 28.03.2019 10:24, Geert Uytterhoeven wrote: > >>> On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> > >>> wrote: > >>>> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and > >>>> the > >>>> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], > >>>> which > >>>> mirrors v4.18-rc1 commit [4] in mainline. > >>>> > >>>> JFYI, quite far away in the delivery chain, we've received below > >>>> report: > >>>> > >>>>> With this patch [2-4] there are reports about broken data > >>>>> communication with 115200 baud with SXM module. Reverting > >>>>> this patch results in successful communication, again. > >>>> > >>>> While this scarce information barely helps anybody, I thought that > >>>> sharing it with you might be beneficial in case you collect several > >>>> reports linked to this specific commit in future, meaning it > >>>> potentially > >>>> adds a regression. > >>>> > >>>> Also, if you are aware of any userland changes that might be > >>>> required/assumed by this patch or in case you have any alternative > >>>> ideas how to avoid reverting this patch, your feedback would be very > >>>> appreciated. > >>> > >>> Thanks for your report! > >>> > >>> [TLDR: skip to the suggested fix below; I only noticed the bug after > >>> writing the below paragraphs, which are still useful > >>> questions to > >>> let us reproduce the issue] > >>> > >>> Which SoC are you using? > >>> I assume this is on a custom board, as Salvator-X(S) and ULCB have > >>> external SCIF clock crystals, which allow to use a perfect 115200 bps, > >>> hence the affected code path is not exercised: > >>> > >>> sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 > >>> sh-sci e6550000.serial: Using clk scif for 115200+0 bps > >>> > >>> Does your board have an external SCIF clock? Which frequency? > >>> Can you check the clock values and deviation for your configuration, by > >>> changing the calls to print the above information from dev_dbg() to > >>> dev_info()? > >>> > >>> Does adding the DIV_ROUND_CLOSEST(), as suggested in my review > >>> of the posted patch, help? > >>> Perhaps the sampling point shift is inverted? Does -shift work better? > >>> > >>> [possible solution] > >>> > >>>> + int shift = min(-8, max(7, deviation > >>>> / 2)); > >>> > >>> Oops, min and max are exchanged! > >>> > >>> I guess using > >>> > >>> int shift = clamp(deviation / 2, -8, 7) > >>> > >>> instead fixes the issue? > >> > >> > >> Uh, that was fast :) Many thanks! > >> > >> We will test this as fast as possible! But due to the long delivery > >> chain Eugeniu mentioned this will take some time. I'll try my best to > >> come back to you as fast as possible. > > > > > > Just for the archives: We are testing the attached patch. > > > * Testing the patch [5] > > - int shift = min(-8, max(7, deviation / 2)); > + int shift = clamp(deviation / 2, -8, 7); > > does *not* fix our issue. Or in other words: Testing was *not* successful. I'm sorry to hear that. > * However, from review point of view we think that it fixes a serious > bug. So maybe it should be applied, anyhow? Yes, submitted. > * Using strace we managed to get some more information about the usage > of the serial port [6]. With this, we are talking about 57600 and not 115200 > > * Switching to dev_info() [7] as requested above we get > > [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, > base_baud = 0) is a hscif > [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 > [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps > [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 > [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps > > * We are talking about a custom r8a7796 board The above values match with what I see on Salvator-X, after disabling scif_clk in DTS. If your board does have scif_clk populated, you want to describe that in DT, as it may improve the situation. BTW, have you verified the actual clock rate used? It wouldn't be the first time that a crystal description in DTS has a typo in its clock-frequency property, breaking UART communication at "high" speeds. A simple way to do that is outputting a square wave using: yes U | tr -d "\n" > /dev/ttySC3 This should give an (approximate) square wave of 28797 Hz. > From 9a3c199f02cb66cb67e93e0824b03b622ab3b024 Mon Sep 17 00:00:00 2001 > From: Dirk Behme <dirk.behme@de.bosch.com> > Date: Fri, 29 Mar 2019 06:39:31 +0100 > Subject: [PATCH] serial: sh-sci: Enable debug output > > Enable some debug output as requested by Geert in > > https://patchwork.kernel.org/patch/10322809/#22554727 > > Change-Id: Icd2f97138516a0e40726fa1ccd50a69abb57cb76 > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > --- > drivers/tty/serial/sh-sci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index eba08cd892e5..32210cf2413c 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2161,7 +2161,7 @@ static int sci_brg_calc(struct sci_port *s, > unsigned int bps, > break; > } > > - dev_dbg(s->port.dev, "BRG: %u%+d bps using DL %u SR %u\n", bps, > + dev_info(s->port.dev, "BRG: %u%+d bps using DL %u SR %u\n", bps, > min_err, *dlr, *srr + 1); > return min_err; > } > @@ -2376,7 +2376,7 @@ static void sci_set_termios(struct uart_port > *port, struct ktermios *termios, > > done: > if (best_clk >= 0) > - dev_dbg(port->dev, "Using clk %pC for %u%+d bps\n", > + dev_info(port->dev, "Using clk %pC for %u%+d bps\n", > s->clks[best_clk], baud, min_err); > > sci_port_enable(s); You way want to enable printing of the other debug lines that contain "+d bps", for comparison with SCK and BRR alternatives. But in the absence of a suitable scif_clk, the clock rate achieved using the BRG is the best option. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 9:46 ` Geert Uytterhoeven @ 2019-03-29 12:13 ` Dirk Behme 2019-03-29 13:00 ` Geert Uytterhoeven 0 siblings, 1 reply; 18+ messages in thread From: Dirk Behme @ 2019-03-29 12:13 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH On 29.03.2019 10:46, Geert Uytterhoeven wrote: > Hi Dirk, > > On Fri, Mar 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: >> On 28.03.2019 12:30, Dirk Behme wrote: >>> On 28.03.2019 11:16, Dirk Behme wrote: >>>> On 28.03.2019 10:24, Geert Uytterhoeven wrote: >>>>> On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> >>>>> wrote: >>>>>> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and >>>>>> the >>>>>> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], >>>>>> which >>>>>> mirrors v4.18-rc1 commit [4] in mainline. >>>>>> >>>>>> JFYI, quite far away in the delivery chain, we've received below >>>>>> report: >>>>>> >>>>>>> With this patch [2-4] there are reports about broken data >>>>>>> communication with 115200 baud with SXM module. Reverting >>>>>>> this patch results in successful communication, again. >>>>>> >>>>>> While this scarce information barely helps anybody, I thought that >>>>>> sharing it with you might be beneficial in case you collect several >>>>>> reports linked to this specific commit in future, meaning it >>>>>> potentially >>>>>> adds a regression. >>>>>> >>>>>> Also, if you are aware of any userland changes that might be >>>>>> required/assumed by this patch or in case you have any alternative >>>>>> ideas how to avoid reverting this patch, your feedback would be very >>>>>> appreciated. >>>>> >>>>> Thanks for your report! >>>>> >>>>> [TLDR: skip to the suggested fix below; I only noticed the bug after >>>>> writing the below paragraphs, which are still useful >>>>> questions to >>>>> let us reproduce the issue] >>>>> >>>>> Which SoC are you using? >>>>> I assume this is on a custom board, as Salvator-X(S) and ULCB have >>>>> external SCIF clock crystals, which allow to use a perfect 115200 bps, >>>>> hence the affected code path is not exercised: >>>>> >>>>> sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 >>>>> sh-sci e6550000.serial: Using clk scif for 115200+0 bps >>>>> >>>>> Does your board have an external SCIF clock? Which frequency? >>>>> Can you check the clock values and deviation for your configuration, by >>>>> changing the calls to print the above information from dev_dbg() to >>>>> dev_info()? >>>>> >>>>> Does adding the DIV_ROUND_CLOSEST(), as suggested in my review >>>>> of the posted patch, help? >>>>> Perhaps the sampling point shift is inverted? Does -shift work better? >>>>> >>>>> [possible solution] >>>>> >>>>>> + int shift = min(-8, max(7, deviation >>>>>> / 2)); >>>>> >>>>> Oops, min and max are exchanged! >>>>> >>>>> I guess using >>>>> >>>>> int shift = clamp(deviation / 2, -8, 7) >>>>> >>>>> instead fixes the issue? >>>> >>>> >>>> Uh, that was fast :) Many thanks! >>>> >>>> We will test this as fast as possible! But due to the long delivery >>>> chain Eugeniu mentioned this will take some time. I'll try my best to >>>> come back to you as fast as possible. >>> >>> >>> Just for the archives: We are testing the attached patch. >> >> >> * Testing the patch [5] >> >> - int shift = min(-8, max(7, deviation / 2)); >> + int shift = clamp(deviation / 2, -8, 7); >> >> does *not* fix our issue. Or in other words: Testing was *not* successful. > > I'm sorry to hear that. > >> * However, from review point of view we think that it fixes a serious >> bug. So maybe it should be applied, anyhow? > > Yes, submitted. > >> * Using strace we managed to get some more information about the usage >> of the serial port [6]. With this, we are talking about 57600 and not 115200 >> >> * Switching to dev_info() [7] as requested above we get >> >> [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, >> base_baud = 0) is a hscif >> [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 >> [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps >> [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 >> [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps >> >> * We are talking about a custom r8a7796 board > > The above values match with what I see on Salvator-X, after disabling > scif_clk in DTS. > > If your board does have scif_clk populated, you want to describe that in > DT, as it may improve the situation. > > BTW, have you verified the actual clock rate used? It wouldn't be the > first time that a crystal description in DTS has a typo in its > clock-frequency property, breaking UART communication at "high" > speeds. > A simple way to do that is outputting a square wave using: > > yes U | tr -d "\n" > /dev/ttySC3 > > This should give an (approximate) square wave of 28797 Hz. > >> From 9a3c199f02cb66cb67e93e0824b03b622ab3b024 Mon Sep 17 00:00:00 2001 >> From: Dirk Behme <dirk.behme@de.bosch.com> >> Date: Fri, 29 Mar 2019 06:39:31 +0100 >> Subject: [PATCH] serial: sh-sci: Enable debug output >> >> Enable some debug output as requested by Geert in >> >> https://patchwork.kernel.org/patch/10322809/#22554727 >> >> Change-Id: Icd2f97138516a0e40726fa1ccd50a69abb57cb76 >> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> >> --- >> drivers/tty/serial/sh-sci.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> index eba08cd892e5..32210cf2413c 100644 >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -2161,7 +2161,7 @@ static int sci_brg_calc(struct sci_port *s, >> unsigned int bps, >> break; >> } >> >> - dev_dbg(s->port.dev, "BRG: %u%+d bps using DL %u SR %u\n", bps, >> + dev_info(s->port.dev, "BRG: %u%+d bps using DL %u SR %u\n", bps, >> min_err, *dlr, *srr + 1); >> return min_err; >> } >> @@ -2376,7 +2376,7 @@ static void sci_set_termios(struct uart_port >> *port, struct ktermios *termios, >> >> done: >> if (best_clk >= 0) >> - dev_dbg(port->dev, "Using clk %pC for %u%+d bps\n", >> + dev_info(port->dev, "Using clk %pC for %u%+d bps\n", >> s->clks[best_clk], baud, min_err); >> >> sci_port_enable(s); > > You way want to enable printing of the other debug lines that contain > "+d bps", for comparison with SCK and BRR alternatives. > But in the absence of a suitable scif_clk, the clock rate achieved using > the BRG is the best option. Do you have any idea what might be the difference between reverting "serial: sh-sci: Support for HSCIF RX sampling point adjustment" (works) and not reverting that (doesn't work for us), then? Best regards Dirk P.S.: I'll check the other topics above, thanks! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 12:13 ` Dirk Behme @ 2019-03-29 13:00 ` Geert Uytterhoeven 2019-03-29 14:11 ` Ulrich Hecht 2019-04-01 6:05 ` Dirk Behme 0 siblings, 2 replies; 18+ messages in thread From: Geert Uytterhoeven @ 2019-03-29 13:00 UTC (permalink / raw) To: Dirk Behme Cc: Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH Hi Dirk, On Fri, Mar 29, 2019 at 1:13 PM Dirk Behme <dirk.behme@de.bosch.com> wrote: > On 29.03.2019 10:46, Geert Uytterhoeven wrote: > > On Fri, Mar 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: > >> On 28.03.2019 12:30, Dirk Behme wrote: > >>> On 28.03.2019 11:16, Dirk Behme wrote: > >>>> On 28.03.2019 10:24, Geert Uytterhoeven wrote: > >>>>> On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> > >>>>> wrote: > >>>>>> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and > >>>>>> the > >>>>>> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], > >>>>>> which > >>>>>> mirrors v4.18-rc1 commit [4] in mainline. > >>>>>> > >>>>>> JFYI, quite far away in the delivery chain, we've received below > >>>>>> report: > >>>>>> > >>>>>>> With this patch [2-4] there are reports about broken data > >>>>>>> communication with 115200 baud with SXM module. Reverting > >>>>>>> this patch results in successful communication, again. > >>>>>> > >>>>>> While this scarce information barely helps anybody, I thought that > >>>>>> sharing it with you might be beneficial in case you collect several > >>>>>> reports linked to this specific commit in future, meaning it > >>>>>> potentially > >>>>>> adds a regression. > >>>>>> > >>>>>> Also, if you are aware of any userland changes that might be > >>>>>> required/assumed by this patch or in case you have any alternative > >>>>>> ideas how to avoid reverting this patch, your feedback would be very > >>>>>> appreciated. > >>>>> > >>>>> Thanks for your report! > >>>>> > >>>>> [TLDR: skip to the suggested fix below; I only noticed the bug after > >>>>> writing the below paragraphs, which are still useful > >>>>> questions to > >>>>> let us reproduce the issue] > >>>>> > >>>>> Which SoC are you using? > >>>>> I assume this is on a custom board, as Salvator-X(S) and ULCB have > >>>>> external SCIF clock crystals, which allow to use a perfect 115200 bps, > >>>>> hence the affected code path is not exercised: > >>>>> > >>>>> sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 > >>>>> sh-sci e6550000.serial: Using clk scif for 115200+0 bps > >>>>> > >>>>> Does your board have an external SCIF clock? Which frequency? > >>>>> Can you check the clock values and deviation for your configuration, by > >>>>> changing the calls to print the above information from dev_dbg() to > >>>>> dev_info()? > >>>>> > >>>>> Does adding the DIV_ROUND_CLOSEST(), as suggested in my review > >>>>> of the posted patch, help? > >>>>> Perhaps the sampling point shift is inverted? Does -shift work better? > >>>>> > >>>>> [possible solution] > >>>>> > >>>>>> + int shift = min(-8, max(7, deviation > >>>>>> / 2)); > >>>>> > >>>>> Oops, min and max are exchanged! > >>>>> > >>>>> I guess using > >>>>> > >>>>> int shift = clamp(deviation / 2, -8, 7) > >>>>> > >>>>> instead fixes the issue? > >>>> > >>>> > >>>> Uh, that was fast :) Many thanks! > >>>> > >>>> We will test this as fast as possible! But due to the long delivery > >>>> chain Eugeniu mentioned this will take some time. I'll try my best to > >>>> come back to you as fast as possible. > >>> > >>> > >>> Just for the archives: We are testing the attached patch. > >> > >> > >> * Testing the patch [5] > >> > >> - int shift = min(-8, max(7, deviation / 2)); > >> + int shift = clamp(deviation / 2, -8, 7); > >> > >> does *not* fix our issue. Or in other words: Testing was *not* successful. > > > > I'm sorry to hear that. > > > >> * However, from review point of view we think that it fixes a serious > >> bug. So maybe it should be applied, anyhow? > > > > Yes, submitted. > > > >> * Using strace we managed to get some more information about the usage > >> of the serial port [6]. With this, we are talking about 57600 and not 115200 > >> > >> * Switching to dev_info() [7] as requested above we get > >> > >> [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, > >> base_baud = 0) is a hscif > >> [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 > >> [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps > >> [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 > >> [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps > Do you have any idea what might be the difference between reverting > "serial: sh-sci: Support for HSCIF RX sampling point adjustment" (works) > and not reverting that (doesn't work for us), then? Before that commit, the RX sampling point was not shifted. After that commit, it was incorrectly shifted by -8[*]. With my fix, it is shifted by 7[*], to compensate for a clock rate that is slightly off. [*] In units of cycles of the sampling clock, which runs at SR * 57595 = 575950 Hz. However, doing the above calculation shows that's something wrong with the formula used by the driver: with SR = 10, the default sampling point at the center is at SR / 2 = 5, so the shift must be within [-4, +4], which is exceeded by using a value of 7. deviation = min_err * srr * last_stop / 2 / baud; With: min_err = -5 srr = 9 last_stop = 19 baud = 57600 Note that srr and baud are unsigned. Hence the multiplication and divisions are done in unsigned arithmetic, and we get deviation = 37282 instead of 0. Oops... Fixed by: - int deviation = min_err * srr * last_stop / 2 / baud; + int deviation = (int)(min_err * srr * last_stop) / 2 / + (int)baud; Before I sent a patch: Uli, shouldn't the formula use "(srr + 1)" instead of "srr", as the actual sampling rate factor is one more than the value programmed in HSSRR.SRCYC? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 13:00 ` Geert Uytterhoeven @ 2019-03-29 14:11 ` Ulrich Hecht 2019-04-01 6:05 ` Dirk Behme 1 sibling, 0 replies; 18+ messages in thread From: Ulrich Hecht @ 2019-03-29 14:11 UTC (permalink / raw) To: Geert Uytterhoeven, Dirk Behme Cc: Eugeniu Rosca, Ulrich Hecht, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH > On March 29, 2019 at 2:00 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Fixed by: > > - int deviation = min_err * srr * last_stop / 2 / baud; > + int deviation = (int)(min_err * srr * last_stop) / 2 / > + (int)baud; > > Before I sent a patch: Uli, shouldn't the formula use "(srr + 1)" > instead of "srr", as the actual sampling rate factor is one more than > the value programmed in HSSRR.SRCYC? Yes, it should. CU Uli ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment 2019-03-29 13:00 ` Geert Uytterhoeven 2019-03-29 14:11 ` Ulrich Hecht @ 2019-04-01 6:05 ` Dirk Behme 1 sibling, 0 replies; 18+ messages in thread From: Dirk Behme @ 2019-04-01 6:05 UTC (permalink / raw) To: Geert Uytterhoeven, Ulrich Hecht Cc: Eugeniu Rosca, Wolfram Sang, Linux-Renesas, open list:SERIAL DRIVERS, Eugeniu Rosca, George G. Davis, Andy Lowe, Joshua Frkuska, Tobias Franzen, Magnus Damm, Greg KH [-- Attachment #1: Type: text/plain, Size: 6057 bytes --] Hi Geert, On 29.03.2019 14:00, Geert Uytterhoeven wrote: > Hi Dirk, > > On Fri, Mar 29, 2019 at 1:13 PM Dirk Behme <dirk.behme@de.bosch.com> wrote: >> On 29.03.2019 10:46, Geert Uytterhoeven wrote: >>> On Fri, Mar 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: >>>> On 28.03.2019 12:30, Dirk Behme wrote: >>>>> On 28.03.2019 11:16, Dirk Behme wrote: >>>>>> On 28.03.2019 10:24, Geert Uytterhoeven wrote: >>>>>>> On Wed, Mar 27, 2019 at 7:36 PM Eugeniu Rosca <erosca@de.adit-jv.com> >>>>>>> wrote: >>>>>>>> We've recently switched from rcar-3.7.x to rcar-3.9.x [1] kernel and >>>>>>>> the >>>>>>>> latter contains this patch [2] by virtue of rcar-3.9.0 commit [3], >>>>>>>> which >>>>>>>> mirrors v4.18-rc1 commit [4] in mainline. >>>>>>>> >>>>>>>> JFYI, quite far away in the delivery chain, we've received below >>>>>>>> report: >>>>>>>> >>>>>>>>> With this patch [2-4] there are reports about broken data >>>>>>>>> communication with 115200 baud with SXM module. Reverting >>>>>>>>> this patch results in successful communication, again. >>>>>>>> >>>>>>>> While this scarce information barely helps anybody, I thought that >>>>>>>> sharing it with you might be beneficial in case you collect several >>>>>>>> reports linked to this specific commit in future, meaning it >>>>>>>> potentially >>>>>>>> adds a regression. >>>>>>>> >>>>>>>> Also, if you are aware of any userland changes that might be >>>>>>>> required/assumed by this patch or in case you have any alternative >>>>>>>> ideas how to avoid reverting this patch, your feedback would be very >>>>>>>> appreciated. >>>>>>> >>>>>>> Thanks for your report! >>>>>>> >>>>>>> [TLDR: skip to the suggested fix below; I only noticed the bug after >>>>>>> writing the below paragraphs, which are still useful >>>>>>> questions to >>>>>>> let us reproduce the issue] >>>>>>> >>>>>>> Which SoC are you using? >>>>>>> I assume this is on a custom board, as Salvator-X(S) and ULCB have >>>>>>> external SCIF clock crystals, which allow to use a perfect 115200 bps, >>>>>>> hence the affected code path is not exercised: >>>>>>> >>>>>>> sh-sci e6550000.serial: BRG: 115200+0 bps using DL 4 SR 32 >>>>>>> sh-sci e6550000.serial: Using clk scif for 115200+0 bps >>>>>>> >>>>>>> Does your board have an external SCIF clock? Which frequency? >>>>>>> Can you check the clock values and deviation for your configuration, by >>>>>>> changing the calls to print the above information from dev_dbg() to >>>>>>> dev_info()? >>>>>>> >>>>>>> Does adding the DIV_ROUND_CLOSEST(), as suggested in my review >>>>>>> of the posted patch, help? >>>>>>> Perhaps the sampling point shift is inverted? Does -shift work better? >>>>>>> >>>>>>> [possible solution] >>>>>>> >>>>>>>> + int shift = min(-8, max(7, deviation >>>>>>>> / 2)); >>>>>>> >>>>>>> Oops, min and max are exchanged! >>>>>>> >>>>>>> I guess using >>>>>>> >>>>>>> int shift = clamp(deviation / 2, -8, 7) >>>>>>> >>>>>>> instead fixes the issue? >>>>>> >>>>>> >>>>>> Uh, that was fast :) Many thanks! >>>>>> >>>>>> We will test this as fast as possible! But due to the long delivery >>>>>> chain Eugeniu mentioned this will take some time. I'll try my best to >>>>>> come back to you as fast as possible. >>>>> >>>>> >>>>> Just for the archives: We are testing the attached patch. >>>> >>>> >>>> * Testing the patch [5] >>>> >>>> - int shift = min(-8, max(7, deviation / 2)); >>>> + int shift = clamp(deviation / 2, -8, 7); >>>> >>>> does *not* fix our issue. Or in other words: Testing was *not* successful. >>> >>> I'm sorry to hear that. >>> >>>> * However, from review point of view we think that it fixes a serious >>>> bug. So maybe it should be applied, anyhow? >>> >>> Yes, submitted. >>> >>>> * Using strace we managed to get some more information about the usage >>>> of the serial port [6]. With this, we are talking about 57600 and not 115200 >>>> >>>> * Switching to dev_info() [7] as requested above we get >>>> >>>> [ 0.553256] e6560000.serial: ttySC3 at MMIO 0xe6560000 (irq = 41, >>>> base_baud = 0) is a hscif >>>> [ 161.418527] sh-sci e6560000.serial: BRG: 9600+0 bps using DL 1462 SR 19 >>>> [ 161.418543] sh-sci e6560000.serial: Using clk s3d1 for 9600+0 bps >>>> [ 161.418813] sh-sci e6560000.serial: BRG: 57600-5 bps using DL 463 SR 10 >>>> [ 161.418824] sh-sci e6560000.serial: Using clk s3d1 for 57600-5 bps > >> Do you have any idea what might be the difference between reverting >> "serial: sh-sci: Support for HSCIF RX sampling point adjustment" (works) >> and not reverting that (doesn't work for us), then? > > Before that commit, the RX sampling point was not shifted. > After that commit, it was incorrectly shifted by -8[*]. > With my fix, it is shifted by 7[*], to compensate for a clock rate that > is slightly off. > > [*] In units of cycles of the sampling clock, which runs at SR * 57595 = > 575950 Hz. > > However, doing the above calculation shows that's something wrong with > the formula used by the driver: with SR = 10, the default sampling point > at the center is at SR / 2 = 5, so the shift must be within [-4, +4], which > is exceeded by using a value of 7. > > deviation = min_err * srr * last_stop / 2 / baud; > > With: > > min_err = -5 > srr = 9 > last_stop = 19 > baud = 57600 > > Note that srr and baud are unsigned. Hence the multiplication and > divisions are done in unsigned arithmetic, and we get deviation = 37282 > instead of 0. Oops... > > Fixed by: > > - int deviation = min_err * srr * last_stop / 2 / baud; > + int deviation = (int)(min_err * srr * last_stop) / 2 / > + (int)baud; > > Before I sent a patch: Uli, shouldn't the formula use "(srr + 1)" > instead of "srr", as the actual sampling rate factor is one more than > the value programmed in HSSRR.SRCYC? Using both attached patches together our tests have been *successful*, now. :) Many thanks! Best regards Dirk [-- Attachment #2: 0002-serial-sh-sci-Fix-HSCIF-RX-deviation-calculation.patch --] [-- Type: text/plain, Size: 1772 bytes --] From eac98a31c3f654ac2e7d6d77dcc648d4a504c1a5 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven <geert+renesas@glider.be> Date: Mon, 1 Apr 2019 06:30:04 +0200 Subject: [PATCH 2/2] serial: sh-sci: Fix HSCIF RX deviation calculation Doing the deviation calculation manually shows that's something wrong with the formula used by the driver: with SR = 10, the default sampling point at the center is at SR / 2 = 5, so the shift must be within [-4, +4], which is exceeded by using a value of 7. deviation = min_err * srr * last_stop / 2 / baud; With: min_err = -5 srr = 9 last_stop = 19 baud = 57600 Note that srr and baud are unsigned. Hence the multiplication and divisions are done in unsigned arithmetic, and we get deviation = 37282 instead of 0. Fix this. Additionally, use "(srr + 1)" instead of "srr", as the actual sampling rate factor is one more than the value programmed in HSSRR.SRCYC. Fixes: 63ba1e00f178a448 ("serial: sh-sci: Support for HSCIF RX sampling point adjustment") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/tty/serial/sh-sci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 69896d586a29..971a2dee4d1e 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2439,7 +2439,8 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, * center of the last stop bit in sampling clocks. */ int last_stop = bits * 2 - 1; - int deviation = min_err * srr * last_stop / 2 / baud; + int deviation = (int)(min_err * (srr + 1) * last_stop) / + 2 / (int)baud; if (abs(deviation) >= 2) { /* At least two sampling clocks off at the -- 2.20.0 [-- Attachment #3: 0001-serial-sh-sci-Fix-HSCIF-RX-sampling-point-adjustment.patch --] [-- Type: text/plain, Size: 1153 bytes --] From a6d8b37dae6ee8621002804ca7841c913b54483b Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven <geert+renesas@glider.be> Date: Fri, 29 Mar 2019 10:10:26 +0100 Subject: [PATCH 1/2] serial: sh-sci: Fix HSCIF RX sampling point adjustment The calculation of the sampling point has min() and max() exchanged. Fix this by using the clamp() helper instead. Fixes: 63ba1e00f178a448 ("serial: sh-sci: Support for HSCIF RX sampling point adjustment") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/tty/serial/sh-sci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index eba08cd892e5..69896d586a29 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2446,7 +2446,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, * last stop bit; we can increase the error * margin by shifting the sampling point. */ - int shift = min(-8, max(7, deviation / 2)); + int shift = clamp(deviation / 2, -8, 7); hssrr |= (shift << HSCIF_SRHP_SHIFT) & HSCIF_SRHP_MASK; -- 2.20.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-04-01 6:05 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-04 15:48 [PATCH v2] serial: sh-sci: Support for HSCIF RX sampling point adjustment Ulrich Hecht 2018-04-24 15:15 ` Geert Uytterhoeven 2019-03-27 18:35 ` Eugeniu Rosca 2019-03-28 8:33 ` Wolfram Sang 2019-03-28 9:24 ` Geert Uytterhoeven 2019-03-28 10:16 ` Dirk Behme 2019-03-28 11:30 ` Dirk Behme 2019-03-29 7:05 ` Dirk Behme 2019-03-29 9:46 ` Ulrich Hecht 2019-03-29 9:56 ` Geert Uytterhoeven 2019-03-29 10:35 ` Ulrich Hecht 2019-03-29 11:01 ` Ulrich Hecht 2019-03-29 11:17 ` Dirk Behme 2019-03-29 9:46 ` Geert Uytterhoeven 2019-03-29 12:13 ` Dirk Behme 2019-03-29 13:00 ` Geert Uytterhoeven 2019-03-29 14:11 ` Ulrich Hecht 2019-04-01 6:05 ` Dirk Behme
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).