From: Yu Tu <yu.tu@amlogic.com>
To: Jerome Brunet <jbrunet@baylibre.com>,
<linux-serial@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-amlogic@lists.infradead.org>,
<linux-kernel@vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
Neil Armstrong <narmstrong@baylibre.com>,
Kevin Hilman <khilman@baylibre.com>,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Subject: Re: [PATCH V6 3/5] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame
Date: Mon, 21 Feb 2022 16:52:13 +0800 [thread overview]
Message-ID: <0d00b84d-2e8c-3c12-d820-cf6f7142083d@amlogic.com> (raw)
In-Reply-To: <1jfspi2i5s.fsf@starbuckisacylon.baylibre.com>
Hi Jerome,
Thank you very much for your reply. At present, the problem of
switching UART baud rate stuck has been solved. I'm ready to send the
next edition. However, I still don't understand your suggestion and need
to communicate with you.
On 2022/1/21 5:40, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Tue 18 Jan 2022 at 11:09, Yu Tu <yu.tu@amlogic.com> wrote:
>
>> Using the common Clock code to describe the UART baud rate clock
>> makes it easier for the UART driver to be compatible with the
>> baud rate requirements of the UART IP on different meson chips.
>>
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>> drivers/tty/serial/meson_uart.c | 195 +++++++++++++++++++++++---------
>> 1 file changed, 142 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..92fa91c825e6 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -6,6 +6,7 @@
>> */
>>
>> #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> #include <linux/console.h>
>> #include <linux/delay.h>
>> #include <linux/init.h>
>> @@ -65,9 +66,7 @@
>> #define AML_UART_RECV_IRQ(c) ((c) & 0xff)
>>
>> /* AML_UART_REG5 bits */
>> -#define AML_UART_BAUD_MASK 0x7fffff
>> #define AML_UART_BAUD_USE BIT(23)
>> -#define AML_UART_BAUD_XTAL BIT(24)
>>
>> #define AML_UART_PORT_NUM 12
>> #define AML_UART_PORT_OFFSET 6
>> @@ -76,6 +75,11 @@
>> #define AML_UART_POLL_USEC 5
>> #define AML_UART_TIMEOUT_USEC 10000
>>
>> +struct meson_uart_data {
>> + struct clk *baud_clk;
>> + bool use_xtal_clk;
>> +};
>> +
>> static struct uart_driver meson_uart_driver;
>>
>> static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>> @@ -293,19 +297,17 @@ 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 *private_data = port->private_data;
>> u32 val;
>>
>> while (!meson_uart_tx_empty(port))
>> cpu_relax();
>>
>> - if (port->uartclk == 24000000) {
>> - val = ((port->uartclk / 3) / baud) - 1;
>> - val |= AML_UART_BAUD_XTAL;
>> - } else {
>> - val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> - }
>> + val = readl(port->membase + AML_UART_REG5);
>> val |= AML_UART_BAUD_USE;
>> writel(val, port->membase + AML_UART_REG5);
>> +
>> + clk_set_rate(private_data->baud_clk, baud);
>> }
>>
>> static void meson_uart_set_termios(struct uart_port *port,
>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port,
>>
>> static void meson_uart_release_port(struct uart_port *port)
>> {
>> - /* nothing to do */
>> + struct meson_uart_data *private_data = port->private_data;
>> +
>> + clk_disable_unprepare(private_data->baud_clk);
>> }
>>
>> static int meson_uart_request_port(struct uart_port *port)
>> {
>> + struct meson_uart_data *private_data = port->private_data;
>> + int ret;
>> +
>> + ret = clk_prepare_enable(private_data->baud_clk);
>> + if (ret)
>> + return ret;
>> +
>> return 0;
>> }
>>
>> @@ -629,57 +640,105 @@ static struct uart_driver meson_uart_driver = {
>> .cons = MESON_SERIAL_CONSOLE,
>> };
>>
>> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>> - const char *id)
>> -{
>> - struct clk *clk = NULL;
>> - int ret;
>> -
>> - clk = devm_clk_get(dev, id);
>> - if (IS_ERR(clk))
>> - return clk;
>> -
>> - ret = clk_prepare_enable(clk);
>> - if (ret) {
>> - dev_err(dev, "couldn't enable clk\n");
>> - return ERR_PTR(ret);
>> - }
>> -
>> - devm_add_action_or_reset(dev,
>> - (void(*)(void *))clk_disable_unprepare,
>> - clk);
>> -
>> - return clk;
>> -}
>> +static struct clk_div_table xtal_div_table[] = {
>> + {0, 3},
>> + {1, 1},
>> + {2, 2},
>> + {3, 2},
>> +};
>>
>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>> - struct uart_port *port)
>> +static int meson_uart_probe_clocks(struct uart_port *port)
>> {
>> - struct clk *clk_xtal = NULL;
>> - struct clk *clk_pclk = NULL;
>> - struct clk *clk_baud = NULL;
>> + struct meson_uart_data *private_data = port->private_data;
>> + struct clk *clk_baud, *clk_xtal;
>> + struct clk_hw *hw;
>> + char clk_name[32];
>> + struct clk_parent_data use_xtal_mux_parents[2] = {
>> + { .index = -1, },
>> + { .index = -1, },
>> + };
>
> You are using hw pointers later, you don't need to init the index to -1
> I think
>
I'm going to delete them in the next version.
like: struct clk_parent_data use_xtal_mux_parents[2] = { };
>>
>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> - if (IS_ERR(clk_pclk))
>> - return PTR_ERR(clk_pclk);
>> + clk_baud = devm_clk_get(port->dev, "baud");
>> + if (IS_ERR(clk_baud)) {
>> + dev_err(port->dev, "Failed to get the 'baud' clock\n");
>> + return PTR_ERR(clk_baud);
>> + }
>>
>> - clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> + clk_xtal = devm_clk_get(port->dev, "xtal");
>> if (IS_ERR(clk_xtal))
>> - return PTR_ERR(clk_xtal);
>> -
>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> - if (IS_ERR(clk_baud))
>> - return PTR_ERR(clk_baud);
>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>> + "Failed to get the 'xtal' clock\n");
>> +
>> + if (private_data->use_xtal_clk) {
>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> + "xtal_div");
>> + hw = devm_clk_hw_register_divider_table(port->dev,
>> + clk_name,
>> + __clk_get_name(clk_baud),
>> + CLK_SET_RATE_NO_REPARENT,
>> + port->membase + AML_UART_REG5,
>> + 26, 2,
>> + CLK_DIVIDER_READ_ONLY,
>> + xtal_div_table, NULL);
>> + if (IS_ERR(hw))
>> + return PTR_ERR(hw);
>> +
>> + use_xtal_mux_parents[1].hw = hw;
>> + } else {
>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> + "clk81_div4");
>> + hw = devm_clk_hw_register_fixed_factor(port->dev,
>> + clk_name,
>> + __clk_get_name(clk_baud),
>> + CLK_SET_RATE_NO_REPARENT,
>> + 1, 4);
>> + if (IS_ERR(hw))
>> + return PTR_ERR(hw);
>> +
>> + use_xtal_mux_parents[0].hw = hw;
>> + }
>
> The above is still wrong.
>
> use_xtal_mux_parents initialize both parent to nothing
> And you init the parent in the conditional above.
> It is means only one path is actually set instead of both.
>
> The mux always has 2 sources - Both should be set regardless of the HW version
> You just add
> * /4 on path 0 on legacy SoC
> * the funky divider on path 1 on newer SoC.
>
You do have a point, so I was wondering if you had any suggestions.
>>
>> - port->uartclk = clk_get_rate(clk_baud);
>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> + "use_xtal");
>> + hw = __devm_clk_hw_register_mux(port->dev, NULL,
>> + clk_name,
>> + ARRAY_SIZE(use_xtal_mux_parents),
>> + NULL, NULL,
>> + use_xtal_mux_parents,
>> + CLK_SET_RATE_PARENT,
>> + port->membase + AML_UART_REG5,
>> + 24, 0x1,
>> + CLK_MUX_READ_ONLY,
>> + NULL, NULL);
>> + if (IS_ERR(hw))
>> + return PTR_ERR(hw);
>> +
>> + port->uartclk = clk_hw_get_rate(hw);
>> +
>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev),
>> + "baud_div");
>> + hw = devm_clk_hw_register_divider(port->dev,
>> + clk_name,
>> + clk_hw_get_name(hw),
>> + CLK_SET_RATE_PARENT,
>> + port->membase + AML_UART_REG5,
>> + 0, 23,
>> + CLK_DIVIDER_ROUND_CLOSEST,
>> + NULL);
>> + if (IS_ERR(hw))
>> + return PTR_ERR(hw);
>> +
>> + private_data->baud_clk = hw->clk;
>>
>> return 0;
>> }
>>
>> static int meson_uart_probe(struct platform_device *pdev)
>> {
>> + struct meson_uart_data *private_data;
>> struct resource *res_mem;
>> struct uart_port *port;
>> + struct clk *pclk;
>> u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>> int ret = 0;
>> int irq;
>> @@ -705,6 +764,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>> if (!res_mem)
>> return -ENODEV;
>>
>> + pclk = devm_clk_get(&pdev->dev, "pclk");
>> + if (IS_ERR(pclk))
>> + return dev_err_probe(&pdev->dev, PTR_ERR(pclk),
>> + "Failed to get the 'pclk' clock\n");
>> +
>> + ret = clk_prepare_enable(pclk);
>> + if (ret)
>> + return ret;
>> +
>
> I think this is unbalanced.
I don't understand what you mean. How should I change it? Can you tell
me more specifically.
>
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> return irq;
>> @@ -724,9 +792,13 @@ static int meson_uart_probe(struct platform_device *pdev)
>> if (IS_ERR(port->membase))
>> return PTR_ERR(port->membase);
>>
>> - ret = meson_uart_probe_clocks(pdev, port);
>> - if (ret)
>> - return ret;
>> + private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
>> + GFP_KERNEL);
>> + if (!private_data)
>> + return -ENOMEM;
>> +
>> + if (device_get_match_data(&pdev->dev))
>> + private_data->use_xtal_clk = true;
>>
>> port->iotype = UPIO_MEM;
>> port->mapbase = res_mem->start;
>> @@ -740,6 +812,11 @@ static int meson_uart_probe(struct platform_device *pdev)
>> port->x_char = 0;
>> port->ops = &meson_uart_ops;
>> port->fifosize = fifosize;
>> + port->private_data = private_data;
>> +
>> + ret = meson_uart_probe_clocks(port);
>> + if (ret)
>> + return ret;
>>
>> meson_ports[pdev->id] = port;
>> platform_set_drvdata(pdev, port);
>> @@ -766,10 +843,22 @@ static int meson_uart_remove(struct platform_device *pdev)
>> }
>>
>> 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,meson6-uart",
>> + .data = (void *)false,
>> + },
>> + {
>> + .compatible = "amlogic,meson8-uart",
>> + .data = (void *)false,
>> + },
>> + {
>> + .compatible = "amlogic,meson8b-uart",
>> + .data = (void *)false,
>> + },
>> + {
>> + .compatible = "amlogic,meson-gx-uart",
>> + .data = (void *)true,
>> + },
>> { /* sentinel */ },
>> };
>> MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
>
next prev parent reply other threads:[~2022-02-21 8:52 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-18 3:09 [PATCH V6 0/5] Use CCF to describe the UART baud rate clock Yu Tu
2022-01-18 3:09 ` [PATCH V6 1/5] tty: serial: meson: Move request the register region to probe Yu Tu
2022-01-18 9:35 ` Jiri Slaby
2022-01-18 3:09 ` [PATCH V6 2/5] tty: serial: meson: Use devm_ioremap_resource to get register mapped memory Yu Tu
2022-01-18 9:36 ` Jiri Slaby
2022-01-18 3:09 ` [PATCH V6 3/5] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame Yu Tu
2022-01-18 9:39 ` Jiri Slaby
2022-01-19 6:00 ` Yu Tu
2022-01-20 21:48 ` Jerome Brunet
2022-02-21 8:26 ` Yu Tu
2022-01-20 21:40 ` Jerome Brunet
2022-01-21 2:51 ` Yu Tu
2022-02-21 8:52 ` Yu Tu [this message]
2022-01-18 3:09 ` [PATCH V6 4/5] tty: serial: meson: Make some bit of the REG5 register writable Yu Tu
2022-01-20 21:49 ` Jerome Brunet
2022-02-21 8:24 ` Yu Tu
2022-01-18 3:09 ` [PATCH V6 5/5] tty: serial: meson: Added S4 SOC compatibility Yu Tu
2022-01-19 22:37 ` [PATCH V6 0/5] Use CCF to describe the UART baud rate clock Kevin Hilman
2022-01-20 8:43 ` Yu Tu
2022-01-24 19:58 ` Kevin Hilman
2022-02-07 14: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=0d00b84d-2e8c-3c12-d820-cf6f7142083d@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).