[PATCH v4 2/2] x86: Update cbmem driver

[Bin Meng]

Hi Simon,

On Wed, Sep 20, 2023 at 10:59 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 19 Sept 2023 at 02:47, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Sep 11, 2023 at 3:13 AM Simon Glass <sjg at chromium.org> 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 <sjg at chromium.org>
> > > ---
> > >
> > > Changes in v4:
> > > - Add some comments to help understand the overflow mechanism
> > >
> > > Changes in v2:
> > > - Update to support the new overflow mechanism
> > >
> > >  drivers/misc/Kconfig         |  8 +++++++
> > >  drivers/misc/cbmem_console.c | 43 ++++++++++++++++++++++--------------
> > >  2 files changed, 34 insertions(+), 17 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..3cbe9fb1a46a 100644
> > > --- a/drivers/misc/cbmem_console.c
> > > +++ b/drivers/misc/cbmem_console.c
> > > @@ -5,27 +5,37 @@
> > >
> > >  #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;
> > > +       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;
> > > +
> > > +       /* preserve the overflow flag if present */
> > > +       flags = cons->cursor & ~CBMC_CURSOR_MASK;
> > > +
> > > +       cons->body[pos++] = data;
> > > +
> > > +       /*
> > > +        * Deal with overflow - the flag may be cleared by another program which
> > > +        * reads the buffer out, e.g. Linux
> > > +        */
> >
> > U-Boot is not memory resident, so this does not sound like a correct
> > overflow mechanism to me.
>
> I am not sure what you mean. This logic is used in coreboot and some
> payloads, so I want to do the same in U-Boot. It basically let's
> U-Boot handle an overflow properly by setting the overflow flag. A
> later program (e.g. Linux) can then tell that that an overflow
> occurred.
>

Okay, I modified the comment to

       /*
       * Deal with overflow - the flag may be cleared by another program which
       * reads the buffer out later, e.g. Linux
       */

which makes more sense.

applied to u-boot-x86/next, thanks!

Regards,
Bin