Hi Simon

On 08/02/2018 06:56 PM, Simon Glass wrote:

Hi Patrice,

On 1 August 2018 at 09:58, 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

---

Changes in v2:

• Update SERIAL_BITS_MASK and SERIAL_STOP_MASK
• Update serial_setconfig prototype

drivers/serial/serial-uclass.c | 16 ++++++++++++++++ include/serial.h | 42 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 321d23ee93bf..a7556c9b7bc3 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -287,6 +287,20 @@ void serial_setbrg(void) ops->setbrg(gd->cur_serial_dev, gd->baudrate); }

+int serial_setconfig(uint config) +{

•   struct dm_serial_ops *ops;
  

•   if (!gd->cur_serial_dev)
  

•           return 0;
  

•   ops = serial_get_ops(gd->cur_serial_dev);
  

•   if (ops->setconfig)
  

•           return ops->setconfig(gd->cur_serial_dev, config);
  

•   return 0;
  

+}

• void serial_stdio_init(void) { }

@@ -398,6 +412,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..4f104f6a7188 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

Sorry I should have said this explicitly, but can you please update the other masks as well?

No problem, i will update this as well

Thanks

Patrice

...

+#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_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_SHIFT 4 +#define SERIAL_STOP_MASK (0x3 << SERIAL_STOP_SHIFT) +#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, uint serial_config);
  

  };

  /**

-- 1.9.1

Regards, Simon

Hi Patrice,

On 1 August 2018 at 09:58, 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

---

Changes in v2:

• Update stm32_serial_setconfig prototype

drivers/serial/serial_stm32.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

Please see below.

Also I worry about bisectability. U-Boot should always build every commit and I worry that your previous patch will break things, fixed by this patch.

diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c index f26234549c3e..53866a23e6fc 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, uint 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);
  

I don't see the benefit of using u8 here. Isn't uint good enough?

•   /*
  

•    * 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) = {

1.9.1

Regards, Simon

Hi Simon

On 08/02/2018 06:57 PM, Simon Glass wrote:

On 1 August 2018 at 09:58, Patrice Chotard patrice.chotard@st.com wrote:

...

Signed-off-by: Patrice Chotard patrice.chotard@st.com

Changes in v2:

• Add sandbox serial test

drivers/serial/sandbox.c | 14 ++++++++++++++ include/common.h | 1 + test/dm/Makefile | 1 + test/dm/serial.c | 26 ++++++++++++++++++++++++++ 4 files changed, 42 insertions(+) create mode 100644 test/dm/serial.c

Reviewed-by: Simon Glass sjg@chromium.org

How about also a test that checks it returns -ENOTSUPP when the wrong options are specified?

Effectively, it currently misses, i will add a specific test.

Thanks

Patrice

...

diff --git a/drivers/serial/sandbox.c b/drivers/serial/sandbox.c index a60dabe58835..94b4fdfb1714 100644 --- a/drivers/serial/sandbox.c +++ b/drivers/serial/sandbox.c @@ -143,6 +143,19 @@ static int sandbox_serial_getc(struct udevice *dev) return result; }

+static int sandbox_serial_setconfig(struct udevice *dev, uint serial_config) +{

•   u8 parity = SERIAL_GET_PARITY(serial_config);
  

•   u8 bits = SERIAL_GET_BITS(serial_config);
  

•   u8 stop = SERIAL_GET_STOP(serial_config);
  

•   if (bits != SERIAL_8_BITS || stop != SERIAL_ONE_STOP ||
  

•       parity != SERIAL_PAR_NONE)
  

•           return -ENOTSUPP; /* not supported in driver*/
  

•   return 0;
  

+}

• static const char * const ansi_colour[] = { "black", "red", "green", "yellow", "blue", "megenta", "cyan", "white",

@@ -173,6 +186,7 @@ static const struct dm_serial_ops sandbox_serial_ops = { .putc = sandbox_serial_putc, .pending = sandbox_serial_pending, .getc = sandbox_serial_getc,

•   .setconfig = sandbox_serial_setconfig,
  

  };

  static const struct udevice_id sandbox_serial_ids[] = {

diff --git a/include/common.h b/include/common.h index 940161f1758b..5c952af5e319 100644 --- a/include/common.h +++ b/include/common.h @@ -359,6 +359,7 @@ void serial_putc_raw(const char); void serial_puts (const char *); int serial_getc (void); int serial_tstc (void); +int serial_setconfig(uint config);

/* $(CPU)/speed.c */ int get_clocks (void); diff --git a/test/dm/Makefile b/test/dm/Makefile index d2ed96c61533..97517c7f825e 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -44,4 +44,5 @@ obj-$(CONFIG_DM_VIDEO) += video.o obj-$(CONFIG_ADC) += adc.o obj-$(CONFIG_SPMI) += spmi.o obj-$(CONFIG_WDT) += wdt.o +obj-$(CONFIG_DM_SERIAL) += serial.o endif diff --git a/test/dm/serial.c b/test/dm/serial.c new file mode 100644 index 000000000000..4d8422eebd34 --- /dev/null +++ b/test/dm/serial.c @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2018, STMicroelectronics
• */

+#include <common.h> +#include <serial.h> +#include <dm.h> +#include <dm/test.h> +#include <test/ut.h>

+static int dm_test_serial(struct unit_test_state *uts) +{

•   struct udevice *dev_serial;
  

•   ut_assertok(uclass_get_device_by_name(UCLASS_SERIAL, "serial",
  

•                                         &dev_serial));
  

•   ut_assertok(serial_tstc());
  

•   ut_assertok(serial_setconfig(SERIAL_DEFAULT_CONFIG));
  

•   return 0;
  

+}

+DM_TEST(dm_test_serial, DM_TESTF_SCAN_FDT);

1.9.1