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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5357 bytes --]

On Mon, Sep 13, 2021 at 05:03:13PM -0500, Alex G. wrote:
> 
> 
> 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.

This reminds me I was tempted to do a cover letter, but didn't.  What
happens is I tell you no.  Most boards are using the standard table of
common rates from 9600 to 115200.  A nice follow-up would be to change
every board with a special case that's not above 115200 to just use the
normal table.  Everyone else?  There's the maximal table.  That's it.
That's even come in fairly recently, for mvebu platforms.

> 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'm going, as much as possible, for migrating the current situation.
There's many places things could be cleaner, but "we'll clean this up
and then migrate ..." is why we're so very very far behind where I hoped
to be.

> 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.

Well, the table and CONFIG_BAUDRATE.

> 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.

I'd certainly like to see something done as a follow-up that makes it
easier to support platforms that can do something faster.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]