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

[Vabhav Sharma (OSS)]

Hi Simon, Stefan,
Thank you for feedback and time

> -----Original Message-----
> From: Simon Glass <sjg at chromium.org>
> Sent: Thursday, October 15, 2020 8:35 PM
> To: Stefan Roese <sr at denx.de>
> Cc: Vabhav Sharma (OSS) <vabhav.sharma at oss.nxp.com>;
> andre.przywara at arm.com; u-boot at lists.denx.de
> Subject: Re: [PATCH v2] drivers: serial: probe all uart devices
> 
> Hi,
> 
> On Wed, 14 Oct 2020 at 06:47, Stefan Roese <sr at 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 at denx.de>
> > >> Sent: Wednesday, September 30, 2020 10:46 AM
> > >> To: Vabhav Sharma (OSS) <vabhav.sharma at oss.nxp.com>;
> > >> andre.przywara at arm.com; u-boot at lists.denx.de; sjg at chromium.org
> > >> Cc: Vabhav Sharma <vabhav.sharma at 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 at 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 at 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.
Sure, I understand.
> 
> Regards,
> Simon