[PATCH linux dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver
Joel Stanley
joel at jms.id.au
Fri Dec 8 14:24:42 AEDT 2017
Hello!
On Fri, Dec 8, 2017 at 1:04 PM, Haiyue Wang <haiyue.wang at linux.intel.com> wrote:
> This patch adds a simple device driver to expose the KCS interface on
> Aspeed SOC (AST2500) as a character device. Such SOC is commonly used
> as BMC (BaseBoard Management Controllers) and this driver implements
> the BMC side of the KCS interface.
>
> The KCS (Keyboard Controller Style) interface is used to perform in-band
> IPMI communication between a host and its BMC.
>
> The device name defaults to '/dev/ipmi-kcsX', X=1,2,3,4.
>
> Signed-off-by: Haiyue Wang <haiyue.wang at linux.intel.com>
Thanks for the patch! This looks like a good start.
Can I ask that we do the review for this on the upstream list? Use
scripts/get_maintainers.pl on your patches to discover the upstream
maintainer. In addition, please cc myself and the OpenBMC list. I
encourage you to do this for any work newly submitted on upstream
drivers.
I'll give this a review now though.
Some general notes:
- Send your device tree bindings in a separate patch
- Put the updates to the device tree themselves in their own patch
- Run the patches through scripts/checkpatch.pl before submitting
> ---
> .../devicetree/bindings/mfd/aspeed-lpc.txt | 33 +-
> arch/arm/boot/dts/aspeed-g5.dtsi | 44 +-
> drivers/char/ipmi/Kconfig | 10 +
> drivers/char/ipmi/Makefile | 1 +
> drivers/char/ipmi/kcs-bmc.c | 827 +++++++++++++++++++++
> include/uapi/linux/kcs-bmc.h | 14 +
> 6 files changed, 925 insertions(+), 4 deletions(-)
> create mode 100644 drivers/char/ipmi/kcs-bmc.c
> create mode 100644 include/uapi/linux/kcs-bmc.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> index a97131a..1e9d119 100644
> --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> @@ -62,12 +62,25 @@ BMC Node
> --------
>
> - compatible: One of:
> - "aspeed,ast2400-lpc-bmc"
> - "aspeed,ast2500-lpc-bmc"
> + "aspeed,ast2400-lpc-bmc", "simple-mfd", "syscon"
> + "aspeed,ast2500-lpc-bmc", "simple-mfd", "syscon"
>
> - reg: contains the physical address and length values of the
> H8S/2168-compatible LPC controller memory region
>
> +BMC KCS Node
> +------------
> +
> +- compatible: One of:
> + "aspeed,ast2500-kcs-bmc"
This hardware doesn't exist on the ast2400?
> +
> +- kcs_chan: The LPC channel number
> +
> +- kcs_regs: The KCS IDR,ODR,STR registers
> +
> +- kcs_addr: The host CPU IO map address
> +
> +
> Host Node
> ---------
>
> @@ -94,8 +107,22 @@ lpc: lpc at 1e789000 {
> ranges = <0x0 0x1e789000 0x1000>;
>
> lpc_bmc: lpc-bmc at 0 {
Ah, the lpc-bmc node. This is Andrew's work, please add him to CC in
the future so he can help review.
> - compatible = "aspeed,ast2500-lpc-bmc";
> + compatible = "aspeed,ast2500-lpc-bmc"; "simple-mfd", "syscon";
Can you explain why we add the simple-mfd property here? The lpc
device already has this, which enables us to have a regmap across the
address space. If there's a further requirement then please explain
it.
> reg = <0x0 0x80>;
> + reg-io-width = <4>;
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x0 0x80>;
> +
> + kcs3: kcs at 3 {
It's convention that the unit name has a corresponding reg property.
It looks like you should use reg = <3>, which means you don't need the
custom kcs_chan property.
> + compatible = "aspeed,ast2500-kcs-bmc";
> + interrupts = <8>;
> + kcs_chan = <3>;
> + kcs_regs = <0x2C 0x38 0x44>;
I suggest placing these register offsets in the driver.
There are a number of channel specific offsets, registers and masks
that need to be specified. From what I can see you specify some here
in the device tree, and others are selected in the switch statements
in your code. It's a good goal to make a code generic with the
hardware differences specified in the device tree, but in this case
it's not worth it.
I suggest a data structure that contains the registers, enable bits
and mask, address bits and mask, etc. This way you can store a pointer
to that data structure containing all of the information, and you
don't need the switch statements in the code.
struct kcs_channel {
u32 idr;
u32 odr
u32 str;
u32 en_reg1;
u32 en_mask1;
u32 en_reg2;
u32 en_mask2;
};
static const struct kcs_channels[4] =
{ .idr = 0x2C,
.odr = 0x38,
.str = 0x44,
.en_reg1 = LPC_HICR2,
.en_mask1 = LPC_HICR2_IBFIF3
etc.
> + kcs_addr = <0xCA2>;
> + status = "okay";
> + };
> };
>
> lpc_host: lpc-host at 80 {
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> index 90f3edf..9f8589f 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -85,3 +85,13 @@ config ASPEED_BT_IPMI_BMC
> Provides a driver for the BT (Block Transfer) IPMI interface
> found on Aspeed SOCs (AST2400 and AST2500). The driver
> implements the BMC side of the BT interface.
> +
> +config ASPEED_KCS_IPMI_BMC
> + depends on ARCH_ASPEED || COMPILE_TEST
> + depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
> + tristate "KCS IPMI bmc driver"
> + default n
n is the default, so you don't need to specify it.
> + help
> + Provides a driver for the KCS (Keyboard Controller Style) IPMI
> + interface found on Aspeed SOCs (AST2500). The driver implements
> + the BMC side of the KCS interface.
> \ No newline at end of file
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index 0d98cd9..35272a7 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
> obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
> obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
> obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
> +obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs-bmc.o
> \ No newline at end of file
> diff --git a/drivers/char/ipmi/kcs-bmc.c b/drivers/char/ipmi/kcs-bmc.c
> new file mode 100644
> index 0000000..720c8b7
> --- /dev/null
> +++ b/drivers/char/ipmi/kcs-bmc.c
> @@ -0,0 +1,827 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2015-2017, Intel Corporation.
> +
> +#include <linux/atomic.h>
> +#include <linux/slab.h>
> +#include <linux/kcs-bmc.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/regmap.h>
> +#include <linux/sched.h>
> +#include <linux/timer.h>
> +
> +/*
> + * This is a BMC device used to communicate to the host
> + */
> +#define DEVICE_NAME "ipmi-kcs-host"
> +
> +
> +/* Different Phases of the KCS Module */
> +#define KCS_PHASE_IDLE 0x00
> +#define KCS_PHASE_WRITE 0x01
> +#define KCS_PHASE_WRITE_END 0x02
> +#define KCS_PHASE_READ 0x03
> +#define KCS_PHASE_ABORT 0x04
> +#define KCS_PHASE_ERROR 0x05
> +
> +/* Abort Phase */
> +#define ABORT_PHASE_ERROR1 0x01
> +#define ABORT_PHASE_ERROR2 0x02
> +
> +/* KCS Command Control codes. */
> +#define KCS_GET_STATUS 0x60
> +#define KCS_ABORT 0x60
> +#define KCS_WRITE_START 0x61
> +#define KCS_WRITE_END 0x62
> +#define KCS_READ_BYTE 0x68
> +
> +/* Status bits.:
> + * - IDLE_STATE. Interface is idle. System software should not be expecting
> + * nor sending any data.
> + * - READ_STATE. BMC is transferring a packet to system software. System
> + * software should be in the "Read Message" state.
> + * - WRITE_STATE. BMC is receiving a packet from system software. System
> + * software should be writing a command to the BMC.
> + * - ERROR_STATE. BMC has detected a protocol violation at the interface level,
> + * or the transfer has been aborted. System software can either
> + * use the "Get_Status" control code to request the nature of
> + * the error, or it can just retry the command.
> + */
> +#define KCS_IDLE_STATE 0
> +#define KCS_READ_STATE 1
> +#define KCS_WRITE_STATE 2
> +#define KCS_ERROR_STATE 3
> +
> +/* KCS Error Codes */
> +#define KCS_NO_ERROR 0x00
> +#define KCS_ABORTED_BY_COMMAND 0x01
> +#define KCS_ILLEGAL_CONTROL_CODE 0x02
> +#define KCS_LENGTH_ERROR 0x06
> +#define KCS_UNSPECIFIED_ERROR 0xFF
> +
> +
> +#define KCS_ZERO_DATA 0
> +
> +/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */
> +#define KCS_STR_STATE(state) (state << 6)
> +#define KCS_STR_STATE_MASK KCS_STR_STATE(0x3)
> +#define KCS_STR_CMD_DAT (1 << 3)
> +#define KCS_STR_ATN (1 << 2)
> +#define KCS_STR_IBF (1 << 1)
You can use the BIT() macro for these.
> +#define KCS_STR_OBF (1)
> +
> +
> +/***************************** LPC Register ****************************/
Drop the ASCII art.
> +#define LPC_HICR0 0x000
> +#define LPC_HICR0_LPC3E (1 << 7)
> +#define LPC_HICR0_LPC2E (1 << 6)
> +#define LPC_HICR0_LPC1E (1 << 5)
BIT() macro again.
> +#define LPC_HICR2 0x008
> +#define LPC_HICR2_IBFIF3 (1 << 3)
> +#define LPC_HICR2_IBFIF2 (1 << 2)
> +#define LPC_HICR2_IBFIF1 (1 << 1)
> +#define LPC_HICR4 0x010
> +#define LPC_HICR4_LADR12AS (1 << 7)
> +#define LPC_HICR4_KCSENBL (1 << 2)
> +#define LPC_LADR3H 0x014
> +#define LPC_LADR3L 0x018
> +#define LPC_LADR12H 0x01C
> +#define LPC_LADR12L 0x020
> +
> +#define LPC_HICRB 0x000
> +#define LPC_HICRB_IBFIF4 (1 << 1)
> +#define LPC_HICRB_LPC4E (1 << 0)
> +#define LPC_LADR4 0x010
> +
> +
> +#define KCS_MSG_BUFSIZ 1024
> +#define KCS_CHANNEL_MAX 4
> +
I'll review the implementation in a later patch, once we have the
bindings worked out.
> +
> +static int kcs_bmc_probe(struct platform_device *pdev)
> +{
> + struct kcs_bmc *kcs_bmc;
> + struct device *dev;
> + u32 chan, addr, io_regs[3];
> + int rc;
> +
> + if (!pdev || !pdev->dev.of_node)
> + return -ENODEV;
Omit these checks.
> +
> + dev = &pdev->dev;
> +
> + rc = of_property_read_u32(dev->of_node, "kcs_chan", &chan);
> + if (rc) {
> + dev_err(dev, "no 'kcs_chan' configured\n");
> + return -ENODEV;
> + }
> +
> + if (chan == 0 || chan > KCS_CHANNEL_MAX) {
> + dev_err(dev, "invalid 'kcs_chan' = '%u' is configured\n", chan);
> + return -ENODEV;
> + }
> +
> + rc = of_property_read_u32(dev->of_node, "kcs_addr", &addr);
> + if (rc) {
> + dev_err(dev, "no 'kcs_addr' configured\n");
> + return -ENODEV;
> + }
> +
> + rc = of_property_read_u32_array(dev->of_node, "kcs_regs", io_regs,
> + ARRAY_SIZE(io_regs));
> + if (rc) {
> + dev_err(dev, "no 'kcs_regs' configured\n");
> + return -ENODEV;
> + }
> +
> + kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc), GFP_KERNEL);
> + if (!kcs_bmc)
> + return -ENOMEM;
> +
> + kcs_bmc->map = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(kcs_bmc->map)) {
> + struct resource *res;
> + void __iomem *base;
> +
> + /*
> + * Assume it's not the MFD-based devicetree description, in
> + * which case generate a regmap ourselves
> + */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base)) {
> + dev_err(dev, "ioremap error\n");
> + return PTR_ERR(base);
> + }
> +
> + kcs_bmc->map = devm_regmap_init_mmio(dev, base,
> + &kcs_regmap_cfg);
> + if (IS_ERR(kcs_bmc->map)) {
> + dev_err(dev, "regmap init error\n");
> + return PTR_ERR(base);
> + }
> +
> + kcs_bmc->offset = 0;
> + } else {
> + rc = of_property_read_u32(dev->of_node, "reg",
> + &kcs_bmc->offset);
> + if (rc) {
> + rc = of_property_read_u32(dev->parent->of_node, "reg",
> + &kcs_bmc->offset);
> + if (rc) {
> + dev_err(dev, "no 'reg' configured\n");
> + return rc;
> + }
> + }
Perhaps this code was copied from the BT driver? It is in place to
support some old device tree bindings that we don't use. As your
bindings specify a syscon, you can omit the else path of this code.
> + }
> +
> + spin_lock_init(&kcs_bmc->lock);
> + kcs_bmc->chan = chan;
> + kcs_bmc->idr = io_regs[0];
> + kcs_bmc->odr = io_regs[1];
> + kcs_bmc->str = io_regs[2];
> +
> + init_waitqueue_head(&kcs_bmc->queue);
> + kcs_bmc->data_in = kmalloc(KCS_MSG_BUFSIZ, GFP_KERNEL);
> + kcs_bmc->data_out = kmalloc(KCS_MSG_BUFSIZ, GFP_KERNEL);
A tip: if you use devm_kzalloc, the kernel device driver
infrastructure will free these buffers when the driver is removed, or
if the probe fails.
> + if (kcs_bmc->data_in == NULL || kcs_bmc->data_out == NULL) {
> + rc = -ENOMEM;
> + dev_err(dev, "KCS%u failed data_in=%p, data_out=%p\n",
> + kcs_bmc->chan,
> + kcs_bmc->data_in,
> + kcs_bmc->data_out);
> + goto bail;
> + }
> +
More information about the openbmc
mailing list