diff mbox series

[linux,dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver

Message ID 1512700492-22384-1-git-send-email-haiyue.wang@linux.intel.com
State Changes Requested, archived
Headers show
Series [linux,dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver | expand

Commit Message

Haiyue Wang Dec. 8, 2017, 2:34 a.m. UTC
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@linux.intel.com>
---
 .../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

Comments

Joel Stanley Dec. 8, 2017, 3:24 a.m. UTC | #1
Hello!

On Fri, Dec 8, 2017 at 1:04 PM, Haiyue Wang <haiyue.wang@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@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@1e789000 {
>         ranges = <0x0 0x1e789000 0x1000>;
>
>         lpc_bmc: lpc-bmc@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@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@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;
> +       }
> +
Haiyue Wang Dec. 8, 2017, 5:54 a.m. UTC | #2
Great notes, will update a new patch later, thanks Joel.

I implemented the kcs by referenced the upstream bt driver. And the upstream maintainer is as bellowing,
I hope the openbmc expert to review my patch firstly, will send to the maintainer soon. :-)

./scripts/get_maintainer.pl drivers/char/ipmi/bt-bmc.c
Corey Minyard <minyard@acm.org> (supporter:IPMI SUBSYSTEM)
openipmi-developer@lists.sourceforge.net (moderated list:IPMI SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)


-----Original Message-----
From: joel.stan@gmail.com [mailto:joel.stan@gmail.com] On Behalf Of Joel Stanley
Sent: Friday, December 8, 2017 11:25
To: Haiyue Wang <haiyue.wang@linux.intel.com>; Andrew Jeffery <andrew@aj.id.au>; Jeremy Kerr <jk@ozlabs.org>; Cédric Le Goater <clg@kaod.org>
Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH linux dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver

Hello!

On Fri, Dec 8, 2017 at 1:04 PM, Haiyue Wang <haiyue.wang@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@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@1e789000 {
>         ranges = <0x0 0x1e789000 0x1000>;
>
>         lpc_bmc: lpc-bmc@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@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@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;
> +       }
> +
Joel Stanley Dec. 8, 2017, 6:01 a.m. UTC | #3
On Fri, Dec 8, 2017 at 4:24 PM, Haiyue Wang <haiyue.wang@linux.intel.com> wrote:
> Great notes, will update a new patch later, thanks Joel.
>
> I implemented the kcs by referenced the upstream bt driver. And the upstream maintainer is as bellowing,
> I hope the openbmc expert to review my patch firstly, will send to the maintainer soon. :-)
>
> ./scripts/get_maintainer.pl drivers/char/ipmi/bt-bmc.c
> Corey Minyard <minyard@acm.org> (supporter:IPMI SUBSYSTEM)
> openipmi-developer@lists.sourceforge.net (moderated list:IPMI SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)

Yep, add to that list:

openbmc@lists.ozlabs.org
clg@fr.ibm.com
jk@ozlabs.org
andrew@aj.id.au
joel@jms.id.au

In the past we have spent a long time pre-reviewing on the openbmc
list, and then had another extensive review process on the upstream
list. I hope it will help us get the code in a mergeable state faster
by doing the work upstream, with the OpenBMC team providing input.

Cheers,

Joel
Cédric Le Goater Dec. 8, 2017, 8:12 a.m. UTC | #4
On 12/08/2017 07:01 AM, Joel Stanley wrote:
> On Fri, Dec 8, 2017 at 4:24 PM, Haiyue Wang <haiyue.wang@linux.intel.com> wrote:
>> Great notes, will update a new patch later, thanks Joel.
>>
>> I implemented the kcs by referenced the upstream bt driver. And the upstream maintainer is as bellowing,
>> I hope the openbmc expert to review my patch firstly, will send to the maintainer soon. :-)
>>
>> ./scripts/get_maintainer.pl drivers/char/ipmi/bt-bmc.c
>> Corey Minyard <minyard@acm.org> (supporter:IPMI SUBSYSTEM)
>> openipmi-developer@lists.sourceforge.net (moderated list:IPMI SUBSYSTEM)
>> linux-kernel@vger.kernel.org (open list)
> 
> Yep, add to that list:
> 
> openbmc@lists.ozlabs.org
> clg@fr.ibm.com
> jk@ozlabs.org
> andrew@aj.id.au
> joel@jms.id.au

and what about the linux-aspeed@lists.ozlabs.org ? 

C.

> In the past we have spent a long time pre-reviewing on the openbmc
> list, and then had another extensive review process on the upstream
> list. I hope it will help us get the code in a mergeable state faster
> by doing the work upstream, with the OpenBMC team providing input.
> 
> Cheers,
> 
> Joel
>
Haiyue Wang Dec. 8, 2017, 12:40 p.m. UTC | #5
Thanks C. 'linux-aspeed' should be a good reviewer. :)


Hi Andrew & Joel,

> @@ -94,8 +107,22 @@ lpc: lpc@1e789000 {
>         ranges = <0x0 0x1e789000 0x1000>;
>
>         lpc_bmc: lpc-bmc@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.
 
  Haiyue: I tried to remove this property, found that we had to add "simple-mfd" property into "aspeed,ast2500-lpc-bmc" node. If not, the nested node kcs1/2/3 can't be loaded successfully.

>                 reg = <0x0 0x80>;
> +               reg-io-width = <4>;
> +
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0x0 0x0 0x80>;
> +
> +               kcs3: kcs@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.

 Haiyue: I added the ' reg = <0x0 0x80> ' property for memory/io space use, and use 'kcs_chan' for channel selection in kcs driver.

> +                       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.

Haiyue: This makes code and device tree more clean, I over-used the device tree. :-). And I defined bellowing, and kept the enable/disable channel
as before. Because you can see that enable / disable have the register setting with different order, and registers number are different also. So for
making the code clean, and let people easily understand the code by checking data sheet, I kept this. :)

/* IPMI 2.0 : 9.5 - KCS Interface Registers */
struct kcs_ioreg {
	u32 idr;
	u32 odr;
	u32 str;
};

static const struct kcs_ioreg kcs_channel_ioregs[KCS_CHANNEL_MAX] = {
	{ .idr = 0x24, .odr = 0x30, .str =  0x3C },
	{ .idr = 0x28, .odr = 0x34, .str =  0x40 },
	{ .idr = 0x2C, .odr = 0x38, .str =  0x44 },
	{ .idr = 0x94, .odr = 0x98, .str =  0x9C },
};

static void kcs_enable_channel(struct kcs_bmc *kcs_bmc, int enable)
{
	switch (kcs_bmc->chan) {
	case 1:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC1E, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF1, 0);
		}
		break;

	case 2:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC2E, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF2, 0);
		}
		break;

	case 3:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
			regmap_update_bits(kcs_bmc->map, LPC_HICR4,
					LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICR0,
					LPC_HICR0_LPC3E, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR4,
					LPC_HICR4_KCSENBL, 0);
			regmap_update_bits(kcs_bmc->map, LPC_HICR2,
					LPC_HICR2_IBFIF3, 0);
		}
		break;

	case 4:
		if (enable) {
			regmap_update_bits(kcs_bmc->map, LPC_HICRB,
					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E);
		} else {
			regmap_update_bits(kcs_bmc->map, LPC_HICRB,
					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
					0);
		}
		break;

	default:
		break;
	}
}

BR,
Haiyue




-----Original Message-----
From: Cédric Le Goater [mailto:clg@kaod.org] 
Sent: Friday, December 8, 2017 16:12
To: Joel Stanley <joel@jms.id.au>; Haiyue Wang <haiyue.wang@linux.intel.com>
Cc: Andrew Jeffery <andrew@aj.id.au>; Jeremy Kerr <jk@ozlabs.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH linux dev-4.10] ipmi: add an Aspeed KCS IPMI BMC driver

On 12/08/2017 07:01 AM, Joel Stanley wrote:
> On Fri, Dec 8, 2017 at 4:24 PM, Haiyue Wang <haiyue.wang@linux.intel.com> wrote:
>> Great notes, will update a new patch later, thanks Joel.
>>
>> I implemented the kcs by referenced the upstream bt driver. And the 
>> upstream maintainer is as bellowing, I hope the openbmc expert to 
>> review my patch firstly, will send to the maintainer soon. :-)
>>
>> ./scripts/get_maintainer.pl drivers/char/ipmi/bt-bmc.c Corey Minyard 
>> <minyard@acm.org> (supporter:IPMI SUBSYSTEM) 
>> openipmi-developer@lists.sourceforge.net (moderated list:IPMI 
>> SUBSYSTEM) linux-kernel@vger.kernel.org (open list)
> 
> Yep, add to that list:
> 
> openbmc@lists.ozlabs.org
> clg@fr.ibm.com
> jk@ozlabs.org
> andrew@aj.id.au
> joel@jms.id.au

and what about the linux-aspeed@lists.ozlabs.org ? 

C.

> In the past we have spent a long time pre-reviewing on the openbmc 
> list, and then had another extensive review process on the upstream 
> list. I hope it will help us get the code in a mergeable state faster 
> by doing the work upstream, with the OpenBMC team providing input.
> 
> Cheers,
> 
> Joel
>
diff mbox series

Patch

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"
+
+- 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@1e789000 {
 	ranges = <0x0 0x1e789000 0x1000>;
 
 	lpc_bmc: lpc-bmc@0 {
-		compatible = "aspeed,ast2500-lpc-bmc";
+		compatible = "aspeed,ast2500-lpc-bmc"; "simple-mfd", "syscon";
 		reg = <0x0 0x80>;
+		reg-io-width = <4>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0 0x80>;
+
+		kcs3: kcs@3 {
+			compatible = "aspeed,ast2500-kcs-bmc";
+			interrupts = <8>;
+			kcs_chan = <3>;
+			kcs_regs = <0x2C 0x38 0x44>;
+			kcs_addr = <0xCA2>;
+			status = "okay";
+		};
 	};
 
 	lpc_host: lpc-host@80 {
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index ba3607c..90f260d 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -135,8 +135,40 @@ 
 			ranges = <0x0 0x1e789000 0x1000>;
 
 			lpc_bmc: lpc-bmc@0 {
-				compatible = "aspeed,ast2500-lpc-bmc";
+				compatible = "aspeed,ast2500-lpc-bmc", "simple-mfd", "syscon";
 				reg = <0x0 0x80>;
+				reg-io-width = <4>;
+
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0x0 0x0 0x80>;
+
+				kcs1: kcs@1 {
+					compatible = "aspeed,ast2500-kcs-bmc";
+					interrupts = <8>;
+					kcs_chan = <1>;
+					kcs_regs = <0x24 0x30 0x3C>;
+					kcs_addr = <0x0>;
+					status = "disabled";
+				};
+
+				kcs2: kcs@2 {
+					compatible = "aspeed,ast2500-kcs-bmc";
+					interrupts = <8>;
+					kcs_chan = <2>;
+					kcs_regs = <0x28 0x34 0x40>;
+					kcs_addr = <0x0>;
+					status = "disabled";
+				};
+
+				kcs3: kcs@3 {
+					compatible = "aspeed,ast2500-kcs-bmc";
+					interrupts = <8>;
+					kcs_chan = <3>;
+					kcs_regs = <0x2C 0x38 0x44>;
+					kcs_addr = <0x0>;
+					status = "disabled";
+				};
 			};
 
 			lpc_host: lpc-host@80 {
@@ -166,6 +198,16 @@ 
 					reg = <0x20 0x24 0x48 0x8>;
 				};
 
+				kcs4: kcs4@80 {
+					compatible = "aspeed,ast2500-kcs-bmc";
+					reg = <0x80 0x20>;
+					interrupts = <8>;
+					kcs_chan = <4>;
+					kcs_regs = <0x14 0x18 0x1C>;
+					kcs_addr = <0x0>;
+					status = "disabled";
+				};
+
 				mbox: mbox@180 {
 					compatible = "aspeed,ast2500-mbox";
 					reg = <0x180 0x5c>;
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
+	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)
+#define KCS_STR_OBF                 (1)
+
+
+/***************************** LPC Register ****************************/
+#define LPC_HICR0            0x000
+#define     LPC_HICR0_LPC3E          (1 << 7)
+#define     LPC_HICR0_LPC2E          (1 << 6)
+#define     LPC_HICR0_LPC1E          (1 << 5)
+#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
+
+struct kcs_bmc {
+	struct regmap *map;
+	int            offset;
+	int            irq;
+	spinlock_t     lock;
+
+	u32 chan;
+	int running;
+
+	u32  idr; /* Input Data Register */
+	u32  odr; /* Output Data Register */
+	u32  str; /* Status Register */
+
+	int  kcs_phase;
+	u8   abort_phase;
+	u8   kcs_error;
+
+	wait_queue_head_t queue;
+	int  data_in_avail;
+	int  data_in_idx;
+	u8  *data_in;
+
+	int  data_out_idx;
+	int  data_out_len;
+	u8  *data_out;
+
+	struct miscdevice miscdev;
+	char name[16];
+};
+
+static u8 kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
+{
+	u32 val = 0;
+	int rc;
+
+	rc = regmap_read(kcs_bmc->map, kcs_bmc->offset + reg, &val);
+	WARN(rc != 0, "kcs_inb failed: %d\n", rc);
+
+	return rc == 0 ? (u8) val : 0;
+}
+
+static void kcs_outb(struct kcs_bmc *kcs_bmc, u8 data, u32 reg)
+{
+	int rc;
+
+	rc = regmap_write(kcs_bmc->map, kcs_bmc->offset + reg, data);
+	WARN(rc != 0, "kcs_outb failed: %d\n", rc);
+}
+
+static void kcs_set_state(struct kcs_bmc *kcs_bmc, u8 state)
+{
+	int rc;
+
+	rc = regmap_update_bits(kcs_bmc->map, kcs_bmc->offset + kcs_bmc->str,
+				KCS_STR_STATE_MASK,
+				KCS_STR_STATE(state));
+	WARN(rc != 0, "KCS_STR_STATE failed: %d\n", rc);
+}
+
+static void kcs_set_atn(struct kcs_bmc *kcs_bmc, unsigned long set)
+{
+	int rc;
+
+	rc = regmap_update_bits(kcs_bmc->map, kcs_bmc->offset + kcs_bmc->str,
+				KCS_STR_ATN,
+				set != 0 ? KCS_STR_ATN : 0);
+	WARN(rc != 0, "KCS_STR_ATN failed: %d\n", rc);
+}
+
+/*********************************************************************
+ * AST_usrGuide_KCS.pdf
+ * 2. Background:
+ *   we note D for Data, and C for Cmd/Status, default rules are
+ *     A. KCS1 / KCS2 ( D / C:X / X+4 )
+ *        D / C : CA0h / CA4h
+ *        D / C : CA8h / CACh
+ *     B. KCS3 ( D / C:XX2h / XX3h )
+ *        D / C : CA2h / CA3h
+ *        D / C : CB2h / CB3h
+ *     C. KCS4
+ *        D / C : CA4h / CA5h
+ *********************************************************************/
+void kcs_set_addr(struct kcs_bmc *kcs_bmc, u16 addr)
+{
+	switch (kcs_bmc->chan) {
+	case 1:
+		regmap_update_bits(kcs_bmc->map, kcs_bmc->offset + LPC_HICR4,
+				LPC_HICR4_LADR12AS,
+				0);
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR12H,
+				addr >> 8);
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR12L,
+				addr & 0xFF);
+		break;
+
+	case 2:
+		regmap_update_bits(kcs_bmc->map, kcs_bmc->offset + LPC_HICR4,
+				LPC_HICR4_LADR12AS,
+				LPC_HICR4_LADR12AS);
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR12H,
+				addr >> 8);
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR12L,
+				addr & 0xFF);
+		break;
+
+	case 3:
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR3H,
+				addr >> 8);
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR3L,
+				addr & 0xFF);
+		break;
+
+	case 4:
+		regmap_write(kcs_bmc->map, kcs_bmc->offset + LPC_LADR4,
+				((addr + 1) << 16) | (addr));
+		break;
+
+	default:
+		break;
+	}
+}
+
+static void kcs_enable_channel(struct kcs_bmc *kcs_bmc, int enable)
+{
+	switch (kcs_bmc->chan) {
+	case 1:
+		if (enable) {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR2,
+					LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR0,
+					LPC_HICR0_LPC1E, LPC_HICR0_LPC1E);
+		} else {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR0,
+					LPC_HICR0_LPC1E, 0);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR2,
+					LPC_HICR2_IBFIF1, 0);
+		}
+		break;
+
+	case 2:
+		if (enable) {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR2,
+					LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR0,
+					LPC_HICR0_LPC2E, LPC_HICR0_LPC2E);
+		} else {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR0,
+					LPC_HICR0_LPC2E, 0);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR2,
+					LPC_HICR2_IBFIF2, 0);
+		}
+		break;
+
+	case 3:
+		if (enable) {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR2,
+					LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR0,
+					LPC_HICR0_LPC3E, LPC_HICR0_LPC3E);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR4,
+					LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL);
+		} else {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR0,
+					LPC_HICR0_LPC3E, 0);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR4,
+					LPC_HICR4_KCSENBL, 0);
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICR2,
+					LPC_HICR2_IBFIF3, 0);
+		}
+		break;
+
+	case 4:
+		if (enable) {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICRB,
+					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
+					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E);
+		} else {
+			regmap_update_bits(kcs_bmc->map,
+					kcs_bmc->offset + LPC_HICRB,
+					LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E,
+					0);
+		}
+		break;
+
+	default:
+		break;
+	}
+}
+
+static void kcs_rx_data(struct kcs_bmc *kcs_bmc)
+{
+	u8 data;
+
+	switch (kcs_bmc->kcs_phase) {
+	case KCS_PHASE_WRITE:
+		kcs_set_state(kcs_bmc, KCS_WRITE_STATE);
+
+		/* set OBF before reading data */
+		kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+
+		if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ)
+			kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
+					kcs_inb(kcs_bmc, kcs_bmc->idr);
+		break;
+
+	case KCS_PHASE_WRITE_END:
+		kcs_set_state(kcs_bmc, KCS_READ_STATE);
+
+		if (kcs_bmc->data_in_idx < KCS_MSG_BUFSIZ)
+			kcs_bmc->data_in[kcs_bmc->data_in_idx++] =
+					kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+		kcs_bmc->kcs_phase = KCS_PHASE_READ;
+		if (kcs_bmc->running) {
+			kcs_bmc->data_in_avail = 1;
+			wake_up_interruptible(&kcs_bmc->queue);
+		}
+		break;
+
+	case KCS_PHASE_READ:
+		if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len)
+			kcs_set_state(kcs_bmc, KCS_IDLE_STATE);
+
+		data = kcs_inb(kcs_bmc, kcs_bmc->idr);
+		if (data != KCS_READ_BYTE) {
+			kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+			kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+			break;
+		}
+
+		if (kcs_bmc->data_out_idx == kcs_bmc->data_out_len) {
+			kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+			kcs_bmc->kcs_phase = KCS_PHASE_IDLE;
+			break;
+		}
+
+		kcs_outb(kcs_bmc, kcs_bmc->data_out[kcs_bmc->data_out_idx++],
+				 kcs_bmc->odr);
+		break;
+
+	case KCS_PHASE_ABORT:
+		switch (kcs_bmc->abort_phase) {
+		case ABORT_PHASE_ERROR1:
+			kcs_set_state(kcs_bmc, KCS_READ_STATE);
+
+			/* Read the Dummy byte */
+			kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+			kcs_outb(kcs_bmc, kcs_bmc->kcs_error, kcs_bmc->odr);
+			kcs_bmc->abort_phase = ABORT_PHASE_ERROR2;
+			break;
+
+		case ABORT_PHASE_ERROR2:
+			kcs_set_state(kcs_bmc, KCS_IDLE_STATE);
+
+			/* Read the Dummy byte */
+			kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+			kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+			kcs_bmc->kcs_phase = KCS_PHASE_IDLE;
+			kcs_bmc->abort_phase = 0;
+			break;
+
+		default:
+			break;
+		}
+
+		break;
+
+	case KCS_PHASE_ERROR:
+		kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+
+		/* Read the Dummy byte */
+		kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+		kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+		break;
+
+	default:
+		kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+
+		/* Read the Dummy byte */
+		kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+		kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+		break;
+	}
+}
+
+static void kcs_rx_cmd(struct kcs_bmc *kcs_bmc)
+{
+	u8 cmd;
+
+	kcs_set_state(kcs_bmc, KCS_WRITE_STATE);
+
+	/* Dummy data to generate OBF */
+	kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+
+	cmd = kcs_inb(kcs_bmc, kcs_bmc->idr);
+	switch (cmd) {
+	case KCS_WRITE_START:
+		kcs_bmc->data_in_avail = 0;
+		kcs_bmc->data_in_idx   = 0;
+		kcs_bmc->kcs_phase     = KCS_PHASE_WRITE;
+		kcs_bmc->kcs_error     = KCS_NO_ERROR;
+		break;
+
+	case KCS_WRITE_END:
+		kcs_bmc->kcs_phase = KCS_PHASE_WRITE_END;
+		break;
+
+	case KCS_ABORT:
+		if (kcs_bmc->kcs_error == KCS_NO_ERROR)
+			kcs_bmc->kcs_error = KCS_ABORTED_BY_COMMAND;
+
+		kcs_bmc->kcs_phase   = KCS_PHASE_ABORT;
+		kcs_bmc->abort_phase = ABORT_PHASE_ERROR1;
+		break;
+
+	default:
+		kcs_bmc->kcs_error = KCS_ILLEGAL_CONTROL_CODE;
+		kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+		kcs_outb(kcs_bmc, kcs_bmc->kcs_error, kcs_bmc->odr);
+		kcs_bmc->kcs_phase = KCS_PHASE_ERROR;
+		break;
+	}
+}
+
+/*
+ * Whenever the BMC is reset (from power-on or a hard reset), the State Bits
+ * are initialized to "11 - Error State". Doing so allows SMS to detect that
+ * the BMC has been reset and that any message in process has been terminated
+ * by the BMC.
+ */
+static void kcs_force_abort(struct kcs_bmc *kcs_bmc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&kcs_bmc->lock, flags);
+	kcs_set_state(kcs_bmc, KCS_ERROR_STATE);
+
+	/* Read the Dummy byte */
+	kcs_inb(kcs_bmc, kcs_bmc->idr);
+
+	kcs_outb(kcs_bmc, KCS_ZERO_DATA, kcs_bmc->odr);
+	kcs_bmc->kcs_phase = KCS_PHASE_ERROR;
+	spin_unlock_irqrestore(&kcs_bmc->lock, flags);
+}
+
+static irqreturn_t kcs_bmc_irq(int irq, void *arg)
+{
+	int rc;
+	u32 sts;
+	struct kcs_bmc *kcs_bmc = arg;
+
+	rc = regmap_read(kcs_bmc->map, kcs_bmc->offset + kcs_bmc->str, &sts);
+	if (rc)
+		return IRQ_NONE;
+
+	sts &= (KCS_STR_IBF | KCS_STR_CMD_DAT);
+
+	switch (sts) {
+	case KCS_STR_IBF | KCS_STR_CMD_DAT:
+		kcs_rx_cmd(kcs_bmc);
+		break;
+
+	case KCS_STR_IBF:
+		kcs_rx_data(kcs_bmc);
+
+	default:
+		return IRQ_NONE;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int kcs_bmc_config_irq(struct kcs_bmc *kcs_bmc,
+			struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc;
+
+	kcs_bmc->irq = platform_get_irq(pdev, 0);
+	if (!kcs_bmc->irq)
+		return -ENODEV;
+
+	rc = devm_request_irq(dev, kcs_bmc->irq, kcs_bmc_irq, IRQF_SHARED,
+			kcs_bmc->name, kcs_bmc);
+	if (rc < 0) {
+		dev_warn(dev, "Unable to request IRQ %d\n", kcs_bmc->irq);
+		kcs_bmc->irq = 0;
+		return rc;
+	}
+
+	return rc;
+}
+
+
+static inline struct kcs_bmc *file_kcs_bmc(struct file *filp)
+{
+	return container_of(filp->private_data, struct kcs_bmc, miscdev);
+}
+
+static int kcs_bmc_open(struct inode *inode, struct file *filp)
+{
+	unsigned long flags;
+	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+
+	if (kcs_bmc->running)
+		return -EBUSY;
+
+	spin_lock_irqsave(&kcs_bmc->lock, flags);
+	kcs_bmc->kcs_phase     = KCS_PHASE_IDLE;
+	kcs_bmc->running       = 1;
+	kcs_bmc->data_in_avail = 0;
+	spin_unlock_irqrestore(&kcs_bmc->lock, flags);
+
+	return 0;
+}
+
+static unsigned int kcs_bmc_poll(struct file *filp, poll_table *wait)
+{
+	unsigned int mask = 0;
+	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+
+	poll_wait(filp, &kcs_bmc->queue, wait);
+
+	if (kcs_bmc->data_in_avail)
+		mask |= POLLIN;
+
+	if (kcs_bmc->kcs_phase == KCS_PHASE_READ)
+		mask |= POLLOUT;
+
+	return mask;
+}
+
+static ssize_t kcs_bmc_read(struct file *filp, char *buf,
+			    size_t count, loff_t *offset)
+{
+	int rv;
+	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+
+	rv = wait_event_interruptible(kcs_bmc->queue,
+				kcs_bmc->data_in_avail != 0);
+	if (rv < 0)
+		return -ERESTARTSYS;
+
+	kcs_bmc->data_in_avail = 0;
+
+	if (count > kcs_bmc->data_in_idx)
+		count = kcs_bmc->data_in_idx;
+
+	if (copy_to_user(buf, kcs_bmc->data_in, count))
+		return -EFAULT;
+
+	return count;
+}
+
+static ssize_t kcs_bmc_write(struct file *filp, const char *buf,
+			     size_t count, loff_t *offset)
+{
+	unsigned long flags;
+	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+
+	if (count < 1 || count > KCS_MSG_BUFSIZ)
+		return -EINVAL;
+
+	if (copy_from_user(kcs_bmc->data_out, buf, count))
+		return -EFAULT;
+
+	spin_lock_irqsave(&kcs_bmc->lock, flags);
+	if (kcs_bmc->kcs_phase == KCS_PHASE_READ) {
+		kcs_bmc->data_out_idx = 1;
+		kcs_bmc->data_out_len = count;
+		kcs_outb(kcs_bmc, kcs_bmc->data_out[0], kcs_bmc->odr);
+	}
+	spin_unlock_irqrestore(&kcs_bmc->lock, flags);
+
+	return count;
+}
+
+static long kcs_bmc_ioctl(struct file *filp, unsigned int cmd,
+			  unsigned long arg)
+{
+	long ret = 0;
+	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+
+	switch (cmd) {
+	case KCS_BMC_IOCTL_SMS_ATN:
+		kcs_set_atn(kcs_bmc, arg);
+		break;
+
+	case KCS_BMC_IOCTL_FORCE_ABORT:
+		kcs_force_abort(kcs_bmc);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int kcs_bmc_release(struct inode *inode, struct file *filp)
+{
+	unsigned long flags;
+	struct kcs_bmc *kcs_bmc = file_kcs_bmc(filp);
+
+	spin_lock_irqsave(&kcs_bmc->lock, flags);
+	kcs_bmc->running = 0;
+	spin_unlock_irqrestore(&kcs_bmc->lock, flags);
+
+	return 0;
+}
+
+static const struct file_operations kcs_bmc_fops = {
+	.owner          = THIS_MODULE,
+	.open           = kcs_bmc_open,
+	.read           = kcs_bmc_read,
+	.write          = kcs_bmc_write,
+	.release        = kcs_bmc_release,
+	.poll           = kcs_bmc_poll,
+	.unlocked_ioctl = kcs_bmc_ioctl,
+};
+
+static const struct regmap_config kcs_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+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;
+
+	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;
+			}
+		}
+	}
+
+	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);
+	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;
+	}
+
+	snprintf(kcs_bmc->name, sizeof(kcs_bmc->name), "ipmi-kcs%u", chan);
+	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
+	kcs_bmc->miscdev.name = kcs_bmc->name;
+	kcs_bmc->miscdev.fops = &kcs_bmc_fops;
+	rc = misc_register(&kcs_bmc->miscdev);
+	if (rc) {
+		dev_err(dev, "Unable to register misc device\n");
+		goto bail;
+	}
+
+	kcs_set_addr(kcs_bmc, addr);
+	kcs_enable_channel(kcs_bmc, 1);
+
+	kcs_bmc_config_irq(kcs_bmc, pdev);
+	if (kcs_bmc->irq)
+		dev_info(dev, "Using IRQ %d\n", kcs_bmc->irq);
+	else
+		dev_info(dev, "No IRQ\n");
+
+	dev_info(dev, "Found BMC KCS%u@0x%X :\n"
+		      " IDR=0x%X, ODR=0x%X, STR=0x%X Offset=0x%X\n",
+			kcs_bmc->chan, addr,
+			kcs_bmc->idr,
+			kcs_bmc->odr,
+			kcs_bmc->str,
+			kcs_bmc->offset);
+
+	dev_set_drvdata(&pdev->dev, kcs_bmc);
+
+	return 0;
+
+bail:
+	kfree(kcs_bmc->data_in);
+	kfree(kcs_bmc->data_out);
+	kcs_bmc->data_in = NULL;
+	kcs_bmc->data_out = NULL;
+	return rc;
+}
+
+static int kcs_bmc_remove(struct platform_device *pdev)
+{
+	struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
+
+	if (kcs_bmc == NULL)
+		return 0;
+
+	platform_set_drvdata(pdev, NULL);
+
+	kfree(kcs_bmc->data_in);
+	kfree(kcs_bmc->data_out);
+	kcs_bmc->data_in = NULL;
+	kcs_bmc->data_out = NULL;
+
+	misc_deregister(&kcs_bmc->miscdev);
+
+	return 0;
+}
+
+static const struct of_device_id kcs_bmc_match[] = {
+	{ .compatible = "aspeed,ast2500-kcs-bmc" },
+	{ },
+};
+
+static struct platform_driver kcs_bmc_driver = {
+	.driver = {
+		.name           = DEVICE_NAME,
+		.of_match_table = kcs_bmc_match,
+	},
+	.probe = kcs_bmc_probe,
+	.remove = kcs_bmc_remove,
+};
+
+module_platform_driver(kcs_bmc_driver);
+
+MODULE_DEVICE_TABLE(of, kcs_bmc_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>");
+MODULE_DESCRIPTION("Linux device interface to the IPMI KCS interface");
+
diff --git a/include/uapi/linux/kcs-bmc.h b/include/uapi/linux/kcs-bmc.h
new file mode 100644
index 0000000..4bbf669
--- /dev/null
+++ b/include/uapi/linux/kcs-bmc.h
@@ -0,0 +1,14 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2015-2017, Intel Corporation.
+
+#ifndef _UAPI_LINUX_KCS_BMC_H
+#define _UAPI_LINUX_KCS_BMC_H
+
+#include <linux/ioctl.h>
+
+#define __KCS_BMC_IOCTL_MAGIC      'K'
+#define KCS_BMC_IOCTL_SMS_ATN      _IOW(__KCS_BMC_IOCTL_MAGIC, 1, unsigned long)
+#define KCS_BMC_IOCTL_FORCE_ABORT  _IO(__KCS_BMC_IOCTL_MAGIC, 2)
+
+#endif /* _UAPI_LINUX_KCS_BMC_H */
+