[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