Re: [PATCH 5/7] ns8250: Use ACPI SPCR table when available to configure serial

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:24AM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt <benh@amazon.com>
>
> "serial auto" is now equivalent to just "serial" and will use the
> SPCR to discover the port if present, otherwise defaults to "com0"
> as before.
>
> This allows to support MMIO ports specified by ACPI which is needed
> on AWS EC2 "metal" instances, and will enable grub to pickup the

s/grub/GRUB/

> port configuration specified by ACPI in other cases.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  docs/grub.texi               | 10 ++++-
>  grub-core/Makefile.core.def  |  1 +
>  grub-core/term/ns8250-spcr.c | 82 ++++++++++++++++++++++++++++++++++++
>  grub-core/term/serial.c      | 13 +++++-
>  include/grub/serial.h        |  1 +
>  5 files changed, 105 insertions(+), 2 deletions(-)
>  create mode 100644 grub-core/term/ns8250-spcr.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 50c811a88..bbebc2aef 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -2719,6 +2719,10 @@ you want to use COM2, you must specify @samp{--unit=1} 
> instead. This
>  command accepts many other options, so please refer to @ref{serial},
>  for more details.
>
> +Without argument or with @samp{--port=auto}, GRUB will attempt to use
> +ACPI when available to auto-detect the default serial port and its
> +configuration.
> +
>  The commands @command{terminal_input} (@pxref{terminal_input}) and
>  @command{terminal_output} (@pxref{terminal_output}) choose which type of
>  terminal you want to use. In the case above, the terminal will be a
> @@ -2737,7 +2741,6 @@ implements few VT100 escape sequences. If you specify 
> this option then
>  GRUB provides you with an alternative menu interface, because the normal
>  menu requires several fancy features of your terminal.
>
> -

Please drop this change.

>  @node Vendor power-on keys
>  @chapter Using GRUB with vendor power-on keys
>
> @@ -4172,6 +4175,11 @@ be in the range 5-8 and stop bits must be 1 or 2. 
> Default is 8 data
>  bits and one stop bit. @var{parity} is one of @samp{no}, @samp{odd},
>  @samp{even} and defaults to @samp{no}.
>
> +In passed no @var{unit} nor @var{port}, or if @var{port} is set to

s/In/If/?

> +@samp{auto} then GRUB will attempt to use ACPI to automatically detect
> +the system default serial port and its configuration. If this information
> +is not available, it will default to @var{unit} 0.
> +
>  The serial port is not used as a communication channel unless the
>  @command{terminal_input} or @command{terminal_output} command is used
>  (@pxref{terminal_input}, @pxref{terminal_output}).
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 95942fc8c..b656bdb44 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2048,6 +2048,7 @@ module = {
>    name = serial;
>    common = term/serial.c;
>    x86 = term/ns8250.c;
> +  x86 = term/ns8250-spcr.c;
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> new file mode 100644
> index 000000000..0786aee1b
> --- /dev/null
> +++ b/grub-core/term/ns8250-spcr.c
> @@ -0,0 +1,82 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2000,2001,2002,2003,2004,2005,2007,2008,2009,2010  Free 
> Software Foundation, Inc.

s/2000,2001,2002,2003,2004,2005,2007,2008,2009,2010/2022/

> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/ns8250.h>
> +#include <grub/types.h>
> +#include <grub/dl.h>
> +#include <grub/acpi.h>
> +
> +char *
> +grub_ns8250_spcr_init (void)
> +{
> +  struct grub_acpi_spcr *spcr;
> +  struct grub_serial_config config;
> +
> +  spcr = grub_acpi_find_table (GRUB_ACPI_SPCR_SIGNATURE);
> +  if (!spcr)

spcr == NULL

> +    return 0;

Please use NULL instead of 0 in pointer context. Please fix same
problem in the other places/patches too.

> +  if (spcr->hdr.revision < 2)
> +    return 0;
> +  if (spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550 &&
> +      spcr->intf_type != GRUB_ACPI_SPCR_INTF_TYPE_16550X)
> +    return 0;
> +  /* For now, we only support byte accesses */
> +  if (spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_BYTE &&
> +      spcr->base_addr.access_size != GRUB_ACPI_GENADDR_SIZE_LGCY)
> +    return 0;
> +  config.word_len = 8;
> +  config.parity = GRUB_SERIAL_PARITY_NONE;
> +  config.stop_bits = GRUB_SERIAL_STOP_BITS_1;
> +  config.base_clock = 1843200;

Where this number come from? Please define a constant or at least put
a comment before this line.

> +  if (spcr->flow_control & GRUB_ACPI_SPCR_FC_RTSCTS)
> +    config.rtscts = 1;
> +  else
> +    config.rtscts = 0;
> +  switch (spcr->baud_rate)
> +    {
> +      case GRUB_ACPI_SPCR_BAUD_9600:
> +        config.speed = 9600;
> +        break;
> +      case GRUB_ACPI_SPCR_BAUD_19200:
> +        config.speed = 19200;
> +        break;
> +      case GRUB_ACPI_SPCR_BAUD_57600:
> +        config.speed = 57600;
> +        break;
> +      case GRUB_ACPI_SPCR_BAUD_115200:
> +        config.speed = 115200;
> +        break;
> +      case GRUB_ACPI_SPCR_BAUD_CURRENT:
> +      default:
> +       /* We don't (yet) have a way to read the currently
> +        * configured speed in HW, so let's use a sane default
> +        */

Please align the comment with GRUB coding style [1].

> +        config.speed = 115200;
> +        break;
> +    };
> +  switch (spcr->base_addr.space_id)
> +    {
> +      case GRUB_ACPI_GENADDR_MEM_SPACE:
> +        return grub_serial_ns8250_add_mmio(spcr->base_addr.addr, &config);
> +      case GRUB_ACPI_GENADDR_IO_SPACE:
> +        return grub_serial_ns8250_add_port(spcr->base_addr.addr, &config);
> +      default:
> +        return 0;
> +    };
> +}
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index a1707d2f7..f57fe45ef 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -160,6 +160,17 @@ grub_serial_find (const char *name)
>        if (!name)
>       return NULL;
>
> +      FOR_SERIAL_PORTS (port)
> +     if (grub_strcmp (port->name, name) == 0)
> +       break;
> +    }
> +  if (!port && grub_strcmp (name, "auto") == 0)
> +    {
> +      /* Look for an SPCR if any. If not, default to com0 */
> +      name = grub_ns8250_spcr_init();

Missing space before "(".

> +      if (!name)
> +        name = "com0";
> +
>        FOR_SERIAL_PORTS (port)
>       if (grub_strcmp (port->name, name) == 0)
>         break;
> @@ -214,7 +225,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, 
> char **args)
>      name = args[0];
>
>    if (!name)
> -    name = "com0";
> +    name = "auto";
>
>    port = grub_serial_find (name);
>    if (!port)
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index 7efc956b0..646bdf1bb 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -185,6 +185,7 @@ grub_serial_config_defaults (struct grub_serial_port 
> *port)
>
>  #if defined(__mips__) || defined (__i386__) || defined (__x86_64__)
>  void grub_ns8250_init (void);
> +char * grub_ns8250_spcr_init (void);

Please drop space after "*".

>  char *grub_serial_ns8250_add_port (grub_port_t port, struct 
> grub_serial_config *config);
>  char *grub_serial_ns8250_add_mmio(grub_addr_t addr, struct 
> grub_serial_config *config);
>  #endif

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments