1.Added 12Mhz as the clock source for calculating baud rate.
2.Added S4 SOC compatibility.
Yu Tu (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
tty: serial: meson: Added S4 SOC compatibility
V1 -> V2: Change format as discussed in the email.
Link:https://lore.kernel.org/linux-amlogic/[email protected]/
drivers/tty/serial/meson_uart.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
base-commit: f398e0aa325c61fa20903833a5b534ecb8e6e418
--
2.33.1
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 <[email protected]>
---
drivers/tty/serial/meson_uart.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
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);
--
2.33.1
Make UART driver compatible with S4 SOC UART. Meanwhile, the S4 SOC
UART uses 12MHz as the clock source for baud rate calculations.
Signed-off-by: Yu Tu <[email protected]>
---
drivers/tty/serial/meson_uart.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 58bd2723c004..43941f21735f 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -790,11 +790,19 @@ static int meson_uart_remove(struct platform_device *pdev)
return 0;
}
+static struct meson_uart_data s4_uart_date = {
+ .has_xtal_div2 = true,
+};
+
static const struct of_device_id meson_uart_dt_match[] = {
{ .compatible = "amlogic,meson6-uart" },
{ .compatible = "amlogic,meson8-uart" },
{ .compatible = "amlogic,meson8b-uart" },
{ .compatible = "amlogic,meson-gx-uart" },
+ {
+ .compatible = "amlogic,meson-s4-uart",
+ .data = (void *)&s4_uart_date,
+ },
{ /* sentinel */ },
};
MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
--
2.33.1
On Mon, Apr 18, 2022 at 8:50 AM Yu Tu <[email protected]> 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;
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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 <[email protected]> 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,
--
js
suse labs
On Tue, Apr 19, 2022 at 10:38 AM Jiri Slaby <[email protected]> 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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On 2022/4/21 16:49, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
>
> On 18/04/2022 07:32, Yu Tu wrote:
>> Make UART driver compatible with S4 SOC UART. Meanwhile, the S4 SOC
>> UART uses 12MHz as the clock source for baud rate calculations.
>>
>> Signed-off-by: Yu Tu <[email protected]>
>> ---
>> drivers/tty/serial/meson_uart.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c
>> b/drivers/tty/serial/meson_uart.c
>> index 58bd2723c004..43941f21735f 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -790,11 +790,19 @@ static int meson_uart_remove(struct
>> platform_device *pdev)
>> return 0;
>> }
>> +static struct meson_uart_data s4_uart_date = {
>
> Should be s4_uart_data instead of s4_uart_date
>
I will prepare the next version and correct it.
>> + .has_xtal_div2 = true,
>> +};
>> +
>> static const struct of_device_id meson_uart_dt_match[] = {
>> { .compatible = "amlogic,meson6-uart" },
>> { .compatible = "amlogic,meson8-uart" },
>> { .compatible = "amlogic,meson8b-uart" },
>> { .compatible = "amlogic,meson-gx-uart" },
>> + {
>> + .compatible = "amlogic,meson-s4-uart",
>> + .data = (void *)&s4_uart_date,
>
> Here same
>
>> + },
>> { /* sentinel */ },
>> };
>> MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
> With this change, it's fine for me.
>
> Neil
>
> .
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 <[email protected]> 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,
On 18/04/2022 07:32, Yu Tu wrote:
> Make UART driver compatible with S4 SOC UART. Meanwhile, the S4 SOC
> UART uses 12MHz as the clock source for baud rate calculations.
>
> Signed-off-by: Yu Tu <[email protected]>
> ---
> drivers/tty/serial/meson_uart.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 58bd2723c004..43941f21735f 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -790,11 +790,19 @@ static int meson_uart_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static struct meson_uart_data s4_uart_date = {
Should be s4_uart_data instead of s4_uart_date
> + .has_xtal_div2 = true,
> +};
> +
> static const struct of_device_id meson_uart_dt_match[] = {
> { .compatible = "amlogic,meson6-uart" },
> { .compatible = "amlogic,meson8-uart" },
> { .compatible = "amlogic,meson8b-uart" },
> { .compatible = "amlogic,meson-gx-uart" },
> + {
> + .compatible = "amlogic,meson-s4-uart",
> + .data = (void *)&s4_uart_date,
Here same
> + },
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
With this change, it's fine 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 <[email protected]> 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 Andy,
On 19/04/2022 10:21, Andy Shevchenko wrote:
> On Tue, Apr 19, 2022 at 10:38 AM Jiri Slaby <[email protected]> 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 <[email protected]> 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 <[email protected]> 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;
>>
>>
>
> .