[PATCH 5/5] serial: Rework CONFIG_SYS_BAUDRATE_TABLE

[Tom Rini]

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.  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210924/50171a00/attachment.sig>