Hi Sean, On Fri, 11 Mar 2022 at 22:53, Sean Anderson <sean...@gmail.com> wrote: > > On 3/12/22 12:02 AM, Simon Glass wrote: > > Hi Sean, > > > > On Thu, 10 Mar 2022 at 13:51, Sean Anderson <sean.ander...@seco.com> wrote: > >> > >> Some serial drivers can be vastly more efficient when printing multiple > >> characters at once. Non-DM serial has had a puts option for these sorts > >> of drivers; implement it for DM serial as well. > >> > >> Because we have to add carriage returns, we can't just pass the whole > >> string directly to the serial driver. Instead, we print up to the > >> newline, then print a carriage return, and then continue on. This is > >> less efficient, but it is better than printing each character > >> individually. It also avoids having to allocate memory just to add a few > >> characters. > >> > >> Drivers may perform short writes (such as filling a FIFO) and return the > >> number of characters written in len. We loop over them in the same way > >> that _serial_putc loops over putc. > >> > >> This results in around 148 bytes of bloat for all boards with DM_SERIAL > >> enabled: > > > > So let's put it behind a Kconfig, particularly for SPL. > > I've added a config for this for v3. > > >> > >> vexpress_aemv8a_juno: all +148 rodata +8 text +140 > >> u-boot: add: 2/0, grow: 0/-2 bytes: 232/-92 (140) > >> function old new delta > >> _serial_puts - 200 +200 > >> strchrnul - 32 +32 > >> serial_puts 68 24 -44 > >> serial_stub_puts 56 8 -48 > >> > >> Signed-off-by: Sean Anderson <sean.ander...@seco.com> > >> --- > >> > >> Changes in v2: > >> - New > >> > >> drivers/serial/serial-uclass.c | 27 +++++++++++++++++++++++++-- > >> include/serial.h | 18 ++++++++++++++++++ > >> 2 files changed, 43 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/serial/serial-uclass.c > >> b/drivers/serial/serial-uclass.c > >> index 362cedd955..352ad986f7 100644 > >> --- a/drivers/serial/serial-uclass.c > >> +++ b/drivers/serial/serial-uclass.c > >> @@ -199,8 +199,31 @@ static void _serial_putc(struct udevice *dev, char ch) > >> > >> static void _serial_puts(struct udevice *dev, const char *str) > >> { > >> - while (*str) > >> - _serial_putc(dev, *str++); > >> + struct dm_serial_ops *ops = serial_get_ops(dev); > >> + > >> + if (!ops->puts) { > >> + while (*str) > >> + _serial_putc(dev, *str++); > >> + } > >> + > >> + do { > >> + const char *newline = strchrnul(str, '\n'); > >> + size_t len = newline - str + !!*newline; > >> + > >> + do { > >> + int err; > >> + size_t written = len; > >> + > >> + err = ops->puts(dev, str, &written); > >> + if (err && err != -EAGAIN) > >> + return; > >> + str += written; > >> + len -= written; > >> + } while (len); > >> + > >> + if (*newline) > >> + _serial_putc(dev, '\r'); > >> + } while (*str); > >> } > >> > >> static int __serial_getc(struct udevice *dev) > >> diff --git a/include/serial.h b/include/serial.h > >> index 2681d26c82..ea96d904d8 100644 > >> --- a/include/serial.h > >> +++ b/include/serial.h > >> @@ -195,6 +195,24 @@ struct dm_serial_ops { > >> * @return 0 if OK, -ve on error > >> */ > >> int (*putc)(struct udevice *dev, const char ch); > >> + /** > >> + * puts() - Write a string > >> + * > >> + * This writes a string. This function should be implemented only > >> if > >> + * writing multiple characters at once is more performant than just > >> + * calling putc() in a loop. > >> + * > >> + * If the whole string cannot be written at once, then @len should > >> be > >> + * set to the number of characters written, and this function > >> should > >> + * return -EAGAIN. > >> + * > >> + * @dev: Device pointer > >> + * @s: The string to write > >> + * @len: The length of the string to write. This should be set to > >> the > >> + * number of characters actually written on return. > > > > How about returning the number of characters written? That is more > > like the posix write() function and saves an arg. > > OK, how about positive return is bytes written and negative error.
SGTM > > >> + * @return 0 if OK, -ve on error > >> + */ > >> + int (*puts)(struct udevice *dev, const char *s, size_t *len); > >> /** > >> * pending() - Check if input/output characters are waiting > >> * > >> -- > >> 2.25.1 > >> > > > > Is it possible to add a test to test/dm/serial.c ? > > I can have a look, but note that there is no test for putc/getc/etc. If > putc/puts is broken, then the console output will be missing/garbled. This > also happens after console recording IIRC, so I think we would need a > second buffer in sandbox_serial... Yes that's true. We do have console recording now, but that is at a higher level. There is a membuff in the sandbox serial driver which I suspect could be used here, but it would need some tweaking. Perhaps to keep it simple the sandbox driver could just keep a count of the number of characters output, that a test could check? Regards, Simon