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

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@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

On Tue, Sep 13, 2022 at 12:26:46PM +0200, 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@linux.vnet.ibm.com>
Reviewed-by: Abhishek Singh Tomar <abhishek@linux.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
> + *
> + * Most of the binding is implemented in libmctp, but we need to provide
> + * accessors for the LPC FW space (for the packet buffer) and for the KCS
> + * peripheral (for the interrupt mechanism).
> + */
> +
> +struct astlpc_ops_data {
> +	uint16_t kcs_data_addr; /* LPC IO space offset for the data register */
> +	uint16_t kcs_stat_addr;
> +
> +	/* address of the packet exchange buffer in FW space */
> +	uint32_t lpc_fw_addr;
> +};
> +
> +static int astlpc_kcs_reg_read(void *binding_data,
> +			       enum mctp_binding_astlpc_kcs_reg reg,
> +			       uint8_t *val)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +	uint32_t addr, tmp;
> +	int rc;
> +
> +	if (reg == MCTP_ASTLPC_KCS_REG_STATUS)
> +		addr = ops_data->kcs_stat_addr;
> +	else if (reg == MCTP_ASTLPC_KCS_REG_DATA)
> +		addr = ops_data->kcs_data_addr;
> +	else
> +		return OPAL_PARAMETER;
> +
> +	rc = lpc_read(OPAL_LPC_IO, addr, &tmp, 1);
> +	if (!rc)
> +		*val = tmp & 0xff;
> +	else
> +		*val = 0xff;
> +
> +	mctp_prdebug("%s 0x%hhx from %s (%d)",
> +		     __func__, *val, reg ? "status" : "data", rc);
> +
> +	return rc;
> +}
> +
> +static int astlpc_kcs_reg_write(void *binding_data,
> +				enum mctp_binding_astlpc_kcs_reg reg,
> +				uint8_t val)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +	uint32_t addr;
> +
> +	mctp_prdebug("%s 0x%hhx to %s",
> +		     __func__, val, reg ? "status" : "data");
> +
> +	if (reg == MCTP_ASTLPC_KCS_REG_STATUS)
> +		addr = ops_data->kcs_stat_addr;
> +	else if (reg == MCTP_ASTLPC_KCS_REG_DATA)
> +		addr = ops_data->kcs_data_addr;
> +	else
> +		return OPAL_PARAMETER;
> +
> +	return lpc_write(OPAL_LPC_IO, addr, val, 1);
> +}
> +
> +static int astlpc_read(void *binding_data, void *buf, long offset,
> +		       size_t len)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +
> +	mctp_prdebug("%s %zu bytes from 0x%lx (lpc: 0x%lx)",
> +		     __func__, len, offset,
> +		     ops_data->lpc_fw_addr + offset);
> +	return lpc_fw_read(ops_data->lpc_fw_addr + offset, buf, len);
> +}
> +
> +static int astlpc_write(void *binding_data, const void *buf,
> +			long offset, size_t len)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +
> +	mctp_prdebug("%s %zu bytes to offset 0x%lx (lpc: 0x%lx)",
> +		     __func__, len, offset,
> +		     ops_data->lpc_fw_addr + offset);
> +	return lpc_fw_write(ops_data->lpc_fw_addr + offset, buf, len);
> +}
> +
> +static const struct mctp_binding_astlpc_ops astlpc_ops = {
> +	.kcs_read = astlpc_kcs_reg_read,
> +	.kcs_write = astlpc_kcs_reg_write,
> +	.lpc_read = astlpc_read,
> +	.lpc_write = astlpc_write,
> +};
> +
> +static struct mctp_binding_astlpc *astlpc;
> +static struct astlpc_ops_data *ops_data;
> +
> +/* Keyboard Controller Style (KCS) data register address */
> +#define KCS_DATA_REG 0xca2
> +
> +/* Keyboard Controller Style (KCS) status register address */
> +#define KCS_STATUS_REG 0xca3
> +
> +/* Serial IRQ number for KCS interface */
> +#define KCS_SERIAL_IRQ 11
> +
> +#define KCS_STATUS_BMC_READY 0x80
> +
> +bool irq_active;
> +
> +/* we need a poller to crank the mctp state machine during boot */
> +static void astlpc_poller(struct timer *t __unused,
> +			  void *data __unused, uint64_t now __unused)
> +{
> +	mctp_astlpc_poll(astlpc);
> +
> +	/* once booted we can rely on the KCS interrupt */
> +	if ((opal_booting() && !irq_active))
> +		schedule_timer(&ast_boot_timer, msecs_to_tb(5));
> +}
> +
> +/* at runtime the interrupt should handle it */
> +static void astlpc_interrupt(uint32_t chip_id __unused,
> +			     uint32_t irq_msk __unused)
> +{
> +	if (!irq_active) {
> +		mctp_prerr("IRQ Active! Disabling boot poller...");
> +		irq_active = true;
> +	}
> +
> +	mctp_astlpc_poll(astlpc);
> +}
> +
> +static struct lpc_client kcs_lpc_client = {
> +	.reset = NULL,
> +	.interrupt = astlpc_interrupt,
> +	.interrupts = LPC_IRQ(KCS_SERIAL_IRQ),
> +};
> +
> +static bool astlpc_ready(void)
> +{
> +	uint32_t tmp = 0;
> +
> +	/* Probe for MCTP support by reading the STAT register of KCS#4 */
> +	if (lpc_probe_read(OPAL_LPC_IO, KCS_STATUS_REG, &tmp, 1)) {
> +		mctp_prdebug("KCS4 probe failed, skipping MCTP init");
> +		return false;
> +	}
> +
> +	return !!(tmp & KCS_STATUS_BMC_READY); /* high bit indicates BMC is listening */
> +}
> +
> +#define DESIRED_MTU 32768
> +
> +static void astlpc_destroy(void)
> +{
> +	if (astlpc) {
> +		mctp_astlpc_destroy(astlpc);
> +		cancel_timer(&ast_boot_timer);
> +		astlpc = NULL;
> +	}
> +}
> +
> +static int astlpc_binding(void)
> +{
> +	ops_data = zalloc(sizeof(struct astlpc_ops_data));
> +	if (!ops_data)
> +		return OPAL_NO_MEM;
> +
> +	/*
> +	 * Current OpenBMC systems put the MCTP buffer 1MB down from
> +	 * the end of the LPC FW range.
> +	 *
> +	 * The size of the FW range is: 0x1000_0000 so the window be at:
> +	 *
> +	 *   0x1000_0000 - 2**20 == 0xff00000
> +	 */
> +	ops_data->lpc_fw_addr = 0xff00000;
> +
> +	/* values chosen by the OpenBMC driver */
> +	ops_data->kcs_data_addr = KCS_DATA_REG;
> +	ops_data->kcs_stat_addr = KCS_STATUS_REG;
> +
> +	/* Initialise the binding */
> +	astlpc = mctp_astlpc_init(MCTP_BINDING_ASTLPC_MODE_HOST,
> +				  DESIRED_MTU,
> +				  NULL,
> +				  &astlpc_ops,
> +				  ops_data);
> +	if (!astlpc) {
> +		mctp_prerr("binding init failed");
> +		return OPAL_HARDWARE;
> +	}
> +
> +	/* register an lpc client so we get an interrupt */
> +	lpc_register_client(0, &kcs_lpc_client, IRQ_ATTR_TARGET_OPAL);
> +
> +	init_timer(&ast_boot_timer, astlpc_poller, astlpc);
> +	schedule_timer(&ast_boot_timer, msecs_to_tb(5));
> +
> +	/* Register the binding to the LPC bus we are using for this
> +	 * MCTP configuration.
> +	 */
> +	if (mctp_register_bus(mctp,
> +			      mctp_binding_astlpc_core(astlpc),
> +			      HOST_EID)) {
> +		mctp_prerr("failed to register bus");
> +		goto err;
> +	}
> +
> +	return OPAL_SUCCESS;
> +
> +err:
> +	astlpc_destroy();
> +	free(ops_data);
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +static const struct ast_mctp_backend astlpc_backend = {
> +	.ready = astlpc_ready,
> +	.binding = astlpc_binding,
> +	.destroy = astlpc_destroy,
> +};
> +
> +static void *mctp_malloc(size_t size) { return malloc(size); }
> +static void mctp_free(void *ptr) { return free(ptr); }
> +static void *mctp_realloc(void *ptr, size_t size)
> +{
> +	return realloc(ptr, size);
> +}
> +
> +#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
> +
> +int ast_mctp_ready(void)
> +{
> +	if (!mctp_backend->ready())
> +		return OPAL_BUSY;
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len)
> +{
> +	uint8_t tag = 0;
> +
> +	return mctp_message_tx(mctp, eid, MCTP_MESSAGE_TO_DST, tag, msg, len);
> +}
> +
> +void ast_mctp_stop_polling(void)
> +{
> +	irq_active = true;
> +}
> +
> +#define HOST_MAX_INCOMING_MESSAGE_ALLOCATION 131072
> +
> +/*
> + * Initialize mctp binding for hbrt and provide interfaces for sending
> + * and receiving mctp messages.
> + */
> +int ast_mctp_init(uint8_t mode, void (*fn)(uint8_t src_eid, bool tag_owner,
> +		  uint8_t msg_tag, void *data, void *msg, size_t len))
> +{
> +	uint64_t start;
> +
> +	if (mode == MCTP_BINDING_ASTLPC_MODE)
> +		mctp_backend = &astlpc_backend;
> +	else {
> +		mctp_prerr("Unknown AST binding mode: %u", mode);
> +		return OPAL_PARAMETER;
> +	}
> +
> +	if (!mctp_backend->ready())
> +		return OPAL_HARDWARE;
> +
> +	/* skiboot's malloc/free/realloc are macros so they need
> +	 * wrappers
> +	 */
> +	mctp_set_alloc_ops(mctp_malloc, mctp_free, mctp_realloc);
> +
> +	/*
> +	 * /-----\                                 /---------\
> +	 * | bmc | (eid: 8) <- lpc/kcs -> (eid: 9) | skiboot |
> +	 * \-----/               pcie              \---------/
> +	 *           astlpc kcs / astpcie binding
> +	 */
> +	mctp = mctp_init();
> +	if (!mctp) {
> +		mctp_prerr("mctp init failed");
> +		return OPAL_HARDWARE;
> +	}
> +
> +#ifdef AST_MCTP_DEBUG
> +	/* Setup the trace hook */
> +	mctp_set_log_custom(mctp_log);
> +#endif
> +
> +	mctp_prinfo("libmctp initialized");
> +
> +	/* Set the max message size to be large enough */
> +	mctp_set_max_message_size(mctp, HOST_MAX_INCOMING_MESSAGE_ALLOCATION);
> +
> +	/* Setup the message rx callback */
> +	mctp_set_rx_all(mctp, fn, NULL);
> +
> +	/* Initialise the binding */
> +	if (mctp_backend->binding())
> +		goto err;
> +
> +	/* Initializing the channel requires running the poll function
> +	 * a few times to handle the initial setup since we need the
> +	 * BMC to reply.
> +	 *
> +	 * Unfortuntately there's no good way to determine if this
> +	 * worked or not so poll for a bit with a timeout. :(
> +	 *
> +	 * We have to check (bus->state == mctp_bus_state_constructed)
> +	 * otherwise mctp_message_tx() will reject the message.
> +	 */
> +	start = mftb();
> +	while (tb_compare(mftb(), start + msecs_to_tb(250)) == TB_ABEFOREB)
> +		time_wait_ms(10);
> +
> +	mctp_prwarn("%s - The mctp bus state should be correct now !!!",
> +		    __func__);
> +
> +	return OPAL_SUCCESS;
> +
> +err:
> +	mctp_prerr("Unable to initialize MCTP");
> +	mctp_destroy(mctp);
> +	mctp = NULL;
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +void ast_mctp_exit(void)
> +{
> +	if (mctp_backend && mctp_backend->destroy)
> +		mctp_backend->destroy();
> +
> +	if (mctp)
> +		mctp_destroy(mctp);
> +
> +	mctp = NULL;
> +}
> diff --git a/include/ast.h b/include/ast.h
> index 5e932398..55eeae28 100644
> --- a/include/ast.h
> +++ b/include/ast.h
> @@ -91,6 +91,17 @@ void ast_setup_ibt(uint16_t io_base, uint8_t irq);
>  /* MBOX configuration */
>  void ast_setup_sio_mbox(uint16_t io_base, uint8_t irq);
> 
> +/* AST MCTP configuration */
> +#define MCTP_BINDING_ASTLPC_MODE 0
> +#define MCTP_BINDING_ASTPCIE_MODE 1
> +
> +int ast_mctp_ready(void);
> +int ast_mctp_init(uint8_t mode, void (*fn)(uint8_t src_eid, bool tag_owner,
> +		  uint8_t msg_tag, void *data, void *msg, size_t len));
> +void ast_mctp_exit(void);
> +void ast_mctp_stop_polling(void);
> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len);
> +
>  #endif /* __SKIBOOT__ */
> 
>  /*
> -- 
> 2.37.3
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

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@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


> + *
> + * Most of the binding is implemented in libmctp, but we need to provide
> + * accessors for the LPC FW space (for the packet buffer) and for the KCS
> + * peripheral (for the interrupt mechanism).
> + */
> +
> +struct astlpc_ops_data {
> +	uint16_t kcs_data_addr; /* LPC IO space offset for the data register */
> +	uint16_t kcs_stat_addr;
> +
> +	/* address of the packet exchange buffer in FW space */
> +	uint32_t lpc_fw_addr;
> +};
> +
> +static int astlpc_kcs_reg_read(void *binding_data,
> +			       enum mctp_binding_astlpc_kcs_reg reg,
> +			       uint8_t *val)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +	uint32_t addr, tmp;
> +	int rc;
> +
> +	if (reg == MCTP_ASTLPC_KCS_REG_STATUS)
> +		addr = ops_data->kcs_stat_addr;
> +	else if (reg == MCTP_ASTLPC_KCS_REG_DATA)
> +		addr = ops_data->kcs_data_addr;
> +	else
> +		return OPAL_PARAMETER;
> +
> +	rc = lpc_read(OPAL_LPC_IO, addr, &tmp, 1);
> +	if (!rc)
> +		*val = tmp & 0xff;
> +	else
> +		*val = 0xff;
> +
> +	mctp_prdebug("%s 0x%hhx from %s (%d)",
> +		     __func__, *val, reg ? "status" : "data", rc);
> +
> +	return rc;
> +}
> +
> +static int astlpc_kcs_reg_write(void *binding_data,
> +				enum mctp_binding_astlpc_kcs_reg reg,
> +				uint8_t val)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +	uint32_t addr;
> +
> +	mctp_prdebug("%s 0x%hhx to %s",
> +		     __func__, val, reg ? "status" : "data");
> +
> +	if (reg == MCTP_ASTLPC_KCS_REG_STATUS)
> +		addr = ops_data->kcs_stat_addr;
> +	else if (reg == MCTP_ASTLPC_KCS_REG_DATA)
> +		addr = ops_data->kcs_data_addr;
> +	else
> +		return OPAL_PARAMETER;
> +
> +	return lpc_write(OPAL_LPC_IO, addr, val, 1);
> +}
> +
> +static int astlpc_read(void *binding_data, void *buf, long offset,
> +		       size_t len)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +
> +	mctp_prdebug("%s %zu bytes from 0x%lx (lpc: 0x%lx)",
> +		     __func__, len, offset,
> +		     ops_data->lpc_fw_addr + offset);
> +	return lpc_fw_read(ops_data->lpc_fw_addr + offset, buf, len);
> +}
> +
> +static int astlpc_write(void *binding_data, const void *buf,
> +			long offset, size_t len)
> +{
> +	struct astlpc_ops_data *ops_data = binding_data;
> +
> +	mctp_prdebug("%s %zu bytes to offset 0x%lx (lpc: 0x%lx)",
> +		     __func__, len, offset,
> +		     ops_data->lpc_fw_addr + offset);
> +	return lpc_fw_write(ops_data->lpc_fw_addr + offset, buf, len);
> +}
> +
> +static const struct mctp_binding_astlpc_ops astlpc_ops = {
> +	.kcs_read = astlpc_kcs_reg_read,
> +	.kcs_write = astlpc_kcs_reg_write,
> +	.lpc_read = astlpc_read,
> +	.lpc_write = astlpc_write,
> +};
> +
> +static struct mctp_binding_astlpc *astlpc;
> +static struct astlpc_ops_data *ops_data;
> +
> +/* Keyboard Controller Style (KCS) data register address */
> +#define KCS_DATA_REG 0xca2
> +
> +/* Keyboard Controller Style (KCS) status register address */
> +#define KCS_STATUS_REG 0xca3
> +
> +/* Serial IRQ number for KCS interface */
> +#define KCS_SERIAL_IRQ 11


These should probably be defined in platforms/astbmc/common.c and added 
to the device tree and the value read here from the device tree, like 
it's done for other LPC clients (UART, BT, MBOX).


> +
> +#define KCS_STATUS_BMC_READY 0x80
> +
> +bool irq_active;
> +
> +/* we need a poller to crank the mctp state machine during boot */
> +static void astlpc_poller(struct timer *t __unused,
> +			  void *data __unused, uint64_t now __unused)
> +{
> +	mctp_astlpc_poll(astlpc);
> +
> +	/* once booted we can rely on the KCS interrupt */
> +	if ((opal_booting() && !irq_active))


We shoult talk about interrupts, I'm pretty convinced the call to 
opal_booting() is useless, since external interrupts are not going to 
fire while we are in skiboot. But this could all be moot if we use an 
opal poller (see below).


> +		schedule_timer(&ast_boot_timer, msecs_to_tb(5));
> +}
> +
> +/* at runtime the interrupt should handle it */
> +static void astlpc_interrupt(uint32_t chip_id __unused,
> +			     uint32_t irq_msk __unused)
> +{
> +	if (!irq_active) {
> +		mctp_prerr("IRQ Active! Disabling boot poller...");
> +		irq_active = true;
> +	}
> +
> +	mctp_astlpc_poll(astlpc);
> +}
> +
> +static struct lpc_client kcs_lpc_client = {
> +	.reset = NULL,
> +	.interrupt = astlpc_interrupt,
> +	.interrupts = LPC_IRQ(KCS_SERIAL_IRQ),
> +};
> +
> +static bool astlpc_ready(void)
> +{
> +	uint32_t tmp = 0;
> +
> +	/* Probe for MCTP support by reading the STAT register of KCS#4 */
> +	if (lpc_probe_read(OPAL_LPC_IO, KCS_STATUS_REG, &tmp, 1)) {


We've talked about this, I've done some reading and I don't understand 
the need for lpc_probe_read() here. My understanding is that the "probe" 
version is more forgiving in case of errors (it temporarily masks the 
interrupt, does a read, then checks the error status register to report 
errors) and it is meant to be used with invalid addresses for example. 
It's not the case here. I've been running with a simple lpc_read() and 
it's working as expected for me.


> +		mctp_prdebug("KCS4 probe failed, skipping MCTP init");
> +		return false;
> +	}
> +
> +	return !!(tmp & KCS_STATUS_BMC_READY); /* high bit indicates BMC is listening */
> +}
> +
> +#define DESIRED_MTU 32768
> +
> +static void astlpc_destroy(void)
> +{
> +	if (astlpc) {
> +		mctp_astlpc_destroy(astlpc);
> +		cancel_timer(&ast_boot_timer);
> +		astlpc = NULL;
> +	}
> +}
> +
> +static int astlpc_binding(void)
> +{
> +	ops_data = zalloc(sizeof(struct astlpc_ops_data));
> +	if (!ops_data)
> +		return OPAL_NO_MEM;
> +
> +	/*
> +	 * Current OpenBMC systems put the MCTP buffer 1MB down from
> +	 * the end of the LPC FW range.
> +	 *
> +	 * The size of the FW range is: 0x1000_0000 so the window be at:
> +	 *
> +	 *   0x1000_0000 - 2**20 == 0xff00000
> +	 */
> +	ops_data->lpc_fw_addr = 0xff00000;


The fw address is already defined in HDAT! Look for "mctp_base" in 
bmc_create_node(). Maybe it could be added to the device tree from HDAT 
and read here?


> +
> +	/* values chosen by the OpenBMC driver */
> +	ops_data->kcs_data_addr = KCS_DATA_REG;
> +	ops_data->kcs_stat_addr = KCS_STATUS_REG;
> +
> +	/* Initialise the binding */
> +	astlpc = mctp_astlpc_init(MCTP_BINDING_ASTLPC_MODE_HOST,
> +				  DESIRED_MTU,
> +				  NULL,
> +				  &astlpc_ops,
> +				  ops_data);
> +	if (!astlpc) {
> +		mctp_prerr("binding init failed");
> +		return OPAL_HARDWARE;
> +	}
> +
> +	/* register an lpc client so we get an interrupt */
> +	lpc_register_client(0, &kcs_lpc_client, IRQ_ATTR_TARGET_OPAL);
> +
> +	init_timer(&ast_boot_timer, astlpc_poller, astlpc);
> +	schedule_timer(&ast_boot_timer, msecs_to_tb(5));


I'm thinking we should convert this to an opal poller (by calling 
opal_add_poller()) instead of defining our own timer. It would call our 
polling function whenever opal_run_pollers() is called and we don't need 
to manage the timer (by re-arming it each time astlpc_poller() is called).


> +
> +	/* Register the binding to the LPC bus we are using for this
> +	 * MCTP configuration.
> +	 */
> +	if (mctp_register_bus(mctp,
> +			      mctp_binding_astlpc_core(astlpc),
> +			      HOST_EID)) {
> +		mctp_prerr("failed to register bus");
> +		goto err;
> +	}
> +
> +	return OPAL_SUCCESS;
> +
> +err:


The timer needs to be cleaned up on the error path.


> +	astlpc_destroy();
> +	free(ops_data);
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +static const struct ast_mctp_backend astlpc_backend = {
> +	.ready = astlpc_ready,
> +	.binding = astlpc_binding,
> +	.destroy = astlpc_destroy,
> +};
> +
> +static void *mctp_malloc(size_t size) { return malloc(size); }
> +static void mctp_free(void *ptr) { return free(ptr); }
> +static void *mctp_realloc(void *ptr, size_t size)
> +{
> +	return realloc(ptr, size);
> +}
> +
> +#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?
We can keep AST_MCTP_DEBUG just as a mean to control the libmctp tracing 
only.


> +
> +int ast_mctp_ready(void)
> +{
> +	if (!mctp_backend->ready())


That's an exported API, so it would be better to check if mctp_backend 
and mctp_backend->ready are non-NULL before calling it.


> +		return OPAL_BUSY;
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len)
> +{
> +	uint8_t tag = 0;
> +
> +	return mctp_message_tx(mctp, eid, MCTP_MESSAGE_TO_DST, tag, msg, len);
> +}
> +
> +void ast_mctp_stop_polling(void)
> +{


This function looks like dead code. Leftover from debug?


> +	irq_active = true;
> +}
> +
> +#define HOST_MAX_INCOMING_MESSAGE_ALLOCATION 131072
> +
> +/*
> + * Initialize mctp binding for hbrt and provide interfaces for sending
> + * and receiving mctp messages.
> + */
> +int ast_mctp_init(uint8_t mode, void (*fn)(uint8_t src_eid, bool tag_owner,
> +		  uint8_t msg_tag, void *data, void *msg, size_t len))
> +{
> +	uint64_t start;
> +
> +	if (mode == MCTP_BINDING_ASTLPC_MODE)
> +		mctp_backend = &astlpc_backend;
> +	else {
> +		mctp_prerr("Unknown AST binding mode: %u", mode);
> +		return OPAL_PARAMETER;
> +	}
> +
> +	if (!mctp_backend->ready())
> +		return OPAL_HARDWARE;
> +
> +	/* skiboot's malloc/free/realloc are macros so they need
> +	 * wrappers
> +	 */
> +	mctp_set_alloc_ops(mctp_malloc, mctp_free, mctp_realloc);
> +
> +	/*
> +	 * /-----\                                 /---------\
> +	 * | bmc | (eid: 8) <- lpc/kcs -> (eid: 9) | skiboot |
> +	 * \-----/               pcie              \---------/
> +	 *           astlpc kcs / astpcie binding
> +	 */
> +	mctp = mctp_init();
> +	if (!mctp) {
> +		mctp_prerr("mctp init failed");
> +		return OPAL_HARDWARE;
> +	}
> +
> +#ifdef AST_MCTP_DEBUG
> +	/* Setup the trace hook */
> +	mctp_set_log_custom(mctp_log);
> +#endif
> +
> +	mctp_prinfo("libmctp initialized");
> +
> +	/* Set the max message size to be large enough */
> +	mctp_set_max_message_size(mctp, HOST_MAX_INCOMING_MESSAGE_ALLOCATION);
> +
> +	/* Setup the message rx callback */
> +	mctp_set_rx_all(mctp, fn, NULL);
> +
> +	/* Initialise the binding */
> +	if (mctp_backend->binding())
> +		goto err;
> +
> +	/* Initializing the channel requires running the poll function
> +	 * a few times to handle the initial setup since we need the
> +	 * BMC to reply.
> +	 *
> +	 * Unfortuntately there's no good way to determine if this
> +	 * worked or not so poll for a bit with a timeout. :(
> +	 *
> +	 * We have to check (bus->state == mctp_bus_state_constructed)
> +	 * otherwise mctp_message_tx() will reject the message.
> +	 */
> +	start = mftb();
> +	while (tb_compare(mftb(), start + msecs_to_tb(250)) == TB_ABEFOREB)
> +		time_wait_ms(10);


Why not a simple time_wait_ms() for the full duration? It would call the 
opal_run_pollers(), which check the timers.

   Fred



> +
> +	mctp_prwarn("%s - The mctp bus state should be correct now !!!",
> +		    __func__);
> +
> +	return OPAL_SUCCESS;
> +
> +err:
> +	mctp_prerr("Unable to initialize MCTP");
> +	mctp_destroy(mctp);
> +	mctp = NULL;
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +void ast_mctp_exit(void)
> +{
> +	if (mctp_backend && mctp_backend->destroy)
> +		mctp_backend->destroy();
> +
> +	if (mctp)
> +		mctp_destroy(mctp);
> +
> +	mctp = NULL;
> +}
> diff --git a/include/ast.h b/include/ast.h
> index 5e932398..55eeae28 100644
> --- a/include/ast.h
> +++ b/include/ast.h
> @@ -91,6 +91,17 @@ void ast_setup_ibt(uint16_t io_base, uint8_t irq);
>   /* MBOX configuration */
>   void ast_setup_sio_mbox(uint16_t io_base, uint8_t irq);
>   
> +/* AST MCTP configuration */
> +#define MCTP_BINDING_ASTLPC_MODE 0
> +#define MCTP_BINDING_ASTPCIE_MODE 1
> +
> +int ast_mctp_ready(void);
> +int ast_mctp_init(uint8_t mode, void (*fn)(uint8_t src_eid, bool tag_owner,
> +		  uint8_t msg_tag, void *data, void *msg, size_t len));
> +void ast_mctp_exit(void);
> +void ast_mctp_stop_polling(void);
> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len);
> +
>   #endif /* __SKIBOOT__ */
>   
>   /*

Le 06/04/2023 à 19:16, Frederic Barrat a écrit :
>
>
> 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@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 
>
>

Thanks.

>
>> + *
>> + * Most of the binding is implemented in libmctp, but we need to 
>> provide
>> + * accessors for the LPC FW space (for the packet buffer) and for 
>> the KCS
>> + * peripheral (for the interrupt mechanism).
>> + */
>> +
>> +struct astlpc_ops_data {
>> +    uint16_t kcs_data_addr; /* LPC IO space offset for the data 
>> register */
>> +    uint16_t kcs_stat_addr;
>> +
>> +    /* address of the packet exchange buffer in FW space */
>> +    uint32_t lpc_fw_addr;
>> +};
>> +
>> +static int astlpc_kcs_reg_read(void *binding_data,
>> +                   enum mctp_binding_astlpc_kcs_reg reg,
>> +                   uint8_t *val)
>> +{
>> +    struct astlpc_ops_data *ops_data = binding_data;
>> +    uint32_t addr, tmp;
>> +    int rc;
>> +
>> +    if (reg == MCTP_ASTLPC_KCS_REG_STATUS)
>> +        addr = ops_data->kcs_stat_addr;
>> +    else if (reg == MCTP_ASTLPC_KCS_REG_DATA)
>> +        addr = ops_data->kcs_data_addr;
>> +    else
>> +        return OPAL_PARAMETER;
>> +
>> +    rc = lpc_read(OPAL_LPC_IO, addr, &tmp, 1);
>> +    if (!rc)
>> +        *val = tmp & 0xff;
>> +    else
>> +        *val = 0xff;
>> +
>> +    mctp_prdebug("%s 0x%hhx from %s (%d)",
>> +             __func__, *val, reg ? "status" : "data", rc);
>> +
>> +    return rc;
>> +}
>> +
>> +static int astlpc_kcs_reg_write(void *binding_data,
>> +                enum mctp_binding_astlpc_kcs_reg reg,
>> +                uint8_t val)
>> +{
>> +    struct astlpc_ops_data *ops_data = binding_data;
>> +    uint32_t addr;
>> +
>> +    mctp_prdebug("%s 0x%hhx to %s",
>> +             __func__, val, reg ? "status" : "data");
>> +
>> +    if (reg == MCTP_ASTLPC_KCS_REG_STATUS)
>> +        addr = ops_data->kcs_stat_addr;
>> +    else if (reg == MCTP_ASTLPC_KCS_REG_DATA)
>> +        addr = ops_data->kcs_data_addr;
>> +    else
>> +        return OPAL_PARAMETER;
>> +
>> +    return lpc_write(OPAL_LPC_IO, addr, val, 1);
>> +}
>> +
>> +static int astlpc_read(void *binding_data, void *buf, long offset,
>> +               size_t len)
>> +{
>> +    struct astlpc_ops_data *ops_data = binding_data;
>> +
>> +    mctp_prdebug("%s %zu bytes from 0x%lx (lpc: 0x%lx)",
>> +             __func__, len, offset,
>> +             ops_data->lpc_fw_addr + offset);
>> +    return lpc_fw_read(ops_data->lpc_fw_addr + offset, buf, len);
>> +}
>> +
>> +static int astlpc_write(void *binding_data, const void *buf,
>> +            long offset, size_t len)
>> +{
>> +    struct astlpc_ops_data *ops_data = binding_data;
>> +
>> +    mctp_prdebug("%s %zu bytes to offset 0x%lx (lpc: 0x%lx)",
>> +             __func__, len, offset,
>> +             ops_data->lpc_fw_addr + offset);
>> +    return lpc_fw_write(ops_data->lpc_fw_addr + offset, buf, len);
>> +}
>> +
>> +static const struct mctp_binding_astlpc_ops astlpc_ops = {
>> +    .kcs_read = astlpc_kcs_reg_read,
>> +    .kcs_write = astlpc_kcs_reg_write,
>> +    .lpc_read = astlpc_read,
>> +    .lpc_write = astlpc_write,
>> +};
>> +
>> +static struct mctp_binding_astlpc *astlpc;
>> +static struct astlpc_ops_data *ops_data;
>> +
>> +/* Keyboard Controller Style (KCS) data register address */
>> +#define KCS_DATA_REG 0xca2
>> +
>> +/* Keyboard Controller Style (KCS) status register address */
>> +#define KCS_STATUS_REG 0xca3
>> +
>> +/* Serial IRQ number for KCS interface */
>> +#define KCS_SERIAL_IRQ 11
>
>
> These should probably be defined in platforms/astbmc/common.c and 
> added to the device tree and the value read here from the device tree, 
> like it's done for other LPC clients (UART, BT, MBOX).
>

ok will do.

>
>> +
>> +#define KCS_STATUS_BMC_READY 0x80
>> +
>> +bool irq_active;
>> +
>> +/* we need a poller to crank the mctp state machine during boot */
>> +static void astlpc_poller(struct timer *t __unused,
>> +              void *data __unused, uint64_t now __unused)
>> +{
>> +    mctp_astlpc_poll(astlpc);
>> +
>> +    /* once booted we can rely on the KCS interrupt */
>> +    if ((opal_booting() && !irq_active))
>
>
> We shoult talk about interrupts, I'm pretty convinced the call to 
> opal_booting() is useless, since external interrupts are not going to 
> fire while we are in skiboot. But this could all be moot if we use an 
> opal poller (see below).
>
>
>> + schedule_timer(&ast_boot_timer, msecs_to_tb(5));
>> +}
>> +
>> +/* at runtime the interrupt should handle it */
>> +static void astlpc_interrupt(uint32_t chip_id __unused,
>> +                 uint32_t irq_msk __unused)
>> +{
>> +    if (!irq_active) {
>> +        mctp_prerr("IRQ Active! Disabling boot poller...");
>> +        irq_active = true;
>> +    }
>> +
>> +    mctp_astlpc_poll(astlpc);
>> +}
>> +
>> +static struct lpc_client kcs_lpc_client = {
>> +    .reset = NULL,
>> +    .interrupt = astlpc_interrupt,
>> +    .interrupts = LPC_IRQ(KCS_SERIAL_IRQ),
>> +};
>> +
>> +static bool astlpc_ready(void)
>> +{
>> +    uint32_t tmp = 0;
>> +
>> +    /* Probe for MCTP support by reading the STAT register of KCS#4 */
>> +    if (lpc_probe_read(OPAL_LPC_IO, KCS_STATUS_REG, &tmp, 1)) {
>
>
> We've talked about this, I've done some reading and I don't understand 
> the need for lpc_probe_read() here. My understanding is that the 
> "probe" version is more forgiving in case of errors (it temporarily 
> masks the interrupt, does a read, then checks the error status 
> register to report errors) and it is meant to be used with invalid 
> addresses for example. It's not the case here. I've been running with 
> a simple lpc_read() and it's working as expected for me.
>
>

you have probably reason. We will use lpc_read() instead.

>> +        mctp_prdebug("KCS4 probe failed, skipping MCTP init");
>> +        return false;
>> +    }
>> +
>> +    return !!(tmp & KCS_STATUS_BMC_READY); /* high bit indicates BMC 
>> is listening */
>> +}
>> +
>> +#define DESIRED_MTU 32768
>> +
>> +static void astlpc_destroy(void)
>> +{
>> +    if (astlpc) {
>> +        mctp_astlpc_destroy(astlpc);
>> +        cancel_timer(&ast_boot_timer);
>> +        astlpc = NULL;
>> +    }
>> +}
>> +
>> +static int astlpc_binding(void)
>> +{
>> +    ops_data = zalloc(sizeof(struct astlpc_ops_data));
>> +    if (!ops_data)
>> +        return OPAL_NO_MEM;
>> +
>> +    /*
>> +     * Current OpenBMC systems put the MCTP buffer 1MB down from
>> +     * the end of the LPC FW range.
>> +     *
>> +     * The size of the FW range is: 0x1000_0000 so the window be at:
>> +     *
>> +     *   0x1000_0000 - 2**20 == 0xff00000
>> +     */
>> +    ops_data->lpc_fw_addr = 0xff00000;
>
>
> The fw address is already defined in HDAT! Look for "mctp_base" in 
> bmc_create_node(). Maybe it could be added to the device tree from 
> HDAT and read here?
>
>

Thanks.

>> +
>> +    /* values chosen by the OpenBMC driver */
>> +    ops_data->kcs_data_addr = KCS_DATA_REG;
>> +    ops_data->kcs_stat_addr = KCS_STATUS_REG;
>> +
>> +    /* Initialise the binding */
>> +    astlpc = mctp_astlpc_init(MCTP_BINDING_ASTLPC_MODE_HOST,
>> +                  DESIRED_MTU,
>> +                  NULL,
>> +                  &astlpc_ops,
>> +                  ops_data);
>> +    if (!astlpc) {
>> +        mctp_prerr("binding init failed");
>> +        return OPAL_HARDWARE;
>> +    }
>> +
>> +    /* register an lpc client so we get an interrupt */
>> +    lpc_register_client(0, &kcs_lpc_client, IRQ_ATTR_TARGET_OPAL);
>> +
>> +    init_timer(&ast_boot_timer, astlpc_poller, astlpc);
>> +    schedule_timer(&ast_boot_timer, msecs_to_tb(5));
>
>
> I'm thinking we should convert this to an opal poller (by calling 
> opal_add_poller()) instead of defining our own timer. It would call 
> our polling function whenever opal_run_pollers() is called and we 
> don't need to manage the timer (by re-arming it each time 
> astlpc_poller() is called).
>
>
>> +
>> +    /* Register the binding to the LPC bus we are using for this
>> +     * MCTP configuration.
>> +     */
>> +    if (mctp_register_bus(mctp,
>> +                  mctp_binding_astlpc_core(astlpc),
>> +                  HOST_EID)) {
>> +        mctp_prerr("failed to register bus");
>> +        goto err;
>> +    }
>> +
>> +    return OPAL_SUCCESS;
>> +
>> +err:
>
>
> The timer needs to be cleaned up on the error path.
>

Good catch!

>
>> +    astlpc_destroy();
>> +    free(ops_data);
>> +
>> +    return OPAL_HARDWARE;
>> +}
>> +
>> +static const struct ast_mctp_backend astlpc_backend = {
>> +    .ready = astlpc_ready,
>> +    .binding = astlpc_binding,
>> +    .destroy = astlpc_destroy,
>> +};
>> +
>> +static void *mctp_malloc(size_t size) { return malloc(size); }
>> +static void mctp_free(void *ptr) { return free(ptr); }
>> +static void *mctp_realloc(void *ptr, size_t size)
>> +{
>> +    return realloc(ptr, size);
>> +}
>> +
>> +#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?
> We can keep AST_MCTP_DEBUG just as a mean to control the libmctp 
> tracing only.
>

ast-mctp.c is intrinsically linked to libmctp. For this reason we used 
the same way of tracing.
But no problem to use prolog.

>
>> +
>> +int ast_mctp_ready(void)
>> +{
>> +    if (!mctp_backend->ready())
>
>
> That's an exported API, so it would be better to check if mctp_backend 
> and mctp_backend->ready are non-NULL before calling it.
>

Correct.

>
>> +        return OPAL_BUSY;
>> +
>> +    return OPAL_SUCCESS;
>> +}
>> +
>> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len)
>> +{
>> +    uint8_t tag = 0;
>> +
>> +    return mctp_message_tx(mctp, eid, MCTP_MESSAGE_TO_DST, tag, msg, 
>> len);
>> +}
>> +
>> +void ast_mctp_stop_polling(void)
>> +{
>
>
> This function looks like dead code. Leftover from debug?
>

Certainly, yes !

>
>> +    irq_active = true;
>> +}
>> +
>> +#define HOST_MAX_INCOMING_MESSAGE_ALLOCATION 131072
>> +
>> +/*
>> + * Initialize mctp binding for hbrt and provide interfaces for sending
>> + * and receiving mctp messages.
>> + */
>> +int ast_mctp_init(uint8_t mode, void (*fn)(uint8_t src_eid, bool 
>> tag_owner,
>> +          uint8_t msg_tag, void *data, void *msg, size_t len))
>> +{
>> +    uint64_t start;
>> +
>> +    if (mode == MCTP_BINDING_ASTLPC_MODE)
>> +        mctp_backend = &astlpc_backend;
>> +    else {
>> +        mctp_prerr("Unknown AST binding mode: %u", mode);
>> +        return OPAL_PARAMETER;
>> +    }
>> +
>> +    if (!mctp_backend->ready())
>> +        return OPAL_HARDWARE;
>> +
>> +    /* skiboot's malloc/free/realloc are macros so they need
>> +     * wrappers
>> +     */
>> +    mctp_set_alloc_ops(mctp_malloc, mctp_free, mctp_realloc);
>> +
>> +    /*
>> +     * /-----\                                 /---------\
>> +     * | bmc | (eid: 8) <- lpc/kcs -> (eid: 9) | skiboot |
>> +     * \-----/               pcie              \---------/
>> +     *           astlpc kcs / astpcie binding
>> +     */
>> +    mctp = mctp_init();
>> +    if (!mctp) {
>> +        mctp_prerr("mctp init failed");
>> +        return OPAL_HARDWARE;
>> +    }
>> +
>> +#ifdef AST_MCTP_DEBUG
>> +    /* Setup the trace hook */
>> +    mctp_set_log_custom(mctp_log);
>> +#endif
>> +
>> +    mctp_prinfo("libmctp initialized");
>> +
>> +    /* Set the max message size to be large enough */
>> +    mctp_set_max_message_size(mctp, 
>> HOST_MAX_INCOMING_MESSAGE_ALLOCATION);
>> +
>> +    /* Setup the message rx callback */
>> +    mctp_set_rx_all(mctp, fn, NULL);
>> +
>> +    /* Initialise the binding */
>> +    if (mctp_backend->binding())
>> +        goto err;
>> +
>> +    /* Initializing the channel requires running the poll function
>> +     * a few times to handle the initial setup since we need the
>> +     * BMC to reply.
>> +     *
>> +     * Unfortuntately there's no good way to determine if this
>> +     * worked or not so poll for a bit with a timeout. :(
>> +     *
>> +     * We have to check (bus->state == mctp_bus_state_constructed)
>> +     * otherwise mctp_message_tx() will reject the message.
>> +     */
>> +    start = mftb();
>> +    while (tb_compare(mftb(), start + msecs_to_tb(250)) == TB_ABEFOREB)
>> +        time_wait_ms(10);
>
>
> Why not a simple time_wait_ms() for the full duration? It would call 
> the opal_run_pollers(), which check the timers.
>
>   Fred
>
>
>
>> +
>> +    mctp_prwarn("%s - The mctp bus state should be correct now !!!",
>> +            __func__);
>> +
>> +    return OPAL_SUCCESS;
>> +
>> +err:
>> +    mctp_prerr("Unable to initialize MCTP");
>> +    mctp_destroy(mctp);
>> +    mctp = NULL;
>> +
>> +    return OPAL_HARDWARE;
>> +}
>> +
>> +void ast_mctp_exit(void)
>> +{
>> +    if (mctp_backend && mctp_backend->destroy)
>> +        mctp_backend->destroy();
>> +
>> +    if (mctp)
>> +        mctp_destroy(mctp);
>> +
>> +    mctp = NULL;
>> +}
>> diff --git a/include/ast.h b/include/ast.h
>> index 5e932398..55eeae28 100644
>> --- a/include/ast.h
>> +++ b/include/ast.h
>> @@ -91,6 +91,17 @@ void ast_setup_ibt(uint16_t io_base, uint8_t irq);
>>   /* MBOX configuration */
>>   void ast_setup_sio_mbox(uint16_t io_base, uint8_t irq);
>>   +/* AST MCTP configuration */
>> +#define MCTP_BINDING_ASTLPC_MODE 0
>> +#define MCTP_BINDING_ASTPCIE_MODE 1
>> +
>> +int ast_mctp_ready(void);
>> +int ast_mctp_init(uint8_t mode, void (*fn)(uint8_t src_eid, bool 
>> tag_owner,
>> +          uint8_t msg_tag, void *data, void *msg, size_t len));
>> +void ast_mctp_exit(void);
>> +void ast_mctp_stop_polling(void);
>> +int ast_mctp_message_tx(uint8_t eid, uint8_t *msg, int len);
>> +
>>   #endif /* __SKIBOOT__ */
>>     /*

On Thu, 6 Apr 2023 at 17:16, Frederic Barrat <fbarrat@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@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.