* [PATCH V4 0/5] the UART driver compatible with @ 2022-01-10 8:55 Yu Tu 2022-01-10 8:56 ` [PATCH V4 1/5] dt-bindings: serial: meson: Drop Yu Tu ` (5 more replies) 0 siblings, 6 replies; 15+ messages in thread From: Yu Tu @ 2022-01-10 8:55 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu 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. Add Meson S4 SoC compatible. Yu Tu (5): dt-bindings: serial: meson: Drop compatible = amlogic,meson-gx-uart. tty: serial: meson: Request the register region in meson_uart_probe() tty: serial: meson: The UART baud rate calculation is described using the common clock code. tty: serial: meson: Make the bit24 and bit [26,27] of the UART_REG5 register writable tty: serial: meson: Added S4 SOC compatibility. V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as discussed in the email V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change the DTS before it can be deleted V3 -> V4: Change CCF to describe the UART baud rate clock as discussed in the email. Link:https://lore.kernel.org/linux-amlogic/20211230102110.3861-4-yu.tu@amlogic.com/ .../bindings/serial/amlogic,meson-uart.yaml | 10 +- drivers/tty/serial/meson_uart.c | 244 ++++++++++++------ 2 files changed, 177 insertions(+), 77 deletions(-) -- 2.33.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V4 1/5] dt-bindings: serial: meson: Drop 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu @ 2022-01-10 8:56 ` Yu Tu 2022-01-10 8:56 ` [PATCH V4 2/5] tty: serial: meson: Request the register Yu Tu ` (4 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Yu Tu @ 2022-01-10 8:56 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu Deprecated, don't use anymore because compatible = amlogic,meson-gx-uart. Don't differentiate between GXBB, GXL and G12A which have different revisions of the UART IP. So it's split into GXBB, GXL and G12A. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml index 72e8868db3e0..ad9f1f4537a0 100644 --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml @@ -28,7 +28,10 @@ properties: - amlogic,meson6-uart - amlogic,meson8-uart - amlogic,meson8b-uart - - amlogic,meson-gx-uart + - amlogic,meson-gx-uart # deprecated, use revision specific property below + - amlogic,meson-gxbb-uart + - amlogic,meson-gxl-uart + - amlogic,meson-g12a-uart - amlogic,meson-s4-uart - const: amlogic,meson-ao-uart - description: Everything-Else power domain UART controller @@ -36,7 +39,10 @@ properties: - amlogic,meson6-uart - amlogic,meson8-uart - amlogic,meson8b-uart - - amlogic,meson-gx-uart + - amlogic,meson-gx-uart # deprecated, use revision specific property below + - amlogic,meson-gxbb-uart + - amlogic,meson-gxl-uart + - amlogic,meson-g12a-uart - amlogic,meson-s4-uart reg: -- 2.33.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V4 2/5] tty: serial: meson: Request the register 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu 2022-01-10 8:56 ` [PATCH V4 1/5] dt-bindings: serial: meson: Drop Yu Tu @ 2022-01-10 8:56 ` Yu Tu 2022-01-10 8:56 ` [PATCH V4 3/5] tty: serial: meson: The UART baud rate Yu Tu ` (3 subsequent siblings) 5 siblings, 0 replies; 15+ messages in thread From: Yu Tu @ 2022-01-10 8:56 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu This simplifies resetting the UART controller during probe and will make it easier to integrate the common clock code which will require the registers at probe time as well. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 45e00d928253..7570958d010c 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port, static void meson_uart_release_port(struct uart_port *port) { - devm_iounmap(port->dev, port->membase); - port->membase = NULL; - devm_release_mem_region(port->dev, port->mapbase, port->mapsize); + /* nothing to do */ } static int meson_uart_request_port(struct uart_port *port) { - if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize, - dev_name(port->dev))) { - dev_err(port->dev, "Memory region busy\n"); - return -EBUSY; - } - - port->membase = devm_ioremap(port->dev, port->mapbase, - port->mapsize); - if (!port->membase) - return -ENOMEM; - return 0; } @@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev) if (!port) return -ENOMEM; + 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; @@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev) platform_set_drvdata(pdev, port); /* reset port before registering (and possibly registering console) */ - if (meson_uart_request_port(port) >= 0) { - meson_uart_reset(port); - meson_uart_release_port(port); - } + meson_uart_reset(port); ret = uart_add_one_port(&meson_uart_driver, port); if (ret) -- 2.33.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu 2022-01-10 8:56 ` [PATCH V4 1/5] dt-bindings: serial: meson: Drop Yu Tu 2022-01-10 8:56 ` [PATCH V4 2/5] tty: serial: meson: Request the register Yu Tu @ 2022-01-10 8:56 ` Yu Tu 2022-01-10 14:29 ` Jerome Brunet ` (2 more replies) 2022-01-10 8:56 ` [PATCH V4 4/5] tty: serial: meson: Make the bit24 and Yu Tu ` (2 subsequent siblings) 5 siblings, 3 replies; 15+ messages in thread From: Yu Tu @ 2022-01-10 8:56 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu 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 | 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; + + 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;\ +}) + +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; + } - 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"); 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; + 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); -- 2.33.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-10 8:56 ` [PATCH V4 3/5] tty: serial: meson: The UART baud rate Yu Tu @ 2022-01-10 14:29 ` Jerome Brunet 2022-01-11 7:04 ` Yu Tu 2022-01-12 16:08 ` kernel test robot 2022-01-12 20:05 ` kernel test robot 2 siblings, 1 reply; 15+ messages in thread From: Jerome Brunet @ 2022-01-10 14:29 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Martin Blumenstingl On Mon 10 Jan 2022 at 16:56, Yu Tu <yu.tu@amlogic.com> wrote: Title is bad (like several other patches in this series) - Please fix it > 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 | 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 > + > + 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. > + > +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 ? > > - 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 > > 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 > > + 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@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-10 14:29 ` Jerome Brunet @ 2022-01-11 7:04 ` Yu Tu 2022-01-17 8:26 ` Jerome Brunet 0 siblings, 1 reply; 15+ messages in thread From: Yu Tu @ 2022-01-11 7:04 UTC (permalink / raw) To: Jerome Brunet, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Martin Blumenstingl 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@amlogic.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@amlogic.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! >> >> - 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? >> >> 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@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-11 7:04 ` Yu Tu @ 2022-01-17 8:26 ` Jerome Brunet 2022-01-17 9:03 ` Yu Tu 0 siblings, 1 reply; 15+ messages in thread From: Jerome Brunet @ 2022-01-17 8:26 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Martin Blumenstingl On Tue 11 Jan 2022 at 15:04, Yu Tu <yu.tu@amlogic.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@amlogic.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@amlogic.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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-17 8:26 ` Jerome Brunet @ 2022-01-17 9:03 ` Yu Tu 0 siblings, 0 replies; 15+ messages in thread From: Yu Tu @ 2022-01-17 9:03 UTC (permalink / raw) To: Jerome Brunet, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Martin Blumenstingl Hi Jerome, Thank you very much for your reply. On 2022/1/17 16:26, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Tue 11 Jan 2022 at 15:04, Yu Tu <yu.tu@amlogic.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@amlogic.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@amlogic.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 ? > Yes, You are right. I don't see the need to split "amlogic, meson-gx-uart" compatible right now. I think the following change { .compatible = "amlogic,meson-gx-uart", .data = (void *)true, }, can satisfy all UART IP using XTAL. Because CCF automatically selects an appropriate clock source when setting baud rate. I wonder if you agree with that. Or maybe you have a better idea. I'd like to know your opinion >> >> >> >>>> - 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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-amlogic > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-10 8:56 ` [PATCH V4 3/5] tty: serial: meson: The UART baud rate Yu Tu 2022-01-10 14:29 ` Jerome Brunet @ 2022-01-12 16:08 ` kernel test robot 2022-01-12 20:05 ` kernel test robot 2 siblings, 0 replies; 15+ messages in thread From: kernel test robot @ 2022-01-12 16:08 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: kbuild-all, Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman Hi Yu, Thank you for the patch! Yet something to improve: [auto build test ERROR on tty/tty-testing] [also build test ERROR on next-20220112] [cannot apply to robh/for-next soc/for-next clk/clk-next v5.16] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Yu-Tu/the-UART-driver-compatible-with/20220110-170020 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing config: m68k-randconfig-m031-20220112 (https://download.01.org/0day-ci/archive/20220113/202201130013.KVlPG5Gj-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/6d55159e6626326d9001d4c8f20b914a69c2c40b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Yu-Tu/the-UART-driver-compatible-with/20220110-170020 git checkout 6d55159e6626326d9001d4c8f20b914a69c2c40b # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): m68k-linux-ld: arch/m68k/kernel/machine_kexec.o: in function `machine_kexec': machine_kexec.c:(.text+0x6a): undefined reference to `m68k_mmutype' m68k-linux-ld: machine_kexec.c:(.text+0x72): undefined reference to `m68k_cputype' m68k-linux-ld: arch/m68k/kernel/relocate_kernel.o:(.m68k_fixup+0x0): undefined reference to `M68K_FIXUP_MEMOFFSET' m68k-linux-ld: arch/m68k/kernel/relocate_kernel.o:(.m68k_fixup+0x8): undefined reference to `M68K_FIXUP_MEMOFFSET' m68k-linux-ld: arch/m68k/kernel/uboot.o: in function `process_uboot_commandline': uboot.c:(.init.text+0x32): undefined reference to `_init_sp' m68k-linux-ld: drivers/tty/serial/meson_uart.o: in function `meson_uart_probe_clocks': meson_uart.c:(.text+0x4de): undefined reference to `__clk_get_name' >> m68k-linux-ld: meson_uart.c:(.text+0x53c): undefined reference to `__devm_clk_hw_register_divider' >> m68k-linux-ld: meson_uart.c:(.text+0x5a8): undefined reference to `__devm_clk_hw_register_mux' >> m68k-linux-ld: meson_uart.c:(.text+0x5c0): undefined reference to `clk_hw_get_rate' >> m68k-linux-ld: meson_uart.c:(.text+0x5d2): undefined reference to `clk_hw_get_name' m68k-linux-ld: meson_uart.c:(.text+0x622): undefined reference to `__devm_clk_hw_register_divider' >> m68k-linux-ld: meson_uart.c:(.text+0x63c): undefined reference to `clk_hw_get_clk' >> m68k-linux-ld: meson_uart.c:(.text+0x666): undefined reference to `__clk_get_name' >> m68k-linux-ld: meson_uart.c:(.text+0x6ae): undefined reference to `devm_clk_hw_register_fixed_factor' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 3/5] tty: serial: meson: The UART baud rate 2022-01-10 8:56 ` [PATCH V4 3/5] tty: serial: meson: The UART baud rate Yu Tu 2022-01-10 14:29 ` Jerome Brunet 2022-01-12 16:08 ` kernel test robot @ 2022-01-12 20:05 ` kernel test robot 2 siblings, 0 replies; 15+ messages in thread From: kernel test robot @ 2022-01-12 20:05 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: kbuild-all, Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman Hi Yu, Thank you for the patch! Yet something to improve: [auto build test ERROR on tty/tty-testing] [also build test ERROR on next-20220112] [cannot apply to robh/for-next soc/for-next clk/clk-next v5.16] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Yu-Tu/the-UART-driver-compatible-with/20220110-170020 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing config: powerpc-randconfig-s031-20220112 (https://download.01.org/0day-ci/archive/20220113/202201130331.dzluDr3h-lkp@intel.com/config) compiler: powerpc-linux-gcc (GCC) 11.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-dirty # https://github.com/0day-ci/linux/commit/6d55159e6626326d9001d4c8f20b914a69c2c40b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Yu-Tu/the-UART-driver-compatible-with/20220110-170020 git checkout 6d55159e6626326d9001d4c8f20b914a69c2c40b # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): powerpc-linux-ld: drivers/tty/serial/meson_uart.o: in function `meson_uart_probe_clocks': meson_uart.c:(.text+0x464): undefined reference to `__clk_get_name' >> powerpc-linux-ld: meson_uart.c:(.text+0x4bc): undefined reference to `__devm_clk_hw_register_divider' >> powerpc-linux-ld: meson_uart.c:(.text+0x4fc): undefined reference to `__clk_get_name' >> powerpc-linux-ld: meson_uart.c:(.text+0x518): undefined reference to `devm_clk_hw_register_fixed_factor' >> powerpc-linux-ld: meson_uart.c:(.text+0x5a8): undefined reference to `__devm_clk_hw_register_mux' >> powerpc-linux-ld: meson_uart.c:(.text+0x5c0): undefined reference to `clk_hw_get_rate' >> powerpc-linux-ld: meson_uart.c:(.text+0x5f8): undefined reference to `clk_hw_get_name' powerpc-linux-ld: meson_uart.c:(.text+0x640): undefined reference to `__devm_clk_hw_register_divider' >> powerpc-linux-ld: meson_uart.c:(.text+0x65c): undefined reference to `clk_hw_get_clk' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V4 4/5] tty: serial: meson: Make the bit24 and 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu ` (2 preceding siblings ...) 2022-01-10 8:56 ` [PATCH V4 3/5] tty: serial: meson: The UART baud rate Yu Tu @ 2022-01-10 8:56 ` Yu Tu 2022-01-10 8:56 ` [PATCH V4 5/5] tty: serial: meson: Added S4 SOC compatibility Yu Tu 2022-01-10 9:27 ` [PATCH V4 0/5] the UART driver compatible with Neil Armstrong 5 siblings, 0 replies; 15+ messages in thread From: Yu Tu @ 2022-01-10 8:56 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu The UART_REG5 register defaults to 0. The console port is set in ROMCODE. But other UART ports default to 0, so make bit24 and bit[26,27] writable so that the UART can choose a more appropriate clock. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 1004fd0b0c9e..fd128878e91a 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -693,7 +693,7 @@ static int meson_uart_probe_clocks(struct uart_port *port) CLK_SET_RATE_NO_REPARENT, port->membase + AML_UART_REG5, 26, 2, - CLK_DIVIDER_READ_ONLY, + CLK_DIVIDER_ROUND_CLOSEST, xtal_div_table, NULL); if (IS_ERR(hw)) return PTR_ERR(hw); @@ -719,7 +719,7 @@ static int meson_uart_probe_clocks(struct uart_port *port) CLK_SET_RATE_PARENT, port->membase + AML_UART_REG5, 24, 0x1, - CLK_MUX_READ_ONLY, + CLK_MUX_ROUND_CLOSEST, NULL, NULL); if (IS_ERR(hw)) return PTR_ERR(hw); -- 2.33.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V4 5/5] tty: serial: meson: Added S4 SOC compatibility. 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu ` (3 preceding siblings ...) 2022-01-10 8:56 ` [PATCH V4 4/5] tty: serial: meson: Make the bit24 and Yu Tu @ 2022-01-10 8:56 ` Yu Tu 2022-01-10 9:27 ` [PATCH V4 0/5] the UART driver compatible with Neil Armstrong 5 siblings, 0 replies; 15+ messages in thread From: Yu Tu @ 2022-01-10 8:56 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu Make UART driver compatible with S4 SOC UART. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index fd128878e91a..d6aa04cc31ba 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -863,6 +863,10 @@ static const struct of_device_id meson_uart_dt_match[] = { .compatible = "amlogic,meson-g12a-uart", .data = (void *)true, }, + { + .compatible = "amlogic,meson-s4-uart", + .data = (void *)true, + }, /* * deprecated, don't use anymore because it doesn't differentiate * between GXBB, GXL and G12A which have different revisions -- 2.33.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V4 0/5] the UART driver compatible with 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu ` (4 preceding siblings ...) 2022-01-10 8:56 ` [PATCH V4 5/5] tty: serial: meson: Added S4 SOC compatibility Yu Tu @ 2022-01-10 9:27 ` Neil Armstrong 2022-01-10 9:35 ` Yu Tu 5 siblings, 1 reply; 15+ messages in thread From: Neil Armstrong @ 2022-01-10 9:27 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Hi, On 10/01/2022 09:55, Yu Tu 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. Add Meson S4 SoC compatible. > > Yu Tu (5): > dt-bindings: serial: meson: Drop compatible = amlogic,meson-gx-uart. > tty: serial: meson: Request the register region in meson_uart_probe() > tty: serial: meson: The UART baud rate calculation is described using > the common clock code. > tty: serial: meson: Make the bit24 and bit [26,27] of the UART_REG5 > register writable > tty: serial: meson: Added S4 SOC compatibility. Weird, the subjects are fine in the cover letter but are all truncated in the email thread: [PATCH V4 0/5] the UART driver compatible with [PATCH V4 1/5] dt-bindings: serial: meson: Drop [PATCH V4 2/5] tty: serial: meson: Request the register ... Only the last one is OK. > > V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as > discussed in the email > V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change > the DTS before it can be deleted > V3 -> V4: Change CCF to describe the UART baud rate clock as discussed > in the email. > > Link:https://lore.kernel.org/linux-amlogic/20211230102110.3861-4-yu.tu@amlogic.com/ > > .../bindings/serial/amlogic,meson-uart.yaml | 10 +- > drivers/tty/serial/meson_uart.c | 244 ++++++++++++------ > 2 files changed, 177 insertions(+), 77 deletions(-) > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 0/5] the UART driver compatible with 2022-01-10 9:27 ` [PATCH V4 0/5] the UART driver compatible with Neil Armstrong @ 2022-01-10 9:35 ` Yu Tu 2022-01-10 9:43 ` Neil Armstrong 0 siblings, 1 reply; 15+ messages in thread From: Yu Tu @ 2022-01-10 9:35 UTC (permalink / raw) To: Neil Armstrong, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Hi Neil, Thank you very much for your reply. On 2022/1/10 17:27, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > Hi, > > On 10/01/2022 09:55, Yu Tu 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. Add Meson S4 SoC compatible. >> >> Yu Tu (5): >> dt-bindings: serial: meson: Drop compatible = amlogic,meson-gx-uart. >> tty: serial: meson: Request the register region in meson_uart_probe() >> tty: serial: meson: The UART baud rate calculation is described using >> the common clock code. >> tty: serial: meson: Make the bit24 and bit [26,27] of the UART_REG5 >> register writable >> tty: serial: meson: Added S4 SOC compatibility. > > Weird, the subjects are fine in the cover letter but are all truncated in the email thread: > [PATCH V4 0/5] the UART driver compatible with > [PATCH V4 1/5] dt-bindings: serial: meson: Drop > [PATCH V4 2/5] tty: serial: meson: Request the register > ... > Only the last one is OK. > I saw the subject was a little long, I changed it manually, do I need to regenerate and send? > >> >> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as >> discussed in the email >> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change >> the DTS before it can be deleted >> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed >> in the email. >> >> Link:https://lore.kernel.org/linux-amlogic/20211230102110.3861-4-yu.tu@amlogic.com/ >> >> .../bindings/serial/amlogic,meson-uart.yaml | 10 +- >> drivers/tty/serial/meson_uart.c | 244 ++++++++++++------ >> 2 files changed, 177 insertions(+), 77 deletions(-) >> > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V4 0/5] the UART driver compatible with 2022-01-10 9:35 ` Yu Tu @ 2022-01-10 9:43 ` Neil Armstrong 0 siblings, 0 replies; 15+ messages in thread From: Neil Armstrong @ 2022-01-10 9:43 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, devicetree Cc: Greg Kroah-Hartman, Rob Herring, Jiri Slaby, Kevin Hilman, Jerome Brunet, Martin Blumenstingl On 10/01/2022 10:35, Yu Tu wrote: > Hi Neil, > Thank you very much for your reply. > > On 2022/1/10 17:27, Neil Armstrong wrote: >> [ EXTERNAL EMAIL ] >> >> Hi, >> >> On 10/01/2022 09:55, Yu Tu 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. Add Meson S4 SoC compatible. >>> >>> Yu Tu (5): >>> dt-bindings: serial: meson: Drop compatible = amlogic,meson-gx-uart. >>> tty: serial: meson: Request the register region in meson_uart_probe() >>> tty: serial: meson: The UART baud rate calculation is described using >>> the common clock code. >>> tty: serial: meson: Make the bit24 and bit [26,27] of the UART_REG5 >>> register writable >>> tty: serial: meson: Added S4 SOC compatibility. >> >> Weird, the subjects are fine in the cover letter but are all truncated in the email thread: >> [PATCH V4 0/5] the UART driver compatible with >> [PATCH V4 1/5] dt-bindings: serial: meson: Drop >> [PATCH V4 2/5] tty: serial: meson: Request the register >> ... >> Only the last one is OK. >> > I saw the subject was a little long, I changed it manually, do I need to regenerate and send? yes please, the mail subject will be the commit title so it must be short but descriptive enough. "dt-bindings: serial: meson: Drop" is too short. "dt-bindings: serial: meson: Drop compatible = amlogic,meson-gx-uart" is ok but formulation is wrong, and remove special characters like "=", "[", "]"... The best title could be : "dt-bindings: serial: meson: Drop legacy amlogic,meson-gx-uart compatible" If the subject it too long, reduce it and enhance the commit message with the details. Neil >> >>> >>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as >>> discussed in the email >>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change >>> the DTS before it can be deleted >>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed >>> in the email. >>> >>> Link:https://lore.kernel.org/linux-amlogic/20211230102110.3861-4-yu.tu@amlogic.com/ >>> >>> .../bindings/serial/amlogic,meson-uart.yaml | 10 +- >>> drivers/tty/serial/meson_uart.c | 244 ++++++++++++------ >>> 2 files changed, 177 insertions(+), 77 deletions(-) >>> >> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-01-17 9:03 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-10 8:55 [PATCH V4 0/5] the UART driver compatible with Yu Tu 2022-01-10 8:56 ` [PATCH V4 1/5] dt-bindings: serial: meson: Drop Yu Tu 2022-01-10 8:56 ` [PATCH V4 2/5] tty: serial: meson: Request the register Yu Tu 2022-01-10 8:56 ` [PATCH V4 3/5] tty: serial: meson: The UART baud rate Yu Tu 2022-01-10 14:29 ` Jerome Brunet 2022-01-11 7:04 ` Yu Tu 2022-01-17 8:26 ` Jerome Brunet 2022-01-17 9:03 ` Yu Tu 2022-01-12 16:08 ` kernel test robot 2022-01-12 20:05 ` kernel test robot 2022-01-10 8:56 ` [PATCH V4 4/5] tty: serial: meson: Make the bit24 and Yu Tu 2022-01-10 8:56 ` [PATCH V4 5/5] tty: serial: meson: Added S4 SOC compatibility Yu Tu 2022-01-10 9:27 ` [PATCH V4 0/5] the UART driver compatible with Neil Armstrong 2022-01-10 9:35 ` Yu Tu 2022-01-10 9:43 ` Neil Armstrong
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).