| Message ID | 20220418053202.24528-2-yu.tu@amlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add q 12MHz internal clock rate to calculate | expand |
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 > + val |= AML_UART_BAUD_XTAL_DIV2; > + } > + val |= DIV_ROUND_CLOSEST(port->uartclk / xtal_div, baud) - 1;
Hi Andy, Thank you for your advice. On 2022/4/18 20:09, Andy Shevchenko wrote: > [ EXTERNAL EMAIL ] > > 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) My statement is a whole. There should be no spaces. > >> 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. I don't have a problem with your suggestion.Let's see What Neil has to say. > >> +}; > > ... > >> + unsigned int xtal_div = 3; > >> + if (private_data && private_data->has_xtal_div2) { >> + xtal_div = 2; > > Better to define privata data always > > >> + val |= AML_UART_BAUD_XTAL_DIV2; >> + } >> + val |= DIV_ROUND_CLOSEST(port->uartclk / xtal_div, baud) - 1; > >
Hi, On 19. 04. 22, 9:29, Yu Tu wrote: > Hi Andy, > Thank you for your advice. > > On 2022/4/18 20:09, Andy Shevchenko wrote: >> [ EXTERNAL EMAIL ] >> >> 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) > My statement is a whole. There should be no spaces. Period should be followed by a space, of course. >>> 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. > 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? thanks,
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.
Hi Jiri, Thanks for the tip. On 2022/4/19 15:38, Jiri Slaby wrote: > [ EXTERNAL EMAIL ] > > Hi, > > On 19. 04. 22, 9:29, Yu Tu wrote: >> Hi Andy, >> Thank you for your advice. >> >> On 2022/4/18 20:09, Andy Shevchenko wrote: >>> [ EXTERNAL EMAIL ] >>> >>> 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) >> My statement is a whole. There should be no spaces. > > Period should be followed by a space, of course. I got it .This will be corrected in the next version. > >>>> 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. >> 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? > > thanks,
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 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. > > >> + val |= AML_UART_BAUD_XTAL_DIV2; >> + } >> + val |= DIV_ROUND_CLOSEST(port->uartclk / xtal_div, baud) - 1; > >
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(-)