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 <tr...@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-p...@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
