* [PATCH 0/4] lib: utils/serial: Collection of UART code improvements
@ 2022-07-18 17:20 Andrew Jones
2022-07-18 17:20 ` [PATCH 1/4] lib: utils/fdt: Factor out common uart node code Andrew Jones
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: Andrew Jones @ 2022-07-18 17:20 UTC (permalink / raw)
To: opensbi
This series is a collection of changes I made while reviewing the UART
code. I've compile tested it and boot it on QEMU (which of course only
tests uart8250). I did also pass in a DTB with 0 for 'current-speed' in
the UART node and nothing interesting happened, so I guess that's good.
Andrew Jones (4):
lib: utils/fdt: Factor out common uart node code
lib: utils/serial: Initialize platform_uart_data to zero
lib: serial: Clean up coding style in sifive-uart.c
lib: utils/serial: Ensure baudrate is non-zero before using
lib/utils/fdt/fdt_helper.c | 131 ++++++--------------
lib/utils/serial/fdt_serial_gaisler.c | 2 +-
lib/utils/serial/fdt_serial_shakti.c | 2 +-
lib/utils/serial/fdt_serial_sifive.c | 2 +-
lib/utils/serial/fdt_serial_uart8250.c | 2 +-
lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +-
lib/utils/serial/gaisler-uart.c | 2 +-
lib/utils/serial/shakti-uart.c | 8 +-
lib/utils/serial/sifive-uart.c | 13 +-
lib/utils/serial/uart8250.c | 7 +-
platform/fpga/openpiton/platform.c | 2 +-
11 files changed, 66 insertions(+), 107 deletions(-)
--
2.36.1
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/4] lib: utils/fdt: Factor out common uart node code 2022-07-18 17:20 [PATCH 0/4] lib: utils/serial: Collection of UART code improvements Andrew Jones @ 2022-07-18 17:20 ` Andrew Jones 2022-07-30 5:24 ` Anup Patel 2022-07-18 17:20 ` [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero Andrew Jones ` (2 subsequent siblings) 3 siblings, 1 reply; 22+ messages in thread From: Andrew Jones @ 2022-07-18 17:20 UTC (permalink / raw) To: opensbi Factor out the common code used by the fdt UART node parsers, allowing us to drop duplicate code. Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- lib/utils/fdt/fdt_helper.c | 131 +++++++++++-------------------------- 1 file changed, 39 insertions(+), 92 deletions(-) diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c index 2bdb1ef23e55..c6abf804014d 100644 --- a/lib/utils/fdt/fdt_helper.c +++ b/lib/utils/fdt/fdt_helper.c @@ -314,8 +314,10 @@ int fdt_parse_timebase_frequency(void *fdt, unsigned long *freq) return 0; } -int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, - struct platform_uart_data *uart) +static int fdt_parse_uart_node_common(void *fdt, int nodeoffset, + struct platform_uart_data *uart, + unsigned long default_freq, + unsigned long default_baud) { int len, rc; const fdt32_t *val; @@ -338,13 +340,28 @@ int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, if (len > 0 && val) uart->freq = fdt32_to_cpu(*val); else - uart->freq = DEFAULT_UART_FREQ; + uart->freq = default_freq; val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); if (len > 0 && val) uart->baud = fdt32_to_cpu(*val); else - uart->baud = DEFAULT_UART_BAUD; + uart->baud = default_baud; + + return 0; +} + +int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, + struct platform_uart_data *uart) +{ + int rc; + + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, + DEFAULT_UART_FREQ, + DEFAULT_UART_BAUD); + + if (rc) + return rc; /* For Gaisler APBUART, the reg-shift and reg-io-width are fixed .*/ uart->reg_shift = DEFAULT_UART_REG_SHIFT; @@ -356,69 +373,26 @@ int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, int fdt_parse_shakti_uart_node(void *fdt, int nodeoffset, struct platform_uart_data *uart) { - int len, rc; - const fdt32_t *val; - uint64_t reg_addr, reg_size; - - if (nodeoffset < 0 || !uart || !fdt) - return SBI_ENODEV; - - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, - ®_addr, ®_size); - if (rc < 0 || !reg_addr || !reg_size) - return SBI_ENODEV; - uart->addr = reg_addr; + int rc; - /** - * UART address is mandaotry. clock-frequency and current-speed - * may not be present. Don't return error. - */ - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); - if (len > 0 && val) - uart->freq = fdt32_to_cpu(*val); - else - uart->freq = DEFAULT_SHAKTI_UART_FREQ; + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, + DEFAULT_SHAKTI_UART_FREQ, + DEFAULT_SHAKTI_UART_BAUD); - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); - if (len > 0 && val) - uart->baud = fdt32_to_cpu(*val); - else - uart->baud = DEFAULT_SHAKTI_UART_BAUD; - - return 0; + return rc ? : 0; } int fdt_parse_sifive_uart_node(void *fdt, int nodeoffset, struct platform_uart_data *uart) { - int len, rc; - const fdt32_t *val; - uint64_t reg_addr, reg_size; - - if (nodeoffset < 0 || !uart || !fdt) - return SBI_ENODEV; - - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, - ®_addr, ®_size); - if (rc < 0 || !reg_addr || !reg_size) - return SBI_ENODEV; - uart->addr = reg_addr; + int rc; - /** - * UART address is mandaotry. clock-frequency and current-speed - * may not be present. Don't return error. - */ - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); - if (len > 0 && val) - uart->freq = fdt32_to_cpu(*val); - else - uart->freq = DEFAULT_SIFIVE_UART_FREQ; + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, + DEFAULT_SIFIVE_UART_FREQ, + DEFAULT_SIFIVE_UART_BAUD); - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); - if (len > 0 && val) - uart->baud = fdt32_to_cpu(*val); - else - uart->baud = DEFAULT_SIFIVE_UART_BAUD; + if (rc) + return rc; /* For SiFive UART, the reg-shift and reg-io-width are fixed .*/ uart->reg_shift = DEFAULT_SIFIVE_UART_REG_SHIFT; @@ -432,32 +406,13 @@ int fdt_parse_uart8250_node(void *fdt, int nodeoffset, { int len, rc; const fdt32_t *val; - uint64_t reg_addr, reg_size; - - if (nodeoffset < 0 || !uart || !fdt) - return SBI_ENODEV; - - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, - ®_addr, ®_size); - if (rc < 0 || !reg_addr || !reg_size) - return SBI_ENODEV; - uart->addr = reg_addr; - /** - * UART address is mandaotry. clock-frequency and current-speed - * may not be present. Don't return error. - */ - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); - if (len > 0 && val) - uart->freq = fdt32_to_cpu(*val); - else - uart->freq = DEFAULT_UART_FREQ; + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, + DEFAULT_UART_FREQ, + DEFAULT_UART_BAUD); - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); - if (len > 0 && val) - uart->baud = fdt32_to_cpu(*val); - else - uart->baud = DEFAULT_UART_BAUD; + if (rc) + return rc; val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "reg-shift", &len); if (len > 0 && val) @@ -499,18 +454,10 @@ int fdt_parse_xlnx_uartlite_node(void *fdt, int nodeoffset, struct platform_uart_data *uart) { int rc; - uint64_t reg_addr, reg_size; - if (nodeoffset < 0 || !uart || !fdt) - return SBI_ENODEV; + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, 0, 0); - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, - ®_addr, ®_size); - if (rc < 0 || !reg_addr || !reg_size) - return SBI_ENODEV; - uart->addr = reg_addr; - - return 0; + return rc ? : 0; } int fdt_parse_aplic_node(void *fdt, int nodeoff, struct aplic_data *aplic) -- 2.36.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 1/4] lib: utils/fdt: Factor out common uart node code 2022-07-18 17:20 ` [PATCH 1/4] lib: utils/fdt: Factor out common uart node code Andrew Jones @ 2022-07-30 5:24 ` Anup Patel 2022-07-30 10:19 ` Anup Patel 0 siblings, 1 reply; 22+ messages in thread From: Anup Patel @ 2022-07-30 5:24 UTC (permalink / raw) To: opensbi On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > Factor out the common code used by the fdt UART node parsers, > allowing us to drop duplicate code. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > lib/utils/fdt/fdt_helper.c | 131 +++++++++++-------------------------- > 1 file changed, 39 insertions(+), 92 deletions(-) > > diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c > index 2bdb1ef23e55..c6abf804014d 100644 > --- a/lib/utils/fdt/fdt_helper.c > +++ b/lib/utils/fdt/fdt_helper.c > @@ -314,8 +314,10 @@ int fdt_parse_timebase_frequency(void *fdt, unsigned long *freq) > return 0; > } > > -int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > - struct platform_uart_data *uart) > +static int fdt_parse_uart_node_common(void *fdt, int nodeoffset, > + struct platform_uart_data *uart, > + unsigned long default_freq, > + unsigned long default_baud) > { > int len, rc; > const fdt32_t *val; > @@ -338,13 +340,28 @@ int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > if (len > 0 && val) > uart->freq = fdt32_to_cpu(*val); > else > - uart->freq = DEFAULT_UART_FREQ; > + uart->freq = default_freq; > > val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > if (len > 0 && val) > uart->baud = fdt32_to_cpu(*val); > else > - uart->baud = DEFAULT_UART_BAUD; > + uart->baud = default_baud; > + > + return 0; > +} > + > +int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > + struct platform_uart_data *uart) > +{ > + int rc; > + > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > + DEFAULT_UART_FREQ, > + DEFAULT_UART_BAUD); > + > + if (rc) > + return rc; > > /* For Gaisler APBUART, the reg-shift and reg-io-width are fixed .*/ > uart->reg_shift = DEFAULT_UART_REG_SHIFT; > @@ -356,69 +373,26 @@ int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > int fdt_parse_shakti_uart_node(void *fdt, int nodeoffset, > struct platform_uart_data *uart) > { > - int len, rc; > - const fdt32_t *val; > - uint64_t reg_addr, reg_size; > - > - if (nodeoffset < 0 || !uart || !fdt) > - return SBI_ENODEV; > - > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > - ®_addr, ®_size); > - if (rc < 0 || !reg_addr || !reg_size) > - return SBI_ENODEV; > - uart->addr = reg_addr; > + int rc; > > - /** > - * UART address is mandaotry. clock-frequency and current-speed > - * may not be present. Don't return error. > - */ > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); > - if (len > 0 && val) > - uart->freq = fdt32_to_cpu(*val); > - else > - uart->freq = DEFAULT_SHAKTI_UART_FREQ; > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > + DEFAULT_SHAKTI_UART_FREQ, > + DEFAULT_SHAKTI_UART_BAUD); > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > - if (len > 0 && val) > - uart->baud = fdt32_to_cpu(*val); > - else > - uart->baud = DEFAULT_SHAKTI_UART_BAUD; > - > - return 0; > + return rc ? : 0; > } > > int fdt_parse_sifive_uart_node(void *fdt, int nodeoffset, > struct platform_uart_data *uart) > { > - int len, rc; > - const fdt32_t *val; > - uint64_t reg_addr, reg_size; > - > - if (nodeoffset < 0 || !uart || !fdt) > - return SBI_ENODEV; > - > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > - ®_addr, ®_size); > - if (rc < 0 || !reg_addr || !reg_size) > - return SBI_ENODEV; > - uart->addr = reg_addr; > + int rc; > > - /** > - * UART address is mandaotry. clock-frequency and current-speed > - * may not be present. Don't return error. > - */ > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); > - if (len > 0 && val) > - uart->freq = fdt32_to_cpu(*val); > - else > - uart->freq = DEFAULT_SIFIVE_UART_FREQ; > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > + DEFAULT_SIFIVE_UART_FREQ, > + DEFAULT_SIFIVE_UART_BAUD); > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > - if (len > 0 && val) > - uart->baud = fdt32_to_cpu(*val); > - else > - uart->baud = DEFAULT_SIFIVE_UART_BAUD; > + if (rc) > + return rc; > > /* For SiFive UART, the reg-shift and reg-io-width are fixed .*/ > uart->reg_shift = DEFAULT_SIFIVE_UART_REG_SHIFT; > @@ -432,32 +406,13 @@ int fdt_parse_uart8250_node(void *fdt, int nodeoffset, > { > int len, rc; > const fdt32_t *val; > - uint64_t reg_addr, reg_size; > - > - if (nodeoffset < 0 || !uart || !fdt) > - return SBI_ENODEV; > - > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > - ®_addr, ®_size); > - if (rc < 0 || !reg_addr || !reg_size) > - return SBI_ENODEV; > - uart->addr = reg_addr; > > - /** > - * UART address is mandaotry. clock-frequency and current-speed > - * may not be present. Don't return error. > - */ > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); > - if (len > 0 && val) > - uart->freq = fdt32_to_cpu(*val); > - else > - uart->freq = DEFAULT_UART_FREQ; > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > + DEFAULT_UART_FREQ, > + DEFAULT_UART_BAUD); > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > - if (len > 0 && val) > - uart->baud = fdt32_to_cpu(*val); > - else > - uart->baud = DEFAULT_UART_BAUD; > + if (rc) > + return rc; > > val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "reg-shift", &len); > if (len > 0 && val) > @@ -499,18 +454,10 @@ int fdt_parse_xlnx_uartlite_node(void *fdt, int nodeoffset, > struct platform_uart_data *uart) > { > int rc; > - uint64_t reg_addr, reg_size; > > - if (nodeoffset < 0 || !uart || !fdt) > - return SBI_ENODEV; > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, 0, 0); > > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > - ®_addr, ®_size); > - if (rc < 0 || !reg_addr || !reg_size) > - return SBI_ENODEV; > - uart->addr = reg_addr; > - > - return 0; > + return rc ? : 0; > } > > int fdt_parse_aplic_node(void *fdt, int nodeoff, struct aplic_data *aplic) > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/4] lib: utils/fdt: Factor out common uart node code 2022-07-30 5:24 ` Anup Patel @ 2022-07-30 10:19 ` Anup Patel 0 siblings, 0 replies; 22+ messages in thread From: Anup Patel @ 2022-07-30 10:19 UTC (permalink / raw) To: opensbi On Sat, Jul 30, 2022 at 10:54 AM Anup Patel <anup@brainfault.org> wrote: > > On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > Factor out the common code used by the fdt UART node parsers, > > allowing us to drop duplicate code. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > Looks good to me. > > Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > > Regards, > Anup > > > --- > > lib/utils/fdt/fdt_helper.c | 131 +++++++++++-------------------------- > > 1 file changed, 39 insertions(+), 92 deletions(-) > > > > diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c > > index 2bdb1ef23e55..c6abf804014d 100644 > > --- a/lib/utils/fdt/fdt_helper.c > > +++ b/lib/utils/fdt/fdt_helper.c > > @@ -314,8 +314,10 @@ int fdt_parse_timebase_frequency(void *fdt, unsigned long *freq) > > return 0; > > } > > > > -int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > > - struct platform_uart_data *uart) > > +static int fdt_parse_uart_node_common(void *fdt, int nodeoffset, > > + struct platform_uart_data *uart, > > + unsigned long default_freq, > > + unsigned long default_baud) > > { > > int len, rc; > > const fdt32_t *val; > > @@ -338,13 +340,28 @@ int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > > if (len > 0 && val) > > uart->freq = fdt32_to_cpu(*val); > > else > > - uart->freq = DEFAULT_UART_FREQ; > > + uart->freq = default_freq; > > > > val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > > if (len > 0 && val) > > uart->baud = fdt32_to_cpu(*val); > > else > > - uart->baud = DEFAULT_UART_BAUD; > > + uart->baud = default_baud; > > + > > + return 0; > > +} > > + > > +int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > > + struct platform_uart_data *uart) > > +{ > > + int rc; > > + > > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > > + DEFAULT_UART_FREQ, > > + DEFAULT_UART_BAUD); > > + > > + if (rc) > > + return rc; > > > > /* For Gaisler APBUART, the reg-shift and reg-io-width are fixed .*/ > > uart->reg_shift = DEFAULT_UART_REG_SHIFT; > > @@ -356,69 +373,26 @@ int fdt_parse_gaisler_uart_node(void *fdt, int nodeoffset, > > int fdt_parse_shakti_uart_node(void *fdt, int nodeoffset, > > struct platform_uart_data *uart) > > { > > - int len, rc; > > - const fdt32_t *val; > > - uint64_t reg_addr, reg_size; > > - > > - if (nodeoffset < 0 || !uart || !fdt) > > - return SBI_ENODEV; > > - > > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > > - ®_addr, ®_size); > > - if (rc < 0 || !reg_addr || !reg_size) > > - return SBI_ENODEV; > > - uart->addr = reg_addr; > > + int rc; > > > > - /** > > - * UART address is mandaotry. clock-frequency and current-speed > > - * may not be present. Don't return error. > > - */ > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); > > - if (len > 0 && val) > > - uart->freq = fdt32_to_cpu(*val); > > - else > > - uart->freq = DEFAULT_SHAKTI_UART_FREQ; > > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > > + DEFAULT_SHAKTI_UART_FREQ, > > + DEFAULT_SHAKTI_UART_BAUD); > > > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > > - if (len > 0 && val) > > - uart->baud = fdt32_to_cpu(*val); > > - else > > - uart->baud = DEFAULT_SHAKTI_UART_BAUD; > > - > > - return 0; > > + return rc ? : 0; > > } > > > > int fdt_parse_sifive_uart_node(void *fdt, int nodeoffset, > > struct platform_uart_data *uart) > > { > > - int len, rc; > > - const fdt32_t *val; > > - uint64_t reg_addr, reg_size; > > - > > - if (nodeoffset < 0 || !uart || !fdt) > > - return SBI_ENODEV; > > - > > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > > - ®_addr, ®_size); > > - if (rc < 0 || !reg_addr || !reg_size) > > - return SBI_ENODEV; > > - uart->addr = reg_addr; > > + int rc; > > > > - /** > > - * UART address is mandaotry. clock-frequency and current-speed > > - * may not be present. Don't return error. > > - */ > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); > > - if (len > 0 && val) > > - uart->freq = fdt32_to_cpu(*val); > > - else > > - uart->freq = DEFAULT_SIFIVE_UART_FREQ; > > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > > + DEFAULT_SIFIVE_UART_FREQ, > > + DEFAULT_SIFIVE_UART_BAUD); > > > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > > - if (len > 0 && val) > > - uart->baud = fdt32_to_cpu(*val); > > - else > > - uart->baud = DEFAULT_SIFIVE_UART_BAUD; > > + if (rc) > > + return rc; > > > > /* For SiFive UART, the reg-shift and reg-io-width are fixed .*/ > > uart->reg_shift = DEFAULT_SIFIVE_UART_REG_SHIFT; > > @@ -432,32 +406,13 @@ int fdt_parse_uart8250_node(void *fdt, int nodeoffset, > > { > > int len, rc; > > const fdt32_t *val; > > - uint64_t reg_addr, reg_size; > > - > > - if (nodeoffset < 0 || !uart || !fdt) > > - return SBI_ENODEV; > > - > > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > > - ®_addr, ®_size); > > - if (rc < 0 || !reg_addr || !reg_size) > > - return SBI_ENODEV; > > - uart->addr = reg_addr; > > > > - /** > > - * UART address is mandaotry. clock-frequency and current-speed > > - * may not be present. Don't return error. > > - */ > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "clock-frequency", &len); > > - if (len > 0 && val) > > - uart->freq = fdt32_to_cpu(*val); > > - else > > - uart->freq = DEFAULT_UART_FREQ; > > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, > > + DEFAULT_UART_FREQ, > > + DEFAULT_UART_BAUD); > > > > - val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "current-speed", &len); > > - if (len > 0 && val) > > - uart->baud = fdt32_to_cpu(*val); > > - else > > - uart->baud = DEFAULT_UART_BAUD; > > + if (rc) > > + return rc; > > > > val = (fdt32_t *)fdt_getprop(fdt, nodeoffset, "reg-shift", &len); > > if (len > 0 && val) > > @@ -499,18 +454,10 @@ int fdt_parse_xlnx_uartlite_node(void *fdt, int nodeoffset, > > struct platform_uart_data *uart) > > { > > int rc; > > - uint64_t reg_addr, reg_size; > > > > - if (nodeoffset < 0 || !uart || !fdt) > > - return SBI_ENODEV; > > + rc = fdt_parse_uart_node_common(fdt, nodeoffset, uart, 0, 0); > > > > - rc = fdt_get_node_addr_size(fdt, nodeoffset, 0, > > - ®_addr, ®_size); > > - if (rc < 0 || !reg_addr || !reg_size) > > - return SBI_ENODEV; > > - uart->addr = reg_addr; > > - > > - return 0; > > + return rc ? : 0; > > } > > > > int fdt_parse_aplic_node(void *fdt, int nodeoff, struct aplic_data *aplic) > > -- > > 2.36.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-18 17:20 [PATCH 0/4] lib: utils/serial: Collection of UART code improvements Andrew Jones 2022-07-18 17:20 ` [PATCH 1/4] lib: utils/fdt: Factor out common uart node code Andrew Jones @ 2022-07-18 17:20 ` Andrew Jones 2022-07-18 17:42 ` Jessica Clarke 2022-07-30 5:25 ` Anup Patel 2022-07-18 17:20 ` [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c Andrew Jones 2022-07-18 17:20 ` [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using Andrew Jones 3 siblings, 2 replies; 22+ messages in thread From: Andrew Jones @ 2022-07-18 17:20 UTC (permalink / raw) To: opensbi While it doesn't look like there are any current cases of using uninitialized data, let's zero all the UART data members to be safe. Zero may not actually be better than a random number in some cases, so all structure members should still be validated before use, but at least zero is usually easier to debug than some random stack garbage... Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- lib/utils/serial/fdt_serial_gaisler.c | 2 +- lib/utils/serial/fdt_serial_shakti.c | 2 +- lib/utils/serial/fdt_serial_sifive.c | 2 +- lib/utils/serial/fdt_serial_uart8250.c | 2 +- lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- platform/fpga/openpiton/platform.c | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c index 9a9f9a8b7962..74988e34bcd8 100644 --- a/lib/utils/serial/fdt_serial_gaisler.c +++ b/lib/utils/serial/fdt_serial_gaisler.c @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, const struct fdt_match *match) { int rc; - struct platform_uart_data uart; + struct platform_uart_data uart = { 0 }; rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); if (rc) diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c index 4f91419ef53c..0e056303dbb5 100644 --- a/lib/utils/serial/fdt_serial_shakti.c +++ b/lib/utils/serial/fdt_serial_shakti.c @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, const struct fdt_match *match) { int rc; - struct platform_uart_data uart; + struct platform_uart_data uart = { 0 }; rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); if (rc) diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c index f4c833c1573d..3ca10913eee3 100644 --- a/lib/utils/serial/fdt_serial_sifive.c +++ b/lib/utils/serial/fdt_serial_sifive.c @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, const struct fdt_match *match) { int rc; - struct platform_uart_data uart; + struct platform_uart_data uart = { 0 }; rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); if (rc) diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c index 544b7416de42..0b95f2d9e44e 100644 --- a/lib/utils/serial/fdt_serial_uart8250.c +++ b/lib/utils/serial/fdt_serial_uart8250.c @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, const struct fdt_match *match) { int rc; - struct platform_uart_data uart; + struct platform_uart_data uart = { 0 }; rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); if (rc) diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c index 466e16e82d8c..9f04aea3673b 100644 --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, const struct fdt_match *match) { int rc; - struct platform_uart_data uart; + struct platform_uart_data uart = { 0 }; rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); if (rc) diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c index 7ca21236bfef..5ff7d200aba9 100644 --- a/platform/fpga/openpiton/platform.c +++ b/platform/fpga/openpiton/platform.c @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { static int openpiton_early_init(bool cold_boot) { void *fdt; - struct platform_uart_data uart_data; + struct platform_uart_data uart_data = { 0 }; struct plic_data plic_data; unsigned long aclint_freq; uint64_t clint_addr; -- 2.36.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-18 17:20 ` [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero Andrew Jones @ 2022-07-18 17:42 ` Jessica Clarke 2022-07-18 18:35 ` Andrew Jones 2022-07-30 5:25 ` Anup Patel 1 sibling, 1 reply; 22+ messages in thread From: Jessica Clarke @ 2022-07-18 17:42 UTC (permalink / raw) To: opensbi On 18 Jul 2022, at 18:20, Andrew Jones <ajones@ventanamicro.com> wrote: > > While it doesn't look like there are any current cases of using > uninitialized data, let's zero all the UART data members to be > safe. Zero may not actually be better than a random number in > some cases, so all structure members should still be validated > before use, but at least zero is usually easier to debug than > some random stack garbage... If fdt_parse_foo_uart_node leaves the platform_uart_data uninitialised (at least, parts that matter) then that seems like a bug to me. This just masks such bugs from any kind of sanitiser... Jess > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > lib/utils/serial/fdt_serial_gaisler.c | 2 +- > lib/utils/serial/fdt_serial_shakti.c | 2 +- > lib/utils/serial/fdt_serial_sifive.c | 2 +- > lib/utils/serial/fdt_serial_uart8250.c | 2 +- > lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- > platform/fpga/openpiton/platform.c | 2 +- > 6 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c > index 9a9f9a8b7962..74988e34bcd8 100644 > --- a/lib/utils/serial/fdt_serial_gaisler.c > +++ b/lib/utils/serial/fdt_serial_gaisler.c > @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c > index 4f91419ef53c..0e056303dbb5 100644 > --- a/lib/utils/serial/fdt_serial_shakti.c > +++ b/lib/utils/serial/fdt_serial_shakti.c > @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c > index f4c833c1573d..3ca10913eee3 100644 > --- a/lib/utils/serial/fdt_serial_sifive.c > +++ b/lib/utils/serial/fdt_serial_sifive.c > @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c > index 544b7416de42..0b95f2d9e44e 100644 > --- a/lib/utils/serial/fdt_serial_uart8250.c > +++ b/lib/utils/serial/fdt_serial_uart8250.c > @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > index 466e16e82d8c..9f04aea3673b 100644 > --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c > +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c > index 7ca21236bfef..5ff7d200aba9 100644 > --- a/platform/fpga/openpiton/platform.c > +++ b/platform/fpga/openpiton/platform.c > @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { > static int openpiton_early_init(bool cold_boot) > { > void *fdt; > - struct platform_uart_data uart_data; > + struct platform_uart_data uart_data = { 0 }; > struct plic_data plic_data; > unsigned long aclint_freq; > uint64_t clint_addr; > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-18 17:42 ` Jessica Clarke @ 2022-07-18 18:35 ` Andrew Jones 2022-07-18 18:39 ` Jessica Clarke 0 siblings, 1 reply; 22+ messages in thread From: Andrew Jones @ 2022-07-18 18:35 UTC (permalink / raw) To: opensbi On Mon, Jul 18, 2022 at 06:42:40PM +0100, Jessica Clarke wrote: > On 18 Jul 2022, at 18:20, Andrew Jones <ajones@ventanamicro.com> wrote: > > > > While it doesn't look like there are any current cases of using > > uninitialized data, let's zero all the UART data members to be > > safe. Zero may not actually be better than a random number in > > some cases, so all structure members should still be validated > > before use, but at least zero is usually easier to debug than > > some random stack garbage... > > If fdt_parse_foo_uart_node leaves the platform_uart_data uninitialised > (at least, parts that matter) then that seems like a bug to me. This > just masks such bugs from any kind of sanitiser... Hi Jess, That's an interesting approach. Which tool(s) will point that out? I just tried compiling with both gcc[1] and clang[2], noting that we compile with -Wall, but neither complained when I purposely commented out a used UART field. [1] https://github.com/riscv/riscv-gnu-toolchain [2] clang-14.0.0-1.fc36.x86_64 Thanks, drew > > Jess > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > lib/utils/serial/fdt_serial_gaisler.c | 2 +- > > lib/utils/serial/fdt_serial_shakti.c | 2 +- > > lib/utils/serial/fdt_serial_sifive.c | 2 +- > > lib/utils/serial/fdt_serial_uart8250.c | 2 +- > > lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- > > platform/fpga/openpiton/platform.c | 2 +- > > 6 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c > > index 9a9f9a8b7962..74988e34bcd8 100644 > > --- a/lib/utils/serial/fdt_serial_gaisler.c > > +++ b/lib/utils/serial/fdt_serial_gaisler.c > > @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c > > index 4f91419ef53c..0e056303dbb5 100644 > > --- a/lib/utils/serial/fdt_serial_shakti.c > > +++ b/lib/utils/serial/fdt_serial_shakti.c > > @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c > > index f4c833c1573d..3ca10913eee3 100644 > > --- a/lib/utils/serial/fdt_serial_sifive.c > > +++ b/lib/utils/serial/fdt_serial_sifive.c > > @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c > > index 544b7416de42..0b95f2d9e44e 100644 > > --- a/lib/utils/serial/fdt_serial_uart8250.c > > +++ b/lib/utils/serial/fdt_serial_uart8250.c > > @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > > index 466e16e82d8c..9f04aea3673b 100644 > > --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c > > +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > > @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c > > index 7ca21236bfef..5ff7d200aba9 100644 > > --- a/platform/fpga/openpiton/platform.c > > +++ b/platform/fpga/openpiton/platform.c > > @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { > > static int openpiton_early_init(bool cold_boot) > > { > > void *fdt; > > - struct platform_uart_data uart_data; > > + struct platform_uart_data uart_data = { 0 }; > > struct plic_data plic_data; > > unsigned long aclint_freq; > > uint64_t clint_addr; > > -- > > 2.36.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-18 18:35 ` Andrew Jones @ 2022-07-18 18:39 ` Jessica Clarke 2022-07-19 10:30 ` Andrew Jones 0 siblings, 1 reply; 22+ messages in thread From: Jessica Clarke @ 2022-07-18 18:39 UTC (permalink / raw) To: opensbi On 18 Jul 2022, at 19:35, Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Jul 18, 2022 at 06:42:40PM +0100, Jessica Clarke wrote: >> On 18 Jul 2022, at 18:20, Andrew Jones <ajones@ventanamicro.com> wrote: >>> >>> While it doesn't look like there are any current cases of using >>> uninitialized data, let's zero all the UART data members to be >>> safe. Zero may not actually be better than a random number in >>> some cases, so all structure members should still be validated >>> before use, but at least zero is usually easier to debug than >>> some random stack garbage... >> >> If fdt_parse_foo_uart_node leaves the platform_uart_data uninitialised >> (at least, parts that matter) then that seems like a bug to me. This >> just masks such bugs from any kind of sanitiser... > > Hi Jess, > > That's an interesting approach. Which tool(s) will point that out? > I just tried compiling with both gcc[1] and clang[2], noting that we > compile with -Wall, but neither complained when I purposely commented > out a used UART field. > > [1] https://github.com/riscv/riscv-gnu-toolchain > [2] clang-14.0.0-1.fc36.x86_64 Like other sanitisers it needs some runtime support, but MSan, i.e. -fsanitize=memory. Jess > Thanks, > drew > >> >> Jess >> >>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com> >>> --- >>> lib/utils/serial/fdt_serial_gaisler.c | 2 +- >>> lib/utils/serial/fdt_serial_shakti.c | 2 +- >>> lib/utils/serial/fdt_serial_sifive.c | 2 +- >>> lib/utils/serial/fdt_serial_uart8250.c | 2 +- >>> lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- >>> platform/fpga/openpiton/platform.c | 2 +- >>> 6 files changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c >>> index 9a9f9a8b7962..74988e34bcd8 100644 >>> --- a/lib/utils/serial/fdt_serial_gaisler.c >>> +++ b/lib/utils/serial/fdt_serial_gaisler.c >>> @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, >>> const struct fdt_match *match) >>> { >>> int rc; >>> - struct platform_uart_data uart; >>> + struct platform_uart_data uart = { 0 }; >>> >>> rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); >>> if (rc) >>> diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c >>> index 4f91419ef53c..0e056303dbb5 100644 >>> --- a/lib/utils/serial/fdt_serial_shakti.c >>> +++ b/lib/utils/serial/fdt_serial_shakti.c >>> @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, >>> const struct fdt_match *match) >>> { >>> int rc; >>> - struct platform_uart_data uart; >>> + struct platform_uart_data uart = { 0 }; >>> >>> rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); >>> if (rc) >>> diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c >>> index f4c833c1573d..3ca10913eee3 100644 >>> --- a/lib/utils/serial/fdt_serial_sifive.c >>> +++ b/lib/utils/serial/fdt_serial_sifive.c >>> @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, >>> const struct fdt_match *match) >>> { >>> int rc; >>> - struct platform_uart_data uart; >>> + struct platform_uart_data uart = { 0 }; >>> >>> rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); >>> if (rc) >>> diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c >>> index 544b7416de42..0b95f2d9e44e 100644 >>> --- a/lib/utils/serial/fdt_serial_uart8250.c >>> +++ b/lib/utils/serial/fdt_serial_uart8250.c >>> @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, >>> const struct fdt_match *match) >>> { >>> int rc; >>> - struct platform_uart_data uart; >>> + struct platform_uart_data uart = { 0 }; >>> >>> rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); >>> if (rc) >>> diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c >>> index 466e16e82d8c..9f04aea3673b 100644 >>> --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c >>> +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c >>> @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, >>> const struct fdt_match *match) >>> { >>> int rc; >>> - struct platform_uart_data uart; >>> + struct platform_uart_data uart = { 0 }; >>> >>> rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); >>> if (rc) >>> diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c >>> index 7ca21236bfef..5ff7d200aba9 100644 >>> --- a/platform/fpga/openpiton/platform.c >>> +++ b/platform/fpga/openpiton/platform.c >>> @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { >>> static int openpiton_early_init(bool cold_boot) >>> { >>> void *fdt; >>> - struct platform_uart_data uart_data; >>> + struct platform_uart_data uart_data = { 0 }; >>> struct plic_data plic_data; >>> unsigned long aclint_freq; >>> uint64_t clint_addr; >>> -- >>> 2.36.1 >>> >>> >>> -- >>> opensbi mailing list >>> opensbi at lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/opensbi >> ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-18 18:39 ` Jessica Clarke @ 2022-07-19 10:30 ` Andrew Jones 0 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2022-07-19 10:30 UTC (permalink / raw) To: opensbi On Mon, Jul 18, 2022 at 07:39:23PM +0100, Jessica Clarke wrote: > On 18 Jul 2022, at 19:35, Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Mon, Jul 18, 2022 at 06:42:40PM +0100, Jessica Clarke wrote: > >> On 18 Jul 2022, at 18:20, Andrew Jones <ajones@ventanamicro.com> wrote: > >>> > >>> While it doesn't look like there are any current cases of using > >>> uninitialized data, let's zero all the UART data members to be > >>> safe. Zero may not actually be better than a random number in > >>> some cases, so all structure members should still be validated > >>> before use, but at least zero is usually easier to debug than > >>> some random stack garbage... > >> > >> If fdt_parse_foo_uart_node leaves the platform_uart_data uninitialised > >> (at least, parts that matter) then that seems like a bug to me. This > >> just masks such bugs from any kind of sanitiser... > > > > Hi Jess, > > > > That's an interesting approach. Which tool(s) will point that out? > > I just tried compiling with both gcc[1] and clang[2], noting that we > > compile with -Wall, but neither complained when I purposely commented > > out a used UART field. > > > > [1] https://github.com/riscv/riscv-gnu-toolchain > > [2] clang-14.0.0-1.fc36.x86_64 > > Like other sanitisers it needs some runtime support, but MSan, i.e. > -fsanitize=memory. Thanks, Jess. I'm a complete sanitizer noob and appreciate the pointer, which has inspired me to do a bit of reading. I really like the idea of getting comprehensive, automated tests in place allowing us to configure debug builds with several sanitizers, but I see a few challenges 1) We need to link to the sanitizer runtime libraries. This may not be so easy for opensbi to do, depending on what the runtime libraries depend on. The libraries will likely expect fprintf to work, at a minimum, for example. 2) We need a way to get sanitizer output. Once we have UART(s) initialized we can use them (sanitizers appear to typically output to stderr), but until then we need to write the logs to memory where they can be dumped. (Maybe opensbi can provide an early, debug console device that does this.) 3) Not all sanitizers are supported by all architectures. Indeed MSan is not yet supported for RISCV (maybe it will be soon [1]) 4) We need automated tests giving us near full coverage in order for the sanitizers to do their thing. (Those would be good to have even without sanitizers, but they're not yet there.) With those challenges in mind, I don't believe we should assume we'll be able to run a complete set of sanitizers on opensbi very soon. We should probably consider other options for ensuring robustness and debugability of the code. Regarding this patch, I'm fine dropping it if the consensus is that it's not better to zero memory which may be used uninitialized. And, if that's the case, maybe we should document that policy somewhere. [1] https://reviews.llvm.org/D97897 Thanks, drew > > Jess > > > Thanks, > > drew > > > >> > >> Jess > >> > >>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > >>> --- > >>> lib/utils/serial/fdt_serial_gaisler.c | 2 +- > >>> lib/utils/serial/fdt_serial_shakti.c | 2 +- > >>> lib/utils/serial/fdt_serial_sifive.c | 2 +- > >>> lib/utils/serial/fdt_serial_uart8250.c | 2 +- > >>> lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- > >>> platform/fpga/openpiton/platform.c | 2 +- > >>> 6 files changed, 6 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c > >>> index 9a9f9a8b7962..74988e34bcd8 100644 > >>> --- a/lib/utils/serial/fdt_serial_gaisler.c > >>> +++ b/lib/utils/serial/fdt_serial_gaisler.c > >>> @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, > >>> const struct fdt_match *match) > >>> { > >>> int rc; > >>> - struct platform_uart_data uart; > >>> + struct platform_uart_data uart = { 0 }; > >>> > >>> rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); > >>> if (rc) > >>> diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c > >>> index 4f91419ef53c..0e056303dbb5 100644 > >>> --- a/lib/utils/serial/fdt_serial_shakti.c > >>> +++ b/lib/utils/serial/fdt_serial_shakti.c > >>> @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, > >>> const struct fdt_match *match) > >>> { > >>> int rc; > >>> - struct platform_uart_data uart; > >>> + struct platform_uart_data uart = { 0 }; > >>> > >>> rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); > >>> if (rc) > >>> diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c > >>> index f4c833c1573d..3ca10913eee3 100644 > >>> --- a/lib/utils/serial/fdt_serial_sifive.c > >>> +++ b/lib/utils/serial/fdt_serial_sifive.c > >>> @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, > >>> const struct fdt_match *match) > >>> { > >>> int rc; > >>> - struct platform_uart_data uart; > >>> + struct platform_uart_data uart = { 0 }; > >>> > >>> rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); > >>> if (rc) > >>> diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c > >>> index 544b7416de42..0b95f2d9e44e 100644 > >>> --- a/lib/utils/serial/fdt_serial_uart8250.c > >>> +++ b/lib/utils/serial/fdt_serial_uart8250.c > >>> @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, > >>> const struct fdt_match *match) > >>> { > >>> int rc; > >>> - struct platform_uart_data uart; > >>> + struct platform_uart_data uart = { 0 }; > >>> > >>> rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); > >>> if (rc) > >>> diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > >>> index 466e16e82d8c..9f04aea3673b 100644 > >>> --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c > >>> +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > >>> @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, > >>> const struct fdt_match *match) > >>> { > >>> int rc; > >>> - struct platform_uart_data uart; > >>> + struct platform_uart_data uart = { 0 }; > >>> > >>> rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); > >>> if (rc) > >>> diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c > >>> index 7ca21236bfef..5ff7d200aba9 100644 > >>> --- a/platform/fpga/openpiton/platform.c > >>> +++ b/platform/fpga/openpiton/platform.c > >>> @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { > >>> static int openpiton_early_init(bool cold_boot) > >>> { > >>> void *fdt; > >>> - struct platform_uart_data uart_data; > >>> + struct platform_uart_data uart_data = { 0 }; > >>> struct plic_data plic_data; > >>> unsigned long aclint_freq; > >>> uint64_t clint_addr; > >>> -- > >>> 2.36.1 > >>> > >>> > >>> -- > >>> opensbi mailing list > >>> opensbi at lists.infradead.org > >>> http://lists.infradead.org/mailman/listinfo/opensbi > >> > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-18 17:20 ` [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero Andrew Jones 2022-07-18 17:42 ` Jessica Clarke @ 2022-07-30 5:25 ` Anup Patel 2022-07-30 10:20 ` Anup Patel 1 sibling, 1 reply; 22+ messages in thread From: Anup Patel @ 2022-07-30 5:25 UTC (permalink / raw) To: opensbi On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > While it doesn't look like there are any current cases of using > uninitialized data, let's zero all the UART data members to be > safe. Zero may not actually be better than a random number in > some cases, so all structure members should still be validated > before use, but at least zero is usually easier to debug than > some random stack garbage... > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > lib/utils/serial/fdt_serial_gaisler.c | 2 +- > lib/utils/serial/fdt_serial_shakti.c | 2 +- > lib/utils/serial/fdt_serial_sifive.c | 2 +- > lib/utils/serial/fdt_serial_uart8250.c | 2 +- > lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- > platform/fpga/openpiton/platform.c | 2 +- > 6 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c > index 9a9f9a8b7962..74988e34bcd8 100644 > --- a/lib/utils/serial/fdt_serial_gaisler.c > +++ b/lib/utils/serial/fdt_serial_gaisler.c > @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c > index 4f91419ef53c..0e056303dbb5 100644 > --- a/lib/utils/serial/fdt_serial_shakti.c > +++ b/lib/utils/serial/fdt_serial_shakti.c > @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c > index f4c833c1573d..3ca10913eee3 100644 > --- a/lib/utils/serial/fdt_serial_sifive.c > +++ b/lib/utils/serial/fdt_serial_sifive.c > @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c > index 544b7416de42..0b95f2d9e44e 100644 > --- a/lib/utils/serial/fdt_serial_uart8250.c > +++ b/lib/utils/serial/fdt_serial_uart8250.c > @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > index 466e16e82d8c..9f04aea3673b 100644 > --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c > +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, > const struct fdt_match *match) > { > int rc; > - struct platform_uart_data uart; > + struct platform_uart_data uart = { 0 }; > > rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); > if (rc) > diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c > index 7ca21236bfef..5ff7d200aba9 100644 > --- a/platform/fpga/openpiton/platform.c > +++ b/platform/fpga/openpiton/platform.c > @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { > static int openpiton_early_init(bool cold_boot) > { > void *fdt; > - struct platform_uart_data uart_data; > + struct platform_uart_data uart_data = { 0 }; > struct plic_data plic_data; > unsigned long aclint_freq; > uint64_t clint_addr; > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero 2022-07-30 5:25 ` Anup Patel @ 2022-07-30 10:20 ` Anup Patel 0 siblings, 0 replies; 22+ messages in thread From: Anup Patel @ 2022-07-30 10:20 UTC (permalink / raw) To: opensbi On Sat, Jul 30, 2022 at 10:55 AM Anup Patel <anup@brainfault.org> wrote: > > On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > While it doesn't look like there are any current cases of using > > uninitialized data, let's zero all the UART data members to be > > safe. Zero may not actually be better than a random number in > > some cases, so all structure members should still be validated > > before use, but at least zero is usually easier to debug than > > some random stack garbage... > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > Looks good to me. > > Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > > Regards, > Anup > > > --- > > lib/utils/serial/fdt_serial_gaisler.c | 2 +- > > lib/utils/serial/fdt_serial_shakti.c | 2 +- > > lib/utils/serial/fdt_serial_sifive.c | 2 +- > > lib/utils/serial/fdt_serial_uart8250.c | 2 +- > > lib/utils/serial/fdt_serial_xlnx_uartlite.c | 2 +- > > platform/fpga/openpiton/platform.c | 2 +- > > 6 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/lib/utils/serial/fdt_serial_gaisler.c b/lib/utils/serial/fdt_serial_gaisler.c > > index 9a9f9a8b7962..74988e34bcd8 100644 > > --- a/lib/utils/serial/fdt_serial_gaisler.c > > +++ b/lib/utils/serial/fdt_serial_gaisler.c > > @@ -15,7 +15,7 @@ static int serial_gaisler_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_gaisler_uart_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_shakti.c b/lib/utils/serial/fdt_serial_shakti.c > > index 4f91419ef53c..0e056303dbb5 100644 > > --- a/lib/utils/serial/fdt_serial_shakti.c > > +++ b/lib/utils/serial/fdt_serial_shakti.c > > @@ -13,7 +13,7 @@ static int serial_shakti_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_shakti_uart_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_sifive.c b/lib/utils/serial/fdt_serial_sifive.c > > index f4c833c1573d..3ca10913eee3 100644 > > --- a/lib/utils/serial/fdt_serial_sifive.c > > +++ b/lib/utils/serial/fdt_serial_sifive.c > > @@ -15,7 +15,7 @@ static int serial_sifive_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_sifive_uart_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_uart8250.c b/lib/utils/serial/fdt_serial_uart8250.c > > index 544b7416de42..0b95f2d9e44e 100644 > > --- a/lib/utils/serial/fdt_serial_uart8250.c > > +++ b/lib/utils/serial/fdt_serial_uart8250.c > > @@ -15,7 +15,7 @@ static int serial_uart8250_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_uart8250_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/lib/utils/serial/fdt_serial_xlnx_uartlite.c b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > > index 466e16e82d8c..9f04aea3673b 100644 > > --- a/lib/utils/serial/fdt_serial_xlnx_uartlite.c > > +++ b/lib/utils/serial/fdt_serial_xlnx_uartlite.c > > @@ -15,7 +15,7 @@ static int serial_xlnx_uartlite_init(void *fdt, int nodeoff, > > const struct fdt_match *match) > > { > > int rc; > > - struct platform_uart_data uart; > > + struct platform_uart_data uart = { 0 }; > > > > rc = fdt_parse_xlnx_uartlite_node(fdt, nodeoff, &uart); > > if (rc) > > diff --git a/platform/fpga/openpiton/platform.c b/platform/fpga/openpiton/platform.c > > index 7ca21236bfef..5ff7d200aba9 100644 > > --- a/platform/fpga/openpiton/platform.c > > +++ b/platform/fpga/openpiton/platform.c > > @@ -69,7 +69,7 @@ static struct aclint_mtimer_data mtimer = { > > static int openpiton_early_init(bool cold_boot) > > { > > void *fdt; > > - struct platform_uart_data uart_data; > > + struct platform_uart_data uart_data = { 0 }; > > struct plic_data plic_data; > > unsigned long aclint_freq; > > uint64_t clint_addr; > > -- > > 2.36.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c 2022-07-18 17:20 [PATCH 0/4] lib: utils/serial: Collection of UART code improvements Andrew Jones 2022-07-18 17:20 ` [PATCH 1/4] lib: utils/fdt: Factor out common uart node code Andrew Jones 2022-07-18 17:20 ` [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero Andrew Jones @ 2022-07-18 17:20 ` Andrew Jones 2022-07-20 15:13 ` Xiang W 2022-07-30 5:27 ` Anup Patel 2022-07-18 17:20 ` [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using Andrew Jones 3 siblings, 2 replies; 22+ messages in thread From: Andrew Jones @ 2022-07-18 17:20 UTC (permalink / raw) To: opensbi Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- lib/utils/serial/sifive-uart.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c index 9478a77f8054..7078611a5274 100644 --- a/lib/utils/serial/sifive-uart.c +++ b/lib/utils/serial/sifive-uart.c @@ -48,12 +48,12 @@ static inline unsigned int uart_min_clk_divisor(uint64_t in_freq, uint64_t max_target_hz) { uint64_t quotient = (in_freq + max_target_hz - 1) / (max_target_hz); + /* Avoid underflow */ - if (quotient == 0) { + if (quotient == 0) return 0; - } else { + else return quotient - 1; - } } static u32 get_reg(u32 num) @@ -77,8 +77,10 @@ static void sifive_uart_putc(char ch) static int sifive_uart_getc(void) { u32 ret = get_reg(UART_REG_RXFIFO); + if (!(ret & UART_RXFIFO_EMPTY)) return ret & UART_RXFIFO_DATA; + return -1; } @@ -97,10 +99,13 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) /* Configure baudrate */ if (in_freq) set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); + /* Disable interrupts */ set_reg(UART_REG_IE, 0); + /* Enable TX */ set_reg(UART_REG_TXCTRL, UART_TXCTRL_TXEN); + /* Enable Rx */ set_reg(UART_REG_RXCTRL, UART_RXCTRL_RXEN); -- 2.36.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c 2022-07-18 17:20 ` [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c Andrew Jones @ 2022-07-20 15:13 ` Xiang W 2022-07-30 5:27 ` Anup Patel 1 sibling, 0 replies; 22+ messages in thread From: Xiang W @ 2022-07-20 15:13 UTC (permalink / raw) To: opensbi ? 2022-07-18???? 19:20 +0200?Andrew Jones??? > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> Look good to me. Reviewed-by: Xiang W <wxjstz@126.com> > --- > ?lib/utils/serial/sifive-uart.c | 11 ++++++++--- > ?1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > index 9478a77f8054..7078611a5274 100644 > --- a/lib/utils/serial/sifive-uart.c > +++ b/lib/utils/serial/sifive-uart.c > @@ -48,12 +48,12 @@ static inline unsigned int uart_min_clk_divisor(uint64_t in_freq, > ????????????????????????????????????????????????uint64_t max_target_hz) > ?{ > ????????uint64_t quotient = (in_freq + max_target_hz - 1) / (max_target_hz); > + > ????????/* Avoid underflow */ > -???????if (quotient == 0) { > +???????if (quotient == 0) > ????????????????return 0; > -???????} else { > +???????else > ????????????????return quotient - 1; > -???????} > ?} > ? > ?static u32 get_reg(u32 num) > @@ -77,8 +77,10 @@ static void sifive_uart_putc(char ch) > ?static int sifive_uart_getc(void) > ?{ > ????????u32 ret = get_reg(UART_REG_RXFIFO); > + > ????????if (!(ret & UART_RXFIFO_EMPTY)) > ????????????????return ret & UART_RXFIFO_DATA; > + > ????????return -1; > ?} > ? > @@ -97,10 +99,13 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > ????????/* Configure baudrate */ > ????????if (in_freq) > ????????????????set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > + > ????????/* Disable interrupts */ > ????????set_reg(UART_REG_IE, 0); > + > ????????/* Enable TX */ > ????????set_reg(UART_REG_TXCTRL, UART_TXCTRL_TXEN); > + > ????????/* Enable Rx */ > ????????set_reg(UART_REG_RXCTRL, UART_RXCTRL_RXEN); > ? > -- > 2.36.1 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c 2022-07-18 17:20 ` [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c Andrew Jones 2022-07-20 15:13 ` Xiang W @ 2022-07-30 5:27 ` Anup Patel 2022-07-30 10:20 ` Anup Patel 1 sibling, 1 reply; 22+ messages in thread From: Anup Patel @ 2022-07-30 5:27 UTC (permalink / raw) To: opensbi On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > lib/utils/serial/sifive-uart.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > index 9478a77f8054..7078611a5274 100644 > --- a/lib/utils/serial/sifive-uart.c > +++ b/lib/utils/serial/sifive-uart.c > @@ -48,12 +48,12 @@ static inline unsigned int uart_min_clk_divisor(uint64_t in_freq, > uint64_t max_target_hz) > { > uint64_t quotient = (in_freq + max_target_hz - 1) / (max_target_hz); > + > /* Avoid underflow */ > - if (quotient == 0) { > + if (quotient == 0) > return 0; > - } else { > + else > return quotient - 1; > - } > } > > static u32 get_reg(u32 num) > @@ -77,8 +77,10 @@ static void sifive_uart_putc(char ch) > static int sifive_uart_getc(void) > { > u32 ret = get_reg(UART_REG_RXFIFO); > + > if (!(ret & UART_RXFIFO_EMPTY)) > return ret & UART_RXFIFO_DATA; > + > return -1; > } > > @@ -97,10 +99,13 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > /* Configure baudrate */ > if (in_freq) > set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > + > /* Disable interrupts */ > set_reg(UART_REG_IE, 0); > + > /* Enable TX */ > set_reg(UART_REG_TXCTRL, UART_TXCTRL_TXEN); > + > /* Enable Rx */ > set_reg(UART_REG_RXCTRL, UART_RXCTRL_RXEN); > > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c 2022-07-30 5:27 ` Anup Patel @ 2022-07-30 10:20 ` Anup Patel 0 siblings, 0 replies; 22+ messages in thread From: Anup Patel @ 2022-07-30 10:20 UTC (permalink / raw) To: opensbi On Sat, Jul 30, 2022 at 10:57 AM Anup Patel <anup@brainfault.org> wrote: > > On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > Looks good to me. > > Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > > Regards, > Anup > > > --- > > lib/utils/serial/sifive-uart.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > > index 9478a77f8054..7078611a5274 100644 > > --- a/lib/utils/serial/sifive-uart.c > > +++ b/lib/utils/serial/sifive-uart.c > > @@ -48,12 +48,12 @@ static inline unsigned int uart_min_clk_divisor(uint64_t in_freq, > > uint64_t max_target_hz) > > { > > uint64_t quotient = (in_freq + max_target_hz - 1) / (max_target_hz); > > + > > /* Avoid underflow */ > > - if (quotient == 0) { > > + if (quotient == 0) > > return 0; > > - } else { > > + else > > return quotient - 1; > > - } > > } > > > > static u32 get_reg(u32 num) > > @@ -77,8 +77,10 @@ static void sifive_uart_putc(char ch) > > static int sifive_uart_getc(void) > > { > > u32 ret = get_reg(UART_REG_RXFIFO); > > + > > if (!(ret & UART_RXFIFO_EMPTY)) > > return ret & UART_RXFIFO_DATA; > > + > > return -1; > > } > > > > @@ -97,10 +99,13 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > /* Configure baudrate */ > > if (in_freq) > > set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > > + > > /* Disable interrupts */ > > set_reg(UART_REG_IE, 0); > > + > > /* Enable TX */ > > set_reg(UART_REG_TXCTRL, UART_TXCTRL_TXEN); > > + > > /* Enable Rx */ > > set_reg(UART_REG_RXCTRL, UART_RXCTRL_RXEN); > > > > -- > > 2.36.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-18 17:20 [PATCH 0/4] lib: utils/serial: Collection of UART code improvements Andrew Jones ` (2 preceding siblings ...) 2022-07-18 17:20 ` [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c Andrew Jones @ 2022-07-18 17:20 ` Andrew Jones 2022-07-20 15:12 ` Xiang W 2022-07-30 5:31 ` Anup Patel 3 siblings, 2 replies; 22+ messages in thread From: Andrew Jones @ 2022-07-18 17:20 UTC (permalink / raw) To: opensbi RISC-V doesn't generate exceptions on divide-by-zero, but the result, all bits set, is not likely what people expect either. In all cases where we divide by baudrate there's a chance it's zero (when the DT it came from is "bad"). To avoid difficult to debug situations, leave baudrate dependent registers alone when baudrate is zero, as, also in all cases, it appears we can skip initialization of those registers and still [hopefully] have a functioning UART. Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- lib/utils/serial/gaisler-uart.c | 2 +- lib/utils/serial/shakti-uart.c | 8 ++++++-- lib/utils/serial/sifive-uart.c | 2 +- lib/utils/serial/uart8250.c | 7 +++++-- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c index 5f30ee43b719..eec75cca0c98 100644 --- a/lib/utils/serial/gaisler-uart.c +++ b/lib/utils/serial/gaisler-uart.c @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) uart_base = (volatile char *)base; /* Configure baudrate */ - if (in_freq) + if (in_freq && baudrate) set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); ctrl = get_reg(UART_REG_CTRL); diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c index 5f2fe75c9f33..2f09f141b9cf 100644 --- a/lib/utils/serial/shakti-uart.c +++ b/lib/utils/serial/shakti-uart.c @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) { uart_base = (volatile char *)base; - u16 baud = (u16)(in_freq/(16 * baudrate)); - writew(baud, uart_base + REG_BAUD); + u16 baud; + + if (baudrate) { + baud = (u16)(in_freq / (16 * baudrate)); + writew(baud, uart_base + REG_BAUD); + } sbi_console_set_device(&shakti_console); diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c index 7078611a5274..3581d47a6d4b 100644 --- a/lib/utils/serial/sifive-uart.c +++ b/lib/utils/serial/sifive-uart.c @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) uart_baudrate = baudrate; /* Configure baudrate */ - if (in_freq) + if (in_freq && baudrate) set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); /* Disable interrupts */ diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c index 38ea11af4d31..99bf1bf733f2 100644 --- a/lib/utils/serial/uart8250.c +++ b/lib/utils/serial/uart8250.c @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, u32 reg_width, u32 reg_offset) { - u16 bdiv; + u16 bdiv = 0; uart8250_base = (volatile char *)base + reg_offset; uart8250_reg_shift = reg_shift; @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, uart8250_in_freq = in_freq; uart8250_baudrate = baudrate; - bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); + if (uart8250_baudrate) { + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / + (16 * uart8250_baudrate); + } /* Disable all interrupts */ set_reg(UART_IER_OFFSET, 0x00); -- 2.36.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-18 17:20 ` [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using Andrew Jones @ 2022-07-20 15:12 ` Xiang W 2022-07-20 17:16 ` Andrew Jones 2022-07-30 5:31 ` Anup Patel 1 sibling, 1 reply; 22+ messages in thread From: Xiang W @ 2022-07-20 15:12 UTC (permalink / raw) To: opensbi ? 2022-07-18???? 19:20 +0200?Andrew Jones??? > RISC-V doesn't generate exceptions on divide-by-zero, but the result, > all bits set, is not likely what people expect either. In all cases > where we divide by baudrate there's a chance it's zero (when the DT > it came from is "bad"). To avoid difficult to debug situations, leave > baudrate dependent registers alone when baudrate is zero, as, also in > all cases, it appears we can skip initialization of those registers > and still [hopefully] have a functioning UART. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> If the parameter is wrong we should skip initialization and should not execute sbi_console_set_device. Regards, Xiang W > --- > ?lib/utils/serial/gaisler-uart.c | 2 +- > ?lib/utils/serial/shakti-uart.c? | 8 ++++++-- > ?lib/utils/serial/sifive-uart.c? | 2 +- > ?lib/utils/serial/uart8250.c???? | 7 +++++-- > ?4 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > index 5f30ee43b719..eec75cca0c98 100644 > --- a/lib/utils/serial/gaisler-uart.c > +++ b/lib/utils/serial/gaisler-uart.c > @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > ????????uart_base = (volatile char *)base; > ? > ????????/* Configure baudrate */ > -???????if (in_freq) > +???????if (in_freq && baudrate) > ????????????????set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); > ? > ????????ctrl = get_reg(UART_REG_CTRL); > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > index 5f2fe75c9f33..2f09f141b9cf 100644 > --- a/lib/utils/serial/shakti-uart.c > +++ b/lib/utils/serial/shakti-uart.c > @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { > ?int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > ?{ > ????????uart_base = (volatile char *)base; > -???????u16 baud = (u16)(in_freq/(16 * baudrate)); > -???????writew(baud, uart_base + REG_BAUD); > +???????u16 baud; > + > +???????if (baudrate) { > +???????????????baud = (u16)(in_freq / (16 * baudrate)); > +???????????????writew(baud, uart_base + REG_BAUD); > +???????} > ? > ????????sbi_console_set_device(&shakti_console); > ? > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > index 7078611a5274..3581d47a6d4b 100644 > --- a/lib/utils/serial/sifive-uart.c > +++ b/lib/utils/serial/sifive-uart.c > @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > ????????uart_baudrate = baudrate; > ? > ????????/* Configure baudrate */ > -???????if (in_freq) > +???????if (in_freq && baudrate) > ????????????????set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > ? > ????????/* Disable interrupts */ > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > index 38ea11af4d31..99bf1bf733f2 100644 > --- a/lib/utils/serial/uart8250.c > +++ b/lib/utils/serial/uart8250.c > @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { > ?int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > ????????????????? u32 reg_width, u32 reg_offset) > ?{ > -???????u16 bdiv; > +???????u16 bdiv = 0; > ? > ????????uart8250_base????? = (volatile char *)base + reg_offset; > ????????uart8250_reg_shift = reg_shift; > @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > ????????uart8250_in_freq?? = in_freq; > ????????uart8250_baudrate? = baudrate; > ? > -???????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > +???????if (uart8250_baudrate) { > +???????????????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / > +????????????????????? (16 * uart8250_baudrate); > +???????} > ? > ????????/* Disable all interrupts */ > ????????set_reg(UART_IER_OFFSET, 0x00); > -- > 2.36.1 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-20 15:12 ` Xiang W @ 2022-07-20 17:16 ` Andrew Jones 2022-07-20 17:25 ` Xiang W 0 siblings, 1 reply; 22+ messages in thread From: Andrew Jones @ 2022-07-20 17:16 UTC (permalink / raw) To: opensbi On Wed, Jul 20, 2022 at 11:12:18PM +0800, Xiang W wrote: > ? 2022-07-18???? 19:20 +0200?Andrew Jones??? > > RISC-V doesn't generate exceptions on divide-by-zero, but the result, > > all bits set, is not likely what people expect either. In all cases > > where we divide by baudrate there's a chance it's zero (when the DT > > it came from is "bad"). To avoid difficult to debug situations, leave > > baudrate dependent registers alone when baudrate is zero, as, also in > > all cases, it appears we can skip initialization of those registers > > and still [hopefully] have a functioning UART. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > If the parameter is wrong we should skip initialization and should not > execute sbi_console_set_device. Hi Xiang, While there should be some DT validation somewhere that points out bad parameters, I don't think opensbi is the best place to do it, especially for UART configuration. While we certainly can't progress in all cases we detect bad input, in the case of a zero baudrate I believe we can, as it appears setting baudrate registers is typically optional for UARTs. We could also consider adding a log buffer which early init can write errors and warnings to. After the console has been initialized (with our best effort) we can then output all the errors and warnings from the log buffer. If we had something like that, then we could add a warning about the baudrate to that log along with warnings/errors for any other DT parameters we fail to validate but attempt to work around. Thanks, drew > > Regards, > Xiang W > > --- > > ?lib/utils/serial/gaisler-uart.c | 2 +- > > ?lib/utils/serial/shakti-uart.c? | 8 ++++++-- > > ?lib/utils/serial/sifive-uart.c? | 2 +- > > ?lib/utils/serial/uart8250.c???? | 7 +++++-- > > ?4 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > > index 5f30ee43b719..eec75cca0c98 100644 > > --- a/lib/utils/serial/gaisler-uart.c > > +++ b/lib/utils/serial/gaisler-uart.c > > @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > ????????uart_base = (volatile char *)base; > > ? > > ????????/* Configure baudrate */ > > -???????if (in_freq) > > +???????if (in_freq && baudrate) > > ????????????????set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); > > ? > > ????????ctrl = get_reg(UART_REG_CTRL); > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > > index 5f2fe75c9f33..2f09f141b9cf 100644 > > --- a/lib/utils/serial/shakti-uart.c > > +++ b/lib/utils/serial/shakti-uart.c > > @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { > > ?int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > ?{ > > ????????uart_base = (volatile char *)base; > > -???????u16 baud = (u16)(in_freq/(16 * baudrate)); > > -???????writew(baud, uart_base + REG_BAUD); > > +???????u16 baud; > > + > > +???????if (baudrate) { > > +???????????????baud = (u16)(in_freq / (16 * baudrate)); > > +???????????????writew(baud, uart_base + REG_BAUD); > > +???????} > > ? > > ????????sbi_console_set_device(&shakti_console); > > ? > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > > index 7078611a5274..3581d47a6d4b 100644 > > --- a/lib/utils/serial/sifive-uart.c > > +++ b/lib/utils/serial/sifive-uart.c > > @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > ????????uart_baudrate = baudrate; > > ? > > ????????/* Configure baudrate */ > > -???????if (in_freq) > > +???????if (in_freq && baudrate) > > ????????????????set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > > ? > > ????????/* Disable interrupts */ > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > > index 38ea11af4d31..99bf1bf733f2 100644 > > --- a/lib/utils/serial/uart8250.c > > +++ b/lib/utils/serial/uart8250.c > > @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { > > ?int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > ????????????????? u32 reg_width, u32 reg_offset) > > ?{ > > -???????u16 bdiv; > > +???????u16 bdiv = 0; > > ? > > ????????uart8250_base????? = (volatile char *)base + reg_offset; > > ????????uart8250_reg_shift = reg_shift; > > @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > ????????uart8250_in_freq?? = in_freq; > > ????????uart8250_baudrate? = baudrate; > > ? > > -???????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > > +???????if (uart8250_baudrate) { > > +???????????????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / > > +????????????????????? (16 * uart8250_baudrate); > > +???????} > > ? > > ????????/* Disable all interrupts */ > > ????????set_reg(UART_IER_OFFSET, 0x00); > > -- > > 2.36.1 > > > > > > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-20 17:16 ` Andrew Jones @ 2022-07-20 17:25 ` Xiang W 2022-07-21 5:35 ` Andrew Jones 0 siblings, 1 reply; 22+ messages in thread From: Xiang W @ 2022-07-20 17:25 UTC (permalink / raw) To: opensbi ? 2022-07-20???? 19:16 +0200?Andrew Jones??? > On Wed, Jul 20, 2022 at 11:12:18PM +0800, Xiang W wrote: > > ? 2022-07-18???? 19:20 +0200?Andrew Jones??? > > > RISC-V doesn't generate exceptions on divide-by-zero, but the result, > > > all bits set, is not likely what people expect either. In all cases > > > where we divide by baudrate there's a chance it's zero (when the DT > > > it came from is "bad"). To avoid difficult to debug situations, leave > > > baudrate dependent registers alone when baudrate is zero, as, also in > > > all cases, it appears we can skip initialization of those registers > > > and still [hopefully] have a functioning UART. > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > If the parameter is wrong we should skip initialization and should not > > execute sbi_console_set_device. > > Hi Xiang, > > While there should be some DT validation somewhere that points out bad > parameters, I don't think opensbi is the best place to do it, especially > for UART configuration. While we certainly can't progress in all cases > we detect bad input, in the case of a zero baudrate I believe we can, as > it appears setting baudrate registers is typically optional for UARTs. > > We could also consider adding a log buffer which early init can write > errors and warnings to. After the console has been initialized (with > our best effort) we can then output all the errors and warnings from the > log buffer. If we had something like that, then we could add a warning > about the baudrate to that log along with warnings/errors for any other > DT parameters we fail to validate but attempt to work around. > > Thanks, > drew I mean: int xxx_uart_init(...) { if (baudrate == 0) return 0; ... // console_dev should not be set because initialization failed } > > > > > Regards, > > Xiang W > > > --- > > > ?lib/utils/serial/gaisler-uart.c | 2 +- > > > ?lib/utils/serial/shakti-uart.c? | 8 ++++++-- > > > ?lib/utils/serial/sifive-uart.c? | 2 +- > > > ?lib/utils/serial/uart8250.c???? | 7 +++++-- > > > ?4 files changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > > > index 5f30ee43b719..eec75cca0c98 100644 > > > --- a/lib/utils/serial/gaisler-uart.c > > > +++ b/lib/utils/serial/gaisler-uart.c > > > @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > > ????????uart_base = (volatile char *)base; > > > ? > > > ????????/* Configure baudrate */ > > > -???????if (in_freq) > > > +???????if (in_freq && baudrate) > > > ????????????????set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); > > > ? > > > ????????ctrl = get_reg(UART_REG_CTRL); > > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > > > index 5f2fe75c9f33..2f09f141b9cf 100644 > > > --- a/lib/utils/serial/shakti-uart.c > > > +++ b/lib/utils/serial/shakti-uart.c > > > @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { > > > ?int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > > ?{ > > > ????????uart_base = (volatile char *)base; > > > -???????u16 baud = (u16)(in_freq/(16 * baudrate)); > > > -???????writew(baud, uart_base + REG_BAUD); > > > +???????u16 baud; > > > + > > > +???????if (baudrate) { > > > +???????????????baud = (u16)(in_freq / (16 * baudrate)); > > > +???????????????writew(baud, uart_base + REG_BAUD); > > > +???????} > > > ? > > > ????????sbi_console_set_device(&shakti_console); > > > ? > > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > > > index 7078611a5274..3581d47a6d4b 100644 > > > --- a/lib/utils/serial/sifive-uart.c > > > +++ b/lib/utils/serial/sifive-uart.c > > > @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > > ????????uart_baudrate = baudrate; > > > ? > > > ????????/* Configure baudrate */ > > > -???????if (in_freq) > > > +???????if (in_freq && baudrate) > > > ????????????????set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > > > ? > > > ????????/* Disable interrupts */ > > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > > > index 38ea11af4d31..99bf1bf733f2 100644 > > > --- a/lib/utils/serial/uart8250.c > > > +++ b/lib/utils/serial/uart8250.c > > > @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { > > > ?int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > > ????????????????? u32 reg_width, u32 reg_offset) > > > ?{ > > > -???????u16 bdiv; > > > +???????u16 bdiv = 0; > > > ? > > > ????????uart8250_base????? = (volatile char *)base + reg_offset; > > > ????????uart8250_reg_shift = reg_shift; > > > @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > > ????????uart8250_in_freq?? = in_freq; > > > ????????uart8250_baudrate? = baudrate; > > > ? > > > -???????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > > > +???????if (uart8250_baudrate) { > > > +???????????????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / > > > +????????????????????? (16 * uart8250_baudrate); > > > +???????} > > > ? > > > ????????/* Disable all interrupts */ > > > ????????set_reg(UART_IER_OFFSET, 0x00); > > > -- > > > 2.36.1 > > > > > > > > > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-20 17:25 ` Xiang W @ 2022-07-21 5:35 ` Andrew Jones 0 siblings, 0 replies; 22+ messages in thread From: Andrew Jones @ 2022-07-21 5:35 UTC (permalink / raw) To: opensbi On Thu, Jul 21, 2022 at 01:25:44AM +0800, Xiang W wrote: > ? 2022-07-20???? 19:16 +0200?Andrew Jones??? > > On Wed, Jul 20, 2022 at 11:12:18PM +0800, Xiang W wrote: > > > ? 2022-07-18???? 19:20 +0200?Andrew Jones??? > > > > RISC-V doesn't generate exceptions on divide-by-zero, but the result, > > > > all bits set, is not likely what people expect either. In all cases > > > > where we divide by baudrate there's a chance it's zero (when the DT > > > > it came from is "bad"). To avoid difficult to debug situations, leave > > > > baudrate dependent registers alone when baudrate is zero, as, also in > > > > all cases, it appears we can skip initialization of those registers > > > > and still [hopefully] have a functioning UART. > > > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > If the parameter is wrong we should skip initialization and should not > > > execute sbi_console_set_device. > > > > Hi Xiang, > > > > While there should be some DT validation somewhere that points out bad > > parameters, I don't think opensbi is the best place to do it, especially > > for UART configuration. While we certainly can't progress in all cases > > we detect bad input, in the case of a zero baudrate I believe we can, as > > it appears setting baudrate registers is typically optional for UARTs. > > > > We could also consider adding a log buffer which early init can write > > errors and warnings to. After the console has been initialized (with > > our best effort) we can then output all the errors and warnings from the > > log buffer. If we had something like that, then we could add a warning > > about the baudrate to that log along with warnings/errors for any other > > DT parameters we fail to validate but attempt to work around. > > > > Thanks, > > drew > I mean: > > int xxx_uart_init(...) > { > if (baudrate == 0) > return 0; > ... > // console_dev should not be set because initialization failed > } I understood your suggestion, but I disagree. If it's possible to initialize a device without all information in the DT being present or valid, then, IMHO, opensbi should do so. Outputting errors/warnings is good, but giving up turns opensbi into a DT validator which it isn't well equipped to be. It's a particularly bad DT validator when parsing UART DT nodes or any other nodes before the console is available. I do agree that if we try to initialize the console despite detecting bad information in the DT that we risk not having a working console and not easily understanding why. I think the early log buffer I suggest above could help with that. When debugging the first thing one would do is to dump the contents of that buffer to see if any messages can point them in the right direction. I'll try to pull together some log buffer patches and post for discussion. Thanks, drew > > > > > > > > > Regards, > > > Xiang W > > > > --- > > > > ?lib/utils/serial/gaisler-uart.c | 2 +- > > > > ?lib/utils/serial/shakti-uart.c? | 8 ++++++-- > > > > ?lib/utils/serial/sifive-uart.c? | 2 +- > > > > ?lib/utils/serial/uart8250.c???? | 7 +++++-- > > > > ?4 files changed, 13 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > > > > index 5f30ee43b719..eec75cca0c98 100644 > > > > --- a/lib/utils/serial/gaisler-uart.c > > > > +++ b/lib/utils/serial/gaisler-uart.c > > > > @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > > > ????????uart_base = (volatile char *)base; > > > > ? > > > > ????????/* Configure baudrate */ > > > > -???????if (in_freq) > > > > +???????if (in_freq && baudrate) > > > > ????????????????set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); > > > > ? > > > > ????????ctrl = get_reg(UART_REG_CTRL); > > > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > > > > index 5f2fe75c9f33..2f09f141b9cf 100644 > > > > --- a/lib/utils/serial/shakti-uart.c > > > > +++ b/lib/utils/serial/shakti-uart.c > > > > @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { > > > > ?int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > > > ?{ > > > > ????????uart_base = (volatile char *)base; > > > > -???????u16 baud = (u16)(in_freq/(16 * baudrate)); > > > > -???????writew(baud, uart_base + REG_BAUD); > > > > +???????u16 baud; > > > > + > > > > +???????if (baudrate) { > > > > +???????????????baud = (u16)(in_freq / (16 * baudrate)); > > > > +???????????????writew(baud, uart_base + REG_BAUD); > > > > +???????} > > > > ? > > > > ????????sbi_console_set_device(&shakti_console); > > > > ? > > > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > > > > index 7078611a5274..3581d47a6d4b 100644 > > > > --- a/lib/utils/serial/sifive-uart.c > > > > +++ b/lib/utils/serial/sifive-uart.c > > > > @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > > > ????????uart_baudrate = baudrate; > > > > ? > > > > ????????/* Configure baudrate */ > > > > -???????if (in_freq) > > > > +???????if (in_freq && baudrate) > > > > ????????????????set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > > > > ? > > > > ????????/* Disable interrupts */ > > > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > > > > index 38ea11af4d31..99bf1bf733f2 100644 > > > > --- a/lib/utils/serial/uart8250.c > > > > +++ b/lib/utils/serial/uart8250.c > > > > @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { > > > > ?int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > > > ????????????????? u32 reg_width, u32 reg_offset) > > > > ?{ > > > > -???????u16 bdiv; > > > > +???????u16 bdiv = 0; > > > > ? > > > > ????????uart8250_base????? = (volatile char *)base + reg_offset; > > > > ????????uart8250_reg_shift = reg_shift; > > > > @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > > > ????????uart8250_in_freq?? = in_freq; > > > > ????????uart8250_baudrate? = baudrate; > > > > ? > > > > -???????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > > > > +???????if (uart8250_baudrate) { > > > > +???????????????bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / > > > > +????????????????????? (16 * uart8250_baudrate); > > > > +???????} > > > > ? > > > > ????????/* Disable all interrupts */ > > > > ????????set_reg(UART_IER_OFFSET, 0x00); > > > > -- > > > > 2.36.1 > > > > > > > > > > > > > > > > > > > > -- > > > opensbi mailing list > > > opensbi at lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-18 17:20 ` [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using Andrew Jones 2022-07-20 15:12 ` Xiang W @ 2022-07-30 5:31 ` Anup Patel 2022-07-30 10:21 ` Anup Patel 1 sibling, 1 reply; 22+ messages in thread From: Anup Patel @ 2022-07-30 5:31 UTC (permalink / raw) To: opensbi On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > RISC-V doesn't generate exceptions on divide-by-zero, but the result, > all bits set, is not likely what people expect either. In all cases > where we divide by baudrate there's a chance it's zero (when the DT > it came from is "bad"). To avoid difficult to debug situations, leave > baudrate dependent registers alone when baudrate is zero, as, also in > all cases, it appears we can skip initialization of those registers > and still [hopefully] have a functioning UART. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup > --- > lib/utils/serial/gaisler-uart.c | 2 +- > lib/utils/serial/shakti-uart.c | 8 ++++++-- > lib/utils/serial/sifive-uart.c | 2 +- > lib/utils/serial/uart8250.c | 7 +++++-- > 4 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > index 5f30ee43b719..eec75cca0c98 100644 > --- a/lib/utils/serial/gaisler-uart.c > +++ b/lib/utils/serial/gaisler-uart.c > @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > uart_base = (volatile char *)base; > > /* Configure baudrate */ > - if (in_freq) > + if (in_freq && baudrate) > set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); > > ctrl = get_reg(UART_REG_CTRL); > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > index 5f2fe75c9f33..2f09f141b9cf 100644 > --- a/lib/utils/serial/shakti-uart.c > +++ b/lib/utils/serial/shakti-uart.c > @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { > int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > { > uart_base = (volatile char *)base; > - u16 baud = (u16)(in_freq/(16 * baudrate)); > - writew(baud, uart_base + REG_BAUD); > + u16 baud; > + > + if (baudrate) { > + baud = (u16)(in_freq / (16 * baudrate)); > + writew(baud, uart_base + REG_BAUD); > + } > > sbi_console_set_device(&shakti_console); > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > index 7078611a5274..3581d47a6d4b 100644 > --- a/lib/utils/serial/sifive-uart.c > +++ b/lib/utils/serial/sifive-uart.c > @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > uart_baudrate = baudrate; > > /* Configure baudrate */ > - if (in_freq) > + if (in_freq && baudrate) > set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > > /* Disable interrupts */ > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > index 38ea11af4d31..99bf1bf733f2 100644 > --- a/lib/utils/serial/uart8250.c > +++ b/lib/utils/serial/uart8250.c > @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { > int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > u32 reg_width, u32 reg_offset) > { > - u16 bdiv; > + u16 bdiv = 0; > > uart8250_base = (volatile char *)base + reg_offset; > uart8250_reg_shift = reg_shift; > @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > uart8250_in_freq = in_freq; > uart8250_baudrate = baudrate; > > - bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > + if (uart8250_baudrate) { > + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / > + (16 * uart8250_baudrate); > + } > > /* Disable all interrupts */ > set_reg(UART_IER_OFFSET, 0x00); > -- > 2.36.1 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using 2022-07-30 5:31 ` Anup Patel @ 2022-07-30 10:21 ` Anup Patel 0 siblings, 0 replies; 22+ messages in thread From: Anup Patel @ 2022-07-30 10:21 UTC (permalink / raw) To: opensbi On Sat, Jul 30, 2022 at 11:01 AM Anup Patel <anup@brainfault.org> wrote: > > On Mon, Jul 18, 2022 at 10:50 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > RISC-V doesn't generate exceptions on divide-by-zero, but the result, > > all bits set, is not likely what people expect either. In all cases > > where we divide by baudrate there's a chance it's zero (when the DT > > it came from is "bad"). To avoid difficult to debug situations, leave > > baudrate dependent registers alone when baudrate is zero, as, also in > > all cases, it appears we can skip initialization of those registers > > and still [hopefully] have a functioning UART. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > Looks good to me. > > Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > > Regards, > Anup > > > --- > > lib/utils/serial/gaisler-uart.c | 2 +- > > lib/utils/serial/shakti-uart.c | 8 ++++++-- > > lib/utils/serial/sifive-uart.c | 2 +- > > lib/utils/serial/uart8250.c | 7 +++++-- > > 4 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/lib/utils/serial/gaisler-uart.c b/lib/utils/serial/gaisler-uart.c > > index 5f30ee43b719..eec75cca0c98 100644 > > --- a/lib/utils/serial/gaisler-uart.c > > +++ b/lib/utils/serial/gaisler-uart.c > > @@ -70,7 +70,7 @@ int gaisler_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > uart_base = (volatile char *)base; > > > > /* Configure baudrate */ > > - if (in_freq) > > + if (in_freq && baudrate) > > set_reg(UART_REG_SCALER, in_freq / (baudrate * 8 + 7)); > > > > ctrl = get_reg(UART_REG_CTRL); > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > > index 5f2fe75c9f33..2f09f141b9cf 100644 > > --- a/lib/utils/serial/shakti-uart.c > > +++ b/lib/utils/serial/shakti-uart.c > > @@ -47,8 +47,12 @@ static struct sbi_console_device shakti_console = { > > int shakti_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > { > > uart_base = (volatile char *)base; > > - u16 baud = (u16)(in_freq/(16 * baudrate)); > > - writew(baud, uart_base + REG_BAUD); > > + u16 baud; > > + > > + if (baudrate) { > > + baud = (u16)(in_freq / (16 * baudrate)); > > + writew(baud, uart_base + REG_BAUD); > > + } > > > > sbi_console_set_device(&shakti_console); > > > > diff --git a/lib/utils/serial/sifive-uart.c b/lib/utils/serial/sifive-uart.c > > index 7078611a5274..3581d47a6d4b 100644 > > --- a/lib/utils/serial/sifive-uart.c > > +++ b/lib/utils/serial/sifive-uart.c > > @@ -97,7 +97,7 @@ int sifive_uart_init(unsigned long base, u32 in_freq, u32 baudrate) > > uart_baudrate = baudrate; > > > > /* Configure baudrate */ > > - if (in_freq) > > + if (in_freq && baudrate) > > set_reg(UART_REG_DIV, uart_min_clk_divisor(in_freq, baudrate)); > > > > /* Disable interrupts */ > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > > index 38ea11af4d31..99bf1bf733f2 100644 > > --- a/lib/utils/serial/uart8250.c > > +++ b/lib/utils/serial/uart8250.c > > @@ -93,7 +93,7 @@ static struct sbi_console_device uart8250_console = { > > int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > u32 reg_width, u32 reg_offset) > > { > > - u16 bdiv; > > + u16 bdiv = 0; > > > > uart8250_base = (volatile char *)base + reg_offset; > > uart8250_reg_shift = reg_shift; > > @@ -101,7 +101,10 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > uart8250_in_freq = in_freq; > > uart8250_baudrate = baudrate; > > > > - bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > > + if (uart8250_baudrate) { > > + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / > > + (16 * uart8250_baudrate); > > + } > > > > /* Disable all interrupts */ > > set_reg(UART_IER_OFFSET, 0x00); > > -- > > 2.36.1 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-07-30 10:21 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-18 17:20 [PATCH 0/4] lib: utils/serial: Collection of UART code improvements Andrew Jones 2022-07-18 17:20 ` [PATCH 1/4] lib: utils/fdt: Factor out common uart node code Andrew Jones 2022-07-30 5:24 ` Anup Patel 2022-07-30 10:19 ` Anup Patel 2022-07-18 17:20 ` [PATCH 2/4] lib: utils/serial: Initialize platform_uart_data to zero Andrew Jones 2022-07-18 17:42 ` Jessica Clarke 2022-07-18 18:35 ` Andrew Jones 2022-07-18 18:39 ` Jessica Clarke 2022-07-19 10:30 ` Andrew Jones 2022-07-30 5:25 ` Anup Patel 2022-07-30 10:20 ` Anup Patel 2022-07-18 17:20 ` [PATCH 3/4] lib: serial: Clean up coding style in sifive-uart.c Andrew Jones 2022-07-20 15:13 ` Xiang W 2022-07-30 5:27 ` Anup Patel 2022-07-30 10:20 ` Anup Patel 2022-07-18 17:20 ` [PATCH 4/4] lib: utils/serial: Ensure baudrate is non-zero before using Andrew Jones 2022-07-20 15:12 ` Xiang W 2022-07-20 17:16 ` Andrew Jones 2022-07-20 17:25 ` Xiang W 2022-07-21 5:35 ` Andrew Jones 2022-07-30 5:31 ` Anup Patel 2022-07-30 10:21 ` Anup Patel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox