serial: mvebu-uart: Support for higher baudrates

Hello Greg,

this is v9 of series adding support for higher baudrates on A37xx UART.

I've fixed the two things discovered in v8 (commit message typo and order of last two patches).

Marek

Changes since v8:
* fixed typo in commit message of patch 3
* switched patches 5 and 6 as requested by Gregory Clement

Changes since v7:
* fixed lint errors in yaml binding file
* added Reviewed-by tags
* changed commit messages and comments a little
* fixed indentation at some places
* swapped patch 2 and 3 (dt-binding defining new binding should go before the driver adding usage of that new binding)

Changes in v6:
* fixed yaml binding file and dts files

Changes in v5:
* fixed yaml binding file

Changes in v4:
* converted armada3700-uart-clock documentation to YAML
* split documentation changes into two commits:
- first which adds clock documentation
- second which updates UART documentation

Changes in v3:
v3 is rebased on top of Linus master branch and all already applied patches were dropped. There are no changes in patches itself since v2.

Pali Rohár (6):
math64: New DIV_U64_ROUND_CLOSEST helper
dt-bindings: mvebu-uart: document DT bindings for

    marvell,armada-3700-uart-clock

serial: mvebu-uart: implement UART clock driver for configuring UART
base clock
dt-bindings: mvebu-uart: update information about UART clock
serial: mvebu-uart: implement support for baudrates higher than 230400
Bd
arm64: dts: marvell: armada-37xx: add device node for UART clock and
use it

 .../clock/marvell,armada-3700-uart-clock.yaml |  59 ++
 .../devicetree/bindings/serial/mvebu-uart.txt |   9 +-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |  14 +-
 drivers/tty/serial/Kconfig                    |   1 +
 drivers/tty/serial/mvebu-uart.c               | 596 +++++++++++++++++-
 include/linux/math64.h                        |  13 +
 6 files changed, 671 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml

2.34.1

From: Pali Rohár <[email protected]>

Provide DIV_U64_ROUND_CLOSEST helper which uses div_u64 to perform division rounded to the closest integer using unsigned 64bit dividend and unsigned 32bit divisor.

---
 include/linux/math64.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 2928f03d6d46..a14f40de1dca 100644
@@ -300,6 +300,19 @@ u64 mul_u64_u64_div_u64(u64 a, u64 mul, u64 div);
 #define DIV64_U64_ROUND_CLOSEST(dividend, divisor)	\
 	({ u64 _tmp = (divisor); div64_u64((dividend) + _tmp / 2, _tmp); })
 
+/*
+ * DIV_U64_ROUND_CLOSEST - unsigned 64bit divide with 32bit divisor rounded to nearest integer
+ * @dividend: unsigned 64bit dividend
+ * @divisor: unsigned 32bit divisor
+ *
+ * Divide unsigned 64bit dividend by unsigned 32bit divisor
+ * and round to closest integer.
+ *
+ * Return: dividend / divisor rounded to nearest integer
+ */
+#define DIV_U64_ROUND_CLOSEST(dividend, divisor)	\
+	({ u32 _tmp = (divisor); div_u64((u64)(dividend) + _tmp / 2, _tmp); })
+
 /*
  * DIV_S64_ROUND_CLOSEST - signed 64bit divide with 32bit divisor rounded to nearest integer
  * @dividend: signed 64bit dividend

2.34.1

From: Pali Rohár <[email protected]>

Add DT bindings documentation for device nodes with compatible string "marvell,armada-3700-uart-clock".

---
 .../clock/marvell,armada-3700-uart-clock.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml

new file mode 100644
index 000000000000..175f5c8f2bc5
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Marvell Armada 3720 UART clocks
+
+maintainers:
+  - Pali Rohár <[email protected]>
+
+properties:
+  compatible:
+    const: marvell,armada-3700-uart-clock
+
+  reg:
+    items:
+      - description: UART Clock Control Register
+      - description: UART 2 Baud Rate Divisor Register
+
+  clocks:
+    description: |
+      List of parent clocks suitable for UART from following set:
+        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
+      UART clock can use one from this set and when more are provided
+      then kernel would choose and configure the most suitable one.
+      It is suggest to specify at least one TBG clock to achieve
+      baudrates above 230400 and also to specify clock which bootloader
+      used for UART (most probably xtal) for smooth boot log on UART.
+
+  clock-names:
+    items:
+      - const: TBG-A-P
+      - const: TBG-B-P
+      - const: TBG-A-S
+      - const: TBG-B-S
+      - const: xtal
+    minItems: 1
+
+  '#clock-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    uartclk: clock-controller@12010 {
+      compatible = "marvell,armada-3700-uart-clock";
+      reg = <0x12010 0x4>, <0x12210 0x4>;
+      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
+      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
+      #clock-cells = <1>;
+    };

2.34.1

Quoting Marek Behún (2022-02-19 07:28:14)

From: Pali Rohár <[email protected]>

Implement a new device driver for controlling UART clocks on Marvell Armada 3700 SoC. This device driver is loaded for devices which match the compatible string "marvell,armada-3700-uart-clock".

There are more pitfalls related to UART clocks:
- both UARTs use same parent clock source (which can be xtal or one of the TBG clocks),
- if a TBG clock is used as the parent clock, there are two additional divisors that can both be configured to divide the rate by 1, 2, ... 6, but these divisors are again shared between the two UART controllers on the SOC,
- the configuration of the parent clock source and divisors is done in the address space of the first UART controller, UART1. Clocks can be gated separately for UART1 and UART2, but this setting also lives in the address space of UART1,
- Marvell's Functional Specification for Armada 3720 document has the clock gating bits swapped, so the one described to gate UART1 clock actually gates UART2 and vice versa,
- each UART has it's own "special divisor", and this uses the parent clock described above. These divisors are configure in each UART's address space separately.

Thus the driver for UART2 controller needs to have access to UART1 address space, since UART1 address space contains some bits exclusive for UART2 and also some bits which are shared between UART1 and UART2.

Also, during boot, when early console is active on one of the UARTs, and we want to switch parent clock from xtal (default) to TBG (to be more flexible with baudrates), the driver changing UART clocks also needs to be able to change the "special divisor", so that the baudrate of earlycon is not changed when swtiching to normal console. Thus the clock driver also needs to be able to access UART2 register space, for UART2's "special divisor".

For these reasons, this new UART clock driver does not use ioremap_resource(), but only ioremap() to prevent resource conflicts between UART clock driver and UART driver.

We need to share only two 32-bit registers between the UART driver and the UART clock driver:
- UART Clock Control
- UART 2 Baud Rate Divisor
Access to these two registers are protected by one spinlock to prevent any conflicts. Access is required only during probing, when changing baudrate or during suspend/resume.

Hardware can be configured to use one of following clocks as UART parent
clock: TBG-A-P, TBG-B-P, TBG-A-S, TBG-B-S, xtal. Not every clock is usable for higher buadrates. Any subset can be specified in the device-tree and the driver will choose the best one which also still supports the mandatory baudrate of 9600 Bd. For smooth boot log output it is needed to specify clock used by early console, otherwise garbage would be printed on UART during probe of UART clock driver and transitioning from early console to normal console.

We are implementing this to be able to configure TBG clock as UART parent clock, which is required to be able to achieve higher baudrates than 230400 Bd. We achieve this by referencing this new UART clock device node in UART's device node. UART clock device driver automatically chooses the best clock source for UART driver.

Until now, UART's device-tree node needed to reference one of the static clocks (xtal or one of the TBGs) as parent clock in the clocks phandle - the parent clock which was configured before booting the kernel. If bootloader changed UART's parent clock, it needed to change the clocks phandle in DTB correspondingly before booting.

From now on both the old mechanism (xtal or TBG referenced as parent clock in clocks phandle) and the new one (UART clock referenced in the clocks phandle) are supported, to provide full backward compatibility with existing DTS files, full backward compatibility with existing boot loaders, and to provide new features (runtime clock configuration to allow higher baudrates than 230400 Bd). New features are available only with new DTS files.

There was also a discussion about how the UART node and the clock-controller node could be wrapped together in a new binding [1, 2]. As explained there, this is not possible if we want to keep backwards compatibility with existing bootloaders, and thus we are doing this by putting the UART clock-controller node inside the UART1 node.

[1] https://lore.kernel.org/linux-serial/20220120000651.in7s6nazif5qjkme@pali/
[2] https://lore.kernel.org/linux-serial/[email protected]/

---
Changes since v8:
- fixed typo in commit message
Changes since v7:
- commit message was rewritten
- changed comments a little
- fixed indentation at some places
- added Marek's Reviewed-by tag

---
 drivers/tty/serial/Kconfig      |   1 +
 drivers/tty/serial/mvebu-uart.c | 521 +++++++++++++++++++++++++++++++-
 2 files changed, 520 insertions(+), 2 deletions(-)

index 0e5ccb25bdb1..9bc1325dd7ba 100644
@@ -1445,6 +1445,7 @@ config SERIAL_STM32_CONSOLE
 config SERIAL_MVEBU_UART
 	bool "Marvell EBU serial port support"
 	depends on ARCH_MVEBU || COMPILE_TEST
+	depends on COMMON_CLK
 	select SERIAL_CORE
 	help
 	  This driver is for Marvell EBU SoC's UART. If you have a machine

2.34.1

Quoting Marek Behún (2022-02-19 07:28:15)

From: Pali Rohár <[email protected]>

Device "marvell,armada-3700-uart" should use "marvell,armada-3700-uart-clock" compatible clock.

---
 Documentation/devicetree/bindings/serial/mvebu-uart.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 2d0dbdf32d1d..a062bbca532c 100644
@@ -14,7 +14,10 @@ Required properties:
       is provided (possible only with the "marvell,armada-3700-uart"
       compatible string for backward compatibility), it will only work
       if the baudrate was initialized by the bootloader and no baudrate
-      change will then be possible.
+      change will then be possible. When provided it should be UART1-clk
+      for standard variant of UART and UART2-clk for extended variant
+      of UART. TBG clock (with UART TBG divisors d1=d2=1) or xtal clock
+      should not be used and are supported only for backward compatibility.
 - interrupts:
     - Must contain three elements for the standard variant of the IP
       (marvell,armada-3700-uart): "uart-sum", "uart-tx" and "uart-rx",

@@ -34,7 +37,7 @@ Example:
 	uart0: serial@12000 {
 		compatible = "marvell,armada-3700-uart";
 		reg = <0x12000 0x18>;
-		clocks = <&xtalclk>;
+		clocks = <&uartclk 0>;
 		interrupts =
 		<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
 		<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,

@@ -45,7 +48,7 @@ Example:
 	uart1: serial@12200 {
 		compatible = "marvell,armada-3700-uart-ext";
 		reg = <0x12200 0x30>;
-		clocks = <&xtalclk>;
+		clocks = <&uartclk 1>;
 		interrupts =
 		<GIC_SPI 30 IRQ_TYPE_EDGE_RISING>,
 		<GIC_SPI 31 IRQ_TYPE_EDGE_RISING>;

2.34.1

Quoting Marek Behún (2022-02-19 07:28:16)

From: Pali Rohár <[email protected]>

Implement simple usage of fractional divisor. When main divisor D is too large to represent requested baudrate then use divisor M from the fractional divisor feature. All the M prescalers are set to the same and maximal value 63, so the fractional part of the fractional divisor is not used at all. We also determine upper limit for possible baudrates.

Experiments show that UART at baudrate 1500000 Bd with this configuration is stable. So there is no need to implement complicated calculation of fractional coefficients yet.

To use this feature with higher baudrates, it is required to use UART clock provided by UART clock driver. Default boot xtal clock is not capable of higher baudrates.

---
 drivers/tty/serial/mvebu-uart.c | 83 ++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 18 deletions(-)

index 56278b29f5f5..2e9263888ddc 100644
@@ -99,6 +99,7 @@
 #define UART_OSAMP		0x14
 #define  OSAMP_DEFAULT_DIVISOR	16
 #define  OSAMP_DIVISORS_MASK	0x3F3F3F3F
+#define  OSAMP_MAX_DIVISOR	63
 
 #define MVEBU_NR_UARTS		2
 

@@ -479,18 +480,60 @@ static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
 		return -EOPNOTSUPP;
 
 	/*
-	 * The baudrate is derived from the UART clock thanks to two divisors:
-	 *   > D ("baud generator"): can divide the clock from 2 to 2^10 - 1.
-	 *   > M ("fractional divisor"): allows a better accuracy for
-	 *     baudrates higher than 230400.
+	 * The baudrate is derived from the UART clock thanks to divisors:
+	 *   > d1 * d2 ("TBG divisors"): can divide only TBG clock from 1 to 6
+	 *   > D ("baud generator"): can divide the clock from 1 to 1023
+	 *   > M ("fractional divisor"): allows a better accuracy (from 1 to 63)
 	 *
-	 * As the derivation of M is rather complicated, the code sticks to its
-	 * default value (x16) when all the prescalers are zeroed, and only
-	 * makes use of D to configure the desired baudrate.
+	 * Exact formulas for calculating baudrate:
+	 *
+	 * with default x16 scheme:
+	 *   baudrate = xtal / (d * 16)
+	 *   baudrate = tbg / (d1 * d2 * d * 16)
+	 *
+	 * with fractional divisor:
+	 *   baudrate = 10 * xtal / (d * (3 * (m1 + m2) + 2 * (m3 + m4)))
+	 *   baudrate = 10 * tbg / (d1*d2 * d * (3 * (m1 + m2) + 2 * (m3 + m4)))
+	 *
+	 * Oversampling value:
+	 *   osamp = (m1 << 0) | (m2 << 8) | (m3 << 16) | (m4 << 24);
+	 *
+	 * Where m1 controls number of clock cycles per bit for bits 1,2,3;
+	 * m2 for bits 4,5,6; m3 for bits 7,8 and m4 for bits 9,10.
+	 *
+	 * To simplify baudrate setup set all the M prescalers to the same
+	 * value. For baudrates 9600 Bd and higher, it is enough to use the
+	 * default (x16) divisor or fractional divisor with M = 63, so there
+	 * is no need to use real fractional support (where the M prescalers
+	 * are not equal).
+	 *
+	 * When all the M prescalers are zeroed then default (x16) divisor is
+	 * used. Default x16 scheme is more stable than M (fractional divisor),
+	 * so use M only when D divisor is not enough to derive baudrate.
+	 *
+	 * Member port->uartclk is either xtal clock rate or TBG clock rate
+	 * divided by (d1 * d2). So d1 and d2 are already set by the UART clock
+	 * driver (and UART driver itself cannot change them). Moreover they are
+	 * shared between both UARTs.
 	 */
+
 	m_divisor = OSAMP_DEFAULT_DIVISOR;
 	d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
 
+	if (d_divisor > BRDV_BAUD_MAX) {
+		/*
+		 * Experiments show that small M divisors are unstable.
+		 * Use maximal possible M = 63 and calculate D divisor.
+		 */
+		m_divisor = OSAMP_MAX_DIVISOR;
+		d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
+	}
+
+	if (d_divisor < 1)
+		d_divisor = 1;
+	else if (d_divisor > BRDV_BAUD_MAX)
+		d_divisor = BRDV_BAUD_MAX;
+
 	spin_lock_irqsave(&mvebu_uart_lock, flags);
 	brdv = readl(port->membase + UART_BRDV);
 	brdv &= ~BRDV_BAUD_MASK;

@@ -500,6 +543,9 @@ static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
 
 	osamp = readl(port->membase + UART_OSAMP);
 	osamp &= ~OSAMP_DIVISORS_MASK;
+	if (m_divisor != OSAMP_DEFAULT_DIVISOR)
+		osamp |= (m_divisor << 0) | (m_divisor << 8) |
+			(m_divisor << 16) | (m_divisor << 24);
 	writel(osamp, port->membase + UART_OSAMP);
 
 	return 0;

@@ -529,14 +575,16 @@ static void mvebu_uart_set_termios(struct uart_port *port,
 		port->ignore_status_mask |= STAT_RX_RDY(port) | STAT_BRK_ERR;
 
 	/*
-	 * Maximal divisor is 1023 * 16 when using default (x16) scheme.
-	 * Maximum achievable frequency with simple baudrate divisor is 230400.
-	 * Since the error per bit frame would be of more than 15%, achieving
-	 * higher frequencies would require to implement the fractional divisor
-	 * feature.
+	 * Maximal divisor is 1023 and maximal fractional divisor is 63. And
+	 * experiments show that baudrates above 1/80 of parent clock rate are
+	 * not stable. So disallow baudrates above 1/80 of the parent clock
+	 * rate. If port->uartclk is not available, then
+	 * mvebu_uart_baud_rate_set() fails, so values min_baud and max_baud
+	 * in this case do not matter.
 	 */
-	min_baud = DIV_ROUND_UP(port->uartclk, 1023 * 16);
-	max_baud = 230400;
+	min_baud = DIV_ROUND_UP(port->uartclk, BRDV_BAUD_MAX *
+				OSAMP_MAX_DIVISOR);
+	max_baud = port->uartclk / 80;
 
 	baud = uart_get_baud_rate(port, termios, old, min_baud, max_baud);
 	if (mvebu_uart_baud_rate_set(port, baud)) {

@@ -1395,14 +1443,14 @@ static int mvebu_uart_clock_probe(struct platform_device *pdev)
 			 * Calculate the smallest TBG d1 and d2 divisors that
 			 * still can provide 9600 baudrate.
 			 */
-			d1 = DIV_ROUND_UP(rate, 9600 * OSAMP_DEFAULT_DIVISOR *
+			d1 = DIV_ROUND_UP(rate, 9600 * OSAMP_MAX_DIVISOR *
 					  BRDV_BAUD_MAX);
 			if (d1 < 1)
 				d1 = 1;
 			else if (d1 > CLK_TBG_DIV1_MAX)
 				d1 = CLK_TBG_DIV1_MAX;
 
-			d2 = DIV_ROUND_UP(rate, 9600 * OSAMP_DEFAULT_DIVISOR *
+			d2 = DIV_ROUND_UP(rate, 9600 * OSAMP_MAX_DIVISOR *
 					  BRDV_BAUD_MAX * d1);
 			if (d2 < 1)
 				d2 = 1;

@@ -1417,8 +1465,7 @@ static int mvebu_uart_clock_probe(struct platform_device *pdev)
 		}
 
 		/* Skip clock source which cannot provide 9600 baudrate */
-		if (rate > 9600 * OSAMP_DEFAULT_DIVISOR * BRDV_BAUD_MAX * d1 *
-			   d2)
+		if (rate > 9600 * OSAMP_MAX_DIVISOR * BRDV_BAUD_MAX * d1 * d2)
 			continue;
 
 		/*

2.34.1

From: Pali Rohár <[email protected]>

Define DT node for UART clock "marvell,armada-3700-uart-clock" and use this UART clock as a base clock for all UART devices.

---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 673f4906eef9..da2f6b5c4b20 100644
@@ -132,10 +132,20 @@ avs: avs@11500 {
 				reg = <0x11500 0x40>;
 			};
 
+			uartclk: clock-controller@12010 {
+				compatible = "marvell,armada-3700-uart-clock";
+				reg = <0x12010 0x4>, <0x12210 0x4>;
+				clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>,
+					 <&tbg 3>, <&xtalclk>;
+				clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S",
+					      "TBG-B-S", "xtal";
+				#clock-cells = <1>;
+			};
+
 			uart0: serial@12000 {
 				compatible = "marvell,armada-3700-uart";
 				reg = <0x12000 0x18>;
-				clocks = <&xtalclk>;
+				clocks = <&uartclk 0>;
 				interrupts =
 				<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
 				<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,

@@ -147,7 +157,7 @@ uart0: serial@12000 {
 			uart1: serial@12200 {
 				compatible = "marvell,armada-3700-uart-ext";
 				reg = <0x12200 0x30>;
-				clocks = <&xtalclk>;
+				clocks = <&uartclk 1>;
 				interrupts =
 				<GIC_SPI 30 IRQ_TYPE_EDGE_RISING>,
 				<GIC_SPI 31 IRQ_TYPE_EDGE_RISING>;

2.34.1

Hello Greg! I think that this patch series has now all reviews. Could you look at it if you can finally take it? Or is there still something missing?

It was taken into tty-testing today 10:29.

Marek