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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
---
drivers/tty/serial/meson_uart.c | 34 ++-------------------------------
1 file changed, 2 insertions(+), 32 deletions(-)
@@ -625,10 +625,7 @@ meson_serial_early_console_setup(struct earlycon_device *device, const char *optdevice->con->write=meson_serial_early_console_write;return0;}-/* 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);
@@ -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)returnret;
@@ -800,9 +773,6 @@ static int meson_uart_remove(struct platform_device *pdev)}staticconststructof_device_idmeson_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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
---
drivers/tty/serial/meson_uart.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
@@ -397,24 +397,11 @@ static int meson_uart_verify_port(struct uart_port *port,staticvoidmeson_uart_release_port(structuart_port*port){-devm_iounmap(port->dev,port->membase);-port->membase=NULL;-devm_release_mem_region(port->dev,port->mapbase,port->mapsize);+/* nothing to do */}staticintmeson_uart_request_port(structuart_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;-return0;}
@@ -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))+returnPTR_ERR(port->membase);+ret=meson_uart_probe_clocks(pdev,port);if(ret)returnret;
@@ -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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
---
.../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
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 <redacted>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
2 files changed, 257 insertions(+), 55 deletions(-)
@@ -295,19 +306,17 @@ static int meson_uart_startup(struct uart_port *port)staticvoidmeson_uart_change_speed(structuart_port*port,unsignedlongbaud){+structmeson_uart_data*private_data=port->private_data;u32val;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);}staticvoidmeson_uart_set_termios(structuart_port*port,
@@ -397,11 +406,27 @@ static int meson_uart_verify_port(struct uart_port *port,staticvoidmeson_uart_release_port(structuart_port*port){-/* nothing to do */+structmeson_uart_data*private_data=port->private_data;++clk_disable_unprepare(private_data->baud_clk);+clk_disable_unprepare(private_data->pclk);}staticintmeson_uart_request_port(structuart_port*port){+structmeson_uart_data*private_data=port->private_data;+intret;++ret=clk_prepare_enable(private_data->pclk);+if(ret)+returnret;++ret=clk_prepare_enable(private_data->baud_clk);+if(ret){+clk_disable_unprepare(private_data->pclk);+returnret;+}+return0;}
@@ -629,56 +654,175 @@ static struct uart_driver meson_uart_driver = {.cons=MESON_SERIAL_CONSOLE,};-staticinlinestructclk*meson_uart_probe_clock(structdevice*dev,-constchar*id)+staticintmeson_uart_register_clk(structuart_port*port,+constchar*name_suffix,+conststructclk_parent_data*parent_data,+unsignedintnum_parents,+conststructclk_ops*ops,+structclk_hw*hw){-structclk*clk=NULL;+structclk_init_datainit={};+charclk_name[32];intret;-clk=devm_clk_get(dev,id);-if(IS_ERR(clk))-returnclk;+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");-returnERR_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)+returndev_err_probe(port->dev,ret,+"Failed to register the '%s' clock\n",+clk_name);-returnclk;+returnret;}-staticintmeson_uart_probe_clocks(structplatform_device*pdev,-structuart_port*port)-{-structclk*clk_xtal=NULL;-structclk*clk_pclk=NULL;-structclk*clk_baud=NULL;+staticintmeson_uart_probe_clocks(structuart_port*port,+boolregister_clk81_div4)+{+structmeson_uart_data*private_data=port->private_data;+structclk_parent_datause_xtal_mux_parents[2]={+{.index=-1,},+{.index=-1,},+};+structclk_parent_dataxtal_clk_sel_mux_parents[2]={};+structclk_parent_dataxtal2_clk_sel_mux_parents[2]={};+structclk_parent_dataxtal_div_parent={.fw_name="xtal",};+structclk_parent_dataclk81_div_parent={.fw_name="baud",};+structclk_parent_databaud_div_parent={};+structclk*clk_baud,*clk_xtal;+intret;-clk_pclk=meson_uart_probe_clock(&pdev->dev,"pclk");-if(IS_ERR(clk_pclk))-returnPTR_ERR(clk_pclk);+private_data->pclk=devm_clk_get(port->dev,"pclk");+if(IS_ERR(private_data->pclk))+returndev_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))+returndev_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))-returnPTR_ERR(clk_xtal);+returndev_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)+returnret;-clk_baud=meson_uart_probe_clock(&pdev->dev,"baud");-if(IS_ERR(clk_baud))-returnPTR_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)+returnret;++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)+returnret;++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)+returnret;++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)+returnret;++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)+returnret;++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)+returnret;++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))+returndev_err_probe(port->dev,+PTR_ERR(private_data->baud_clk),+"Failed to request the 'baud_rate' clock\n");return0;}staticintmeson_uart_probe(structplatform_device*pdev){+structmeson_uart_data*private_data;structresource*res_mem,*res_irq;+structclk*clk_baud,*clk_xtal;+boolregister_clk81_div4;structuart_port*port;intret=0;intid=-1;
@@ -711,18 +855,37 @@ static int meson_uart_probe(struct platform_device *pdev)return-EBUSY;}-port=devm_kzalloc(&pdev->dev,sizeof(structuart_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))+returndev_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))+returndev_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))+returndev_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))returnPTR_ERR(port->membase);-ret=meson_uart_probe_clocks(pdev,port);-if(ret)-returnret;-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)+returnret;meson_ports[pdev->id]=port;platform_set_drvdata(pdev,port);
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 <redacted>
---
drivers/tty/serial/meson_uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
drivers/tty/serial/meson_uart.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
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 <redacted>
---
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 <redacted>
---
.../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
On Thu, Dec 30, 2021 at 06:21:06PM +0800, Yu Tu wrote:
quoted hunk
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 <redacted>
---
drivers/tty/serial/meson_uart.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
@@ -397,24 +397,11 @@ static int meson_uart_verify_port(struct uart_port *port,staticvoidmeson_uart_release_port(structuart_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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-30 22:22:40
On Thu, Dec 30, 2021 at 11:21 AM Yu Tu [off-list ref] 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 <redacted>
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);
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-30 22:34:58
Hello,
as Greg already mentioned the $subject line is very long.
On Thu, Dec 30, 2021 at 11:21 AM Yu Tu [off-list ref] 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.
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-30 22:41:24
On Thu, Dec 30, 2021 at 11:22 AM Yu Tu [off-list ref] 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);
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-30 22:44:52
On Thu, Dec 30, 2021 at 11:22 AM Yu Tu [off-list ref] 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-30 23:13:52
On Thu, Dec 30, 2021 at 11:21 AM Yu Tu [off-list ref] wrote:
[...]
quoted hunk
@@ -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).
[...]
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:
quoted
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 <redacted>
---
.../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[ EXTERNAL EMAIL ]
On Thu, Dec 30, 2021 at 06:21:08PM +0800, Yu Tu wrote:
quoted
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 <redacted>
---
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 [off-list ref] wrote:
quoted
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 <redacted>
I have just realized that this cannot be dropped until my series "ARM:
dts: meson: fix UART device-tree schema validation" [0] is merged
[...]
quoted
-/* 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.
[ EXTERNAL EMAIL ]
Hello,
as Greg already mentioned the $subject line is very long.
On Thu, Dec 30, 2021 at 11:21 AM Yu Tu [off-list ref] wrote:
quoted
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.
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.
[ EXTERNAL EMAIL ]
On Thu, Dec 30, 2021 at 11:22 AM Yu Tu [off-list ref] wrote:
quoted
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.
[...]
quoted
+ 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.
[ EXTERNAL EMAIL ]
On Thu, Dec 30, 2021 at 11:22 AM Yu Tu [off-list ref] wrote:
[...]
quoted
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.
[ EXTERNAL EMAIL ]
On Thu, Dec 30, 2021 at 11:21 AM Yu Tu [off-list ref] wrote:
[...]
quoted
@@ -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.
[...]
quoted
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.
.
[...]
quoted
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?
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.
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-31 15:33:01
On Fri, Dec 31, 2021 at 12:24 PM Yu Tu [off-list ref] wrote:
[...]
quoted
quoted
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?
[...]
quoted
quoted
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2021-12-31 15:35:37
On Fri, Dec 31, 2021 at 11:27 AM Yu Tu [off-list ref] wrote:
[...]
quoted
quoted
-/* 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.
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 [off-list ref] wrote:
[...]
quoted
quoted
quoted
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.
[...]
quoted
quoted
quoted
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.
quoted
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.
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: 2022-01-02 19:37:04
Hi,
On Sat, Jan 1, 2022 at 2:30 PM Yu Tu [off-list ref] wrote:
[...]
quoted
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu 30 Dec 2021 at 18:21, Yu Tu [off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
2 files changed, 257 insertions(+), 55 deletions(-)
@@ -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.
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.
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.
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
quoted hunk
+ 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
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.
quoted hunk
+ 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);
On Sat 01 Jan 2022 at 21:30, Yu Tu [off-list ref] wrote:
Hi Martin,
Thank you very much for your reply.
On 2021/12/31 23:32, Martin Blumenstingl wrote:
quoted
[ EXTERNAL EMAIL ]
On Fri, Dec 31, 2021 at 12:24 PM Yu Tu [off-list ref] wrote:
[...]
quoted
quoted
quoted
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
quoted
[...]
quoted
quoted
quoted
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.
quoted
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.
From: Neil Armstrong <hidden> Date: 2022-01-03 14:59:39
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 [off-list ref] wrote:
[...]
quoted
quoted
quoted
-/* 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.
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Neil Armstrong <hidden> Date: 2022-01-03 15:30:01
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:
quoted
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
quoted
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.
quoted
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.
quoted
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
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:
quoted
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:
quoted
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
quoted
quoted
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.
quoted
quoted
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?
quoted
quoted
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Neil Armstrong <hidden> Date: 2022-01-03 16:35:07
On 03/01/2022 17:29, Greg Kroah-Hartman wrote:
On Mon, Jan 03, 2022 at 04:29:56PM +0100, Neil Armstrong wrote:
quoted
quoted
quoted
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
quoted
quoted
quoted
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.
quoted
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.
quoted
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
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 [off-list ref] wrote:
[...]
quoted
quoted
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 :-)
quoted
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?
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 [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
2 files changed, 257 insertions(+), 55 deletions(-)
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.
@@ -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.
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.
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.
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.
quoted
+ 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.
quoted
+ 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
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.
quoted
+ 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);
On Tue 04 Jan 2022 at 17:57, Yu Tu [off-list ref] wrote:
Hi Jerome,
Thank you very much for your reply.
On 2022/1/3 20:40, Jerome Brunet wrote:
quoted
[ EXTERNAL EMAIL ]
On Thu 30 Dec 2021 at 18:21, Yu Tu [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
2 files changed, 257 insertions(+), 55 deletions(-)
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.
@@ -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.
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.
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.
quoted
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.
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.
quoted
+ 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.
quoted
quoted
+ 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
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.
quoted
+ 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);
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 [off-list ref] wrote:
quoted
Hi Jerome,
Thank you very much for your reply.
On 2022/1/3 20:40, Jerome Brunet wrote:
quoted
[ EXTERNAL EMAIL ]
On Thu 30 Dec 2021 at 18:21, Yu Tu [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/meson_uart.c | 311 ++++++++++++++++++++++++++------
2 files changed, 257 insertions(+), 55 deletions(-)
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.
@@ -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.
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.
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.
quoted
quoted
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.
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.
quoted
+ 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.
quoted
quoted
+ 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
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.
quoted
+ 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);
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:
quoted
[ EXTERNAL EMAIL ]
On Tue 04 Jan 2022 at 17:57, Yu Tu [off-list ref] wrote:
quoted
Hi Jerome,
Thank you very much for your reply.
On 2022/1/3 20:40, Jerome Brunet wrote:
quoted
[ EXTERNAL EMAIL ]
On Thu 30 Dec 2021 at 18:21, Yu Tu [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/meson_uart.c | 311
++++++++++++++++++++++++++------
2 files changed, 257 insertions(+), 55 deletions(-)
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
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.
*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.
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?
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.
quoted
quoted
quoted
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.
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.
quoted
+ 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.
quoted
quoted
+ 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
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.
quoted
+ 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