[U-Boot] [PATCH v1 0/2] Replace serial setparity by setconfig
This series : _ replace setparity ops by more complete setconfig in serial uclass _ replace setparity by setconfig in STM32 serial driver
Patrice Chotard (1): serial: stm32: Replace setparity by setconfig
Patrick Delaunay (1): dm: serial: Replace setparity by setconfig
drivers/serial/serial-uclass.c | 14 ++++++++++++++ drivers/serial/serial_stm32.c | 21 +++++++++++++++------ include/serial.h | 42 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 68 insertions(+), 9 deletions(-)
From: Patrick Delaunay patrick.delaunay@st.com
Replace setparity by more generic setconfig ops to allow uart parity, bits word length and stop bits number change.
Adds SERIAL_GET_PARITY/BITS/STOP macros.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com ---
drivers/serial/serial-uclass.c | 14 ++++++++++++++ include/serial.h | 42 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23ee93bf..9f523751ce17 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -287,6 +287,18 @@ void serial_setbrg(void) ops->setbrg(gd->cur_serial_dev, gd->baudrate); }
+void serial_setconfig(u8 config) +{ + struct dm_serial_ops *ops; + + if (!gd->cur_serial_dev) + return; + + ops = serial_get_ops(gd->cur_serial_dev); + if (ops->setconfig) + ops->setconfig(gd->cur_serial_dev, config); +} + void serial_stdio_init(void) { } @@ -398,6 +410,8 @@ static int serial_post_probe(struct udevice *dev) ops->pending += gd->reloc_off; if (ops->clear) ops->clear += gd->reloc_off; + if (ops->setconfig) + ops->setconfig += gd->reloc_off; #if CONFIG_POST & CONFIG_SYS_POST_UART if (ops->loop) ops->loop += gd->reloc_off diff --git a/include/serial.h b/include/serial.h index b9ef6d91c9c5..61c1362e9e32 100644 --- a/include/serial.h +++ b/include/serial.h @@ -73,6 +73,39 @@ enum serial_par { SERIAL_PAR_EVEN };
+#define SERIAL_PAR_MASK 0x03 +#define SERIAL_PAR_SHIFT 0 +#define SERIAL_GET_PARITY(config) \ + ((config & SERIAL_PAR_MASK) >> SERIAL_PAR_SHIFT) + +enum serial_bits { + SERIAL_5_BITS, + SERIAL_6_BITS, + SERIAL_7_BITS, + SERIAL_8_BITS +}; + +#define SERIAL_BITS_MASK 0x0C +#define SERIAL_BITS_SHIFT 2 +#define SERIAL_GET_BITS(config) \ + ((config & SERIAL_BITS_MASK) >> SERIAL_BITS_SHIFT) + +enum serial_stop { + SERIAL_HALF_STOP, /* 0.5 stop bit */ + SERIAL_ONE_STOP, /* 1 stop bit */ + SERIAL_ONE_HALF_STOP, /* 1.5 stop bit */ + SERIAL_TWO_STOP /* 2 stop bit */ +}; + +#define SERIAL_STOP_MASK 0x30 +#define SERIAL_STOP_SHIFT 4 +#define SERIAL_GET_STOP(config) \ + ((config & SERIAL_STOP_MASK) >> SERIAL_STOP_SHIFT) + +#define SERIAL_DEFAULT_CONFIG SERIAL_PAR_NONE << SERIAL_PAR_SHIFT | \ + SERIAL_8_BITS << SERIAL_BITS_SHIFT | \ + SERIAL_ONE_STOP << SERIAL_STOP_SHIFT + /** * struct struct dm_serial_ops - Driver model serial operations * @@ -150,15 +183,18 @@ struct dm_serial_ops { int (*loop)(struct udevice *dev, int on); #endif /** - * setparity() - Set up the parity + * setconfig() - Set up the uart configuration + * (parity, 5/6/7/8 bits word length, stop bits) * - * Set up a new parity for this device. + * Set up a new config for this device. * * @dev: Device pointer * @parity: parity to use + * @bits: bits number to use + * @stop: stop bits number to use * @return 0 if OK, -ve on error */ - int (*setparity)(struct udevice *dev, enum serial_par parity); + int (*setconfig)(struct udevice *dev, u8 serial_config); };
/**
Hi Patrice,
On 30 July 2018 at 09:23, Patrice Chotard patrice.chotard@st.com wrote:
From: Patrick Delaunay patrick.delaunay@st.com
Replace setparity by more generic setconfig ops to allow uart parity, bits word length and stop bits number change.
Adds SERIAL_GET_PARITY/BITS/STOP macros.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com
drivers/serial/serial-uclass.c | 14 ++++++++++++++ include/serial.h | 42 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23ee93bf..9f523751ce17 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -287,6 +287,18 @@ void serial_setbrg(void) ops->setbrg(gd->cur_serial_dev, gd->baudrate); }
+void serial_setconfig(u8 config) +{
struct dm_serial_ops *ops;if (!gd->cur_serial_dev)return;ops = serial_get_ops(gd->cur_serial_dev);if (ops->setconfig)ops->setconfig(gd->cur_serial_dev, config);+}
void serial_stdio_init(void) { } @@ -398,6 +410,8 @@ static int serial_post_probe(struct udevice *dev) ops->pending += gd->reloc_off; if (ops->clear) ops->clear += gd->reloc_off;
if (ops->setconfig)ops->setconfig += gd->reloc_off;#if CONFIG_POST & CONFIG_SYS_POST_UART if (ops->loop) ops->loop += gd->reloc_off diff --git a/include/serial.h b/include/serial.h index b9ef6d91c9c5..61c1362e9e32 100644 --- a/include/serial.h +++ b/include/serial.h @@ -73,6 +73,39 @@ enum serial_par { SERIAL_PAR_EVEN };
+#define SERIAL_PAR_MASK 0x03 +#define SERIAL_PAR_SHIFT 0 +#define SERIAL_GET_PARITY(config) \
((config & SERIAL_PAR_MASK) >> SERIAL_PAR_SHIFT)+enum serial_bits {
SERIAL_5_BITS,SERIAL_6_BITS,SERIAL_7_BITS,SERIAL_8_BITS+};
+#define SERIAL_BITS_MASK 0x0C +#define SERIAL_BITS_SHIFT 2
For consistency:
#define SERIAL_BITS_SHIFT 2 #define SERIAL_BITS_MASK (0x3 << SERIAL_BITS_SHIFT)
+#define SERIAL_GET_BITS(config) \
((config & SERIAL_BITS_MASK) >> SERIAL_BITS_SHIFT)+enum serial_stop {
SERIAL_HALF_STOP, /* 0.5 stop bit */SERIAL_ONE_STOP, /* 1 stop bit */SERIAL_ONE_HALF_STOP, /* 1.5 stop bit */SERIAL_TWO_STOP /* 2 stop bit */+};
+#define SERIAL_STOP_MASK 0x30 +#define SERIAL_STOP_SHIFT 4
same here
+#define SERIAL_GET_STOP(config) \
((config & SERIAL_STOP_MASK) >> SERIAL_STOP_SHIFT)+#define SERIAL_DEFAULT_CONFIG SERIAL_PAR_NONE << SERIAL_PAR_SHIFT | \
SERIAL_8_BITS << SERIAL_BITS_SHIFT | \SERIAL_ONE_STOP << SERIAL_STOP_SHIFT/**
- struct struct dm_serial_ops - Driver model serial operations
@@ -150,15 +183,18 @@ struct dm_serial_ops { int (*loop)(struct udevice *dev, int on); #endif /**
* setparity() - Set up the parity
* setconfig() - Set up the uart configuration* (parity, 5/6/7/8 bits word length, stop bits) *
* Set up a new parity for this device.
* Set up a new config for this device. * * @dev: Device pointer * @parity: parity to use* @bits: bits number to use* @stop: stop bits number to use * @return 0 if OK, -ve on error */
int (*setparity)(struct udevice *dev, enum serial_par parity);
int (*setconfig)(struct udevice *dev, u8 serial_config);
Please make this uint instead of u8. There is no point in using u8 since the compiler will use a register anyway. It can only make code size worse, if the compile add masking, etc.
};
/**
1.9.1
Also you need a serial_setconfig() function call in the uclass so people can call it.
Perhaps that could have separate parameters for each setting, so that the caller does not have to make up a mask? I'm not sure if that is better or not.
Also we need to call this from sandbox code for testing purposes,
Regards, Simon
Hi Simon
On 07/31/2018 01:52 PM, Simon Glass wrote:
Hi Patrice,
On 30 July 2018 at 09:23, Patrice Chotard patrice.chotard@st.com wrote:
From: Patrick Delaunay patrick.delaunay@st.com
Replace setparity by more generic setconfig ops to allow uart parity, bits word length and stop bits number change.
Adds SERIAL_GET_PARITY/BITS/STOP macros.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com
drivers/serial/serial-uclass.c | 14 ++++++++++++++ include/serial.h | 42 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23ee93bf..9f523751ce17 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -287,6 +287,18 @@ void serial_setbrg(void) ops->setbrg(gd->cur_serial_dev, gd->baudrate); }
+void serial_setconfig(u8 config) +{
struct dm_serial_ops *ops;if (!gd->cur_serial_dev)return;ops = serial_get_ops(gd->cur_serial_dev);if (ops->setconfig)ops->setconfig(gd->cur_serial_dev, config);+}
- void serial_stdio_init(void) { }
@@ -398,6 +410,8 @@ static int serial_post_probe(struct udevice *dev) ops->pending += gd->reloc_off; if (ops->clear) ops->clear += gd->reloc_off;
if (ops->setconfig)ops->setconfig += gd->reloc_off;diff --git a/include/serial.h b/include/serial.h index b9ef6d91c9c5..61c1362e9e32 100644 --- a/include/serial.h +++ b/include/serial.h @@ -73,6 +73,39 @@ enum serial_par { SERIAL_PAR_EVEN };
+#define SERIAL_PAR_MASK 0x03 +#define SERIAL_PAR_SHIFT 0 +#define SERIAL_GET_PARITY(config) \
((config & SERIAL_PAR_MASK) >> SERIAL_PAR_SHIFT)+enum serial_bits {
SERIAL_5_BITS,SERIAL_6_BITS,SERIAL_7_BITS,SERIAL_8_BITS+};
+#define SERIAL_BITS_MASK 0x0C +#define SERIAL_BITS_SHIFT 2
For consistency:
#define SERIAL_BITS_SHIFT 2 #define SERIAL_BITS_MASK (0x3 << SERIAL_BITS_SHIFT)
Ok
+#define SERIAL_GET_BITS(config) \
((config & SERIAL_BITS_MASK) >> SERIAL_BITS_SHIFT)+enum serial_stop {
SERIAL_HALF_STOP, /* 0.5 stop bit */SERIAL_ONE_STOP, /* 1 stop bit */SERIAL_ONE_HALF_STOP, /* 1.5 stop bit */SERIAL_TWO_STOP /* 2 stop bit */+};
+#define SERIAL_STOP_MASK 0x30 +#define SERIAL_STOP_SHIFT 4
same here
ok
+#define SERIAL_GET_STOP(config) \
((config & SERIAL_STOP_MASK) >> SERIAL_STOP_SHIFT)+#define SERIAL_DEFAULT_CONFIG SERIAL_PAR_NONE << SERIAL_PAR_SHIFT | \
SERIAL_8_BITS << SERIAL_BITS_SHIFT | \SERIAL_ONE_STOP << SERIAL_STOP_SHIFT- /**
- struct struct dm_serial_ops - Driver model serial operations
@@ -150,15 +183,18 @@ struct dm_serial_ops { int (*loop)(struct udevice *dev, int on); #endif /**
* setparity() - Set up the parity
* setconfig() - Set up the uart configuration* (parity, 5/6/7/8 bits word length, stop bits) *
* Set up a new parity for this device.
* Set up a new config for this device. * * @dev: Device pointer * @parity: parity to use* @bits: bits number to use* @stop: stop bits number to use * @return 0 if OK, -ve on error */
int (*setparity)(struct udevice *dev, enum serial_par parity);
int (*setconfig)(struct udevice *dev, u8 serial_config);Please make this uint instead of u8. There is no point in using u8 since the compiler will use a register anyway. It can only make code size worse, if the compile add masking, etc.
ok
};
/**
1.9.1
Also you need a serial_setconfig() function call in the uclass so people can call it.
I already add serial_setconfig() function call in serial-uclass at the beginning of this patch ;-)
Perhaps that could have separate parameters for each setting, so that the caller does not have to make up a mask? I'm not sure if that is better or not.
Don't know what is better, currently only STM32 platforms will use it, internally we already use this API.
Also we need to call this from sandbox code for testing purposes,
Ok i will add a test for this.
Thanks
Patrice
Regards, Simon
Hi Patrice,
On 31 July 2018 at 07:53, Patrice CHOTARD patrice.chotard@st.com wrote:
Hi Simon
On 07/31/2018 01:52 PM, Simon Glass wrote:
Hi Patrice,
On 30 July 2018 at 09:23, Patrice Chotard patrice.chotard@st.com wrote:
From: Patrick Delaunay patrick.delaunay@st.com
Replace setparity by more generic setconfig ops to allow uart parity, bits word length and stop bits number change.
Adds SERIAL_GET_PARITY/BITS/STOP macros.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com
drivers/serial/serial-uclass.c | 14 ++++++++++++++ include/serial.h | 42 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23ee93bf..9f523751ce17 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -287,6 +287,18 @@ void serial_setbrg(void) ops->setbrg(gd->cur_serial_dev, gd->baudrate); }
+void serial_setconfig(u8 config) +{
struct dm_serial_ops *ops;if (!gd->cur_serial_dev)return;ops = serial_get_ops(gd->cur_serial_dev);if (ops->setconfig)ops->setconfig(gd->cur_serial_dev, config);+}
- void serial_stdio_init(void) { }
@@ -398,6 +410,8 @@ static int serial_post_probe(struct udevice *dev) ops->pending += gd->reloc_off; if (ops->clear) ops->clear += gd->reloc_off;
if (ops->setconfig)ops->setconfig += gd->reloc_off;diff --git a/include/serial.h b/include/serial.h index b9ef6d91c9c5..61c1362e9e32 100644 --- a/include/serial.h +++ b/include/serial.h @@ -73,6 +73,39 @@ enum serial_par { SERIAL_PAR_EVEN };
+#define SERIAL_PAR_MASK 0x03 +#define SERIAL_PAR_SHIFT 0 +#define SERIAL_GET_PARITY(config) \
((config & SERIAL_PAR_MASK) >> SERIAL_PAR_SHIFT)+enum serial_bits {
SERIAL_5_BITS,SERIAL_6_BITS,SERIAL_7_BITS,SERIAL_8_BITS+};
+#define SERIAL_BITS_MASK 0x0C +#define SERIAL_BITS_SHIFT 2
For consistency:
#define SERIAL_BITS_SHIFT 2 #define SERIAL_BITS_MASK (0x3 << SERIAL_BITS_SHIFT)
Ok
+#define SERIAL_GET_BITS(config) \
((config & SERIAL_BITS_MASK) >> SERIAL_BITS_SHIFT)+enum serial_stop {
SERIAL_HALF_STOP, /* 0.5 stop bit */SERIAL_ONE_STOP, /* 1 stop bit */SERIAL_ONE_HALF_STOP, /* 1.5 stop bit */SERIAL_TWO_STOP /* 2 stop bit */+};
+#define SERIAL_STOP_MASK 0x30 +#define SERIAL_STOP_SHIFT 4
same here
ok
+#define SERIAL_GET_STOP(config) \
((config & SERIAL_STOP_MASK) >> SERIAL_STOP_SHIFT)+#define SERIAL_DEFAULT_CONFIG SERIAL_PAR_NONE << SERIAL_PAR_SHIFT | \
SERIAL_8_BITS << SERIAL_BITS_SHIFT | \SERIAL_ONE_STOP << SERIAL_STOP_SHIFT- /**
- struct struct dm_serial_ops - Driver model serial operations
@@ -150,15 +183,18 @@ struct dm_serial_ops { int (*loop)(struct udevice *dev, int on); #endif /**
* setparity() - Set up the parity
* setconfig() - Set up the uart configuration* (parity, 5/6/7/8 bits word length, stop bits) *
* Set up a new parity for this device.
* Set up a new config for this device. * * @dev: Device pointer * @parity: parity to use* @bits: bits number to use* @stop: stop bits number to use * @return 0 if OK, -ve on error */
int (*setparity)(struct udevice *dev, enum serial_par parity);
int (*setconfig)(struct udevice *dev, u8 serial_config);Please make this uint instead of u8. There is no point in using u8 since the compiler will use a register anyway. It can only make code size worse, if the compile add masking, etc.
ok
};
/**
1.9.1
Also you need a serial_setconfig() function call in the uclass so people can call it.
I already add serial_setconfig() function call in serial-uclass at the beginning of this patch ;-)
OK good. Please change u8 there too :-)
Perhaps that could have separate parameters for each setting, so that the caller does not have to make up a mask? I'm not sure if that is better or not.
Don't know what is better, currently only STM32 platforms will use it, internally we already use this API.
OK well I think what you have will work. We can separate out the parameters later if we see a need.
Also we need to call this from sandbox code for testing purposes,
Ok i will add a test for this.
Regards, Simon
Replace stm32_serial_setparity by stm32_serial_setconfig which allows to set serial bits number, parity and stop bits number. Only parity setting is implemented.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com ---
drivers/serial/serial_stm32.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c index f26234549c3e..e6d383601c6c 100644 --- a/drivers/serial/serial_stm32.c +++ b/drivers/serial/serial_stm32.c @@ -47,20 +47,28 @@ static int stm32_serial_setbrg(struct udevice *dev, int baudrate) return 0; }
-static int stm32_serial_setparity(struct udevice *dev, enum serial_par parity) +static int stm32_serial_setconfig(struct udevice *dev, u8 serial_config) { struct stm32x7_serial_platdata *plat = dev_get_platdata(dev); bool stm32f4 = plat->uart_info->stm32f4; u8 uart_enable_bit = plat->uart_info->uart_enable_bit; u32 cr1 = plat->base + CR1_OFFSET(stm32f4); u32 config = 0; - - if (stm32f4) - return -EINVAL; /* not supported in driver*/ + u8 parity = SERIAL_GET_PARITY(serial_config); + u8 bits = SERIAL_GET_BITS(serial_config); + u8 stop = SERIAL_GET_STOP(serial_config); + + /* + * only parity config is implemented, check if other serial settings + * are the default one. + * (STM32F4 serial IP didn't support parity setting) + */ + if (bits != SERIAL_8_BITS || stop != SERIAL_ONE_STOP || stm32f4) + return -ENOTSUPP; /* not supported in driver*/
clrbits_le32(cr1, USART_CR1_RE | USART_CR1_TE | BIT(uart_enable_bit)); /* update usart configuration (uart need to be disable) - * PCE: parity check control + * PCE: parity check enable * PS : '0' : Even / '1' : Odd * M[1:0] = '00' : 8 Data bits * M[1:0] = '01' : 9 Data bits with parity @@ -77,6 +85,7 @@ static int stm32_serial_setparity(struct udevice *dev, enum serial_par parity) config = USART_CR1_PCE | USART_CR1_M0; break; } + clrsetbits_le32(cr1, USART_CR1_PCE | USART_CR1_PS | USART_CR1_M1 | USART_CR1_M0, @@ -210,7 +219,7 @@ static const struct dm_serial_ops stm32_serial_ops = { .pending = stm32_serial_pending, .getc = stm32_serial_getc, .setbrg = stm32_serial_setbrg, - .setparity = stm32_serial_setparity + .setconfig = stm32_serial_setconfig };
U_BOOT_DRIVER(serial_stm32) = {
Hi Patrice,
On 30 July 2018 at 09:23, Patrice Chotard patrice.chotard@st.com wrote:
Replace stm32_serial_setparity by stm32_serial_setconfig which allows to set serial bits number, parity and stop bits number. Only parity setting is implemented.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com
drivers/serial/serial_stm32.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
Looks OK, apart from changes requested in the first patch.
Regards, Simon
participants (3)
-
Patrice CHOTARD -
Patrice Chotard
-
Simon Glass