serial: 8250_mtk: Fix high-speed baud rates clamping

Commit Message

Serge Semin July 14, 2020, 12:41 p.m. UTC

Commit 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250
port") fixed limits of a baud rate setting for a generic 8250 port.
In other words since that commit the baud rate has been permitted to be
within [uartclk / 16 / UART_DIV_MAX; uartclk / 16], which is absolutely
normal for a standard 8250 UART port. But there are custom 8250 ports,
which provide extended baud rate limits. In particular the Mediatek 8250
port can work with baud rates up to "uartclk" speed.

Normally that and any other peculiarity is supposed to be handled in a
custom set_termios() callback implemented in the vendor-specific
8250-port glue-driver. Currently that is how it's done for the most of
the vendor-specific 8250 ports, but for some reason for Mediatek a
solution has been spread out to both the glue-driver and to the generic
8250-port code. Due to that a bug has been introduced, which permitted the
extended baud rate limit for all even for standard 8250-ports. The bug
has been fixed by the commit 7b668c064ec3 ("serial: 8250: Fix max baud
limit in generic 8250 port") by narrowing the baud rates limit back down to
the normal bounds. Unfortunately by doing so we also broke the
Mediatek-specific extended bauds feature.

A fix of the problem described above is twofold. First since we can't get
back the extended baud rate limits feature to the generic set_termios()
function and that method supports only a standard baud rates range, the
requested baud rate must be locally stored before calling it and then
restored back to the new termios structure after the generic set_termios()
finished its magic business. By doing so we still use the
serial8250_do_set_termios() method to set the LCR/MCR/FCR/etc. registers,
while the extended baud rate setting procedure will be performed later in
the custom Mediatek-specific set_termios() callback. Second since a true
baud rate is now fully calculated in the custom set_termios() method we
need to locally update the port timeout by calling the
uart_update_timeout() function. After the fixes described above are
implemented in the 8250_mtk.c driver, the Mediatek 8250-port should
get back to normally working with extended baud rates.

Link: https://lore.kernel.org/linux-serial/[e-mail]

Fixes: 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250 port")
Reported-by: Daniel Winkler <[e-mail]>
Signed-off-by: Serge Semin <[e-mail]>

---

Folks, sorry for a delay with the problem fix. A solution is turned out to
be a bit more complicated than I originally thought in my comment to the
Daniel revert-patch.

Please also note, that I don't have a Mediatek hardware to test the
solution suggested in the patch. The code is written as on so called
the tip of the pen after digging into the 8250_mtk.c and 8250_port.c
drivers code. So please Daniel or someone with Mediatek 8250-port
available on a board test this patch first and report about the results in
reply to this emailing thread. After that, if your conclusion is positive
and there is no objection against the solution design the patch can be
merged in.

Cc: Alexey Malahov <[e-mail]>
Cc: Daniel Winkler <[e-mail]>
Cc: Aaron Sierra <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Lukas Wunner <[e-mail]>
Cc: Vignesh Raghavendra <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: BlueZ <[e-mail]>
Cc: chromeos-bluetooth-upstreaming <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/tty/serial/8250/8250_mtk.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Daniel Winkler July 14, 2020, 8:45 p.m. UTC | #1

Thank you Sergey for looking into this. Adding folks working on this
platform to perform validation of the proposed patch.

Best,
Daniel

On Tue, Jul 14, 2020 at 5:41 AM Serge Semin
<[e-mail]> wrote:
>
> Commit 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250
> port") fixed limits of a baud rate setting for a generic 8250 port.
> In other words since that commit the baud rate has been permitted to be
> within [uartclk / 16 / UART_DIV_MAX; uartclk / 16], which is absolutely
> normal for a standard 8250 UART port. But there are custom 8250 ports,
> which provide extended baud rate limits. In particular the Mediatek 8250
> port can work with baud rates up to "uartclk" speed.
>
> Normally that and any other peculiarity is supposed to be handled in a
> custom set_termios() callback implemented in the vendor-specific
> 8250-port glue-driver. Currently that is how it's done for the most of
> the vendor-specific 8250 ports, but for some reason for Mediatek a
> solution has been spread out to both the glue-driver and to the generic
> 8250-port code. Due to that a bug has been introduced, which permitted the
> extended baud rate limit for all even for standard 8250-ports. The bug
> has been fixed by the commit 7b668c064ec3 ("serial: 8250: Fix max baud
> limit in generic 8250 port") by narrowing the baud rates limit back down to
> the normal bounds. Unfortunately by doing so we also broke the
> Mediatek-specific extended bauds feature.
>
> A fix of the problem described above is twofold. First since we can't get
> back the extended baud rate limits feature to the generic set_termios()
> function and that method supports only a standard baud rates range, the
> requested baud rate must be locally stored before calling it and then
> restored back to the new termios structure after the generic set_termios()
> finished its magic business. By doing so we still use the
> serial8250_do_set_termios() method to set the LCR/MCR/FCR/etc. registers,
> while the extended baud rate setting procedure will be performed later in
> the custom Mediatek-specific set_termios() callback. Second since a true
> baud rate is now fully calculated in the custom set_termios() method we
> need to locally update the port timeout by calling the
> uart_update_timeout() function. After the fixes described above are
> implemented in the 8250_mtk.c driver, the Mediatek 8250-port should
> get back to normally working with extended baud rates.
>
> Link: https://lore.kernel.org/linux-serial/[e-mail]
>
> Fixes: 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250 port")
> Reported-by: Daniel Winkler <[e-mail]>
> Signed-off-by: Serge Semin <[e-mail]>
>
> ---
>
> Folks, sorry for a delay with the problem fix. A solution is turned out to
> be a bit more complicated than I originally thought in my comment to the
> Daniel revert-patch.
>
> Please also note, that I don't have a Mediatek hardware to test the
> solution suggested in the patch. The code is written as on so called
> the tip of the pen after digging into the 8250_mtk.c and 8250_port.c
> drivers code. So please Daniel or someone with Mediatek 8250-port
> available on a board test this patch first and report about the results in
> reply to this emailing thread. After that, if your conclusion is positive
> and there is no objection against the solution design the patch can be
> merged in.
>
> Cc: Alexey Malahov <[e-mail]>
> Cc: Daniel Winkler <[e-mail]>
> Cc: Aaron Sierra <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Lukas Wunner <[e-mail]>
> Cc: Vignesh Raghavendra <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: BlueZ <[e-mail]>
> Cc: chromeos-bluetooth-upstreaming <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/tty/serial/8250/8250_mtk.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index f839380c2f4c..98b8a3e30733 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -306,8 +306,21 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
>         }
>  #endif
>
> +       /*
> +        * Store the requested baud rate before calling the generic 8250
> +        * set_termios method. Standard 8250 port expects bauds to be
> +        * no higher than (uartclk / 16) so the baud will be clamped if it
> +        * gets out of that bound. Mediatek 8250 port supports speed
> +        * higher than that, therefore we'll get original baud rate back
> +        * after calling the generic set_termios method and recalculate
> +        * the speed later in this method.
> +        */
> +       baud = tty_termios_baud_rate(termios);
> +
>         serial8250_do_set_termios(port, termios, old);
>
> +       tty_termios_encode_baud_rate(termios, baud, baud);
> +
>         /*
>          * Mediatek UARTs use an extra highspeed register (MTK_UART_HIGHS)
>          *
> @@ -339,6 +352,11 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
>          */
>         spin_lock_irqsave(&port->lock, flags);
>
> +       /*
> +        * Update the per-port timeout.
> +        */
> +       uart_update_timeout(port, termios->c_cflag, baud);
> +
>         /* set DLAB we have cval saved in up->lcr from the call to the core */
>         serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
>         serial_dl_write(up, quot);
> --
> 2.26.2
>

Claire Chang July 15, 2020, 5:36 a.m. UTC | #2

On Wed, Jul 15, 2020 at 4:45 AM Daniel Winkler <[e-mail]> wrote:
>
> Thank you Sergey for looking into this. Adding folks working on this
> platform to perform validation of the proposed patch.
>
> Best,
> Daniel
>
> On Tue, Jul 14, 2020 at 5:41 AM Serge Semin
> <[e-mail]> wrote:
> >
> > Commit 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250
> > port") fixed limits of a baud rate setting for a generic 8250 port.
> > In other words since that commit the baud rate has been permitted to be
> > within [uartclk / 16 / UART_DIV_MAX; uartclk / 16], which is absolutely
> > normal for a standard 8250 UART port. But there are custom 8250 ports,
> > which provide extended baud rate limits. In particular the Mediatek 8250
> > port can work with baud rates up to "uartclk" speed.
> >
> > Normally that and any other peculiarity is supposed to be handled in a
> > custom set_termios() callback implemented in the vendor-specific
> > 8250-port glue-driver. Currently that is how it's done for the most of
> > the vendor-specific 8250 ports, but for some reason for Mediatek a
> > solution has been spread out to both the glue-driver and to the generic
> > 8250-port code. Due to that a bug has been introduced, which permitted the
> > extended baud rate limit for all even for standard 8250-ports. The bug
> > has been fixed by the commit 7b668c064ec3 ("serial: 8250: Fix max baud
> > limit in generic 8250 port") by narrowing the baud rates limit back down to
> > the normal bounds. Unfortunately by doing so we also broke the
> > Mediatek-specific extended bauds feature.
> >
> > A fix of the problem described above is twofold. First since we can't get
> > back the extended baud rate limits feature to the generic set_termios()
> > function and that method supports only a standard baud rates range, the
> > requested baud rate must be locally stored before calling it and then
> > restored back to the new termios structure after the generic set_termios()
> > finished its magic business. By doing so we still use the
> > serial8250_do_set_termios() method to set the LCR/MCR/FCR/etc. registers,
> > while the extended baud rate setting procedure will be performed later in
> > the custom Mediatek-specific set_termios() callback. Second since a true
> > baud rate is now fully calculated in the custom set_termios() method we
> > need to locally update the port timeout by calling the
> > uart_update_timeout() function. After the fixes described above are
> > implemented in the 8250_mtk.c driver, the Mediatek 8250-port should
> > get back to normally working with extended baud rates.
> >
> > Link: https://lore.kernel.org/linux-serial/[e-mail]
> >
> > Fixes: 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250 port")
> > Reported-by: Daniel Winkler <[e-mail]>
> > Signed-off-by: Serge Semin <[e-mail]>
Tested-by: Claire Chang <[e-mail]>
> >
> > ---
> >
> > Folks, sorry for a delay with the problem fix. A solution is turned out to
> > be a bit more complicated than I originally thought in my comment to the
> > Daniel revert-patch.
> >
> > Please also note, that I don't have a Mediatek hardware to test the
> > solution suggested in the patch. The code is written as on so called
> > the tip of the pen after digging into the 8250_mtk.c and 8250_port.c
> > drivers code. So please Daniel or someone with Mediatek 8250-port
> > available on a board test this patch first and report about the results in
> > reply to this emailing thread. After that, if your conclusion is positive
> > and there is no objection against the solution design the patch can be
> > merged in.
I tested it with mt8183 + QCA6174.
The UART Bluetooth works fine with this fix.
Thanks!
> >
> > Cc: Alexey Malahov <[e-mail]>
> > Cc: Daniel Winkler <[e-mail]>
> > Cc: Aaron Sierra <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>
> > Cc: Lukas Wunner <[e-mail]>
> > Cc: Vignesh Raghavendra <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: BlueZ <[e-mail]>
> > Cc: chromeos-bluetooth-upstreaming <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  drivers/tty/serial/8250/8250_mtk.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > index f839380c2f4c..98b8a3e30733 100644
> > --- a/drivers/tty/serial/8250/8250_mtk.c
> > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > @@ -306,8 +306,21 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> >         }
> >  #endif
> >
> > +       /*
> > +        * Store the requested baud rate before calling the generic 8250
> > +        * set_termios method. Standard 8250 port expects bauds to be
> > +        * no higher than (uartclk / 16) so the baud will be clamped if it
> > +        * gets out of that bound. Mediatek 8250 port supports speed
> > +        * higher than that, therefore we'll get original baud rate back
> > +        * after calling the generic set_termios method and recalculate
> > +        * the speed later in this method.
> > +        */
> > +       baud = tty_termios_baud_rate(termios);
> > +
> >         serial8250_do_set_termios(port, termios, old);
> >
> > +       tty_termios_encode_baud_rate(termios, baud, baud);
> > +
> >         /*
> >          * Mediatek UARTs use an extra highspeed register (MTK_UART_HIGHS)
> >          *
> > @@ -339,6 +352,11 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> >          */
> >         spin_lock_irqsave(&port->lock, flags);
> >
> > +       /*
> > +        * Update the per-port timeout.
> > +        */
> > +       uart_update_timeout(port, termios->c_cflag, baud);
> > +
> >         /* set DLAB we have cval saved in up->lcr from the call to the core */
> >         serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
> >         serial_dl_write(up, quot);
> > --
> > 2.26.2
> >

Serge Semin July 15, 2020, 4:28 p.m. UTC | #3

On Wed, Jul 15, 2020 at 01:36:04PM +0800, Claire Chang wrote:
> On Wed, Jul 15, 2020 at 4:45 AM Daniel Winkler <[e-mail]> wrote:
> >
> > Thank you Sergey for looking into this. Adding folks working on this
> > platform to perform validation of the proposed patch.
> >
> > Best,
> > Daniel
> >
> > On Tue, Jul 14, 2020 at 5:41 AM Serge Semin
> > <[e-mail]> wrote:
> > >
> > > Commit 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250
> > > port") fixed limits of a baud rate setting for a generic 8250 port.
> > > In other words since that commit the baud rate has been permitted to be
> > > within [uartclk / 16 / UART_DIV_MAX; uartclk / 16], which is absolutely
> > > normal for a standard 8250 UART port. But there are custom 8250 ports,
> > > which provide extended baud rate limits. In particular the Mediatek 8250
> > > port can work with baud rates up to "uartclk" speed.
> > >
> > > Normally that and any other peculiarity is supposed to be handled in a
> > > custom set_termios() callback implemented in the vendor-specific
> > > 8250-port glue-driver. Currently that is how it's done for the most of
> > > the vendor-specific 8250 ports, but for some reason for Mediatek a
> > > solution has been spread out to both the glue-driver and to the generic
> > > 8250-port code. Due to that a bug has been introduced, which permitted the
> > > extended baud rate limit for all even for standard 8250-ports. The bug
> > > has been fixed by the commit 7b668c064ec3 ("serial: 8250: Fix max baud
> > > limit in generic 8250 port") by narrowing the baud rates limit back down to
> > > the normal bounds. Unfortunately by doing so we also broke the
> > > Mediatek-specific extended bauds feature.
> > >
> > > A fix of the problem described above is twofold. First since we can't get
> > > back the extended baud rate limits feature to the generic set_termios()
> > > function and that method supports only a standard baud rates range, the
> > > requested baud rate must be locally stored before calling it and then
> > > restored back to the new termios structure after the generic set_termios()
> > > finished its magic business. By doing so we still use the
> > > serial8250_do_set_termios() method to set the LCR/MCR/FCR/etc. registers,
> > > while the extended baud rate setting procedure will be performed later in
> > > the custom Mediatek-specific set_termios() callback. Second since a true
> > > baud rate is now fully calculated in the custom set_termios() method we
> > > need to locally update the port timeout by calling the
> > > uart_update_timeout() function. After the fixes described above are
> > > implemented in the 8250_mtk.c driver, the Mediatek 8250-port should
> > > get back to normally working with extended baud rates.
> > >
> > > Link: https://lore.kernel.org/linux-serial/[e-mail]
> > >
> > > Fixes: 7b668c064ec3 ("serial: 8250: Fix max baud limit in generic 8250 port")
> > > Reported-by: Daniel Winkler <[e-mail]>
> > > Signed-off-by: Serge Semin <[e-mail]>

> Tested-by: Claire Chang <[e-mail]>
> > >
> > > ---
> > >
> > > Folks, sorry for a delay with the problem fix. A solution is turned out to
> > > be a bit more complicated than I originally thought in my comment to the
> > > Daniel revert-patch.
> > >
> > > Please also note, that I don't have a Mediatek hardware to test the
> > > solution suggested in the patch. The code is written as on so called
> > > the tip of the pen after digging into the 8250_mtk.c and 8250_port.c
> > > drivers code. So please Daniel or someone with Mediatek 8250-port
> > > available on a board test this patch first and report about the results in
> > > reply to this emailing thread. After that, if your conclusion is positive
> > > and there is no objection against the solution design the patch can be
> > > merged in.
> I tested it with mt8183 + QCA6174.
> The UART Bluetooth works fine with this fix.
> Thanks!

Great! Thanks.

Greg, Jiri, Matthias since a test's showed the patch correctness you may now
merge it in if you are ok with its design. It shall fix the problem Daniel
reported.

-Sergey

> > >
> > > Cc: Alexey Malahov <[e-mail]>
> > > Cc: Daniel Winkler <[e-mail]>
> > > Cc: Aaron Sierra <[e-mail]>
> > > Cc: Andy Shevchenko <[e-mail]>
> > > Cc: Lukas Wunner <[e-mail]>
> > > Cc: Vignesh Raghavendra <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Cc: BlueZ <[e-mail]>
> > > Cc: chromeos-bluetooth-upstreaming <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > ---
> > >  drivers/tty/serial/8250/8250_mtk.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > > index f839380c2f4c..98b8a3e30733 100644
> > > --- a/drivers/tty/serial/8250/8250_mtk.c
> > > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > > @@ -306,8 +306,21 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > >         }
> > >  #endif
> > >
> > > +       /*
> > > +        * Store the requested baud rate before calling the generic 8250
> > > +        * set_termios method. Standard 8250 port expects bauds to be
> > > +        * no higher than (uartclk / 16) so the baud will be clamped if it
> > > +        * gets out of that bound. Mediatek 8250 port supports speed
> > > +        * higher than that, therefore we'll get original baud rate back
> > > +        * after calling the generic set_termios method and recalculate
> > > +        * the speed later in this method.
> > > +        */
> > > +       baud = tty_termios_baud_rate(termios);
> > > +
> > >         serial8250_do_set_termios(port, termios, old);
> > >
> > > +       tty_termios_encode_baud_rate(termios, baud, baud);
> > > +
> > >         /*
> > >          * Mediatek UARTs use an extra highspeed register (MTK_UART_HIGHS)
> > >          *
> > > @@ -339,6 +352,11 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > >          */
> > >         spin_lock_irqsave(&port->lock, flags);
> > >
> > > +       /*
> > > +        * Update the per-port timeout.
> > > +        */
> > > +       uart_update_timeout(port, termios->c_cflag, baud);
> > > +
> > >         /* set DLAB we have cval saved in up->lcr from the call to the core */
> > >         serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
> > >         serial_dl_write(up, quot);
> > > --
> > > 2.26.2
> > >

Patch

diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index f839380c2f4c..98b8a3e30733 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -306,8 +306,21 @@  mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
 	}
 #endif
 
+	/*
+	 * Store the requested baud rate before calling the generic 8250
+	 * set_termios method. Standard 8250 port expects bauds to be
+	 * no higher than (uartclk / 16) so the baud will be clamped if it
+	 * gets out of that bound. Mediatek 8250 port supports speed
+	 * higher than that, therefore we'll get original baud rate back
+	 * after calling the generic set_termios method and recalculate
+	 * the speed later in this method.
+	 */
+	baud = tty_termios_baud_rate(termios);
+
 	serial8250_do_set_termios(port, termios, old);
 
+	tty_termios_encode_baud_rate(termios, baud, baud);
+
 	/*
 	 * Mediatek UARTs use an extra highspeed register (MTK_UART_HIGHS)
 	 *
@@ -339,6 +352,11 @@  mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
 	 */
 	spin_lock_irqsave(&port->lock, flags);
 
+	/*
+	 * Update the per-port timeout.
+	 */
+	uart_update_timeout(port, termios->c_cflag, baud);
+
 	/* set DLAB we have cval saved in up->lcr from the call to the core */
 	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
 	serial_dl_write(up, quot);