* [PATCH v2 0/5] LPC/MBOX work @ 2016-12-22 6:06 Cyril Bur 2016-12-22 6:06 ` [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication Cyril Bur ` (4 more replies) 0 siblings, 5 replies; 28+ messages in thread From: Cyril Bur @ 2016-12-22 6:06 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication 2016-12-22 6:06 [PATCH v2 0/5] LPC/MBOX work Cyril Bur @ 2016-12-22 6:06 ` Cyril Bur 2016-12-23 1:01 ` Andrew Jeffery 2016-12-22 6:06 ` [PATCH v2 2/5] ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap Cyril Bur ` (3 subsequent siblings) 4 siblings, 1 reply; 28+ messages in thread From: Cyril Bur @ 2016-12-22 6:06 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication 2016-12-22 6:06 ` [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication Cyril Bur @ 2016-12-23 1:01 ` Andrew Jeffery 2016-12-23 7:50 ` Cyril Bur 0 siblings, 1 reply; 28+ messages in thread From: Andrew Jeffery @ 2016-12-23 1:01 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication 2016-12-23 1:01 ` Andrew Jeffery @ 2016-12-23 7:50 ` Cyril Bur 0 siblings, 0 replies; 28+ messages in thread From: Cyril Bur @ 2016-12-23 7:50 UTC (permalink / raw) To: Andrew Jeffery, openbmc; +Cc: millerjo 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>; ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/5] ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap 2016-12-22 6:06 [PATCH v2 0/5] LPC/MBOX work Cyril Bur 2016-12-22 6:06 ` [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication Cyril Bur @ 2016-12-22 6:06 ` Cyril Bur 2016-12-22 6:06 ` [PATCH v2 3/5] ARM: dts: aspeed: Move mbox under lpc_host node Cyril Bur ` (2 subsequent siblings) 4 siblings, 0 replies; 28+ messages in thread From: Cyril Bur @ 2016-12-22 6:06 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 3/5] ARM: dts: aspeed: Move mbox under lpc_host node 2016-12-22 6:06 [PATCH v2 0/5] LPC/MBOX work Cyril Bur 2016-12-22 6:06 ` [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication Cyril Bur 2016-12-22 6:06 ` [PATCH v2 2/5] ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap Cyril Bur @ 2016-12-22 6:06 ` Cyril Bur 2016-12-22 6:06 ` [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver Cyril Bur 2016-12-22 6:06 ` [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver Cyril Bur 4 siblings, 0 replies; 28+ messages in thread From: Cyril Bur @ 2016-12-22 6:06 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2016-12-22 6:06 [PATCH v2 0/5] LPC/MBOX work Cyril Bur ` (2 preceding siblings ...) 2016-12-22 6:06 ` [PATCH v2 3/5] ARM: dts: aspeed: Move mbox under lpc_host node Cyril Bur @ 2016-12-22 6:06 ` Cyril Bur 2016-12-23 2:42 ` Andrew Jeffery 2017-01-08 21:39 ` Benjamin Herrenschmidt 2016-12-22 6:06 ` [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver Cyril Bur 4 siblings, 2 replies; 28+ messages in thread From: Cyril Bur @ 2016-12-22 6:06 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2016-12-22 6:06 ` [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver Cyril Bur @ 2016-12-23 2:42 ` Andrew Jeffery 2016-12-23 7:56 ` Cyril Bur 2017-01-08 21:39 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 28+ messages in thread From: Andrew Jeffery @ 2016-12-23 2:42 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2016-12-23 2:42 ` Andrew Jeffery @ 2016-12-23 7:56 ` Cyril Bur 2017-01-03 1:24 ` Andrew Jeffery 0 siblings, 1 reply; 28+ messages in thread From: Cyril Bur @ 2016-12-23 7:56 UTC (permalink / raw) To: Andrew Jeffery, openbmc; +Cc: millerjo 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 */ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2016-12-23 7:56 ` Cyril Bur @ 2017-01-03 1:24 ` Andrew Jeffery 2017-01-08 21:44 ` Benjamin Herrenschmidt 2017-01-08 21:45 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 28+ messages in thread From: Andrew Jeffery @ 2017-01-03 1:24 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-03 1:24 ` Andrew Jeffery @ 2017-01-08 21:44 ` Benjamin Herrenschmidt 2017-01-08 23:54 ` Andrew Jeffery 2017-01-08 21:45 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 28+ messages in thread From: Benjamin Herrenschmidt @ 2017-01-08 21:44 UTC (permalink / raw) To: Andrew Jeffery, Cyril Bur, openbmc; +Cc: millerjo 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-08 21:44 ` Benjamin Herrenschmidt @ 2017-01-08 23:54 ` Andrew Jeffery 0 siblings, 0 replies; 28+ messages in thread From: Andrew Jeffery @ 2017-01-08 23:54 UTC (permalink / raw) To: benh, Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-03 1:24 ` Andrew Jeffery 2017-01-08 21:44 ` Benjamin Herrenschmidt @ 2017-01-08 21:45 ` Benjamin Herrenschmidt 2017-01-09 22:09 ` Cyril Bur 1 sibling, 1 reply; 28+ messages in thread From: Benjamin Herrenschmidt @ 2017-01-08 21:45 UTC (permalink / raw) To: Andrew Jeffery, Cyril Bur, openbmc; +Cc: millerjo 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-08 21:45 ` Benjamin Herrenschmidt @ 2017-01-09 22:09 ` Cyril Bur 2017-01-09 22:55 ` Andrew Jeffery 0 siblings, 1 reply; 28+ messages in thread From: Cyril Bur @ 2017-01-09 22:09 UTC (permalink / raw) To: benh, Andrew Jeffery, openbmc; +Cc: millerjo 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. > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-09 22:09 ` Cyril Bur @ 2017-01-09 22:55 ` Andrew Jeffery 2017-01-09 23:18 ` Cyril Bur ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Andrew Jeffery @ 2017-01-09 22:55 UTC (permalink / raw) To: Cyril Bur, benh, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-09 22:55 ` Andrew Jeffery @ 2017-01-09 23:18 ` Cyril Bur 2017-01-10 0:07 ` Cyril Bur 2017-01-10 4:28 ` Benjamin Herrenschmidt 2 siblings, 0 replies; 28+ messages in thread From: Cyril Bur @ 2017-01-09 23:18 UTC (permalink / raw) To: Andrew Jeffery, benh, openbmc; +Cc: millerjo 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-09 22:55 ` Andrew Jeffery 2017-01-09 23:18 ` Cyril Bur @ 2017-01-10 0:07 ` Cyril Bur 2017-01-10 0:10 ` Andrew Jeffery 2017-01-10 4:28 ` Benjamin Herrenschmidt 2 siblings, 1 reply; 28+ messages in thread From: Cyril Bur @ 2017-01-10 0:07 UTC (permalink / raw) To: Andrew Jeffery, benh, openbmc; +Cc: millerjo 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-10 0:07 ` Cyril Bur @ 2017-01-10 0:10 ` Andrew Jeffery 0 siblings, 0 replies; 28+ messages in thread From: Andrew Jeffery @ 2017-01-10 0:10 UTC (permalink / raw) To: Cyril Bur, benh, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-09 22:55 ` Andrew Jeffery 2017-01-09 23:18 ` Cyril Bur 2017-01-10 0:07 ` Cyril Bur @ 2017-01-10 4:28 ` Benjamin Herrenschmidt 2017-01-10 4:36 ` Cyril Bur 2 siblings, 1 reply; 28+ messages in thread From: Benjamin Herrenschmidt @ 2017-01-10 4:28 UTC (permalink / raw) To: Andrew Jeffery, Cyril Bur, openbmc; +Cc: millerjo 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-10 4:28 ` Benjamin Herrenschmidt @ 2017-01-10 4:36 ` Cyril Bur 2017-01-10 4:40 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 28+ messages in thread From: Cyril Bur @ 2017-01-10 4:36 UTC (permalink / raw) To: benh, Andrew Jeffery, openbmc; +Cc: millerjo 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. > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2017-01-10 4:36 ` Cyril Bur @ 2017-01-10 4:40 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 28+ messages in thread From: Benjamin Herrenschmidt @ 2017-01-10 4:40 UTC (permalink / raw) To: Cyril Bur, Andrew Jeffery, openbmc; +Cc: millerjo 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver 2016-12-22 6:06 ` [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver Cyril Bur 2016-12-23 2:42 ` Andrew Jeffery @ 2017-01-08 21:39 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 28+ messages in thread From: Benjamin Herrenschmidt @ 2017-01-08 21:39 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo 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 */ ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver 2016-12-22 6:06 [PATCH v2 0/5] LPC/MBOX work Cyril Bur ` (3 preceding siblings ...) 2016-12-22 6:06 ` [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver Cyril Bur @ 2016-12-22 6:06 ` Cyril Bur 2016-12-22 23:54 ` Cyril Bur 2016-12-23 2:55 ` Andrew Jeffery 4 siblings, 2 replies; 28+ messages in thread From: Cyril Bur @ 2016-12-22 6:06 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver 2016-12-22 6:06 ` [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver Cyril Bur @ 2016-12-22 23:54 ` Cyril Bur 2016-12-23 2:43 ` Andrew Jeffery 2016-12-23 2:55 ` Andrew Jeffery 1 sibling, 1 reply; 28+ messages in thread From: Cyril Bur @ 2016-12-22 23:54 UTC (permalink / raw) To: openbmc; +Cc: millerjo 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 */ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver 2016-12-22 23:54 ` Cyril Bur @ 2016-12-23 2:43 ` Andrew Jeffery 0 siblings, 0 replies; 28+ messages in thread From: Andrew Jeffery @ 2016-12-23 2:43 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver 2016-12-22 6:06 ` [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver Cyril Bur 2016-12-22 23:54 ` Cyril Bur @ 2016-12-23 2:55 ` Andrew Jeffery 2016-12-23 8:04 ` Cyril Bur 1 sibling, 1 reply; 28+ messages in thread From: Andrew Jeffery @ 2016-12-23 2:55 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver 2016-12-23 2:55 ` Andrew Jeffery @ 2016-12-23 8:04 ` Cyril Bur 2017-01-03 1:33 ` Andrew Jeffery 0 siblings, 1 reply; 28+ messages in thread From: Cyril Bur @ 2016-12-23 8:04 UTC (permalink / raw) To: Andrew Jeffery, openbmc; +Cc: millerjo 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver 2016-12-23 8:04 ` Cyril Bur @ 2017-01-03 1:33 ` Andrew Jeffery 0 siblings, 0 replies; 28+ messages in thread From: Andrew Jeffery @ 2017-01-03 1:33 UTC (permalink / raw) To: Cyril Bur, openbmc; +Cc: millerjo [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-01-10 4:41 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-22 6:06 [PATCH v2 0/5] LPC/MBOX work Cyril Bur 2016-12-22 6:06 ` [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication Cyril Bur 2016-12-23 1:01 ` Andrew Jeffery 2016-12-23 7:50 ` Cyril Bur 2016-12-22 6:06 ` [PATCH v2 2/5] ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap Cyril Bur 2016-12-22 6:06 ` [PATCH v2 3/5] ARM: dts: aspeed: Move mbox under lpc_host node Cyril Bur 2016-12-22 6:06 ` [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver Cyril Bur 2016-12-23 2:42 ` Andrew Jeffery 2016-12-23 7:56 ` Cyril Bur 2017-01-03 1:24 ` Andrew Jeffery 2017-01-08 21:44 ` Benjamin Herrenschmidt 2017-01-08 23:54 ` Andrew Jeffery 2017-01-08 21:45 ` Benjamin Herrenschmidt 2017-01-09 22:09 ` Cyril Bur 2017-01-09 22:55 ` Andrew Jeffery 2017-01-09 23:18 ` Cyril Bur 2017-01-10 0:07 ` Cyril Bur 2017-01-10 0:10 ` Andrew Jeffery 2017-01-10 4:28 ` Benjamin Herrenschmidt 2017-01-10 4:36 ` Cyril Bur 2017-01-10 4:40 ` Benjamin Herrenschmidt 2017-01-08 21:39 ` Benjamin Herrenschmidt 2016-12-22 6:06 ` [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver Cyril Bur 2016-12-22 23:54 ` Cyril Bur 2016-12-23 2:43 ` Andrew Jeffery 2016-12-23 2:55 ` Andrew Jeffery 2016-12-23 8:04 ` Cyril Bur 2017-01-03 1:33 ` Andrew Jeffery
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.