Thank you for quick turn around!
Now patches looks much better but...
On Thu, Dec 22, 2022 at 05:12:57PM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt <b...@amazon.com>
>
> This adds the ability to explicitely add an MMIO based serial port
> via the 'serial' command. The syntax is:
>
> serial --port=mmio,<hex_address>{.b,.w,.l,.q}
>
> Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> ---
> docs/grub.texi | 26 ++++++++++++++++++--
> grub-core/term/serial.c | 53 +++++++++++++++++++++++++++++++++++++----
> 2 files changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index fd22a69fa..502ca2ef7 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4168,8 +4168,24 @@ Commands usable anywhere in the menu and in the
> command-line.
> @deffn Command serial [@option{--unit=unit}] [@option{--port=port}]
> [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}]
> [@option{--stop=stop}]
> Initialize a serial device. @var{unit} is a number in the range 0-3
> specifying which serial port to use; default is 0, which corresponds to
> -the port often called COM1. @var{port} is the I/O port where the UART
> -is to be found; if specified it takes precedence over @var{unit}.
> +the port often called COM1.
> +
> +@var{port} is the I/O port where the UART is to be found or, if prefixed
> +with @samp{mmio,}, the MMIO address of the UART. If specified it takes
> +precedence over @var{unit}.
> +
> +Additionally, an MMIO address can be suffixed with:
> +@itemize @bullet
> +@item
> +@samp{.b} for bytes access (default)
> +@item
> +@samp{.w} for 16-bit word access
> +@item
> +@samp{.l} for 32-bit long word access or
> +@item
> +@samp{.q} for 64-bit long long word access
> +@end itemize
> +
> @var{speed} is the transmission speed; default is 9600. @var{word} and
> @var{stop} are the number of data bits and stop bits. Data bits must
> be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
> @@ -4185,6 +4201,12 @@ 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}).
>
> +Examples:
> +@example
> +serial --port=3f8 --speed=9600
> +serial --port=mmio,fefb0000.l --speed=115200
> +@end example
> +
> See also @ref{Serial terminal}.
> @end deffn
>
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index ed7b398b7..d374495cb 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -152,8 +152,8 @@ grub_serial_find (const char *name)
> break;
>
> #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) &&
> !defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
> - if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
> - && grub_isxdigit (name [sizeof ("port") - 1]))
> + if (!port && grub_strncmp (name, "port", grub_strlen ("port")) == 0
Maybe I was a bit imprecise but I was thinking about converting
grub_memcmp() to grub_strncmp() in your patches/changes only.
Sorry about that.
If you want still to do that I would prefer if you do this in separate
patch. And please do not change "sizeof () - 1" to "grub_strlen ()".
In the worst case I am OK with a sentence in the commit message saying
you change grub_memcmp() to grub_strncmp() on the occasion. Otherwise it
is confusing to see such changes in the patch. Though separate patch
is preferred way to do this...
And please replace grub_memcmp() with grub_strncmp() and 4 with
'sizeof ("mmio") - 1' in patch #3.
> + && grub_isxdigit (name [grub_strlen ("port")]))
> {
> name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof
> ("port") - 1],
> 0, 16), NULL);
> @@ -164,6 +164,49 @@ grub_serial_find (const char *name)
> if (grub_strcmp (port->name, name) == 0)
> break;
> }
> + if (!port && grub_strncmp (name, "mmio,", grub_strlen ("mmio,")) == 0
> + && grub_isxdigit (name [grub_strlen ("mmio,")]))
> + {
> + const char *p1, *p = &name[grub_strlen ("mmio,")];
s/grub_strlen ("mmio,")/sizeof ("mmio,") - 1/
> + grub_addr_t addr = grub_strtoul (p, &p1, 16);
> + unsigned int acc_size = 1;
> + unsigned int nlen = p1 - p;
> +
> + /* Check address validity and general syntax */
> + if (addr == 0 || (p1[0] != '\0' && p1[0] != '.'))
This condition looks wrong. I think it should be
if (*p == '\0' || (*p1 != '.' && *p1 != '\0') || addr == 0)
I think the most important thing is lack of "*p == '\0'". Am I right?
And I changed a logic a bit to make it more readable...
Additionally, maybe we should print an error here to give a hint to
a user what is wrong. The grub_error(..., N_()) is your friend...
> + return NULL;
> + if (p1[0] == '.')
> + switch(p1[1])
> + {
> + case 'w':
> + acc_size = 2;
> + break;
> + case 'l':
> + acc_size = 3;
> + break;
> + case 'q':
> + acc_size = 4;
> + break;
> + case 'b':
> + acc_size = 1;
> + /* fallthrough */
> + default:
> + /*
> + * Should we abort for an unknown size ? Let's just default
> + * to 1 byte, it would increase the chance that the user who
> + * did a typo can actually see the console.
> + */
> + acc_size = 1;
> + }
> + name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
> + if (!name)
> + return NULL;
> +
> + FOR_SERIAL_PORTS (port)
> + if (grub_strncmp (port->name, name, nlen) == 0) {
> + break;
> + }
> + }
> if (!port && grub_strcmp (name, "auto") == 0)
> {
> /* Look for an SPCR if any. If not, default to com0 */
> @@ -178,9 +221,9 @@ grub_serial_find (const char *name)
> #endif
>
> #ifdef GRUB_MACHINE_IEEE1275
> - if (!port && grub_memcmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) ==
> 0)
> + if (!port && grub_strncmp (name, "ieee1275/", grub_strlen ("ieee1275/"))
> == 0)
Ditto.
> {
> - name = grub_ofserial_add_port (&name[sizeof ("ieee1275/") - 1]);
> + name = grub_ofserial_add_port (&name[grub_strlen ("ieee1275/")]);
> if (!name)
> return NULL;
>
> @@ -212,7 +255,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc,
> char **args)
>
> if (state[OPTION_PORT].set)
> {
> - if (grub_memcmp (state[OPTION_PORT].arg, "mmio", 4) == 0)
> + if (grub_strncmp (state[OPTION_PORT].arg, "mmio,", grub_strlen
> ("mmio,")) == 0)
Ditto.
And if you fix this please fix coding style in
grub_le_to_cpu*()/grub_cpu_to_le*()
calls in patch #7 too.
I hope I did not miss other issues this time...
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
