[PATCH linux 1/2] dt-bindings: misc: Add bindings for Aspeed ast2400/2500 LPC mbox registers

[PATCH linux 1/2] dt-bindings: misc: Add bindings for Aspeed ast2400/2500 LPC mbox registers

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
Before I attempt to upstream this again, I thought I would run it
past this list.
Joel: I'm quite sure what you're carrying in the tree at the moment
is the same plus or minus some whitespace. I've made strong use of
sed to remove any 'mailbox' reference.
If you could double check, thanks.

When I do upstream I'll have a cover-letter which will explain how
Aspeed haven't made our naming lives easy by calling these registers
'mbox'. It makes it impossible to justify calling this driver
anything else but it does expose us to the confusion as to why it
isn't in drivers/mailbox, which really isn't the place for it.

 .../bindings/misc/aspeed,ast2400-mbox.txt          | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt

diff --git a/Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt b/Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt
new file mode 100644
index 000000000000..b33276e058d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt
@@ -0,0 +1,35 @@
+* Aspeed MBOX Registers
+
+The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
+(BaseBoard Management Controllers) and the MBOX registers can be used
+to perform in-band communication with their host.
+
+Required properties:
+
+- compatible : should be one of
+	"aspeed,ast2400-mbox"
+	"aspeed,ast2500-mbox"
+- reg: physical address and size of the registers
+- interrupts: interrupt number to use for the device
+
+The mbox registers are attached to the LPC bus on AST2400 and AST2500
+platforms, as such the mbox node should exist as a child of the LPC
+node.
+
+Example:
+
+	lpc@1e789000 {
+		compatible = "aspeed,ast2500-lpc", "simple-mfd";
+		reg = <0x1e789000 0x1000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		...
+
+		mbox@180 {
+			compatible = "aspeed,ast2400-mbox";
+			reg = <0x180 0x18>;
+			interrupts = <46>;
+		};
+	};
-- 
2.16.2

[PATCH linux 2/2] drivers/misc: Add Aspeed LPC mbox register driver

[Cyril Bur]

This provides access to the LPC mbox registers on the ast2400 and
ast2500 SoCs.

This driver allows arbitrary reads and writes to the 16 mbox data registers
on the Aspeed SoC LPC bus. These Aspeed chips are often used in BMC
systems. This access allows for an inband communication channel with its
host.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/misc/Kconfig           |   8 +
 drivers/misc/Makefile          |   1 +
 drivers/misc/aspeed-lpc-mbox.c | 343 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 352 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-mbox.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 03605f8fc0dc..ea1a723d0db3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -488,6 +488,14 @@ 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_MBOX
+	tristate "Aspeed ast2400/2500 LPC MBOX Controller"
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	help
+	  Provides a driver for the LPC attached MBOX registers on Aspeed
+	  SoCs (ast2400/2500). This allows for interrupt driven
+	  communication between a HOST and BMC.
+
 config PCI_ENDPOINT_TEST
 	depends on PCI
 	select CRC32
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c3c8624f4d95..32b1db960f33 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
+obj-$(CONFIG_ASPEED_LPC_MBOX)	+= aspeed-lpc-mbox.o
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
 obj-$(CONFIG_OCXL)		+= ocxl/
 obj-$(CONFIG_MISC_RTSX)		+= cardreader/
diff --git a/drivers/misc/aspeed-lpc-mbox.c b/drivers/misc/aspeed-lpc-mbox.c
new file mode 100644
index 000000000000..c4e468ab3524
--- /dev/null
+++ b/drivers/misc/aspeed-lpc-mbox.c
@@ -0,0 +1,343 @@
+/*
+ * Copyright 2018 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of_irq.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_NUM_REGS 16
+
+#define ASPEED_MBOX_DATA_0 0x00
+#define ASPEED_MBOX_STATUS_0 0x40
+#define ASPEED_MBOX_STATUS_1 0x44
+#define ASPEED_MBOX_BMC_CTRL 0x48
+#define   ASPEED_MBOX_CTRL_RECV BIT(7)
+#define   ASPEED_MBOX_CTRL_MASK BIT(1)
+#define   ASPEED_MBOX_CTRL_SEND BIT(0)
+#define ASPEED_MBOX_HOST_CTRL 0x4c
+#define ASPEED_MBOX_INTERRUPT_0 0x50
+#define ASPEED_MBOX_INTERRUPT_1 0x54
+
+struct aspeed_mbox {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	unsigned int		base;
+	wait_queue_head_t	queue;
+	struct mutex		mutex;
+};
+
+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->regmap, 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->regmap, 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);
+
+	if (atomic_inc_return(&aspeed_mbox_open_count) == 1) {
+		/*
+		 * Clear the interrupt status bit if it was left on and unmask
+		 * interrupts.
+		 * ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
+		 */
+		aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV,
+				ASPEED_MBOX_BMC_CTRL);
+		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);
+	char __user *p = buf;
+	ssize_t ret;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	if (count + *ppos > ASPEED_MBOX_NUM_REGS)
+		return -EINVAL;
+
+	if (file->f_flags & O_NONBLOCK) {
+		if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
+			ASPEED_MBOX_CTRL_RECV))
+			return -EAGAIN;
+	} else if (wait_event_interruptible(mbox->queue,
+			aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
+			ASPEED_MBOX_CTRL_RECV)) {
+		return -ERESTARTSYS;
+	}
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
+		uint8_t reg = aspeed_mbox_inb(mbox,
+				ASPEED_MBOX_DATA_0 + (i * 4));
+
+		ret = __put_user(reg, p);
+		if (ret)
+			goto out_unlock;
+
+		p++;
+		count--;
+	}
+
+	/*
+	 * ASPEED_MBOX_CTRL_RECV bit is write to clear
+	 * also unmasks in 1 step
+	 */
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV,
+			ASPEED_MBOX_BMC_CTRL);
+	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 char __user *p = buf;
+	ssize_t ret;
+	char c;
+	int i;
+
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	if (count + *ppos > ASPEED_MBOX_NUM_REGS)
+		return -EINVAL;
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
+		ret = __get_user(c, p);
+		if (ret)
+			goto out_unlock;
+
+		aspeed_mbox_outb(mbox, c, ASPEED_MBOX_DATA_0 + (i * 4));
+		p++;
+		count--;
+	}
+
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_SEND, ASPEED_MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static unsigned int aspeed_mbox_poll(struct file *file, poll_table *wait)
+{
+	struct aspeed_mbox *mbox = file_mbox(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &mbox->queue, wait);
+
+	if (aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) & ASPEED_MBOX_CTRL_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 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,
+};
+
+static irqreturn_t aspeed_mbox_irq(int irq, void *arg)
+{
+	struct aspeed_mbox *mbox = arg;
+
+	if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL)
+				& ASPEED_MBOX_CTRL_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_CTRL_MASK, ASPEED_MBOX_BMC_CTRL);
+
+	wake_up(&mbox->queue);
+	return IRQ_HANDLED;
+}
+
+static int aspeed_mbox_config_irq(struct aspeed_mbox *mbox,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int 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.
+	 */
+	aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_0); /* regs 0 - 7 */
+	aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_1); /* regs 8 - 15 */
+
+	/* These registers are write one to clear. Clear them. */
+	aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_0);
+	aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_1);
+
+	aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
+	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->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(mbox->regmap)) {
+		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 of_device_id aspeed_mbox_match[] = {
+	{ .compatible = "aspeed,ast2400-mbox" },
+	{ .compatible = "aspeed,ast2500-mbox" },
+	{ },
+};
+
+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@gmail.com>");
+MODULE_DESCRIPTION("Aspeed LPC MBOX reg device driver");
-- 
2.16.2

Re: [PATCH linux 1/2] dt-bindings: misc: Add bindings for Aspeed ast2400/2500 LPC mbox registers

[Joel Stanley]

On Wed, Feb 21, 2018 at 1:55 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> Before I attempt to upstream this again, I thought I would run it
> past this list.
> Joel: I'm quite sure what you're carrying in the tree at the moment
> is the same plus or minus some whitespace. I've made strong use of
> sed to remove any 'mailbox' reference.
> If you could double check, thanks.
>
> When I do upstream I'll have a cover-letter which will explain how
> Aspeed haven't made our naming lives easy by calling these registers
> 'mbox'. It makes it impossible to justify calling this driver
> anything else but it does expose us to the confusion as to why it
> isn't in drivers/mailbox, which really isn't the place for it.
>
>  .../bindings/misc/aspeed,ast2400-mbox.txt          | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt
>
> diff --git a/Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt b/Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt
> new file mode 100644
> index 000000000000..b33276e058d6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed,ast2400-mbox.txt

Please add this to
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt under the Host
Node Children header. See this one for an example:

 https://patchwork.kernel.org/patch/10227223/

> @@ -0,0 +1,35 @@
> +* Aspeed MBOX Registers
> +
> +The Aspeed SOCs (AST2400 and AST2500) are commonly used as BMCs
> +(BaseBoard Management Controllers) and the MBOX registers can be used

You can lose the part before 'and'.

> +to perform in-band communication with their host.
> +
> +Required properties:
> +
> +- compatible : should be one of
> +       "aspeed,ast2400-mbox"
> +       "aspeed,ast2500-mbox"
> +- reg: physical address and size of the registers
> +- interrupts: interrupt number to use for the device

Make sure this matches the other examples.


> +
> +The mbox registers are attached to the LPC bus on AST2400 and AST2500
> +platforms, as such the mbox node should exist as a child of the LPC
> +node.

You can lose this paragraph too.

> +
> +Example:
> +
> +       lpc@1e789000 {
> +               compatible = "aspeed,ast2500-lpc", "simple-mfd";
> +               reg = <0x1e789000 0x1000>;
> +
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               ...
> +
> +               mbox@180 {
> +                       compatible = "aspeed,ast2400-mbox";
> +                       reg = <0x180 0x18>;
> +                       interrupts = <46>;
> +               };

Just include the mbox node, drop the lpc bit.

The bindings I have in the dev-4.13 tree have 0x5c as the size.
Looking at the datasheet it looks like 0x5c is correct.

Cheers,

Joel

Re: [PATCH linux 2/2] drivers/misc: Add Aspeed LPC mbox register driver

[Joel Stanley]

On Wed, Feb 21, 2018 at 1:55 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> This provides access to the LPC mbox registers on the ast2400 and
> ast2500 SoCs.
>
> This driver allows arbitrary reads and writes to the 16 mbox data registers

arbitrary sounds scary. Perhaps drop that word?

> on the Aspeed SoC LPC bus. These Aspeed chips are often used in BMC

ASPEED is all caps. I didn't used to, but I'm trying to converge on that.

> systems. This access allows for an inband communication channel with its
> host.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/misc/Kconfig           |   8 +
>  drivers/misc/Makefile          |   1 +
>  drivers/misc/aspeed-lpc-mbox.c | 343 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 352 insertions(+)
>  create mode 100644 drivers/misc/aspeed-lpc-mbox.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 03605f8fc0dc..ea1a723d0db3 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -488,6 +488,14 @@ 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_MBOX
> +       tristate "Aspeed ast2400/2500 LPC MBOX Controller"
> +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> +       help
> +         Provides a driver for the LPC attached MBOX registers on Aspeed
> +         SoCs (ast2400/2500). This allows for interrupt driven

delete (ast2400/2500)

> +         communication between a HOST and BMC.

s/HOST/host/

> +
>  config PCI_ENDPOINT_TEST
>         depends on PCI
>         select CRC32
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c3c8624f4d95..32b1db960f33 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)         += cxl/
>  obj-$(CONFIG_ASPEED_LPC_CTRL)  += aspeed-lpc-ctrl.o
>  obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> +obj-$(CONFIG_ASPEED_LPC_MBOX)  += aspeed-lpc-mbox.o
>  obj-$(CONFIG_PCI_ENDPOINT_TEST)        += pci_endpoint_test.o
>  obj-$(CONFIG_OCXL)             += ocxl/
>  obj-$(CONFIG_MISC_RTSX)                += cardreader/
> diff --git a/drivers/misc/aspeed-lpc-mbox.c b/drivers/misc/aspeed-lpc-mbox.c
> new file mode 100644
> index 000000000000..c4e468ab3524
> --- /dev/null
> +++ b/drivers/misc/aspeed-lpc-mbox.c
> @@ -0,0 +1,343 @@
> +/*
> + * Copyright 2018 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */

Cool kids do this one now:

// SPDX-License-Identifier: GPL-2.0+
// Copyright 2018 IBM Corporation

> +
> +#include <linux/interrupt.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.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_NUM_REGS 16
> +
> +#define ASPEED_MBOX_DATA_0 0x00
> +#define ASPEED_MBOX_STATUS_0 0x40
> +#define ASPEED_MBOX_STATUS_1 0x44
> +#define ASPEED_MBOX_BMC_CTRL 0x48
> +#define   ASPEED_MBOX_CTRL_RECV BIT(7)
> +#define   ASPEED_MBOX_CTRL_MASK BIT(1)
> +#define   ASPEED_MBOX_CTRL_SEND BIT(0)
> +#define ASPEED_MBOX_HOST_CTRL 0x4c
> +#define ASPEED_MBOX_INTERRUPT_0 0x50
> +#define ASPEED_MBOX_INTERRUPT_1 0x54
> +
> +struct aspeed_mbox {
> +       struct miscdevice       miscdev;
> +       struct regmap           *regmap;
> +       unsigned int            base;
> +       wait_queue_head_t       queue;
> +       struct mutex            mutex;
> +};
> +
> +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->regmap, 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;
> +}

This is a mess. I'd prefer:

static u8 aspeed_mbox_inb(struct aspeed_mbox *mbox, int reg)
{
       int val;

       regmap_read(mbox->regmap, mbox->base + reg, &val);

       return val;
}

static void aspeed_mbox_outb(struct aspeed_mbox *mbox, u8 data, int reg)
{
       regmap_write(mbox->regmap, mbox->base + reg, data);
}

Reads and writes to a mmio address will never fail. Regmap incurs
enough overhead already; we don't need to add more in the driver.

> +
> +static void aspeed_mbox_outb(struct aspeed_mbox *mbox, u8 data, int reg)
> +{
> +       int rc = regmap_write(mbox->regmap, 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);
> +
> +       if (atomic_inc_return(&aspeed_mbox_open_count) == 1) {
> +               /*
> +                * Clear the interrupt status bit if it was left on and unmask
> +                * interrupts.
> +                * ASPEED_MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
> +                */
> +               aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV,
> +                               ASPEED_MBOX_BMC_CTRL);
> +               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);
> +       char __user *p = buf;
> +       ssize_t ret;
> +       int i;
> +
> +       if (!access_ok(VERIFY_WRITE, buf, count))
> +               return -EFAULT;
> +
> +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> +               return -EINVAL;
> +
> +       if (file->f_flags & O_NONBLOCK) {
> +               if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> +                       ASPEED_MBOX_CTRL_RECV))
> +                       return -EAGAIN;
> +       } else if (wait_event_interruptible(mbox->queue,
> +                       aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) &
> +                       ASPEED_MBOX_CTRL_RECV)) {
> +               return -ERESTARTSYS;
> +       }
> +
> +       mutex_lock(&mbox->mutex);
> +
> +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> +               uint8_t reg = aspeed_mbox_inb(mbox,
> +                               ASPEED_MBOX_DATA_0 + (i * 4));
> +
> +               ret = __put_user(reg, p);
> +               if (ret)
> +                       goto out_unlock;
> +
> +               p++;
> +               count--;
> +       }
> +
> +       /*
> +        * ASPEED_MBOX_CTRL_RECV bit is write to clear
> +        * also unmasks in 1 step
> +        */
> +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV,
> +                       ASPEED_MBOX_BMC_CTRL);
> +       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 char __user *p = buf;
> +       ssize_t ret;
> +       char c;
> +       int i;
> +
> +       if (!access_ok(VERIFY_READ, buf, count))
> +               return -EFAULT;
> +
> +       if (count + *ppos > ASPEED_MBOX_NUM_REGS)
> +               return -EINVAL;
> +
> +       mutex_lock(&mbox->mutex);
> +
> +       for (i = *ppos; count > 0 && i < ASPEED_MBOX_NUM_REGS; i++) {
> +               ret = __get_user(c, p);
> +               if (ret)
> +                       goto out_unlock;
> +
> +               aspeed_mbox_outb(mbox, c, ASPEED_MBOX_DATA_0 + (i * 4));
> +               p++;
> +               count--;
> +       }
> +
> +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_SEND, ASPEED_MBOX_BMC_CTRL);
> +       ret = p - buf;
> +
> +out_unlock:
> +       mutex_unlock(&mbox->mutex);
> +       return ret;
> +}
> +
> +static unsigned int aspeed_mbox_poll(struct file *file, poll_table *wait)
> +{
> +       struct aspeed_mbox *mbox = file_mbox(file);
> +       unsigned int mask = 0;
> +
> +       poll_wait(file, &mbox->queue, wait);
> +
> +       if (aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL) & ASPEED_MBOX_CTRL_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 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,
> +};
> +
> +static irqreturn_t aspeed_mbox_irq(int irq, void *arg)
> +{
> +       struct aspeed_mbox *mbox = arg;
> +
> +       if (!(aspeed_mbox_inb(mbox, ASPEED_MBOX_BMC_CTRL)
> +                               & ASPEED_MBOX_CTRL_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_CTRL_MASK, ASPEED_MBOX_BMC_CTRL);
> +
> +       wake_up(&mbox->queue);
> +       return IRQ_HANDLED;
> +}
> +
> +static int aspeed_mbox_config_irq(struct aspeed_mbox *mbox,
> +               struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       int 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.
> +        */
> +       aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_0); /* regs 0 - 7 */
> +       aspeed_mbox_outb(mbox, 0x00, ASPEED_MBOX_INTERRUPT_1); /* regs 8 - 15 */
> +
> +       /* These registers are write one to clear. Clear them. */
> +       aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_0);
> +       aspeed_mbox_outb(mbox, 0xff, ASPEED_MBOX_STATUS_1);
> +
> +       aspeed_mbox_outb(mbox, ASPEED_MBOX_CTRL_RECV, ASPEED_MBOX_BMC_CTRL);
> +       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->regmap = syscon_node_to_regmap(
> +                       pdev->dev.parent->of_node);
> +       if (IS_ERR(mbox->regmap)) {
> +               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 of_device_id aspeed_mbox_match[] = {
> +       { .compatible = "aspeed,ast2400-mbox" },
> +       { .compatible = "aspeed,ast2500-mbox" },
> +       { },
> +};
> +
> +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@gmail.com>");
> +MODULE_DESCRIPTION("Aspeed LPC MBOX reg device driver");
> --
> 2.16.2
>

Re: [PATCH linux 2/2] drivers/misc: Add Aspeed LPC mbox register driver

[Andrew Jeffery]

> > +
> > +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->regmap, 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;
> > +}
> 
> This is a mess. I'd prefer:
> 
> static u8 aspeed_mbox_inb(struct aspeed_mbox *mbox, int reg)
> {
>        int val;
> 
>        regmap_read(mbox->regmap, mbox->base + reg, &val);
> 
>        return val;
> }
> 
> static void aspeed_mbox_outb(struct aspeed_mbox *mbox, u8 data, int reg)
> {
>        regmap_write(mbox->regmap, mbox->base + reg, data);
> }
> 
> Reads and writes to a mmio address will never fail. Regmap incurs
> enough overhead already; we don't need to add more in the driver.
> 

The mess was my suggestion when the patches were first posted, and I apologise for that now. Lets go with Joel's cleaned up version.

Cheers,

Andrew