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@xxxxxxxxxxxx> wrote:
On 29.03.2019 10:46, Geert Uytterhoeven wrote:
On Fri, Mar 29, 2019 at 8:05 AM Dirk Behme <dirk.behme@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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
From eac98a31c3f654ac2e7d6d77dcc648d4a504c1a5 Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
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@xxxxxxxxx>
---
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
From a6d8b37dae6ee8621002804ca7841c913b54483b Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
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@xxxxxxxxx>
---
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
