| Message ID | 20210925121958.26001-1-pali@kernel.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [RFC] RFC: Replace CONFIG_SYS_BAUDRATE_TABLE by board and UART driver rounding functions | expand |
On Sat, Sep 25, 2021 at 02:19:58PM +0200, Pali Rohár wrote: > Add new functions which returns the nearest baudrate and use them instead > of hardcoded and incomplete CONFIG_SYS_BAUDRATE_TABLE compile time option. > > Add implementation of rounding function for serial_mvebu_a3700 driver and > also for A3720 Espressobin board which has integrated pl2303 USB<->UART > converter, which basically limits baudrates which can user set. > > Completely remove CONFIG_SYS_BAUDRATE_TABLE defines from all A3720 boards > as now with rounding functions it is not used anymore. > > NOTE: This is just an example how to kill CONFIG_SYS_BAUDRATE_TABLE compile > time definitions. I tested it that it works on A3720 Turris Mox board. I > have not tested A3720 Espressobin board yet. > > More discussion on this approach is required, so take this just as RFC > change. > > Signed-off-by: Pali Rohár <pali@kernel.org> Thanks for doing this. My first question is, is this really per-board? Or per SoC / UART chipset. I would hope for example that for plain old ns16550s this would be a generic function, perhaps with an optional board call-out for board design limitations. This does feel like a reasonable amount of code for platforms like this that were supporting what was the maximal rate table before.
On Saturday 25 September 2021 09:51:08 Tom Rini wrote: > On Sat, Sep 25, 2021 at 02:19:58PM +0200, Pali Rohár wrote: > > > Add new functions which returns the nearest baudrate and use them instead > > of hardcoded and incomplete CONFIG_SYS_BAUDRATE_TABLE compile time option. > > > > Add implementation of rounding function for serial_mvebu_a3700 driver and > > also for A3720 Espressobin board which has integrated pl2303 USB<->UART > > converter, which basically limits baudrates which can user set. > > > > Completely remove CONFIG_SYS_BAUDRATE_TABLE defines from all A3720 boards > > as now with rounding functions it is not used anymore. > > > > NOTE: This is just an example how to kill CONFIG_SYS_BAUDRATE_TABLE compile > > time definitions. I tested it that it works on A3720 Turris Mox board. I > > have not tested A3720 Espressobin board yet. > > > > More discussion on this approach is required, so take this just as RFC > > change. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Thanks for doing this. My first question is, is this really per-board? There is per UART chipset rounding / limitation and this should be implemented in UART driver (like I did it in serial_mvebu_a3700.c). Then there can be per SoC limitation, e.g. SoC has unstable clock source for high baudrates. Meaning that UART chipset can support higher baudrates (as exposed by UART driver) but for some reasons they are not possible on SoC. And then there can be really per-board limitations, like it is for Espressobin board. In most cases SoC has UART pins which people can connect to other UART device (e.g. to other SoC with UART or to some universal USB<->UART cables for "computers"). But Espressobin board has integrated pl2303 chip which is soldered directly to A3720 SoC UART pins. This pl2303 chip is USB<->UART converter and board itself has just micro-USB port (which is soldered to this pl2303). So at the end you can on this board use UART only via that integrated micro-USB port, which talks with standard USB protocol. Therefore on this board you have two limitations for UART baudrates. First one is exposed by UART SoC chip (serial_mvebu_a3700.c) and second one is exposed by what that pl2303 supports. And this second limitation is board specific -- not A3720 SoC. Other boards with A3720 SoC, e.g. Turris Mox has exposed directly UART pins and you are free to connect any USB<->UART cable to these pins as you want. And this design with soldered USB<->UART chip on the board is not rare. So U-Boot should have better support for it. Note that lot of times clock source for these USB<->UART chips and SoC UART chip is different (because these USB based chips have its own clock source). So there are less common supported baudrates which can be used on the boards. And when talking about high speed baudrates (above 115200 bauds) then lot of times these "common supported baudrates" are just non-standard baudrate numbers (e.g. 3 125 000 bauds). Because you have to choose divisors both both chips to produce baudrate with only about 2-3% difference. > Or per SoC / UART chipset. I would hope for example that for plain old > ns16550s this would be a generic function, perhaps with an optional > board call-out for board design limitations. This does feel like a > reasonable amount of code for platforms like this that were supporting > what was the maximal rate table before. > > -- > Tom
PING. Any progress in this direction? Any comments or objections in this approach? On Saturday 25 September 2021 16:23:13 Pali Rohár wrote: > On Saturday 25 September 2021 09:51:08 Tom Rini wrote: > > On Sat, Sep 25, 2021 at 02:19:58PM +0200, Pali Rohár wrote: > > > > > Add new functions which returns the nearest baudrate and use them instead > > > of hardcoded and incomplete CONFIG_SYS_BAUDRATE_TABLE compile time option. > > > > > > Add implementation of rounding function for serial_mvebu_a3700 driver and > > > also for A3720 Espressobin board which has integrated pl2303 USB<->UART > > > converter, which basically limits baudrates which can user set. > > > > > > Completely remove CONFIG_SYS_BAUDRATE_TABLE defines from all A3720 boards > > > as now with rounding functions it is not used anymore. > > > > > > NOTE: This is just an example how to kill CONFIG_SYS_BAUDRATE_TABLE compile > > > time definitions. I tested it that it works on A3720 Turris Mox board. I > > > have not tested A3720 Espressobin board yet. > > > > > > More discussion on this approach is required, so take this just as RFC > > > change. > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > Thanks for doing this. My first question is, is this really per-board? > > There is per UART chipset rounding / limitation and this should be > implemented in UART driver (like I did it in serial_mvebu_a3700.c). > > Then there can be per SoC limitation, e.g. SoC has unstable clock source > for high baudrates. Meaning that UART chipset can support higher > baudrates (as exposed by UART driver) but for some reasons they are not > possible on SoC. > > And then there can be really per-board limitations, like it is for > Espressobin board. In most cases SoC has UART pins which people can > connect to other UART device (e.g. to other SoC with UART or to some > universal USB<->UART cables for "computers"). But Espressobin board > has integrated pl2303 chip which is soldered directly to A3720 SoC UART > pins. This pl2303 chip is USB<->UART converter and board itself has just > micro-USB port (which is soldered to this pl2303). So at the end you can > on this board use UART only via that integrated micro-USB port, which > talks with standard USB protocol. > > Therefore on this board you have two limitations for UART baudrates. > First one is exposed by UART SoC chip (serial_mvebu_a3700.c) and second > one is exposed by what that pl2303 supports. And this second limitation > is board specific -- not A3720 SoC. Other boards with A3720 SoC, e.g. > Turris Mox has exposed directly UART pins and you are free to connect > any USB<->UART cable to these pins as you want. > > And this design with soldered USB<->UART chip on the board is not rare. > So U-Boot should have better support for it. > > Note that lot of times clock source for these USB<->UART chips and SoC > UART chip is different (because these USB based chips have its own > clock source). So there are less common supported baudrates which can be > used on the boards. And when talking about high speed baudrates (above > 115200 bauds) then lot of times these "common supported baudrates" are > just non-standard baudrate numbers (e.g. 3 125 000 bauds). Because you > have to choose divisors both both chips to produce baudrate with only > about 2-3% difference. > > > Or per SoC / UART chipset. I would hope for example that for plain old > > ns16550s this would be a generic function, perhaps with an optional > > board call-out for board design limitations. This does feel like a > > reasonable amount of code for platforms like this that were supporting > > what was the maximal rate table before. > > > > -- > > Tom
On Wed, Apr 06, 2022 at 03:30:11PM +0200, Pali Rohár wrote: > PING. Any progress in this direction? Any comments or objections in this approach? I keep meaning to reply to this. I think this is the right direction, I'd just like to see it implemented for some other UARTs as well, and this hasn't re-appeared at the top of my CONFIG migrate list yet. > > On Saturday 25 September 2021 16:23:13 Pali Rohár wrote: > > On Saturday 25 September 2021 09:51:08 Tom Rini wrote: > > > On Sat, Sep 25, 2021 at 02:19:58PM +0200, Pali Rohár wrote: > > > > > > > Add new functions which returns the nearest baudrate and use them instead > > > > of hardcoded and incomplete CONFIG_SYS_BAUDRATE_TABLE compile time option. > > > > > > > > Add implementation of rounding function for serial_mvebu_a3700 driver and > > > > also for A3720 Espressobin board which has integrated pl2303 USB<->UART > > > > converter, which basically limits baudrates which can user set. > > > > > > > > Completely remove CONFIG_SYS_BAUDRATE_TABLE defines from all A3720 boards > > > > as now with rounding functions it is not used anymore. > > > > > > > > NOTE: This is just an example how to kill CONFIG_SYS_BAUDRATE_TABLE compile > > > > time definitions. I tested it that it works on A3720 Turris Mox board. I > > > > have not tested A3720 Espressobin board yet. > > > > > > > > More discussion on this approach is required, so take this just as RFC > > > > change. > > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > > > Thanks for doing this. My first question is, is this really per-board? > > > > There is per UART chipset rounding / limitation and this should be > > implemented in UART driver (like I did it in serial_mvebu_a3700.c). > > > > Then there can be per SoC limitation, e.g. SoC has unstable clock source > > for high baudrates. Meaning that UART chipset can support higher > > baudrates (as exposed by UART driver) but for some reasons they are not > > possible on SoC. > > > > And then there can be really per-board limitations, like it is for > > Espressobin board. In most cases SoC has UART pins which people can > > connect to other UART device (e.g. to other SoC with UART or to some > > universal USB<->UART cables for "computers"). But Espressobin board > > has integrated pl2303 chip which is soldered directly to A3720 SoC UART > > pins. This pl2303 chip is USB<->UART converter and board itself has just > > micro-USB port (which is soldered to this pl2303). So at the end you can > > on this board use UART only via that integrated micro-USB port, which > > talks with standard USB protocol. > > > > Therefore on this board you have two limitations for UART baudrates. > > First one is exposed by UART SoC chip (serial_mvebu_a3700.c) and second > > one is exposed by what that pl2303 supports. And this second limitation > > is board specific -- not A3720 SoC. Other boards with A3720 SoC, e.g. > > Turris Mox has exposed directly UART pins and you are free to connect > > any USB<->UART cable to these pins as you want. > > > > And this design with soldered USB<->UART chip on the board is not rare. > > So U-Boot should have better support for it. > > > > Note that lot of times clock source for these USB<->UART chips and SoC > > UART chip is different (because these USB based chips have its own > > clock source). So there are less common supported baudrates which can be > > used on the boards. And when talking about high speed baudrates (above > > 115200 bauds) then lot of times these "common supported baudrates" are > > just non-standard baudrate numbers (e.g. 3 125 000 bauds). Because you > > have to choose divisors both both chips to produce baudrate with only > > about 2-3% difference. > > > > > Or per SoC / UART chipset. I would hope for example that for plain old > > > ns16550s this would be a generic function, perhaps with an optional > > > board call-out for board design limitations. This does feel like a > > > reasonable amount of code for platforms like this that were supporting > > > what was the maximal rate table before. > > > > > > -- > > > Tom
diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c index fdc873b1952f..0cd9ea6a1515 100644 --- a/board/Marvell/mvebu_armada-37xx/board.c +++ b/board/Marvell/mvebu_armada-37xx/board.c @@ -435,3 +435,31 @@ int ft_board_setup(void *blob, struct bd_info *bd) return 0; } #endif + +int board_round_baudrate(int baudrate) +{ + /* + * Espressobin has on-board pl2303 connected to A3720 UART. + * So calculate the final real baudrate supported by pl2303. + * Code from linux kernel function pl2303_encode_baud_rate_divisor() + * Exact formula is: baudrate = 12M * 32 / (mantissa * 4^exponent) + */ + unsigned int baseline, mantissa, exponent; + + baseline = 12000000 * 32; + mantissa = baseline / baudrate; + if (mantissa == 0) + mantissa = 1; + exponent = 0; + while (mantissa >= 512) { + if (exponent < 7) { + mantissa >>= 2; + exponent++; + } else { + mantissa = 511; + break; + } + } + + return (baseline / mantissa) >> (exponent << 1); +} diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 8171b17faf88..0d725ed764e6 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -299,6 +299,20 @@ int serial_tstc(void) return _serial_tstc(gd->cur_serial_dev); } +int serial_round_baudrate(int baudrate) +{ + struct dm_serial_ops *ops; + + if (!gd->cur_serial_dev) + return 0; + + ops = serial_get_ops(gd->cur_serial_dev); + if (!ops->round_baudrate) + return 0; + + return ops->round_baudrate(gd->cur_serial_dev, baudrate); +} + void serial_setbrg(void) { struct dm_serial_ops *ops; @@ -378,6 +392,11 @@ static int serial_stub_tstc(struct stdio_dev *sdev) } #endif +int __weak board_round_baudrate(int baudrate) +{ + return 0; +} + /** * on_baudrate() - Update the actual baudrate when the env var changes * @@ -388,6 +407,8 @@ static int on_baudrate(const char *name, const char *value, enum env_op op, { int i; int baudrate; + int real_baudrate; + int board_baudrate; switch (op) { case env_op_create: @@ -397,20 +418,50 @@ static int on_baudrate(const char *name, const char *value, enum env_op op, */ baudrate = dectoul(value, NULL); - /* Not actually changing */ - if (gd->baudrate == baudrate) - return 0; + real_baudrate = serial_round_baudrate(baudrate); - for (i = 0; i < ARRAY_SIZE(baudrate_table); ++i) { - if (baudrate == baudrate_table[i]) - break; - } - if (i == ARRAY_SIZE(baudrate_table)) { - if ((flags & H_FORCE) == 0) + if (real_baudrate) { + /* Baudrate is supported if is within 3% tolerance */ + if (100 * real_baudrate < baudrate * (100 - 3) || + 100 * real_baudrate > baudrate * (100 + 3)) { printf("## Baudrate %d bps not supported\n", baudrate); - return 1; + return 1; + } + baudrate = real_baudrate; + } else { + /* + * If round_baudrate() callback is unsupported then + * check supported baudrate against baudrate_table[] + */ + for (i = 0; i < ARRAY_SIZE(baudrate_table); ++i) { + if (baudrate == baudrate_table[i]) + break; + } + if (i == ARRAY_SIZE(baudrate_table)) { + if ((flags & H_FORCE) == 0) + printf("## Baudrate %d bps not supported\n", + baudrate); + return 1; + } + } + + board_baudrate = board_round_baudrate(baudrate); + + if (board_baudrate) { + /* Baudrate is supported if is within 3% tolerance */ + if (100 * board_baudrate < baudrate * (100 - 3) || + 100 * board_baudrate > baudrate * (100 + 3)) { + printf("## Baudrate %d bps not supported by the board\n", + baudrate); + return 1; + } } + + /* Not actually changing */ + if (gd->baudrate == baudrate) + return 0; + if ((flags & H_INTERACTIVE) != 0) { printf("## Switch baudrate to %d bps and press ENTER ...\n", baudrate); diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index 8dc4e5880e6b..d8bf3e97c486 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -147,6 +147,44 @@ static int mvebu_serial_setbrg(struct udevice *dev, int baudrate) return 0; } +static int mvebu_serial_round_baudrate(struct udevice *dev, int baudrate) +{ + struct mvebu_plat *plat = dev_get_plat(dev); + u32 divider, d1, d2, m; + + m = 16; + d1 = d2 = 1; + divider = DIV_ROUND_CLOSEST(plat->tbg_rate, baudrate * d1 * d2 * m); + + if (divider < 1) + divider = 1; + else if (divider > 1023) { + d1 = 6; + divider = DIV_ROUND_CLOSEST(plat->tbg_rate, + baudrate * d1 * d2 * m); + if (divider < 1) + divider = 1; + else if (divider > 1023) { + d2 = 6; + divider = DIV_ROUND_CLOSEST(plat->tbg_rate, + baudrate * d1 * d2 * m); + if (divider < 1) + divider = 1; + else if (divider > 1023) { + m = 63; + divider = DIV_ROUND_CLOSEST(plat->tbg_rate, + baudrate * d1 * d2 * m); + if (divider < 1) + divider = 1; + else if (divider > 1023) + divider = 1023; + } + } + } + + return DIV_ROUND_CLOSEST(plat->tbg_rate, d1 * d2 * m * divider); +} + static int mvebu_serial_probe(struct udevice *dev) { struct mvebu_plat *plat = dev_get_plat(dev); @@ -335,6 +373,7 @@ static const struct dm_serial_ops mvebu_serial_ops = { .pending = mvebu_serial_pending, .getc = mvebu_serial_getc, .setbrg = mvebu_serial_setbrg, + .round_baudrate = mvebu_serial_round_baudrate, }; static const struct udevice_id mvebu_serial_ids[] = { diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h index c8c34d7d92dd..6f7279f2ae4a 100644 --- a/include/configs/mvebu_armada-37xx.h +++ b/include/configs/mvebu_armada-37xx.h @@ -17,14 +17,6 @@ #define CONFIG_SYS_BOOTM_LEN SZ_64M /* Increase max gunzip size */ -#define CONFIG_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 } - /* * For booting Linux, the board info and command line data * have to be in the first 8 MB of memory, since this is diff --git a/include/configs/turris_mox.h b/include/configs/turris_mox.h index 671283982356..03813613e193 100644 --- a/include/configs/turris_mox.h +++ b/include/configs/turris_mox.h @@ -22,14 +22,6 @@ /* auto boot */ -#define CONFIG_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 } - /* * For booting Linux, the board info and command line data * have to be in the first 8 MB of memory, since this is diff --git a/include/serial.h b/include/serial.h index 6d1e62c6770c..5e8a7501b9a6 100644 --- a/include/serial.h +++ b/include/serial.h @@ -176,6 +176,30 @@ struct dm_serial_ops { * @return 0 if OK, -ve on error */ int (*setbrg)(struct udevice *dev, int baudrate); + /** + * round_baudrate() - Return the nearest available baudrate + * + * Return the exact baudrate value which would be set by setbrg() + * if is called with this @baudrate argument. So this function + * should return the nearest available baudrate. + * + * This function must not change baudrate generator. Its purpose + * is just to test if particular baudrate value is supported + * and to calculate baudrate tolerance. + * + * Caller may specify number 0 to retrieve the smallest possible + * supported baudrate and INT_MAX number to retrive the highest + * possible supported baudrate. + * + * Returning zero value is same as if this function is not + * implemented at all. Meaning that driver cannot predict what is + * the real final baudrate value. + * + * @dev: Device pointer + * @baudrate: Baudrate to round + * @return real baudrate value (see above) + */ + int (*round_baudrate)(struct udevice *dev, int baudrate); /** * getc() - Read a character and return it *
Add new functions which returns the nearest baudrate and use them instead of hardcoded and incomplete CONFIG_SYS_BAUDRATE_TABLE compile time option. Add implementation of rounding function for serial_mvebu_a3700 driver and also for A3720 Espressobin board which has integrated pl2303 USB<->UART converter, which basically limits baudrates which can user set. Completely remove CONFIG_SYS_BAUDRATE_TABLE defines from all A3720 boards as now with rounding functions it is not used anymore. NOTE: This is just an example how to kill CONFIG_SYS_BAUDRATE_TABLE compile time definitions. I tested it that it works on A3720 Turris Mox board. I have not tested A3720 Espressobin board yet. More discussion on this approach is required, so take this just as RFC change. Signed-off-by: Pali Rohár <pali@kernel.org> --- board/Marvell/mvebu_armada-37xx/board.c | 28 ++++++++++ drivers/serial/serial-uclass.c | 71 +++++++++++++++++++++---- drivers/serial/serial_mvebu_a3700.c | 39 ++++++++++++++ include/configs/mvebu_armada-37xx.h | 8 --- include/configs/turris_mox.h | 8 --- include/serial.h | 24 +++++++++ 6 files changed, 152 insertions(+), 26 deletions(-)