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

["Alex G." <mr.nuke.me@gmail.com>]

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 <trini@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