[PATCH v2 0/5] LPC/MBOX work

[PATCH v2 0/5] LPC/MBOX work

[Cyril Bur]

Hi all,

Sending this v2 a little prematurely but it will be good to have some eyes on it.

I haven't been able to boot the host with this as there are quite a few caveats.
First of all hostboot expects the flash at a very specific place on the host LPC
bus per platform. This creates problems when a machine of a certain type has a
flash chip of a different size as to what it should have. This renders the UNMAP
ioctl() somewhat useless. We should be able to compensate for this in userspace,
which is good since I don't want any of these caveats worked around in the kernel.

I have had some troubles with regmap_write() but it is possible the bug has been
fixed. I will test in the coming days and confirm.

I have attempted to address all the review comments:
 - Use of regmap
 - Open counts to avoid opening by different processes
 - Style fixes
 - Move mbox to drivers/mailbox
 - Extra descriptions

Notable exception that I haven't gotten around to but will happily do once we
get to a working state is preventing mutiple threads from writing mbox registers
at once

Thanks,

Cyril 

Cyril Bur (5):
  ARM: dts: aspeed: Reserve BMC ram for host to BMC communication
  ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap
  ARM: dts: aspeed: Move mbox under lpc_host node
  drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
  drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

 arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts  |   6 +
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts  |   7 +
 arch/arm/boot/dts/aspeed-g4.dtsi               |  36 +++
 drivers/mailbox/Kconfig                        |   9 +
 drivers/mailbox/Makefile                       |   3 +
 drivers/mailbox/aspeed-mbox.c                  | 354 +++++++++++++++++++++++++
 drivers/misc/Kconfig                           |   9 +
 drivers/misc/Makefile                          |   1 +
 drivers/misc/aspeed-lpc-ctrl.c                 | 292 ++++++++++++++++++++
 include/uapi/linux/aspeed-lpc-ctrl.h           |  25 ++
 include/uapi/linux/aspeed-mbox.h               |  23 ++
 13 files changed, 777 insertions(+)
 create mode 100644 drivers/mailbox/aspeed-mbox.c
 create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
 create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
 create mode 100644 include/uapi/linux/aspeed-mbox.h

-- 
2.11.0

[PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication

[Cyril Bur]

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

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
index 13e9f18fbed8..b60044800ea1 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
@@ -29,6 +29,18 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
+	};
+
+	flash_buffer@1e789000 {
+		compatible = "aspeed,ast2400-lpc-ctrl";
+		memory-region = <&flash_memory>;
+		reg = <0x1e789000 0x8>;
 	};
 
         leds {
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 7c78eb1c01ff..1ed2a9ba7ab8 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,ast2400-mbox";
+				reg = <0x1e789200 0x5c>;
+				interrupts = <46>;
+			};
+
 			wdt1: wdt@1e785000 {
 				compatible = "aspeed,ast2400-wdt";
 				reg = <0x1e785000 0x1c>;
-- 
2.11.0

Re: [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication

[Andrew Jeffery]

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

Hi Cyril,

On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 12 ++++++++++++
>  arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> index 13e9f18fbed8..b60044800ea1 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> @@ -29,6 +29,18 @@
> >  			no-map;
> >  			reg = <0x5f000000 0x01000000>; /* 16MB */
> >  		};
> +
> > > +		flash_memory: region@54000000 {
> > +			compatible = "aspeed,ast2400-lpc-ctrl";
> > +			no-map;
> > +			reg = <0x54000000 0x04000000>; /* 64M */
> > +		};
> > +	};
> +
> > +	flash_buffer@1e789000 {
> > +		compatible = "aspeed,ast2400-lpc-ctrl";
> > +		memory-region = <&flash_memory>;
> > +		reg = <0x1e789000 0x8>;
>  	};

Save this for addition in 2/5?

>  
>          leds {
> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
> index 7c78eb1c01ff..1ed2a9ba7ab8 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,ast2400-mbox";
> > +				reg = <0x1e789200 0x5c>;
> > +				interrupts = <46>;
> +			};

Save this for addition in 3/5?

Or maybe just squash them all together?

Andrew

> +
> > >  			wdt1: wdt@1e785000 {
> >  				compatible = "aspeed,ast2400-wdt";
> >  				reg = <0x1e785000 0x1c>;

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

Re: [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication

[Cyril Bur]

On Fri, 2016-12-23 at 11:31 +1030, Andrew Jeffery wrote:
> Hi Cyril,
> 
> On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > 
> > ---
> >  arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 12 ++++++++++++
> >  arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > index 13e9f18fbed8..b60044800ea1 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> > @@ -29,6 +29,18 @@
> > >  			no-map;
> > >  			reg = <0x5f000000 0x01000000>; /* 16MB */
> > >  		};
> > 
> > +
> > > > +		flash_memory: region@54000000 {
> > > 
> > > +			compatible = "aspeed,ast2400-lpc-ctrl";
> > > +			no-map;
> > > +			reg = <0x54000000 0x04000000>; /* 64M */
> > > +		};
> > > +	};
> > 
> > +
> > > +	flash_buffer@1e789000 {
> > > +		compatible = "aspeed,ast2400-lpc-ctrl";
> > > +		memory-region = <&flash_memory>;
> > > +		reg = <0x1e789000 0x8>;
> > 
> >  	};
> 
> Save this for addition in 2/5?
> 
> >  
> >          leds {
> > diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
> > index 7c78eb1c01ff..1ed2a9ba7ab8 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,ast2400-mbox";
> > > +				reg = <0x1e789200 0x5c>;
> > > +				interrupts = <46>;
> > 
> > +			};
> 
> Save this for addition in 3/5?
> 
> Or maybe just squash them all together?
> 

Yeah I'm not sure these three commits make sense - it might just be
easier to have 1 dts changes commit.

Thanks,

Cyril

> Andrew
> 
> > +
> > > >  			wdt1: wdt@1e785000 {
> > > 
> > >  				compatible = "aspeed,ast2400-wdt";
> > >  				reg = <0x1e785000 0x1c>;

[PATCH v2 2/5] ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap

[Cyril Bur]

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

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts b/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts
index 9dc3e67fc98c..19aeb3eb4a0a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts
@@ -29,6 +29,12 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
 	ahb {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts b/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts
index 68946aa34e31..a99fef94a616 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts
@@ -33,6 +33,12 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
 	leds {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts b/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts
index 639b8f877184..12b1cbcdf9d6 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts
@@ -29,6 +29,12 @@
 			no-map;
 			reg = <0x5f000000 0x01000000>; /* 16MB */
 		};
+	
+		flash_memory: region@54000000 {
+			compatible = "aspeed,ast2400-lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64M */
+		};
 	};
 
 	ahb {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
index b60044800ea1..bba911d79fbf 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
@@ -37,11 +37,6 @@
 		};
 	};
 
-	flash_buffer@1e789000 {
-		compatible = "aspeed,ast2400-lpc-ctrl";
-		memory-region = <&flash_memory>;
-		reg = <0x1e789000 0x8>;
-	};
 
         leds {
                 compatible = "gpio-leds";
diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 1ed2a9ba7ab8..2db7915de790 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -33,6 +33,36 @@
 		#size-cells = <1>;
 		ranges;
 
+		lpc: lpc@1e789000 {
+			compatible = "aspeed,ast2400-lpc", "simple-mfd";
+			reg = <0x1e789000 0x1000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0x1e789000 0x1000>;
+
+			lpc_bmc: lpc-bmc@0 {
+				compatible = "aspeed,ast2400-lpc-bmc";
+				reg = <0x0 0x80>;
+			};
+
+			lpc_host: lpc-host@80 {
+				compatible = "aspeed,ast2400-lpc-host", "simple-mfd", "syscon";
+				reg = <0x80 0x1e0>;
+				reg-io-width = <4>;
+	
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0x0 0x80 0x1e0>;
+
+				lpc-ctrl@0 {
+					compatible = "aspeed,ast2400-lpc-ctrl";
+					memory-region = <&flash_memory>;
+					reg = <0x0 0x80>;
+				};
+			};
+		};
+
 		vic: interrupt-controller@1e6c0080 {
 			compatible = "aspeed,ast2400-vic";
 			interrupt-controller;
-- 
2.11.0

[PATCH v2 3/5] ARM: dts: aspeed: Move mbox under lpc_host node

[Cyril Bur]

The mbox registers are on the lpc_host bus this should be reflected in the device tree

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/arm/boot/dts/aspeed-g4.dtsi | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 2db7915de790..864136d2b73c 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -60,6 +60,12 @@
 					memory-region = <&flash_memory>;
 					reg = <0x0 0x80>;
 				};
+
+				mbox: mbox@180 {
+					compatible = "aspeed,ast2400-mbox";
+					reg = <0x180 0x5c>;
+					interrupts = <46>;
+				};
 			};
 		};
 
@@ -900,12 +906,6 @@
 				interrupts = <8>;
 			};
 
-			mbox: mbox@1e789200 {
-				compatible = "aspeed,ast2400-mbox";
-				reg = <0x1e789200 0x5c>;
-				interrupts = <46>;
-			};
-
 			wdt1: wdt@1e785000 {
 				compatible = "aspeed,ast2400-wdt";
 				reg = <0x1e785000 0x1c>;
-- 
2.11.0

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

[Cyril Bur]

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

An ioctl() is used to allow users to write to a specific register with a specific
value. This is useful as the other end have configured the mbox registers to
provide an interrupt when a specific regster gets written to.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 drivers/mailbox/Kconfig          |   9 +
 drivers/mailbox/Makefile         |   3 +
 drivers/mailbox/aspeed-mbox.c    | 354 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/aspeed-mbox.h |  23 +++
 4 files changed, 389 insertions(+)
 create mode 100644 drivers/mailbox/aspeed-mbox.c
 create mode 100644 include/uapi/linux/aspeed-mbox.h

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 5305923752d2..815cfe852dea 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOX
 	  It is used to send short messages between ARM64-bit cores and
 	  the SLIMpro Management Engine, primarily for PM. Say Y here if you
 	  want to use the APM X-Gene SLIMpro IPCM support.
+
+config ASPEED_MBOX
+	depends on ARCH_ASPEED || COMPILE_TEST
+	bool "Aspeed ast2400/2500 Mailbox Controller"
+	default "y"
+	---help---
+	  Provides a driver for the MBOX regsiters found on Aspeed SOCs
+	  (AST2400 and AST2500). This driver provides a device for aspeed
+	  mbox registers
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 0be3e742bb7d..99d17f3b9552 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
 obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
 
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
+
+obj-$(CONFIG_ASPEED_MBOX)		+= aspeed-mbox.o
+
diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
new file mode 100644
index 000000000000..a5da7fc0fd23
--- /dev/null
+++ b/drivers/mailbox/aspeed-mbox.c
@@ -0,0 +1,354 @@
+/*
+ * 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/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/aspeed-mbox.h>
+
+#define DEVICE_NAME	"aspeed-mbox"
+
+#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 BIT(7)
+#define   MBOX_CTRL_MASK BIT(1)
+#define   MBOX_CTRL_SEND BIT(0)
+#define MBOX_HOST_CTRL 0x4c
+#define MBOX_INTERRUPT_0 0x50
+#define MBOX_INTERRUPT_1 0x54
+
+struct mbox {
+	struct miscdevice	miscdev;
+	struct regmap		*regmap;
+	unsigned int		base;
+	int			irq;
+	wait_queue_head_t	queue;
+	struct timer_list	poll_timer;
+};
+
+static atomic_t mbox_open_count = ATOMIC_INIT(0);
+
+static u8 mbox_inb(struct mbox *mbox, int reg)
+{
+	/*
+	 * The mbox registers are actually only 1 byte but are addressed 4
+	 * bytes appart. The other three bytes are marked 'reserved', they
+	 * *should* be zero but lets not rely on it.
+	 * I am going to rely on the fact we can casually read/write to them...
+	 */
+	unsigned int val = 0xff; /* If regmap throws an error return 0xff */
+	regmap_read(mbox->regmap, mbox->base + reg, &val);
+	return val & 0xff;
+}
+
+static void mbox_outb(struct mbox *mbox, u8 data, int reg)
+{
+	regmap_write(mbox->regmap, mbox->base + reg, data);
+}
+
+static struct mbox *file_mbox(struct file *file)
+{
+	return container_of(file->private_data, struct mbox, miscdev);
+}
+
+static int mbox_open(struct inode *inode, struct file *file)
+{
+	struct mbox *mbox = file_mbox(file);
+
+	if (atomic_inc_return(&mbox_open_count) == 1) {
+		/*
+		 * Clear the interrupt status bit if it was left on and unmask
+		 * interrupts.
+		 * MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
+		 */
+		mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+		return 0;
+	}
+
+	atomic_dec(&mbox_open_count);
+	return -EBUSY;
+}
+
+static ssize_t mbox_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct mbox *mbox = file_mbox(file);
+	char __user *p = buf;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	WARN_ON(*ppos);
+
+	if (wait_event_interruptible(mbox->queue,
+		mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
+		return -ERESTARTSYS;
+
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
+		if (__put_user(mbox_inb(mbox, MBOX_DATA_0 + (i * 4)), p++))
+			return -EFAULT;
+
+	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
+	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return p - buf;
+}
+
+static ssize_t mbox_write(struct file *file, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct mbox *mbox = file_mbox(file);
+	const char __user *p = buf;
+	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, c, MBOX_DATA_0 + (i * 4));
+		p++;
+	}
+
+	mbox_outb(mbox, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
+
+	return p - buf;
+}
+
+static long mbox_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	struct aspeed_mbox_atn atn;
+	struct mbox *mbox = file_mbox(file);
+	void __user *p = (void __user *)param;
+
+	switch (cmd) {
+	case ASPEED_MBOX_IOCTL_ATN:
+		if (copy_from_user(&atn, p, sizeof(atn)))
+			return -EFAULT;
+		if (atn.reg > 15)
+			return -EINVAL;
+
+		mbox_outb(mbox, atn.val, atn.reg);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static unsigned int mbox_poll(struct file *file, poll_table *wait)
+{
+	struct mbox *mbox = file_mbox(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &mbox->queue, wait);
+
+	if (mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
+		mask |= POLLIN;
+
+	return mask;
+}
+
+static int mbox_release(struct inode *inode, struct file *file)
+{
+	atomic_dec(&mbox_open_count);
+	return 0;
+}
+
+static const struct file_operations mbox_fops = {
+	.owner		= THIS_MODULE,
+	.open		= mbox_open,
+	.read		= mbox_read,
+	.write		= mbox_write,
+	.release	= mbox_release,
+	.poll		= mbox_poll,
+	.unlocked_ioctl	= mbox_ioctl,
+};
+
+static void poll_timer(unsigned long data)
+{
+	struct mbox *mbox = (void *)data;
+
+	mbox->poll_timer.expires += msecs_to_jiffies(500);
+	wake_up(&mbox->queue);
+	add_timer(&mbox->poll_timer);
+}
+
+static irqreturn_t mbox_irq(int irq, void *arg)
+{
+	struct mbox *mbox = arg;
+
+	if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
+		return IRQ_NONE;
+
+	/*
+	 * Leave the status bit set so that we know the data is for us,
+	 * clear it once it has been read.
+	 */
+
+	/* Mask it off, we'll clear it when we the data gets read */
+	mbox_outb(mbox, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
+
+	wake_up(&mbox->queue);
+	return IRQ_HANDLED;
+}
+
+static int mbox_config_irq(struct mbox *mbox,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	mbox->irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!mbox->irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, mbox->irq, mbox_irq, IRQF_SHARED,
+			DEVICE_NAME, mbox);
+	if (rc < 0) {
+		dev_warn(dev, "Unable to request IRQ %d\n", mbox->irq);
+		mbox->irq = 0;
+		return rc;
+	}
+
+	/*
+	 * Disable all register based interrupts, we'll have to change
+	 * this, protocol seemingly will require regs 0 and 15
+	 */
+	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
+	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
+
+	/* W1C */
+	mbox_outb(mbox, 0xff, MBOX_STATUS_0);
+	mbox_outb(mbox, 0xff, MBOX_STATUS_1);
+
+	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return 0;
+}
+
+static int mbox_probe(struct platform_device *pdev)
+{
+	struct mbox *mbox;
+	struct device *dev;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+	dev_info(dev, "Found mbox host device\n");
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, mbox);
+
+	rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
+	if (rc) {
+		dev_err(dev, "Couldn't read reg device-tree property\n");
+		goto out;
+	}
+
+	mbox->regmap = syscon_node_to_regmap(
+			pdev->dev.parent->of_node);
+	if (IS_ERR(mbox->regmap)) {
+		dev_err(dev, "Couldn't get regmap\n");
+		rc = -ENODEV;
+		goto out;
+	}
+
+	init_waitqueue_head(&mbox->queue);
+
+	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
+	mbox->miscdev.name = DEVICE_NAME;
+	mbox->miscdev.fops = &mbox_fops;
+	mbox->miscdev.parent = dev;
+	rc = misc_register(&mbox->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register device\n");
+		goto out;
+	}
+
+	mbox_config_irq(mbox, pdev);
+
+	if (mbox->irq) {
+		dev_info(dev, "Using IRQ %d\n", mbox->irq);
+	} else {
+		dev_info(dev, "No IRQ; using timer\n");
+		setup_timer(&mbox->poll_timer, poll_timer,
+				(unsigned long)mbox);
+		mbox->poll_timer.expires = jiffies + msecs_to_jiffies(10);
+		add_timer(&mbox->poll_timer);
+	}
+
+out:
+	return rc;
+
+}
+
+static int mbox_remove(struct platform_device *pdev)
+{
+	struct mbox *mbox = dev_get_drvdata(&pdev->dev);
+
+	misc_deregister(&mbox->miscdev);
+	if (!mbox->irq)
+		del_timer_sync(&mbox->poll_timer);
+	mbox = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id mbox_match[] = {
+	{ .compatible = "aspeed,ast2400-mbox" },
+	{ },
+};
+
+static struct platform_driver mbox_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = mbox_match,
+	},
+	.probe = mbox_probe,
+	.remove = mbox_remove,
+};
+
+module_platform_driver(mbox_driver);
+
+MODULE_DEVICE_TABLE(of, mbox_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
+MODULE_DESCRIPTION("Linux device interface to the Aspeed MBOX registers");
diff --git a/include/uapi/linux/aspeed-mbox.h b/include/uapi/linux/aspeed-mbox.h
new file mode 100644
index 000000000000..f4e6a9242fd0
--- /dev/null
+++ b/include/uapi/linux/aspeed-mbox.h
@@ -0,0 +1,23 @@
+/*
+ * 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>
+
+struct aspeed_mbox_atn {
+	uint8_t reg;
+	uint8_t val;
+};
+
+#define __ASPEED_MBOX_IOCTL_MAGIC	0xb1
+#define ASPEED_MBOX_IOCTL_ATN	_IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)
+
+#endif /* _UAPI_LINUX_MBOX_HOST_H */
-- 
2.11.0

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

[Andrew Jeffery]

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

Hi Cyril,

This looks good to me. A couple of minor comments below.

On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> This provides access to the mbox registers on the ast2400 and ast2500 boards.
> 
> An ioctl() is used to allow users to write to a specific register with a specific
> value. This is useful as the other end have configured the mbox registers to
> provide an interrupt when a specific regster gets written to.
> 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
>  drivers/mailbox/Kconfig          |   9 +
>  drivers/mailbox/Makefile         |   3 +
>  drivers/mailbox/aspeed-mbox.c    | 354 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/aspeed-mbox.h |  23 +++
>  4 files changed, 389 insertions(+)
>  create mode 100644 drivers/mailbox/aspeed-mbox.c
>  create mode 100644 include/uapi/linux/aspeed-mbox.h
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 5305923752d2..815cfe852dea 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOX
> >  	  It is used to send short messages between ARM64-bit cores and
> >  	  the SLIMpro Management Engine, primarily for PM. Say Y here if you
> >  	  want to use the APM X-Gene SLIMpro IPCM support.
> +
> +config ASPEED_MBOX
> > +	depends on ARCH_ASPEED || COMPILE_TEST
> > +	bool "Aspeed ast2400/2500 Mailbox Controller"
> > +	default "y"
> > +	---help---
> > +	  Provides a driver for the MBOX regsiters found on Aspeed SOCs
> > +	  (AST2400 and AST2500). This driver provides a device for aspeed
> > +	  mbox registers
>  endif
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 0be3e742bb7d..99d17f3b9552 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>  obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>  
> >  obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
> +
> > +obj-$(CONFIG_ASPEED_MBOX)		+= aspeed-mbox.o
> +
> diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> new file mode 100644
> index 000000000000..a5da7fc0fd23
> --- /dev/null
> +++ b/drivers/mailbox/aspeed-mbox.c
> @@ -0,0 +1,354 @@
> +/*
> + * 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/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/aspeed-mbox.h>
> +
> > +#define DEVICE_NAME	"aspeed-mbox"
> +
> +#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 BIT(7)
> +#define   MBOX_CTRL_MASK BIT(1)
> +#define   MBOX_CTRL_SEND BIT(0)
> +#define MBOX_HOST_CTRL 0x4c
> +#define MBOX_INTERRUPT_0 0x50
> +#define MBOX_INTERRUPT_1 0x54
> +
> +struct mbox {
> > > +	struct miscdevice	miscdev;
> > > +	struct regmap		*regmap;
> > > +	unsigned int		base;
> > > +	int			irq;
> > > +	wait_queue_head_t	queue;
> > > +	struct timer_list	poll_timer;
> +};
> +
> +static atomic_t mbox_open_count = ATOMIC_INIT(0);
> +
> +static u8 mbox_inb(struct mbox *mbox, int reg)
> +{
> > +	/*
> > +	 * The mbox registers are actually only 1 byte but are addressed 4
> > +	 * bytes appart. The other three bytes are marked 'reserved', they
> > +	 * *should* be zero but lets not rely on it.
> > +	 * I am going to rely on the fact we can casually read/write to them...
> > +	 */
> > +	unsigned int val = 0xff; /* If regmap throws an error return 0xff */
> > +	regmap_read(mbox->regmap, mbox->base + reg, &val);
> +	return val & 0xff;
> +}
> +
> +static void mbox_outb(struct mbox *mbox, u8 data, int reg)
> +{
> +	regmap_write(mbox->regmap, mbox->base + reg, data);

We could use regmap_update_bits() here, but ultimately it's not going
to matter. Should we say something if regmap_write() fails (it
shouldn't, but if it does that's extra concerning)?

> +}
> +
> +static struct mbox *file_mbox(struct file *file)
> +{
> > +	return container_of(file->private_data, struct mbox, miscdev);
> +}
> +
> +static int mbox_open(struct inode *inode, struct file *file)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> +
> > +	if (atomic_inc_return(&mbox_open_count) == 1) {
> > +		/*
> > +		 * Clear the interrupt status bit if it was left on and unmask
> > +		 * interrupts.
> > +		 * MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
> > +		 */
> > +		mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +		return 0;
> > +	}
> +
> > +	atomic_dec(&mbox_open_count);
> > +	return -EBUSY;
> +}
> +
> +static ssize_t mbox_read(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> > +	char __user *p = buf;
> > +	int i;
> +
> > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > +		return -EFAULT;
> +
> > +	WARN_ON(*ppos);
> +
> > +	if (wait_event_interruptible(mbox->queue,
> > +		mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > +		return -ERESTARTSYS;
> +
> > +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
> > +		if (__put_user(mbox_inb(mbox, MBOX_DATA_0 + (i * 4)), p++))
> > +			return -EFAULT;
> +
> > +	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> > +	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +	return p - buf;
> +}
> +
> +static ssize_t mbox_write(struct file *file, const char __user *buf,
> > +				size_t count, loff_t *ppos)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> > +	const char __user *p = buf;
> > +	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, c, MBOX_DATA_0 + (i * 4));
> > +		p++;
> > +	}
> +
> > +	mbox_outb(mbox, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
> +
> > +	return p - buf;
> +}
> +
> +static long mbox_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long param)
> +{
> > +	struct aspeed_mbox_atn atn;
> > +	struct mbox *mbox = file_mbox(file);
> > +	void __user *p = (void __user *)param;
> +
> > +	switch (cmd) {
> +	case ASPEED_MBOX_IOCTL_ATN:

I think we should rename the IOCTL as what we do below doesn't
necessarily raise an interrupt.

> +		if (copy_from_user(&atn, p, sizeof(atn)))
> > +			return -EFAULT;
> > +		if (atn.reg > 15)
> > +			return -EINVAL;
> +
> > +		mbox_outb(mbox, atn.val, atn.reg);
> > +		return 0;
> > +	}
> +
> > +	return -EINVAL;
> +}
> +
> +static unsigned int mbox_poll(struct file *file, poll_table *wait)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> > +	unsigned int mask = 0;
> +
> > +	poll_wait(file, &mbox->queue, wait);
> +
> > +	if (mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> > +		mask |= POLLIN;
> +
> > +	return mask;
> +}
> +
> +static int mbox_release(struct inode *inode, struct file *file)
> +{
> > +	atomic_dec(&mbox_open_count);
> > +	return 0;
> +}
> +
> +static const struct file_operations mbox_fops = {
> > > +	.owner		= THIS_MODULE,
> > > +	.open		= mbox_open,
> > > +	.read		= mbox_read,
> > > +	.write		= mbox_write,
> > > +	.release	= mbox_release,
> > > +	.poll		= mbox_poll,
> > > +	.unlocked_ioctl	= mbox_ioctl,
> +};
> +
> +static void poll_timer(unsigned long data)
> +{
> > +	struct mbox *mbox = (void *)data;
> +
> > +	mbox->poll_timer.expires += msecs_to_jiffies(500);
> > +	wake_up(&mbox->queue);
> > +	add_timer(&mbox->poll_timer);
> +}
> +
> +static irqreturn_t mbox_irq(int irq, void *arg)
> +{
> > +	struct mbox *mbox = arg;
> +
> > +	if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > +		return IRQ_NONE;
> +
> > +	/*
> > +	 * Leave the status bit set so that we know the data is for us,
> > +	 * clear it once it has been read.
> > +	 */
> +
> > +	/* Mask it off, we'll clear it when we the data gets read */
> > +	mbox_outb(mbox, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
> +
> > +	wake_up(&mbox->queue);
> > +	return IRQ_HANDLED;
> +}
> +
> +static int mbox_config_irq(struct mbox *mbox,
> > +		struct platform_device *pdev)
> +{
> > +	struct device *dev = &pdev->dev;
> > +	int rc;
> +
> > +	mbox->irq = irq_of_parse_and_map(dev->of_node, 0);
> > +	if (!mbox->irq)
> > +		return -ENODEV;
> +
> > +	rc = devm_request_irq(dev, mbox->irq, mbox_irq, IRQF_SHARED,
> > +			DEVICE_NAME, mbox);
> > +	if (rc < 0) {
> > +		dev_warn(dev, "Unable to request IRQ %d\n", mbox->irq);
> > +		mbox->irq = 0;
> > +		return rc;
> > +	}
> +
> > +	/*
> > +	 * Disable all register based interrupts, we'll have to change
> > +	 * this, protocol seemingly will require regs 0 and 15
> > +	 */
> > +	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
> > +	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
> +
> > +	/* W1C */
> > +	mbox_outb(mbox, 0xff, MBOX_STATUS_0);
> > +	mbox_outb(mbox, 0xff, MBOX_STATUS_1);
> +
> > +	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +	return 0;
> +}
> +
> +static int mbox_probe(struct platform_device *pdev)
> +{
> > +	struct mbox *mbox;
> > +	struct device *dev;
> > +	int rc;
> +
> > +	if (!pdev || !pdev->dev.of_node)
> > +		return -ENODEV;
> +
> > +	dev = &pdev->dev;
> > +	dev_info(dev, "Found mbox host device\n");
> +
> > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +	if (!mbox)
> > +		return -ENOMEM;
> +
> > +	dev_set_drvdata(&pdev->dev, mbox);
> +
> > +	rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
> > +	if (rc) {
> > +		dev_err(dev, "Couldn't read reg device-tree property\n");
> > +		goto out;
> > +	}
> +
> > +	mbox->regmap = syscon_node_to_regmap(
> > +			pdev->dev.parent->of_node);
> > +	if (IS_ERR(mbox->regmap)) {
> > +		dev_err(dev, "Couldn't get regmap\n");
> > +		rc = -ENODEV;
> > +		goto out;
> > +	}
> +
> > +	init_waitqueue_head(&mbox->queue);
> +
> > +	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	mbox->miscdev.name = DEVICE_NAME;
> > +	mbox->miscdev.fops = &mbox_fops;
> > +	mbox->miscdev.parent = dev;
> > +	rc = misc_register(&mbox->miscdev);
> > +	if (rc) {
> > +		dev_err(dev, "Unable to register device\n");
> > +		goto out;
> > +	}
> +
> > +	mbox_config_irq(mbox, pdev);
> +
> > +	if (mbox->irq) {
> > +		dev_info(dev, "Using IRQ %d\n", mbox->irq);
> > +	} else {
> > +		dev_info(dev, "No IRQ; using timer\n");
> > +		setup_timer(&mbox->poll_timer, poll_timer,
> > +				(unsigned long)mbox);
> > +		mbox->poll_timer.expires = jiffies + msecs_to_jiffies(10);
> > +		add_timer(&mbox->poll_timer);
> > +	}
> +
> +out:
> > +	return rc;
> +
> +}
> +
> +static int mbox_remove(struct platform_device *pdev)
> +{
> > +	struct mbox *mbox = dev_get_drvdata(&pdev->dev);
> +
> > +	misc_deregister(&mbox->miscdev);
> > +	if (!mbox->irq)
> > +		del_timer_sync(&mbox->poll_timer);
> > +	mbox = NULL;
> +
> > +	return 0;
> +}
> +
> +static const struct of_device_id mbox_match[] = {
> > +	{ .compatible = "aspeed,ast2400-mbox" },
> > +	{ },
> +};
> +
> +static struct platform_driver mbox_driver = {
> > +	.driver = {
> > > +		.name		= DEVICE_NAME,
> > +		.of_match_table = mbox_match,
> > +	},
> > +	.probe = mbox_probe,
> > +	.remove = mbox_remove,
> +};
> +
> +module_platform_driver(mbox_driver);
> +
> +MODULE_DEVICE_TABLE(of, mbox_match);
> +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Linux device interface to the Aspeed MBOX registers");
> diff --git a/include/uapi/linux/aspeed-mbox.h b/include/uapi/linux/aspeed-mbox.h
> new file mode 100644
> index 000000000000..f4e6a9242fd0
> --- /dev/null
> +++ b/include/uapi/linux/aspeed-mbox.h
> @@ -0,0 +1,23 @@
> +/*
> + * 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>
> +
> +struct aspeed_mbox_atn {
> > +	uint8_t reg;
> > +	uint8_t val;
> +};
> +
> > +#define __ASPEED_MBOX_IOCTL_MAGIC	0xb1
> > +#define ASPEED_MBOX_IOCTL_ATN	_IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)
> +
> +#endif /* _UAPI_LINUX_MBOX_HOST_H */

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

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

[Cyril Bur]

On Fri, 2016-12-23 at 13:12 +1030, Andrew Jeffery wrote:
> Hi Cyril,
> 
> This looks good to me. A couple of minor comments below.
> 
> On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > This provides access to the mbox registers on the ast2400 and ast2500 boards.
> > 
> > An ioctl() is used to allow users to write to a specific register with a specific
> > value. This is useful as the other end have configured the mbox registers to
> > provide an interrupt when a specific regster gets written to.
> > 
> > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > 
> > ---
> >  drivers/mailbox/Kconfig          |   9 +
> >  drivers/mailbox/Makefile         |   3 +
> >  drivers/mailbox/aspeed-mbox.c    | 354 +++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/aspeed-mbox.h |  23 +++
> >  4 files changed, 389 insertions(+)
> >  create mode 100644 drivers/mailbox/aspeed-mbox.c
> >  create mode 100644 include/uapi/linux/aspeed-mbox.h
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index 5305923752d2..815cfe852dea 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOX
> > >  	  It is used to send short messages between ARM64-bit cores and
> > >  	  the SLIMpro Management Engine, primarily for PM. Say Y here if you
> > >  	  want to use the APM X-Gene SLIMpro IPCM support.
> > 
> > +
> > +config ASPEED_MBOX
> > > +	depends on ARCH_ASPEED || COMPILE_TEST
> > > +	bool "Aspeed ast2400/2500 Mailbox Controller"
> > > +	default "y"
> > > +	---help---
> > > +	  Provides a driver for the MBOX regsiters found on Aspeed SOCs
> > > +	  (AST2400 and AST2500). This driver provides a device for aspeed
> > > +	  mbox registers
> > 
> >  endif
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index 0be3e742bb7d..99d17f3b9552 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
> >  obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
> >  
> > >  obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
> > 
> > +
> > > +obj-$(CONFIG_ASPEED_MBOX)		+= aspeed-mbox.o
> > 
> > +
> > diff --git a/drivers/mailbox/aspeed-mbox.c b/drivers/mailbox/aspeed-mbox.c
> > new file mode 100644
> > index 000000000000..a5da7fc0fd23
> > --- /dev/null
> > +++ b/drivers/mailbox/aspeed-mbox.c
> > @@ -0,0 +1,354 @@
> > +/*
> > + * 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/mfd/syscon.h>
> > +#include <linux/regmap.h>
> > +#include <linux/aspeed-mbox.h>
> > +
> > > +#define DEVICE_NAME	"aspeed-mbox"
> > 
> > +
> > +#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 BIT(7)
> > +#define   MBOX_CTRL_MASK BIT(1)
> > +#define   MBOX_CTRL_SEND BIT(0)
> > +#define MBOX_HOST_CTRL 0x4c
> > +#define MBOX_INTERRUPT_0 0x50
> > +#define MBOX_INTERRUPT_1 0x54
> > +
> > +struct mbox {
> > > > +	struct miscdevice	miscdev;
> > > > +	struct regmap		*regmap;
> > > > +	unsigned int		base;
> > > > +	int			irq;
> > > > +	wait_queue_head_t	queue;
> > > > +	struct timer_list	poll_timer;
> > 
> > +};
> > +
> > +static atomic_t mbox_open_count = ATOMIC_INIT(0);
> > +
> > +static u8 mbox_inb(struct mbox *mbox, int reg)
> > +{
> > > +	/*
> > > +	 * The mbox registers are actually only 1 byte but are addressed 4
> > > +	 * bytes appart. The other three bytes are marked 'reserved', they
> > > +	 * *should* be zero but lets not rely on it.
> > > +	 * I am going to rely on the fact we can casually read/write to them...
> > > +	 */
> > > +	unsigned int val = 0xff; /* If regmap throws an error return 0xff */
> > > +	regmap_read(mbox->regmap, mbox->base + reg, &val);
> > 
> > +	return val & 0xff;
> > +}
> > +
> > +static void mbox_outb(struct mbox *mbox, u8 data, int reg)
> > +{
> > +	regmap_write(mbox->regmap, mbox->base + reg, data);
> 
> We could use regmap_update_bits() here, but ultimately it's not going
> to matter. Should we say something if regmap_write() fails (it
> shouldn't, but if it does that's extra concerning)?

regmap_update_bits() to about touching the reserved section? Probs a
good idea.
Yeah I thought about putting a dev_err() in there. I'm not opposed, I
sort of though, we'll you never see them or when you do something
really bad has happened you'll probably notice everything broke...
Still in the incredibly unlikely event that a print proves useful I'm
happy to add it.

> 
> > +}
> > +
> > +static struct mbox *file_mbox(struct file *file)
> > +{
> > > +	return container_of(file->private_data, struct mbox, miscdev);
> > 
> > +}
> > +
> > +static int mbox_open(struct inode *inode, struct file *file)
> > +{
> > > +	struct mbox *mbox = file_mbox(file);
> > 
> > +
> > > +	if (atomic_inc_return(&mbox_open_count) == 1) {
> > > +		/*
> > > +		 * Clear the interrupt status bit if it was left on and unmask
> > > +		 * interrupts.
> > > +		 * MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step
> > > +		 */
> > > +		mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > > +		return 0;
> > > +	}
> > 
> > +
> > > +	atomic_dec(&mbox_open_count);
> > > +	return -EBUSY;
> > 
> > +}
> > +
> > +static ssize_t mbox_read(struct file *file, char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > 
> > +{
> > > +	struct mbox *mbox = file_mbox(file);
> > > +	char __user *p = buf;
> > > +	int i;
> > 
> > +
> > > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > > +		return -EFAULT;
> > 
> > +
> > > +	WARN_ON(*ppos);
> > 
> > +
> > > +	if (wait_event_interruptible(mbox->queue,
> > > +		mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > > +		return -ERESTARTSYS;
> > 
> > +
> > > +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
> > > +		if (__put_user(mbox_inb(mbox, MBOX_DATA_0 + (i * 4)), p++))
> > > +			return -EFAULT;
> > 
> > +
> > > +	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> > > +	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > > +	return p - buf;
> > 
> > +}
> > +
> > +static ssize_t mbox_write(struct file *file, const char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > 
> > +{
> > > +	struct mbox *mbox = file_mbox(file);
> > > +	const char __user *p = buf;
> > > +	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, c, MBOX_DATA_0 + (i * 4));
> > > +		p++;
> > > +	}
> > 
> > +
> > > +	mbox_outb(mbox, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
> > 
> > +
> > > +	return p - buf;
> > 
> > +}
> > +
> > +static long mbox_ioctl(struct file *file, unsigned int cmd,
> > > +		unsigned long param)
> > 
> > +{
> > > +	struct aspeed_mbox_atn atn;
> > > +	struct mbox *mbox = file_mbox(file);
> > > +	void __user *p = (void __user *)param;
> > 
> > +
> > > +	switch (cmd) {
> > 
> > +	case ASPEED_MBOX_IOCTL_ATN:
> 
> I think we should rename the IOCTL as what we do below doesn't
> necessarily raise an interrupt.
> 

Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?


Thanks,

Cyril

> > +		if (copy_from_user(&atn, p, sizeof(atn)))
> > > +			return -EFAULT;
> > > +		if (atn.reg > 15)
> > > +			return -EINVAL;
> > 
> > +
> > > +		mbox_outb(mbox, atn.val, atn.reg);
> > > +		return 0;
> > > +	}
> > 
> > +
> > > +	return -EINVAL;
> > 
> > +}
> > +
> > +static unsigned int mbox_poll(struct file *file, poll_table *wait)
> > +{
> > > +	struct mbox *mbox = file_mbox(file);
> > > +	unsigned int mask = 0;
> > 
> > +
> > > +	poll_wait(file, &mbox->queue, wait);
> > 
> > +
> > > +	if (mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> > > +		mask |= POLLIN;
> > 
> > +
> > > +	return mask;
> > 
> > +}
> > +
> > +static int mbox_release(struct inode *inode, struct file *file)
> > +{
> > > +	atomic_dec(&mbox_open_count);
> > > +	return 0;
> > 
> > +}
> > +
> > +static const struct file_operations mbox_fops = {
> > > > +	.owner		= THIS_MODULE,
> > > > +	.open		= mbox_open,
> > > > +	.read		= mbox_read,
> > > > +	.write		= mbox_write,
> > > > +	.release	= mbox_release,
> > > > +	.poll		= mbox_poll,
> > > > +	.unlocked_ioctl	= mbox_ioctl,
> > 
> > +};
> > +
> > +static void poll_timer(unsigned long data)
> > +{
> > > +	struct mbox *mbox = (void *)data;
> > 
> > +
> > > +	mbox->poll_timer.expires += msecs_to_jiffies(500);
> > > +	wake_up(&mbox->queue);
> > > +	add_timer(&mbox->poll_timer);
> > 
> > +}
> > +
> > +static irqreturn_t mbox_irq(int irq, void *arg)
> > +{
> > > +	struct mbox *mbox = arg;
> > 
> > +
> > > +	if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > > +		return IRQ_NONE;
> > 
> > +
> > > +	/*
> > > +	 * Leave the status bit set so that we know the data is for us,
> > > +	 * clear it once it has been read.
> > > +	 */
> > 
> > +
> > > +	/* Mask it off, we'll clear it when we the data gets read */
> > > +	mbox_outb(mbox, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
> > 
> > +
> > > +	wake_up(&mbox->queue);
> > > +	return IRQ_HANDLED;
> > 
> > +}
> > +
> > +static int mbox_config_irq(struct mbox *mbox,
> > > +		struct platform_device *pdev)
> > 
> > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	int rc;
> > 
> > +
> > > +	mbox->irq = irq_of_parse_and_map(dev->of_node, 0);
> > > +	if (!mbox->irq)
> > > +		return -ENODEV;
> > 
> > +
> > > +	rc = devm_request_irq(dev, mbox->irq, mbox_irq, IRQF_SHARED,
> > > +			DEVICE_NAME, mbox);
> > > +	if (rc < 0) {
> > > +		dev_warn(dev, "Unable to request IRQ %d\n", mbox->irq);
> > > +		mbox->irq = 0;
> > > +		return rc;
> > > +	}
> > 
> > +
> > > +	/*
> > > +	 * Disable all register based interrupts, we'll have to change
> > > +	 * this, protocol seemingly will require regs 0 and 15
> > > +	 */
> > > +	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
> > > +	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
> > 
> > +
> > > +	/* W1C */
> > > +	mbox_outb(mbox, 0xff, MBOX_STATUS_0);
> > > +	mbox_outb(mbox, 0xff, MBOX_STATUS_1);
> > 
> > +
> > > +	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > > +	return 0;
> > 
> > +}
> > +
> > +static int mbox_probe(struct platform_device *pdev)
> > +{
> > > +	struct mbox *mbox;
> > > +	struct device *dev;
> > > +	int rc;
> > 
> > +
> > > +	if (!pdev || !pdev->dev.of_node)
> > > +		return -ENODEV;
> > 
> > +
> > > +	dev = &pdev->dev;
> > > +	dev_info(dev, "Found mbox host device\n");
> > 
> > +
> > > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > +	if (!mbox)
> > > +		return -ENOMEM;
> > 
> > +
> > > +	dev_set_drvdata(&pdev->dev, mbox);
> > 
> > +
> > > +	rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
> > > +	if (rc) {
> > > +		dev_err(dev, "Couldn't read reg device-tree property\n");
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	mbox->regmap = syscon_node_to_regmap(
> > > +			pdev->dev.parent->of_node);
> > > +	if (IS_ERR(mbox->regmap)) {
> > > +		dev_err(dev, "Couldn't get regmap\n");
> > > +		rc = -ENODEV;
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	init_waitqueue_head(&mbox->queue);
> > 
> > +
> > > +	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
> > > +	mbox->miscdev.name = DEVICE_NAME;
> > > +	mbox->miscdev.fops = &mbox_fops;
> > > +	mbox->miscdev.parent = dev;
> > > +	rc = misc_register(&mbox->miscdev);
> > > +	if (rc) {
> > > +		dev_err(dev, "Unable to register device\n");
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	mbox_config_irq(mbox, pdev);
> > 
> > +
> > > +	if (mbox->irq) {
> > > +		dev_info(dev, "Using IRQ %d\n", mbox->irq);
> > > +	} else {
> > > +		dev_info(dev, "No IRQ; using timer\n");
> > > +		setup_timer(&mbox->poll_timer, poll_timer,
> > > +				(unsigned long)mbox);
> > > +		mbox->poll_timer.expires = jiffies + msecs_to_jiffies(10);
> > > +		add_timer(&mbox->poll_timer);
> > > +	}
> > 
> > +
> > +out:
> > > +	return rc;
> > 
> > +
> > +}
> > +
> > +static int mbox_remove(struct platform_device *pdev)
> > +{
> > > +	struct mbox *mbox = dev_get_drvdata(&pdev->dev);
> > 
> > +
> > > +	misc_deregister(&mbox->miscdev);
> > > +	if (!mbox->irq)
> > > +		del_timer_sync(&mbox->poll_timer);
> > > +	mbox = NULL;
> > 
> > +
> > > +	return 0;
> > 
> > +}
> > +
> > +static const struct of_device_id mbox_match[] = {
> > > +	{ .compatible = "aspeed,ast2400-mbox" },
> > > +	{ },
> > 
> > +};
> > +
> > +static struct platform_driver mbox_driver = {
> > > +	.driver = {
> > > > +		.name		= DEVICE_NAME,
> > > 
> > > +		.of_match_table = mbox_match,
> > > +	},
> > > +	.probe = mbox_probe,
> > > +	.remove = mbox_remove,
> > 
> > +};
> > +
> > +module_platform_driver(mbox_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, mbox_match);
> > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > 
> > +MODULE_DESCRIPTION("Linux device interface to the Aspeed MBOX registers");
> > diff --git a/include/uapi/linux/aspeed-mbox.h b/include/uapi/linux/aspeed-mbox.h
> > new file mode 100644
> > index 000000000000..f4e6a9242fd0
> > --- /dev/null
> > +++ b/include/uapi/linux/aspeed-mbox.h
> > @@ -0,0 +1,23 @@
> > +/*
> > + * 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>
> > +
> > +struct aspeed_mbox_atn {
> > > +	uint8_t reg;
> > > +	uint8_t val;
> > 
> > +};
> > +
> > > +#define __ASPEED_MBOX_IOCTL_MAGIC	0xb1
> > > +#define ASPEED_MBOX_IOCTL_ATN	_IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)
> > 
> > +
> > +#endif /* _UAPI_LINUX_MBOX_HOST_H */

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

[Andrew Jeffery]

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

On Fri, 2016-12-23 at 18:56 +1100, Cyril Bur wrote:
> On Fri, 2016-12-23 at 13:12 +1030, Andrew Jeffery wrote:
> > On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > > 
> > > +
> > > +static void mbox_outb(struct mbox *mbox, u8 data, int reg)
> > > +{
> > > +	regmap_write(mbox->regmap, mbox->base + reg, data);
> > 
> > We could use regmap_update_bits() here, but ultimately it's not going
> > to matter. Should we say something if regmap_write() fails (it
> > shouldn't, but if it does that's extra concerning)?
> 
> regmap_update_bits() to about touching the reserved section? Probs a
> good idea.

regmap_update_bits() will just write back what it read for the
untouched bits, which should be 0 in accordance with the datasheet.
It's not really a change in behaviour as we will still be writing to
reserved bits, but I think it makes the intent clear.

> Yeah I thought about putting a dev_err() in there. I'm not opposed, I
> sort of though, we'll you never see them or when you do something
> really bad has happened you'll probably notice everything broke...
> Still in the incredibly unlikely event that a print proves useful I'm
> happy to add it.

I feel an OCD itch to have something there, but I will leave it up to
you.

> > > > +	switch (cmd) {
> > > 
> > > +	case ASPEED_MBOX_IOCTL_ATN:
> > 
> > I think we should rename the IOCTL as what we do below doesn't
> > necessarily raise an interrupt.
> > 
> 
> Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?

That suggestion works for me.

Andrew

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

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

[Benjamin Herrenschmidt]

On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> 
> regmap_update_bits() will just write back what it read for the
> untouched bits, which should be 0 in accordance with the datasheet.
> It's not really a change in behaviour as we will still be writing to
> reserved bits, but I think it makes the intent clear.

It wastes cycles though...

Cheers,
Ben.

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

[Andrew Jeffery]

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

On Sun, 2017-01-08 at 15:44 -0600, Benjamin Herrenschmidt wrote:
> On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > 
> > regmap_update_bits() will just write back what it read for the
> > untouched bits, which should be 0 in accordance with the datasheet.
> > It's not really a change in behaviour as we will still be writing to
> > reserved bits, but I think it makes the intent clear.
> 
> It wastes cycles though...

True. 

That sways me towards leaving it as a regmap_write().

Andrew

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

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

[Benjamin Herrenschmidt]

On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > > I think we should rename the IOCTL as what we do below doesn't
> > > necessarily raise an interrupt.
> > > 
> > 
> > Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?
> 
> That suggestion works for me.

If we are going to do that, maybe we should make this a write()
at a specific lpos...

Otherwise call it something like SET_ATTN or SET_FLAGS ...

Cheers,
Ben.

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

[Cyril Bur]

On Sun, 2017-01-08 at 15:45 -0600, Benjamin Herrenschmidt wrote:
> On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > > > I think we should rename the IOCTL as what we do below doesn't
> > > > necessarily raise an interrupt.
> > > >  
> > > 
> > >  
> > > Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?
> > 
> > That suggestion works for me.
> 

Sorry about the blank one, I'll try typing something this time.

> If we are going to do that, maybe we should make this a write()
> at a specific lpos...
> 

Andrew, Joel what do you think of this, a write of count 1 at a
specific pos. I like this since it removes ioctls all together and
isn't any harder for userspace.

> Otherwise call it something like SET_ATTN or SET_FLAGS ...
> 
> Cheers,
> Ben.
> 

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

[Andrew Jeffery]

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

On Tue, 2017-01-10 at 09:09 +1100, Cyril Bur wrote:
> On Sun, 2017-01-08 at 15:45 -0600, Benjamin Herrenschmidt wrote:
> > On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > > > > I think we should rename the IOCTL as what we do below doesn't
> > > > > necessarily raise an interrupt.
> > > > >  
> > > > 
> > > >  
> > > > Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?
> > > 
> > > That suggestion works for me.
> 
> Sorry about the blank one, I'll try typing something this time.
> 
> > If we are going to do that, maybe we should make this a write()
> > at a specific lpos...
> > 
> 
> Andrew, Joel what do you think of this, a write of count 1 at a
> specific pos. I like this since it removes ioctls all together and
> isn't any harder for userspace.

I'm in favour of removing the ioctl. So the logic would be:

1. If lpos is zero, assume a MBOX_NUM_DATA_REGS-sized write as we do
currently
2. If lpos is non-zero, assume a single byte write

On that, should we be testing the assumptions about buffer sizes?
Currently we don't (we use the MBOX_NUM_DATA_REGS rather than count).

Andrew

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

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

[Cyril Bur]

On Tue, 2017-01-10 at 09:25 +1030, Andrew Jeffery wrote:
> On Tue, 2017-01-10 at 09:09 +1100, Cyril Bur wrote:
> > On Sun, 2017-01-08 at 15:45 -0600, Benjamin Herrenschmidt wrote:
> > > On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > > > > > I think we should rename the IOCTL as what we do below doesn't
> > > > > > necessarily raise an interrupt.
> > > > > >  
> > > > > 
> > > > >  
> > > > > Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?
> > > > 
> > > > That suggestion works for me.
> > 
> > Sorry about the blank one, I'll try typing something this time.
> > 
> > > If we are going to do that, maybe we should make this a write()
> > > at a specific lpos...
> > > 
> > 
> > Andrew, Joel what do you think of this, a write of count 1 at a
> > specific pos. I like this since it removes ioctls all together and
> > isn't any harder for userspace.
> 
> I'm in favour of removing the ioctl. So the logic would be:
> 

Cool done.

> 1. If lpos is zero, assume a MBOX_NUM_DATA_REGS-sized write as we do
> currently

Just to clarify - if lpos is zero anything smaller than or equal to
MBOX_NUM_DATA_REGS is good.

> 2. If lpos is non-zero, assume a single byte write
> 

Sounds good.

> On that, should we be testing the assumptions about buffer sizes?
> Currently we don't (we use the MBOX_NUM_DATA_REGS rather than count).
> 

Yeah. Simple checks for your condition 1 and 2.

> Andrew

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

[Cyril Bur]

On Tue, 2017-01-10 at 09:25 +1030, Andrew Jeffery wrote:
> On Tue, 2017-01-10 at 09:09 +1100, Cyril Bur wrote:
> > On Sun, 2017-01-08 at 15:45 -0600, Benjamin Herrenschmidt wrote:
> > > On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > > > > > I think we should rename the IOCTL as what we do below doesn't
> > > > > > necessarily raise an interrupt.
> > > > > >  
> > > > > 
> > > > >  
> > > > > Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?
> > > > 
> > > > That suggestion works for me.
> > 
> > Sorry about the blank one, I'll try typing something this time.
> > 
> > > If we are going to do that, maybe we should make this a write()
> > > at a specific lpos...
> > > 
> > 
> > Andrew, Joel what do you think of this, a write of count 1 at a
> > specific pos. I like this since it removes ioctls all together and
> > isn't any harder for userspace.
> 
> I'm in favour of removing the ioctl. So the logic would be:
> 
> 1. If lpos is zero, assume a MBOX_NUM_DATA_REGS-sized write as we do
> currently
> 2. If lpos is non-zero, assume a single byte write
> 

Ok so I started writing it - and I thought myself, wait why limit to
single byte writes of lpos is non-zero... I mean, they can do what they
want... what if they want to update two consequtive bytes, it seems
silly to have to require two write()s to do that no?

> On that, should we be testing the assumptions about buffer sizes?
> Currently we don't (we use the MBOX_NUM_DATA_REGS rather than count).
> 
> Andrew

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

[Andrew Jeffery]

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

On Tue, 2017-01-10 at 11:07 +1100, Cyril Bur wrote:
> On Tue, 2017-01-10 at 09:25 +1030, Andrew Jeffery wrote:
> > On Tue, 2017-01-10 at 09:09 +1100, Cyril Bur wrote:
> > > On Sun, 2017-01-08 at 15:45 -0600, Benjamin Herrenschmidt wrote:
> > > > On Tue, 2017-01-03 at 11:54 +1030, Andrew Jeffery wrote:
> > > > > > > I think we should rename the IOCTL as what we do below
> > > > > > > doesn't
> > > > > > > necessarily raise an interrupt.
> > > > > > >  
> > > > > > 
> > > > > >  
> > > > > > Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?
> > > > > 
> > > > > That suggestion works for me.
> > > 
> > > Sorry about the blank one, I'll try typing something this time.
> > > 
> > > > If we are going to do that, maybe we should make this a write()
> > > > at a specific lpos...
> > > > 
> > > 
> > > Andrew, Joel what do you think of this, a write of count 1 at a
> > > specific pos. I like this since it removes ioctls all together
> > > and
> > > isn't any harder for userspace.
> > 
> > I'm in favour of removing the ioctl. So the logic would be:
> > 
> > 1. If lpos is zero, assume a MBOX_NUM_DATA_REGS-sized write as we
> > do
> > currently
> > 2. If lpos is non-zero, assume a single byte write
> > 
> 
> Ok so I started writing it - and I thought myself, wait why limit to
> single byte writes of lpos is non-zero... I mean, they can do what
> they
> want... what if they want to update two consequtive bytes, it seems
> silly to have to require two write()s to do that no?

Sure, that was predicated on not checking count. Which was partly what
the question below was about. If we check count then I don't see why
there would need to be constraints beyond the number of mailbox data
registers.

> 
> > On that, should we be testing the assumptions about buffer sizes?
> > Currently we don't (we use the MBOX_NUM_DATA_REGS rather than
> > count).
> > 
> > Andrew

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

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

[Benjamin Herrenschmidt]

On Tue, 2017-01-10 at 09:25 +1030, Andrew Jeffery wrote:
> I'm in favour of removing the ioctl. So the logic would be:
> 
> 1. If lpos is zero, assume a MBOX_NUM_DATA_REGS-sized write as we do
> currently
> 2. If lpos is non-zero, assume a single byte write
> 
> On that, should we be testing the assumptions about buffer sizes?
> Currently we don't (we use the MBOX_NUM_DATA_REGS rather than count).

I would enforce the lpos is some specific magic value such as 0x1000
for the special write bcs we might want to support partial writes
inside the 16 bytes area...

Cheers,
Ben.

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

[Cyril Bur]

On Mon, 2017-01-09 at 22:28 -0600, Benjamin Herrenschmidt wrote:
> On Tue, 2017-01-10 at 09:25 +1030, Andrew Jeffery wrote:
> > I'm in favour of removing the ioctl. So the logic would be:
> > 
> > 1. If lpos is zero, assume a MBOX_NUM_DATA_REGS-sized write as we do
> > currently
> > 2. If lpos is non-zero, assume a single byte write
> > 
> > On that, should we be testing the assumptions about buffer sizes?
> > Currently we don't (we use the MBOX_NUM_DATA_REGS rather than count).
> 
> I would enforce the lpos is some specific magic value such as 0x1000
> for the special write bcs we might want to support partial writes
> inside the 16 bytes area...

So, its basically no more work (I think...) to just have partial
writes, which... satisfy this requirement no?

Just so we're on the same page - theres nothing 'special' about the
write it's just that he other end has mapped that 'data' register to
trigger an interrupt... the kernel doesn't know that nor does it need
to, the write is like any other, the interrupt gets triggered by
hardware.

> 
> Cheers,
> Ben.
> 

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

[Benjamin Herrenschmidt]

On Tue, 2017-01-10 at 15:36 +1100, Cyril Bur wrote:
> So, its basically no more work (I think...) to just have partial
> writes, which... satisfy this requirement no?
> 
> Just so we're on the same page - theres nothing 'special' about the
> write it's just that he other end has mapped that 'data' register to
> trigger an interrupt... the kernel doesn't know that nor does it need
> to, the write is like any other, the interrupt gets triggered by
> hardware.

Ok, that's fine then

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

[Benjamin Herrenschmidt]

On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:

> +
> +static ssize_t mbox_read(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> > +	char __user *p = buf;
> > +	int i;
> +
> > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > +		return -EFAULT;
> +
> > +	WARN_ON(*ppos);

Why the above ? Isn't that a user-triggerable WARN_ON ?

> +	if (wait_event_interruptible(mbox->queue,
> > +		mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > +		return -ERESTARTSYS;
> +
> > +	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
> > +		if (__put_user(mbox_inb(mbox, MBOX_DATA_0 + (i * 4)), p++))
> > +			return -EFAULT;
> +
> > +	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */
> > +	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +	return p - buf;
> +}

Not a huge deal, but concurrent reads by 2 processes are doing
to do weird stuff. Ideally you also want to support the
O_NONBLOCK case in case the user daemon want to "poll":

	if (file->f_flags & O_NONBLOCK) {
		if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)))
			return -EAGAIN;
	} else if (wait_event....)


> +static ssize_t mbox_write(struct file *file, const char __user *buf,
> > +				size_t count, loff_t *ppos)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> > +	const char __user *p = buf;
> > +	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, c, MBOX_DATA_0 + (i * 4));
> > +		p++;
> > +	}
> +
> > +	mbox_outb(mbox, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
> +
> > +	return p - buf;
> +}

Do you want to block til the previous one was consumed ?

> +static long mbox_ioctl(struct file *file, unsigned int cmd,
> > +		unsigned long param)
> +{
> > +	struct aspeed_mbox_atn atn;
> > +	struct mbox *mbox = file_mbox(file);
> > +	void __user *p = (void __user *)param;
> +
> > +	switch (cmd) {
> > +	case ASPEED_MBOX_IOCTL_ATN:
> > +		if (copy_from_user(&atn, p, sizeof(atn)))
> > +			return -EFAULT;
> > +		if (atn.reg > 15)
> > +			return -EINVAL;
> +
> > +		mbox_outb(mbox, atn.val, atn.reg);
> > +		return 0;
> > +	}
> +
> > +	return -EINVAL;
> +}
> +
> +static unsigned int mbox_poll(struct file *file, poll_table *wait)
> +{
> > +	struct mbox *mbox = file_mbox(file);
> > +	unsigned int mask = 0;
> +
> > +	poll_wait(file, &mbox->queue, wait);
> +
> > +	if (mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> > +		mask |= POLLIN;
> +
> > +	return mask;
> +}

What about a POLLOUT too in order to know that the write is consumed ?
(MBOX_CTRL_SEND is clear)

> +static int mbox_release(struct inode *inode, struct file *file)
> +{
> > +	atomic_dec(&mbox_open_count);
> > +	return 0;
> +}
> +
> +static const struct file_operations mbox_fops = {
> > > +	.owner		= THIS_MODULE,
> > > +	.open		= mbox_open,
> > > +	.read		= mbox_read,
> > > +	.write		= mbox_write,
> > > +	.release	= mbox_release,
> > > +	.poll		= mbox_poll,
> > > +	.unlocked_ioctl	= mbox_ioctl,
> +};
> +
> +static void poll_timer(unsigned long data)
> +{
> > +	struct mbox *mbox = (void *)data;
> +
> > +	mbox->poll_timer.expires += msecs_to_jiffies(500);
> > +	wake_up(&mbox->queue);
> > +	add_timer(&mbox->poll_timer);
> +}
> +
> +static irqreturn_t mbox_irq(int irq, void *arg)
> +{
> > +	struct mbox *mbox = arg;
> +
> > +	if (!(mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
> > +		return IRQ_NONE;
> +
> > +	/*
> > +	 * Leave the status bit set so that we know the data is for us,
> > +	 * clear it once it has been read.
> > +	 */
> +
> > +	/* Mask it off, we'll clear it when we the data gets read */
> > +	mbox_outb(mbox, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
> +
> > +	wake_up(&mbox->queue);
> > +	return IRQ_HANDLED;
> +}
> +
> +static int mbox_config_irq(struct mbox *mbox,
> > +		struct platform_device *pdev)
> +{
> > +	struct device *dev = &pdev->dev;
> > +	int rc;
> +
> > +	mbox->irq = irq_of_parse_and_map(dev->of_node, 0);
> > +	if (!mbox->irq)
> > +		return -ENODEV;
> +
> > +	rc = devm_request_irq(dev, mbox->irq, mbox_irq, IRQF_SHARED,
> > +			DEVICE_NAME, mbox);
> > +	if (rc < 0) {
> > +		dev_warn(dev, "Unable to request IRQ %d\n", mbox->irq);
> > +		mbox->irq = 0;
> > +		return rc;
> > +	}
> +
> > +	/*
> > +	 * Disable all register based interrupts, we'll have to change
> > +	 * this, protocol seemingly will require regs 0 and 15
> > +	 */
> > +	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_0); /* regs 0 - 7 */
> > +	mbox_outb(mbox, 0x00, MBOX_INTERRUPT_1); /* regs 8 - 15 */
> +
> > +	/* W1C */
> > +	mbox_outb(mbox, 0xff, MBOX_STATUS_0);
> > +	mbox_outb(mbox, 0xff, MBOX_STATUS_1);
> +
> > +	mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
> > +	return 0;
> +}
> +
> +static int mbox_probe(struct platform_device *pdev)
> +{
> > +	struct mbox *mbox;
> > +	struct device *dev;
> > +	int rc;
> +
> > +	if (!pdev || !pdev->dev.of_node)
> > +		return -ENODEV;
> +
> > +	dev = &pdev->dev;
> > +	dev_info(dev, "Found mbox host device\n");
> +
> > +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > +	if (!mbox)
> > +		return -ENOMEM;
> +
> > +	dev_set_drvdata(&pdev->dev, mbox);
> +
> > +	rc = of_property_read_u32(dev->of_node, "reg", &mbox->base);
> > +	if (rc) {
> > +		dev_err(dev, "Couldn't read reg device-tree property\n");
> > +		goto out;
> > +	}
> +
> > +	mbox->regmap = syscon_node_to_regmap(
> > +			pdev->dev.parent->of_node);
> > +	if (IS_ERR(mbox->regmap)) {
> > +		dev_err(dev, "Couldn't get regmap\n");
> > +		rc = -ENODEV;
> > +		goto out;
> > +	}
> +
> > +	init_waitqueue_head(&mbox->queue);
> +
> > +	mbox->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	mbox->miscdev.name = DEVICE_NAME;
> > +	mbox->miscdev.fops = &mbox_fops;
> > +	mbox->miscdev.parent = dev;
> > +	rc = misc_register(&mbox->miscdev);
> > +	if (rc) {
> > +		dev_err(dev, "Unable to register device\n");
> > +		goto out;
> > +	}
> +
> > +	mbox_config_irq(mbox, pdev);
> +
> > +	if (mbox->irq) {
> > +		dev_info(dev, "Using IRQ %d\n", mbox->irq);
> > +	} else {
> > +		dev_info(dev, "No IRQ; using timer\n");
> > +		setup_timer(&mbox->poll_timer, poll_timer,
> > +				(unsigned long)mbox);
> > +		mbox->poll_timer.expires = jiffies + msecs_to_jiffies(10);
> > +		add_timer(&mbox->poll_timer);
> > +	}
> +
> +out:
> > +	return rc;
> +
> +}
> +
> +static int mbox_remove(struct platform_device *pdev)
> +{
> > +	struct mbox *mbox = dev_get_drvdata(&pdev->dev);
> +
> > +	misc_deregister(&mbox->miscdev);
> > +	if (!mbox->irq)
> > +		del_timer_sync(&mbox->poll_timer);
> > +	mbox = NULL;
> +
> > +	return 0;
> +}
> +
> +static const struct of_device_id mbox_match[] = {
> > +	{ .compatible = "aspeed,ast2400-mbox" },
> > +	{ },
> +};
> +
> +static struct platform_driver mbox_driver = {
> > +	.driver = {
> > > +		.name		= DEVICE_NAME,
> > +		.of_match_table = mbox_match,
> > +	},
> > +	.probe = mbox_probe,
> > +	.remove = mbox_remove,
> +};
> +
> +module_platform_driver(mbox_driver);
> +
> +MODULE_DEVICE_TABLE(of, mbox_match);
> +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Linux device interface to the Aspeed MBOX registers");
> diff --git a/include/uapi/linux/aspeed-mbox.h b/include/uapi/linux/aspeed-mbox.h
> new file mode 100644
> index 000000000000..f4e6a9242fd0
> --- /dev/null
> +++ b/include/uapi/linux/aspeed-mbox.h
> @@ -0,0 +1,23 @@
> +/*
> + * 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>
> +
> +struct aspeed_mbox_atn {
> > +	uint8_t reg;
> > +	uint8_t val;
> +};
> +
> > +#define __ASPEED_MBOX_IOCTL_MAGIC	0xb1
> > +#define ASPEED_MBOX_IOCTL_ATN	_IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)
> +
> +#endif /* _UAPI_LINUX_MBOX_HOST_H */

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

[Cyril Bur]

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

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

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

Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Cyril Bur]

On Thu, 2016-12-22 at 17:06 +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                 |   9 ++
>  drivers/misc/Makefile                |   1 +
>  drivers/misc/aspeed-lpc-ctrl.c       | 292 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/aspeed-lpc-ctrl.h |  25 +++
>  4 files changed, 327 insertions(+)
>  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
>  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b4667742..f1e1c043d91c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGE
>  	  An empty message will only clear the display at driver init time. Any other
>  	  printf()-formatted message is valid with newline and escape codes.
>  
> +config ASPEED_LPC_CTRL
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	bool "Build a driver to control the BMC to HOST LPC bus"
> +	default "y"
> +	---help---
> +	  Provides a driver to control BMC to HOST LPC mappings through
> +	  ioctl()s, the driver aso provides a read/write interface to a BMC ram
> +	  region where host LPC can be buffered.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index b2fb6dbffcef..cdcd1af48971 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO)		+= echo/
>  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> +obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> new file mode 100644
> index 000000000000..4698d8fa5a4c
> --- /dev/null
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -0,0 +1,292 @@
> +/*
> + * 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/mtd/mtd.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/aspeed-lpc-ctrl.h>
> +
> +#define DEVICE_NAME	"aspeed-lpc-ctrl"
> +
> +#define HICR7 0x8
> +#define HICR8 0xc
> +
> +struct lpc_ctrl {
> +	struct miscdevice	miscdev;
> +	struct regmap		*regmap;
> +	phys_addr_t		base;
> +	resource_size_t		size;
> +	uint32_t		pnor_size;
> +	uint32_t		pnor_base;
> +};
> +
> +static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
> +
> +static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
> +{
> +	return container_of(file->private_data, struct lpc_ctrl, miscdev);
> +}
> +
> +static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> +	unsigned long vsize = vma->vm_end - vma->vm_start;
> +
> +	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
> +		return -EINVAL;
> +
> +	/* Other checks? */
> +
> +	if (remap_pfn_range(vma, vma->vm_start,
> +		(lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
> +		vsize, vma->vm_page_prot))
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static int lpc_ctrl_open(struct inode *inode, struct file *file)
> +{
> +	if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
> +		return 0;
> +
> +	atomic_dec(&lpc_ctrl_open_count);
> +	return -EBUSY;
> +}
> +
> +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	if (!access_ok(VERIFY_WRITE, buf, count))
> +		return -EFAULT;
> +
> +	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)
> +{
> +	long rc;
> +	struct lpc_mapping map;
> +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> +	void __user *p = (void __user *)param;
> +
> +	switch (cmd) {
> +	case LPC_CTRL_IOCTL_SIZE:
> +		return copy_to_user(p, &lpc_ctrl->size,
> +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> +	case LPC_CTRL_IOCTL_MAP:
> +		if (copy_from_user(&map, p, sizeof(map)))
> +			return -EFAULT;
> +
> +
> +		/*
> +		 * The top half of HICR7 is the MSB of the BMC address of the
> +		 * mapping.
> +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> +		 * firmware space address of the mapping.
> +		 *
> +		 * The 1 bits in the top of half of HICR8 represent the bits
> +		 * (in the requested address) that should be ignored and
> +		 * replaced with those from the top half of HICR7.
> +		 * The 1 bits in the bottom half of HICR8 represent the bits
> +		 * (in the requested address) that should be kept and pass
> +		 * into the BMC address space.
> +		 */
> +
> +		regmap_write(lpc_ctrl->regmap, HICR7,
> +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> +		regmap_write(lpc_ctrl->regmap, HICR8,
> +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> +		return 0;
> +	case LPC_CTRL_IOCTL_UNMAP:
> +		/*
> +		 * The top nibble in host lpc addresses references which
> +		 * firmware space, use space zero hence the & 0x0fff
> +		 */
> +
> +		dev_info(lpc_ctrl->miscdev.parent, "Setting HICR7 to 0x%08x\n",
> +			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
> +		rc = regmap_write(lpc_ctrl->regmap, HICR7,
> +			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
> +		if (rc)
> +			return rc;
> +		dev_info(lpc_ctrl->miscdev.parent, "Setting HICR8 to 0x%08x\n",
> +			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> +			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> +		return rc;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> +{
> +	atomic_dec(&lpc_ctrl_open_count);
> +	return 0;
> +}
> +
> +static const struct file_operations lpc_ctrl_fops = {
> +	.owner		= THIS_MODULE,
> +	.mmap		= lpc_ctrl_mmap,
> +	.open		= lpc_ctrl_open,
> +	.read		= lpc_ctrl_read,
> +	.write		= lpc_ctrl_write,
> +	.release	= lpc_ctrl_release,
> +	.unlocked_ioctl	= lpc_ctrl_ioctl,
> +};
> +
> +static int lpc_ctrl_probe(struct platform_device *pdev)
> +{
> +	struct lpc_ctrl *lpc_ctrl;
> +	struct device *dev;
> +	struct device_node *node;
> +	struct resource resm;
> +	struct mtd_info *mtd;
> +	int rc;
> +
> +	if (!pdev || !pdev->dev.of_node)
> +		return -ENODEV;
> +
> +	mtd = get_mtd_device_nm("1e630000.spi:pnor@0");

I rebased right before sending and admitedly pressed send before it
finished booting, turns out we've changed it back to simply "pnor". Was
this intentional?

> +	if (IS_ERR(mtd)) {
> +		dev_err(&pdev->dev, "Couldn't find pnor\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	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);
> +
> +	node = of_get_parent(mtd_get_of_node(mtd));
> +	if (!node) {
> +		dev_err(dev, "Couldn't get MTD parent OF node\n");
> +		return -ENODEV;
> +	}
> +	lpc_ctrl->pnor_size = mtd->size;
> +	rc = of_property_read_u32_index(node, "reg", 2,
> +			&lpc_ctrl->pnor_base);
> +	if (rc)
> +		return rc;
> +
> +	dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
> +		lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
> +
> +	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->regmap = syscon_node_to_regmap(
> +			pdev->dev.parent->of_node);
> +	if (IS_ERR(lpc_ctrl->regmap)) {
> +		dev_err(dev, "Couldn't get regmap\n");
> +		rc = -ENODEV;
> +		goto out;
> +	}
> +
> +	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> +	lpc_ctrl->miscdev.name = DEVICE_NAME;
> +	lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
> +	lpc_ctrl->miscdev.parent = dev;
> +	rc = misc_register(&lpc_ctrl->miscdev);
> +	if (rc)
> +		dev_err(dev, "Unable to register device\n");
> +	else
> +		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> +			lpc_ctrl->base, lpc_ctrl->size);
> +
> +out:
> +	return rc;
> +}
> +
> +static int lpc_ctrl_remove(struct platform_device *pdev)
> +{
> +	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> +
> +	misc_deregister(&lpc_ctrl->miscdev);
> +	lpc_ctrl = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lpc_ctrl_match[] = {
> +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> +	{ },
> +};
> +
> +static struct platform_driver lpc_ctrl_driver = {
> +	.driver = {
> +		.name		= DEVICE_NAME,
> +		.of_match_table = lpc_ctrl_match,
> +	},
> +	.probe = lpc_ctrl_probe,
> +	.remove = lpc_ctrl_remove,
> +};
> +
> +module_platform_driver(lpc_ctrl_driver);
> +
> +MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Linux device interface to control LPC bus");
> diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
> new file mode 100644
> index 000000000000..c5f1caf827ac
> --- /dev/null
> +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_LINUX_LPC_CTRL_H
> +#define _UAPI_LINUX_LPC_CTRL_H
> +
> +#include <linux/ioctl.h>
> +
> +struct lpc_mapping {
> +	uint32_t hostaddr;
> +	uint32_t size;
> +};
> +
> +#define __LPC_CTRL_IOCTL_MAGIC	0xb2
> +#define LPC_CTRL_IOCTL_SIZE	_IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
> +#define LPC_CTRL_IOCTL_MAP	_IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
> +#define LPC_CTRL_IOCTL_UNMAP	_IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
> +
> +#endif /* _UAPI_LINUX_LPC_CTRL_H */

Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Andrew Jeffery]

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

On Fri, 2016-12-23 at 10:54 +1100, Cyril Bur wrote:
> > > +     mtd = get_mtd_device_nm("1e630000.spi:pnor@0");
> 
> I rebased right before sending and admitedly pressed send before it
> finished booting, turns out we've changed it back to simply "pnor". Was
> this intentional?

Can we just derive it from the devicetree? That way we wouldn't care
what it was.

Andrew

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

Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Andrew Jeffery]

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

On Thu, 2016-12-22 at 17:06 +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                 |   9 ++
>  drivers/misc/Makefile                |   1 +
>  drivers/misc/aspeed-lpc-ctrl.c       | 292 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/aspeed-lpc-ctrl.h |  25 +++
>  4 files changed, 327 insertions(+)
>  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
>  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b4667742..f1e1c043d91c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGE
> >  	  An empty message will only clear the display at driver init time. Any other
> >  	  printf()-formatted message is valid with newline and escape codes.
>  
> +config ASPEED_LPC_CTRL
> +	depends on ARCH_ASPEED || COMPILE_TEST

Something that I should have mentioned on 4/5 as well is that we now
need to say:

    depends on REGMAP && MFD_SYSCON

> +	bool "Build a driver to control the BMC to HOST LPC bus"
> > +	default "y"
> > +	---help---
> > +	  Provides a driver to control BMC to HOST LPC mappings through
> > +	  ioctl()s, the driver aso provides a read/write interface to a BMC ram
> > +	  region where host LPC can be buffered.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index b2fb6dbffcef..cdcd1af48971 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> > @@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO)		+= echo/
> >  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
> >  obj-$(CONFIG_CXL_BASE)		+= cxl/
>  obj-$(CONFIG_PANEL)             += panel.o
> > +obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> new file mode 100644
> index 000000000000..4698d8fa5a4c
> --- /dev/null
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -0,0 +1,292 @@
> +/*
> + * 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/mtd/mtd.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/aspeed-lpc-ctrl.h>
> +
> > +#define DEVICE_NAME	"aspeed-lpc-ctrl"
> +
> +#define HICR7 0x8
> +#define HICR8 0xc
> +
> +struct lpc_ctrl {
> > > +	struct miscdevice	miscdev;
> > > +	struct regmap		*regmap;
> > > +	phys_addr_t		base;
> > > +	resource_size_t		size;
> > > +	uint32_t		pnor_size;
> > > +	uint32_t		pnor_base;
> +};
> +
> +static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
> +
> +static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
> +{
> > +	return container_of(file->private_data, struct lpc_ctrl, miscdev);
> +}
> +
> +static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > +	unsigned long vsize = vma->vm_end - vma->vm_start;
> +
> > +	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
> > +		return -EINVAL;
> +
> > +	/* Other checks? */
> +
> > +	if (remap_pfn_range(vma, vma->vm_start,
> > +		(lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
> > +		vsize, vma->vm_page_prot))
> > +		return -EAGAIN;
> +
> > +	return 0;
> +}
> +
> +static int lpc_ctrl_open(struct inode *inode, struct file *file)
> +{
> > +	if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
> > +		return 0;
> +
> > +	atomic_dec(&lpc_ctrl_open_count);
> > +	return -EBUSY;
> +}
> +
> +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> +{
> > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > +		return -EFAULT;
> +
> > +	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)
> +{
> > +	long rc;
> > +	struct lpc_mapping map;
> > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > +	void __user *p = (void __user *)param;
> +
> > +	switch (cmd) {
> > +	case LPC_CTRL_IOCTL_SIZE:
> > +		return copy_to_user(p, &lpc_ctrl->size,
> > +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > +	case LPC_CTRL_IOCTL_MAP:
> > +		if (copy_from_user(&map, p, sizeof(map)))
> > +			return -EFAULT;
> +
> +

Whitespace (double line break).

> +		/*
> > +		 * The top half of HICR7 is the MSB of the BMC address of the
> > +		 * mapping.
> > +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> > +		 * firmware space address of the mapping.
> > +		 *
> > +		 * The 1 bits in the top of half of HICR8 represent the bits
> > +		 * (in the requested address) that should be ignored and
> > +		 * replaced with those from the top half of HICR7.
> > +		 * The 1 bits in the bottom half of HICR8 represent the bits
> > +		 * (in the requested address) that should be kept and pass
> > +		 * into the BMC address space.
> +		 */

If only we could put this comment into the datasheet and re-publish it.

> +
> > +		regmap_write(lpc_ctrl->regmap, HICR7,
> > +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> > +		regmap_write(lpc_ctrl->regmap, HICR8,
> +			(~(map.size - 1)) | ((map.size >> 16) - 1));

What are your thoughts on error handling

> +		return 0;
> > +	case LPC_CTRL_IOCTL_UNMAP:
> > +		/*
> > +		 * The top nibble in host lpc addresses references which
> > +		 * firmware space, use space zero hence the & 0x0fff
> > +		 */
> +
> > +		dev_info(lpc_ctrl->miscdev.parent, "Setting HICR7 to 0x%08x\n",
> +			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));

Lets drop the dev_info()s, we've confirmed this works with the fixes to
the mbox driver.

> +             rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > +			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
> > +		if (rc)
> > +			return rc;
> > +		dev_info(lpc_ctrl->miscdev.parent, "Setting HICR8 to 0x%08x\n",
> > +			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> > +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > +			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> > +		return rc;
> > +	}
> +
> > +	return -EINVAL;
> +}
> +
> +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> +{
> > +	atomic_dec(&lpc_ctrl_open_count);
> > +	return 0;
> +}
> +
> +static const struct file_operations lpc_ctrl_fops = {
> > > +	.owner		= THIS_MODULE,
> > > +	.mmap		= lpc_ctrl_mmap,
> > > +	.open		= lpc_ctrl_open,
> > > +	.read		= lpc_ctrl_read,
> > > +	.write		= lpc_ctrl_write,
> > > +	.release	= lpc_ctrl_release,
> > > +	.unlocked_ioctl	= lpc_ctrl_ioctl,
> +};
> +
> +static int lpc_ctrl_probe(struct platform_device *pdev)
> +{
> > +	struct lpc_ctrl *lpc_ctrl;
> > +	struct device *dev;
> > +	struct device_node *node;
> > +	struct resource resm;
> > +	struct mtd_info *mtd;
> > +	int rc;
> +
> > +	if (!pdev || !pdev->dev.of_node)
> > +		return -ENODEV;
> +
> > > +	mtd = get_mtd_device_nm("1e630000.spi:pnor@0");
> > +	if (IS_ERR(mtd)) {
> > +		dev_err(&pdev->dev, "Couldn't find pnor\n");
> > +		return -EPROBE_DEFER;
> > +	}
> +
> > +	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);
> +
> > +	node = of_get_parent(mtd_get_of_node(mtd));
> > +	if (!node) {
> > +		dev_err(dev, "Couldn't get MTD parent OF node\n");
> > +		return -ENODEV;
> > +	}
> > +	lpc_ctrl->pnor_size = mtd->size;
> > +	rc = of_property_read_u32_index(node, "reg", 2,
> > +			&lpc_ctrl->pnor_base);
> > +	if (rc)
> > +		return rc;
> +
> > +	dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
> > +		lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
> +
> > +	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");

I think this is good enough for the moment?

> +		rc = -EINVAL;
> > +		goto out;
> > +	}
> +
> > +	rc = of_address_to_resource(node, 0, &resm);
> > +	of_node_put(node);
> > +	if (rc) {
> > +		dev_err(dev, "Could address to resource\n");
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> +
> > +	lpc_ctrl->size = resource_size(&resm);
> > +	lpc_ctrl->base = resm.start;
> +
> > +	lpc_ctrl->regmap = syscon_node_to_regmap(
> > +			pdev->dev.parent->of_node);
> > +	if (IS_ERR(lpc_ctrl->regmap)) {
> > +		dev_err(dev, "Couldn't get regmap\n");
> > +		rc = -ENODEV;
> > +		goto out;
> > +	}
> +
> > +	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	lpc_ctrl->miscdev.name = DEVICE_NAME;
> > +	lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
> > +	lpc_ctrl->miscdev.parent = dev;
> > +	rc = misc_register(&lpc_ctrl->miscdev);
> > +	if (rc)
> > +		dev_err(dev, "Unable to register device\n");
> > +	else
> > +		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > +			lpc_ctrl->base, lpc_ctrl->size);
> +
> +out:
> > +	return rc;
> +}
> +
> +static int lpc_ctrl_remove(struct platform_device *pdev)
> +{
> > +	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> +
> +	misc_deregister(&lpc_ctrl->miscdev);

It feels like there should be a devm_misc_register()...

> +	lpc_ctrl = NULL;
> +
> > +	return 0;
> +}
> +
> +static const struct of_device_id lpc_ctrl_match[] = {
> +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },

Another issue I meant to mention on 4/5 was we'll need bindings
documentation for these compatible strings.

It's probably also worth adding the AST2500.

> +	{ },
> +};
> +
> +static struct platform_driver lpc_ctrl_driver = {
> > +	.driver = {
> > > +		.name		= DEVICE_NAME,
> > +		.of_match_table = lpc_ctrl_match,
> > +	},
> > +	.probe = lpc_ctrl_probe,
> > +	.remove = lpc_ctrl_remove,
> +};
> +
> +module_platform_driver(lpc_ctrl_driver);
> +
> +MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
> +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> +MODULE_DESCRIPTION("Linux device interface to control LPC bus");
> diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
> new file mode 100644
> index 000000000000..c5f1caf827ac
> --- /dev/null
> +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#ifndef _UAPI_LINUX_LPC_CTRL_H
> +#define _UAPI_LINUX_LPC_CTRL_H
> +
> +#include <linux/ioctl.h>
> +
> +struct lpc_mapping {
> > +	uint32_t hostaddr;
> > +	uint32_t size;
> +};
> +
> > +#define __LPC_CTRL_IOCTL_MAGIC	0xb2
> > +#define LPC_CTRL_IOCTL_SIZE	_IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
> > +#define LPC_CTRL_IOCTL_MAP	_IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
> > +#define LPC_CTRL_IOCTL_UNMAP	_IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
> +
> +#endif /* _UAPI_LINUX_LPC_CTRL_H */

Cheers,

Andrew

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

Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Cyril Bur]

On Fri, 2016-12-23 at 13:25 +1030, Andrew Jeffery wrote:
> On Thu, 2016-12-22 at 17:06 +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                 |   9 ++
> >  drivers/misc/Makefile                |   1 +
> >  drivers/misc/aspeed-lpc-ctrl.c       | 292 +++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/aspeed-lpc-ctrl.h |  25 +++
> >  4 files changed, 327 insertions(+)
> >  create mode 100644 drivers/misc/aspeed-lpc-ctrl.c
> >  create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index a216b4667742..f1e1c043d91c 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGE
> > >  	  An empty message will only clear the display at driver init time. Any other
> > >  	  printf()-formatted message is valid with newline and escape codes.
> > 
> >  
> > +config ASPEED_LPC_CTRL
> > +	depends on ARCH_ASPEED || COMPILE_TEST
> 
> Something that I should have mentioned on 4/5 as well is that we now
> need to say:
> 
>     depends on REGMAP && MFD_SYSCON
> 

Yeah my bad should have figured that out.

Thanks

> > +	bool "Build a driver to control the BMC to HOST LPC bus"
> > > +	default "y"
> > > +	---help---
> > > +	  Provides a driver to control BMC to HOST LPC mappings through
> > > +	  ioctl()s, the driver aso provides a read/write interface to a BMC ram
> > > +	  region where host LPC can be buffered.
> > 
> > +
> >  source "drivers/misc/c2port/Kconfig"
> >  source "drivers/misc/eeprom/Kconfig"
> >  source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index b2fb6dbffcef..cdcd1af48971 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > > @@ -57,3 +57,4 @@ obj-$(CONFIG_ECHO)		+= echo/
> > >  obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
> > >  obj-$(CONFIG_CXL_BASE)		+= cxl/
> > 
> >  obj-$(CONFIG_PANEL)             += panel.o
> > > +obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
> > 
> > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> > new file mode 100644
> > index 000000000000..4698d8fa5a4c
> > --- /dev/null
> > +++ b/drivers/misc/aspeed-lpc-ctrl.c
> > @@ -0,0 +1,292 @@
> > +/*
> > + * 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/mtd/mtd.h>
> > +#include <linux/timer.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/regmap.h>
> > +#include <linux/aspeed-lpc-ctrl.h>
> > +
> > > +#define DEVICE_NAME	"aspeed-lpc-ctrl"
> > 
> > +
> > +#define HICR7 0x8
> > +#define HICR8 0xc
> > +
> > +struct lpc_ctrl {
> > > > +	struct miscdevice	miscdev;
> > > > +	struct regmap		*regmap;
> > > > +	phys_addr_t		base;
> > > > +	resource_size_t		size;
> > > > +	uint32_t		pnor_size;
> > > > +	uint32_t		pnor_base;
> > 
> > +};
> > +
> > +static atomic_t lpc_ctrl_open_count = ATOMIC_INIT(0);
> > +
> > +static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
> > +{
> > > +	return container_of(file->private_data, struct lpc_ctrl, miscdev);
> > 
> > +}
> > +
> > +static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > +	unsigned long vsize = vma->vm_end - vma->vm_start;
> > 
> > +
> > > +	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
> > > +		return -EINVAL;
> > 
> > +
> > > +	/* Other checks? */
> > 
> > +
> > > +	if (remap_pfn_range(vma, vma->vm_start,
> > > +		(lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
> > > +		vsize, vma->vm_page_prot))
> > > +		return -EAGAIN;
> > 
> > +
> > > +	return 0;
> > 
> > +}
> > +
> > +static int lpc_ctrl_open(struct inode *inode, struct file *file)
> > +{
> > > +	if (atomic_inc_return(&lpc_ctrl_open_count) == 1)
> > > +		return 0;
> > 
> > +
> > > +	atomic_dec(&lpc_ctrl_open_count);
> > > +	return -EBUSY;
> > 
> > +}
> > +
> > +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
> > > +				size_t count, loff_t *ppos)
> > 
> > +{
> > > +	if (!access_ok(VERIFY_WRITE, buf, count))
> > > +		return -EFAULT;
> > 
> > +
> > > +	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)
> > 
> > +{
> > > +	long rc;
> > > +	struct lpc_mapping map;
> > > +	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
> > > +	void __user *p = (void __user *)param;
> > 
> > +
> > > +	switch (cmd) {
> > > +	case LPC_CTRL_IOCTL_SIZE:
> > > +		return copy_to_user(p, &lpc_ctrl->size,
> > > +			sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
> > > +	case LPC_CTRL_IOCTL_MAP:
> > > +		if (copy_from_user(&map, p, sizeof(map)))
> > > +			return -EFAULT;
> > 
> > +
> > +
> 
> Whitespace (double line break).
> 

Thanks

> > +		/*
> > > +		 * The top half of HICR7 is the MSB of the BMC address of the
> > > +		 * mapping.
> > > +		 * The bottom half of HICR7 is the MSB of the HOST LPC
> > > +		 * firmware space address of the mapping.
> > > +		 *
> > > +		 * The 1 bits in the top of half of HICR8 represent the bits
> > > +		 * (in the requested address) that should be ignored and
> > > +		 * replaced with those from the top half of HICR7.
> > > +		 * The 1 bits in the bottom half of HICR8 represent the bits
> > > +		 * (in the requested address) that should be kept and pass
> > > +		 * into the BMC address space.
> > 
> > +		 */
> 
> If only we could put this comment into the datasheet and re-publish it.
> 

Glad it makes sense!

> > +
> > > +		regmap_write(lpc_ctrl->regmap, HICR7,
> > > +				(lpc_ctrl->base | (map.hostaddr >> 16)));
> > > +		regmap_write(lpc_ctrl->regmap, HICR8,
> > 
> > +			(~(map.size - 1)) | ((map.size >> 16) - 1));
> 
> What are your thoughts on error handling
> 

I'll do the same as unmap.

> > +		return 0;
> > > +	case LPC_CTRL_IOCTL_UNMAP:
> > > +		/*
> > > +		 * The top nibble in host lpc addresses references which
> > > +		 * firmware space, use space zero hence the & 0x0fff
> > > +		 */
> > 
> > +
> > > +		dev_info(lpc_ctrl->miscdev.parent, "Setting HICR7 to 0x%08x\n",
> > 
> > +			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
> 
> Lets drop the dev_info()s, we've confirmed this works with the fixes to
> the mbox driver.

Yep.

> 
> > +             rc = regmap_write(lpc_ctrl->regmap, HICR7,
> > > +			lpc_ctrl->pnor_base | (((-lpc_ctrl->pnor_size) >> 16) & 0x0fff));
> > > +		if (rc)
> > > +			return rc;
> > > +		dev_info(lpc_ctrl->miscdev.parent, "Setting HICR8 to 0x%08x\n",
> > > +			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> > > +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> > > +			(~(lpc_ctrl->pnor_size - 1)) | ((lpc_ctrl->pnor_size >> 16) - 1));
> > > +		return rc;
> > > +	}
> > 
> > +
> > > +	return -EINVAL;
> > 
> > +}
> > +
> > +static int lpc_ctrl_release(struct inode *inode, struct file *file)
> > +{
> > > +	atomic_dec(&lpc_ctrl_open_count);
> > > +	return 0;
> > 
> > +}
> > +
> > +static const struct file_operations lpc_ctrl_fops = {
> > > > +	.owner		= THIS_MODULE,
> > > > +	.mmap		= lpc_ctrl_mmap,
> > > > +	.open		= lpc_ctrl_open,
> > > > +	.read		= lpc_ctrl_read,
> > > > +	.write		= lpc_ctrl_write,
> > > > +	.release	= lpc_ctrl_release,
> > > > +	.unlocked_ioctl	= lpc_ctrl_ioctl,
> > 
> > +};
> > +
> > +static int lpc_ctrl_probe(struct platform_device *pdev)
> > +{
> > > +	struct lpc_ctrl *lpc_ctrl;
> > > +	struct device *dev;
> > > +	struct device_node *node;
> > > +	struct resource resm;
> > > +	struct mtd_info *mtd;
> > > +	int rc;
> > 
> > +
> > > +	if (!pdev || !pdev->dev.of_node)
> > > +		return -ENODEV;
> > 
> > +
> > > > +	mtd = get_mtd_device_nm("1e630000.spi:pnor@0");
> > > 
> > > +	if (IS_ERR(mtd)) {
> > > +		dev_err(&pdev->dev, "Couldn't find pnor\n");
> > > +		return -EPROBE_DEFER;
> > > +	}
> > 
> > +
> > > +	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);
> > 
> > +
> > > +	node = of_get_parent(mtd_get_of_node(mtd));
> > > +	if (!node) {
> > > +		dev_err(dev, "Couldn't get MTD parent OF node\n");
> > > +		return -ENODEV;
> > > +	}
> > > +	lpc_ctrl->pnor_size = mtd->size;
> > > +	rc = of_property_read_u32_index(node, "reg", 2,
> > > +			&lpc_ctrl->pnor_base);
> > > +	if (rc)
> > > +		return rc;
> > 
> > +
> > > +	dev_info(dev, "Host PNOR base: 0x%08x size: 0x%08x\n",
> > > +		lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);
> > 
> > +
> > > +	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");
> 
> I think this is good enough for the moment?
> 

Yep, would you prefer I remove the comment? My userspace daemon
wouldn't handle a buffer smaller than flash so even if we wrote it we
couldn't use it.

> > +		rc = -EINVAL;
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	rc = of_address_to_resource(node, 0, &resm);
> > > +	of_node_put(node);
> > > +	if (rc) {
> > > +		dev_err(dev, "Could address to resource\n");
> > > +		rc = -ENOMEM;
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	lpc_ctrl->size = resource_size(&resm);
> > > +	lpc_ctrl->base = resm.start;
> > 
> > +
> > > +	lpc_ctrl->regmap = syscon_node_to_regmap(
> > > +			pdev->dev.parent->of_node);
> > > +	if (IS_ERR(lpc_ctrl->regmap)) {
> > > +		dev_err(dev, "Couldn't get regmap\n");
> > > +		rc = -ENODEV;
> > > +		goto out;
> > > +	}
> > 
> > +
> > > +	lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
> > > +	lpc_ctrl->miscdev.name = DEVICE_NAME;
> > > +	lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
> > > +	lpc_ctrl->miscdev.parent = dev;
> > > +	rc = misc_register(&lpc_ctrl->miscdev);
> > > +	if (rc)
> > > +		dev_err(dev, "Unable to register device\n");
> > > +	else
> > > +		dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
> > > +			lpc_ctrl->base, lpc_ctrl->size);
> > 
> > +
> > +out:
> > > +	return rc;
> > 
> > +}
> > +
> > +static int lpc_ctrl_remove(struct platform_device *pdev)
> > +{
> > > +	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> > 
> > +
> > +	misc_deregister(&lpc_ctrl->miscdev);
> 
> It feels like there should be a devm_misc_register()...
> 

Uh sorry?

> > +	lpc_ctrl = NULL;
> > +
> > > +	return 0;
> > 
> > +}
> > +
> > +static const struct of_device_id lpc_ctrl_match[] = {
> > +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> 
> Another issue I meant to mention on 4/5 was we'll need bindings
> documentation for these compatible strings.
> 
> It's probably also worth adding the AST2500.
> 

True,

Thanks,

Cyril

> > +	{ },
> > +};
> > +
> > +static struct platform_driver lpc_ctrl_driver = {
> > > +	.driver = {
> > > > +		.name		= DEVICE_NAME,
> > > 
> > > +		.of_match_table = lpc_ctrl_match,
> > > +	},
> > > +	.probe = lpc_ctrl_probe,
> > > +	.remove = lpc_ctrl_remove,
> > 
> > +};
> > +
> > +module_platform_driver(lpc_ctrl_driver);
> > +
> > +MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
> > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Cyril Bur <cyrilbur@gmail.com>");
> > 
> > +MODULE_DESCRIPTION("Linux device interface to control LPC bus");
> > diff --git a/include/uapi/linux/aspeed-lpc-ctrl.h b/include/uapi/linux/aspeed-lpc-ctrl.h
> > new file mode 100644
> > index 000000000000..c5f1caf827ac
> > --- /dev/null
> > +++ b/include/uapi/linux/aspeed-lpc-ctrl.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Copyright 2016 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#ifndef _UAPI_LINUX_LPC_CTRL_H
> > +#define _UAPI_LINUX_LPC_CTRL_H
> > +
> > +#include <linux/ioctl.h>
> > +
> > +struct lpc_mapping {
> > > +	uint32_t hostaddr;
> > > +	uint32_t size;
> > 
> > +};
> > +
> > > +#define __LPC_CTRL_IOCTL_MAGIC	0xb2
> > > +#define LPC_CTRL_IOCTL_SIZE	_IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
> > > +#define LPC_CTRL_IOCTL_MAP	_IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
> > > +#define LPC_CTRL_IOCTL_UNMAP	_IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
> > 
> > +
> > +#endif /* _UAPI_LINUX_LPC_CTRL_H */
> 
> Cheers,
> 
> Andrew

Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver

[Andrew Jeffery]

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

On Fri, 2016-12-23 at 19:04 +1100, Cyril Bur wrote:
> On Fri, 2016-12-23 at 13:25 +1030, Andrew Jeffery wrote:
> > On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > > +	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");
> > 
> > I think this is good enough for the moment?
> > 
> 
> Yep, would you prefer I remove the comment? My userspace daemon
> wouldn't handle a buffer smaller than flash so even if we wrote it we
> couldn't use it.

I think removing the comment is the right way to go.

> 
> > > +static int lpc_ctrl_remove(struct platform_device *pdev)
> > > +{
> > > > +	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
> > > 
> > > +
> > > +	misc_deregister(&lpc_ctrl->miscdev);
> > 
> > It feels like there should be a devm_misc_register()...
> > 
> 
> Uh sorry?

Just a passing comment on the misc_* interfaces. Nothing to worry
about.

> 
> > > > > > +	lpc_ctrl = NULL;
> > > +
> > > > +	return 0;
> > > 
> > > +}
> > > +
> > > +static const struct of_device_id lpc_ctrl_match[] = {
> > > +	{ .compatible = "aspeed,ast2400-lpc-ctrl" },
> > 
> > Another issue I meant to mention on 4/5 was we'll need bindings
> > documentation for these compatible strings.
> > 
> > It's probably also worth adding the AST2500.
> > 
> 
> True,

Mainly because we should be targeting this at mainline, not the OpenBMC
kernel tree[1]:

    Once you are sure a driver needs to be written, you should develop
    and test the driver, before sending it upstream to the relevant
    maintainers. You should feel welcome to cc the OpenBMC list when
    sending upstream, so other kernel developers can provide input where
    appropriate. Be sure to follow the upstream development process.

    In the past patches underwent 'pre-review' on the OpenBMC mailing
    list. While this is useful for developers who are not familiar with
    writing kernel code, it has lead to confusion about the upstreaming
    process, so now we do all of our development in the community. 

[1] https://github.com/openbmc/docs/blob/master/kernel-development.md#developing-a-new-driver

Andrew

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