[PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE

[Pali Rohár]

On Friday 24 September 2021 18:07:36 Tom Rini wrote:
> On Fri, Sep 24, 2021 at 09:08:15PM +0200, Pali Rohár wrote:
> > 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...
> 
> I don't think this will open new problems, since I do intend to fire off
> the "easy" reduction option of switching to either standard (current
> default) or maximal tables, rather than N tables.
> 
> > 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.
> 
> The problem I have is none of this is new, and predates Kconfig being
> introduced.  Now, if you're saying you'll fire off a patch series to do
> the above, in the next few months even, OK, yes, I can set aside this
> particular series for now.

Done: http://patchwork.ozlabs.org/project/uboot/patch/20210925121958.26001-1-pali@kernel.org/

Just as RFC and example how it could look like. If you like it we can
continue discussion in thread for above patch.

> But setting aside some of the ugly defines
> until we can convert them to something cleaner is why we're at this
> stagnant point in the conversion.
> 
> -- 
> Tom