From: "Pali Rohár" <pali@kernel.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Michael Turquette" <mturquette@baylibre.com>,
"Stephen Boyd" <sboyd@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Gregory Clement" <gregory.clement@bootlin.com>,
"Sebastian Hesselbarth" <sebastian.hesselbarth@gmail.com>,
"Vladimir Vid" <vladimir.vid@sartura.hr>,
"Marek Behún" <kabel@kernel.org>,
"Geert Uytterhoeven" <geert@linux-m68k.org>,
linux-clk@vger.kernel.org, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/5] serial: mvebu-uart: implement UART clock driver for configuring UART base clock
Date: Sat, 24 Jul 2021 11:48:16 +0200 [thread overview]
Message-ID: <20210724094816.2y3peclaftx26kwj@pali> (raw)
In-Reply-To: <20210717180540.ersg5bslik6ivjie@pali>
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.
next prev parent reply other threads:[~2021-07-24 9:48 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 22:48 [PATCH 00/10] serial: mvebu-uart: Fixes and new support for higher baudrates Pali Rohár
2021-06-24 22:49 ` [PATCH 01/10] serial: mvebu-uart: fix calculation of clock divisor Pali Rohár
2021-06-24 22:49 ` [PATCH 02/10] serial: mvebu-uart: do not allow changing baudrate when uartclk is not available Pali Rohár
2021-06-24 22:49 ` [PATCH 03/10] serial: mvebu-uart: correctly calculate minimal possible baudrate Pali Rohár
2021-06-24 22:49 ` [PATCH 04/10] dt-bindings: mvebu-uart: fix documentation Pali Rohár
2021-06-24 22:49 ` [PATCH 05/10] arm64: dts: marvell: armada-37xx: Fix reg for standard variant of UART Pali Rohár
2021-06-24 22:49 ` [PATCH 06/10] serial: mvebu-uart: remove unused member nb from struct mvebu_uart Pali Rohár
2021-06-24 22:49 ` [PATCH 07/10] serial: mvebu-uart: implement UART clock driver for configuring UART base clock Pali Rohár
2021-06-24 22:49 ` [PATCH 08/10] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock Pali Rohár
2021-06-24 22:49 ` [PATCH 09/10] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it Pali Rohár
2021-06-24 22:49 ` [PATCH 10/10] serial: mvebu-uart: implement support for baudrates higher than 230400 Pali Rohár
2021-06-25 14:36 ` [PATCH v2 00/11] serial: mvebu-uart: Fixes and new support for higher baudrates Pali Rohár
2021-06-25 14:36 ` [PATCH v2 01/11] serial: mvebu-uart: fix calculation of clock divisor Pali Rohár
2021-06-25 14:36 ` [PATCH v2 02/11] serial: mvebu-uart: do not allow changing baudrate when uartclk is not available Pali Rohár
2021-06-25 14:36 ` [PATCH v2 03/11] serial: mvebu-uart: correctly calculate minimal possible baudrate Pali Rohár
2021-06-25 14:36 ` [PATCH v2 04/11] dt-bindings: mvebu-uart: fix documentation Pali Rohár
2021-06-25 14:36 ` [PATCH v2 05/11] arm64: dts: marvell: armada-37xx: Fix reg for standard variant of UART Pali Rohár
2021-06-25 14:36 ` [PATCH v2 06/11] serial: mvebu-uart: remove unused member nb from struct mvebu_uart Pali Rohár
2021-06-25 14:36 ` [PATCH v2 07/11] math64: New DIV_U64_ROUND_CLOSEST helper Pali Rohár
2021-06-25 15:22 ` Geert Uytterhoeven
2021-06-25 15:38 ` Pali Rohár
2021-06-25 15:50 ` Willy Tarreau
2021-06-25 17:39 ` Geert Uytterhoeven
2021-06-25 17:44 ` Pali Rohár
2021-06-25 18:11 ` Geert Uytterhoeven
2021-07-19 12:45 ` Andy Shevchenko
2021-06-25 14:36 ` [PATCH v2 08/11] serial: mvebu-uart: implement UART clock driver for configuring UART base clock Pali Rohár
2021-06-25 14:36 ` [PATCH v2 09/11] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock Pali Rohár
2021-06-25 14:36 ` [PATCH v2 10/11] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it Pali Rohár
2021-06-25 14:36 ` [PATCH v2 11/11] serial: mvebu-uart: implement support for baudrates higher than 230400 Pali Rohár
2021-07-17 12:38 ` [PATCH v3 0/5] serial: mvebu-uart: Support for higher baudrates Pali Rohár
2021-07-17 12:38 ` [PATCH v3 1/5] math64: New DIV_U64_ROUND_CLOSEST helper Pali Rohár
2021-07-19 12:47 ` Andy Shevchenko
2021-07-22 21:57 ` Pali Rohár
2021-07-24 11:38 ` David Laight
2021-07-17 12:38 ` [PATCH v3 2/5] serial: mvebu-uart: implement UART clock driver for configuring UART base clock Pali Rohár
2021-07-17 17:26 ` Andrew Lunn
2021-07-17 18:05 ` Pali Rohár
2021-07-24 9:48 ` Pali Rohár [this message]
2021-07-24 16:33 ` Andrew Lunn
2021-07-25 12:14 ` Pali Rohár
2021-07-17 12:38 ` [PATCH v3 3/5] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock Pali Rohár
2021-07-17 17:30 ` [PATCH v3 3/5] dt-bindings: mvebu-uart: document DT bindings for marvell, armada-3700-uart-clock Andrew Lunn
2021-08-02 14:45 ` Pali Rohár
2021-07-17 12:38 ` [PATCH v3 4/5] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it Pali Rohár
2021-07-17 12:38 ` [PATCH v3 5/5] serial: mvebu-uart: implement support for baudrates higher than 230400 Pali Rohár
2021-08-02 14:45 ` [PATCH v4 0/6] serial: mvebu-uart: Support for higher baudrates Pali Rohár
2021-08-02 14:45 ` [PATCH v4 1/6] math64: New DIV_U64_ROUND_CLOSEST helper Pali Rohár
2021-08-02 14:45 ` [PATCH v4 2/6] serial: mvebu-uart: implement UART clock driver for configuring UART base clock Pali Rohár
2021-08-02 14:45 ` [PATCH v4 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock Pali Rohár
2021-08-06 0:30 ` Stephen Boyd
2021-08-06 8:28 ` Pali Rohár
2021-08-12 19:33 ` Stephen Boyd
2021-08-12 20:08 ` Pali Rohár
2021-08-02 14:45 ` [PATCH v4 4/6] dt-bindings: mvebu-uart: update information about UART clock Pali Rohár
2021-08-02 14:45 ` [PATCH v4 5/6] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it Pali Rohár
2021-08-02 14:45 ` [PATCH v4 6/6] serial: mvebu-uart: implement support for baudrates higher than 230400 Pali Rohár
2021-08-09 14:53 ` [PATCH v5 0/6] serial: mvebu-uart: Support for higher baudrates Pali Rohár
2021-08-09 14:53 ` [PATCH v5 1/6] math64: New DIV_U64_ROUND_CLOSEST helper Pali Rohár
2021-08-09 14:53 ` [PATCH v5 2/6] serial: mvebu-uart: implement UART clock driver for configuring UART base clock Pali Rohár
2021-08-09 14:53 ` [PATCH v5 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock Pali Rohár
2021-08-09 14:53 ` [PATCH v5 4/6] dt-bindings: mvebu-uart: update information about UART clock Pali Rohár
2021-08-09 14:53 ` [PATCH v5 5/6] arm64: dts: marvell: armada-37xx: add device node for UART clock and use it Pali Rohár
2021-08-09 14:53 ` [PATCH v5 6/6] serial: mvebu-uart: implement support for baudrates higher than 230400 Pali Rohár
2021-08-20 17:22 ` [PATCH v5 0/6] serial: mvebu-uart: Support for higher baudrates Pali Rohár
2021-08-31 8:35 ` Pali Rohár
2021-09-21 7:41 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210724094816.2y3peclaftx26kwj@pali \
--to=pali@kernel.org \
--cc=andrew@lunn.ch \
--cc=geert@linux-m68k.org \
--cc=gregkh@linuxfoundation.org \
--cc=gregory.clement@bootlin.com \
--cc=kabel@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=sebastian.hesselbarth@gmail.com \
--cc=vladimir.vid@sartura.hr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).