LPC/MBOX work
v2 cover LPC/MBOX work
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
Signature · 1 line
2.11.0
PATCH v2 1/5 ARM: dts: aspeed: Reserve BMC ram for host to BMC communication
--- arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 12 ++++++++++++ arch/arm/boot/dts/aspeed-g4.dtsi | 6 ++++++ 2 files changed, 18 insertions(+)
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +12 -0
@@ -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 {
arch/arm/boot/dts/aspeed-g4.dtsi +6 -0
@@ -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>;
Signature · 1 line
2.11.0
Re: [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication
Hi Cyril,
Quoted 22 lines · Cyril Bur
Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>---
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 12 ++++++++++++ arch/arm/boot/dts/aspeed-g4.dtsi | 6 ++++++2 files changed, 18 insertions(+)arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0
-0@@ -29,6 +29,18 @@ > no-map;Quoted 2 lines
reg = <
0x5f0000000x01000000>; /* 16MB */ };+
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 38
Quoted 6 lines
Quoted 1 line
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +1
-0+ flash_memory: region@54000000 {arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +5
-0+ compatible = "aspeed,ast2400-lpc-ctrl"; + no-map; + reg = <0x54000000 0x04000000>; /* 64M */ + }; + };+
Quoted 5 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 43
Quoted 4 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +4
-0+ flash_buffer@1e789000 { + compatible = "aspeed,ast2400-lpc-ctrl"; + memory-region = <&flash_memory>; + reg = <0x1e789000 0x8>;};
Save this for addition in 2/5?
Quoted 14 lines
leds {arch/arm/boot/dts/aspeed-g4.dtsi +0
-0@@ -870,6 +870,12 @@ > interrupts = <8>; > };Quoted 4 lines
Quoted 1 line
arch/arm/boot/dts/aspeed-g4.dtsi +1
-0+ mbox: mbox@1e789200 {arch/arm/boot/dts/aspeed-g4.dtsi +3
-0+ compatible = "aspeed,ast2400-mbox"; + reg = <0x1e789200 0x5c>; + interrupts = <46>;
arch/arm/boot/dts/aspeed-g4.dtsi
Quoted 1 line
arch/arm/boot/dts/aspeed-g4.dtsi +1
-0+ };
Save this for addition in 3/5?
Or maybe just squash them all together?
Andrew
Quoted 4 lines
+
Quoted 3 lines
Quoted 1 line
wdt1: wdt@1e785000 {
compatible = "aspeed,ast2400-wdt"; reg = <
0x1e7850000x1c>;
Re: [PATCH v2 1/5] ARM: dts: aspeed: Reserve BMC ram for host to BMC communication
Quoted 59 lines · Andrew Jeffery
Hi Cyril,
Quoted 26 lines · Cyril Bur
Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>---
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 12 ++++++++++++ arch/arm/boot/dts/aspeed-g4.dtsi | 6 ++++++2 files changed, 18 insertions(+)arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0
-0@@ -29,6 +29,18 @@ > no-map;Quoted 2 lines
reg = <
0x5f0000000x01000000>; /* 16MB */ };+
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 38
Quoted 7 lines
Quoted 1 line
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +1
-0+ flash_memory: region@54000000 {arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +5
-0+ compatible = "aspeed,ast2400-lpc-ctrl"; + no-map; + reg = <0x54000000 0x04000000>; /* 64M */ + }; + };+
Quoted 6 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 43
Quoted 4 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +4
-0+ flash_buffer@1e789000 { + compatible = "aspeed,ast2400-lpc-ctrl"; + memory-region = <&flash_memory>; + reg = <0x1e789000 0x8>;};
Save this for addition in 2/5?
Quoted 17 lines
leds {arch/arm/boot/dts/aspeed-g4.dtsi +0
-0@@ -870,6 +870,12 @@ > interrupts = <8>; > };Quoted 5 lines
Quoted 1 line
arch/arm/boot/dts/aspeed-g4.dtsi +1
-0+ mbox: mbox@1e789200 {arch/arm/boot/dts/aspeed-g4.dtsi +3
-0+ compatible = "aspeed,ast2400-mbox"; + reg = <0x1e789200 0x5c>; + interrupts = <46>;arch/arm/boot/dts/aspeed-g4.dtsi
Quoted 1 line
arch/arm/boot/dts/aspeed-g4.dtsi +1
-0+ };Save this for addition in 3/5?
Quoted 2 lines
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
Quoted 7 lines
Andrew
Quoted 5 lines
+
Quoted 4 lines
Quoted 1 line
wdt1: wdt@1e785000 {
compatible = "aspeed,ast2400-wdt"; reg = <
0x1e7850000x1c>;
PATCH v2 2/5 ARM: dts: aspeed: Put the lpc_ctrl under lpc_host node for regmap
In reply to Cyril Bur--- 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(-)
arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts +6 -0
@@ -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 {
arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts +6 -0
@@ -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 {
arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts +6 -0
@@ -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 {
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -5
@@ -37,11 +37,6 @@ }; }; - flash_buffer@1e789000 { - compatible = "aspeed,ast2400-lpc-ctrl"; - memory-region = <&flash_memory>; - reg = <0x1e789000 0x8>; - }; leds { compatible = "gpio-leds";
arch/arm/boot/dts/aspeed-g4.dtsi +30 -0
@@ -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;
Signature · 1 line
2.11.0
PATCH v2 3/5 ARM: dts: aspeed: Move mbox under lpc_host node
In reply to Cyril BurThe mbox registers are on the lpc_host bus this should be reflected in the device tree
--- arch/arm/boot/dts/aspeed-g4.dtsi | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
arch/arm/boot/dts/aspeed-g4.dtsi +6 -6
@@ -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>;
Signature · 1 line
2.11.0
PATCH v2 4/5 drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
In reply to Cyril BurThis 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.
--- 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
drivers/mailbox/Kconfig +9 -0
@@ -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
drivers/mailbox/Makefile +3 -0
@@ -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 +
drivers/mailbox/aspeed-mbox.c new file +354 -0
354 lines not on this page · raw message
include/uapi/linux/aspeed-mbox.h new file +23 -0
@@ -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 */
Signature · 1 line
2.11.0
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Hi Cyril,
This looks good to me. A couple of minor comments below.
Quoted 119 lines · Cyril Bur
This provides access to the mbox registers on the ast2400 and ast2500 boards.
An
ioctl()is used to allow users to write to a specific register with a specific value. This is useful as the other end have configured the mbox registers to provide an interrupt when a specific regster gets written to.Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>---
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.ccreate mode 100644include/uapi/linux/aspeed-mbox.hdrivers/mailbox/Kconfig +0
-0@@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOXQuoted 3 lines
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.
drivers/mailbox/Kconfig +2
-0+ +config ASPEED_MBOXdrivers/mailbox/Kconfig config XGENE_SLIMPRO_MBOX · line 134
Quoted 7 lines
drivers/mailbox/Kconfig +7
-0+ 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 registersendif
drivers/mailbox/Makefile +0
-0@@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.oobj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
Quoted 1 line
obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o
+
Quoted 1 line
+obj-$(CONFIG_ASPEED_MBOX) += aspeed-mbox.o
drivers/mailbox/Makefile +1
-0+drivers/mailbox/aspeed-mbox.c new file +31
-0@@ -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> +Quoted 1 line
+#define
DEVICE_NAME"aspeed-mbox"drivers/mailbox/aspeed-mbox.c new file +16
-0+ +#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 {Quoted 6 lines
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+struct miscdevice miscdev; +struct regmap *regmap; +unsigned int base; +int irq; +wait_queue_head_t queue; +struct timer_list poll_timer;drivers/mailbox/aspeed-mbox.c new file +6
-0+}; + +static atomic_t mbox_open_count =ATOMIC_INIT (0 ); + +static u8 mbox_inb (struct mbox *mbox,int reg) +{Quoted 8 lines
drivers/mailbox/aspeed-mbox.c new file +8
-0+/* +* 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);
drivers/mailbox/aspeed-mbox.c line 74
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+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)?
Quoted 84 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static struct mbox *file_mbox (struct file *file) +{Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ return container_of(file->private_data, struct mbox, miscdev);drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_open (struct inode *inode,struct file *file) +{Review
Quoted 1 line
(quoted diff) +1
-0+ struct mbox *mbox = file_mbox(file);+
drivers/mailbox/aspeed-mbox.c line 94
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+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 ; + }+
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+atomic_dec (&mbox_open_count); +return -EBUSY ;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static ssize_t mbox_read(struct file *file, char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
drivers/mailbox/aspeed-mbox.c line 105
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct mbox *mbox =file_mbox (file); +char __user *p = buf; +int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
drivers/mailbox/aspeed-mbox.c line 114
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+if (wait_event_interruptible (mbox->queue, +mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV )) +return -ERESTARTSYS ;+
drivers/mailbox/aspeed-mbox.c line 118
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+for (i =0 ; i <MBOX_NUM_DATA_REGS ; i++) +if (__put_user (mbox_inb (mbox,MBOX_DATA_0 + (i *4 )), p++)) +return -EFAULT ;+
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */ +mbox_outb (mbox,MBOX_CTRL_RECV ,MBOX_BMC_CTRL ); +return p - buf;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static ssize_t mbox_write(struct file *file, const char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
drivers/mailbox/aspeed-mbox.c line 131
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+struct mbox *mbox =file_mbox (file); +const char __user *p = buf; +char c; +int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_READ, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
drivers/mailbox/aspeed-mbox.c line 140
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+for (i =0 ; i <MBOX_NUM_DATA_REGS ; i++) { +if (__get_user (c, p)) +return -EFAULT ;+
drivers/mailbox/aspeed-mbox.c line 144
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+mbox_outb (mbox, c,MBOX_DATA_0 + (i *4 )); + p++; + }+
drivers/mailbox/aspeed-mbox.c line 146
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+mbox_outb (mbox,MBOX_CTRL_SEND ,MBOX_BMC_CTRL );+
Quoted 1 line
(quoted diff) +1
-0+ return p - buf;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static long mbox_ioctl(struct file *file, unsigned int cmd,drivers/mailbox/aspeed-mbox.c line 152
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ unsigned long param)+{
drivers/mailbox/aspeed-mbox.c line 156
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct aspeed_mbox_atn atn; +struct mbox *mbox =file_mbox (file); +void __user *p = (void __user *)param;+
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+switch (cmd) {
drivers/mailbox/aspeed-mbox.c line 159
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+case ASPEED_MBOX_IOCTL_ATN :
I think we should rename the IOCTL as what we do below doesn't necessarily raise an interrupt.
Quoted 224 lines
drivers/mailbox/aspeed-mbox.c new file +1
-0+if (copy_from_user (&atn, p,sizeof (atn)))drivers/mailbox/aspeed-mbox.c line 163
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+return -EFAULT ; +if (atn.reg >15 ) +return -EINVAL ;+
drivers/mailbox/aspeed-mbox.c line 167
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+mbox_outb (mbox, atn.val, atn.reg); +return 0 ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return -EINVAL;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static unsigned int mbox_poll (struct file *file,poll_table *wait) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ struct mbox *mbox = file_mbox(file); + unsigned int mask = 0;+
drivers/mailbox/aspeed-mbox.c line 177
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+poll_wait (file, &mbox->queue, wait);+
drivers/mailbox/aspeed-mbox.c line 180
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV ) + mask |=POLLIN ;+
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+return mask;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_release (struct inode *inode,struct file *file) +{Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+atomic_dec (&mbox_open_count); +return 0 ;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static const struct file_operations mbox_fops = {Quoted 7 lines
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ .owner = THIS_MODULE, + .open = mbox_open, + .read = mbox_read, + .write = mbox_write, + .release = mbox_release, + .poll = mbox_poll, + .unlocked_ioctl = mbox_ioctl,drivers/mailbox/aspeed-mbox.c new file +4
-0+}; + +static void poll_timer (unsigned long data) +{drivers/mailbox/aspeed-mbox.c line 203
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox = (void *)data;+
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox->poll_timer.expires +=msecs_to_jiffies (500 ); +wake_up (&mbox->queue); +add_timer (&mbox->poll_timer);drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static irqreturn_t mbox_irq (int irq,void *arg) +{drivers/mailbox/aspeed-mbox.c line 212
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox = arg;+
drivers/mailbox/aspeed-mbox.c line 215
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (!(mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV )) +return IRQ_NONE ;+
drivers/mailbox/aspeed-mbox.c line 220
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+/* +* Leave the status bit set so that we know the data is for us, +* clear it once it has been read. +*/ +
drivers/mailbox/aspeed-mbox.c line 223
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+/* Mask it off, we'll clear it when we the data gets read */ +mbox_outb (mbox,MBOX_CTRL_MASK ,MBOX_BMC_CTRL );+
Quoted 2 lines
(quoted diff) +2
-0+ wake_up(&mbox->queue); + return IRQ_HANDLED;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static int mbox_config_irq(struct mbox *mbox,drivers/mailbox/aspeed-mbox.c line 230
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ struct platform_device *pdev)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device *dev = &pdev->dev; + int rc;+
drivers/mailbox/aspeed-mbox.c line 237
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox->irq =irq_of_parse_and_map (dev->of_node,0 ); +if (!mbox->irq) +return -ENODEV ;+
drivers/mailbox/aspeed-mbox.c line 245
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 252
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+/* +* 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 */ +
drivers/mailbox/aspeed-mbox.c line 256
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+/* W1C */ +mbox_outb (mbox,0xff ,MBOX_STATUS_0 ); +mbox_outb (mbox,0xff ,MBOX_STATUS_1 );+
Quoted 2 lines
(quoted diff) +2
-0+ mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL); + return 0;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_probe (struct platform_device *pdev) +{drivers/mailbox/aspeed-mbox.c line 266
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct mbox *mbox; +struct device *dev; +int rc;+
drivers/mailbox/aspeed-mbox.c line 269
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (!pdev || !pdev->dev.of_node) +return -ENODEV ;+
drivers/mailbox/aspeed-mbox.c line 272
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ dev = &pdev->dev; +dev_info (dev,"Found mbox host device\n" );+
drivers/mailbox/aspeed-mbox.c line 276
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (!mbox) +return -ENOMEM ;+
drivers/mailbox/aspeed-mbox.c line 278
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+dev_set_drvdata (&pdev->dev, mbox);+
drivers/mailbox/aspeed-mbox.c line 284
Quoted 5 lines
drivers/mailbox/aspeed-mbox.c new file +5
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 292
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 294
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+init_waitqueue_head (&mbox->queue);+
drivers/mailbox/aspeed-mbox.c line 304
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 306
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+mbox_config_irq (mbox, pdev);+
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+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); + }drivers/mailbox/aspeed-mbox.c new file +2
-0+ +out:Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/mailbox/aspeed-mbox.c new file +5
-0+ +} + +static int mbox_remove (struct platform_device *pdev) +{drivers/mailbox/aspeed-mbox.c line 325
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox =dev_get_drvdata (&pdev->dev);+
drivers/mailbox/aspeed-mbox.c line 330
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+misc_deregister (&mbox->miscdev); +if (!mbox->irq) +del_timer_sync (&mbox->poll_timer); + mbox =NULL ;+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static const struct of_device_id mbox_match[] = {Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ { .compatible = "aspeed,ast2400-mbox" }, + { },drivers/mailbox/aspeed-mbox.c new file +3
-0+}; + +static struct platform_driver mbox_driver = {Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +1
-0+ .driver = {Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ .name = DEVICE_NAME,drivers/mailbox/aspeed-mbox.c new file +4
-0+ .of_match_table = mbox_match, + }, + .probe = mbox_probe, + .remove = mbox_remove,drivers/mailbox/aspeed-mbox.c new file +6
-0+}; + +module_platform_driver (mbox_driver); + +MODULE_DEVICE_TABLE (of, mbox_match); +MODULE_LICENSE ("GPL" );Quoted 1 line
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
drivers/mailbox/aspeed-mbox.c new file +1
-0+MODULE_DESCRIPTION ("Linux device interface to the Aspeed MBOX registers" );include/uapi/linux/aspeed-mbox.h new file +15
-0@@ -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 {Quoted 2 lines
include/uapi/linux/aspeed-mbox.h new file +2
-0+uint8_t reg; +uint8_t val;include/uapi/linux/aspeed-mbox.h new file +2
-0+}; +Quoted 2 lines
include/uapi/linux/aspeed-mbox.h new file +2
-0+#define __ASPEED_MBOX_IOCTL_MAGIC 0xb1 +#define ASPEED_MBOX_IOCTL_ATN _IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)include/uapi/linux/aspeed-mbox.h new file +2
-0+ +#endif /* _UAPI_LINUX_MBOX_HOST_H */
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 134 lines · Andrew Jeffery
Hi Cyril,
This looks good to me. A couple of minor comments below.
Quoted 129 lines · Cyril Bur
This provides access to the mbox registers on the ast2400 and ast2500 boards.
An
ioctl()is used to allow users to write to a specific register with a specific value. This is useful as the other end have configured the mbox registers to provide an interrupt when a specific regster gets written to.Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>---
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.ccreate mode 100644include/uapi/linux/aspeed-mbox.hdrivers/mailbox/Kconfig +0
-0@@ -123,4 +123,13 @@ config XGENE_SLIMPRO_MBOXQuoted 3 lines
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.
drivers/mailbox/Kconfig +2
-0+ +config ASPEED_MBOXdrivers/mailbox/Kconfig config XGENE_SLIMPRO_MBOX · line 134
Quoted 7 lines
drivers/mailbox/Kconfig +7
-0+ 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 registersendif
drivers/mailbox/Makefile +0
-0@@ -25,3 +25,6 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.oobj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
Quoted 1 line
obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o
+
Quoted 1 line
+obj-$(CONFIG_ASPEED_MBOX) += aspeed-mbox.o
drivers/mailbox/Makefile +1
-0+drivers/mailbox/aspeed-mbox.c new file +31
-0@@ -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> +Quoted 1 line
+#define
DEVICE_NAME"aspeed-mbox"drivers/mailbox/aspeed-mbox.c new file +16
-0+ +#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 {Quoted 6 lines
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+struct miscdevice miscdev; +struct regmap *regmap; +unsigned int base; +int irq; +wait_queue_head_t queue; +struct timer_list poll_timer;drivers/mailbox/aspeed-mbox.c new file +6
-0+}; + +static atomic_t mbox_open_count =ATOMIC_INIT (0 ); + +static u8 mbox_inb (struct mbox *mbox,int reg) +{Quoted 8 lines
drivers/mailbox/aspeed-mbox.c new file +8
-0+/* +* 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);drivers/mailbox/aspeed-mbox.c new file +2
-0+return val &0xff ; +}
Quoted 8 lines
drivers/mailbox/aspeed-mbox.c line 74
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+ +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 ifregmap_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.
Quoted 105 lines
Quoted 105 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static struct mbox *file_mbox (struct file *file) +{Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ return container_of(file->private_data, struct mbox, miscdev);drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_open (struct inode *inode,struct file *file) +{Review
Quoted 1 line
(quoted diff) +1
-0+ struct mbox *mbox = file_mbox(file);+
drivers/mailbox/aspeed-mbox.c line 94
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+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 ; + }+
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+atomic_dec (&mbox_open_count); +return -EBUSY ;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static ssize_t mbox_read(struct file *file, char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
drivers/mailbox/aspeed-mbox.c line 105
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct mbox *mbox =file_mbox (file); +char __user *p = buf; +int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
drivers/mailbox/aspeed-mbox.c line 114
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+if (wait_event_interruptible (mbox->queue, +mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV )) +return -ERESTARTSYS ;+
drivers/mailbox/aspeed-mbox.c line 118
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+for (i =0 ; i <MBOX_NUM_DATA_REGS ; i++) +if (__put_user (mbox_inb (mbox,MBOX_DATA_0 + (i *4 )), p++)) +return -EFAULT ;+
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step */ +mbox_outb (mbox,MBOX_CTRL_RECV ,MBOX_BMC_CTRL ); +return p - buf;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static ssize_t mbox_write(struct file *file, const char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
drivers/mailbox/aspeed-mbox.c line 131
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+struct mbox *mbox =file_mbox (file); +const char __user *p = buf; +char c; +int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_READ, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
drivers/mailbox/aspeed-mbox.c line 140
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+for (i =0 ; i <MBOX_NUM_DATA_REGS ; i++) { +if (__get_user (c, p)) +return -EFAULT ;+
drivers/mailbox/aspeed-mbox.c line 144
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+mbox_outb (mbox, c,MBOX_DATA_0 + (i *4 )); + p++; + }+
drivers/mailbox/aspeed-mbox.c line 146
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+mbox_outb (mbox,MBOX_CTRL_SEND ,MBOX_BMC_CTRL );+
Quoted 1 line
(quoted diff) +1
-0+ return p - buf;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static long mbox_ioctl(struct file *file, unsigned int cmd,drivers/mailbox/aspeed-mbox.c line 152
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ unsigned long param)+{
drivers/mailbox/aspeed-mbox.c line 156
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct aspeed_mbox_atn atn; +struct mbox *mbox =file_mbox (file); +void __user *p = (void __user *)param;+
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c line 159
Quoted 3 lines
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+switch (cmd) {drivers/mailbox/aspeed-mbox.c new file +1
-0+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
Quoted 268 lines
Quoted 268 lines
drivers/mailbox/aspeed-mbox.c new file +1
-0+if (copy_from_user (&atn, p,sizeof (atn)))drivers/mailbox/aspeed-mbox.c line 163
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+return -EFAULT ; +if (atn.reg >15 ) +return -EINVAL ;+
drivers/mailbox/aspeed-mbox.c line 167
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+mbox_outb (mbox, atn.val, atn.reg); +return 0 ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return -EINVAL;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static unsigned int mbox_poll (struct file *file,poll_table *wait) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ struct mbox *mbox = file_mbox(file); + unsigned int mask = 0;+
drivers/mailbox/aspeed-mbox.c line 177
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+poll_wait (file, &mbox->queue, wait);+
drivers/mailbox/aspeed-mbox.c line 180
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV ) + mask |=POLLIN ;+
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+return mask;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_release (struct inode *inode,struct file *file) +{Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+atomic_dec (&mbox_open_count); +return 0 ;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static const struct file_operations mbox_fops = {Quoted 7 lines
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ .owner = THIS_MODULE, + .open = mbox_open, + .read = mbox_read, + .write = mbox_write, + .release = mbox_release, + .poll = mbox_poll, + .unlocked_ioctl = mbox_ioctl,drivers/mailbox/aspeed-mbox.c new file +4
-0+}; + +static void poll_timer (unsigned long data) +{drivers/mailbox/aspeed-mbox.c line 203
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox = (void *)data;+
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox->poll_timer.expires +=msecs_to_jiffies (500 ); +wake_up (&mbox->queue); +add_timer (&mbox->poll_timer);drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static irqreturn_t mbox_irq (int irq,void *arg) +{drivers/mailbox/aspeed-mbox.c line 212
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox = arg;+
drivers/mailbox/aspeed-mbox.c line 215
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (!(mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV )) +return IRQ_NONE ;+
drivers/mailbox/aspeed-mbox.c line 220
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+/* +* Leave the status bit set so that we know the data is for us, +* clear it once it has been read. +*/ +
drivers/mailbox/aspeed-mbox.c line 223
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+/* Mask it off, we'll clear it when we the data gets read */ +mbox_outb (mbox,MBOX_CTRL_MASK ,MBOX_BMC_CTRL );+
Quoted 2 lines
(quoted diff) +2
-0+ wake_up(&mbox->queue); + return IRQ_HANDLED;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static int mbox_config_irq(struct mbox *mbox,drivers/mailbox/aspeed-mbox.c line 230
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ struct platform_device *pdev)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device *dev = &pdev->dev; + int rc;+
drivers/mailbox/aspeed-mbox.c line 237
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox->irq =irq_of_parse_and_map (dev->of_node,0 ); +if (!mbox->irq) +return -ENODEV ;+
drivers/mailbox/aspeed-mbox.c line 245
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 252
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+/* +* 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 */ +
drivers/mailbox/aspeed-mbox.c line 256
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+/* W1C */ +mbox_outb (mbox,0xff ,MBOX_STATUS_0 ); +mbox_outb (mbox,0xff ,MBOX_STATUS_1 );+
Quoted 2 lines
(quoted diff) +2
-0+ mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL); + return 0;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_probe (struct platform_device *pdev) +{drivers/mailbox/aspeed-mbox.c line 266
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct mbox *mbox; +struct device *dev; +int rc;+
drivers/mailbox/aspeed-mbox.c line 269
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (!pdev || !pdev->dev.of_node) +return -ENODEV ;+
drivers/mailbox/aspeed-mbox.c line 272
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ dev = &pdev->dev; +dev_info (dev,"Found mbox host device\n" );+
drivers/mailbox/aspeed-mbox.c line 276
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (!mbox) +return -ENOMEM ;+
drivers/mailbox/aspeed-mbox.c line 278
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+dev_set_drvdata (&pdev->dev, mbox);+
drivers/mailbox/aspeed-mbox.c line 284
Quoted 5 lines
drivers/mailbox/aspeed-mbox.c new file +5
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 292
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 294
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+init_waitqueue_head (&mbox->queue);+
drivers/mailbox/aspeed-mbox.c line 304
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 306
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+mbox_config_irq (mbox, pdev);+
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+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); + }drivers/mailbox/aspeed-mbox.c new file +2
-0+ +out:Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/mailbox/aspeed-mbox.c new file +5
-0+ +} + +static int mbox_remove (struct platform_device *pdev) +{drivers/mailbox/aspeed-mbox.c line 325
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox =dev_get_drvdata (&pdev->dev);+
drivers/mailbox/aspeed-mbox.c line 330
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+misc_deregister (&mbox->miscdev); +if (!mbox->irq) +del_timer_sync (&mbox->poll_timer); + mbox =NULL ;+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static const struct of_device_id mbox_match[] = {Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ { .compatible = "aspeed,ast2400-mbox" }, + { },drivers/mailbox/aspeed-mbox.c new file +3
-0+}; + +static struct platform_driver mbox_driver = {Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +1
-0+ .driver = {Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ .name = DEVICE_NAME,drivers/mailbox/aspeed-mbox.c new file +4
-0+ .of_match_table = mbox_match, + }, + .probe = mbox_probe, + .remove = mbox_remove,drivers/mailbox/aspeed-mbox.c new file +6
-0+}; + +module_platform_driver (mbox_driver); + +MODULE_DEVICE_TABLE (of, mbox_match); +MODULE_LICENSE ("GPL" );Quoted 1 line
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
drivers/mailbox/aspeed-mbox.c new file +1
-0+MODULE_DESCRIPTION ("Linux device interface to the Aspeed MBOX registers" );include/uapi/linux/aspeed-mbox.h new file +15
-0@@ -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 {Quoted 2 lines
include/uapi/linux/aspeed-mbox.h new file +2
-0+uint8_t reg; +uint8_t val;include/uapi/linux/aspeed-mbox.h new file +2
-0+}; +Quoted 2 lines
include/uapi/linux/aspeed-mbox.h new file +2
-0+#define __ASPEED_MBOX_IOCTL_MAGIC 0xb1 +#define ASPEED_MBOX_IOCTL_ATN _IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)include/uapi/linux/aspeed-mbox.h new file +2
-0+ +#endif /* _UAPI_LINUX_MBOX_HOST_H */
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 12 lines · Cyril Bur
Quoted 10 lines · Andrew Jeffery
drivers/mailbox/aspeed-mbox.c line 74
Quoted 5 lines · Cyril Bur
drivers/mailbox/aspeed-mbox.c new file +4
-0+ +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 ifregmap_write()fails (it shouldn't, but if it does that's extra concerning)?
Quoted 2 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c line 159
Quoted 3 lines
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+switch (cmd) {drivers/mailbox/aspeed-mbox.c new file +1
-0+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
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 5 lines · Andrew Jeffery
regmap_update_bits()will just write back what it read for the untouched bits, which should be 0 in accordance with the datasheet. It's not really a change in behaviour as we will still be writing to reserved bits, but I think it makes the intent clear.
It wastes cycles though...
Cheers,
Ben.
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 5 lines · Andrew Jeffery
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
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
In reply to Andrew JefferyQuoted 7 lines · Andrew Jeffery
Quoted 5 lines
Quoted 3 lines
I think we should rename the IOCTL as what we do below doesn't necessarily raise an interrupt.
Agreed, taking unput :).ASPEED_MBOX_IOCTL_WRITE_BYTE?That suggestion works for me.
If we are going to do that, maybe we should make this a write() at a specific lpos...
Otherwise call it something like SET_ATTN or SET_FLAGS ...
Cheers,
Ben.
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 8 lines · Andrew Jeffery
Quoted 6 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Otherwise call it something like
SET_ATTNorSET_FLAGS...Cheers,
Ben.
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 13 lines · Cyril Bur
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 8 lines · Andrew Jeffery
Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
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
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 18 lines · Andrew Jeffery
Quoted 17 lines · Cyril Bur
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 8 lines · Andrew Jeffery
Quoted 6 lines
Quoted 3 lines
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.
Quoted 3 lines
If we are going to do that, maybe we should make this a
write()at a specific lpos...
Quoted 6 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
2. If lpos is non-zero, assume a single byte write
Sounds good.
Quoted 3 lines
On that, should we be testing the assumptions about buffer sizes? Currently we don't (we use the
MBOX_NUM_DATA_REGSrather than count).
Yeah. Simple checks for your condition 1 and 2.
Quoted 1 line
Andrew
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
In reply to Andrew JefferyQuoted 22 lines · Andrew Jeffery
Quoted 13 lines · Cyril Bur
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 8 lines · Andrew Jeffery
Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
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?
Quoted 4 lines
On that, should we be testing the assumptions about buffer sizes? Currently we don't (we use the
MBOX_NUM_DATA_REGSrather than count).Andrew
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 33 lines · Cyril Bur
Quoted 24 lines · Andrew Jeffery
Quoted 14 lines · Cyril Bur
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 9 lines · Andrew Jeffery
Quoted 7 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 3 lines
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.Quoted 7 lines
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
Quoted 5 lines
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.
Quoted 6 lines
Quoted 6 lines
On that, should we be testing the assumptions about buffer sizes? Currently we don't (we use the
MBOX_NUM_DATA_REGSrather than count).Andrew
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
In reply to Andrew JefferyQuoted 8 lines · Andrew Jeffery
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 writeOn that, should we be testing the assumptions about buffer sizes? Currently we don't (we use the
MBOX_NUM_DATA_REGSrather than count).
I would enforce the lpos is some specific magic value such as 0x1000 for the special write bcs we might want to support partial writes inside the 16 bytes area...
Cheers,
Ben.
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 7 lines · Benjamin Herrenschmidt
Quoted 6 lines · Andrew Jeffery
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
Quoted 6 lines
Quoted 2 lines
On that, should we be testing the assumptions about buffer sizes? Currently we don't (we use the
MBOX_NUM_DATA_REGSrather than count).I would enforce the lpos is some specific magic value such as
0x1000for 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.
Quoted 4 lines
Cheers,
Ben.
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
Quoted 8 lines · Cyril Bur
So, its basically no more work (I think...) to just have partial writes, which... satisfy this requirement no?
Just so we're on the same page - theres nothing 'special' about the write it's just that he other end has mapped that 'data' register to trigger an interrupt... the kernel doesn't know that nor does it need to, the write is like any other, the interrupt gets triggered by hardware.
Ok, that's fine then
Re: [PATCH v2 4/5] drivers/mailbox: Add aspeed ast2400/ast2500 mbox driver
In reply to Cyril BurQuoted 4 lines · Cyril Bur
(quoted diff) +2
-0+ +static ssize_t mbox_read(struct file *file, char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
Quoted 8 lines
drivers/mailbox/aspeed-mbox.c line 105
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct mbox *mbox =file_mbox (file); +char __user *p = buf; +int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);
Why the above ? Isn't that a user-triggerable WARN_ON ?
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +1
-0+ if (wait_event_interruptible(mbox->queue,drivers/mailbox/aspeed-mbox.c line 114
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ mbox_inb(mbox, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)) +return -ERESTARTSYS ;+
Quoted 8 lines
drivers/mailbox/aspeed-mbox.c line 118
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+for (i =0 ; i <MBOX_NUM_DATA_REGS ; i++) +if (__put_user (mbox_inb (mbox,MBOX_DATA_0 + (i *4 )), p++)) +return -EFAULT ;+
drivers/mailbox/aspeed-mbox.c line 122
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+/* 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....)Quoted 17 lines
+static
ssize_tmbox_write(struct file *file, const char__user*buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
drivers/mailbox/aspeed-mbox.c line 131
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+struct mbox *mbox =file_mbox (file); +const char __user *p = buf; +char c; +int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_READ, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
drivers/mailbox/aspeed-mbox.c line 140
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+for (i =0 ; i <MBOX_NUM_DATA_REGS ; i++) { +if (__get_user (c, p)) +return -EFAULT ;+
Quoted 8 lines
drivers/mailbox/aspeed-mbox.c line 144
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+mbox_outb (mbox, c,MBOX_DATA_0 + (i *4 )); + p++; + }+
drivers/mailbox/aspeed-mbox.c line 146
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+mbox_outb (mbox,MBOX_CTRL_SEND ,MBOX_BMC_CTRL );+
Review
Quoted 1 line
(quoted diff) +1
-0+ return p - buf;+}
Do you want to block til the previous one was consumed ?
Quoted 26 lines
+static long mbox_ioctl(struct file *file, unsigned int cmd,
drivers/mailbox/aspeed-mbox.c line 152
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ unsigned long param)+{
drivers/mailbox/aspeed-mbox.c line 156
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct aspeed_mbox_atn atn; +struct mbox *mbox =file_mbox (file); +void __user *p = (void __user *)param;+
drivers/mailbox/aspeed-mbox.c line 163
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+switch (cmd) { +case ASPEED_MBOX_IOCTL_ATN : +if (copy_from_user (&atn, p,sizeof (atn))) +return -EFAULT ; +if (atn.reg >15 ) +return -EINVAL ;+
drivers/mailbox/aspeed-mbox.c line 167
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+mbox_outb (mbox, atn.val, atn.reg); +return 0 ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return -EINVAL;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static unsigned int mbox_poll (struct file *file,poll_table *wait) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ struct mbox *mbox = file_mbox(file); + unsigned int mask = 0;+
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c line 177
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+poll_wait (file, &mbox->queue, wait);+
drivers/mailbox/aspeed-mbox.c line 180
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV ) + mask |=POLLIN ;+
drivers/mailbox/aspeed-mbox.c line 182
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+return mask;+}
What about a POLLOUT too in order to know that the write is consumed ? (MBOX_CTRL_SEND is clear)
Quoted 199 lines
(quoted diff) +2
-0+static int mbox_release(struct inode *inode, struct file *file) +{Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+atomic_dec (&mbox_open_count); +return 0 ;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static const struct file_operations mbox_fops = {Quoted 7 lines
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ .owner = THIS_MODULE, + .open = mbox_open, + .read = mbox_read, + .write = mbox_write, + .release = mbox_release, + .poll = mbox_poll, + .unlocked_ioctl = mbox_ioctl,drivers/mailbox/aspeed-mbox.c new file +4
-0+}; + +static void poll_timer (unsigned long data) +{drivers/mailbox/aspeed-mbox.c line 203
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox = (void *)data;+
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox->poll_timer.expires +=msecs_to_jiffies (500 ); +wake_up (&mbox->queue); +add_timer (&mbox->poll_timer);drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static irqreturn_t mbox_irq (int irq,void *arg) +{drivers/mailbox/aspeed-mbox.c line 212
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox = arg;+
drivers/mailbox/aspeed-mbox.c line 215
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (!(mbox_inb (mbox,MBOX_BMC_CTRL ) &MBOX_CTRL_RECV )) +return IRQ_NONE ;+
drivers/mailbox/aspeed-mbox.c line 220
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+/* +* Leave the status bit set so that we know the data is for us, +* clear it once it has been read. +*/ +
drivers/mailbox/aspeed-mbox.c line 223
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+/* Mask it off, we'll clear it when we the data gets read */ +mbox_outb (mbox,MBOX_CTRL_MASK ,MBOX_BMC_CTRL );+
Quoted 2 lines
(quoted diff) +2
-0+ wake_up(&mbox->queue); + return IRQ_HANDLED;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static int mbox_config_irq(struct mbox *mbox,drivers/mailbox/aspeed-mbox.c line 230
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ struct platform_device *pdev)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device *dev = &pdev->dev; + int rc;+
drivers/mailbox/aspeed-mbox.c line 237
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox->irq =irq_of_parse_and_map (dev->of_node,0 ); +if (!mbox->irq) +return -ENODEV ;+
drivers/mailbox/aspeed-mbox.c line 245
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 252
Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +6
-0+/* +* 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 */ +
drivers/mailbox/aspeed-mbox.c line 256
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+/* W1C */ +mbox_outb (mbox,0xff ,MBOX_STATUS_0 ); +mbox_outb (mbox,0xff ,MBOX_STATUS_1 );+
Quoted 2 lines
(quoted diff) +2
-0+ mbox_outb(mbox, MBOX_CTRL_RECV, MBOX_BMC_CTRL); + return 0;drivers/mailbox/aspeed-mbox.c new file +4
-0+} + +static int mbox_probe (struct platform_device *pdev) +{drivers/mailbox/aspeed-mbox.c line 266
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+struct mbox *mbox; +struct device *dev; +int rc;+
drivers/mailbox/aspeed-mbox.c line 269
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+if (!pdev || !pdev->dev.of_node) +return -ENODEV ;+
drivers/mailbox/aspeed-mbox.c line 272
Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ dev = &pdev->dev; +dev_info (dev,"Found mbox host device\n" );+
drivers/mailbox/aspeed-mbox.c line 276
Quoted 3 lines
drivers/mailbox/aspeed-mbox.c new file +3
-0+ mbox =devm_kzalloc (dev,sizeof (*mbox),GFP_KERNEL ); +if (!mbox) +return -ENOMEM ;+
drivers/mailbox/aspeed-mbox.c line 278
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+dev_set_drvdata (&pdev->dev, mbox);+
drivers/mailbox/aspeed-mbox.c line 284
Quoted 5 lines
drivers/mailbox/aspeed-mbox.c new file +5
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 292
Quoted 7 lines
drivers/mailbox/aspeed-mbox.c new file +7
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 294
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+init_waitqueue_head (&mbox->queue);+
drivers/mailbox/aspeed-mbox.c line 304
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+ 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; + }+
drivers/mailbox/aspeed-mbox.c line 306
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+mbox_config_irq (mbox, pdev);+
Quoted 9 lines
drivers/mailbox/aspeed-mbox.c new file +9
-0+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); + }drivers/mailbox/aspeed-mbox.c new file +2
-0+ +out:Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/mailbox/aspeed-mbox.c new file +5
-0+ +} + +static int mbox_remove (struct platform_device *pdev) +{drivers/mailbox/aspeed-mbox.c line 325
Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+struct mbox *mbox =dev_get_drvdata (&pdev->dev);+
drivers/mailbox/aspeed-mbox.c line 330
Quoted 4 lines
drivers/mailbox/aspeed-mbox.c new file +4
-0+misc_deregister (&mbox->miscdev); +if (!mbox->irq) +del_timer_sync (&mbox->poll_timer); + mbox =NULL ;+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/mailbox/aspeed-mbox.c new file +3
-0+} + +static const struct of_device_id mbox_match[] = {Quoted 2 lines
drivers/mailbox/aspeed-mbox.c new file +2
-0+ { .compatible = "aspeed,ast2400-mbox" }, + { },drivers/mailbox/aspeed-mbox.c new file +3
-0+}; + +static struct platform_driver mbox_driver = {Quoted 6 lines
drivers/mailbox/aspeed-mbox.c new file +1
-0+ .driver = {Quoted 1 line
drivers/mailbox/aspeed-mbox.c new file +1
-0+ .name = DEVICE_NAME,drivers/mailbox/aspeed-mbox.c new file +4
-0+ .of_match_table = mbox_match, + }, + .probe = mbox_probe, + .remove = mbox_remove,drivers/mailbox/aspeed-mbox.c new file +6
-0+}; + +module_platform_driver (mbox_driver); + +MODULE_DEVICE_TABLE (of, mbox_match); +MODULE_LICENSE ("GPL" );Quoted 1 line
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
drivers/mailbox/aspeed-mbox.c new file +1
-0+MODULE_DESCRIPTION ("Linux device interface to the Aspeed MBOX registers" );include/uapi/linux/aspeed-mbox.h new file +15
-0@@ -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 {Quoted 2 lines
include/uapi/linux/aspeed-mbox.h new file +2
-0+uint8_t reg; +uint8_t val;include/uapi/linux/aspeed-mbox.h new file +2
-0+}; +Quoted 2 lines
include/uapi/linux/aspeed-mbox.h new file +2
-0+#define __ASPEED_MBOX_IOCTL_MAGIC 0xb1 +#define ASPEED_MBOX_IOCTL_ATN _IOW(__ASPEED_MBOX_IOCTL_MAGIC, 0x00, struct aspeed_mbox_atn)include/uapi/linux/aspeed-mbox.h new file +2
-0+ +#endif /* _UAPI_LINUX_MBOX_HOST_H */
PATCH v2 5/5 drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
In reply to Cyril BurThis 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
--- 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
drivers/misc/Kconfig +9 -0
@@ -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"
drivers/misc/Makefile +1 -0
@@ -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
drivers/misc/aspeed-lpc-ctrl.c new file +292 -0
292 lines not on this page · raw message
include/uapi/linux/aspeed-lpc-ctrl.h new file +25 -0
@@ -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 */
Signature · 1 line
2.11.0
Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
Quoted 229 lines · Cyril Bur
This driver exposes a reserved chunk of BMC ram to userspace as well as an ioctl interface to control the BMC<->HOST mapping of the LPC bus. This allows for a communication channel between the BMC and the host
Signed-off-by Cyril Bur <[email protected]>--- 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.hdrivers/misc/Kconfig +9
-0@@ -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"drivers/misc/Makefile +1
-0@@ -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.odrivers/misc/aspeed-lpc-ctrl.c new file +180
-0@@ -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;
drivers/misc/aspeed-lpc-ctrl.c line 190
Quoted 10 lines
drivers/misc/aspeed-lpc-ctrl.c new file +10
-0+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?
Quoted 133 lines
drivers/misc/aspeed-lpc-ctrl.c new file +102
-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 <[email protected]>" ); +MODULE_DESCRIPTION ("Linux device interface to control LPC bus" );include/uapi/linux/aspeed-lpc-ctrl.h new file +25
-0@@ -0,0 +1,25 @@ +/* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License +* as published by the Free Software Foundation; either version +* 2 of the License, or (at your option) any later version. +*/ + +#ifndef _UAPI_LINUX_LPC_CTRL_H +#define _UAPI_LINUX_LPC_CTRL_H + +#include <linux/ioctl.h> + +struct lpc_mapping { +uint32_t hostaddr; +uint32_t size; +}; + +#define __LPC_CTRL_IOCTL_MAGIC 0xb2 +#define LPC_CTRL_IOCTL_SIZE _IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t) +#define LPC_CTRL_IOCTL_MAP _IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping) +#define LPC_CTRL_IOCTL_UNMAP _IO(__LPC_CTRL_IOCTL_MAGIC, 0x02) + +#endif /* _UAPI_LINUX_LPC_CTRL_H */
Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
Quoted 5 lines · Cyril Bur
Quoted 1 line
Quoted 1 line
+ 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
Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
In reply to Cyril BurQuoted 22 lines · Cyril Bur
This driver exposes a reserved chunk of BMC ram to userspace as well as an ioctl interface to control the BMC<->HOST mapping of the LPC bus. This allows for a communication channel between the BMC and the host
Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>---
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.ccreate mode 100644include/uapi/linux/aspeed-lpc-ctrl.hdrivers/misc/Kconfig +0
-0@@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGEQuoted 2 lines
An empty message will only clear the display at driver init time. Any other printf()-formatted message is valid with newline and escape codes.
drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 808
Quoted 2 lines
drivers/misc/Kconfig +2
-0+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
Quoted 143 lines
drivers/misc/Kconfig +1
-0+ bool "Build a driver to control the BMC to HOST LPC bus"drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 814
Quoted 5 lines
drivers/misc/Kconfig +5
-0+ 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"drivers/misc/Makefile +0
-0Quoted 3 lines
(quoted diff) +0
-0@@ -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
Quoted 1 line
+obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
drivers/misc/aspeed-lpc-ctrl.c new file +33
-0@@ -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> +Quoted 1 line
+#define
DEVICE_NAME"aspeed-lpc-ctrl"drivers/misc/aspeed-lpc-ctrl.c new file +5
-0+ +#define HICR7 0x8 +#define HICR8 0xc + +struct lpc_ctrl {Quoted 6 lines
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+struct miscdevice miscdev; +struct regmap *regmap; +phys_addr_t base; +resource_size_t size; +uint32_t pnor_size; +uint32_t pnor_base;drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+}; + +static atomic_t lpc_ctrl_open_count =ATOMIC_INIT (0 ); + +static struct lpc_ctrl *file_lpc_ctrl (struct file *file) +{Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ return container_of(file->private_data, struct lpc_ctrl, miscdev);drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_mmap (struct file *file,struct vm_area_struct *vma) +{drivers/misc/aspeed-lpc-ctrl.c line 58
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+struct lpc_ctrl *lpc_ctrl =file_lpc_ctrl (file); +unsigned long vsize = vma->vm_end - vma->vm_start;+
drivers/misc/aspeed-lpc-ctrl.c line 61
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size) +return -EINVAL ;+
drivers/misc/aspeed-lpc-ctrl.c line 63
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+/* Other checks? */ +
drivers/misc/aspeed-lpc-ctrl.c line 68
Quoted 4 lines
drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+if (remap_pfn_range (vma, vma->vm_start, + (lpc_ctrl->base >>PAGE_SHIFT ) + vma->vm_pgoff, + vsize, vma->vm_page_prot)) +return -EAGAIN ;+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_open (struct inode *inode,struct file *file) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ if (atomic_inc_return(&lpc_ctrl_open_count) == 1) + return 0;+
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+atomic_dec (&lpc_ctrl_open_count); +return -EBUSY ;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
Quoted 1 line
(quoted diff) +1
-0+ return -EPERM;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_READ, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
Quoted 1 line
(quoted diff) +1
-0+ return -EPERM;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,drivers/misc/aspeed-lpc-ctrl.c line 105
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ unsigned long param)+{
drivers/misc/aspeed-lpc-ctrl.c line 110
Quoted 4 lines
drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+long rc; +struct lpc_mapping map; +struct lpc_ctrl *lpc_ctrl =file_lpc_ctrl (file); +void __user *p = (void __user *)param;+
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+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 ;
drivers/misc/aspeed-lpc-ctrl.c
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +
Whitespace (double line break).
drivers/misc/aspeed-lpc-ctrl.c
Quoted 13 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+/* Quoted 11 lines
drivers/misc/aspeed-lpc-ctrl.c new file +11
-0+ * 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.drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+*/
If only we could put this comment into the datasheet and re-publish it.
drivers/misc/aspeed-lpc-ctrl.c line 138
Quoted 5 lines
+
Quoted 3 lines
drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+regmap_write (lpc_ctrl->regmap,HICR7 , + (lpc_ctrl->base | (map.hostaddr >>16 ))); + regmap_write(lpc_ctrl->regmap, HICR8,drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ (~(map.size - 1)) | ((map.size >> 16) - 1));
What are your thoughts on error handling
drivers/misc/aspeed-lpc-ctrl.c
Quoted 9 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+return 0 ;drivers/misc/aspeed-lpc-ctrl.c line 144
Quoted 5 lines
drivers/misc/aspeed-lpc-ctrl.c new file +5
-0+case LPC_CTRL_IOCTL_UNMAP : +/* +* The top nibble in host lpc addresses references which +* firmware space, use space zero hence the & 0x0fff +*/ +
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ dev_info(lpc_ctrl->miscdev.parent, "Setting HICR7 to 0x%08x\n",drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ 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.
Quoted 76 lines
+ rc = regmap_write(lpc_ctrl->regmap, HICR7,
drivers/misc/aspeed-lpc-ctrl.c line 157
Quoted 9 lines
drivers/misc/aspeed-lpc-ctrl.c new file +9
-0+ 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; + }+
Quoted 1 line
(quoted diff) +1
-0+ return -EINVAL;drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_release (struct inode *inode,struct file *file) +{Quoted 2 lines
(quoted diff) +2
-0+ atomic_dec(&lpc_ctrl_open_count); + return 0;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static const struct file_operations lpc_ctrl_fops = {Quoted 7 lines
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+ .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,drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+}; + +static int lpc_ctrl_probe (struct platform_device *pdev) +{drivers/misc/aspeed-lpc-ctrl.c line 185
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+struct lpc_ctrl *lpc_ctrl; +struct device *dev; +struct device_node *node; +struct resource resm; +struct mtd_info *mtd; +int rc;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!pdev || !pdev->dev.of_node) + return -ENODEV;+
drivers/misc/aspeed-lpc-ctrl.c line 194
Quoted 5 lines
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ mtd =get_mtd_device_nm ("1e630000.spi:pnor@0" );drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+if (IS_ERR (mtd)) { +dev_err (&pdev->dev,"Couldn't find pnor\n" ); +return -EPROBE_DEFER ; + }+
drivers/misc/aspeed-lpc-ctrl.c line 196
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ dev = &pdev->dev;+
drivers/misc/aspeed-lpc-ctrl.c line 200
Quoted 3 lines
drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+ lpc_ctrl =devm_kzalloc (dev,sizeof (*lpc_ctrl),GFP_KERNEL ); +if (!lpc_ctrl) +return -ENOMEM ;+
drivers/misc/aspeed-lpc-ctrl.c line 202
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+dev_set_drvdata (&pdev->dev, lpc_ctrl);+
drivers/misc/aspeed-lpc-ctrl.c line 213
Quoted 10 lines
drivers/misc/aspeed-lpc-ctrl.c new file +10
-0+ 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;+
drivers/misc/aspeed-lpc-ctrl.c line 216
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+dev_info (dev,"Host PNOR base: 0x%08x size: 0x%08x\n" , + lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);+
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+ node =of_parse_phandle (dev->of_node,"memory-region" ,0 ); +if (!node) { +/* +* Should probaby handle this by allocating 4-64k now and +* using that +*/
drivers/misc/aspeed-lpc-ctrl.c line 224
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+dev_err (dev,"Didn't find reserved memory\n" );
I think this is good enough for the moment?
Quoted 41 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ rc = -EINVAL ;Review
Quoted 2 lines
(quoted diff) +2
-0+ goto out; + }+
drivers/misc/aspeed-lpc-ctrl.c line 235
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+ 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; + }+
drivers/misc/aspeed-lpc-ctrl.c line 238
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ lpc_ctrl->size =resource_size (&resm); + lpc_ctrl->base = resm.start;+
drivers/misc/aspeed-lpc-ctrl.c line 246
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+ 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; + }+
Quoted 10 lines
drivers/misc/aspeed-lpc-ctrl.c new file +10
-0+ 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);drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +out:Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_remove (struct platform_device *pdev) +{Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+struct lpc_ctrl *lpc_ctrl =dev_get_drvdata (&pdev->dev);
drivers/misc/aspeed-lpc-ctrl.c line 267
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +misc_deregister (&lpc_ctrl->miscdev);
It feels like there should be a devm_misc_register()...
drivers/misc/aspeed-lpc-ctrl.c line 274
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ lpc_ctrl =NULL ; +Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/misc/aspeed-lpc-ctrl.c new file +4
-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.
Quoted 47 lines
drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+ { }, +}; + +static struct platform_driver lpc_ctrl_driver = {Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ .driver = {Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ .name = DEVICE_NAME,drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+ .of_match_table = lpc_ctrl_match, + }, + .probe = lpc_ctrl_probe, + .remove = lpc_ctrl_remove,drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+}; + +module_platform_driver (lpc_ctrl_driver); + +MODULE_DEVICE_TABLE (of, lpc_ctrl_match); +MODULE_LICENSE ("GPL" );Quoted 1 line
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+MODULE_DESCRIPTION ("Linux device interface to control LPC bus" );include/uapi/linux/aspeed-lpc-ctrl.h new file +15
-0@@ -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 {Quoted 2 lines
include/uapi/linux/aspeed-lpc-ctrl.h new file +2
-0+uint32_t hostaddr; +uint32_t size;include/uapi/linux/aspeed-lpc-ctrl.h new file +2
-0+}; +Quoted 4 lines
include/uapi/linux/aspeed-lpc-ctrl.h new file +4
-0+#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)
include/uapi/linux/aspeed-lpc-ctrl.h line 25
Quoted 2 lines
include/uapi/linux/aspeed-lpc-ctrl.h new file +2
-0+ +#endif /* _UAPI_LINUX_LPC_CTRL_H */
Cheers,
Andrew
Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
Quoted 24 lines · Andrew Jeffery
Quoted 23 lines · Cyril Bur
This driver exposes a reserved chunk of BMC ram to userspace as well as an ioctl interface to control the BMC<->HOST mapping of the LPC bus. This allows for a communication channel between the BMC and the host
Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>---
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.ccreate mode 100644include/uapi/linux/aspeed-lpc-ctrl.hdrivers/misc/Kconfig +0
-0@@ -804,6 +804,15 @@ config PANEL_BOOT_MESSAGEQuoted 2 lines
An empty message will only clear the display at driver init time. Any other printf()-formatted message is valid with newline and escape codes.
Quoted 9 lines
drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 808
Quoted 3 lines
drivers/misc/Kconfig +2
-0+config ASPEED_LPC_CTRL + depends on ARCH_ASPEED || COMPILE_TESTSomething 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
Quoted 159 lines
Quoted 159 lines
drivers/misc/Kconfig +1
-0+ bool "Build a driver to control the BMC to HOST LPC bus"drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 814
Quoted 5 lines
drivers/misc/Kconfig +5
-0+ 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"drivers/misc/Makefile +0
-0Quoted 3 lines
(quoted diff) +0
-0@@ -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
Quoted 1 line
+obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
drivers/misc/aspeed-lpc-ctrl.c new file +33
-0@@ -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> +Quoted 1 line
+#define
DEVICE_NAME"aspeed-lpc-ctrl"drivers/misc/aspeed-lpc-ctrl.c new file +5
-0+ +#define HICR7 0x8 +#define HICR8 0xc + +struct lpc_ctrl {Quoted 6 lines
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+struct miscdevice miscdev; +struct regmap *regmap; +phys_addr_t base; +resource_size_t size; +uint32_t pnor_size; +uint32_t pnor_base;drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+}; + +static atomic_t lpc_ctrl_open_count =ATOMIC_INIT (0 ); + +static struct lpc_ctrl *file_lpc_ctrl (struct file *file) +{Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ return container_of(file->private_data, struct lpc_ctrl, miscdev);drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_mmap (struct file *file,struct vm_area_struct *vma) +{drivers/misc/aspeed-lpc-ctrl.c line 58
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+struct lpc_ctrl *lpc_ctrl =file_lpc_ctrl (file); +unsigned long vsize = vma->vm_end - vma->vm_start;+
drivers/misc/aspeed-lpc-ctrl.c line 61
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size) +return -EINVAL ;+
drivers/misc/aspeed-lpc-ctrl.c line 63
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+/* Other checks? */ +
drivers/misc/aspeed-lpc-ctrl.c line 68
Quoted 4 lines
drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+if (remap_pfn_range (vma, vma->vm_start, + (lpc_ctrl->base >>PAGE_SHIFT ) + vma->vm_pgoff, + vsize, vma->vm_page_prot)) +return -EAGAIN ;+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_open (struct inode *inode,struct file *file) +{Review
Quoted 2 lines
(quoted diff) +2
-0+ if (atomic_inc_return(&lpc_ctrl_open_count) == 1) + return 0;+
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+atomic_dec (&lpc_ctrl_open_count); +return -EBUSY ;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
Quoted 1 line
(quoted diff) +1
-0+ return -EPERM;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,Review
Quoted 1 line
(quoted diff) +1
-0+ size_t count, loff_t *ppos)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!access_ok(VERIFY_READ, buf, count)) + return -EFAULT;+
Review
Quoted 1 line
(quoted diff) +1
-0+ WARN_ON(*ppos);+
Quoted 1 line
(quoted diff) +1
-0+ return -EPERM;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,drivers/misc/aspeed-lpc-ctrl.c line 105
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ unsigned long param)+{
drivers/misc/aspeed-lpc-ctrl.c line 110
Quoted 4 lines
drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+long rc; +struct lpc_mapping map; +struct lpc_ctrl *lpc_ctrl =file_lpc_ctrl (file); +void __user *p = (void __user *)param;+
Quoted 13 lines
drivers/misc/aspeed-lpc-ctrl.c
Quoted 10 lines
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+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 ;drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +Whitespace (double line break).
Thanks
Quoted 17 lines
Quoted 13 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+/* Quoted 11 lines
drivers/misc/aspeed-lpc-ctrl.c new file +11
-0+ * 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.drivers/misc/aspeed-lpc-ctrl.c
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+*/ If only we could put this comment into the datasheet and re-publish it.
Glad it makes sense!
Quoted 9 lines
drivers/misc/aspeed-lpc-ctrl.c line 138
Quoted 6 lines
+
Quoted 3 lines
drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+regmap_write (lpc_ctrl->regmap,HICR7 , + (lpc_ctrl->base | (map.hostaddr >>16 ))); + regmap_write(lpc_ctrl->regmap, HICR8,drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ (~(map.size - 1)) | ((map.size >> 16) - 1));What are your thoughts on error handling
I'll do the same as unmap.
Quoted 8 lines
Quoted 8 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+return 0 ;drivers/misc/aspeed-lpc-ctrl.c line 144
Quoted 5 lines
drivers/misc/aspeed-lpc-ctrl.c new file +5
-0+case LPC_CTRL_IOCTL_UNMAP : +/* +* The top nibble in host lpc addresses references which +* firmware space, use space zero hence the & 0x0fff +*/ +
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c
Quoted 3 lines
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ dev_info(lpc_ctrl->miscdev.parent, "Setting HICR7 to 0x%08x\n",drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ 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.
Quoted 84 lines
Quoted 84 lines
+ rc = regmap_write(lpc_ctrl->regmap, HICR7,
drivers/misc/aspeed-lpc-ctrl.c line 157
Quoted 9 lines
drivers/misc/aspeed-lpc-ctrl.c new file +9
-0+ 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; + }+
Quoted 1 line
(quoted diff) +1
-0+ return -EINVAL;drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_release (struct inode *inode,struct file *file) +{Quoted 2 lines
(quoted diff) +2
-0+ atomic_dec(&lpc_ctrl_open_count); + return 0;drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+} + +static const struct file_operations lpc_ctrl_fops = {Quoted 7 lines
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+ .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,drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+}; + +static int lpc_ctrl_probe (struct platform_device *pdev) +{drivers/misc/aspeed-lpc-ctrl.c line 185
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+struct lpc_ctrl *lpc_ctrl; +struct device *dev; +struct device_node *node; +struct resource resm; +struct mtd_info *mtd; +int rc;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!pdev || !pdev->dev.of_node) + return -ENODEV;+
drivers/misc/aspeed-lpc-ctrl.c line 194
Quoted 6 lines
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ mtd =get_mtd_device_nm ("1e630000.spi:pnor@0" );drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+if (IS_ERR (mtd)) { +dev_err (&pdev->dev,"Couldn't find pnor\n" ); +return -EPROBE_DEFER ; + }+
drivers/misc/aspeed-lpc-ctrl.c line 196
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ dev = &pdev->dev;+
drivers/misc/aspeed-lpc-ctrl.c line 200
Quoted 3 lines
drivers/misc/aspeed-lpc-ctrl.c new file +3
-0+ lpc_ctrl =devm_kzalloc (dev,sizeof (*lpc_ctrl),GFP_KERNEL ); +if (!lpc_ctrl) +return -ENOMEM ;+
drivers/misc/aspeed-lpc-ctrl.c line 202
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+dev_set_drvdata (&pdev->dev, lpc_ctrl);+
drivers/misc/aspeed-lpc-ctrl.c line 213
Quoted 10 lines
drivers/misc/aspeed-lpc-ctrl.c new file +10
-0+ 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;+
drivers/misc/aspeed-lpc-ctrl.c line 216
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+dev_info (dev,"Host PNOR base: 0x%08x size: 0x%08x\n" , + lpc_ctrl->pnor_base, lpc_ctrl->pnor_size);+
Quoted 11 lines
drivers/misc/aspeed-lpc-ctrl.c line 224
Quoted 8 lines
Quoted 6 lines
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+ node =of_parse_phandle (dev->of_node,"memory-region" ,0 ); +if (!node) { +/* +* Should probaby handle this by allocating 4-64k now and +* using that +*/ drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+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.
Quoted 46 lines
Quoted 46 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ rc = -EINVAL ;Review
Quoted 2 lines
(quoted diff) +2
-0+ goto out; + }+
drivers/misc/aspeed-lpc-ctrl.c line 235
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+ 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; + }+
drivers/misc/aspeed-lpc-ctrl.c line 238
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ lpc_ctrl->size =resource_size (&resm); + lpc_ctrl->base = resm.start;+
drivers/misc/aspeed-lpc-ctrl.c line 246
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +7
-0+ 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; + }+
Quoted 10 lines
drivers/misc/aspeed-lpc-ctrl.c new file +10
-0+ 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);drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +out:Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+} + +static int lpc_ctrl_remove (struct platform_device *pdev) +{
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c line 267
Quoted 4 lines
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+struct lpc_ctrl *lpc_ctrl =dev_get_drvdata (&pdev->dev);drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +misc_deregister (&lpc_ctrl->miscdev);It feels like there should be a
devm_misc_register()...
Uh sorry?
Quoted 12 lines
drivers/misc/aspeed-lpc-ctrl.c line 274
Quoted 8 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ lpc_ctrl =NULL ; +Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/misc/aspeed-lpc-ctrl.c new file +4
-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.
Quoted 2 lines
It's probably also worth adding the AST2500.
True,
Thanks,
Cyril
Quoted 58 lines
Quoted 52 lines
drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+ { }, +}; + +static struct platform_driver lpc_ctrl_driver = {Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ .driver = {Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ .name = DEVICE_NAME,drivers/misc/aspeed-lpc-ctrl.c new file +4
-0+ .of_match_table = lpc_ctrl_match, + }, + .probe = lpc_ctrl_probe, + .remove = lpc_ctrl_remove,drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+}; + +module_platform_driver (lpc_ctrl_driver); + +MODULE_DEVICE_TABLE (of, lpc_ctrl_match); +MODULE_LICENSE ("GPL" );Quoted 1 line
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+MODULE_DESCRIPTION ("Linux device interface to control LPC bus" );include/uapi/linux/aspeed-lpc-ctrl.h new file +15
-0@@ -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 {Quoted 2 lines
include/uapi/linux/aspeed-lpc-ctrl.h new file +2
-0+uint32_t hostaddr; +uint32_t size;include/uapi/linux/aspeed-lpc-ctrl.h new file +2
-0+}; +Quoted 4 lines
include/uapi/linux/aspeed-lpc-ctrl.h new file +4
-0+#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)include/uapi/linux/aspeed-lpc-ctrl.h line 25
Quoted 2 lines
include/uapi/linux/aspeed-lpc-ctrl.h new file +2
-0+ +#endif /* _UAPI_LINUX_LPC_CTRL_H */ Cheers,
Andrew
Re: [PATCH v2 5/5] drivers/misc: Add aspeed ast2400/ast2500 lpc controlling driver
Quoted 9 lines · Cyril Bur
Quoted 8 lines · Andrew Jeffery
Quoted 7 lines · Cyril Bur
drivers/misc/aspeed-lpc-ctrl.c new file +6
-0+ node =of_parse_phandle (dev->of_node,"memory-region" ,0 ); +if (!node) { +/* +* Should probaby handle this by allocating 4-64k now and +* using that +*/
Quoted 8 lines
Quoted 4 lines
drivers/misc/aspeed-lpc-ctrl.c line 224
Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+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.
Quoted 11 lines
Quoted 10 lines
drivers/misc/aspeed-lpc-ctrl.c line 267
Quoted 7 lines
drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+static int lpc_ctrl_remove (struct platform_device *pdev) +{Quoted 1 line
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+struct lpc_ctrl *lpc_ctrl =dev_get_drvdata (&pdev->dev);drivers/misc/aspeed-lpc-ctrl.c new file +2
-0+ +misc_deregister (&lpc_ctrl->miscdev);It feels like there should be a
devm_misc_register()...
Quoted 1 line
Uh sorry?
Just a passing comment on the misc_* interfaces. Nothing to worry about.
Quoted 13 lines
Quoted 13 lines
drivers/misc/aspeed-lpc-ctrl.c line 274
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c line 268
Quoted 2 lines
drivers/misc/aspeed-lpc-ctrl.c new file +1
-0+ lpc_ctrl =NULL ;+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/misc/aspeed-lpc-ctrl.c new file +4
-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.
Quoted 4 lines
Quoted 2 lines
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