| Message ID | 20210409082524.3480-1-uli+renesas@fpond.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | serial: sh-sci: correct units in comment about DMA timeout | expand |
Hi Ulrich-san, Thank you for the patch! > From: Ulrich Hecht, Sent: Friday, April 9, 2021 5:25 PM > > Since the transition to hrtimers, the calculation does not involve jiffies > any longer, which has led to confusion. State the times in ms instead. IIUC, the unit of rx_timeout was changed from milliseconds to microseconds when hrtimer is used. So, almost all comments was not needed. Also, I'm wondering if the following condition is not needed or not. This is because this was "20ms". But, perhaps, the driver will not set rx_timeout to "20us" or small. if (s->rx_timeout < 20) s->rx_timeout = 20; Best regards, Yoshihiro Shimoda > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu> > --- > drivers/tty/serial/sh-sci.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index e3af97a59856..c4ce4cd120ba 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2613,11 +2613,11 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, > * Calculate delay for 2 DMA buffers (4 FIFO). > * See serial_core.c::uart_update_timeout(). > * With 10 bits (CS8), 250Hz, 115200 baud and 64 bytes FIFO, the above > - * function calculates 1 jiffie for the data plus 5 jiffies for the > - * "slop(e)." Then below we calculate 5 jiffies (20ms) for 2 DMA > - * buffers (4 FIFO sizes), but when performing a faster transfer, the > - * value obtained by this formula is too small. Therefore, if the value > - * is smaller than 20ms, use 20ms as the timeout value for DMA. > + * function calculates 4ms for the data plus 20ms for the "slop(e)." > + * Then below we calculate 20ms for 2 DMA buffers (4 FIFO sizes), > + * but when performing a faster transfer, the value obtained by this > + * formula is too small. Therefore, if the value is smaller than > + * 20ms, use 20ms as the timeout value for DMA. > */ > s->rx_frame = (10000 * bits) / (baud / 100); > #ifdef CONFIG_SERIAL_SH_SCI_DMA > -- > 2.20.1
> On 04/09/2021 2:16 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Thank you for the patch! > > > From: Ulrich Hecht, Sent: Friday, April 9, 2021 5:25 PM > > > > Since the transition to hrtimers, the calculation does not involve jiffies > > any longer, which has led to confusion. State the times in ms instead. > > IIUC, the unit of rx_timeout was changed from milliseconds to microseconds > when hrtimer is used. D'oh! > So, almost all comments was not needed. > > Also, I'm wondering if the following condition is not needed or not. > This is because this was "20ms". But, perhaps, the driver will not > set rx_timeout to "20us" or small. > > if (s->rx_timeout < 20) > s->rx_timeout = 20; A more helpful version of the comment is in 3089f381fbaf5: /* * Calculate delay for 1.5 DMA buffers: see * drivers/serial/serial_core.c::uart_update_timeout(). With 10 bits * (CS8), 250Hz, 115200 baud and 64 bytes FIFO, the above function * calculates 1 jiffie for the data plus 5 jiffies for the "slop(e)." * Then below we calculate 3 jiffies (12ms) for 1.5 DMA buffers (3 FIFO * sizes), but it has been found out experimentally, that this is not * enough: the driver too often needlessly runs on a DMA timeout. 20ms * as a minimum seem to work perfectly. */ I think we still want that, but it should of course be 20000, not 20. CU Uli
Hi Ulrich-san, > From: Ulrich Hecht, Sent: Saturday, April 10, 2021 1:05 AM > > > On 04/09/2021 2:16 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: <snip> > > So, almost all comments was not needed. > > > > Also, I'm wondering if the following condition is not needed or not. > > This is because this was "20ms". But, perhaps, the driver will not > > set rx_timeout to "20us" or small. > > > > if (s->rx_timeout < 20) > > s->rx_timeout = 20; > > A more helpful version of the comment is in 3089f381fbaf5: > > /* > * Calculate delay for 1.5 DMA buffers: see > * drivers/serial/serial_core.c::uart_update_timeout(). With 10 bits > * (CS8), 250Hz, 115200 baud and 64 bytes FIFO, the above function > * calculates 1 jiffie for the data plus 5 jiffies for the "slop(e)." > * Then below we calculate 3 jiffies (12ms) for 1.5 DMA buffers (3 FIFO > * sizes), but it has been found out experimentally, that this is not > * enough: the driver too often needlessly runs on a DMA timeout. 20ms > * as a minimum seem to work perfectly. > */ > > I think we still want that, but it should of course be 20000, not 20. Hmm, when we use HSCIF with 10 bits, 3000000 baud and 128 bytes FIFO, the rx_timeout value will be set to 1536 (us). So, if we set rx_timeout to 20000 (us) as a minimum value, the sh-sci' behavior will be back to non hrtimer support, IIUC. Perhaps, describing uart_update_timeout() and the jiffies value of uart_port->timeout with 115200 baud here may cause misreading?? I didn't understand the purpose of uart_port->timeout yet thought. But, at least, the current driver uses hrtimer to improve latency for HSCIF, the driver should not set 20000 (us) as a minimum value. Best regards, Yoshihiro Shimoda
> On 04/12/2021 8:23 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Hmm, when we use HSCIF with 10 bits, 3000000 baud and 128 bytes FIFO, > the rx_timeout value will be set to 1536 (us). So, if we set rx_timeout > to 20000 (us) as a minimum value, the sh-sci' behavior will be back to > non hrtimer support, IIUC. > > Perhaps, describing uart_update_timeout() and the jiffies value of > uart_port->timeout with 115200 baud here may cause misreading?? > I didn't understand the purpose of uart_port->timeout yet thought. > But, at least, the current driver uses hrtimer to improve latency > for HSCIF, the driver should not set 20000 (us) as a minimum value. Not having looked at this stuff in a while, I was under the impression that the rx timeout is an error condition, when it is in fact part of normal (DMA) operation. I think it was indeed the reference to uart_update_timeout() that threw me off... So if my understanding is correct now, we should scrap the minimum timeout code entirely because the condition it is supposed to prevent cannot occur any longer due to the switch to hrtimers. Did I get that right? CU Uli
Hi Ulrich-san, > From: Ulrich Hecht, Sent: Monday, April 12, 2021 5:04 PM > > > On 04/12/2021 8:23 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > Hmm, when we use HSCIF with 10 bits, 3000000 baud and 128 bytes FIFO, > > the rx_timeout value will be set to 1536 (us). So, if we set rx_timeout > > to 20000 (us) as a minimum value, the sh-sci' behavior will be back to > > non hrtimer support, IIUC. > > > > Perhaps, describing uart_update_timeout() and the jiffies value of > > uart_port->timeout with 115200 baud here may cause misreading?? > > I didn't understand the purpose of uart_port->timeout yet thought. > > But, at least, the current driver uses hrtimer to improve latency > > for HSCIF, the driver should not set 20000 (us) as a minimum value. > > Not having looked at this stuff in a while, I was under the impression that the rx timeout is an error condition, when > it is in fact part of normal (DMA) operation. I think it was indeed the reference to uart_update_timeout() that threw > me off... I think so... > So if my understanding is correct now, we should scrap the minimum timeout code entirely because the condition it is supposed > to prevent cannot occur any longer due to the switch to hrtimers. Did I get that right? Yes, this is the same as my understanding. Best regards, Yoshihiro Shimoda > CU > Uli
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index e3af97a59856..c4ce4cd120ba 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2613,11 +2613,11 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, * Calculate delay for 2 DMA buffers (4 FIFO). * See serial_core.c::uart_update_timeout(). * With 10 bits (CS8), 250Hz, 115200 baud and 64 bytes FIFO, the above - * function calculates 1 jiffie for the data plus 5 jiffies for the - * "slop(e)." Then below we calculate 5 jiffies (20ms) for 2 DMA - * buffers (4 FIFO sizes), but when performing a faster transfer, the - * value obtained by this formula is too small. Therefore, if the value - * is smaller than 20ms, use 20ms as the timeout value for DMA. + * function calculates 4ms for the data plus 20ms for the "slop(e)." + * Then below we calculate 20ms for 2 DMA buffers (4 FIFO sizes), + * but when performing a faster transfer, the value obtained by this + * formula is too small. Therefore, if the value is smaller than + * 20ms, use 20ms as the timeout value for DMA. */ s->rx_frame = (10000 * bits) / (baud / 100); #ifdef CONFIG_SERIAL_SH_SCI_DMA
Since the transition to hrtimers, the calculation does not involve jiffies any longer, which has led to confusion. State the times in ms instead. Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu> --- drivers/tty/serial/sh-sci.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)