[Skiboot] [PATCH V6 02/21] hw/ast-bmc: Initialize ast lpc mctp binding

[Joel Stanley]

On Thu, 6 Apr 2023 at 17:16, Frederic Barrat <fbarrat at linux.ibm.com> wrote:
>
>
>
> On 13/09/2022 12:26, Christophe Lombard wrote:
> > The Management Component Transport Protocol (MCTP) defines a communication
> > model intended to facilitate communication.
> >
> > This patch initialize MCTP binding over LPC Bus interface.
> >
> > Several steps must be performed:
> > - Initialize the MCTP core (mctp_init()).
> > - Initialize a hardware binding as AST LPC mode host (mctp_astlpc_init()).
> > - Register the hardware binding with the core (mctp_register_bus()), using
> > a predefined EID (Host default is 9).
> >
> > To transmit a MCTP message, mctp_message_tx() is used.
> > To receive a MCTP message, a callback need to be provided and registered
> > through mctp_set_rx_all().
> >
> > For the transfer of MCTP messages, two basics components are used:
> > - A window of the LPC FW address space, where reads and writes are
> > forwarded to BMC memory.
> > - An interrupt mechanism using the KCS interface.
> >
> > hw/ast-bmc/ast-mctp.c is compilated if the compiler flag CONFIG_PLDM is
> > set.
> >
> > Signed-off-by: Christophe Lombard <clombard at linux.vnet.ibm.com>
> > ---
> >   hw/ast-bmc/Makefile.inc |   7 +
> >   hw/ast-bmc/ast-mctp.c   | 386 ++++++++++++++++++++++++++++++++++++++++
> >   include/ast.h           |  11 ++
> >   3 files changed, 404 insertions(+)
> >   create mode 100644 hw/ast-bmc/ast-mctp.c
> >
> > diff --git a/hw/ast-bmc/Makefile.inc b/hw/ast-bmc/Makefile.inc
> > index e7ded0e8..546f2bc7 100644
> > --- a/hw/ast-bmc/Makefile.inc
> > +++ b/hw/ast-bmc/Makefile.inc
> > @@ -2,5 +2,12 @@
> >   SUBDIRS += hw/ast-bmc
> >
> >   AST_BMC_OBJS  = ast-io.o ast-sf-ctrl.o
> > +
> > +ifeq ($(CONFIG_PLDM),1)
> > +CPPFLAGS += -I$(SRC)/libmctp/
> > +#CFLAGS += -DAST_MCTP_DEBUG
> > +AST_BMC_OBJS += ast-mctp.o
> > +endif
> > +
> >   AST_BMC = hw/ast-bmc/built-in.a
> >   $(AST_BMC): $(AST_BMC_OBJS:%=hw/ast-bmc/%)
> > diff --git a/hw/ast-bmc/ast-mctp.c b/hw/ast-bmc/ast-mctp.c
> > new file mode 100644
> > index 00000000..1e7bdb51
> > --- /dev/null
> > +++ b/hw/ast-bmc/ast-mctp.c
> > @@ -0,0 +1,386 @@
> > +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> > +// Copyright 2022 IBM Corp.
> > +
> > +#define pr_fmt(fmt) "AST-MCTP: " fmt
> > +
> > +#include <lpc.h>
> > +#include <interrupts.h>
> > +#include <timer.h>
> > +#include <timebase.h>
> > +#include <debug_descriptor.h>
> > +#include <ast.h>
> > +#include <console.h>
> > +#include <libmctp.h>
> > +#include <libmctp-cmds.h>
> > +#include <libmctp-log.h>
> > +#include <libmctp-astlpc.h>
> > +
> > +/*
> > + * For the EIDs: the valid range is 8-254.
> > + * host default = 9
> > + */
> > +uint8_t HOST_EID = 9;
> > +
> > +struct timer ast_boot_timer;
> > +static struct mctp *mctp;
> > +
> > +struct ast_mctp_backend {
> > +     bool (*ready)(void);
> > +     int (*binding)(void);
> > +     void (*destroy)(void);
> > +};
> > +const struct ast_mctp_backend *mctp_backend;
> > +
> > +/*
> > + * The AST LPC binding is described here:
> > + *
> > + * https://github.com/openbmc/libmctp/blob/master/docs/bindings/vendor-astlpc.md
>
>
> The correct URL is now:
> https://github.com/openbmc/libmctp/blob/master/docs/bindings/vendor-ibm-astlpc.md

Perhaps use a tag, so it doesn't go away the next time someone
reorganises the files:

https://github.com/openbmc/libmctp/blob/v0.11/docs/bindings/vendor-ibm-astlpc.md

> > +#ifdef AST_MCTP_DEBUG
> > +char buffer[320];
> > +static void mctp_log(int log_lvl, const char *fmt, va_list va)
> > +{
> > +     snprintf(buffer, sizeof(buffer), "%s\n", fmt);
> > +     vprlog(log_lvl, buffer, va);
> > +}
> > +#endif
>
>
> I don't really like how logging is done in this file. We always reuse
> the libmctp trace macro, which means:
> - if we don't define AST_MCTP_DEBUG, we get no traces at all, not even
> errors.
> - if we define AST_MCTP_DEBUG, then we also get the traces from libmctp,
> which floods the in-memory skiboot trace buffer with traces such as:
> [  371.500175890,7] AST-MCTP: astlpc_kcs_reg_read 0xc0 from status (0)
> [  371.500219079,7] astlpc: host: mctp_astlpc_poll: status: 0xc0
>
> Why not use the usual prlog() for traces from this file?

+1

> We can keep AST_MCTP_DEBUG just as a mean to control the libmctp tracing
> only.