diff mbox 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

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

Commit Message

Yu Tu April 18, 2022, 5:32 a.m. UTC
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(-)

Comments

Neil Armstrong April 21, 2022, 8:44 a.m. UTC | #1
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
Yu Tu April 21, 2022, 9:46 a.m. UTC | #2
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 mbox series

Patch

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);