[PATCH v2 26/28] serial: dm: Add support for puts

[Simon Glass]

Hi Sean,

On Fri, 11 Mar 2022 at 22:53, Sean Anderson <seanga2 at 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.anderson at 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.anderson at 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