| Message ID | 20210624224909.6350-1-pali@kernel.org |
|---|---|
| Headers | show |
| Series | serial: mvebu-uart: Fixes and new support for higher baudrates | expand |
This patch series fixes mvebu-uart driver used on Marvell Armada 37xx
boards and add support for baudrates higher than 230400.
In v2 was added patch with DIV_U64_ROUND_CLOSEST helper and changed
patch "implement UART clock driver for configuring UART base clock" to
fix compile errors on 32-bit archs, including usage of new math helper.
Pali Rohár (11):
serial: mvebu-uart: fix calculation of clock divisor
serial: mvebu-uart: do not allow changing baudrate when uartclk is not
available
serial: mvebu-uart: correctly calculate minimal possible baudrate
dt-bindings: mvebu-uart: fix documentation
arm64: dts: marvell: armada-37xx: Fix reg for standard variant of UART
serial: mvebu-uart: remove unused member nb from struct mvebu_uart
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
arm64: dts: marvell: armada-37xx: add device node for UART clock and
use it
serial: mvebu-uart: implement support for baudrates higher than 230400
.../bindings/clock/armada3700-uart-clock.txt | 24 +
.../devicetree/bindings/serial/mvebu-uart.txt | 15 +-
.../arm64/boot/dts/marvell/armada-3720-db.dts | 4 +
.../dts/marvell/armada-3720-espressobin.dtsi | 4 +
.../dts/marvell/armada-3720-turris-mox.dts | 4 +
.../boot/dts/marvell/armada-3720-uDPU.dts | 4 +
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 17 +-
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/mvebu-uart.c | 605 +++++++++++++++++-
include/linux/math64.h | 13 +
10 files changed, 661 insertions(+), 30 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt
Hi Pali, On Fri, Jun 25, 2021 at 4:37 PM Pali Rohár <pali@kernel.org> wrote: > 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> Thanks for your patch! > --- a/include/linux/math64.h > +++ b/include/linux/math64.h > @@ -281,6 +281,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); }) Given "dividend" should already be an unsigned 64-bit value, I don't think the cast to "u64" is needed. Similar macros in this file also don't have the cast. > + > /* > * DIV_S64_ROUND_CLOSEST - signed 64bit divide with 32bit divisor rounded to nearest integer > * @dividend: signed 64bit dividend With the above nit fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote: > Hi Pali, > > On Fri, Jun 25, 2021 at 4:37 PM Pali Rohár <pali@kernel.org> wrote: > > 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> > > Thanks for your patch! > > > --- a/include/linux/math64.h > > +++ b/include/linux/math64.h > > @@ -281,6 +281,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); }) > > Given "dividend" should already be an unsigned 64-bit value, I don't > think the cast to "u64" is needed. Similar macros in this file also > don't have the cast. It is just to ensure that plus operation between dividend and _tmp is evaluated in 64-bit context to prevent overflow. Just a case when user calls this macro with 32-bit dividend param. As it is a macro (and not inline function) type is not automatically enforced. DIV_S64_ROUND_CLOSEST macro assigns its argument into temporary 64-bit variable which then ensures usage of 64-bit arithmetic operations. Same applies for DIV64_U64_ROUND_CLOSEST and DIV64_U64_ROUND_UP macros. So this is reason why I added explicit cast to u64. > > > + > > /* > > * DIV_S64_ROUND_CLOSEST - signed 64bit divide with 32bit divisor rounded to nearest integer > > * @dividend: signed 64bit dividend > > With the above nit fixed: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Fri, Jun 25, 2021 at 05:38:03PM +0200, Pali Rohár wrote: > On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote: > > > +/* > > > + * 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); }) > > > > Given "dividend" should already be an unsigned 64-bit value, I don't > > think the cast to "u64" is needed. Similar macros in this file also > > don't have the cast. > > It is just to ensure that plus operation between dividend and _tmp is > evaluated in 64-bit context to prevent overflow. Just a case when user > calls this macro with 32-bit dividend param. As it is a macro (and not > inline function) type is not automatically enforced. I agree, a large u32 argument added to _tmp/2 could overflow and remain 32 bits, yielding an incorrect result. The cast is mandatory here (and will either emit no code, or be useful). The only trap I'm seeing is if a negative signed int is passed in dividend, it will be sign-extended and will give a large u64 value. A preliminary u32 cast could avoid this but would break valid u64 arguments, and I'd claim we never know what the user wants if this happens in the first place. Willy
Hi Willy, On Fri, Jun 25, 2021 at 5:50 PM Willy Tarreau <w@1wt.eu> wrote: > On Fri, Jun 25, 2021 at 05:38:03PM +0200, Pali Rohár wrote: > > On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote: > > > > +/* > > > > + * 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); }) > > > > > > Given "dividend" should already be an unsigned 64-bit value, I don't > > > think the cast to "u64" is needed. Similar macros in this file also > > > don't have the cast. > > > > It is just to ensure that plus operation between dividend and _tmp is > > evaluated in 64-bit context to prevent overflow. Just a case when user > > calls this macro with 32-bit dividend param. As it is a macro (and not > > inline function) type is not automatically enforced. > > I agree, a large u32 argument added to _tmp/2 could overflow and remain > 32 bits, yielding an incorrect result. The cast is mandatory here (and > will either emit no code, or be useful). Fair enough. So we want to add a cast to DIV64_U64_ROUND_CLOSEST() above, too? > The only trap I'm seeing is if a negative signed int is passed in dividend, > it will be sign-extended and will give a large u64 value. A preliminary > u32 cast could avoid this but would break valid u64 arguments, and I'd > claim we never know what the user wants if this happens in the first place. Yep. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Friday 25 June 2021 19:39:10 Geert Uytterhoeven wrote: > Hi Willy, > > On Fri, Jun 25, 2021 at 5:50 PM Willy Tarreau <w@1wt.eu> wrote: > > On Fri, Jun 25, 2021 at 05:38:03PM +0200, Pali Rohár wrote: > > > On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote: > > > > > +/* > > > > > + * 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); }) > > > > > > > > Given "dividend" should already be an unsigned 64-bit value, I don't > > > > think the cast to "u64" is needed. Similar macros in this file also > > > > don't have the cast. > > > > > > It is just to ensure that plus operation between dividend and _tmp is > > > evaluated in 64-bit context to prevent overflow. Just a case when user > > > calls this macro with 32-bit dividend param. As it is a macro (and not > > > inline function) type is not automatically enforced. > > > > I agree, a large u32 argument added to _tmp/2 could overflow and remain > > 32 bits, yielding an incorrect result. The cast is mandatory here (and > > will either emit no code, or be useful). > > Fair enough. > So we want to add a cast to DIV64_U64_ROUND_CLOSEST() above, too? For DIV64_U64_ROUND_CLOSEST() it is not needed. divisor is copied into u64 _tmp variable and therefore "(dividend) + _tmp / 2" is already evaluated in 64-bit context even when dividend is only 32-bit. The only trap is that negative value as written below. > > The only trap I'm seeing is if a negative signed int is passed in dividend, > > it will be sign-extended and will give a large u64 value. A preliminary > > u32 cast could avoid this but would break valid u64 arguments, and I'd > > claim we never know what the user wants if this happens in the first place. > > Yep. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Pali, On Fri, Jun 25, 2021 at 7:44 PM Pali Rohár <pali@kernel.org> wrote: > On Friday 25 June 2021 19:39:10 Geert Uytterhoeven wrote: > > On Fri, Jun 25, 2021 at 5:50 PM Willy Tarreau <w@1wt.eu> wrote: > > > On Fri, Jun 25, 2021 at 05:38:03PM +0200, Pali Rohár wrote: > > > > On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote: > > > > > > +/* > > > > > > + * 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); }) > > > > > > > > > > Given "dividend" should already be an unsigned 64-bit value, I don't > > > > > think the cast to "u64" is needed. Similar macros in this file also > > > > > don't have the cast. > > > > > > > > It is just to ensure that plus operation between dividend and _tmp is > > > > evaluated in 64-bit context to prevent overflow. Just a case when user > > > > calls this macro with 32-bit dividend param. As it is a macro (and not > > > > inline function) type is not automatically enforced. > > > > > > I agree, a large u32 argument added to _tmp/2 could overflow and remain > > > 32 bits, yielding an incorrect result. The cast is mandatory here (and > > > will either emit no code, or be useful). > > > > Fair enough. > > So we want to add a cast to DIV64_U64_ROUND_CLOSEST() above, too? > > For DIV64_U64_ROUND_CLOSEST() it is not needed. divisor is copied into > u64 _tmp variable and therefore "(dividend) + _tmp / 2" is already > evaluated in 64-bit context even when dividend is only 32-bit. Thanks, I stand corrected. Time to enter weekend mode... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Sat, Jul 17, 2021 at 02:38:26PM +0200, Pali Rohár wrote: > @@ -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); Hi Pali You only need spin_lock_irqsave() if you plan on taking the spinlock in an interrupt handler. It seems unlikely the baud rate will be changed in interrupt context? Please check, and then swap to plain spin_lock(). > 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; > + /* 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; > + } I don't see any range checks in the patch which verifies the requested baud rate is actually possible. With code like this, it seems like the baud rate change will be successful, but the actual baud rate will not be what is requested. > + /* 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); Here it looks like UART1 could request a baud rate change, which ends up setting the clocks so that UART2 is out of range? Could the change for UART1 be successful, but you end up breaking UART2? I'm thinking when you are at opposite ends of the scale. UART2 is running at 110baud and UART1 at 230400baud. Andrew
On Sat, Jul 17, 2021 at 02:38:27PM +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> > --- > .../bindings/clock/armada3700-uart-clock.txt | 24 +++++++++++++++++++ > .../devicetree/bindings/serial/mvebu-uart.txt | 9 ++++--- > 2 files changed, 30 insertions(+), 3 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt > > diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt > new file mode 100644 > index 000000000000..144bc6d7eae8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt Since this is a new binding, please use YAML. Andrew
On Saturday 17 July 2021 19:26:51 Andrew Lunn wrote: > On Sat, Jul 17, 2021 at 02:38:26PM +0200, Pali Rohár wrote: > > @@ -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); > > Hi Pali > > You only need spin_lock_irqsave() if you plan on taking the spinlock > in an interrupt handler. It seems unlikely the baud rate will be > changed in interrupt context? Please check, and then swap to plain > spin_lock(). Hello! Ok, I will check it. > > 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; > > > + /* 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; > > + } > > I don't see any range checks in the patch which verifies the requested > baud rate is actually possible. With code like this, it seems like the > baud rate change will be successful, but the actual baud rate will not > be what is requested. This code is in function which changes parent UART clock from one used by bootloader to clock which will be used by kernel UART driver. Yes, it is possible if you configure something unusual in bootloader that that this code breaks it. But I think there is not so much what we can done here. In other patches is updated function mvebu_uart_set_termios() which verifies that you can set particular baudrate. > > + /* 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); > > Here it looks like UART1 could request a baud rate change, which ends > up setting the clocks so that UART2 is out of range? Could the change > for UART1 be successful, but you end up breaking UART2? I'm thinking > when you are at opposite ends of the scale. UART2 is running at > 110baud and UART1 at 230400baud. This code is also in function which just do one time change of UART parent clock. Once clk driver is probed this parent clock (and its d1 and d2 divisors) are not changed anymore. Parent clock and divisors are chosen in way that kernel can always configure minimal baudrate 9600 on both UARTs. You are right that some combinations are not possible. But with these patches it is fixed what is supported at clk driver probe time. In v3 patch 5/5 is described how to calculate final baudrate from parent clock and divisors d1, d2, d, m1, m2, m3, m4. Note that parent clock and divisors d1 and d2 are shared for both UARTs. Other parameters (d, m1, m2, m3, m4) can be set differently both UART1 and UART2. Changing shared values is not possible during usage of UART. If you have any idea how to improve current implementation, please let me know. Also note that all A3720 boards have disabled UART2 in DTS. And I'm not sure if there is somebody who uses UART2 or who uses both UARTs.
On Fri, Jun 25, 2021 at 6:39 PM Pali Rohár <pali@kernel.org> wrote: > On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote: > > On Fri, Jun 25, 2021 at 4:37 PM Pali Rohár <pali@kernel.org> wrote: ... > > > +/* > > > + * DIV_U64_ROUND_CLOSEST - unsigned 64bit divide with 32bit divisor rounded to nearest integer > > > + * @dividend: unsigned 64bit dividend (1) > > > + * @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); }) > > > > Given "dividend" should already be an unsigned 64-bit value, I don't > > think the cast to "u64" is needed. Similar macros in this file also > > don't have the cast. > > It is just to ensure that plus operation between dividend and _tmp is > evaluated in 64-bit context to prevent overflow. Just a case when user > calls this macro with 32-bit dividend param. This contradicts (1). > As it is a macro (and not > inline function) type is not automatically enforced. > > DIV_S64_ROUND_CLOSEST macro assigns its argument into temporary 64-bit > variable which then ensures usage of 64-bit arithmetic operations. Same > applies for DIV64_U64_ROUND_CLOSEST and DIV64_U64_ROUND_UP macros. > > So this is reason why I added explicit cast to u64. I don't see the reason for casting in the current code. Probably you need to rephrase documentation to explain why it's there. -- With Best Regards, Andy Shevchenko
On Saturday 17 July 2021 20:05:40 Pali Rohár wrote: > On Saturday 17 July 2021 19:26:51 Andrew Lunn wrote: > > On Sat, Jul 17, 2021 at 02:38:26PM +0200, Pali Rohár wrote: > > > @@ -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); > > > > Hi Pali > > > > You only need spin_lock_irqsave() if you plan on taking the spinlock > > in an interrupt handler. It seems unlikely the baud rate will be > > changed in interrupt context? Please check, and then swap to plain > > spin_lock(). > > Hello! Ok, I will check it. Well, driver is already using spin_lock_irqsave() in all other functions. And in linux/clk-provider.h is documented that drivers can call clk_enable() from an interrupt, so it means that spin_lock_irqsave() is really needed for mvebu_uart_lock. > > > 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; > > > > > + /* 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; > > > + } > > > > I don't see any range checks in the patch which verifies the requested > > baud rate is actually possible. With code like this, it seems like the > > baud rate change will be successful, but the actual baud rate will not > > be what is requested. > > This code is in function which changes parent UART clock from one used > by bootloader to clock which will be used by kernel UART driver. > > Yes, it is possible if you configure something unusual in bootloader > that that this code breaks it. But I think there is not so much what we > can done here. > > In other patches is updated function mvebu_uart_set_termios() which > verifies that you can set particular baudrate. > > > > + /* 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); > > > > Here it looks like UART1 could request a baud rate change, which ends > > up setting the clocks so that UART2 is out of range? Could the change > > for UART1 be successful, but you end up breaking UART2? I'm thinking > > when you are at opposite ends of the scale. UART2 is running at > > 110baud and UART1 at 230400baud. > > This code is also in function which just do one time change of UART > parent clock. Once clk driver is probed this parent clock (and its d1 > and d2 divisors) are not changed anymore. Parent clock and divisors are > chosen in way that kernel can always configure minimal baudrate 9600 on > both UARTs. > > You are right that some combinations are not possible. But with these > patches it is fixed what is supported at clk driver probe time. > > In v3 patch 5/5 is described how to calculate final baudrate from parent > clock and divisors d1, d2, d, m1, m2, m3, m4. Note that parent clock and > divisors d1 and d2 are shared for both UARTs. Other parameters (d, m1, > m2, m3, m4) can be set differently both UART1 and UART2. Changing shared > values is not possible during usage of UART. > > If you have any idea how to improve current implementation, please let > me know. > > Also note that all A3720 boards have disabled UART2 in DTS. And I'm not > sure if there is somebody who uses UART2 or who uses both UARTs.
On Sat, Jul 24, 2021 at 11:48:16AM +0200, Pali Rohár wrote: > On Saturday 17 July 2021 20:05:40 Pali Rohár wrote: > > On Saturday 17 July 2021 19:26:51 Andrew Lunn wrote: > > > On Sat, Jul 17, 2021 at 02:38:26PM +0200, Pali Rohár wrote: > > > > @@ -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); > > > > > > Hi Pali > > > > > > You only need spin_lock_irqsave() if you plan on taking the spinlock > > > in an interrupt handler. It seems unlikely the baud rate will be > > > changed in interrupt context? Please check, and then swap to plain > > > spin_lock(). > > > > Hello! Ok, I will check it. > > Well, driver is already using spin_lock_irqsave() in all other > functions. And some of those functions are called from interrupt context i expect. For each lock you have, you need to decide if interrupt context is an issue or not. spin_lock_irqsave() is more expansive, since it has to disable interrupts, etc. It can upset real time latency etc. So in the hot path, you want to try to avoid it, unless you actually need it. But changing the baud rate is not the hot path, it hardly every happens, so we can live with the unneeded overhead. > And in linux/clk-provider.h is documented that drivers can call > clk_enable() from an interrupt, so it means that spin_lock_irqsave() is > really needed for mvebu_uart_lock. Sure, drivers can. But in this case, does a driver actually do that? Does it change the baud rate in interrupt context? > > In other patches is updated function mvebu_uart_set_termios() which > > verifies that you can set particular baudrate. Great. It is not clear from the patches or the commit message that this has been considered. It is something worth mentioning, just to avoid questions. > > Also note that all A3720 boards have disabled UART2 in DTS. And I'm not > > sure if there is somebody who uses UART2 or who uses both UARTs. That does not really matter. You should not regression a feature because you think nobody is using it. Andrew
On Saturday 24 July 2021 18:33:33 Andrew Lunn wrote: > On Sat, Jul 24, 2021 at 11:48:16AM +0200, Pali Rohár wrote: > > On Saturday 17 July 2021 20:05:40 Pali Rohár wrote: > > > On Saturday 17 July 2021 19:26:51 Andrew Lunn wrote: > > > > On Sat, Jul 17, 2021 at 02:38:26PM +0200, Pali Rohár wrote: > > > > > @@ -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); > > > > > > > > Hi Pali > > > > > > > > You only need spin_lock_irqsave() if you plan on taking the spinlock > > > > in an interrupt handler. It seems unlikely the baud rate will be > > > > changed in interrupt context? Please check, and then swap to plain > > > > spin_lock(). > > > > > > Hello! Ok, I will check it. > > > > Well, driver is already using spin_lock_irqsave() in all other > > functions. > > And some of those functions are called from interrupt context i > expect. For each lock you have, you need to decide if interrupt > context is an issue or not. spin_lock_irqsave() is more expansive, > since it has to disable interrupts, etc. It can upset real time > latency etc. So in the hot path, you want to try to avoid it, unless > you actually need it. But changing the baud rate is not the hot path, > it hardly every happens, so we can live with the unneeded overhead. It happens either one time during "device" probing (e.g. when connected bluetooth UART device want to use higher baudrate) or when user explicitly ask to change baudrate (e.g. when want to transfer files over UART via x/y/z-modem / kermit protocol). Or maybe if somebody wants to establish and use PPP network over UART. So it should not be a problem. > > And in linux/clk-provider.h is documented that drivers can call > > clk_enable() from an interrupt, so it means that spin_lock_irqsave() is > > really needed for mvebu_uart_lock. > > Sure, drivers can. But in this case, does a driver actually do that? > Does it change the baud rate in interrupt context? Looks like that changing baudrate not. But other places where this lock is used (e.g. in clk callbacks) can be called from interrupt context. But for baudrate change, I think it is not so common action, so there should not be issue with slightly higher overhead. > > > In other patches is updated function mvebu_uart_set_termios() which > > > verifies that you can set particular baudrate. > > Great. It is not clear from the patches or the commit message that > this has been considered. It is something worth mentioning, just to > avoid questions. This check was there also prior my patches. I only "extended" it to match what is supported by this patch series. > > > Also note that all A3720 boards have disabled UART2 in DTS. And I'm not > > > sure if there is somebody who uses UART2 or who uses both UARTs. > > That does not really matter. You should not regression a feature > because you think nobody is using it. I know. That is why I introduced code which recalculates divisors registers to not change baudrate of UART2 during loading of UART1 and also introduction of this clk subdriver with locks to prevent any regressions.
On Saturday 17 July 2021 19:30:19 Andrew Lunn wrote: > On Sat, Jul 17, 2021 at 02:38:27PM +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> > > --- > > .../bindings/clock/armada3700-uart-clock.txt | 24 +++++++++++++++++++ > > .../devicetree/bindings/serial/mvebu-uart.txt | 9 ++++--- > > 2 files changed, 30 insertions(+), 3 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt > > > > diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt > > new file mode 100644 > > index 000000000000..144bc6d7eae8 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt > > Since this is a new binding, please use YAML. Changed in v4.
This patch series fixes mvebu-uart driver used on Marvell Armada 37xx boards and add support for baudrates higher than 230400. Pali Rohár (10): serial: mvebu-uart: fix calculation of clock divisor serial: mvebu-uart: do not allow changing baudrate when uartclk is not available serial: mvebu-uart: correctly calculate minimal possible baudrate dt-bindings: mvebu-uart: fix documentation arm64: dts: marvell: armada-37xx: Fix reg for standard variant of UART serial: mvebu-uart: remove unused member nb from struct mvebu_uart 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 arm64: dts: marvell: armada-37xx: add device node for UART clock and use it serial: mvebu-uart: implement support for baudrates higher than 230400 .../bindings/clock/armada3700-uart-clock.txt | 24 + .../devicetree/bindings/serial/mvebu-uart.txt | 15 +- .../arm64/boot/dts/marvell/armada-3720-db.dts | 4 + .../dts/marvell/armada-3720-espressobin.dtsi | 4 + .../dts/marvell/armada-3720-turris-mox.dts | 4 + .../boot/dts/marvell/armada-3720-uDPU.dts | 4 + arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 17 +- drivers/tty/serial/mvebu-uart.c | 603 +++++++++++++++++- 8 files changed, 645 insertions(+), 30 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/armada3700-uart-clock.txt