[PATCH 0/5] tty: serial: uartlite: Disable changing fixed parameters

[PATCH 0/5] tty: serial: uartlite: Disable changing fixed parameters

[Sean Anderson]

The uartlite device is a "soft" device and certain parameters (such as
data bits, parity, and baud) are configured at synthesis time, and
cannot be discovered at runtime. Fortunately, bindings for this device
typically include some of these parameters (especially baud rate).
Instead of silently letting Linux's termios drift away from what the
hardware is actually doing, make the termios reflect the hardware, and
prevent them from being changed. With this series applied, the user
recieves an error message the first time they try and change these
termios:

    # stty parity
    [    7.221696] uartlite 84000000.serial: only 'n' parity supported
    [    7.222139] uartlite 84000000.serial: only 8 data bits supported
    stty: standard input: cannot perform all requested operations

In addition, the configured baud/parity/bits/etc. are exposed through
the standard termios ioctls, instead of using the default termios for
unconfigured ttys.


Sean Anderson (5):
  dt-bindings: serial: uartlite: Convert to json-schema
  dt-bindings: serial: uartlite: Add properties for synthesis-time
    parameters
  sh: j2: Update uartlite binding with data and parity properties
  tty: serial: uartlite: Initialize termios with fixed synthesis
    parameters
  tty: serial: uartlite: Prevent changing fixed parameters

 .../bindings/serial/xlnx,opb-uartlite.txt     |  23 ----
 .../bindings/serial/xlnx,opb-uartlite.yaml    |  92 ++++++++++++++
 arch/sh/boot/dts/j2_mimas_v2.dts              |   2 +
 drivers/tty/serial/uartlite.c                 | 118 ++++++++++++++++--
 4 files changed, 203 insertions(+), 32 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.txt
 create mode 100644 Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml

-- 
2.25.1

[PATCH 1/5] dt-bindings: serial: uartlite: Convert to json-schema

[Sean Anderson]

This converts the existing documentation for the uartlite binding to
json-schema.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 .../bindings/serial/xlnx,opb-uartlite.txt     | 23 --------
 .../bindings/serial/xlnx,opb-uartlite.yaml    | 53 +++++++++++++++++++
 2 files changed, 53 insertions(+), 23 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.txt
 create mode 100644 Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml

diff --git a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.txt b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.txt
deleted file mode 100644
index c37deb44dead..000000000000
--- a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.txt
+++ /dev/null
@@ -1,23 +0,0 @@
-Xilinx Axi Uartlite controller Device Tree Bindings
----------------------------------------------------------
-
-Required properties:
-- compatible		: Can be either of
-				"xlnx,xps-uartlite-1.00.a"
-				"xlnx,opb-uartlite-1.00.b"
-- reg			: Physical base address and size of the Axi Uartlite
-			  registers map.
-- interrupts		: Should contain the UART controller interrupt.
-
-Optional properties:
-- port-number		: Set Uart port number
-- clock-names		: Should be "s_axi_aclk"
-- clocks		: Input clock specifier. Refer to common clock bindings.
-
-Example:
-serial@800c0000 {
-	compatible = "xlnx,xps-uartlite-1.00.a";
-	reg = <0x0 0x800c0000 0x10000>;
-	interrupts = <0x0 0x6e 0x1>;
-	port-number = <0>;
-};
diff --git a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
new file mode 100644
index 000000000000..4ef29784ae97
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/xlnx,opb-uartlite.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Axi Uartlite
+
+maintainers:
+  - Peter Korsgaard <jacmet@sunsite.dk>
+
+properties:
+  compatible:
+    contains:
+      enum:
+        - xlnx,xps-uartlite-1.00.a
+        - xlnx,opb-uartlite-1.00.b
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  port-number:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Set Uart port number
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: s_axi_aclk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - $ref: /schemas/serial.yaml#
+
+additionalProperties: true
+
+examples:
+  - |
+      serial@800c0000 {
+        compatible = "xlnx,xps-uartlite-1.00.a";
+        reg = <0x800c0000 0x10000>;
+        interrupts = <0x0 0x6e 0x1>;
+        port-number = <0>;
+      };
+...
-- 
2.25.1

[PATCH 2/5] dt-bindings: serial: uartlite: Add properties for synthesis-time parameters

[Sean Anderson]

The uartlite device is a "soft" device. Many parameters, such as baud
rate, data bits, and the presence of a parity bit are configured before
synthesis and may not be changed (or discovered) at runtime. However, we
must know what these settings are in order to properly calculate the
uart timeout (and to inform the user about the actual baud of the uart).

These properties are present for out-of-tree bindings generated by
Xilinx's tools. However, they are also (mostly) present in in-tree
bindings. I chose current-speed over xlnx,baudrate primarily because it
seemed to be used by more existing bindings.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 .../bindings/serial/xlnx,opb-uartlite.yaml    | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
index 4ef29784ae97..28859e70e60f 100644
--- a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
+++ b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
@@ -32,13 +32,49 @@ properties:
   clock-names:
     const: s_axi_aclk
 
+  current-speed:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The fixed baud rate that the device was configured for.
+
+  xlnx,data-bits:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [5, 6, 7, 8]
+    default: 8
+    description:
+      The fixed number of data bits that the device was configured for.
+
+  xlnx,use-parity:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    default: 0
+    description:
+      Whether parity checking was enabled when the device was configured.
+
+  xlnx,odd-parity:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    description:
+      Whether odd parity was configured.
+
 required:
   - compatible
   - reg
   - interrupts
+  - current-speed
+  - xlnx,data-bits
+  - xlnx,use-parity
 
 allOf:
   - $ref: /schemas/serial.yaml#
+  - if:
+      properties:
+        xlnx,use-parity:
+          contains:
+            const: 1
+    then:
+      required:
+        - xlnx,odd-parity
 
 additionalProperties: true
 
@@ -49,5 +85,8 @@ examples:
         reg = <0x800c0000 0x10000>;
         interrupts = <0x0 0x6e 0x1>;
         port-number = <0>;
+        current-speed = <115200>;
+        xlnx,data-bits = <8>;
+        xlnx,use-parity = <0>;
       };
 ...
-- 
2.25.1

[PATCH 3/5] sh: j2: Update uartlite binding with data and parity properties

[Sean Anderson]

These properties are necessary for properly calculating the uart
timeout. I inspected the J2 source code, and believe these values to be
correct.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/sh/boot/dts/j2_mimas_v2.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/sh/boot/dts/j2_mimas_v2.dts b/arch/sh/boot/dts/j2_mimas_v2.dts
index 9f4742fab329..fa9562f78d53 100644
--- a/arch/sh/boot/dts/j2_mimas_v2.dts
+++ b/arch/sh/boot/dts/j2_mimas_v2.dts
@@ -88,6 +88,8 @@ uart0: serial@100 {
 			clock-frequency = <125000000>;
 			compatible = "xlnx,xps-uartlite-1.00.a";
 			current-speed = <19200>;
+			xlnx,use-parity = <0>;
+			xlnx,data-bits = <8>;
 			device_type = "serial";
 			interrupts = <0x12>;
 			port-number = <0>;
-- 
2.25.1

[PATCH 4/5] tty: serial: uartlite: Initialize termios with fixed synthesis parameters

[Sean Anderson]

This reads the various new devicetree parameters to discover how the
uart was configured when it was synthesized. Note that these properties
are fixed and undiscoverable. Once we have determined how the uart is
configured, we set the termios to let users know, and to initialize the
timeout to the correct value.

The defaults match ulite_console_setup. xlnx,use-parity,
xlnx,odd-parity, and xlnx,data-bits are optional since there were
in-tree users (and presumably out-of-tree users) who did not set them.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/tty/serial/uartlite.c | 66 +++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index f42ccc40ffa6..39c17ab206ca 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -60,9 +60,20 @@
 static struct uart_port *console_port;
 #endif
 
+/**
+ * struct uartlite_data: Driver private data
+ * reg_ops: Functions to read/write registers
+ * clk: Our parent clock, if present
+ * baud: The baud rate configured when this device was synthesized
+ * parity: The parity settings, like for uart_set_options()
+ * bits: The number of data bits
+ */
 struct uartlite_data {
 	const struct uartlite_reg_ops *reg_ops;
 	struct clk *clk;
+	int baud;
+	int parity;
+	int bits;
 };
 
 struct uartlite_reg_ops {
@@ -652,6 +663,9 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
 	port->type = PORT_UNKNOWN;
 	port->line = id;
 	port->private_data = pdata;
+	/* Initialize the termios to what was configured at synthesis-time */
+	uart_set_options(port, NULL, pdata->baud, pdata->parity, pdata->bits,
+			 'n');
 
 	dev_set_drvdata(dev, port);
 
@@ -756,18 +770,58 @@ static int ulite_probe(struct platform_device *pdev)
 	struct uartlite_data *pdata;
 	int irq, ret;
 	int id = pdev->id;
-#ifdef CONFIG_OF
-	const __be32 *prop;
 
-	prop = of_get_property(pdev->dev.of_node, "port-number", NULL);
-	if (prop)
-		id = be32_to_cpup(prop);
-#endif
 	pdata = devm_kzalloc(&pdev->dev, sizeof(struct uartlite_data),
 			     GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
+	if (IS_ENABLED(CONFIG_OF)) {
+		const char *prop;
+		struct device_node *np = pdev->dev.of_node;
+		u32 val;
+
+		prop = "port-number";
+		ret = of_property_read_u32(np, prop, &id);
+		if (ret && ret != -EINVAL)
+of_err:
+			return dev_err_probe(&pdev->dev, ret,
+					     "could not read %s\n", prop);
+
+		prop = "current-speed";
+		ret = of_property_read_u32(np, prop, &pdata->baud);
+		if (ret)
+			goto of_err;
+
+		prop = "xlnx,use-parity";
+		ret = of_property_read_u32(np, prop, &val);
+		if (ret && ret != -EINVAL)
+			goto of_err;
+
+		if (val) {
+			prop = "xlnx,odd-parity";
+			ret = of_property_read_u32(np, prop, &val);
+			if (ret)
+				goto of_err;
+
+			if (val)
+				pdata->parity = 'o';
+			else
+				pdata->parity = 'e';
+		} else {
+			pdata->parity = 'n';
+		}
+
+		prop = "xlnx,data-bits";
+		ret = of_property_read_u32(np, prop, &pdata->bits);
+		if (ret && ret != -EINVAL)
+			goto of_err;
+	} else {
+		pdata->baud = 9600;
+		pdata->parity = 'n';
+		pdata->bits = 8;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
-- 
2.25.1

[PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Sean Anderson]

This device does not support changing baud, parity, data bits, stop
bits, or detecting breaks. Disable "changing" these settings to prevent
their termios from diverging from the actual state of the uart. To inform
users of these limitations, warn if the new termios change these
parameters. We only do this once to avoid spamming the log. These
warnings are inspired by those in the sifive driver.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index 39c17ab206ca..0aed70039f46 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
 			      struct ktermios *old)
 {
 	unsigned long flags;
-	unsigned int baud;
+	struct uartlite_data *pdata = port->private_data;
+	tcflag_t old_cflag;
+
+	if (termios->c_iflag & BRKINT)
+		dev_err_once(port->dev, "BREAK detection not supported\n");
+	termios->c_iflag &= ~BRKINT;
+
+	if (termios->c_cflag & CSTOPB)
+		dev_err_once(port->dev, "only one stop bit supported\n");
+	termios->c_cflag &= ~CSTOPB;
+
+	old_cflag = termios->c_cflag;
+	termios->c_cflag &= ~(PARENB | PARODD);
+	if (pdata->parity == 'e')
+		termios->c_cflag |= PARENB;
+	else if (pdata->parity == 'o')
+		termios->c_cflag |= PARENB | PARODD;
+
+	if (termios->c_cflag != old_cflag)
+		dev_err_once(port->dev, "only '%c' parity supported\n",
+			     pdata->parity);
+
+	old_cflag = termios->c_cflag;
+	termios->c_cflag &= ~CSIZE;
+	switch (termios->c_cflag & CSIZE) {
+	case 5:
+		termios->c_cflag |= CS5;
+		break;
+	case 6:
+		termios->c_cflag |= CS6;
+		break;
+	case 7:
+		termios->c_cflag |= CS7;
+		break;
+	default:
+	case 8:
+		termios->c_cflag |= CS8;
+		break;
+	}
+	if (termios->c_cflag != old_cflag)
+		dev_err_once(port->dev, "only %d data bits supported\n",
+			     pdata->bits);
+
+	old_cflag = termios->c_cflag;
+	tty_termios_encode_baud_rate(termios, pdata->baud, pdata->baud);
+	if (termios->c_cflag != old_cflag)
+		dev_err_once(port->dev, "only %d baud supported\n",
+			     pdata->baud);
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -337,8 +384,7 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
 			| ULITE_STATUS_FRAME | ULITE_STATUS_OVERRUN;
 
 	/* update timeout */
-	baud = uart_get_baud_rate(port, termios, old, 0, 460800);
-	uart_update_timeout(port, termios->c_cflag, baud);
+	uart_update_timeout(port, termios->c_cflag, pdata->baud);
 
 	spin_unlock_irqrestore(&port->lock, flags);
 }
-- 
2.25.1

Re: [PATCH 2/5] dt-bindings: serial: uartlite: Add properties for synthesis-time parameters

[Michal Simek]

On 7/24/21 12:31 AM, Sean Anderson wrote:
> The uartlite device is a "soft" device. Many parameters, such as baud
> rate, data bits, and the presence of a parity bit are configured before
> synthesis and may not be changed (or discovered) at runtime. However, we
> must know what these settings are in order to properly calculate the
> uart timeout (and to inform the user about the actual baud of the uart).
> 
> These properties are present for out-of-tree bindings generated by
> Xilinx's tools. However, they are also (mostly) present in in-tree
> bindings. I chose current-speed over xlnx,baudrate primarily because it
> seemed to be used by more existing bindings.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  .../bindings/serial/xlnx,opb-uartlite.yaml    | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
> index 4ef29784ae97..28859e70e60f 100644
> --- a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
> +++ b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
> @@ -32,13 +32,49 @@ properties:
>    clock-names:
>      const: s_axi_aclk
>  
> +  current-speed:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The fixed baud rate that the device was configured for.
> +
> +  xlnx,data-bits:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [5, 6, 7, 8]
> +    default: 8
> +    description:
> +      The fixed number of data bits that the device was configured for.
> +
> +  xlnx,use-parity:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    default: 0
> +    description:
> +      Whether parity checking was enabled when the device was configured.
> +
> +  xlnx,odd-parity:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +    description:
> +      Whether odd parity was configured.
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
> +  - current-speed
> +  - xlnx,data-bits
> +  - xlnx,use-parity

I think all of these should be optional properties because were required
in past. Likely a lot of xilinx dt binding files have them but as it is
visible sh also has it without it.

M

>  
>  allOf:
>    - $ref: /schemas/serial.yaml#
> +  - if:
> +      properties:
> +        xlnx,use-parity:
> +          contains:
> +            const: 1
> +    then:
> +      required:
> +        - xlnx,odd-parity
>  
>  additionalProperties: true
>  
> @@ -49,5 +85,8 @@ examples:
>          reg = <0x800c0000 0x10000>;
>          interrupts = <0x0 0x6e 0x1>;
>          port-number = <0>;
> +        current-speed = <115200>;
> +        xlnx,data-bits = <8>;
> +        xlnx,use-parity = <0>;
>        };
>  ...
> 

Re: [PATCH 0/5] tty: serial: uartlite: Disable changing fixed parameters

[Michal Simek]

+Shubhrajyoti

On 7/24/21 12:31 AM, Sean Anderson wrote:
> The uartlite device is a "soft" device and certain parameters (such as
> data bits, parity, and baud) are configured at synthesis time, and
> cannot be discovered at runtime. Fortunately, bindings for this device
> typically include some of these parameters (especially baud rate).
> Instead of silently letting Linux's termios drift away from what the
> hardware is actually doing, make the termios reflect the hardware, and
> prevent them from being changed. With this series applied, the user
> recieves an error message the first time they try and change these
> termios:
> 
>     # stty parity
>     [    7.221696] uartlite 84000000.serial: only 'n' parity supported
>     [    7.222139] uartlite 84000000.serial: only 8 data bits supported
>     stty: standard input: cannot perform all requested operations
> 
> In addition, the configured baud/parity/bits/etc. are exposed through
> the standard termios ioctls, instead of using the default termios for
> unconfigured ttys.
> 
> 
> Sean Anderson (5):
>   dt-bindings: serial: uartlite: Convert to json-schema
>   dt-bindings: serial: uartlite: Add properties for synthesis-time
>     parameters
>   sh: j2: Update uartlite binding with data and parity properties
>   tty: serial: uartlite: Initialize termios with fixed synthesis
>     parameters
>   tty: serial: uartlite: Prevent changing fixed parameters
> 
>  .../bindings/serial/xlnx,opb-uartlite.txt     |  23 ----
>  .../bindings/serial/xlnx,opb-uartlite.yaml    |  92 ++++++++++++++
>  arch/sh/boot/dts/j2_mimas_v2.dts              |   2 +
>  drivers/tty/serial/uartlite.c                 | 118 ++++++++++++++++--
>  4 files changed, 203 insertions(+), 32 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.txt
>  create mode 100644 Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
> 

Re: [PATCH 2/5] dt-bindings: serial: uartlite: Add properties for synthesis-time parameters

[Sean Anderson]

On 7/26/21 8:30 AM, Michal Simek wrote:
 >
 >
 > On 7/24/21 12:31 AM, Sean Anderson wrote:
 >> The uartlite device is a "soft" device. Many parameters, such as baud
 >> rate, data bits, and the presence of a parity bit are configured before
 >> synthesis and may not be changed (or discovered) at runtime. However, we
 >> must know what these settings are in order to properly calculate the
 >> uart timeout (and to inform the user about the actual baud of the uart).
 >>
 >> These properties are present for out-of-tree bindings generated by
 >> Xilinx's tools. However, they are also (mostly) present in in-tree
 >> bindings. I chose current-speed over xlnx,baudrate primarily because it
 >> seemed to be used by more existing bindings.
 >>
 >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> ---
 >>
 >>  .../bindings/serial/xlnx,opb-uartlite.yaml    | 39 +++++++++++++++++++
 >>  1 file changed, 39 insertions(+)
 >>
 >> diff --git a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
 >> index 4ef29784ae97..28859e70e60f 100644
 >> --- a/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
 >> +++ b/Documentation/devicetree/bindings/serial/xlnx,opb-uartlite.yaml
 >> @@ -32,13 +32,49 @@ properties:
 >>    clock-names:
 >>      const: s_axi_aclk
 >>
 >> +  current-speed:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    description:
 >> +      The fixed baud rate that the device was configured for.
 >> +
 >> +  xlnx,data-bits:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [5, 6, 7, 8]
 >> +    default: 8
 >> +    description:
 >> +      The fixed number of data bits that the device was configured for.
 >> +
 >> +  xlnx,use-parity:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [0, 1]
 >> +    default: 0
 >> +    description:
 >> +      Whether parity checking was enabled when the device was configured.
 >> +
 >> +  xlnx,odd-parity:
 >> +    $ref: /schemas/types.yaml#/definitions/uint32
 >> +    enum: [0, 1]
 >> +    description:
 >> +      Whether odd parity was configured.
 >> +
 >>  required:
 >>    - compatible
 >>    - reg
 >>    - interrupts
 >> +  - current-speed
 >> +  - xlnx,data-bits
 >> +  - xlnx,use-parity
 >
 > I think all of these should be optional properties because were required
 > in past. Likely a lot of xilinx dt binding files have them but as it is
 > visible sh also has it without it.

As I understand it, the "required" here covers both properties which the
driver cannot function without and properties which should be set for all
new bindings. That is, the 'required" here is perscriptive. The worst
that happens is that the bindings author gets a warning when doing
dtbs_check, which is exactly what we want to happen.

In the driver itself, only "current-speed" is required. However, I could
make that optional as well (although perhaps with a warning). I feel
much less comfortable guessing the default baud rate than guessing 8n1.

--Sean

 >
 > M
 >
 >>
 >>  allOf:
 >>    - $ref: /schemas/serial.yaml#
 >> +  - if:
 >> +      properties:
 >> +        xlnx,use-parity:
 >> +          contains:
 >> +            const: 1
 >> +    then:
 >> +      required:
 >> +        - xlnx,odd-parity
 >>
 >>  additionalProperties: true
 >>
 >> @@ -49,5 +85,8 @@ examples:
 >>          reg = <0x800c0000 0x10000>;
 >>          interrupts = <0x0 0x6e 0x1>;
 >>          port-number = <0>;
 >> +        current-speed = <115200>;
 >> +        xlnx,data-bits = <8>;
 >> +        xlnx,use-parity = <0>;
 >>        };
 >>  ...
 >>
 >

Re: [PATCH 4/5] tty: serial: uartlite: Initialize termios with fixed synthesis parameters

[Sean Anderson]

On 7/23/21 6:31 PM, Sean Anderson wrote:
> This reads the various new devicetree parameters to discover how the
> uart was configured when it was synthesized. Note that these properties
> are fixed and undiscoverable. Once we have determined how the uart is
> configured, we set the termios to let users know, and to initialize the
> timeout to the correct value.
>
> The defaults match ulite_console_setup. xlnx,use-parity,
> xlnx,odd-parity, and xlnx,data-bits are optional since there were
> in-tree users (and presumably out-of-tree users) who did not set them.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>   drivers/tty/serial/uartlite.c | 66 +++++++++++++++++++++++++++++++----
>   1 file changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index f42ccc40ffa6..39c17ab206ca 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -60,9 +60,20 @@
>   static struct uart_port *console_port;
>   #endif
>
> +/**
> + * struct uartlite_data: Driver private data
> + * reg_ops: Functions to read/write registers
> + * clk: Our parent clock, if present
> + * baud: The baud rate configured when this device was synthesized
> + * parity: The parity settings, like for uart_set_options()
> + * bits: The number of data bits
> + */
>   struct uartlite_data {
>   	const struct uartlite_reg_ops *reg_ops;
>   	struct clk *clk;
> +	int baud;
> +	int parity;
> +	int bits;
>   };
>
>   struct uartlite_reg_ops {
> @@ -652,6 +663,9 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
>   	port->type = PORT_UNKNOWN;
>   	port->line = id;
>   	port->private_data = pdata;
> +	/* Initialize the termios to what was configured at synthesis-time */
> +	uart_set_options(port, NULL, pdata->baud, pdata->parity, pdata->bits,
> +			 'n');

I did some testing today, and discovered that the termios are not set
properly. I think I missed this the first time around because on
Microblaze QEMU this UART is the console, and the baud etc. gets set
properly because of stdout-path (or bootargs). However, uart_set_options
doesn't actually do anything with the termios when co is NULL.

The initial termios are set up by tty_init_termios (called from
tty_init_dev). They come from either tty->driver->init_termios, or from
tty->driver->termios[idx]. There is only one init_termios per-driver,
so we would need to have multiple drivers if we wanted to have multiple
UARTs with different (e.g.) bauds.

The indexed termios are designed to keep the settings from the previous
time the tty was opened. So I think (ab)using them is not too terrible,
especially since we will only set them once. Unfortunately, we cannot
use tty_save_termios to initialize the indexed termio, since the tty is
not set until tty_port_open, which is called after tty_init_dev.

Based on this, I think the neatest cut would be something like

/* perhaps just do this in ulite_assign? */
ulite_request_port () {
	/* ... */
	termios = &ulite_uart_driver.tty_driver->termios[port->line];
	termios = kzalloc(sizeof(*termios));
	if (!termios)
		/* ... */
	termios.c_cflags = /* ... */
	/* etc */
}

Unfortunately, according to include/linux/serial_core.h, tty_driver is
not supposed to be touched by the low-level driver. But I think we have
a bit of an unusual case here with a device that can't change baud. If
anyone has other suggestions, I'm all for them.

--Sean

>
>   	dev_set_drvdata(dev, port);
>
> @@ -756,18 +770,58 @@ static int ulite_probe(struct platform_device *pdev)
>   	struct uartlite_data *pdata;
>   	int irq, ret;
>   	int id = pdev->id;
> -#ifdef CONFIG_OF
> -	const __be32 *prop;
>
> -	prop = of_get_property(pdev->dev.of_node, "port-number", NULL);
> -	if (prop)
> -		id = be32_to_cpup(prop);
> -#endif
>   	pdata = devm_kzalloc(&pdev->dev, sizeof(struct uartlite_data),
>   			     GFP_KERNEL);
>   	if (!pdata)
>   		return -ENOMEM;
>
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		const char *prop;
> +		struct device_node *np = pdev->dev.of_node;
> +		u32 val;
> +
> +		prop = "port-number";
> +		ret = of_property_read_u32(np, prop, &id);
> +		if (ret && ret != -EINVAL)
> +of_err:
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "could not read %s\n", prop);
> +
> +		prop = "current-speed";
> +		ret = of_property_read_u32(np, prop, &pdata->baud);
> +		if (ret)
> +			goto of_err;
> +
> +		prop = "xlnx,use-parity";
> +		ret = of_property_read_u32(np, prop, &val);
> +		if (ret && ret != -EINVAL)
> +			goto of_err;
> +
> +		if (val) {
> +			prop = "xlnx,odd-parity";
> +			ret = of_property_read_u32(np, prop, &val);
> +			if (ret)
> +				goto of_err;
> +
> +			if (val)
> +				pdata->parity = 'o';
> +			else
> +				pdata->parity = 'e';
> +		} else {
> +			pdata->parity = 'n';
> +		}
> +
> +		prop = "xlnx,data-bits";
> +		ret = of_property_read_u32(np, prop, &pdata->bits);
> +		if (ret && ret != -EINVAL)
> +			goto of_err;
> +	} else {
> +		pdata->baud = 9600;
> +		pdata->parity = 'n';
> +		pdata->bits = 8;
> +	}
> +
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	if (!res)
>   		return -ENODEV;
>

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Greg Kroah-Hartman]

On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
> This device does not support changing baud, parity, data bits, stop
> bits, or detecting breaks. Disable "changing" these settings to prevent
> their termios from diverging from the actual state of the uart. To inform
> users of these limitations, warn if the new termios change these
> parameters. We only do this once to avoid spamming the log. These
> warnings are inspired by those in the sifive driver.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index 39c17ab206ca..0aed70039f46 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
>  			      struct ktermios *old)
>  {
>  	unsigned long flags;
> -	unsigned int baud;
> +	struct uartlite_data *pdata = port->private_data;
> +	tcflag_t old_cflag;
> +
> +	if (termios->c_iflag & BRKINT)
> +		dev_err_once(port->dev, "BREAK detection not supported\n");
> +	termios->c_iflag &= ~BRKINT;
> +
> +	if (termios->c_cflag & CSTOPB)
> +		dev_err_once(port->dev, "only one stop bit supported\n");
> +	termios->c_cflag &= ~CSTOPB;
> +
> +	old_cflag = termios->c_cflag;
> +	termios->c_cflag &= ~(PARENB | PARODD);
> +	if (pdata->parity == 'e')
> +		termios->c_cflag |= PARENB;
> +	else if (pdata->parity == 'o')
> +		termios->c_cflag |= PARENB | PARODD;
> +
> +	if (termios->c_cflag != old_cflag)
> +		dev_err_once(port->dev, "only '%c' parity supported\n",
> +			     pdata->parity);

Through all of this, you are warning that nothing is supported, yet you
are continuing on as if all of this worked just fine.

That feels odd, why is this needed at all?  If you really do not support
any changes here, why even fake it?

thanks,

greg k-h

Re: [PATCH 4/5] tty: serial: uartlite: Initialize termios with fixed synthesis parameters

[Greg Kroah-Hartman]

On Mon, Jul 26, 2021 at 04:53:39PM -0400, Sean Anderson wrote:
> 
> 
> On 7/23/21 6:31 PM, Sean Anderson wrote:
> > This reads the various new devicetree parameters to discover how the
> > uart was configured when it was synthesized. Note that these properties
> > are fixed and undiscoverable. Once we have determined how the uart is
> > configured, we set the termios to let users know, and to initialize the
> > timeout to the correct value.
> > 
> > The defaults match ulite_console_setup. xlnx,use-parity,
> > xlnx,odd-parity, and xlnx,data-bits are optional since there were
> > in-tree users (and presumably out-of-tree users) who did not set them.
> > 
> > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> > ---
> > 
> >   drivers/tty/serial/uartlite.c | 66 +++++++++++++++++++++++++++++++----
> >   1 file changed, 60 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> > index f42ccc40ffa6..39c17ab206ca 100644
> > --- a/drivers/tty/serial/uartlite.c
> > +++ b/drivers/tty/serial/uartlite.c
> > @@ -60,9 +60,20 @@
> >   static struct uart_port *console_port;
> >   #endif
> > 
> > +/**
> > + * struct uartlite_data: Driver private data
> > + * reg_ops: Functions to read/write registers
> > + * clk: Our parent clock, if present
> > + * baud: The baud rate configured when this device was synthesized
> > + * parity: The parity settings, like for uart_set_options()
> > + * bits: The number of data bits
> > + */
> >   struct uartlite_data {
> >   	const struct uartlite_reg_ops *reg_ops;
> >   	struct clk *clk;
> > +	int baud;
> > +	int parity;
> > +	int bits;
> >   };
> > 
> >   struct uartlite_reg_ops {
> > @@ -652,6 +663,9 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
> >   	port->type = PORT_UNKNOWN;
> >   	port->line = id;
> >   	port->private_data = pdata;
> > +	/* Initialize the termios to what was configured at synthesis-time */
> > +	uart_set_options(port, NULL, pdata->baud, pdata->parity, pdata->bits,
> > +			 'n');
> 
> I did some testing today, and discovered that the termios are not set
> properly. I think I missed this the first time around because on
> Microblaze QEMU this UART is the console, and the baud etc. gets set
> properly because of stdout-path (or bootargs). However, uart_set_options
> doesn't actually do anything with the termios when co is NULL.
> 
> The initial termios are set up by tty_init_termios (called from
> tty_init_dev). They come from either tty->driver->init_termios, or from
> tty->driver->termios[idx]. There is only one init_termios per-driver,
> so we would need to have multiple drivers if we wanted to have multiple
> UARTs with different (e.g.) bauds.
> 
> The indexed termios are designed to keep the settings from the previous
> time the tty was opened. So I think (ab)using them is not too terrible,
> especially since we will only set them once. Unfortunately, we cannot
> use tty_save_termios to initialize the indexed termio, since the tty is
> not set until tty_port_open, which is called after tty_init_dev.
> 
> Based on this, I think the neatest cut would be something like
> 
> /* perhaps just do this in ulite_assign? */
> ulite_request_port () {
> 	/* ... */
> 	termios = &ulite_uart_driver.tty_driver->termios[port->line];
> 	termios = kzalloc(sizeof(*termios));
> 	if (!termios)
> 		/* ... */
> 	termios.c_cflags = /* ... */
> 	/* etc */
> }
> 
> Unfortunately, according to include/linux/serial_core.h, tty_driver is
> not supposed to be touched by the low-level driver. But I think we have
> a bit of an unusual case here with a device that can't change baud. If
> anyone has other suggestions, I'm all for them.

If a driver can not support changes in baud rates, then just ignore all
changes to baud rates as there will not be an issue for anyone :)

thanks,

greg k-h

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Sean Anderson]

On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote:
> On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
>> This device does not support changing baud, parity, data bits, stop
>> bits, or detecting breaks. Disable "changing" these settings to prevent
>> their termios from diverging from the actual state of the uart. To inform
>> users of these limitations, warn if the new termios change these
>> parameters. We only do this once to avoid spamming the log. These
>> warnings are inspired by those in the sifive driver.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>>  drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
>> index 39c17ab206ca..0aed70039f46 100644
>> --- a/drivers/tty/serial/uartlite.c
>> +++ b/drivers/tty/serial/uartlite.c
>> @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
>>  			      struct ktermios *old)
>>  {
>>  	unsigned long flags;
>> -	unsigned int baud;
>> +	struct uartlite_data *pdata = port->private_data;
>> +	tcflag_t old_cflag;
>> +
>> +	if (termios->c_iflag & BRKINT)
>> +		dev_err_once(port->dev, "BREAK detection not supported\n");
>> +	termios->c_iflag &= ~BRKINT;
>> +
>> +	if (termios->c_cflag & CSTOPB)
>> +		dev_err_once(port->dev, "only one stop bit supported\n");
>> +	termios->c_cflag &= ~CSTOPB;
>> +
>> +	old_cflag = termios->c_cflag;
>> +	termios->c_cflag &= ~(PARENB | PARODD);
>> +	if (pdata->parity == 'e')
>> +		termios->c_cflag |= PARENB;
>> +	else if (pdata->parity == 'o')
>> +		termios->c_cflag |= PARENB | PARODD;
>> +
>> +	if (termios->c_cflag != old_cflag)
>> +		dev_err_once(port->dev, "only '%c' parity supported\n",
>> +			     pdata->parity);
>
> Through all of this, you are warning that nothing is supported, yet you
> are continuing on as if all of this worked just fine.

We don't. The idea is that we see if (e.g.) CSIZE is something the
hardware can't produce, warn about it (once), and then set it to what we
can support. That way the user can (programmatically) detect if this
device can support their use-case. So e.g. if you you have a serial bus
or something, the driver can find out that (e.g.) the UART has the wrong
CSIZE, and it can fail to probe. Before this series, it would continue
along as if nothing was wrong, and the user then has to debug why their
device does not work as expected.

--Sean

>
> That feels odd, why is this needed at all?  If you really do not support
> any changes here, why even fake it?
>
> thanks,
>
> greg k-h
>

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Greg Kroah-Hartman]

On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote:
> 
> 
> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote:
> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
> > > This device does not support changing baud, parity, data bits, stop
> > > bits, or detecting breaks. Disable "changing" these settings to prevent
> > > their termios from diverging from the actual state of the uart. To inform
> > > users of these limitations, warn if the new termios change these
> > > parameters. We only do this once to avoid spamming the log. These
> > > warnings are inspired by those in the sifive driver.
> > > 
> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> > > ---
> > > 
> > >  drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
> > >  1 file changed, 49 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> > > index 39c17ab206ca..0aed70039f46 100644
> > > --- a/drivers/tty/serial/uartlite.c
> > > +++ b/drivers/tty/serial/uartlite.c
> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
> > >  			      struct ktermios *old)
> > >  {
> > >  	unsigned long flags;
> > > -	unsigned int baud;
> > > +	struct uartlite_data *pdata = port->private_data;
> > > +	tcflag_t old_cflag;
> > > +
> > > +	if (termios->c_iflag & BRKINT)
> > > +		dev_err_once(port->dev, "BREAK detection not supported\n");
> > > +	termios->c_iflag &= ~BRKINT;
> > > +
> > > +	if (termios->c_cflag & CSTOPB)
> > > +		dev_err_once(port->dev, "only one stop bit supported\n");
> > > +	termios->c_cflag &= ~CSTOPB;
> > > +
> > > +	old_cflag = termios->c_cflag;
> > > +	termios->c_cflag &= ~(PARENB | PARODD);
> > > +	if (pdata->parity == 'e')
> > > +		termios->c_cflag |= PARENB;
> > > +	else if (pdata->parity == 'o')
> > > +		termios->c_cflag |= PARENB | PARODD;
> > > +
> > > +	if (termios->c_cflag != old_cflag)
> > > +		dev_err_once(port->dev, "only '%c' parity supported\n",
> > > +			     pdata->parity);
> > 
> > Through all of this, you are warning that nothing is supported, yet you
> > are continuing on as if all of this worked just fine.
> 
> We don't. The idea is that we see if (e.g.) CSIZE is something the
> hardware can't produce, warn about it (once), and then set it to what we
> can support.

So you are ignoring what the user wanted, and doing whatever you wanted.

As you can only support one setting, why even care?  Just set it to what
you want and ignore userspace's requests.  Of course that is a pain but
no one is going to notice kernel log messages either, right?

> That way the user can (programmatically) detect if this
> device can support their use-case.

How will a user program read the kernel error log for this?

> So e.g. if you you have a serial bus
> or something, the driver can find out that (e.g.) the UART has the wrong
> CSIZE, and it can fail to probe.

What will fail to probe?  Where?

> Before this series, it would continue along as if nothing was wrong,
> and the user then has to debug why their device does not work as
> expected.

Why not fix your broken uart?  :)

thanks,

greg k-h

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Sean Anderson]

On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote:
 > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote:
 >>
 >>
 >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote:
 >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
 >> > > This device does not support changing baud, parity, data bits, stop
 >> > > bits, or detecting breaks. Disable "changing" these settings to prevent
 >> > > their termios from diverging from the actual state of the uart. To inform
 >> > > users of these limitations, warn if the new termios change these
 >> > > parameters. We only do this once to avoid spamming the log. These
 >> > > warnings are inspired by those in the sifive driver.
 >> > >
 >> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
 >> > > ---
 >> > >
 >> > >  drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
 >> > >  1 file changed, 49 insertions(+), 3 deletions(-)
 >> > >
 >> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
 >> > > index 39c17ab206ca..0aed70039f46 100644
 >> > > --- a/drivers/tty/serial/uartlite.c
 >> > > +++ b/drivers/tty/serial/uartlite.c
 >> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
 >> > >  			      struct ktermios *old)
 >> > >  {
 >> > >  	unsigned long flags;
 >> > > -	unsigned int baud;
 >> > > +	struct uartlite_data *pdata = port->private_data;
 >> > > +	tcflag_t old_cflag;
 >> > > +
 >> > > +	if (termios->c_iflag & BRKINT)
 >> > > +		dev_err_once(port->dev, "BREAK detection not supported\n");
 >> > > +	termios->c_iflag &= ~BRKINT;
 >> > > +
 >> > > +	if (termios->c_cflag & CSTOPB)
 >> > > +		dev_err_once(port->dev, "only one stop bit supported\n");
 >> > > +	termios->c_cflag &= ~CSTOPB;
 >> > > +
 >> > > +	old_cflag = termios->c_cflag;
 >> > > +	termios->c_cflag &= ~(PARENB | PARODD);
 >> > > +	if (pdata->parity == 'e')
 >> > > +		termios->c_cflag |= PARENB;
 >> > > +	else if (pdata->parity == 'o')
 >> > > +		termios->c_cflag |= PARENB | PARODD;
 >> > > +
 >> > > +	if (termios->c_cflag != old_cflag)
 >> > > +		dev_err_once(port->dev, "only '%c' parity supported\n",
 >> > > +			     pdata->parity);
 >> >
 >> > Through all of this, you are warning that nothing is supported, yet you
 >> > are continuing on as if all of this worked just fine.
 >>
 >> We don't. The idea is that we see if (e.g.) CSIZE is something the
 >> hardware can't produce, warn about it (once), and then set it to what we
 >> can support.
 >
 > So you are ignoring what the user wanted, and doing whatever you wanted.
 >
 > As you can only support one setting, why even care?  Just set it to what
 > you want and ignore userspace's requests.

That is exactly what we are doing. We set it to what we can support and
ignore what userspace requested.

 > Of course that is a pain but
 > no one is going to notice kernel log messages either, right?

*shrug* Why does sifive_serial_set_termios do it?

 >
 >> That way the user can (programmatically) detect if this
 >> device can support their use-case.
 >
 > How will a user program read the kernel error log for this?

The idea is that after calling tcsetattr, you call tcgetattr to see
which of your termios got applied. Then you can handle that however you
want. This is what (e.g.) stty does:

     # stty parity
     [    7.221696] uartlite 84000000.serial: only 'n' parity supported
     [    7.222139] uartlite 84000000.serial: only 8 data bits supported
     stty: standard input: cannot perform all requested operations

The dmesg lines are just to help out whoever is trying to figure out why
their stty command failed.

 >
 >> So e.g. if you you have a serial bus
 >> or something, the driver can find out that (e.g.) the UART has the wrong
 >> CSIZE, and it can fail to probe.
 >
 > What will fail to probe?  Where?

The serial bus device. Though, it looks like things such as
ttyport_set_baudrate/serdev_device_set_baudrate do not check the
termios after setting them, so in-kernel drivers don't handle this as
well as userspace can.

 >> Before this series, it would continue along as if nothing was wrong,
 >> and the user then has to debug why their device does not work as
 >> expected.
 >
 > Why not fix your broken uart?  :)

I believe this is a feature, designed to reduce the amount of FPGA
resources :)

--Sean

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Greg Kroah-Hartman]

On Thu, Jul 29, 2021 at 11:43:08AM -0400, Sean Anderson wrote:
> 
> 
> On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote:
> > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote:
> >>
> >>
> >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote:
> >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
> >> > > This device does not support changing baud, parity, data bits, stop
> >> > > bits, or detecting breaks. Disable "changing" these settings to prevent
> >> > > their termios from diverging from the actual state of the uart. To inform
> >> > > users of these limitations, warn if the new termios change these
> >> > > parameters. We only do this once to avoid spamming the log. These
> >> > > warnings are inspired by those in the sifive driver.
> >> > >
> >> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> > > ---
> >> > >
> >> > >  drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
> >> > >  1 file changed, 49 insertions(+), 3 deletions(-)
> >> > >
> >> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> >> > > index 39c17ab206ca..0aed70039f46 100644
> >> > > --- a/drivers/tty/serial/uartlite.c
> >> > > +++ b/drivers/tty/serial/uartlite.c
> >> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
> >> > >  			      struct ktermios *old)
> >> > >  {
> >> > >  	unsigned long flags;
> >> > > -	unsigned int baud;
> >> > > +	struct uartlite_data *pdata = port->private_data;
> >> > > +	tcflag_t old_cflag;
> >> > > +
> >> > > +	if (termios->c_iflag & BRKINT)
> >> > > +		dev_err_once(port->dev, "BREAK detection not supported\n");
> >> > > +	termios->c_iflag &= ~BRKINT;
> >> > > +
> >> > > +	if (termios->c_cflag & CSTOPB)
> >> > > +		dev_err_once(port->dev, "only one stop bit supported\n");
> >> > > +	termios->c_cflag &= ~CSTOPB;
> >> > > +
> >> > > +	old_cflag = termios->c_cflag;
> >> > > +	termios->c_cflag &= ~(PARENB | PARODD);
> >> > > +	if (pdata->parity == 'e')
> >> > > +		termios->c_cflag |= PARENB;
> >> > > +	else if (pdata->parity == 'o')
> >> > > +		termios->c_cflag |= PARENB | PARODD;
> >> > > +
> >> > > +	if (termios->c_cflag != old_cflag)
> >> > > +		dev_err_once(port->dev, "only '%c' parity supported\n",
> >> > > +			     pdata->parity);
> >> >
> >> > Through all of this, you are warning that nothing is supported, yet you
> >> > are continuing on as if all of this worked just fine.
> >>
> >> We don't. The idea is that we see if (e.g.) CSIZE is something the
> >> hardware can't produce, warn about it (once), and then set it to what we
> >> can support.
> >
> > So you are ignoring what the user wanted, and doing whatever you wanted.
> >
> > As you can only support one setting, why even care?  Just set it to what
> > you want and ignore userspace's requests.
> 
> That is exactly what we are doing. We set it to what we can support and
> ignore what userspace requested.

If you can only support one set of options, just set it and always fail
the tcsetattr call which will allow userspace to know it shouldn't have
tried to do that.

> > Of course that is a pain but
> > no one is going to notice kernel log messages either, right?
> 
> *shrug* Why does sifive_serial_set_termios do it?

I didn't notice it during the review process.  Doesn't mean you should
copy a bad example :)

thanks,

greg k-h

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Sean Anderson]

On 7/30/21 1:25 AM, Greg Kroah-Hartman wrote:
> On Thu, Jul 29, 2021 at 11:43:08AM -0400, Sean Anderson wrote:
>>
>>
>> On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote:
>> > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote:
>> >>
>> >>
>> >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote:
>> >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
>> >> > > This device does not support changing baud, parity, data bits, stop
>> >> > > bits, or detecting breaks. Disable "changing" these settings to prevent
>> >> > > their termios from diverging from the actual state of the uart. To inform
>> >> > > users of these limitations, warn if the new termios change these
>> >> > > parameters. We only do this once to avoid spamming the log. These
>> >> > > warnings are inspired by those in the sifive driver.
>> >> > >
>> >> > > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> >> > > ---
>> >> > >
>> >> > >  drivers/tty/serial/uartlite.c | 52 +++++++++++++++++++++++++++++++++--
>> >> > >  1 file changed, 49 insertions(+), 3 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
>> >> > > index 39c17ab206ca..0aed70039f46 100644
>> >> > > --- a/drivers/tty/serial/uartlite.c
>> >> > > +++ b/drivers/tty/serial/uartlite.c
>> >> > > @@ -314,7 +314,54 @@ static void ulite_set_termios(struct uart_port *port, struct ktermios *termios,
>> >> > >  			      struct ktermios *old)
>> >> > >  {
>> >> > >  	unsigned long flags;
>> >> > > -	unsigned int baud;
>> >> > > +	struct uartlite_data *pdata = port->private_data;
>> >> > > +	tcflag_t old_cflag;
>> >> > > +
>> >> > > +	if (termios->c_iflag & BRKINT)
>> >> > > +		dev_err_once(port->dev, "BREAK detection not supported\n");
>> >> > > +	termios->c_iflag &= ~BRKINT;
>> >> > > +
>> >> > > +	if (termios->c_cflag & CSTOPB)
>> >> > > +		dev_err_once(port->dev, "only one stop bit supported\n");
>> >> > > +	termios->c_cflag &= ~CSTOPB;
>> >> > > +
>> >> > > +	old_cflag = termios->c_cflag;
>> >> > > +	termios->c_cflag &= ~(PARENB | PARODD);
>> >> > > +	if (pdata->parity == 'e')
>> >> > > +		termios->c_cflag |= PARENB;
>> >> > > +	else if (pdata->parity == 'o')
>> >> > > +		termios->c_cflag |= PARENB | PARODD;
>> >> > > +
>> >> > > +	if (termios->c_cflag != old_cflag)
>> >> > > +		dev_err_once(port->dev, "only '%c' parity supported\n",
>> >> > > +			     pdata->parity);
>> >> >
>> >> > Through all of this, you are warning that nothing is supported, yet you
>> >> > are continuing on as if all of this worked just fine.
>> >>
>> >> We don't. The idea is that we see if (e.g.) CSIZE is something the
>> >> hardware can't produce, warn about it (once), and then set it to what we
>> >> can support.
>> >
>> > So you are ignoring what the user wanted, and doing whatever you wanted.
>> >
>> > As you can only support one setting, why even care?  Just set it to what
>> > you want and ignore userspace's requests.
>>
>> That is exactly what we are doing. We set it to what we can support and
>> ignore what userspace requested.
>
> If you can only support one set of options, just set it and always fail
> the tcsetattr call which will allow userspace to know it shouldn't have
> tried to do that.

We can't. set_termios returns void. All we can do is set the termios to
what they should be.

>
>> > Of course that is a pain but
>> > no one is going to notice kernel log messages either, right?
>>
>> *shrug* Why does sifive_serial_set_termios do it?
>
> I didn't notice it during the review process.  Doesn't mean you should
> copy a bad example :)

I didn't know it was a bad example :)

Generally, I presume that any recently-added drivers are using the best
practices for their subsystems. If they weren't, I'd have expected them
to have been rejected during review. Of course, that doesn't always
happen (such as in this case), but in the absence of documentation, code
is the next best source of how things should be.

In any case, I am not particularly attached to these particular
warnings, as long as the termios get set to what we support.

--Sean

Re: [PATCH 5/5] tty: serial: uartlite: Prevent changing fixed parameters

[Maarten Brock]

On 2021-07-30 07:25, Greg Kroah-Hartman wrote:
> On Thu, Jul 29, 2021 at 11:43:08AM -0400, Sean Anderson wrote:
>> 
>> 
>> On 7/29/21 11:32 AM, Greg Kroah-Hartman wrote:
>> > On Thu, Jul 29, 2021 at 11:26:59AM -0400, Sean Anderson wrote:
>> >>
>> >>
>> >> On 7/29/21 11:01 AM, Greg Kroah-Hartman wrote:
>> >> > On Fri, Jul 23, 2021 at 06:31:51PM -0400, Sean Anderson wrote:
>> >> > Through all of this, you are warning that nothing is supported, yet you
>> >> > are continuing on as if all of this worked just fine.
>> >>
>> >> We don't. The idea is that we see if (e.g.) CSIZE is something the
>> >> hardware can't produce, warn about it (once), and then set it to what we
>> >> can support.
>> >
>> > So you are ignoring what the user wanted, and doing whatever you wanted.
>> >
>> > As you can only support one setting, why even care?  Just set it to what
>> > you want and ignore userspace's requests.
>> 
>> That is exactly what we are doing. We set it to what we can support 
>> and
>> ignore what userspace requested.
> 
> If you can only support one set of options, just set it and always fail
> the tcsetattr call which will allow userspace to know it shouldn't have
> tried to do that.

I have to disagree strongly here against *always*. If the user calls 
tcsetattr
to set it to exactly what is supported it *should not fail*. Every 
decent
(terminal) program will start by setting or getting the initial 
settings.

Maarten