[PATCH linux 0/3] LPC/MBOX work

[PATCH linux 0/3] LPC/MBOX work

[Cyril Bur]

Hello all,

I sent an email a while ago about my work to be able for the host to
be able to access to contents of the pnor without nessesarily being
able to modify it.

This involves a protocol for requests through the mbox regsters on the
BMC and moving the HOST<->BMC LPC bus mapping to a region of BMC RAM.

These are the kernel patches required to do this. I am going to do the
changes for the dts for all platforms but I have some questions as to
how much RAM these platforms truely have. You'll notice I bumped the
palmetto from 256 to 512, this was in response to my palmetto having
512 megs. Part of the reason for this is that while I'm at it, I may
as well reserve off the framebuffer memory (which there is work to
perhaps use as well), the location of which depends on how much ram
the BMC has. I'm thinking everything is actually 512, if that is
accurate, I'll add those changes to a v2.

Cedric (or anyone really): Do you know of a way the lpc-ctrl driver
could query the MTD subsystem for the size of the PNOR? There is
currently a hack in the UNMAP ioctl() which can't hang around.

I'm not sure driver/misc is the place for these, thoughts?

Thanks,

Cyril

Cyril Bur (3):
  ARM: dts: aspeed: Update palmetto device tree
  drivers/misc: Add aspeed mbox driver
  drivers/misc: Add aspeed lpc controlling driver

 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts |  25 +-
 arch/arm/boot/dts/aspeed-g4.dtsi              |   6 +
 drivers/misc/Kconfig                          |  14 ++
 drivers/misc/Makefile                         |   2 +
 drivers/misc/lpc-ctrl.c                       | 251 ++++++++++++++++++++
 drivers/misc/mbox-host.c                      | 326 ++++++++++++++++++++++++++
 include/uapi/linux/lpc-ctrl.h                 |  25 ++
 include/uapi/linux/mbox-host.h                |  18 ++
 8 files changed, 666 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/lpc-ctrl.c
 create mode 100644 drivers/misc/mbox-host.c
 create mode 100644 include/uapi/linux/lpc-ctrl.h
 create mode 100644 include/uapi/linux/mbox-host.h

-- 
2.10.2

[PATCH linux 1/3] ARM: dts: aspeed: Update palmetto device tree

[Cyril Bur]

Palmettos have 512mb of ram. There is always framebuffer memory at the
top of ram.

This patch also reserves BMC ram for host to BMC communication.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 25 ++++++++++++++++++++++++-
 arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
index acaceda2..e810ae7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
@@ -17,7 +17,30 @@
 	};
 
 	memory {
-		reg = <0x40000000 0x10000000>;
+		reg = <0x40000000 0x20000000>;
+	};
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells= <1>;
+		ranges;
+
+		flash_mem: region@52000000 {
+			compatible = "aspeed,lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64m */
+		};
+
+		fb_mem: framebuffer@5f000000 {
+			no-map;
+			reg = <0x5f000000 0x01000000>; /* 16m */
+		};
+	};
+
+	flash_buffer {
+		compatible = "aspeed,lpc-ctrl";
+		memory-region = <&flash_mem>;
+		reg = <0x1e789000 8>;
 	};
 
         leds {
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 7c78eb1..2eba968 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -870,6 +870,12 @@
 				interrupts = <8>;
 			};
 
+			mbox: mbox@1e789200 {
+				compatible = "aspeed,mbox-host";
+				reg = <0x1e789200 0x5c>;
+				interrupts = <46>;
+			};
+
 			wdt1: wdt@1e785000 {
 				compatible = "aspeed,ast2400-wdt";
 				reg = <0x1e785000 0x1c>;
-- 
2.10.2

[PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Cyril Bur]

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/misc/Kconfig           |   7 +
 drivers/misc/Makefile          |   2 +
 drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/mbox-host.h |  18 +++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/misc/mbox-host.c
 create mode 100644 include/uapi/linux/mbox-host.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a216b46..44f191d 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -804,6 +804,13 @@ 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 MBOX_HOST
+	depends on ARCH_ASPEED
+	bool "Build MBOX driver for BMC to HOST communication"
+	default "n"
+	---help---
+	  Build the MBOX driver
+
 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 b2fb6dbf..7bcaf30 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
+>>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
diff --git a/drivers/misc/mbox-host.c b/drivers/misc/mbox-host.c
new file mode 100644
index 0000000..9d11da4
--- /dev/null
+++ b/drivers/misc/mbox-host.c
@@ -0,0 +1,326 @@
+/*
+ * 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/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/mbox-host.h>
+
+#define DEVICE_NAME	"mbox-host"
+
+#define MBOX_NUM_REGS 16
+#define MBOX_NUM_DATA_REGS 14
+
+#define MBOX_DATA_0 0x00
+#define MBOX_STATUS_0 0x40
+#define MBOX_STATUS_1 0x44
+#define MBOX_BMC_CTRL 0x48
+	#define MBOX_CTRL_RECV 0x80
+	#define MBOX_CTRL_MASK 0x02
+	#define MBOX_CTRL_SEND 0x01
+#define MBOX_HOST_CTRL 0x4c
+#define MBOX_INTERRUPT_0 0x50
+#define MBOX_INTERRUPT_1 0x54
+
+struct mbox_host {
+	struct miscdevice	miscdev;
+	void __iomem		*base;
+	int			irq;
+	wait_queue_head_t	queue;
+	struct timer_list	poll_timer;
+};
+
+static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
+{
+	return ioread8(mbox_host->base + reg);
+}
+
+static void mbox_outb(struct mbox_host *mbox_host, u8 data, int reg)
+{
+	iowrite8(data, mbox_host->base + reg);
+}
+
+static struct mbox_host *file_mbox_host(struct file *file)
+{
+	return container_of(file->private_data, struct mbox_host, miscdev);
+}
+
+static int mbox_host_open(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static ssize_t mbox_host_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+	char __user *p = buf;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	WARN_ON(*ppos);
+
+	if (wait_event_interruptible(mbox_host->queue,
+		mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
+		return -ERESTARTSYS;
+
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
+		if (__put_user(mbox_inb(mbox_host, MBOX_DATA_0 + (i * 4)), p++))
+			return -EFAULT;
+
+	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
+	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return p - buf;
+}
+
+static ssize_t mbox_host_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+	const char __user *p = buf;
+	char c;
+	int i;
+
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	WARN_ON(*ppos);
+
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++) {
+		if (__get_user(c, p))
+			return -EFAULT;
+
+		mbox_outb(mbox_host, c, MBOX_DATA_0 + (i * 4));
+		p++;
+	}
+
+	mbox_outb(mbox_host, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
+
+	return p - buf;
+}
+
+static long mbox_host_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+
+	switch (cmd) {
+	case MBOX_HOST_IOCTL_ATN:
+		mbox_outb(mbox_host, param, MBOX_DATA_0 + 15);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int mbox_host_release(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static unsigned int mbox_host_poll(struct file *file, poll_table *wait)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &mbox_host->queue, wait);
+
+	if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
+		mask |= POLLIN;
+
+	return mask;
+}
+
+static const struct file_operations mbox_host_fops = {
+	.owner		= THIS_MODULE,
+	.open		= mbox_host_open,
+	.read		= mbox_host_read,
+	.write		= mbox_host_write,
+	.release	= mbox_host_release,
+	.poll		= mbox_host_poll,
+	.unlocked_ioctl	= mbox_host_ioctl,
+};
+
+static void poll_timer(unsigned long data)
+{
+	struct mbox_host *mbox_host = (void *)data;
+
+	mbox_host->poll_timer.expires += msecs_to_jiffies(500);
+	wake_up(&mbox_host->queue);
+	add_timer(&mbox_host->poll_timer);
+}
+
+static irqreturn_t mbox_host_irq(int irq, void *arg)
+{
+	struct mbox_host *mbox_host = arg;
+
+	if (!(mbox_inb(mbox_host, 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_host, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
+
+	wake_up(&mbox_host->queue);
+	return IRQ_HANDLED;
+}
+
+static int mbox_host_config_irq(struct mbox_host *mbox_host,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	mbox_host->irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!mbox_host->irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, mbox_host->irq, mbox_host_irq, IRQF_SHARED,
+			DEVICE_NAME, mbox_host);
+	if (rc < 0) {
+		dev_warn(dev, "Unable to request IRQ %d\n", mbox_host->irq);
+		mbox_host->irq = 0;
+		return rc;
+	}
+
+	/*
+	 * Disable all register based interrupts, we'll have to change
+	 * this, protocol seemingly will require regs 0 and 15
+	 */
+	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
+	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
+
+	/* W1C */
+	mbox_outb(mbox_host, 0xff, MBOX_STATUS_0);
+	mbox_outb(mbox_host, 0xff, MBOX_STATUS_1);
+
+	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return 0;
+}
+
+static int mbox_host_probe(struct platform_device *pdev)
+{
+	struct mbox_host *mbox_host;
+	struct device *dev;
+	struct resource *res;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+	dev_info(dev, "Found mbox host device\n");
+
+	mbox_host = devm_kzalloc(dev, sizeof(*mbox_host), GFP_KERNEL);
+	if (!mbox_host)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, mbox_host);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Unable to find resources\n");
+		rc = -ENXIO;
+		goto out_free;
+	}
+
+	mbox_host->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!mbox_host->base) {
+		rc = -ENOMEM;
+		goto out_free;
+	}
+	init_waitqueue_head(&mbox_host->queue);
+
+	mbox_host->miscdev.minor = MISC_DYNAMIC_MINOR;
+	mbox_host->miscdev.name = DEVICE_NAME;
+	mbox_host->miscdev.fops = &mbox_host_fops;
+	mbox_host->miscdev.parent = dev;
+	rc = misc_register(&mbox_host->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register device\n");
+		goto out_unmap;
+	}
+
+	mbox_host_config_irq(mbox_host, pdev);
+
+	if (mbox_host->irq) {
+		dev_info(dev, "Using IRQ %d\n", mbox_host->irq);
+	} else {
+		dev_info(dev, "No IRQ; using timer\n");
+		setup_timer(&mbox_host->poll_timer, poll_timer,
+				(unsigned long)mbox_host);
+		mbox_host->poll_timer.expires = jiffies + msecs_to_jiffies(10);
+		add_timer(&mbox_host->poll_timer);
+	}
+	return 0;
+
+out_unmap:
+	devm_iounmap(&pdev->dev, mbox_host->base);
+
+out_free:
+	devm_kfree(dev, mbox_host);
+	return rc;
+
+}
+
+static int mbox_host_remove(struct platform_device *pdev)
+{
+	struct mbox_host *mbox_host = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&mbox_host->miscdev);
+	if (!mbox_host->irq)
+		del_timer_sync(&mbox_host->poll_timer);
+	devm_iounmap(&pdev->dev, mbox_host->base);
+	devm_kfree(&pdev->dev, mbox_host);
+	mbox_host = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id mbox_host_match[] = {
+	{ .compatible = "aspeed,mbox-host" },
+	{ },
+};
+
+static struct platform_driver mbox_host_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = mbox_host_match,
+	},
+	.probe = mbox_host_probe,
+	.remove = mbox_host_remove,
+};
+
+module_platform_driver(mbox_host_driver);
+
+MODULE_DEVICE_TABLE(of, mbox_host_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
+MODULE_DESCRIPTION("Linux device interface to the MBOX interface");
diff --git a/include/uapi/linux/mbox-host.h b/include/uapi/linux/mbox-host.h
new file mode 100644
index 0000000..3888f43
--- /dev/null
+++ b/include/uapi/linux/mbox-host.h
@@ -0,0 +1,18 @@
+/*
+ * 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_MBOX_HOST_H
+#define _UAPI_LINUX_MBOX_HOST_H
+
+#include <linux/ioctl.h>
+
+#define __MBOX_HOST_IOCTL_MAGIC	0xb1
+#define MBOX_HOST_IOCTL_ATN	_IOW(__MBOX_HOST_IOCTL_MAGIC, 0x00, uint8_t)
+
+#endif /* _UAPI_LINUX_MBOX_HOST_H */
-- 
2.10.2

[PATCH linux 3/3] drivers/misc: Add aspeed 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          |   7 ++
 drivers/misc/Makefile         |   2 +-
 drivers/misc/lpc-ctrl.c       | 251 ++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/lpc-ctrl.h |  25 +++++
 4 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/lpc-ctrl.c
 create mode 100644 include/uapi/linux/lpc-ctrl.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 44f191d..2bd3f92 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -811,6 +811,13 @@ config MBOX_HOST
 	---help---
 	  Build the MBOX driver
 
+config LPC_CTRL
+	depends on ARCH_ASPEED
+	bool "Build a driver to control the BMC to HOST LPC bus"
+	default "n"
+	---help---
+	  This will also use a BMC ram region defined in the device tree
+
 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 7bcaf30..e8502de 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
 obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
+obj-$(CONFIG_LPC_CTRL)		+= lpc-ctrl.o
diff --git a/drivers/misc/lpc-ctrl.c b/drivers/misc/lpc-ctrl.c
new file mode 100644
index 0000000..f72f44f
--- /dev/null
+++ b/drivers/misc/lpc-ctrl.c
@@ -0,0 +1,251 @@
+/*
+ * 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/mm.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/lpc-ctrl.h>
+
+#define DEVICE_NAME	"lpc-ctrl"
+
+#define LPC_HICR7 0x88
+#define LPC_HICR8 0x8c
+
+struct lpc_ctrl {
+	struct miscdevice	miscdev;
+	void __iomem		*ctrl;
+	phys_addr_t			base;
+	resource_size_t		size;
+};
+
+
+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)
+{
+	return 0;
+}
+
+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;
+
+	WARN_ON(*ppos);
+
+	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;
+
+	WARN_ON(*ppos);
+
+	return -EPERM;
+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	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;
+
+		iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
+				lpc_ctrl->ctrl + LPC_HICR7);
+		iowrite32((~(map.size - 1)) | ((map.size >> 16) - 1),
+				lpc_ctrl->ctrl + LPC_HICR8);
+		return 0;
+	case LPC_CTRL_IOCTL_UNMAP:
+		if (copy_from_user(&map, p, sizeof(map)))
+			return -EFAULT;
+
+		iowrite32((0x3000 << 16) | (0x0e00),
+				lpc_ctrl->ctrl + LPC_HICR7);
+		iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) - 1),
+				lpc_ctrl->ctrl + LPC_HICR8);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file *file)
+{
+	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 *res;
+	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;
+
+	dev_set_drvdata(&pdev->dev, lpc_ctrl);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Unable to find resources\n");
+		rc = -ENXIO;
+		goto out_free;
+	}
+
+	/* Todo unmap this on fail cases and exit */
+	lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
+	if (!lpc_ctrl->ctrl) {
+		rc = -ENOMEM;
+		goto out_free;
+	}
+
+	node = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!node) {
+		/*
+		 * Should probaby handle this by allocating 4-64k now and
+		 * using that
+		 */
+		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->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");
+		goto out;
+	}
+
+	dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+			lpc_ctrl->base, lpc_ctrl->size);
+	return 0;
+
+out:
+	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
+out_free:
+	devm_kfree(dev, lpc_ctrl);
+	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);
+	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
+	devm_kfree(&pdev->dev, lpc_ctrl);
+	lpc_ctrl = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
+	{ .compatible = "aspeed,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/lpc-ctrl.h b/include/uapi/linux/lpc-ctrl.h
new file mode 100644
index 0000000..c5f1caf
--- /dev/null
+++ b/include/uapi/linux/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.10.2

Re: [PATCH linux 1/3] ARM: dts: aspeed: Update palmetto device tree

[Joel Stanley]

Hi Cyril,

On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> Palmettos have 512mb of ram. There is always framebuffer memory at the
> top of ram.
>
> This patch also reserves BMC ram for host to BMC communication.
>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 25 ++++++++++++++++++++++++-
>  arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> index acaceda2..e810ae7 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> @@ -17,7 +17,30 @@
>         };
>
>         memory {
> -               reg = <0x40000000 0x10000000>;
> +               reg = <0x40000000 0x20000000>;
> +       };

Can you send this in a separate patch? It should go into the tree now.

> +
> +       reserved-memory {
> +               #address-cells = <1>;
> +               #size-cells= <1>;
> +               ranges;
> +
> +               flash_mem: region@52000000 {
> +                       compatible = "aspeed,lpc-ctrl";
> +                       no-map;
> +                       reg = <0x54000000 0x04000000>; /* 64m */
> +               };
> +
> +               fb_mem: framebuffer@5f000000 {
> +                       no-map;
> +                       reg = <0x5f000000 0x01000000>; /* 16m */
> +               };

This should be a separate patch too, which we can take straight away.

> +       };
> +
> +       flash_buffer {

It's convention to put the reg value after @ in the node name:

  flash_buffer@1e789000

> +               compatible = "aspeed,lpc-ctrl";

Upstream demands we put the SoC in the compatible string

 aspeed,ast2400-lpc-ctrl.

> +               memory-region = <&flash_mem>;
> +               reg = <0x1e789000 8>;

Make the second cell be 0x8 for consistency.

>         };

> --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi
> @@ -870,6 +870,12 @@
>                                 interrupts = <8>;
>                         };
>
> +                       mbox: mbox@1e789200 {
> +                               compatible = "aspeed,mbox-host";

aspeed,ast2400-mbox-host

Not sure on the name here.

> +                               reg = <0x1e789200 0x5c>;
> +                               interrupts = <46>;
> +                       };
> +
>                         wdt1: wdt@1e785000 {
>                                 compatible = "aspeed,ast2400-wdt";
>                                 reg = <0x1e785000 0x1c>;
> --
> 2.10.2
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH linux 3/3] drivers/misc: Add aspeed lpc controlling driver

[Joel Stanley]

Hi Cyril,

On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> 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

Jagha and Andrew are both interested in a LPC Host Controller driver.

I like your ioctl approach.

Is the register layout generic to many LPC Host Controllers, or is it
specific to the Aspeed chips? If it's specific then perhaps we should
add the aspeed_ prefix to the filename, kconfig symbol and functions.

Andrew has recently sent device tree bindings upstream for the LPC
address space. You should review them to make sure they describe the
hardware well enough for your purposes:

 http://patchwork.ozlabs.org/patch/693032/

With these bindings you will need to perform the memory accesses
through regmap, instead of directly mapping the address space
yourself.

Cheers,

Joel

>
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/misc/Kconfig          |   7 ++
>  drivers/misc/Makefile         |   2 +-
>  drivers/misc/lpc-ctrl.c       | 251 ++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/lpc-ctrl.h |  25 +++++
>  4 files changed, 284 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/misc/lpc-ctrl.c
>  create mode 100644 include/uapi/linux/lpc-ctrl.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 44f191d..2bd3f92 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -811,6 +811,13 @@ config MBOX_HOST
>         ---help---
>           Build the MBOX driver
>
> +config LPC_CTRL
> +       depends on ARCH_ASPEED
> +       bool "Build a driver to control the BMC to HOST LPC bus"
> +       default "n"
> +       ---help---
> +         This will also use a BMC ram region defined in the device tree
> +
>  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 7bcaf30..e8502de 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)         += cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
>  obj-$(CONFIG_MBOX_HOST)                += mbox-host.o
> ->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
> +obj-$(CONFIG_LPC_CTRL)         += lpc-ctrl.o
> diff --git a/drivers/misc/lpc-ctrl.c b/drivers/misc/lpc-ctrl.c
> new file mode 100644
> index 0000000..f72f44f
> --- /dev/null
> +++ b/drivers/misc/lpc-ctrl.c
> @@ -0,0 +1,251 @@
> +/*
> + * 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/mm.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/lpc-ctrl.h>
> +
> +#define DEVICE_NAME    "lpc-ctrl"
> +
> +#define LPC_HICR7 0x88
> +#define LPC_HICR8 0x8c
> +
> +struct lpc_ctrl {
> +       struct miscdevice       miscdev;
> +       void __iomem            *ctrl;
> +       phys_addr_t                     base;
> +       resource_size_t         size;
> +};
> +
> +
> +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)
> +{
> +       return 0;
> +}
> +
> +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;
> +
> +       WARN_ON(*ppos);
> +
> +       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;
> +
> +       WARN_ON(*ppos);
> +
> +       return -EPERM;
> +}
> +
> +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> +               unsigned long param)
> +{
> +       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;
> +
> +               iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
> +                               lpc_ctrl->ctrl + LPC_HICR7);
> +               iowrite32((~(map.size - 1)) | ((map.size >> 16) - 1),
> +                               lpc_ctrl->ctrl + LPC_HICR8);
> +               return 0;
> +       case LPC_CTRL_IOCTL_UNMAP:
> +               if (copy_from_user(&map, p, sizeof(map)))
> +                       return -EFAULT;
> +
> +               iowrite32((0x3000 << 16) | (0x0e00),
> +                               lpc_ctrl->ctrl + LPC_HICR7);
> +               iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) - 1),
> +                               lpc_ctrl->ctrl + LPC_HICR8);
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> +{
> +       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 *res;
> +       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;
> +
> +       dev_set_drvdata(&pdev->dev, lpc_ctrl);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_err(dev, "Unable to find resources\n");
> +               rc = -ENXIO;
> +               goto out_free;
> +       }
> +
> +       /* Todo unmap this on fail cases and exit */
> +       lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
> +       if (!lpc_ctrl->ctrl) {
> +               rc = -ENOMEM;
> +               goto out_free;
> +       }
> +
> +       node = of_parse_phandle(dev->of_node, "memory-region", 0);
> +       if (!node) {
> +               /*
> +                * Should probaby handle this by allocating 4-64k now and
> +                * using that
> +                */
> +               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->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");
> +               goto out;
> +       }
> +
> +       dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> +                       lpc_ctrl->base, lpc_ctrl->size);
> +       return 0;
> +
> +out:
> +       devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
> +out_free:
> +       devm_kfree(dev, lpc_ctrl);
> +       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);
> +       devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
> +       devm_kfree(&pdev->dev, lpc_ctrl);
> +       lpc_ctrl = NULL;
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id lpc_ctrl_match[] = {
> +       { .compatible = "aspeed,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/lpc-ctrl.h b/include/uapi/linux/lpc-ctrl.h
> new file mode 100644
> index 0000000..c5f1caf
> --- /dev/null
> +++ b/include/uapi/linux/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.10.2
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Joel Stanley]

On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

Explain why we need this chardev, why it needs ioctls, etc.

> ---
>  drivers/misc/Kconfig           |   7 +
>  drivers/misc/Makefile          |   2 +
>  drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/mbox-host.h |  18 +++
>  4 files changed, 353 insertions(+)
>  create mode 100644 drivers/misc/mbox-host.c
>  create mode 100644 include/uapi/linux/mbox-host.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b46..44f191d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,13 @@ 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 MBOX_HOST
> +       depends on ARCH_ASPEED

or COMPILE_TEST

> +       bool "Build MBOX driver for BMC to HOST communication"
> +       default "n"

If it's depending on ARCH_ASPEED I think default y is ok.

> +       ---help---
> +         Build the MBOX driver

More text here please.

> +
>  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 b2fb6dbf..7bcaf30 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile

"misc seems a bit too misc" - Andrew Jeffery.

How about drivers/soc? Or drivers/mfd?

> +#define DEVICE_NAME    "mbox-host"
> +
> +#define MBOX_NUM_REGS 16
> +#define MBOX_NUM_DATA_REGS 14
> +
> +#define MBOX_DATA_0 0x00
> +#define MBOX_STATUS_0 0x40
> +#define MBOX_STATUS_1 0x44
> +#define MBOX_BMC_CTRL 0x48
> +       #define MBOX_CTRL_RECV 0x80
> +       #define MBOX_CTRL_MASK 0x02
> +       #define MBOX_CTRL_SEND 0x01

If you want to do show these are bits in the CTRL register, do this. I
think it's more readable to represent the bits as shifts, or even
better, using the BIT() macro.

#define MBOX_BMC_CTRL 0x48
#define   MBOX_CTRL_RECV BIT(7)
#define   MBOX_CTRL_MASK BIT(1)

etc.

> +#define MBOX_HOST_CTRL 0x4c
> +#define MBOX_INTERRUPT_0 0x50
> +#define MBOX_INTERRUPT_1 0x54
> +

> +
> +static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
> +{
> +       return ioread8(mbox_host->base + reg);
> +}
> +
> +static void mbox_outb(struct mbox_host *mbox_host, u8 data, int reg)
> +{
> +       iowrite8(data, mbox_host->base + reg);
> +}

As mentioned in the other email, all access to the LPC address space
is through regmap.

> +
> +static struct mbox_host *file_mbox_host(struct file *file)
> +{
> +       return container_of(file->private_data, struct mbox_host, miscdev);
> +}
> +
> +static int mbox_host_open(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}

This means userspace can open this device an unlimited number of
times. I think we only want one user at a time?


> +static int mbox_host_release(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}

Why not omit the callback?

> +
> +static unsigned int mbox_host_poll(struct file *file, poll_table *wait)
> +{
> +       struct mbox_host *mbox_host = file_mbox_host(file);
> +       unsigned int mask = 0;
> +
> +       poll_wait(file, &mbox_host->queue, wait);
> +
> +       if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> +               mask |= POLLIN;
> +
> +       return mask;
> +}
> +
> +static const struct file_operations mbox_host_fops = {
> +       .owner          = THIS_MODULE,
> +       .open           = mbox_host_open,
> +       .read           = mbox_host_read,
> +       .write          = mbox_host_write,
> +       .release        = mbox_host_release,
> +       .poll           = mbox_host_poll,
> +       .unlocked_ioctl = mbox_host_ioctl,
> +};
> +
> +static void poll_timer(unsigned long data)
> +{
> +       struct mbox_host *mbox_host = (void *)data;
> +
> +       mbox_host->poll_timer.expires += msecs_to_jiffies(500);

Why 500?

> +       wake_up(&mbox_host->queue);
> +       add_timer(&mbox_host->poll_timer);
> +}
> +

Re: [PATCH linux 1/3] ARM: dts: aspeed: Update palmetto device tree

[Cyril Bur]

On Tue, 2016-12-13 at 11:44 +1100, Joel Stanley wrote:
> Hi Cyril,
> 
> On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> > Palmettos have 512mb of ram. There is always framebuffer memory at
> > the
> > top of ram.
> > 
> > This patch also reserves BMC ram for host to BMC communication.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 25
> > ++++++++++++++++++++++++-
> >  arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > index acaceda2..e810ae7 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > @@ -17,7 +17,30 @@
> >         };
> > 
> >         memory {
> > -               reg = <0x40000000 0x10000000>;
> > +               reg = <0x40000000 0x20000000>;
> > +       };
> 
> Can you send this in a separate patch? It should go into the tree
> now.
> 

Yep, should I send just for palmetto? 

Do you have a list of other platforms? I figure all the ast2400 based
aspeed-bmc-opp-* should get this change, should I include them all in
one patch or all individually?

> > +
> > +       reserved-memory {
> > +               #address-cells = <1>;
> > +               #size-cells= <1>;
> > +               ranges;
> > +
> > +               flash_mem: region@52000000 {
> > +                       compatible = "aspeed,lpc-ctrl";
> > +                       no-map;
> > +                       reg = <0x54000000 0x04000000>; /* 64m */
> > +               };
> > +
> > +               fb_mem: framebuffer@5f000000 {
> > +                       no-map;
> > +                       reg = <0x5f000000 0x01000000>; /* 16m */
> > +               };
> 
> This should be a separate patch too, which we can take straight away.
> 

I'll resend.

> > +       };
> > +
> > +       flash_buffer {
> 
> It's convention to put the reg value after @ in the node name:
> 
>   flash_buffer@1e789000
> 
> > +               compatible = "aspeed,lpc-ctrl";
> 
> Upstream demands we put the SoC in the compatible string
> 
>  aspeed,ast2400-lpc-ctrl.
> 
> > +               memory-region = <&flash_mem>;
> > +               reg = <0x1e789000 8>;
> 
> Make the second cell be 0x8 for consistency.
> 
> >         };
> > --- a/arch/arm/boot/dts/aspeed-g4.dtsi
> > +++ b/arch/arm/boot/dts/aspeed-g4.dtsi
> > @@ -870,6 +870,12 @@
> >                                 interrupts = <8>;
> >                         };
> > 
> > +                       mbox: mbox@1e789200 {
> > +                               compatible = "aspeed,mbox-host";
> 
> aspeed,ast2400-mbox-host
> 
> Not sure on the name here.
> 

Yeah I'm not sold on the name either...


Thanks for review.

Cyril

> > +                               reg = <0x1e789200 0x5c>;
> > +                               interrupts = <46>;
> > +                       };
> > +
> >                         wdt1: wdt@1e785000 {
> >                                 compatible = "aspeed,ast2400-wdt";
> >                                 reg = <0x1e785000 0x1c>;
> > --
> > 2.10.2
> > 
> > _______________________________________________
> > openbmc mailing list
> > openbmc@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH linux 1/3] ARM: dts: aspeed: Update palmetto device tree

[Joel Stanley]

On Tue, Dec 13, 2016 at 2:11 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
>> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>> > @@ -17,7 +17,30 @@
>> >         };
>> >
>> >         memory {
>> > -               reg = <0x40000000 0x10000000>;
>> > +               reg = <0x40000000 0x20000000>;
>> > +       };
>>
>> Can you send this in a separate patch? It should go into the tree
>> now.
>>
>
> Yep, should I send just for palmetto?
>
> Do you have a list of other platforms? I figure all the ast2400 based
> aspeed-bmc-opp-* should get this change, should I include them all in
> one patch or all individually?

One patch will do. I checked all of the machines and they all have
512MB (minus bits that I assume are reserved for framebuffers, etc.):

Barreleye: 499020 kB
Palmetto: 424204 kB
Firestone: 416020 kB
Garrison: 416012 kB

Cheers,

Joel

Re: [PATCH linux 1/3] ARM: dts: aspeed: Update palmetto device tree

[Cyril Bur]

On Tue, 2016-12-13 at 14:29 +1100, Joel Stanley wrote:
> On Tue, Dec 13, 2016 at 2:11 PM, Cyril Bur <cyrilbur@gmail.com>
> wrote:
> > > > --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > > > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > > > @@ -17,7 +17,30 @@
> > > >         };
> > > > 
> > > >         memory {
> > > > -               reg = <0x40000000 0x10000000>;
> > > > +               reg = <0x40000000 0x20000000>;
> > > > +       };
> > > 
> > > Can you send this in a separate patch? It should go into the tree
> > > now.
> > > 
> > 
> > Yep, should I send just for palmetto?
> > 
> > Do you have a list of other platforms? I figure all the ast2400
> > based
> > aspeed-bmc-opp-* should get this change, should I include them all
> > in
> > one patch or all individually?
> 
> One patch will do. I checked all of the machines and they all have
> 512MB (minus bits that I assume are reserved for framebuffers, etc.):
> 
> Barreleye: 499020 kB
> Palmetto: 424204 kB
> Firestone: 416020 kB
> Garrison: 416012 kB
> 
> 

Ok great thanks!

Final question, I assume the same thing in a separate patch is good for
the framebuffer, I'd forgotten that the size of VGA memory can be
altered by a hardware strapping reg '0x1E6E2070 Hardware Strapping
Table' bits 3:2 define it. The default is for 16MB, is it safe assume
the platforms will come up with strapping reg in default state?

> Cheers,
> 
> Joel

Re: [PATCH linux 1/3] ARM: dts: aspeed: Update palmetto device tree

[Joel Stanley]

On Tue, Dec 13, 2016 at 2:48 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> Final question, I assume the same thing in a separate patch is good for
> the framebuffer, I'd forgotten that the size of VGA memory can be
> altered by a hardware strapping reg '0x1E6E2070 Hardware Strapping
> Table' bits 3:2 define it. The default is for 16MB, is it safe assume
> the platforms will come up with strapping reg in default state?

No. They will come up in the strapped state.

The strapped state depends on a bunch of GPIOs being pulled up or down
at boot time.

We will move to using u-boot to populate the memory information in the
device tree before passing it to the kernel. For now you can hardcode
the values.

Cheers,

Joel

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Cédric Le Goater]

On 12/13/2016 02:26 AM, Joel Stanley wrote:
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index b2fb6dbf..7bcaf30 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
> "misc seems a bit too misc" - Andrew Jeffery.
> 
> How about drivers/soc? Or drivers/mfd?
> 

I would say drivers/mailbox. There are quite a few of these drivers already there.

C. 

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Cédric Le Goater]

Hello Cyril,

On 12/09/2016 06:43 AM, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/misc/Kconfig           |   7 +
>  drivers/misc/Makefile          |   2 +
>  drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/mbox-host.h |  18 +++
>  4 files changed, 353 insertions(+)
>  create mode 100644 drivers/misc/mbox-host.c
>  create mode 100644 include/uapi/linux/mbox-host.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b46..44f191d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,13 @@ 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 MBOX_HOST
> +	depends on ARCH_ASPEED
> +	bool "Build MBOX driver for BMC to HOST communication"
> +	default "n"
> +	---help---
> +	  Build the MBOX driver

nah. we need a little more doc here :)

>  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 b2fb6dbf..7bcaf30 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,3 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/
>  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> +obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
> +>>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
> diff --git a/drivers/misc/mbox-host.c b/drivers/misc/mbox-host.c
> new file mode 100644
> index 0000000..9d11da4
> --- /dev/null
> +++ b/drivers/misc/mbox-host.c
> @@ -0,0 +1,326 @@
> +/*
> + * 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/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/mbox-host.h>
> +
> +#define DEVICE_NAME	"mbox-host"
> +
> +#define MBOX_NUM_REGS 16
> +#define MBOX_NUM_DATA_REGS 14
> +
> +#define MBOX_DATA_0 0x00
> +#define MBOX_STATUS_0 0x40
> +#define MBOX_STATUS_1 0x44
> +#define MBOX_BMC_CTRL 0x48
> +	#define MBOX_CTRL_RECV 0x80
> +	#define MBOX_CTRL_MASK 0x02
> +	#define MBOX_CTRL_SEND 0x01
> +#define MBOX_HOST_CTRL 0x4c
> +#define MBOX_INTERRUPT_0 0x50
> +#define MBOX_INTERRUPT_1 0x54
> +
> +struct mbox_host {
> +	struct miscdevice	miscdev;
> +	void __iomem		*base;
> +	int			irq;
> +	wait_queue_head_t	queue;
> +	struct timer_list	poll_timer;
> +};

you should may be use an atomic to prevent multiple process to 
open the device. I don't think we want more than one ? 

What about multiple threads ? Maybe we need a mutex to protect 
the read and write ops ? 


> +static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
> +{
> +	return ioread8(mbox_host->base + reg);
> +}
> +
> +static void mbox_outb(struct mbox_host *mbox_host, u8 data, int reg)
> +{
> +	iowrite8(data, mbox_host->base + reg);
> +}
> +
> +static struct mbox_host *file_mbox_host(struct file *file)
> +{
> +	return container_of(file->private_data, struct mbox_host, miscdev);
> +}
> +
> +static int mbox_host_open(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static ssize_t mbox_host_read(struct file *file, char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	struct mbox_host *mbox_host = file_mbox_host(file);
> +	char __user *p = buf;
> +	int i;
> +
> +	if (!access_ok(VERIFY_WRITE, buf, count))
> +		return -EFAULT;
> +
> +	WARN_ON(*ppos);
> +
> +	if (wait_event_interruptible(mbox_host->queue,
> +		mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> +		return -ERESTARTSYS;
> +
> +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
> +		if (__put_user(mbox_inb(mbox_host, MBOX_DATA_0 + (i * 4)), p++))
> +			return -EFAULT;

could you change the code to not use byte at a time call from userspace ? and
any how, __put_user(mbox_inb(...)) looks like it could be splitted :)

> +	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> +	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> +	return p - buf;
> +}
> +
> +static ssize_t mbox_host_write(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	struct mbox_host *mbox_host = file_mbox_host(file);
> +	const char __user *p = buf;
> +	char c;
> +	int i;
> +
> +	if (!access_ok(VERIFY_READ, buf, count))
> +		return -EFAULT;
> +
> +	WARN_ON(*ppos);
> +
> +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++) {
> +		if (__get_user(c, p))
> +			return -EFAULT;
> +
> +		mbox_outb(mbox_host, c, MBOX_DATA_0 + (i * 4));
> +		p++;
> +	}

same here.

> +	mbox_outb(mbox_host, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
> +
> +	return p - buf;
> +}
> +
> +static long mbox_host_ioctl(struct file *file, unsigned int cmd,
> +		unsigned long param)
> +{
> +	struct mbox_host *mbox_host = file_mbox_host(file);
> +
> +	switch (cmd) {
> +	case MBOX_HOST_IOCTL_ATN:
> +		mbox_outb(mbox_host, param, MBOX_DATA_0 + 15);

15 ? 

> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mbox_host_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static unsigned int mbox_host_poll(struct file *file, poll_table *wait)
> +{
> +	struct mbox_host *mbox_host = file_mbox_host(file);
> +	unsigned int mask = 0;
> +
> +	poll_wait(file, &mbox_host->queue, wait);
> +
> +	if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> +		mask |= POLLIN;
> +
> +	return mask;
> +}
> +
> +static const struct file_operations mbox_host_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= mbox_host_open,
> +	.read		= mbox_host_read,
> +	.write		= mbox_host_write,
> +	.release	= mbox_host_release,
> +	.poll		= mbox_host_poll,
> +	.unlocked_ioctl	= mbox_host_ioctl,
> +};
> +
> +static void poll_timer(unsigned long data)
> +{
> +	struct mbox_host *mbox_host = (void *)data;
> +
> +	mbox_host->poll_timer.expires += msecs_to_jiffies(500);
> +	wake_up(&mbox_host->queue);
> +	add_timer(&mbox_host->poll_timer);
> +}
> +
> +static irqreturn_t mbox_host_irq(int irq, void *arg)
> +{
> +	struct mbox_host *mbox_host = arg;
> +
> +	if (!(mbox_inb(mbox_host, 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_host, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
> +
> +	wake_up(&mbox_host->queue);
> +	return IRQ_HANDLED;
> +}
> +
> +static int mbox_host_config_irq(struct mbox_host *mbox_host,
> +		struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int rc;
> +
> +	mbox_host->irq = irq_of_parse_and_map(dev->of_node, 0);
> +	if (!mbox_host->irq)
> +		return -ENODEV;
> +
> +	rc = devm_request_irq(dev, mbox_host->irq, mbox_host_irq, IRQF_SHARED,
> +			DEVICE_NAME, mbox_host);
> +	if (rc < 0) {
> +		dev_warn(dev, "Unable to request IRQ %d\n", mbox_host->irq);
> +		mbox_host->irq = 0;
> +		return rc;
> +	}
> +
> +	/*
> +	 * Disable all register based interrupts, we'll have to change
> +	 * this, protocol seemingly will require regs 0 and 15
> +	 */
> +	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
> +	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
> +
> +	/* W1C */
> +	mbox_outb(mbox_host, 0xff, MBOX_STATUS_0);
> +	mbox_outb(mbox_host, 0xff, MBOX_STATUS_1);
> +
> +	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> +	return 0;
> +}
> +
> +static int mbox_host_probe(struct platform_device *pdev)
> +{
> +	struct mbox_host *mbox_host;
> +	struct device *dev;
> +	struct resource *res;
> +	int rc;
> +
> +	if (!pdev || !pdev->dev.of_node)
> +		return -ENODEV;
> +
> +	dev = &pdev->dev;
> +	dev_info(dev, "Found mbox host device\n");
> +
> +	mbox_host = devm_kzalloc(dev, sizeof(*mbox_host), GFP_KERNEL);
> +	if (!mbox_host)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, mbox_host);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "Unable to find resources\n");
> +		rc = -ENXIO;
> +		goto out_free;
> +	}
> +
> +	mbox_host->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!mbox_host->base) {
> +		rc = -ENOMEM;
> +		goto out_free;
> +	}
> +	init_waitqueue_head(&mbox_host->queue);
> +
> +	mbox_host->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	mbox_host->miscdev.name = DEVICE_NAME;
> +	mbox_host->miscdev.fops = &mbox_host_fops;
> +	mbox_host->miscdev.parent = dev;
> +	rc = misc_register(&mbox_host->miscdev);
> +	if (rc) {
> +		dev_err(dev, "Unable to register device\n");
> +		goto out_unmap;
> +	}
> +
> +	mbox_host_config_irq(mbox_host, pdev);
> +
> +	if (mbox_host->irq) {
> +		dev_info(dev, "Using IRQ %d\n", mbox_host->irq);
> +	} else {
> +		dev_info(dev, "No IRQ; using timer\n");
> +		setup_timer(&mbox_host->poll_timer, poll_timer,
> +				(unsigned long)mbox_host);
> +		mbox_host->poll_timer.expires = jiffies + msecs_to_jiffies(10);
> +		add_timer(&mbox_host->poll_timer);
> +	}
> +	return 0;
> +
> +out_unmap:
> +	devm_iounmap(&pdev->dev, mbox_host->base);
> +
> +out_free:
> +	devm_kfree(dev, mbox_host);
> +	return rc;

These calls should be handled automatically by the driver when you use the devm_*
routines

> +}
> +
> +static int mbox_host_remove(struct platform_device *pdev)
> +{
> +	struct mbox_host *mbox_host = dev_get_drvdata(&pdev->dev);
> +
> +	misc_deregister(&mbox_host->miscdev);
> +	if (!mbox_host->irq)
> +		del_timer_sync(&mbox_host->poll_timer);
> +	devm_iounmap(&pdev->dev, mbox_host->base);
> +	devm_kfree(&pdev->dev, mbox_host);


These calls should be handled automatically by the driver.


> +	mbox_host = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mbox_host_match[] = {
> +	{ .compatible = "aspeed,mbox-host" },

you need a soc revision here. 


Thanks,

C. 

> +	{ },
> +};
> +
> +static struct platform_driver mbox_host_driver = {
> +	.driver = {
> +		.name		= DEVICE_NAME,
> +		.of_match_table = mbox_host_match,
> +	},
> +	.probe = mbox_host_probe,
> +	.remove = mbox_host_remove,
> +};
> +
> +module_platform_driver(mbox_host_driver);
> +
> +MODULE_DEVICE_TABLE(of, mbox_host_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Linux device interface to the MBOX interface");
> diff --git a/include/uapi/linux/mbox-host.h b/include/uapi/linux/mbox-host.h
> new file mode 100644
> index 0000000..3888f43
> --- /dev/null
> +++ b/include/uapi/linux/mbox-host.h
> @@ -0,0 +1,18 @@
> +/*
> + * 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_MBOX_HOST_H
> +#define _UAPI_LINUX_MBOX_HOST_H
> +
> +#include <linux/ioctl.h>
> +
> +#define __MBOX_HOST_IOCTL_MAGIC	0xb1
> +#define MBOX_HOST_IOCTL_ATN	_IOW(__MBOX_HOST_IOCTL_MAGIC, 0x00, uint8_t)
> +
> +#endif /* _UAPI_LINUX_MBOX_HOST_H */
> 

Re: [PATCH linux 0/3] LPC/MBOX work

[Cédric Le Goater]

On 12/09/2016 06:43 AM, Cyril Bur wrote:
> Cedric (or anyone really): Do you know of a way the lpc-ctrl driver
> could query the MTD subsystem for the size of the PNOR? There is
> currently a hack in the UNMAP ioctl() which can't hang around.

well, it is a platform device so you could loop on them I guess. 
Or maybe use register_mtd_user() ? 

C.

Re: [PATCH linux 0/3] LPC/MBOX work

[Cyril Bur]

On Tue, 2016-12-13 at 11:24 +0100, Cédric Le Goater wrote:
> On 12/09/2016 06:43 AM, Cyril Bur wrote:
> > Cedric (or anyone really): Do you know of a way the lpc-ctrl driver
> > could query the MTD subsystem for the size of the PNOR? There is
> > currently a hack in the UNMAP ioctl() which can't hang around.
> 
> well, it is a platform device so you could loop on them I guess. 
> Or maybe use register_mtd_user() ? 

Oh yes looks like that will work.

Thanks,

Cyril
> 
> C.

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Andrew Jeffery]

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

On Fri, 2016-12-09 at 16:43 +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/misc/Kconfig           |   7 +
>  drivers/misc/Makefile          |   2 +
>  drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++

Is drivers/misc the right spot? drivers/mailbox seems to be a thing -
is there any reason it doesn't live there?

>  include/uapi/linux/mbox-host.h |  18 +++
>  4 files changed, 353 insertions(+)
>  create mode 100644 drivers/misc/mbox-host.c
>  create mode 100644 include/uapi/linux/mbox-host.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b46..44f191d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,13 @@ 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 MBOX_HOST
> +	depends on ARCH_ASPEED

Given we depend on ARCH_ASPEED maybe we should have 'ASPEED' in the
config symbol? ASPEED_MBOX_HOST?

Also, as Joel's pointed out in other patches, we should probably add ||
COMPILE_TEST as well.

Andrew

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

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Andrew Jeffery]

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

On Fri, 2016-12-09 at 16:43 +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> 
> ---
>  drivers/misc/Kconfig           |   7 +
>  drivers/misc/Makefile          |   2 +
>  drivers/misc/mbox-host.c       | 326
> +++++++++++++++++++++++++++++++++++++++++

Is drivers/misc the right spot? drivers/mailbox seems to be a thing -
is there any reason it doesn't live there?

>  include/uapi/linux/mbox-host.h |  18 +++
>  4 files changed, 353 insertions(+)
>  create mode 100644 drivers/misc/mbox-host.c
>  create mode 100644 include/uapi/linux/mbox-host.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b46..44f191d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,13 @@ 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 MBOX_HOST
> +	depends on ARCH_ASPEED

Given we depend on ARCH_ASPEED maybe we should have 'ASPEED' in the
config symbol? ASPEED_MBOX_HOST?

Also, as Joel's pointed out in other patches, we should probably add ||
COMPILE_TEST as well.

Andrew

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

Re: [PATCH linux 3/3] drivers/misc: Add aspeed lpc controlling driver

[Andrew Jeffery]

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

On Fri, 2016-12-09 at 16:43 +1100, Cyril Bur wrote:
> 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          |   7 ++
>  drivers/misc/Makefile         |   2 +-
>  drivers/misc/lpc-ctrl.c       | 251 ++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/lpc-ctrl.h |  25 +++++

The naming might be a bit generic. Control of what? Any LPC device?

>  4 files changed, 284 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/misc/lpc-ctrl.c
>  create mode 100644 include/uapi/linux/lpc-ctrl.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 44f191d..2bd3f92 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -811,6 +811,13 @@ config MBOX_HOST
> >  	---help---
> >  	  Build the MBOX driver
>  
> +config LPC_CTRL
> +	depends on ARCH_ASPEED

Again, maybe we need ASPEED in the config symbol name? And ||
COMPILE_TEST here too.

> +	bool "Build a driver to control the BMC to HOST LPC bus"
> > +	default "n"
> > +	---help---
> > +	  This will also use a BMC ram region defined in the device tree
> +
>  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 7bcaf30..e8502de 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> > @@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
> >  obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> >  obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
> ->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver

This doesn't look right.

> +obj-$(CONFIG_LPC_CTRL)		+= lpc-ctrl.o
> diff --git a/drivers/misc/lpc-ctrl.c b/drivers/misc/lpc-ctrl.c
> new file mode 100644
> index 0000000..f72f44f
> --- /dev/null
> +++ b/drivers/misc/lpc-ctrl.c
> @@ -0,0 +1,251 @@
> +/*
> + * 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/mm.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/lpc-ctrl.h>
> +
> > +#define DEVICE_NAME	"lpc-ctrl"
> +
> +#define LPC_HICR7 0x88
> +#define LPC_HICR8 0x8c
> +
> +struct lpc_ctrl {
> > > +	struct miscdevice	miscdev;
> > > +	void __iomem		*ctrl;
> > > +	phys_addr_t			base;
> > > +	resource_size_t		size;
> +};
> +
> +
> +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)
> +{
> > +	return 0;
> +}
> +
> +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;
> +
> > +	WARN_ON(*ppos);
> +
> > +	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;
> +
> > +	WARN_ON(*ppos);
> +
> > +	return -EPERM;
> +}
> +
> +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long param)
> +{
> > +	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;
> +
> > +		iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
> > +				lpc_ctrl->ctrl + LPC_HICR7);
> > +		iowrite32((~(map.size - 1)) | ((map.size >> 16) - 1),
> +				lpc_ctrl->ctrl + LPC_HICR8);

I think these operations could do with some explanation.

> +		return 0;
> > +	case LPC_CTRL_IOCTL_UNMAP:
> > +		if (copy_from_user(&map, p, sizeof(map)))
> +			return -EFAULT;

Is this required? We don't use map below.

> +
> > +		iowrite32((0x3000 << 16) | (0x0e00),
> > +				lpc_ctrl->ctrl + LPC_HICR7);
> > +		iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) - 1),
> +				lpc_ctrl->ctrl + LPC_HICR8);

Can you comment/#define the magic numbers? I'm not across why we use
these values.

> +		return 0;
> > +	}
> +
> > +	return -EINVAL;
> +}
> +
> +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> +{
> > +	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 *res;
> > +	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;
> +
> > +	dev_set_drvdata(&pdev->dev, lpc_ctrl);
> +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(dev, "Unable to find resources\n");
> > +		rc = -ENXIO;
> > +		goto out_free;
> > +	}
> +
> +	/* Todo unmap this on fail cases and exit */

You're using devm_*, which if I understand correctly should handle this
for you.

> +	lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
> > +	if (!lpc_ctrl->ctrl) {
> > +		rc = -ENOMEM;
> > +		goto out_free;
> > +	}
> +
> > +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> > +	if (!node) {
> > +		/*
> > +		 * Should probaby handle this by allocating 4-64k now and
> > +		 * using that
> > +		 */
> > +		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->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");
> > +		goto out;
> > +	}
> +
> > +	dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > +			lpc_ctrl->base, lpc_ctrl->size);
> > +	return 0;
> +
> +out:
> > +	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
> +out_free:
> +	devm_kfree(dev, lpc_ctrl);

You shouldn't have to worry about these devm_*() calls.

> +	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);
> > +	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
> +	devm_kfree(&pdev->dev, lpc_ctrl);

As above.

Andrew

> +	lpc_ctrl = NULL;
> +
> > +	return 0;
> +}
> +
> +static const struct of_device_id lpc_ctrl_match[] = {
> > +	{ .compatible = "aspeed,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/lpc-ctrl.h b/include/uapi/linux/lpc-ctrl.h
> new file mode 100644
> index 0000000..c5f1caf
> --- /dev/null
> +++ b/include/uapi/linux/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 */

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

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Cyril Bur]

On Tue, 2016-12-13 at 12:26 +1100, Joel Stanley wrote:
> On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur@gmail.com> wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> 
> Explain why we need this chardev, why it needs ioctls, etc.
> 

Yeah, we should probably also work on the name and stuff

> > ---
> >  drivers/misc/Kconfig           |   7 +
> >  drivers/misc/Makefile          |   2 +
> >  drivers/misc/mbox-host.c       | 326
> > +++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/mbox-host.h |  18 +++
> >  4 files changed, 353 insertions(+)
> >  create mode 100644 drivers/misc/mbox-host.c
> >  create mode 100644 include/uapi/linux/mbox-host.h
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index a216b46..44f191d 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -804,6 +804,13 @@ 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 MBOX_HOST
> > +       depends on ARCH_ASPEED
> 
> or COMPILE_TEST

Is both a good solution?

> 
> > +       bool "Build MBOX driver for BMC to HOST communication"
> > +       default "n"
> 
> If it's depending on ARCH_ASPEED I think default y is ok.
> 

Yes

> > +       ---help---
> > +         Build the MBOX driver
> 
> More text here please.
> 
> > +
> >  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 b2fb6dbf..7bcaf30 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> 
> "misc seems a bit too misc" - Andrew Jeffery.
> 
> How about drivers/soc? Or drivers/mfd?

I think we settled on mailbox?

> 
> > +#define DEVICE_NAME    "mbox-host"
> > +
> > +#define MBOX_NUM_REGS 16
> > +#define MBOX_NUM_DATA_REGS 14
> > +
> > +#define MBOX_DATA_0 0x00
> > +#define MBOX_STATUS_0 0x40
> > +#define MBOX_STATUS_1 0x44
> > +#define MBOX_BMC_CTRL 0x48
> > +       #define MBOX_CTRL_RECV 0x80
> > +       #define MBOX_CTRL_MASK 0x02
> > +       #define MBOX_CTRL_SEND 0x01
> 
> If you want to do show these are bits in the CTRL register, do this.
> I
> think it's more readable to represent the bits as shifts, or even
> better, using the BIT() macro.
> 
> #define MBOX_BMC_CTRL 0x48
> #define   MBOX_CTRL_RECV BIT(7)
> #define   MBOX_CTRL_MASK BIT(1)
> 
> etc.
> 

Ah yes looks much better now, thanks

> > +#define MBOX_HOST_CTRL 0x4c
> > +#define MBOX_INTERRUPT_0 0x50
> > +#define MBOX_INTERRUPT_1 0x54
> > +
> > +
> > +static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
> > +{
> > +       return ioread8(mbox_host->base + reg);
> > +}
> > +
> > +static void mbox_outb(struct mbox_host *mbox_host, u8 data, int
> > reg)
> > +{
> > +       iowrite8(data, mbox_host->base + reg);
> > +}
> 
> As mentioned in the other email, all access to the LPC address space
> is through regmap.

Yes.

> 
> > +
> > +static struct mbox_host *file_mbox_host(struct file *file)
> > +{
> > +       return container_of(file->private_data, struct mbox_host,
> > miscdev);
> > +}
> > +
> > +static int mbox_host_open(struct inode *inode, struct file *file)
> > +{
> > +       return 0;
> > +}
> 
> This means userspace can open this device an unlimited number of
> times. I think we only want one user at a time?
> 

That would be wise yes

> 
> > +static int mbox_host_release(struct inode *inode, struct file
> > *file)
> > +{
> > +       return 0;
> > +}
> 
> Why not omit the callback?
> 
> > +
> > +static unsigned int mbox_host_poll(struct file *file, poll_table
> > *wait)
> > +{
> > +       struct mbox_host *mbox_host = file_mbox_host(file);
> > +       unsigned int mask = 0;
> > +
> > +       poll_wait(file, &mbox_host->queue, wait);
> > +
> > +       if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> > +               mask |= POLLIN;
> > +
> > +       return mask;
> > +}
> > +
> > +static const struct file_operations mbox_host_fops = {
> > +       .owner          = THIS_MODULE,
> > +       .open           = mbox_host_open,
> > +       .read           = mbox_host_read,
> > +       .write          = mbox_host_write,
> > +       .release        = mbox_host_release,
> > +       .poll           = mbox_host_poll,
> > +       .unlocked_ioctl = mbox_host_ioctl,
> > +};
> > +
> > +static void poll_timer(unsigned long data)
> > +{
> > +       struct mbox_host *mbox_host = (void *)data;
> > +
> > +       mbox_host->poll_timer.expires += msecs_to_jiffies(500);
> 
> Why 500?

Why not? I feel like I should test and justify a number, I honestly
have no idea whats good but 500msecs feels decent... perhaps I'll
determine a better number, or a good reason for 500 in my infinite
spare time ;)


Thanks for the review,

Cyril

> 
> > +       wake_up(&mbox_host->queue);
> > +       add_timer(&mbox_host->poll_timer);
> > +}
> > +

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Cyril Bur]

On Tue, 2016-12-13 at 09:14 +0100, Cédric Le Goater wrote:
> Hello Cyril,
> 
> On 12/09/2016 06:43 AM, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> >  drivers/misc/Kconfig           |   7 +
> >  drivers/misc/Makefile          |   2 +
> >  drivers/misc/mbox-host.c       | 326
> > +++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/mbox-host.h |  18 +++
> >  4 files changed, 353 insertions(+)
> >  create mode 100644 drivers/misc/mbox-host.c
> >  create mode 100644 include/uapi/linux/mbox-host.h
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index a216b46..44f191d 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -804,6 +804,13 @@ 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 MBOX_HOST
> > +	depends on ARCH_ASPEED
> > +	bool "Build MBOX driver for BMC to HOST communication"
> > +	default "n"
> > +	---help---
> > +	  Build the MBOX driver
> 
> nah. we need a little more doc here :)

Probably :), I'm terrible at this kind of stuff - I'll be sure to have
something before we finally upstream it.

> 
> >  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 b2fb6dbf..7bcaf30 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -57,3 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/
> >  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
> >  obj-$(CONFIG_CXL_BASE)		+= cxl/
> >  obj-$(CONFIG_PANEL)             += panel.o
> > +obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
> > +>>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
> > diff --git a/drivers/misc/mbox-host.c b/drivers/misc/mbox-host.c
> > new file mode 100644
> > index 0000000..9d11da4
> > --- /dev/null
> > +++ b/drivers/misc/mbox-host.c
> > @@ -0,0 +1,326 @@
> > +/*
> > + * 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/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/errno.h>
> > +#include <linux/poll.h>
> > +#include <linux/sched.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/slab.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/delay.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/timer.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mbox-host.h>
> > +
> > +#define DEVICE_NAME	"mbox-host"
> > +
> > +#define MBOX_NUM_REGS 16
> > +#define MBOX_NUM_DATA_REGS 14
> > +
> > +#define MBOX_DATA_0 0x00
> > +#define MBOX_STATUS_0 0x40
> > +#define MBOX_STATUS_1 0x44
> > +#define MBOX_BMC_CTRL 0x48
> > +	#define MBOX_CTRL_RECV 0x80
> > +	#define MBOX_CTRL_MASK 0x02
> > +	#define MBOX_CTRL_SEND 0x01
> > +#define MBOX_HOST_CTRL 0x4c
> > +#define MBOX_INTERRUPT_0 0x50
> > +#define MBOX_INTERRUPT_1 0x54
> > +
> > +struct mbox_host {
> > +	struct miscdevice	miscdev;
> > +	void __iomem		*base;
> > +	int			irq;
> > +	wait_queue_head_t	queue;
> > +	struct timer_list	poll_timer;
> > +};
> 
> you should may be use an atomic to prevent multiple process to 
> open the device. I don't think we want more than one ? 
> 

Yes, I suppose it would be wise - I'll add that

> What about multiple threads ? Maybe we need a mutex to protect 
> the read and write ops ? 

Thoughts? I'm not against letting userspace shoot its self in the foot
here. Protecting against multiple processes does make sense but if
you're going to have two threads writing, you're doing something
wrong... I don't have particularly strong feelings either way...

> 
> 
> > +static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
> > +{
> > +	return ioread8(mbox_host->base + reg);
> > +}
> > +
> > +static void mbox_outb(struct mbox_host *mbox_host, u8 data, int
> > reg)
> > +{
> > +	iowrite8(data, mbox_host->base + reg);
> > +}
> > +
> > +static struct mbox_host *file_mbox_host(struct file *file)
> > +{
> > +	return container_of(file->private_data, struct mbox_host,
> > miscdev);
> > +}
> > +
> > +static int mbox_host_open(struct inode *inode, struct file *file)
> > +{
> > +	return 0;
> > +}
> > +
> > +static ssize_t mbox_host_read(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	struct mbox_host *mbox_host = file_mbox_host(file);
> > +	char __user *p = buf;
> > +	int i;
> > +
> > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > +		return -EFAULT;
> > +
> > +	WARN_ON(*ppos);
> > +
> > +	if (wait_event_interruptible(mbox_host->queue,
> > +		mbox_inb(mbox_host, MBOX_BMC_CTRL) &
> > MBOX_CTRL_RECV))
> > +		return -ERESTARTSYS;
> > +
> > +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
> > +		if (__put_user(mbox_inb(mbox_host, MBOX_DATA_0 +
> > (i * 4)), p++))
> > +			return -EFAULT;
> 
> could you change the code to not use byte at a time call from
> userspace ? and
> any how, __put_user(mbox_inb(...)) looks like it could be splitted :)

Yeah, I could/should buffer it.

> 
> > +	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
> > */
> > +	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +	return p - buf;
> > +}
> > +
> > +static ssize_t mbox_host_write(struct file *file, const char
> > __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	struct mbox_host *mbox_host = file_mbox_host(file);
> > +	const char __user *p = buf;
> > +	char c;
> > +	int i;
> > +
> > +	if (!access_ok(VERIFY_READ, buf, count))
> > +		return -EFAULT;
> > +
> > +	WARN_ON(*ppos);
> > +
> > +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++) {
> > +		if (__get_user(c, p))
> > +			return -EFAULT;
> > +
> > +		mbox_outb(mbox_host, c, MBOX_DATA_0 + (i * 4));
> > +		p++;
> > +	}
> 
> same here.

Yep.

> 
> > +	mbox_outb(mbox_host, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
> > +
> > +	return p - buf;
> > +}
> > +
> > +static long mbox_host_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long param)
> > +{
> > +	struct mbox_host *mbox_host = file_mbox_host(file);
> > +
> > +	switch (cmd) {
> > +	case MBOX_HOST_IOCTL_ATN:
> > +		mbox_outb(mbox_host, param, MBOX_DATA_0 + 15);
> 
> 15 ? 

Ah yes nice catch. Ignoring the obvious issue that 15 is quite magic,
theres actually a bit more of a problem. This is really a part of the
userspace protocol I've developed. The kernel really shouldn't know
which byte the host is listening to receive interrupts and I'm not
super comfortable with this. Perhaps I should actually change the
ioctl() to actually let userspace write a single byte to any of the 16
data regs, this means the kernel doesn't actually have to know anything
special about my protocol.

Yeah I'll just do that, I'm glad we all agree. heh

> 
> > +		return 0;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int mbox_host_release(struct inode *inode, struct file
> > *file)
> > +{
> > +	return 0;
> > +}
> > +
> > +static unsigned int mbox_host_poll(struct file *file, poll_table
> > *wait)
> > +{
> > +	struct mbox_host *mbox_host = file_mbox_host(file);
> > +	unsigned int mask = 0;
> > +
> > +	poll_wait(file, &mbox_host->queue, wait);
> > +
> > +	if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> > +		mask |= POLLIN;
> > +
> > +	return mask;
> > +}
> > +
> > +static const struct file_operations mbox_host_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.open		= mbox_host_open,
> > +	.read		= mbox_host_read,
> > +	.write		= mbox_host_write,
> > +	.release	= mbox_host_release,
> > +	.poll		= mbox_host_poll,
> > +	.unlocked_ioctl	= mbox_host_ioctl,
> > +};
> > +
> > +static void poll_timer(unsigned long data)
> > +{
> > +	struct mbox_host *mbox_host = (void *)data;
> > +
> > +	mbox_host->poll_timer.expires += msecs_to_jiffies(500);
> > +	wake_up(&mbox_host->queue);
> > +	add_timer(&mbox_host->poll_timer);
> > +}
> > +
> > +static irqreturn_t mbox_host_irq(int irq, void *arg)
> > +{
> > +	struct mbox_host *mbox_host = arg;
> > +
> > +	if (!(mbox_inb(mbox_host, 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_host, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
> > +
> > +	wake_up(&mbox_host->queue);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mbox_host_config_irq(struct mbox_host *mbox_host,
> > +		struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	int rc;
> > +
> > +	mbox_host->irq = irq_of_parse_and_map(dev->of_node, 0);
> > +	if (!mbox_host->irq)
> > +		return -ENODEV;
> > +
> > +	rc = devm_request_irq(dev, mbox_host->irq, mbox_host_irq,
> > IRQF_SHARED,
> > +			DEVICE_NAME, mbox_host);
> > +	if (rc < 0) {
> > +		dev_warn(dev, "Unable to request IRQ %d\n",
> > mbox_host->irq);
> > +		mbox_host->irq = 0;
> > +		return rc;
> > +	}
> > +
> > +	/*
> > +	 * Disable all register based interrupts, we'll have to
> > change
> > +	 * this, protocol seemingly will require regs 0 and 15
> > +	 */
> > +	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_0); /* regs 0 -
> > 7 */
> > +	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_1); /* regs 8 -
> > 15 */
> > +
> > +	/* W1C */
> > +	mbox_outb(mbox_host, 0xff, MBOX_STATUS_0);
> > +	mbox_outb(mbox_host, 0xff, MBOX_STATUS_1);
> > +
> > +	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +	return 0;
> > +}
> > +
> > +static int mbox_host_probe(struct platform_device *pdev)
> > +{
> > +	struct mbox_host *mbox_host;
> > +	struct device *dev;
> > +	struct resource *res;
> > +	int rc;
> > +
> > +	if (!pdev || !pdev->dev.of_node)
> > +		return -ENODEV;
> > +
> > +	dev = &pdev->dev;
> > +	dev_info(dev, "Found mbox host device\n");
> > +
> > +	mbox_host = devm_kzalloc(dev, sizeof(*mbox_host),
> > GFP_KERNEL);
> > +	if (!mbox_host)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(&pdev->dev, mbox_host);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(dev, "Unable to find resources\n");
> > +		rc = -ENXIO;
> > +		goto out_free;
> > +	}
> > +
> > +	mbox_host->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (!mbox_host->base) {
> > +		rc = -ENOMEM;
> > +		goto out_free;
> > +	}
> > +	init_waitqueue_head(&mbox_host->queue);
> > +
> > +	mbox_host->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	mbox_host->miscdev.name = DEVICE_NAME;
> > +	mbox_host->miscdev.fops = &mbox_host_fops;
> > +	mbox_host->miscdev.parent = dev;
> > +	rc = misc_register(&mbox_host->miscdev);
> > +	if (rc) {
> > +		dev_err(dev, "Unable to register device\n");
> > +		goto out_unmap;
> > +	}
> > +
> > +	mbox_host_config_irq(mbox_host, pdev);
> > +
> > +	if (mbox_host->irq) {
> > +		dev_info(dev, "Using IRQ %d\n", mbox_host->irq);
> > +	} else {
> > +		dev_info(dev, "No IRQ; using timer\n");
> > +		setup_timer(&mbox_host->poll_timer, poll_timer,
> > +				(unsigned long)mbox_host);
> > +		mbox_host->poll_timer.expires = jiffies +
> > msecs_to_jiffies(10);
> > +		add_timer(&mbox_host->poll_timer);
> > +	}
> > +	return 0;
> > +
> > +out_unmap:
> > +	devm_iounmap(&pdev->dev, mbox_host->base);
> > +
> > +out_free:
> > +	devm_kfree(dev, mbox_host);
> > +	return rc;
> 
> These calls should be handled automatically by the driver when you
> use the devm_*
> routines

Yep, wasn't sure, I'll take them out

> 
> > +}
> > +
> > +static int mbox_host_remove(struct platform_device *pdev)
> > +{
> > +	struct mbox_host *mbox_host = dev_get_drvdata(&pdev->dev);
> > +
> > +	misc_deregister(&mbox_host->miscdev);
> > +	if (!mbox_host->irq)
> > +		del_timer_sync(&mbox_host->poll_timer);
> > +	devm_iounmap(&pdev->dev, mbox_host->base);
> > +	devm_kfree(&pdev->dev, mbox_host);
> 
> 
> These calls should be handled automatically by the driver.
> 
> 
> > +	mbox_host = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mbox_host_match[] = {
> > +	{ .compatible = "aspeed,mbox-host" },
> 
> you need a soc revision here. 
> 

Ack

> 

Thanks for the review,

Cyril

> Thanks,
> 
> C. 
> 
> > +	{ },
> > +};
> > +
> > +static struct platform_driver mbox_host_driver = {
> > +	.driver = {
> > +		.name		= DEVICE_NAME,
> > +		.of_match_table = mbox_host_match,
> > +	},
> > +	.probe = mbox_host_probe,
> > +	.remove = mbox_host_remove,
> > +};
> > +
> > +module_platform_driver(mbox_host_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, mbox_host_match);
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > +MODULE_DESCRIPTION("Linux device interface to the MBOX
> > interface");
> > diff --git a/include/uapi/linux/mbox-host.h
> > b/include/uapi/linux/mbox-host.h
> > new file mode 100644
> > index 0000000..3888f43
> > --- /dev/null
> > +++ b/include/uapi/linux/mbox-host.h
> > @@ -0,0 +1,18 @@
> > +/*
> > + * 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_MBOX_HOST_H
> > +#define _UAPI_LINUX_MBOX_HOST_H
> > +
> > +#include <linux/ioctl.h>
> > +
> > +#define __MBOX_HOST_IOCTL_MAGIC	0xb1
> > +#define MBOX_HOST_IOCTL_ATN	_IOW(__MBOX_HOST_IOCTL_MAGIC,
> > 0x00, uint8_t)
> > +
> > +#endif /* _UAPI_LINUX_MBOX_HOST_H */
> > 
> 
> 

Re: [PATCH linux 3/3] drivers/misc: Add aspeed lpc controlling driver

[Cyril Bur]

On Thu, 2016-12-15 at 11:28 +1030, Andrew Jeffery wrote:
> On Fri, 2016-12-09 at 16:43 +1100, Cyril Bur wrote:
> > 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          |   7 ++
> >  drivers/misc/Makefile         |   2 +-
> >  drivers/misc/lpc-ctrl.c       | 251
> > ++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/lpc-ctrl.h |  25 +++++
> 
> The naming might be a bit generic. Control of what? Any LPC device?
> 
> >  4 files changed, 284 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/misc/lpc-ctrl.c
> >  create mode 100644 include/uapi/linux/lpc-ctrl.h
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 44f191d..2bd3f92 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -811,6 +811,13 @@ config MBOX_HOST
> > >  	---help---
> > >  	  Build the MBOX driver
> > 
> >  
> > +config LPC_CTRL
> > +	depends on ARCH_ASPEED
> 
> Again, maybe we need ASPEED in the config symbol name? And ||
> COMPILE_TEST here too.

Yeah will do.

> 
> > +	bool "Build a driver to control the BMC to HOST LPC bus"
> > > +	default "n"
> > > +	---help---
> > > +	  This will also use a BMC ram region defined in the
> > > device tree
> > 
> > +
> >  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 7bcaf30..e8502de 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > > @@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+=
> > > vexpress-syscfg.o
> > >  obj-$(CONFIG_CXL_BASE)		+= cxl/
> > 
> >  obj-$(CONFIG_PANEL)             += panel.o
> > >  obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
> > 
> > ->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
> 
> This doesn't look right.

Oops! How did that happen :/

> 
> > +obj-$(CONFIG_LPC_CTRL)		+= lpc-ctrl.o
> > diff --git a/drivers/misc/lpc-ctrl.c b/drivers/misc/lpc-ctrl.c
> > new file mode 100644
> > index 0000000..f72f44f
> > --- /dev/null
> > +++ b/drivers/misc/lpc-ctrl.c
> > @@ -0,0 +1,251 @@
> > +/*
> > + * 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/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/errno.h>
> > +#include <linux/poll.h>
> > +#include <linux/sched.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/slab.h>
> > +#include <linux/init.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/delay.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/timer.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/lpc-ctrl.h>
> > +
> > > +#define DEVICE_NAME	"lpc-ctrl"
> > 
> > +
> > +#define LPC_HICR7 0x88
> > +#define LPC_HICR8 0x8c
> > +
> > +struct lpc_ctrl {
> > > > +	struct miscdevice	miscdev;
> > > > +	void __iomem		*ctrl;
> > > > +	phys_addr_t			base;
> > > > +	resource_size_t		size;
> > 
> > +};
> > +
> > +
> > +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)
> > +{
> > > +	return 0;
> > 
> > +}
> > +
> > +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;
> > 
> > +
> > > +	WARN_ON(*ppos);
> > 
> > +
> > > +	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;
> > 
> > +
> > > +	WARN_ON(*ppos);
> > 
> > +
> > > +	return -EPERM;
> > 
> > +}
> > +
> > +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
> > > +		unsigned long param)
> > 
> > +{
> > > +	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;
> > 
> > +
> > > +		iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
> > > +				lpc_ctrl->ctrl + LPC_HICR7);
> > > +		iowrite32((~(map.size - 1)) | ((map.size >> 16)
> > > - 1),
> > 
> > +				lpc_ctrl->ctrl + LPC_HICR8);
> 
> I think these operations could do with some explanation.

Agreed. I'll see if I can english a simple comment.

> 
> > +		return 0;
> > > +	case LPC_CTRL_IOCTL_UNMAP:
> > > +		if (copy_from_user(&map, p, sizeof(map)))
> > 
> > +			return -EFAULT;
> 
> Is this required? We don't use map below.

Correct - I've changed the ioctl() to not take map

> 
> > +
> > > +		iowrite32((0x3000 << 16) | (0x0e00),
> > > +				lpc_ctrl->ctrl + LPC_HICR7);
> > > +		iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) -
> > > 1),
> > 
> > +				lpc_ctrl->ctrl + LPC_HICR8);
> 
> Can you comment/#define the magic numbers? I'm not across why we use
> these values.

So these actually go away thanks to Cedric pointing out that I can
quite easily ask the mtd layer for PNOR information. I haven't as yet
found 0x3000 but I suspect I can find that too.

Anyway, this is pointing the LPC bus back at the actual flash. The
flash is at address 0x30000000 on the BMC (the 0x3000 represents that)
and the SBE expects it to be mapped down from 0x0fffffff so for a flash
of size 0x02000000 (HICR8 uses this...) then 0x0e000000 (the other
nibble of HICR7) is where it should be on the host. Anyhow, not that
important this will get replaced by asking the MTD layer which will be
less hardcoded.
> 
> > +		return 0;
> > > +	}
> > 
> > +
> > > +	return -EINVAL;
> > 
> > +}
> > +
> > +static int lpc_ctrl_release(struct inode *inode, struct file
> > *file)
> > +{
> > > +	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 *res;
> > > +	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;
> > 
> > +
> > > +	dev_set_drvdata(&pdev->dev, lpc_ctrl);
> > 
> > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res) {
> > > +		dev_err(dev, "Unable to find resources\n");
> > > +		rc = -ENXIO;
> > > +		goto out_free;
> > > +	}
> > 
> > +
> > +	/* Todo unmap this on fail cases and exit */
> 
> You're using devm_*, which if I understand correctly should handle
> this
> for you.

Yes, I believe you're right, I wasn't sure when I wrote this... should
be fine.

I'll address all of this. Thanks for the review,

Cyril

> 
> > +	lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
> > > +	if (!lpc_ctrl->ctrl) {
> > > +		rc = -ENOMEM;
> > > +		goto out_free;
> > > +	}
> > 
> > +
> > > +	node = of_parse_phandle(dev->of_node, "memory-region",
> > > 0);
> > > +	if (!node) {
> > > +		/*
> > > +		 * Should probaby handle this by allocating 4-
> > > 64k now and
> > > +		 * using that
> > > +		 */
> > > +		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->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");
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > > +			lpc_ctrl->base, lpc_ctrl->size);
> > > +	return 0;
> > 
> > +
> > +out:
> > > +	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
> > 
> > +out_free:
> > +	devm_kfree(dev, lpc_ctrl);
> 
> You shouldn't have to worry about these devm_*() calls.
> 
> > +	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);
> > > +	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
> > 
> > +	devm_kfree(&pdev->dev, lpc_ctrl);
> 
> As above.
> 
> Andrew
> 
> > +	lpc_ctrl = NULL;
> > +
> > > +	return 0;
> > 
> > +}
> > +
> > +static const struct of_device_id lpc_ctrl_match[] = {
> > > +	{ .compatible = "aspeed,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/lpc-ctrl.h
> > b/include/uapi/linux/lpc-ctrl.h
> > new file mode 100644
> > index 0000000..c5f1caf
> > --- /dev/null
> > +++ b/include/uapi/linux/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 */

Re: [PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Andrew Jeffery]

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

On Thu, 2016-12-15 at 18:59 +1100, Cyril Bur wrote:
> > > +config MBOX_HOST
> > > +       depends on ARCH_ASPEED
> > 
> > or COMPILE_TEST
> 
> Is both a good solution?

I think Joel meant "|| COMPILE_TEST"

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