* Re: [PATCH v2 0/1] Revert "serial: 8250: Fix max baud limit in generic 8250 port"
@ 2020-07-01 22:37 ` Serge Semin
0 siblings, 0 replies; 13+ messages in thread
From: Serge Semin @ 2020-07-01 22:37 UTC (permalink / raw)
To: Daniel Winkler
Cc: chromeos-bluetooth-upstreaming, Vignesh Raghavendra,
Greg Kroah-Hartman, linux-kernel, Serge Semin, Alexey Malahov,
BlueZ, Lukas Wunner, linux-mediatek, stable, linux-serial,
Jiri Slaby, abhishekpandit, Aaron Sierra, Andy Shevchenko
On Wed, Jul 01, 2020 at 02:13:36PM -0700, Daniel Winkler wrote:
>
> This change regresses the QCA6174A-3 bluetooth chip, preventing
> firmware from being properly loaded. Without this change, the
> chip works as intended.
>
> The device is the Kukui Chromebook using the Mediatek chipset
> and the 8250_mtk uart. Initial controller baudrate is 115200
> and operating speed is 3000000. Our entire suite of bluetooth
> tests now fail on this platform due to an apparent failure to
> sync its firmware on initialization.
Ok. It's mediatek 8250 driver, which is responsible for the failure.
Then we'll have two options:
1) Add a new capability like UART_CAP_NO16DIV and take it into account
in the serial8250_get_baud_rate() method.
I don't have a documentation for the Mediatek UART port, but it seems to me
that that controller calculates the baud rate differently from the standard
8250 port. A standard 8250 port does that by the next formulae:
baud = uartclk / (16 * divisor).
While it seems to me that the Mediatek port uses the formulae like:
baud = uartclk / divisor. (Please, correct me if I'm wrong)
If so, then we could introduce a new capability like UART_CAP_NO16DIV. The
8250_mtk driver will add it to the 8250-port capabilities field. The
serial8250_get_baud_rate() method should be altered in a way so one would check
whether the UART_CAP_NO16DIV flag is set and if it is then the
uart_get_baud_rate() function will be called without uartclk normalized by the
factor of 16.
2) Manually call serial8250_do_set_divisor() in the custom set_termios()
callback.
Just add the uart_update_timeout() and serial8250_do_set_divisor() methods
invocation into the mtk8250_set_termios() function, which the original commit
81bb549fdf14 ("serial: 8250_mtk: support big baud rate") author should have
done in the first place.
-Sergey
>
> The driver is in the cros tree at drivers/bluetooth/hci_qca.c
> and uses the serdev interface. Specifically, this is the
> QCA_ROME chipset.
>
>
> Daniel Winkler (1):
> Revert "serial: 8250: Fix max baud limit in generic 8250 port"
>
> drivers/tty/serial/8250/8250_port.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> --
> 2.27.0.212.ge8ba1cc988-goog
>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/1] Revert "serial: 8250: Fix max baud limit in generic 8250 port"
2020-07-01 22:37 ` Serge Semin
(?)
@ 2020-07-02 4:11 ` Lukas Wunner
2020-07-02 8:46 ` Andy Shevchenko
-1 siblings, 1 reply; 13+ messages in thread
From: Lukas Wunner @ 2020-07-02 4:11 UTC (permalink / raw)
To: Serge Semin
Cc: Daniel Winkler, Serge Semin, linux-serial, linux-mediatek,
Alexey Malahov, BlueZ, chromeos-bluetooth-upstreaming, stable,
abhishekpandit, Aaron Sierra, Andy Shevchenko,
Greg Kroah-Hartman, Jiri Slaby, Vignesh Raghavendra,
linux-kernel
On Thu, Jul 02, 2020 at 01:37:13AM +0300, Serge Semin wrote:
> 1) Add a new capability like UART_CAP_NO16DIV and take it into account
> in the serial8250_get_baud_rate() method.
>
> I don't have a documentation for the Mediatek UART port, but it seems to me
> that that controller calculates the baud rate differently from the standard
> 8250 port. A standard 8250 port does that by the next formulae:
> baud = uartclk / (16 * divisor).
> While it seems to me that the Mediatek port uses the formulae like:
> baud = uartclk / divisor. (Please, correct me if I'm wrong)
8250_bcm2835aux.c seems to suffer from a similar issue and
solves it like this in the ->probe hook:
/* the HW-clock divider for bcm2835aux is 8,
* but 8250 expects a divider of 16,
* so we have to multiply the actual clock by 2
* to get identical baudrates.
*/
up.port.uartclk = clk_get_rate(data->clk) * 2;
> 2) Manually call serial8250_do_set_divisor() in the custom set_termios()
> callback.
>
> Just add the uart_update_timeout() and serial8250_do_set_divisor() methods
> invocation into the mtk8250_set_termios() function, which the original commit
> 81bb549fdf14 ("serial: 8250_mtk: support big baud rate") author should have
> done in the first place.
That sound preferable as adding new quirks into core code feels
like a case of midlayer fallacy:
https://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html
Thanks,
Lukas
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/1] Revert "serial: 8250: Fix max baud limit in generic 8250 port"
2020-07-02 4:11 ` Lukas Wunner
@ 2020-07-02 8:46 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2020-07-02 8:46 UTC (permalink / raw)
To: Lukas Wunner
Cc: Serge Semin, Daniel Winkler, Serge Semin, linux-serial,
linux-mediatek, Alexey Malahov, BlueZ,
chromeos-bluetooth-upstreaming, stable, abhishekpandit,
Aaron Sierra, Greg Kroah-Hartman, Jiri Slaby,
Vignesh Raghavendra, linux-kernel
On Thu, Jul 02, 2020 at 06:11:52AM +0200, Lukas Wunner wrote:
> On Thu, Jul 02, 2020 at 01:37:13AM +0300, Serge Semin wrote:
> > 1) Add a new capability like UART_CAP_NO16DIV and take it into account
> > in the serial8250_get_baud_rate() method.
> >
> > I don't have a documentation for the Mediatek UART port, but it seems to me
> > that that controller calculates the baud rate differently from the standard
> > 8250 port. A standard 8250 port does that by the next formulae:
> > baud = uartclk / (16 * divisor).
> > While it seems to me that the Mediatek port uses the formulae like:
> > baud = uartclk / divisor. (Please, correct me if I'm wrong)
>
> 8250_bcm2835aux.c seems to suffer from a similar issue and
> solves it like this in the ->probe hook:
>
> /* the HW-clock divider for bcm2835aux is 8,
> * but 8250 expects a divider of 16,
> * so we have to multiply the actual clock by 2
> * to get identical baudrates.
> */
> up.port.uartclk = clk_get_rate(data->clk) * 2;
8250_mid for example lies about UART clock due to this. It has a comment in the
code in its ->set_termios().
Yes, we have a lot of possibilities here to fix, I guess. We have custom
termios callback, also get and set divisor and so on.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/1] Revert "serial: 8250: Fix max baud limit in generic 8250 port"
@ 2020-07-02 8:46 ` Andy Shevchenko
0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2020-07-02 8:46 UTC (permalink / raw)
To: Lukas Wunner
Cc: chromeos-bluetooth-upstreaming, Vignesh Raghavendra,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel, Serge Semin,
Alexey Malahov, Serge Semin, BlueZ, linux-mediatek, stable,
linux-serial, Daniel Winkler, abhishekpandit, Aaron Sierra
On Thu, Jul 02, 2020 at 06:11:52AM +0200, Lukas Wunner wrote:
> On Thu, Jul 02, 2020 at 01:37:13AM +0300, Serge Semin wrote:
> > 1) Add a new capability like UART_CAP_NO16DIV and take it into account
> > in the serial8250_get_baud_rate() method.
> >
> > I don't have a documentation for the Mediatek UART port, but it seems to me
> > that that controller calculates the baud rate differently from the standard
> > 8250 port. A standard 8250 port does that by the next formulae:
> > baud = uartclk / (16 * divisor).
> > While it seems to me that the Mediatek port uses the formulae like:
> > baud = uartclk / divisor. (Please, correct me if I'm wrong)
>
> 8250_bcm2835aux.c seems to suffer from a similar issue and
> solves it like this in the ->probe hook:
>
> /* the HW-clock divider for bcm2835aux is 8,
> * but 8250 expects a divider of 16,
> * so we have to multiply the actual clock by 2
> * to get identical baudrates.
> */
> up.port.uartclk = clk_get_rate(data->clk) * 2;
8250_mid for example lies about UART clock due to this. It has a comment in the
code in its ->set_termios().
Yes, we have a lot of possibilities here to fix, I guess. We have custom
termios callback, also get and set divisor and so on.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/1] Revert "serial: 8250: Fix max baud limit in generic 8250 port"
2020-07-01 22:37 ` Serge Semin
@ 2020-07-02 7:37 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-02 7:37 UTC (permalink / raw)
To: Serge Semin
Cc: Daniel Winkler, Serge Semin, linux-serial, linux-mediatek,
Alexey Malahov, BlueZ, chromeos-bluetooth-upstreaming, stable,
abhishekpandit, Aaron Sierra, Andy Shevchenko, Jiri Slaby,
Lukas Wunner, Vignesh Raghavendra, linux-kernel
On Thu, Jul 02, 2020 at 01:37:13AM +0300, Serge Semin wrote:
> On Wed, Jul 01, 2020 at 02:13:36PM -0700, Daniel Winkler wrote:
> >
> > This change regresses the QCA6174A-3 bluetooth chip, preventing
> > firmware from being properly loaded. Without this change, the
> > chip works as intended.
> >
> > The device is the Kukui Chromebook using the Mediatek chipset
> > and the 8250_mtk uart. Initial controller baudrate is 115200
> > and operating speed is 3000000. Our entire suite of bluetooth
> > tests now fail on this platform due to an apparent failure to
> > sync its firmware on initialization.
>
> Ok. It's mediatek 8250 driver, which is responsible for the failure.
> Then we'll have two options:
>
> 1) Add a new capability like UART_CAP_NO16DIV and take it into account
> in the serial8250_get_baud_rate() method.
>
> I don't have a documentation for the Mediatek UART port, but it seems to me
> that that controller calculates the baud rate differently from the standard
> 8250 port. A standard 8250 port does that by the next formulae:
> baud = uartclk / (16 * divisor).
> While it seems to me that the Mediatek port uses the formulae like:
> baud = uartclk / divisor. (Please, correct me if I'm wrong)
> If so, then we could introduce a new capability like UART_CAP_NO16DIV. The
> 8250_mtk driver will add it to the 8250-port capabilities field. The
> serial8250_get_baud_rate() method should be altered in a way so one would check
> whether the UART_CAP_NO16DIV flag is set and if it is then the
> uart_get_baud_rate() function will be called without uartclk normalized by the
> factor of 16.
>
> 2) Manually call serial8250_do_set_divisor() in the custom set_termios()
> callback.
>
> Just add the uart_update_timeout() and serial8250_do_set_divisor() methods
> invocation into the mtk8250_set_termios() function, which the original commit
> 81bb549fdf14 ("serial: 8250_mtk: support big baud rate") author should have
> done in the first place.
Sounds like a sane fix, thanks for looking into this.
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/1] Revert "serial: 8250: Fix max baud limit in generic 8250 port"
@ 2020-07-02 7:37 ` Greg Kroah-Hartman
0 siblings, 0 replies; 13+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-02 7:37 UTC (permalink / raw)
To: Serge Semin
Cc: chromeos-bluetooth-upstreaming, Vignesh Raghavendra, Jiri Slaby,
linux-kernel, Serge Semin, Alexey Malahov, BlueZ, Lukas Wunner,
linux-mediatek, stable, linux-serial, Daniel Winkler,
abhishekpandit, Aaron Sierra, Andy Shevchenko
On Thu, Jul 02, 2020 at 01:37:13AM +0300, Serge Semin wrote:
> On Wed, Jul 01, 2020 at 02:13:36PM -0700, Daniel Winkler wrote:
> >
> > This change regresses the QCA6174A-3 bluetooth chip, preventing
> > firmware from being properly loaded. Without this change, the
> > chip works as intended.
> >
> > The device is the Kukui Chromebook using the Mediatek chipset
> > and the 8250_mtk uart. Initial controller baudrate is 115200
> > and operating speed is 3000000. Our entire suite of bluetooth
> > tests now fail on this platform due to an apparent failure to
> > sync its firmware on initialization.
>
> Ok. It's mediatek 8250 driver, which is responsible for the failure.
> Then we'll have two options:
>
> 1) Add a new capability like UART_CAP_NO16DIV and take it into account
> in the serial8250_get_baud_rate() method.
>
> I don't have a documentation for the Mediatek UART port, but it seems to me
> that that controller calculates the baud rate differently from the standard
> 8250 port. A standard 8250 port does that by the next formulae:
> baud = uartclk / (16 * divisor).
> While it seems to me that the Mediatek port uses the formulae like:
> baud = uartclk / divisor. (Please, correct me if I'm wrong)
> If so, then we could introduce a new capability like UART_CAP_NO16DIV. The
> 8250_mtk driver will add it to the 8250-port capabilities field. The
> serial8250_get_baud_rate() method should be altered in a way so one would check
> whether the UART_CAP_NO16DIV flag is set and if it is then the
> uart_get_baud_rate() function will be called without uartclk normalized by the
> factor of 16.
>
> 2) Manually call serial8250_do_set_divisor() in the custom set_termios()
> callback.
>
> Just add the uart_update_timeout() and serial8250_do_set_divisor() methods
> invocation into the mtk8250_set_termios() function, which the original commit
> 81bb549fdf14 ("serial: 8250_mtk: support big baud rate") author should have
> done in the first place.
Sounds like a sane fix, thanks for looking into this.
greg k-h
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 13+ messages in thread