LPC/MBOX work

PATCH Superseded Cyril Bur 4 participants 22 messages Dec 9, 2016 – Dec 18, 2016

Subsystems (3)

Device tree bindings Char and misc drivers Arm/Aspeed machine support

lore

Share & tools

lore

Cyril Bur

author

2016-12-09 05:43 UTC #0 lore raw

cover LPC/MBOX work

Hello all,

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

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

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

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

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

Thanks,

Cyril

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

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

Signature · 1 line

2.10.2

Cyril Bur

author

2016-12-09 05:43 UTC #1 lore raw

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

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

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

Signed-off-by Cyril Bur <[email protected]>

---
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 25 ++++++++++++++++++++++++-
 arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +24 -1

index acaceda2..e810ae7 100644
@@ -17,7 +17,30 @@
 	};
 
 	memory {
-		reg = <0x40000000 0x10000000>;
+		reg = <0x40000000 0x20000000>;
+	};
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells= <1>;
+		ranges;
+
+		flash_mem: region@52000000 {
+			compatible = "aspeed,lpc-ctrl";
+			no-map;
+			reg = <0x54000000 0x04000000>; /* 64m */
+		};
+
+		fb_mem: framebuffer@5f000000 {
+			no-map;
+			reg = <0x5f000000 0x01000000>; /* 16m */
+		};
+	};
+
+	flash_buffer {
+		compatible = "aspeed,lpc-ctrl";
+		memory-region = <&flash_mem>;
+		reg = <0x1e789000 8>;
 	};
 
         leds {

arch/arm/boot/dts/aspeed-g4.dtsi +6 -0

index 7c78eb1..2eba968 100644
@@ -870,6 +870,12 @@
 				interrupts = <8>;
 			};
 
+			mbox: mbox@1e789200 {
+				compatible = "aspeed,mbox-host";
+				reg = <0x1e789200 0x5c>;
+				interrupts = <46>;
+			};
+
 			wdt1: wdt@1e785000 {
 				compatible = "aspeed,ast2400-wdt";
 				reg = <0x1e785000 0x1c>;

Signature · 1 line

2.10.2

Joel Stanley

maintainer

2016-12-13 00:44 UTC #2 lore raw

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

Hi Cyril,

Quoted 16 lines · Cyril Bur

Palmettos have 512mb of ram. There is always framebuffer memory at the top of ram.
This patch also reserves BMC ram for host to BMC communication.
Signed-off-by Cyril Bur <[email protected]>
---
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 25 ++++++++++++++++++++++++-
 arch/arm/boot/dts/aspeed-g4.dtsi              |  6 ++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -0
index acaceda2..e810ae7 100644

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 21

Quoted 6 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +2 -1

@@ -17,7 +17,30 @@
        };
 
        memory {
-               reg = <0x40000000 0x10000000>;
+               reg = <0x40000000 0x20000000>;
+       };

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

Quoted 6 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +6 -0

+
+       reserved-memory {
+               #address-cells = <1>;
+               #size-cells= <1>;
+               ranges;
+

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 37

Quoted 10 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +10 -0

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

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

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 40

Quoted 3 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +3 -0
+       };
+
+       flash_buffer {

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

flash_buffer@1e789000

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 41

Quoted 1 line

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +1 -0
+               compatible = "aspeed,lpc-ctrl";

Upstream demands we put the SoC in the compatible string

aspeed,ast2400-lpc-ctrl.

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 43

Quoted 2 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +2 -0
+               memory-region = <&flash_mem>;
+               reg = <0x1e789000 8>;

Make the second cell be 0x8 for consistency.

arch/arm/boot/dts/aspeed-g4.dtsi line 874

Quoted 10 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -0

};

arch/arm/boot/dts/aspeed-g4.dtsi +2 -0

@@ -870,6 +870,12 @@
                                interrupts = <8>;
                        };
 
+                       mbox: mbox@1e789200 {
+                               compatible = "aspeed,mbox-host";

aspeed,ast2400-mbox-host

Not sure on the name here.

Quoted 14 lines

arch/arm/boot/dts/aspeed-g4.dtsi +4 -1

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

2.10.2

_______________________________________________
openbmc mailing list
[email protected]
https://lists.ozlabs.org/listinfo/openbmc

Cyril Bur

author

2016-12-13 03:11 UTC #3 lore raw

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

Quoted 24 lines · Joel Stanley

Hi Cyril,
Quoted 21 lines · Cyril Bur
Palmettos have 512mb of ram. There is always framebuffer memory at the
top of ram.
This patch also reserves BMC ram for host to BMC communication.
Signed-off-by Cyril Bur <[email protected]>
---
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 25
++++++++++++++++++++++++-
arch/arm/boot/dts/aspeed-g4.dtsi | 6 ++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -0
index acaceda2..e810ae7 100644
@@ -17,7 +17,30 @@
         };
 

Quoted 8 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 21
Quoted 4 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +2 -1
         memory {
-               reg = <0x40000000 0x10000000>;
+               reg = <0x40000000 0x20000000>;
+       };
Can you send this in a separate patch? It should go into the tree now.

Yep, should I send just for palmetto?

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

Quoted 10 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +10 -0

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

Quoted 9 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 37
Quoted 6 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +6 -0
+               };
+
+               fb_mem: framebuffer@5f000000 {
+                       no-map;
+                       reg = <0x5f000000 0x01000000>; /* 16m */
+               };
This should be a separate patch too, which we can take straight away.

I'll resend.

Quoted 26 lines

arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 40
Quoted 3 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +3 -0
+       };
+
+       flash_buffer {
It's convention to put the reg value after @ in the node name:
flash_buffer@1e789000
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 41
Quoted 1 line
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +1 -0
+               compatible = "aspeed,lpc-ctrl";
Upstream demands we put the SoC in the compatible string
aspeed,ast2400-lpc-ctrl.
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 43
Quoted 2 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +2 -0
+               memory-region = <&flash_mem>;
+               reg = <0x1e789000 8>;
Make the second cell be 0x8 for consistency.
Quoted 7 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -0
         };
arch/arm/boot/dts/aspeed-g4.dtsi +0 -0
@@ -870,6 +870,12 @@
                                 interrupts = <8>;
                         };
 

Quoted 7 lines

arch/arm/boot/dts/aspeed-g4.dtsi line 874
Quoted 2 lines
arch/arm/boot/dts/aspeed-g4.dtsi +2 -0
+                       mbox: mbox@1e789200 {
+                               compatible = "aspeed,mbox-host";
aspeed,ast2400-mbox-host
Not sure on the name here.

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

Thanks for review.

Cyril

Quoted 14 lines

arch/arm/boot/dts/aspeed-g4.dtsi +4 -1

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

2.10.2

_______________________________________________
openbmc mailing list
[email protected]
https://lists.ozlabs.org/listinfo/openbmc

Joel Stanley

maintainer

2016-12-13 03:30 UTC #4 lore raw

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

Quoted 14 lines · Cyril Bur

Quoted 5 lines
Quoted 5 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -0
@@ -17,7 +17,30 @@
        };
 
Quoted 8 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 21
Quoted 4 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +2 -1
        memory {
-               reg = <0x40000000 0x10000000>;
+               reg = <0x40000000 0x20000000>;
+       };
Can you send this in a separate patch? It should go into the tree now.

Quoted 5 lines

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

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

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

Cheers,

Joel

Cyril Bur

author

2016-12-13 03:48 UTC #5 lore raw

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

Quoted 24 lines · Joel Stanley

Quoted 14 lines · Cyril Bur
Quoted 5 lines
Quoted 5 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +0 -0
@@ -17,7 +17,30 @@
         };
 
Quoted 8 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts line 21
Quoted 4 lines
arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +2 -1
         memory {
-               reg = <0x40000000 0x10000000>;
+               reg = <0x40000000 0x20000000>;
+       };
Can you send this in a separate patch? It should go into the tree now.
Quoted 7 lines
Yep, should I send just for palmetto?
Do you have a list of other platforms? I figure all the ast2400 based
aspeed-bmc-opp-* should get this change, should I include them all in
one patch or all individually?

Quoted 9 lines

One patch will do. I checked all of the machines and they all have 512MB (minus bits that I assume are reserved for framebuffers, etc.):
Barreleye: 499020 kB
Palmetto: 424204 kB
Firestone: 416020 kB
Garrison: 416012 kB

Ok great thanks!

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

Quoted 3 lines

Cheers,
Joel

Joel Stanley

maintainer

2016-12-13 04:34 UTC #6 lore raw

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

Quoted 5 lines · Cyril Bur

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

No. They will come up in the strapped state.

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

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

Cheers,

Joel

Cyril Bur

author

2016-12-09 05:43 UTC #7 lore raw

PATCH 2/3 drivers/misc: Add aspeed mbox driver

In reply to Cyril Bur

Signed-off-by Cyril Bur <[email protected]>

---
 drivers/misc/Kconfig           |   7 +
 drivers/misc/Makefile          |   2 +
 drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/mbox-host.h |  18 +++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/misc/mbox-host.c
 create mode 100644 include/uapi/linux/mbox-host.h

drivers/misc/Kconfig +7 -0

index a216b46..44f191d 100644
@@ -804,6 +804,13 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.
 
+config MBOX_HOST
+	depends on ARCH_ASPEED
+	bool "Build MBOX driver for BMC to HOST communication"
+	default "n"
+	---help---
+	  Build the MBOX driver
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

drivers/misc/Makefile +2 -0

index b2fb6dbf..7bcaf30 100644
@@ -57,3 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
+>>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver

drivers/misc/mbox-host.c new file +326 -0

326 lines not on this page · raw message

include/uapi/linux/mbox-host.h new file +18 -0

new file mode 100644
index 0000000..3888f43
@@ -0,0 +1,18 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_MBOX_HOST_H
+#define _UAPI_LINUX_MBOX_HOST_H
+
+#include <linux/ioctl.h>
+
+#define __MBOX_HOST_IOCTL_MAGIC	0xb1
+#define MBOX_HOST_IOCTL_ATN	_IOW(__MBOX_HOST_IOCTL_MAGIC, 0x00, uint8_t)
+
+#endif /* _UAPI_LINUX_MBOX_HOST_H */

Signature · 1 line

2.10.2

Joel Stanley

maintainer

2016-12-13 01:26 UTC #8 lore raw

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

Quoted 1 line · Cyril Bur

Signed-off-by Cyril Bur <[email protected]>

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

Quoted 14 lines

---
 drivers/misc/Kconfig           |   7 +
 drivers/misc/Makefile          |   2 +
 drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/mbox-host.h |  18 +++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/misc/mbox-host.c
 create mode 100644 include/uapi/linux/mbox-host.h

drivers/misc/Kconfig +0 -0

index a216b46..44f191d 100644

drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 808

Quoted 5 lines

drivers/misc/Kconfig +2 -0

@@ -804,6 +804,13 @@ config PANEL_BOOT_MESSAGE
          An empty message will only clear the display at driver init time. Any other
          printf()-formatted message is valid with newline and escape codes.
 
+config MBOX_HOST
+       depends on ARCH_ASPEED

or COMPILE_TEST

drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 810

Quoted 2 lines

drivers/misc/Kconfig +2 -0

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

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

drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 812

Quoted 2 lines

drivers/misc/Kconfig +2 -0
+       ---help---
+         Build the MBOX driver

More text here please.

drivers/misc/Makefile

Quoted 8 lines

drivers/misc/Kconfig +1 -0

+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

drivers/misc/Makefile +0 -0

index b2fb6dbf..7bcaf30 100644

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

How about drivers/soc? Or drivers/mfd?

drivers/misc/Makefile

Quoted 12 lines

drivers/misc/Makefile +12 -0

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

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

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

etc.

Quoted 5 lines

drivers/misc/Makefile +4 -0

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

drivers/misc/Makefile

Quoted 10 lines

drivers/misc/Makefile +10 -0

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

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

drivers/misc/Makefile

Quoted 10 lines

drivers/misc/Makefile +10 -0

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

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

drivers/misc/Makefile

Quoted 4 lines

drivers/misc/Makefile +4 -0

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

Why not omit the callback?

Quoted 19 lines

drivers/misc/Makefile +19 -0

+
+static unsigned int mbox_host_poll(struct file *file, poll_table *wait)
+{
+       struct mbox_host *mbox_host = file_mbox_host(file);
+       unsigned int mask = 0;
+
+       poll_wait(file, &mbox_host->queue, wait);
+
+       if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
+               mask |= POLLIN;
+
+       return mask;
+}
+
+static const struct file_operations mbox_host_fops = {
+       .owner          = THIS_MODULE,
+       .open           = mbox_host_open,
+       .read           = mbox_host_read,
+       .write          = mbox_host_write,

drivers/misc/Makefile

Quoted 10 lines

drivers/misc/Makefile +10 -0

+       .release        = mbox_host_release,
+       .poll           = mbox_host_poll,
+       .unlocked_ioctl = mbox_host_ioctl,
+};
+
+static void poll_timer(unsigned long data)
+{
+       struct mbox_host *mbox_host = (void *)data;
+
+       mbox_host->poll_timer.expires += msecs_to_jiffies(500);

Why 500?

Quoted 4 lines

drivers/misc/Makefile +4 -0

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

Cédric Le Goater

2016-12-13 08:41 UTC #9 lore raw

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

Quoted 8 lines · Joel Stanley

drivers/misc/Makefile
Quoted 4 lines
drivers/misc/Makefile +0 -0
index b2fb6dbf..7bcaf30 100644
"misc seems a bit too misc" - Andrew Jeffery.
How about drivers/soc? Or drivers/mfd?

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

Cyril Bur

author

2016-12-15 07:59 UTC #10 lore raw

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

In reply to Joel Stanley

Quoted 5 lines · Joel Stanley

Quoted 1 line · Cyril Bur
Signed-off-by Cyril Bur <[email protected]>
Explain why we need this chardev, why it needs ioctls, etc.

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

Quoted 20 lines

Quoted 20 lines
---
drivers/misc/Kconfig           |   7 +
drivers/misc/Makefile          |   2 +
drivers/misc/mbox-host.c       | 326
+++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/mbox-host.h |  18 +++
 4 files changed, 353 insertions(+)
create mode 100644 drivers/misc/mbox-host.c create mode 100644 include/uapi/linux/mbox-host.h
drivers/misc/Kconfig +0 -0
index a216b46..44f191d 100644
@@ -804,6 +804,13 @@ config PANEL_BOOT_MESSAGE
An empty message will only clear the display at driver init time. Any other
printf()-formatted message is valid with newline and escape codes.

Quoted 4 lines

Quoted 2 lines
+config MBOX_HOST
+ depends on ARCH_ASPEED
or COMPILE_TEST

Is both a good solution?

Quoted 6 lines

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

Yes

Quoted 16 lines

Quoted 2 lines
+ ---help---
+ Build the MBOX driver
More text here please.
drivers/misc/Makefile
Quoted 8 lines
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +0 -0
index b2fb6dbf..7bcaf30 100644
"misc seems a bit too misc" - Andrew Jeffery.

Quoted 1 line

How about drivers/soc? Or drivers/mfd?

I think we settled on mailbox?

Quoted 19 lines

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

Quoted 6 lines

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

Ah yes looks much better now, thanks

Quoted 10 lines

drivers/misc/Makefile +10 -0

+#define MBOX_HOST_CTRL 0x4c
+#define MBOX_INTERRUPT_0 0x50
+#define MBOX_INTERRUPT_1 0x54
+
+
+static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
+{
+       return ioread8(mbox_host->base + reg);
+}
+

Quoted 8 lines

drivers/misc/Makefile
Quoted 5 lines
+static void mbox_outb(struct mbox_host *mbox_host, u8 data, int reg)
drivers/misc/Makefile +3 -0
+{
+       iowrite8(data, mbox_host->base + reg);
+}
As mentioned in the other email, all access to the LPC address space is through regmap.

Yes.

Quoted 16 lines

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

That would be wise yes

Quoted 33 lines

drivers/misc/Makefile

Quoted 6 lines

+static int mbox_host_release(struct inode *inode, struct file *file)
drivers/misc/Makefile +3 -0
+{
+       return 0;
+}

Why not omit the callback?

Quoted 24 lines

drivers/misc/Makefile +2 -0

+
+static unsigned int mbox_host_poll(struct file *file, poll_table

*wait)

drivers/misc/Makefile +21 -0

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

Quoted 8 lines

drivers/misc/Makefile

Quoted 6 lines

drivers/misc/Makefile +6 -0

+
+static void poll_timer(unsigned long data)
+{
+       struct mbox_host *mbox_host = (void *)data;
+
+       mbox_host->poll_timer.expires += msecs_to_jiffies(500);

Why 500?

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

Thanks for the review,

Cyril

Quoted 5 lines

Quoted 5 lines
+ wake_up(&mbox_host->queue);
+ add_timer(&mbox_host->poll_timer);
drivers/misc/Makefile +2 -0
+}
+

Andrew Jeffery

2016-12-15 22:50 UTC #11 lore raw

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

Quoted 6 lines · Cyril Bur

Quoted 4 lines
Quoted 2 lines
+config MBOX_HOST
+ depends on ARCH_ASPEED

or COMPILE_TEST
Is both a good solution?

I think Joel meant "|| COMPILE_TEST"

Cédric Le Goater

2016-12-13 08:21 UTC #12 lore raw

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

In reply to Cyril Bur

Hello Cyril,

Quoted 15 lines · Cyril Bur

Signed-off-by Cyril Bur <[email protected]>

---
 drivers/misc/Kconfig           |   7 +
 drivers/misc/Makefile          |   2 +
 drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/mbox-host.h |  18 +++
 4 files changed, 353 insertions(+)
 create mode 100644 drivers/misc/mbox-host.c
 create mode 100644 include/uapi/linux/mbox-host.h

drivers/misc/Kconfig +0 -0

index a216b46..44f191d 100644

drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 812

Quoted 9 lines

drivers/misc/Kconfig +6 -0

@@ -804,6 +804,13 @@ config PANEL_BOOT_MESSAGE
 	  An empty message will only clear the display at driver init time. Any other
 	  printf()-formatted message is valid with newline and escape codes.
 
+config MBOX_HOST
+	depends on ARCH_ASPEED
+	bool "Build MBOX driver for BMC to HOST communication"
+	default "n"
+	---help---
+	  Build the MBOX driver

nah. we need a little more doc here :)

Quoted 61 lines

drivers/misc/Kconfig +0 -0

 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

drivers/misc/Makefile +2 -0

index b2fb6dbf..7bcaf30 100644
@@ -57,3 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
+>>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver

drivers/misc/mbox-host.c new file +42 -0

new file mode 100644
index 0000000..9d11da4
@@ -0,0 +1,326 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/mbox-host.h>
+
+#define DEVICE_NAME	"mbox-host"
+
+#define MBOX_NUM_REGS 16
+#define MBOX_NUM_DATA_REGS 14
+
+#define MBOX_DATA_0 0x00
+#define MBOX_STATUS_0 0x40
+#define MBOX_STATUS_1 0x44
+#define MBOX_BMC_CTRL 0x48
+	#define MBOX_CTRL_RECV 0x80
+	#define MBOX_CTRL_MASK 0x02
+	#define MBOX_CTRL_SEND 0x01
+#define MBOX_HOST_CTRL 0x4c

drivers/misc/mbox-host.c line 52

Quoted 10 lines

drivers/misc/mbox-host.c new file +10 -0

+#define MBOX_INTERRUPT_0 0x50
+#define MBOX_INTERRUPT_1 0x54
+
+struct mbox_host {
+	struct miscdevice	miscdev;
+	void __iomem		*base;
+	int			irq;
+	wait_queue_head_t	queue;
+	struct timer_list	poll_timer;
+};

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

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

Quoted 29 lines

drivers/misc/mbox-host.c new file +29 -0

+static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
+{
+	return ioread8(mbox_host->base + reg);
+}
+
+static void mbox_outb(struct mbox_host *mbox_host, u8 data, int reg)
+{
+	iowrite8(data, mbox_host->base + reg);
+}
+
+static struct mbox_host *file_mbox_host(struct file *file)
+{
+	return container_of(file->private_data, struct mbox_host, miscdev);
+}
+
+static int mbox_host_open(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static ssize_t mbox_host_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+	char __user *p = buf;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;

drivers/misc/mbox-host.c line 92

Quoted 10 lines

drivers/misc/mbox-host.c new file +10 -0

+
+	WARN_ON(*ppos);
+
+	if (wait_event_interruptible(mbox_host->queue,
+		mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV))
+		return -ERESTARTSYS;
+
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
+		if (__put_user(mbox_inb(mbox_host, MBOX_DATA_0 + (i * 4)), p++))
+			return -EFAULT;

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

Quoted 15 lines

drivers/misc/mbox-host.c new file +15 -0

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

drivers/misc/mbox-host.c line 118

Quoted 10 lines

drivers/misc/mbox-host.c new file +10 -0

+
+	WARN_ON(*ppos);
+
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++) {
+		if (__get_user(c, p))
+			return -EFAULT;
+
+		mbox_outb(mbox_host, c, MBOX_DATA_0 + (i * 4));
+		p++;
+	}

same here.

drivers/misc/mbox-host.c line 132

Quoted 13 lines

drivers/misc/mbox-host.c new file +13 -0

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

15 ?

Quoted 147 lines

drivers/misc/mbox-host.c new file +147 -0

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

drivers/misc/mbox-host.c line 289

Quoted 10 lines

drivers/misc/mbox-host.c new file +10 -0

+		add_timer(&mbox_host->poll_timer);
+	}
+	return 0;
+
+out_unmap:
+	devm_iounmap(&pdev->dev, mbox_host->base);
+
+out_free:
+	devm_kfree(dev, mbox_host);
+	return rc;

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

drivers/misc/mbox-host.c line 301

Quoted 11 lines

drivers/misc/mbox-host.c new file +11 -0

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

These calls should be handled automatically by the driver.

drivers/misc/mbox-host.c line 308

Quoted 7 lines

drivers/misc/mbox-host.c new file +7 -0

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

you need a soc revision here.

Thanks,

Quoted 43 lines

drivers/misc/mbox-host.c new file +18 -0

+	{ },
+};
+
+static struct platform_driver mbox_host_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = mbox_host_match,
+	},
+	.probe = mbox_host_probe,
+	.remove = mbox_host_remove,
+};
+
+module_platform_driver(mbox_host_driver);
+
+MODULE_DEVICE_TABLE(of, mbox_host_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
+MODULE_DESCRIPTION("Linux device interface to the MBOX interface");

include/uapi/linux/mbox-host.h new file +18 -0

new file mode 100644
index 0000000..3888f43
@@ -0,0 +1,18 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_MBOX_HOST_H
+#define _UAPI_LINUX_MBOX_HOST_H
+
+#include <linux/ioctl.h>
+
+#define __MBOX_HOST_IOCTL_MAGIC	0xb1
+#define MBOX_HOST_IOCTL_ATN	_IOW(__MBOX_HOST_IOCTL_MAGIC, 0x00, uint8_t)
+
+#endif /* _UAPI_LINUX_MBOX_HOST_H */

Cyril Bur

author

2016-12-15 08:01 UTC #13 lore raw

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

Quoted 24 lines · Cédric Le Goater

Hello Cyril,
Quoted 21 lines · Cyril Bur
Signed-off-by Cyril Bur <[email protected]>
---
drivers/misc/Kconfig           |   7 +
drivers/misc/Makefile          |   2 +
drivers/misc/mbox-host.c       | 326
+++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/mbox-host.h |  18 +++
 4 files changed, 353 insertions(+)
create mode 100644 drivers/misc/mbox-host.c create mode 100644 include/uapi/linux/mbox-host.h
drivers/misc/Kconfig +0 -0
index a216b46..44f191d 100644
@@ -804,6 +804,13 @@ config PANEL_BOOT_MESSAGE
An empty message will only clear the display at driver init time. Any other
printf()-formatted message is valid with newline and escape codes.

Quoted 8 lines

drivers/misc/Kconfig config PANEL_BOOT_MESSAGE · line 812
Quoted 6 lines
drivers/misc/Kconfig +6 -0
+config MBOX_HOST
+	depends on ARCH_ASPEED
+	bool "Build MBOX driver for BMC to HOST communication"
+	default "n"
+	---help---
+	  Build the MBOX driver
nah. we need a little more doc here :)

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

Quoted 67 lines

source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"

drivers/misc/Makefile +0 -0

index b2fb6dbf..7bcaf30 100644
@@ -57,3 +57,5 @@ obj-$(CONFIG_ECHO)		+= echo/

obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
obj-$(CONFIG_PANEL) += panel.o

drivers/misc/Makefile +2 -0

+obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
+>>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver

drivers/misc/mbox-host.c new file +47 -0

new file mode 100644
index 0000000..9d11da4
@@ -0,0 +1,326 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/mbox-host.h>
+
+#define DEVICE_NAME	"mbox-host"
+
+#define MBOX_NUM_REGS 16
+#define MBOX_NUM_DATA_REGS 14
+
+#define MBOX_DATA_0 0x00
+#define MBOX_STATUS_0 0x40
+#define MBOX_STATUS_1 0x44
+#define MBOX_BMC_CTRL 0x48
+	#define MBOX_CTRL_RECV 0x80
+	#define MBOX_CTRL_MASK 0x02
+	#define MBOX_CTRL_SEND 0x01
+#define MBOX_HOST_CTRL 0x4c
+#define MBOX_INTERRUPT_0 0x50
+#define MBOX_INTERRUPT_1 0x54
+
+struct mbox_host {
+	struct miscdevice	miscdev;

Quoted 9 lines

drivers/misc/mbox-host.c line 52
Quoted 5 lines
drivers/misc/mbox-host.c new file +5 -0
+	void __iomem		*base;
+	int			irq;
+	wait_queue_head_t	queue;
+	struct timer_list	poll_timer;
+};
you should may be use an atomic to prevent multiple process to open the device. I don't think we want more than one ?

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

Quoted 2 lines

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

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

Quoted 41 lines

drivers/misc/mbox-host.c new file +6 -0

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

reg)

drivers/misc/mbox-host.c new file +7 -0

+{
+	iowrite8(data, mbox_host->base + reg);
+}
+
+static struct mbox_host *file_mbox_host(struct file *file)
+{
+	return container_of(file->private_data, struct mbox_host,

miscdev);

drivers/misc/mbox-host.c new file +21 -0

+}
+
+static int mbox_host_open(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static ssize_t mbox_host_read(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+	char __user *p = buf;
+	int i;
+
+	if (!access_ok(VERIFY_WRITE, buf, count))
+		return -EFAULT;
+
+	WARN_ON(*ppos);
+
+	if (wait_event_interruptible(mbox_host->queue,
+		mbox_inb(mbox_host, MBOX_BMC_CTRL) &

MBOX_CTRL_RECV))

drivers/misc/mbox-host.c new file +2 -0

+		return -ERESTARTSYS;
+

Quoted 8 lines

drivers/misc/mbox-host.c
Quoted 4 lines
drivers/misc/mbox-host.c new file +2 -0
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++)
+		if (__put_user(mbox_inb(mbox_host, MBOX_DATA_0 +
(i * 4)), p++))
drivers/misc/mbox-host.c new file +1 -0
+			return -EFAULT;
could you change the code to not use byte at a time call from userspace ? and
any how, __put_user(mbox_inb(...)) looks like it could be splitted :)

Yeah, I could/should buffer it.

Quoted 22 lines

drivers/misc/mbox-host.c new file +1 -0

+	/* MBOX_CTRL_RECV bit is W1C, this also unmasks in 1 step

drivers/misc/mbox-host.c new file +5 -0

+	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return p - buf;
+}
+
+static ssize_t mbox_host_write(struct file *file, const char

__user *buf,

drivers/misc/mbox-host.c new file +13 -0

+				size_t count, loff_t *ppos)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+	const char __user *p = buf;
+	char c;
+	int i;
+
+	if (!access_ok(VERIFY_READ, buf, count))
+		return -EFAULT;
+
+	WARN_ON(*ppos);
+
+	for (i = 0; i < MBOX_NUM_DATA_REGS; i++) {

Quoted 8 lines

drivers/misc/mbox-host.c line 118
Quoted 6 lines
drivers/misc/mbox-host.c new file +6 -0
+		if (__get_user(c, p))
+			return -EFAULT;
+
+		mbox_outb(mbox_host, c, MBOX_DATA_0 + (i * 4));
+		p++;
+	}
same here.

Yep.

Quoted 16 lines

Quoted 4 lines

drivers/misc/mbox-host.c new file +3 -0

+	mbox_outb(mbox_host, MBOX_CTRL_SEND, MBOX_BMC_CTRL);
+
+	return p - buf;

drivers/misc/mbox-host.c line 132

Quoted 10 lines

drivers/misc/mbox-host.c new file +10 -0

+}
+
+static long mbox_host_ioctl(struct file *file, unsigned int cmd,
+		unsigned long param)
+{
+	struct mbox_host *mbox_host = file_mbox_host(file);
+
+	switch (cmd) {
+	case MBOX_HOST_IOCTL_ATN:
+		mbox_outb(mbox_host, param, MBOX_DATA_0 + 15);

15 ?

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

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

Quoted 166 lines

drivers/misc/mbox-host.c new file +7 -0

+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int mbox_host_release(struct inode *inode, struct file

*file)

drivers/misc/mbox-host.c new file +5 -0

+{
+	return 0;
+}
+
+static unsigned int mbox_host_poll(struct file *file, poll_table

*wait)

drivers/misc/mbox-host.c new file +36 -0

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

MBOX_CTRL_RECV))

drivers/misc/mbox-host.c new file +4 -0

+		return IRQ_NONE;
+
+	/*
+	 * Leave the status bit set so that we know the data is

for us,

drivers/misc/mbox-host.c new file +4 -0

+	 * clear it once it has been read.
+	 */
+
+	/* Mask it off, we'll clear it when we the data gets read

drivers/misc/mbox-host.c new file +17 -0

+	mbox_outb(mbox_host, MBOX_CTRL_MASK, MBOX_BMC_CTRL);
+
+	wake_up(&mbox_host->queue);
+	return IRQ_HANDLED;
+}
+
+static int mbox_host_config_irq(struct mbox_host *mbox_host,
+		struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	mbox_host->irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!mbox_host->irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, mbox_host->irq, mbox_host_irq,

IRQF_SHARED,

drivers/misc/mbox-host.c new file +3 -0

+			DEVICE_NAME, mbox_host);
+	if (rc < 0) {
+		dev_warn(dev, "Unable to request IRQ %d\n",

mbox_host->irq);

drivers/misc/mbox-host.c new file +6 -0

+		mbox_host->irq = 0;
+		return rc;
+	}
+
+	/*
+	 * Disable all register based interrupts, we'll have to

change

drivers/misc/mbox-host.c new file +3 -0

+	 * this, protocol seemingly will require regs 0 and 15
+	 */
+	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_0); /* regs 0 -

7 */

drivers/misc/mbox-host.c new file +1 -0

+	mbox_outb(mbox_host, 0x00, MBOX_INTERRUPT_1); /* regs 8 -

15 */

drivers/misc/mbox-host.c new file +23 -0

+
+	/* W1C */
+	mbox_outb(mbox_host, 0xff, MBOX_STATUS_0);
+	mbox_outb(mbox_host, 0xff, MBOX_STATUS_1);
+
+	mbox_outb(mbox_host, MBOX_CTRL_RECV, MBOX_BMC_CTRL);
+	return 0;
+}
+
+static int mbox_host_probe(struct platform_device *pdev)
+{
+	struct mbox_host *mbox_host;
+	struct device *dev;
+	struct resource *res;
+	int rc;
+
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
+	dev = &pdev->dev;
+	dev_info(dev, "Found mbox host device\n");
+
+	mbox_host = devm_kzalloc(dev, sizeof(*mbox_host),

GFP_KERNEL);

drivers/misc/mbox-host.c new file +38 -0

+	if (!mbox_host)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, mbox_host);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Unable to find resources\n");
+		rc = -ENXIO;
+		goto out_free;
+	}
+
+	mbox_host->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!mbox_host->base) {
+		rc = -ENOMEM;
+		goto out_free;
+	}
+	init_waitqueue_head(&mbox_host->queue);
+
+	mbox_host->miscdev.minor = MISC_DYNAMIC_MINOR;
+	mbox_host->miscdev.name = DEVICE_NAME;
+	mbox_host->miscdev.fops = &mbox_host_fops;
+	mbox_host->miscdev.parent = dev;
+	rc = misc_register(&mbox_host->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register device\n");
+		goto out_unmap;
+	}
+
+	mbox_host_config_irq(mbox_host, pdev);
+
+	if (mbox_host->irq) {
+		dev_info(dev, "Using IRQ %d\n", mbox_host->irq);
+	} else {
+		dev_info(dev, "No IRQ; using timer\n");
+		setup_timer(&mbox_host->poll_timer, poll_timer,
+				(unsigned long)mbox_host);
+		mbox_host->poll_timer.expires = jiffies +

msecs_to_jiffies(10);

drivers/misc/mbox-host.c new file +6 -0

+		add_timer(&mbox_host->poll_timer);
+	}
+	return 0;
+
+out_unmap:
+	devm_iounmap(&pdev->dev, mbox_host->base);

Quoted 8 lines

drivers/misc/mbox-host.c line 289
Quoted 4 lines
drivers/misc/mbox-host.c new file +4 -0
+
+out_free:
+	devm_kfree(dev, mbox_host);
+	return rc;
These calls should be handled automatically by the driver when you use the devm_*
routines

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

Quoted 17 lines

drivers/misc/mbox-host.c line 301

Quoted 12 lines

drivers/misc/mbox-host.c new file +11 -0

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

These calls should be handled automatically by the driver.

Quoted 10 lines

drivers/misc/mbox-host.c line 308
Quoted 7 lines
drivers/misc/mbox-host.c new file +7 -0
+	mbox_host = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id mbox_host_match[] = {
+	{ .compatible = "aspeed,mbox-host" },
you need a soc revision here.

Ack

Quoted 1 line

Thanks for the review,

Cyril

Quoted 52 lines

Thanks,

Quoted 46 lines

drivers/misc/mbox-host.c new file +18 -0

+	{ },
+};
+
+static struct platform_driver mbox_host_driver = {
+	.driver = {
+		.name		= DEVICE_NAME,
+		.of_match_table = mbox_host_match,
+	},
+	.probe = mbox_host_probe,
+	.remove = mbox_host_remove,
+};
+
+module_platform_driver(mbox_host_driver);
+
+MODULE_DEVICE_TABLE(of, mbox_host_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
+MODULE_DESCRIPTION("Linux device interface to the MBOX

interface");

include/uapi/linux/mbox-host.h new file +16 -0

new file mode 100644
index 0000000..3888f43
@@ -0,0 +1,18 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_MBOX_HOST_H
+#define _UAPI_LINUX_MBOX_HOST_H
+
+#include <linux/ioctl.h>
+
+#define __MBOX_HOST_IOCTL_MAGIC	0xb1
+#define MBOX_HOST_IOCTL_ATN	_IOW(__MBOX_HOST_IOCTL_MAGIC,

0x00, uint8_t)

include/uapi/linux/mbox-host.h new file +2 -0

+
+#endif /* _UAPI_LINUX_MBOX_HOST_H */

Andrew Jeffery

2016-12-14 23:14 UTC #14 lore raw

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

In reply to Cyril Bur

Quoted 7 lines · Cyril Bur

Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>
---
drivers/misc/Kconfig           |   7 +
drivers/misc/Makefile          |   2 +
drivers/misc/mbox-host.c       | 326
+++++++++++++++++++++++++++++++++++++++++

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

Quoted 16 lines

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

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

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

Andrew

Andrew Jeffery

2016-12-18 23:40 UTC #15 lore raw

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

In reply to Cyril Bur

Quoted 5 lines · Cyril Bur

Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>
---
drivers/misc/Kconfig           |   7 +
drivers/misc/Makefile          |   2 +
drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++

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

Quoted 13 lines

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

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

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

Andrew

Cyril Bur

author

2016-12-09 05:43 UTC #16 lore raw

PATCH 3/3 drivers/misc: Add aspeed lpc controlling driver

In reply to 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          |   7 ++
 drivers/misc/Makefile         |   2 +-
 drivers/misc/lpc-ctrl.c       | 251 ++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/lpc-ctrl.h |  25 +++++
 4 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/lpc-ctrl.c
 create mode 100644 include/uapi/linux/lpc-ctrl.h

drivers/misc/Kconfig +7 -0

index 44f191d..2bd3f92 100644
@@ -811,6 +811,13 @@ config MBOX_HOST
 	---help---
 	  Build the MBOX driver
 
+config LPC_CTRL
+	depends on ARCH_ASPEED
+	bool "Build a driver to control the BMC to HOST LPC bus"
+	default "n"
+	---help---
+	  This will also use a BMC ram region defined in the device tree
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

drivers/misc/Makefile +1 -1

index 7bcaf30..e8502de 100644
@@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
 obj-$(CONFIG_MBOX_HOST)		+= mbox-host.o
->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
+obj-$(CONFIG_LPC_CTRL)		+= lpc-ctrl.o

drivers/misc/lpc-ctrl.c new file +251 -0

251 lines not on this page · raw message

include/uapi/linux/lpc-ctrl.h new file +25 -0

new file mode 100644
index 0000000..c5f1caf
@@ -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.10.2

Joel Stanley

maintainer

2016-12-13 00:51 UTC #17 lore raw

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

Hi Cyril,

Quoted 3 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

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

I like your ioctl approach.

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

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

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

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

Cheers,

Joel

Quoted 334 lines

Signed-off-by Cyril Bur <[email protected]>

---
 drivers/misc/Kconfig          |   7 ++
 drivers/misc/Makefile         |   2 +-
 drivers/misc/lpc-ctrl.c       | 251 ++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/lpc-ctrl.h |  25 +++++
 4 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/lpc-ctrl.c
 create mode 100644 include/uapi/linux/lpc-ctrl.h

drivers/misc/Kconfig +7 -0

index 44f191d..2bd3f92 100644
@@ -811,6 +811,13 @@ config MBOX_HOST
        ---help---
          Build the MBOX driver
 
+config LPC_CTRL
+       depends on ARCH_ASPEED
+       bool "Build a driver to control the BMC to HOST LPC bus"
+       default "n"
+       ---help---
+         This will also use a BMC ram region defined in the device tree
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

drivers/misc/Makefile +1 -1

index 7bcaf30..e8502de 100644
@@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)         += cxl/
 obj-$(CONFIG_PANEL)             += panel.o
 obj-$(CONFIG_MBOX_HOST)                += mbox-host.o
->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
+obj-$(CONFIG_LPC_CTRL)         += lpc-ctrl.o

drivers/misc/lpc-ctrl.c new file +251 -0

new file mode 100644
index 0000000..f72f44f
@@ -0,0 +1,251 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/lpc-ctrl.h>
+
+#define DEVICE_NAME    "lpc-ctrl"
+
+#define LPC_HICR7 0x88
+#define LPC_HICR8 0x8c
+
+struct lpc_ctrl {
+       struct miscdevice       miscdev;
+       void __iomem            *ctrl;
+       phys_addr_t                     base;
+       resource_size_t         size;
+};
+
+
+static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
+{
+       return container_of(file->private_data, struct lpc_ctrl, miscdev);
+}
+
+static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
+{
+       struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+       unsigned long vsize = vma->vm_end - vma->vm_start;
+
+       if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
+               return -EINVAL;
+
+       /* Other checks? */
+
+       if (remap_pfn_range(vma, vma->vm_start,
+               (lpc_ctrl->base >> PAGE_SHIFT) + vma->vm_pgoff,
+               vsize, vma->vm_page_prot))
+               return -EAGAIN;
+
+       return 0;
+}
+
+static int lpc_ctrl_open(struct inode *inode, struct file *file)
+{
+       return 0;
+}
+
+static ssize_t lpc_ctrl_read(struct file *file, char __user *buf,
+                               size_t count, loff_t *ppos)
+{
+       if (!access_ok(VERIFY_WRITE, buf, count))
+               return -EFAULT;
+
+       WARN_ON(*ppos);
+
+       return -EPERM;
+}
+
+static ssize_t lpc_ctrl_write(struct file *file, const char __user *buf,
+                               size_t count, loff_t *ppos)
+{
+       if (!access_ok(VERIFY_READ, buf, count))
+               return -EFAULT;
+
+       WARN_ON(*ppos);
+
+       return -EPERM;
+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
+               unsigned long param)
+{
+       struct lpc_mapping map;
+       struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+       void __user *p = (void __user *)param;
+
+       switch (cmd) {
+       case LPC_CTRL_IOCTL_SIZE:
+               return copy_to_user(p, &lpc_ctrl->size,
+                       sizeof(lpc_ctrl->size)) ? -EFAULT : 0;
+       case LPC_CTRL_IOCTL_MAP:
+               if (copy_from_user(&map, p, sizeof(map)))
+                       return -EFAULT;
+
+               iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
+                               lpc_ctrl->ctrl + LPC_HICR7);
+               iowrite32((~(map.size - 1)) | ((map.size >> 16) - 1),
+                               lpc_ctrl->ctrl + LPC_HICR8);
+               return 0;
+       case LPC_CTRL_IOCTL_UNMAP:
+               if (copy_from_user(&map, p, sizeof(map)))
+                       return -EFAULT;
+
+               iowrite32((0x3000 << 16) | (0x0e00),
+                               lpc_ctrl->ctrl + LPC_HICR7);
+               iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) - 1),
+                               lpc_ctrl->ctrl + LPC_HICR8);
+               return 0;
+       }
+
+       return -EINVAL;
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file *file)
+{
+       return 0;
+}
+
+static const struct file_operations lpc_ctrl_fops = {
+       .owner          = THIS_MODULE,
+       .mmap           = lpc_ctrl_mmap,
+       .open           = lpc_ctrl_open,
+       .read           = lpc_ctrl_read,
+       .write          = lpc_ctrl_write,
+       .release        = lpc_ctrl_release,
+       .unlocked_ioctl = lpc_ctrl_ioctl,
+};
+
+static int lpc_ctrl_probe(struct platform_device *pdev)
+{
+       struct lpc_ctrl *lpc_ctrl;
+       struct device *dev;
+       struct device_node *node;
+       struct resource *res;
+       struct resource resm;
+       int rc;
+
+       if (!pdev || !pdev->dev.of_node)
+               return -ENODEV;
+
+       dev = &pdev->dev;
+
+       lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
+       if (!lpc_ctrl)
+               return -ENOMEM;
+
+       dev_set_drvdata(&pdev->dev, lpc_ctrl);
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (!res) {
+               dev_err(dev, "Unable to find resources\n");
+               rc = -ENXIO;
+               goto out_free;
+       }
+
+       /* Todo unmap this on fail cases and exit */
+       lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
+       if (!lpc_ctrl->ctrl) {
+               rc = -ENOMEM;
+               goto out_free;
+       }
+
+       node = of_parse_phandle(dev->of_node, "memory-region", 0);
+       if (!node) {
+               /*
+                * Should probaby handle this by allocating 4-64k now and
+                * using that
+                */
+               dev_err(dev, "Didn't find reserved memory\n");
+               rc = -EINVAL;
+               goto out;
+       }
+
+       rc = of_address_to_resource(node, 0, &resm);
+       of_node_put(node);
+       if (rc) {
+               dev_err(dev, "Could address to resource\n");
+               rc = -ENOMEM;
+               goto out;
+       }
+
+       lpc_ctrl->size = resource_size(&resm);
+       lpc_ctrl->base = resm.start;
+
+       lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR;
+       lpc_ctrl->miscdev.name = DEVICE_NAME;
+       lpc_ctrl->miscdev.fops = &lpc_ctrl_fops;
+       lpc_ctrl->miscdev.parent = dev;
+       rc = misc_register(&lpc_ctrl->miscdev);
+       if (rc) {
+               dev_err(dev, "Unable to register device\n");
+               goto out;
+       }
+
+       dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+                       lpc_ctrl->base, lpc_ctrl->size);
+       return 0;
+
+out:
+       devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
+out_free:
+       devm_kfree(dev, lpc_ctrl);
+       return rc;
+}
+
+static int lpc_ctrl_remove(struct platform_device *pdev)
+{
+       struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
+
+       misc_deregister(&lpc_ctrl->miscdev);
+       devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
+       devm_kfree(&pdev->dev, lpc_ctrl);
+       lpc_ctrl = NULL;
+
+       return 0;
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
+       { .compatible = "aspeed,lpc-ctrl" },
+       { },
+};
+
+static struct platform_driver lpc_ctrl_driver = {
+       .driver = {
+               .name           = DEVICE_NAME,
+               .of_match_table = lpc_ctrl_match,
+       },
+       .probe = lpc_ctrl_probe,
+       .remove = lpc_ctrl_remove,
+};
+
+module_platform_driver(lpc_ctrl_driver);
+
+MODULE_DEVICE_TABLE(of, lpc_ctrl_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cyril Bur <[email protected]>");
+MODULE_DESCRIPTION("Linux device interface to control LPC bus");

include/uapi/linux/lpc-ctrl.h new file +25 -1

new file mode 100644
index 0000000..c5f1caf
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _UAPI_LINUX_LPC_CTRL_H
+#define _UAPI_LINUX_LPC_CTRL_H
+
+#include <linux/ioctl.h>
+
+struct lpc_mapping {
+       uint32_t hostaddr;
+       uint32_t size;
+};
+
+#define __LPC_CTRL_IOCTL_MAGIC 0xb2
+#define LPC_CTRL_IOCTL_SIZE    _IOR(__LPC_CTRL_IOCTL_MAGIC, 0x00, uint32_t)
+#define LPC_CTRL_IOCTL_MAP     _IOW(__LPC_CTRL_IOCTL_MAGIC, 0x01, struct lpc_mapping)
+#define LPC_CTRL_IOCTL_UNMAP   _IO(__LPC_CTRL_IOCTL_MAGIC, 0x02)
+
+#endif /* _UAPI_LINUX_LPC_CTRL_H */
--

2.10.2

_______________________________________________
openbmc mailing list
[email protected]
https://lists.ozlabs.org/listinfo/openbmc

Andrew Jeffery

2016-12-15 00:58 UTC #18 lore raw

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

In reply to Cyril Bur

Quoted 10 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          |   7 ++
drivers/misc/Makefile         |   2 +-
drivers/misc/lpc-ctrl.c       | 251 ++++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/lpc-ctrl.h |  25 +++++

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

Quoted 12 lines

 4 files changed, 284 insertions(+), 1 deletion(-)
create mode 100644 drivers/misc/lpc-ctrl.c
create mode 100644 include/uapi/linux/lpc-ctrl.h
drivers/misc/Kconfig +0 -0
index 44f191d..2bd3f92 100644
@@ -811,6 +811,13 @@ config MBOX_HOST
Quoted 2 lines
---help---
Build the MBOX driver

drivers/misc/Kconfig config MBOX_HOST · line 815

Quoted 2 lines

drivers/misc/Kconfig +2 -0
+config LPC_CTRL
+	depends on ARCH_ASPEED

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

Quoted 15 lines

drivers/misc/Kconfig +1 -0
+	bool "Build a driver to control the BMC to HOST LPC bus"
drivers/misc/Kconfig config MBOX_HOST · line 819
Quoted 3 lines
drivers/misc/Kconfig +3 -0
+	default "n"
+	---help---
+	  This will also use a BMC ram region defined in the device tree
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +0 -0
index 7bcaf30..e8502de 100644
Quoted 2 lines
(quoted diff) +0 -0
@@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
obj-$(CONFIG_PANEL) += panel.o

Quoted 2 lines

Quoted 1 line
obj-$(CONFIG_MBOX_HOST) += mbox-host.o
->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver

This doesn't look right.

Quoted 118 lines

drivers/misc/Makefile +1 -0

+obj-$(CONFIG_LPC_CTRL)		+= lpc-ctrl.o

drivers/misc/lpc-ctrl.c new file +30 -0

new file mode 100644
index 0000000..f72f44f
@@ -0,0 +1,251 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/lpc-ctrl.h>
+

Quoted 1 line

+#define DEVICE_NAME "lpc-ctrl"

drivers/misc/lpc-ctrl.c new file +5 -0

+
+#define LPC_HICR7 0x88
+#define LPC_HICR8 0x8c
+
+struct lpc_ctrl {

Quoted 4 lines

drivers/misc/lpc-ctrl.c new file +4 -0

+	struct miscdevice	miscdev;
+	void __iomem		*ctrl;
+	phys_addr_t			base;
+	resource_size_t		size;

drivers/misc/lpc-ctrl.c new file +5 -0

+};
+
+
+static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
+{

Quoted 1 line

drivers/misc/lpc-ctrl.c new file +1 -0
+	return container_of(file->private_data, struct lpc_ctrl, miscdev);

drivers/misc/lpc-ctrl.c new file +4 -0

+}
+
+static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct *vma)
+{

drivers/misc/lpc-ctrl.c line 52

Quoted 2 lines

drivers/misc/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/lpc-ctrl.c line 55

Quoted 2 lines

drivers/misc/lpc-ctrl.c new file +2 -0

+	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl->size)
+		return -EINVAL;

drivers/misc/lpc-ctrl.c line 57

Quoted 1 line

drivers/misc/lpc-ctrl.c new file +1 -0
+	/* Other checks? */

drivers/misc/lpc-ctrl.c line 62

Quoted 4 lines

drivers/misc/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/lpc-ctrl.c new file +4 -0

+}
+
+static int lpc_ctrl_open(struct inode *inode, struct file *file)
+{

Quoted 1 line

(quoted diff) +1 -0
+	return 0;

drivers/misc/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/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/lpc-ctrl.c new file +3 -0

+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,

drivers/misc/lpc-ctrl.c line 95

Quoted 1 line

drivers/misc/lpc-ctrl.c new file +1 -0
+		unsigned long param)

drivers/misc/lpc-ctrl.c line 99

Quoted 3 lines

drivers/misc/lpc-ctrl.c new file +3 -0

+	struct lpc_mapping map;
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	void __user *p = (void __user *)param;

drivers/misc/lpc-ctrl.c line 107

Quoted 7 lines

drivers/misc/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;

Quoted 3 lines

drivers/misc/lpc-ctrl.c new file +3 -0

+		iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
+				lpc_ctrl->ctrl + LPC_HICR7);
+		iowrite32((~(map.size - 1)) | ((map.size >> 16) - 1),

drivers/misc/lpc-ctrl.c

Quoted 1 line

drivers/misc/lpc-ctrl.c new file +1 -0
+				lpc_ctrl->ctrl + LPC_HICR8);

I think these operations could do with some explanation.

drivers/misc/lpc-ctrl.c

Quoted 4 lines

drivers/misc/lpc-ctrl.c new file +1 -0
+		return 0;
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	case LPC_CTRL_IOCTL_UNMAP:
+		if (copy_from_user(&map, p, sizeof(map)))
drivers/misc/lpc-ctrl.c new file +1 -0
+			return -EFAULT;

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

drivers/misc/lpc-ctrl.c

Quoted 5 lines

+
Quoted 3 lines
drivers/misc/lpc-ctrl.c new file +3 -0
+		iowrite32((0x3000 << 16) | (0x0e00),
+				lpc_ctrl->ctrl + LPC_HICR7);
+		iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) - 1),
drivers/misc/lpc-ctrl.c new file +1 -0
+				lpc_ctrl->ctrl + LPC_HICR8);

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

Quoted 47 lines

drivers/misc/lpc-ctrl.c new file +1 -0
+		return 0;
Review
Quoted 1 line
(quoted diff) +1 -0
+	}
+
Quoted 1 line
(quoted diff) +1 -0
+	return -EINVAL;
drivers/misc/lpc-ctrl.c new file +4 -0
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file *file)
+{
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/misc/lpc-ctrl.c new file +3 -0
+}
+
+static const struct file_operations lpc_ctrl_fops = {
Quoted 7 lines
Quoted 7 lines
drivers/misc/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/lpc-ctrl.c new file +4 -0
+};
+
+static int lpc_ctrl_probe(struct platform_device *pdev)
+{
drivers/misc/lpc-ctrl.c line 150
Quoted 6 lines
drivers/misc/lpc-ctrl.c new file +6 -0
+	struct lpc_ctrl *lpc_ctrl;
+	struct device *dev;
+	struct device_node *node;
+	struct resource *res;
+	struct resource resm;
+	int rc;
+
drivers/misc/lpc-ctrl.c line 153
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
drivers/misc/lpc-ctrl.c line 155
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	dev = &pdev->dev;
+
drivers/misc/lpc-ctrl.c line 159
Quoted 3 lines
drivers/misc/lpc-ctrl.c new file +3 -0
+	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl), GFP_KERNEL);
+	if (!lpc_ctrl)
+		return -ENOMEM;
+
drivers/misc/lpc-ctrl.c line 161
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	dev_set_drvdata(&pdev->dev, lpc_ctrl);
+
Quoted 6 lines
drivers/misc/lpc-ctrl.c new file +6 -0
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Unable to find resources\n");
+		rc = -ENXIO;
+		goto out_free;
+	}

drivers/misc/lpc-ctrl.c

Quoted 2 lines

drivers/misc/lpc-ctrl.c new file +2 -0
+
+	/* Todo unmap this on fail cases and exit */

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

Quoted 44 lines

drivers/misc/lpc-ctrl.c new file +1 -0
+	lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
drivers/misc/lpc-ctrl.c line 175
Quoted 4 lines
drivers/misc/lpc-ctrl.c new file +4 -0
+	if (!lpc_ctrl->ctrl) {
+		rc = -ENOMEM;
+		goto out_free;
+	}
+
drivers/misc/lpc-ctrl.c line 186
Quoted 10 lines
drivers/misc/lpc-ctrl.c new file +10 -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
+		 */
+		dev_err(dev, "Didn't find reserved memory\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
drivers/misc/lpc-ctrl.c line 194
Quoted 7 lines
drivers/misc/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/lpc-ctrl.c line 197
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	lpc_ctrl->size = resource_size(&resm);
+	lpc_ctrl->base = resm.start;
+
drivers/misc/lpc-ctrl.c line 207
Quoted 9 lines
drivers/misc/lpc-ctrl.c new file +9 -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");
+		goto out;
+	}
+
Quoted 3 lines
drivers/misc/lpc-ctrl.c new file +3 -0
+	dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+			lpc_ctrl->base, lpc_ctrl->size);
+	return 0;
drivers/misc/lpc-ctrl.c new file +2 -0
+
+out:
Quoted 1 line
(quoted diff) +1 -0
+	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);

drivers/misc/lpc-ctrl.c line 216

Quoted 2 lines

drivers/misc/lpc-ctrl.c new file +2 -0
+out_free:
+	devm_kfree(dev, lpc_ctrl);

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

drivers/misc/lpc-ctrl.c line 226

Quoted 10 lines

drivers/misc/lpc-ctrl.c new file +5 -0
+	return rc;
+}
+
+static int lpc_ctrl_remove(struct platform_device *pdev)
+{
drivers/misc/lpc-ctrl.c line 222
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
+
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	misc_deregister(&lpc_ctrl->miscdev);
+	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
drivers/misc/lpc-ctrl.c new file +1 -0
+	devm_kfree(&pdev->dev, lpc_ctrl);

As above.

Andrew

Quoted 56 lines

drivers/misc/lpc-ctrl.c new file +2 -0
+	lpc_ctrl = NULL;
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/misc/lpc-ctrl.c new file +3 -0
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	{ .compatible = "aspeed,lpc-ctrl" },
+	{ },
drivers/misc/lpc-ctrl.c new file +3 -0
+};
+
+static struct platform_driver lpc_ctrl_driver = {
Quoted 6 lines
drivers/misc/lpc-ctrl.c new file +1 -0
+	.driver = {
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+		.name		= DEVICE_NAME,
drivers/misc/lpc-ctrl.c new file +4 -0
+		.of_match_table = lpc_ctrl_match,
+	},
+	.probe = lpc_ctrl_probe,
+	.remove = lpc_ctrl_remove,
drivers/misc/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/lpc-ctrl.c new file +1 -0
+MODULE_DESCRIPTION("Linux device interface to control LPC bus");
include/uapi/linux/lpc-ctrl.h new file +15 -0
new file mode 100644
index 0000000..c5f1caf
@@ -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/lpc-ctrl.h new file +2 -0
+	uint32_t hostaddr;
+	uint32_t size;
include/uapi/linux/lpc-ctrl.h new file +2 -0
+};
+
Quoted 4 lines
include/uapi/linux/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/lpc-ctrl.h new file +2 -0
+
+#endif /* _UAPI_LINUX_LPC_CTRL_H */

Cyril Bur

author

2016-12-15 08:01 UTC #19 lore raw

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

Quoted 31 lines · Andrew Jeffery

Quoted 7 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 8 lines
Quoted 1 line
Signed-off-by Cyril Bur <[email protected]>
---
drivers/misc/Kconfig          |   7 ++
drivers/misc/Makefile         |   2 +-
drivers/misc/lpc-ctrl.c       | 251
++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/lpc-ctrl.h |  25 +++++
The naming might be a bit generic. Control of what? Any LPC device?
Quoted 12 lines
 4 files changed, 284 insertions(+), 1 deletion(-)
create mode 100644 drivers/misc/lpc-ctrl.c
create mode 100644 include/uapi/linux/lpc-ctrl.h
drivers/misc/Kconfig +0 -0
index 44f191d..2bd3f92 100644
@@ -811,6 +811,13 @@ config MBOX_HOST
Quoted 2 lines
---help---
Build the MBOX driver

Quoted 6 lines

drivers/misc/Kconfig config MBOX_HOST · line 815
Quoted 3 lines
drivers/misc/Kconfig +2 -0
+config LPC_CTRL
+	depends on ARCH_ASPEED
Again, maybe we need ASPEED in the config symbol name? And || COMPILE_TEST here too.

Yeah will do.

Quoted 20 lines

Quoted 20 lines
drivers/misc/Kconfig +1 -0
+	bool "Build a driver to control the BMC to HOST LPC bus"
Quoted 4 lines
drivers/misc/Kconfig +3 -0
+	default "n"
+	---help---
+	  This will also use a BMC ram region defined in the
device tree
+
source "drivers/misc/c2port/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +0 -0
index 7bcaf30..e8502de 100644
Quoted 3 lines
(quoted diff) +0 -0
@@ -58,4 +58,4 @@ obj-$(CONFIG_VEXPRESS_SYSCFG)	+=
vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
obj-$(CONFIG_PANEL) += panel.o

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
obj-$(CONFIG_MBOX_HOST) += mbox-host.o
->>>>>>> 6252f95... fixup! drivers/misc: Add aspeed mbox driver
This doesn't look right.

Oops! How did that happen :/

Quoted 140 lines

Quoted 140 lines
drivers/misc/Makefile +1 -0
+obj-$(CONFIG_LPC_CTRL)		+= lpc-ctrl.o
drivers/misc/lpc-ctrl.c new file +30 -0
new file mode 100644
index 0000000..f72f44f
@@ -0,0 +1,251 @@
+/*
+ * Copyright 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/lpc-ctrl.h>
+
Quoted 1 line
+#define DEVICE_NAME "lpc-ctrl"
drivers/misc/lpc-ctrl.c new file +5 -0
+
+#define LPC_HICR7 0x88
+#define LPC_HICR8 0x8c
+
+struct lpc_ctrl {
Quoted 4 lines
Quoted 4 lines
drivers/misc/lpc-ctrl.c new file +4 -0
+	struct miscdevice	miscdev;
+	void __iomem		*ctrl;
+	phys_addr_t			base;
+	resource_size_t		size;
drivers/misc/lpc-ctrl.c new file +5 -0
+};
+
+
+static struct lpc_ctrl *file_lpc_ctrl(struct file *file)
+{
Quoted 2 lines
(quoted diff) +1 -0
+	return container_of(file->private_data, struct lpc_ctrl,
miscdev);
drivers/misc/lpc-ctrl.c new file +3 -0
+}
+
+static int lpc_ctrl_mmap(struct file *file, struct vm_area_struct
*vma)
+{
drivers/misc/lpc-ctrl.c line 52
Quoted 2 lines
drivers/misc/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;
+
Review
Quoted 3 lines
(quoted diff) +1 -0
+	if (vma->vm_pgoff + vsize > lpc_ctrl->base + lpc_ctrl-
Quoted 1 line
size)
(quoted diff) +1 -0
+		return -EINVAL;
+
drivers/misc/lpc-ctrl.c line 57
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	/* Other checks? */
+
drivers/misc/lpc-ctrl.c line 62
Quoted 4 lines
drivers/misc/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/lpc-ctrl.c new file +4 -0
+}
+
+static int lpc_ctrl_open(struct inode *inode, struct file *file)
+{
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/misc/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/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/lpc-ctrl.c new file +3 -0
+}
+
+static long lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
drivers/misc/lpc-ctrl.c line 95
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+		unsigned long param)
+{
drivers/misc/lpc-ctrl.c line 99
Quoted 3 lines
drivers/misc/lpc-ctrl.c new file +3 -0
+	struct lpc_mapping map;
+	struct lpc_ctrl *lpc_ctrl = file_lpc_ctrl(file);
+	void __user *p = (void __user *)param;
+
drivers/misc/lpc-ctrl.c line 107
Quoted 7 lines
drivers/misc/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;
+

Quoted 8 lines

drivers/misc/lpc-ctrl.c
Quoted 6 lines
Quoted 4 lines
drivers/misc/lpc-ctrl.c new file +3 -1
+		iowrite32(lpc_ctrl->base | (map.hostaddr >> 16),
+				lpc_ctrl->ctrl + LPC_HICR7);
+		iowrite32((~(map.size - 1)) | ((map.size >> 16)
- 1),
drivers/misc/lpc-ctrl.c new file +1 -0
+				lpc_ctrl->ctrl + LPC_HICR8);
I think these operations could do with some explanation.

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

Quoted 8 lines

drivers/misc/lpc-ctrl.c
Quoted 6 lines
drivers/misc/lpc-ctrl.c new file +1 -0
+		return 0;
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	case LPC_CTRL_IOCTL_UNMAP:
+		if (copy_from_user(&map, p, sizeof(map)))
drivers/misc/lpc-ctrl.c new file +1 -0
+			return -EFAULT;
Is this required? We don't use map below.

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

Quoted 11 lines

drivers/misc/lpc-ctrl.c
Quoted 8 lines
+
Quoted 4 lines
drivers/misc/lpc-ctrl.c new file +3 -0
+		iowrite32((0x3000 << 16) | (0x0e00),
+				lpc_ctrl->ctrl + LPC_HICR7);
+		iowrite32(((~(0x0200 - 1)) << 16) | ((0x0200) -
1),
drivers/misc/lpc-ctrl.c new file +1 -0
+				lpc_ctrl->ctrl + LPC_HICR8);
Can you comment/#define the magic numbers? I'm not across why we use these values.

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

Anyway, this is pointing the LPC bus back at the actual flash. The flash is at address 0x30000000 on the BMC (the 0x3000 represents that) and the SBE expects it to be mapped down from 0x0fffffff so for a flash of size 0x02000000 (HICR8 uses this...) then 0x0e000000 (the other nibble of HICR7) is where it should be on the host. Anyhow, not that important this will get replaced by asking the MTD layer which will be less hardcoded.

Quoted 53 lines

Quoted 53 lines
drivers/misc/lpc-ctrl.c new file +1 -0
+		return 0;
Review
Quoted 1 line
(quoted diff) +1 -0
+	}
+
Quoted 1 line
(quoted diff) +1 -0
+	return -EINVAL;
drivers/misc/lpc-ctrl.c new file +3 -0
+}
+
+static int lpc_ctrl_release(struct inode *inode, struct file
*file)
+{
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/misc/lpc-ctrl.c new file +3 -0
+}
+
+static const struct file_operations lpc_ctrl_fops = {
Quoted 7 lines
Quoted 7 lines
drivers/misc/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/lpc-ctrl.c new file +4 -0
+};
+
+static int lpc_ctrl_probe(struct platform_device *pdev)
+{
drivers/misc/lpc-ctrl.c line 150
Quoted 6 lines
drivers/misc/lpc-ctrl.c new file +6 -0
+	struct lpc_ctrl *lpc_ctrl;
+	struct device *dev;
+	struct device_node *node;
+	struct resource *res;
+	struct resource resm;
+	int rc;
+
drivers/misc/lpc-ctrl.c line 153
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	if (!pdev || !pdev->dev.of_node)
+		return -ENODEV;
+
drivers/misc/lpc-ctrl.c line 155
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	dev = &pdev->dev;
+
drivers/misc/lpc-ctrl.c line 159
Quoted 4 lines
(quoted diff) +1 -0
+	lpc_ctrl = devm_kzalloc(dev, sizeof(*lpc_ctrl),
GFP_KERNEL);
drivers/misc/lpc-ctrl.c new file +2 -0
+	if (!lpc_ctrl)
+		return -ENOMEM;
+
drivers/misc/lpc-ctrl.c line 161
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	dev_set_drvdata(&pdev->dev, lpc_ctrl);
+

Quoted 13 lines

drivers/misc/lpc-ctrl.c
Quoted 9 lines
Quoted 6 lines
drivers/misc/lpc-ctrl.c new file +6 -0
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Unable to find resources\n");
+		rc = -ENXIO;
+		goto out_free;
+	}
drivers/misc/lpc-ctrl.c new file +2 -0
+
+	/* Todo unmap this on fail cases and exit */
You're using devm_*, which if I understand correctly should handle this
for you.

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

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

Cyril

Quoted 143 lines

Quoted 54 lines
drivers/misc/lpc-ctrl.c new file +1 -0
+	lpc_ctrl->ctrl = devm_ioremap_resource(&pdev->dev, res);
drivers/misc/lpc-ctrl.c line 175
Quoted 4 lines
drivers/misc/lpc-ctrl.c new file +4 -0
+	if (!lpc_ctrl->ctrl) {
+		rc = -ENOMEM;
+		goto out_free;
+	}
+
drivers/misc/lpc-ctrl.c line 186
Quoted 12 lines
(quoted diff) +1 -0
+	node = of_parse_phandle(dev->of_node, "memory-region",
0);
drivers/misc/lpc-ctrl.c new file +3 -0
+	if (!node) {
+		/*
+		 * Should probaby handle this by allocating 4-
64k now and
drivers/misc/lpc-ctrl.c new file +6 -0
+		 * using that
+		 */
+		dev_err(dev, "Didn't find reserved memory\n");
+		rc = -EINVAL;
+		goto out;
+	}
+
drivers/misc/lpc-ctrl.c line 194
Quoted 7 lines
drivers/misc/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/lpc-ctrl.c line 197
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	lpc_ctrl->size = resource_size(&resm);
+	lpc_ctrl->base = resm.start;
+
drivers/misc/lpc-ctrl.c line 207
Quoted 9 lines
drivers/misc/lpc-ctrl.c new file +9 -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");
+		goto out;
+	}
+
Quoted 3 lines
drivers/misc/lpc-ctrl.c new file +3 -0
+	dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
+			lpc_ctrl->base, lpc_ctrl->size);
+	return 0;
drivers/misc/lpc-ctrl.c new file +2 -0
+
+out:
Quoted 1 line
(quoted diff) +1 -0
+	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
drivers/misc/lpc-ctrl.c line 216
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+out_free:
+	devm_kfree(dev, lpc_ctrl);
You shouldn't have to worry about these devm_*() calls.
drivers/misc/lpc-ctrl.c line 226
Quoted 12 lines
drivers/misc/lpc-ctrl.c new file +5 -0
+	return rc;
+}
+
+static int lpc_ctrl_remove(struct platform_device *pdev)
+{
drivers/misc/lpc-ctrl.c line 222
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+	struct lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
+
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	misc_deregister(&lpc_ctrl->miscdev);
+	devm_iounmap(&pdev->dev, lpc_ctrl->ctrl);
drivers/misc/lpc-ctrl.c new file +1 -0
+	devm_kfree(&pdev->dev, lpc_ctrl);
As above.
Andrew
Quoted 67 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	lpc_ctrl = NULL;
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/misc/lpc-ctrl.c new file +3 -0
+}
+
+static const struct of_device_id lpc_ctrl_match[] = {
Quoted 2 lines
drivers/misc/lpc-ctrl.c new file +2 -0
+	{ .compatible = "aspeed,lpc-ctrl" },
+	{ },
drivers/misc/lpc-ctrl.c new file +3 -0
+};
+
+static struct platform_driver lpc_ctrl_driver = {
Quoted 7 lines
drivers/misc/lpc-ctrl.c new file +1 -0
+	.driver = {
Quoted 1 line
drivers/misc/lpc-ctrl.c new file +1 -0
+		.name		= DEVICE_NAME,
drivers/misc/lpc-ctrl.c new file +4 -0
+		.of_match_table = lpc_ctrl_match,
+	},
+	.probe = lpc_ctrl_probe,
+	.remove = lpc_ctrl_remove,
drivers/misc/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/lpc-ctrl.c new file +1 -0
+MODULE_DESCRIPTION("Linux device interface to control LPC bus");
include/uapi/linux/lpc-ctrl.h new file +15 -0
new file mode 100644
index 0000000..c5f1caf
@@ -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/lpc-ctrl.h new file +2 -0
+	uint32_t hostaddr;
+	uint32_t size;
include/uapi/linux/lpc-ctrl.h new file +2 -0
+};
+
Quoted 7 lines
(quoted diff) +2 -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/lpc-ctrl.h new file +2 -0
+
+#endif /* _UAPI_LINUX_LPC_CTRL_H */

Cédric Le Goater

2016-12-13 10:24 UTC #20 lore raw

In reply to Cyril Bur

Quoted 3 lines · Cyril Bur

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

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

Cyril Bur

author

2016-12-14 00:13 UTC #21 lore raw

Quoted 7 lines · Cédric Le Goater

Quoted 3 lines · Cyril Bur
Cedric (or anyone really): Do you know of a way the lpc-ctrl driver could query the MTD subsystem for the size of the PNOR? There is currently a hack in the UNMAP ioctl() which can't hang around.
well, it is a platform device so you could loop on them I guess. Or maybe use register_mtd_user() ?

Oh yes looks like that will work.

Thanks,

Cyril

Quoted 2 lines

C.