driver: uart-stm32: add line_ctrl_set/get support #8306

qianfan-Zhao · 2018-06-11T04:27:12Z

support change bandrate and hardware flow control at runtime

Signed-off-by: qianfan Zhao [email protected]

codecov-io · 2018-06-11T04:55:27Z

Codecov Report

Merging #8306 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #8306   +/-   ##
======================================
  Coverage    53.1%   53.1%           
======================================
  Files         218     218           
  Lines       26883   26883           
  Branches     5952    5952           
======================================
  Hits        14277   14277           
  Misses      10172   10172           
  Partials     2434    2434

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aa58ff...b6ecfc7. Read the comment docs.

erwango · 2018-06-11T09:08:56Z

drivers/serial/uart_stm32.h

@@ -19,6 +19,8 @@ struct uart_stm32_config {
 	struct stm32_pclken pclken;
 	/* Baud rate */
 	u32_t baud_rate;
+	/* HW flow control */
+	u32_t rts_cts:1;


"config" struct is device startup configuration.
You should not use it to store information that will be modified at run time.

yes, it's my mistake and actually 'config' is const and can't changed.

I want move baud_rate from uart_stm32_config to uart_stm32_data. Because uart_stm32_data can be changed at runtime. what's your opinion?

It should be ok to do that.
But what about setting/reading directly in registers?

change baud rate need disable usart first and then calc clock rate and then set registers. thats was almost did in stm32_init.

I had updated this pr setting directly in registers now. please review again

erwango · 2018-06-11T09:27:16Z

drivers/serial/uart_stm32.c

+	} while (0)
+
+	switch (ctrl) {
+	case LINE_CTRL_BAUD_RATE:


You should not store information in baud_rate which is used to hold start-up configuration.
I propose you update registers directly.

erwango · 2018-06-11T09:29:59Z

drivers/serial/uart_stm32.c

@@ -314,6 +293,15 @@ static int uart_stm32_init(struct device *dev)
 				 LL_USART_PARITY_NONE,
 				 LL_USART_STOPBITS_1);

+	/* HW flow control */
+	if (config->rts_cts) {


You should provide a start up configuration otherwise behavior is undefined here.

erwango · 2018-06-12T09:27:33Z

drivers/serial/uart_stm32.c

+
+	switch (ctrl) {
+	case LINE_CTRL_BAUD_RATE:
+		change_if_not_equal(baud_rate, val);


If we're able to update directly registers, why do you need to store information in driver data?
Using registers, you can save some memory.

erwango

Not sure we need to use data fields to store information, since we can use device registers.

qianfan-Zhao · 2018-06-12T10:14:09Z

@erwango

Not sure we need to use data fields to store information, since we can use device registers.

No, you should not do that. We must using an variable to store the baud_rate setted before.
the baud_rate we read using LL_USART_GetBaudRate maybe not equal the baud_rate setted using LL_USART_SetBaudRate because the BRR is inaccurate for some specify baud rate.

eg: I had set uart's baud rate to 115200, but the baudrate readed is 115107.

332             baud_rate = LL_USART_GetBaudRate(UartInstance, clock_rate, LL_USART_OVERSAMPLING_16);

(gdb) p baud_rate 
$1 = 115107

erwango · 2018-06-18T11:29:44Z

drivers/serial/uart_stm32.c

+		return -ENOTSUP;
+	}
+
+	return 0;


This line could not be accessed, code coverage tools will complain.
I propose to remove default and return -ENOSUP here.

erwango · 2018-06-18T11:30:48Z

drivers/serial/uart_stm32.c

 };									\
 									\
 static struct uart_stm32_data uart_stm32_data_##name = {		\
+	.baud_rate = CONFIG_UART_STM32_##name##_BAUD_RATE,		\
+	.rts_cts = 0,							\


Please provide a constant here so one can known what default value is actually.

sorry can you explain more clear?

Replace 0 with something like RTS_HW_FC_DISABLED

erwango · 2018-06-18T11:32:41Z

drivers/serial/uart_stm32.c

+static inline void uart_stm32_set_baud_rate(struct device *dev,
+					    u32_t clock_rate, u32_t baud_rate)
+{
+#if defined(CONFIG_SOC_SERIES_STM32L0X) || defined(CONFIG_SOC_SERIES_STM32L4X)


Please take care to rework this if #8359 is merged first

erwango · 2018-06-18T11:33:39Z

No, you should not do that. We must using an variable to store the baud_rate setted before.

Ok, let's keep it that way then

qianfan-Zhao · 2018-06-20T00:23:21Z

@erwango Done

erwango · 2018-06-29T08:45:53Z

recheck

erwango · 2018-06-29T09:57:15Z

@qianfan-Zhao , can you fix CI issue, so we can continue the review?

-:41: ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#41: FILE: drivers/serial/uart_stm32.c:264:
+	USART_TypeDef *UartInstance = UART_STRUCT(dev);

qianfan-Zhao · 2018-06-29T13:54:08Z

@erwango But I think the best way is no space, there shouldn't add a space between '*' and point. eg:

int p; / good /
int * p; / bad */

Can we ignore this error?

erwango · 2018-06-29T14:47:25Z

@qianfan-Zhao

Can we ignore this error?

I don't think so. It is strange that this "error" has not been detected before.
Was there any recent change in checkpatch.pl?

qianfan-Zhao · 2018-06-29T14:55:20Z

@erwango

This is the lastly commit for checkpatch.pl, seems that scripts haven't changed this days.

commit 65f0c67906941050171511c2e4c67a1e939ddd2d
Author: Anas Nashif <[email protected]>
Date:   Tue May 8 10:19:30 2018 -0500

    checkpatch: downgrade COMPLEX_MACRO to a warning
    
    This is generating lots of false positives and we keep overriding them
    manually, so downgrade and mark it as a warning.
    
    Signed-off-by: Anas Nashif <[email protected]>

erwango · 2018-06-29T14:58:48Z

This is really strange since CAN driver introduction also introduced some CAN_TypeDef *can = cfg->can; recently and no one complained

erwango

@qianfan-Zhao

Is that mandatory to store RTS_CTS in uart_stm32_data_XXX?
Can't we set and get info directly in registers?

erwango · 2018-10-19T10:02:31Z

drivers/serial/uart_stm32.h

 #ifdef CONFIG_UART_INTERRUPT_DRIVEN
 	uart_irq_callback_t user_cb;
 #endif
+


Remove this extra line

qianfan-Zhao · 2018-10-21T06:30:03Z

@erwango I had updated this pr based on your requested. Get RTS_CTS informations direct from register.

And has another issue fix:
I had noticed that not all serials(UART/USART) are supported hard flow control feature, and stm32cube provide a function to detect that, so I had update uart_stm32_line_ctrl_set in this way:

static int uart_stm32_line_ctrl_set(struct device *dev,
				    u32_t ctrl, u32_t val)
{
	USART_TypeDef *UartInstance = UART_STRUCT(dev);
	struct uart_stm32_data *data = DEV_DATA(dev);
	int result = 0;

	LL_USART_Disable(UartInstance);

	switch (ctrl) {
	case LINE_CTRL_BAUD_RATE:
		uart_stm32_set_baud_rate(dev, data->clock_rate, val);
		data->baud_rate = val;
		break;
	case LINE_CTRL_RTS:
		if (!IS_UART_HWFLOW_INSTANCE(UartInstance)) {
			result = -EINVAL;
			goto done;
		} else if (val) {
			LL_USART_SetHWFlowCtrl(UartInstance,
					       LL_USART_HWCONTROL_RTS_CTS);
		} else {
			LL_USART_SetHWFlowCtrl(UartInstance,
					       LL_USART_HWCONTROL_NONE);
		}
		break;
	default:
		result = -ENOTSUP;
		break;
	}

done:
	LL_USART_Enable(UartInstance);
	return result;
}

Please review this section carefully. I don't know if return -EINVAL if an uart instance doesn't support hardware flow control is OK.

qianfan-Zhao · 2018-10-21T07:52:13Z

@erwango Could you please also review PR #8307 ?

erwango

Minor comments, otherwise looks good

erwango · 2018-10-30T15:43:49Z

drivers/serial/uart_stm32.c

-#else
-	uart_stm32_usart_set_baud_rate(dev, clock_rate, baud_rate);
-#endif
+	uart_stm32_set_baud_rate(dev, data->clock_rate, baud_rate);


Why not using "data->baud_rate" directly there as well?

erwango · 2018-10-30T15:47:04Z

drivers/serial/uart_stm32.c

+		break;
+	case LINE_CTRL_RTS:
+		if (!IS_UART_HWFLOW_INSTANCE(UartInstance)) {
+			result = -EINVAL;


Maybe ENOTSUP would fit better here

erwango · 2018-10-30T15:49:58Z

drivers/serial/uart_stm32.c

+		}
+		break;
+	default:
+		result = -ENOTSUP;


EINVAL at this point?

qianfan-Zhao · 2018-10-31T00:12:05Z

@erwango recheck

erwango · 2018-11-08T16:45:16Z

@qianfan-Zhao ,

Can you fix CI and we're good:

-:41: ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#41: FILE: drivers/serial/uart_stm32.c:267:

USART_TypeDef *UartInstance = UART_STRUCT(dev);

erwango · 2018-11-09T09:07:03Z

Well, I'm unsure what is the issue here.
Checkpatch doesn't complain on my side and seems good to uncrustify as well.

qianfan-Zhao · 2018-11-09T09:31:21Z

@erwango Or just fix this issue to let Shippable happy?

erwango · 2018-11-09T09:49:00Z

@qianfan-Zhao:

@erwango Or just fix this issue to let Shippable happy?

Sure, if you manage to

support change bandrate and hardware flow control at runtime Signed-off-by: qianfan Zhao <[email protected]>

erwango · 2018-11-12T10:54:28Z

@qianfan-Zhao , this PR will now conflicts with #11185

Do you plan to rebase this PR on top of #11185 when merged?
Then, we'll try to get it merged much faster

qianfan-Zhao · 2018-11-12T11:20:55Z

@erwango sure, i will

erwango · 2019-01-02T15:05:28Z

#11185 merged

erwango · 2019-02-06T10:38:39Z

@qianfan-Zhao , closing this as #11185 merged.

qianfan-Zhao requested a review from erwango as a code owner June 11, 2018 04:27

qianfan-Zhao force-pushed the stm32-line-ctrl branch 2 times, most recently from 45ffb37 to d27d8c8 Compare June 11, 2018 06:55

erwango requested changes Jun 11, 2018

View reviewed changes

qianfan-Zhao force-pushed the stm32-line-ctrl branch from d27d8c8 to f9ae244 Compare June 11, 2018 10:03

erwango added platform: STM32 ST Micro STM32 area: UART Universal Asynchronous Receiver-Transmitter labels Jun 11, 2018

qianfan-Zhao force-pushed the stm32-line-ctrl branch 5 times, most recently from 2493699 to e40efad Compare June 12, 2018 05:57

erwango approved these changes Jun 12, 2018

View reviewed changes

erwango requested changes Jun 12, 2018

View reviewed changes

erwango requested changes Jun 18, 2018

View reviewed changes

qianfan-Zhao force-pushed the stm32-line-ctrl branch from e40efad to 2bd1367 Compare June 20, 2018 00:05

qianfan-Zhao force-pushed the stm32-line-ctrl branch from 2bd1367 to 55ec6ad Compare July 12, 2018 00:00

galak mentioned this pull request Jul 17, 2018

Driver API review/cleanup/rework #5697

Closed

33 tasks

erwango requested changes Oct 19, 2018

View reviewed changes

drivers/serial/uart_stm32.h Outdated

#ifdef CONFIG_UART_INTERRUPT_DRIVEN

uart_irq_callback_t user_cb;

#endif

Copy link

Member

erwango Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this extra line

qianfan-Zhao force-pushed the stm32-line-ctrl branch from 55ec6ad to a87b08e Compare October 21, 2018 06:26

qianfan-Zhao force-pushed the stm32-line-ctrl branch from a87b08e to 64e9541 Compare October 21, 2018 07:49

Mierunski mentioned this pull request Oct 24, 2018

api: uart: Add new asynchronous UART API (Transfer sub-API) #10820

Merged

erwango requested changes Oct 30, 2018

View reviewed changes

qianfan-Zhao force-pushed the stm32-line-ctrl branch from 64e9541 to 70485d4 Compare October 31, 2018 00:11

qianfan-Zhao force-pushed the stm32-line-ctrl branch from 70485d4 to 2b69d05 Compare November 9, 2018 07:49

qianfan-Zhao closed this Nov 9, 2018

qianfan-Zhao reopened this Nov 9, 2018

driver: uart-stm32: add line_ctrl_set/get support

b6ecfc7

support change bandrate and hardware flow control at runtime Signed-off-by: qianfan Zhao <[email protected]>

qianfan-Zhao force-pushed the stm32-line-ctrl branch from 2b69d05 to b6ecfc7 Compare November 9, 2018 10:22

qianfan-Zhao mentioned this pull request Jan 11, 2019

serial: stm32: Implement configure and config_get api calls #12366

Merged

erwango closed this Feb 6, 2019

driver: uart-stm32: add line_ctrl_set/get support #8306

driver: uart-stm32: add line_ctrl_set/get support #8306

Conversation

qianfan-Zhao commented Jun 11, 2018

codecov-io commented Jun 11, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango left a comment

Choose a reason for hiding this comment

qianfan-Zhao commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Jun 18, 2018

qianfan-Zhao commented Jun 20, 2018

erwango commented Jun 29, 2018

erwango commented Jun 29, 2018

qianfan-Zhao commented Jun 29, 2018

erwango commented Jun 29, 2018

qianfan-Zhao commented Jun 29, 2018

erwango commented Jun 29, 2018

erwango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qianfan-Zhao commented Oct 21, 2018

qianfan-Zhao commented Oct 21, 2018

erwango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qianfan-Zhao commented Oct 31, 2018

erwango commented Nov 8, 2018

erwango commented Nov 9, 2018

qianfan-Zhao commented Nov 9, 2018

erwango commented Nov 9, 2018

erwango commented Nov 12, 2018

qianfan-Zhao commented Nov 12, 2018

erwango commented Jan 2, 2019

erwango commented Feb 6, 2019

codecov-io commented Jun 11, 2018 •

edited

Loading

qianfan-Zhao commented Jun 12, 2018 •

edited

Loading