bsp: cvitek: fix cvitek uart driver can not repeat configure #9257

Z8MAN8 · 2024-07-27T09:12:35Z

After first called from uart open, the dw8250_uart_configure API cannot be called again. Otherwise, this will mess up the device, and uart will not act properly to interrupt again.

Analysis: Configure uart device will close recive interrupte, causing uart device to malfunction.

Solution: After configure uart device, enable the device's recive interrupte.

Reviewed-by: Chen Wang [email protected]

拉取/合并请求描述：(PR description)

[

为什么提交这份PR (why to submit this PR)

Fixed #9150

你的解决方案是什么 (what is your solution)

After configure uart device, enable the device's recive interrupte.

请提供验证的bsp和config (provide the config and bsp)

BSP: bsp/cvitek

.config:

action:

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality：

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

已经仔细查看过代码改动的对比 Already check the difference between PR and old code
代码风格正确，包括缩进空格，命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
没有垃圾代码，代码尽量精简，不包含#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up
所有变更均有原因及合理的，并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
代码是高质量的 Code in this PR is of high quality
已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification

Z8MAN8 · 2024-07-27T09:13:33Z

@unicornx

BernardXiong · 2024-07-28T12:19:43Z

bsp/cvitek/drivers/drv_uart.c

@@ -132,6 +132,8 @@ static rt_err_t dw8250_uart_configure(struct rt_serial_device *serial, struct se
 clock_divisor = DIV_ROUND_CLOSEST(UART_INPUT_CLK, 16 * serial->config.baud_rate);
 dw8250_uart_setbrg(base, clock_divisor);

+ dw8250_write32(base, UART_IER, UART_IER_RDI);


这里应该不是这么来操作。如果涉及到中断，可以先行判断之前的状态是什么样的。

unicornx · 2024-07-29T05:47:38Z

@Z8MAN8 请在 #9150 里 AT 我一下，这样我就可以 assign 那个 issue 给你了。

unicornx · 2024-07-29T06:04:46Z

bsp/cvitek/drivers/drv_uart.c

@@ -132,6 +134,9 @@ static rt_err_t dw8250_uart_configure(struct rt_serial_device *serial, struct se
 clock_divisor = DIV_ROUND_CLOSEST(UART_INPUT_CLK, 16 * serial->config.baud_rate);
 dw8250_uart_setbrg(base, clock_divisor);

+ if(last_ier_state == UART_IER_RDI)


if 和 ( 之间应该有空格，这个 formating 检查不出来吗？

另外，直接判断 last_ier_state == UART_IER_RDI 不妥，而是要判断 IER[0] 这一位以前是否是 1，IER 其他位也可能会是 1。

if 和 ( 之间应该有空格，这个 formating 检查不出来吗？

另外，直接判断 last_ier_state == UART_IER_RDI 不妥，而是要判断 IER[0] 这一位以前是否是 1，IER 其他位也可能会是 1。

其他的 bit 位是不是也应该恢复？所以这个 issue 的完整 fix 应该就是一个完整的 backup -> restore 的操作。

formating没检查出来，已手动更改，位判断也已解决

formating没检查出来，已手动更改，位判断也已解决

只恢复 UART_IER_RDI 这个位是不够的，其他 bit 的值也需要恢复。

BTW：我不知道 github 是否有机制重新发起下一次 review，你可以研究一下。
最土的方法，你下次每一次改好重新 push 后，可以在 PR 里明确 AT 所有的 reviewer 然后请求第 N 次 review。有时候 review 的 PR 多了我也不知道你是否改过了，以及是否重新 push 过。

formating没检查出来，已手动更改，位判断也已解决

只恢复 UART_IER_RDI 这个位是不够的，其他 bit 的值也需要恢复。

BTW：我不知道 github 是否有机制重新发起下一次 review，你可以研究一下。最土的方法，你下次每一次改好重新 push 后，可以在 PR 里明确 AT 所有的 reviewer 然后请求第 N 次 review。有时候 review 的 PR 多了我也不知道你是否改过了，以及是否重新 push 过。

追加一个问题：

为啥当初代码里是直接将 UART_IER 置 0，传统的做法一般是对需要的位设置，其他位保持不变。
另外，这个 dw8250_uart_configure 为什么用不到 cfg 这个参数？请研究一下。

没有保持其他位确实是有问题的，根据 cfg 传参进行配置这部分确实缺少了，需要补上

已重新修改，请重新review

unicornx

comments 见上。

unicornx · 2024-08-16T08:00:45Z

这个 PR 先解决 #9150 的问题吧，cfg 传参进行配置这部分缺少的问题后面再单独修改吧。

Z8MAN8 · 2024-08-16T08:36:00Z

这个 PR 先解决 #9150 的问题吧，cfg 传参进行配置这部分缺少的问题后面再单独修改吧。

好的明白

After first called from uart open, the dw8250_uart_configure API cannot be called again. Otherwise, this will mess up the device, and uart will not act properly to interrupt again. Analysis: Configure uart device will close recive interrupte, causing uart device to malfunction. Solution: After configure uart device, enable the device's recive interrupte. Signed-off-by: Shicheng Chu <[email protected]> Reviewed-by: Chen Wang <[email protected]>

unicornx · 2024-08-16T07:58:52Z

bsp/cvitek/drivers/drv_uart.c

@@ -132,6 +134,9 @@ static rt_err_t dw8250_uart_configure(struct rt_serial_device *serial, struct se
 clock_divisor = DIV_ROUND_CLOSEST(UART_INPUT_CLK, 16 * serial->config.baud_rate);
 dw8250_uart_setbrg(base, clock_divisor);

+ if (last_ier_state & UART_IER_RDI)
+ dw8250_write32(base, UART_IER, UART_IER_RDI);


再说一遍：这里只恢复了 UART_IER_RDI 对应的 bit 位，那么原先其他的 bit 位状态还是被覆盖为 0 了。

mysterywolf · 2024-08-28T22:47:49Z

@unicornx 请确认这个PR是否可以合并？

unicornx · 2024-08-29T00:03:19Z

@unicornx 请确认这个PR是否可以合并？

我这里没有问题了，也 approve 了。但我看熊大还没有 approve。

unicornx · 2024-08-29T01:07:57Z

@BernardXiong hi, 请 review & commnet，谢谢。

Rbb666 added the BSP: Cvitek BSP related with cvitek label Jul 27, 2024

mysterywolf requested a review from unicornx July 28, 2024 09:00

BernardXiong reviewed Jul 28, 2024

View reviewed changes

Z8MAN8 force-pushed the cvitek_uart_driver_cfg_bug branch from 9ca3d50 to 9e4dcf5 Compare July 28, 2024 15:30

unicornx self-assigned this Jul 29, 2024

unicornx reviewed Jul 29, 2024

View reviewed changes

unicornx suggested changes Jul 29, 2024

View reviewed changes

Z8MAN8 force-pushed the cvitek_uart_driver_cfg_bug branch from 9e4dcf5 to 953db55 Compare July 29, 2024 06:41

unicornx assigned Z8MAN8 and unassigned unicornx Jul 29, 2024

Z8MAN8 force-pushed the cvitek_uart_driver_cfg_bug branch from 953db55 to 9fb118d Compare August 23, 2024 14:23

Z8MAN8 requested review from BernardXiong and unicornx August 23, 2024 14:24

unicornx approved these changes Aug 25, 2024

View reviewed changes

mysterywolf approved these changes Aug 29, 2024

View reviewed changes

BernardXiong approved these changes Sep 4, 2024

View reviewed changes

BernardXiong added the +1 Agree +1 label Sep 4, 2024

mysterywolf merged commit cea7670 into RT-Thread:master Sep 4, 2024
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bsp: cvitek: fix cvitek uart driver can not repeat configure #9257

bsp: cvitek: fix cvitek uart driver can not repeat configure #9257

Z8MAN8 commented Jul 27, 2024 •

edited

Loading

Z8MAN8 commented Jul 27, 2024

BernardXiong Jul 28, 2024

Z8MAN8 Jul 28, 2024

unicornx commented Jul 29, 2024

unicornx Jul 29, 2024 •

edited

Loading

unicornx Jul 29, 2024

Z8MAN8 Jul 29, 2024

unicornx Jul 29, 2024 •

edited

Loading

unicornx Jul 29, 2024 •

edited

Loading

Z8MAN8 Jul 29, 2024

Z8MAN8 Aug 23, 2024

unicornx left a comment

unicornx commented Aug 16, 2024

Z8MAN8 commented Aug 16, 2024

unicornx Aug 16, 2024

mysterywolf commented Aug 28, 2024

unicornx commented Aug 29, 2024

unicornx commented Aug 29, 2024

bsp: cvitek: fix cvitek uart driver can not repeat configure #9257

bsp: cvitek: fix cvitek uart driver can not repeat configure #9257

Conversation

Z8MAN8 commented Jul 27, 2024 • edited Loading

拉取/合并请求描述：(PR description)

为什么提交这份PR (why to submit this PR)

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

当前拉取/合并请求的状态 Intent for your PR

代码质量 Code Quality：

Z8MAN8 commented Jul 27, 2024

BernardXiong Jul 28, 2024

Choose a reason for hiding this comment

Z8MAN8 Jul 28, 2024

Choose a reason for hiding this comment

unicornx commented Jul 29, 2024

unicornx Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

unicornx Jul 29, 2024

Choose a reason for hiding this comment

Z8MAN8 Jul 29, 2024

Choose a reason for hiding this comment

unicornx Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

unicornx Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Z8MAN8 Jul 29, 2024

Choose a reason for hiding this comment

Z8MAN8 Aug 23, 2024

Choose a reason for hiding this comment

unicornx left a comment

Choose a reason for hiding this comment

unicornx commented Aug 16, 2024

Z8MAN8 commented Aug 16, 2024

unicornx Aug 16, 2024

Choose a reason for hiding this comment

mysterywolf commented Aug 28, 2024

unicornx commented Aug 29, 2024

unicornx commented Aug 29, 2024

Z8MAN8 commented Jul 27, 2024 •

edited

Loading

unicornx Jul 29, 2024 •

edited

Loading

unicornx Jul 29, 2024 •

edited

Loading

unicornx Jul 29, 2024 •

edited

Loading