Re: [PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE

[Alex G.]

On 9/13/21 4:24 PM, Tom Rini wrote:
In order to move CONFIG_SYS_BAUDRATE_TABLE to Kconfig, we need to rework
the logic a bit.  Rename the users of CONFIG_SYS_BAUDRATE_TABLE to
SYS_BAUDRATE_TABLE.  Introduce a series of CONFIG_BAUDRATE_TABLE_...
that include some number of baud rates.  These match all existing users.
The help for each entry specifies what the exact table is, for a given
option.  Define what SYS_BAUDRATE_TABLE will be in include/serial.h now.

Signed-off-by: Tom Rini <tr...@konsulko.com>
---

diff --git a/include/serial.h b/include/serial.h
index 6d1e62c6770c..150644c4c3d4 100644
--- a/include/serial.h
+++ b/include/serial.h
@@ -3,6 +3,42 @@
  
  #include <post.h>
  
+#if defined(CONFIG_BAUDRATE_TABLE_300_TO_38400_115200)
+#define SYS_BAUDRATE_TABLE     { 300, 600, 1200, 2400, 4800, 9600, 19200, \
+                                 38400, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_115200)
+#define SYS_BAUDRATE_TABLE     { 300, 600, 1200, 2400, 4800, 9600, 19200, \
+                                 38400, 57600, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_230400)
+#define SYS_BAUDRATE_TABLE     { 300, 600, 1200, 2400, 4800, 9600, 19200, \
+                                 38400, 57600, 115200, 230400 }
+#elif defined(CONFIG_BAUDRATE_TABLE_300_TO_6000000)
+#define SYS_BAUDRATE_TABLE     { 300, 600, 1200, 1800, 2400, 4800, 9600, \
+                                 19200, 38400, 57600, 115200, 230400, \
+                                 460800, 500000, 576000, 921600, 1000000, \
+                                 1152000, 1500000, 2000000, 2500000, \
+                                 3000000, 3500000, 4000000, 4500000, \
+                                 5000000, 5500000, 6000000 }
+#elif defined(CONFIG_BAUDRATE_TABLE_4800_TO_115200)
+#define SYS_BAUDRATE_TABLE     { 4800, 9600, 19200, 38400, 57600, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_115200)
+#define SYS_BAUDRATE_TABLE     { 9600, 19200, 38400, 57600, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400)
+#define SYS_BAUDRATE_TABLE     { 9600, 19200, 38400, 57600, 115200, 230400 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_460800)
+#define SYS_BAUDRATE_TABLE     { 9600, 19200, 38400, 57600, 115200, 230400, 
460800 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_921600)
+#define SYS_BAUDRATE_TABLE     { 9600, 19200, 38400, 57600, 115200, 230400, \
+                                 460800, 921600 }
+#elif defined(CONFIG_BAUDRATE_TABLE_9600_TO_230400_500000_1500000)
+#define SYS_BAUDRATE_TABLE     { 9600, 19200, 38400, 57600, 115200, 230400, \
+                                 500000, 1500000 }
+#elif defined(CONFIG_BAUDRATE_TABLE_38400_115200_ONLY)
+#define SYS_BAUDRATE_TABLE     { 38400, 115200 }
+#elif defined(CONFIG_BAUDRATE_TABLE_115200_ONLY)
+#define SYS_BAUDRATE_TABLE     { 115200 }
+#endif
+
  struct serial_device {
        /* enough bytes to match alignment of following func pointer */
        char    name[16];



This opens the gates to #ifdefing the heck out of serial.h. What happens 
to my board that goes from 300 to 2000000?
 * We need a new Kconfig and new ifdef
What happens to my other board that goes from 300 to 2500000?
 * We need a new Kconfig and new ifdef
The pattern doesn't look promising.

I actually think this change can make the situation worse. We trade 
having an antiquated and inconvenient SYS_BAUDRATE_TABLE for one Kconfig 
per each possible baudrate combination. How does this make sense?

I've seen situations were SPL boots with 2Mbaud and executes 
succesfully, u-boot starts up with 2Mbaud just fine. few lines later, 
u-boot, downswitches to 115200 because CONFIG_SYS_BAUDRATE_TABLE says so.

Suggestion I: Can we have a MIN/MAX value for baudrates, and have the 
code work from there ?

Suggestion II: Define the Kconfig SYS_BAUDRATE_TABLE table to a C array, 
like 'default "{ 300, 420, 690}" ' and forego the #ifdefs in serial.h

Suggestion III: Get rid of the logic that says "baudrate must be one of 
these predefined values" and let the serial driver return -ENOBUENO or 
-EINVAL if the hardware really can't do that baudrate. Most UARTs 
nowadays can do a wide range of values, and the baudrate table doesn't 
model that very well. Combine this with a CONFIG_MAX_BAUDRATE so that 
boards with shitty RS232 converters can set a safe upper limit -- and 
make sure CONFIG_BAUDRATE also enforces this.

There's a lot of unrealized potential here.

Alex