| Message ID | 20220418053202.24528-2-yu.tu@amlogic.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2,1/2] tty: serial: meson: Add a 12MHz internal clock rate to calculate baud rate in order to meet the baud rate requirements of special BT modules | expand |
Hi Andy, On 19/04/2022 10:21, Andy Shevchenko wrote: > On Tue, Apr 19, 2022 at 10:38 AM Jiri Slaby <jirislaby@kernel.org> wrote: >> On 19. 04. 22, 9:29, Yu Tu wrote: >>> On 2022/4/18 20:09, Andy Shevchenko wrote: >>>> On Mon, Apr 18, 2022 at 8:50 AM Yu Tu <yu.tu@amlogic.com> wrote: > > ... > >>>>> +struct meson_uart_data { >>>>> + bool has_xtal_div2; >>>> >>>> I would prefer to see this as an unsigned int and with a less >>>> particular name, e.g. xtal_div would suffice. >>> I don't have a problem with your suggestion.Let's see What Neil has to say. >> >> Actually why uint provided it's a boolean value? Or do you mean to store >> the divisor directly in this member, Andy? > > Yes I was thinking to provide the value and then always provide the > private data. In such cases we don't need an additional condition. > Actually, the original boolean "has_xtal_div2" is right because it encodes if the HW has an internal /2 divider for the XTAL clock input path. The HW historically has a /3 divider on the same path, and new HW now has both. So the boolean indicates if the /2 divider is present so it can be used. So I'm in favour of keeping the boolean type. For the naming, it seems appropriate for me. Neil
Hi Neil, On 2022/4/21 16:46, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > Hi Andy, > > On 18/04/2022 14:09, Andy Shevchenko wrote: >> On Mon, Apr 18, 2022 at 8:50 AM Yu Tu <yu.tu@amlogic.com> wrote: >>> >>> A /2 divider over XTAL was introduced since G12A, and is preferred >>> to be used over the still present /3 divider since it provides much >>> closer frequencies vs the request baudrate.Especially the BT module >> >> 'e. E' (mind the space) >> >>> uses 3Mhz baud rate. 8Mhz calculations can lead to baud rate bias, >>> causing some problems. >> >> ... >> >>> +struct meson_uart_data { >>> + bool has_xtal_div2; >> >> I would prefer to see this as an unsigned int and with a less >> particular name, e.g. xtal_div would suffice. >> >>> +}; >> >> ... >> >>> + unsigned int xtal_div = 3; >> >>> + if (private_data && private_data->has_xtal_div2) { >>> + xtal_div = 2; >> >> Better to define privata data always > > While I'm in favor of defining private data, here 3 and 2 are actually > the values > 2 and 3 used to divide. > > The code is easy to read and we quickly understand this value is the > clock divider. > Therefore, this place should be modified according to your previous suggestion.I will prepare the next version. >> >> >>> + val |= AML_UART_BAUD_XTAL_DIV2; >>> + } >>> + val |= DIV_ROUND_CLOSEST(port->uartclk / xtal_div, >>> baud) - 1; >> >> > > .
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 8e59624935af..58bd2723c004 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -68,6 +68,7 @@ #define AML_UART_BAUD_MASK 0x7fffff #define AML_UART_BAUD_USE BIT(23) #define AML_UART_BAUD_XTAL BIT(24) +#define AML_UART_BAUD_XTAL_DIV2 BIT(27) #define AML_UART_PORT_NUM 12 #define AML_UART_PORT_OFFSET 6 @@ -80,6 +81,10 @@ static struct uart_driver meson_uart_driver; static struct uart_port *meson_ports[AML_UART_PORT_NUM]; +struct meson_uart_data { + bool has_xtal_div2; +}; + static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl) { } @@ -293,13 +298,20 @@ static int meson_uart_startup(struct uart_port *port) static void meson_uart_change_speed(struct uart_port *port, unsigned long baud) { - u32 val; + const struct meson_uart_data *private_data = port->private_data; + u32 val = 0; while (!meson_uart_tx_empty(port)) cpu_relax(); if (port->uartclk == 24000000) { - val = DIV_ROUND_CLOSEST(port->uartclk / 3, baud) - 1; + unsigned int xtal_div = 3; + + if (private_data && private_data->has_xtal_div2) { + xtal_div = 2; + val |= AML_UART_BAUD_XTAL_DIV2; + } + val |= DIV_ROUND_CLOSEST(port->uartclk / xtal_div, baud) - 1; val |= AML_UART_BAUD_XTAL; } else { val = DIV_ROUND_CLOSEST(port->uartclk / 4, baud) - 1; @@ -749,6 +761,7 @@ static int meson_uart_probe(struct platform_device *pdev) port->x_char = 0; port->ops = &meson_uart_ops; port->fifosize = fifosize; + port->private_data = (void *)device_get_match_data(&pdev->dev); meson_ports[pdev->id] = port; platform_set_drvdata(pdev, port);
A /2 divider over XTAL was introduced since G12A, and is preferred to be used over the still present /3 divider since it provides much closer frequencies vs the request baudrate.Especially the BT module uses 3Mhz baud rate. 8Mhz calculations can lead to baud rate bias, causing some problems. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)