[PATCH v3 0/5] LPC/MBOX work

[PATCH v3 0/5] LPC/MBOX work

[Cyril Bur]

I thought I would put it one final time on here mostly because
patch 5/5, the mbox driver, got quite a rework today thanks for the
review Ben!

Notable differences is that I've completely ditched the ioctl() in
favour of arbitrary reads and writes. This gives userspace the
ability to touch only one reg which was the aim of the ioctl() anyway.
The userspace complexity is similar, or perhaps a little more simple,
no sacrifice there.

In the process of having the kernel allow access of any sized
read/write to all 16 of the data regs I realise that some of the
intended protocol had snuck in, or rather hadn't been completed
removed, that got addressed too.

Also added a mutex to avoid multiple writers.

Thanks,

Cyril

Cyril Bur (5):
  Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings
  Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings
  ARM: dts: aspeed: Add mailbox and LPC Control nodes
  drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
  drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver

 .../devicetree/bindings/mailbox/aspeed-mbox.txt    |  44 +++
 .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   |  78 +++++
 arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts     |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts     |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts      |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts      |   7 +
 arch/arm/boot/dts/aspeed-g4.dtsi                   |  38 +++
 drivers/mailbox/Kconfig                            |   9 +
 drivers/mailbox/Makefile                           |   3 +
 drivers/mailbox/aspeed-mbox.c                      | 365 +++++++++++++++++++++
 drivers/misc/Kconfig                               |   9 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/aspeed-lpc-ctrl.c                     | 282 ++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h               |  25 ++
 14 files changed, 879 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
 create mode 100644 drivers/mailbox/aspeed-mbox.c
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

-- 
2.11.0

[PATCH v3 1/5] Documentation: dt: mailbox: Add Aspeed ast2400/2500 bindings

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 .../devicetree/bindings/mailbox/aspeed-mbox.txt    | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
new file mode 100644
index 000000000000..633cd534d91c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/aspeed-mbox.txt
@@ -0,0 +1,44 @@
+ASpeed Mailbox Driver
+=====================
+
+The ASpeed mailbox allows for communication between different
+processors. The mailbox on the ASpeed ast2400 and ast2500 is a set of
+16 single byte data registers along with interrupt and configuration
+registers directly on the SoC. These are memory mapped on the aspeed
+and can be accessed via the SuperIO registers on the other processor.
+
+Device Node:
+============
+This represents the mailbox on the Soc.
+
+As the mailbox registers sit on the LPC bus, it makes most sense for
+the device to be within the LPC host node. See
+Documentation/devicetree/bindings/mfd/aspeed-lpc.txt for more
+information. This does not have to be the case, provided the reg
+property can give the full address of the mbox registers.
+
+Required Properties:
+--------------------
+- compatible:	Should be one of the following,
+				"aspeed,ast2400-mbox" for ast2400 SoCs
+				"aspeed,ast2500-mbox" for ast2500 SoCs
+
+- reg:			Contains the mailbox address register range (base
+				address and length). Keeping in mind that if the node
+				exists within the LPC host node and that base is
+				relative to that.
+
+- interrupts:	Contains interrupt information for the mailbox device.
+
+- #mbox-cells:	Common property, should be 1.
+
+Example:
+--------
+
+mbox: mbox@180 {
+	compatible = "aspeed,ast2400-mbox";
+	reg = <0x180 0x5c>;
+	interrupts = <46>;
+	#mbox-cells = <1>;
+};
+
-- 
2.11.0

[PATCH v3 2/5] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 .../devicetree/bindings/misc/aspeed-lpc-ctrl.txt   | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt

diff --git a/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
new file mode 100644
index 000000000000..be7c63c72401
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
@@ -0,0 +1,78 @@
+ASpeed LPC Control
+==================
+This binding defines the LPC control for ASpeed SoCs. Portitions of
+the LPC bus can be access by other processors on the system, address
+ranges on the bus can map accesses from another processor to regions
+of the ASpeed SoC memory space.
+
+Reserved Memory:
+================
+The driver provides functionality to map the LPC bus to a region of
+ASpeed ram. A phandle to a reserved memory node must be provided so
+that the driver can safely use this region.
+
+Flash:
+======
+The driver provides functionality to unmap the LPC bus from ASpeed
+RAM, historically the default mapping has been to the SPI flash
+controller on the ASpeed SoC, a phandle to this node should be
+supplied.
+
+Device Node:
+============
+
+As LPC bus configuation registers are at the start of the LPC bus
+memory space, it makes most sense for the device to be within the LPC
+host node. See Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
+for more information. This does not have to be the case, provided the
+reg property can give the full address of the LPC bus.
+
+Required properties:
+--------------------
+
+- compatible:		"aspeed,ast2400-lpc-ctrl" for ASpeed ast2400 SoCs
+					"aspeed,ast2500-lpc-ctrl" for ASpeed ast2500 SoCs
+
+- reg:				Location and size of the configuation registers	for
+					the LPC bus. Note that if the device node is within
+					the LPC host node then base is relative to that.
+
+- memory-region:	phandle of the reserved memory region
+- flash:			phandle of the SPI flash controller
+
+Example:
+--------
+
+reserved-memory {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges;
+
+	...
+
+	flash_memory: region@54000000 {
+		compatible = "aspeed,ast2400-lpc-ctrl";
+		no-map;
+		reg = <0x54000000 0x04000000>; /* 64M */
+	};
+};
+
+host_pnor: spi@1e630000 {
+	reg = < 0x1e630000 0x18
+			0x30000000 0x02000000 >;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "aspeed,ast2400-smc";
+
+	...
+
+};
+
+
+lpc-ctrl@0 {
+	compatible = "aspeed,ast2400-lpc-ctrl";
+	memory-region = <&flash_memory>;
+	flash = <&host_pnor>;
+	reg = <0x0 0x80>;
+};
+
-- 
2.11.0

[PATCH v3 3/5] ARM: dts: aspeed: Add mailbox and LPC Control nodes

[Cyril Bur]

This reserves BMC ram for host to BMC communication required by the LPC
control driver.

As both these devices exist on the LPC bus these nodes are children of a
new LPC node

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts |  6 ++++
 arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts |  6 ++++
 arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts  |  6 ++++
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts  |  7 +++++
 arch/arm/boot/dts/aspeed-g4.dtsi               | 38 ++++++++++++++++++++++++++
 5 files changed, 63 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts b/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts
index 9dc3e67fc98c..19aeb3eb4a0a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts
@@ -29,6 +29,12 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
 	ahb {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts b/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts
index 68946aa34e31..a99fef94a616 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts
@@ -33,6 +33,12 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
 	leds {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts b/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts
index 639b8f877184..246ed04bf8b3 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts
@@ -29,6 +29,12 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
 	ahb {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
index 13e9f18fbed8..bba911d79fbf 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
@@ -29,8 +29,15 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
+
         leds {
                 compatible = "gpio-leds";
 
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 7c78eb1c01ff..c84e833c9480 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -33,6 +33,44 @@
 		#size-cells = <1>;
 		ranges;
 
+		lpc: lpc@1e789000 {
+			compatible = "aspeed,ast2400-lpc", "simple-mfd";
+			reg = <0x1e789000 0x1000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0x1e789000 0x1000>;
+
+			lpc_bmc: lpc-bmc@0 {
+				compatible = "aspeed,ast2400-lpc-bmc";
+				reg = <0x0 0x80>;
+			};
+
+			lpc_host: lpc-host@80 {
+				compatible = "aspeed,ast2400-lpc-host", "simple-mfd", "syscon";
+				reg = <0x80 0x1e0>;
+				reg-io-width = <4>;
+
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0x0 0x80 0x1e0>;
+
+				lpc-ctrl@0 {
+					compatible = "aspeed,ast2400-lpc-ctrl";
+					memory-region = <&flash_memory>;
+					flash = <&host_pnor>;
+					reg = <0x0 0x80>;
+				};
+
+				mbox: mbox@180 {
+					compatible = "aspeed,ast2400-mbox";
+					reg = <0x180 0x5c>;
+					interrupts = <46>;
+					#mbox-cells = <1>;
+				};
+			};
+		};
+
 		vic: interrupt-controller@1e6c0080 {
 			compatible = "aspeed,ast2400-vic";
 			interrupt-controller;
-- 
2.11.0

[PATCH v3 4/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Cyril Bur]

This driver exposes a reserved chunk of BMC ram to userspace as well as
an ioctl interface to control the BMC<->HOST mapping of the LPC bus.
This allows for a communication channel between the BMC and the host

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/misc/Kconfig                 |   9 ++
 drivers/misc/Makefile                |   1 +
 drivers/misc/aspeed-lpc-ctrl.c       | 282 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h |  25 ++++
 4 files changed, 317 insertions(+)
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a216b4667742..61ca9025ccb9 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.
 
+config ASPEED_LPC_CTRL
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	bool "Build a driver to control the BMC to HOST LPC bus"
+	default "y"
+	---help---
+	  Provides a driver to control BMC to HOST LPC mappings through
+	  ioctl()s, the driver aso provides a read/write interface to a BMC ram
+	  region where host LPC can be buffered.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index b2fb6dbffcef..cdcd1af48971 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
new file mode 100644
index 000000000000..8b95fac2f55b
--- /dev/null
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -0,0 +1,282 @@
+/*
+ * Copyright 2016 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/delay.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+
+#include <linux/aspeed-lpc-ctrl.h>
+
+#define DEVICE_NAME	"aspeed-lpc-ctrl"
+
+#define HICR7 0x8
+#define HICR8 0xc
+
+struct lpc_ctrl {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	phys_addr_t		base;
+	resource_size_t		size;
+	uint32_t		pnor_size;
+	uint32_t		pnor_base;
+};
+
+static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
+
+static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
+{
+	return container_of(file->private_data, struct lpc_ctrl, miscdev);
+}
+
+static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	unsigned long vsize = vma->vm_end - vma->vm_start;
+
+	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
+		return -EINVAL;
+
+	/* Other checks? */
+
+	if (remap_pfn_range(vma, vma->vm_start,
+		(lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
+		vsize, vma->vm_page_prot))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int lpc_ctrl_open(struct inode *inode, struct file *file)
+{
+	if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
+		return 0;
+
+	atomic_dec(&lpc_ctrl_open_count);
+	return -EBUSY;
+}
+
+static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	return -EPERM;
+}
+
+static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	return -EPERM;
+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	long rc;
+	struct lpc_mapping map;
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	void __user *p = (void __user *)param;
+
+	switch (cmd) {
+	case LPC_CTRL_IOCTL_SIZE:
+		return copy_to_user(p, &lpc_ctrl->size,
+			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
+	case LPC_CTRL_IOCTL_MAP:
+		if (copy_from_user(&map, p, sizeof(map)))
+			return -EFAULT;
+
+
+		/*
+		 * The top half of HICR7 is the MSB of the BMC address of the
+		 * mapping.
+		 * The bottom half of HICR7 is the MSB of the HOST LPC
+		 * firmware space address of the mapping.
+		 *
+		 * The 1 bits in the top of half of HICR8 represent the bits
+		 * (in the requested address) that should be ignored and
+		 * replaced with those from the top half of HICR7.
+		 * The 1 bits in the bottom half of HICR8 represent the bits
+		 * (in the requested address) that should be kept and pass
+		 * into the BMC address space.
+		 */
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR7,
+				(lpc_ctrl->base | (map.hostaddr >> 16)));
+		if (rc)
+			return rc;
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+			(~(map.size - 1)) | ((map.size >> 16) - 1));
+		return rc;
+	case LPC_CTRL_IOCTL_UNMAP:
+		/*
+		 * The top nibble in host lpc addresses references which
+		 * firmware space, use space zero hence the & 0x0fff
+		 */
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR7,
+			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
+		if (rc)
+			return rc;
+
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
+		return rc;
+	}
+
+	return -EINVAL;
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&lpc_ctrl_open_count);
+	return 0;
+}
+
+static const struct file_operations lpc_ctrl_fops = {
+	.owner		= THIS_MODULE,
+	.mmap		= lpc_ctrl_mmap,
+	.open		= lpc_ctrl_open,
+	.read		= lpc_ctrl_read,
+	.write		= lpc_ctrl_write,
+	.release	= lpc_ctrl_release,
+	.unlocked_ioctl	= lpc_ctrl_ioctl,
+};
+
+static int lpc_ctrl_probe(struct platform_device *pdev)
+{
+	struct lpc_ctrl *lpc_ctrl;
+	struct device *dev;
+	struct device_node *node;
+	struct resource resm;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+
+	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
+	if (!lpc_ctrl)
+		return -ENOMEM;
+
+	node = of_parse_phandle(dev->of_node, "flash", 0);
+	if (!node) {
+		dev_err(dev, "Didn't find host pnor flash node\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	rc = of_property_read_u32_index(node, "reg", 3,
+			&lpc_ctrl->pnor_size);
+	if (rc)
+		return rc;
+	rc = of_property_read_u32_index(node, "reg", 2,
+			&lpc_ctrl->pnor_base);
+	if (rc)
+		return rc;
+
+	dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
+		lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
+
+	dev_set_drvdata(&pdev->dev, lpc_ctrl);
+
+	node = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!node) {
+		dev_err(dev, "Didn't find reserved memory\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = of_address_to_resource(node, 0, &resm);
+	of_node_put(node);
+	if (rc) {
+		dev_err(dev, "Could address to resource\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	lpc_ctrl->size = resource_size(&resm);
+	lpc_ctrl->base = resm.start;
+
+	lpc_ctrl->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(lpc_ctrl->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
+	lpc_ctrl->miscdev.name = DEVICE_NAME;
+	lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
+	lpc_ctrl->miscdev.parent = dev;
+	rc = misc_register(&lpc_ctrl->miscdev);
+	if (rc)
+		dev_err(dev, "Unable to register device\n");
+	else
+		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+			lpc_ctrl->base, lpc_ctrl->size);
+
+out:
+	return rc;
+}
+
+static int lpc_ctrl_remove(struct platform_device *pdev)
+{
+	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&lpc_ctrl->miscdev);
+	lpc_ctrl = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
+	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
+	{ },
+};
+
+static struct platform_driver lpc_ctrl_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = lpc_ctrl_match,
+	},
+	.probe = lpc_ctrl_probe,
+	.remove = lpc_ctrl_remove,
+};
+
+module_platform_driver(lpc_ctrl_driver);
+
+MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
+MODULE_DESCRIPTION("Linux device interface to control LPC bus");
diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
new file mode 100644
index 000000000000..c5f1caf827ac
--- /dev/null
+++ b/include/uapi/linux/aspeed-lpc-ctrl.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#ifndef _UAPI_LINUX_LPC_CTRL_H
+#define _UAPI_LINUX_LPC_CTRL_H
+
+#include <linux/ioctl.h>
+
+struct lpc_mapping {
+	uint32_t hostaddr;
+	uint32_t size;
+};
+
+#define __LPC_CTRL_IOCTL_MAGIC	0xb2
+#define LPC_CTRL_IOCTL_SIZE	_IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
+#define LPC_CTRL_IOCTL_MAP	_IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
+#define LPC_CTRL_IOCTL_UNMAP	_IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
+
+#endif /* _UAPI_LINUX_LPC_CTRL_H */
-- 
2.11.0

[PATCH v3 5/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver

[Cyril Bur]

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

This driver allows arbitrary reads and writes to the 16 data registers as
the other end may have configured the mbox hardware to provide an
interrupt when a specific register gets written to.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mailbox/Kconfig       |   9 ++
 drivers/mailbox/Makefile      |   3 +
 drivers/mailbox/aspeed-mbox.c | 365 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/mailbox/aspeed-mbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 5305923752d2..b9aebe139b9c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOX
 	  It is used to send short messages between ARM64-bit cores and
 	  the SLIMpro Management Engine, primarily for PM. Say Y here if you
 	  want to use the APM X-Gene SLIMpro IPCM support.
+
+config ASPEED_MBOX
+	depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
+	bool "Aspeed ast2400/2500 Mailbox Controller"
+	default "y"
+	---help---
+	  Provides a driver for the MBOX registers found on Aspeed SOCs
+	  (AST2400 and AST2500). This driver provides a device for aspeed
+	  mbox registers
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 0be3e742bb7d..99d17f3b9552 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
 obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
 
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
+
+obj-$(CONFIG_ASPEED_MBOX)		+= aspeed-mbox.o
+
diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
new file mode 100644
index 000000000000..6820f73085db
--- /dev/null
+++ b/drivers/mailbox/aspeed-mbox.c
@@ -0,0 +1,365 @@
+/*
+ * Copyright 2016 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/delay.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/mfd/syscon.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+
+#define DEVICE_NAME	"aspeed-mbox"
+
+#define MBOX_NUM_REGS 16
+
+#define MBOX_DATA_0 0x00
+#define MBOX_STATUS_0 0x40
+#define MBOX_STATUS_1 0x44
+#define MBOX_BMC_CTRL 0x48
+#define   MBOX_CTRL_RECV BIT(7)
+#define   MBOX_CTRL_MASK BIT(1)
+#define   MBOX_CTRL_SEND BIT(0)
+#define MBOX_HOST_CTRL 0x4c
+#define MBOX_INTERRUPT_0 0x50
+#define MBOX_INTERRUPT_1 0x54
+
+struct mbox {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	unsigned int		base;
+	int			irq;
+	wait_queue_head_t	queue;
+	struct timer_list	poll_timer;
+	struct mutex		mutex;
+};
+
+static atomic_t mbox_open_count = ATOMIC_INIT(0);
+
+static u8 mbox_inb(struct 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 mbox_outb(struct 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 mbox *file_mbox(struct file *file)
+{
+	return container_of(file->private_data, struct mbox, miscdev);
+}
+
+static int mbox_open(struct inode *inode, struct file *file)
+{
+	struct mbox *mbox = file_mbox(file);
+
+	if (atomic_inc_return(&mbox_open_count) == 1) {
+		/*
+		 * Clear the interrupt status bit if it was left on and unmask
+		 * interrupts.
+		 * MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
+		 */
+		mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+		return 0;
+	}
+
+	atomic_dec(&mbox_open_count);
+	return -EBUSY;
+}
+
+static ssize_t mbox_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct 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 > MBOX_NUM_REGS)
+		return -EINVAL;
+
+	if ((file->f_flags & O_NONBLOCK) &&
+		!(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
+			return -EAGAIN;
+	else if (wait_event_interruptible(mbox->queue,
+			 mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
+		return -ERESTARTSYS;
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < MBOX_NUM_REGS; i++, p++, count--) {
+		if (__put_user(mbox_inb(mbox, MBOX_DATA_0 + (i * 4)), p)) {
+			ret = -EFAULT;
+			goto out_unlock;
+		}
+	}
+
+	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
+	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static ssize_t mbox_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct 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 > MBOX_NUM_REGS)
+		return -EINVAL;
+
+	mutex_lock(&mbox->mutex);
+
+	for (i = *ppos; count > 0 && i < MBOX_NUM_REGS; i++) {
+		if (__get_user(c, p)) {
+			ret = -EFAULT;
+			goto out_unlock;
+		}
+
+		mbox_outb(mbox, c, MBOX_DATA_0 + (i * 4));
+		p++;
+		count--;
+	}
+
+	mbox_outb(mbox, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
+	ret = p - buf;
+
+out_unlock:
+	mutex_unlock(&mbox->mutex);
+	return ret;
+}
+
+static unsigned int mbox_poll(struct file *file, poll_table *wait)
+{
+	struct mbox *mbox = file_mbox(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &mbox->queue, wait);
+
+	if (mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
+		mask |= POLLIN;
+
+	return mask;
+}
+
+static int mbox_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&mbox_open_count);
+	return 0;
+}
+
+static const struct file_operations mbox_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_seek_end_llseek,
+	.read		= mbox_read,
+	.write		= mbox_write,
+	.open		= mbox_open,
+	.release	= mbox_release,
+	.poll		= mbox_poll,
+};
+
+static void poll_timer(unsigned long data)
+{
+	struct mbox *mbox = (void *)data;
+
+	mbox->poll_timer.expires += msecs_to_jiffies(500);
+	wake_up(&mbox->queue);
+	add_timer(&mbox->poll_timer);
+}
+
+static irqreturn_t mbox_irq(int irq, void *arg)
+{
+	struct mbox *mbox = arg;
+
+	if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & 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 */
+	mbox_outb(mbox, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
+
+	wake_up(&mbox->queue);
+	return IRQ_HANDLED;
+}
+
+static int mbox_config_irq(struct mbox *mbox,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	mbox->irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!mbox->irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, mbox->irq, mbox_irq, IRQF_SHARED,
+			DEVICE_NAME, mbox);
+	if (rc < 0) {
+		dev_warn(dev, "Unable to request IRQ %d\n", mbox->irq);
+		mbox->irq = 0;
+		return rc;
+	}
+
+	/*
+	 * Disable all register based interrupts.
+	 */
+	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
+	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
+
+	/* W1C */
+	mbox_outb(mbox, 0xff, MBOX_STATUS_0);
+	mbox_outb(mbox, 0xff, MBOX_STATUS_1);
+
+	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return 0;
+}
+
+static int mbox_probe(struct platform_device *pdev)
+{
+	struct mbox *mbox;
+	struct device *dev;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+	dev_info(dev, "Found mbox host device\n");
+
+	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");
+		goto out;
+	}
+
+	mbox->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(mbox->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	mutex_init(&mbox->mutex);
+	init_waitqueue_head(&mbox->queue);
+
+	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
+	mbox->miscdev.name = DEVICE_NAME;
+	mbox->miscdev.fops = &mbox_fops;
+	mbox->miscdev.parent = dev;
+	rc = misc_register(&mbox->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register device\n");
+		goto out;
+	}
+
+	mbox_config_irq(mbox, pdev);
+
+	if (mbox->irq) {
+		dev_info(dev, "Using IRQ %d\n", mbox->irq);
+	} else {
+		dev_info(dev, "No IRQ; using timer\n");
+		setup_timer(&mbox->poll_timer, poll_timer,
+				(unsigned long)mbox);
+		mbox->poll_timer.expires = jiffies + msecs_to_jiffies(10);
+		add_timer(&mbox->poll_timer);
+	}
+
+out:
+	return rc;
+
+}
+
+static int mbox_remove(struct platform_device *pdev)
+{
+	struct mbox *mbox = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&mbox->miscdev);
+	if (!mbox->irq)
+		del_timer_sync(&mbox->poll_timer);
+	mbox = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id mbox_match[] = {
+	{ .compatible = "aspeed,ast2400-mbox" },
+	{ },
+};
+
+static struct platform_driver mbox_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = mbox_match,
+	},
+	.probe = mbox_probe,
+	.remove = mbox_remove,
+};
+
+module_platform_driver(mbox_driver);
+
+MODULE_DEVICE_TABLE(of, mbox_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
+MODULE_DESCRIPTION("Linux device interface to the Aspeed MBOX registers");
-- 
2.11.0

Re: [PATCH v3 5/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver

[Joel Stanley]

On Tue, Jan 10, 2017 at 8:06 PM, Cyril Bur <cyrilbur@gmail.com> wrote:

Nit: make the subject "Add Aspeed mailbox driver".

> This provides access to the mbox registers on the ast2400 and ast2500
> boards.
>
> This driver allows arbitrary reads and writes to the 16 data registers as
> the other end may have configured the mbox hardware to provide an
> interrupt when a specific register gets written to.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/mailbox/Kconfig       |   9 ++
>  drivers/mailbox/Makefile      |   3 +
>  drivers/mailbox/aspeed-mbox.c | 365 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
>  create mode 100644 drivers/mailbox/aspeed-mbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 5305923752d2..b9aebe139b9c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOX
>           It is used to send short messages between ARM64-bit cores and
>           the SLIMpro Management Engine, primarily for PM. Say Y here if you
>           want to use the APM X-Gene SLIMpro IPCM support.
> +
> +config ASPEED_MBOX
> +       depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
> +       bool "Aspeed ast2400/2500 Mailbox Controller"
> +       default "y"
> +       ---help---
> +         Provides a driver for the MBOX registers found on Aspeed SOCs
> +         (AST2400 and AST2500). This driver provides a device for aspeed
> +         mbox registers
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 0be3e742bb7d..99d17f3b9552 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>  obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>
>  obj-$(CONFIG_HI6220_MBOX)      += hi6220-mailbox.o
> +
> +obj-$(CONFIG_ASPEED_MBOX)              += aspeed-mbox.o
> +
> diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> new file mode 100644
> index 000000000000..6820f73085db
> --- /dev/null
> +++ b/drivers/mailbox/aspeed-mbox.c
> @@ -0,0 +1,365 @@
> +/*
> + * Copyright 2016 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/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/regmap.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> +
> +#define DEVICE_NAME    "aspeed-mbox"
> +
> +#define MBOX_NUM_REGS 16
> +
> +#define MBOX_DATA_0 0x00
> +#define MBOX_STATUS_0 0x40
> +#define MBOX_STATUS_1 0x44
> +#define MBOX_BMC_CTRL 0x48
> +#define   MBOX_CTRL_RECV BIT(7)
> +#define   MBOX_CTRL_MASK BIT(1)
> +#define   MBOX_CTRL_SEND BIT(0)
> +#define MBOX_HOST_CTRL 0x4c
> +#define MBOX_INTERRUPT_0 0x50
> +#define MBOX_INTERRUPT_1 0x54
> +
> +struct mbox {

This is the aspeed mailbox device driver. Kernel convention is to
prefix the functions and structure with aspeed_.

I mentioned this in the first review I did.

> +       struct miscdevice       miscdev;
> +       struct regmap           *regmap;
> +       unsigned int            base;
> +       int                     irq;
> +       wait_queue_head_t       queue;
> +       struct timer_list       poll_timer;
> +       struct mutex            mutex;
> +};
> +
> +static atomic_t mbox_open_count = ATOMIC_INIT(0);
> +
> +static u8 mbox_inb(struct 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 mbox_outb(struct 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 mbox *file_mbox(struct file *file)
> +{
> +       return container_of(file->private_data, struct mbox, miscdev);
> +}
> +
> +static int mbox_open(struct inode *inode, struct file *file)
> +{
> +       struct mbox *mbox = file_mbox(file);
> +
> +       if (atomic_inc_return(&mbox_open_count) == 1) {
> +               /*
> +                * Clear the interrupt status bit if it was left on and unmask
> +                * interrupts.
> +                * MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
> +                */
> +               mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> +               return 0;
> +       }
> +
> +       atomic_dec(&mbox_open_count);
> +       return -EBUSY;
> +}
> +
> +static ssize_t mbox_read(struct file *file, char __user *buf,
> +                               size_t count, loff_t *ppos)
> +{
> +       struct 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 > MBOX_NUM_REGS)
> +               return -EINVAL;
> +
> +       if ((file->f_flags & O_NONBLOCK) &&
> +               !(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> +                       return -EAGAIN;
> +       else if (wait_event_interruptible(mbox->queue,
> +                        mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))

You read MBOX_BMC_CTRL twice. Is that intentional?

I suggest rearranging your code to read the register once.

> +               return -ERESTARTSYS;
> +
> +       mutex_lock(&mbox->mutex);
> +
> +       for (i = *ppos; count > 0 && i < MBOX_NUM_REGS; i++, p++, count--) {

Can this have another couple of variables?

Perhaps increment the pointer in the loop.

> +               if (__put_user(mbox_inb(mbox, MBOX_DATA_0 + (i * 4)), p)) {

This is hard to read. Split up the code so you do fewer things on a line.

mbox_inb()
ret = put_user()
if (ret)
   goto out;

> +                       ret = -EFAULT;
> +                       goto out_unlock;
> +               }
> +       }
> +
> +       /* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> +       mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> +       ret = p - buf;
> +
> +out_unlock:
> +       mutex_unlock(&mbox->mutex);
> +       return ret;
> +}
> +
> +static ssize_t mbox_write(struct file *file, const char __user *buf,
> +                               size_t count, loff_t *ppos)
> +{
> +       struct 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 > MBOX_NUM_REGS)
> +               return -EINVAL;
> +
> +       mutex_lock(&mbox->mutex);
> +
> +       for (i = *ppos; count > 0 && i < MBOX_NUM_REGS; i++) {
> +               if (__get_user(c, p)) {
> +                       ret = -EFAULT;
> +                       goto out_unlock;
> +               }
> +
> +               mbox_outb(mbox, c, MBOX_DATA_0 + (i * 4));
> +               p++;
> +               count--;
> +       }
> +
> +       mbox_outb(mbox, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
> +       ret = p - buf;
> +
> +out_unlock:
> +       mutex_unlock(&mbox->mutex);
> +       return ret;
> +}
> +
> +static unsigned int mbox_poll(struct file *file, poll_table *wait)
> +{
> +       struct mbox *mbox = file_mbox(file);
> +       unsigned int mask = 0;
> +
> +       poll_wait(file, &mbox->queue, wait);
> +
> +       if (mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> +               mask |= POLLIN;
> +
> +       return mask;
> +}
> +
> +static int mbox_release(struct inode *inode, struct file *file)
> +{
> +       atomic_dec(&mbox_open_count);
> +       return 0;
> +}
> +
> +static const struct file_operations mbox_fops = {
> +       .owner          = THIS_MODULE,
> +       .llseek         = no_seek_end_llseek,
> +       .read           = mbox_read,
> +       .write          = mbox_write,
> +       .open           = mbox_open,
> +       .release        = mbox_release,
> +       .poll           = mbox_poll,
> +};
> +
> +static void poll_timer(unsigned long data)
> +{
> +       struct mbox *mbox = (void *)data;
> +
> +       mbox->poll_timer.expires += msecs_to_jiffies(500);
> +       wake_up(&mbox->queue);
> +       add_timer(&mbox->poll_timer);
> +}
> +
> +static irqreturn_t mbox_irq(int irq, void *arg)
> +{
> +       struct mbox *mbox = arg;
> +
> +       if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & 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 */
> +       mbox_outb(mbox, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
> +
> +       wake_up(&mbox->queue);
> +       return IRQ_HANDLED;
> +}
> +
> +static int mbox_config_irq(struct mbox *mbox,
> +               struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       int rc;
> +
> +       mbox->irq = irq_of_parse_and_map(dev->of_node, 0);
> +       if (!mbox->irq)
> +               return -ENODEV;
> +
> +       rc = devm_request_irq(dev, mbox->irq, mbox_irq, IRQF_SHARED,
> +                       DEVICE_NAME, mbox);
> +       if (rc < 0) {
> +               dev_warn(dev, "Unable to request IRQ %d\n", mbox->irq);
> +               mbox->irq = 0;
> +               return rc;
> +       }
> +
> +       /*
> +        * Disable all register based interrupts.
> +        */
> +       mbox_outb(mbox, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
> +       mbox_outb(mbox, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
> +
> +       /* W1C */

What does this mean?

> +       mbox_outb(mbox, 0xff, MBOX_STATUS_0);
> +       mbox_outb(mbox, 0xff, MBOX_STATUS_1);
> +
> +       mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> +       return 0;

You return values from the function but don't use them.

> +}
> +
> +static int mbox_probe(struct platform_device *pdev)
> +{
> +       struct mbox *mbox;
> +       struct device *dev;
> +       int rc;
> +
> +       if (!pdev || !pdev->dev.of_node)
> +               return -ENODEV;

Drop these checks.

> +
> +       dev = &pdev->dev;
> +       dev_info(dev, "Found mbox host device\n");

Drop this.

> +
> +       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");
> +               goto out;
You don't do anything in out, so just return here.

> +       }
>
> +       mbox->regmap = syscon_node_to_regmap(
> +                       pdev->dev.parent->of_node);
> +       if (IS_ERR(mbox->regmap)) {
> +               dev_err(dev, "Couldn't get regmap\n");
> +               rc = -ENODEV;
> +               goto out;

Return here.

> +       }
> +
> +       mutex_init(&mbox->mutex);
> +       init_waitqueue_head(&mbox->queue);
> +
> +       mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
> +       mbox->miscdev.name = DEVICE_NAME;
> +       mbox->miscdev.fops = &mbox_fops;
> +       mbox->miscdev.parent = dev;
> +       rc = misc_register(&mbox->miscdev);
> +       if (rc) {
> +               dev_err(dev, "Unable to register device\n");
> +               goto out;

Do you need to cleanup the waitqueue?

> +       }
> +
> +       mbox_config_irq(mbox, pdev);
> +
> +       if (mbox->irq) {
> +               dev_info(dev, "Using IRQ %d\n", mbox->irq);
> +       } else {
> +               dev_info(dev, "No IRQ; using timer\n");

What decides if we're using an irq or not? Can we simplify it and only
support one case?

> +               setup_timer(&mbox->poll_timer, poll_timer,
> +                               (unsigned long)mbox);
> +               mbox->poll_timer.expires = jiffies + msecs_to_jiffies(10);
> +               add_timer(&mbox->poll_timer);
> +       }
> +
> +out:
> +       return rc;
> +
> +}
> +
> +static int mbox_remove(struct platform_device *pdev)
> +{
> +       struct mbox *mbox = dev_get_drvdata(&pdev->dev);
> +
> +       misc_deregister(&mbox->miscdev);
> +       if (!mbox->irq)
> +               del_timer_sync(&mbox->poll_timer);
> +       mbox = NULL;

What?

> +
> +       return 0;
> +}
> +
> +static const struct of_device_id mbox_match[] = {
> +       { .compatible = "aspeed,ast2400-mbox" },

This needs an ast2500 string.

> +       { },
> +};
> +
> +static struct platform_driver mbox_driver = {
> +       .driver = {
> +               .name           = DEVICE_NAME,
> +               .of_match_table = mbox_match,
> +       },
> +       .probe = mbox_probe,
> +       .remove = mbox_remove,
> +};
> +
> +module_platform_driver(mbox_driver);
> +
> +MODULE_DEVICE_TABLE(of, mbox_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Linux device interface to the Aspeed MBOX registers");

Aspeed mailbox device driver?

> --
> 2.11.0
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH v3 3/5] ARM: dts: aspeed: Add mailbox and LPC Control nodes

[Joel Stanley]

On Tue, Jan 10, 2017 at 8:06 PM, Cyril Bur <cyrilbur@gmail.com> wrote:

> index 7c78eb1c01ff..c84e833c9480 100644
> --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi

> +                       lpc_host: lpc-host@80 {
> +                               compatible = "aspeed,ast2400-lpc-host", "simple-mfd", "syscon";
> +                               reg = <0x80 0x1e0>;
> +                               reg-io-width = <4>;
> +
> +                               #address-cells = <1>;
> +                               #size-cells = <1>;
> +                               ranges = <0x0 0x80 0x1e0>;
> +
> +                               lpc-ctrl@0 {
> +                                       compatible = "aspeed,ast2400-lpc-ctrl";
> +                                       memory-region = <&flash_memory>;

You can't have a phandle to the dts in the dtsi.

Cheers,

Joel

> +                                       flash = <&host_pnor>;
> +                                       reg = <0x0 0x80>;
> +                               };
> +

Re: [PATCH v3 5/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Tue, 2017-01-10 at 20:06 +1100, Cyril Bur wrote:
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/regmap.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
> +

Given Joel's feedback on various issues, please also audit these
includes, we don't need a number of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/5] Documentation: dt: misc: Add Aspeed ast2400/2500 LPC Control bindings

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

On Tue, 2017-01-10 at 20:06 +1100, Cyril Bur wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed-lpc-ctrl.txt
> @@ -0,0 +1,78 @@
> +ASpeed LPC Control
> +==================
> +This binding defines the LPC control for ASpeed SoCs. Portitions of

Typo here ('Portions'), but otherwise the bindings look okay.

> +the LPC bus can be access by other processors on the system, address
> +ranges on the bus can map accesses from another processor to regions
> +of the ASpeed SoC memory space.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 5/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver

[Joel Stanley]

On Wed, Jan 11, 2017 at 11:33 AM, Joel Stanley <joel@jms.id.au> wrote:
>> +       if ((file->f_flags & O_NONBLOCK) &&
>> +               !(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
>> +                       return -EAGAIN;
>> +       else if (wait_event_interruptible(mbox->queue,
>> +                        mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
>
> You read MBOX_BMC_CTRL twice. Is that intentional?

In fact, you keep reading it forever thanks to the magic of the
wait_event_interruptible macro.

Carry on.

Cheers,

Joel

Re: [PATCH v3 5/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver

[Cyril Bur]

On Wed, 2017-01-11 at 14:54 +1100, Joel Stanley wrote:
> On Wed, Jan 11, 2017 at 11:33 AM, Joel Stanley <joel@jms.id.au> wrote:
> > > +       if ((file->f_flags & O_NONBLOCK) &&
> > > +               !(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > > +                       return -EAGAIN;
> > > +       else if (wait_event_interruptible(mbox->queue,
> > > +                        mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > 
> > You read MBOX_BMC_CTRL twice. Is that intentional?
> 
> In fact, you keep reading it forever thanks to the magic of the
> wait_event_interruptible macro.
> 

Heh, yes. Although your spidey senses were correct the if condition is
wrong.

> Carry on.
> 
> Cheers,
> 
> Joel