[PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Pali Rohár]

This patch series add support for baudrates higher than 230400 on
Marvell Armada 37xx boards.

Changes in v7:
* fixed lint errors in yaml binding file

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
  serial: mvebu-uart: implement UART clock driver for configuring UART
    base clock
  dt-bindings: mvebu-uart: document DT bindings for
    marvell,armada-3700-uart-clock
  dt-bindings: mvebu-uart: update information about UART clock
  arm64: dts: marvell: armada-37xx: add device node for UART clock and
    use it
  serial: mvebu-uart: implement support for baudrates higher than 230400

 .../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               | 592 +++++++++++++++++-
 include/linux/math64.h                        |  13 +
 6 files changed, 667 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml

-- 
2.20.1

[PATCH v7 1/6] math64: New DIV_U64_ROUND_CLOSEST helper

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 include/linux/math64.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/math64.h b/include/linux/math64.h
index 2928f03d6d46..a14f40de1dca 100644
--- a/include/linux/math64.h
+++ b/include/linux/math64.h
@@ -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.20.1

[PATCH v7 2/6] serial: mvebu-uart: implement UART clock driver for configuring UART base clock

[Pali Rohár]

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

There are more pitfalls related to UART clocks. Both UARTs use same base
clock source. Also divisors for TBG base clock are shared between both
UARTs and are configured only from UART1 address space. Clocks can be
enabled / disabled separately for UART1 and UART2, but they are controlled
only from UART1 address space. Moreover Marvell Armada 3700 Functional
Specifications has swapped bits for enabling/disabling UART1 and UART2
clocks.

So driver for controlling UART2 needs to have access to UART1 address space
as UART1 address space contains some bits exclusively used by UART2 and
also bits which are shared for both UART1 and UART2.

For changing UART base clock (which controls both UARTs) during boot when
UART driver is not ready and only early console is active, is not simple
operation as it is required to also recalculate divisors to not change UART
baudrate used by early console. So for this operation UART1 clock driver
needs to access also into address space of UART2 where are registers for
UART2 divisors.

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

Shared between drivers are only two 4-bytes registers: UART Clock Control
and UART 2 Baud Rate Divisor. Access to these two registers are protected
by one spinlock to prevent any conflicts. Access is required only during
probe time, changing baudrate and during suspend/resume.

Hardware can be configured to use one of following clocks as UART base
clock: TBG-A-P, TBG-B-P, TBG-A-S, TBG-B-S, xtal. Not every clock is usable
for higher buadrates. In DT node can be specified any subset and kernel
choose the best one, which still supports required baudrate 9600. For
smooth boot log output it is needed to specify clock used by early console
otherwise garbage would be put on UART during probing for UART clock driver
and transitioning from early console to normal console.

This change is required to enable and configure TBG clock as a base clock
for UART. TBG clock is required to achieve higher baudrates than 230400.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/tty/serial/Kconfig      |   1 +
 drivers/tty/serial/mvebu-uart.c | 519 +++++++++++++++++++++++++++++++-
 2 files changed, 518 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 131a6a587acd..fe1a54231b19 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1444,6 +1444,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
diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index 231de29a6452..f3fb1f3718f2 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -8,12 +8,14 @@
 */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/math64.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -68,8 +70,31 @@
 #define  STAT_BRK_ERR		(STAT_BRK_DET | STAT_FRM_ERR \
 				 | STAT_PAR_ERR | STAT_OVR_ERR)
 
+/*
+ * Marvell Armada 3700 Functional Specifications describes that bit 21 of UART
+ * Clock Control register controls UART1 and bit 20 controls UART2. But in
+ * reality bit 21 controls UART2 and bit 20 controls UART1. This seems to be a
+ * bug in Marvell documentation. Hence following CLK_DIS macros are swapped.
+ */
+
 #define UART_BRDV		0x10
+/* These bits are located in UART1 address space and control UART2 */
+#define  UART2_CLK_DIS		BIT(21)
+/* These bits are located in UART1 address space and control UART1 */
+#define  UART1_CLK_DIS		BIT(20)
+/* These bits are located in UART1 address space and control both UARTs */
+#define  CLK_NO_XTAL		BIT(19)
+#define  CLK_TBG_DIV1_SHIFT	15
+#define  CLK_TBG_DIV1_MASK	0x7
+#define  CLK_TBG_DIV1_MAX	6
+#define  CLK_TBG_DIV2_SHIFT	12
+#define  CLK_TBG_DIV2_MASK	0x7
+#define  CLK_TBG_DIV2_MAX	6
+#define  CLK_TBG_SEL_SHIFT	10
+#define  CLK_TBG_SEL_MASK	0x3
+/* These bits are located in both UARTs address space */
 #define  BRDV_BAUD_MASK         0x3FF
+#define  BRDV_BAUD_MAX		BRDV_BAUD_MASK
 
 #define UART_OSAMP		0x14
 #define  OSAMP_DEFAULT_DIVISOR	16
@@ -153,6 +178,8 @@ static struct mvebu_uart *to_mvuart(struct uart_port *port)
 
 static struct uart_port mvebu_uart_ports[MVEBU_NR_UARTS];
 
+static DEFINE_SPINLOCK(mvebu_uart_lock);
+
 /* Core UART Driver Operations */
 static unsigned int mvebu_uart_tx_empty(struct uart_port *port)
 {
@@ -445,6 +472,7 @@ static void mvebu_uart_shutdown(struct uart_port *port)
 static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
 {
 	unsigned int d_divisor, m_divisor;
+	unsigned long flags;
 	u32 brdv, osamp;
 
 	if (!port->uartclk)
@@ -463,10 +491,12 @@ static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
 	m_divisor = OSAMP_DEFAULT_DIVISOR;
 	d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
 
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
 	brdv = readl(port->membase + UART_BRDV);
 	brdv &= ~BRDV_BAUD_MASK;
 	brdv |= d_divisor;
 	writel(brdv, port->membase + UART_BRDV);
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
 
 	osamp = readl(port->membase + UART_OSAMP);
 	osamp &= ~OSAMP_DIVISORS_MASK;
@@ -762,6 +792,7 @@ static int mvebu_uart_suspend(struct device *dev)
 {
 	struct mvebu_uart *mvuart = dev_get_drvdata(dev);
 	struct uart_port *port = mvuart->port;
+	unsigned long flags;
 
 	uart_suspend_port(&mvebu_uart_driver, port);
 
@@ -770,7 +801,9 @@ static int mvebu_uart_suspend(struct device *dev)
 	mvuart->pm_regs.ctrl = readl(port->membase + UART_CTRL(port));
 	mvuart->pm_regs.intr = readl(port->membase + UART_INTR(port));
 	mvuart->pm_regs.stat = readl(port->membase + UART_STAT);
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
 	mvuart->pm_regs.brdv = readl(port->membase + UART_BRDV);
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
 	mvuart->pm_regs.osamp = readl(port->membase + UART_OSAMP);
 
 	device_set_wakeup_enable(dev, true);
@@ -782,13 +815,16 @@ static int mvebu_uart_resume(struct device *dev)
 {
 	struct mvebu_uart *mvuart = dev_get_drvdata(dev);
 	struct uart_port *port = mvuart->port;
+	unsigned long flags;
 
 	writel(mvuart->pm_regs.rbr, port->membase + UART_RBR(port));
 	writel(mvuart->pm_regs.tsh, port->membase + UART_TSH(port));
 	writel(mvuart->pm_regs.ctrl, port->membase + UART_CTRL(port));
 	writel(mvuart->pm_regs.intr, port->membase + UART_INTR(port));
 	writel(mvuart->pm_regs.stat, port->membase + UART_STAT);
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
 	writel(mvuart->pm_regs.brdv, port->membase + UART_BRDV);
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
 	writel(mvuart->pm_regs.osamp, port->membase + UART_OSAMP);
 
 	uart_resume_port(&mvebu_uart_driver, port);
@@ -972,6 +1008,476 @@ static struct platform_driver mvebu_uart_platform_driver = {
 	},
 };
 
+/* This code is based on clk-fixed-factor.c driver and modified. */
+
+struct mvebu_uart_clock {
+	struct clk_hw clk_hw;
+	int clock_idx;
+	u32 pm_context_reg1;
+	u32 pm_context_reg2;
+};
+
+struct mvebu_uart_clock_base {
+	struct mvebu_uart_clock clocks[2];
+	unsigned int parent_rates[5];
+	int parent_idx;
+	unsigned int div;
+	void __iomem *reg1;
+	void __iomem *reg2;
+	bool configured;
+};
+
+#define PARENT_CLOCK_XTAL 4
+
+#define to_uart_clock(hw) container_of(hw, struct mvebu_uart_clock, clk_hw)
+#define to_uart_clock_base(uart_clock) container_of(uart_clock, \
+	struct mvebu_uart_clock_base, clocks[uart_clock->clock_idx])
+
+static int mvebu_uart_clock_prepare(struct clk_hw *hw)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+	unsigned int prev_clock_idx, prev_clock_rate, prev_d1d2;
+	unsigned int parent_clock_idx, parent_clock_rate;
+	unsigned long flags;
+	unsigned int d1, d2;
+	u64 divisor;
+	u32 val;
+
+	/*
+	 * This function just reconfigures UART Clock Control register (located
+	 * in UART1 address space which controls both UART1 and UART2) to
+	 * selected UART base clock and recalculate current UART1/UART2 divisors
+	 * in their address spaces, so final baudrate will not be changed by
+	 * switching UART base clock. This is required otherwise kernel boot log
+	 * stops working. It is needed to ensure that UART baudrate does not
+	 * change during this setup. It is one time operation, so based on
+	 * "configured" member this function is skipped on second call. Because
+	 * this UART Clock Control register (UART_BRDV) is shared between UART1
+	 * baudrate function, UART1 clock selector and UART2 clock selector,
+	 * every access to UART_BRDV (reg1) needs to be protected by lock.
+	 */
+
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
+
+	if (uart_clock_base->configured) {
+		spin_unlock_irqrestore(&mvebu_uart_lock, flags);
+		return 0;
+	}
+
+	parent_clock_idx = uart_clock_base->parent_idx;
+	parent_clock_rate = uart_clock_base->parent_rates[parent_clock_idx];
+
+	val = readl(uart_clock_base->reg1);
+
+	if (uart_clock_base->div > CLK_TBG_DIV1_MAX) {
+		d1 = CLK_TBG_DIV1_MAX;
+		d2 = uart_clock_base->div / CLK_TBG_DIV1_MAX;
+	} else {
+		d1 = uart_clock_base->div;
+		d2 = 1;
+	}
+
+	if (val & CLK_NO_XTAL) {
+		prev_clock_idx = (val >> CLK_TBG_SEL_SHIFT) & CLK_TBG_SEL_MASK;
+		prev_d1d2 = ((val >> CLK_TBG_DIV1_SHIFT) & CLK_TBG_DIV1_MASK)
+			  * ((val >> CLK_TBG_DIV2_SHIFT) & CLK_TBG_DIV2_MASK);
+	} else {
+		prev_clock_idx = PARENT_CLOCK_XTAL;
+		prev_d1d2 = 1;
+	}
+
+	/* Note that uart_clock_base->parent_rates[i] may not be available */
+	prev_clock_rate = uart_clock_base->parent_rates[prev_clock_idx];
+
+	/* Recalculate UART1 divisor so UART1 baudrate does not change */
+	if (prev_clock_rate) {
+		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
+						parent_clock_rate * prev_d1d2,
+						prev_clock_rate * d1 * d2);
+		if (divisor < 1)
+			divisor = 1;
+		else if (divisor > BRDV_BAUD_MAX)
+			divisor = BRDV_BAUD_MAX;
+		val = (val & ~BRDV_BAUD_MASK) | divisor;
+	}
+
+	if (parent_clock_idx != PARENT_CLOCK_XTAL) {
+		/* Do not use XTAL, select TBG clock and TBG d1 * d2 divisors */
+		val |= CLK_NO_XTAL;
+		val &= ~(CLK_TBG_DIV1_MASK << CLK_TBG_DIV1_SHIFT);
+		val |= d1 << CLK_TBG_DIV1_SHIFT;
+		val &= ~(CLK_TBG_DIV2_MASK << CLK_TBG_DIV2_SHIFT);
+		val |= d2 << CLK_TBG_DIV2_SHIFT;
+		val &= ~(CLK_TBG_SEL_MASK << CLK_TBG_SEL_SHIFT);
+		val |= parent_clock_idx << CLK_TBG_SEL_SHIFT;
+	} else {
+		/* Use XTAL, TBG bits are then ignored */
+		val &= ~CLK_NO_XTAL;
+	}
+
+	writel(val, uart_clock_base->reg1);
+
+	/* Recalculate UART2 divisor so UART2 baudrate does not change */
+	if (prev_clock_rate) {
+		val = readl(uart_clock_base->reg2);
+		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
+						parent_clock_rate * prev_d1d2,
+						prev_clock_rate * d1 * d2);
+		if (divisor < 1)
+			divisor = 1;
+		else if (divisor > BRDV_BAUD_MAX)
+			divisor = BRDV_BAUD_MAX;
+		val = (val & ~BRDV_BAUD_MASK) | divisor;
+		writel(val, uart_clock_base->reg2);
+	}
+
+	uart_clock_base->configured = true;
+
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
+
+	return 0;
+}
+
+static int mvebu_uart_clock_enable(struct clk_hw *hw)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
+
+	val = readl(uart_clock_base->reg1);
+
+	if (uart_clock->clock_idx == 0)
+		val &= ~UART1_CLK_DIS;
+	else
+		val &= ~UART2_CLK_DIS;
+
+	writel(val, uart_clock_base->reg1);
+
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
+
+	return 0;
+}
+
+static void mvebu_uart_clock_disable(struct clk_hw *hw)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
+
+	val = readl(uart_clock_base->reg1);
+
+	if (uart_clock->clock_idx == 0)
+		val |= UART1_CLK_DIS;
+	else
+		val |= UART2_CLK_DIS;
+
+	writel(val, uart_clock_base->reg1);
+
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
+}
+
+static int mvebu_uart_clock_is_enabled(struct clk_hw *hw)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+	u32 val;
+
+	val = readl(uart_clock_base->reg1);
+
+	if (uart_clock->clock_idx == 0)
+		return !(val & UART1_CLK_DIS);
+	else
+		return !(val & UART2_CLK_DIS);
+}
+
+static int mvebu_uart_clock_save_context(struct clk_hw *hw)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
+	uart_clock->pm_context_reg1 = readl(uart_clock_base->reg1);
+	uart_clock->pm_context_reg2 = readl(uart_clock_base->reg2);
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
+
+	return 0;
+}
+
+static void mvebu_uart_clock_restore_context(struct clk_hw *hw)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&mvebu_uart_lock, flags);
+	writel(uart_clock->pm_context_reg1, uart_clock_base->reg1);
+	writel(uart_clock->pm_context_reg2, uart_clock_base->reg2);
+	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
+}
+
+static unsigned long mvebu_uart_clock_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+
+	return parent_rate / uart_clock_base->div;
+}
+
+static long mvebu_uart_clock_round_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long *parent_rate)
+{
+	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
+	struct mvebu_uart_clock_base *uart_clock_base =
+						to_uart_clock_base(uart_clock);
+
+	return *parent_rate / uart_clock_base->div;
+}
+
+static int mvebu_uart_clock_set_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long parent_rate)
+{
+	/*
+	 * We must report success but we can do so unconditionally because
+	 * mvebu_uart_clock_round_rate returns values that ensure this call is a
+	 * nop.
+	 */
+
+	return 0;
+}
+
+static const struct clk_ops mvebu_uart_clock_ops = {
+	.prepare = mvebu_uart_clock_prepare,
+	.enable = mvebu_uart_clock_enable,
+	.disable = mvebu_uart_clock_disable,
+	.is_enabled = mvebu_uart_clock_is_enabled,
+	.save_context = mvebu_uart_clock_save_context,
+	.restore_context = mvebu_uart_clock_restore_context,
+	.round_rate = mvebu_uart_clock_round_rate,
+	.set_rate = mvebu_uart_clock_set_rate,
+	.recalc_rate = mvebu_uart_clock_recalc_rate,
+};
+
+static int mvebu_uart_clock_register(struct device *dev,
+				     struct mvebu_uart_clock *uart_clock,
+				     const char *name,
+				     const char *parent_name)
+{
+	struct clk_init_data init = { };
+
+	uart_clock->clk_hw.init = &init;
+
+	init.name = name;
+	init.ops = &mvebu_uart_clock_ops;
+	init.flags = 0;
+	init.num_parents = 1;
+	init.parent_names = &parent_name;
+
+	return devm_clk_hw_register(dev, &uart_clock->clk_hw);
+}
+
+static int mvebu_uart_clock_probe(struct platform_device *pdev)
+{
+	static const char *const uart_clk_names[] = { "uart_1", "uart_2" };
+	static const char *const parent_clk_names[] = { "TBG-A-P", "TBG-B-P",
+							"TBG-A-S", "TBG-B-S",
+							"xtal" };
+	struct clk *parent_clks[ARRAY_SIZE(parent_clk_names)];
+	struct mvebu_uart_clock_base *uart_clock_base;
+	struct clk_hw_onecell_data *hw_clk_data;
+	struct device *dev = &pdev->dev;
+	int i, parent_clk_idx, ret;
+	unsigned long div, rate;
+	struct resource *res;
+	unsigned int d1, d2;
+
+	BUILD_BUG_ON(ARRAY_SIZE(uart_clk_names) !=
+		     ARRAY_SIZE(uart_clock_base->clocks));
+	BUILD_BUG_ON(ARRAY_SIZE(parent_clk_names) !=
+		     ARRAY_SIZE(uart_clock_base->parent_rates));
+
+	uart_clock_base = devm_kzalloc(dev,
+				       sizeof(*uart_clock_base),
+				       GFP_KERNEL);
+	if (!uart_clock_base)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Couldn't get first register\n");
+		return -ENOENT;
+	}
+
+	/*
+	 * UART Clock Control register (reg1 / UART_BRDV) is in address range
+	 * of UART1 (standard UART variant), controls clock source and dividers
+	 * for both UART1 and UART2 and is supplied via DT as first resource.
+	 * Therefore use ioremap() function rather than ioremap_resource() to
+	 * avoid conflicts with UART1 driver. Access to UART_BRDV is protected
+	 * by lock shared between clock and UART driver.
+	 */
+	uart_clock_base->reg1 = devm_ioremap(dev, res->start,
+					     resource_size(res));
+	if (IS_ERR(uart_clock_base->reg1))
+		return PTR_ERR(uart_clock_base->reg1);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(dev, "Couldn't get second register\n");
+		return -ENOENT;
+	}
+
+	/*
+	 * UART 2 Baud Rate Divisor register (reg2 / UART_BRDV) is in address
+	 * range of UART2 (extended UART variant), controls only one UART2
+	 * specific divider and is supplied via DT as second resource.
+	 * Therefore use ioremap() function rather than ioremap_resource() to
+	 * avoid conflicts with UART2 driver. Access to UART_BRDV is protected
+	 * by lock shared between clock and UART driver.
+	 */
+	uart_clock_base->reg2 = devm_ioremap(dev, res->start,
+					     resource_size(res));
+	if (IS_ERR(uart_clock_base->reg2))
+		return PTR_ERR(uart_clock_base->reg2);
+
+	hw_clk_data = devm_kzalloc(dev,
+				   struct_size(hw_clk_data, hws,
+					       ARRAY_SIZE(uart_clk_names)),
+				   GFP_KERNEL);
+	if (!hw_clk_data)
+		return -ENOMEM;
+
+	hw_clk_data->num = ARRAY_SIZE(uart_clk_names);
+	for (i = 0; i < ARRAY_SIZE(uart_clk_names); i++) {
+		hw_clk_data->hws[i] = &uart_clock_base->clocks[i].clk_hw;
+		uart_clock_base->clocks[i].clock_idx = i;
+	}
+
+	parent_clk_idx = -1;
+
+	for (i = 0; i < ARRAY_SIZE(parent_clk_names); i++) {
+		parent_clks[i] = devm_clk_get(dev, parent_clk_names[i]);
+		if (IS_ERR(parent_clks[i])) {
+			if (PTR_ERR(parent_clks[i]) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			dev_warn(dev, "Couldn't get the parent clock %s: %ld\n",
+				parent_clk_names[i], PTR_ERR(parent_clks[i]));
+			continue;
+		}
+
+		ret = clk_prepare_enable(parent_clks[i]);
+		if (ret) {
+			dev_warn(dev, "Couldn't enable parent clock %s: %d\n",
+				parent_clk_names[i], ret);
+			continue;
+		}
+		rate = clk_get_rate(parent_clks[i]);
+		uart_clock_base->parent_rates[i] = rate;
+
+		if (i != PARENT_CLOCK_XTAL) {
+			/*
+			 * Calculate the smallest TBG d1 and d2 divisors that
+			 * still can provide 9600 baudrate.
+			 */
+			d1 = DIV_ROUND_UP(rate, 9600 * OSAMP_DEFAULT_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 *
+						BRDV_BAUD_MAX * d1);
+			if (d2 < 1)
+				d2 = 1;
+			else if (d2 > CLK_TBG_DIV2_MAX)
+				d2 = CLK_TBG_DIV2_MAX;
+		} else {
+			/*
+			 * When UART clock uses XTAL clock as a source then it
+			 * is not possible to use d1 and d2 divisors.
+			 */
+			d1 = d2 = 1;
+		}
+
+		/* Skip clock source which cannot provide 9600 baudrate */
+		if (rate > 9600 * OSAMP_DEFAULT_DIVISOR * BRDV_BAUD_MAX * d1 * d2)
+			continue;
+
+		/*
+		 * Choose TBG clock source with the smallest divisors. Use XTAL
+		 * clock source only in case TBG is not available as XTAL cannot
+		 * be used for baudrates higher than 230400.
+		 */
+		if (parent_clk_idx == -1 ||
+		    (i != PARENT_CLOCK_XTAL && div > d1 * d2)) {
+			parent_clk_idx = i;
+			div = d1 * d2;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(parent_clk_names); i++) {
+		if (i == parent_clk_idx || IS_ERR(parent_clks[i]))
+			continue;
+		clk_disable_unprepare(parent_clks[i]);
+		devm_clk_put(dev, parent_clks[i]);
+	}
+
+	if (parent_clk_idx == -1) {
+		dev_err(dev, "No usable parent clock\n");
+		return -ENOENT;
+	}
+
+	uart_clock_base->parent_idx = parent_clk_idx;
+	uart_clock_base->div = div;
+
+	dev_notice(dev, "Using parent clock %s as base UART clock\n",
+		   __clk_get_name(parent_clks[parent_clk_idx]));
+
+	for (i = 0; i < ARRAY_SIZE(uart_clk_names); i++) {
+		ret = mvebu_uart_clock_register(dev,
+				&uart_clock_base->clocks[i],
+				uart_clk_names[i],
+				__clk_get_name(parent_clks[parent_clk_idx]));
+		if (ret) {
+			dev_err(dev, "Can't register UART clock %d: %d\n",
+				i, ret);
+			return ret;
+		}
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   hw_clk_data);
+}
+
+static const struct of_device_id mvebu_uart_clock_of_match[] = {
+	{ .compatible = "marvell,armada-3700-uart-clock", },
+	{ }
+};
+
+static struct platform_driver mvebu_uart_clock_platform_driver = {
+	.probe = mvebu_uart_clock_probe,
+	.driver		= {
+		.name	= "mvebu-uart-clock",
+		.of_match_table = mvebu_uart_clock_of_match,
+	},
+};
+
 static int __init mvebu_uart_init(void)
 {
 	int ret;
@@ -980,10 +1486,19 @@ static int __init mvebu_uart_init(void)
 	if (ret)
 		return ret;
 
+	ret = platform_driver_register(&mvebu_uart_clock_platform_driver);
+	if (ret) {
+		uart_unregister_driver(&mvebu_uart_driver);
+		return ret;
+	}
+
 	ret = platform_driver_register(&mvebu_uart_platform_driver);
-	if (ret)
+	if (ret) {
+		platform_driver_unregister(&mvebu_uart_clock_platform_driver);
 		uart_unregister_driver(&mvebu_uart_driver);
+		return ret;
+	}
 
-	return ret;
+	return 0;
 }
 arch_initcall(mvebu_uart_init);
-- 
2.20.1

[PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

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

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v7
* Fix errors

Changes in v6
* Fix license
* Rename node to clock-controller@12010
* Remove maxItems
---
 .../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

diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
new file mode 100644
index 000000000000..175f5c8f2bc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
@@ -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 <pali@kernel.org>
+
+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.20.1

[PATCH v7 4/6] dt-bindings: mvebu-uart: update information about UART clock

[Pali Rohár]

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

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 Documentation/devicetree/bindings/serial/mvebu-uart.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/mvebu-uart.txt b/Documentation/devicetree/bindings/serial/mvebu-uart.txt
index 2d0dbdf32d1d..a062bbca532c 100644
--- a/Documentation/devicetree/bindings/serial/mvebu-uart.txt
+++ b/Documentation/devicetree/bindings/serial/mvebu-uart.txt
@@ -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.20.1

[PATCH v7 5/6] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it

[Pali Rohár]

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

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v6:
* Do not disable uartclk by default
* Rename node to clock-controller@12010
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 9acc5d2b5a00..f9bfe73d8ec2 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -132,10 +132,20 @@
 				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 @@
 			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.20.1

[PATCH v7 6/6] serial: mvebu-uart: implement support for baudrates higher than 230400

[Pali Rohár]

This change implements simple usage of fractional divisor. When main
divisor D is too large to represent requested baudrate then use divisor M
from fractional divisor feature. All the M prescalers are set to same and
maximal value 63, so fractional part is not used at all.

Tests showed that UART at 1500000 baudrate via this configuration is stable
and usable. 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 and this change also contains code for determining upper
limit of possible baudrate.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/tty/serial/mvebu-uart.c | 79 ++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index f3fb1f3718f2..925b3e0c098c 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -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,59 @@ 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 same value.
+	 * For 9600 baudrate and higher it is enough to use just default (x16)
+	 * divisor or fractional divisor with M = 63, so there is no need to
+	 * use real fractional support (when 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 derivate baudrate.
+	 *
+	 * Member port->uartclk is either xtal clock rate or TBG clock rate
+	 * divided by (d1 * d2). So UART clock driver already sets d1 and d2
+	 * divisors and UART driver cannot change them. Moreover they are
+	 * shared with both UARTs.
 	 */
+
 	m_divisor = OSAMP_DEFAULT_DIVISOR;
 	d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
 
+	if (d_divisor > BRDV_BAUD_MAX) {
+		/*
+		 * Experiments showed that small M divisors are unstable.
+		 * So 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 +542,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 +574,14 @@ 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 showed that baudrates above 1/80 of base clock are not
+	 * stable and usable. So disallow baudrate above 1/80 of the base clock.
+	 * When port->uartclk is not available then mvebu_uart_baud_rate_set()
+	 * fails so values min_baud and max_baud in this case does 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)) {
@@ -1394,14 +1439,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;
@@ -1416,7 +1461,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.20.1

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Pali Rohár]

Gregory, could you please review this patch series?

On Thursday 30 September 2021 11:58:32 Pali Rohár wrote:
> This patch series add support for baudrates higher than 230400 on
> Marvell Armada 37xx boards.
> 
> Changes in v7:
> * fixed lint errors in yaml binding file
> 
> 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
>   serial: mvebu-uart: implement UART clock driver for configuring UART
>     base clock
>   dt-bindings: mvebu-uart: document DT bindings for
>     marvell,armada-3700-uart-clock
>   dt-bindings: mvebu-uart: update information about UART clock
>   arm64: dts: marvell: armada-37xx: add device node for UART clock and
>     use it
>   serial: mvebu-uart: implement support for baudrates higher than 230400
> 
>  .../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               | 592 +++++++++++++++++-
>  include/linux/math64.h                        |  13 +
>  6 files changed, 667 insertions(+), 21 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> 
> -- 
> 2.20.1
> 

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Rob Herring]

On Thu, 30 Sep 2021 11:58:35 +0200, Pali Rohár wrote:
> This change adds DT bindings documentation for device nodes with compatible
> string "marvell,armada-3700-uart-clock".
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v7
> * Fix errors
> 
> Changes in v6
> * Fix license
> * Rename node to clock-controller@12010
> * Remove maxItems
> ---
>  .../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
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v7 4/6] dt-bindings: mvebu-uart: update information about UART clock

[Rob Herring]

On Thu, Sep 30, 2021 at 4:59 AM Pali Rohár <pali@kernel.org> wrote:
>
> Device "marvell,armada-3700-uart" should use
> "marvell,armada-3700-uart-clock" compatible clock.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  Documentation/devicetree/bindings/serial/mvebu-uart.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v7 5/6] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it

[Gregory CLEMENT]

Hello Pali,

> This change defines DT node for UART clock "marvell,armada-3700-uart-clock"
> and use this UART clock as a base clock for all UART devices.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
>
> ---
> Changes in v6:
> * Do not disable uartclk by default
> * Rename node to clock-controller@12010
> ---
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index 9acc5d2b5a00..f9bfe73d8ec2 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -132,10 +132,20 @@
>  				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>;

What happens if we have a new kernel using on old device tree ?

Gregory

>  				interrupts =
>  				<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
>  				<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> @@ -147,7 +157,7 @@
>  			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.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v7 2/6] serial: mvebu-uart: implement UART clock driver for configuring UART base clock

[Gregory CLEMENT]

Hello Pali,

> This patch implements a new device driver for controlling UART clocks on
> Marvell Armada 3700 SoC. This device driver is loaded for devices which
> match compatible string "marvell,armada-3700-uart-clock".
>
> There are more pitfalls related to UART clocks. Both UARTs use same base
> clock source. Also divisors for TBG base clock are shared between both
> UARTs and are configured only from UART1 address space. Clocks can be
> enabled / disabled separately for UART1 and UART2, but they are controlled
> only from UART1 address space. Moreover Marvell Armada 3700 Functional
> Specifications has swapped bits for enabling/disabling UART1 and UART2
> clocks.
>
> So driver for controlling UART2 needs to have access to UART1 address space
> as UART1 address space contains some bits exclusively used by UART2 and
> also bits which are shared for both UART1 and UART2.
>
> For changing UART base clock (which controls both UARTs) during boot when
> UART driver is not ready and only early console is active, is not simple
> operation as it is required to also recalculate divisors to not change UART
> baudrate used by early console. So for this operation UART1 clock driver
> needs to access also into address space of UART2 where are registers for
> UART2 divisors.
>
> For these reasons, this new device driver for UART clocks does not use
> ioremap_resource(), but only ioremap() to prevent resource conflicts
> between UART clock driver and UART driver.
>
> Shared between drivers are only two 4-bytes registers: UART Clock Control
> and UART 2 Baud Rate Divisor. Access to these two registers are protected
> by one spinlock to prevent any conflicts. Access is required only during
> probe time, changing baudrate and during suspend/resume.
>
> Hardware can be configured to use one of following clocks as UART base
> clock: TBG-A-P, TBG-B-P, TBG-A-S, TBG-B-S, xtal. Not every clock is usable
> for higher buadrates. In DT node can be specified any subset and kernel
> choose the best one, which still supports required baudrate 9600. For
> smooth boot log output it is needed to specify clock used by early console
> otherwise garbage would be put on UART during probing for UART clock driver
> and transitioning from early console to normal console.
>
> This change is required to enable and configure TBG clock as a base clock
> for UART. TBG clock is required to achieve higher baudrates than
> 230400.

Did you have a review from the clock maintainer for this driver ?

I found it very unusual to have the implementation of a clok driver
inside an uart driver.

Gregory

>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/tty/serial/Kconfig      |   1 +
>  drivers/tty/serial/mvebu-uart.c | 519 +++++++++++++++++++++++++++++++-
>  2 files changed, 518 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 131a6a587acd..fe1a54231b19 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1444,6 +1444,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
> diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
> index 231de29a6452..f3fb1f3718f2 100644
> --- a/drivers/tty/serial/mvebu-uart.c
> +++ b/drivers/tty/serial/mvebu-uart.c
> @@ -8,12 +8,14 @@
>  */
>  
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> +#include <linux/math64.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> @@ -68,8 +70,31 @@
>  #define  STAT_BRK_ERR		(STAT_BRK_DET | STAT_FRM_ERR \
>  				 | STAT_PAR_ERR | STAT_OVR_ERR)
>  
> +/*
> + * Marvell Armada 3700 Functional Specifications describes that bit 21 of UART
> + * Clock Control register controls UART1 and bit 20 controls UART2. But in
> + * reality bit 21 controls UART2 and bit 20 controls UART1. This seems to be a
> + * bug in Marvell documentation. Hence following CLK_DIS macros are swapped.
> + */
> +
>  #define UART_BRDV		0x10
> +/* These bits are located in UART1 address space and control UART2 */
> +#define  UART2_CLK_DIS		BIT(21)
> +/* These bits are located in UART1 address space and control UART1 */
> +#define  UART1_CLK_DIS		BIT(20)
> +/* These bits are located in UART1 address space and control both UARTs */
> +#define  CLK_NO_XTAL		BIT(19)
> +#define  CLK_TBG_DIV1_SHIFT	15
> +#define  CLK_TBG_DIV1_MASK	0x7
> +#define  CLK_TBG_DIV1_MAX	6
> +#define  CLK_TBG_DIV2_SHIFT	12
> +#define  CLK_TBG_DIV2_MASK	0x7
> +#define  CLK_TBG_DIV2_MAX	6
> +#define  CLK_TBG_SEL_SHIFT	10
> +#define  CLK_TBG_SEL_MASK	0x3
> +/* These bits are located in both UARTs address space */
>  #define  BRDV_BAUD_MASK         0x3FF
> +#define  BRDV_BAUD_MAX		BRDV_BAUD_MASK
>  
>  #define UART_OSAMP		0x14
>  #define  OSAMP_DEFAULT_DIVISOR	16
> @@ -153,6 +178,8 @@ static struct mvebu_uart *to_mvuart(struct uart_port *port)
>  
>  static struct uart_port mvebu_uart_ports[MVEBU_NR_UARTS];
>  
> +static DEFINE_SPINLOCK(mvebu_uart_lock);
> +
>  /* Core UART Driver Operations */
>  static unsigned int mvebu_uart_tx_empty(struct uart_port *port)
>  {
> @@ -445,6 +472,7 @@ static void mvebu_uart_shutdown(struct uart_port *port)
>  static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
>  {
>  	unsigned int d_divisor, m_divisor;
> +	unsigned long flags;
>  	u32 brdv, osamp;
>  
>  	if (!port->uartclk)
> @@ -463,10 +491,12 @@ static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
>  	m_divisor = OSAMP_DEFAULT_DIVISOR;
>  	d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
>  
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
>  	brdv = readl(port->membase + UART_BRDV);
>  	brdv &= ~BRDV_BAUD_MASK;
>  	brdv |= d_divisor;
>  	writel(brdv, port->membase + UART_BRDV);
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
>  
>  	osamp = readl(port->membase + UART_OSAMP);
>  	osamp &= ~OSAMP_DIVISORS_MASK;
> @@ -762,6 +792,7 @@ static int mvebu_uart_suspend(struct device *dev)
>  {
>  	struct mvebu_uart *mvuart = dev_get_drvdata(dev);
>  	struct uart_port *port = mvuart->port;
> +	unsigned long flags;
>  
>  	uart_suspend_port(&mvebu_uart_driver, port);
>  
> @@ -770,7 +801,9 @@ static int mvebu_uart_suspend(struct device *dev)
>  	mvuart->pm_regs.ctrl = readl(port->membase + UART_CTRL(port));
>  	mvuart->pm_regs.intr = readl(port->membase + UART_INTR(port));
>  	mvuart->pm_regs.stat = readl(port->membase + UART_STAT);
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
>  	mvuart->pm_regs.brdv = readl(port->membase + UART_BRDV);
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
>  	mvuart->pm_regs.osamp = readl(port->membase + UART_OSAMP);
>  
>  	device_set_wakeup_enable(dev, true);
> @@ -782,13 +815,16 @@ static int mvebu_uart_resume(struct device *dev)
>  {
>  	struct mvebu_uart *mvuart = dev_get_drvdata(dev);
>  	struct uart_port *port = mvuart->port;
> +	unsigned long flags;
>  
>  	writel(mvuart->pm_regs.rbr, port->membase + UART_RBR(port));
>  	writel(mvuart->pm_regs.tsh, port->membase + UART_TSH(port));
>  	writel(mvuart->pm_regs.ctrl, port->membase + UART_CTRL(port));
>  	writel(mvuart->pm_regs.intr, port->membase + UART_INTR(port));
>  	writel(mvuart->pm_regs.stat, port->membase + UART_STAT);
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
>  	writel(mvuart->pm_regs.brdv, port->membase + UART_BRDV);
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
>  	writel(mvuart->pm_regs.osamp, port->membase + UART_OSAMP);
>  
>  	uart_resume_port(&mvebu_uart_driver, port);
> @@ -972,6 +1008,476 @@ static struct platform_driver mvebu_uart_platform_driver = {
>  	},
>  };
>  
> +/* This code is based on clk-fixed-factor.c driver and modified. */
> +
> +struct mvebu_uart_clock {
> +	struct clk_hw clk_hw;
> +	int clock_idx;
> +	u32 pm_context_reg1;
> +	u32 pm_context_reg2;
> +};
> +
> +struct mvebu_uart_clock_base {
> +	struct mvebu_uart_clock clocks[2];
> +	unsigned int parent_rates[5];
> +	int parent_idx;
> +	unsigned int div;
> +	void __iomem *reg1;
> +	void __iomem *reg2;
> +	bool configured;
> +};
> +
> +#define PARENT_CLOCK_XTAL 4
> +
> +#define to_uart_clock(hw) container_of(hw, struct mvebu_uart_clock, clk_hw)
> +#define to_uart_clock_base(uart_clock) container_of(uart_clock, \
> +	struct mvebu_uart_clock_base, clocks[uart_clock->clock_idx])
> +
> +static int mvebu_uart_clock_prepare(struct clk_hw *hw)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +	unsigned int prev_clock_idx, prev_clock_rate, prev_d1d2;
> +	unsigned int parent_clock_idx, parent_clock_rate;
> +	unsigned long flags;
> +	unsigned int d1, d2;
> +	u64 divisor;
> +	u32 val;
> +
> +	/*
> +	 * This function just reconfigures UART Clock Control register (located
> +	 * in UART1 address space which controls both UART1 and UART2) to
> +	 * selected UART base clock and recalculate current UART1/UART2 divisors
> +	 * in their address spaces, so final baudrate will not be changed by
> +	 * switching UART base clock. This is required otherwise kernel boot log
> +	 * stops working. It is needed to ensure that UART baudrate does not
> +	 * change during this setup. It is one time operation, so based on
> +	 * "configured" member this function is skipped on second call. Because
> +	 * this UART Clock Control register (UART_BRDV) is shared between UART1
> +	 * baudrate function, UART1 clock selector and UART2 clock selector,
> +	 * every access to UART_BRDV (reg1) needs to be protected by lock.
> +	 */
> +
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> +
> +	if (uart_clock_base->configured) {
> +		spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> +		return 0;
> +	}
> +
> +	parent_clock_idx = uart_clock_base->parent_idx;
> +	parent_clock_rate = uart_clock_base->parent_rates[parent_clock_idx];
> +
> +	val = readl(uart_clock_base->reg1);
> +
> +	if (uart_clock_base->div > CLK_TBG_DIV1_MAX) {
> +		d1 = CLK_TBG_DIV1_MAX;
> +		d2 = uart_clock_base->div / CLK_TBG_DIV1_MAX;
> +	} else {
> +		d1 = uart_clock_base->div;
> +		d2 = 1;
> +	}
> +
> +	if (val & CLK_NO_XTAL) {
> +		prev_clock_idx = (val >> CLK_TBG_SEL_SHIFT) & CLK_TBG_SEL_MASK;
> +		prev_d1d2 = ((val >> CLK_TBG_DIV1_SHIFT) & CLK_TBG_DIV1_MASK)
> +			  * ((val >> CLK_TBG_DIV2_SHIFT) & CLK_TBG_DIV2_MASK);
> +	} else {
> +		prev_clock_idx = PARENT_CLOCK_XTAL;
> +		prev_d1d2 = 1;
> +	}
> +
> +	/* Note that uart_clock_base->parent_rates[i] may not be available */
> +	prev_clock_rate = uart_clock_base->parent_rates[prev_clock_idx];
> +
> +	/* Recalculate UART1 divisor so UART1 baudrate does not change */
> +	if (prev_clock_rate) {
> +		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
> +						parent_clock_rate * prev_d1d2,
> +						prev_clock_rate * d1 * d2);
> +		if (divisor < 1)
> +			divisor = 1;
> +		else if (divisor > BRDV_BAUD_MAX)
> +			divisor = BRDV_BAUD_MAX;
> +		val = (val & ~BRDV_BAUD_MASK) | divisor;
> +	}
> +
> +	if (parent_clock_idx != PARENT_CLOCK_XTAL) {
> +		/* Do not use XTAL, select TBG clock and TBG d1 * d2 divisors */
> +		val |= CLK_NO_XTAL;
> +		val &= ~(CLK_TBG_DIV1_MASK << CLK_TBG_DIV1_SHIFT);
> +		val |= d1 << CLK_TBG_DIV1_SHIFT;
> +		val &= ~(CLK_TBG_DIV2_MASK << CLK_TBG_DIV2_SHIFT);
> +		val |= d2 << CLK_TBG_DIV2_SHIFT;
> +		val &= ~(CLK_TBG_SEL_MASK << CLK_TBG_SEL_SHIFT);
> +		val |= parent_clock_idx << CLK_TBG_SEL_SHIFT;
> +	} else {
> +		/* Use XTAL, TBG bits are then ignored */
> +		val &= ~CLK_NO_XTAL;
> +	}
> +
> +	writel(val, uart_clock_base->reg1);
> +
> +	/* Recalculate UART2 divisor so UART2 baudrate does not change */
> +	if (prev_clock_rate) {
> +		val = readl(uart_clock_base->reg2);
> +		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
> +						parent_clock_rate * prev_d1d2,
> +						prev_clock_rate * d1 * d2);
> +		if (divisor < 1)
> +			divisor = 1;
> +		else if (divisor > BRDV_BAUD_MAX)
> +			divisor = BRDV_BAUD_MAX;
> +		val = (val & ~BRDV_BAUD_MASK) | divisor;
> +		writel(val, uart_clock_base->reg2);
> +	}
> +
> +	uart_clock_base->configured = true;
> +
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int mvebu_uart_clock_enable(struct clk_hw *hw)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> +
> +	val = readl(uart_clock_base->reg1);
> +
> +	if (uart_clock->clock_idx == 0)
> +		val &= ~UART1_CLK_DIS;
> +	else
> +		val &= ~UART2_CLK_DIS;
> +
> +	writel(val, uart_clock_base->reg1);
> +
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> +
> +	return 0;
> +}
> +
> +static void mvebu_uart_clock_disable(struct clk_hw *hw)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> +
> +	val = readl(uart_clock_base->reg1);
> +
> +	if (uart_clock->clock_idx == 0)
> +		val |= UART1_CLK_DIS;
> +	else
> +		val |= UART2_CLK_DIS;
> +
> +	writel(val, uart_clock_base->reg1);
> +
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> +}
> +
> +static int mvebu_uart_clock_is_enabled(struct clk_hw *hw)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +	u32 val;
> +
> +	val = readl(uart_clock_base->reg1);
> +
> +	if (uart_clock->clock_idx == 0)
> +		return !(val & UART1_CLK_DIS);
> +	else
> +		return !(val & UART2_CLK_DIS);
> +}
> +
> +static int mvebu_uart_clock_save_context(struct clk_hw *hw)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> +	uart_clock->pm_context_reg1 = readl(uart_clock_base->reg1);
> +	uart_clock->pm_context_reg2 = readl(uart_clock_base->reg2);
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> +
> +	return 0;
> +}
> +
> +static void mvebu_uart_clock_restore_context(struct clk_hw *hw)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> +	writel(uart_clock->pm_context_reg1, uart_clock_base->reg1);
> +	writel(uart_clock->pm_context_reg2, uart_clock_base->reg2);
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> +}
> +
> +static unsigned long mvebu_uart_clock_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +
> +	return parent_rate / uart_clock_base->div;
> +}
> +
> +static long mvebu_uart_clock_round_rate(struct clk_hw *hw, unsigned long rate,
> +					unsigned long *parent_rate)
> +{
> +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> +	struct mvebu_uart_clock_base *uart_clock_base =
> +						to_uart_clock_base(uart_clock);
> +
> +	return *parent_rate / uart_clock_base->div;
> +}
> +
> +static int mvebu_uart_clock_set_rate(struct clk_hw *hw, unsigned long rate,
> +				     unsigned long parent_rate)
> +{
> +	/*
> +	 * We must report success but we can do so unconditionally because
> +	 * mvebu_uart_clock_round_rate returns values that ensure this call is a
> +	 * nop.
> +	 */
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops mvebu_uart_clock_ops = {
> +	.prepare = mvebu_uart_clock_prepare,
> +	.enable = mvebu_uart_clock_enable,
> +	.disable = mvebu_uart_clock_disable,
> +	.is_enabled = mvebu_uart_clock_is_enabled,
> +	.save_context = mvebu_uart_clock_save_context,
> +	.restore_context = mvebu_uart_clock_restore_context,
> +	.round_rate = mvebu_uart_clock_round_rate,
> +	.set_rate = mvebu_uart_clock_set_rate,
> +	.recalc_rate = mvebu_uart_clock_recalc_rate,
> +};
> +
> +static int mvebu_uart_clock_register(struct device *dev,
> +				     struct mvebu_uart_clock *uart_clock,
> +				     const char *name,
> +				     const char *parent_name)
> +{
> +	struct clk_init_data init = { };
> +
> +	uart_clock->clk_hw.init = &init;
> +
> +	init.name = name;
> +	init.ops = &mvebu_uart_clock_ops;
> +	init.flags = 0;
> +	init.num_parents = 1;
> +	init.parent_names = &parent_name;
> +
> +	return devm_clk_hw_register(dev, &uart_clock->clk_hw);
> +}
> +
> +static int mvebu_uart_clock_probe(struct platform_device *pdev)
> +{
> +	static const char *const uart_clk_names[] = { "uart_1", "uart_2" };
> +	static const char *const parent_clk_names[] = { "TBG-A-P", "TBG-B-P",
> +							"TBG-A-S", "TBG-B-S",
> +							"xtal" };
> +	struct clk *parent_clks[ARRAY_SIZE(parent_clk_names)];
> +	struct mvebu_uart_clock_base *uart_clock_base;
> +	struct clk_hw_onecell_data *hw_clk_data;
> +	struct device *dev = &pdev->dev;
> +	int i, parent_clk_idx, ret;
> +	unsigned long div, rate;
> +	struct resource *res;
> +	unsigned int d1, d2;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(uart_clk_names) !=
> +		     ARRAY_SIZE(uart_clock_base->clocks));
> +	BUILD_BUG_ON(ARRAY_SIZE(parent_clk_names) !=
> +		     ARRAY_SIZE(uart_clock_base->parent_rates));
> +
> +	uart_clock_base = devm_kzalloc(dev,
> +				       sizeof(*uart_clock_base),
> +				       GFP_KERNEL);
> +	if (!uart_clock_base)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "Couldn't get first register\n");
> +		return -ENOENT;
> +	}
> +
> +	/*
> +	 * UART Clock Control register (reg1 / UART_BRDV) is in address range
> +	 * of UART1 (standard UART variant), controls clock source and dividers
> +	 * for both UART1 and UART2 and is supplied via DT as first resource.
> +	 * Therefore use ioremap() function rather than ioremap_resource() to
> +	 * avoid conflicts with UART1 driver. Access to UART_BRDV is protected
> +	 * by lock shared between clock and UART driver.
> +	 */
> +	uart_clock_base->reg1 = devm_ioremap(dev, res->start,
> +					     resource_size(res));
> +	if (IS_ERR(uart_clock_base->reg1))
> +		return PTR_ERR(uart_clock_base->reg1);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(dev, "Couldn't get second register\n");
> +		return -ENOENT;
> +	}
> +
> +	/*
> +	 * UART 2 Baud Rate Divisor register (reg2 / UART_BRDV) is in address
> +	 * range of UART2 (extended UART variant), controls only one UART2
> +	 * specific divider and is supplied via DT as second resource.
> +	 * Therefore use ioremap() function rather than ioremap_resource() to
> +	 * avoid conflicts with UART2 driver. Access to UART_BRDV is protected
> +	 * by lock shared between clock and UART driver.
> +	 */
> +	uart_clock_base->reg2 = devm_ioremap(dev, res->start,
> +					     resource_size(res));
> +	if (IS_ERR(uart_clock_base->reg2))
> +		return PTR_ERR(uart_clock_base->reg2);
> +
> +	hw_clk_data = devm_kzalloc(dev,
> +				   struct_size(hw_clk_data, hws,
> +					       ARRAY_SIZE(uart_clk_names)),
> +				   GFP_KERNEL);
> +	if (!hw_clk_data)
> +		return -ENOMEM;
> +
> +	hw_clk_data->num = ARRAY_SIZE(uart_clk_names);
> +	for (i = 0; i < ARRAY_SIZE(uart_clk_names); i++) {
> +		hw_clk_data->hws[i] = &uart_clock_base->clocks[i].clk_hw;
> +		uart_clock_base->clocks[i].clock_idx = i;
> +	}
> +
> +	parent_clk_idx = -1;
> +
> +	for (i = 0; i < ARRAY_SIZE(parent_clk_names); i++) {
> +		parent_clks[i] = devm_clk_get(dev, parent_clk_names[i]);
> +		if (IS_ERR(parent_clks[i])) {
> +			if (PTR_ERR(parent_clks[i]) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			dev_warn(dev, "Couldn't get the parent clock %s: %ld\n",
> +				parent_clk_names[i], PTR_ERR(parent_clks[i]));
> +			continue;
> +		}
> +
> +		ret = clk_prepare_enable(parent_clks[i]);
> +		if (ret) {
> +			dev_warn(dev, "Couldn't enable parent clock %s: %d\n",
> +				parent_clk_names[i], ret);
> +			continue;
> +		}
> +		rate = clk_get_rate(parent_clks[i]);
> +		uart_clock_base->parent_rates[i] = rate;
> +
> +		if (i != PARENT_CLOCK_XTAL) {
> +			/*
> +			 * Calculate the smallest TBG d1 and d2 divisors that
> +			 * still can provide 9600 baudrate.
> +			 */
> +			d1 = DIV_ROUND_UP(rate, 9600 * OSAMP_DEFAULT_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 *
> +						BRDV_BAUD_MAX * d1);
> +			if (d2 < 1)
> +				d2 = 1;
> +			else if (d2 > CLK_TBG_DIV2_MAX)
> +				d2 = CLK_TBG_DIV2_MAX;
> +		} else {
> +			/*
> +			 * When UART clock uses XTAL clock as a source then it
> +			 * is not possible to use d1 and d2 divisors.
> +			 */
> +			d1 = d2 = 1;
> +		}
> +
> +		/* Skip clock source which cannot provide 9600 baudrate */
> +		if (rate > 9600 * OSAMP_DEFAULT_DIVISOR * BRDV_BAUD_MAX * d1 * d2)
> +			continue;
> +
> +		/*
> +		 * Choose TBG clock source with the smallest divisors. Use XTAL
> +		 * clock source only in case TBG is not available as XTAL cannot
> +		 * be used for baudrates higher than 230400.
> +		 */
> +		if (parent_clk_idx == -1 ||
> +		    (i != PARENT_CLOCK_XTAL && div > d1 * d2)) {
> +			parent_clk_idx = i;
> +			div = d1 * d2;
> +		}
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(parent_clk_names); i++) {
> +		if (i == parent_clk_idx || IS_ERR(parent_clks[i]))
> +			continue;
> +		clk_disable_unprepare(parent_clks[i]);
> +		devm_clk_put(dev, parent_clks[i]);
> +	}
> +
> +	if (parent_clk_idx == -1) {
> +		dev_err(dev, "No usable parent clock\n");
> +		return -ENOENT;
> +	}
> +
> +	uart_clock_base->parent_idx = parent_clk_idx;
> +	uart_clock_base->div = div;
> +
> +	dev_notice(dev, "Using parent clock %s as base UART clock\n",
> +		   __clk_get_name(parent_clks[parent_clk_idx]));
> +
> +	for (i = 0; i < ARRAY_SIZE(uart_clk_names); i++) {
> +		ret = mvebu_uart_clock_register(dev,
> +				&uart_clock_base->clocks[i],
> +				uart_clk_names[i],
> +				__clk_get_name(parent_clks[parent_clk_idx]));
> +		if (ret) {
> +			dev_err(dev, "Can't register UART clock %d: %d\n",
> +				i, ret);
> +			return ret;
> +		}
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   hw_clk_data);
> +}
> +
> +static const struct of_device_id mvebu_uart_clock_of_match[] = {
> +	{ .compatible = "marvell,armada-3700-uart-clock", },
> +	{ }
> +};
> +
> +static struct platform_driver mvebu_uart_clock_platform_driver = {
> +	.probe = mvebu_uart_clock_probe,
> +	.driver		= {
> +		.name	= "mvebu-uart-clock",
> +		.of_match_table = mvebu_uart_clock_of_match,
> +	},
> +};
> +
>  static int __init mvebu_uart_init(void)
>  {
>  	int ret;
> @@ -980,10 +1486,19 @@ static int __init mvebu_uart_init(void)
>  	if (ret)
>  		return ret;
>  
> +	ret = platform_driver_register(&mvebu_uart_clock_platform_driver);
> +	if (ret) {
> +		uart_unregister_driver(&mvebu_uart_driver);
> +		return ret;
> +	}
> +
>  	ret = platform_driver_register(&mvebu_uart_platform_driver);
> -	if (ret)
> +	if (ret) {
> +		platform_driver_unregister(&mvebu_uart_clock_platform_driver);
>  		uart_unregister_driver(&mvebu_uart_driver);
> +		return ret;
> +	}
>  
> -	return ret;
> +	return 0;
>  }
>  arch_initcall(mvebu_uart_init);
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v7 5/6] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it

[Pali Rohár]

On Wednesday 13 October 2021 16:13:16 Gregory CLEMENT wrote:
> Hello Pali,
> 
> > This change defines DT node for UART clock "marvell,armada-3700-uart-clock"
> > and use this UART clock as a base clock for all UART devices.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> >
> > ---
> > Changes in v6:
> > * Do not disable uartclk by default
> > * Rename node to clock-controller@12010
> > ---
> >  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > index 9acc5d2b5a00..f9bfe73d8ec2 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > @@ -132,10 +132,20 @@
> >  				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>;
> 
> What happens if we have a new kernel using on old device tree ?

Hello! There would be no change. New kernel would work as old kernel.
Into clocks property you can still set xtatclk as before and it would
still works.

> Gregory
> 
> >  				interrupts =
> >  				<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
> >  				<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> > @@ -147,7 +157,7 @@
> >  			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.20.1
> >
> 
> -- 
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com

Re: [PATCH v7 2/6] serial: mvebu-uart: implement UART clock driver for configuring UART base clock

[Pali Rohár]

On Wednesday 13 October 2021 16:16:10 Gregory CLEMENT wrote:
> Hello Pali,
> 
> > This patch implements a new device driver for controlling UART clocks on
> > Marvell Armada 3700 SoC. This device driver is loaded for devices which
> > match compatible string "marvell,armada-3700-uart-clock".
> >
> > There are more pitfalls related to UART clocks. Both UARTs use same base
> > clock source. Also divisors for TBG base clock are shared between both
> > UARTs and are configured only from UART1 address space. Clocks can be
> > enabled / disabled separately for UART1 and UART2, but they are controlled
> > only from UART1 address space. Moreover Marvell Armada 3700 Functional
> > Specifications has swapped bits for enabling/disabling UART1 and UART2
> > clocks.
> >
> > So driver for controlling UART2 needs to have access to UART1 address space
> > as UART1 address space contains some bits exclusively used by UART2 and
> > also bits which are shared for both UART1 and UART2.
> >
> > For changing UART base clock (which controls both UARTs) during boot when
> > UART driver is not ready and only early console is active, is not simple
> > operation as it is required to also recalculate divisors to not change UART
> > baudrate used by early console. So for this operation UART1 clock driver
> > needs to access also into address space of UART2 where are registers for
> > UART2 divisors.
> >
> > For these reasons, this new device driver for UART clocks does not use
> > ioremap_resource(), but only ioremap() to prevent resource conflicts
> > between UART clock driver and UART driver.
> >
> > Shared between drivers are only two 4-bytes registers: UART Clock Control
> > and UART 2 Baud Rate Divisor. Access to these two registers are protected
> > by one spinlock to prevent any conflicts. Access is required only during
> > probe time, changing baudrate and during suspend/resume.
> >
> > Hardware can be configured to use one of following clocks as UART base
> > clock: TBG-A-P, TBG-B-P, TBG-A-S, TBG-B-S, xtal. Not every clock is usable
> > for higher buadrates. In DT node can be specified any subset and kernel
> > choose the best one, which still supports required baudrate 9600. For
> > smooth boot log output it is needed to specify clock used by early console
> > otherwise garbage would be put on UART during probing for UART clock driver
> > and transitioning from early console to normal console.
> >
> > This change is required to enable and configure TBG clock as a base clock
> > for UART. TBG clock is required to achieve higher baudrates than
> > 230400.
> 
> Did you have a review from the clock maintainer for this driver ?

Nobody did review (yet).

> I found it very unusual to have the implementation of a clok driver
> inside an uart driver.

Yes, it is unusual, but nothing better can be done with current HW
design (UART1 regs controls UART2) and already used DTS file to provide
backward compatibility.

> Gregory
> 
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  drivers/tty/serial/Kconfig      |   1 +
> >  drivers/tty/serial/mvebu-uart.c | 519 +++++++++++++++++++++++++++++++-
> >  2 files changed, 518 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 131a6a587acd..fe1a54231b19 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -1444,6 +1444,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
> > diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
> > index 231de29a6452..f3fb1f3718f2 100644
> > --- a/drivers/tty/serial/mvebu-uart.c
> > +++ b/drivers/tty/serial/mvebu-uart.c
> > @@ -8,12 +8,14 @@
> >  */
> >  
> >  #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >  #include <linux/console.h>
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/math64.h>
> >  #include <linux/of.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_device.h>
> > @@ -68,8 +70,31 @@
> >  #define  STAT_BRK_ERR		(STAT_BRK_DET | STAT_FRM_ERR \
> >  				 | STAT_PAR_ERR | STAT_OVR_ERR)
> >  
> > +/*
> > + * Marvell Armada 3700 Functional Specifications describes that bit 21 of UART
> > + * Clock Control register controls UART1 and bit 20 controls UART2. But in
> > + * reality bit 21 controls UART2 and bit 20 controls UART1. This seems to be a
> > + * bug in Marvell documentation. Hence following CLK_DIS macros are swapped.
> > + */
> > +
> >  #define UART_BRDV		0x10
> > +/* These bits are located in UART1 address space and control UART2 */
> > +#define  UART2_CLK_DIS		BIT(21)
> > +/* These bits are located in UART1 address space and control UART1 */
> > +#define  UART1_CLK_DIS		BIT(20)
> > +/* These bits are located in UART1 address space and control both UARTs */
> > +#define  CLK_NO_XTAL		BIT(19)
> > +#define  CLK_TBG_DIV1_SHIFT	15
> > +#define  CLK_TBG_DIV1_MASK	0x7
> > +#define  CLK_TBG_DIV1_MAX	6
> > +#define  CLK_TBG_DIV2_SHIFT	12
> > +#define  CLK_TBG_DIV2_MASK	0x7
> > +#define  CLK_TBG_DIV2_MAX	6
> > +#define  CLK_TBG_SEL_SHIFT	10
> > +#define  CLK_TBG_SEL_MASK	0x3
> > +/* These bits are located in both UARTs address space */
> >  #define  BRDV_BAUD_MASK         0x3FF
> > +#define  BRDV_BAUD_MAX		BRDV_BAUD_MASK
> >  
> >  #define UART_OSAMP		0x14
> >  #define  OSAMP_DEFAULT_DIVISOR	16
> > @@ -153,6 +178,8 @@ static struct mvebu_uart *to_mvuart(struct uart_port *port)
> >  
> >  static struct uart_port mvebu_uart_ports[MVEBU_NR_UARTS];
> >  
> > +static DEFINE_SPINLOCK(mvebu_uart_lock);
> > +
> >  /* Core UART Driver Operations */
> >  static unsigned int mvebu_uart_tx_empty(struct uart_port *port)
> >  {
> > @@ -445,6 +472,7 @@ static void mvebu_uart_shutdown(struct uart_port *port)
> >  static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
> >  {
> >  	unsigned int d_divisor, m_divisor;
> > +	unsigned long flags;
> >  	u32 brdv, osamp;
> >  
> >  	if (!port->uartclk)
> > @@ -463,10 +491,12 @@ static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
> >  	m_divisor = OSAMP_DEFAULT_DIVISOR;
> >  	d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
> >  
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> >  	brdv = readl(port->membase + UART_BRDV);
> >  	brdv &= ~BRDV_BAUD_MASK;
> >  	brdv |= d_divisor;
> >  	writel(brdv, port->membase + UART_BRDV);
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> >  
> >  	osamp = readl(port->membase + UART_OSAMP);
> >  	osamp &= ~OSAMP_DIVISORS_MASK;
> > @@ -762,6 +792,7 @@ static int mvebu_uart_suspend(struct device *dev)
> >  {
> >  	struct mvebu_uart *mvuart = dev_get_drvdata(dev);
> >  	struct uart_port *port = mvuart->port;
> > +	unsigned long flags;
> >  
> >  	uart_suspend_port(&mvebu_uart_driver, port);
> >  
> > @@ -770,7 +801,9 @@ static int mvebu_uart_suspend(struct device *dev)
> >  	mvuart->pm_regs.ctrl = readl(port->membase + UART_CTRL(port));
> >  	mvuart->pm_regs.intr = readl(port->membase + UART_INTR(port));
> >  	mvuart->pm_regs.stat = readl(port->membase + UART_STAT);
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> >  	mvuart->pm_regs.brdv = readl(port->membase + UART_BRDV);
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> >  	mvuart->pm_regs.osamp = readl(port->membase + UART_OSAMP);
> >  
> >  	device_set_wakeup_enable(dev, true);
> > @@ -782,13 +815,16 @@ static int mvebu_uart_resume(struct device *dev)
> >  {
> >  	struct mvebu_uart *mvuart = dev_get_drvdata(dev);
> >  	struct uart_port *port = mvuart->port;
> > +	unsigned long flags;
> >  
> >  	writel(mvuart->pm_regs.rbr, port->membase + UART_RBR(port));
> >  	writel(mvuart->pm_regs.tsh, port->membase + UART_TSH(port));
> >  	writel(mvuart->pm_regs.ctrl, port->membase + UART_CTRL(port));
> >  	writel(mvuart->pm_regs.intr, port->membase + UART_INTR(port));
> >  	writel(mvuart->pm_regs.stat, port->membase + UART_STAT);
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> >  	writel(mvuart->pm_regs.brdv, port->membase + UART_BRDV);
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> >  	writel(mvuart->pm_regs.osamp, port->membase + UART_OSAMP);
> >  
> >  	uart_resume_port(&mvebu_uart_driver, port);
> > @@ -972,6 +1008,476 @@ static struct platform_driver mvebu_uart_platform_driver = {
> >  	},
> >  };
> >  
> > +/* This code is based on clk-fixed-factor.c driver and modified. */
> > +
> > +struct mvebu_uart_clock {
> > +	struct clk_hw clk_hw;
> > +	int clock_idx;
> > +	u32 pm_context_reg1;
> > +	u32 pm_context_reg2;
> > +};
> > +
> > +struct mvebu_uart_clock_base {
> > +	struct mvebu_uart_clock clocks[2];
> > +	unsigned int parent_rates[5];
> > +	int parent_idx;
> > +	unsigned int div;
> > +	void __iomem *reg1;
> > +	void __iomem *reg2;
> > +	bool configured;
> > +};
> > +
> > +#define PARENT_CLOCK_XTAL 4
> > +
> > +#define to_uart_clock(hw) container_of(hw, struct mvebu_uart_clock, clk_hw)
> > +#define to_uart_clock_base(uart_clock) container_of(uart_clock, \
> > +	struct mvebu_uart_clock_base, clocks[uart_clock->clock_idx])
> > +
> > +static int mvebu_uart_clock_prepare(struct clk_hw *hw)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +	unsigned int prev_clock_idx, prev_clock_rate, prev_d1d2;
> > +	unsigned int parent_clock_idx, parent_clock_rate;
> > +	unsigned long flags;
> > +	unsigned int d1, d2;
> > +	u64 divisor;
> > +	u32 val;
> > +
> > +	/*
> > +	 * This function just reconfigures UART Clock Control register (located
> > +	 * in UART1 address space which controls both UART1 and UART2) to
> > +	 * selected UART base clock and recalculate current UART1/UART2 divisors
> > +	 * in their address spaces, so final baudrate will not be changed by
> > +	 * switching UART base clock. This is required otherwise kernel boot log
> > +	 * stops working. It is needed to ensure that UART baudrate does not
> > +	 * change during this setup. It is one time operation, so based on
> > +	 * "configured" member this function is skipped on second call. Because
> > +	 * this UART Clock Control register (UART_BRDV) is shared between UART1
> > +	 * baudrate function, UART1 clock selector and UART2 clock selector,
> > +	 * every access to UART_BRDV (reg1) needs to be protected by lock.
> > +	 */
> > +
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> > +
> > +	if (uart_clock_base->configured) {
> > +		spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> > +		return 0;
> > +	}
> > +
> > +	parent_clock_idx = uart_clock_base->parent_idx;
> > +	parent_clock_rate = uart_clock_base->parent_rates[parent_clock_idx];
> > +
> > +	val = readl(uart_clock_base->reg1);
> > +
> > +	if (uart_clock_base->div > CLK_TBG_DIV1_MAX) {
> > +		d1 = CLK_TBG_DIV1_MAX;
> > +		d2 = uart_clock_base->div / CLK_TBG_DIV1_MAX;
> > +	} else {
> > +		d1 = uart_clock_base->div;
> > +		d2 = 1;
> > +	}
> > +
> > +	if (val & CLK_NO_XTAL) {
> > +		prev_clock_idx = (val >> CLK_TBG_SEL_SHIFT) & CLK_TBG_SEL_MASK;
> > +		prev_d1d2 = ((val >> CLK_TBG_DIV1_SHIFT) & CLK_TBG_DIV1_MASK)
> > +			  * ((val >> CLK_TBG_DIV2_SHIFT) & CLK_TBG_DIV2_MASK);
> > +	} else {
> > +		prev_clock_idx = PARENT_CLOCK_XTAL;
> > +		prev_d1d2 = 1;
> > +	}
> > +
> > +	/* Note that uart_clock_base->parent_rates[i] may not be available */
> > +	prev_clock_rate = uart_clock_base->parent_rates[prev_clock_idx];
> > +
> > +	/* Recalculate UART1 divisor so UART1 baudrate does not change */
> > +	if (prev_clock_rate) {
> > +		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
> > +						parent_clock_rate * prev_d1d2,
> > +						prev_clock_rate * d1 * d2);
> > +		if (divisor < 1)
> > +			divisor = 1;
> > +		else if (divisor > BRDV_BAUD_MAX)
> > +			divisor = BRDV_BAUD_MAX;
> > +		val = (val & ~BRDV_BAUD_MASK) | divisor;
> > +	}
> > +
> > +	if (parent_clock_idx != PARENT_CLOCK_XTAL) {
> > +		/* Do not use XTAL, select TBG clock and TBG d1 * d2 divisors */
> > +		val |= CLK_NO_XTAL;
> > +		val &= ~(CLK_TBG_DIV1_MASK << CLK_TBG_DIV1_SHIFT);
> > +		val |= d1 << CLK_TBG_DIV1_SHIFT;
> > +		val &= ~(CLK_TBG_DIV2_MASK << CLK_TBG_DIV2_SHIFT);
> > +		val |= d2 << CLK_TBG_DIV2_SHIFT;
> > +		val &= ~(CLK_TBG_SEL_MASK << CLK_TBG_SEL_SHIFT);
> > +		val |= parent_clock_idx << CLK_TBG_SEL_SHIFT;
> > +	} else {
> > +		/* Use XTAL, TBG bits are then ignored */
> > +		val &= ~CLK_NO_XTAL;
> > +	}
> > +
> > +	writel(val, uart_clock_base->reg1);
> > +
> > +	/* Recalculate UART2 divisor so UART2 baudrate does not change */
> > +	if (prev_clock_rate) {
> > +		val = readl(uart_clock_base->reg2);
> > +		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
> > +						parent_clock_rate * prev_d1d2,
> > +						prev_clock_rate * d1 * d2);
> > +		if (divisor < 1)
> > +			divisor = 1;
> > +		else if (divisor > BRDV_BAUD_MAX)
> > +			divisor = BRDV_BAUD_MAX;
> > +		val = (val & ~BRDV_BAUD_MASK) | divisor;
> > +		writel(val, uart_clock_base->reg2);
> > +	}
> > +
> > +	uart_clock_base->configured = true;
> > +
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mvebu_uart_clock_enable(struct clk_hw *hw)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +	unsigned long flags;
> > +	u32 val;
> > +
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> > +
> > +	val = readl(uart_clock_base->reg1);
> > +
> > +	if (uart_clock->clock_idx == 0)
> > +		val &= ~UART1_CLK_DIS;
> > +	else
> > +		val &= ~UART2_CLK_DIS;
> > +
> > +	writel(val, uart_clock_base->reg1);
> > +
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mvebu_uart_clock_disable(struct clk_hw *hw)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +	unsigned long flags;
> > +	u32 val;
> > +
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> > +
> > +	val = readl(uart_clock_base->reg1);
> > +
> > +	if (uart_clock->clock_idx == 0)
> > +		val |= UART1_CLK_DIS;
> > +	else
> > +		val |= UART2_CLK_DIS;
> > +
> > +	writel(val, uart_clock_base->reg1);
> > +
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> > +}
> > +
> > +static int mvebu_uart_clock_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +	u32 val;
> > +
> > +	val = readl(uart_clock_base->reg1);
> > +
> > +	if (uart_clock->clock_idx == 0)
> > +		return !(val & UART1_CLK_DIS);
> > +	else
> > +		return !(val & UART2_CLK_DIS);
> > +}
> > +
> > +static int mvebu_uart_clock_save_context(struct clk_hw *hw)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> > +	uart_clock->pm_context_reg1 = readl(uart_clock_base->reg1);
> > +	uart_clock->pm_context_reg2 = readl(uart_clock_base->reg2);
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mvebu_uart_clock_restore_context(struct clk_hw *hw)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&mvebu_uart_lock, flags);
> > +	writel(uart_clock->pm_context_reg1, uart_clock_base->reg1);
> > +	writel(uart_clock->pm_context_reg2, uart_clock_base->reg2);
> > +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
> > +}
> > +
> > +static unsigned long mvebu_uart_clock_recalc_rate(struct clk_hw *hw,
> > +						  unsigned long parent_rate)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +
> > +	return parent_rate / uart_clock_base->div;
> > +}
> > +
> > +static long mvebu_uart_clock_round_rate(struct clk_hw *hw, unsigned long rate,
> > +					unsigned long *parent_rate)
> > +{
> > +	struct mvebu_uart_clock *uart_clock = to_uart_clock(hw);
> > +	struct mvebu_uart_clock_base *uart_clock_base =
> > +						to_uart_clock_base(uart_clock);
> > +
> > +	return *parent_rate / uart_clock_base->div;
> > +}
> > +
> > +static int mvebu_uart_clock_set_rate(struct clk_hw *hw, unsigned long rate,
> > +				     unsigned long parent_rate)
> > +{
> > +	/*
> > +	 * We must report success but we can do so unconditionally because
> > +	 * mvebu_uart_clock_round_rate returns values that ensure this call is a
> > +	 * nop.
> > +	 */
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct clk_ops mvebu_uart_clock_ops = {
> > +	.prepare = mvebu_uart_clock_prepare,
> > +	.enable = mvebu_uart_clock_enable,
> > +	.disable = mvebu_uart_clock_disable,
> > +	.is_enabled = mvebu_uart_clock_is_enabled,
> > +	.save_context = mvebu_uart_clock_save_context,
> > +	.restore_context = mvebu_uart_clock_restore_context,
> > +	.round_rate = mvebu_uart_clock_round_rate,
> > +	.set_rate = mvebu_uart_clock_set_rate,
> > +	.recalc_rate = mvebu_uart_clock_recalc_rate,
> > +};
> > +
> > +static int mvebu_uart_clock_register(struct device *dev,
> > +				     struct mvebu_uart_clock *uart_clock,
> > +				     const char *name,
> > +				     const char *parent_name)
> > +{
> > +	struct clk_init_data init = { };
> > +
> > +	uart_clock->clk_hw.init = &init;
> > +
> > +	init.name = name;
> > +	init.ops = &mvebu_uart_clock_ops;
> > +	init.flags = 0;
> > +	init.num_parents = 1;
> > +	init.parent_names = &parent_name;
> > +
> > +	return devm_clk_hw_register(dev, &uart_clock->clk_hw);
> > +}
> > +
> > +static int mvebu_uart_clock_probe(struct platform_device *pdev)
> > +{
> > +	static const char *const uart_clk_names[] = { "uart_1", "uart_2" };
> > +	static const char *const parent_clk_names[] = { "TBG-A-P", "TBG-B-P",
> > +							"TBG-A-S", "TBG-B-S",
> > +							"xtal" };
> > +	struct clk *parent_clks[ARRAY_SIZE(parent_clk_names)];
> > +	struct mvebu_uart_clock_base *uart_clock_base;
> > +	struct clk_hw_onecell_data *hw_clk_data;
> > +	struct device *dev = &pdev->dev;
> > +	int i, parent_clk_idx, ret;
> > +	unsigned long div, rate;
> > +	struct resource *res;
> > +	unsigned int d1, d2;
> > +
> > +	BUILD_BUG_ON(ARRAY_SIZE(uart_clk_names) !=
> > +		     ARRAY_SIZE(uart_clock_base->clocks));
> > +	BUILD_BUG_ON(ARRAY_SIZE(parent_clk_names) !=
> > +		     ARRAY_SIZE(uart_clock_base->parent_rates));
> > +
> > +	uart_clock_base = devm_kzalloc(dev,
> > +				       sizeof(*uart_clock_base),
> > +				       GFP_KERNEL);
> > +	if (!uart_clock_base)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(dev, "Couldn't get first register\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	/*
> > +	 * UART Clock Control register (reg1 / UART_BRDV) is in address range
> > +	 * of UART1 (standard UART variant), controls clock source and dividers
> > +	 * for both UART1 and UART2 and is supplied via DT as first resource.
> > +	 * Therefore use ioremap() function rather than ioremap_resource() to
> > +	 * avoid conflicts with UART1 driver. Access to UART_BRDV is protected
> > +	 * by lock shared between clock and UART driver.
> > +	 */
> > +	uart_clock_base->reg1 = devm_ioremap(dev, res->start,
> > +					     resource_size(res));
> > +	if (IS_ERR(uart_clock_base->reg1))
> > +		return PTR_ERR(uart_clock_base->reg1);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (!res) {
> > +		dev_err(dev, "Couldn't get second register\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	/*
> > +	 * UART 2 Baud Rate Divisor register (reg2 / UART_BRDV) is in address
> > +	 * range of UART2 (extended UART variant), controls only one UART2
> > +	 * specific divider and is supplied via DT as second resource.
> > +	 * Therefore use ioremap() function rather than ioremap_resource() to
> > +	 * avoid conflicts with UART2 driver. Access to UART_BRDV is protected
> > +	 * by lock shared between clock and UART driver.
> > +	 */
> > +	uart_clock_base->reg2 = devm_ioremap(dev, res->start,
> > +					     resource_size(res));
> > +	if (IS_ERR(uart_clock_base->reg2))
> > +		return PTR_ERR(uart_clock_base->reg2);
> > +
> > +	hw_clk_data = devm_kzalloc(dev,
> > +				   struct_size(hw_clk_data, hws,
> > +					       ARRAY_SIZE(uart_clk_names)),
> > +				   GFP_KERNEL);
> > +	if (!hw_clk_data)
> > +		return -ENOMEM;
> > +
> > +	hw_clk_data->num = ARRAY_SIZE(uart_clk_names);
> > +	for (i = 0; i < ARRAY_SIZE(uart_clk_names); i++) {
> > +		hw_clk_data->hws[i] = &uart_clock_base->clocks[i].clk_hw;
> > +		uart_clock_base->clocks[i].clock_idx = i;
> > +	}
> > +
> > +	parent_clk_idx = -1;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(parent_clk_names); i++) {
> > +		parent_clks[i] = devm_clk_get(dev, parent_clk_names[i]);
> > +		if (IS_ERR(parent_clks[i])) {
> > +			if (PTR_ERR(parent_clks[i]) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +			dev_warn(dev, "Couldn't get the parent clock %s: %ld\n",
> > +				parent_clk_names[i], PTR_ERR(parent_clks[i]));
> > +			continue;
> > +		}
> > +
> > +		ret = clk_prepare_enable(parent_clks[i]);
> > +		if (ret) {
> > +			dev_warn(dev, "Couldn't enable parent clock %s: %d\n",
> > +				parent_clk_names[i], ret);
> > +			continue;
> > +		}
> > +		rate = clk_get_rate(parent_clks[i]);
> > +		uart_clock_base->parent_rates[i] = rate;
> > +
> > +		if (i != PARENT_CLOCK_XTAL) {
> > +			/*
> > +			 * Calculate the smallest TBG d1 and d2 divisors that
> > +			 * still can provide 9600 baudrate.
> > +			 */
> > +			d1 = DIV_ROUND_UP(rate, 9600 * OSAMP_DEFAULT_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 *
> > +						BRDV_BAUD_MAX * d1);
> > +			if (d2 < 1)
> > +				d2 = 1;
> > +			else if (d2 > CLK_TBG_DIV2_MAX)
> > +				d2 = CLK_TBG_DIV2_MAX;
> > +		} else {
> > +			/*
> > +			 * When UART clock uses XTAL clock as a source then it
> > +			 * is not possible to use d1 and d2 divisors.
> > +			 */
> > +			d1 = d2 = 1;
> > +		}
> > +
> > +		/* Skip clock source which cannot provide 9600 baudrate */
> > +		if (rate > 9600 * OSAMP_DEFAULT_DIVISOR * BRDV_BAUD_MAX * d1 * d2)
> > +			continue;
> > +
> > +		/*
> > +		 * Choose TBG clock source with the smallest divisors. Use XTAL
> > +		 * clock source only in case TBG is not available as XTAL cannot
> > +		 * be used for baudrates higher than 230400.
> > +		 */
> > +		if (parent_clk_idx == -1 ||
> > +		    (i != PARENT_CLOCK_XTAL && div > d1 * d2)) {
> > +			parent_clk_idx = i;
> > +			div = d1 * d2;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(parent_clk_names); i++) {
> > +		if (i == parent_clk_idx || IS_ERR(parent_clks[i]))
> > +			continue;
> > +		clk_disable_unprepare(parent_clks[i]);
> > +		devm_clk_put(dev, parent_clks[i]);
> > +	}
> > +
> > +	if (parent_clk_idx == -1) {
> > +		dev_err(dev, "No usable parent clock\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	uart_clock_base->parent_idx = parent_clk_idx;
> > +	uart_clock_base->div = div;
> > +
> > +	dev_notice(dev, "Using parent clock %s as base UART clock\n",
> > +		   __clk_get_name(parent_clks[parent_clk_idx]));
> > +
> > +	for (i = 0; i < ARRAY_SIZE(uart_clk_names); i++) {
> > +		ret = mvebu_uart_clock_register(dev,
> > +				&uart_clock_base->clocks[i],
> > +				uart_clk_names[i],
> > +				__clk_get_name(parent_clks[parent_clk_idx]));
> > +		if (ret) {
> > +			dev_err(dev, "Can't register UART clock %d: %d\n",
> > +				i, ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> > +					   hw_clk_data);
> > +}
> > +
> > +static const struct of_device_id mvebu_uart_clock_of_match[] = {
> > +	{ .compatible = "marvell,armada-3700-uart-clock", },
> > +	{ }
> > +};
> > +
> > +static struct platform_driver mvebu_uart_clock_platform_driver = {
> > +	.probe = mvebu_uart_clock_probe,
> > +	.driver		= {
> > +		.name	= "mvebu-uart-clock",
> > +		.of_match_table = mvebu_uart_clock_of_match,
> > +	},
> > +};
> > +
> >  static int __init mvebu_uart_init(void)
> >  {
> >  	int ret;
> > @@ -980,10 +1486,19 @@ static int __init mvebu_uart_init(void)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = platform_driver_register(&mvebu_uart_clock_platform_driver);
> > +	if (ret) {
> > +		uart_unregister_driver(&mvebu_uart_driver);
> > +		return ret;
> > +	}
> > +
> >  	ret = platform_driver_register(&mvebu_uart_platform_driver);
> > -	if (ret)
> > +	if (ret) {
> > +		platform_driver_unregister(&mvebu_uart_clock_platform_driver);
> >  		uart_unregister_driver(&mvebu_uart_driver);
> > +		return ret;
> > +	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  arch_initcall(mvebu_uart_init);
> > -- 
> > 2.20.1
> >
> 
> -- 
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Stephen Boyd]

Quoting Pali Rohár (2021-09-30 02:58:35)
> diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> new file mode 100644
> index 000000000000..175f5c8f2bc5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> @@ -0,0 +1,59 @@
[..]
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    uartclk: clock-controller@12010 {

The uart device is at 0x12000 and the clock-controller is at 0x12010?
This looks like a node is being put into DT to represent a clk driver.
Why can't we register a clk from the uart device driver itself? I think
we talked about this a month or two ago but it still isn't clear to me.

> +      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.20.1
>

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> Quoting Pali Rohár (2021-09-30 02:58:35)
> > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > new file mode 100644
> > index 000000000000..175f5c8f2bc5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > @@ -0,0 +1,59 @@
> [..]
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    uartclk: clock-controller@12010 {
> 
> The uart device is at 0x12000 and the clock-controller is at 0x12010?
> This looks like a node is being put into DT to represent a clk driver.
> Why can't we register a clk from the uart device driver itself? I think
> we talked about this a month or two ago but it still isn't clear to me.

We have already talked about it and I have already wrote reasons. UART
clk is shared for both UART1 and UART2. And UART clk regs are in both
address spaces of UART1 and UART2. UART1 or UART2 can be independently
disabled on particular board (as pins are MPP which may be configured to
different function). So you have a board only with UART2, you have to
disable UART1 node, but at the same time you have to access UART clk to
drive UART2. And UART clk bits are in UART1 address space.

> > +      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.20.1
> >

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > new file mode 100644
> > > index 000000000000..175f5c8f2bc5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > @@ -0,0 +1,59 @@
> > [..]
> > > +  '#clock-cells':
> > > +    const: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - '#clock-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    uartclk: clock-controller@12010 {
> > 
> > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > This looks like a node is being put into DT to represent a clk driver.
> > Why can't we register a clk from the uart device driver itself? I think
> > we talked about this a month or two ago but it still isn't clear to me.
> 
> We have already talked about it and I have already wrote reasons. UART
> clk is shared for both UART1 and UART2. And UART clk regs are in both
> address spaces of UART1 and UART2. UART1 or UART2 can be independently
> disabled on particular board (as pins are MPP which may be configured to
> different function). So you have a board only with UART2, you have to
> disable UART1 node, but at the same time you have to access UART clk to
> drive UART2. And UART clk bits are in UART1 address space.

It is explained also in commit message of patch 2/6.

> > > +      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.20.1
> > >

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Stephen Boyd]

Quoting Pali Rohár (2021-10-15 02:37:01)
> On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> > On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > new file mode 100644
> > > > index 000000000000..175f5c8f2bc5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > @@ -0,0 +1,59 @@
> > > [..]
> > > > +  '#clock-cells':
> > > > +    const: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - '#clock-cells'
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    uartclk: clock-controller@12010 {
> > > 
> > > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > > This looks like a node is being put into DT to represent a clk driver.
> > > Why can't we register a clk from the uart device driver itself? I think
> > > we talked about this a month or two ago but it still isn't clear to me.
> > 
> > We have already talked about it and I have already wrote reasons. UART
> > clk is shared for both UART1 and UART2. And UART clk regs are in both
> > address spaces of UART1 and UART2. UART1 or UART2 can be independently
> > disabled on particular board (as pins are MPP which may be configured to
> > different function). So you have a board only with UART2, you have to
> > disable UART1 node, but at the same time you have to access UART clk to
> > drive UART2. And UART clk bits are in UART1 address space.
> 
> It is explained also in commit message of patch 2/6.

Cool, thanks for the pointer.

Why are the two uarts split into different device nodes? It looks like
it's one device that was split into two nodes because they're fairly
similar hardware blocks, and one or the other may not be used on the
board so we want to use status = "disabled" to indicate that. Sadly the
hardware team has delivered them as a single package into the SoC at
address 0x12000 and then stuck a common clk for both uarts into the same
uart wrapper. Here's a clk, job done!

Is it a problem to map UART1 address space when it isn't used on the
board? I'm trying to understand why it can't work to register two uart
ports from one device node and driver. It seems to be the main reason
why we're introducing another node for the clk registers when it feels
like it could all be handled in the existing uart driver.

For example, we could have a static clk pointer in the uart driver
indicating the clk has been registered, and then register the clk if
uart1 or uart2 is the first device to probe and then store that clk in a
global (with clk_hw_get_clk(), I think that's a thing now). If uart2
probes first it can take the reg property and subtract some number to
find the clk, and if uart1 probes first it can take the reg property and
add some number to find the clk. Either way, the binding doesn't change
in this case and we don't have to add another binding for this same uart
hardware.

Then if someone wants to cleanup the binding they can combine both uarts
into one node, make a new compatible string and add some property to
indicate that one or the other uart isn't used. Probably also add some
property to map the uart alias to the uart hardware block inside the
wrapper node.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell, armada-3700-uart-clock

[Mark Kettenis]

> From: Stephen Boyd <sboyd@kernel.org>
> Date: Fri, 15 Oct 2021 14:55:47 -0700
> 
> Quoting Pali Rohár (2021-10-15 02:37:01)
> > On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> > > On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > > > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..175f5c8f2bc5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > @@ -0,0 +1,59 @@
> > > > [..]
> > > > > +  '#clock-cells':
> > > > > +    const: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - clocks
> > > > > +  - clock-names
> > > > > +  - '#clock-cells'
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    uartclk: clock-controller@12010 {
> > > > 
> > > > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > > > This looks like a node is being put into DT to represent a clk driver.
> > > > Why can't we register a clk from the uart device driver itself? I think
> > > > we talked about this a month or two ago but it still isn't clear to me.
> > > 
> > > We have already talked about it and I have already wrote reasons. UART
> > > clk is shared for both UART1 and UART2. And UART clk regs are in both
> > > address spaces of UART1 and UART2. UART1 or UART2 can be independently
> > > disabled on particular board (as pins are MPP which may be configured to
> > > different function). So you have a board only with UART2, you have to
> > > disable UART1 node, but at the same time you have to access UART clk to
> > > drive UART2. And UART clk bits are in UART1 address space.
> > 
> > It is explained also in commit message of patch 2/6.
> 
> Cool, thanks for the pointer.
> 
> Why are the two uarts split into different device nodes? It looks like
> it's one device that was split into two nodes because they're fairly
> similar hardware blocks, and one or the other may not be used on the
> board so we want to use status = "disabled" to indicate that. Sadly the
> hardware team has delivered them as a single package into the SoC at
> address 0x12000 and then stuck a common clk for both uarts into the same
> uart wrapper. Here's a clk, job done!
> 
> Is it a problem to map UART1 address space when it isn't used on the
> board? I'm trying to understand why it can't work to register two uart
> ports from one device node and driver.

Separate nodes are needed because stdin-path and stdout-patch need to
be able to point at a specific device node.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Friday 15 October 2021 14:55:47 Stephen Boyd wrote:
> Quoting Pali Rohár (2021-10-15 02:37:01)
> > On Friday 15 October 2021 11:09:37 Pali Rohár wrote:
> > > On Thursday 14 October 2021 17:13:03 Stephen Boyd wrote:
> > > > Quoting Pali Rohár (2021-09-30 02:58:35)
> > > > > diff --git a/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..175f5c8f2bc5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> > > > > @@ -0,0 +1,59 @@
> > > > [..]
> > > > > +  '#clock-cells':
> > > > > +    const: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - clocks
> > > > > +  - clock-names
> > > > > +  - '#clock-cells'
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    uartclk: clock-controller@12010 {
> > > > 
> > > > The uart device is at 0x12000 and the clock-controller is at 0x12010?
> > > > This looks like a node is being put into DT to represent a clk driver.
> > > > Why can't we register a clk from the uart device driver itself? I think
> > > > we talked about this a month or two ago but it still isn't clear to me.
> > > 
> > > We have already talked about it and I have already wrote reasons. UART
> > > clk is shared for both UART1 and UART2. And UART clk regs are in both
> > > address spaces of UART1 and UART2. UART1 or UART2 can be independently
> > > disabled on particular board (as pins are MPP which may be configured to
> > > different function). So you have a board only with UART2, you have to
> > > disable UART1 node, but at the same time you have to access UART clk to
> > > drive UART2. And UART clk bits are in UART1 address space.
> > 
> > It is explained also in commit message of patch 2/6.
> 
> Cool, thanks for the pointer.
> 
> Why are the two uarts split into different device nodes?

I do not know. Looks like decision of people implementing this driver
and providing DT bindings.

I cannot change this existing DT binding due to backward compatibility.

> It looks like
> it's one device that was split into two nodes because they're fairly
> similar hardware blocks, and one or the other may not be used on the
> board so we want to use status = "disabled" to indicate that. Sadly the
> hardware team has delivered them as a single package into the SoC at
> address 0x12000 and then stuck a common clk for both uarts into the same
> uart wrapper. Here's a clk, job done!
> 
> Is it a problem to map UART1 address space when it isn't used on the
> board? I'm trying to understand why it can't work to register two uart
> ports from one device node and driver. It seems to be the main reason
> why we're introducing another node for the clk registers when it feels
> like it could all be handled in the existing uart driver.

Mapping address space should work. Also because in UART1 address space
is configuration of UART clock (which is common for both UART1 and
UART2). Moreover each UART has its own bit for disabling clock and these
two bits are in UART1 address space. Also it is a good idea to disable
unused UART clock (which seems to be magically/automatically done by
kernel when nobody use specified UART clock, as UART clock driver
exports two clocks).

> For example, we could have a static clk pointer in the uart driver
> indicating the clk has been registered, and then register the clk if
> uart1 or uart2 is the first device to probe and then store that clk in a
> global (with clk_hw_get_clk(), I think that's a thing now). If uart2
> probes first it can take the reg property and subtract some number to
> find the clk, and if uart1 probes first it can take the reg property and
> add some number to find the clk. Either way, the binding doesn't change
> in this case and we don't have to add another binding for this same uart
> hardware.
> 
> Then if someone wants to cleanup the binding they can combine both uarts
> into one node, make a new compatible string and add some property to
> indicate that one or the other uart isn't used. Probably also add some
> property to map the uart alias to the uart hardware block inside the
> wrapper node.

If I was designing this driver and DTS bindings I would have choose
something like this:

uart@0x12000 {
    reg = <0x12000 0x18>, <0x12200 0x30>;
    clock-controller {
        ...
    };
    serial1 {
        ...
        status = "disabled";
    };
    serial2 {
        ...
        status = "disabled";
    };
};

Meaning that 0x12000 node would be 3 subnodes and all registers would be
defined in top level nodes and would be handled by one driver.

This is really how hardware block looks like. But it is not backward
compatible...

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Pali Rohár]

On Thursday 30 September 2021 11:58:32 Pali Rohár wrote:
> This patch series add support for baudrates higher than 230400 on
> Marvell Armada 37xx boards.

Stephen, Gregory, are there any issues with this patch series?
If not, could you take them?

> Changes in v7:
> * fixed lint errors in yaml binding file
> 
> 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
>   serial: mvebu-uart: implement UART clock driver for configuring UART
>     base clock
>   dt-bindings: mvebu-uart: document DT bindings for
>     marvell,armada-3700-uart-clock
>   dt-bindings: mvebu-uart: update information about UART clock
>   arm64: dts: marvell: armada-37xx: add device node for UART clock and
>     use it
>   serial: mvebu-uart: implement support for baudrates higher than 230400
> 
>  .../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               | 592 +++++++++++++++++-
>  include/linux/math64.h                        |  13 +
>  6 files changed, 667 insertions(+), 21 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> 
> -- 
> 2.20.1
> 

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Gregory CLEMENT]

Hello Pali,

> On Thursday 30 September 2021 11:58:32 Pali Rohár wrote:
>> This patch series add support for baudrates higher than 230400 on
>> Marvell Armada 37xx boards.
>
> Stephen, Gregory, are there any issues with this patch series?

I am not found of these changes but let's apply it as I didn't take time
to do a better review.

However I can't apply the dt part if the driver is not merged.

Gregory

> If not, could you take them?
>
>> Changes in v7:
>> * fixed lint errors in yaml binding file
>> 
>> 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
>>   serial: mvebu-uart: implement UART clock driver for configuring UART
>>     base clock
>>   dt-bindings: mvebu-uart: document DT bindings for
>>     marvell,armada-3700-uart-clock
>>   dt-bindings: mvebu-uart: update information about UART clock
>>   arm64: dts: marvell: armada-37xx: add device node for UART clock and
>>     use it
>>   serial: mvebu-uart: implement support for baudrates higher than 230400
>> 
>>  .../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               | 592 +++++++++++++++++-
>>  include/linux/math64.h                        |  13 +
>>  6 files changed, 667 insertions(+), 21 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
>> 
>> -- 
>> 2.20.1
>> 

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Pali Rohár]

Hello Stephen!

On Friday 17 December 2021 18:23:00 Gregory CLEMENT wrote:
> Hello Pali,
> 
> > On Thursday 30 September 2021 11:58:32 Pali Rohár wrote:
> >> This patch series add support for baudrates higher than 230400 on
> >> Marvell Armada 37xx boards.
> >
> > Stephen, Gregory, are there any issues with this patch series?
> 
> I am not found of these changes but let's apply it as I didn't take time
> to do a better review.
> 
> However I can't apply the dt part if the driver is not merged.

Stephen, are there any issues with driver (non-DT) part changes in this
patch series? If not, could you please merge them? This patch series is
here on the list since September without any request for change...

We would really like to see support for higher baudrates for Armada 3720
platforms in mainline kernel.

> Gregory
> 
> > If not, could you take them?
> >
> >> Changes in v7:
> >> * fixed lint errors in yaml binding file
> >> 
> >> 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
> >>   serial: mvebu-uart: implement UART clock driver for configuring UART
> >>     base clock
> >>   dt-bindings: mvebu-uart: document DT bindings for
> >>     marvell,armada-3700-uart-clock
> >>   dt-bindings: mvebu-uart: update information about UART clock
> >>   arm64: dts: marvell: armada-37xx: add device node for UART clock and
> >>     use it
> >>   serial: mvebu-uart: implement support for baudrates higher than 230400
> >> 
> >>  .../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               | 592 +++++++++++++++++-
> >>  include/linux/math64.h                        |  13 +
> >>  6 files changed, 667 insertions(+), 21 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/marvell,armada-3700-uart-clock.yaml
> >> 
> >> -- 
> >> 2.20.1
> >> 
> 
> -- 
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Stephen Boyd]

Quoting Pali Rohár (2022-01-14 02:51:00)
> Hello Stephen!
> 
> On Friday 17 December 2021 18:23:00 Gregory CLEMENT wrote:
> > Hello Pali,
> > 
> > > On Thursday 30 September 2021 11:58:32 Pali Rohár wrote:
> > >> This patch series add support for baudrates higher than 230400 on
> > >> Marvell Armada 37xx boards.
> > >
> > > Stephen, Gregory, are there any issues with this patch series?
> > 
> > I am not found of these changes but let's apply it as I didn't take time
> > to do a better review.
> > 
> > However I can't apply the dt part if the driver is not merged.
> 
> Stephen, are there any issues with driver (non-DT) part changes in this
> patch series? If not, could you please merge them? This patch series is
> here on the list since September without any request for change...

Oh I missed this thread. I thought it was going to be resent.

> 
> We would really like to see support for higher baudrates for Armada 3720
> platforms in mainline kernel.

If we're adding new support why can't we break with backwards
compatibility for the binding and do it a different way?

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Pali Rohár]

On Friday 14 January 2022 14:56:58 Stephen Boyd wrote:
> Quoting Pali Rohár (2022-01-14 02:51:00)
> > Hello Stephen!
> > 
> > On Friday 17 December 2021 18:23:00 Gregory CLEMENT wrote:
> > > Hello Pali,
> > > 
> > > > On Thursday 30 September 2021 11:58:32 Pali Rohár wrote:
> > > >> This patch series add support for baudrates higher than 230400 on
> > > >> Marvell Armada 37xx boards.
> > > >
> > > > Stephen, Gregory, are there any issues with this patch series?
> > > 
> > > I am not found of these changes but let's apply it as I didn't take time
> > > to do a better review.
> > > 
> > > However I can't apply the dt part if the driver is not merged.
> > 
> > Stephen, are there any issues with driver (non-DT) part changes in this
> > patch series? If not, could you please merge them? This patch series is
> > here on the list since September without any request for change...
> 
> Oh I missed this thread. I thought it was going to be resent.
> 
> > 
> > We would really like to see support for higher baudrates for Armada 3720
> > platforms in mainline kernel.
> 
> If we're adding new support why can't we break with backwards
> compatibility for the binding and do it a different way?

Because DTS are backwards compatible. I was told more times that kernel
drivers should work correctly with older DTS files. On some boards are
DTB files provided by bootloader and they do not use in-kernel DTS
files.

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Stephen Boyd]

Quoting Pali Rohár (2022-01-14 15:05:49)
> On Friday 14 January 2022 14:56:58 Stephen Boyd wrote:
> > 
> > If we're adding new support why can't we break with backwards
> > compatibility for the binding and do it a different way?
> 
> Because DTS are backwards compatible. I was told more times that kernel
> drivers should work correctly with older DTS files. On some boards are
> DTB files provided by bootloader and they do not use in-kernel DTS
> files.

I'm not suggesting to break the kernel driver when used with older DTBs.
New features are fair game to change the compatible string and do
something different. If the user wants the new feature they update their
DTB. We shouldn't be constrained by backwards compatibility here.

Re: [PATCH v7 0/6] serial: mvebu-uart: Support for higher baudrates

[Pali Rohár]

On Friday 14 January 2022 15:16:55 Stephen Boyd wrote:
> Quoting Pali Rohár (2022-01-14 15:05:49)
> > On Friday 14 January 2022 14:56:58 Stephen Boyd wrote:
> > > 
> > > If we're adding new support why can't we break with backwards
> > > compatibility for the binding and do it a different way?
> > 
> > Because DTS are backwards compatible. I was told more times that kernel
> > drivers should work correctly with older DTS files. On some boards are
> > DTB files provided by bootloader and they do not use in-kernel DTS
> > files.
> 
> I'm not suggesting to break the kernel driver when used with older DTBs.
> New features are fair game to change the compatible string and do
> something different. If the user wants the new feature they update their
> DTB. We shouldn't be constrained by backwards compatibility here.

And what do you suggest to do? Separate UART0 and UART1 nodes are still
needed because as Mark wrote stdin-path and stdout-patch could be
different.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Stephen Boyd]

Quoting Pali Rohár (2021-10-15 23:42:10)
> 
> If I was designing this driver and DTS bindings I would have choose
> something like this:
> 
> uart@0x12000 {

Drop the 0x

>     reg = <0x12000 0x18>, <0x12200 0x30>;
>     clock-controller {
>         ...
>     };

Drop this node and put whatever properties are inside into the parent
node.

>     serial1 {
>         ...
>         status = "disabled";
>     };
>     serial2 {
>         ...
>         status = "disabled";
>     };
> };
> 
> Meaning that 0x12000 node would be 3 subnodes and all registers would be
> defined in top level nodes and would be handled by one driver.
> 
> This is really how hardware block looks like. But it is not backward
> compatible...

Sounds good to me. I presume we need the serial child nodes so we can
reference them from the stdout-path?

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> Quoting Pali Rohár (2021-10-15 23:42:10)
> > 
> > If I was designing this driver and DTS bindings I would have choose
> > something like this:
> > 
> > uart@0x12000 {
> 
> Drop the 0x
> 
> >     reg = <0x12000 0x18>, <0x12200 0x30>;
> >     clock-controller {
> >         ...
> >     };
> 
> Drop this node and put whatever properties are inside into the parent
> node.
> 
> >     serial1 {
> >         ...
> >         status = "disabled";
> >     };
> >     serial2 {
> >         ...
> >         status = "disabled";
> >     };
> > };
> > 
> > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > defined in top level nodes and would be handled by one driver.
> > 
> > This is really how hardware block looks like. But it is not backward
> > compatible...
> 
> Sounds good to me. I presume we need the serial child nodes so we can
> reference them from the stdout-path?

Yes, exactly, separate nodes for serial1 and serial2 are still required.

But dropping clock controller is not possible as for higher baudrates we
need to use and configure uart clock controller. Without it we just get
comparable feature support which is already present in driver.

But, I do not fully understand now, why to change this DTS bindings in
this incompatible way? What it brings? Because for me now it looks like
that this change does not bring anything useful, only breaks current DTS
bindings.

Driver changes would still look in the similar / same way like it is in
current patch series because bindings already contains separate nodes,
just they are children of top level node which represents in internal
registers.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Marek Behún]

On Sat, 15 Jan 2022 12:50:18 +0100
Pali Rohár <pali@kernel.org> wrote:

> On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > 
> > > If I was designing this driver and DTS bindings I would have choose
> > > something like this:
> > > 
> > > uart@0x12000 {  
> > 
> > Drop the 0x
> >   
> > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > >     clock-controller {
> > >         ...
> > >     };  
> > 
> > Drop this node and put whatever properties are inside into the parent
> > node.
> >   
> > >     serial1 {
> > >         ...
> > >         status = "disabled";
> > >     };
> > >     serial2 {
> > >         ...
> > >         status = "disabled";
> > >     };
> > > };
> > > 
> > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > defined in top level nodes and would be handled by one driver.
> > > 
> > > This is really how hardware block looks like. But it is not backward
> > > compatible...  
> > 
> > Sounds good to me. I presume we need the serial child nodes so we can
> > reference them from the stdout-path?  
> 
> Yes, exactly, separate nodes for serial1 and serial2 are still required.
> 
> But dropping clock controller is not possible as for higher baudrates we
> need to use and configure uart clock controller. Without it we just get
> comparable feature support which is already present in driver.

What Stephen means is making clock controller out of the uart node
directly. No need to add separate subnode just for clock controller.

Marek

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> On Sat, 15 Jan 2022 12:50:18 +0100
> Pali Rohár <pali@kernel.org> wrote:
> 
> > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > 
> > > > If I was designing this driver and DTS bindings I would have choose
> > > > something like this:
> > > > 
> > > > uart@0x12000 {  
> > > 
> > > Drop the 0x
> > >   
> > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > >     clock-controller {
> > > >         ...
> > > >     };  
> > > 
> > > Drop this node and put whatever properties are inside into the parent
> > > node.
> > >   
> > > >     serial1 {
> > > >         ...
> > > >         status = "disabled";
> > > >     };
> > > >     serial2 {
> > > >         ...
> > > >         status = "disabled";
> > > >     };
> > > > };
> > > > 
> > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > defined in top level nodes and would be handled by one driver.
> > > > 
> > > > This is really how hardware block looks like. But it is not backward
> > > > compatible...  
> > > 
> > > Sounds good to me. I presume we need the serial child nodes so we can
> > > reference them from the stdout-path?  
> > 
> > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > 
> > But dropping clock controller is not possible as for higher baudrates we
> > need to use and configure uart clock controller. Without it we just get
> > comparable feature support which is already present in driver.
> 
> What Stephen means is making clock controller out of the uart node
> directly. No need to add separate subnode just for clock controller.

This is already implemented in v7 patch series. Clock controller is
already outside of uart nodes.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Stephen Boyd]

Quoting Pali Rohár (2022-01-15 04:26:18)
> On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > On Sat, 15 Jan 2022 12:50:18 +0100
> > Pali Rohár <pali@kernel.org> wrote:
> > 
> > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > 
> > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > something like this:
> > > > > 
> > > > > uart@0x12000 {  
> > > > 
> > > > Drop the 0x
> > > >   
> > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > >     clock-controller {
> > > > >         ...
> > > > >     };  
> > > > 
> > > > Drop this node and put whatever properties are inside into the parent
> > > > node.
> > > >   
> > > > >     serial1 {
> > > > >         ...
> > > > >         status = "disabled";
> > > > >     };
> > > > >     serial2 {
> > > > >         ...
> > > > >         status = "disabled";
> > > > >     };
> > > > > };
> > > > > 
> > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > defined in top level nodes and would be handled by one driver.
> > > > > 
> > > > > This is really how hardware block looks like. But it is not backward
> > > > > compatible...  
> > > > 
> > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > reference them from the stdout-path?  
> > > 
> > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > 
> > > But dropping clock controller is not possible as for higher baudrates we
> > > need to use and configure uart clock controller. Without it we just get
> > > comparable feature support which is already present in driver.
> > 
> > What Stephen means is making clock controller out of the uart node
> > directly. No need to add separate subnode just for clock controller.
> 
> This is already implemented in v7 patch series. Clock controller is
> already outside of uart nodes.

I mean to combine the uart node and the clock-controller node together

	uart-wrapper {
		reg = <0x12000 0x18>, <0x12200 0x30>;
		#clock-cells ...

		serial1 {
			...
		};

		serial2 {
			...
		};
	};

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:
> Quoting Pali Rohár (2022-01-15 04:26:18)
> > On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > > On Sat, 15 Jan 2022 12:50:18 +0100
> > > Pali Rohár <pali@kernel.org> wrote:
> > > 
> > > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > > 
> > > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > > something like this:
> > > > > > 
> > > > > > uart@0x12000 {  
> > > > > 
> > > > > Drop the 0x
> > > > >   
> > > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > > >     clock-controller {
> > > > > >         ...
> > > > > >     };  
> > > > > 
> > > > > Drop this node and put whatever properties are inside into the parent
> > > > > node.
> > > > >   
> > > > > >     serial1 {
> > > > > >         ...
> > > > > >         status = "disabled";
> > > > > >     };
> > > > > >     serial2 {
> > > > > >         ...
> > > > > >         status = "disabled";
> > > > > >     };
> > > > > > };
> > > > > > 
> > > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > > defined in top level nodes and would be handled by one driver.
> > > > > > 
> > > > > > This is really how hardware block looks like. But it is not backward
> > > > > > compatible...  
> > > > > 
> > > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > > reference them from the stdout-path?  
> > > > 
> > > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > > 
> > > > But dropping clock controller is not possible as for higher baudrates we
> > > > need to use and configure uart clock controller. Without it we just get
> > > > comparable feature support which is already present in driver.
> > > 
> > > What Stephen means is making clock controller out of the uart node
> > > directly. No need to add separate subnode just for clock controller.
> > 
> > This is already implemented in v7 patch series. Clock controller is
> > already outside of uart nodes.
> 
> I mean to combine the uart node and the clock-controller node together
> 
> 	uart-wrapper {
> 		reg = <0x12000 0x18>, <0x12200 0x30>;
> 		#clock-cells ...
> 
> 		serial1 {
> 			...
> 		};
> 
> 		serial2 {
> 			...
> 		};
> 	};

Ok, now I see what you mean.

But problem is that this is not backward compatible change. And would
not work per existing DT bindings definitions, which defines how
bootloader should set configured clocks.

As I wrote in emails 3 months ago, this new "proposed" DTS definition is
something which I would have chosen if I had designed this driver and
bindings in past. But that did not happen and different approach is
already widely in used.

To support existing DTS definitions and bootloaders, it is really
required to have current structure backward compatible like it is
defined in current DT bindings document. And my changes in this patch
series are backward compatible.

To change DTS structure, it would be needed to provide uart nodes in DTS
files two times: once in old style (the current one) and second time in
this new style.

But such thing would even more complicate updating driver and it needs
to be implemented.

Plus this would open a question how to define default stdout-path if
there would be 4 serial nodes, where one pair would describe old style
and second pair new style; meaning that 2 cross nodes would describe
same define.

For me this looks like a more complications and I do not see any benefit
from it.

It is really important to break backward compatibility, just to try
having new cleaner API at the cost of having more complications and
requirement for more development and also important maintenance?

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Stephen Boyd]

Quoting Pali Rohár (2022-01-19 16:06:51)
> On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:
> > Quoting Pali Rohár (2022-01-15 04:26:18)
> > > On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > > > On Sat, 15 Jan 2022 12:50:18 +0100
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > > 
> > > > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > > > 
> > > > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > > > something like this:
> > > > > > > 
> > > > > > > uart@0x12000 {  
> > > > > > 
> > > > > > Drop the 0x
> > > > > >   
> > > > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > > > >     clock-controller {
> > > > > > >         ...
> > > > > > >     };  
> > > > > > 
> > > > > > Drop this node and put whatever properties are inside into the parent
> > > > > > node.
> > > > > >   
> > > > > > >     serial1 {
> > > > > > >         ...
> > > > > > >         status = "disabled";
> > > > > > >     };
> > > > > > >     serial2 {
> > > > > > >         ...
> > > > > > >         status = "disabled";
> > > > > > >     };
> > > > > > > };
> > > > > > > 
> > > > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > > > defined in top level nodes and would be handled by one driver.
> > > > > > > 
> > > > > > > This is really how hardware block looks like. But it is not backward
> > > > > > > compatible...  
> > > > > > 
> > > > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > > > reference them from the stdout-path?  
> > > > > 
> > > > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > > > 
> > > > > But dropping clock controller is not possible as for higher baudrates we
> > > > > need to use and configure uart clock controller. Without it we just get
> > > > > comparable feature support which is already present in driver.
> > > > 
> > > > What Stephen means is making clock controller out of the uart node
> > > > directly. No need to add separate subnode just for clock controller.
> > > 
> > > This is already implemented in v7 patch series. Clock controller is
> > > already outside of uart nodes.
> > 
> > I mean to combine the uart node and the clock-controller node together
> > 
> >       uart-wrapper {
> >               reg = <0x12000 0x18>, <0x12200 0x30>;
> >               #clock-cells ...
> > 
> >               serial1 {
> >                       ...
> >               };
> > 
> >               serial2 {
> >                       ...
> >               };
> >       };
> 
> Ok, now I see what you mean.
> 
> But problem is that this is not backward compatible change. And would
> not work per existing DT bindings definitions, which defines how
> bootloader should set configured clocks.
> 
> As I wrote in emails 3 months ago, this new "proposed" DTS definition is
> something which I would have chosen if I had designed this driver and
> bindings in past. But that did not happen and different approach is
> already widely in used.
> 
> To support existing DTS definitions and bootloaders, it is really
> required to have current structure backward compatible like it is
> defined in current DT bindings document. And my changes in this patch
> series are backward compatible.

I'm lost. Is the bootloader the one that's expecting some particular
serial node format and updating something? What is the bootloader doing?

> 
> To change DTS structure, it would be needed to provide uart nodes in DTS
> files two times: once in old style (the current one) and second time in
> this new style.

That's not a good idea. Why do we need to support both at the same time?

> 
> But such thing would even more complicate updating driver and it needs
> to be implemented.
> 
> Plus this would open a question how to define default stdout-path if
> there would be 4 serial nodes, where one pair would describe old style
> and second pair new style; meaning that 2 cross nodes would describe
> same define.

Huh? We shouldn't have both bindings present in the DTB.

> 
> For me this looks like a more complications and I do not see any benefit
> from it.
> 
> It is really important to break backward compatibility, just to try
> having new cleaner API at the cost of having more complications and
> requirement for more development and also important maintenance?

It's important to not make DT nodes have reg properties that overlap.
Maybe this is a DT purist viewpoint and I'm totally off base! I think
Rob did ack this binding already so I must be coming from the wrong
angle.

Nothing prevents register overlap from happening in practice, but it's
good to avoid such a situation as it clearly divides the I/O space by
assigning an address range to a particular device. In this case, we see
the two uarts are really one device, but we need two nodes in DT for
stdout-path, so we make some child nodes and have the driver figure out
which serial port to use for the console.

We shouldn't be adding more nodes to DT to get drivers to probe for
device I/O spaces that have already been described in DT. When this
happens, we learn that some I/O range is actually a combination of
functions, like uart and clks, and thus we should be able to add any
required properties to the existing DT node to support that new feature
that wasn't described before in the binding.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Wednesday 19 January 2022 22:01:47 Stephen Boyd wrote:
> Quoting Pali Rohár (2022-01-19 16:06:51)
> > On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:
> > > Quoting Pali Rohár (2022-01-15 04:26:18)
> > > > On Saturday 15 January 2022 13:05:09 Marek Behún wrote:
> > > > > On Sat, 15 Jan 2022 12:50:18 +0100
> > > > > Pali Rohár <pali@kernel.org> wrote:
> > > > > 
> > > > > > On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:
> > > > > > > Quoting Pali Rohár (2021-10-15 23:42:10)  
> > > > > > > > 
> > > > > > > > If I was designing this driver and DTS bindings I would have choose
> > > > > > > > something like this:
> > > > > > > > 
> > > > > > > > uart@0x12000 {  
> > > > > > > 
> > > > > > > Drop the 0x
> > > > > > >   
> > > > > > > >     reg = <0x12000 0x18>, <0x12200 0x30>;
> > > > > > > >     clock-controller {
> > > > > > > >         ...
> > > > > > > >     };  
> > > > > > > 
> > > > > > > Drop this node and put whatever properties are inside into the parent
> > > > > > > node.
> > > > > > >   
> > > > > > > >     serial1 {
> > > > > > > >         ...
> > > > > > > >         status = "disabled";
> > > > > > > >     };
> > > > > > > >     serial2 {
> > > > > > > >         ...
> > > > > > > >         status = "disabled";
> > > > > > > >     };
> > > > > > > > };
> > > > > > > > 
> > > > > > > > Meaning that 0x12000 node would be 3 subnodes and all registers would be
> > > > > > > > defined in top level nodes and would be handled by one driver.
> > > > > > > > 
> > > > > > > > This is really how hardware block looks like. But it is not backward
> > > > > > > > compatible...  
> > > > > > > 
> > > > > > > Sounds good to me. I presume we need the serial child nodes so we can
> > > > > > > reference them from the stdout-path?  
> > > > > > 
> > > > > > Yes, exactly, separate nodes for serial1 and serial2 are still required.
> > > > > > 
> > > > > > But dropping clock controller is not possible as for higher baudrates we
> > > > > > need to use and configure uart clock controller. Without it we just get
> > > > > > comparable feature support which is already present in driver.
> > > > > 
> > > > > What Stephen means is making clock controller out of the uart node
> > > > > directly. No need to add separate subnode just for clock controller.
> > > > 
> > > > This is already implemented in v7 patch series. Clock controller is
> > > > already outside of uart nodes.
> > > 
> > > I mean to combine the uart node and the clock-controller node together
> > > 
> > >       uart-wrapper {
> > >               reg = <0x12000 0x18>, <0x12200 0x30>;
> > >               #clock-cells ...
> > > 
> > >               serial1 {
> > >                       ...
> > >               };
> > > 
> > >               serial2 {
> > >                       ...
> > >               };
> > >       };
> > 
> > Ok, now I see what you mean.
> > 
> > But problem is that this is not backward compatible change. And would
> > not work per existing DT bindings definitions, which defines how
> > bootloader should set configured clocks.
> > 
> > As I wrote in emails 3 months ago, this new "proposed" DTS definition is
> > something which I would have chosen if I had designed this driver and
> > bindings in past. But that did not happen and different approach is
> > already widely in used.
> > 
> > To support existing DTS definitions and bootloaders, it is really
> > required to have current structure backward compatible like it is
> > defined in current DT bindings document. And my changes in this patch
> > series are backward compatible.
> 
> I'm lost. Is the bootloader the one that's expecting some particular
> serial node format and updating something? What is the bootloader doing?

If bootloader uses or configures UART to different clock it needs to
update "clocks" property in DT. Otherwise UART would be unusable and
there would be no dmesg output.

A3720 heavily depends that bootloader patches at boot time DTB file to
the layout of the current hardware.

> > 
> > To change DTS structure, it would be needed to provide uart nodes in DTS
> > files two times: once in old style (the current one) and second time in
> > this new style.
> 
> That's not a good idea. Why do we need to support both at the same time?

Because old bootloaders do not and will never support this new style. It
is not only linux kernel project who provides DTB files. Also bootloader
itself has own DTB files and use it for booting (e.g kernel). For some
boards is in-kernel-tree DTS file only as a reference. So it is
important that kernel can use and support DTS files from old version and
also from the new patched version. Gregory (A3720 DTS files maintainer)
always ask me what happens if I try to boot new patched kernel drivers
with old unmodified DTS files and wants to know if nothing is broken by
introduced changed.

> > 
> > But such thing would even more complicate updating driver and it needs
> > to be implemented.
> > 
> > Plus this would open a question how to define default stdout-path if
> > there would be 4 serial nodes, where one pair would describe old style
> > and second pair new style; meaning that 2 cross nodes would describe
> > same define.
> 
> Huh? We shouldn't have both bindings present in the DTB.

Ideally yes, I would like to see to prevent it. But for backward
compatibility we really need old bindings still present (as explained
above).

So really I see two options here: Make changes in patches backward
compatible (old nodes stay in DT and also kernel would be able to use
old DT). Or let old bindings untouched in DT and new backward
incompatible definitions would have to be in separate nodes.

> > 
> > For me this looks like a more complications and I do not see any benefit
> > from it.
> > 
> > It is really important to break backward compatibility, just to try
> > having new cleaner API at the cost of having more complications and
> > requirement for more development and also important maintenance?
> 
> It's important to not make DT nodes have reg properties that overlap.
> Maybe this is a DT purist viewpoint and I'm totally off base! I think
> Rob did ack this binding already so I must be coming from the wrong
> angle.

I know this. In case it happens that driver for "one DT node" needs to
access regs of "another DT node" then regmap interface is used and
driver access regs of "another DT node" via regmap. No overlapping is in
DT. But here it is not possible to use regmap as "another DT node" is in
"disabled" state on some boards. And so regmap driver is not bound to
it.

In beginning there was not overlapping in DT because people have not
looked properly that some registers of uart2 are in uart1 space and did
not exported them to driver (bootloader initialized them to some sane
values and nobody noticed that they are required).

This overlapping starting to be required after I properly looked how
driver is working, how it maps to HW and how to implement choosing
clocks and allowing to change baudrate to higher values.

> Nothing prevents register overlap from happening in practice, but it's
> good to avoid such a situation as it clearly divides the I/O space by
> assigning an address range to a particular device. In this case, we see
> the two uarts are really one device, but we need two nodes in DT for
> stdout-path, so we make some child nodes and have the driver figure out
> which serial port to use for the console.
> 
> We shouldn't be adding more nodes to DT to get drivers to probe for
> device I/O spaces that have already been described in DT. When this
> happens, we learn that some I/O range is actually a combination of
> functions, like uart and clks, and thus we should be able to add any
> required properties to the existing DT node to support that new feature
> that wasn't described before in the binding.

Re: [PATCH v7 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Stephen Boyd]

Quoting Pali Rohár (2022-01-20 01:26:41)
> On Wednesday 19 January 2022 22:01:47 Stephen Boyd wrote:
> > > 
> > > Ok, now I see what you mean.
> > > 
> > > But problem is that this is not backward compatible change. And would
> > > not work per existing DT bindings definitions, which defines how
> > > bootloader should set configured clocks.
> > > 
> > > As I wrote in emails 3 months ago, this new "proposed" DTS definition is
> > > something which I would have chosen if I had designed this driver and
> > > bindings in past. But that did not happen and different approach is
> > > already widely in used.
> > > 
> > > To support existing DTS definitions and bootloaders, it is really
> > > required to have current structure backward compatible like it is
> > > defined in current DT bindings document. And my changes in this patch
> > > series are backward compatible.
> > 
> > I'm lost. Is the bootloader the one that's expecting some particular
> > serial node format and updating something? What is the bootloader doing?
> 
> If bootloader uses or configures UART to different clock it needs to
> update "clocks" property in DT. Otherwise UART would be unusable and
> there would be no dmesg output.

Got it! I didn't see that part mentioned anywhere in the commit text
though. To the uninformed reviewer like me it is hard to know about this
bootloader design unless the commit text explains that there's no other
way to do this.

> 
> A3720 heavily depends that bootloader patches at boot time DTB file to
> the layout of the current hardware.
> 
> > > 
> > > To change DTS structure, it would be needed to provide uart nodes in DTS
> > > files two times: once in old style (the current one) and second time in
> > > this new style.
> > 
> > That's not a good idea. Why do we need to support both at the same time?
> 
> Because old bootloaders do not and will never support this new style. It
> is not only linux kernel project who provides DTB files. Also bootloader
> itself has own DTB files and use it for booting (e.g kernel). For some
> boards is in-kernel-tree DTS file only as a reference. So it is
> important that kernel can use and support DTS files from old version and
> also from the new patched version. Gregory (A3720 DTS files maintainer)
> always ask me what happens if I try to boot new patched kernel drivers
> with old unmodified DTS files and wants to know if nothing is broken by
> introduced changed.
> 
> > > 
> > > But such thing would even more complicate updating driver and it needs
> > > to be implemented.
> > > 
> > > Plus this would open a question how to define default stdout-path if
> > > there would be 4 serial nodes, where one pair would describe old style
> > > and second pair new style; meaning that 2 cross nodes would describe
> > > same define.
> > 
> > Huh? We shouldn't have both bindings present in the DTB.
> 
> Ideally yes, I would like to see to prevent it. But for backward
> compatibility we really need old bindings still present (as explained
> above).
> 
> So really I see two options here: Make changes in patches backward
> compatible (old nodes stay in DT and also kernel would be able to use
> old DT). Or let old bindings untouched in DT and new backward
> incompatible definitions would have to be in separate nodes.

Ok I understand now. We have to keep both the serial nodes because the
bootloader is patching them. To make matters worse, one or the other
node may be disabled so we can't even add the new bits to the uart1
node. Can you update the commit text to record this sad state of affairs
and indicate that the only way to support this is to make a new node in
DT that the bootloader doesn't know about?