[PATCH linux dev-4.7 v2] drivers/fsi: Add FSI SBEFIFO driver
Joel Stanley
joel at jms.id.au
Wed May 10 18:15:45 AEST 2017
Hi Brad,
I started looking at this a while ago and didn't get around to
finishing. I'm sending my comments for the bit I did look at so you
can fix those issues.
On Fri, Apr 21, 2017 at 4:21 AM, Brad Bishop
<bradleyb at fuzziesquirrel.com> wrote:
> IBM POWER9 processors contain some embedded hardware and software bits
> collectively referred to as the self boot engine (SBE). One role of
> the SBE is to act as a proxy that provides access to the registers of
> the POWER chip from other (embedded) systems.
Is are "other embedded systems" service processors (FSP, BMC)?
> The POWER9 chip contains a hardware frontend for communicating with
> the SBE from remote systems called the SBEFIFO. The SBEFIFO logic
> is contained within an FSI CFAM (see Documentation/fsi) and as such
> the driver implements an FSI bus device.
>
> The SBE expects to communicate using a defined wire protocol; however,
> the driver knows nothing of the protocol and only provides raw access
> to the fifo device to userspace applications wishing to communicate with
> the SBE using the wire protocol.
>
> The SBEFIFO consists of two hardware fifos. The upstream fifo is used by
> the driver to transfer data to the SBE on the POWER chip, from the system
> hosting the driver. The downstream fifo is used by the driver to transfer
> data from the SBE on the power chip to the system hosting the driver.
>
> Signed-off-by: Brad Bishop <bradleyb at fuzziesquirrel.com>
> ---
> v2:
> Unexpand list_first_entry_or_null.
> Unexpand list_for_each_entry_safe.
> Reschedule a full 500ms later when fifos are busy.
> ---
> drivers/fsi/Kconfig | 6 +
> drivers/fsi/Makefile | 1 +
> drivers/fsi/fsi-sbefifo.c | 810 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 817 insertions(+)
> create mode 100644 drivers/fsi/fsi-sbefifo.c
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index c78b9b6e..087f239 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -18,6 +18,12 @@ config FSI_MASTER_GPIO
> ---help---
> This option enables a FSI master driver using GPIO lines.
>
> +config FSI_SBEFIFO
> + tristate "SBEFIFO FSI client device driver"
> + depends on FSI
> + ---help---
> + This option enables an FSI based SBEFIFO device driver.
"You should select this for kernels that run on service processors for
IBM Power9 systems" or something.
> +
> config FSI_SCOM
> tristate "SCOM FSI client device driver"
> depends on FSI
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> index 3a106ba..edd84b2 100644
> --- a/drivers/fsi/Makefile
> +++ b/drivers/fsi/Makefile
> @@ -1,5 +1,6 @@
>
> obj-$(CONFIG_FSI) += fsi-core.o
> obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
> +obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
> obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
> obj-$(CONFIG_FSI_I2C) += i2c/
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> new file mode 100644
> index 0000000..c289508
> --- /dev/null
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -0,0 +1,810 @@
> +/*
> + * Copyright (C) IBM Corporation 2017
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Check out the wording we use in other drivers.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/idr.h>
> +#include <linux/fsi.h>
> +#include <linux/list.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <linux/uaccess.h>
> +
> +/*
> + * The SBEFIFO is a pipe-like FSI device for communicating with
> + * the self boot engine on POWER processors.
> + */
> +
> +#define DEVICE_NAME "sbefifo"
> +#define FSI_ENGID_SBE 0x22
Should these engine IDs be kept in in a common fsi header?
> +#define SBEFIFO_BUF_CNT 32
> +
> +#define SBEFIFO_UP 0x00 /* Up register offset */
> +#define SBEFIFO_DWN 0x40 /* Down register offset */
> +
> +#define SBEFIFO_STS 0x04
> +#define SBEFIFO_EMPTY BIT(20)
Are STS and EOT_RAISE bits too? It's confusing to have a mixture.
> +#define SBEFIFO_EOT_RAISE 0x08
> +#define SBEFIFO_EOT_MAGIC 0xffffffff
> +#define SBEFIFO_EOT_ACK 0x14
> +
> +struct sbefifo {
> + struct timer_list poll_timer;
> + struct fsi_device *fsi_dev;
> + struct miscdevice mdev;
> + wait_queue_head_t wait;
> + struct list_head link;
> + struct list_head xfrs;
> + struct kref kref;
I tried to work out what the kref was counting. Can you explain how
the reference and timers are used?
> + spinlock_t lock;
> + char name[32];
The miscdev has a name too. What do we need this one for?
> + int idx;
> + int rc;
Try to create some groupings here for things that are common.
struct sbefifo {
struct fsi_device *fsi_dev;
struct miscdevice mdev;
int idx;
struct timer_list poll_timer;
wait_queue_head_t wait;
struct list_head link;
struct list_head xfrs;
struct kref kref;
spinlock_t lock;
int rc;
}
> +};
> +
> +struct sbefifo_buf {
> + u32 buf[SBEFIFO_BUF_CNT];
> + unsigned long flags;
> +#define SBEFIFO_BUF_FULL 1
> + u32 *rpos;
> + u32 *wpos;
> +};
> +
> +struct sbefifo_xfr {
> + struct sbefifo_buf *rbuf;
> + struct sbefifo_buf *wbuf;
> + struct list_head client;
> + struct list_head xfrs;
> + unsigned long flags;
> +#define SBEFIFO_XFR_WRITE_DONE 1
> +#define SBEFIFO_XFR_RESP_PENDING 2
> +#define SBEFIFO_XFR_COMPLETE 3
> +#define SBEFIFO_XFR_CANCEL 4
> +};
> +
> +struct sbefifo_client {
> + struct sbefifo_buf rbuf;
> + struct sbefifo_buf wbuf;
> + struct list_head xfrs;
> + struct sbefifo *dev;
> + struct kref kref;
> +};
> +
> +static struct list_head sbefifo_fifos;
> +
> +static DEFINE_IDA(sbefifo_ida);
> +
> +static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
> +{
> + return fsi_device_read(sbefifo->fsi_dev, reg, word, sizeof(*word));
> +}
> +
> +static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
> +{
> + return fsi_device_write(sbefifo->fsi_dev, reg, &word, sizeof(word));
> +}
> +
> +static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
> +{
> + return sbefifo_inw(sbefifo, SBEFIFO_DWN, word);
> +}
> +
> +static int sbefifo_writew(struct sbefifo *sbefifo, u32 word)
> +{
> + return sbefifo_outw(sbefifo, SBEFIFO_UP, word);
> +}
> +
> +static int sbefifo_ack_eot(struct sbefifo *sbefifo)
> +{
> + u32 discard;
> + int ret;
> +
> + /* Discard the EOT word. */
> + ret = sbefifo_readw(sbefifo, &discard);
> + if (ret)
> + return ret;
> +
> + return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
> + SBEFIFO_EOT_MAGIC);
> +}
> +
> +static size_t sbefifo_dev_nwreadable(u32 sts)
> +{
> + static const u32 FIFO_NTRY_CNT_MSK = 0x000f0000;
> + static const unsigned int FIFO_NTRY_CNT_SHIFT = 16;
> +
> + return (sts & FIFO_NTRY_CNT_MSK) >> FIFO_NTRY_CNT_SHIFT;
> +}
> +
> +static size_t sbefifo_dev_nwwriteable(u32 sts)
> +{
> + static const size_t FIFO_DEPTH = 8;
> +
> + return FIFO_DEPTH - sbefifo_dev_nwreadable(sts);
> +}
> +
> +static void sbefifo_buf_init(struct sbefifo_buf *buf)
> +{
> + WRITE_ONCE(buf->rpos, buf->buf);
> + WRITE_ONCE(buf->wpos, buf->buf);
> +}
> +
> +static size_t sbefifo_buf_nbreadable(struct sbefifo_buf *buf)
non-blocking readable?
> +{
> + size_t n;
> + u32 *rpos = READ_ONCE(buf->rpos);
> + u32 *wpos = READ_ONCE(buf->wpos);
> +
> + if (test_bit(SBEFIFO_BUF_FULL, &buf->flags))
> + n = SBEFIFO_BUF_CNT;
> + else if (rpos <= wpos)
> + n = wpos - rpos;
> + else
> + n = (buf->buf + SBEFIFO_BUF_CNT) - rpos;
> +
> + return n << 2;
> +}
> +
> +static size_t sbefifo_buf_nbwriteable(struct sbefifo_buf *buf)
> +{
> + size_t n;
> + u32 *rpos = READ_ONCE(buf->rpos);
> + u32 *wpos = READ_ONCE(buf->wpos);
> +
> + if (test_bit(SBEFIFO_BUF_FULL, &buf->flags))
> + n = 0;
> + else if (wpos < rpos)
> + n = rpos - wpos;
> + else
> + n = (buf->buf + SBEFIFO_BUF_CNT) - wpos;
> +
> + return n << 2;
> +}
> +
> +/*
> + * Update pointers and flags after doing a buffer read. Return true if the
> + * buffer is now empty;
> + */
> +static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
n is number? or the length of the buffer?
> +{
> + u32 *rpos = READ_ONCE(buf->rpos);
> + u32 *wpos = READ_ONCE(buf->wpos);
> +
> + if (n)
> + clear_bit(SBEFIFO_BUF_FULL, &buf->flags);
> +
> + rpos += (n >> 2);
> + if (rpos == buf->buf + SBEFIFO_BUF_CNT)
> + rpos = buf->buf;
> +
> + WRITE_ONCE(buf->rpos, rpos);
> + return rpos == wpos;
> +}
> +
More information about the openbmc
mailing list