[PATCH v2] drivers: serial: probe all uart devices

[Simon Glass <sjg@chromium.org>]

Hi,

On Wed, 14 Oct 2020 at 06:47, Stefan Roese <sr@denx.de> wrote:
>
> Hi Vabhav,
>
> On 14.10.20 13:15, Vabhav Sharma (OSS) wrote:
> > Hi Stefan,
> > Sorry for delayed reply, Occupied with high priority task
> >
> >> -----Original Message-----
> >> From: Stefan Roese <sr@denx.de>
> >> Sent: Wednesday, September 30, 2020 10:46 AM
> >> To: Vabhav Sharma (OSS) <vabhav.sharma@oss.nxp.com>;
> >> andre.przywara at arm.com; u-boot at lists.denx.de; sjg at chromium.org
> >> Cc: Vabhav Sharma <vabhav.sharma@nxp.com>
> >> Subject: Re: [PATCH v2] drivers: serial: probe all uart devices
> >>
> >> On 29.09.20 19:26, Vabhav Sharma wrote:
> >>> From: Vabhav Sharma <vabhav.sharma@nxp.com>
> >>>
> >>> U-Boot DM model probe only single device at a time which is enabled
> >>> and configured using device tree or platform data method.
> >>>
> >>> PL011 UART IP is SBSA compliant and firmware does the serial port
> >>> set-up, initialization and let the kernel use UART port for sending
> >>> and receiving characters.
> >>>
> >>> Normally software talk to one serial port time but some LayerScape
> >>> platform require all the UART devices enabled in Linux for various use
> >>> case.
> >>>
> >>> Adding support to probe all enabled serial devices like SBSA compliant
> >>> PL011 UART ports probe and initialization by firmware.
> >>>
> >>> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> >>> ---
> >>> v2:
> >>>      Incorporated Stefan review comment, Update #ifdef with macro
> >>>      if (IS_ENABLED)..
> >>
> >> Better, thanks. But...
> >>
> >>> ---
> >>> ---
> >>>    drivers/serial/Kconfig         | 17 +++++++++++++++++
> >>>    drivers/serial/serial-uclass.c | 30 ++++++++++++++++++++++++++++++
> >>>    2 files changed, 47 insertions(+)
> >>>
> >>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index
> >>> e344677..b2e30f1 100644
> >>> --- a/drivers/serial/Kconfig
> >>> +++ b/drivers/serial/Kconfig
> >>> @@ -134,6 +134,23 @@ config SERIAL_SEARCH_ALL
> >>>
> >>>       If unsure, say N.
> >>>
> >>> +config SERIAL_PROBE_ALL
> >>> +   bool "Probe all available serial devices"
> >>> +   depends on DM_SERIAL
> >>> +   default n
> >>> +   help
> >>> +     The serial subsystem only probe for single serial device,
> >>> +     but does not probe for other remaining serial devices.
> >>> +     With this option set,we make probing and searching for
> >>> +     all available devices optional.
> >>> +     Normally, U-Boot talk to one serial port at a time but SBSA
> >>> +     compliant UART devices like PL011 require initialization
> >>> +     by firmware and let the kernel use serial port for sending
> >>> +     and receiving the characters.
> >>> +
> >>> +     If probing is not required for all remaining available
> >>> +     devices other than default current console device, say N.
> >>> +
> >>>    config SPL_DM_SERIAL
> >>>     bool "Enable Driver Model for serial drivers in SPL"
> >>>     depends on DM_SERIAL && SPL_DM
> >>> diff --git a/drivers/serial/serial-uclass.c
> >>> b/drivers/serial/serial-uclass.c index 0027625..d303a66 100644
> >>> --- a/drivers/serial/serial-uclass.c
> >>> +++ b/drivers/serial/serial-uclass.c
> >>> @@ -86,6 +86,11 @@ static void serial_find_console_or_panic(void)
> >>>             uclass_first_device(UCLASS_SERIAL, &dev);
> >>>             if (dev) {
> >>>                     gd->cur_serial_dev = dev;
> >>> +                   if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL)) {
> >>> +                           /* Scanning uclass to probe all devices */
> >>> +                           for (; dev; uclass_next_device(&dev))
> >>> +                                   ;
> >>> +                   }
> >>>                     return;
> >>>             }
> >>>     } else if (CONFIG_IS_ENABLED(OF_CONTROL) && blob) { @@ -96,11
> >>> +101,21 @@ static void serial_find_console_or_panic(void)
> >>>                     if (np
> >> && !uclass_get_device_by_ofnode(UCLASS_SERIAL,
> >>>                                     np_to_ofnode(np), &dev)) {
> >>>                             gd->cur_serial_dev = dev;
> >>> +                           if (IS_ENABLED(CONFIG_SERIAL_PROBE_ALL))
> >> {
> >>> +                                   /* Scanning uclass to probe devices
> >> */
> >>> +                                   for (; dev; uclass_next_device(&dev))
> >>> +                                           ;
> >>> +                           }
> >>
> >> This code sequence (incl. gd->cur_serial_dev = dev;) is repeated multiple
> >> times (below as well). Could you instead move this into a function and call
> >> this function to reduce code and binary size?
> > I understand.
> > Checked and found that 56 bytes of code is added (including enabling
> > the macro on LS platform)
> >
> > Intended to define it earlier but defining one line of code in a
> > function cause more overhead (function call, Stack operations,
> > Pointer arithmetic) in execution time.
>
> So you did measure a penalty in bootup time with this code moved
> into a function? I would have thought that this is neglectable.
>
> > Tradeoff is to choose between Function overhead Vs Binary Size, What
> > is your suggestion
>
> My vote is for the function. Mainly not because of the smaller image
> size, but because of the smaller source code base, being easier to
> maintain (IMHO).

Same for me.

Regards,
Simon