lib: utils/serial: Collection of UART code improvements

PATCH New Andrew Jones 4 participants 22 messages Jul 18, 2022 – Jul 30, 2022 SiFive drivers

Share & tools

Andrew Jones

author

2022-07-18 17:20 UTC #0 lore raw

cover lib: utils/serial: Collection of UART code improvements

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(-)

Signature · 1 line

2.36.1

Andrew Jones

author

2022-07-18 17:20 UTC #1 lore raw

PATCH 1/4 lib: utils/fdt: Factor out common uart node code

Factor out the common code used by the fdt UART node parsers, allowing us to drop duplicate code.

Signed-off-by Andrew Jones <[email protected]>

---
 lib/utils/fdt/fdt_helper.c | 131 +++++++++++--------------------------
 1 file changed, 39 insertions(+), 92 deletions(-)

lib/utils/fdt/fdt_helper.c +39 ~~-92~~

index 2bdb1ef23e55..c6abf804014d 100644
@@ -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,
-				    &reg_addr, &reg_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,
-				    &reg_addr, &reg_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,
-				    &reg_addr, &reg_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,
-				    &reg_addr, &reg_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)

Signature · 1 line

2.36.1

Anup Patel

2022-07-30 05:24 UTC #2 lore raw

PATCH 1/4 lib: utils/fdt: Factor out common uart node code

Quoted 5 lines · Andrew Jones

Factor out the common code used by the fdt UART node parsers, allowing us to drop duplicate code.
Signed-off-by Andrew Jones <[email protected]>

Looks good to me.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 200 lines

---
 lib/utils/fdt/fdt_helper.c | 131 +++++++++++--------------------------
 1 file changed, 39 insertions(+), 92 deletions(-)

lib/utils/fdt/fdt_helper.c +39 ~~-93~~

index 2bdb1ef23e55..c6abf804014d 100644
@@ -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,
-                                   &reg_addr, &reg_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,
-                                   &reg_addr, &reg_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,
-                                   &reg_addr, &reg_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,
-                                   &reg_addr, &reg_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

Anup Patel

Applied

2022-07-30 10:19 UTC #3 lore raw

PATCH 1/4 lib: utils/fdt: Factor out common uart node code

Quoted 11 lines · Anup Patel

Quoted 5 lines · Andrew Jones
Factor out the common code used by the fdt UART node parsers, allowing us to drop duplicate code.
Signed-off-by Andrew Jones <[email protected]>
Looks good to me.
Reviewed-by Anup Patel <[email protected]>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

Quoted 204 lines

Regards,
Anup

Quoted 200 lines

---
 lib/utils/fdt/fdt_helper.c | 131 +++++++++++--------------------------
 1 file changed, 39 insertions(+), 92 deletions(-)

lib/utils/fdt/fdt_helper.c +39 ~~-93~~

index 2bdb1ef23e55..c6abf804014d 100644
@@ -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,
-                                   &reg_addr, &reg_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,
-                                   &reg_addr, &reg_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,
-                                   &reg_addr, &reg_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,
-                                   &reg_addr, &reg_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

Andrew Jones

author

2022-07-18 17:20 UTC #4 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

In reply to Andrew Jones

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 <[email protected]>

---
 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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -1

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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;

Signature · 1 line

2.36.1

Jessica Clarke

2022-07-18 17:42 UTC #5 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

Quoted 7 lines · Andrew Jones

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

Quoted 96 lines

Signed-off-by Andrew Jones <[email protected]>

---

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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -1

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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;

Signature · 7 lines

2.36.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Andrew Jones

author

2022-07-18 18:35 UTC #6 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

Quoted 9 lines · Jessica Clarke

Quoted 7 lines · Andrew Jones
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...

Quoted 3 lines

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

Quoted 100 lines

Jess

Quoted 96 lines

Signed-off-by Andrew Jones <[email protected]>

---

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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -1

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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;

Signature · 7 lines

2.36.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Jessica Clarke

2022-07-18 18:39 UTC #7 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

Quoted 17 lines · Andrew Jones

Quoted 9 lines · Jessica Clarke
Quoted 7 lines · Andrew Jones
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...
Quoted 3 lines
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,

Quoted 7 lines

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

Quoted 103 lines

Thanks,
drew

Quoted 100 lines

Jess

Quoted 96 lines

Signed-off-by Andrew Jones <[email protected]>

---

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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -1

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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;

Signature · 7 lines

2.36.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Andrew Jones

author

2022-07-19 10:30 UTC #8 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

Quoted 23 lines · Jessica Clarke

Quoted 22 lines · Andrew Jones
Quoted 9 lines · Jessica Clarke
Quoted 7 lines · Andrew Jones
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...
Quoted 3 lines
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.

Quoted 5 lines

Quoted 2 lines
[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

Quoted 107 lines

Jess

Quoted 103 lines

Thanks,
drew

Quoted 100 lines

Jess

Quoted 96 lines

Signed-off-by Andrew Jones <[email protected]>

---

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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -1

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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;

Signature · 7 lines

2.36.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Anup Patel

2022-07-30 05:25 UTC #9 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

In reply to Andrew Jones

Quoted 9 lines · Andrew Jones

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 <[email protected]>

Looks good to me.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 95 lines

---
 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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -2

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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

Anup Patel

Applied

2022-07-30 10:20 UTC #10 lore raw

PATCH 2/4 lib: utils/serial: Initialize platform_uart_data to zero

Quoted 10 lines · Anup Patel

Quoted 8 lines · Andrew Jones
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...

Quoted 5 lines

Quoted 1 line
Signed-off-by Andrew Jones <[email protected]>
Looks good to me.
Reviewed-by Anup Patel <[email protected]>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

Quoted 99 lines

Regards,
Anup

Quoted 95 lines

---
 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(-)

lib/utils/serial/fdt_serial_gaisler.c +1 -1

index 9a9f9a8b7962..74988e34bcd8 100644
@@ -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)

lib/utils/serial/fdt_serial_shakti.c +1 -1

index 4f91419ef53c..0e056303dbb5 100644
@@ -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)

lib/utils/serial/fdt_serial_sifive.c +1 -1

index f4c833c1573d..3ca10913eee3 100644
@@ -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)

lib/utils/serial/fdt_serial_uart8250.c +1 -1

index 544b7416de42..0b95f2d9e44e 100644
@@ -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)

lib/utils/serial/fdt_serial_xlnx_uartlite.c +1 -1

index 466e16e82d8c..9f04aea3673b 100644
@@ -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)

platform/fpga/openpiton/platform.c +1 -2

index 7ca21236bfef..5ff7d200aba9 100644
@@ -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

Andrew Jones

author

2022-07-18 17:20 UTC #11 lore raw

PATCH 3/4 lib: serial: Clean up coding style in sifive-uart.c

In reply to Andrew Jones

Signed-off-by Andrew Jones <[email protected]>

---
 lib/utils/serial/sifive-uart.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

lib/utils/serial/sifive-uart.c +8 -3

index 9478a77f8054..7078611a5274 100644
@@ -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);

Signature · 1 line

2.36.1

Xiang W

2022-07-20 15:13 UTC #12 lore raw

PATCH 3/4 lib: serial: Clean up coding style in sifive-uart.c

? 2022-07-18???? 19:20 +0200?Andrew Jones???

Quoted 1 line

Signed-off-by Andrew Jones <[email protected]>

Look good to me.

Reviewed-by Xiang W <[email protected]>

Quoted 53 lines

---
?lib/utils/serial/sifive-uart.c | 11 ++++++++---
?1 file changed, 8 insertions(+), 3 deletions(-)
lib/utils/serial/sifive-uart.c +1 -0
index 9478a77f8054..7078611a5274 100644
@@ -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)
lib/utils/serial/sifive-uart.c +2 -0
@@ -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;
 ?}
?
lib/utils/serial/sifive-uart.c +0 -0
@@ -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);
?
Signature · 1 line
2.36.1

Anup Patel

2022-07-30 05:27 UTC #13 lore raw

PATCH 3/4 lib: serial: Clean up coding style in sifive-uart.c

In reply to Andrew Jones

Quoted 2 lines · Andrew Jones

Signed-off-by Andrew Jones <[email protected]>

Looks good to me.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 57 lines

---
 lib/utils/serial/sifive-uart.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

lib/utils/serial/sifive-uart.c +8 -4

index 9478a77f8054..7078611a5274 100644
@@ -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

Anup Patel

Applied

2022-07-30 10:20 UTC #14 lore raw

PATCH 3/4 lib: serial: Clean up coding style in sifive-uart.c

Quoted 8 lines · Anup Patel

Quoted 2 lines · Andrew Jones
Signed-off-by Andrew Jones <[email protected]>
Looks good to me.
Reviewed-by Anup Patel <[email protected]>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

Quoted 61 lines

Regards,
Anup

Quoted 57 lines

---
 lib/utils/serial/sifive-uart.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

lib/utils/serial/sifive-uart.c +8 -4

index 9478a77f8054..7078611a5274 100644
@@ -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

Andrew Jones

author

2022-07-18 17:20 UTC #15 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

In reply to 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 <[email protected]>

---
 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(-)

lib/utils/serial/gaisler-uart.c +1 -1

index 5f30ee43b719..eec75cca0c98 100644
@@ -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);

lib/utils/serial/shakti-uart.c +6 -2

index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);

lib/utils/serial/sifive-uart.c +1 -1

index 7078611a5274..3581d47a6d4b 100644
@@ -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 */

lib/utils/serial/uart8250.c +5 -2

index 38ea11af4d31..99bf1bf733f2 100644
@@ -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);

Signature · 1 line

2.36.1

Xiang W

2022-07-20 15:12 UTC #16 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

? 2022-07-18???? 19:20 +0200?Andrew Jones???

Quoted 9 lines

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 <[email protected]>

If the parameter is wrong we should skip initialization and should not execute sbi_console_set_device.

Regards,
Xiang W

Quoted 81 lines

---
?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(-)
lib/utils/serial/gaisler-uart.c +0 -0
index 5f30ee43b719..eec75cca0c98 100644
@@ -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);
lib/utils/serial/shakti-uart.c +6 -2
index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);
?
lib/utils/serial/sifive-uart.c +0 -0
index 7078611a5274..3581d47a6d4b 100644
@@ -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 */
lib/utils/serial/uart8250.c +1 -1
index 38ea11af4d31..99bf1bf733f2 100644
@@ -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;
lib/utils/serial/uart8250.c +0 -0
@@ -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);
Signature · 1 line
2.36.1

Andrew Jones

author

2022-07-20 17:16 UTC #17 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

Quoted 9 lines · Xiang W

? 2022-07-18???? 19:20 +0200?Andrew Jones???
Quoted 8 lines
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.

Quoted 3 lines

Quoted 1 line
Signed-off-by Andrew Jones <[email protected]>
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

Quoted 91 lines

Regards,
Xiang W
Quoted 81 lines
---
?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(-)
lib/utils/serial/gaisler-uart.c +0 -0
index 5f30ee43b719..eec75cca0c98 100644
@@ -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);
lib/utils/serial/shakti-uart.c +6 -2
index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);
?
lib/utils/serial/sifive-uart.c +0 -0
index 7078611a5274..3581d47a6d4b 100644
@@ -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 */
lib/utils/serial/uart8250.c +1 -1
index 38ea11af4d31..99bf1bf733f2 100644
@@ -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;
lib/utils/serial/uart8250.c +0 -0
@@ -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);
Signature · 1 line
2.36.1
Signature · 3 lines
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Xiang W

2022-07-20 17:25 UTC #18 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

? 2022-07-20???? 19:16 +0200?Andrew Jones???

Quoted 29 lines · Xiang W

Quoted 9 lines
? 2022-07-18???? 19:20 +0200?Andrew Jones???
Quoted 8 lines
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.
Quoted 3 lines
Quoted 1 line
Signed-off-by Andrew Jones <[email protected]>
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.

Quoted 2 lines

Thanks,
drew

I mean:

int xxx_uart_init(...)

{
	if (baudrate == 0)
		return 0;
	...
	// console_dev should not be set because initialization failed
}

Quoted 92 lines

Quoted 92 lines
Regards,
Xiang W
Quoted 81 lines
---
?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(-)
lib/utils/serial/gaisler-uart.c +0 -0
index 5f30ee43b719..eec75cca0c98 100644
@@ -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);
lib/utils/serial/shakti-uart.c +6 -2
index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);
?
lib/utils/serial/sifive-uart.c +0 -0
index 7078611a5274..3581d47a6d4b 100644
@@ -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 */
lib/utils/serial/uart8250.c +1 -1
index 38ea11af4d31..99bf1bf733f2 100644
@@ -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;
lib/utils/serial/uart8250.c +0 -0
@@ -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);
Signature · 1 line
2.36.1
Signature · 3 lines
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Andrew Jones

author

2022-07-21 05:35 UTC #19 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

Quoted 34 lines · Xiang W

? 2022-07-20???? 19:16 +0200?Andrew Jones???
Quoted 29 lines · Xiang W
Quoted 9 lines
? 2022-07-18???? 19:20 +0200?Andrew Jones???
Quoted 8 lines
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.
Quoted 3 lines
Quoted 1 line
Signed-off-by Andrew Jones <[email protected]>
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.
Quoted 2 lines
Thanks,
drew
I mean:

Quoted 7 lines

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

Quoted 95 lines

Quoted 93 lines
Quoted 93 lines
Regards,
Xiang W
Quoted 81 lines
---
?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(-)
lib/utils/serial/gaisler-uart.c +0 -0
index 5f30ee43b719..eec75cca0c98 100644
@@ -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);
lib/utils/serial/shakti-uart.c +6 -2
index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);
?
lib/utils/serial/sifive-uart.c +0 -0
index 7078611a5274..3581d47a6d4b 100644
@@ -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 */
lib/utils/serial/uart8250.c +1 -1
index 38ea11af4d31..99bf1bf733f2 100644
@@ -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;
lib/utils/serial/uart8250.c +0 -0
@@ -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);
Signature · 1 line
2.36.1
Signature · 3 lines
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Anup Patel

2022-07-30 05:31 UTC #20 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

In reply to Andrew Jones

Quoted 10 lines · 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 <[email protected]>

Looks good to me.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 85 lines

---
 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(-)

lib/utils/serial/gaisler-uart.c +1 -1

index 5f30ee43b719..eec75cca0c98 100644
@@ -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);

lib/utils/serial/shakti-uart.c +6 -2

index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);

lib/utils/serial/sifive-uart.c +1 -1

index 7078611a5274..3581d47a6d4b 100644
@@ -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 */

lib/utils/serial/uart8250.c +5 -3

index 38ea11af4d31..99bf1bf733f2 100644
@@ -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

Anup Patel

Applied

2022-07-30 10:21 UTC #21 lore raw

PATCH 4/4 lib: utils/serial: Ensure baudrate is non-zero before using

Quoted 11 lines · Anup Patel

Quoted 9 lines · 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.

Quoted 5 lines

Quoted 1 line
Signed-off-by Andrew Jones <[email protected]>
Looks good to me.
Reviewed-by Anup Patel <[email protected]>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

Quoted 89 lines

Regards,
Anup

Quoted 85 lines

---
 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(-)

lib/utils/serial/gaisler-uart.c +1 -1

index 5f30ee43b719..eec75cca0c98 100644
@@ -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);

lib/utils/serial/shakti-uart.c +6 -2

index 5f2fe75c9f33..2f09f141b9cf 100644
@@ -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);

lib/utils/serial/sifive-uart.c +1 -1

index 7078611a5274..3581d47a6d4b 100644
@@ -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 */

lib/utils/serial/uart8250.c +5 -3

index 38ea11af4d31..99bf1bf733f2 100644
@@ -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