Message-ID: <HK0PR06MB37792FACF96A533EA192B00C91139@HK0PR06MB3779.apcprd06.prod.outlook.com>
Date:   Fri, 18 Mar 2022 07:59:47 +0000
From:   ChiaWei Wang <chiawei_wang@...eedtech.com>
To:     Joel Stanley <joel@....id.au>, Arnd Bergmann <arnd@...db.de>
CC:     Rob Herring <robh+dt@...nel.org>, Andrew Jeffery <andrew@...id.au>,
        Cyril Bur <cyrilbur@...il.com>,
        devicetree <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-aspeed <linux-aspeed@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: RE: [PATCH v2 1/2] soc: aspeed: Add LPC mailbox support

Hi Joel,

> From: Joel Stanley <joel@....id.au>
> Sent: Friday, March 18, 2022 3:06 PM
> 
> On Tue, 17 Aug 2021 at 02:58, Chia-Wei Wang
> <chiawei_wang@...eedtech.com> wrote:
> >
> > The LPC mailbox controller consists of multiple general registers for
> > the communication between the Host and the BMC.
> > The interrupts for data update signaling are also introduced.
> 
> The concern with this approach is the userspace API. We don't want every
> driver to invent it's own way of communicating with usersapce.
> 
> I know when Cyril first submitted this driver it was suggested to use the kernel
> mailbox subsystem. After investigation it was decided that this hardware is not
> a fit; although it is a mailbox it is more like a IPMI BT or KCS device, that needs
> an interface to push data to and from userspace over the device.
> 

Thanks for the explanation.

The current implementation exports the mailbox register access in the file (device node) read/write way.
The read/write behavior is slightly different than a usual text file though...

The IOCTL is used only to retrieve the number of mailbox registers available.
We can consider exporting this through sysfs as a driver property, perhaps?

Is the IOCTL part the major concern? Or the file-based read/write also needs to be re-considered?

> Chai-Wei, can you link to some userspace programs that use the existing ioctl
> interface?

The kcs_bt_test APP in Aspeed GitHub has the example to access LPC mailbox with read()/write() toward its device node.
However, as kcs_bt_test is for internal test only, it is now removed in the latest release.

You can still find the source in the early release tags (e.g. v00.01.00).
https://github.com/AspeedTech-BMC/aspeed_app/blob/v00.01.00/kcs_bt_test/kcs_bt_test.c#L157

Thanks,
Chiawei

> 
> Arnd, do you have any suggestions?
> 
> >
> > Signed-off-by: Chia-Wei Wang <chiawei_wang@...eedtech.com>
> > Reported-by: kernel test robot <lkp@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> > ---
> >  drivers/soc/aspeed/Kconfig           |  10 +
> >  drivers/soc/aspeed/Makefile          |   9 +-
> >  drivers/soc/aspeed/aspeed-lpc-mbox.c | 418
> > +++++++++++++++++++++++++++
> >  3 files changed, 433 insertions(+), 4 deletions(-)  create mode
> > 100644 drivers/soc/aspeed/aspeed-lpc-mbox.c
> >
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > index 243ca196e6ad..5b31a6e2620c 100644
> > --- a/drivers/soc/aspeed/Kconfig
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -24,6 +24,16 @@ config ASPEED_LPC_SNOOP
> >           allows the BMC to listen on and save the data written by
> >           the host to an arbitrary LPC I/O port.
> >
> > +config ASPEED_LPC_MAILBOX
> > +       tristate "ASPEED LPC mailbox support"
> > +       select REGMAP
> > +       select MFD_SYSCON
> > +       default ARCH_ASPEED
> > +       help
> > +         Provides a driver to control the LPC mailbox which possesses
> > +         up to 32 data registers for the communication between the Host
> > +         and the BMC over LPC.
> > +
> >  config ASPEED_P2A_CTRL
> >         tristate "ASPEED P2A (VGA MMIO to BMC) bridge control"
> >         select REGMAP
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > index fcab7192e1a4..cde6b4514c97 100644
> > --- a/drivers/soc/aspeed/Makefile
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -1,5 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -obj-$(CONFIG_ASPEED_LPC_CTRL)  += aspeed-lpc-ctrl.o
> > -obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> > -obj-$(CONFIG_ASPEED_P2A_CTRL)  += aspeed-p2a-ctrl.o
> > -obj-$(CONFIG_ASPEED_SOCINFO)   += aspeed-socinfo.o
> > +obj-$(CONFIG_ASPEED_LPC_CTRL)          += aspeed-lpc-ctrl.o
> > +obj-$(CONFIG_ASPEED_LPC_SNOOP)         += aspeed-lpc-snoop.o
> > +obj-$(CONFIG_ASPEED_LPC_MAILBOX)       += aspeed-lpc-mbox.o
> > +obj-$(CONFIG_ASPEED_P2A_CTRL)          += aspeed-p2a-ctrl.o
> > +obj-$(CONFIG_ASPEED_SOCINFO)           += aspeed-socinfo.o
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-mbox.c
> > b/drivers/soc/aspeed/aspeed-lpc-mbox.c
> > new file mode 100644
> > index 000000000000..a09ca6a175f7
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-lpc-mbox.c
> > @@ -0,0 +1,418 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2017 IBM Corporation
> > + * Copyright 2021 Aspeed Technology Inc.
> > + */
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/poll.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define DEVICE_NAME    "aspeed-mbox"
> > +
> > +#define ASPEED_MBOX_DR(dr, n)  (dr + (n * 4))
> > +#define ASPEED_MBOX_STR(str, n)        (str + (n / 8) * 4)
> > +#define ASPEED_MBOX_BIE(bie, n)        (bie + (n / 8) * 4)
> > +#define ASPEED_MBOX_HIE(hie, n) (hie + (n / 8) * 4)
> > +
> > +#define ASPEED_MBOX_BCR_RECV   BIT(7)
> > +#define ASPEED_MBOX_BCR_MASK   BIT(1)
> > +#define ASPEED_MBOX_BCR_SEND   BIT(0)
> > +
> > +/* ioctl code */
> > +#define ASPEED_MBOX_IOCTL              0xA3
> > +#define ASPEED_MBOX_IOCTL_GET_SIZE     \
> > +       _IOR(ASPEED_MBOX_IOCTL, 0, struct aspeed_mbox_ioctl_data)
> > +
> > +struct aspeed_mbox_ioctl_data {
> > +       unsigned int data;
> > +};
> > +
> > +struct aspeed_mbox_model {
> > +       unsigned int dr_num;
> > +
> > +       /* offsets to the MBOX registers */
> > +       unsigned int dr;
> > +       unsigned int str;
> > +       unsigned int bcr;
> > +       unsigned int hcr;
> > +       unsigned int bie;
> > +       unsigned int hie;
> > +};
> > +
> > +struct aspeed_mbox {
> > +       struct miscdevice miscdev;
> > +       struct regmap *map;
> > +       unsigned int base;
> > +       wait_queue_head_t queue;
> > +       struct mutex mutex;
> > +       const struct aspeed_mbox_model *model; };
> > +
> > +static atomic_t aspeed_mbox_open_count = ATOMIC_INIT(0);
> > +
> > +static u8 aspeed_mbox_inb(struct aspeed_mbox *mbox, int reg) {
> > +       /*
> > +        * The mbox registers are actually only one byte but are addressed
> > +        * four bytes apart. The other three bytes are marked 'reserved',
> > +        * they *should* be zero but lets not rely on it.
> > +        * I am going to rely on the fact we can casually read/write to
> them...
> > +        */
> > +       unsigned int val = 0xff; /* If regmap throws an error return 0xff */
> > +       int rc = regmap_read(mbox->map, mbox->base + reg, &val);
> > +
> > +       if (rc)
> > +               dev_err(mbox->miscdev.parent, "regmap_read() failed
> with "
> > +                       "%d (reg: 0x%08x)\n", rc, reg);
> > +
> > +       return val & 0xff;
> > +}
> > +
> > +static void aspeed_mbox_outb(struct aspeed_mbox *mbox, u8 data, int
> > +reg) {
> > +       int rc = regmap_write(mbox->map, mbox->base + reg, data);
> > +
> > +       if (rc)
> > +               dev_err(mbox->miscdev.parent, "regmap_write() failed
> with "
> > +                       "%d (data: %u reg: 0x%08x)\n", rc, data, reg);
> > +}
> > +
> > +static struct aspeed_mbox *file_mbox(struct file *file) {
> > +       return container_of(file->private_data, struct aspeed_mbox,
> > +miscdev); }
> > +
> > +static int aspeed_mbox_open(struct inode *inode, struct file *file) {
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +
> > +       if (atomic_inc_return(&aspeed_mbox_open_count) == 1) {
> > +               /*
> > +                * Clear the interrupt status bit if it was left on and
> unmask
> > +                * interrupts.
> > +                * ASPEED_MBOX_BCR_RECV bit is W1C, this also
> unmasks in 1 step
> > +                */
> > +               aspeed_mbox_outb(mbox, ASPEED_MBOX_BCR_RECV,
> model->bcr);
> > +               return 0;
> > +       }
> > +
> > +       atomic_dec(&aspeed_mbox_open_count);
> > +       return -EBUSY;
> > +}
> > +
> > +static ssize_t aspeed_mbox_read(struct file *file, char __user *buf,
> > +                               size_t count, loff_t *ppos) {
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +       char __user *p = buf;
> > +       ssize_t ret;
> > +       int i;
> > +
> > +       if (!access_ok(buf, count))
> > +               return -EFAULT;
> > +
> > +       if (count + *ppos > model->dr_num)
> > +               return -EINVAL;
> > +
> > +       if (file->f_flags & O_NONBLOCK) {
> > +               if (!(aspeed_mbox_inb(mbox, model->bcr) &
> > +                               ASPEED_MBOX_BCR_RECV))
> > +                       return -EAGAIN;
> > +       } else if (wait_event_interruptible(mbox->queue,
> > +                               aspeed_mbox_inb(mbox, model->bcr)
> &
> > +                               ASPEED_MBOX_BCR_RECV)) {
> > +               return -ERESTARTSYS;
> > +       }
> > +
> > +       mutex_lock(&mbox->mutex);
> > +
> > +       for (i = *ppos; count > 0 && i < model->dr_num; i++) {
> > +               uint8_t reg = aspeed_mbox_inb(mbox,
> > + ASPEED_MBOX_DR(model->dr, i));
> > +
> > +               ret = __put_user(reg, p);
> > +               if (ret)
> > +                       goto out_unlock;
> > +
> > +               p++;
> > +               count--;
> > +       }
> > +
> > +       /* ASPEED_MBOX_BCR_RECV bit is write to clear, this also
> unmasks in 1 step */
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_BCR_RECV,
> model->bcr);
> > +       ret = p - buf;
> > +
> > +out_unlock:
> > +       mutex_unlock(&mbox->mutex);
> > +       return ret;
> > +}
> > +
> > +static ssize_t aspeed_mbox_write(struct file *file, const char __user *buf,
> > +                               size_t count, loff_t *ppos) {
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +       const char __user *p = buf;
> > +       ssize_t ret;
> > +       char c;
> > +       int i;
> > +
> > +       if (!access_ok(buf, count))
> > +               return -EFAULT;
> > +
> > +       if (count + *ppos > model->dr_num)
> > +               return -EINVAL;
> > +
> > +       mutex_lock(&mbox->mutex);
> > +
> > +       for (i = *ppos; count > 0 && i < model->dr_num; i++) {
> > +               ret = __get_user(c, p);
> > +               if (ret)
> > +                       goto out_unlock;
> > +
> > +               aspeed_mbox_outb(mbox, c,
> ASPEED_MBOX_DR(model->dr, i));
> > +               p++;
> > +               count--;
> > +       }
> > +
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_BCR_SEND,
> model->bcr);
> > +       ret = p - buf;
> > +
> > +out_unlock:
> > +       mutex_unlock(&mbox->mutex);
> > +       return ret;
> > +}
> > +
> > +static __poll_t aspeed_mbox_poll(struct file *file, poll_table *wait)
> > +{
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +       __poll_t mask = 0;
> > +
> > +       poll_wait(file, &mbox->queue, wait);
> > +
> > +       if (aspeed_mbox_inb(mbox, model->bcr) &
> ASPEED_MBOX_BCR_RECV)
> > +               mask |= POLLIN;
> > +
> > +       return mask;
> > +}
> > +
> > +static int aspeed_mbox_release(struct inode *inode, struct file
> > +*file) {
> > +       atomic_dec(&aspeed_mbox_open_count);
> > +       return 0;
> > +}
> > +
> > +static long aspeed_mbox_ioctl(struct file *file, unsigned int cmd,
> > +                                unsigned long param) {
> > +       long ret = 0;
> > +       struct aspeed_mbox *mbox = file_mbox(file);
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +       struct aspeed_mbox_ioctl_data data;
> > +
> > +       switch (cmd) {
> > +       case ASPEED_MBOX_IOCTL_GET_SIZE:
> > +               data.data = model->dr_num;
> > +               if (copy_to_user((void __user *)param, &data,
> sizeof(data)))
> > +                       ret = -EFAULT;
> > +               break;
> > +       default:
> > +               ret = -ENOTTY;
> > +               break;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct file_operations aspeed_mbox_fops = {
> > +       .owner          = THIS_MODULE,
> > +       .llseek         = no_seek_end_llseek,
> > +       .read           = aspeed_mbox_read,
> > +       .write          = aspeed_mbox_write,
> > +       .open           = aspeed_mbox_open,
> > +       .release        = aspeed_mbox_release,
> > +       .poll           = aspeed_mbox_poll,
> > +       .unlocked_ioctl = aspeed_mbox_ioctl, };
> > +
> > +static irqreturn_t aspeed_mbox_irq(int irq, void *arg) {
> > +       struct aspeed_mbox *mbox = arg;
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +
> > +       if (!(aspeed_mbox_inb(mbox, model->bcr) &
> ASPEED_MBOX_BCR_RECV))
> > +               return IRQ_NONE;
> > +
> > +       /*
> > +        * Leave the status bit set so that we know the data is for us,
> > +        * clear it once it has been read.
> > +        */
> > +
> > +       /* Mask it off, we'll clear it when we the data gets read */
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_BCR_MASK,
> model->bcr);
> > +
> > +       wake_up(&mbox->queue);
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int aspeed_mbox_config_irq(struct aspeed_mbox *mbox,
> > +               struct platform_device *pdev) {
> > +       const struct aspeed_mbox_model *model = mbox->model;
> > +       struct device *dev = &pdev->dev;
> > +       int i, rc, irq;
> > +
> > +       irq = irq_of_parse_and_map(dev->of_node, 0);
> > +       if (!irq)
> > +               return -ENODEV;
> > +
> > +       rc = devm_request_irq(dev, irq, aspeed_mbox_irq,
> > +                       IRQF_SHARED, DEVICE_NAME, mbox);
> > +       if (rc < 0) {
> > +               dev_err(dev, "Unable to request IRQ %d\n", irq);
> > +               return rc;
> > +       }
> > +
> > +       /*
> > +        * Disable all register based interrupts.
> > +        */
> > +       for (i = 0; i < model->dr_num / 8; ++i)
> > +               aspeed_mbox_outb(mbox, 0x00,
> > + ASPEED_MBOX_BIE(model->bie, i));
> > +
> > +       /* These registers are write one to clear. Clear them. */
> > +       for (i = 0; i < model->dr_num / 8; ++i)
> > +               aspeed_mbox_outb(mbox, 0xff,
> > + ASPEED_MBOX_STR(model->str, i));
> > +
> > +       aspeed_mbox_outb(mbox, ASPEED_MBOX_BCR_RECV,
> model->bcr);
> > +       return 0;
> > +}
> > +
> > +static int aspeed_mbox_probe(struct platform_device *pdev) {
> > +       struct aspeed_mbox *mbox;
> > +       struct device *dev;
> > +       int rc;
> > +
> > +       dev = &pdev->dev;
> > +
> > +       mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +       if (!mbox)
> > +               return -ENOMEM;
> > +
> > +       dev_set_drvdata(&pdev->dev, mbox);
> > +
> > +       rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
> > +       if (rc) {
> > +               dev_err(dev, "Couldn't read reg device tree property\n");
> > +               return rc;
> > +       }
> > +
> > +       mbox->model = of_device_get_match_data(dev);
> > +       if (IS_ERR(mbox->model)) {
> > +               dev_err(dev, "Couldn't get model data\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       mbox->map = syscon_node_to_regmap(
> > +                       pdev->dev.parent->of_node);
> > +       if (IS_ERR(mbox->map)) {
> > +               dev_err(dev, "Couldn't get regmap\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       mutex_init(&mbox->mutex);
> > +       init_waitqueue_head(&mbox->queue);
> > +
> > +       mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +       mbox->miscdev.name = DEVICE_NAME;
> > +       mbox->miscdev.fops = &aspeed_mbox_fops;
> > +       mbox->miscdev.parent = dev;
> > +       rc = misc_register(&mbox->miscdev);
> > +       if (rc) {
> > +               dev_err(dev, "Unable to register device\n");
> > +               return rc;
> > +       }
> > +
> > +       rc = aspeed_mbox_config_irq(mbox, pdev);
> > +       if (rc) {
> > +               dev_err(dev, "Failed to configure IRQ\n");
> > +               misc_deregister(&mbox->miscdev);
> > +               return rc;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int aspeed_mbox_remove(struct platform_device *pdev) {
> > +       struct aspeed_mbox *mbox = dev_get_drvdata(&pdev->dev);
> > +
> > +       misc_deregister(&mbox->miscdev);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct aspeed_mbox_model ast2400_model = {
> > +       .dr_num = 16,
> > +       .dr     = 0x0,
> > +       .str = 0x40,
> > +       .bcr = 0x48,
> > +       .hcr = 0x4c,
> > +       .bie = 0x50,
> > +       .hie = 0x58,
> > +};
> > +
> > +static const struct aspeed_mbox_model ast2500_model = {
> > +       .dr_num = 16,
> > +       .dr     = 0x0,
> > +       .str = 0x40,
> > +       .bcr = 0x48,
> > +       .hcr = 0x4c,
> > +       .bie = 0x50,
> > +       .hie = 0x58,
> > +};
> > +
> > +static const struct aspeed_mbox_model ast2600_model = {
> > +       .dr_num = 32,
> > +       .dr     = 0x0,
> > +       .str = 0x80,
> > +       .bcr = 0x90,
> > +       .hcr = 0x94,
> > +       .bie = 0xa0,
> > +       .hie = 0xb0,
> > +};
> > +
> > +static const struct of_device_id aspeed_mbox_match[] = {
> > +       { .compatible = "aspeed,ast2400-mbox",
> > +         .data = &ast2400_model },
> > +       { .compatible = "aspeed,ast2500-mbox",
> > +         .data = &ast2500_model },
> > +       { .compatible = "aspeed,ast2600-mbox",
> > +         .data = &ast2600_model },
> > +       { },
> > +};
> > +
> > +static struct platform_driver aspeed_mbox_driver = {
> > +       .driver = {
> > +               .name           = DEVICE_NAME,
> > +               .of_match_table = aspeed_mbox_match,
> > +       },
> > +       .probe = aspeed_mbox_probe,
> > +       .remove = aspeed_mbox_remove,
> > +};
> > +
> > +module_platform_driver(aspeed_mbox_driver);
> > +MODULE_DEVICE_TABLE(of, aspeed_mbox_match);
> MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@...il.com>");
> > +MODULE_AUTHOR("Chia-Wei Wang <chiawei_wang@...eedtech.com");
> > +MODULE_DESCRIPTION("Aspeed mailbox device driver");
> > --
> > 2.17.1
> >

Powered by blists - more mailing lists