[Skiboot] [PATCH v3 1/4] hw/lpc-mbox: Add skiboot driver for the BMC mbox regs
Suraj Jitindar Singh
sjitindarsingh at gmail.com
Tue Feb 7 11:27:39 AEDT 2017
On Tue, 2017-02-07 at 10:58 +1100, Cyril Bur wrote:
> On Mon, 2017-02-06 at 16:04 +1100, Suraj Jitindar Singh wrote:
> >
> > 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.
> Yeah can't hurt.
>
> >
> > >
> > > + 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...
> Come on wheres your #yolo. Yeah probs save ourselves from it, even if
> it should only happen if the system is hosed, can't hurt.
>
> >
> > >
> > > + 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
> You're wearing out the p key on my keyboard man ;)
http://www.wiggle.com.au/lizard-skins-frame-protector-patch-kit/
Stick some on your 'p' key
>
> >
> > >
> > > + 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.
> True, mbox_init() really should only be called once, it would be on
> crack to ever call it again, I'll add a simple check that if
> mbox.base
> isn't zero just bail.
>
> >
> > >
> > > +}
> > > +
> > > +
> > > 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.
> >
> Expert review though,
>
> Thanks,
>
> Cyril
>
> >
> > Suraj
> >
More information about the Skiboot
mailing list