[coreboot] Re: [PATCH v3 2/2] x86: Update cbmem driver

[Bin Meng]

Hi Simon,

On Thu, Aug 17, 2023 at 9:58 AM Simon Glass  wrote:
> This driver is not actually built since a Kconfig was never created for
> it.
>
> Add a Kconfig (which is already implied by COREBOOT) and update the
> implementation to avoid using unnecessary memory. Drop the #ifdef at the
> top since we can rely on Kconfig to get that right.
>
> To enable it (in addition to serial and video), use:
>
> setenv stdout serial,vidconsole,cbmem
> Signed-off-by: Simon Glass
>
> (no changes since v2)
>
> Changes in v2:
> Update to support the new overflow mechanism
>
> drivers/misc/Kconfig         |  8 ++++++++
>  drivers/misc/cbmem_console.c | 38 +++++++++++++++++++-----------------
>  2 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a6e3f62ecb09..c930e4a361bf 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -122,6 +122,14 @@ config VEXPRESS_CONFIG
>           configuration bus on the Arm Versatile Express boards via
>           a sysreg driver.
>
> +config CBMEM_CONSOLE
>   bool "Write console output to coreboot cbmem"
>   depends on X86
>   help
>     Enables console output to the cbmem console, which is a memory
>     region set up by coreboot to hold a record of all console output.
>     Enable this only if booting from coreboot.
>
> config CMD_CROS_EC
>         bool "Enable crosec command"
>         depends on CROS_EC
> diff --git a/drivers/misc/cbmem_console.c b/drivers/misc/cbmem_console.c
> index 8bbe33d414da..7a47a814f638 100644
> --- a/drivers/misc/cbmem_console.c
> +++ b/drivers/misc/cbmem_console.c
> @@ -5,27 +5,30 @@
>
> #include <common.h>
>  #include <console.h>
> -#ifndef CONFIG_SYS_COREBOOT
> -#error This driver requires coreboot
> -#endif
>
> #include <asm/cb_sysinfo.h>
>
> -struct cbmem_console {
>   u32 buffer_size;
>   u32 buffer_cursor;
>   u8  buffer_body[0];
>
> -}  __attribute__ ((__packed__));
>
> -static struct cbmem_console *cbmem_console_p;
>
> void cbmemc_putc(struct stdio_dev *dev, char data)
>  {
>   int cursor;
>   cursor = cbmem_console_p->buffer_cursor++;
>   if (cursor < cbmem_console_p->buffer_size)
>           cbmem_console_p->buffer_body[cursor] = data;
>   const struct sysinfo_t *sysinfo = cb_get_sysinfo();
>   struct cbmem_console *cons;
>   uint pos, flags;
>   if (!sysinfo)
>           return;
>   cons = sysinfo->cbmem_cons;
>   if (!cons)
>           return;
>   pos = cons->cursor & CBMC_CURSOR_MASK;
>   flags = cons->cursor & ~CBMC_CURSOR_MASK;

This line does not seem useful to me, as we don't use the extracted
flag from the higher bits of cons->cursor.

I guess we can just drop it.
>   cons->body[pos++] = data;
>   if (pos >= cons->size) {
>           pos = 0;
>           flags |= CBMC_OVERFLOW;

Who is supposed to clear the overflow flag?
>   }
>   cons->cursor = flags | pos;
>
> }
>
> void cbmemc_puts(struct stdio_dev *dev, const char *str)
> @@ -40,7 +43,6 @@ int cbmemc_init(void)
>  {
>         int rc;
>         struct stdio_dev cons_dev;
>   cbmem_console_p = lib_sysinfo.cbmem_cons;
>
>   memset(&cons_dev, 0, sizeof(cons_dev));

Regards,
Bin