diff mbox

[v4,3/4] serial: sh-sci: make RX FIFO parameters tunable via sysfs

Message ID CAO3366zSvO=q+nZT+QfKefuH7+x7vy1HttoXPDCeA7a4f5MKSA@mail.gmail.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Ulrich Hecht Feb. 8, 2017, 10:04 a.m. UTC 
On Tue, Feb 7, 2017 at 3:40 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulrich,
>
> On Fri, Feb 3, 2017 at 11:38 AM, Ulrich Hecht
> <ulrich.hecht+renesas@gmail.com> wrote:
>> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
>> only applicable to SCIFA and SCIFB).
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/tty/serial/sh-sci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 87 insertions(+)
>>
>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> index 4a165ed..f95a56c 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
>>         scif_set_rtrg(port, 1);
>>  }
>>
>> +static ssize_t rx_trigger_show(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              char *buf)
>> +{
>> +       struct uart_port *port = dev_get_drvdata(dev);
>> +       struct sci_port *sci = to_sci_port(port);
>> +
>> +       return sprintf(buf, "%d\n", sci->rx_trigger);
>> +}
>> +
>> +static ssize_t rx_trigger_store(struct device *dev,
>> +                               struct device_attribute *attr,
>> +                               const char *buf,
>> +                               size_t count)
>> +{
>> +       struct uart_port *port = dev_get_drvdata(dev);
>> +       struct sci_port *sci = to_sci_port(port);
>> +       long r;
>> +
>> +       if (kstrtol(buf, 0, &r) == -EINVAL)
>> +               return -EINVAL;
>> +       sci->rx_trigger = scif_set_rtrg(port, r);
>> +       scif_set_rtrg(port, 1);
>
> I seem to have missed the above function call during my earlier review.
> What's the purpose of resetting the trigger immediately to 1?

For the software timeout case, the timeout and the trigger levels are
set in the interrupt handler. Setting the threshold to 1 will trigger
that when the next byte of data comes in, and it is easier than
duplicating the logic here.

(There actually is a bug here, in that the threshold should only be
reset to 1 for software timeout IPs (SCIFA and SCIFB), but that is not
what breaks SCIFA, of course.)

> I.e. "echo 1 > /sys/class/tty/ttySC0/device/rx_fifo_trigger" fixes serial
> console input on e.g. armadillo, but echoing 32 into rx_fifo_trigger doesn't
> break it again.

This is intended to work that way. For software timeout devices (SCIFA
and SCIFB), the trigger level is not set in hardware unless an
rx_fifo_timeout > 0 is set as well.

The bug that breaks the SCIFA console is in sci_reset(), which sets a
hardware threshold > 1 for devices for software timeout devices even
though the rx_fifo_timeout is 0. Something like this should fix it:


Could you try and check if that works for you?

CU
Uli

Comments

Geert Uytterhoeven Feb. 8, 2017, 10:38 a.m. UTC | #1 
Hi Uli,

On Wed, Feb 8, 2017 at 11:04 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> On Tue, Feb 7, 2017 at 3:40 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Feb 3, 2017 at 11:38 AM, Ulrich Hecht
>> <ulrich.hecht+renesas@gmail.com> wrote:
>>> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
>>> only applicable to SCIFA and SCIFB).
>>>
>>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/tty/serial/sh-sci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 87 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>>> index 4a165ed..f95a56c 100644
>>> --- a/drivers/tty/serial/sh-sci.c
>>> +++ b/drivers/tty/serial/sh-sci.c
>>> @@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
>>>         scif_set_rtrg(port, 1);
>>>  }
>>>
>>> +static ssize_t rx_trigger_show(struct device *dev,
>>> +                              struct device_attribute *attr,
>>> +                              char *buf)
>>> +{
>>> +       struct uart_port *port = dev_get_drvdata(dev);
>>> +       struct sci_port *sci = to_sci_port(port);
>>> +
>>> +       return sprintf(buf, "%d\n", sci->rx_trigger);
>>> +}
>>> +
>>> +static ssize_t rx_trigger_store(struct device *dev,
>>> +                               struct device_attribute *attr,
>>> +                               const char *buf,
>>> +                               size_t count)
>>> +{
>>> +       struct uart_port *port = dev_get_drvdata(dev);
>>> +       struct sci_port *sci = to_sci_port(port);
>>> +       long r;
>>> +
>>> +       if (kstrtol(buf, 0, &r) == -EINVAL)
>>> +               return -EINVAL;
>>> +       sci->rx_trigger = scif_set_rtrg(port, r);
>>> +       scif_set_rtrg(port, 1);
>>
>> I seem to have missed the above function call during my earlier review.
>> What's the purpose of resetting the trigger immediately to 1?
>
> For the software timeout case, the timeout and the trigger levels are
> set in the interrupt handler. Setting the threshold to 1 will trigger
> that when the next byte of data comes in, and it is easier than
> duplicating the logic here.

OK.

> (There actually is a bug here, in that the threshold should only be
> reset to 1 for software timeout IPs (SCIFA and SCIFB), but that is not
> what breaks SCIFA, of course.)

I guess you'll send a patch to fix that, too?

>> I.e. "echo 1 > /sys/class/tty/ttySC0/device/rx_fifo_trigger" fixes serial
>> console input on e.g. armadillo, but echoing 32 into rx_fifo_trigger doesn't
>> break it again.
>
> This is intended to work that way. For software timeout devices (SCIFA
> and SCIFB), the trigger level is not set in hardware unless an
> rx_fifo_timeout > 0 is set as well.
>
> The bug that breaks the SCIFA console is in sci_reset(), which sets a
> hardware threshold > 1 for devices for software timeout devices even
> though the rx_fifo_timeout is 0. Something like this should fix it:
>
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2179,7 +2179,11 @@ static void sci_reset(struct uart_port *port)
>                         setup_timer(&s->rx_fifo_timer, rx_fifo_timer_fn,
>                                     (unsigned long)s);
>                 } else {
> -                       scif_set_rtrg(port, s->rx_trigger);
> +                       if (port->type == PORT_SCIFA ||
> +                           port->type == PORT_SCIFB)
> +                               scif_set_rtrg(port, 1);
> +                       else
> +                               scif_set_rtrg(port, s->rx_trigger);
>                 }
>         }
>  }
>
> Could you try and check if that works for you?

Thanks, that unbroke serial console input on all my boards with SCIFA consoles.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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
diff mbox

Patch

--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2179,7 +2179,11 @@  static void sci_reset(struct uart_port *port)
                        setup_timer(&s->rx_fifo_timer, rx_fifo_timer_fn,
                                    (unsigned long)s);
                } else {
-                       scif_set_rtrg(port, s->rx_trigger);
+                       if (port->type == PORT_SCIFA ||
+                           port->type == PORT_SCIFB)
+                               scif_set_rtrg(port, 1);
+                       else
+                               scif_set_rtrg(port, s->rx_trigger);
                }
        }
 }