[PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE

[Pali Rohár]

Hello!

On Monday 13 September 2021 18:11:38 Tom Rini wrote:
> 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 at 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 think that above #ifdef hell is the worst what could be done. It does
not solve existing problems and just introduce new way which opens doors
for problems for new boards...

What should be done is to completely kill this CONFIG_SYS_BAUDRATE_TABLE
option and not to convert it to another set of options.

Why to kill it?

1) Probably none of CONFIG_SYS_BAUDRATE_TABLE in include board files is
   correctly defined. Some of them contains only just few baudrates
   copied from other header files without checking that it is correct
   or contains some "radom" definition where is also 9600 and 115200 (as
   probably only these values are tested).

2) Available baudrate values are not specific to board, but rather to
   UART chip used on the board. So only UART driver knowns what are
   supported values.

3) Some boards may have integrated USB<-->UART chip which is "glued"
   directly to the board UART chip. So baudrates possible to set is
   limited to what together USB<-->UART chip and SoC UART supports.


I think the proper way is to do:

1) Extend serial UART DM API with a callback which takes baudrate
   argument and returns the nearest baudrate value which hardware can
   set. Then serial UART drivers should implement it. This
   implementation is simple, because most UART drivers already do it.
   They take baudrate and calculates the best UART clock divisor and set
   it. And from this calculated divisor can be reconstructed the real
   baudrate value.

2) Introduce some board specific function which can limit / filter
   particular baudrate (used in scenario when board has directly USB
   port with integrated USB<-->UART).

2) Currently CONFIG_SYS_BAUDRATE_TABLE is used for checking if baudrate
   value is supported prior setting it. So replace this table by a new
   callback from step 1) and 2) and allow baudrate if is e.g. in 2-3%
   tolerance.

3) Add some platform / SoC option to allow specifying upper or lower
   limit for baudrate. Not as Kconfig option, this is mean as platform
   limitation exposed by e.g. used clock source. Some platforms have
   limitation that for specific clock must have some minimal clock UART
   divisor.

4) Add some optional Kconfig option to limit upper and lower UART
   baudrate.

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