drivers/misc: Add Aspeed LPC control driver
Subsystems (2)
PATCH v4 1/1 drivers/misc: Add Aspeed LPC control driver
In order to manage server systems, there is typically another processor known as a BMC (Baseboard Management Controller) which is responsible for powering the server and other various elements, sometimes fans, often the system flash.
The Aspeed BMC family which is what is used on OpenPOWER machines and a number of x86 as well is typically connected to the host via an LPC (Low Pin Count) bus (among others).
The LPC bus is an ISA bus on steroids. It's generally used by the BMC chip to provide the host with access to the system flash (via MEM/FW cycles) that contains the BIOS or other host firmware along with a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI controllers.
On the BMC chip side, this is all configured via a bunch of registers whose content is related to a given policy of what devices are exposed at a per system level, which is system/vendor specific, so we don't want to bolt that into the BMC kernel. This started with a need to provide something nicer than /dev/mem for user space to configure these things.
One important aspect of the configuration is how the MEM/FW space is exposed to the host (ie, the x86 or POWER). Some registers in that bridge can define a window remapping all or portion of the LPC MEM/FW space to a portion of the BMC internal bus, with no specific limits imposed in HW.
I think it makes sense to ensure that this window is configured by a kernel driver that can apply some serious sanity checks on what it is configured to map.
In practice, user space wants to control this by flipping the mapping between essentially two types of portions of the BMC address space:
- The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes are somewhat more complicated).
- One (or more) reserved area(s) of the BMC physical memory.
The latter is needed for a number of things, such as avoiding letting the host manipulate the innards of the BMC flash controller via some evil backdoor, we want to do flash updates by routing the window to a portion of memory (under control of a mailbox protocol via some separate set of registers) which the host can use to write new data in bulk and then request the BMC to flash it. There are other uses, such as allowing the host to boot from an in-memory flash image rather than the one in flash (very handy for continuous integration and test, the BMC can just download new images).
It is important to note that due to the way the Aspeed chip lets the kernel configure the mapping between host LPC addresses and BMC ram addresses the offset within the window must be a multiple of size. Not doing so will fragment the accessible space rather than simply moving 'zero' upwards. This is caused by the nature of HICR8 being a mask and the way host LPC addresses are translated.
---
v2:
Removed unused functions
Removed use of access_ok()
All input is evil
Reworked the interface as per Benjamin Herrenschmidts vision
V3:
Removed 'default y' from Kconfig
Reordered ioctl() struct fields
Reworeded some comments
V4:
Reorder ioctl() struct fields (again)
drivers/misc/Kconfig | 8 ++ drivers/misc/Makefile | 1 + drivers/misc/aspeed-lpc-ctrl.c | 263 +++++++++++++++++++++++++++++++++++ include/uapi/linux/aspeed-lpc-ctrl.h | 36 +++++ 4 files changed, 308 insertions(+) create mode 100644 drivers/misc/aspeed-lpc-ctrl.c create mode 100644 include/uapi/linux/aspeed-lpc-ctrl.h
drivers/misc/Kconfig +8 -0
@@ -766,6 +766,14 @@ config PANEL_BOOT_MESSAGE An empty message will only clear the display at driver init time. Any other printf()-formatted message is valid with newline and escape codes. +config ASPEED_LPC_CTRL + depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON + tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge control" + ---help--- + Control Aspeed ast2400/2500 HOST LPC to BMC mappings through + ioctl()s, the driver also provides a read/write interface to a BMC ram + region where the host LPC read/write region can be buffered. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +1 -0
@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ obj-$(CONFIG_PANEL) += panel.o +obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o
drivers/misc/aspeed-lpc-ctrl.c new file +263 -0
263 lines not on this page · raw message
include/uapi/linux/aspeed-lpc-ctrl.h new file +36 -0
@@ -0,0 +1,36 @@ +/* +* Copyright 2017 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_ASPEED_LPC_CTRL_H +#define _UAPI_LINUX_ASPEED_LPC_CTRL_H + +#include <linux/ioctl.h> + +/* Window types */ +#define ASPEED_LPC_CTRL_WINDOW_FLASH 1 +#define ASPEED_LPC_CTRL_WINDOW_MEMORY 2 + +struct aspeed_lpc_ctrl_mapping { +__u8 window_type; +__u8 window_id; +__u16 flags; +__u32 addr; +__u32 offset; +__u32 size; +}; + +#define __ASPEED_LPC_CTRL_IOCTL_MAGIC 0xb2 + +#define ASPEED_LPC_CTRL_IOCTL_GET_SIZE _IOWR(__ASPEED_LPC_CTRL_IOCTL_MAGIC, \ + 0x00, struct aspeed_lpc_ctrl_mapping) + +#define ASPEED_LPC_CTRL_IOCTL_MAP _IOW(__ASPEED_LPC_CTRL_IOCTL_MAGIC, \ + 0x01, struct aspeed_lpc_ctrl_mapping) + +#endif /* _UAPI_LINUX_ASPEED_LPC_CTRL_H */
Signature · 1 line
2.11.1
Quoted 8 lines · Cyril Bur
In order to manage server systems, there is typically another processor known as a BMC (Baseboard Management Controller) which is responsible for powering the server and other various elements, sometimes fans, often the system flash.
The Aspeed BMC family which is what is used on OpenPOWER machines and a number of x86 as well is typically connected to the host via an LPC (Low Pin Count) bus (among others).
Perhaps I missed the discussion on previous versions, but here my question. If it's available on x86, how you access it there? This driver seems too OF oriented which is not a case on most of x86 platforms.
Quoted 49 lines
The LPC bus is an ISA bus on steroids. It's generally used by the BMC chip to provide the host with access to the system flash (via MEM/FW cycles) that contains the BIOS or other host firmware along with a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI controllers.
On the BMC chip side, this is all configured via a bunch of registers whose content is related to a given policy of what devices are exposed at a per system level, which is system/vendor specific, so we don't want to bolt that into the BMC kernel. This started with a need to provide something nicer than
/dev/memfor user space to configure these things.One important aspect of the configuration is how the MEM/FW space is exposed to the host (ie, the x86 or POWER). Some registers in that bridge can define a window remapping all or portion of the LPC MEM/FW space to a portion of the BMC internal bus, with no specific limits imposed in HW.
I think it makes sense to ensure that this window is configured by a kernel driver that can apply some serious sanity checks on what it is configured to map.
In practice, user space wants to control this by flipping the mapping between essentially two types of portions of the BMC address space:
- The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes are somewhat more complicated).- One (or more) reserved area(s) of the BMC physical memory.
The latter is needed for a number of things, such as avoiding letting the host manipulate the innards of the BMC flash controller via some evil backdoor, we want to do flash updates by routing the window to a portion of memory (under control of a mailbox protocol via some separate set of registers) which the host can use to write new data in bulk and then request the BMC to flash it. There are other uses, such as allowing the host to boot from an in-memory flash image rather than the one in flash (very handy for continuous integration and test, the BMC can just download new images).
It is important to note that due to the way the Aspeed chip lets the kernel configure the mapping between host LPC addresses and BMC ram addresses the offset within the window must be a multiple of size. Not doing so will fragment the accessible space rather than simply moving 'zero' upwards. This is caused by the nature of HICR8 being a mask and the way host LPC addresses are translated.
Quoted 5 lines
drivers/misc/Kconfig | 8 ++ drivers/misc/Makefile | 1 + drivers/misc/aspeed-lpc-ctrl.c | 263 +++++++++++++++++++++++++++++++++++include/uapi/linux/aspeed-lpc-ctrl.h | 36 +++++
Since it's UAPI can it be more generic in case there will be other LPC implementations / devices?
drivers/misc/Makefile obj-$(CONFIG_ECHO) += echo/ · line 57
Quoted 8 lines
drivers/misc/Makefile +1
-0@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ obj-$(CONFIG_PANEL) += panel.o +obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
Does it suit best under misc?
drivers/misc/Makefile obj-$(CONFIG_ECHO) += echo/
Quoted 10 lines
drivers/misc/Makefile +9
-0+static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, + unsigned long param) +{ + long rc; + struct aspeed_lpc_ctrl_mapping map; + struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); + void __user *p = (void __user *)param; + u32 addr; + u32 size;
Reversed tree?
drivers/misc/Makefile
Quoted 7 lines
drivers/misc/Makefile +7
-0+ rc = regmap_write(lpc_ctrl->regmap, HICR7, + (addr | (map.addr >> 16))); + if (rc) + return rc; + + return regmap_write(lpc_ctrl->regmap, HICR8, + (~(map.size - 1)) | ((map.size >> 16) - 1));
Magic stuff above and here. Perhaps some helpers?
drivers/misc/Makefile
Quoted 8 lines
drivers/misc/Makefile +7
-0+ } + + return -EINVAL; +} + +static const struct file_operations aspeed_lpc_ctrl_fops = { + .owner = THIS_MODULE,
Do we still need this?
Quoted 4 lines
drivers/misc/Makefile +3
-0+ .mmap = aspeed_lpc_ctrl_mmap, + .unlocked_ioctl = aspeed_lpc_ctrl_ioctl, +};
drivers/misc/Makefile
Quoted 10 lines
drivers/misc/Makefile +9
-0+static int aspeed_lpc_ctrl_probe(struct platform_device *pdev) +{ + struct aspeed_lpc_ctrl *lpc_ctrl; + struct device *dev; + struct device_node *node; + struct resource resm; + int rc; + + dev = &pdev->dev;
You can do this in the definition block.
Quoted 5 lines
drivers/misc/Makefile +5
-0+ node = of_parse_phandle(dev->of_node, "flash", 0); + if (!node) { + dev_err(dev, "Didn't find host pnor flash node\n"); + return -ENODEV; + }
drivers/misc/Makefile
Quoted 10 lines
drivers/misc/Makefile +9
-0+ + rc = of_property_read_u32_index(node, "reg", 3, + &lpc_ctrl->pnor_size); + if (rc) + return rc; + rc = of_property_read_u32_index(node, "reg", 2, + &lpc_ctrl->pnor_base); + if (rc) + return rc;
Isn't something you may read at once?
drivers/misc/Makefile
Quoted 6 lines
drivers/misc/Makefile +5
-0+ lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR; + lpc_ctrl->miscdev.name = DEVICE_NAME; + lpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops; + lpc_ctrl->miscdev.parent = dev; + rc = misc_register(&lpc_ctrl->miscdev);
No devm_ variant?
drivers/misc/Makefile
Quoted 4 lines
drivers/misc/Makefile +4
-0+ if (rc) + dev_err(dev, "Unable to register device\n"); + else + dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",
%pa
%pa
drivers/misc/Makefile
Quoted 13 lines
drivers/misc/Makefile +12
-0+ lpc_ctrl->mem_base, lpc_ctrl->mem_size); + + return rc; +} +struct aspeed_lpc_ctrl_mapping { + __u8 window_type; + __u8 window_id; + __u16 flags; + __u32 addr; + __u32 offset; + __u32 size; +};
It will suck. So, no room for manoeuvre?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 6 lines · Andy Shevchenko
Quoted 6 lines · Cyril Bur
In order to manage server systems, there is typically another processor known as a BMC (Baseboard Management Controller) which is responsible for powering the server and other various elements, sometimes fans, often the system flash.
Quoted 8 lines
Quoted 3 lines
The Aspeed BMC family which is what is used on OpenPOWER machines and a number of x86 as well is typically connected to the host via an LPC (Low Pin Count) bus (among others).
Perhaps I missed the discussion on previous versions, but here my question. If it's available on x86, how you access it there? This driver seems too OF oriented which is not a case on most of x86 platforms.
This is the BMC (ie salve) side of the LPC bus which is an Aspeed ARM chip whose kernel is device-tree based.
What happens on the "host" side (x86, powerpc, arm64, ...) is not directly relevant here.
.../...
Quoted 11 lines
Quoted 10 lines
It is important to note that due to the way the Aspeed chip lets the kernel configure the mapping between host LPC addresses and BMC ram addresses the offset within the window must be a multiple of size. Not doing so will fragment the accessible space rather than simply moving 'zero' upwards. This is caused by the nature of HICR8 being a mask and the way host LPC addresses are translated. drivers/misc/Kconfig | 8 ++ drivers/misc/Makefile | 1 + drivers/misc/aspeed-lpc-ctrl.c | 263 +++++++++++++++++++++++++++++++++++
include/uapi/linux/aspeed-lpc-ctrl.h| 36 +++++
Quoted 2 lines
Since it's UAPI can it be more generic in case there will be other LPC implementations / devices?
Not really. As I wrote above, this isn't exposing an LPC bus the way you seem to think of it, it's about the *BMC* side of the LPC bus (the slave side) where some fairly chip/board configuration is needed to do things like control the mapping of part of the LPC FW space to the SPI flash or control which LPC devices are made visible to the host etc...
This is meant to be used by OpenBMC to configure the LPC bus properly on the BMC side so the host can boot.
Quoted 9 lines
Quoted 7 lines
drivers/misc/Makefile +0
-0@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO) += echo/obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ obj-$(CONFIG_PANEL) += panel.o +obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
Does it suit best under misc?
Yes. It's basically a chardev giving access to a few very specific registers. I've already explained all of this in series of emails...
It could *almost* be UIO except for the fact that one thing that needs to be configured is the mapping between the LPC bus FW space and some region of the SoC address space, either the SPI controller or some reserved memory used for flash operations, and there is benefit in doing so in the kernel for security purposes as doing so incorrectly would expose the BMC internal physical address space to the host.
Quoted 11 lines
drivers/misc/Makefile
Quoted 9 lines
drivers/misc/Makefile +9
-0+static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd, + unsigned long param) +{ + long rc; + struct aspeed_lpc_ctrl_mapping map; + struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file); + void __user *p = (void __user *)param; + u32 addr; + u32 size;Reversed tree?
Ugh ? One of those new ridiculous coding style rules ?
Quoted 9 lines
Quoted 7 lines
+ rc = regmap_write(lpc_ctrl->regmap, HICR7, + (addr | (map.addr >> 16))); + if (rc)
+ return rc;
+
+ return regmap_write(lpc_ctrl->regmap, HICR8, + (~(map.size - 1)) | ((map.size >> 16) - 1));Magic stuff above and here. Perhaps some helpers?
A helper might be warranted but it's not *that* magic ... pretty obvious what that does ;-)
Quoted 40 lines
drivers/misc/Makefile
Quoted 7 lines
+ }
drivers/misc/Makefile +6
-0+ + return -EINVAL; +} + +static const struct file_operations aspeed_lpc_ctrl_fops = { + .owner = THIS_MODULE,Do we still need this?
drivers/misc/Makefile
Quoted 12 lines
+ .mmap =
aspeed_lpc_ctrl_mmap,
+ .unlocked_ioctl =aspeed_lpc_ctrl_ioctl,drivers/misc/Makefile +10
-0+}; +static int aspeed_lpc_ctrl_probe(struct platform_device *pdev) +{ + struct aspeed_lpc_ctrl *lpc_ctrl; + struct device *dev; + struct device_node *node; + struct resource resm; + int rc; + + dev = &pdev->dev;You can do this in the definition block.
Quoted 6 lines
+ node =
of_parse_phandle(dev->of_node, "flash", 0); + if (!node) {
+ dev_err(dev, "Didn't find host pnor flash node\n"); + return-ENODEV;
+ }
+Quoted 8 lines
+ rc =
of_property_read_u32_index(node, "reg", 3, + &lpc_ctrl->pnor_size); + if (rc)
+ return rc;
+ rc =of_property_read_u32_index(node, "reg", 2, + &lpc_ctrl->pnor_base); + if (rc)
+ return rc;
Quoted 1 line
Isn't something you may read at once?
Actually it should be parsed using of_address_to_resource indeed in case there's any translation in the DT.
Quoted 7 lines
Quoted 5 lines
+ lpc_ctrl->miscdev.minor =
MISC_DYNAMIC_MINOR; + lpc_ctrl->miscdev.name =DEVICE_NAME; + lpc_ctrl->miscdev.fops =&aspeed_lpc_ctrl_fops; + lpc_ctrl->miscdev.parent = dev;
+ rc =misc_register(&lpc_ctrl->miscdev);No
devm_variant?
No real need here, is there ?
Quoted 8 lines
Quoted 4 lines
+ if (rc)
+ dev_err(dev, "Unable to register device\n"); + else
+ dev_info(dev, "Loaded at 0x%08x (0x%08x)\n",%pa
%pa
Quoted 14 lines
drivers/misc/Makefile
Quoted 12 lines
+ lpc_ctrl->mem_base,
lpc_ctrl->mem_size);drivers/misc/Makefile +11
-0+ + return rc; +} +struct aspeed_lpc_ctrl_mapping { + __u8 window_type; + __u8 window_id; + __u16 flags; + __u32 addr; + __u32 offset; + __u32 size; +};It will suck. So, no room for manoeuvre?
What do you mean ?
Cheers,
Ben.
Greg KH
maintainerQuoted 57 lines · Cyril Bur
In order to manage server systems, there is typically another processor known as a BMC (Baseboard Management Controller) which is responsible for powering the server and other various elements, sometimes fans, often the system flash.
The Aspeed BMC family which is what is used on OpenPOWER machines and a number of x86 as well is typically connected to the host via an LPC (Low Pin Count) bus (among others).
The LPC bus is an ISA bus on steroids. It's generally used by the BMC chip to provide the host with access to the system flash (via MEM/FW cycles) that contains the BIOS or other host firmware along with a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI controllers.
On the BMC chip side, this is all configured via a bunch of registers whose content is related to a given policy of what devices are exposed at a per system level, which is system/vendor specific, so we don't want to bolt that into the BMC kernel. This started with a need to provide something nicer than
/dev/memfor user space to configure these things.One important aspect of the configuration is how the MEM/FW space is exposed to the host (ie, the x86 or POWER). Some registers in that bridge can define a window remapping all or portion of the LPC MEM/FW space to a portion of the BMC internal bus, with no specific limits imposed in HW.
I think it makes sense to ensure that this window is configured by a kernel driver that can apply some serious sanity checks on what it is configured to map.
In practice, user space wants to control this by flipping the mapping between essentially two types of portions of the BMC address space:
- The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes are somewhat more complicated).- One (or more) reserved area(s) of the BMC physical memory.
The latter is needed for a number of things, such as avoiding letting the host manipulate the innards of the BMC flash controller via some evil backdoor, we want to do flash updates by routing the window to a portion of memory (under control of a mailbox protocol via some separate set of registers) which the host can use to write new data in bulk and then request the BMC to flash it. There are other uses, such as allowing the host to boot from an in-memory flash image rather than the one in flash (very handy for continuous integration and test, the BMC can just download new images).
It is important to note that due to the way the Aspeed chip lets the kernel configure the mapping between host LPC addresses and BMC ram addresses the offset within the window must be a multiple of size. Not doing so will fragment the accessible space rather than simply moving 'zero' upwards. This is caused by the nature of HICR8 being a mask and the way host LPC addresses are translated.
Quoted 2 lines
Signed-off-by Cyril Bur <[email protected]>---
Without some other reviewed-by: or at least tested-by lines here, I'm not going to take this. Go poke your fellow ppc people to do some work here, it shouldn't be up to me to do it for them :(
thanks,
greg k-h
Patrick Williams
Tested-byQuoted 11 lines · Greg KH
Quoted 2 lines · Cyril Bur
Signed-off-by Cyril Bur <[email protected]>---
Without some other reviewed-by: or at least tested-by lines here, I'm not going to take this. Go poke your fellow ppc people to do some work here, it shouldn't be up to me to do it for them :(
thanks,
greg k-h
We are actively using this driver in the OpenBMC project and have exercised it in booting two different server variants (different motherboards entirely).
Signature · 1 line
Patrick Williams
Quoted 5 lines · Greg KH
Without some other reviewed-by: or at least tested-by lines here, I'm not going to take this. Go poke your fellow ppc people to do some work
here, it shouldn't be up to me to do it for them :(
We are reviewing it ;-) We asked for a change in the reading of the DT "reg" property (he should use the appropriate helpers rather than do it by hand). I'll send my reviewed-by once that has been addressed.
Cheers,
Ben.
Joel Stanley
maintainerHey Greg,
Quoted 9 lines · Greg KH
Quoted 4 lines · Cyril Bur
In order to manage server systems, there is typically another processor known as a BMC (Baseboard Management Controller) which is responsible for powering the server and other various elements, sometimes fans, often the system flash.
Without some other reviewed-by: or at least tested-by lines here, I'm not going to take this. Go poke your fellow ppc people to do some work here, it shouldn't be up to me to do it for them :(
We're on it. Thanks for your review so far.
By the way, this is a driver for an ARM SoC, not a PPC chip. Nevertheless I'm sure those PPC people will still give us a review or two.
Cheers,
Joel
include/uapi/linux/aspeed-lpc-ctrl.h line 27
Quoted 9 lines · Cyril Bur
include/uapi/linux/aspeed-lpc-ctrl.h new file +9
-0+struct aspeed_lpc_ctrl_mapping { + __u8 window_type; + __u8 window_id; + __u16 flags; + __u32 addr; + __u32 offset; + __u32 size; +}; +
Please document the fields
Cheers,
Ben.