| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1j7dayn4vh.fsf@starbuckisacylon.baylibre.com>
Date: Mon, 17 Jan 2022 09:26:40 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Yu Tu <yu.tu@...ogic.com>, linux-serial@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Jiri Slaby <jirislaby@...nel.org>,
Neil Armstrong <narmstrong@...libre.com>,
Kevin Hilman <khilman@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate
On Tue 11 Jan 2022 at 15:04, Yu Tu <yu.tu@...ogic.com> wrote:
> Hi Jerome,
> Thank you very much for your reply.
>
> On 2022/1/10 22:29, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Mon 10 Jan 2022 at 16:56, Yu Tu <yu.tu@...ogic.com> wrote:
>> Title is bad (like several other patches in this series) - Please fix it
>> Ok.
>>> 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@...ogic.com>
>>> ---
>>> drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>>> 1 file changed, 163 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>> index 7570958d010c..1004fd0b0c9e 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,13 @@
>>> #define AML_UART_POLL_USEC 5
>>> #define AML_UART_TIMEOUT_USEC 10000
>>> +struct meson_uart_data {
>>> + struct uart_port port;
>>> + struct clk *pclk;
>>> + 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];
>>> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
>>> static int meson_uart_startup(struct uart_port *port)
>>> {
>>> u32 val;
>>> - int ret = 0;
>>> + int ret;
>>> - val = readl(port->membase + AML_UART_CONTROL);
>>> - val |= AML_UART_CLEAR_ERR;
>>> - writel(val, port->membase + AML_UART_CONTROL);
>>> - val &= ~AML_UART_CLEAR_ERR;
>>> - writel(val, port->membase + AML_UART_CONTROL);
>>> + meson_uart_reset(port);
>>> + val = readl(port->membase + AML_UART_CONTROL);
>>> val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>>> writel(val, port->membase + AML_UART_CONTROL);
>>> @@ -293,19 +296,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 +396,27 @@ 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);
>>> + clk_disable_unprepare(private_data->pclk);
>>> }
>>> 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->pclk);
>>> + if (ret)
>>> + return ret;
>> In the previous version of the series, you already been asked to the
>> enable of pclk in the probe. Optimization should not be mixed with this
>> series
>> Please make sure all comments are addressed before re-posting
>>
> I'm sorry. I misunderstood you earlier. I'm going to move into the probe
> function.
>>> +
>>> + ret = clk_prepare_enable(private_data->baud_clk);
>>> + if (ret) {
>>> + clk_disable_unprepare(private_data->pclk);
>>> + return ret;
>>> + }
>>> +
>>> return 0;
>>> }
>>> @@ -629,55 +646,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)
>>> +#define CLK_NAME(name) \
>>> +({\
>>> + char clk_name[32];\
>>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
>>> + clk_name;\
>>> +})
>> 1) This result in some ugly code being inlined
>> 2) You return a pointer a local variable which is not gurantee to exist
>> anymore outside of this ...
>> Please do this simply in the related function.
> I've overdone it. I'll move it to the meson_uart_probe_clocks function as
> you suggested.
>>
>>> +
>>> +static struct clk_div_table xtal_div_table[] = {
>>> + { 0, 3 },
>>> + { 1, 1 },
>>> + { 2, 2 },
>>> + { 3, 2 },
>>> +};
>>> +
>>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>> {
>>> - struct clk *clk = NULL;
>>> - int ret;
>>> + struct meson_uart_data *private_data = port->private_data;
>>> + struct clk *clk_baud, *clk_xtal;
>>> + struct clk_hw *hw;
>>> + struct clk_parent_data use_xtal_mux_parents[2] = {
>>> + { .index = -1, },
>>> + { .index = -1, },
>>> + };
>>> - clk = devm_clk_get(dev, id);
>>> - if (IS_ERR(clk))
>>> - return clk;
>>> + private_data->pclk = devm_clk_get(port->dev, "pclk");
>>> + if (IS_ERR(private_data->pclk))
>>> + return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
>>> + "Failed to get the 'pclk' clock\n");
>>> - ret = clk_prepare_enable(clk);
>>> - if (ret) {
>>> - dev_err(dev, "couldn't enable clk\n");
>>> - return ERR_PTR(ret);
>>> + 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);
>>> }
>>> - devm_add_action_or_reset(dev,
>>> - (void(*)(void *))clk_disable_unprepare,
>>> - clk);
>>> -
>>> - return clk;
>>> -}
>>> -
>>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>>> - struct uart_port *port)
>>> -{
>>> - struct clk *clk_xtal = NULL;
>>> - struct clk *clk_pclk = NULL;
>>> - struct clk *clk_baud = NULL;
>>> -
>>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>>> - if (IS_ERR(clk_pclk))
>>> - return PTR_ERR(clk_pclk);
>>> -
>>> - 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) {
>>> + hw = devm_clk_hw_register_divider_table(port->dev,
>>> + CLK_NAME(xtal_div),
>>> + __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 {
>>> + hw = devm_clk_hw_register_fixed_factor(port->dev,
>>> + CLK_NAME(clk81_div4),
>>> + __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;
>>> + }
>> Contradiction with previous series.
>> In the previous series we could clearly see that gxbb did not have the
>> /4 divider. It did not have bits 26 and 27 either.
>> Now gxbb get the /4 divider. It operated correctly without it so far, so
>> this is rather suspicious. Could you please comment on this ?
>>
> As Martin and I discussed earlier.
> meson6 and meson8b SoC.There are not have bits 26 and 27 either.
> CLK81 is recommended for this part of SOC.
>
> GXL,GXM,GXBB and AXG SOC.The UART_EE_A_REG5[26] is added.
> UART_EE_A_REG5[26]:
> - 0x0: divide the input clock by 3 (meaning: this internally works
> with an 8MHz clock)
> - 0x1: use the input clock directly without further division (meaning:
> this internally work with an 24MHz clock)
> 8MHz is recommended for this part of SOC. Their UART IP configuration is
> the same.
>
> G12A/B,,SM1,A1 and S4 SOC.The UART_EE_A_REG5[27] is added.
> UART_EE_A_REG5[27]:
> - 0x0: use the clock as configured in UART_EE_A_REG5[26]
> - 0x1: divide the input clock by 2 (meaning: this internally works
> with an 12MHz clock)
> 12MHz is recommended for this part of SOC.
>
> NOTE:
> 1. clk81 /4 divider.
> There is a 4 divider inside, this is what the internal SOC designer told
> me. But they forgot to document it. So it makes people feel suspicious.
>
> 2.The UART_EE_A_REG5 default value is 0,except for the console port(set in
> romcode).All the baud rate clocks supported by UART can work, but different
> chip usage scenarios suggest using that clock to reduce jitter.
>
> In summary, I would like to know your suggestions, including how to change
> compatible.
> Thank you so much!
>From your comment, gxbb should have "true" instead of false, isn't it ?
>
> >>
>>> - port->uartclk = clk_get_rate(clk_baud);
>>> + hw = __devm_clk_hw_register_mux(port->dev, NULL,
>>> + CLK_NAME(use_xtal),
>>> + 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);
>>> +
>>> + hw = devm_clk_hw_register_divider(port->dev,
>>> + CLK_NAME(baud_div),
>>> + 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 = clk_hw_get_clk(hw, "baud_rate");
>> I have already commented on the use of this function in the previous
>> version.
>> Please make sure all comments are addressed before re-posting
>>
> I'm sorry. I misunderstood you earlier. I will be changed to
> "private_data->baud_clk = hw->clk;".What do you think?
yes
>>> 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;
>>> u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
>>> @@ -716,18 +783,20 @@ static int meson_uart_probe(struct platform_device *pdev)
>>> return -EBUSY;
>>> }
>>> - port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port),
>>> GFP_KERNEL);
>>> - if (!port)
>>> + 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 = &private_data->port;
>>> +
>>> port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
>>> if (IS_ERR(port->membase))
>>> return PTR_ERR(port->membase);
>>> - ret = meson_uart_probe_clocks(pdev, port);
>>> - if (ret)
>>> - return ret;
>>> -
>>> port->iotype = UPIO_MEM;
>>> port->mapbase = res_mem->start;
>>> port->mapsize = resource_size(res_mem);
>>> @@ -740,7 +809,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;
>> port has private_data
>> private_data has port
>> Is it really necessary to have both ... looks to me that port in
>> private_data is overkill
>>
> It's very thoughtful of you. I'll delete it as you suggested.
>>> + ret = meson_uart_probe_clocks(port);
>>> + if (ret)
>>> + return ret;
>>> meson_ports[pdev->id] = port;
>>> platform_set_drvdata(pdev, port);
>>> @@ -766,10 +839,39 @@ 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-gxbb-uart",
>>> + .data = (void *)false,
>>> + },
>>> + {
>>> + .compatible = "amlogic,meson-gxl-uart",
>>> + .data = (void *)true,
>>> + },
>>> + {
>>> + .compatible = "amlogic,meson-g12a-uart",
>>> + .data = (void *)true,
>>> + },
>>> + /*
>>> + * deprecated, don't use anymore because it doesn't differentiate
>>> + * between GXBB, GXL and G12A which have different revisions
>>> + * of the UART IP.
>>> + */
>>> + {
>>> + .compatible = "amlogic,meson-gx-uart",
>>> + .data = (void *)false,
>>> + },
>>> { /* sentinel */ },
>>> };
>>> MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
>>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
Powered by blists - more mailing lists