[Skiboot] [PATCH v3 1/4] hw/lpc-mbox: Add skiboot driver for the BMC mbox regs
Suraj Jitindar Singh
sjitindarsingh at gmail.com
Mon Feb 6 16:04:54 AEDT 2017
On Thu, 2017-02-02 at 17:54 +1100, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> hw/Makefile.inc | 2 +-
> hw/lpc-mbox.c | 278
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/lpc-mbox.h | 58 +++++++++++
> include/skiboot.h | 1 +
> 4 files changed, 338 insertions(+), 1 deletion(-)
> create mode 100644 hw/lpc-mbox.c
> create mode 100644 include/lpc-mbox.h
>
> diff --git a/hw/Makefile.inc b/hw/Makefile.inc
> index f2dc328e..d87f85e4 100644
> --- a/hw/Makefile.inc
> +++ b/hw/Makefile.inc
> @@ -6,7 +6,7 @@ HW_OBJS += nx.o nx-rng.o nx-crypto.o nx-842.o
> HW_OBJS += p7ioc.o p7ioc-inits.o p7ioc-phb.o
> HW_OBJS += phb3.o sfc-ctrl.o fake-rtc.o bt.o p8-i2c.o prd.o
> HW_OBJS += dts.o lpc-rtc.o npu.o npu-hw-procedures.o xive.o phb4.o
> -HW_OBJS += fake-nvram.o
> +HW_OBJS += fake-nvram.o lpc-mbox.o
> HW=hw/built-in.o
>
> # FIXME hack this for now
> diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c
> new file mode 100644
> index 00000000..2376f45f
> --- /dev/null
> +++ b/hw/lpc-mbox.c
> @@ -0,0 +1,278 @@
> +/* Copyright 2017 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions
> and
> + * limitations under the License.
> + */
> +
> +#define pr_fmt(fmt) "LPC-MBOX: " fmt
> +
> +#include <skiboot.h>
> +#include <lpc.h>
> +#include <console.h>
> +#include <opal.h>
> +#include <device.h>
> +#include <interrupts.h>
> +#include <processor.h>
> +#include <errorlog.h>
> +#include <trace.h>
> +#include <timebase.h>
> +#include <timer.h>
> +#include <cpu.h>
> +#include <chip.h>
> +#include <io.h>
> +
> +#include <lpc-mbox.h>
> +
> +#define MBOX_FLAG_REG 0x0f
> +#define MBOX_STATUS_0 0x10
> +#define MBOX_STATUS_ATTN (1 << 7)
> +#define MBOX_STATUS_1 0x11
> +#define MBOX_BMC_CTRL 0x12
> +#define MBOX_CTRL_INT_STATUS (1 << 7)
> +#define MBOX_CTRL_INT_MASK (1 << 1)
> +#define MBOX_CTRL_INT_SEND (1 << 0)
> +#define MBOX_HOST_CTRL 0x13
> +#define MBOX_BMC_INT_EN_0 0x14
> +#define MBOX_BMC_INT_EN_1 0x15
> +#define MBOX_HOST_INT_EN_0 0x16
> +#define MBOX_HOST_INT_EN_1 0x17
> +
> +#define MBOX_MAX_QUEUE_LEN 5
> +
> +#define BMC_RESET 1
> +#define BMC_COMPLETE 2
> +
> +struct mbox {
> + uint32_t base;
> + int queue_len;
> + bool irq_ok;
> + uint8_t seq;
> + struct timer poller;
> + struct lock lock; /* Protect in_flight */
> + struct bmc_mbox_msg *in_flight;
> +};
> +
> +static struct mbox mbox;
> +
> +/*
> + * MBOX accesses
> + */
> +
> +static void bmc_mbox_outb(uint8_t val, uint8_t reg)
> +{
> + lpc_outb(val, mbox.base + reg);
> +}
> +
> +static uint8_t bmc_mbox_inb(uint8_t reg)
> +{
> + return lpc_inb(mbox.base + reg);
> +}
> +
> +static void bmc_mbox_recv_message(struct bmc_mbox_msg *msg)
> +{
> + int i;
> +
> + msg->response = bmc_mbox_inb(13);
> + msg->seq = bmc_mbox_inb(1);
Is it worth #defining these register offsets
e.g.
#define MBOX_REG_CMD 0
#define MBOX_REG_SEQ 1
...
I mean you probably know what they are if you're reading/writing this
code and they are defined in the protocol, but might slightly improve
code readability/clarity.
> + prlog(PR_DEBUG, "Receving message resp %d seq: %d\n",
> + msg->response, msg->seq);
> + for (i = 0; i < BMC_MBOX_DATA_BYTES; i++) {
> + msg->data[i] = bmc_mbox_inb(i + 2);
> + prlog(PR_TRACE, "Read byte %d val 0x%02x\n", i, msg-
> >data[i]);
> + }
> + prlog(PR_DEBUG, "Done\n");
> +}
> +
> +/* This needs work, don't write the data bytes that aren't needed */
> +static void bmc_mbox_send_message(struct bmc_mbox_msg *msg)
> +{
> + int i;
> +
> + if (!lpc_ok())
> + /* We're going to have to handle this better */
> + prlog(PR_ERR, "LPC isn't ok\n");
fair enough I guess
> + prlog(PR_DEBUG, "Sending command %d seq %d\n", msg->command,
> msg->seq);
> + bmc_mbox_outb(msg->command, 0);
> + bmc_mbox_outb(msg->seq, 1);
>
As above
> + for (i = 0; i < BMC_MBOX_DATA_BYTES; i++) {
> + prlog(PR_TRACE, "Sending byte %d val %d\n", i + 2,
> msg->data[i]);
> + bmc_mbox_outb(msg->data[i], i + 2);
> + }
> +
> + /* Ping */
> + prlog(PR_DEBUG, "Sending BMC interrupt\n");
> + bmc_mbox_outb(MBOX_CTRL_INT_SEND, MBOX_HOST_CTRL);
> +}
> +
> +int bmc_mbox_enqueue(struct bmc_mbox_msg *msg)
> +{
> + if (!mbox.base) {
> + prlog(PR_CRIT, "Using MBOX without init!\n");
> + return OPAL_WRONG_STATE;
> + }
> +
> + lock(&mbox.lock);
> + if (mbox.in_flight) {
> + prlog(PR_DEBUG, "MBOX message already in flight\n");
> + unlock(&mbox.lock);
> + return OPAL_BUSY;
> + }
> +
> + mbox.in_flight = msg;
> + unlock(&mbox.lock);
> +
> + msg->seq = ++mbox.seq;
> + bmc_mbox_send_message(msg);
> +
> + schedule_timer(&mbox.poller, mbox.irq_ok ?
> + TIMER_POLL :
> msecs_to_tb(MBOX_DEFAULT_POLL_MS));
> +
> + return 0;
> +}
> +
> +static void mbox_poll(struct timer *t __unused, void *data __unused,
> + uint64_t now __unused)
> +{
> + struct bmc_mbox_msg *msg;
> +
> + /* This is a 'registered' the message you just sent me */
That sentence doesn't make sense at all...
> + if (bmc_mbox_inb(MBOX_HOST_CTRL) & MBOX_CTRL_INT_STATUS) {
> + /* W1C on that reg */
> + bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
> +
> + prlog(PR_INSANE, "Got a regular interrupt\n");
> + /*
> + * Better implementations could allow for having
> + * mulitiple outstanding messages to the BMC, in
> that
> + * case the responded message wouldn't necessarily
> be
> + * list_top()
> + */
> + /*
> + * This should be safe lockless
> + */
> + msg = mbox.in_flight;
> + if (msg == NULL) {
> + prlog(PR_CRIT, "Couldn't find the
> message!!\n");
> + return;
> + }
Should you check that the sequence numbers match? It may not be
possible for a mismatch at this stage but in the event you allow
multiple messages in the queue it seems like you should esp. given you
test for other things which for now are impossible.
> + bmc_mbox_recv_message(msg);
> + msg->callback(msg);
Do you trust were this is being set? Should you check
if(msg->callback)
msg->callback(msg);
else
// ...problem...
> + prlog(PR_INSANE, "Callback returned\n");
> +
> + /* Yeah we'll need locks here */
> + lock(&mbox.lock);
> + mbox.in_flight = NULL;
> + unlock(&mbox.lock);
> + }
> +
> + /* This is to indicate that the BMC has information to tell
> us */
> + if (bmc_mbox_inb(MBOX_STATUS_1) & MBOX_STATUS_ATTN) {
> + uint8_t action;
> +
> + /* W1C on that reg */
> + bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_STATUS_1);
> +
> + action = bmc_mbox_inb(MBOX_FLAG_REG);
> + prlog(PR_INSANE, "Got a status register interrupt
> with action 0x%02x\n",
> + action);
> +
> + if (action & BMC_RESET) {
> + /* TODO Freak */
> + prlog(PR_WARNING, "BMC reset detected\n");
> + action &= ~BMC_RESET;
> + }
> +
> + if (action)
> + prlog(PR_ERR, "Got a status bit set that
> don't know about: 0x%02x\n",
> + action);
> + }
> +
> + schedule_timer(&mbox.poller,
> + mbox.irq_ok ? TIMER_POLL :
> msecs_to_tb(MBOX_DEFAULT_POLL_MS));
> +}
> +
> +static void mbox_irq(uint32_t chip_id __unused, uint32_t irq_mask
> __unused)
> +{
> + mbox.irq_ok = true;
> + mbox_poll(NULL, NULL, 0);
> +}
> +
> +static struct lpc_client mbox_lpc_client = {
> + .interrupt = mbox_irq,
> +};
> +
> +static bool mbox_init_hw(void)
> +{
> + /*
> + * Turns out there isn't anything to do.
> + * It might be a good idea to santise the registers though.
> + * TODO
> + */
> + return true;
> +}
> +
> +void mbox_init(void)
> +{
> + const struct dt_property *prop;
> + struct dt_node *n;
*nit*
np is usually used for node pointers
> + uint32_t irq, chip_id;
> +
> + prlog(PR_DEBUG, "Attempting mbox init\n");
> + n = dt_find_compatible_node(dt_root, NULL, "mbox");
> + if (!n) {
> + prlog(PR_ERR, "No device tree entry\n");
> + return;
> + }
> +
> + /* Read the interrupts property if any */
> + irq = dt_prop_get_u32_def(n, "interrupts", 0);
> + if (!irq) {
> + prlog(PR_ERR, "No interrupts property\n");
> + return;
> + }
> +
> + if (!lpc_present()) {
> + prlog(PR_ERR, "LPC not present\n");
> + return;
> + }
> +
> + /* Get IO base */
> + prop = dt_find_property(n, "reg");
> + if (!prop) {
> + prlog(PR_ERR, "Can't find reg property\n");
> + return;
> + }
> + if (dt_property_get_cell(prop, 0) != OPAL_LPC_IO) {
> + prlog(PR_ERR, "Only supports IO addresses\n");
> + return;
> + }
> + mbox.base = dt_property_get_cell(prop, 1);
> +
> + if (!mbox_init_hw()) {
> + prlog(PR_DEBUG, "Couldn't init HW\n");
> + return;
> + }
> +
> + mbox.queue_len = 0;
> + mbox.in_flight = NULL;
> + init_lock(&mbox.lock);
> +
> + init_timer(&mbox.poller, mbox_poll, NULL);
> +
> + chip_id = dt_get_chip_id(n);
> + mbox_lpc_client.interrupts = LPC_IRQ(irq);
> + lpc_register_client(chip_id, &mbox_lpc_client);
> + prlog(PR_DEBUG, "Using %d chipid and %d IRQ at 0x%08x\n",
> chip_id, irq, mbox.base);
Do we need to limit mbox_init to only being called once? One reason I
can think of is because it sets the .in_flight to null without taking
the lock, for example.
> +}
> +
> +
> diff --git a/include/lpc-mbox.h b/include/lpc-mbox.h
> new file mode 100644
> index 00000000..f9a54d9a
> --- /dev/null
> +++ b/include/lpc-mbox.h
> @@ -0,0 +1,58 @@
> +/* Copyright 2017 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions
> and
> + * limitations under the License.
> + */
> +
> +#ifndef __LPC_MBOX_H
> +#define __LPC_MBOX_H
> +
> +#include <opal.h>
> +#include <ccan/endian/endian.h>
> +
> +#define BMC_MBOX_DATA_BYTES 11
> +
> +#define MBOX_C_RESET_STATE 0x01
> +#define MBOX_C_GET_MBOX_INFO 0x02
> +#define MBOX_C_GET_FLASH_INFO 0x03
> +#define MBOX_C_CREATE_READ_WINDOW 0x04
> +#define MBOX_C_CLOSE_WINDOW 0x05
> +#define MBOX_C_CREATE_WRITE_WINDOW 0x06
> +#define MBOX_C_MARK_WRITE_DIRTY 0x07
> +#define MBOX_C_WRITE_FLUSH 0x08
> +#define MBOX_C_BMC_EVENT_ACK 0x09
> +
> +#define MBOX_R_SUCCESS 0x01
> +#define MBOX_R_PARAM_ERROR 0x02
> +#define MBOX_R_WRITE_ERROR 0x03
> +#define MBOX_R_SYSTEM_ERROR 0x04
> +#define MBOX_R_TIMEOUT 0x05
> +
> +/* Default poll interval before interrupts are working */
> +#define MBOX_DEFAULT_POLL_MS 200
> +
> +struct bmc_mbox_msg {
> + uint8_t command;
> + uint8_t seq;
> + uint8_t data[BMC_MBOX_DATA_BYTES];
> + uint8_t response;
> + uint8_t host;
> + uint8_t bmc;
> + void (*callback)(struct bmc_mbox_msg *);
> + void *priv;
> + struct list_node link;
> +};
> +
> +int bmc_mbox_enqueue(struct bmc_mbox_msg *msg);
> +
> +#endif /* __LPC_MBOX_H */
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 2ea64de3..bc716f9f 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -216,6 +216,7 @@ extern int phb4_preload_capp_ucode(void);
> extern void phb4_preload_vpd(void);
> extern void probe_npu(void);
> extern void uart_init(void);
> +extern void mbox_init(void);
> extern void homer_init(void);
> extern void occ_pstates_init(void);
> extern void slw_init(void);
I'm no expert, but everything else looks sensible.
Suraj
More information about the Skiboot
mailing list