| Message ID | 20220310205059.499269-27-sean.anderson@seco.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | arm: semihosting: Cleanups and new features | expand |
On 3/10/22 3:50 PM, Sean Anderson 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: > > 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@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++); This is missing an early return. Will be fixed in v3. --Sean > + } > + > + 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. > + * @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 > * >
Hi Sean, On Thu, 10 Mar 2022 at 13:51, Sean Anderson <sean.anderson@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. > > 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@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. > + * @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 ? Regards, Simon
On 3/12/22 12:02 AM, Simon Glass wrote: > Hi Sean, > > On Thu, 10 Mar 2022 at 13:51, Sean Anderson <sean.anderson@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@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. >> + * @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... --Sean
Hi Sean, On Fri, 11 Mar 2022 at 22:53, Sean Anderson <seanga2@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@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@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
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. + * @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 *
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: 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@seco.com> --- Changes in v2: - New drivers/serial/serial-uclass.c | 27 +++++++++++++++++++++++++-- include/serial.h | 18 ++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-)