From: Yu Tu <yu.tu@amlogic.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: <linux-serial@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-amlogic@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, Jiri Slaby <jirislaby@kernel.org>,
Neil Armstrong <narmstrong@baylibre.com>,
Kevin Hilman <khilman@baylibre.com>,
Jerome Brunet <jbrunet@baylibre.com>,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Subject: Re: [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip
Date: Wed, 22 Dec 2021 17:28:04 +0800 [thread overview]
Message-ID: <eae96731-d231-ab2f-934f-70a9fc3f8409@amlogic.com> (raw)
In-Reply-To: <YcGDk3eC3sIstYjs@kroah.com>
On 2021/12/21 15:34, Greg Kroah-Hartman wrote:
> [ EXTERNAL EMAIL ]
>
> On Tue, Dec 21, 2021 at 03:16:34PM +0800, Yu Tu wrote:
>> The S4 SoC on-chip UART uses a 12M clock as the clock source for
>> calculating the baud rate of the UART. But previously, chips used 24M or
>> other clock sources. So add this change. The specific clock source is
>> determined by chip design.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>> drivers/tty/serial/meson_uart.c | 62 +++++++++++++++++++++++++++++----
>> 1 file changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 69450a461c48..557c24d954a2 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -19,6 +19,7 @@
>> #include <linux/serial_core.h>
>> #include <linux/tty.h>
>> #include <linux/tty_flip.h>
>> +#include <linux/of_device.h>
>>
>> /* Register offsets */
>> #define AML_UART_WFIFO 0x00
>> @@ -68,6 +69,8 @@
>> #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_TICK BIT(26)
>> +#define AML_UART_BAUD_XTAL_DIV2 BIT(27)
>>
>> #define AML_UART_PORT_NUM 12
>> #define AML_UART_PORT_OFFSET 6
>> @@ -80,6 +83,11 @@ static struct uart_driver meson_uart_driver;
>>
>> static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>
>> +struct meson_uart_data {
>> + /*A clock source that calculates baud rates*/
>
> Please use spaces in your comments.
I will correct this mistake in the next patch.
>
>> + unsigned int xtal_tick_en;
>
> What is "_en" for?
>
> "enabled"?
>
> Spell it out please.
You're right.I will correct as you suggested.
>
> And why an unsigned int for a boolean flag?
It is my thoughtless, I will correct.
>
>> +};
>> +
>> static void meson_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> {
>> }
>> @@ -294,16 +302,29 @@ static int meson_uart_startup(struct uart_port *port)
>>
>> static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>> {
>> + struct meson_uart_data *uart_data = port->private_data;
>> u32 val;
>>
>> while (!meson_uart_tx_empty(port))
>> cpu_relax();
>>
>> + val = readl_relaxed(port->membase + AML_UART_REG5);
>> + val &= ~AML_UART_BAUD_MASK;
>> +
>> if (port->uartclk == 24000000) {
>> - val = ((port->uartclk / 3) / baud) - 1;
>> - val |= AML_UART_BAUD_XTAL;
>> + if (uart_data->xtal_tick_en) {
>> + val = (port->uartclk / 2 + baud / 2) / baud - 1;
>> + val |= (AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_DIV2);
>> + } else {
>> + val = ((port->uartclk / 3) + baud / 2) / baud - 1;
>> + val &= (~(AML_UART_BAUD_XTAL_TICK |
>> + AML_UART_BAUD_XTAL_DIV2));
>> + val |= AML_UART_BAUD_XTAL;
>> + }
>> } else {
>> val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> + val &= (~(AML_UART_BAUD_XTAL | AML_UART_BAUD_XTAL_TICK |
>> + AML_UART_BAUD_XTAL_DIV2));
>> }
>> val |= AML_UART_BAUD_USE;
>> writel(val, port->membase + AML_UART_REG5);
>> @@ -714,6 +735,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>> {
>> struct resource *res_mem, *res_irq;
>> struct uart_port *port;
>> + struct meson_uart_data *uart_data;
>> int ret = 0;
>> int id = -1;
>>
>> @@ -729,6 +751,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>> }
>> }
>>
>> + uart_data = of_device_get_match_data(&pdev->dev);
>> + if (!uart_data)
>> + return -EINVAL;
>
> Wrong spacing.
>
> Always use checkpatch.pl on your patches before sending them out.
Sorry, this is a rookie mistake.But I did check it locally before
sending it. I will follow your advice strictly later.
>
> And did you just break existing systems? Do you know if all older ones
> will still work with that call?
>
It does affect older systems, but the new and older baud rates are not
the same. I checked the documents before I made any changes. So this
change is compatible with the older.
>> +
>> if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
>> return -EINVAL;
>>
>> @@ -770,6 +796,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>> port->x_char = 0;
>> port->ops = &meson_uart_ops;
>> port->fifosize = 64;
>> + port->private_data = uart_data;
>>
>> meson_ports[pdev->id] = port;
>> platform_set_drvdata(pdev, port);
>> @@ -798,14 +825,35 @@ static int meson_uart_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static const struct meson_uart_data meson_uart_data = {
>> + .xtal_tick_en = 0,
>> +};
>> +
>> +static const struct meson_uart_data s4_meson_uart_data = {
>> + .xtal_tick_en = 1,
>> +};
>
> As your whole structure just has one bit, why not just use that as the
> data value, instead of a structure? No need to be complex here at all.
>
It is my thoughtless, I will correct.
> thanks,
>
> greg k-h
>
next prev parent reply other threads:[~2021-12-22 9:28 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-21 7:16 [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC Yu Tu
2021-12-21 7:16 ` [PATCH 1/3] tty: serial: meson: modify request_irq and free_irq Yu Tu
2021-12-21 7:30 ` Greg Kroah-Hartman
2021-12-22 8:44 ` Yu Tu
2021-12-21 7:16 ` [PATCH 2/3] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit Yu Tu
2021-12-21 7:32 ` Greg Kroah-Hartman
2021-12-22 9:01 ` Yu Tu
2021-12-21 7:16 ` [PATCH 3/3] tty: serial: meson: add UART driver compatible with S4 SoC on-chip Yu Tu
2021-12-21 7:34 ` Greg Kroah-Hartman
2021-12-22 9:28 ` Yu Tu [this message]
2021-12-24 17:25 ` Martin Blumenstingl
2021-12-27 6:56 ` Yu Tu
2021-12-27 11:58 ` Jerome Brunet
2021-12-27 20:04 ` Martin Blumenstingl
2021-12-28 11:24 ` Yu Tu
2021-12-21 7:30 ` [PATCH 0/3] the UART driver compatible with the Amlogic Meson S4 SoC Greg Kroah-Hartman
2021-12-22 8:19 ` Yu Tu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eae96731-d231-ab2f-934f-70a9fc3f8409@amlogic.com \
--to=yu.tu@amlogic.com \
--cc=gregkh@linuxfoundation.org \
--cc=jbrunet@baylibre.com \
--cc=jirislaby@kernel.org \
--cc=khilman@baylibre.com \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=narmstrong@baylibre.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).