diff mbox

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

Message ID 1486118300-12633-4-git-send-email-ulrich.hecht+renesas@gmail.com (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Ulrich Hecht Feb. 3, 2017, 10:38 a.m. UTC 
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(+)

Comments

Geert Uytterhoeven Feb. 7, 2017, 2:40 p.m. UTC | #1 
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?

This means I can change the value of scif->rx_trigger on the fly, but the
actual value programmed in the hardware is always 1?

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.

> +       return count;
> +}

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
Laurent Pinchart Feb. 12, 2017, 4:04 p.m. UTC | #2 
Hi Ulrich,

Thank you for the patch.

On Friday 03 Feb 2017 11:38:19 Ulrich Hecht 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);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(rx_fifo_trigger, 0644, rx_trigger_show,
> rx_trigger_store); +
> +static ssize_t rx_fifo_timeout_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_fifo_timeout);
> +}
> +
> +static ssize_t rx_fifo_timeout_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_fifo_timeout = r;
> +	scif_set_rtrg(port, 1);
> +	if (r > 0)
> +		setup_timer(&sci->rx_fifo_timer, rx_fifo_timer_fn,
> +			    (unsigned long)sci);

Where's the locking ?

> +	return count;
> +}
> +
> +static DEVICE_ATTR(rx_fifo_timeout, 0644, rx_fifo_timeout_show,
> rx_fifo_timeout_store); +
> +
>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>  static void sci_dma_tx_complete(void *arg)
>  {
> @@ -2886,6 +2946,15 @@ static int sci_remove(struct platform_device *dev)
> 
>  	sci_cleanup_single(port);
> 
> +	if (port->port.fifosize > 1) {
> +		sysfs_remove_file(&dev->dev.kobj,
> +				  &dev_attr_rx_fifo_trigger.attr);
> +	}
> +	if (port->port.type == PORT_SCIFA || port->port.type == PORT_SCIFB) {
> +		sysfs_remove_file(&dev->dev.kobj,
> +				  &dev_attr_rx_fifo_timeout.attr);
> +	}

No need for curly braces.

> +
>  	return 0;
>  }
> 
> @@ -3051,6 +3120,24 @@ static int sci_probe(struct platform_device *dev)
>  	if (ret)
>  		return ret;
> 
> +	if (sp->port.fifosize > 1) {
> +		ret = sysfs_create_file(&dev->dev.kobj,
> +				&dev_attr_rx_fifo_trigger.attr);

You should use device_create_file for device attributes. Even better would be 
to create an attribute group if possible.

> +		if (ret)
> +			return ret;
> +	}
> +	if (sp->port.type == PORT_SCIFA || sp->port.type ==  PORT_SCIFB) {
> +		ret = sysfs_create_file(&dev->dev.kobj,
> +				&dev_attr_rx_fifo_timeout.attr);
> +		if (ret) {
> +			if (sp->port.fifosize > 1) {
> +				sysfs_remove_file(&dev->dev.kobj,
> +					&dev_attr_rx_fifo_trigger.attr);
> +			}
> +			return ret;
> +		}
> +	}
> +
>  #ifdef CONFIG_SH_STANDARD_BIOS
>  	sh_bios_gdb_detach();
>  #endif
diff mbox

Patch

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);
+	return count;
+}
+
+static DEVICE_ATTR(rx_fifo_trigger, 0644, rx_trigger_show, rx_trigger_store);
+
+static ssize_t rx_fifo_timeout_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_fifo_timeout);
+}
+
+static ssize_t rx_fifo_timeout_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_fifo_timeout = r;
+	scif_set_rtrg(port, 1);
+	if (r > 0)
+		setup_timer(&sci->rx_fifo_timer, rx_fifo_timer_fn,
+			    (unsigned long)sci);
+	return count;
+}
+
+static DEVICE_ATTR(rx_fifo_timeout, 0644, rx_fifo_timeout_show, rx_fifo_timeout_store);
+
+
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 static void sci_dma_tx_complete(void *arg)
 {
@@ -2886,6 +2946,15 @@  static int sci_remove(struct platform_device *dev)
 
 	sci_cleanup_single(port);
 
+	if (port->port.fifosize > 1) {
+		sysfs_remove_file(&dev->dev.kobj,
+				  &dev_attr_rx_fifo_trigger.attr);
+	}
+	if (port->port.type == PORT_SCIFA || port->port.type == PORT_SCIFB) {
+		sysfs_remove_file(&dev->dev.kobj,
+				  &dev_attr_rx_fifo_timeout.attr);
+	}
+
 	return 0;
 }
 
@@ -3051,6 +3120,24 @@  static int sci_probe(struct platform_device *dev)
 	if (ret)
 		return ret;
 
+	if (sp->port.fifosize > 1) {
+		ret = sysfs_create_file(&dev->dev.kobj,
+				&dev_attr_rx_fifo_trigger.attr);
+		if (ret)
+			return ret;
+	}
+	if (sp->port.type == PORT_SCIFA || sp->port.type ==  PORT_SCIFB) {
+		ret = sysfs_create_file(&dev->dev.kobj,
+				&dev_attr_rx_fifo_timeout.attr);
+		if (ret) {
+			if (sp->port.fifosize > 1) {
+				sysfs_remove_file(&dev->dev.kobj,
+					&dev_attr_rx_fifo_trigger.attr);
+			}
+			return ret;
+		}
+	}
+
 #ifdef CONFIG_SH_STANDARD_BIOS
 	sh_bios_gdb_detach();
 #endif