* [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson @ 2021-12-30 10:21 Yu Tu 2021-12-30 10:21 ` [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code Yu Tu ` (5 more replies) 0 siblings, 6 replies; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu 1.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. 2.Fix some omissions 3.An interrupt error occurs when the user opens (/dev/ttyAML0) twice in a row Yu Tu (6): tty: serial: meson: Drop the legacy compatible strings and clock code tty: serial: meson: Request the register region in meson_uart_probe() dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart. tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() 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 Link:https://lore.kernel.org/linux-amlogic/20211221071634.25980-2-yu.tu@amlogic.com/ .../bindings/serial/amlogic,meson-uart.yaml | 10 +- drivers/tty/serial/Kconfig | 1 + drivers/tty/serial/meson_uart.c | 367 +++++++++++++----- 3 files changed, 273 insertions(+), 105 deletions(-) -- 2.33.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu @ 2021-12-30 10:21 ` Yu Tu 2021-12-30 22:22 ` Martin Blumenstingl 2021-12-30 10:21 ` [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() Yu Tu ` (4 subsequent siblings) 5 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu All mainline .dts files have been using the stable UART since Linux 4.16. Drop the legacy compatible strings and related clock code. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 34 ++------------------------------- 1 file changed, 2 insertions(+), 32 deletions(-) diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index d2c08b760f83..c9a37602ffd0 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -625,10 +625,7 @@ meson_serial_early_console_setup(struct earlycon_device *device, const char *opt device->con->write = meson_serial_early_console_write; return 0; } -/* Legacy bindings, should be removed when no more used */ -OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart", - meson_serial_early_console_setup); -/* Stable bindings */ + OF_EARLYCON_DECLARE(meson, "amlogic,meson-ao-uart", meson_serial_early_console_setup); @@ -668,25 +665,6 @@ static inline struct clk *meson_uart_probe_clock(struct device *dev, return clk; } -/* - * This function gets clocks in the legacy non-stable DT bindings. - * This code will be remove once all the platforms switch to the - * new DT bindings. - */ -static int meson_uart_probe_clocks_legacy(struct platform_device *pdev, - struct uart_port *port) -{ - struct clk *clk = NULL; - - clk = meson_uart_probe_clock(&pdev->dev, NULL); - if (IS_ERR(clk)) - return PTR_ERR(clk); - - port->uartclk = clk_get_rate(clk); - - return 0; -} - static int meson_uart_probe_clocks(struct platform_device *pdev, struct uart_port *port) { @@ -750,12 +728,7 @@ static int meson_uart_probe(struct platform_device *pdev) if (!port) return -ENOMEM; - /* Use legacy way until all platforms switch to new bindings */ - if (of_device_is_compatible(pdev->dev.of_node, "amlogic,meson-uart")) - ret = meson_uart_probe_clocks_legacy(pdev, port); - else - ret = meson_uart_probe_clocks(pdev, port); - + ret = meson_uart_probe_clocks(pdev, port); if (ret) return ret; @@ -800,9 +773,6 @@ static int meson_uart_remove(struct platform_device *pdev) } static const struct of_device_id meson_uart_dt_match[] = { - /* Legacy bindings, should be removed when no more used */ - { .compatible = "amlogic,meson-uart" }, - /* Stable bindings */ { .compatible = "amlogic,meson6-uart" }, { .compatible = "amlogic,meson8-uart" }, { .compatible = "amlogic,meson8b-uart" }, -- 2.33.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2021-12-30 10:21 ` [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code Yu Tu @ 2021-12-30 22:22 ` Martin Blumenstingl 2021-12-31 10:27 ` Yu Tu 0 siblings, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-30 22:22 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On Thu, Dec 30, 2021 at 11:21 AM Yu Tu <yu.tu@amlogic.com> wrote: > > All mainline .dts files have been using the stable UART since Linux > 4.16. Drop the legacy compatible strings and related clock code. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> I have just realized that this cannot be dropped until my series "ARM: dts: meson: fix UART device-tree schema validation" [0] is merged [...] > -/* Legacy bindings, should be removed when no more used */ > -OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart", > - meson_serial_early_console_setup); This part is still needed as long as above series is not merged yet. If we remove this then earlycon will stop working on the 32-bit SoCs unless [0] is merged. All other code below - except the of_device_id entry - can still be removed since meson8.dtsi and meson8b.dtsi are using the non-legacy clocks already. Sorry for only noticing this now. Best regards, Martin [0] https://patchwork.kernel.org/project/linux-amlogic/cover/20211227180026.4068352-1-martin.blumenstingl@googlemail.com/ ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2021-12-30 22:22 ` Martin Blumenstingl @ 2021-12-31 10:27 ` Yu Tu 2021-12-31 15:35 ` Martin Blumenstingl 0 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2021-12-31 10:27 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet Hi Martin, Thank you very much for your reply. On 2021/12/31 6:22, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > On Thu, Dec 30, 2021 at 11:21 AM Yu Tu <yu.tu@amlogic.com> wrote: >> >> All mainline .dts files have been using the stable UART since Linux >> 4.16. Drop the legacy compatible strings and related clock code. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> > I have just realized that this cannot be dropped until my series "ARM: > dts: meson: fix UART device-tree schema validation" [0] is merged > > [...] >> -/* Legacy bindings, should be removed when no more used */ >> -OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart", >> - meson_serial_early_console_setup); > This part is still needed as long as above series is not merged yet. > If we remove this then earlycon will stop working on the 32-bit SoCs > unless [0] is merged. > > All other code below - except the of_device_id entry - can still be > removed since meson8.dtsi and meson8b.dtsi are using the non-legacy > clocks already. > > Sorry for only noticing this now. > I will add it back in the next patch and delete it after your submission is merged. > > Best regards, > Martin > > > [0] https://patchwork.kernel.org/project/linux-amlogic/cover/20211227180026.4068352-1-martin.blumenstingl@googlemail.com/ > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2021-12-31 10:27 ` Yu Tu @ 2021-12-31 15:35 ` Martin Blumenstingl 2022-01-03 14:59 ` Neil Armstrong 0 siblings, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-31 15:35 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On Fri, Dec 31, 2021 at 11:27 AM Yu Tu <yu.tu@amlogic.com> wrote: [...] > >> -/* Legacy bindings, should be removed when no more used */ > >> -OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart", > >> - meson_serial_early_console_setup); > > This part is still needed as long as above series is not merged yet. > > If we remove this then earlycon will stop working on the 32-bit SoCs > > unless [0] is merged. > > > > All other code below - except the of_device_id entry - can still be > > removed since meson8.dtsi and meson8b.dtsi are using the non-legacy > > clocks already. > > > > Sorry for only noticing this now. > > > I will add it back in the next patch and delete it after your submission > is merged. I have just seen that Greg has already added this patch to the tty-next tree [1] In this case there's nothing to do on your end - I'll simply ask Neil to also queue my 32-bit SoC UART .dts fixes [0] for 5.17 Best regards, Martin [0] https://patchwork.kernel.org/project/linux-amlogic/cover/20211227180026.4068352-1-martin.blumenstingl@googlemail.com/ [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=ad234e2bac274a43c9fa540bde8cd9f0c627b71f ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2021-12-31 15:35 ` Martin Blumenstingl @ 2022-01-03 14:59 ` Neil Armstrong 2022-01-03 15:19 ` Greg Kroah-Hartman 0 siblings, 1 reply; 39+ messages in thread From: Neil Armstrong @ 2022-01-03 14:59 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu Hi Greg, Martin just saw this patch was applied, but the serie wasn't reviewed enough and it will break earlycon support on the ARMv7 Amlogic SoCs fore 5.17. Anyway, I'll push the corresponding DT fixes for 5.17-rc1. Would it be possible we also receive the notification when those patches are applied ? Maybe a MAINTAINERS entry is missing so we can receive them ? It would help me track those TTY and USB patches more easily. Thanks ! Neil On 31/12/2021 16:35, Martin Blumenstingl wrote: > On Fri, Dec 31, 2021 at 11:27 AM Yu Tu <yu.tu@amlogic.com> wrote: > [...] >>>> -/* Legacy bindings, should be removed when no more used */ >>>> -OF_EARLYCON_DECLARE(meson, "amlogic,meson-uart", >>>> - meson_serial_early_console_setup); >>> This part is still needed as long as above series is not merged yet. >>> If we remove this then earlycon will stop working on the 32-bit SoCs >>> unless [0] is merged. >>> >>> All other code below - except the of_device_id entry - can still be >>> removed since meson8.dtsi and meson8b.dtsi are using the non-legacy >>> clocks already. >>> >>> Sorry for only noticing this now. >>> >> I will add it back in the next patch and delete it after your submission >> is merged. > I have just seen that Greg has already added this patch to the tty-next tree [1] > In this case there's nothing to do on your end - I'll simply ask Neil > to also queue my 32-bit SoC UART .dts fixes [0] for 5.17 > > > Best regards, > Martin > > > [0] https://patchwork.kernel.org/project/linux-amlogic/cover/20211227180026.4068352-1-martin.blumenstingl@googlemail.com/ > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=ad234e2bac274a43c9fa540bde8cd9f0c627b71f > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2022-01-03 14:59 ` Neil Armstrong @ 2022-01-03 15:19 ` Greg Kroah-Hartman 2022-01-03 15:29 ` Neil Armstrong 0 siblings, 1 reply; 39+ messages in thread From: Greg Kroah-Hartman @ 2022-01-03 15:19 UTC (permalink / raw) To: Neil Armstrong Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Mon, Jan 03, 2022 at 03:59:33PM +0100, Neil Armstrong wrote: > Hi Greg, > > Martin just saw this patch was applied, but the serie wasn't reviewed enough and it will break > earlycon support on the ARMv7 Amlogic SoCs fore 5.17. Ok, what should I revert? > Anyway, I'll push the corresponding DT fixes for 5.17-rc1. How did we get out of sync here? > Would it be possible we also receive the notification when those patches are applied ? > Maybe a MAINTAINERS entry is missing so we can receive them ? That would be good, so that people can review the patches. Otherwise I have to just guess :) > It would help me track those TTY and USB patches more easily. I recommend MAINTAINERS entries for drivers that are not listed and that you care about seeing the changes for. thanks, greg k-h ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2022-01-03 15:19 ` Greg Kroah-Hartman @ 2022-01-03 15:29 ` Neil Armstrong 2022-01-03 16:29 ` Greg Kroah-Hartman 0 siblings, 1 reply; 39+ messages in thread From: Neil Armstrong @ 2022-01-03 15:29 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu Hi Greg, On 03/01/2022 16:19, Greg Kroah-Hartman wrote: > A: http://en.wikipedia.org/wiki/Top_post > Q: Were do I find info about this thing called top-posting? > A: Because it messes up the order in which people normally read text. > Q: Why is top-posting such a bad thing? > A: Top-posting. > Q: What is the most annoying thing in e-mail? > > A: No. > Q: Should I include quotations after my reply? > > http://daringfireball.net/2007/07/on_to> > On Mon, Jan 03, 2022 at 03:59:33PM +0100, Neil Armstrong wrote: >> Hi Greg, >> >> Martin just saw this patch was applied, but the serie wasn't reviewed enough and it will break >> earlycon support on the ARMv7 Amlogic SoCs fore 5.17. > > Ok, what should I revert? None, we have a fix in the pipe > >> Anyway, I'll push the corresponding DT fixes for 5.17-rc1. > > How did we get out of sync here? The serie wasn't fully reviewed, and I was out of office when it was applied. > >> Would it be possible we also receive the notification when those patches are applied ? >> Maybe a MAINTAINERS entry is missing so we can receive them ? > > That would be good, so that people can review the patches. Otherwise I > have to just guess :) exact, I naively thought it would be matched in the: N: meson entry but it seems an proper entry for drivers/tty/serial/meson_uart.c is needed. > >> It would help me track those TTY and USB patches more easily. > > I recommend MAINTAINERS entries for drivers that are not listed and that > you care about seeing the changes for. This is why we have a regex to match these. I can submit a patch to have a proper entry if the regex is not enough/appropriate. BTW can you point us how are selected the recipients of the notification messages you send ? Thanks, Neil > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2022-01-03 15:29 ` Neil Armstrong @ 2022-01-03 16:29 ` Greg Kroah-Hartman 2022-01-03 16:35 ` Neil Armstrong 0 siblings, 1 reply; 39+ messages in thread From: Greg Kroah-Hartman @ 2022-01-03 16:29 UTC (permalink / raw) To: Neil Armstrong Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu On Mon, Jan 03, 2022 at 04:29:56PM +0100, Neil Armstrong wrote: > Hi Greg, > On 03/01/2022 16:19, Greg Kroah-Hartman wrote: > > A: http://en.wikipedia.org/wiki/Top_post > > Q: Were do I find info about this thing called top-posting? > > A: Because it messes up the order in which people normally read text. > > Q: Why is top-posting such a bad thing? > > A: Top-posting. > > Q: What is the most annoying thing in e-mail? > > > > A: No. > > Q: Should I include quotations after my reply? > > > > http://daringfireball.net/2007/07/on_to> > > On Mon, Jan 03, 2022 at 03:59:33PM +0100, Neil Armstrong wrote: > >> Hi Greg, > >> > >> Martin just saw this patch was applied, but the serie wasn't reviewed enough and it will break > >> earlycon support on the ARMv7 Amlogic SoCs fore 5.17. > > > > Ok, what should I revert? > > None, we have a fix in the pipe > > > > >> Anyway, I'll push the corresponding DT fixes for 5.17-rc1. > > > > How did we get out of sync here? > > The serie wasn't fully reviewed, and I was out of office when it was applied. > > > > >> Would it be possible we also receive the notification when those patches are applied ? > >> Maybe a MAINTAINERS entry is missing so we can receive them ? > > > > That would be good, so that people can review the patches. Otherwise I > > have to just guess :) > > exact, I naively thought it would be matched in the: > N: meson > entry but it seems an proper entry for drivers/tty/serial/meson_uart.c is needed. Try it, does that work when running get_maintainer.pl on this patch? > >> It would help me track those TTY and USB patches more easily. > > > > I recommend MAINTAINERS entries for drivers that are not listed and that > > you care about seeing the changes for. > > This is why we have a regex to match these. Great, the submitter should have used that. > I can submit a patch to have a proper entry if the regex is not enough/appropriate. Whatever makes the tools work is fine with me. > BTW can you point us how are selected the recipients of the notification messages you send ? For when I apply a patch, everyone on the ack/signed-off-by/reviewed-by list gets a response. I do not hit mailing lists with the notification as that's just too much noise. Been doing it this way for well over a decade now, nothing new here :) thanks, greg k-h ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code 2022-01-03 16:29 ` Greg Kroah-Hartman @ 2022-01-03 16:35 ` Neil Armstrong 0 siblings, 0 replies; 39+ messages in thread From: Neil Armstrong @ 2022-01-03 16:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu On 03/01/2022 17:29, Greg Kroah-Hartman wrote: > On Mon, Jan 03, 2022 at 04:29:56PM +0100, Neil Armstrong wrote: >>>> Would it be possible we also receive the notification when those patches are applied ? >>>> Maybe a MAINTAINERS entry is missing so we can receive them ? >>> >>> That would be good, so that people can review the patches. Otherwise I >>> have to just guess :) >> >> exact, I naively thought it would be matched in the: >> N: meson >> entry but it seems an proper entry for drivers/tty/serial/meson_uart.c is needed. > > Try it, does that work when running get_maintainer.pl on this patch? Yes it does work, so no problem here > >>>> It would help me track those TTY and USB patches more easily. >>> >>> I recommend MAINTAINERS entries for drivers that are not listed and that >>> you care about seeing the changes for. >> >> This is why we have a regex to match these. > > Great, the submitter should have used that. > >> I can submit a patch to have a proper entry if the regex is not enough/appropriate. > > Whatever makes the tools work is fine with me. > >> BTW can you point us how are selected the recipients of the notification messages you send ? > > For when I apply a patch, everyone on the ack/signed-off-by/reviewed-by > list gets a response. I do not hit mailing lists with the notification > as that's just too much noise. > > Been doing it this way for well over a decade now, nothing new here :) Ok right, no problem, other maintainers (e.g: net, sound) and default b4 behavior is to send notification to same recipient as original patch. It it fits everyone for a decade, no need to change ! We have very low patches for tty & usb anyway Neil > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu 2021-12-30 10:21 ` [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code Yu Tu @ 2021-12-30 10:21 ` Yu Tu 2021-12-30 12:29 ` Greg Kroah-Hartman 2021-12-30 10:21 ` [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart Yu Tu ` (3 subsequent siblings) 5 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, 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 c9a37602ffd0..99efe62a1507 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -397,24 +397,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; } @@ -728,6 +715,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; @@ -749,10 +740,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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() 2021-12-30 10:21 ` [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() Yu Tu @ 2021-12-30 12:29 ` Greg Kroah-Hartman 2021-12-30 22:28 ` Martin Blumenstingl 0 siblings, 1 reply; 39+ messages in thread From: Greg Kroah-Hartman @ 2021-12-30 12:29 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl On Thu, Dec 30, 2021 at 06:21:06PM +0800, Yu Tu wrote: > 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 c9a37602ffd0..99efe62a1507 100644 > --- a/drivers/tty/serial/meson_uart.c > +++ b/drivers/tty/serial/meson_uart.c > @@ -397,24 +397,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 */ > } Are you sure a release call like this can be "empty"? That goes against the normal way the driver model works. If it is empty, why have it at all? thanks, greg k-h ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() 2021-12-30 12:29 ` Greg Kroah-Hartman @ 2021-12-30 22:28 ` Martin Blumenstingl 0 siblings, 0 replies; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-30 22:28 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet Hi Greg, On Thu, Dec 30, 2021 at 1:29 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: [...] > > 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 */ > > } > > Are you sure a release call like this can be "empty"? That goes against > the normal way the driver model works. If it is empty, why have it at > all? In patch #4 from this series some logic is added here again. I can think of three options here (in no particular order): - keep this patch as-is - remove the empty function from this patch and it back in patch #4 from this series - try to split some of the logic from patch #4 so the relevant clk_prepare_enable/clk_disable_unpepare calls are added here (meaning that this function has logic inside it at all times throughout this series) - (if you have another suggestion then please let us know) Best regards, Martin ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart. 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu 2021-12-30 10:21 ` [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code Yu Tu 2021-12-30 10:21 ` [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() Yu Tu @ 2021-12-30 10:21 ` Yu Tu 2021-12-30 12:28 ` Greg Kroah-Hartman 2021-12-30 22:34 ` Martin Blumenstingl 2021-12-30 10:21 ` [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible Yu Tu ` (2 subsequent siblings) 5 siblings, 2 replies; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu Deprecated, don't use anymore because compatible = amlogic,meson-gx-uart don't differentiate between GXBB and GXL which have different revisions of the UART IP. So it's split into GXBB and GXL. 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 75ebc9952a99..b03040a83a9f 100644 --- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml +++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml @@ -28,14 +28,20 @@ properties: - amlogic,meson6-uart - amlogic,meson8-uart - amlogic,meson8b-uart - - amlogic,meson-gx-uart + - 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 enum: - amlogic,meson6-uart - amlogic,meson8-uart - amlogic,meson8b-uart - - amlogic,meson-gx-uart + - amlogic,meson-gxbb-uart + - amlogic,meson-gxl-uart + - amlogic,meson-g12a-uart + - amlogic,meson-s4-uart reg: maxItems: 1 -- 2.33.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart. 2021-12-30 10:21 ` [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart Yu Tu @ 2021-12-30 12:28 ` Greg Kroah-Hartman 2021-12-31 10:18 ` Yu Tu 2021-12-30 22:34 ` Martin Blumenstingl 1 sibling, 1 reply; 39+ messages in thread From: Greg Kroah-Hartman @ 2021-12-30 12:28 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl On Thu, Dec 30, 2021 at 06:21:07PM +0800, Yu Tu wrote: > Deprecated, don't use anymore because compatible = amlogic,meson-gx-uart > don't differentiate between GXBB and GXL which have different > revisions of the UART IP. So it's split into GXBB and GXL. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) Again, your subject line is way too long. thanks, greg k-h ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart. 2021-12-30 12:28 ` Greg Kroah-Hartman @ 2021-12-31 10:18 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2021-12-31 10:18 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl Hi Greg, Thank you for your reply. On 2021/12/30 20:28, Greg Kroah-Hartman wrote: > [ EXTERNAL EMAIL ] > > On Thu, Dec 30, 2021 at 06:21:07PM +0800, Yu Tu wrote: >> Deprecated, don't use anymore because compatible = amlogic,meson-gx-uart >> don't differentiate between GXBB and GXL which have different >> revisions of the UART IP. So it's split into GXBB and GXL. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) > > Again, your subject line is way too long. > I will correct. > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart. 2021-12-30 10:21 ` [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart Yu Tu 2021-12-30 12:28 ` Greg Kroah-Hartman @ 2021-12-30 22:34 ` Martin Blumenstingl 2021-12-31 10:35 ` Yu Tu 1 sibling, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-30 22:34 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet Hello, as Greg already mentioned the $subject line is very long. On Thu, Dec 30, 2021 at 11:21 AM Yu Tu <yu.tu@amlogic.com> wrote: > Deprecated, don't use anymore because compatible = amlogic,meson-gx-uart > don't differentiate between GXBB and GXL which have different > revisions of the UART IP. So it's split into GXBB and GXL. actually it's split into GXBB, GXL and G12A [...] > - - amlogic,meson-gx-uart > + - amlogic,meson-gxbb-uart > + - amlogic,meson-gxl-uart > + - amlogic,meson-g12a-uart > + - amlogic,meson-s4-uart In addition to Greg's comment I suggest splitting this into two patches: - one where the "amlogic,meson-gx-uart" compatible is marked as deprecated (Documentation/devicetree/bindings/power/supply/bq27xxx.yaml has an example for deprecated entries) and GXBB, GXL and G12A compatible strings are added instead - another one where the new S4 compatible string is added The idea here is to have "one logical change per patch". Deprecating and replacing "amlogic,meson-gx-uart" is one logical change. Adding a new compatible string is another logical change. I am hoping that this will also make it easier to find a shorter $subject line (which according to the patch submission guide [0] should be 70-75 characters: "the summary must be no more than 70-75 characters") Best regards, Martin [0] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#the-canonical-patch-format ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart. 2021-12-30 22:34 ` Martin Blumenstingl @ 2021-12-31 10:35 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2021-12-31 10:35 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On 2021/12/31 6:34, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > Hello, > > as Greg already mentioned the $subject line is very long. > > On Thu, Dec 30, 2021 at 11:21 AM Yu Tu <yu.tu@amlogic.com> wrote: >> Deprecated, don't use anymore because compatible = amlogic,meson-gx-uart >> don't differentiate between GXBB and GXL which have different >> revisions of the UART IP. So it's split into GXBB and GXL. > actually it's split into GXBB, GXL and G12A > > [...] >> - - amlogic,meson-gx-uart >> + - amlogic,meson-gxbb-uart >> + - amlogic,meson-gxl-uart >> + - amlogic,meson-g12a-uart >> + - amlogic,meson-s4-uart > In addition to Greg's comment I suggest splitting this into two patches: > - one where the "amlogic,meson-gx-uart" compatible is marked as > deprecated (Documentation/devicetree/bindings/power/supply/bq27xxx.yaml > has an example for deprecated entries) and GXBB, GXL and G12A > compatible strings are added instead > - another one where the new S4 compatible string is added > > The idea here is to have "one logical change per patch". > Deprecating and replacing "amlogic,meson-gx-uart" is one logical change. > Adding a new compatible string is another logical change. > I am hoping that this will also make it easier to find a shorter > $subject line (which according to the patch submission guide [0] > should be 70-75 characters: "the summary must be no more than 70-75 > characters") > I will split the two patches in the next version.One was deprecating and replacing "AmLogic, Meson-GX-Uart" and the other added S4 compatible. > > Best regards, > Martin > > > [0] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#the-canonical-patch-format > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu ` (2 preceding siblings ...) 2021-12-30 10:21 ` [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart Yu Tu @ 2021-12-30 10:21 ` Yu Tu 2021-12-30 12:27 ` Greg Kroah-Hartman ` (2 more replies) 2021-12-30 10:21 ` [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit Yu Tu 2021-12-30 10:21 ` [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() Yu Tu 5 siblings, 3 replies; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, 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/Kconfig | 1 + drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------ 2 files changed, 257 insertions(+), 55 deletions(-) diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 780908d43557..32e238173036 100644 --- a/drivers/tty/serial/Kconfig +++ b/drivers/tty/serial/Kconfig @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI config SERIAL_MESON tristate "Meson serial port support" depends on ARCH_MESON + depends on COMMON_CLK select SERIAL_CORE help This enables the driver for the on-chip UARTs of the Amlogic diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 99efe62a1507..07eb1f40aaaa 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,21 @@ #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; + struct clk_divider baud_div; + struct clk_mux use_xtal_mux; + struct clk_mux xtal_clk_sel_mux; + struct clk_mux xtal2_clk_sel_mux; + struct clk_fixed_factor xtal_div2; + struct clk_fixed_factor xtal_div3; + struct clk_fixed_factor clk81_div4; + bool no_clk81_input; + bool has_xtal_clk_sel; +}; + static struct uart_driver meson_uart_driver; static struct uart_port *meson_ports[AML_UART_PORT_NUM]; @@ -270,14 +284,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); @@ -295,19 +306,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, @@ -397,11 +406,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,56 +654,175 @@ 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) +static int meson_uart_register_clk(struct uart_port *port, + const char *name_suffix, + const struct clk_parent_data *parent_data, + unsigned int num_parents, + const struct clk_ops *ops, + struct clk_hw *hw) { - struct clk *clk = NULL; + struct clk_init_data init = { }; + char clk_name[32]; int ret; - clk = devm_clk_get(dev, id); - if (IS_ERR(clk)) - return clk; + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), + name_suffix); - ret = clk_prepare_enable(clk); - if (ret) { - dev_err(dev, "couldn't enable clk\n"); - return ERR_PTR(ret); - } + init.name = clk_name; + init.ops = ops; + init.flags = CLK_SET_RATE_PARENT; + init.parent_data = parent_data; + init.num_parents = num_parents; + + hw->init = &init; - devm_add_action_or_reset(dev, - (void(*)(void *))clk_disable_unprepare, - clk); + ret = devm_clk_hw_register(port->dev, hw); + if (ret) + return dev_err_probe(port->dev, ret, + "Failed to register the '%s' clock\n", + clk_name); - return clk; + return ret; } -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; +static int meson_uart_probe_clocks(struct uart_port *port, + bool register_clk81_div4) +{ + struct meson_uart_data *private_data = port->private_data; + struct clk_parent_data use_xtal_mux_parents[2] = { + { .index = -1, }, + { .index = -1, }, + }; + struct clk_parent_data xtal_clk_sel_mux_parents[2] = { }; + struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { }; + struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", }; + struct clk_parent_data clk81_div_parent = { .fw_name = "baud", }; + struct clk_parent_data baud_div_parent = { }; + struct clk *clk_baud, *clk_xtal; + int ret; - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); - if (IS_ERR(clk_pclk)) - return PTR_ERR(clk_pclk); + 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"); + + clk_baud = devm_clk_get(port->dev, "baud"); + if (IS_ERR(clk_baud)) + return dev_err_probe(port->dev, PTR_ERR(clk_baud), + "Failed to get the 'baud' clock\n"); - 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); + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), + "Failed to get the 'xtal' clock\n"); + + private_data->xtal_div3.mult = 1; + private_data->xtal_div3.div = 3; + ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent, + 1, &clk_fixed_factor_ops, + &private_data->xtal_div3.hw); + if (ret) + return ret; - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); - if (IS_ERR(clk_baud)) - return PTR_ERR(clk_baud); + if (register_clk81_div4) { + private_data->clk81_div4.mult = 1; + private_data->clk81_div4.div = 4; + ret = meson_uart_register_clk(port, "clk81_div4", + &clk81_div_parent, 1, + &clk_fixed_factor_ops, + &private_data->clk81_div4.hw); + if (ret) + return ret; + + use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw; + } - port->uartclk = clk_get_rate(clk_baud); + if (private_data->has_xtal_clk_sel) { + private_data->xtal_div2.mult = 1; + private_data->xtal_div2.div = 2; + ret = meson_uart_register_clk(port, "xtal_div2", + &xtal_div_parent, 1, + &clk_fixed_factor_ops, + &private_data->xtal_div2.hw); + if (ret) + return ret; + + xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw; + xtal_clk_sel_mux_parents[1].fw_name = "xtal"; + + private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5; + private_data->xtal_clk_sel_mux.mask = 0x1; + private_data->xtal_clk_sel_mux.shift = 26; + private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; + ret = meson_uart_register_clk(port, "xtal_clk_sel", + xtal_clk_sel_mux_parents, + ARRAY_SIZE(xtal_clk_sel_mux_parents), + &clk_mux_ops, + &private_data->xtal_clk_sel_mux.hw); + if (ret) + return ret; + + xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw; + xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw; + + private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5; + private_data->xtal2_clk_sel_mux.mask = 0x1; + private_data->xtal2_clk_sel_mux.shift = 27; + private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; + ret = meson_uart_register_clk(port, "xtal2_clk_sel", + xtal2_clk_sel_mux_parents, + ARRAY_SIZE(xtal2_clk_sel_mux_parents), + &clk_mux_ops, + &private_data->xtal2_clk_sel_mux.hw); + if (ret) + return ret; + + use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw; + } else { + use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw; + } + + private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5; + private_data->use_xtal_mux.mask = 0x1; + private_data->use_xtal_mux.shift = 24; + private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST; + ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents, + ARRAY_SIZE(use_xtal_mux_parents), + &clk_mux_ops, + &private_data->use_xtal_mux.hw); + if (ret) + return ret; + + baud_div_parent.hw = &private_data->use_xtal_mux.hw; + + private_data->baud_div.reg = port->membase + AML_UART_REG5; + private_data->baud_div.shift = 0; + private_data->baud_div.width = 23; + private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST; + ret = meson_uart_register_clk(port, "baud_div", + &baud_div_parent, 1, + &clk_divider_ops, + &private_data->baud_div.hw); + if (ret) + return ret; + + private_data->baud_clk = devm_clk_hw_get_clk(port->dev, + &private_data->baud_div.hw, + "baud_rate"); + if (IS_ERR(private_data->baud_clk)) + return dev_err_probe(port->dev, + PTR_ERR(private_data->baud_clk), + "Failed to request the 'baud_rate' clock\n"); return 0; } static int meson_uart_probe(struct platform_device *pdev) { + struct meson_uart_data *private_data; struct resource *res_mem, *res_irq; + struct clk *clk_baud, *clk_xtal; + bool register_clk81_div4; struct uart_port *port; int ret = 0; int id = -1; @@ -711,18 +855,37 @@ 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->has_xtal_clk_sel = true; + + private_data->pclk = devm_clk_get(&pdev->dev, "pclk"); + if (IS_ERR(private_data->pclk)) + return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk), + "Failed to get the 'pclk' clock\n"); + + clk_baud = devm_clk_get(&pdev->dev, "baud"); + if (IS_ERR(clk_baud)) + return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud), + "Failed to get the 'baud' clock\n"); + + clk_xtal = devm_clk_get(&pdev->dev, "xtal"); + if (IS_ERR(clk_xtal)) + return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal), + "Failed to get the 'xtal' clock\n"); + + register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud); + + 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); @@ -735,6 +898,12 @@ static int meson_uart_probe(struct platform_device *pdev) port->x_char = 0; port->ops = &meson_uart_ops; port->fifosize = 64; + port->uartclk = clk_get_rate(clk_baud); + port->private_data = private_data; + + ret = meson_uart_probe_clocks(port, register_clk81_div4); + if (ret) + return ret; meson_ports[pdev->id] = port; platform_set_drvdata(pdev, port); @@ -761,10 +930,42 @@ 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, + }, + { + .compatible = "amlogic,meson-s4-uart", + .data = (void *)true, + }, + /* + * deprecated, don't use anymore because it doesn't differentiate + * between GXBB and GXL 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-30 10:21 ` [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible Yu Tu @ 2021-12-30 12:27 ` Greg Kroah-Hartman 2021-12-31 10:23 ` Yu Tu 2021-12-30 23:13 ` Martin Blumenstingl 2022-01-03 12:40 ` Jerome Brunet 2 siblings, 1 reply; 39+ messages in thread From: Greg Kroah-Hartman @ 2021-12-30 12:27 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl On Thu, Dec 30, 2021 at 06:21:08PM +0800, 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 > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- Your subject line is very odd, please fix up. thanks, greg k-h ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-30 12:27 ` Greg Kroah-Hartman @ 2021-12-31 10:23 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2021-12-31 10:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl On 2021/12/30 20:27, Greg Kroah-Hartman wrote: > [ EXTERNAL EMAIL ] > > On Thu, Dec 30, 2021 at 06:21:08PM +0800, 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 >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- > > Your subject line is very odd, please fix up. > I will correct. > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-30 10:21 ` [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible Yu Tu 2021-12-30 12:27 ` Greg Kroah-Hartman @ 2021-12-30 23:13 ` Martin Blumenstingl 2021-12-31 11:24 ` Yu Tu 2022-01-03 12:40 ` Jerome Brunet 2 siblings, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-30 23:13 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On Thu, Dec 30, 2021 at 11:21 AM Yu Tu <yu.tu@amlogic.com> wrote: [...] > @@ -270,14 +284,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); I suggest splitting this into a separate patch. In general I think it's a good idea to re-use meson_uart_reset here if possible. However, if during testing it turns out that this doesn't work then we can revert this single patch which updates meson_uart_startup() only - instead of reverting the whole transition to the common clock framework. [...] > 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; > + } This code is from my original suggestion - and I had a doubt there which I forgot to add as a comment originally: Can you confirm that accessing the UART controller registers works even when "pclk" is turned off? I am asking this because the common clock framework can access the clocks at any time. And I have seen SoCs which would hang when trying to access a module's registers while the module's pclk is turned off. [...] > port->fifosize = 64; commit 27d44e05d7b85d ("tty: serial: meson: retrieve port FIFO size from DT") [0] from May 2021 has changed this line to: port->fifosize = fifosize; So your patch currently does not apply to linux-next (or even Linus' mainline tree). [...] > 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, > + }, > + { > + .compatible = "amlogic,meson-s4-uart", > + .data = (void *)true, > + }, > + /* > + * deprecated, don't use anymore because it doesn't differentiate > + * between GXBB and GXL which have different revisions of the UART IP. > + */ > + { > + .compatible = "amlogic,meson-gx-uart", > + .data = (void *)false, > + }, For this change I think it's also best to split it into separate changes, similar to the dt-bindings: 1) deprecate and replace "amlogic,meson-gx-uart" 2) another one to add the S4 compatible string 3) and a third one with the big common clock framework change (adding the .data attributes) This is about the "Separate each logical change into a separate patch" rule from the Linux kernel patch submission guide [1] Also I hope that it will make it (at least a bit) easier for others to also review this patch. Best regards, Martin [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/tty/serial/meson_uart.c?id=27d44e05d7b85d9d4cfe0a3c0663ea49752ece93 [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#separate-your-changes ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-30 23:13 ` Martin Blumenstingl @ 2021-12-31 11:24 ` Yu Tu 2021-12-31 15:32 ` Martin Blumenstingl 0 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2021-12-31 11:24 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On 2021/12/31 7:13, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > On Thu, Dec 30, 2021 at 11:21 AM Yu Tu <yu.tu@amlogic.com> wrote: > [...] >> @@ -270,14 +284,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); > I suggest splitting this into a separate patch. In general I think > it's a good idea to re-use meson_uart_reset here if possible. > However, if during testing it turns out that this doesn't work then we > can revert this single patch which updates meson_uart_startup() only - > instead of reverting the whole transition to the common clock > framework. > I approve of your suggestion. I will make the next patch. > [...] >> 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; >> + } > This code is from my original suggestion - and I had a doubt there > which I forgot to add as a comment originally: > Can you confirm that accessing the UART controller registers works > even when "pclk" is turned off? > I am asking this because the common clock framework can access the > clocks at any time. > And I have seen SoCs which would hang when trying to access a module's > registers while the module's pclk is turned off. On all meson platforms, the default pclk for all UART is turned on during the u-boot phase. When registering uart pclk in the kernel phase, the CLK_IGNORE_UNUSED flag is added. So the real shutdown is when the standby goes down, the parent clk shuts down. . > > [...] >> port->fifosize = 64; > commit 27d44e05d7b85d ("tty: serial: meson: retrieve port FIFO size > from DT") [0] from May 2021 has changed this line to: > port->fifosize = fifosize; > So your patch currently does not apply to linux-next (or even Linus' > mainline tree). > So do I need to wait for [0] patch merged before I can continue to make changes ? What can I do before? > [...] >> 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, >> + }, >> + { >> + .compatible = "amlogic,meson-s4-uart", >> + .data = (void *)true, >> + }, >> + /* >> + * deprecated, don't use anymore because it doesn't differentiate >> + * between GXBB and GXL which have different revisions of the UART IP. >> + */ >> + { >> + .compatible = "amlogic,meson-gx-uart", >> + .data = (void *)false, >> + }, > For this change I think it's also best to split it into separate > changes, similar to the dt-bindings: > 1) deprecate and replace "amlogic,meson-gx-uart" > 2) another one to add the S4 compatible string > 3) and a third one with the big common clock framework change (adding > the .data attributes) > > This is about the "Separate each logical change into a separate patch" > rule from the Linux kernel patch submission guide [1] > Also I hope that it will make it (at least a bit) easier for others to > also review this patch. > I quite agree with your suggestion. I'll do as you suggest. > > Best regards, > Martin > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/tty/serial/meson_uart.c?id=27d44e05d7b85d9d4cfe0a3c0663ea49752ece93 > [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#separate-your-changes > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-31 11:24 ` Yu Tu @ 2021-12-31 15:32 ` Martin Blumenstingl 2022-01-01 13:30 ` Yu Tu 0 siblings, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-31 15:32 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On Fri, Dec 31, 2021 at 12:24 PM Yu Tu <yu.tu@amlogic.com> wrote: [...] > >> 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; > >> + } > > This code is from my original suggestion - and I had a doubt there > > which I forgot to add as a comment originally: > > Can you confirm that accessing the UART controller registers works > > even when "pclk" is turned off? > > I am asking this because the common clock framework can access the > > clocks at any time. > > And I have seen SoCs which would hang when trying to access a module's > > registers while the module's pclk is turned off. > On all meson platforms, the default pclk for all UART is turned on > during the u-boot phase. When registering uart pclk in the kernel phase, > the CLK_IGNORE_UNUSED flag is added. So the real shutdown is when the > standby goes down, the parent clk shuts down. Interesting, thanks for sharing that u-boot turns these clocks on. Let's say someone wanted to make u-boot save power and turn off all UART clocks except the one for uart_AO (where we typically connect the serial console). In that case the pclk of uart_C (just to choose an example here) is turned off. Would there be a problem then accessing the registers of uart_C before clk_prepare_enable is called? [...] > >> port->fifosize = 64; > > commit 27d44e05d7b85d ("tty: serial: meson: retrieve port FIFO size > > from DT") [0] from May 2021 has changed this line to: > > port->fifosize = fifosize; > > So your patch currently does not apply to linux-next (or even Linus' > > mainline tree). > > > So do I need to wait for [0] patch merged before I can continue to make > changes ? These changes are already merged. > What can I do before? You should base your changes on top of the tty.git/tty-next branch [1] where Greg (the maintainer of this tree) will pick up the patches once they are good (got enough Acked-by/Reviewed-by, etc.). I suspect that you based your changes on an older or stable kernel version (let's say 5.10). New functionality should always get into the -next tree where various auto-build robots will compile-test the changes and we even have Kernel CI where changes are tested on real hardware (BayLibre even maintains Amlogic boards in their Kernel CI labs). Let's say Amlogic updates to Linux 5.17 next year then the patches are already included in that kernel version - instead of being only available in Linux 5.10. Best regards, Martin [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/log/?h=tty-next ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-31 15:32 ` Martin Blumenstingl @ 2022-01-01 13:30 ` Yu Tu 2022-01-02 19:36 ` Martin Blumenstingl 2022-01-03 13:50 ` Jerome Brunet 0 siblings, 2 replies; 39+ messages in thread From: Yu Tu @ 2022-01-01 13:30 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet Hi Martin, Thank you very much for your reply. On 2021/12/31 23:32, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > On Fri, Dec 31, 2021 at 12:24 PM Yu Tu <yu.tu@amlogic.com> wrote: > [...] >>>> 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; >>>> + } >>> This code is from my original suggestion - and I had a doubt there >>> which I forgot to add as a comment originally: >>> Can you confirm that accessing the UART controller registers works >>> even when "pclk" is turned off? >>> I am asking this because the common clock framework can access the >>> clocks at any time. >>> And I have seen SoCs which would hang when trying to access a module's >>> registers while the module's pclk is turned off. >> On all meson platforms, the default pclk for all UART is turned on >> during the u-boot phase. When registering uart pclk in the kernel phase, >> the CLK_IGNORE_UNUSED flag is added. So the real shutdown is when the >> standby goes down, the parent clk shuts down. > Interesting, thanks for sharing that u-boot turns these clocks on. > Let's say someone wanted to make u-boot save power and turn off all > UART clocks except the one for uart_AO (where we typically connect the > serial console). > In that case the pclk of uart_C (just to choose an example here) is > turned off. Would there be a problem then accessing the registers of > uart_C before clk_prepare_enable is called? The way you describe it, it does hang. This would not be recommended on actual projects. At present, AmLogic chips are older than S4 Soc, and we have no way to deal with this problem. We have to tell customers not to use it in this way。Customers rarely use it in real projects.On the S4 SOC we will use a clock like the UART pclk to control the shutdown using two registers, one safe (need to operate in EL3) and one normal (EL1). It will only be closed if both registers are closed. This mainly prevents misoperation. With your experience, I'd like to know how you deal with this kind of problem. > > [...] >>>> port->fifosize = 64; >>> commit 27d44e05d7b85d ("tty: serial: meson: retrieve port FIFO size >>> from DT") [0] from May 2021 has changed this line to: >>> port->fifosize = fifosize; >>> So your patch currently does not apply to linux-next (or even Linus' >>> mainline tree). >>> >> So do I need to wait for [0] patch merged before I can continue to make >> changes ? > These changes are already merged. > >> What can I do before? > You should base your changes on top of the tty.git/tty-next branch [1] > where Greg (the maintainer of this tree) will pick up the patches once > they are good (got enough Acked-by/Reviewed-by, etc.). > I suspect that you based your changes on an older or stable kernel > version (let's say 5.10). New functionality should always get into the > -next tree where various auto-build robots will compile-test the > changes and we even have Kernel CI where changes are tested on real > hardware (BayLibre even maintains Amlogic boards in their Kernel CI > labs). Let's say Amlogic updates to Linux 5.17 next year then the > patches are already included in that kernel version - instead of being > only available in Linux 5.10. > I'm sorry, I did branch confirm there was a mistake, I have corrected. > > Best regards, > Martin > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/log/?h=tty-next > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-01 13:30 ` Yu Tu @ 2022-01-02 19:36 ` Martin Blumenstingl 2022-01-04 8:20 ` Yu Tu 2022-01-03 13:50 ` Jerome Brunet 1 sibling, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2022-01-02 19:36 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet Hi, On Sat, Jan 1, 2022 at 2:30 PM Yu Tu <yu.tu@amlogic.com> wrote: [...] > > Interesting, thanks for sharing that u-boot turns these clocks on. > > Let's say someone wanted to make u-boot save power and turn off all > > UART clocks except the one for uart_AO (where we typically connect the > > serial console). > > In that case the pclk of uart_C (just to choose an example here) is > > turned off. Would there be a problem then accessing the registers of > > uart_C before clk_prepare_enable is called? > The way you describe it, it does hang. This would not be recommended on > actual projects. > > At present, AmLogic chips are older than S4 Soc, and we have no way to > deal with this problem. We have to tell customers not to use it in this > way。Customers rarely use it in real projects.On the S4 SOC we will use > a clock like the UART pclk to control the shutdown using two registers, > one safe (need to operate in EL3) and one normal (EL1). It will only be > closed if both registers are closed. This mainly prevents misoperation. oh, interesting that there's some updates specifically with the S4 SoCs :-) > With your experience, I'd like to know how you deal with this kind of > problem. Before this patch the driver simply turns on the clock from within meson_uart_probe() (specifically it does so in meson_uart_probe_clock()). I think there's advanced power-saving techniques. Maybe for now we keep it simple and just enable the clock(s) at probe time and disable them at driver remove time. What do you think? Best regards, Martin ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-02 19:36 ` Martin Blumenstingl @ 2022-01-04 8:20 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2022-01-04 8:20 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet Hi Martin, Thank you very much for your reply. On 2022/1/3 3:36, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > Hi, > > On Sat, Jan 1, 2022 at 2:30 PM Yu Tu <yu.tu@amlogic.com> wrote: > [...] >>> Interesting, thanks for sharing that u-boot turns these clocks on. >>> Let's say someone wanted to make u-boot save power and turn off all >>> UART clocks except the one for uart_AO (where we typically connect the >>> serial console). >>> In that case the pclk of uart_C (just to choose an example here) is >>> turned off. Would there be a problem then accessing the registers of >>> uart_C before clk_prepare_enable is called? >> The way you describe it, it does hang. This would not be recommended on >> actual projects. >> >> At present, AmLogic chips are older than S4 Soc, and we have no way to >> deal with this problem. We have to tell customers not to use it in this >> way。Customers rarely use it in real projects.On the S4 SOC we will use >> a clock like the UART pclk to control the shutdown using two registers, >> one safe (need to operate in EL3) and one normal (EL1). It will only be >> closed if both registers are closed. This mainly prevents misoperation. > oh, interesting that there's some updates specifically with the S4 SoCs :-) > >> With your experience, I'd like to know how you deal with this kind of >> problem. > Before this patch the driver simply turns on the clock from within > meson_uart_probe() (specifically it does so in > meson_uart_probe_clock()). > I think there's advanced power-saving techniques. Maybe for now we > keep it simple and just enable the clock(s) at probe time and disable > them at driver remove time. What do you think? > I agree with you. > > Best regards, > Martin > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-01 13:30 ` Yu Tu 2022-01-02 19:36 ` Martin Blumenstingl @ 2022-01-03 13:50 ` Jerome Brunet 1 sibling, 0 replies; 39+ messages in thread From: Jerome Brunet @ 2022-01-03 13:50 UTC (permalink / raw) To: Yu Tu, Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman On Sat 01 Jan 2022 at 21:30, Yu Tu <yu.tu@amlogic.com> wrote: > Hi Martin, > Thank you very much for your reply. > > On 2021/12/31 23:32, Martin Blumenstingl wrote: >> [ EXTERNAL EMAIL ] >> On Fri, Dec 31, 2021 at 12:24 PM Yu Tu <yu.tu@amlogic.com> wrote: >> [...] >>>>> 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; >>>>> + } >>>> This code is from my original suggestion - and I had a doubt there >>>> which I forgot to add as a comment originally: >>>> Can you confirm that accessing the UART controller registers works >>>> even when "pclk" is turned off? >>>> I am asking this because the common clock framework can access the >>>> clocks at any time. >>>> And I have seen SoCs which would hang when trying to access a module's >>>> registers while the module's pclk is turned off. >>> On all meson platforms, the default pclk for all UART is turned on >>> during the u-boot phase. When registering uart pclk in the kernel phase, >>> the CLK_IGNORE_UNUSED flag is added. So the real shutdown is when the >>> standby goes down, the parent clk shuts down. >> Interesting, thanks for sharing that u-boot turns these clocks on. >> Let's say someone wanted to make u-boot save power and turn off all >> UART clocks except the one for uart_AO (where we typically connect the >> serial console). >> In that case the pclk of uart_C (just to choose an example here) is >> turned off. Would there be a problem then accessing the registers of >> uart_C before clk_prepare_enable is called? > The way you describe it, it does hang. This would not be recommended on > actual projects. > > At present, AmLogic chips are older than S4 Soc, and we have no way to deal > with this problem. We have to tell customers not to use it in this way。 > Customers rarely use it in real projects.On the S4 SOC we will use a clock > like the UART pclk to control the shutdown using two registers, one safe > (need to operate in EL3) and one normal (EL1). It will only be closed if > both registers are closed. This mainly prevents misoperation. > > With your experience, I'd like to know how you deal with this kind of > problem. Relying on the CLK_IGNORE_UNUSED and the boot loader is unsafe. As Martin is suggesting, the driver must ensure pclk is on before touching any register. This was done in the probe before in probe before your change which seems safe but maybe not optimal. Again, If you wish to optimize that, please do so in another dedicated change so we can discuss it and make sure it is still safe >> [...] >>>>> port->fifosize = 64; >>>> commit 27d44e05d7b85d ("tty: serial: meson: retrieve port FIFO size >>>> from DT") [0] from May 2021 has changed this line to: >>>> port->fifosize = fifosize; >>>> So your patch currently does not apply to linux-next (or even Linus' >>>> mainline tree). >>>> >>> So do I need to wait for [0] patch merged before I can continue to make >>> changes ? >> These changes are already merged. >> >>> What can I do before? >> You should base your changes on top of the tty.git/tty-next branch [1] >> where Greg (the maintainer of this tree) will pick up the patches once >> they are good (got enough Acked-by/Reviewed-by, etc.). >> I suspect that you based your changes on an older or stable kernel >> version (let's say 5.10). New functionality should always get into the >> -next tree where various auto-build robots will compile-test the >> changes and we even have Kernel CI where changes are tested on real >> hardware (BayLibre even maintains Amlogic boards in their Kernel CI >> labs). Let's say Amlogic updates to Linux 5.17 next year then the >> patches are already included in that kernel version - instead of being >> only available in Linux 5.10. >> > I'm sorry, I did branch confirm there was a mistake, I have corrected. >> Best regards, >> Martin >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/log/?h=tty-next >> ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2021-12-30 10:21 ` [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible Yu Tu 2021-12-30 12:27 ` Greg Kroah-Hartman 2021-12-30 23:13 ` Martin Blumenstingl @ 2022-01-03 12:40 ` Jerome Brunet 2022-01-04 9:57 ` Yu Tu 2 siblings, 1 reply; 39+ messages in thread From: Jerome Brunet @ 2022-01-03 12:40 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Martin Blumenstingl On Thu 30 Dec 2021 at 18:21, Yu Tu <yu.tu@amlogic.com> wrote: > Using the common Clock code to describe the UART baud rate clock makes > it easier for the UART driver to be compatible with the baud rate > requirements of the UART IP on different meson chips > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > drivers/tty/serial/Kconfig | 1 + > drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------ > 2 files changed, 257 insertions(+), 55 deletions(-) > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 780908d43557..32e238173036 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI > config SERIAL_MESON > tristate "Meson serial port support" > depends on ARCH_MESON > + depends on COMMON_CLK > select SERIAL_CORE > help > This enables the driver for the on-chip UARTs of the Amlogic > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > index 99efe62a1507..07eb1f40aaaa 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,21 @@ > #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; > + struct clk_divider baud_div; > + struct clk_mux use_xtal_mux; > + struct clk_mux xtal_clk_sel_mux; > + struct clk_mux xtal2_clk_sel_mux; > + struct clk_fixed_factor xtal_div2; > + struct clk_fixed_factor xtal_div3; > + struct clk_fixed_factor clk81_div4; Keeping all these internal elements around is not useful since they are registered using devm_ > + bool no_clk81_input; What is this ? > + bool has_xtal_clk_sel; > +}; > + > static struct uart_driver meson_uart_driver; > > static struct uart_port *meson_ports[AML_UART_PORT_NUM]; > @@ -270,14 +284,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); > > @@ -295,19 +306,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) { This check shows that previous code assumed bit 24 was left untouched Below you can see that GXBB and newer used the XTAL path while older used the other Your change makes this dynamic which is another "unexpected" change. Please make the bit 24 mux RO to start with so the behavior remains unchanged for older SoCs. > - 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, > @@ -397,11 +406,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,56 +654,175 @@ 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) > +static int meson_uart_register_clk(struct uart_port *port, > + const char *name_suffix, > + const struct clk_parent_data *parent_data, > + unsigned int num_parents, > + const struct clk_ops *ops, > + struct clk_hw *hw) > { > - struct clk *clk = NULL; > + struct clk_init_data init = { }; > + char clk_name[32]; > int ret; > > - clk = devm_clk_get(dev, id); > - if (IS_ERR(clk)) > - return clk; > + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), > + name_suffix); > > - ret = clk_prepare_enable(clk); > - if (ret) { > - dev_err(dev, "couldn't enable clk\n"); > - return ERR_PTR(ret); > - } > + init.name = clk_name; > + init.ops = ops; > + init.flags = CLK_SET_RATE_PARENT; > + init.parent_data = parent_data; > + init.num_parents = num_parents; > + > + hw->init = &init; > > - devm_add_action_or_reset(dev, > - (void(*)(void *))clk_disable_unprepare, > - clk); > + ret = devm_clk_hw_register(port->dev, hw); > + if (ret) > + return dev_err_probe(port->dev, ret, > + "Failed to register the '%s' clock\n", > + clk_name); > > - return clk; > + return ret; > } > > -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; > +static int meson_uart_probe_clocks(struct uart_port *port, > + bool register_clk81_div4) > +{ > + struct meson_uart_data *private_data = port->private_data; > + struct clk_parent_data use_xtal_mux_parents[2] = { > + { .index = -1, }, > + { .index = -1, }, > + }; > + struct clk_parent_data xtal_clk_sel_mux_parents[2] = { }; > + struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { }; > + struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", }; > + struct clk_parent_data clk81_div_parent = { .fw_name = "baud", }; > + struct clk_parent_data baud_div_parent = { }; > + struct clk *clk_baud, *clk_xtal; > + int ret; > > - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); > - if (IS_ERR(clk_pclk)) > - return PTR_ERR(clk_pclk); > + 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"); > + > + clk_baud = devm_clk_get(port->dev, "baud"); > + if (IS_ERR(clk_baud)) > + return dev_err_probe(port->dev, PTR_ERR(clk_baud), > + "Failed to get the 'baud' clock\n"); > > - 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); > + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), > + "Failed to get the 'xtal' clock\n"); > + > + private_data->xtal_div3.mult = 1; > + private_data->xtal_div3.div = 3; > + ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent, > + 1, &clk_fixed_factor_ops, > + &private_data->xtal_div3.hw); > + if (ret) > + return ret; > > - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); > - if (IS_ERR(clk_baud)) > - return PTR_ERR(clk_baud); > + if (register_clk81_div4) { Clock dividers represent HW elements. The presence of an HW element cannot dependent on the current of the input clocks. There is no way this is right > + private_data->clk81_div4.mult = 1; > + private_data->clk81_div4.div = 4; > + ret = meson_uart_register_clk(port, "clk81_div4", > + &clk81_div_parent, 1, > + &clk_fixed_factor_ops, > + &private_data->clk81_div4.hw); > + if (ret) > + return ret; > + > + use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw; This is only going to be used on meson8 and older. Worst case it should depend on this compatible but I don't think this would be right here. IMO, this shows that the UART "baud" input was actually fed by a derivation of "fclk_div4" instead of "clk_81" on these older SoCs. So instead of registering what I suspect to be a fake element, the meson8 clock controller driver and DT should be fixed a this should go away. > + } > > - port->uartclk = clk_get_rate(clk_baud); > + if (private_data->has_xtal_clk_sel) { > + private_data->xtal_div2.mult = 1; > + private_data->xtal_div2.div = 2; > + ret = meson_uart_register_clk(port, "xtal_div2", > + &xtal_div_parent, 1, > + &clk_fixed_factor_ops, > + &private_data->xtal_div2.hw); > + if (ret) > + return ret; > + > + xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw; > + xtal_clk_sel_mux_parents[1].fw_name = "xtal"; > + > + private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5; > + private_data->xtal_clk_sel_mux.mask = 0x1; > + private_data->xtal_clk_sel_mux.shift = 26; > + private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; > + ret = meson_uart_register_clk(port, "xtal_clk_sel", > + xtal_clk_sel_mux_parents, > + ARRAY_SIZE(xtal_clk_sel_mux_parents), > + &clk_mux_ops, > + &private_data->xtal_clk_sel_mux.hw); > + if (ret) > + return ret; > + > + xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw; > + xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw; > + > + private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5; > + private_data->xtal2_clk_sel_mux.mask = 0x1; > + private_data->xtal2_clk_sel_mux.shift = 27; > + private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; > + ret = meson_uart_register_clk(port, "xtal2_clk_sel", > + xtal2_clk_sel_mux_parents, > + ARRAY_SIZE(xtal2_clk_sel_mux_parents), > + &clk_mux_ops, > + &private_data->xtal2_clk_sel_mux.hw); > + if (ret) > + return ret; > + > + use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw; > + } else { > + use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw; Well the above is a bit over-complicated. If I summarize: GXBB and older used a fixed divider of 3. Bits 26 and 27 read 0 according to the documentation. Chips after GXBB have 2 configurable divider of 2 and 3. bits 26 and 27 selects which of these dividers is used. So the above could be replaced with a single divider covering bits 26 and 27 with the following divider table { 2, 2, 1, 3 }. The divider should use RO ops and not have CLK_SET_RATE_PARENT. It can be used for all chips variant like this, including the older ones. The only information you need to carry if whether or not you want to make this divider modifiable. This means using the dt data to store clk_div_ops or clk_div_ro_ops pointer. To avoid changing the behavior of the older platforms in this patch, I would suggest to make everything use clk_div_ro_ops first, and make another patch to use clk_div_ops if necessary. > + } > + > + private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5; > + private_data->use_xtal_mux.mask = 0x1; > + private_data->use_xtal_mux.shift = 24; > + private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST; > + ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents, > + ARRAY_SIZE(use_xtal_mux_parents), > + &clk_mux_ops, Use RO ops here to start with. You can make this writable with another patch explicitly describing the change. > + &private_data->use_xtal_mux.hw); > + if (ret) > + return ret; > + > + baud_div_parent.hw = &private_data->use_xtal_mux.hw; > + > + private_data->baud_div.reg = port->membase + AML_UART_REG5; > + private_data->baud_div.shift = 0; > + private_data->baud_div.width = 23; > + private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST; > + ret = meson_uart_register_clk(port, "baud_div", > + &baud_div_parent, 1, > + &clk_divider_ops, > + &private_data->baud_div.hw); > + if (ret) > + return ret; > + > + private_data->baud_clk = devm_clk_hw_get_clk(port->dev, > + &private_data->baud_div.hw, > + "baud_rate"); There is a problem with this function in CCF. It will pin the driver to itself, making it unremovable. It is an ongoing topic. For now, just use "hw->clk" to get the struct *clk. > + if (IS_ERR(private_data->baud_clk)) > + return dev_err_probe(port->dev, I don't think anything can defer here, so dev_err_probe() is not necessary I think > + PTR_ERR(private_data->baud_clk), > + "Failed to request the 'baud_rate' clock\n"); > > return 0; > } > > static int meson_uart_probe(struct platform_device *pdev) > { > + struct meson_uart_data *private_data; > struct resource *res_mem, *res_irq; > + struct clk *clk_baud, *clk_xtal; > + bool register_clk81_div4; > struct uart_port *port; > int ret = 0; > int id = -1; > @@ -711,18 +855,37 @@ 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->has_xtal_clk_sel = true; > + > + private_data->pclk = devm_clk_get(&pdev->dev, "pclk"); > + if (IS_ERR(private_data->pclk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk), > + "Failed to get the 'pclk' clock\n"); > + > + clk_baud = devm_clk_get(&pdev->dev, "baud"); > + if (IS_ERR(clk_baud)) > + return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud), > + "Failed to get the 'baud' clock\n"); > + > + clk_xtal = devm_clk_get(&pdev->dev, "xtal"); > + if (IS_ERR(clk_xtal)) > + return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal), > + "Failed to get the 'xtal' clock\n"); > + This is second time you call devm_clk_get() on these clocks. One instance has to go away > + register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud); > + The above is a ugly way to distinguish the meson8 (32bit) SoC family from the rest. This definitely not the way to achieve it. The right is the compatible data but here I think it is not necessary. The clock input should be fixed. > + 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); > @@ -735,6 +898,12 @@ static int meson_uart_probe(struct platform_device *pdev) > port->x_char = 0; > port->ops = &meson_uart_ops; > port->fifosize = 64; > + port->uartclk = clk_get_rate(clk_baud); > + port->private_data = private_data; > + > + ret = meson_uart_probe_clocks(port, register_clk81_div4); > + if (ret) > + return ret; > > meson_ports[pdev->id] = port; > platform_set_drvdata(pdev, port); > @@ -761,10 +930,42 @@ 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, > + }, > + { > + .compatible = "amlogic,meson-s4-uart", > + .data = (void *)true, > + }, > + /* > + * deprecated, don't use anymore because it doesn't differentiate > + * between GXBB and GXL 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); ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-03 12:40 ` Jerome Brunet @ 2022-01-04 9:57 ` Yu Tu 2022-01-04 10:36 ` Jerome Brunet 0 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2022-01-04 9:57 UTC (permalink / raw) To: Jerome Brunet, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Martin Blumenstingl Hi Jerome, Thank you very much for your reply. On 2022/1/3 20:40, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Thu 30 Dec 2021 at 18:21, Yu Tu <yu.tu@amlogic.com> wrote: > >> Using the common Clock code to describe the UART baud rate clock makes >> it easier for the UART driver to be compatible with the baud rate >> requirements of the UART IP on different meson chips >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> drivers/tty/serial/Kconfig | 1 + >> drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------ >> 2 files changed, 257 insertions(+), 55 deletions(-) >> >> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >> index 780908d43557..32e238173036 100644 >> --- a/drivers/tty/serial/Kconfig >> +++ b/drivers/tty/serial/Kconfig >> @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI >> config SERIAL_MESON >> tristate "Meson serial port support" >> depends on ARCH_MESON >> + depends on COMMON_CLK >> select SERIAL_CORE >> help >> This enables the driver for the on-chip UARTs of the Amlogic >> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >> index 99efe62a1507..07eb1f40aaaa 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,21 @@ >> #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; >> + struct clk_divider baud_div; >> + struct clk_mux use_xtal_mux; >> + struct clk_mux xtal_clk_sel_mux; >> + struct clk_mux xtal2_clk_sel_mux; >> + struct clk_fixed_factor xtal_div2; >> + struct clk_fixed_factor xtal_div3; >> + struct clk_fixed_factor clk81_div4; > > Keeping all these internal elements around is not useful since they are > registered using devm_ > I'm sorry. I don't know what you mean. That's exactly what you said, but what's wrong, I don't understand. Do you have any better suggestions, please specify specific points, preferably give examples. >> + bool no_clk81_input; > > What is this ? > To distinguish between clK81 and XTAL. >> + bool has_xtal_clk_sel; >> +}; >> + >> static struct uart_driver meson_uart_driver; >> >> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >> @@ -270,14 +284,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); >> >> @@ -295,19 +306,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) { > > This check shows that previous code assumed bit 24 was left untouched > Below you can see that GXBB and newer used the XTAL path while older > used the other > > Your change makes this dynamic which is another "unexpected" change. > Please make the bit 24 mux RO to start with so the behavior remains unchanged > for older SoCs. I agree with what you say, and I will correct it. > >> - 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, >> @@ -397,11 +406,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,56 +654,175 @@ 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) >> +static int meson_uart_register_clk(struct uart_port *port, >> + const char *name_suffix, >> + const struct clk_parent_data *parent_data, >> + unsigned int num_parents, >> + const struct clk_ops *ops, >> + struct clk_hw *hw) >> { >> - struct clk *clk = NULL; >> + struct clk_init_data init = { }; >> + char clk_name[32]; >> int ret; >> >> - clk = devm_clk_get(dev, id); >> - if (IS_ERR(clk)) >> - return clk; >> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >> + name_suffix); >> >> - ret = clk_prepare_enable(clk); >> - if (ret) { >> - dev_err(dev, "couldn't enable clk\n"); >> - return ERR_PTR(ret); >> - } >> + init.name = clk_name; >> + init.ops = ops; >> + init.flags = CLK_SET_RATE_PARENT; >> + init.parent_data = parent_data; >> + init.num_parents = num_parents; >> + >> + hw->init = &init; >> >> - devm_add_action_or_reset(dev, >> - (void(*)(void *))clk_disable_unprepare, >> - clk); >> + ret = devm_clk_hw_register(port->dev, hw); >> + if (ret) >> + return dev_err_probe(port->dev, ret, >> + "Failed to register the '%s' clock\n", >> + clk_name); >> >> - return clk; >> + return ret; >> } >> >> -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; >> +static int meson_uart_probe_clocks(struct uart_port *port, >> + bool register_clk81_div4) >> +{ >> + struct meson_uart_data *private_data = port->private_data; >> + struct clk_parent_data use_xtal_mux_parents[2] = { >> + { .index = -1, }, >> + { .index = -1, }, >> + }; >> + struct clk_parent_data xtal_clk_sel_mux_parents[2] = { }; >> + struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { }; >> + struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", }; >> + struct clk_parent_data clk81_div_parent = { .fw_name = "baud", }; >> + struct clk_parent_data baud_div_parent = { }; >> + struct clk *clk_baud, *clk_xtal; >> + int ret; >> >> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >> - if (IS_ERR(clk_pclk)) >> - return PTR_ERR(clk_pclk); >> + 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"); >> + >> + clk_baud = devm_clk_get(port->dev, "baud"); >> + if (IS_ERR(clk_baud)) >> + return dev_err_probe(port->dev, PTR_ERR(clk_baud), >> + "Failed to get the 'baud' clock\n"); >> >> - 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); >> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >> + "Failed to get the 'xtal' clock\n"); >> + >> + private_data->xtal_div3.mult = 1; >> + private_data->xtal_div3.div = 3; >> + ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent, >> + 1, &clk_fixed_factor_ops, >> + &private_data->xtal_div3.hw); >> + if (ret) >> + return ret; >> >> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >> - if (IS_ERR(clk_baud)) >> - return PTR_ERR(clk_baud); >> + if (register_clk81_div4) { > > Clock dividers represent HW elements. The presence of an HW element cannot > dependent on the current of the input clocks. There is no way this is right > >> + private_data->clk81_div4.mult = 1; >> + private_data->clk81_div4.div = 4; >> + ret = meson_uart_register_clk(port, "clk81_div4", >> + &clk81_div_parent, 1, >> + &clk_fixed_factor_ops, >> + &private_data->clk81_div4.hw); >> + if (ret) >> + return ret; >> + >> + use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw; > > This is only going to be used on meson8 and older. Worst case it should depend > on this compatible but I don't think this would be right here. > > IMO, this shows that the UART "baud" input was actually fed by a > derivation of "fclk_div4" instead of "clk_81" on these older SoCs. > > So instead of registering what I suspect to be a fake element, the > meson8 clock controller driver and DT should be fixed a this should go away. > Virtually all UART controllers are supported, but they are not used after Meson8. So the description is more reasonable in UART driver. >> + } >> >> - port->uartclk = clk_get_rate(clk_baud); >> + if (private_data->has_xtal_clk_sel) { >> + private_data->xtal_div2.mult = 1; >> + private_data->xtal_div2.div = 2; >> + ret = meson_uart_register_clk(port, "xtal_div2", >> + &xtal_div_parent, 1, >> + &clk_fixed_factor_ops, >> + &private_data->xtal_div2.hw); >> + if (ret) >> + return ret; >> + >> + xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw; >> + xtal_clk_sel_mux_parents[1].fw_name = "xtal"; >> + >> + private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5; >> + private_data->xtal_clk_sel_mux.mask = 0x1; >> + private_data->xtal_clk_sel_mux.shift = 26; >> + private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >> + ret = meson_uart_register_clk(port, "xtal_clk_sel", >> + xtal_clk_sel_mux_parents, >> + ARRAY_SIZE(xtal_clk_sel_mux_parents), >> + &clk_mux_ops, >> + &private_data->xtal_clk_sel_mux.hw); >> + if (ret) >> + return ret; >> + >> + xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw; >> + xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw; >> + >> + private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5; >> + private_data->xtal2_clk_sel_mux.mask = 0x1; >> + private_data->xtal2_clk_sel_mux.shift = 27; >> + private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >> + ret = meson_uart_register_clk(port, "xtal2_clk_sel", >> + xtal2_clk_sel_mux_parents, >> + ARRAY_SIZE(xtal2_clk_sel_mux_parents), >> + &clk_mux_ops, >> + &private_data->xtal2_clk_sel_mux.hw); >> + if (ret) >> + return ret; >> + >> + use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw; >> + } else { >> + use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw; > > Well the above is a bit over-complicated. If I summarize: > > GXBB and older used a fixed divider of 3. Bits 26 and 27 read > 0 according to the documentation. > > Chips after GXBB have 2 configurable divider of 2 and 3. bits 26 and 27 > selects which of these dividers is used. > > So the above could be replaced with a single divider covering bits 26 and 27 > with the following divider table { 2, 2, 1, 3 }. The divider should use > RO ops and not have CLK_SET_RATE_PARENT. It can be used for all chips > variant like this, including the older ones. > As you say, the way you say it is complicated. I think you're complicating things.I don't understand what you're trying to achieve. Can you be more specific? > The only information you need to carry if whether or not you want to > make this divider modifiable. This means using the dt data to store > clk_div_ops or clk_div_ro_ops pointer. > > To avoid changing the behavior of the older platforms in this patch, I would > suggest to make everything use clk_div_ro_ops first, and make another > patch to use clk_div_ops if necessary. > > >> + } >> + >> + private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5; >> + private_data->use_xtal_mux.mask = 0x1; >> + private_data->use_xtal_mux.shift = 24; >> + private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST; >> + ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents, >> + ARRAY_SIZE(use_xtal_mux_parents), >> + &clk_mux_ops, > > Use RO ops here to start with. > You can make this writable with another patch explicitly describing the change. I agree with what you say, and I will correct it. > >> + &private_data->use_xtal_mux.hw); >> + if (ret) >> + return ret; >> + >> + baud_div_parent.hw = &private_data->use_xtal_mux.hw; >> + >> + private_data->baud_div.reg = port->membase + AML_UART_REG5; >> + private_data->baud_div.shift = 0; >> + private_data->baud_div.width = 23; >> + private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST; >> + ret = meson_uart_register_clk(port, "baud_div", >> + &baud_div_parent, 1, >> + &clk_divider_ops, >> + &private_data->baud_div.hw); >> + if (ret) >> + return ret; >> + >> + private_data->baud_clk = devm_clk_hw_get_clk(port->dev, >> + &private_data->baud_div.hw, >> + "baud_rate"); > > There is a problem with this function in CCF. It will pin the driver to > itself, making it unremovable. It is an ongoing topic. For now, just use > "hw->clk" to get the struct *clk. > >> + if (IS_ERR(private_data->baud_clk)) >> + return dev_err_probe(port->dev, > > I don't think anything can defer here, so dev_err_probe() is not > necessary I think > I agree with what you say, and I will correct it. >> + PTR_ERR(private_data->baud_clk), >> + "Failed to request the 'baud_rate' clock\n"); >> >> return 0; >> } >> >> static int meson_uart_probe(struct platform_device *pdev) >> { >> + struct meson_uart_data *private_data; >> struct resource *res_mem, *res_irq; >> + struct clk *clk_baud, *clk_xtal; >> + bool register_clk81_div4; >> struct uart_port *port; >> int ret = 0; >> int id = -1; >> @@ -711,18 +855,37 @@ 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->has_xtal_clk_sel = true; >> + >> + private_data->pclk = devm_clk_get(&pdev->dev, "pclk"); >> + if (IS_ERR(private_data->pclk)) >> + return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk), >> + "Failed to get the 'pclk' clock\n"); >> + >> + clk_baud = devm_clk_get(&pdev->dev, "baud"); >> + if (IS_ERR(clk_baud)) >> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud), >> + "Failed to get the 'baud' clock\n"); >> + >> + clk_xtal = devm_clk_get(&pdev->dev, "xtal"); >> + if (IS_ERR(clk_xtal)) >> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal), >> + "Failed to get the 'xtal' clock\n"); >> + > > This is second time you call devm_clk_get() on these clocks. One > instance has to go away > I agree with what you say, and I will correct it. >> + register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud); >> + > > The above is a ugly way to distinguish the meson8 (32bit) SoC family > from the rest. This definitely not the way to achieve it. > > The right is the compatible data but here I think it is not > necessary. The clock input should be fixed. > >> + 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); >> @@ -735,6 +898,12 @@ static int meson_uart_probe(struct platform_device *pdev) >> port->x_char = 0; >> port->ops = &meson_uart_ops; >> port->fifosize = 64; >> + port->uartclk = clk_get_rate(clk_baud); >> + port->private_data = private_data; >> + >> + ret = meson_uart_probe_clocks(port, register_clk81_div4); >> + if (ret) >> + return ret; >> >> meson_ports[pdev->id] = port; >> platform_set_drvdata(pdev, port); >> @@ -761,10 +930,42 @@ 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, >> + }, >> + { >> + .compatible = "amlogic,meson-s4-uart", >> + .data = (void *)true, >> + }, >> + /* >> + * deprecated, don't use anymore because it doesn't differentiate >> + * between GXBB and GXL 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); > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-04 9:57 ` Yu Tu @ 2022-01-04 10:36 ` Jerome Brunet 2022-01-04 14:35 ` Yu Tu 0 siblings, 1 reply; 39+ messages in thread From: Jerome Brunet @ 2022-01-04 10:36 UTC (permalink / raw) To: Yu Tu, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Martin Blumenstingl On Tue 04 Jan 2022 at 17:57, Yu Tu <yu.tu@amlogic.com> wrote: > Hi Jerome, > Thank you very much for your reply. > > On 2022/1/3 20:40, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Thu 30 Dec 2021 at 18:21, Yu Tu <yu.tu@amlogic.com> wrote: >> >>> Using the common Clock code to describe the UART baud rate clock makes >>> it easier for the UART driver to be compatible with the baud rate >>> requirements of the UART IP on different meson chips >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> drivers/tty/serial/Kconfig | 1 + >>> drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------ >>> 2 files changed, 257 insertions(+), 55 deletions(-) >>> >>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >>> index 780908d43557..32e238173036 100644 >>> --- a/drivers/tty/serial/Kconfig >>> +++ b/drivers/tty/serial/Kconfig >>> @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI >>> config SERIAL_MESON >>> tristate "Meson serial port support" >>> depends on ARCH_MESON >>> + depends on COMMON_CLK >>> select SERIAL_CORE >>> help >>> This enables the driver for the on-chip UARTs of the Amlogic >>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>> index 99efe62a1507..07eb1f40aaaa 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,21 @@ >>> #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; >>> + struct clk_divider baud_div; >>> + struct clk_mux use_xtal_mux; >>> + struct clk_mux xtal_clk_sel_mux; >>> + struct clk_mux xtal2_clk_sel_mux; >>> + struct clk_fixed_factor xtal_div2; >>> + struct clk_fixed_factor xtal_div3; >>> + struct clk_fixed_factor clk81_div4; >> Keeping all these internal elements around is not useful since they are >> registered using devm_ >> > I'm sorry. I don't know what you mean. That's exactly what you said, but > what's wrong, I don't understand. Do you have any better suggestions, > please specify specific points, preferably give examples. I'm saying that you don't need to keep reference to the internal elements of the clock tree you have registered since devm_ will take care of the removal later on. IOW, Once they are registered, the pointer is never used again so you don't need it in the private data. >>> + bool no_clk81_input; >> What is this ? >> > To distinguish between clK81 and XTAL. ... Yet, it is not used >>> + bool has_xtal_clk_sel; >>> +}; >>> + >>> static struct uart_driver meson_uart_driver; >>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>> @@ -270,14 +284,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); >>> @@ -295,19 +306,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) { >> This check shows that previous code assumed bit 24 was left untouched >> Below you can see that GXBB and newer used the XTAL path while older >> used the other >> Your change makes this dynamic which is another "unexpected" change. >> Please make the bit 24 mux RO to start with so the behavior remains unchanged >> for older SoCs. > I agree with what you say, and I will correct it. >> >>> - 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, >>> @@ -397,11 +406,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,56 +654,175 @@ 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) >>> +static int meson_uart_register_clk(struct uart_port *port, >>> + const char *name_suffix, >>> + const struct clk_parent_data *parent_data, >>> + unsigned int num_parents, >>> + const struct clk_ops *ops, >>> + struct clk_hw *hw) >>> { >>> - struct clk *clk = NULL; >>> + struct clk_init_data init = { }; >>> + char clk_name[32]; >>> int ret; >>> - clk = devm_clk_get(dev, id); >>> - if (IS_ERR(clk)) >>> - return clk; >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + name_suffix); >>> - ret = clk_prepare_enable(clk); >>> - if (ret) { >>> - dev_err(dev, "couldn't enable clk\n"); >>> - return ERR_PTR(ret); >>> - } >>> + init.name = clk_name; >>> + init.ops = ops; >>> + init.flags = CLK_SET_RATE_PARENT; >>> + init.parent_data = parent_data; >>> + init.num_parents = num_parents; >>> + >>> + hw->init = &init; >>> - devm_add_action_or_reset(dev, >>> - (void(*)(void *))clk_disable_unprepare, >>> - clk); >>> + ret = devm_clk_hw_register(port->dev, hw); >>> + if (ret) >>> + return dev_err_probe(port->dev, ret, >>> + "Failed to register the '%s' clock\n", >>> + clk_name); >>> - return clk; >>> + return ret; >>> } >>> -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; >>> +static int meson_uart_probe_clocks(struct uart_port *port, >>> + bool register_clk81_div4) >>> +{ >>> + struct meson_uart_data *private_data = port->private_data; >>> + struct clk_parent_data use_xtal_mux_parents[2] = { >>> + { .index = -1, }, >>> + { .index = -1, }, >>> + }; >>> + struct clk_parent_data xtal_clk_sel_mux_parents[2] = { }; >>> + struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { }; >>> + struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", }; >>> + struct clk_parent_data clk81_div_parent = { .fw_name = "baud", }; >>> + struct clk_parent_data baud_div_parent = { }; >>> + struct clk *clk_baud, *clk_xtal; >>> + int ret; >>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >>> - if (IS_ERR(clk_pclk)) >>> - return PTR_ERR(clk_pclk); >>> + 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"); >>> + >>> + clk_baud = devm_clk_get(port->dev, "baud"); >>> + if (IS_ERR(clk_baud)) >>> + return dev_err_probe(port->dev, PTR_ERR(clk_baud), >>> + "Failed to get the 'baud' clock\n"); >>> - 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); >>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >>> + "Failed to get the 'xtal' clock\n"); >>> + >>> + private_data->xtal_div3.mult = 1; >>> + private_data->xtal_div3.div = 3; >>> + ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent, >>> + 1, &clk_fixed_factor_ops, >>> + &private_data->xtal_div3.hw); >>> + if (ret) >>> + return ret; >>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >>> - if (IS_ERR(clk_baud)) >>> - return PTR_ERR(clk_baud); >>> + if (register_clk81_div4) { >> Clock dividers represent HW elements. The presence of an HW element >> cannot >> dependent on the current of the input clocks. There is no way this is right >> >>> + private_data->clk81_div4.mult = 1; >>> + private_data->clk81_div4.div = 4; >>> + ret = meson_uart_register_clk(port, "clk81_div4", >>> + &clk81_div_parent, 1, >>> + &clk_fixed_factor_ops, >>> + &private_data->clk81_div4.hw); >>> + if (ret) >>> + return ret; >>> + >>> + use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw; >> This is only going to be used on meson8 and older. Worst case it should >> depend >> on this compatible but I don't think this would be right here. >> IMO, this shows that the UART "baud" input was actually fed by a >> derivation of "fclk_div4" instead of "clk_81" on these older SoCs. >> So instead of registering what I suspect to be a fake element, the >> meson8 clock controller driver and DT should be fixed a this should go away. >> > Virtually all UART controllers are supported, but they are not used after > Meson8. So the description is more reasonable in UART driver. No it is not. It's not virtual, the support for meson8 is there in mainline. Unless you tell me there is an actual /4 divider in the UART block of the meson8 and older and that is was removed from the newer SoC, the bit above should be removed and the meson8 clock tree fixed. I think it is more probable that the /4 was never there to begin with, and that clk81/4 aka "fclk_div4", which is already available from the clock controller, was actually routed on these older SoCs. >>> + } >>> - port->uartclk = clk_get_rate(clk_baud); >>> + if (private_data->has_xtal_clk_sel) { >>> + private_data->xtal_div2.mult = 1; >>> + private_data->xtal_div2.div = 2; >>> + ret = meson_uart_register_clk(port, "xtal_div2", >>> + &xtal_div_parent, 1, >>> + &clk_fixed_factor_ops, >>> + &private_data->xtal_div2.hw); >>> + if (ret) >>> + return ret; >>> + >>> + xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw; >>> + xtal_clk_sel_mux_parents[1].fw_name = "xtal"; >>> + >>> + private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5; >>> + private_data->xtal_clk_sel_mux.mask = 0x1; >>> + private_data->xtal_clk_sel_mux.shift = 26; >>> + private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >>> + ret = meson_uart_register_clk(port, "xtal_clk_sel", >>> + xtal_clk_sel_mux_parents, >>> + ARRAY_SIZE(xtal_clk_sel_mux_parents), >>> + &clk_mux_ops, >>> + &private_data->xtal_clk_sel_mux.hw); >>> + if (ret) >>> + return ret; >>> + >>> + xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw; >>> + xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw; >>> + >>> + private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5; >>> + private_data->xtal2_clk_sel_mux.mask = 0x1; >>> + private_data->xtal2_clk_sel_mux.shift = 27; >>> + private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >>> + ret = meson_uart_register_clk(port, "xtal2_clk_sel", >>> + xtal2_clk_sel_mux_parents, >>> + ARRAY_SIZE(xtal2_clk_sel_mux_parents), >>> + &clk_mux_ops, >>> + &private_data->xtal2_clk_sel_mux.hw); >>> + if (ret) >>> + return ret; >>> + >>> + use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw; >>> + } else { >>> + use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw; >> Well the above is a bit over-complicated. If I summarize: >> GXBB and older used a fixed divider of 3. Bits 26 and 27 read >> 0 according to the documentation. >> Chips after GXBB have 2 configurable divider of 2 and 3. bits 26 and 27 >> selects which of these dividers is used. >> So the above could be replaced with a single divider covering bits 26 and >> 27 >> with the following divider table { 2, 2, 1, 3 }. The divider should use >> RO ops and not have CLK_SET_RATE_PARENT. It can be used for all chips >> variant like this, including the older ones. >> > As you say, the way you say it is complicated. I think you're complicating > things.I don't understand what you're trying to achieve. > Can you be more specific? I'm proposing to replace the 4 elements above, paths, parent_table and all with a single divider with a specific table. It accomplishes the same thing. |--------+--------+-----------| | Bit 27 | Bit 26 | Div Value | |--------+--------+-----------| | 0 | 0 | 3 | |--------+--------+-----------| | 0 | 1 | 1 | |--------+--------+-----------| | 1 | 0 | 2 | |--------+--------+-----------| | 1 | 1 | 2 | |--------+--------+-----------| https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk-provider.h?h=v5.16-rc8#n602 something like: static struct clk_div_table reg5_table[] = { { 0, 3 }, { 1, 1 }, { 2, 2 }, { 3, 2 }, }; I'm also pointing out that, since bit 26 and 27 reads 0 on the older SoCs, the divider would work for these too, as long as the Ops is read-only. So you don't even need to register different element depending on the SoC which simplify things a bit more. If you prefer to keep a fixed divider for the older ones, It's fine by me. It gives a bit more work but it is closer to reality. The newer SoC could still use the custom divider regardless. Last, and as explained on other bits, that part of the clock path should remain RO at first, to keep behavior stable with this change. You may change that later on, in a dedicated patch describing the change. > >> The only information you need to carry if whether or not you want to >> make this divider modifiable. This means using the dt data to store >> clk_div_ops or clk_div_ro_ops pointer. >> To avoid changing the behavior of the older platforms in this patch, I >> would >> suggest to make everything use clk_div_ro_ops first, and make another >> patch to use clk_div_ops if necessary. >> >>> + } >>> + >>> + private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5; >>> + private_data->use_xtal_mux.mask = 0x1; >>> + private_data->use_xtal_mux.shift = 24; >>> + private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST; >>> + ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents, >>> + ARRAY_SIZE(use_xtal_mux_parents), >>> + &clk_mux_ops, >> Use RO ops here to start with. >> You can make this writable with another patch explicitly describing the change. > I agree with what you say, and I will correct it. >> >>> + &private_data->use_xtal_mux.hw); >>> + if (ret) >>> + return ret; >>> + >>> + baud_div_parent.hw = &private_data->use_xtal_mux.hw; >>> + >>> + private_data->baud_div.reg = port->membase + AML_UART_REG5; >>> + private_data->baud_div.shift = 0; >>> + private_data->baud_div.width = 23; >>> + private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST; >>> + ret = meson_uart_register_clk(port, "baud_div", >>> + &baud_div_parent, 1, >>> + &clk_divider_ops, >>> + &private_data->baud_div.hw); >>> + if (ret) >>> + return ret; >>> + >>> + private_data->baud_clk = devm_clk_hw_get_clk(port->dev, >>> + &private_data->baud_div.hw, >>> + "baud_rate"); >> There is a problem with this function in CCF. It will pin the driver to >> itself, making it unremovable. It is an ongoing topic. For now, just use >> "hw->clk" to get the struct *clk. >> >>> + if (IS_ERR(private_data->baud_clk)) >>> + return dev_err_probe(port->dev, >> I don't think anything can defer here, so dev_err_probe() is not >> necessary I think >> > I agree with what you say, and I will correct it. >>> + PTR_ERR(private_data->baud_clk), >>> + "Failed to request the 'baud_rate' clock\n"); >>> return 0; >>> } >>> static int meson_uart_probe(struct platform_device *pdev) >>> { >>> + struct meson_uart_data *private_data; >>> struct resource *res_mem, *res_irq; >>> + struct clk *clk_baud, *clk_xtal; >>> + bool register_clk81_div4; >>> struct uart_port *port; >>> int ret = 0; >>> int id = -1; >>> @@ -711,18 +855,37 @@ 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->has_xtal_clk_sel = true; >>> + >>> + private_data->pclk = devm_clk_get(&pdev->dev, "pclk"); >>> + if (IS_ERR(private_data->pclk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk), >>> + "Failed to get the 'pclk' clock\n"); >>> + >>> + clk_baud = devm_clk_get(&pdev->dev, "baud"); >>> + if (IS_ERR(clk_baud)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud), >>> + "Failed to get the 'baud' clock\n"); >>> + >>> + clk_xtal = devm_clk_get(&pdev->dev, "xtal"); >>> + if (IS_ERR(clk_xtal)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal), >>> + "Failed to get the 'xtal' clock\n"); >>> + >> This is second time you call devm_clk_get() on these clocks. One >> instance has to go away >> > I agree with what you say, and I will correct it. >>> + register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud); >>> + >> The above is a ugly way to distinguish the meson8 (32bit) SoC family >> from the rest. This definitely not the way to achieve it. >> The right is the compatible data but here I think it is not >> necessary. The clock input should be fixed. >> >>> + 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); >>> @@ -735,6 +898,12 @@ static int meson_uart_probe(struct platform_device *pdev) >>> port->x_char = 0; >>> port->ops = &meson_uart_ops; >>> port->fifosize = 64; >>> + port->uartclk = clk_get_rate(clk_baud); >>> + port->private_data = private_data; >>> + >>> + ret = meson_uart_probe_clocks(port, register_clk81_div4); >>> + if (ret) >>> + return ret; >>> meson_ports[pdev->id] = port; >>> platform_set_drvdata(pdev, port); >>> @@ -761,10 +930,42 @@ 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, >>> + }, >>> + { >>> + .compatible = "amlogic,meson-s4-uart", >>> + .data = (void *)true, >>> + }, >>> + /* >>> + * deprecated, don't use anymore because it doesn't differentiate >>> + * between GXBB and GXL 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); >> ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-04 10:36 ` Jerome Brunet @ 2022-01-04 14:35 ` Yu Tu 2022-01-05 5:53 ` Yu Tu 0 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2022-01-04 14:35 UTC (permalink / raw) To: Jerome Brunet, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Martin Blumenstingl Hi Jerome, Thank you very much for your patient reply. I have learned a lot from it. On 2022/1/4 18:36, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > > On Tue 04 Jan 2022 at 17:57, Yu Tu <yu.tu@amlogic.com> wrote: > >> Hi Jerome, >> Thank you very much for your reply. >> >> On 2022/1/3 20:40, Jerome Brunet wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On Thu 30 Dec 2021 at 18:21, Yu Tu <yu.tu@amlogic.com> wrote: >>> >>>> Using the common Clock code to describe the UART baud rate clock makes >>>> it easier for the UART driver to be compatible with the baud rate >>>> requirements of the UART IP on different meson chips >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> drivers/tty/serial/Kconfig | 1 + >>>> drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------ >>>> 2 files changed, 257 insertions(+), 55 deletions(-) >>>> >>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >>>> index 780908d43557..32e238173036 100644 >>>> --- a/drivers/tty/serial/Kconfig >>>> +++ b/drivers/tty/serial/Kconfig >>>> @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI >>>> config SERIAL_MESON >>>> tristate "Meson serial port support" >>>> depends on ARCH_MESON >>>> + depends on COMMON_CLK >>>> select SERIAL_CORE >>>> help >>>> This enables the driver for the on-chip UARTs of the Amlogic >>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>>> index 99efe62a1507..07eb1f40aaaa 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,21 @@ >>>> #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; >>>> + struct clk_divider baud_div; >>>> + struct clk_mux use_xtal_mux; >>>> + struct clk_mux xtal_clk_sel_mux; >>>> + struct clk_mux xtal2_clk_sel_mux; >>>> + struct clk_fixed_factor xtal_div2; >>>> + struct clk_fixed_factor xtal_div3; >>>> + struct clk_fixed_factor clk81_div4; >>> Keeping all these internal elements around is not useful since they are >>> registered using devm_ >>> >> I'm sorry. I don't know what you mean. That's exactly what you said, but >> what's wrong, I don't understand. Do you have any better suggestions, >> please specify specific points, preferably give examples. > > I'm saying that you don't need to keep reference to the internal > elements of the clock tree you have registered since devm_ will take > care of the removal later on. IOW, Once they are registered, the pointer > is never used again so you don't need it in the private data. > I understand you now. What you say is right. I will correct it. > >>>> + bool no_clk81_input; >>> What is this ? >>> >> To distinguish between clK81 and XTAL. > > ... Yet, it is not used > >>>> + bool has_xtal_clk_sel; >>>> +}; >>>> + >>>> static struct uart_driver meson_uart_driver; >>>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>>> @@ -270,14 +284,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); >>>> @@ -295,19 +306,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) { >>> This check shows that previous code assumed bit 24 was left untouched >>> Below you can see that GXBB and newer used the XTAL path while older >>> used the other >>> Your change makes this dynamic which is another "unexpected" change. >>> Please make the bit 24 mux RO to start with so the behavior remains unchanged >>> for older SoCs. >> I agree with what you say, and I will correct it. >>> >>>> - 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, >>>> @@ -397,11 +406,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,56 +654,175 @@ 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) >>>> +static int meson_uart_register_clk(struct uart_port *port, >>>> + const char *name_suffix, >>>> + const struct clk_parent_data *parent_data, >>>> + unsigned int num_parents, >>>> + const struct clk_ops *ops, >>>> + struct clk_hw *hw) >>>> { >>>> - struct clk *clk = NULL; >>>> + struct clk_init_data init = { }; >>>> + char clk_name[32]; >>>> int ret; >>>> - clk = devm_clk_get(dev, id); >>>> - if (IS_ERR(clk)) >>>> - return clk; >>>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>>> + name_suffix); >>>> - ret = clk_prepare_enable(clk); >>>> - if (ret) { >>>> - dev_err(dev, "couldn't enable clk\n"); >>>> - return ERR_PTR(ret); >>>> - } >>>> + init.name = clk_name; >>>> + init.ops = ops; >>>> + init.flags = CLK_SET_RATE_PARENT; >>>> + init.parent_data = parent_data; >>>> + init.num_parents = num_parents; >>>> + >>>> + hw->init = &init; >>>> - devm_add_action_or_reset(dev, >>>> - (void(*)(void *))clk_disable_unprepare, >>>> - clk); >>>> + ret = devm_clk_hw_register(port->dev, hw); >>>> + if (ret) >>>> + return dev_err_probe(port->dev, ret, >>>> + "Failed to register the '%s' clock\n", >>>> + clk_name); >>>> - return clk; >>>> + return ret; >>>> } >>>> -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; >>>> +static int meson_uart_probe_clocks(struct uart_port *port, >>>> + bool register_clk81_div4) >>>> +{ >>>> + struct meson_uart_data *private_data = port->private_data; >>>> + struct clk_parent_data use_xtal_mux_parents[2] = { >>>> + { .index = -1, }, >>>> + { .index = -1, }, >>>> + }; >>>> + struct clk_parent_data xtal_clk_sel_mux_parents[2] = { }; >>>> + struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { }; >>>> + struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", }; >>>> + struct clk_parent_data clk81_div_parent = { .fw_name = "baud", }; >>>> + struct clk_parent_data baud_div_parent = { }; >>>> + struct clk *clk_baud, *clk_xtal; >>>> + int ret; >>>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >>>> - if (IS_ERR(clk_pclk)) >>>> - return PTR_ERR(clk_pclk); >>>> + 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"); >>>> + >>>> + clk_baud = devm_clk_get(port->dev, "baud"); >>>> + if (IS_ERR(clk_baud)) >>>> + return dev_err_probe(port->dev, PTR_ERR(clk_baud), >>>> + "Failed to get the 'baud' clock\n"); >>>> - 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); >>>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >>>> + "Failed to get the 'xtal' clock\n"); >>>> + >>>> + private_data->xtal_div3.mult = 1; >>>> + private_data->xtal_div3.div = 3; >>>> + ret = meson_uart_register_clk(port, "xtal_div3", &xtal_div_parent, >>>> + 1, &clk_fixed_factor_ops, >>>> + &private_data->xtal_div3.hw); >>>> + if (ret) >>>> + return ret; >>>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >>>> - if (IS_ERR(clk_baud)) >>>> - return PTR_ERR(clk_baud); >>>> + if (register_clk81_div4) { >>> Clock dividers represent HW elements. The presence of an HW element >>> cannot >>> dependent on the current of the input clocks. There is no way this is right >>> >>>> + private_data->clk81_div4.mult = 1; >>>> + private_data->clk81_div4.div = 4; >>>> + ret = meson_uart_register_clk(port, "clk81_div4", >>>> + &clk81_div_parent, 1, >>>> + &clk_fixed_factor_ops, >>>> + &private_data->clk81_div4.hw); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw; >>> This is only going to be used on meson8 and older. Worst case it should >>> depend >>> on this compatible but I don't think this would be right here. >>> IMO, this shows that the UART "baud" input was actually fed by a >>> derivation of "fclk_div4" instead of "clk_81" on these older SoCs. >>> So instead of registering what I suspect to be a fake element, the >>> meson8 clock controller driver and DT should be fixed a this should go away. >>> >> Virtually all UART controllers are supported, but they are not used after >> Meson8. So the description is more reasonable in UART driver. > > No it is not. It's not virtual, the support for meson8 is there in > mainline. > > Unless you tell me there is an actual /4 divider in the UART block of > the meson8 and older and that is was removed from the newer SoC, > the bit above should be removed and the meson8 clock tree fixed. > > I think it is more probable that the /4 was never there to begin with, > and that clk81/4 aka "fclk_div4", which is already available from the > clock controller, was actually routed on these older SoCs. I will check with the chip design department if it is divided by 4. I'll get back to you. > >>>> + } >>>> - port->uartclk = clk_get_rate(clk_baud); >>>> + if (private_data->has_xtal_clk_sel) { >>>> + private_data->xtal_div2.mult = 1; >>>> + private_data->xtal_div2.div = 2; >>>> + ret = meson_uart_register_clk(port, "xtal_div2", >>>> + &xtal_div_parent, 1, >>>> + &clk_fixed_factor_ops, >>>> + &private_data->xtal_div2.hw); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw; >>>> + xtal_clk_sel_mux_parents[1].fw_name = "xtal"; >>>> + >>>> + private_data->xtal_clk_sel_mux.reg = port->membase + AML_UART_REG5; >>>> + private_data->xtal_clk_sel_mux.mask = 0x1; >>>> + private_data->xtal_clk_sel_mux.shift = 26; >>>> + private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >>>> + ret = meson_uart_register_clk(port, "xtal_clk_sel", >>>> + xtal_clk_sel_mux_parents, >>>> + ARRAY_SIZE(xtal_clk_sel_mux_parents), >>>> + &clk_mux_ops, >>>> + &private_data->xtal_clk_sel_mux.hw); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + xtal2_clk_sel_mux_parents[0].hw = &private_data->xtal_clk_sel_mux.hw; >>>> + xtal2_clk_sel_mux_parents[1].hw = &private_data->xtal_div2.hw; >>>> + >>>> + private_data->xtal2_clk_sel_mux.reg = port->membase + AML_UART_REG5; >>>> + private_data->xtal2_clk_sel_mux.mask = 0x1; >>>> + private_data->xtal2_clk_sel_mux.shift = 27; >>>> + private_data->xtal2_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >>>> + ret = meson_uart_register_clk(port, "xtal2_clk_sel", >>>> + xtal2_clk_sel_mux_parents, >>>> + ARRAY_SIZE(xtal2_clk_sel_mux_parents), >>>> + &clk_mux_ops, >>>> + &private_data->xtal2_clk_sel_mux.hw); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + use_xtal_mux_parents[1].hw = &private_data->xtal2_clk_sel_mux.hw; >>>> + } else { >>>> + use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw; >>> Well the above is a bit over-complicated. If I summarize: >>> GXBB and older used a fixed divider of 3. Bits 26 and 27 read >>> 0 according to the documentation. >>> Chips after GXBB have 2 configurable divider of 2 and 3. bits 26 and 27 >>> selects which of these dividers is used. >>> So the above could be replaced with a single divider covering bits 26 and >>> 27 >>> with the following divider table { 2, 2, 1, 3 }. The divider should use >>> RO ops and not have CLK_SET_RATE_PARENT. It can be used for all chips >>> variant like this, including the older ones. >>> >> As you say, the way you say it is complicated. I think you're complicating >> things.I don't understand what you're trying to achieve. >> Can you be more specific? > > I'm proposing to replace the 4 elements above, paths, parent_table and > all with a single divider with a specific table. It accomplishes the same thing. > > |--------+--------+-----------| > | Bit 27 | Bit 26 | Div Value | > |--------+--------+-----------| > | 0 | 0 | 3 | > |--------+--------+-----------| > | 0 | 1 | 1 | > |--------+--------+-----------| > | 1 | 0 | 2 | > |--------+--------+-----------| > | 1 | 1 | 2 | > |--------+--------+-----------| > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk-provider.h?h=v5.16-rc8#n602 > > something like: > > static struct clk_div_table reg5_table[] = { > { 0, 3 }, > { 1, 1 }, > { 2, 2 }, > { 3, 2 }, > }; > > I'm also pointing out that, since bit 26 and 27 reads 0 on the older > SoCs, the divider would work for these too, as long as the Ops is > read-only. So you don't even need to register different element > depending on the SoC which simplify things a bit more. > > If you prefer to keep a fixed divider for the older ones, It's fine by > me. It gives a bit more work but it is closer to reality. The newer SoC > could still use the custom divider regardless. > > Last, and as explained on other bits, that part of the clock path should > remain RO at first, to keep behavior stable with this change. You may > change that later on, in a dedicated patch describing the change. > I understand what you mean, and i will try to realize it. I hope you can help review and give me your suggestions. >> >>> The only information you need to carry if whether or not you want to >>> make this divider modifiable. This means using the dt data to store >>> clk_div_ops or clk_div_ro_ops pointer. >>> To avoid changing the behavior of the older platforms in this patch, I >>> would >>> suggest to make everything use clk_div_ro_ops first, and make another >>> patch to use clk_div_ops if necessary. >>> >>>> + } >>>> + >>>> + private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5; >>>> + private_data->use_xtal_mux.mask = 0x1; >>>> + private_data->use_xtal_mux.shift = 24; >>>> + private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST; >>>> + ret = meson_uart_register_clk(port, "use_xtal", use_xtal_mux_parents, >>>> + ARRAY_SIZE(use_xtal_mux_parents), >>>> + &clk_mux_ops, >>> Use RO ops here to start with. >>> You can make this writable with another patch explicitly describing the change. >> I agree with what you say, and I will correct it. >>> >>>> + &private_data->use_xtal_mux.hw); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + baud_div_parent.hw = &private_data->use_xtal_mux.hw; >>>> + >>>> + private_data->baud_div.reg = port->membase + AML_UART_REG5; >>>> + private_data->baud_div.shift = 0; >>>> + private_data->baud_div.width = 23; >>>> + private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST; >>>> + ret = meson_uart_register_clk(port, "baud_div", >>>> + &baud_div_parent, 1, >>>> + &clk_divider_ops, >>>> + &private_data->baud_div.hw); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + private_data->baud_clk = devm_clk_hw_get_clk(port->dev, >>>> + &private_data->baud_div.hw, >>>> + "baud_rate"); >>> There is a problem with this function in CCF. It will pin the driver to >>> itself, making it unremovable. It is an ongoing topic. For now, just use >>> "hw->clk" to get the struct *clk. >>> >>>> + if (IS_ERR(private_data->baud_clk)) >>>> + return dev_err_probe(port->dev, >>> I don't think anything can defer here, so dev_err_probe() is not >>> necessary I think >>> >> I agree with what you say, and I will correct it. >>>> + PTR_ERR(private_data->baud_clk), >>>> + "Failed to request the 'baud_rate' clock\n"); >>>> return 0; >>>> } >>>> static int meson_uart_probe(struct platform_device *pdev) >>>> { >>>> + struct meson_uart_data *private_data; >>>> struct resource *res_mem, *res_irq; >>>> + struct clk *clk_baud, *clk_xtal; >>>> + bool register_clk81_div4; >>>> struct uart_port *port; >>>> int ret = 0; >>>> int id = -1; >>>> @@ -711,18 +855,37 @@ 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->has_xtal_clk_sel = true; >>>> + >>>> + private_data->pclk = devm_clk_get(&pdev->dev, "pclk"); >>>> + if (IS_ERR(private_data->pclk)) >>>> + return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk), >>>> + "Failed to get the 'pclk' clock\n"); >>>> + >>>> + clk_baud = devm_clk_get(&pdev->dev, "baud"); >>>> + if (IS_ERR(clk_baud)) >>>> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud), >>>> + "Failed to get the 'baud' clock\n"); >>>> + >>>> + clk_xtal = devm_clk_get(&pdev->dev, "xtal"); >>>> + if (IS_ERR(clk_xtal)) >>>> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal), >>>> + "Failed to get the 'xtal' clock\n"); >>>> + >>> This is second time you call devm_clk_get() on these clocks. One >>> instance has to go away >>> >> I agree with what you say, and I will correct it. >>>> + register_clk81_div4 = clk_get_rate(clk_xtal) != clk_get_rate(clk_baud); >>>> + >>> The above is a ugly way to distinguish the meson8 (32bit) SoC family >>> from the rest. This definitely not the way to achieve it. >>> The right is the compatible data but here I think it is not >>> necessary. The clock input should be fixed. >>> >>>> + 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); >>>> @@ -735,6 +898,12 @@ static int meson_uart_probe(struct platform_device *pdev) >>>> port->x_char = 0; >>>> port->ops = &meson_uart_ops; >>>> port->fifosize = 64; >>>> + port->uartclk = clk_get_rate(clk_baud); >>>> + port->private_data = private_data; >>>> + >>>> + ret = meson_uart_probe_clocks(port, register_clk81_div4); >>>> + if (ret) >>>> + return ret; >>>> meson_ports[pdev->id] = port; >>>> platform_set_drvdata(pdev, port); >>>> @@ -761,10 +930,42 @@ 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, >>>> + }, >>>> + { >>>> + .compatible = "amlogic,meson-s4-uart", >>>> + .data = (void *)true, >>>> + }, >>>> + /* >>>> + * deprecated, don't use anymore because it doesn't differentiate >>>> + * between GXBB and GXL 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); >>> > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible. 2022-01-04 14:35 ` Yu Tu @ 2022-01-05 5:53 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2022-01-05 5:53 UTC (permalink / raw) To: Jerome Brunet, linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Martin Blumenstingl Hi Jerome, On 2022/1/4 22:35, Yu Tu wrote: > Hi Jerome, > Thank you very much for your patient reply. I have learned a lot > from it. > > On 2022/1/4 18:36, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> >> On Tue 04 Jan 2022 at 17:57, Yu Tu <yu.tu@amlogic.com> wrote: >> >>> Hi Jerome, >>> Thank you very much for your reply. >>> >>> On 2022/1/3 20:40, Jerome Brunet wrote: >>>> [ EXTERNAL EMAIL ] >>>> >>>> On Thu 30 Dec 2021 at 18:21, Yu Tu <yu.tu@amlogic.com> wrote: >>>> >>>>> Using the common Clock code to describe the UART baud rate clock makes >>>>> it easier for the UART driver to be compatible with the baud rate >>>>> requirements of the UART IP on different meson chips >>>>> >>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>> --- >>>>> drivers/tty/serial/Kconfig | 1 + >>>>> drivers/tty/serial/meson_uart.c | 311 >>>>> ++++++++++++++++++++++++++------ >>>>> 2 files changed, 257 insertions(+), 55 deletions(-) >>>>> >>>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >>>>> index 780908d43557..32e238173036 100644 >>>>> --- a/drivers/tty/serial/Kconfig >>>>> +++ b/drivers/tty/serial/Kconfig >>>>> @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI >>>>> config SERIAL_MESON >>>>> tristate "Meson serial port support" >>>>> depends on ARCH_MESON >>>>> + depends on COMMON_CLK >>>>> select SERIAL_CORE >>>>> help >>>>> This enables the driver for the on-chip UARTs of the Amlogic >>>>> diff --git a/drivers/tty/serial/meson_uart.c >>>>> b/drivers/tty/serial/meson_uart.c >>>>> index 99efe62a1507..07eb1f40aaaa 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,21 @@ >>>>> #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; >>>>> + struct clk_divider baud_div; >>>>> + struct clk_mux use_xtal_mux; >>>>> + struct clk_mux xtal_clk_sel_mux; >>>>> + struct clk_mux xtal2_clk_sel_mux; >>>>> + struct clk_fixed_factor xtal_div2; >>>>> + struct clk_fixed_factor xtal_div3; >>>>> + struct clk_fixed_factor clk81_div4; >>>> Keeping all these internal elements around is not useful since they are >>>> registered using devm_ >>>> >>> I'm sorry. I don't know what you mean. That's exactly what you said, but >>> what's wrong, I don't understand. Do you have any better suggestions, >>> please specify specific points, preferably give examples. >> >> I'm saying that you don't need to keep reference to the internal >> elements of the clock tree you have registered since devm_ will take >> care of the removal later on. IOW, Once they are registered, the pointer >> is never used again so you don't need it in the private data. >> > I understand you now. What you say is right. I will correct it. >> >>>>> + bool no_clk81_input; >>>> What is this ? >>>> >>> To distinguish between clK81 and XTAL. >> >> ... Yet, it is not used >> >>>>> + bool has_xtal_clk_sel; >>>>> +}; >>>>> + >>>>> static struct uart_driver meson_uart_driver; >>>>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>>>> @@ -270,14 +284,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); >>>>> @@ -295,19 +306,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) { >>>> This check shows that previous code assumed bit 24 was left untouched >>>> Below you can see that GXBB and newer used the XTAL path while older >>>> used the other >>>> Your change makes this dynamic which is another "unexpected" change. >>>> Please make the bit 24 mux RO to start with so the behavior remains >>>> unchanged >>>> for older SoCs. >>> I agree with what you say, and I will correct it. >>>> >>>>> - 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, >>>>> @@ -397,11 +406,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,56 +654,175 @@ 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) >>>>> +static int meson_uart_register_clk(struct uart_port *port, >>>>> + const char *name_suffix, >>>>> + const struct clk_parent_data *parent_data, >>>>> + unsigned int num_parents, >>>>> + const struct clk_ops *ops, >>>>> + struct clk_hw *hw) >>>>> { >>>>> - struct clk *clk = NULL; >>>>> + struct clk_init_data init = { }; >>>>> + char clk_name[32]; >>>>> int ret; >>>>> - clk = devm_clk_get(dev, id); >>>>> - if (IS_ERR(clk)) >>>>> - return clk; >>>>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", >>>>> dev_name(port->dev), >>>>> + name_suffix); >>>>> - ret = clk_prepare_enable(clk); >>>>> - if (ret) { >>>>> - dev_err(dev, "couldn't enable clk\n"); >>>>> - return ERR_PTR(ret); >>>>> - } >>>>> + init.name = clk_name; >>>>> + init.ops = ops; >>>>> + init.flags = CLK_SET_RATE_PARENT; >>>>> + init.parent_data = parent_data; >>>>> + init.num_parents = num_parents; >>>>> + >>>>> + hw->init = &init; >>>>> - devm_add_action_or_reset(dev, >>>>> - (void(*)(void *))clk_disable_unprepare, >>>>> - clk); >>>>> + ret = devm_clk_hw_register(port->dev, hw); >>>>> + if (ret) >>>>> + return dev_err_probe(port->dev, ret, >>>>> + "Failed to register the '%s' clock\n", >>>>> + clk_name); >>>>> - return clk; >>>>> + return ret; >>>>> } >>>>> -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; >>>>> +static int meson_uart_probe_clocks(struct uart_port *port, >>>>> + bool register_clk81_div4) >>>>> +{ >>>>> + struct meson_uart_data *private_data = port->private_data; >>>>> + struct clk_parent_data use_xtal_mux_parents[2] = { >>>>> + { .index = -1, }, >>>>> + { .index = -1, }, >>>>> + }; >>>>> + struct clk_parent_data xtal_clk_sel_mux_parents[2] = { }; >>>>> + struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { }; >>>>> + struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", }; >>>>> + struct clk_parent_data clk81_div_parent = { .fw_name = "baud", }; >>>>> + struct clk_parent_data baud_div_parent = { }; >>>>> + struct clk *clk_baud, *clk_xtal; >>>>> + int ret; >>>>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >>>>> - if (IS_ERR(clk_pclk)) >>>>> - return PTR_ERR(clk_pclk); >>>>> + 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"); >>>>> + >>>>> + clk_baud = devm_clk_get(port->dev, "baud"); >>>>> + if (IS_ERR(clk_baud)) >>>>> + return dev_err_probe(port->dev, PTR_ERR(clk_baud), >>>>> + "Failed to get the 'baud' clock\n"); >>>>> - 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); >>>>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >>>>> + "Failed to get the 'xtal' clock\n"); >>>>> + >>>>> + private_data->xtal_div3.mult = 1; >>>>> + private_data->xtal_div3.div = 3; >>>>> + ret = meson_uart_register_clk(port, "xtal_div3", >>>>> &xtal_div_parent, >>>>> + 1, &clk_fixed_factor_ops, >>>>> + &private_data->xtal_div3.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >>>>> - if (IS_ERR(clk_baud)) >>>>> - return PTR_ERR(clk_baud); >>>>> + if (register_clk81_div4) { >>>> Clock dividers represent HW elements. The presence of an HW element >>>> cannot >>>> dependent on the current of the input clocks. There is no way this >>>> is right >>>> >>>>> + private_data->clk81_div4.mult = 1; >>>>> + private_data->clk81_div4.div = 4; >>>>> + ret = meson_uart_register_clk(port, "clk81_div4", >>>>> + &clk81_div_parent, 1, >>>>> + &clk_fixed_factor_ops, >>>>> + &private_data->clk81_div4.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw; >>>> This is only going to be used on meson8 and older. Worst case it should >>>> depend >>>> on this compatible but I don't think this would be right here. >>>> IMO, this shows that the UART "baud" input was actually fed by a >>>> derivation of "fclk_div4" instead of "clk_81" on these older SoCs. >>>> So instead of registering what I suspect to be a fake element, the >>>> meson8 clock controller driver and DT should be fixed a this should >>>> go away. >>>> >>> Virtually all UART controllers are supported, but they are not used >>> after >>> Meson8. So the description is more reasonable in UART driver. >> >> No it is not. It's not virtual, the support for meson8 is there in >> mainline. >> >> Unless you tell me there is an actual /4 divider in the UART block of >> the meson8 and older and that is was removed from the newer SoC, >> the bit above should be removed and the meson8 clock tree fixed. >> >> I think it is more probable that the /4 was never there to begin with, >> and that clk81/4 aka "fclk_div4", which is already available from the >> clock controller, was actually routed on these older SoCs. > I will check with the chip design department if it is divided by 4. I'll > get back to you. I confirmed with the chip design department that there is a 4 divider inside. So I'm going to use the divider 4 for the description, but only the older meson8 is in use, so I'm going to pass the parameter through compatible. Do you have any better suggestions? >> >>>>> + } >>>>> - port->uartclk = clk_get_rate(clk_baud); >>>>> + if (private_data->has_xtal_clk_sel) { >>>>> + private_data->xtal_div2.mult = 1; >>>>> + private_data->xtal_div2.div = 2; >>>>> + ret = meson_uart_register_clk(port, "xtal_div2", >>>>> + &xtal_div_parent, 1, >>>>> + &clk_fixed_factor_ops, >>>>> + &private_data->xtal_div2.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw; >>>>> + xtal_clk_sel_mux_parents[1].fw_name = "xtal"; >>>>> + >>>>> + private_data->xtal_clk_sel_mux.reg = port->membase + >>>>> AML_UART_REG5; >>>>> + private_data->xtal_clk_sel_mux.mask = 0x1; >>>>> + private_data->xtal_clk_sel_mux.shift = 26; >>>>> + private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST; >>>>> + ret = meson_uart_register_clk(port, "xtal_clk_sel", >>>>> + xtal_clk_sel_mux_parents, >>>>> + ARRAY_SIZE(xtal_clk_sel_mux_parents), >>>>> + &clk_mux_ops, >>>>> + &private_data->xtal_clk_sel_mux.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + xtal2_clk_sel_mux_parents[0].hw = >>>>> &private_data->xtal_clk_sel_mux.hw; >>>>> + xtal2_clk_sel_mux_parents[1].hw = >>>>> &private_data->xtal_div2.hw; >>>>> + >>>>> + private_data->xtal2_clk_sel_mux.reg = port->membase + >>>>> AML_UART_REG5; >>>>> + private_data->xtal2_clk_sel_mux.mask = 0x1; >>>>> + private_data->xtal2_clk_sel_mux.shift = 27; >>>>> + private_data->xtal2_clk_sel_mux.flags = >>>>> CLK_MUX_ROUND_CLOSEST; >>>>> + ret = meson_uart_register_clk(port, "xtal2_clk_sel", >>>>> + xtal2_clk_sel_mux_parents, >>>>> + ARRAY_SIZE(xtal2_clk_sel_mux_parents), >>>>> + &clk_mux_ops, >>>>> + &private_data->xtal2_clk_sel_mux.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + use_xtal_mux_parents[1].hw = >>>>> &private_data->xtal2_clk_sel_mux.hw; >>>>> + } else { >>>>> + use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw; >>>> Well the above is a bit over-complicated. If I summarize: >>>> GXBB and older used a fixed divider of 3. Bits 26 and 27 read >>>> 0 according to the documentation. >>>> Chips after GXBB have 2 configurable divider of 2 and 3. bits 26 and 27 >>>> selects which of these dividers is used. >>>> So the above could be replaced with a single divider covering bits >>>> 26 and >>>> 27 >>>> with the following divider table { 2, 2, 1, 3 }. The divider should use >>>> RO ops and not have CLK_SET_RATE_PARENT. It can be used for all chips >>>> variant like this, including the older ones. >>>> >>> As you say, the way you say it is complicated. I think you're >>> complicating >>> things.I don't understand what you're trying to achieve. >>> Can you be more specific? >> >> I'm proposing to replace the 4 elements above, paths, parent_table and >> all with a single divider with a specific table. It accomplishes the >> same thing. >> >> |--------+--------+-----------| >> | Bit 27 | Bit 26 | Div Value | >> |--------+--------+-----------| >> | 0 | 0 | 3 | >> |--------+--------+-----------| >> | 0 | 1 | 1 | >> |--------+--------+-----------| >> | 1 | 0 | 2 | >> |--------+--------+-----------| >> | 1 | 1 | 2 | >> |--------+--------+-----------| >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk-provider.h?h=v5.16-rc8#n602 >> >> >> something like: >> >> static struct clk_div_table reg5_table[] = { >> { 0, 3 }, >> { 1, 1 }, >> { 2, 2 }, >> { 3, 2 }, >> }; >> >> I'm also pointing out that, since bit 26 and 27 reads 0 on the older >> SoCs, the divider would work for these too, as long as the Ops is >> read-only. So you don't even need to register different element >> depending on the SoC which simplify things a bit more. >> >> If you prefer to keep a fixed divider for the older ones, It's fine by >> me. It gives a bit more work but it is closer to reality. The newer SoC >> could still use the custom divider regardless. >> >> Last, and as explained on other bits, that part of the clock path should >> remain RO at first, to keep behavior stable with this change. You may >> change that later on, in a dedicated patch describing the change. >> > I understand what you mean, and i will try to realize it. I hope you can > help review and give me your suggestions. >>> >>>> The only information you need to carry if whether or not you want to >>>> make this divider modifiable. This means using the dt data to store >>>> clk_div_ops or clk_div_ro_ops pointer. >>>> To avoid changing the behavior of the older platforms in this patch, I >>>> would >>>> suggest to make everything use clk_div_ro_ops first, and make another >>>> patch to use clk_div_ops if necessary. >>>> >>>>> + } >>>>> + >>>>> + private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5; >>>>> + private_data->use_xtal_mux.mask = 0x1; >>>>> + private_data->use_xtal_mux.shift = 24; >>>>> + private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST; >>>>> + ret = meson_uart_register_clk(port, "use_xtal", >>>>> use_xtal_mux_parents, >>>>> + ARRAY_SIZE(use_xtal_mux_parents), >>>>> + &clk_mux_ops, >>>> Use RO ops here to start with. >>>> You can make this writable with another patch explicitly describing >>>> the change. >>> I agree with what you say, and I will correct it. >>>> >>>>> + &private_data->use_xtal_mux.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + baud_div_parent.hw = &private_data->use_xtal_mux.hw; >>>>> + >>>>> + private_data->baud_div.reg = port->membase + AML_UART_REG5; >>>>> + private_data->baud_div.shift = 0; >>>>> + private_data->baud_div.width = 23; >>>>> + private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST; >>>>> + ret = meson_uart_register_clk(port, "baud_div", >>>>> + &baud_div_parent, 1, >>>>> + &clk_divider_ops, >>>>> + &private_data->baud_div.hw); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + private_data->baud_clk = devm_clk_hw_get_clk(port->dev, >>>>> + &private_data->baud_div.hw, >>>>> + "baud_rate"); >>>> There is a problem with this function in CCF. It will pin the driver to >>>> itself, making it unremovable. It is an ongoing topic. For now, just >>>> use >>>> "hw->clk" to get the struct *clk. >>>> >>>>> + if (IS_ERR(private_data->baud_clk)) >>>>> + return dev_err_probe(port->dev, >>>> I don't think anything can defer here, so dev_err_probe() is not >>>> necessary I think >>>> >>> I agree with what you say, and I will correct it. >>>>> + PTR_ERR(private_data->baud_clk), >>>>> + "Failed to request the 'baud_rate' clock\n"); >>>>> return 0; >>>>> } >>>>> static int meson_uart_probe(struct platform_device *pdev) >>>>> { >>>>> + struct meson_uart_data *private_data; >>>>> struct resource *res_mem, *res_irq; >>>>> + struct clk *clk_baud, *clk_xtal; >>>>> + bool register_clk81_div4; >>>>> struct uart_port *port; >>>>> int ret = 0; >>>>> int id = -1; >>>>> @@ -711,18 +855,37 @@ 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->has_xtal_clk_sel = true; >>>>> + >>>>> + private_data->pclk = devm_clk_get(&pdev->dev, "pclk"); >>>>> + if (IS_ERR(private_data->pclk)) >>>>> + return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk), >>>>> + "Failed to get the 'pclk' clock\n"); >>>>> + >>>>> + clk_baud = devm_clk_get(&pdev->dev, "baud"); >>>>> + if (IS_ERR(clk_baud)) >>>>> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud), >>>>> + "Failed to get the 'baud' clock\n"); >>>>> + >>>>> + clk_xtal = devm_clk_get(&pdev->dev, "xtal"); >>>>> + if (IS_ERR(clk_xtal)) >>>>> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal), >>>>> + "Failed to get the 'xtal' clock\n"); >>>>> + >>>> This is second time you call devm_clk_get() on these clocks. One >>>> instance has to go away >>>> >>> I agree with what you say, and I will correct it. >>>>> + register_clk81_div4 = clk_get_rate(clk_xtal) != >>>>> clk_get_rate(clk_baud); >>>>> + >>>> The above is a ugly way to distinguish the meson8 (32bit) SoC family >>>> from the rest. This definitely not the way to achieve it. >>>> The right is the compatible data but here I think it is not >>>> necessary. The clock input should be fixed. >>>> >>>>> + 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); >>>>> @@ -735,6 +898,12 @@ static int meson_uart_probe(struct >>>>> platform_device *pdev) >>>>> port->x_char = 0; >>>>> port->ops = &meson_uart_ops; >>>>> port->fifosize = 64; >>>>> + port->uartclk = clk_get_rate(clk_baud); >>>>> + port->private_data = private_data; >>>>> + >>>>> + ret = meson_uart_probe_clocks(port, register_clk81_div4); >>>>> + if (ret) >>>>> + return ret; >>>>> meson_ports[pdev->id] = port; >>>>> platform_set_drvdata(pdev, port); >>>>> @@ -761,10 +930,42 @@ 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, >>>>> + }, >>>>> + { >>>>> + .compatible = "amlogic,meson-s4-uart", >>>>> + .data = (void *)true, >>>>> + }, >>>>> + /* >>>>> + * deprecated, don't use anymore because it doesn't differentiate >>>>> + * between GXBB and GXL 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); >>>> >> ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu ` (3 preceding siblings ...) 2021-12-30 10:21 ` [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible Yu Tu @ 2021-12-30 10:21 ` Yu Tu 2021-12-30 22:44 ` Martin Blumenstingl 2021-12-30 10:21 ` [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() Yu Tu 5 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu This change is an improvement. The meson_uart_shutdown function should have the opposite operation to the meson_uart_startup function, but the meson_uart_shutdown of AML_UART_TX_EN is logically missing. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 07eb1f40aaaa..45e41f20cba3 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -140,7 +140,7 @@ static void meson_uart_shutdown(struct uart_port *port) spin_lock_irqsave(&port->lock, flags); val = readl(port->membase + AML_UART_CONTROL); - val &= ~AML_UART_RX_EN; + val &= ~(AML_UART_RX_EN | AML_UART_TX_EN); val &= ~(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN); writel(val, port->membase + AML_UART_CONTROL); -- 2.33.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit 2021-12-30 10:21 ` [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit Yu Tu @ 2021-12-30 22:44 ` Martin Blumenstingl 2021-12-31 10:42 ` Yu Tu 0 siblings, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-30 22:44 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On Thu, Dec 30, 2021 at 11:22 AM Yu Tu <yu.tu@amlogic.com> wrote: [...] > val = readl(port->membase + AML_UART_CONTROL); > - val &= ~AML_UART_RX_EN; > + val &= ~(AML_UART_RX_EN | AML_UART_TX_EN); > val &= ~(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN); > writel(val, port->membase + AML_UART_CONTROL); This looks fine to me but I think it would be good to Cc the author of commit 855ddcab352c15 ("ARM: meson: serial: only disable tx irq on stop") The meson_uart driver which Carlo added eight years ago did clear AML_UART_TX_EN here, but it was changed with the commit I mentioned above. Best regards, Martin ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit 2021-12-30 22:44 ` Martin Blumenstingl @ 2021-12-31 10:42 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2021-12-31 10:42 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On 2021/12/31 6:44, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > On Thu, Dec 30, 2021 at 11:22 AM Yu Tu <yu.tu@amlogic.com> wrote: > [...] >> val = readl(port->membase + AML_UART_CONTROL); >> - val &= ~AML_UART_RX_EN; >> + val &= ~(AML_UART_RX_EN | AML_UART_TX_EN); >> val &= ~(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN); >> writel(val, port->membase + AML_UART_CONTROL); > This looks fine to me but I think it would be good to Cc the author of > commit 855ddcab352c15 ("ARM: meson: serial: only disable tx irq on > stop") > The meson_uart driver which Carlo added eight years ago did clear > AML_UART_TX_EN here, but it was changed with the commit I mentioned > above. > I'm going to delete this change this time. Because there is no bug at present, if this is a bug encountered later, i will prepare to submit this change. > > Best regards, > Martin > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu ` (4 preceding siblings ...) 2021-12-30 10:21 ` [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit Yu Tu @ 2021-12-30 10:21 ` Yu Tu 2021-12-30 22:41 ` Martin Blumenstingl 5 siblings, 1 reply; 39+ messages in thread From: Yu Tu @ 2021-12-30 10:21 UTC (permalink / raw) To: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel Cc: Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet, Martin Blumenstingl, Yu Tu Because an interrupt error occurs when the user opens /dev/ttyAML* but don't close it, and then opens the same port again. This problem is encountered in actual projects. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- drivers/tty/serial/meson_uart.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c index 45e41f20cba3..0dd3f5b35768 100644 --- a/drivers/tty/serial/meson_uart.c +++ b/drivers/tty/serial/meson_uart.c @@ -135,8 +135,6 @@ static void meson_uart_shutdown(struct uart_port *port) unsigned long flags; u32 val; - free_irq(port->irq, port); - spin_lock_irqsave(&port->lock, flags); val = readl(port->membase + AML_UART_CONTROL); @@ -284,7 +282,6 @@ static void meson_uart_reset(struct uart_port *port) static int meson_uart_startup(struct uart_port *port) { u32 val; - int ret; meson_uart_reset(port); @@ -298,10 +295,7 @@ static int meson_uart_startup(struct uart_port *port) val = (AML_UART_RECV_IRQ(1) | AML_UART_XMIT_IRQ(port->fifosize / 2)); writel(val, port->membase + AML_UART_MISC); - ret = request_irq(port->irq, meson_uart_interrupt, 0, - port->name, port); - - return ret; + return 0; } static void meson_uart_change_speed(struct uart_port *port, unsigned long baud) @@ -908,6 +902,14 @@ static int meson_uart_probe(struct platform_device *pdev) meson_ports[pdev->id] = port; platform_set_drvdata(pdev, port); + ret = devm_request_irq(&pdev->dev, port->irq, meson_uart_interrupt, + 0, dev_name(&pdev->dev), port); + if (ret) { + dev_err(&pdev->dev, "failed to request uart irq: %d\n", + ret); + return ret; + } + /* reset port before registering (and possibly registering console) */ meson_uart_reset(port); -- 2.33.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() 2021-12-30 10:21 ` [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() Yu Tu @ 2021-12-30 22:41 ` Martin Blumenstingl 2021-12-31 10:37 ` Yu Tu 0 siblings, 1 reply; 39+ messages in thread From: Martin Blumenstingl @ 2021-12-30 22:41 UTC (permalink / raw) To: Yu Tu Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On Thu, Dec 30, 2021 at 11:22 AM Yu Tu <yu.tu@amlogic.com> wrote: > > Because an interrupt error occurs when the user opens /dev/ttyAML* but > don't close it, and then opens the same port again. This problem is > encountered in actual projects. I would like to hear from the serial driver maintainers whether the described problem is a userspace or driver bug. If it's a driver bug then this should be sent as a separate patch (unrelated to this series) with a fixes tag. [...] > + ret = devm_request_irq(&pdev->dev, port->irq, meson_uart_interrupt, > + 0, dev_name(&pdev->dev), port); You can replace dev_name(&pdev->dev) with NULL to achieve the same result with less code. dev_name(dev) is the default value, see [0] Best regards, Martin [0] https://elixir.bootlin.com/linux/v5.15/source/kernel/irq/devres.c#L64 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() 2021-12-30 22:41 ` Martin Blumenstingl @ 2021-12-31 10:37 ` Yu Tu 0 siblings, 0 replies; 39+ messages in thread From: Yu Tu @ 2021-12-31 10:37 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-serial, linux-arm-kernel, linux-amlogic, linux-kernel, Greg Kroah-Hartman, Jiri Slaby, Neil Armstrong, Vyacheslav, Kevin Hilman, Jerome Brunet On 2021/12/31 6:41, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > On Thu, Dec 30, 2021 at 11:22 AM Yu Tu <yu.tu@amlogic.com> wrote: >> >> Because an interrupt error occurs when the user opens /dev/ttyAML* but >> don't close it, and then opens the same port again. This problem is >> encountered in actual projects. > I would like to hear from the serial driver maintainers whether the > described problem is a userspace or driver bug. > > If it's a driver bug then this should be sent as a separate patch > (unrelated to this series) with a fixes tag. > > [...] >> + ret = devm_request_irq(&pdev->dev, port->irq, meson_uart_interrupt, >> + 0, dev_name(&pdev->dev), port); > You can replace dev_name(&pdev->dev) with NULL to achieve the same > result with less code. > dev_name(dev) is the default value, see [0] > I think you are right, I will remove this patch. I learned a lot. > > Best regards, > Martin > > > [0] https://elixir.bootlin.com/linux/v5.15/source/kernel/irq/devres.c#L64 > ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2022-01-05 5:54 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-30 10:21 [PATCH V3 0/6] the UART driver compatible with the Amlogic Meson Yu Tu 2021-12-30 10:21 ` [PATCH V3 1/6] tty: serial: meson: Drop the legacy compatible strings and clock code Yu Tu 2021-12-30 22:22 ` Martin Blumenstingl 2021-12-31 10:27 ` Yu Tu 2021-12-31 15:35 ` Martin Blumenstingl 2022-01-03 14:59 ` Neil Armstrong 2022-01-03 15:19 ` Greg Kroah-Hartman 2022-01-03 15:29 ` Neil Armstrong 2022-01-03 16:29 ` Greg Kroah-Hartman 2022-01-03 16:35 ` Neil Armstrong 2021-12-30 10:21 ` [PATCH V3 2/6] tty: serial: meson: Request the register region in meson_uart_probe() Yu Tu 2021-12-30 12:29 ` Greg Kroah-Hartman 2021-12-30 22:28 ` Martin Blumenstingl 2021-12-30 10:21 ` [PATCH V3 3/6] dt-bindings: serial: meson: Support S4 SoC uart. Also Drop compatible = amlogic,meson-gx-uart Yu Tu 2021-12-30 12:28 ` Greg Kroah-Hartman 2021-12-31 10:18 ` Yu Tu 2021-12-30 22:34 ` Martin Blumenstingl 2021-12-31 10:35 ` Yu Tu 2021-12-30 10:21 ` [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible Yu Tu 2021-12-30 12:27 ` Greg Kroah-Hartman 2021-12-31 10:23 ` Yu Tu 2021-12-30 23:13 ` Martin Blumenstingl 2021-12-31 11:24 ` Yu Tu 2021-12-31 15:32 ` Martin Blumenstingl 2022-01-01 13:30 ` Yu Tu 2022-01-02 19:36 ` Martin Blumenstingl 2022-01-04 8:20 ` Yu Tu 2022-01-03 13:50 ` Jerome Brunet 2022-01-03 12:40 ` Jerome Brunet 2022-01-04 9:57 ` Yu Tu 2022-01-04 10:36 ` Jerome Brunet 2022-01-04 14:35 ` Yu Tu 2022-01-05 5:53 ` Yu Tu 2021-12-30 10:21 ` [PATCH V3 5/6] tty: serial: meson: meson_uart_shutdown omit clear AML_UART_TX_EN bit Yu Tu 2021-12-30 22:44 ` Martin Blumenstingl 2021-12-31 10:42 ` Yu Tu 2021-12-30 10:21 ` [PATCH V3 6/6] tty: serial: meson: Change request_irq to devm_request_irq and move devm_request_irq to meson_uart_probe() Yu Tu 2021-12-30 22:41 ` Martin Blumenstingl 2021-12-31 10:37 ` Yu Tu
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).